Editorial: putImageData: Clarify that the operation is not a memcpy #11042

ccameron-chromium · 2025-02-18T10:50:01Z

Ensure that it be clear that putImage will convert from the source ImageData's color space and pixel format to the output bitmap's color space and pixel format.

Also rename dirtyX,dirtyY,dirtyWidth,dirtyHeight to sx,sy,sw,sh. The original names reflect the use case where content is being rendered into an ImageData by JavaScript and that one wishes to copy the "dirty" part of the source ImageData to the destination canvas. This can be confusing because this process will "dirty" the destination canvas. Change this to use the same source and destination parameter names that are used by drawImage and related functions.

/canvas.html ( diff )

annevk · 2025-02-18T12:40:19Z

source

  <span>ImageData</span> <span data-x="dom-context-2d-getImageData">getImageData</span>([EnforceRange] long sx, [EnforceRange] long sy, [EnforceRange] long sw, [EnforceRange] long sh, optional <span>ImageDataSettings</span> settings = {});
  undefined <span data-x="dom-context-2d-putImageData">putImageData</span>(<span>ImageData</span> imagedata, [EnforceRange] long dx, [EnforceRange] long dy);
-  undefined <span data-x="dom-context-2d-putImageData">putImageData</span>(<span>ImageData</span> imagedata, [EnforceRange] long dx, [EnforceRange] long dy, [EnforceRange] long dirtyX, [EnforceRange] long dirtyY, [EnforceRange] long dirtyWidth, [EnforceRange] long dirtyHeight);
+  undefined <span data-x="dom-context-2d-putImageData">putImageData</span>(<span>ImageData</span> imagedata, [EnforceRange] long dx, [EnforceRange] long dy, [EnforceRange] long sx, [EnforceRange] long sy, [EnforceRange] long sw, [EnforceRange] long sh);


We should probably do this separately, but I think it would be good to use non-abbreviated parameter names here.

Yeah, removed it. And I agree it should stay as the current names. It's an extremely bizarre API.

I was not suggesting the current names necessarily, but destinationX and sourceX would be better than dx and sx. Maybe we can fix imagedata -> imageData now?

Actually, it's such a strange behavior (I didn't realize the details of it until editing the spec here!) that I don't want to mess with the "dirty" variable names in this patch.

But imagedata to imageData is safe. I updated that throughout the full spec.

And I agree it should stay as the current names. It's an extremely bizarre API.

Yes, from the top comment I was going to say "beware! these are not the same!" and I agree dirtyX et al. are super weird. Every time I face them, I have to check again some old answer of mine onstackoverflow to remember what these represent exactly. I don't think we can touch it now, but "dirty" seems quite à-propos.

But all of sx, sy, sw, and sh in getImageData, and, dx and dy in putImageData can indeed be changed to sourceX, sourceY, sourceWidth, sourceHeight, destinationX, and destinationY. But maybe that's worth its own PR since drawImage would also need to be updated.

annevk · 2025-02-18T12:42:59Z

source

  <p>The <dfn method for="CanvasImageData"><code
-  data-x="dom-context-2d-putImageData">putImageData()</code></dfn> method writes data from
-  <code>ImageData</code> structures back to the rendering context's <span>output bitmap</span>. Its
-  arguments are: <var>imagedata</var>, <var>dx</var>, <var>dy</var>, <var>dirtyX</var>,
-  <var>dirtyY</var>, <var>dirtyWidth</var>, and <var>dirtyHeight</var>.</p>
+  data-x="dom-context-2d-putImageData">putImageData()</code></dfn> method converts and copies data
+  from <code>ImageData</code> structures to the rendering context's <span>output bitmap</span>. Its
+  arguments are: <var>imagedata</var>, <var>dx</var>, <var>dy</var>, <var>sx</var>,
+  <var>sy</var>, <var>sw</var>, and <var>sh</var>.</p>

-  <p>When the last four arguments to this method are omitted, they must be assumed to have the
-  values 0, 0, the <code data-x="dom-imagedata-width">width</code> member of the <var>imagedata</var> structure, and the <code data-x="dom-imagedata-height">height</code>
-  member of the <var>imagedata</var> structure, respectively.</p>
+  <p>Then last four arguments to this method indicate the source rectangle for the copy. When they
+  are omitted, they must be assumed to have the values 0, 0, the
+  <code data-x="dom-imagedata-width">width</code> member of the <var>imagedata</var> structure, and
+  the <code data-x="dom-imagedata-height">height</code> member of the <var>imagedata</var>
+  structure, respectively.</p>


Can we modernize this to

The putImageData(imageData, dx, dy, ...) method steps are:

and since there's an overload I guess we need that intro twice and then they both call into a shared algorithm. That would also remove the need for this weird "if they are omitted" language.

Changed it to a common algorithm (modernized) with both methods documented separately and calling the common algorithm.

annevk · 2025-02-19T09:38:11Z

source

  <span>ImageData</span> <span data-x="dom-context-2d-getImageData">getImageData</span>([EnforceRange] long sx, [EnforceRange] long sy, [EnforceRange] long sw, [EnforceRange] long sh, optional <span>ImageDataSettings</span> settings = {});
  undefined <span data-x="dom-context-2d-putImageData">putImageData</span>(<span>ImageData</span> imagedata, [EnforceRange] long dx, [EnforceRange] long dy);
-  undefined <span data-x="dom-context-2d-putImageData">putImageData</span>(<span>ImageData</span> imagedata, [EnforceRange] long dx, [EnforceRange] long dy, [EnforceRange] long dirtyX, [EnforceRange] long dirtyY, [EnforceRange] long dirtyWidth, [EnforceRange] long dirtyHeight);
+  undefined <span data-x="dom-context-2d-putImageData">putImageData</span>(<span>ImageData</span> imagedata, [EnforceRange] long dx, [EnforceRange] long dy, [EnforceRange] long sx, [EnforceRange] long sy, [EnforceRange] long sw, [EnforceRange] long sh);


I was not suggesting the current names necessarily, but destinationX and sourceX would be better than dx and sx. Maybe we can fix imagedata -> imageData now?

annevk · 2025-02-19T09:40:15Z

source

-  arguments are: <var>imagedata</var>, <var>dx</var>, <var>dy</var>, <var>dirtyX</var>,
-  <var>dirtyY</var>, <var>dirtyWidth</var>, and <var>dirtyHeight</var>.</p>
+  data-x="dom-context-2d-putImageData-short">putImageData(<var>imagedata</var>,
+  <var>dx</var>, <var>dy</var>)</code></dfn> method will


The format is:

The X(...) method steps are to ...

you also want to supply the arguments in the same order and not rely on the reader to do some argument name mapping. That also means you don't set dirtyWidth to something but instead you'd pass imageData's width. (Though ideally not the public API for that, but maybe we only have public API at this point. Another thing to fix in due course.)

Changed this to do the "steps are to [algorithm name], given [params]" format.

Ensure that it be clear that putImage will convert from the source ImageData's color space and pixel format to the output bitmap's color space and pixel format. Also document the two separate putImageData methods as separate methods with a common algorithm, rather than a single method with optional parameters.

annevk

Thanks! Very close now. @Kaiido want to do a pass as well?

annevk · 2025-02-19T13:00:54Z

source

+  <var>dx</var>, <var>dy</var>)</code></dfn> method steps are to
+  <span data-x="dom-context2d-putImageData-common">put pixels from an
+  <code>ImageData</code> onto a bitmap</span>, given
+  <var>imageData</var>, the <span>output bitmap</span> of the rendering context,


We can make the second argument <span>this</span>'s <span>output bitmap</span>, right?

source

Kaiido

LGTM, modulo a small nit.
Though I'd have expected to see something about alpha-multiplication in the algo, but that's already an improvement. Thanks.

Kaiido · 2025-02-20T01:10:46Z

source

-   in the rendering context's <span>output bitmap</span>.</p></li>
+   data-x=""><var>dirtyY</var>+<var>dirtyHeight</var></span></span>,
+   set the pixel with coordinate (<span data-x=""><var>dx</var>+<var>x</var></span>,
+   <var>dy</var>+<var>y</var>) in <var>bitmap</var> to the color at coordinate


https://html.spec.whatwg.org/multipage/canvas.html#canvas-pixel-arraybuffer doesn't talk at all about "color" but really represents values in an ArrayBuffer, so maybe "to the color of the pixel at coordinate"?.
Also, this is not new, but maybe the alpha: false special case should be handled here explicitly? Though since here we're not in the correct CanvasSettings interface, that might be complex...

Updated it to read "to the color of the pixel at coordinate".

I can follow up with a patch that indicates the alpha behavior (and for that patch I'll need to include new WPT tests, since it doesn't look like that behavior is tested yet).

annevk · 2025-02-20T15:36:04Z

Thanks @ccameron-chromium!

Kaiido · 2025-02-21T06:19:28Z

Looks like the "Fixes #11037" edit got lost and didn't apply. Is there more to that issue that hasn't been fixed here?

annevk · 2025-02-21T07:29:50Z

Thank you for pointing that out! @ccameron-chromium in the future please be aware that you only get to write/edit OP once and then you have to let the bot do an edit before you do another one.

annevk reviewed Feb 18, 2025

View reviewed changes

ccameron-chromium force-pushed the put_image_data branch 3 times, most recently from 3086f9a to ec10c44 Compare February 19, 2025 09:22

annevk reviewed Feb 19, 2025

View reviewed changes

ccameron-chromium added 2 commits February 19, 2025 11:44

Change imagedata to imageData

15275a5

ccameron-chromium force-pushed the put_image_data branch from ec10c44 to 15275a5 Compare February 19, 2025 11:50

annevk reviewed Feb 19, 2025

View reviewed changes

Incorporate review feedback

ab38752

Kaiido reviewed Feb 20, 2025

View reviewed changes

Incorporate review feedback

b476e94

annevk approved these changes Feb 20, 2025

View reviewed changes

annevk merged commit 6a3787f into whatwg:main Feb 20, 2025
2 checks passed

domenic added clarification Standard could be clearer topic: canvas labels Feb 21, 2025

Editorial: putImageData: Clarify that the operation is not a memcpy #11042

Editorial: putImageData: Clarify that the operation is not a memcpy #11042

Uh oh!

Conversation

ccameron-chromium commented Feb 18, 2025 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kaiido left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

annevk commented Feb 20, 2025

Uh oh!

Kaiido commented Feb 21, 2025

Uh oh!

annevk commented Feb 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

ccameron-chromium commented Feb 18, 2025 •

edited by pr-preview bot

Loading