Open Bug 560067 Opened 14 years ago Updated 2 months ago

animated images inside visibility:hidden still eat CPU

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
ASSIGNED
Project Flags:
Performance Impact medium

People

(Reporter: Dolske, Assigned: emilio)

References

(Blocks 1 open bug,
URL
)

Details

(4 keywords)

Attachments

(4 files)

Bug 560067 - Only tick the refresh driver if the animated images are locked (i.e., tracked by the document). r=tnikkel
48 bytes, text/x-phabricator-request
Details | Review
Bug 560067 - Remove image.mem.allow_locking_in_content_processes. r=tnikkel
48 bytes, text/x-phabricator-request
Details | Review
Bug 560067 - Minor clean-up in the refresh driver image animation code. r=tnikkel
48 bytes, text/x-phabricator-request
Details | Review
Bug 560067 - Ensure visibility tracking code doesn't choke on visibility:hidden images. r=tnikkel
48 bytes, text/x-phabricator-request
Details | Review 
From previous work with the video controls, I ran into problems with the APNG throbber unexpectedly eating CPU. When it's hidden with display:none things are ok, but when it's hidden with visibility:hidden CPU usage does not go down.

See previous history in bug 483841 (partial fix to problem), bug 484935 (switch video controls to display:none), and bug 521890 (use CSS transitions).
I'd love a testcase here!  Justin, just a pointer to the apng throbber in question would be helpful.

I just ran into this in about:preferences, and I'm adding a workaround with display: none in bug 1723188. I wonder how much power this wastes on the web.

The profiler markers I'm adding in bug 1723181 makes this easy to notice in profiles.

Emilio, any idea of who might be interested in working on this? Or if it's easy, where one would start to figure this out and fix it?

Blocks: power-usage
Flags: needinfo?(emilio)
Keywords: perf, power

I'm confused, this is supposed to work, see bug 1237454. As I was writing this, I noticed bug 1456389 is open, but per the patch in the bug above, it doesn't seem this is a transform / opacity animation (we should really fix that if it's still a problem though).

It seems the animation you're hitting is an svg animation inside an image. Are the animations from <xul:image> or <html:img>? XUL images don't do proper visibility tracking: https://searchfox.org/mozilla-central/rev/525ff01397317b4b24b1a2cc51fe4511ec49ac8b/layout/xul/nsImageBoxFrame.cpp#283-287

So depending on the root cause of your bug the fix might be different, and this might or might not be a XUL-only problem (in which case switching to <html:img> would be appreciated :-))

Flags: needinfo?(emilio) → needinfo?(florian)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Sorry for the delay in replying here.

It seems the animation you're hitting is an svg animation inside an image.

I might be confused, but I don't see any SVG involved here, the image is an animated PNG: chrome://global/skin/icons/loading.png

Are the animations from <xul:image> or <html:img>?

Both, there are 3 <xul:image> with the CSS setting list-style-image: url("chrome://global/skin/icons/loading.png");, and one <html:img class="update-throbber" src="chrome://global/skin/icons/loading.png".

I tried removing the 3 xul images to get the XUL case out of the way, and the result was that I still had one animation going on continuously.

The <html:img> tag is in a <hbox> with visibility: hidden; set by CSS.

Flags: needinfo?(florian)

Ok, I'll try to reproduce and take a look, thanks.

Flags: needinfo?(emilio)

So fwiw in a simple test-case like this I don't see the refresh driver ticking:

<!doctype html>
<div style="visibility: hidden">
  <img src="chrome://global/skin/icons/loading.png">
</div>

We don't have visibility tracking for CSS images and such, which is
rather unfortunate, but this gets animated <img>s inside a visibility:
hidden subtree to not waste CPU time.

Do you know if we have any existing tests for this visibility-tracking
code?

Assignee: nobody → emilio
Status: NEW → ASSIGNED

visibility: hidden still goes into BuildDisplayListForChild (because
children might be visible), but the frame itself is not visible.

Depends on D122116

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

So fwiw in a simple test-case like this I don't see the refresh driver ticking:

Since this comment you found a way to reproduce?

Yes, it actually ticks the refresh driver all the time, it's just that it does virtually nothing, but the timer keeps firing.

Review ping?

Flags: needinfo?(tnikkel)
Whiteboard: [qf]

Sorry for the delay, I need to take some time to page this code back into my head.

Whiteboard: [qf] → [qf:p2:resource]
Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3762549a7c11
Remove image.mem.allow_locking_in_content_processes. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/0f57aaf9c078
Minor clean-up in the refresh driver image animation code. r=tnikkel

I think the browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js test keeps vsync enabled at the end of its run because of this bug. It shows the bookmarks toolbar, where it has bookmarked about:robots, which has an animated favicon. Here a profile of that test: https://share.firefox.dev/31Umi04

Flags: needinfo?(emilio)
Performance Impact: --- → P2
Keywords: perf:resource-use
Whiteboard: [qf:p2:resource]
See Also: → 1766284

(In reply to Florian Quèze [:florian] from comment #17)

I think the browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js test keeps vsync enabled at the end of its run because of this bug. It shows the bookmarks toolbar, where it has bookmarked about:robots, which has an animated favicon. Here a profile of that test: https://share.firefox.dev/31Umi04

I filed bug 1766284 on the browser_click_bookmarks_on_toolbar.js issue, as the patches in this bug don't fix it.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aadc872f6553
Ensure visibility tracking code doesn't choke on visibility:hidden images. r=tnikkel
Regressions: 1781096
No longer regressions: 1781096
Severity: normal → S3

The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Flags: needinfo?(tnikkel)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: