Separate IDL sections for Canvas/WebGL/ImageData #32

ccameron-chromium · 2020-08-31T18:30:35Z

Add separate subsections for the IDL changes for CanvasColorSpace
and CanvasColorEncoding, CanvasRenderingContext2D, WebGL 2, and
ImageData.

Add a pixelFormat context creation attribute for WebGL, in which pixel
formats such as GL_RGBA8 and GL_SRGB8_ALPHA8 may be specified.
This complements the CanvasColorEncoding attribute by allowing
WebGL to indicate the particular GL interpretation of the bits in the
backbuffer.

Add changes to getImageData and createImageData CanvasRenderingContext2D
APIs.

Add subsections for behavior changes to HTMLCanvasElement.

Add TODOs regarding extended color spaces and HDR support.

Add TODOs regarding UNPACK_COLORSPACE_CONVERSION_WEBGL.
Add subsections for behavior changes to HTMLCanvasElement.

Add TODOs regarding extended color spaces and HDR support.

Add TODOs regarding UNPACK_COLORSPACE_CONVERSION_WEBGL.

Add separate subsections for the IDL changes for CanvasColorSpace and CanvasColorEncoding, CanvasRenderingContext2D, WebGL 2, and ImageData. Add a pixelFormat context creation attribute for WebGL, in which pixel formats such as GL_RGBA8 and GL_SRGB8_ALPHA8 may be specified. This complements the CanvasColorEncoding attribute by allowing WebGL to indicate the particular GL interpretation of the bits in the backbuffer. Add changes to getImageData and createImageData CanvasRenderingContext2D APIs. Add subsections for behavior changes to HTMLCanvasElement. Add TODOs regarding extended color spaces and HDR support. Add TODOs regarding UNPACK_COLORSPACE_CONVERSION_WEBGL. Add subsections for behavior changes to HTMLCanvasElement. Add TODOs regarding extended color spaces and HDR support. Add TODOs regarding UNPACK_COLORSPACE_CONVERSION_WEBGL.

ccameron-chromium · 2020-08-31T18:40:11Z

In this patch I've added an explicit WebGL pixel format type (e.g, gl.RGB8, gl.SRGB8_ALPHA8) to the WebGL context creation parameters. I've kept the existing CanvasColorEncoding (renamed to CanvasColorType) along with the WebGL context creation parameter, but added a TODO suggesting that we remove one of them

I think that ultimately WebGL's pixel format specification will need to be distinct from 2D canvas. 2D canvas can't meaningfully support all formats (e.g, gl.SRGB8_ALPHA) in the same way that WebGL can. Also, 2D canvas has a lot of flexibility about what it does behind the scenes (e.g, one could request 10 bits per channel, and 2D canvas could lie and just use 16 bits per channel without much consequence, while for WebGL those details can leak through).

I renamed CanvasColorEncoding to CanvasColorType, but I'm still not happy with the name -- suggestions very welcome. It may be that we should make it so that they attribute keys for it and ImageDataStorageFormat be the same (e.g, CanvasStorageFormat).

It also may be that the WebGL changes should be moved into a two separate WebGL extensions. One to allow specifying the pixel format of the backbuffer, and another to add a pile of UNPACK_COLORSPACE_CONVERSION_WEBGL enums. In that case, the only WebGL change here would be the color space in which the compositor interprets WebGL pixel values (sRGB v P3, etc).

kenrussell · 2020-08-31T23:12:48Z

Thanks @ccameron-chromium for re-uploading this PR!

Repo owners (@jdashg , @fserb, others), let's discuss these changes before merging them.

ccameron-chromium · 2020-09-10T06:28:02Z

Was there any further feedback on this PR?

fserb · 2020-09-10T14:54:50Z

LGTM

RafaelCintron

Would be good to separate this PR into a cleanup one and a different one which adds functionality. The title is misleading as it's doing more than just making separate IDL sections.

RafaelCintron · 2020-09-10T16:10:24Z

CanvasColorSpaceProposal.md

 };

 partial interface CanvasRenderingContext2D {
  CanvasRenderingContext2DSettings getContextAttributes();
+  ImageData getImageData(long sx, long sy, long sw, long sh, optional ImageDataSettings imageDataSettings);


I do not see ImageDataSettings mentioned anywhere in this document besides this IDL definition.

Please add a definition in this document along with a section on why it is needed and the behavior of getImageData/createImageData when passed the settings object.

I will create a separate patch on ImageDataSettings (it was there in the past, maybe it got lost in a shuffle).

RafaelCintron · 2020-09-10T16:12:40Z

CanvasColorSpaceProposal.md


-#### Feature detection
-2D rendering contexts are to expose a new getContextAttributes() method, that works much like the method of the same name on WebGLRenderingContext. The method returns the "actual context attributes" which represents the settings that were successfully applied at context creation time. The settings attribute reflects the result of running the algorithm for coercing the settings argument for 2D contexts, as well as the result of any fallbacks that may have happened as a result of options not being supported by the UA.
+Values stored in the WebGL backbuffers  are in the canvas's color space, as specified by the colorSpace context attribute.


Nit: Extra space between the words "backbuffers" and "are".

RafaelCintron · 2020-09-10T16:22:51Z

CanvasColorSpaceProposal.md


-* Should float16 allow alpha values outside [0,1] range at any stage of the pipeline? What would they mean?
+* How should feature detection work? The combinations of CanvasColorSpace and CanvasColorType supported by 2D Canvas and WebGL are, for now, obviously "all of them", but this is not likely to remain the case.


The canvas context attributes can already be queried by the user after the canvas context has been created. If the query returns a dictionary with no colorSpace or storageFormat members, the developer can conclude they were ignored.

If a browser were to add, say, "adobe-rgb" as a color space, how do does one detect ahead of time that that is supported? It would be nice to be able to check before creating the context.

(But I'm new in this area, so maybe that's the convention)

@ccameron-chromium , you're right that there's no way to tell ahead of time whether there is support.
You have to try things and see whether the request was honored.

ImageBitmap has a similar problem with ImageBitmapOptions which hasn't been resolved.

Thanks for the context!

RafaelCintron · 2020-09-10T16:31:28Z

CanvasColorSpaceProposal.md


-* Should we support custom color spaces based on ICC profiles? Would offer ultimate flexibility. Would be hard to make implementations as efficient as built-in color spaces, in particular for implement linearPixelMath for profiles that have arbitrary transfer curves.
+Issues:


How will a web developer determine whether advanced color management is necessary?

On an SDR monitor, using the proposed options incurs unnecessary texture space (for float16 textures) and processing time to tonemap from the source to destination color gamuts/spaces.

The color gamut media queries can be used to determine if there is a benefit to using p3 or rec2020.
https://developer.mozilla.org/en-US/docs/Web/CSS/@media/color-gamut

(A similar media query should be added for HDR, but hasn't, to my knowledge).

RafaelCintron · 2020-09-10T16:36:15Z

CanvasColorSpaceProposal.md

+| - | - | - |
+| "uint8" | false | RGB8 (default) |
+| "uint8" | false | SRGB8 |
+| "uint8" | true | RGBA8 (default) |
+| "uint8" | true | SRGB8_ALPHA8 |
+| "float16" | true | RGBA16F |


If the web developer creates a canvas with pixelFormat of GL_NONE (or doesn't include it in the first place) and subsequently queries the context attributes after creation, will the pixelFormat member be GL_NONE or the value chosen from the table?

Might be best to keep things simple and force developers to specify a pixelFormat from the table.

I'll remove the GL_NONE part. The reason I added GL_NONE as a default value is because I thought a default value was required, and there is no default value that works universally.

RafaelCintron · 2020-09-10T16:38:43Z

CanvasColorSpaceProposal.md

-
-#### Selecting the best encoding for the user agent's display device
-Selection should be based on the best color space match (see above). For srgb, at least 8 bits per component is recommended; for p3, 10 bits; and for rec2020, 12 bits.  The float16 format is suitable for any colorspace.  There may soon be a proposal to add a way of detecting HDR displays, possibly something like "window.screen.isHDR()" (TBD), which would be a good hint to use the float16 format.
+Note that because there exists no direct access to the canvas backbuffer, it is not required that the colorType be truly respected. From the user's point of view, there will be no way to know if the true backing has higher precision than what was requested (e.g, the canvas is actually float32, when float16 was specified).


When would the canvas be float32?

If the implementation, behind the scenes, prefers float32 (for any implementor reason). That's a bit more of a theoretical example, but as we add formats, more realistic examples may come up. For instance, a user could request 10-bit color, and some implementations (due to driver bugs, etc), may prefer to just use 16-bit color.

kdashg

The title really needs to be fixed.

kdashg · 2020-09-10T17:33:21Z

CanvasColorSpaceProposal.md

+<pre>
+partial dictionary WebGLContextAttributes {
+  CanvasColorSpace colorSpace = "srgb";
+  CanvasColorType colorType = "uint8";


unorm8 is more precise, and I would prefer it. Propose this separately.

I'd prefer that the unorm8 or uint8 type be removed entirely, in favor of GLenum (e.g, SRGB8_ALPHA8).

kdashg · 2020-09-10T17:34:25Z

CanvasColorSpaceProposal.md

+partial dictionary WebGLContextAttributes {
+  CanvasColorSpace colorSpace = "srgb";
+  CanvasColorType colorType = "uint8";
+  GLenum pixelFormat = GL_NONE;


Propose this separately. What's the usecase for decoupling this from colorSpace/Type?

2D canvas can't meaningfully support all formats (e.g, gl.SRGB8_ALPHA) in the same way that WebGL can. Also, 2D canvas has a lot of flexibility about what it does behind the scenes, because 2D canvas doesn't expose direct access to the framebuffer (e.g, one could request 10 bits per channel, and 2D canvas could lie and just use 16 bits per channel without much consequence, while for WebGL those details can leak through).

kdashg · 2020-09-10T17:36:49Z

CanvasColorSpaceProposal.md


-enum ImageDataStorageType {
+enum ImageDataStorageFormat {


Type is more consistent than Format, given the overloads of "format" in the context of WebGL.

Sounds good, I'll use the Type name in a separate patch that just addresses ImageData issues.

kdashg · 2020-09-10T17:38:32Z

CanvasColorSpaceProposal.md


-  ImageDataColorSettings getColorSettings();
+partial interface ImageData {
+  ImageDataAttributes getAttributes();


Why not use a readonly attribute?

(because I didn't know it was an option :)

kdashg · 2020-09-10T17:39:57Z

CanvasColorSpaceProposal.md

-* Should there be API-level support for mixing chromaticities and transfer functions, including use of no-op transfer functions?
+* Should we unify ImageDataStorageFormat with CanvasColorType? This might create more hassle than it's worth, because there are types for CanvasColorType (e.g, float16) that cannot exist for ImageDataStorageFormat at this time.
+
+* For ImageData, is it appropriate to be able to query the ImageDataStorageFormat, when that can be inferred from the type of the data member? This would make sense if we supported both uint8 and unorm8.


What is uint8 that isn't actually unorm8?

Integer-based texture formats (GL_RGBA8I, MTLPixelFormatRGBA8Uint, etc).

From the perspective of someone using 2D canvas, "unorm" is a foreign concept. In WebGL, it makes sense.

But I'll do this separately.

kdashg · 2020-09-10T17:41:10Z

CanvasColorSpaceProposal.md

 enum CanvasColorSpaceEnum {
  "srgb", // default
+  "display-p3",


Add this separately

kdashg · 2020-09-10T17:43:59Z

CanvasColorSpaceProposal.md

-  const CanvasColorEncodingEnum unorm8 = "unorm8";
-  const CanvasColorEncodingEnum unorm8Srgb = "unorm8-srgb";
-  const CanvasColorEncodingEnum float16 = "float16";
+interface CanvasColorType {


Encoding is more specific and accurate than Type.

I'm not particularly wedded to any name, but I feel that "encoding" is bringing along the baggage of sRGB framebuffers, which is unique to WebGL. We should circle back once we decide how to handle sRGB (in that area I do feel strongly that it should be moved to a WebGL-only area).

ccameron-chromium · 2020-09-10T18:38:57Z

I'm going to turn this into a bunch of separate patches. This proposal needs a serious amount of revision. I had hoped that we could make sweeping changes like this instead of slicing them into smaller proposals, but I can go with the smaller proposals.

ccameron-chromium · 2020-09-10T20:48:54Z

I've split out two of the smaller features into new pulls at
#33 (very small, just p3)
#34 (slightly larger, ImageData and a bit of ImageBitmap)

ccameron-chromium requested a review from kdashg August 31, 2020 18:30

kenrussell requested review from kenrussell and fserb August 31, 2020 23:11

RafaelCintron suggested changes Sep 10, 2020

View reviewed changes

kdashg requested changes Sep 10, 2020

View reviewed changes

kdashg reviewed Sep 10, 2020

View reviewed changes

ccameron-chromium closed this Sep 10, 2020


		* Should float16 allow alpha values outside [0,1] range at any stage of the pipeline? What would they mean?
		* How should feature detection work? The combinations of CanvasColorSpace and CanvasColorType supported by 2D Canvas and WebGL are, for now, obviously "all of them", but this is not likely to remain the case.


		* Should we support custom color spaces based on ICC profiles? Would offer ultimate flexibility. Would be hard to make implementations as efficient as built-in color spaces, in particular for implement linearPixelMath for profiles that have arbitrary transfer curves.
		Issues:

Separate IDL sections for Canvas/WebGL/ImageData #32

Separate IDL sections for Canvas/WebGL/ImageData #32

Conversation

ccameron-chromium commented Aug 31, 2020

ccameron-chromium commented Aug 31, 2020

kenrussell commented Aug 31, 2020

ccameron-chromium commented Sep 10, 2020

fserb commented Sep 10, 2020

RafaelCintron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdashg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccameron-chromium Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccameron-chromium commented Sep 10, 2020

ccameron-chromium commented Sep 10, 2020

ccameron-chromium Sep 10, 2020 •

edited

Loading