Closed
Bug 1341881
Opened 8 years ago
Closed 8 years ago
Slow synchronous image decoding in Google Slides
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: ehsan.akhgari, Assigned: tnikkel)
References
(Regressed 2 open bugs)
Details
Attachments
(1 file)
(deleted),
patch
|
aosmond
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
See this profile: https://perfht.ml/2l0UzPV
Here is what I did: I loaded <https://docs.google.com/presentation/d/1gqQpeG1vHky4nZ45mIYSC5DAEFX-5HipjemLwhBDe1c/edit?pli=1> in a tab (needs LDAP login) and waited for it to be fully loaded, and then after a while I clicked on the thumbnail for slide 5.
We spend 578ms decoding images here.
Reporter | ||
Comment 1•8 years ago
|
||
FWIW I see this issue all over the place when I click around this slide deck. Here is another one: <https://perfht.ml/2kNFeqt> It's basically adding a visible lag to most clicks on the slide thumbnails.
Assignee | ||
Comment 2•8 years ago
|
||
This is because of some code that hasn't been changed in a very long time always asking for a sync decode of SVG <image> elements:
https://dxr.mozilla.org/mozilla-central/rev/b06968288cff469814bf830aa90f1c84da490f61/layout/svg/nsSVGImageFrame.cpp#392
I don't think we need that anymore. SVG <image>s should be much more modernized in how they track decoding now.
(In reply to :Ehsan Akhgari from comment #0)
> We spend 578ms decoding images here.
Is that a debug build? I'm seeing numbers about 5x as small.
Assignee | ||
Comment 3•8 years ago
|
||
Assignee: nobody → tnikkel
Attachment #8840171 -
Flags: review?(aosmond)
Updated•8 years ago
|
Attachment #8840171 -
Flags: review?(aosmond) → review+
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c61b11debf42
Don't do sync decode for SVG <image> elements images. r=aosmond
Reporter | ||
Comment 5•8 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #2)
> This is because of some code that hasn't been changed in a very long time
> always asking for a sync decode of SVG <image> elements:
>
> https://dxr.mozilla.org/mozilla-central/rev/
> b06968288cff469814bf830aa90f1c84da490f61/layout/svg/nsSVGImageFrame.cpp#392
>
> I don't think we need that anymore. SVG <image>s should be much more
> modernized in how they track decoding now.
Thanks a lot for the quick fix!
> (In reply to :Ehsan Akhgari from comment #0)
> > We spend 578ms decoding images here.
>
> Is that a debug build? I'm seeing numbers about 5x as small.
No, it's a normal Nightly. I'm pretty sure my machine wasn't under any load at the time. Not sure why we're seeing this huge of a difference. :/
Comment 6•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8840171 [details] [diff] [review]
patch
Approval Request Comment
[Feature/Bug causing the regression]: probably bug 435296 from 2009 (ie relatively ancient)
[User impact if declined]: spend too much time sync decoding images in svg <image>s
[Is this code covered by automated tests?]: the code is yes, but not this specific issue because it's a perf issue
[Has the fix been verified in Nightly?]: I've verified the patch fixes the issue myself
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really
[Why is the change risky/not risky?]: the main risk would be in images not being decoded when we paint. the risk should be minimal as we use the same visibility tracking as html <img> elements for svg <image>s, and html <img> elements don't use "sync decode", they don't even use "sync decode if fast", they don't ask for any decoding when drawing. So the risk is even less than the existing risk in html <img>s.
[String changes made/needed]: none
Flags: needinfo?(tnikkel)
Attachment #8840171 -
Flags: approval-mozilla-aurora?
Comment 9•8 years ago
|
||
Comment on attachment 8840171 [details] [diff] [review]
patch
Fix the slow sync issue in google slides and was verified. Aurora53+.
Attachment #8840171 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
status-firefox53:
--- → affected
Comment 10•8 years ago
|
||
bugherder uplift |
Comment 11•8 years ago
|
||
Setting qe-verify- based on Timothy's assessment on manual testing needs (see Comment 8).
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•