Closed Bug 1416328 Opened 7 years ago Closed 6 years ago

Implement <img decoding="sync|async|auto">

Categories

(Core :: Graphics: ImageLib, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox63 + fixed

People

(Reporter: dholbert, Assigned: aosmond)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [gfx-noted])

Attachments

(3 files, 8 obsolete files)

4.83 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.08 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
13.30 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
See https://github.com/whatwg/html/issues/1920#issuecomment-343319890

Not specced yet, but likely will be soon. We should consider implementing <img decode="sync"> to force sync decoding (when or before we paint).  See the backscroll on that github issue for more.
A bit more rough backround here:
For an <img> with decode=sync, the expectation would be: when the <img> load event fires (say, for a dynamically-created <img> element), the author can immediately insert the <img> into the document and expect that we will be able to paint it on the next frame (which might mean we have to decode it synchronously during that paint, and that might cause a bit of jank or checkerboarding if it's slow to decode -- and that's OK, because that's what the author is asking for).

I forget exactly how our image-decoding currently works, but I don't think we make that guarantee right now.

On the other hand: decode="async" would be a hint that we do *not* need to decode as aggressively. That might be a no-op (matching our default behavior) right now --- I'm not sure.
FWIW, the Chrome team has posted an intent-to-implement (back when this attribute was called "async"):
 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9i6wgXv7c7c
...and then intent-to-ship:
 https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/MbXp16hQclY/bQjegyrbAgAJ

As noted there, the HTML spec change landed in https://github.com/whatwg/html/pull/3221 , and there's also an explainer doc at https://docs.google.com/document/d/1VvrEGAjcnto5egR_rnX7PR5ymqWayMNRPYSbCf1bC4I/edit
Summary: Implement <img decode="sync|async"> → Implement <img decoding="sync|async|auto">
Whiteboard: [gfx-noted]
I want this to fix bug 1421150, which depends on images loading right away. Once this and that bug are fixed, I can maybe land bug 1436247.
Assignee: nobody → aosmond
Adds the bits needed to nsImageLoadingContent to support this feature; turns out what we need is already present in nsImageFrame/nsSVGImageFrame.
This exposes the feature on the img element itself. This feature does not appear to be hidden behind https from the WebKit commit so I did the same.
This strips out the exceptions we put in the web-platform tests for not implementing the decoding attribute.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70bd8b5bb62b5d939698936fd14c77524fbfff17
Fix the WPT tests by making the decoding attribute a proper enum attribute (TIL).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=483e1e34cdc87b4db0626b23f0eb03236894b3b9
Attachment #8952476 - Attachment is obsolete: true
Attachment #8952475 - Attachment is obsolete: true
Attachment #8990413 - Flags: review?(tnikkel)
Attachment #8952711 - Attachment is obsolete: true
Attachment #8990416 - Flags: review?(tnikkel)
I guess I'll need a DOM peer part 2 (since I'm exposing WebIDL) to review after and send an intent to ship/implement, before landing.
Attachment #8952477 - Attachment is obsolete: true
Attachment #8990418 - Flags: review?(tnikkel)
I should also note that this is now shipping in Safari ;).
Comment on attachment 8990416 [details] [diff] [review]
0002-Bug-1416328-Part-2.-Expose-decoding-attribute-for-im.patch, v2

Looks fine to me but should get someone from DOM to review.
Attachment #8990416 - Flags: review?(tnikkel) → review+
Attachment #8990418 - Flags: review?(tnikkel) → review+
Comment on attachment 8990413 [details] [diff] [review]
0001-Bug-1416328-Part-1.-Add-configurable-synchronous-dec.patch, v2

Review of attachment 8990413 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsImageLoadingContent.h
@@ +459,5 @@
>  
> +  /**
> +   * Depending on the configured decoding hint, and/or how recently we updated
> +   * the image request, force or stop the frame from decoding the image
> +   * synchronously.

Can you add "when it is drawn" to this comment?

@@ +461,5 @@
> +   * Depending on the configured decoding hint, and/or how recently we updated
> +   * the image request, force or stop the frame from decoding the image
> +   * synchronously.
> +   * @param aPrepareNextRequest True if this is when updating the image request.
> +   * @param aFrame The frame to configure, uses primary frame if unset.

Can you explain that this will always be our primary frame but we don't have access to it via any other way when called from FrameCreated?
Attachment #8990413 - Flags: review?(tnikkel) → review+
Incorporate review feedback.
Attachment #8990413 - Attachment is obsolete: true
Attachment #8995277 - Flags: review?(tnikkel)
WebKit didn't seem to restrict the new attribute to secure contexts (https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/HTMLImageElement.idl) so neither did I.
Attachment #8990416 - Attachment is obsolete: true
Attachment #8995286 - Flags: review?(bzbarsky)
Attachment #8995277 - Flags: review?(tnikkel) → review+
Comment on attachment 8995286 [details] [diff] [review]
0002-Bug-1416328-Part-2.-Expose-decoding-attribute-for-im.patch, v3 [carries r=tnikkel]

> https://whatpr.org/html/3221/images.html#decoding-images

Why not link to the actual spec?

It looks to me like there is actually a spec bug here; the spec needs to say that "decoding" is an enumerated attribute, and doesn't do that.  Please file a spec issue.

r=me
Attachment #8995286 - Flags: review?(bzbarsky) → review+
Chrome seems to have done this for SVG image elements too, but we arent?
(In reply to Robert Longson [:longsonr] from comment #19)
> Chrome seems to have done this for SVG image elements too, but we arent?

Seems like we should too. Shouldn't be hard, just need the dom attribute bits on the svg image node type.
Flags: needinfo?(aosmond)
Hm correct you are, I confirmed it is implemented in the Blink engine on the SVG image element. It appears Safari did the same as us, but I can believe that is an oversight.
All I changed was 1) moved kDecodingTable(Default) to be a protected static member of nsImageLoadingContent, and 2) exposed the decoding attribute on SVGImageElement as well.
Attachment #8995286 - Attachment is obsolete: true
Flags: needinfo?(aosmond)
Attachment #8996701 - Flags: review?(bzbarsky)
Comment on attachment 8996701 [details] [diff] [review]
0002-Bug-1416328-Part-2.-Expose-decoding-attribute-for-im.patch, v4

> +  enum ImageDecodingType : uint8_t {

I assume this is not an enum class so we can use the values in EnumTable without casting?  Might be worth documenting.

On the other hand, if we _can_ use an enum class here might be nice to.

>+  virtual bool ParseAttribute(int32_t aNamespaceID,

Take out the "virtual" please, per code style (since this method has "override" already).

r=me
Attachment #8996701 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #23)
> Comment on attachment 8996701 [details] [diff] [review]
> 0002-Bug-1416328-Part-2.-Expose-decoding-attribute-for-im.patch, v4
> 
> > +  enum ImageDecodingType : uint8_t {
> 
> I assume this is not an enum class so we can use the values in EnumTable
> without casting?  Might be worth documenting.
> 
> On the other hand, if we _can_ use an enum class here might be nice to.
> 

I switched to the enum class. I would love to say enum vs enum class was well considered, just didn't try. Only needed the static cast for the comparison (although I've yet to use try.)

> >+  virtual bool ParseAttribute(int32_t aNamespaceID,
> 
> Take out the "virtual" please, per code style (since this method has
> "override" already).
> 
> r=me

Done.
Attachment #8996701 - Attachment is obsolete: true
Attachment #8996984 - Flags: review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f426c672aab
Part 1. Add configurable synchronous decoding hint to nsImageLoadingContent. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/630049a9ac3b
Part 2. Expose decoding attribute for img elements. r=bz,tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7ac43b0e13
Part 3. Enable img decoding attribute web-platform-tests. r=tnikkel
I've documented this new attribute; see

* https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img
* https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement
* https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/decoding

I've also submitted a PR to the browser-compat-data repo to update the compat data:

* https://github.com/mdn/browser-compat-data/pull/2723

And added a note to the Fx 63 rel notes:

* https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63#HTML

Can you please review the docs for me? Thanks!
Flags: needinfo?(aosmond)
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #27)
> I've documented this new attribute; see
> 
> * https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img
> * https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement
> * https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/decoding
> 
> I've also submitted a PR to the browser-compat-data repo to update the
> compat data:
> 
> * https://github.com/mdn/browser-compat-data/pull/2723
> 
> And added a note to the Fx 63 rel notes:
> 
> * https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63#HTML
> 
> Can you please review the docs for me? Thanks!

I reviewed the above docs, looks good to me!
Flags: needinfo?(aosmond)
Andrew, https://twitter.com/jaffathecake/status/925702096300707842 suggested that fixing this would remove the flash on Google Images when images are swapped. However, testing this with the latest Nightly, this flash still appears to be there. Is Firefox' implementation faulty, or is there something else going on?
Flags: needinfo?(miket)
Flags: needinfo?(aosmond)
I don't see this feature (the decoding="" img attribute) being used at all on Google Images.

 - I poked around in devtools and didn't see that attribute being present
 - I added a printf at the line where we parse the value, https://hg.mozilla.org/mozilla-central/rev/630049a9ac3b#l3.34 , and that printf didn't get triggered at all during my use of Google images (searching for "cats" and clicking on a few image results).
 - (I manually added decoding="async" to an image and made sure that triggered my printf.)

I guess Google Images' usage of this feature is still pending (?)
Flags: needinfo?(aosmond)
Also I think that twitter thread is mostly about Google Photos being a use-case for the "img.decode()" API, which we don't implement (yet).

That's a related but different feature from what was implemented here. It seems to have been standardized at https://github.com/whatwg/html/issues/2037 & https://github.com/whatwg/html/pull/2332 , and its spec text lives at https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-decode

I'll spin off another bug for that.
See Also: → 1501794
(I filed 1501794 on the img.decode API which I think is the answer to comment 29.)
Flags: needinfo?(miket)
Daniel, you're right! The Twitter example refers indeed to the img.decode() API, which is not what this bug is about (although it is related). Thanks.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: