Closed Bug 1116735 Opened 5 years ago Closed 5 years ago

Allocate frames in the Decoder instead of in RasterImage

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 3 obsolete files)

Moving the site of imgFrame allocation from RasterImage to Decoder will simplify the implementation of bug 1116733 - basically, consider this bug to be the purely refactoring components of that bug, spun out into a separate bug.
Depends on: 1116737
Attached patch Allocate frames in the decoder (obsolete) — Splinter Review
Here we go. Another pure refactoring patch. The frame allocation code from RasterImage ends up living in Decoder. There is a small amount of state update code that actually needs to be in RasterImage, so that moves into RasterImage::OnAddedFrame.

Related code is updated as needed. There are a few tweaks necessary in various places, such as the necessity of setting the size in advance for non-size-decode Decoder objects so that the frame preallocation code succeeds. If we start allocating the first frame off-main-thread as well, we won't need this, but for now it works well.
Attachment #8542928 - Flags: review?(tnikkel)
Attached patch Allocate frames in the decoder (obsolete) — Splinter Review
I noticed a stray comment that shouldn't have been in this patch. Removed.
Attachment #8543452 - Flags: review?(tnikkel)
Attachment #8542928 - Attachment is obsolete: true
Attachment #8542928 - Flags: review?(tnikkel)
Blocks: 1118087
Attachment #8543452 - Flags: review?(tnikkel) → review+
Attached patch Allocate frames in the decoder (obsolete) — Splinter Review
So the try job revealed a bug in the BMP decoder with this patch applied. The reason is that the BMP decoder doesn't process the BMP header if HasSize() returns true. Since HasSize() always returns true for full decodes after this patch, the BMP decoder always failed. I fixed this by adding a new boolean member variable to nsBMPDecoder explicitly recording whether we've processed the header, instead of depending on HasSize().
Attachment #8543452 - Attachment is obsolete: true
Thanks for the review, Timothy!

Here's a try job for the version of the patch in comment 4:

https://tbpl.mozilla.org/?tree=Try&rev=bd5be3955cd0
Looks green on try except for a failure to build in non-unified mode due to a missing header in Decoder.cpp. Here's a new try job to test the fix for that:

https://tbpl.mozilla.org/?tree=Try&rev=05f4d7b970e7
OK, apparently 2D.h does not pull in SurfaceFormat. Here's another attempt:

https://tbpl.mozilla.org/?tree=Try&rev=1ef8af873746
Sheesh. Not sure why I couldn't see it earlier today; the problem is that I need a using statement in this patch. =\
Attachment #8545472 - Attachment is obsolete: true
Here we go. Should work this time:

https://tbpl.mozilla.org/?tree=Try&rev=782159d40f7a
Alright, this one is finally green. That needed far too many tries. Sigh.

https://tbpl.mozilla.org/?tree=Try&rev=ff28ef936d4b
https://hg.mozilla.org/mozilla-central/rev/0933c1aef197
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.