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.

Sign up for GitHub

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

Jump to bottom

Add drawingBufferToneMapping #3668

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

Add drawingBufferToneMapping #3668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions specs/latest/1.0/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,11 @@ <h3><a name="THE_DRAWING_BUFFER">The Drawing Buffer</a></h3>
an <a href="https://html.spec.whatwg.org/multipage/webappapis.html#imagebitmap">ImageBitmap</a> <a href="#refsHTML">[HTML]</a>
from this context's canvas.
</p>
<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.

</p>
<div class="note">
<p>
While it is sometimes desirable to preserve the drawing buffer, it can cause significant
Expand Down Expand Up @@ -443,6 +448,46 @@ <h3><a name="THE_DRAWING_BUFFER">The Drawing Buffer</a></h3>
generate <code>INVALID_VALUE</code>.

If allocation fails, generate <code>OUT_OF_MEMORY</code>.
</dd>

<dt class="idl-code">undefined drawingBufferToneMapping(WebGLToneMapping toneMapping);
<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 •
edited
Loading

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>

<p>
The allowed values are:
</p>
<dl><dd>
<p>
<em>standard</em>
</p>
<p>
All colors that are inside of the standard dynamic range of the screen shall not
be changed, and all colors that outside of the standard dynamic range of the
screen shall be projected to the standard dynamic range of the screen.
</p>
<div class="note">
This projection is often accomplished by clamping color values in the color
space of the screen to the [0, 1] interval.
</div>
<p>
<em>extended</em>
</p>
<p>
All colors that are inside of the extended dynamic range of the screen shall not
be changed, and all colors that outside of the extended dynamic range of the
screen shall be projected to the extended dynamic range of the screen.
</p>
<div class="note">
This projection is often accomplished by clamping color values in the color
space of the screen to the interval of values that the screen is capable of
displaying, which may include values greater than 1.
</div>
</dd></dl>
</dd>
</dl>

<!-- ======================================================================================================= -->
Expand Down Expand Up @@ -780,6 +825,15 @@ <h3>Types</h3>
// The power preference settings are documented in the WebGLContextAttributes
// section of the specification.
enum WebGLPowerPreference { "default", "low-power", "high-performance" };

enum WebGLToneMappingMode {
"standard",
"extended",
};

dictionary WebGLToneMapping {
WebGLToneMappingMode mode = "standard";
};
</pre>

<!-- ======================================================================================================= -->
Expand Down Expand Up @@ -1794,6 +1848,7 @@ <h3><a name="WEBGLRENDERINGCONTEXT">The WebGL context</a></h3>
object? getExtension(DOMString name);

undefined drawingBufferStorage(GLenum sizedFormat, unsigned long width, unsigned long height);
undefined drawingBufferToneMapping(WebGLToneMapping toneMapping);

undefined activeTexture(GLenum texture);
undefined attachShader(WebGLProgram program, WebGLShader shader);
Expand Down
9 changes: 9 additions & 0 deletions specs/latest/1.0/webgl.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ typedef unrestricted float GLclampf;
// section of the specification.
enum WebGLPowerPreference { "default", "low-power", "high-performance" };

enum WebGLToneMappingMode {
"standard",
"extended",
};

dictionary WebGLToneMapping {
WebGLToneMappingMode mode = "standard";
};

dictionary WebGLContextAttributes {
boolean alpha = true;
Expand Down Expand Up @@ -553,6 +561,7 @@ interface mixin WebGLRenderingContextBase
object? getExtension(DOMString name);

undefined drawingBufferStorage(GLenum sizedFormat, unsigned long width, unsigned long height);
undefined drawingBufferToneMapping(WebGLToneMapping toneMapping);

undefined activeTexture(GLenum texture);
undefined attachShader(WebGLProgram program, WebGLShader shader);
Expand Down
Loading