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

Add extended range #4500

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ccameron-chromium
Copy link
Contributor

@ccameron-chromium ccameron-chromium commented Feb 27, 2024

Add color metadata to canvas configuration, with the ability to specify an extended range.

Move the clarification of the handling out out-of-range values to the subsection on specific values, and add an example of extended range values.

Fixes #4239

Copy link
Contributor

github-actions bot commented Feb 27, 2024

Previews, as seen when this build job started (7a3b59a):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

The worked examples are great. A few small comments (and holding "editor" approval until others have reviewed)

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@kainino0x
Copy link
Contributor

Fixes #4239

FYI I added this to the PR summary to link the issue. Is it correct that landing this would fix/close that issue?

kainino0x and others added 2 commits February 27, 2024 11:45
From kainino0x review

Co-authored-by: Kai Ninomiya <kainino1@gmail.com>
@ccameron-chromium
Copy link
Contributor Author

Fixes #4239

FYI I added this to the PR summary to link the issue. Is it correct that landing this would fix/close that issue?

Yes, thanks!

@ccameron-chromium
Copy link
Contributor Author

Fixes #4239

FYI I added this to the PR summary to link the issue. Is it correct that landing this would fix/close that issue?

Yes!

@kainino0x
Copy link
Contributor

Friendly review bump @kdashg @mwyrzykowski

mwyrzykowski
mwyrzykowski previously approved these changes Mar 18, 2024
@mwyrzykowski
Copy link

Friendly review bump @kdashg @mwyrzykowski

Sorry! It looks good, I forgot to click approve 👍

kdashg
kdashg previously approved these changes Mar 27, 2024
@kainino0x kainino0x marked this pull request as draft April 1, 2024 22:23
@kainino0x
Copy link
Contributor

kainino0x commented Apr 1, 2024

@ccameron-chromium let me know that CSS is hotly debating gamut mapping right now, and we may need to be less prescriptive in this spec lest we contradict what CSS eventually does. I've converted this to draft so it can't be landed accidentally.

@ccameron-chromium
Copy link
Contributor Author

ccameron-chromium commented Apr 1, 2024

@ccameron-chromium let me know that CSS is hotly debating gamut mapping right now, and we may need to be less prescriptive in this spec lest we contradict what CSS eventually does. I've converted this to draft so it can't be landed accidentally.

We had originally called the modes "default" and "extended", and then made them more descriptive by calling them "clamp-to-standard" and "clamp-to-extended". While this accurately describes the current behavior on all browsers (as did PR 4400), I think we shouldn't be so explicit in the names of the enums.

The core behavior that we want to capture is that

  • in the "standard" behavior, no colors in the SDR gamut of the display will be altered, and all colors outside of the SDR gamut will be projected onto the gamut broder
  • in the "extended" behavior, the same thing, except for the HDR gamut

I could imagine changing the names to "project-to-standard" and "project-to-extended", or just sticking with "standard" and "extended". The behavior of clamping is a projection (the L-infinity error minimizing one, for what that's worth). If there a preference between these two name schemes?

@kainino0x
Copy link
Contributor

I probably should have put this on the agenda for today's meeting, but didn't think about it. We're probably not having another WG meeting for 3 weeks but that should be OK.

I've put this on the editors' agenda for this coming Monday so we can talk about naming and then try to get reapproval.

spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@kainino0x kainino0x marked this pull request as ready for review April 25, 2024 00:46
@kainino0x kainino0x dismissed stale reviews from kdashg and mwyrzykowski April 25, 2024 00:46

proposal became less strict

Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

The names and language LGTM, just small comments to label non-normative language as non-normative.

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@kainino0x
Copy link
Contributor

@kdashg @mwyrzykowski could you re-review with the loosened wording and new naming?

@kainino0x kainino0x added the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Apr 27, 2024
@kainino0x kainino0x added the api WebGPU API label May 21, 2024
@Kangz
Copy link
Contributor

Kangz commented May 21, 2024

GPU Web WG 2024-05-15
  • KN: The CSS WG is still discussing whether clamping is required, so this PR loosens the verbiage to be more lenient so it can match what CSS does eventually.
  • KN: The two changes are the enums names that say "project" and the text saying it is projected in the color space, but not necessarily by clamping.
  • KG: Will review offline.

@kainino0x
Copy link
Contributor

@kdashg review bump!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api WebGPU API needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extended brightness range rendering
5 participants