Closed Bug 1454824 Opened 2 years ago Closed 2 years ago
Disable discarding of animated image frames
There are a number of bugs open (bug 1444537 and bug 1444537) due to the landing of bug 523950. These problems are not insurmountable, but we are getting too close to release to want to land anything risky. Hence I propose we increase the image.animated.decode-on-demand.threshold-kb threshold from 20MB to ~4GB to avoid the algorithm trigger causing the discarding of frames. Since most animated images have a smaller footprint than 20MB, the path taken by the larger images is well tested. It will be very similar behaviour as to that in prior releases as well.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
I selected 4194303 (or (2^32 / 1024) - 1) because 4194303 * 1024 won't overflow a 32-bit size_t in the calculations in AnimationSurfaceProvider::AnimationSurfaceProvider. https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/image/AnimationSurfaceProvider.cpp#44 try (64-bit): https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7f3d8e092e72a5848169fd9c056ff98e8ca5f07 try (32-bit): https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f8f16b0bd816798590e7370ee2b83a554bd8756
Attachment #8968709 - Flags: review?(tnikkel)
Attachment #8968709 - Flags: review?(tnikkel) → review+
Comment on attachment 8968709 [details] [diff] [review] bug1454824.patch, v1 Approval Request Comment [Feature/Bug causing the regression]: Bug 523950 [User impact if declined]: High CPU use as described in bug 1444537 and bug 1454149. May experience shutdown hangs as well. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: No, the pref change is only for beta. [Needs manual test from QE? If yes, steps to reproduce]: Go to the github.com page from bug 1454149 and verify the CPU usage is similar or better as compared to 59. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: It is a simple pref change increasing the threshold where we decide to start discarding animated image frames to something very high such that it will never trigger. The code path where we don't discard frames is very well tested as that is most animated images already. The behaviour will be a reversion to what was shipped in 59 and in previous releases. [String changes made/needed]: None.
Attachment #8968709 - Flags: approval-mozilla-beta?
Comment on attachment 8968709 [details] [diff] [review] bug1454824.patch, v1 pref change to avoid triggering a regression with animated gifs, approved for 60.0b14
Attachment #8968709 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
I have managed to reproduce this issue on Firefox 61.0a1 (BuildId:20180417225505) while using the following webpage: -https://blog.github.com/2016-08-19-building-your-first-atom-plugin/ from Bug 1454149 (100% CPU usage). This issue is verified fixed as the CPU usage is much lighter on Firefox 60.0b16 (BuildId:20180426170554) using Windows 10 64bit, macOS 10.13 and Ubuntu 16.04 32bit.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a22564076cae Disable discarding of animated image frames due to high CPU consumption. r=tnikkel
I decided to land this on 61 as well, since I think we aren't going to make the window there either...
Summary: Disable discarding of animated image frames for 60 release → Disable discarding of animated image frames
Let's get a verification run on 61 as well.
This issue is verified fixed on Firefox 61.0b3 (BuildId:20180507191226), Firefox 62.0a1 (BuildId:20180507222648) and Firefox 60.0esr (BuildId:20180503164101) using Windows 10 64bit, macOS 10.13.3 and Ubuntu 16.04 64bit.
You need to log in before you can comment on or make changes to this bug.