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.
Did bug 689623's landing have any effect on this bug?
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!
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.
Scott, let us know if you're planning on taking this on.
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.
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)
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.
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?
(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.
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd536e0571d Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond
Backout by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6360c2ba584 Backed out changeset 6bd536e0571d
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/80bea9982b5f Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond
Pushed by email@example.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
Backout by firstname.lastname@example.org: 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.
Pushed by email@example.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 firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/befc9b7f976d Enable the pref image.mem.animated.discardable to allow discarding of animated images. r=aosmond
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 :)