Closed Bug 1125491 Opened 5 years ago Closed 5 years ago

Only lock during decoding for the initial decode

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Here's the patch.
Attachment #8554138 - Flags: review?(tnikkel)
Fixed a typo.
Attachment #8554139 - Flags: review?(tnikkel)
Attachment #8554138 - Attachment is obsolete: true
Attachment #8554138 - Flags: review?(tnikkel)
Attachment #8554139 - Flags: review?(tnikkel) → review+
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
Heh, *big* improvement.
https://hg.mozilla.org/mozilla-central/rev/91836c912d09
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
This resulted in a ~60 MiB drop in the dark blue line ("RSS: After TP5, tabs closed") on AWSY. Hooray!
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.
Attachment #8554139 - Flags: approval-mozilla-aurora?
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.
Attachment #8554139 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.