1161859 - Compute the size of animated image frames correctly in the SurfaceCache

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Wowzers. We have been computing the size of animated image frames *completely wrong* all this time, causing us to think we were on the verge of OOM'ing when we were really nowhere close. I noticed this thanks to the about:memory changes in bug 1157954. (Though I think we could have noticed it before, it's just that after that patch I started using about:memory differently.)

Here's what went wrong:

- ComputeCost() in SurfaceCache.cpp assumes that surfaces are RBGA/RBGX format and have 4 bytes per pixel. However, animated image frames are often paletted and thus have only 1 byte per pixel. That's a factor of 4, right there.

- Even worse, ComputeCost() used the width and height from the SurfaceKey to do its computation, but the SurfaceKey uses the size of the overall image, _not_ the size of the individual frame. For single-frame images these are the same, but animated images often only have image data for a small subregion of the image on each frame. This caused a tremendous overestimate of memory usage, because if an animated image was 1000x1000, we'd treat every frame as 1000x1000, even if some frames only had a 1x1 surface.

In one test case, we treated an animated image as if it occupied over 1.5GB when decoded. After these changes, we only viewed it as occupying _50MB_.

Comment 1

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

Attachment: Compute the size of animated image frames correctly in the SurfaceCache

Here's the patch. Basically we just need to always use the imgFrame's actual
size (imgFrame::GetSize(), which is determined by the size of aFrameRect in
Decoder::InternalAddFrame) and take bytes-per-pixel into account when calling
SurfaceCache::ComputeCost.

External to SurfaceCache, this change only affects callers of
SurfaceCache::CanHold(), and since nobody but Decoder ever allocates frames with
sizes different than their associated images, or non-RGBA/RGBX frames, we only
need to update Decoder.

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c08afb91a772

Comment 3

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

Comment on attachment 8601847 [details] [diff] [review]
Compute the size of animated image frames correctly in the SurfaceCache

Approval Request Comment
[Feature/regressing bug #]: Bug 1116719.
[User impact if declined]: Some animated GIFs won't play back. They can even cause the browser to think it's near OOM, causing other images to fail to decode and making users restart their browser to recover. This is pretty serious; I'd really like to get this in 38 if possible.
[Describe test coverage new/current, TreeHerder]: Test coming up.
[Risks and why]: Low risk. The patch is pretty straightforward and only affects our estimates of the memory usage of image surfaces; it does not directly affect image rendering or decoding at all.
[String/UUID change made/needed]: None.

Comment 4

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

(In reply to Seth Fowler [:seth] from comment #3)
> [Describe test coverage new/current, TreeHerder]: Test coming up.

Hmm, it's actually impossible to write a test for this right now, because we need to check that we never manage to decode frame N, and there's no way to check that condition except for an inevitably-intermittent timeout.

After bug 11553322 lands, we will be able to reliably detect OOM because it will cause the image's |onerror| notification to fire. So it's better to wait. I'll file a followup to add a test.

Comment 5

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

(In reply to Seth Fowler [:seth] from comment #4)
> After bug 11553322 lands, we will be able to reliably detect OOM because it
> will cause the image's |onerror| notification to fire. So it's better to
> wait. I'll file a followup to add a test.

Ack, no, that's not spec compliant. Drat.

I think we may need to do some work to support testing this. I'll file a followup about it.

Comment 6

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

I filed bug 1161878 about the work necessary to test this.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8601847 [details] [diff] [review]
Compute the size of animated image frames correctly in the SurfaceCache

r=me

Comment 8

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

Thanks for the quick review! That try job doesn't seem to contain any failures introduced by this patch, so I'm going to go ahead and push.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbe25ecbebda

Comment 10

[Sylvestre Ledru [:Sylvestre]]

This is unfortunately probably too late for 38 but we could take it for 38.0.5 and 38.1.0esr.

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #10)
> This is unfortunately probably too late for 38 but we could take it for
> 38.0.5 and 38.1.0esr.

Yeah, 38.1esr sounds good. Just hoping to get it in an ESR release if possible.

Comment 13

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

Comment on attachment 8601847 [details] [diff] [review]
Compute the size of animated image frames correctly in the SurfaceCache

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a fix for a serious regression in our ability to play back longer animated images. Without this fix, not only will the images freeze midway through playback, but the browser will also be stuck in a low-memory state, forcing users to restart the browser. I'd really prefer to have this fix in 38.1 if possible.
User impact if declined:  ^ See above.
Fix Landed on Version: 40. Uplifting everywhere else ASAP.
Risk to taking this patch (and alternatives if risky): This is pretty low risk, as it just corrects our algorithm for estimating the size of an image surface. It doesn't affect higher-risk pathways like decoding or drawing.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Seth I see your comments on the other bug and writing tests, but is there a way we could manually test this?

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8601847 [details] [diff] [review]
Compute the size of animated image frames correctly in the SurfaceCache

Approved for uplift to aurora. Let's see if this helps with OOM and animations.

Comment 16

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

(In reply to Liz Henry (:lizzard) from comment #14)
> Seth I see your comments on the other bug and writing tests, but is there a
> way we could manually test this?

Yeah, a manual test is easy. Load the GIF attached to bug 1150089, and make sure that it (1) plays back at all, and (2) loops, rather than freezing part way through.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/391aa543ea6f

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8601847 [details] [diff] [review]
Compute the size of animated image frames correctly in the SurfaceCache

After reviewing with Seth we're going to take this fix in 38.0.1 for both mainline and ESR. This has noticeable user impact (Seth will provide more details with bug refs) and does not have much risk of regression. The patch has been on m-c for a week and m-a for 5 days at this point as well.

Note that this patch needs to land on:
38.0 relbranch
38.0ESR relbranch
m-r
m-esr38
m-b

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Issues with image loading
[Suggested wording]: Fixed: Images may fail to load
[Links (documentation, blog post, etc)]:

Comment 20

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

> <lmandel> seth: OK. Let's take this. What I need from you is:
> <lmandel> 1.  some dependent bugs that show that users are reporting this

See bug 1150089 for this. I think they're mostly duped against bug 1150089.

> <lmandel> 2. steps to reproduce so that SV can test overnight tonight

See comment 16.

> <lmandel> 3. uplift request for 38 and ESR38

Done!

Comment 21

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

It's probably worth noting that this affects all platforms based on Gecko 38, both desktop and mobile.

Here's a revised release note request:

Release Note Request (optional, but appreciated)
[Why is this notable]: Firefox 37 introduced a bug where large animated images may fail to decode completely. Even worse, once this has happened other images will also start failing to decode, forcing the user to restart their browser to recover. This is fixed in Firefox 38.
[Suggested wording]: Fixed: Large animated images may fail to play and may stop other images from loading
[Links (documentation, blog post, etc)]:

Comment 22

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Needs to change to 38+ AFAICT.

This was uplifted to Aurora prior to Monday's uplift, so Fx39 already has the fix.

Fx 38.0.5 (default):
https://hg.mozilla.org/releases/mozilla-release/rev/aa3a683fd335

Fx 38.0.1 (GECKO380_2015050320_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/570b63d791b9

ESR 38.1 (default):
https://hg.mozilla.org/releases/mozilla-esr38/rev/af674949ec09

ESR 38.0.1 (GECKO380esr_2015050513_RELBRANCH):
https://hg.mozilla.org/releases/mozilla-esr38/rev/117ad2bfcf6c

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This push to mozilla-release (default) broke lots of builds.

Comment 24

[Daniel Holbert [:dholbert]]

The error includes "no known conversion for argument 1 from 'nsIntSize' to 'const IntSize&".

(I think this is fallout from the recent nsIntSize --> IntSize mass-renaming that happened on trunk.)

I think we might just need s/aFrameRect.Size()/aFrameRect.Size().ToIntSize()/, here (in the mozilla-release cset):
  https://hg.mozilla.org/releases/mozilla-release/rev/aa3a683fd335#l1.12

Comment 25

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

(In reply to Daniel Holbert [:dholbert] from comment #24)
> The error includes "no known conversion for argument 1 from 'nsIntSize' to
> 'const IntSize&".
> 
> (I think this is fallout from the recent nsIntSize --> IntSize mass-renaming
> that happened on trunk.)
> 
> I think we might just need
> s/aFrameRect.Size()/aFrameRect.Size().ToIntSize()/, here (in the
> mozilla-release cset):
>   https://hg.mozilla.org/releases/mozilla-release/rev/aa3a683fd335#l1.12

Yep, that looks right. I'm cloning mozilla-release right now to verify that that's the only problem. Will upload a backported version shortly.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Ah dammit, I was worried about that hunk :(. Thanks for looking into it, Seth.

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I can push that to esr38 as well once m-r gets it.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Thanks, it'll need to land on default and the relbranches noted in comment 22.

Comment 29

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

Just pushed the fix:

https://hg.mozilla.org/releases/mozilla-release/rev/5da39cd23ade

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And the 38.0 relbranch: https://hg.mozilla.org/releases/mozilla-release/rev/bb7af314a8ac

And the esr38 relbranch: https://hg.mozilla.org/releases/mozilla-esr38/rev/c305fd7034ff

And default esr38: https://hg.mozilla.org/releases/mozilla-esr38/rev/3ea0032d900a

Hope that was done right.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Looks perfect, thanks!

Comment 32

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue on Firefox 38.0 ESR, verified that the issues if fixed on Windows 7 64-bit, Ubuntu 14.04 32-bit, Mac OS X 10.9.5, Windows 8.1 32-bit using Firefox 38.0.1 ESR and Firefox 38.0.1 RC.

Comment 33

[Catalin Suciu [:csuciu]]

Using the steps from comment #16 I'm able to reproduce on Fennec 38.0
Verifying as fixed on Fennec 38.0.1

Comment 34

[Catalin Suciu [:csuciu]]

Verifying as fixed on Fennec 38.0.5b4