Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding of video-* properties and dynamic-range to handle bi-plane scenarios #4678

Merged
merged 10 commits into from
Mar 16, 2020

Conversation

gregwhitworth
Copy link
Contributor

As resolved in issue #4471 I have added in:

video-color-gamut
video-dynamic-range
video-width
video-height
video-resolution
dynamic-range

And a brief description of the issue along with normative text of when to answer differently between a single or bi-plane scenario.

Specific to dynamic-range we placed a general definition there and are interested in thoughts since standard & high may fluctuate over time but I'd expect the spec text too as well.

@gregwhitworth
Copy link
Contributor Author

@Crissov
Copy link
Contributor

Crissov commented Jan 18, 2020

comment moved to issue

mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
@mwatson2
Copy link

It would of course be great to reference some widely agreed definition of High Dynamic Range or Wide Color Gamut as they pertain to display capabilities. However, I'm not aware that any such thing exists. Given any candidate for such a thing, someone will make a display that is 1 nit less bright or has 0.1% P3 smaller gamut and claim (accurately) that the experience on that display is just as deserving of the moniker.

In any case, the question that the site wishes to answer with these properties is whether the user experience will be better if the HDR / WCG version of the content is provided (and mapped down to the display capability) than if the SDR / NCG version is provided (and mapped up to the display capability). This of course is a matter of opinion. The opinion that matters in this case is the users (or, some may argue, the filmmakers).

So, I think the best we can do is ask the user's agent to answer this question on behalf of the user.

@gregwhitworth
Copy link
Contributor Author

@mwatson2 yeah that was my worry and given the purpose of these properties and the expectation that the output is taken into account your points are valid. @svgeesus are you worried about leaving this up to the UA due to this?

@svgeesus
Copy link
Contributor

@gregwhitworth I'm nt worried enough to want to hold up this PR

@svgeesus
Copy link
Contributor

So I think we can merge this?

…t contrast of the display should be defined by the UA
@gregwhitworth
Copy link
Contributor Author

@svgeesus I made your modifications you asked for. I also added some non-normative text regarding the current state of handling contrast of the display that I'd like you to review quickly. If you're fine with all of that please merge it in.

@svgeesus
Copy link
Contributor

@gregwhitworth thanks for that, all looks good to me. I pinged @frivoal just as the CSSWG f2f was ending, he wants to review more closely and perhaps tighten some wording before merging.

@chcunningham
Copy link

Sorry folks, I'm late to the party. Let me make a pass on the language real quick.

mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Show resolved Hide resolved
Copy link

@chcunningham chcunningham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for all the comments. I took a well caffeinated look at this and I had lots to say. Maybe too much.

mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
@vi-dot-cpp
Copy link
Collaborator

@chcunningham Thanks for your comments. They should all be addressed -- let me know if issues remain. cc: @gregwhitworth @gregwfreedman

Copy link

@chcunningham chcunningham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Vi. Some small follow ups, but I think this is nearly there.

mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved

High dynamic range (HDR) represents the combination of brightness, color depth, transfer function, and color gamut.

<dl dfn-type=value dfn-for="video-dynamic-range">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(repasting so this doesn't git hidden in the resolved thread)

Now that you're referencing the values for non-prefixed dynamic-range, you can remove this whole dfn block of value definitions and just rely on the reference. This is consistent with video-color-gamut.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git hidden

:)

@vi-dot-cpp
Copy link
Collaborator

Ah, thanks -- I had missed some of the more obvious edits. The latest commit should address your CR.

Copy link

@chcunningham chcunningham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Vi. Approved % two final comments.

mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
mediaqueries-4/Overview.bs Outdated Show resolved Hide resolved
@vi-dot-cpp
Copy link
Collaborator

Thanks Chris!

@gregwhitworth
Copy link
Contributor Author

Ok, since we've gotten @chcunningham sign off I'd like to get a review from @frivoal since @svgeesus mentioned he'd like to provide some feedback.

@svgeesus
Copy link
Contributor

That would be nice, but the recent changes have improved this nicely so I would also be fine with merging as-is.

@gregwhitworth
Copy link
Contributor Author

Ok, based on @svgeesus feedback I'm going to merge it in - @frivoal I'm more than happy to address any feedback you may have if you find any issues. If editorial in nature; as always feel free to do them.

@gregwhitworth gregwhitworth merged commit ae672c6 into master Mar 16, 2020
@frivoal
Copy link
Collaborator

frivoal commented Apr 8, 2020

Hi @gregwhitworth, I'm sorry for being so far behind. I do have some feedback, which I'll put it here first, but depending on responses it'll make its way into separate issues / PRs.

procedural / editorial

  • This PR was made against mq4, but the original issue was tagged against mq5, and I do think it would be a better fit there. Also, MQ4 is in CR already, so effectively under a (soft) feature freeze.
  • I'd make all the video-* media features be subsections of 6.7. Video Prefixed Features (turn them from <h3>s into <h4>s): they're not full fledged sections, but just saying "video-foo is the same as foo, in except in this different context" with some clarifications, so I don't think they should have the same importance in the table of contents.

Substantive

  • For section 6.6 Determining contrast and brightness of display, I'd do it a little differently. I think non-normative isn't exactly what we're shooting for, and "UA defined" would be a better phrasing. Also, while I agree it should be up to the UA rather than tightly defined by the spec, it still needs some degree of guidance about what the intent is. We are talking about the screens that have the ability to do "brighter than white" highlights and the like, as typically found in video, rather than screens which have a "normal" top brightness and particularly deep blacks, which could also reasonably be described as being high contrast, but aren't what we're interested in here. While leaving the specifics UA defined, there should be some text to express the intent of that definition.

  • The definition of video-width (same thing for video-height) includes this:

    For continuous media, this is the width of the viewport (as described by CSS2, section 9.1.1 [[!CSS21]]) including the size of a rendered scroll bar (if any). For paged media, this is the width of the page box (as described by CSS2, section 13.2 [[!CSS21]]).

    That text comes from the definition of width, but I don't think it is relevant here: I don't think there currently exists such a thing as a paginated media with a dedicated video plane (or a paginated video plane being itself paginated).
    On the other hand, we should add a mention that this is about the size of the entire video plane, not the layout-dependent portion of it that is being clipped and composited with the rest of the page.

  • The definition of video-width (same thing for video-height) include this:

    lengths are interpreted according to [[#units]].

    I suspect that means we're doing things like measuring in CSS px, not in device pixels. If so, we should be very explicit about this, as this may be counter-intuitive for video. Also, maybe we should consider actually reporting physical pixels here when there is that separate video plane (and if so, also returning 1 as the video-resolution).


Depending on the responses to the points above, I might file issues / PRs differently, but for now, I expect I'll probably directly commit on the master branch for the first 2 points, propose PRs for the next 2, and to open a new issue for the last one.

@vi-dot-cpp
Copy link
Collaborator

Hi @frivoal. Thank you for your review -- your feedback loos good to me and I trust your purview here. Am I understanding correctly that you intend to make Pull Requests for these proposals?

P.S. I hope you are well.

@gregwfreedman
Copy link

hi @frivoal. thanks for reviewing. i agree with your first four points.

with respect to

lengths are interpreted according to [[#units]].

i think that was just to be consistent with other CSS features. the only intent of video-height and video-width is to determine physical pixels of the video plane. given that, what would be your recommendation? if we change the units to physical pixels, then we probably don't need video-resolution.

@syncbot syncbot restored the gwhit-issue-4471 branch July 1, 2020 16:44
@plinss plinss deleted the gwhit-issue-4471 branch July 13, 2020 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants