Split up FrameAnimator's state into a separate AnimationState class

RESOLVED FIXED in Firefox 50

Status

()

Core
ImageLib
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(12 attachments, 8 obsolete attachments)

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
(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8772740 [details] [diff] [review]
(Part 1) - Separate FrameAnimator's state into a separate class, AnimationState.

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)
(Assignee)

Comment 2

2 years ago
Created attachment 8772742 [details] [diff] [review]
(Part 2) - Don't reset the last composited frame index when we reset animation.

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)
(Assignee)

Comment 3

2 years ago
Created attachment 8772743 [details] [diff] [review]
(Part 3) - Get rid of RefreshResult.error, a field which nothing cares about.

Just cleanup.
Attachment #8772743 - Flags: review?(edwin)
(Assignee)

Comment 4

2 years ago
Created attachment 8772755 [details] [diff] [review]
(Part 4) - We only need to check for infinitely long frames in StartAnimation() for the first frame.

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)
(Assignee)

Comment 5

2 years ago
Created attachment 8772756 [details] [diff] [review]
(Part 5) - Wrap frame timeout values in a FrameTimeout type that ensures they're normalized.

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)
(Assignee)

Comment 6

2 years ago
Created attachment 8772757 [details] [diff] [review]
(Part 6) - Don't call GetTimeoutForFrame() in RasterImage.

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)
(Assignee)

Comment 7

2 years ago
Created attachment 8772759 [details] [diff] [review]
(Part 7) - Make FrameAnimator::GetTimeoutForFrame() a private method that doesn't rely on AnimationState.

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)
(Assignee)

Comment 8

2 years ago
Created attachment 8772760 [details] [diff] [review]
(Part 8) - Return a FrameTimeout value from FrameAnimator::GetSingleLoopTime().

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)
(Assignee)

Comment 9

2 years ago
Created attachment 8772762 [details] [diff] [review]
(Part 8) - Determine the loop length of animated images while decoding them.

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)
(Assignee)

Comment 10

2 years ago
Created attachment 8772763 [details] [diff] [review]
(Part 9) - Determine the first frame refresh area of animated images while decoding them.

Similarly, we can determine the first frame refresh area in the decoder too.
Attachment #8772763 - Flags: review?(edwin)
(Assignee)

Comment 11

2 years ago
Created attachment 8772764 [details] [diff] [review]
(Part 10) - Clean up RefreshResult.

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)
(Assignee)

Comment 12

2 years ago
Created attachment 8772765 [details] [diff] [review]
(Part 11) - Use Moz2D types in FrameAnimator code.

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)
(Assignee)

Comment 13

2 years ago
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+
(Assignee)

Comment 20

2 years ago
Thanks for the quick reviews, Edwin! I'll make those changes now. Try looks green, too.
(Assignee)

Comment 21

2 years ago
Created attachment 8773083 [details] [diff] [review]
(Part 5) - Wrap frame timeout values in a FrameTimeout type that ensures they're normalized.

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.
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 22

2 years ago
Created attachment 8773084 [details] [diff] [review]
(Part 6) - Don't call GetTimeoutForFrame() in RasterImage.
(Assignee)

Comment 23

2 years ago
Created attachment 8773085 [details] [diff] [review]
(Part 7) - Make FrameAnimator::GetTimeoutForFrame() a private method that doesn't rely on AnimationState.
(Assignee)

Comment 24

2 years ago
Created attachment 8773086 [details] [diff] [review]
(Part 8) - Return a FrameTimeout value from FrameAnimator::GetSingleLoopTime().
(Assignee)

Comment 25

2 years ago
Created attachment 8773087 [details] [diff] [review]
(Part 9) - Determine the loop length of animated images while decoding them.
(Assignee)

Comment 26

2 years ago
Created attachment 8773088 [details] [diff] [review]
(Part 10) - Determine the first frame refresh area of animated images while decoding them.
(Assignee)

Comment 27

2 years ago
Created attachment 8773089 [details] [diff] [review]
(Part 11) - Clean up RefreshResult.
(Assignee)

Comment 28

2 years ago
Created attachment 8773090 [details] [diff] [review]
(Part 12) - Use Moz2D types in FrameAnimator code.

Comment 29

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1289628
(Assignee)

Updated

2 years ago
Blocks: 1289954
(Assignee)

Updated

2 years ago
Blocks: 1289957
You need to log in before you can comment on or make changes to this bug.