Detect IS_ANIMATED during the metadata decode

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Depends on 1 bug)

unspecified
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(9 attachments, 1 obsolete attachment)

1.19 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
16.44 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.26 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.00 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
9.44 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
11.40 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.96 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.93 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
12.24 KB, patch
Details | Diff | Splinter Review
Assignee

Description

4 years ago
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.
Assignee

Comment 2

4 years ago
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.
Attachment #8647824 - Flags: review?(tnikkel)
Assignee

Comment 3

4 years ago
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.
Attachment #8647829 - Flags: review?(tnikkel)
Assignee

Comment 4

4 years ago
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.)
Attachment #8647831 - Flags: review?(tnikkel)
Assignee

Comment 5

4 years ago
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.)
Attachment #8647832 - Flags: review?(tnikkel)
Attachment #8647824 - Flags: review?(tnikkel) → review+
Assignee

Updated

4 years ago
Depends on: 1194557
Assignee

Comment 6

4 years ago
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.)
Attachment #8647835 - Flags: review?(tnikkel)
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?
Attachment #8647829 - Flags: review?(tnikkel) → review+
Attachment #8647832 - Flags: review?(tnikkel) → review+
Attachment #8647831 - Flags: review?(tnikkel) → review+
Attachment #8647835 - Flags: review?(tnikkel) → review+
Assignee

Comment 8

4 years ago
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.
Attachment #8647862 - Flags: review?(tnikkel)
Assignee

Comment 9

4 years ago
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.
Attachment #8647863 - Flags: review?(tnikkel)
Attachment #8647862 - Flags: review?(tnikkel) → review+
Assignee

Comment 10

4 years ago
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.
Attachment #8647865 - Flags: review?(tnikkel)
Attachment #8647863 - Flags: review?(tnikkel) → review+
Attachment #8647865 - Flags: review?(tnikkel) → review+
Assignee

Comment 11

4 years ago
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.
Attachment #8647870 - Flags: review?(tnikkel)
Assignee

Updated

4 years ago
Blocks: 1185800
Assignee

Comment 13

4 years ago
(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.
Assignee

Comment 14

4 years ago
(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!
Assignee

Updated

4 years ago
Blocks: 1194575
Attachment #8647870 - Flags: review?(tnikkel) → review+
Assignee

Comment 16

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

Updated

4 years ago
Attachment #8647870 - Attachment is obsolete: true

Updated

4 years ago
Depends on: 1195878
Assignee

Updated

4 years ago
Depends on: 1210745

Updated

4 years ago
Depends on: 1233403
Depends on: 1270997
You need to log in before you can comment on or make changes to this bug.