1289957 - Notify RasterImage about new frames in NotifyProgress() and remove OnAddedFrame()

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Right now, RasterImage gets notified when decoders begin a new frame in a special callback, RasterImage::OnAddedFrame(). This exists because decoders used to have no way to yield between frames, but RasterImage still needed to know when a new frame arrived so it could start animating if necessary. Now that decoders yield between frames, we no longer need this, and can deliver the new frame count as part of the call to RasterImage::NotifyProgress() that we use to deliver all other decoding-related updates.

This is the only major refactoring left before we can begin making FrameAnimator and the decoders cooperate more. We need to get to the point where FrameAnimator isn't coupled with RasterImage, and OnAddedFrame() is the main remaining thing keeping it coupled. Plus, it's gross.

Comment 1

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

Attachment: (Part 1) - Fix minor style issues in RasterImage::NotifyProgress.

Just some trivial style tweaks.

Comment 2

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

Attachment: (Part 2) - Do not include future changes in decoder progress notifications.

This is actually a fix for a bug that got introduced in the original patch that
introduced IDecodingTasks (bug 1282259). I'll add a note there. The mistake is
that we need to capture all the progress-related information at the time that we
create the runnable that does our async notification in NotifyProgress(). If we
don't, then we'll inadvertently end up notifying for things that hadn't happened
yet when we created the runnable. That makes writing assertions harder (which is
how I noticed this issue). It's also a data race on the member variables of
Decoder, so it's definitely a bug in its own right, though it doesn't seem to
have caused any issues.

Comment 3

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

Ack, I just noticed that Andrew also fixed the issue in part 2 in bug 1283462. Let's use his version. I'm going to mark part 2 obsolete and renumber the forthcoming patches.

Comment 4

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

Attachment: (Part 2a) - Store the frame count of animated images on AnimationState.

First, we need to stop storing the frame count of animated images on RasterImage
itself. There's not really any reason to keep it on RasterImage; this is just an
artifact of how this code has evolved. While I'm at it, I tried to make things a
little bit more clear to the reader in the FrameAnimator code.

(This will get folded together with Part 2b.)

Comment 5

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

Attachment: (Part 2b) - Notify RasterImage about new frames in NotifyProgress() and remove OnAddedFrame().

Once we have a new place to store the frame count, we send it along with the
bundle of other information in NotifyProgress() and remove OnAddedFrame(), which
now serves no purpose. That means that we need to start triggering animation in
NotifyProgress() in cases where we were waiting for a decoded frame to start the
animation.

A few notes about this patch:

- We remember whether any new frames have been finished since the last time
  TakeCompletedFrameCount() was called because we don't want to generate huge
  numbers of notifications if, say, we get one byte of the image at a time from
  the network. This way, we'll only call NotifyProgress() when something changed.

- In the old code, we began animating when the frame count hit 2. That's because
  in the old code, we got a notification at the *beginning of a new frame*. This
  code instead gives us a notification at the *end of an old frame*. That's why
  we want to check if the frame count hit 1; since one frame ends right before
  another one begins, this is actually checking for the same event - the
  boundary between the first and second frame - as the old code. (I should've
  pointed this out for part 2a as well, I think.) Because even non-animated
  images complete their first frame, we also check if we have an AnimationState
  - if we do, we're animated.

- You'll note that I don't tell AnimationState that decoding is done until after
  the final call to NotifyProgress() in FinalizeDecoder(). That's because
  NotifyProgress() can add one more frame at this point, and so if we didn't
  make this change,  we'd trip the assertion that we don't get any new frames
  after decoding is complete.

Comment 6

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

Try job here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e55dff0b54c

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8775383 [details] [diff] [review]
(Part 2a) - Store the frame count of animated images on AnimationState.

Review of attachment 8775383 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/FrameAnimator.cpp
@@ +52,5 @@
> +    // what we already know about doesn't indicate an error.
> +    return;
> +  }
> +
> +  MOZ_ASSERT_IF(mDoneDecoding, aFrameCount <= mFrameCount);

With the guard above, this is logically equivalent to MOZ_ASSERT(!mDoneDecoding) which is a bit less confusing.

::: image/RasterImage.cpp
@@ +913,5 @@
>    MOZ_ASSERT(ShouldAnimate(), "Should not animate!");
>  
>    // If we're not ready to animate, then set mPendingAnimation, which will cause
>    // us to start animating if and when we do become ready.
> +  mPendingAnimation = !mAnimationState || mAnimationState->KnownFrameCount() < 1;

Should have noticed this in an earlier review, but the latter condition should never be hit given the ShouldAnimate() assertion above.

Though I guess it still could be useful if the assertion is violated in release builds.

Comment 8

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

Thanks for the quick review, Edwin! I'll make those changes.

(In reply to Edwin Flores [:eflores] [:edwin] from comment #7)
> Though I guess it still could be useful if the assertion is violated in
> release builds.

After years of dealing with ImageLib bugs I've learned to be cautious. =)

Comment 9

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

Attachment: (Part 2) - Notify RasterImage about new frames in NotifyProgress() and remove OnAddedFrame().

Addressed review comments and squashed together all of part 2 into one patch.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4fb79a8143
(Part 1) - Fix minor style issues in RasterImage::NotifyProgress. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/70369395b754
(Part 2) - Notify RasterImage about new frames in NotifyProgress() and remove OnAddedFrame(). r=edwin

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c4fb79a8143
https://hg.mozilla.org/mozilla-central/rev/70369395b754