-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 CanvasColorType, with unorm8 (the default) and float16 support #10951
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this can in theory be isolated from the ImageData
changes, but is that useful or even observable (apart from this API itself)?
29f4394
to
4b58b76
Compare
This actually hits lots of testable paths before adding This patch is the proto-WPT test that I'm working against. E.g, if you write an sRGB image to a float16-P3 canvas, then read it back as sRGB, you shouldn't lose any code points. But in our implementation you do... BUT ONLY if you read back as 8-bit (if you read back as float16, then the bug is side-stepped). The fact that there's so much to be gotten right in this area alone is part of why I chopped it into a separate commit. |
cc @whatwg/canvas |
Are there any further issues with the the PR itself? If this is all in line, then I'll file implementation and MDN bugs. |
Nit: The end section about alpha-premult might need an update. I suppose the following sentence doesn't entirely hold anymore:
|
4b58b76
to
8217226
Compare
Thanks for finding that! I've change it to read "division and multiplication using finite precision entails a loss of accuracy". This also fixes a nit where what is lost is accuracy (not precision). |
The default behavior is unorm8, which matches the existing behavior of all user agents. The name unorm8 is chosen to match modern APIs which distinguish between 8-bit unsigned normalized and 8-bit unsigned integer (e.g, VK_FORMAT_R8G8B8A8_UNORM vs VK_FORMAT_R8G8B8A8_UINT in Vulkan, MTLPixelFormatRGBA8Unorm vs MTLPixelFormatRGBA8Uint in Metal, and "rgba8unorm" vs "rgba8uint" in WebGPU). The use of colorType (instead of pixelFormat) is used to avoid the complexity of selecting channel ordering (BGRA vs RGBA) and interactions with the alpha parameter (RGB, RGBX, or BGRX potentially being required if alpha is false).
8217226
to
162f9f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done editorial review and this all looks to be in good order. I understand there's some additional bits to come, but this chunk looks testable and more importantly usable+useful.
|
||
<p class="example">For example, when serializing a 2D context that has | ||
<span data-x="concept-canvas-color-type">color type</span> of | ||
<span data-x="dom-CanvasColorType-float16">float16</span> to <var>type</var> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to other reviewers. The use of the variable type here outside of an algorithm looks wrong at first, but all of the paragraphs in this section are specifying requirements on "a serialization of the bitmap as a file" and there are a few other uses of type. It seems OK to leave this style alone I think.
The default behavior is unorm8, which matches the existing behavior of
all user agents.
The name unorm8 is chosen to match modern APIs which distinguish between
8-bit unsigned normalized and 8-bit unsigned integer (e.g,
VK_FORMAT_R8G8B8A8_UNORM vs VK_FORMAT_R8G8B8A8_UINT in Vulkan,
MTLPixelFormatRGBA8Unorm vs MTLPixelFormatRGBA8Uint in Metal, and
"rgba8unorm" vs "rgba8uint" in WebGPU).
The use of colorType (instead of pixelFormat) is used to avoid the
complexity of selecting channel ordering (BGRA vs RGBA) and interactions
with the alpha parameter (RGB, RGBX, or BGRX potentially being required if
alpha is false).
/canvas.html ( diff )