Closed
Bug 1416328
Opened 7 years ago
Closed 6 years ago
Implement <img decoding="sync|async|auto">
Categories
(Core :: Graphics: ImageLib, enhancement, P3)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla63
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.
Reporter | ||
Comment 1•7 years ago
|
||
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.
Updated•7 years ago
|
Keywords: dev-doc-needed
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Priority: -- → P3
Reporter | ||
Comment 3•6 years ago
|
||
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">
Reporter | ||
Updated•6 years ago
|
See Also: → https://bugs.webkit.org/show_bug.cgi?id=179432
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
See Also: → https://github.com/whatwg/html/pull/3221
Updated•6 years ago
|
Whiteboard: [gfx-noted]
Assignee | ||
Comment 4•6 years ago
|
||
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
Assignee | ||
Comment 5•6 years ago
|
||
This got merged in as https://github.com/whatwg/html/pull/3221. https://html.spec.whatwg.org/#image-decoding-hint
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 6•6 years ago
|
||
Adds the bits needed to nsImageLoadingContent to support this feature; turns out what we need is already present in nsImageFrame/nsSVGImageFrame.
Assignee | ||
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
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
Assignee | ||
Comment 9•6 years ago
|
||
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
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #8952475 -
Attachment is obsolete: true
Attachment #8990413 -
Flags: review?(tnikkel)
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #8952711 -
Attachment is obsolete: true
Attachment #8990416 -
Flags: review?(tnikkel)
Assignee | ||
Comment 12•6 years ago
|
||
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)
Assignee | ||
Comment 13•6 years ago
|
||
I should also note that this is now shipping in Safari ;).
Comment 14•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8990418 -
Flags: review?(tnikkel) → review+
Comment 15•6 years ago
|
||
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+
Assignee | ||
Comment 16•6 years ago
|
||
Incorporate review feedback.
Attachment #8990413 -
Attachment is obsolete: true
Attachment #8995277 -
Flags: review?(tnikkel)
Assignee | ||
Comment 17•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8995277 -
Flags: review?(tnikkel) → review+
Comment 18•6 years ago
|
||
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+
Comment 19•6 years ago
|
||
Chrome seems to have done this for SVG image elements too, but we arent?
Updated•6 years ago
|
status-firefox63:
--- → affected
tracking-firefox63:
--- → +
Comment 20•6 years ago
|
||
(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)
Assignee | ||
Comment 21•6 years ago
|
||
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.
Assignee | ||
Comment 22•6 years ago
|
||
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 23•6 years ago
|
||
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+
Assignee | ||
Comment 24•6 years ago
|
||
(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+
Comment 25•6 years ago
|
||
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
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1f426c672aab https://hg.mozilla.org/mozilla-central/rev/630049a9ac3b https://hg.mozilla.org/mozilla-central/rev/bc7ac43b0e13
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
Comment 27•6 years ago
|
||
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)
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 28•6 years ago
|
||
(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)
Comment 29•6 years ago
|
||
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)
Reporter | ||
Comment 30•6 years ago
|
||
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)
Reporter | ||
Comment 31•6 years ago
|
||
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.
Reporter | ||
Comment 32•6 years ago
|
||
(I filed 1501794 on the img.decode API which I think is the answer to comment 29.)
Flags: needinfo?(miket)
Comment 33•6 years ago
|
||
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.
Description
•