Closed Bug 686905 Opened 13 years ago Closed 7 years ago

Discard animated images to save memory

Categories

(Core :: Graphics: ImageLib, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwir3, Assigned: tnikkel)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 2 obsolete files)

There are times when we can discard animated image frames. We should investigate when we can do this safely, and implement the change to make this happen.
Assignee: nobody → sjohnson
Priority: -- → P3
Blocks: image-suck
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
Did bug 689623's landing have any effect on this bug?
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
No - we never discard animated images. (However, we don't actually know whether an image is animated or not until we've decoded it, so if an animated image never gets near the viewport, it'll never be decoded.)
This bug is strictly about discarding animated images at all. If we can at some point also discard individual frames, bully for us!
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Just for the reference, WebKit uses hardcoded size trigger: if the total size of uncompressed frames exceed 5 megabytes, it would throw away all the frames, except for the current one. 

I'm not sure it's a good approach, threshold is kinda small, and decoding animation again for every loop adds more work for the CPU.

It might be better to consider simply discarding the whole animation after it hasn't been visible for a while.
I seem to recall that at one point there was some kind of issue preventing us from doing this. At this point, though, I think we're good to go, especially after bug 1116716 lands.

I plan to cook up a patch to turn this on as soon as I can find time to do so.
Is there an update on this fix. This bug is causing our product to hang/crash.
See bug 1150302 for more discussion about how important this is.
Blocks: 1130968
Scott, let us know if you're planning on taking this on.
Flags: needinfo?(jaywir3)
Milan, 

I can take this on, but I've been out of the moz development cycle for a bit... it might be faster to have someone else work on this. It's not that I'm not willing to, but it will take a bit for me to get back up to speed.
Flags: needinfo?(jaywir3)
Thanks Scott - I'll chase somebody down; just didn't want to step on your toes if you're in the middle of something.
We have learned to depend on animated images not getting discarded.  Among other things: MOZ_RELEASE_ASSERT(aPlaybackType != PlaybackType::eAnimated || !mAnimationState || mAnimationState->KnownFrameCount() < 1) (Animated frames should be locked)
Assignee: jaywir3 → nobody
Assignee: nobody → aosmond
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7475296085be6ba1afcce5ccf07a79be3b55dae3

It now lets animated images be removed from the cache naturally, and properly requests a redecode when it finds it has been discarded but is needed (special case required because otherwise we would constantly redisplay the composited image if available). Let's see how well this works.
I actually have a patch for this mostly working, I just need to solve the problem of switching back to a tab and having a gif jump from the last drawn frame to where it should be (it's quite annoying).
Well, update the patch to fix the broken gtest just in case.

(In reply to Timothy Nikkel (:tnikkel) from comment #16)
> I actually have a patch for this mostly working, I just need to solve the
> problem of switching back to a tab and having a gif jump from the last drawn
> frame to where it should be (it's quite annoying).

You mean you don't want it to jump to where it should be? Isn't that how it works today even if you keep the decoded data around?
Attachment #8802669 - Attachment is obsolete: true
(In reply to Andrew Osmond [:aosmond] from comment #17)
> You mean you don't want it to jump to where it should be? Isn't that how it
> works today even if you keep the decoded data around?

It first shows the last composited frame, then decodes as quickly as it can to get to where it should be (possibly displaying intermediate frames to get to the final point). The problem is that the jump from the last composited frame to some other random frame is jarring. We want to only display the "correct" frame and no "incorrect" frames.
Not sure if you have another bug tracking your patch -- use this or close as a dupe, as you see fit.
Assignee: aosmond → tnikkel
Attachment #8802893 - Attachment is obsolete: true
Depends on: 1343341
I'll land this once the patches from bug 1343341 land and look stable.
Attachment #8842614 - Flags: review?(aosmond)
Attachment #8842614 - Flags: review?(aosmond) → review+
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd536e0571d
Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80bea9982b5f
Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond
Depends on: 1352282
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/445859a4438c
Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond
backed this out for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=89503066&repo=mozilla-inbound
Flags: needinfo?(tnikkel)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e11dac8513ec
Backed out changeset 445859a4438c for frequent failures in test_discardAnimatedImage.html
(In reply to Carsten Book [:Tomcat] from comment #27)
> backed this out for frequent failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=89503066&repo=mozilla-
> inbound

This is bug 1354499. I will push a simple fix for that when I push this bug again.
Flags: needinfo?(tnikkel)
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50aee6c86901
Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/550f156a3c5f since the test fixes didn't actually work.
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/befc9b7f976d
Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond
https://hg.mozilla.org/mozilla-central/rev/befc9b7f976d
Status: REOPENED → RESOLVED
Closed: 11 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
good news, AWSY found an improvement after this landed:
== Change summary for alert #5925 (as of April 07 2017 21:11 UTC) ==

Improvements:

  6%  Images summary linux64 opt      6994748.84 -> 6584392.43
  5%  Images summary windows10-64-vm opt 7768357.77 -> 7348699.26

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5925
Nice! Really awesome to see this finally happen, 8 years after bug 500402 was fined. :-)
Depends on: 1357738
Depends on: 1358057
No longer depends on: 1358057
Blocks: 1359939
Depends on: 1360572
Depends on: 1364365
Depends on: 1364864
No longer depends on: 1364864
Depends on: 1368440
Depends on: 1372532
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: