-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 img.decode() API for "predecoding" images #2332
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.
Looks generally good, just some comments.
source
Outdated
}; | ||
</pre> | ||
|
||
<p>However, this can cause notable dropped frames ("jank"), as inserting the image into the DOM |
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.
It's not inserting the image into the DOM that triggers a decode, but the subsequent paint. For example, a "display:none" img
may never get decoded.
source
Outdated
<p>Causes the user agent to decode the image, returning a promise that fulfills when decoding is | ||
complete. The decoded image data will then be readily available for at least one frame after the | ||
fulfillment, ensuring that attempting to display the image will complete quickly, without any | ||
decoding delay.</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.
I wonder if this needs a bit of context, like "images usually exist in some encoded form; user agents must decode them into pixel buffer form before drawing them".
source
Outdated
<p>Decode the image's media data entirely into its bitmap form, suitable for rapid | ||
painting to the screen. If this cannot be done (e.g. due to invalid image data), reject | ||
<var>promise</var> with an <span>"<code>EncodingError</code>"</span> | ||
<code>DOMException</code> and abort these substeps.</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.
This should mention non-encoded images, like SVG images, somehow. I guess their promise just always succeeds immediately (or sooner)?
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.
Great point. I wonder if the image processing model already has some way of talking about such images.
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.
Or, if we want SVGs to be rasterized asynchronously, which could be useful, we'd probably want to add width and height arguments to decode. Just a thought. Maybe we could add that later.
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.
Hmm. Would that also be useful for the <img src="100x100.png" width="200" height="50">
case?
For that matter, do we need width/height options to decode
, when instead you can just look at img.width
and img.height
?
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 think using img.width/height can get messy. The intrinsic size is not always the most useful thing: it may not represent the resolution required for display. Implementations may cache a pre-resized version of the image that matches the resolution required for displaying the image element, if it is attached to the DOM. But that does not need to be in the spec, at least not in the normative text. It would be an implementation-specific optimization.
This raises another question: is ctx2d.drawImage expected to be fast after decode()? If so, it should be mentioned explicitly in the spec. Otherwise, we could end-up with implementations that optimize only for DOM painting by baking in the scale and color transform required for painting to the DOM, and then the image resource would need to be re-decoded when drawImage is called to avoid a lossy back-conversion.
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 intrinsic size is not what img.width/img.height represent. That's img.naturalWidth/naturalHeight, I believe. img.width/img.height is the width="" and height="" attributes.
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.
Right, I was misled by th MDN docs (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img). Based on the spec (https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-width), img.width/height seem reasonable as far as optimizing for DOM painting is concerned.
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.
And I just edited MDN for the first time (That was easy!)
source
Outdated
});</pre> | ||
|
||
<p>This latter form avoids the jank of the original, by allowing the user agent to decode the | ||
image <span>in parallel</span>, and only inserting it into the DOM once the decoding process is |
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.
"in parallel" is a bit vague (in parallel to what?). Maybe "off the main thread"?
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 will link to https://html.spec.whatwg.org/#in-parallel which is how the spec talks about being off the main thread. Since this is an example we might be able to get away with undefined terms like "off the main thread" though. I'm not sure if that'd be better.
source
Outdated
<pre>const img = new Image(); | ||
img.src = "supernova.jpg"; | ||
img.decode().then(() => { | ||
requstAnimationFrame(() => document.body.appendChild(img)); |
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 don't think there would be any practical difference between doing an appendChild() inside a rAF callback vs. in a timer, or just when the Promise is fulfilled. It would be more compelling to do the appendChild() then kick off some animation that would otherwise jank.
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.
Huh, OK. I thought it would avoid causing relayout calculations etc., as is usually the benefit of only manipulating the DOM during rAF callbacks. Cf. https://github.com/wilsonpage/fastdom#how-it-works. I guess appending children wouldn't count as a "DOM write" in that sense?
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.
If you do all your DOM mutations in one place (e.g. inside a rAF callback) with no interleaved queries that force layout, then it's a win. But if this is the only thing you do in a given event loop cycle, it doesn't matter where you do it perf-wise.
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.
Right, OK. What I was trying to illustrate in this example was that because we have the soft guarantee that it'll stick around for at least a frame, it is possible to use fastdom-like batching techniques with this API. Whereas if, for example, we only were guaranteed that it stuck around for a single event loop turn after the promise callback, the API would still be usable directly as in the previous example, but you would be unable to batch it together with other mutations at rAF timing.
I'll try to update the example and wording to make that clearer tomorrow.
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.
Typo: s/requstAnimationFrame/requestAnimationFrame/
source
Outdated
form for this period of time.)</p> | ||
</li> | ||
|
||
<li><p>Resolve <var>promise</var> with undefined.</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 probably queue a task first. We wouldn't want the promise to be resolved at a random point among other microtasks.
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.
See the paragraph below https://www.w3.org/2001/tag/doc/promises-guide#resolve-promise, although it's problematic in various ways.
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.
Yeah, in part because we don't normatively reference that document (and no task source is defined, which is rather common unfortunately). Uplifting the promises guide to IDL would be really good...
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.
Do you have a suggestion for the task source? None of the existing ones seem to fit.
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.
No, I don't think anyone implements them yet (at least not to the same extent, there are some distinct buckets I think) so I haven't put much time into them.
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.
In that case I'll just leave it as-is?
The stuff around decode that already exist in the spec is: https://html.spec.whatwg.org/#img-good #img-good is only defined for the current request ("The current request is usually referred to as the img element itself."). The pending request is basically hidden right now. But I need to think more about the implications for Anyway, "fully decodable" is then used by https://html.spec.whatwg.org/#check-the-usability-of-the-image-argument and that in turn is used by From what I understand from this PR this is supposed to work:
correct? For |
source
Outdated
fulfillment, ensuring that attempting to display the image will complete quickly, without any | ||
decoding delay.</p> | ||
|
||
<p>The promise will be rejected with a <span>"<code>EncodingError</code>"</span> |
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.
a -> an
source
Outdated
the following steps:</p> | ||
|
||
<ol> | ||
<!-- TODO pending request!?! --> |
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.
IMHO, if there already is a pending request, decode() should return a reference to the same promise object as was returned by the previous call to decode().
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.
Pending request here refers to img's loading model.
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.
That is indeed what the comment refers to. The promise-caching idea is interesting... I guess it makes sense. Let me try that.
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 gave this a try and it's quite complicated. Mainly around figuring out when to clear it and start returning a new promise. I don't think the benefits are worth the drawbacks of adding the extra statefulness to this API. Happy to discuss further though.
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'm weakly on the side of returning the same promise, as long as the previous promise is still pending. The reasoning for this is that it should be the case that as long as the promise didn't complete yet, any additional promises will resolve/throw at the same time. However, if that's complicated, then maybe it's not worth it. Also forgive me if I'm erroneously conflating the notion of a javascript promise and an object that we use in the UA to track that promise.
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.
Hmm OK, I'll give it another shot. Although I'll need @zcorpan to figure out the image stuff for me first.
source
Outdated
<p>Decode the image's media data entirely into its bitmap form, suitable for rapid | ||
painting to the screen. If this cannot be done (e.g. due to invalid image data), reject | ||
<var>promise</var> with an <span>"<code>EncodingError</code>"</span> | ||
<code>DOMException</code> and abort these substeps.</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.
Or, if we want SVGs to be rasterized asynchronously, which could be useful, we'd probably want to add width and height arguments to decode. Just a thought. Maybe we could add that later.
source
Outdated
document.body.appendChild(new Text("Could not load the nebula :(")); | ||
});</pre> | ||
|
||
<p>This latter form avoids the jank of the original, by allowing the user agent to decode the |
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.
"jank" is too colloquial IMHO. Perhaps "halting" or "juddering" would me more appropriate for formal prose?
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'll just go with "dropped frames" like above, and remove the parenthetical definition of "jank" from that.
I've updated the OP with a list of remaining TODOs and also a running list of test cases I thought of.
I don't think that's supposed to work. Trying to decode a src-less image seems bad. I thought that such code would land us in the "broken" state, but I guess instead it lands us in "unavailable", and then later you mutate the current request? Eek. How would you write spec text for disallowing that? Similarly it seems somewhat OK to me for decode() to fail if you resize your browser window while the decoding is in progress (causing a new image load), but maybe it should keep working. Do you have suggestions on how to phrase the spec to allow that to work? |
Possibly we could change the initial state to broken, but that depends on changing For environment changes, yeah, maybe it should just fail. It would probably be weird/unexpected if the decoded image is a different one from what |
source
Outdated
complete. The decoded image data will then be readily available for at least one frame after the | ||
fulfillment, ensuring that attempting to display the image will complete quickly, without any | ||
decoding delay.</p> | ||
<p>Images usually exist in some encoded form; user agents must decode them into raw pixels |
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.
Rephrase to not use "must"?
source
Outdated
<p>This method causes the user agent to decode the image <span>in parallel</span>, returning a | ||
promise that fulfills when decoding is complete. The decoded image data will then be readily | ||
available for at least one frame after the fulfillment, ensuring that attempting to display the | ||
image will complete quickly, without any decoding delay.</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.
I would phrase this without promising that it will happen quickly, since there are other costs like rasterization involved. Maybe just "ensuring that attempting to display the image will complete without decoding delay?"
source
Outdated
data-x="attr-dim-width">width</code> or <code data-x="attr-dim-height">height</code> attributes, | ||
the decoding process will take the specified dimensions into account.</p> | ||
|
||
<p>The promise will be rejected with an <span>"<code>EncodingError</code>"</span> |
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.
Is this supposed to be DecodingError?
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.
Is this supposed to be DecodingError?
I think encoding is used as a noun, not a verb. As in "There is a problem with the encoding of the data I am decoding."
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.
See https://heycam.github.io/webidl/#encodingerror. There is no DecodingError.
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.
Ah makes sense! thanks
source
Outdated
height</span> and the dimension CSS properties <span>'width'</span> and <span>'height'</span> | ||
(including the <span>presentational hints</span> that are <a href="#dimRendering">provided | ||
by</a> setting the <code data-x="attr-dim-width">width</code> or <code | ||
data-x="attr-dim-height">height</code> attributes).</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.
Hmm.. I was under the impression that we decided to punt on things like scaling (in #1920). I think the difficulty is that if the image is created out of dom with some of these attributes, then when it is inserted into the dom, the sizes might change.
For example, if you say an img has a set width/height but then insert it into a div that has a scale transform on it, then the result is that we should've been using a different width/height to decode/scale/prepare this image. Specifically, the prepared object that we have might now not be appropriate to use for the raster.
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.
Hmm OK. I guess I do not have a full mental model of image processing in my head, in particular I wasn't really aware that decoding and rasterization were separate steps. I guess it makes sense that they would be.
@junov, you were the one originally suggesting that we include rasterization, and were amenable to my suggestion that we do it based on width/height attributes. We didn't think of the container-with-transforms case, I guess. Should we back off to just decoding for now?
@smfr, thoughts?
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 style engine takes care of that resizing and does it using the algorithm specified by the image-rendering CSS property. That way decode() could also be used to prepare images for use with WebGLRenderingContext.texImage2D() and CanvasRenderingContext2D.drawImage(), which ignore style, and ignore the specified size (set via HTMLImageElement.width/height)
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.
Decoding is only size-dependent in some cases, for example with progressive JPEG images where you may be able to get away with decoding only some levels if a large image is being presented in a small rectangle. It can also be a memory saving if you know your destination is small to do a full decode, but into a smaller bitmap.
I think here we should assume that the author knows what they are doing, and have decode() always create a bitmap of the image's intrinsic size.
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.
@domenic I changed my mind. I think the rasterized size is something implementations could cache on the side if they chose to.
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.
Rationale: Using decode() should not have side-effects on the rendered result.
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.
Sounds good. I will revert 35b55e6 and add a sentence that bails out early for "vector graphics or other image formats that don't require a decoding".
I don't think we need to explicitly state that it should decode to the intrinsic width/height but let me know if you disagree and think we should add such a sentence.
@zcorpan Especially if people are wanting me to cache the promise, then what I need is some kind of hook for "current request changes". Then we can be sure to reset the promise whenever the current request changes. If current request is sometimes replaced and sometimes mutated in-place, this seems especially hard. Hopefully you can help out with that as part of fixing #1055 :) |
When the current request changes, I think you should not only reset the cached promise, but also reject it if it has not yet been resolved. Leaving promises unresolved indefinitely is evil. |
ffecd54
to
fb67785
Compare
I'm sorry this got left behind. I was kind of hoping #1055 would get fixed so I could work based on that but that was not a good strategy. I've tried to work around that in the latest commit, which IMO should be ready for merging. I am still not caching the promise. Without a clear spec notion of when the current request changes, I am not sure how to spec it. The semantics @vmpstr suggests above of only returning the promise if the previous one is pending are strange for users: sometimes you get a new promise, sometimes the same promise, depending on the vagaries of what is in flight in other parts of the program. And it just requires juggling so much state. So I'll leave that out. A new promise each time seems nice and clean. (I am willing to be convinced otherwise, but I think someone would need to outline exactly how they think it should be specced/implemented.) @vmpstr is working on tests in https://codereview.chromium.org/2769823002. I think the spec is ready for more review though. @zcorpan if you have time I would really appreciate it. |
I pushed a commit to reject if |
Great catch, thank you! |
@smfr, any more thoughts, or should we merge this? |
I don't think we should merge this until tests have landed. |
I see, I didn't realize we have tests in progress as part of the implementation. |
Re: caching a promise or not. The way I see it, the promise that is returned on any repeated calls to decode() will resolve or be rejected at the same time as the rest of them, that's why I was thinking returning the same promise is enough. However, I see that there may be problems with different script states and wording this in the spec, so I think returning a different promise is fine as well. Another item I ran into while playing around with the implementation are partially loaded images. The way I'm reasoning about this is that when a decode() is called, the promise resolves only when the image is fully loaded and decoded. That is, I don't think there is any benefit for us to decode whatever we currently have loaded and resolve the callback, since appending that item to the dom might still be slow if more bytes have loaded since the promise resolved. I think we should mention something about this in the spec. Specifically, what I have now is that the decode()'s promise resolution implies that load event has happened (in fact my prototype waits for load if the image isn't complete yet). However, this becomes a little awkward for animated GIFs (or other animated images), where the load, AFAIK, is fired when the full GIF is loaded, but the decode is only going to decode the first frame. I think that might still be a reasonable behavior, since subsequent frames of the GIF are at least available, if not decoded, at the time the promise resolves. This might be something that we want to discuss/spec though. |
Hmm. Correct me if I am wrong, but I believe this is taken care of in the spec. In particular, it will wait until the image becomes completely available, then will decode, then will fulfill the promise.
Yes, that is how it is specced, I am pretty sure!
Interesting point. I'll include a note in the spec. |
412ce14
to
6896f32
Compare
Shouldn't this resolve to a value, not just undefined.
vs.
|
No; the image is not in any meaningful way the result of decoding the image. You seem to be making an argument that it would allow less typing, but that's not a priority here. |
59804fa
to
1b61e60
Compare
This API allows the author to request that the user agent decode the image, preferably off the main thread, so that when the returned promise fulfills, the image is ready to be inserted into the document, without causing dropped frames due to decoding delay. Notable design choices made: - Rejects for non-active documents (or documents that become non-active) - Immediately fulfills for vector graphics or other formats that do not require decoding - Fulfills for animated images after all of their frames are loaded, not just the first, ensuring jank-free animation Closes #2037. This also includes the editorial change of giving a <dfn> to the "update the rendering" step, and linking to it where it is mentioned.
1b61e60
to
27a97db
Compare
Seems merging this slipped off my radar. Given positive support from Blink and WebKit, and tests at https://github.com/w3c/web-platform-tests/tree/master/image-decodes (although they should probably be moved to the appropriate directory inside html/), I'll go ahead and merge this. |
Add img.decode() API for "predecoding" images
This API alllows the author to request that the user agent decode the
image, preferably off the main thread, so that when the returned promise
fulfills, the image is ready to be inserted into the document, without
causing dropped frames due to decoding delay.
Notable design choices made:
require decoding
just the first, ensuring jank-free animation
Closes #2037.
This also includes the editorial change of giving a to the "update
the rendering" step, and linking to it where it is mentioned.
Original PR conversation follows:
/cc @junov @vmpstr @smfr.
@zcorpan I could use your help on whether I'm integrating with the img element's processing model right. Particularly the stuff around current and pending request; I should maybe be handling those more/differently. Also curious about using terms like "main thread" and "frame" and "decode" which are not terribly well-defined. Concrete phrasing suggestions welcome.
Remaining TODOs:
Test cases: