Closed Bug 1341881 Opened 7 years ago Closed 7 years ago

Slow synchronous image decoding in Google Slides

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: tnikkel)

References

(Regressed 2 open bugs)

Details

Attachments

(1 file)

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.
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.
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.
Attached patch patchSplinter Review
Assignee: nobody → tnikkel
Attachment #8840171 - Flags: review?(aosmond)
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
(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.  :/
https://hg.mozilla.org/mozilla-central/rev/c61b11debf42
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Is this safe to backport to branches?
Flags: needinfo?(tnikkel)
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 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+
Setting qe-verify- based on Timothy's assessment on manual testing needs (see Comment 8).
Flags: qe-verify-
Depends on: 1366875
Regressions: 1735114
Blocks: 514672
Regressions: 1816504
You need to log in before you can comment on or make changes to this bug.