Closed Bug 969406 Opened 10 years ago Closed 10 years ago

Hidden (positioned offscreen) gif activates the refresh driver

Categories

(Core :: Layout, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: rvitillo, Assigned: seth)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, power)

Attachments

(2 files)

Firefox seems to be consuming more CPU resources while idling on Huffingtonpost compared to Chrome (flash disabled).
Attached file Testcase 1
FF renders the gif in the testcase even though it’s hidden (~60 wakeups/s) while Chrome does not. This bug has some similarities with Bug 968123 but they are not quite the same so I didn’t flag it as a duplicate.
Component: General → Graphics
Product: Firefox → Core
Summary: High power consumption on Huffingtonpost while idling → Hidden gif activates the refresh driver
Layout is response for visibility testing and scheduling paints.
Component: Graphics → Layout
Summary: Hidden gif activates the refresh driver → Hidden (positioned offscreen) gif activates the refresh driver
This looks like a regression from bug 957391.

Previously we only scheduled a refresh driver tick if we found retained data from painting the image previously, now we do it unconditionally.
Depends on: 957391
The problem is that we don't differentiate between decoding completing and advancing to the next frame of an animation, see [1] and [2].

The former requires us to schedule a refresh driver tick (since we wouldn't have painted an un-decoded image before), but the latter we could skip if it wasn't painted previously.

I'm not sure how to fix this since we go through the imgINotificationObserver interface which is exposed to javascript and I'm not sure what would break.

Seth, any ideas for this?

[1] http://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp?rev=2201890b1c3d#572
[2] http://mxr.mozilla.org/mozilla-central/source/image/src/Decoder.cpp?rev=b83af60025b8#264
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> The problem is that we don't differentiate between decoding completing and
> advancing to the next frame of an animation, see [1] and [2].

We are *supposed* to make this distinction (we're not supposed to send FRAME_CHANGED events for frames after the first except as a result of RequestRefresh), but we fail to do so. =\

I noticed this recently and added explicit code to enforce this in bug 1084679.
Flags: needinfo?(seth)
Depends on: 1084679
I changed my mind about the dependency. I think we can trivially solve this without that dependency - we just need to force a paint on FRAME_COMPLETE but not on FRAME_CHANGED. The solution will get even nicer if I change the FRAME_COMPLETE notification to FIRST_FRAME_COMPLETE, which I am considering doing. I don't think we need that right away, though.
No longer depends on: 1084679
Alright, this patch resolves the problem for me locally.
Attachment #8518626 - Flags: review?(matt.woodrow)
Assignee: nobody → seth
Status: NEW → ASSIGNED
Does nsImageFrame have the same problem?
(In reply to Timothy Nikkel (:tn) from comment #11)
> Does nsImageFrame have the same problem?

Nah, because nsImageFrame is managed by nsImageLoadingContent, which properly manages visibility. ImageLoader doesn't do that; it just uses "did this frame get assigned to a layer last time" as a proxy for visibility, if I understand the code correctly.

It would be much better if ImageLoader *did* manage visibility like nsImageLoadingContent. I don't think we can guarantee correct behavior in all cases without doing that. I'll file a followup.
Blocks: 1095291
The followup is bug 1095291.
Attachment #8518626 - Flags: review?(matt.woodrow) → review+
(In reply to Seth Fowler [:seth] from comment #12)
> (In reply to Timothy Nikkel (:tn) from comment #11)
> > Does nsImageFrame have the same problem?
> 
> Nah, because nsImageFrame is managed by nsImageLoadingContent, which
> properly manages visibility. ImageLoader doesn't do that; it just uses "did
> this frame get assigned to a layer last time" as a proxy for visibility, if
> I understand the code correctly.
>
> It would be much better if ImageLoader *did* manage visibility like
> nsImageLoadingContent. I don't think we can guarantee correct behavior in
> all cases without doing that. I'll file a followup.

The visibility tracking that nsImageLoadingContent does for decoded/not-decoded purposes is approximate visibility. It tracks images we want to keep decoded, which are images that are currently visible, or likely to be scrolled into view soon.

If the frame for an image did _not_ have retained data for an associated layer from the last paint (so it wasn't visible at last paint) that means that either 1) the image will not be visible next paint OR 2) something else changed to make the image visible on the next paint, the code to handle that change needs to invalidate that area of the document (otherwise we'd have a more general bug that didn't involve just images). In either case we don't need to invalidate for a frame changed notification. If the frame for an image did have retained data for an associated layer from the last paint then that means that either 1) the image will be visible on the next paint OR 2) the image won't be visible on the next paint and whatever change caused that invalidated the area of the image. In case 1) we need to invalidate for frame changed notifications, in case 2) that part of the document will be invalidated no matter what we do in handling the frame changed notification. So in all cases we should get ideal behaviour.

However if we use the approximate visibility information we use to keep around decoded images we will end up invalidating for images that are just off screen, when we don't need to.

So I think we should invalidate in the same way that we are doing here also for nsImageFrame.
So we discussed this on IRC. It turns out that my mistake had been believing that the nsImageFrame case currently works (i.e., does not cause unnecessary repaints when an animated GIF is offscreen), which turns out to be false. =)

It seems to me that the simplest path forward here is to always store retained data for frames which contain images, even if those images aren't decoded yet. Then the retained data test becomes a 100% reliable test of visibility, which is really what we need here. Both ImageLoader and nsImageFrame should then be converted to only paint if the retained data test passes.

There's also the issue of not activating the refresh driver at all. I haven't worked out the details yet, but we should use the same retained data test for that too if possible. It may need to work a little differently since in that case the frame needs to be aware when the state changes.

I'll file followup bugs about this.
Blocks: 1095783
I filed bug 1095783, as well as a followup to that bug, about the retained data visibility test followup.
Looking at ImageLoader it's actually completely broken. ImageLoader::OnStopFrame is called when we get the LOAD_COMPLETE notification. Load complete means we have all the data from the network, we could get decoded image frame data either before or after it. We should be using decode complete or something depending on the behaviour we want here.
https://hg.mozilla.org/mozilla-central/rev/67c6993d405c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1098652
Comment 18 is getting addressed in bug 1098652.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: