-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add ImageData Float16Array support #11143
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
Conversation
Add the ImageDataPixelFormat enum type, which can take on values of "rgba-unorm8" (the current default of 8-bit unsigned normalized RGBA) and "rgba-float16" (the new option for 16-bit floating point RGBA). Add ImageDataPixelFormat to the ImageDataSettings dictionary used by ImageData constructors, to allow creation of ImageData objects that are backed by Float16Array. Add ImageDataPixelFormat as an attribute to ImageData. Add a new ImageDataArray type which is the union of Uint8ClampedArray and Float16Array. Change the ImageData data attribute to be this union type instead of Uint8ClampedArray. Fix the Canvas Pixel ArrayBuffer section to correctly describe the pixel layout of an ImageBuffer (there was a bug in the spec specifying that an ImageData's pixel data starts at the beginning of its data attributes's backing ArrayBuffer, when what was intended was for it to start at the beginning of its data attribute). Fixes whatwg#10856
5adbe23
to
08076f0
Compare
source
Outdated
@@ -70609,30 +70614,61 @@ try { | |||
<p>To <dfn>initialize an <code>ImageData</code> object</dfn> <var>imageData</var>, given a | |||
positive integer number of rows <var>rows</var>, a positive integer number of pixels per row |
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 isn't new, but aren't the arguments inverted here?
All the call-sites do Initialize imageData given sw
, sh
, settings
set to [...], but here the algorithm expects number of rows
as the first argument and then pixels per row
.
Though maybe this should be fixed separately and I'm sure there is some modernization that could get in there.
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 swapped the ordering of the parameters, since that seems fairly urgent to not have wrong (when we do a modernization pass, we should also change the names to width
and height
).
This PR also adds the I could imagine this being important if you wanted to accept a |
Indeed this is partially for future-proofing. This is intended to handle the situation where there isn't a 1:1 relationship between the It's also there because it felt odd for the And it's also a good way of checking the actual layout (as opposed to the sub-type of the (And feature detection was sort of an afterthought).
Indeed, we would not want to support anything like that (float to uint8, etc). |
Right, I think it's a reasonable member of |
@@ -70491,7 +70497,7 @@ try { | |||
constructor steps are:</p> | |||
|
|||
<ol> | |||
<li><p>Let <var>length</var> be the number of bytes in <var>data</var>.</p></li> | |||
<li><p>Let <var>length</var> be the length of <var>data</var>.</p></li> |
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 should reference some actual algorithm. It seems currently we only have https://webidl.spec.whatwg.org/#buffersource-byte-length so maybe we need to add this to Web IDL first.
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 changed all of the math to be in term of "bytes per pixel", since that's more natural (I had been leaning on the fact that all pixel formats so far are 4 elements per pixel, which works so far, but would require re-writing later down the line), and that made this issue go away.
That said, should we open an issue on Web IDL to add length
to TypedArray
? (Or should we wait until something will use it).
Any further thoughts on this? |
If <var>settings</var> was given and | ||
<var>settings</var>["<code data-x="dom-ImageDataSettings-pixelFormat">pixelFormat</code>"] | ||
equals "<span data-x="dom-ImageDataPixelFormat-rgba-float16">rgba-float16</span>" then | ||
<var>bytesPerPixel</var> is equal to 8.</p></li> |
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 seems wrong. settings is always given after bindings have been resolved so this shouldn't be this conditional unless I'm missing something.
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.
Indeed, settings was conditional in the constructor, not here.
That said, it's easier to re-work this so that settings is not optional, and have the one function that didn't specify settings create its own ImageDataSettings
. I've done that, and non-optional-ized settings. The diff turned out to be fairly small.
Pixel <code data-x="idl-ArrayBuffer">ArrayBuffer</code></span> must have the correct size to | ||
store <var>rows</var> × <var>pixelsPerRow</var> pixels.</p> | ||
<p>If <var>source</var> was given:</p> | ||
<ol> |
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.
Newline before ol.
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.
Done.
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 does not appear to be done?
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.
Oops (I did something here, I assume you!). Should be fixed now (along with the next ol
, which also missed it).
source
Outdated
<p>The <code>ImageDataPixelFormat</code> enumeration is used to specify type of the | ||
<code data-x="dom-imagedata-data">data</code> attribute of an <code>ImageData</code> | ||
and the arrangement and numerical representation of the color components for each pixel.</p> | ||
|
||
<p>The "<dfn enum-value for="ImageDataPixelFormat"><code | ||
data-x="dom-ImageDataPixelFormat-rgba-unorm8">rgba-unorm8</code></dfn>" value indicates that the | ||
<code data-x="dom-imagedata-data">data</code> attribute of an <code>ImageData</code> must be of type | ||
<span data-x="idl-Uint8ClampedArray">Uint8ClampedArray</span>. | ||
The color components of each pixel must be stored in four sequential elements in the order of red, green, blue, and then alpha. | ||
Each element represents the 8-bit unsigned normalized value for that component.</p> | ||
|
||
<p>The "<dfn enum-value for="ImageDataPixelFormat"><code | ||
data-x="dom-ImageDataPixelFormat-rgba-float16">rgba-float16</code></dfn>" value indicates that the | ||
<code data-x="dom-imagedata-data">data</code> attribute of an <code>ImageData</code> must be of type | ||
<span data-x="idl-Float16Array">Float16Array</span>. | ||
The color components of each pixel must be stored in four sequential elements in the order of red, green, blue, and then alpha. | ||
Each element represents the value for that component.</p> | ||
|
||
<p>An <code>ImageData</code> object <dfn data-x="concept-imagedata-bitmap-representation">represents a rectanglar bitmap</dfn> | ||
with width equal to the <code data-x="dom-imagedata-width">width</code> attribute | ||
and height equal to the <code data-x="dom-imagedata-height">height</code> attribute. | ||
The pixel values of this bitmap are stored in the <code data-x="dom-imagedata-data">data</code> attribute | ||
in left-to-right order, row by row from top to bottom, starting with 0 for the top left pixel, | ||
with the order and numerical representation of the color components of each | ||
pixel determined by the <code data-x="dom-imagedata-pixelFormat">pixelFormat</code> attribute. | ||
The color space of the pixel values of the bitmap is determined by the | ||
<code data-x="dom-imagedata-colorSpace">colorSpace</code> attribute.</p> |
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.
The wrapping here seems wrong. I'm also a bit uncomfortable with postponing the cleanup as all of this will have to be redone and re-reviewed. Does it seem untenable to do the cleanup first and then land this?
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.
The wrapping here seems wrong.
I re-wrapped this block to be 100 chars limit (which seems to be mostly adhered to).
I'm also a bit uncomfortable with postponing the cleanup as all of this will have to be redone and re-reviewed. Does it seem untenable to do the cleanup first and then land this?
The cleanup's changes to this section will be fairly small (remove the word attribute and change the xref), so it will be easy to land that on top of this. The other way around will basically require redoing this from scratch, because the cleanup will have moved all of the blocks around, and the cleanup will probably also take a long time in review.
I hope that the work I've done so far (e.g, the cleanup of getContextAttributes
) shows that I'm serious about the stewardship of this area, and that I indeed prefer to prepare the ground before making changes, when it is beneficial. But in this the benefit appears marginal, and I do see that it would be extremely disruptive to the effort that we and the other reviewers have put in to this PR.
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.
Thanks, updated the PR.
If <var>settings</var> was given and | ||
<var>settings</var>["<code data-x="dom-ImageDataSettings-pixelFormat">pixelFormat</code>"] | ||
equals "<span data-x="dom-ImageDataPixelFormat-rgba-float16">rgba-float16</span>" then | ||
<var>bytesPerPixel</var> is equal to 8.</p></li> |
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.
Indeed, settings was conditional in the constructor, not here.
That said, it's easier to re-work this so that settings is not optional, and have the one function that didn't specify settings create its own ImageDataSettings
. I've done that, and non-optional-ized settings. The diff turned out to be fairly small.
Pixel <code data-x="idl-ArrayBuffer">ArrayBuffer</code></span> must have the correct size to | ||
store <var>rows</var> × <var>pixelsPerRow</var> pixels.</p> | ||
<p>If <var>source</var> was given:</p> | ||
<ol> |
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.
Done.
source
Outdated
<p>The <code>ImageDataPixelFormat</code> enumeration is used to specify type of the | ||
<code data-x="dom-imagedata-data">data</code> attribute of an <code>ImageData</code> | ||
and the arrangement and numerical representation of the color components for each pixel.</p> | ||
|
||
<p>The "<dfn enum-value for="ImageDataPixelFormat"><code | ||
data-x="dom-ImageDataPixelFormat-rgba-unorm8">rgba-unorm8</code></dfn>" value indicates that the | ||
<code data-x="dom-imagedata-data">data</code> attribute of an <code>ImageData</code> must be of type | ||
<span data-x="idl-Uint8ClampedArray">Uint8ClampedArray</span>. | ||
The color components of each pixel must be stored in four sequential elements in the order of red, green, blue, and then alpha. | ||
Each element represents the 8-bit unsigned normalized value for that component.</p> | ||
|
||
<p>The "<dfn enum-value for="ImageDataPixelFormat"><code | ||
data-x="dom-ImageDataPixelFormat-rgba-float16">rgba-float16</code></dfn>" value indicates that the | ||
<code data-x="dom-imagedata-data">data</code> attribute of an <code>ImageData</code> must be of type | ||
<span data-x="idl-Float16Array">Float16Array</span>. | ||
The color components of each pixel must be stored in four sequential elements in the order of red, green, blue, and then alpha. | ||
Each element represents the value for that component.</p> | ||
|
||
<p>An <code>ImageData</code> object <dfn data-x="concept-imagedata-bitmap-representation">represents a rectanglar bitmap</dfn> | ||
with width equal to the <code data-x="dom-imagedata-width">width</code> attribute | ||
and height equal to the <code data-x="dom-imagedata-height">height</code> attribute. | ||
The pixel values of this bitmap are stored in the <code data-x="dom-imagedata-data">data</code> attribute | ||
in left-to-right order, row by row from top to bottom, starting with 0 for the top left pixel, | ||
with the order and numerical representation of the color components of each | ||
pixel determined by the <code data-x="dom-imagedata-pixelFormat">pixelFormat</code> attribute. | ||
The color space of the pixel values of the bitmap is determined by the | ||
<code data-x="dom-imagedata-colorSpace">colorSpace</code> attribute.</p> |
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.
The wrapping here seems wrong.
I re-wrapped this block to be 100 chars limit (which seems to be mostly adhered to).
I'm also a bit uncomfortable with postponing the cleanup as all of this will have to be redone and re-reviewed. Does it seem untenable to do the cleanup first and then land this?
The cleanup's changes to this section will be fairly small (remove the word attribute and change the xref), so it will be easy to land that on top of this. The other way around will basically require redoing this from scratch, because the cleanup will have moved all of the blocks around, and the cleanup will probably also take a long time in review.
I hope that the work I've done so far (e.g, the cleanup of getContextAttributes
) shows that I'm serious about the stewardship of this area, and that I indeed prefer to prepare the ground before making changes, when it is beneficial. But in this the benefit appears marginal, and I do see that it would be extremely disruptive to the effort that we and the other reviewers have put in to this PR.
Let me know if there's anything further (once this lands, I'm going to do the patch to move |
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.
Please complete the checkboxes in OP (and leave a comment once you are finished so I get another notification).
Thanks for tackling this! Exciting to see Float16Array
adoption on the web platform side.
Thanks, updated the OP with the newly filed bugs. Will be landing the linked WPT tests (and a bunch more) shortly! |
data-x="dom-imagedata-colorSpace">colorSpace</code></dfn> attribute of <var>imageData</var> to | ||
<var>settings</var>["<dfn dict-member for="ImageDataSettings"><code | ||
data-x="dom-ImageDataSettings-colorSpace">colorSpace</code></dfn>"].</p></li> | ||
|
||
<li><p>Otherwise, if <var>defaultColorSpace</var> was given, then initialize the <code |
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.
Why remove this step? Now createImageData()
doesn't produce the proper ImageData by default.
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.
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.
Sorry. Fixed now!
Add the ImageDataPixelFormat enum type, which can take on values of "rgba-unorm8" (the current default of 8-bit unsigned normalized RGBA) and "rgba-float16" (the new option for 16-bit floating point RGBA).
Add ImageDataPixelFormat to the ImageDataSettings dictionary used by ImageData constructors, to allow creation of ImageData objects that are backed by Float16Array.
Add ImageDataPixelFormat as an attribute to ImageData.
Add a new ImageDataArray type which is the union of Uint8ClampedArray and Float16Array. Change the ImageData data attribute to be this union type instead of Uint8ClampedArray.
Fix the Canvas Pixel ArrayBuffer section to correctly describe the pixel layout of an ImageBuffer (there was a bug in the spec specifying that an ImageData's pixel data starts at the beginning of its data attributes's backing ArrayBuffer, when what was intended was for it to start at the beginning of its data attribute).
Fixes #10856
(See WHATWG Working Mode: Changes for more details.)
/canvas.html ( diff )
/infrastructure.html ( diff )