Discard animated images to save memory

RESOLVED FIXED in Firefox 55

Status

()

Core
ImageLib
P3
normal
RESOLVED FIXED
6 years ago
13 days ago

People

(Reporter: jwir3, Assigned: tnikkel)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Assignee: nobody → sjohnson
(Reporter)

Updated

6 years ago
Duplicate of this bug: 500402
(Reporter)

Updated

5 years ago
Priority: -- → P3
Blocks: 683284
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
Did bug 689623's landing have any effect on this bug?
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 699150
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 → ---

Comment 6

4 years ago
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.

Comment 8

2 years ago
Is there an update on this fix. This bug is causing our product to hang/crash.
Duplicate of this bug: 1150302
See bug 1150302 for more discussion about how important this is.
(Assignee)

Updated

11 months ago
Blocks: 1130968

Updated

11 months ago
Blocks: 1284651
Scott, let us know if you're planning on taking this on.

Updated

11 months ago
Flags: needinfo?(jaywir3)
(Reporter)

Comment 12

11 months ago
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
Created attachment 8802669 [details] [diff] [review]
Remove animated images from cache, v1

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.
Depends on: 414259
(Assignee)

Comment 16

7 months ago
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).
Created attachment 8802893 [details] [diff] [review]
Remove animated images from cache, v2

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
(Assignee)

Comment 18

7 months ago
(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
Blocks: 1222482
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1134576
(Assignee)

Updated

3 months ago
Depends on: 1343341
(Assignee)

Comment 21

3 months ago
Created attachment 8842614 [details] [diff] [review]
Patch to enable the pref

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+

Comment 22

2 months ago
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

Comment 23

2 months ago
Backout by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6360c2ba584
Backed out changeset 6bd536e0571d

Comment 24

2 months ago
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
(Assignee)

Comment 25

2 months ago
Backed out again
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed424d45b500761db8f7831c4d7da957cce3a311
(Assignee)

Updated

2 months ago
Depends on: 1352282

Comment 26

2 months ago
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)

Comment 28

2 months ago
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
(Assignee)

Comment 29

2 months ago
(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)

Comment 30

2 months ago
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.

Comment 32

2 months ago
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
Last Resolved: 4 years ago2 months ago
status-firefox55: --- → fixed
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. :-)
Tumblr and Pinterest :)
See Also: → bug 1130968, bug 1222482
Depends on: 1357738
Depends on: 1358057
No longer depends on: 1358057
Blocks: 1359939
Depends on: 1360572
Depends on: 1364365
Depends on: 1364864
(Assignee)

Updated

13 days ago
No longer depends on: 1364864
You need to log in before you can comment on or make changes to this bug.