createImageBitmap: Allow resizing without cropping #1546
Conversation
|
I don't understand this change. How would you determine the crop rect in step 8 if those arguments are omitted? |
|
Might need a small spec change, but the default crop should be no crop, as in starts at 0,0 and ends width,height. |
|
Isn't that equivalent to what step 2 is already doing by returning 'input' (uncropped)? |
|
Then when does the resize happen? |
|
Ah, now I see the problem! Perhaps we should just move step 2 to after step 6. |
|
I wonder if we should introduce another intermediate variable that is the result of scaling input. It seems a bit weird to just "scale input" in place. Moving the return after step 6 makes sense to me. @jakearchibald, up for making that change? |
|
Shall do |
|
Updated the PR - instead of moving the step, I moved the cropping related parts to substeps. |
| @@ -92513,23 +92511,35 @@ dictionary <dfn>ImageBitmapOptions</dfn> { | |||
| data-x="dom-ImageBitmapOptions-resizeQuality">resizeQuality</code></dfn> option to guide the | |||
There was a problem hiding this comment.
You didn't do the intermediate variable thing that Domenic suggested to avoid resizing in-place. Should be something like "Let resizedImage be be the result of resizing input ..."
|
Ok, take 3. This time I've also moved cropping before resizing, and if the crop rectangle is outside the dimensions of the original image, it's clipped to the dimensions of the input, which is consistent with |
|
|
||
| <li><p>If the <var>sx</var>, <var>sy</var>, <var>sw</var>, and <var>sh</var> arguments are omitted, return <var>input</var>.</p></li> | ||
| <li><p>If not specified, the <var>sx</var> and <var>sy</var> arguments must default to 0, and | ||
| the <var>sw</var> and <var>sh</var> arguments must default to the intrinsic width and height of |
There was a problem hiding this comment.
The notion of intrinsic size needs an x-ref
There was a problem hiding this comment.
Is "intrinsic" needed here, given that image bitmaps have a specific width & height? (This might be related to the whole rasterization thing, as this algorithm probably shouldn't take a bitmap)
There was a problem hiding this comment.
(the current spec doesn't make an x-ref)
There was a problem hiding this comment.
Right, we could just refer to the width and height attributes of input.
|
Updated. Will squash the commits if everyone's happy with it. |
|
|
||
| <li><p>If either or both of <code data-x="dom-ImageBitmapOptions-resizeWidth">resizeWidth</code> | ||
| and <code data-x="dom-ImageBitmapOptions-resizeHeight">resizeHeight</code> members of | ||
| <var>options</var> are less than or equal to 0, then return a promise rejected with | ||
| <span>"<code>InvalidStateError</code>"</span> <code>DOMException</code> and abort these steps.</p></li> | ||
|
|
||
| <li> | ||
|
|
||
| <p>If Let <var>sx</var>, <var>sy</var>, <var>sw</var> and <var>sh</var> are specified, let |
|
Left some editorial comments. Will leave it to @junov to review the normative parts, notably the change from large sw/sh creating black pixels, to them not doing so; I kind of doubt that's web-compatible, but maybe ImageBitmap is new enough that it's fine... |
| <var>sourceRectangle</var> be a rectangle whose corners are the four points (<var>sx</var>, | ||
| <var>sy</var>), (<var>sx</var>+<var>sw</var>, <var>sy</var>),(<var>sx</var>+<var>sw</var>, | ||
| <var>sy</var>+<var>sh</var>), (<var>sx</var>,<var>sy</var>+<var>sh</var>). Otherwise let | ||
| <var>sourceRectangle</var> be a rectangle whose corners are the four points (0,0), (width of |
There was a problem hiding this comment.
width -> <span data-x="dom-ImageBitmap-width">width</span>
Same for height
There was a problem hiding this comment.
I don't think that's correct, since input is bitmap data, not an ImageBitmap. The spec says
An ImageBitmap object whose [[Detached]] internal slot value is false always has associated bitmap data, with a width and a height.
but doesn't have <dfn>s around width and height here (which IMO is fine).
|
+1 for the normative changes |
* No longer exits early if no cropping specified * Cropping happens before resizing * If the crop rectangle is outside the dimensions of the original image, it's clipped, which is consistent with drawImage * If only one of resizeWidth / resizeHeight is specified, it maintains ratio with the cropped area
|
Editorial comments addressed & squashed. Thanks for dragging me through this. |
* No longer exits early if no cropping specified * Cropping happens before resizing * If the crop rectangle is outside the dimensions of the original image, it's clipped, which is consistent with drawImage * If only one of resizeWidth / resizeHeight is specified, it maintains ratio with the cropped area PR: #1546
|
Awesome!! Merged as a10a3a8 (I added one extra forgotten period at the end of a sentence.) |
|
\o/ |
|
Sick. |
* No longer exits early if no cropping specified * Cropping happens before resizing * If the crop rectangle is outside the dimensions of the original image, it's clipped, which is consistent with drawImage * If only one of resizeWidth / resizeHeight is specified, it maintains ratio with the cropped area PR: whatwg#1546
Small change to step 2 of https://html.spec.whatwg.org/#cropped-to-the-source-rectangle-with-formatting, avoids returning just because cropping isn't set.
+@junov