Closed Bug 1288040 Opened 3 years ago Closed 3 years ago

Split up FrameAnimator's state into a separate AnimationState class

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 8 obsolete files)

47.43 KB, patch
eflores
: review+
Details | Diff | Splinter Review
10.73 KB, patch
eflores
: review+
Details | Diff | Splinter Review
2.50 KB, patch
eflores
: review+
Details | Diff | Splinter Review
1.27 KB, patch
eflores
: review+
Details | Diff | Splinter Review
27.49 KB, patch
Details | Diff | Splinter Review
2.86 KB, patch
Details | Diff | Splinter Review
5.39 KB, patch
Details | Diff | Splinter Review
5.23 KB, patch
Details | Diff | Splinter Review
12.81 KB, patch
Details | Diff | Splinter Review
14.45 KB, patch
Details | Diff | Splinter Review
8.48 KB, patch
Details | Diff | Splinter Review
9.20 KB, patch
Details | Diff | Splinter Review
We want to move FrameAnimator into an ISurfaceProvider so that we can decode animated images frame by frame. The tricky thing is that if the FrameAnimator is in the SurfaceCache, that means that it can be discarded! We don't want to lose the state of the animation if that happens, so that means we need to pull the state that needs to surface a discard out into a separate class that can continue to live on RasterImage.

Get ready for a bunch of tedious refactoring, though it's all pretty simple.
This part does the initial work of separating the state out. I realize this
makes things kinda ugly; the remaining patches in this bug will clean things up
as much as possible. By passing the AnimationState in directly in this patch, it
becomes easy to see what we need to clean up in subsequent patches.
Attachment #8772740 - Flags: review?(edwin)
This is the only intentional functional change in this whole patch, but it
shouldn't matter at all. It doesn't make sense that we reset the last composited
frame index when we reset animation. After all, the last frame we composited in
still in FrameAnimator::mCompositingFrame, and despite the animation being
reset, that frame will still be valid. There's no reason why we shouldn't use it
if, say, the animation start again and we happen to find ourselves back on that
same frame in the timeline.

By making this change, we can make mCompositingFrame and
mLastCompositingFrameIndex, which tracks which frame mCompositingFrame actually
contains, live in the same place, which just makes sense. Their lifetimes should
be the same.
Attachment #8772742 - Flags: review?(edwin)
This is another thing that just doesn't make sense. If any other frame has an
infinitely long timeout, then the way we'll find out about that won't be in
StartAnimation(), it'll be because FrameAnimator::RequestRefresh() returns a
RefreshResult which tells us that the animation is finished. There's also no
chance of regression here, since this is purely an optimization.
Attachment #8772755 - Flags: review?(edwin)
We really need to make sure that frame timeout values are always normalized.
This new FrameTimeout class will simplify the code everywhere else and will
greatly increase our confidence in its correctness.

The addition operators aren't used yet, but will be in a later patch.
Attachment #8772756 - Flags: review?(edwin)
Since, due to earlier patches, we only need the timeout for the first frame in
RasterImage, and since that timeout is already available on AnimationState, we
can just get it from there and make this method private on FrameAnimator.

Keep in mind that we want to minimize dependency on FrameAnimator in RasterImage
because it won't have a direct reference to it eventually.
Attachment #8772757 - Flags: review?(edwin)
There's no reason to consult AnimationState in GetTimeoutForFrame() to get the
first frame's timeout if FrameAnimator is the only thing that calls it, because
we don't start animating (and hence calling into FrameAnimator) until we've
fully decoded the first frame. (This decision is made in
RasterImage::OnAddedFrame(); note that until we have the first frame available,
we don't start animation, and just have |mPendingAnimation| to set to let us
know to start it when the first frame is decoded.)
Attachment #8772759 - Flags: review?(edwin)
This is where those addition operators on FrameTimeout come into play. Since
those operators handle infinite timeouts correctly, the loop here is drastically
simplified. The plan is to move this code onto Decoder (as you'll see in an
upcoming patch), so this cleanup will make that further refactoring very simple.
Attachment #8772760 - Flags: review?(edwin)
We can easily determine the loop length of animated images in the decoder. We
should just do it. This is another thing that should live on AnimationState, and
that only needs to be calculated once. Note that this is *drastically* more
efficient than what we do now, because calling into SurfaceCache is expensive as
it requires locking, and the old loop in FrameAnimator required us to do that
*once per frame of the animation*, and we called that function frequently.
Attachment #8772762 - Flags: review?(edwin)
Similarly, we can determine the first frame refresh area in the decoder too.
Attachment #8772763 - Flags: review?(edwin)
This is just a cleanup patch. Let's make this ancient code match our current C++
coding style. No functional changes here.
Attachment #8772764 - Flags: review?(edwin)
One more cleanup patch. Let's get rid of nsIntRect in this code, as long as
we're touching everything anyway.
Attachment #8772765 - Flags: review?(edwin)
Sorry for this huge patch stack, man. This code needed a lot of refactoring. =\

Also, I *swear* there are only 11 patches in this bug. Totally didn't mess up the numbering at all. =)
Comment on attachment 8772756 [details] [diff] [review]
(Part 5) - Wrap frame timeout values in a FrameTimeout type that ensures they're normalized.

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

::: image/imgFrame.h
@@ +49,5 @@
> +/**
> + * FrameTimeout wraps a frame timeout value (measured in milliseconds) after
> + * first normalizing it.  This normalization is necessary because some tools
> + * generate incorrect frame timeout values which we nevertheless have to
> + * support. For this reason, that deals with frame timeouts should always use a

Missing a word ("code"?) before "that deals with".

@@ +266,5 @@
>     * @param aFrameOpacity    Whether this imgFrame is opaque.
>     * @param aDisposalMethod  For animation frames, how this imgFrame is cleared
>     *                         from the compositing frame before the next frame is
>     *                         displayed.
> +   * @param aTimeout         For animation frames, the timeout in milliseconds

"in milliseconds" doesn't make sense here.
Attachment #8772756 - Flags: review?(edwin) → review+
Comment on attachment 8772760 [details] [diff] [review]
(Part 8) - Return a FrameTimeout value from FrameAnimator::GetSingleLoopTime().

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

::: image/FrameAnimator.cpp
@@ -241,5 @@
> -  if (loopTime > 0) {
> -    // We shouldn't be advancing by a whole loop unless we are decoded and know
> -    // what a full loop actually is. GetSingleLoopTime should return -1 so this
> -    // never happens.
> -    MOZ_ASSERT(aState.mDoneDecoding);

I'm always a bit nervous about removing assertions. While it's guaranteed to hold currently AFAICT, it's nice to be defensive for when we have to touch this code again.
Attachment #8772760 - Flags: review?(edwin) → review+
(In reply to Edwin Flores [:eflores] [:edwin] from comment #16)
> I'm always a bit nervous about removing assertions. While it's guaranteed to
> hold currently AFAICT, it's nice to be defensive for when we have to touch
> this code again.

Never mind. I see this code dies in the succeeding patch.
Comment on attachment 8772762 [details] [diff] [review]
(Part 8) - Determine the loop length of animated images while decoding them.

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

::: image/FrameAnimator.h
@@ +88,5 @@
>    int32_t LoopCount() const { return mLoopCount; }
>  
> +  /**
> +   * Set the length of a single loop through this image, as a FrameTimeout value
> +   * in milliseconds.

Again, FrameTimeout should imply milliseconds IMO.

@@ +94,5 @@
> +  void SetLoopLength(FrameTimeout aLength) { mLoopLength = Some(aLength); }
> +
> +  /**
> +   * @return the length of a single loop of this image, as a FrameTimeout value
> +   * in milliseconds. If this image is not finished decoding, is not animated,

and here.
Attachment #8772762 - Flags: review?(edwin) → review+
Comment on attachment 8772763 [details] [diff] [review]
(Part 9) - Determine the first frame refresh area of animated images while decoding them.

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

::: image/Decoder.h
@@ +428,5 @@
>  
>    uint32_t mFrameCount; // Number of frames, including anything in-progress
>    FrameTimeout mLoopLength;  // Length of a single loop of this image.
> +  gfx::IntRect mFirstFrameRefreshArea;  // The area of the image that needs to
> +                                        // be invalidated when the animation loops. 

nit: trailing whitespace

::: image/ImageMetadata.h
@@ +82,5 @@
>    /// The timeout of an animated image's first frame.
>    FrameTimeout mFirstFrameTimeout;
>  
> +  // The area of the image that needs to be invalidated when the animation
> +  // loops. 

nit: trailing whitespace
Attachment #8772763 - Flags: review?(edwin) → review+
Thanks for the quick reviews, Edwin! I'll make those changes now. Try looks green, too.
In this new version of the patch (and the new versions of the subsequent patches
I'm about to upload) I've addressed the review comments and fixed the numbering
of the parts.
Attachment #8772756 - Attachment is obsolete: true
Attachment #8772757 - Attachment is obsolete: true
Attachment #8772759 - Attachment is obsolete: true
Attachment #8772760 - Attachment is obsolete: true
Attachment #8772762 - Attachment is obsolete: true
Attachment #8772763 - Attachment is obsolete: true
Attachment #8772764 - Attachment is obsolete: true
Attachment #8772765 - Attachment is obsolete: true
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8383c2cc9939
(Part 1) - Separate FrameAnimator's state into a separate class, AnimationState. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/34faad78d6f0
(Part 2) - Don't reset the last composited frame index when we reset animation. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2c834df0db
(Part 3) - Get rid of RefreshResult.error, a field which nothing cares about. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/b979622ece87
(Part 4) - We only need to check for infinitely long frames in StartAnimation() for the first frame. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/3760df575458
(Part 5) - Wrap frame timeout values in a FrameTimeout type that ensures they're normalized. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/725304d2b48b
(Part 6) - Don't call GetTimeoutForFrame() in RasterImage. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/76ad9c122376
(Part 7) - Make FrameAnimator::GetTimeoutForFrame() a private method that doesn't rely on AnimationState. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b8a4bc9168
(Part 8) - Return a FrameTimeout value from FrameAnimator::GetSingleLoopTime(). r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec558fefe42f
(Part 9) - Determine the loop length of animated images while decoding them. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a652ffa8bfb
(Part 10) - Determine the first frame refresh area of animated images while decoding them. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dee7c065326
(Part 11) - Clean up RefreshResult. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb7f6c316bb
(Part 12) - Use Moz2D types in FrameAnimator code. r=edwin
Blocks: 1289628
Blocks: 1289957
You need to log in before you can comment on or make changes to this bug.