686905 - Discard animated images to save memory

Status: RESOLVED FIXED

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

Description

[Scott Johnson (:jwir3)]

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.

Comment 2

[Nicholas Nethercote [inactive]]

Did bug 689623's landing have any effect on this bug?

Comment 4

[Joe Drew (not getting mail)]

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.)

Comment 5

[Joe Drew (not getting mail)]

This bug is strictly about discarding animated images at all. If we can at some point also discard individual frames, bully for us!

Comment 6

[Max Stepin]

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.

Comment 7

[Seth Fowler [:seth] [:s2h]]

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

[Ram]

Is there an update on this fix. This bug is causing our product to hang/crash.

Comment 10

[Seth Fowler [:seth] [:s2h]]

See bug 1150302 for more discussion about how important this is.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Scott, let us know if you're planning on taking this on.

Comment 12

[Scott Johnson (:jwir3)]

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.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks Scott - I'll chase somebody down; just didn't want to step on your toes if you're in the middle of something.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

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)

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 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.

Comment 16

[Timothy Nikkel (:tnikkel)]

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).

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 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?

Comment 18

[Timothy Nikkel (:tnikkel)]

(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.

Comment 19

[Andrew Osmond [:aosmond] (he/him)]

Not sure if you have another bug tracking your patch -- use this or close as a dupe, as you see fit.

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: Patch to enable the pref

I'll land this once the patches from bug 1343341 land and look stable.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6360c2ba584
Backed out changeset 6bd536e0571d

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80bea9982b5f
Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond

Comment 25

[Timothy Nikkel (:tnikkel)]

Backed out again
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed424d45b500761db8f7831c4d7da957cce3a311

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/445859a4438c
Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond

Comment 27

[Carsten Book [:Tomcat]]

backed this out for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=89503066&repo=mozilla-inbound

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e11dac8513ec
Backed out changeset 445859a4438c for frequent failures in test_discardAnimatedImage.html

Comment 29

[Timothy Nikkel (:tnikkel)]

(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.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50aee6c86901
Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond

Comment 31

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/550f156a3c5f since the test fixes didn't actually work.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/befc9b7f976d
Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/befc9b7f976d

Comment 34

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 35

[Bobby Holley (:bholley)]

Nice! Really awesome to see this finally happen, 8 years after bug 500402 was fined. :-)

Comment 36

[Milan Sreckovic [:milan] (needinfo for best results)]

Tumblr and Pinterest :)