-
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
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). |
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.
source
Outdated
|
||
<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.
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.
This needs to update getContextAttributes()
as well.
I don't really like that we don't really have anything about a processing model here. It's fairly unusual to introduce enum members in this way as well. Normally enum members are defined in IDL and the processing model makes it clear what kind of impact they have. I guess this matches the colorSpace
prior art, but at some point we'll have to refactor all of this to instead have proper processing models and that will be quite some work.
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).
162f9f3
to
1ded73e
Compare
Thanks, updated this (and will include it in the WPT tests). |
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.
Can you please file the MDN issue?
I'm not quite sure how to deal with the WebKit standards-positions issue as this is only a subset of the whole idea, but I confirmed with colleagues that we are okay with this specific PR so in terms of implementer interest this should be fine.
Done, updated the issue description.
Thanks! |
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).
colorType
documentation toCanvasRenderingContext2DSettings
mdn/content#38087/canvas.html ( diff )