1116735 - Allocate frames in the Decoder instead of in RasterImage

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

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.

Comment 1

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

Attachment: Allocate frames in the decoder

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.

Comment 2

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

Attachment: Allocate frames in the decoder

I noticed a stray comment that shouldn't have been in this patch. Removed.

Comment 3

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

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=2e8ff370b87a

Comment 4

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

Attachment: Allocate frames in the decoder

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().

Comment 5

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

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

Comment 6

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

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

Comment 7

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

OK, apparently 2D.h does not pull in SurfaceFormat. Here's another attempt:

https://tbpl.mozilla.org/?tree=Try&rev=1ef8af873746

Comment 8

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

Attachment: Allocate frames in the decoder

Sheesh. Not sure why I couldn't see it earlier today; the problem is that I need a using statement in this patch. =\

Comment 9

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

Here we go. Should work this time:

https://tbpl.mozilla.org/?tree=Try&rev=782159d40f7a

Comment 10

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

Alright, this one is finally green. That needed far too many tries. Sigh.

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

Comment 11

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

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0933c1aef197

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/0933c1aef197