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 color space support for CanvasRenderingContext2D, ImageData, and ImageBitmap #6562

Merged
merged 28 commits into from
May 3, 2021

Conversation

ccameron-chromium
Copy link
Contributor

@ccameron-chromium ccameron-chromium commented Apr 7, 2021

Add color space support for CanvasRenderingContext2D, ImageData, and ImageBitmap.

NOTE: This PR is being uploaded for initial feedback, it is not ready for wider review (yet).

(See WHATWG Working Mode: Changes for more details.)


/acknowledgements.html ( diff )
/canvas.html ( diff )
/imagebitmap-and-animations.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )

@ccameron-chromium ccameron-chromium marked this pull request as draft April 9, 2021 21:33
@ccameron-chromium ccameron-chromium force-pushed the canvas_color branch 2 times, most recently from 0ea9c96 to 6ca499d Compare April 10, 2021 04:00
@ccameron-chromium ccameron-chromium marked this pull request as ready for review April 10, 2021 04:05
@kenrussell
Copy link
Member

Note to reviewers: this work has been developed in https://github.com/WICG/canvas-color-space/blob/main/CanvasColorSpaceProposal.md and discussed at great length in the W3C's ColorWeb CG: https://github.com/w3c/ColorWeb-CG .

The artifacts here are the result of months of iteration and have been carefully considered.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Did a quick first pass; I've got to run to an appointment but this should help get started!

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
@@ -65610,40 +65678,36 @@ interface <dfn>OffscreenCanvasRenderingContext2D</dfn> {

<div w-nodev>

<!--en-GB--><h5 id="colour-spaces-and-colour-correction">Color spaces and color correction</h5>

Copy link
Member

Choose a reason for hiding this comment

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

A significant amount of normative text here is deleted. Can you help reassure me that it's OK to remove these requirements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Some of the text is moved around, too -- I'll just go through the points in the old code with an explanation of where they went.

  • "The canvas APIs must perform color correction at only two points... when rendering images with their own..."
    • This is now covered by the paragraph at the bottom, "When drawing content to a 2D context, all inputs must be converted to the context's color space before drawing"
    • This is a bit more expansive, covering not just images, but all content (videos and CSS color spaces have color spaces too)
  • "The canvas APIs must perform color correction at only two points... when rendering the actual canvas bitmap to the output device"
    • This is now covered by the phrase "Color space conversion must be applied to the canvas's backing store when rendering the canvas to the output device."
  • "The toDataURL() method, when invoked..."
    • The new behavior is in the "Where output format allows it" paragraph
    • The new behavior is to always use a color profile, instead of never
  • "In user agents that support CSS, the color space used by a canvas element must match the color space used for processing any colors for that element in CSS."
    • I think this was already half-obsolete -- the CSS colors now have a well-defined color space
    • That space is independent of the canvas's color space, and when and how conversion is applied is now defined
    • So I just deleted it
  • "The gamma correction and color space information of images must be handled in such a way ..."
    • This is now at the top "this color space conversion must be identical to the color space conversion that would be applied to an img element..."
    • It also falls out of the fact that relative colorimetric rendering intent is path-independent (you can convert from space X to Y to Z to W or just X to W directly, and, ignoring quantization error, you get the same result)
    • This is also a bit shorter now because I chopped out gamma correction words (gamma is part of the color space, no need to mention is separately)
  • "Furthermore, the rendering of images that have no color correction information..."
    • This behavior changed and is covered by the "Images that specify no color profile information" line
    • This matches what CSS Color Level 4 already says

(Let me know if that clarifies everything!)

Choose a reason for hiding this comment

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

This is also a bit shorter now because I chopped out gamma correction words (gamma is part of the color space, no need to mention is separately)

Good. Partly because the transfer curve is part of the color space definition, and also because the term "gamma correction" is a poor one. Nothing is being corrected, or is less correct about linear; instead the term "gamma encoding" is better. Although still suboptimal because many transfer curves are not a simple power law (gamma is the exponent in a power law).

source Outdated Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Very exciting to see some incremental progress here!

@whatwg/canvas might want to review this as well.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
of images that have no color correction information (such as those returned by the <code
data-x="dom-canvas-toDataURL">toDataURL()</code> method) must be rendered with no color
correction.</p>
<p>Where output format allows it, the <code data-x="dom-canvas-toDataURL">toDataURL()</code> method, when
Copy link
Member

Choose a reason for hiding this comment

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

output formats allow*

It seems better to define this as part of the toDataURL() definition though. Action at a distance is best avoided in specifications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Added this to the "Serializing bitmaps to a file" section. I also clarified that for formats that do not support embedded color profiles, the bitmap should be converted to sRGB before being serialized (the goal being to have a subsequent drawImage of the result from toDataURL be as close to a no-op as possible).

@ccameron-chromium
Copy link
Contributor Author

Thanks everyone for the comments so far. I think that I've covered all of the issues, and I think we've landed in a good place with respect to the questions that we came in here with.

Let me know if there are any other issues or concerns! (And if I've missed any of the corrections -- I did proofread it again again but ... I have an ECC module when reading text that I wrote).

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I found a couple cases remaining in the color space vs colorSpace category.

@@ -60500,6 +60518,9 @@ interface <dfn>Path2D</dfn> {

<li><p><code data-x="concept-canvas-desynchronized">desynchronized</code>, set to this context's
<span data-x="concept-canvas-desynchronized">desynchronized</span>.</p></li>

<li><p><code data-x="concept-canvas-color-space">colorSpace</code>, set to this context's
Copy link
Member

Choose a reason for hiding this comment

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

color space

Copy link
Member

Choose a reason for hiding this comment

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

The problem here is the first one (on this line) should be dom-canvasrenderingcontext2dsettings-colorspace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, all bullet points in this section make that mistake! I updated concept-canvas-alpha,desynchronized,color-space to dom-canvasrenderingcontext2dsettings-alpha,desynchronized,colorSpace

@@ -60500,6 +60518,9 @@ interface <dfn>Path2D</dfn> {

<li><p><code data-x="concept-canvas-desynchronized">desynchronized</code>, set to this context's
<span data-x="concept-canvas-desynchronized">desynchronized</span>.</p></li>

<li><p><code data-x="concept-canvas-color-space">colorSpace</code>, set to this context's
<span>output bitmap</span>'s <span data-x="concept-canvas-color-space">color space</span>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

It's not associated with the output bitmap though, right? It's part of the context and affects how the output bitmap is rendered. Otherwise you need a different concept- here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conceptually there are three distinct things that I may not have been precise about in my language.

  • canvas element, which doesn't have its own color space
  • rendering context (2D or WebGL), which has a color space that affects how things are rendered
  • output bitmap (or backing store), which will be a bitmap where the pixels are in the rendering context's color space

I did a quick sweep of the patch and updated "output bitmap" to be "context" here. Later on, in the "When drawing content to a 2D context..." text, I referred to the "canvas's color space" when the "context's color space" would have been more precise.

source Outdated
values must not be <span data-x="concept-premultiplied-alpha">premultiplied by alpha</span>.</p>
values must not be <span data-x="concept-premultiplied-alpha">premultiplied by alpha</span>.
Pixel values must be converted from the <span>output bitmap</span>'s
<span data-x="dom-CanvasRenderingContext2DSettings-colorSpace">color space</span> to the returned
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gah! Sorry missing it! This time did the s/dom-CanvasRenderingContext2DSettings-colorSpace/concept-canvas-color-space/ for real.

(Note that I didn't remove the "output bitmap" part, because "bitmap" seems most correct in this situation, although equivalent to the "context").

source Outdated
values must not be <span data-x="concept-premultiplied-alpha">premultiplied by alpha</span>.
Pixel values must be converted from the <span>output bitmap</span>'s
<span data-x="dom-CanvasRenderingContext2DSettings-colorSpace">color space</span> to the returned
<code>ImageData</code> object's <span data-x="dom-imagedata-colorSpace">color space</span> using
Copy link
Member

Choose a reason for hiding this comment

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

This too. This is referencing an IDL attribute and those should use <code> (and camelcase).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the first one to s/settings-colorSpace/concept-color-space/, changed the second on to s/span/code/

source Outdated
match the color space used for processing any colors for that element in CSS.</p>
<p class="note">The <a href="https://drafts.csswg.org/css-color/#untagged">Color Spaces of Untagged Colors</a>
section of <cite>CSS Color</cite> specifies that images that specify no color profile information are assumed
to be in the sRGB color space.<ref spec=CSSCOLOR></p>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe merge this note into the note below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense -- merged them into a single note.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Looking great; my review, like @annevk's, is down to some nits at this point.

@@ -60500,6 +60518,9 @@ interface <dfn>Path2D</dfn> {

<li><p><code data-x="concept-canvas-desynchronized">desynchronized</code>, set to this context's
<span data-x="concept-canvas-desynchronized">desynchronized</span>.</p></li>

<li><p><code data-x="concept-canvas-color-space">colorSpace</code>, set to this context's
Copy link
Member

Choose a reason for hiding this comment

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

The problem here is the first one (on this line) should be dom-canvasrenderingcontext2dsettings-colorspace

source Outdated
<p>Color space conversion must be applied to the canvas's backing store when rendering the canvas to the
output device. This color space conversion must be identical to the color space conversion that would
be applied to an <code>img</code> element with a color profile that specifies the same color space as
the canvas's backing store.</p>
Copy link
Member

Choose a reason for hiding this comment

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

"the same color space as the canvas's concept-canvas-color-space", maybe? (Hyperlinked)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the link, but left it reading the same <span data-x="concept-canvas-color-space">color space</span> as the canvas's backing store. Because we're talking about the pixels in the buffer, I think that talking about the color space of the backing store is the right scope (even though the "backing store" space and "context" space are the same).

source Outdated
of images that have no color correction information (such as those returned by the <code
data-x="dom-canvas-toDataURL">toDataURL()</code> method) must be rendered with no color
correction.</p>
<p>When drawing content to a 2D context, all inputs must be converted to the context's color space
Copy link
Member

Choose a reason for hiding this comment

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

"the canvas's concept-canvas-color-space" here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to ``converted to the context's color space``` in the three references.

source Outdated
Comment on lines 65774 to 65775
<p class="note">The color space for CSS colors is defined in <cite>CSS Color</cite>: <ref
spec=CSSCOLOR>. There do not exist any inputs to a 2D context for which the color space is undefined.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p class="note">The color space for CSS colors is defined in <cite>CSS Color</cite>: <ref
spec=CSSCOLOR>. There do not exist any inputs to a 2D context for which the color space is undefined.</p>
<p class="note">The color space for CSS colors is defined in <cite>CSS Color</cite>. There do not
exist any inputs to a 2D context for which the color space is undefined. <ref spec=CSSCOLOR></p>

(stylistic nit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chnaged to a terminal <ref spec=CSSCOLOR></p> in the merged comment.

Pixel values must be converted from the <span>output bitmap</span>'s
<span data-x="dom-CanvasRenderingContext2DSettings-colorSpace">color space</span> to the returned
<code>ImageData</code> object's <span data-x="dom-imagedata-colorSpace">color space</span> using
relative colorimetric intent.
Copy link
Member

Choose a reason for hiding this comment

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

Can "relative colorimetric intent" hyperlink anywhere? (Fine if not, but it would be a nice bonus if it could.)

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the link to the places where it was referenced.

source Show resolved Hide resolved
"<code data-x="dom-PredefinedColorSpace-srgb">srgb</code>".</p>

<p>Initialize the <dfn attribute for="ImageData"><code data-x="dom-imagedata-colorSpace">colorSpace</code></dfn>
attribute of <var>imageData</var> to <var>cs</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

This is missing an <li>, but this and the previous step can be simplified as the dictionary is always passed from the perspective of this algorithm (due to IDL) so you don't have to specify the defaulting here again. You can just say 'Initialize ... to settings["colorSpace"].'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the <li> part.

Holding off on the rest of the change because of the below question about optional-izing the colorSpace key.

source Show resolved Hide resolved
@ccameron-chromium
Copy link
Contributor Author

I've made the ImageDataSettings object to "create an imagedata object" not be optional, and I think it clears things up quite a bit. I also broke getImageData into separate steps in the text, since its paragraph was getting too long.

Still interested in your opinions about the "default to sRGB" behavior. The change to the text would be to make getImageData set the colorSpace member of its ImageDataSettings argument (but only if it isn't set already).

annevk added a commit to whatwg/whatwg.org that referenced this pull request Apr 20, 2021
It has been colorspace in this document since the very beginning (7 November 2016), but on 9 May 2016 we changed colorspace to color space in the HTML Standard quite deliberately: whatwg/html@c92fee2.

Recently this was discussed further in whatwg/html#6562 and we decided to stick with color space.
annevk added a commit to whatwg/whatwg.org that referenced this pull request Apr 20, 2021
It has been colorspace in this document since the very beginning (7 November 2016), but on 9 May 2016 we changed colorspace to color space in the HTML Standard quite deliberately: whatwg/html@c92fee2.

Recently this was discussed further in whatwg/html#6562 and we decided to stick with color space.
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I found some more issues, some might be substantive. (I wish I had gotten further with #3424 as a lot of this would be helped with having more modern foundations.)

<code>ImageData</code> object</span>, with parameter <var>pixelsPerRow</var> set to the
absolute magnitude of <var>sw</var>, and parameter <var>rows</var> set to the absolute magnitude
of <var>sh</var>. Initialize the image data of the new <code>ImageData</code> object to
invoked with two numeric arguments <var>sw</var> and <var>sh</var> and an <code>ImageDataSettings</code>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
invoked with two numeric arguments <var>sw</var> and <var>sh</var> and an <code>ImageDataSettings</code>
invoked with two numeric arguments <var>sw</var> and <var>sh</var>, and an <code>ImageDataSettings</code>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated
@@ -64002,24 +64043,24 @@ try {

<dl class="domintro">
<dt><var>imagedata</var> = new <code subdfn data-x="dom-imagedata">ImageData</code>(<var>sw</var>, <var>sh</var>)</dt>
Copy link
Member

Choose a reason for hiding this comment

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

This also changed. It probably needs to get its own (web developer facing) description separately from createImageData() now they behave somewhat differently when it comes to color space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point -- I separated them out. I also moved the two createImageData functions next to each other (they were shuffled because of the shared definitions).

source Outdated Show resolved Hide resolved
<var>imageData</var> to the <dfn dict-member for="ImageDataSettings"><code data-x="dom-ImageDataSettings-colorSpace">colorSpace</code></dfn>
member of <var>settings</var>.</p>

<p>Otherwise, if <var>defaultColorSpace</var> is specified, then initialize the
Copy link
Member

Choose a reason for hiding this comment

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

This should be a new <li> as well. Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

source Outdated
@@ -95092,6 +95177,7 @@ dictionary <dfn>ImageBitmapOptions</dfn> {
<span>ImageOrientation</span> <span data-x="dom-ImageBitmapOptions-imageOrientation">imageOrientation</span> = "<span data-x="dom-ImageOrientation-none">none</span>";
<span>PremultiplyAlpha</span> <span data-x="dom-ImageBitmapOptions-premultiplyAlpha">premultiplyAlpha</span> = "<span data-x="dom-PremultiplyAlpha-default">default</span>";
<span>ColorSpaceConversion</span> <span data-x="dom-ImageBitmapOptions-colorSpaceConversion">colorSpaceConversion</span> = "<span data-x="dom-ColorSpaceConversion-default">default</span>";
<span>PredefinedColorSpace</span> <span data-x="dom-ImageBitmapOptions-colorSpace">colorSpace</span> = "<span data-x="dom-PredefinedColorSpace-srgb">srgb</span>";
Copy link
Member

Choose a reason for hiding this comment

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

While reading through the new createImageBitmap() text I started to wonder if these options should also apply to <canvas> and ImageData now that they have color space information that can differ from the desired color space. (If nobody wants to implement that I think at the very least we should throw.)

We also don't seem to consider color space for video and the SVG image element. The latter is probably an oversight?

Also, should concept-imagebitmap-bitmap-data be updated to also have a color space, apart from a width and height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's probably best for me to take the createImageBitmap part out of this PR, and then take up the issue along with (or on top of) the changes you were suggesting for createImageBitmap.

The original version of the patch had a much shorter section on createImageBitmap and left things more vague and implementation-defined. I've reverted back to an even smaller version of that (not adding the colorSpace field, and just a few wording here and there).

It was for WebGL and WebGPU textures that we added the colorSpace field, but the same behavior can be accomplished in other ways, and it seems better to leave it out.

While reading through the new createImageBitmap() text I started to wonder if these options should also apply to <canvas> and ImageData now that they have color space information that can differ from the desired color space. (If nobody wants to implement that I think at the very least we should throw.)

It depends on how the ImageBitmap is going to be used.

If the goal is to use the ImageBitmap for a WebGL or WebGPU texture, then all content should be converted to a very well-defined pixel format and color space. Because WebGL and WebGPU aren't color managed, the application needs to know its exact starting place, to do the right math for their manual color management. For this use case, createImageBitmap should arguably allow specifying a pixel format, too (RGBA v BGRA v R10G10B10A2 v half-float, etc).

If the goal is to use the ImageBitmap in a 2D or ImageBitmap RenderingContext, then the exact internal representation doesn't matter, and no content should have its pixel format or color space altered. The 2D and ImageBitmap contexts do color management and pixel format conversion internally. Ideally, from a performance perspective, the content shouldn't be converted at all, and createImageBitmap would create some sort of copy-on-write alias of the original source content.

Also, should concept-imagebitmap-bitmap-data be updated to also have a color space, apart from a width and height?

I don't think so. In the case where we're not sending the data into WebGL or WebGPU, we want ImageBitmaps to be in whatever internal color space is most appropriate for them (which for many elements may even be some YUV video format). Any color space we would expose would likely be a lie in some circumstances.

Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we could press ahead and try to get the colorSpace dictionary member specified here instead of pushing it off. ImageBitmap is the path recommended to web developers nowadays for uploading images into WebGL and WebGPU, since it performs all conversions early on, and avoids synchronous re-conversions at the time of use. ImageBitmap behaves distinctly from <canvas> and ImageData in this regard. WebGL, for example, will perform certain operations like vertical flips for most image sources, but by design, does not do these for ImageBitmaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that ImageBitmap is already being torn between two separate uses (WebGL/GPU vs 2D/ImageBitmap), I don't think it's appropriate to add features to it in this PR. It needs careful separate consideration, and a clear vision of its future.

Copy link

Choose a reason for hiding this comment

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

@kenrussell ImageBitmap is recommended in Chrome, but not in Firefox. Usually you're better off without ImageBitmap intermediaries in Firefox.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would strongly prefer to cover ImageBitmap/WebGL issues in a separate PR.

There is separate work in Khronos for WebGL (which I believe jdashg volunteered for), and I feel that this will significantly expand the scope of the patch (which is narrowly focused on 2D/ImageData). The class of under-defined behavior that you mentioned already exists and is very wide (importing video to WebGL is the most prevalent example), and I think it needs its own focused treatment, rather than to be being picked up incidentally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, since it wasn't clear, I'm volunteering to start on that cleanup starting immediately, I just would prefer it to be separated out from this PR. I can also incorporate #3424 into that work if you'd like.

Copy link
Member

Choose a reason for hiding this comment

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

My worry is that we end up in an inconsistent state, which is generally something we try to avoid. I'll defer to @domenic as to whether that's blocking here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I think we'll be able to resolve the issue on the WebGL side of things (and if that goes as I hope, we'll be able to just tighten up some language on this side).

Copy link
Member

Choose a reason for hiding this comment

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

I took a look and I'm comfortable with how this only touches one part of the problem for now, cleanly leaving the rest of future work. I trust @ccameron-chromium to continue with that work.

I'm not really able to evaluate the extent of the problem, but I found

The class of under-defined behavior that you mentioned already exists and is very wide (importing video to WebGL is the most prevalent example), and I think it needs its own focused treatment, rather than to be being picked up incidentally.

compelling to the extent that it's true, and if we can solve it all at once that'd be lovely.

@domenic
Copy link
Member

domenic commented Apr 22, 2021

I'm working on an editorial update to modernize some stuff while I'm in the area, but it looks good so far. I'll push soon.

However I noticed one potential bigger problem. OffscreenCanvasRenderingContext2D also mixes in CanvasImageData, but does not have a color space member, so the definition of createImageData() doesn't work.

@ccameron-chromium
Copy link
Contributor Author

However I noticed one potential bigger problem. OffscreenCanvasRenderingContext2D also mixes in CanvasImageData, but does not have a color space member, so the definition of createImageData() doesn't work.

I had thought that it mixed in the settings parsing and the color space member, but you're right, it doesn't! I've added these to OffscreenCanvasRenderingContext2D now.

I noticed that OffscreenCanvasRenderingContext2D has a similar alpha property which can be set via CanvasRenderingContext2DSettings. What's odd is that the spec refers to being able to dynamically change it, but there's no API for that (and the setting isn't even accessible -- it's not an attribute, and there is no getContextAttributes mixed in). Is that a known issue?

@domenic
Copy link
Member

domenic commented Apr 22, 2021

I noticed that OffscreenCanvasRenderingContext2D has a similar alpha property which can be set via CanvasRenderingContext2DSettings. What's odd is that the spec refers to being able to dynamically change it, but there's no API for that (and the setting isn't even accessible -- it's not an attribute, and there is no getContextAttributes mixed in). Is that a known issue?

This is probably worth tracking separately... I agree as written it's odd. I think it's trying to say "Set it to true by default. The 'offscreen 2D context creation algorithm' might change this."

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I'm happy with this PR.

I pushed another commit which does a pretty large editorial modernization of the touched sections as they were starting to get really crufty and I was sad. That sort of thing shouldn't be on contributors but while we're in the area I like to leave things better than they started.

Can you make sure to file a WebKit bug and update the OP with it? Also can we get a commitment to test all the things that changed during review, e.g. handling of invalid values, getImageData-then-putImageData, ... any others I missed?

@annevk are you happy with this landing after those steps?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I found a couple nits that I can address before landing. I agree with @domenic on the other remaining TODOs here.

One thing I would really appreciate (and maybe we should add this to the specification as well) is a concrete example. E.g., if you create a context using 'display-p3' and then use fillStyle = hexNotation (as per https://drafts.csswg.org/css-color/#hex-notation). Presumably if you then do getImageData() (and don't do color space conversion; but I guess also if you do) you would see some different values there in the data. I think that would be helpful to demonstrate.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
the actual <code data-x="idl-Uint8ClampedArray">Uint8ClampedArray</code> object passed as the
first argument to the constructor.</p>
<p class="note">This step does not set <span>this</span>'s data to a copy of <var>data</var>.
It's sets it to the actual <code data-x="idl-Uint8ClampedArray">Uint8ClampedArray</code> object
Copy link
Member

Choose a reason for hiding this comment

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

It sets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think an intervening change fixed this (I don't see the typo here anymore)

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@ccameron-chromium
Copy link
Contributor Author

One thing I would really appreciate (and maybe we should add this to the specification as well) is a concrete example.

I've added an example into the ImageData set of examples in the spec, drawing #FF0000 to a canvas and reading back (234, 51, 35) from getImageData.

@annevk
Copy link
Member

annevk commented Apr 28, 2021

Thanks Christopher! Would you like to list your name in the Acknowledgments section? I will give this a final look later and get it landed.

@ccameron-chromium
Copy link
Contributor Author

Can you make sure to file a WebKit bug and update the OP with it?

Yes, updated with the WebKit bug.

Also can we get a commitment to test all the things that changed during review, e.g. handling of invalid values, getImageData-then-putImageData, ... any others I missed?

Yes. We are currently working on adding tests for the behaviors that we have updated during iteration here (the things you mention -- handling of invalid values, and also the default behavior for getImageData and createImageData, once we get them working).

That said, several of the tests have already rolled into upstream (getImageData, toBlob, and putImageData, but all with explicit arguments).

@ccameron-chromium
Copy link
Contributor Author

Thanks Christopher! Would you like to list your name in the Acknowledgments section? I will give this a final look later and get it landed.

Thank you! Yes, certainly, add my name.

@annevk
Copy link
Member

annevk commented Apr 28, 2021

@ccameron-chromium it seems that @domenic's editorial commit got lost when you rebased. I'll try to revive what I can from https://github.com/whatwg/html/compare/eb6c996d7461ac4175b5a47ef381f0fcaa9845ac..93fea05ccf4222977becb51530e85810c24ce218.

@annevk
Copy link
Member

annevk commented Apr 28, 2021

I think all is in order now, but given the mishap I'd like to wait for @domenic to return so he can do a final check and land this Monday.

And once it's landed we can do a small follow-up PR to remove the remaining sRGB references.

@annevk annevk mentioned this pull request Apr 28, 2021
@domenic domenic added addition/proposal New features or enhancements topic: canvas labels May 3, 2021
@domenic domenic merged commit 85cb202 into whatwg:main May 3, 2021
kenrussell added a commit to kenrussell/WebGL that referenced this pull request Jun 15, 2021
Define its behavior both for presenting the rendering context's
results, and uploading textures via texImage2D / texSubImage2D from
DOM inputs (the TexImageSource union type).

Follows whatwg/html#6562 , in which the
PredefinedColorSpace enum was defined, and its behavior for 2D canvas
contexts specified.
kenrussell added a commit to KhronosGroup/WebGL that referenced this pull request May 7, 2022
…ringContextBase. (#3292)

Define their behavior both for presenting the rendering context's
results, and uploading textures via texImage2D / texSubImage2D from
DOM inputs (the TexImageSource union type). Notably:

 - The drawing buffer is reallocated when drawingBufferColorSpace is
   changed.

 - unpackColorSpace applies to ImageBitmaps.

Follows whatwg/html#6562 , in which the
PredefinedColorSpace enum was defined, and its behavior for 2D canvas
contexts specified.

Address review feedback from kainino, ccameron-chromium and kdashg.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: canvas
Development

Successfully merging this pull request may close these issues.

8 participants