We need to be able to tell whether an image is animated *before* we start a full decode. This is important for many reasons: - It eliminates a lot of race conditions. - It eliminates the need to lock images during their first decode, which can make it harder to reclaim memory if we're under memory pressure. - We need to do it to implement downscale-during-decode for GIF, PNG, and ICO images, because we can't downscale such images during decode. - Eventually we want to play back animated images through the media framework, which means that the decoder for an animation would be implemented much differently than decoders for single-frame images.

It's harder to reason about the behavior of the metadata decoder if we deliver the notifications from it at different times. To reduce the space of things we have to worry about, let's ensure that metadata decoders only deliver their notifications in FinalizeDecoder.

Part 2 was a massive patch, so I've split it into a bunch of subpatches. The various subpatches do not compile independently of each other, or else I would've just made them separate parts, but they are hopefully conceptually independent enough to make reviewing easier. I'll fold these together when landing. This first chunk basically accomplishes two goals: - We want to add some animation metadata to ImageMetadata. - We want to do some more complicated stuff when we transfer that metadata to the image - primarily, we want to force a sync redecode if we detect animation during a full decode that we didn't detect during a metadata decode - so it ends up being cleaner to replace ImageMetadata::SetOnImage with a new RasterImage method, RasterImage::SetMetadata. It's worth going into more detail about the sync redecode thing above. A significant source of complexity in this patch is that corrupt GIFs can have a zero delay time for the first frame, which should indicate that they aren't animated, but then have additional frames, meaning that they're animated after all. Such GIFs are very uncommon. These are the same GIFs that were broken by bug 890743. To make such GIFs work, we ensure that we detect the animation during full decodes, even if we initially think the image isn't animated, by decoding far enough to encounter a second frame if one exists. If that happens, SetMetadata() calls RecoverFromLossOfFrames(), which causes a synchronously redecode. Because we mark the image as animated (by creating mAnim) before calling RecoverFromLossOfFrames(), this time it will be decoded as an animated image, and we'll get all the frames. It's important that we discard all the frames we decoded up to that point (which RecoverFromLossOfFrames() does) because we might have creating DDD frames, but DDD doesn't apply to animated images. Part 4 includes a test that ensures that this mechanism works.

We need FrameAnimator::GetTimeoutForFrame() to work even before the first frame is decoded, because as soon as we send the FLAG_IS_ANIMATED notification, we'll register the image with the refresh driver, and when we do that we try to bucket images according to the timeout for their first frame. To make this work, we grab this value during the metadata decode, and give it to FrameAnimator, so FrameAnimator can fall back to the value we've given it even if we haven't actually decoded the first frame. (It's worth noting that once we start playing back animated images with the media framework, we won't need this stuff, since media playback is totally async. So expect this particular complexity to be temporary.)

We don't need to lock images during their initial decode anymore, because we'll detect if the image is animated during the metadata decode. (And even if we hit that rare corrupt GIF case, we'll lock the image at that point and do a sync redecode.)

This part adds a separate DecoderFactory method for creating a decoder for animated images. In the short term there isn't a huge difference between them - mainly the separate methods are better able to enforce policy and correct use of the API. (For example, the method for animated images does not allow the caller to pass a target size for downscale-during-decode at all, since we can't support DDD for animated images.) Eventually, once we start using the media framework for animated images, we will be creating a totally different type of decoder for the animated case. One important factor is that for non-animated images, we always do a first-frame decode. That ensures that even if we hit the corrupt GIF case, we don't accidentally decode all the frames of the image and waste time applying DDD to animated frames. (In general, this minimizes the amount of time we waste on the initial full decode when we hit that case.)

Comment on attachment 8647829 [details] [diff] [review] (Part 2a) - Add animation metadata to ImageMetadata, and let RasterImage interact with ImageMetadata directly Maybe we should rename RecoverFromLossOfFrames?

Believe it or not, this little patch actually does the real work! We detect if an image is animated during the metadata decode, and call the new Decoder method PostIsAnimated() if so. To detect the corrupt GIF case, we also call PostIsAnimated() if appropriate right before we terminate a first-frame decode of a GIF.

Finally, we need to update RasterImage code that assumes that mAnim is not created until we get our second frame. In some cases, we can just delete code - for example, RasterImage::MarkAnimationDecoded() is no longer necessary, because we know mAnim is available before we even start a full decode. In some cases we need to call GetNumFrames() in places where we previously just checked whether mAnim was available, because we may now have mAnim without actually having decoded any frames at all or even having started a full decode.

This is just a minor test refactoring patch, to make part 4 cleaner. We always want a buffered nsIInputStream returned from LoadImage(); this patch moves the code to ensure that into LoadImage() itself, so every caller doesn't have to duplicate it.

This patch adds the tests, using the existing TestMetadata framework. Basically we just tag all the test cases as animated or not, and add some new animated ones. We then verify in CheckMetadata() that the metadata decode produces the expected results. There's one special test case: NoFrameDelayGIFTestCase. This is designed to test the corrupt GIF case that has come up so often in this bug. It has two tests: - The first test, NoFrameDelayGIF, is a normal TestMetadata test. It verifies that we *don't* detect that NoFrameDelayGIFTestCase is animated during the metadata decode. (It's not marked as TEST_CASE_IS_ANIMATED, so CheckMetadata expects this.) - The second test, NoFrameDelayGIFFullDecode, actually creates a RasterImage and triggers a full decode of NoFrameDelayGIFTestCase. It verifies that we fire FLAG_IS_ANIMATED, and that both frames of the image end up in the SurfaceCache. This checks that the recovery code in RasterImage::SetMetadata() works, and that we detect that the second frame exists during the first-frame-only decode of the image.

(In reply to Seth Fowler [:seth] [:s2h] from comment #0) > - We need to do it to implement downscale-during-decode for GIF [and PNG images]... I didn't word that quite right in comment 0; what I meant was that we can't downscale-during-decode *animated* images, so if we want to support DDD for image formats that support animation, we need to be able to detect animation during the metadata decode.

(In reply to Timothy Nikkel (:tn) from comment #7) > Comment on attachment 8647829 [details] [diff] [review] > (Part 2a) - Add animation metadata to ImageMetadata, and let RasterImage > interact with ImageMetadata directly > > Maybe we should rename RecoverFromLossOfFrames? Agreed. I'll file a followup. Thanks for these super-fast reviews!

Trivial tweak to part 4 to avoid a warning that was getting triggered on GCC.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd36b0369e6 https://hg.mozilla.org/integration/mozilla-inbound/rev/167ceb965079 https://hg.mozilla.org/integration/mozilla-inbound/rev/c263278045e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/c035b63e1b81

https://hg.mozilla.org/mozilla-central/rev/ddd36b0369e6 https://hg.mozilla.org/mozilla-central/rev/167ceb965079 https://hg.mozilla.org/mozilla-central/rev/c263278045e2 https://hg.mozilla.org/mozilla-central/rev/c035b63e1b81