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 drawingBufferToneMapping #3668

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ccameron-chromium
Copy link
Contributor

@ccameron-chromium ccameron-chromium commented Jul 24, 2024

<dd>
<p>
The value of <code>toneMapping.mode</code> indicates how the drawing buffer shall be
tone mapped by the HTML page compositor when presented.
Copy link
Contributor

@kkinnunen-apple kkinnunen-apple Jul 29, 2024

Choose a reason for hiding this comment

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

For MSAA contexts, does the projection apply before or after the multisample resolve?

If I recall correctly, current implementations have the problem that already the resolve is done in incorrect colorspace (was it that it's done on srgb values?). Not sure if that factors into the exact pixel equation tone mapping is about to modify...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Short answer: Tone mapping is done after multisample resolve.

Long answer: The "bad behavior" sometimes seen in this issue is related to color space conversion (namely, color conversion happening after the resolve), not tone mapping (which is after-resolve). In the spec we indicate that tone mapping is done after conversion to the color space of the screen, which is always post-resolve.

Really long answer (about color space conversion, not tone mapping): The resolve is most correctly done in a linear space (a space that has a linear relationship to physical light quantities).

The resolve is done on raw pixel values (with one exception). This means:

  • By default, if you're using RGBA8 as the pixel format, and "srgb" or "display-p3" as your color space, then you will get "the wrong values", because the pixel values are not linearly related to physical light.
  • If the framebuffer is SRGB8_ALPHA8, then (IIUC), the resolve will happen after a conversion to linear space, so, you get "the right values".
  • If the framebuffer is RGBA16F and your color space is "srgb" or "display-p3", then you will get "the wrong values".
  • If you specify a linear color space (like the below-mentioned "rec2100-linear"), then you will get "the right values", because the pixels themselves correspond to linear light values. This will probably be the most common case for WebGL and WebGPU HDR applications.

I'd like for us to add "rec2100-linear" as a PredefinedColorSpace very soon, but when we do it, I'll want to make sure we get some extra details right (namely, when converted to an image, which will have to be fixed-point, it should be PQ encoded). I think there is now strong consensus on what's "right" here, but I want those issues to be very-well-ventilated in their own feature proposals. See some more comments below.

<p>
When presenting the drawing buffer, the pixel values in the drawing buffer shall be
converted from the color space of the drawing buffer to the color space of the screen, and
then shall be tone mapped as specified by <code>drawingBufferToneMapping</code>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Question 1: Why not use an attribute, like drawingBufferColorSpace?
The IDL definition does not allow attributes to be dictionaries, so that is not an option.

Question 1b: Why WebGLToneMapping has to be a dictionary instead of the single mode entry it contains? Are there common tone mapping parameters that could be passed here in the future that could not be passed as a separate function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are tone mapping modes that we plan to add which would use more parameters.

Example: HDR10. We would like to add a mode that matches HDR10 rendering (The reason it's not in this specification is because it's usually framed in relation to what would be the rec2100-pq color space, and we want to make sure all of the details of that are sorted out before introducing it. We also want to do it separately to make sure that all of those details are clearly exposed for scrutiny, rather than being put in a giant bundle including other features.)

In that situation, we would want to include content light level info (CLLI) and mastering display color volume (MDCV) metadata, which are standard for use with HDR10. Over in the explainer for the WebGPU version (which has now merged and shipped), we have a worked example as follows (translated to this API):

  gl.drawingBufferStorage(gl.RGBA16F,
                          gl.drawingBufferWidth,
                          gl.drawingBufferHeight);
  gl.drawingBufferColorSpace = "rec2100-linear";
  gl.drawingBufferToneMapping({mode:"hdr10",
                               clli:{maxFALL:200, maxCLL:1200}})

On macOS, this would correspond to using HDR10 CAEDRMetadata, and on Windows it would correspond to using DXGI_HDR_METADATA_HDR10.

If such a canvas were to be then encoded to an HDR image or video stream, then the resulting image or video would be required to be include that metadata.

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.

Extended brightness range rendering
2 participants