1125491 - Only lock during decoding for the initial decode

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Bug 1079627 changed the timing of a lot of things in ImageLib, and it ended up revealing some interesting new failure modes. One potential problem that I discovered while testing it is that it is possible for an image to be discarding during the initial decode, before we discover that it is animated. (This is *very* unlikely, but I've seen it happen in the magical environment of the B2G emulators, where time has no meaning.) This causes us to assert later since we assume that if an image is animated, we never discard any of its frames.

Now, I'm not sure that there's really much keeping us from allowing discarding of animated images, but I don't want to make that change right now. So until we flip that on, we need to ensure that they don't get discarded during decoding, and so in bug 1079627 I made us lock the image while a decoder is running.

That causes problems for the patches in bug 1125490 and bug 1125462, though, because they want to discard even if an image is being redecoded because it was in a background tab that got brought back to the foreground.

The thing is, we don't really need to lock during decoding *every* time - just during the initial decode, when we're not sure if we're animated or not. This patch makes us lock the image only during the initial decode. It adds a flag on each decoder that lets us know whether we locked the image when creating that particular decoder; this is the safest way to ensure that the locks and unlocks are balanced.

Comment 1

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

Attachment: Only lock the image during the initial decode

Here's the patch.

Comment 2

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

Attachment: Only lock the image during the initial decode

Fixed a typo.

Comment 3

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

Thanks for the review, Timothy. It's confirmed that this results in a bug improvement on AWSY; see bug 1122704 comment 7.

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=39ff99ddb6a7

Comment 4

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

Heh, *big* improvement.

Comment 5

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

Looks good! Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/91836c912d09

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/91836c912d09

Comment 7

[Nicholas Nethercote [inactive]]

This resulted in a ~60 MiB drop in the dark blue line ("RSS: After TP5, tabs closed") on AWSY. Hooray!

Comment 8

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

Comment on attachment 8554139 [details] [diff] [review]
Only lock the image during the initial decode

Approval Request Comment
[Feature/regressing bug #]: Part of fix for bug 1125272 and bug 1119938.
[User impact if declined]: Already approved.
[Describe test coverage new/current, TreeHerder]: In 38 for a long time.
[Risks and why]: Low risk; in 38 for a long time.
[String/UUID change made/needed]: None.

Comment 9

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

Pushed to Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/e19fc07f7163

Comment 10

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

Comment on attachment 8554139 [details] [diff] [review]
Only lock the image during the initial decode

Aurora approval previously granted for a collection of ImageLib related bugs that Seth worked on and QE verified before uplift.