1187546 - Make it possible to ask image decoders to only decode the first frame

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Frequently, we only need the first frame of an animated image. Such cases include:

1. Decoding for ImageBitmap, where we will decode an image, grab a surface for the first frame, and just return that.

2. If we start playing back animated images through the media framework, we'll still need to be able to decode the first frame in the traditional way to support APIs like |canvas.drawImage|.

3. If we ever decide that we want to apply downscale-during-decode to the first frame of animated images in some situations, we'd need to be able to decode only the first frame.

Case 1 is an immediate need, and case 2 seems pretty likely to become an issue in the near future, so let's add an API to Decoder to allow callers to request that only the first frame be decoded.

Comment 1

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

Attachment: Make it possible to ask image decoders to only decode the first frame

Here's the patch. It's surprisingly simple.

The only oddities are in the PNG implementation. There are two things worth noting:

- I don't love the idea of making a special magic number returned from setjmp()
  indicate "this is a successful early return and doesn't indicate an error", as
  this seems a bit fragile to me, so I decided to use an out-of-band boolean
  instead.

- Note that we consider the "first frame" of an APNG to be the first
  *non-hidden* frame. This is maintain consistency with what we'd do without
  this patch. We don't even bother allocating a surface for the hidden frame,
  which is just a backwards compatibility feature that enables an APNG to
  provide a frame that's only displayed in PNG viewers that don't support APNG.
  That means we'd never have used the hidden frame for e.g.
  |canvas.drawImage()|, so we shouldn't produce that frame in a first-frame-only
  decode either.

I've tested this code by forcing it on all the time and viewing animated GIFs
and APNGs on the web, and it works as intended. I won't be able to actually land
tests for it until bug 1181863, though, because right now it's not exposed
through any mechanism at all. The new code in bug 1181863 will use it and make
it testable, so tests will land there.

Comment 2

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

Attachment: Make it possible to ask image decoders to only decode the first frame

Minor tweak: we now properly propagate the first-frame-only flag to PNG-in-ICO.

Comment 3

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

Attachment: Make it possible to ask image decoders to only decode the first frame

Once I wrote the tests for bug 1181863, I noticed a couple of issues with this
patch:

- There was an assertion in Decoder's destructor that we consumed all progress
  and all invalidations generated by the decoder. I don't think this assertion
  makes sense when we don't have an image, so I've modified the assertion to not
  fire in that case.

- We needed to pass the fact that we're only decoding the first frame through
  from nsICODecoder to the BMP decoder as well, because even though it has no
  effect on the results in that case, we do trigger an assertion in Decoder if
  we don't pass it through since the BMP decoder doesn't have an image.

With those changes made, everything looks good.

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8640021 [details] [diff] [review]
Make it possible to ask image decoders to only decode the first frame

I don't know the internals of the decoders that well. Is there anyone who does?

Comment 5

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

(In reply to Timothy Nikkel (:tn) from comment #4)
> Comment on attachment 8640021 [details] [diff] [review]
> Make it possible to ask image decoders to only decode the first frame
> 
> I don't know the internals of the decoders that well. Is there anyone who
> does?

I'm not really sure. I think Jeff has some knowledge of them. Glenn is definitely the most knowledgeable person about the PNG decoder.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/070dc7eabe5f

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/070dc7eabe5f