Bug 717872 (omtagif)

Off-main-thread animated images

NEW
Assigned to

Status

()

7 years ago
2 months ago

People

(Reporter: cjones, Assigned: gal)

Tracking

(Blocks: 4 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10slater)

Details

(Whiteboard: [tech-p2][leave open])

Attachments

(9 attachments, 8 obsolete attachments)

1.82 KB, patch
seth
: review+
joe
: checkin+
Details | Diff | Splinter Review
6.50 KB, patch
seth
: review+
joe
: checkin+
Details | Diff | Splinter Review
15.99 KB, patch
seth
: review+
joe
: checkin+
Details | Diff | Splinter Review
14.22 KB, patch
seth
: review+
joe
: checkin+
Details | Diff | Splinter Review
17.60 KB, patch
seth
: review+
joe
: checkin+
Details | Diff | Splinter Review
7.61 KB, patch
seth
: review+
joe
: checkin+
Details | Diff | Splinter Review
39.57 KB, patch
seth
: review+
joe
: checkin+
Details | Diff | Splinter Review
18.25 KB, patch
Details | Diff | Splinter Review
1.38 KB, patch
Details | Diff | Splinter Review
(Disclaimer: I'm tired and stressed and mostly filing this for my own amusement.  But this really is something we could do.)

Currently we both decode and animate animated GIFs on the main thread.  We could better exploit parallelism, and not block content on decoding or vice versa, by decoding off the main thread.  But even with off-main-thread decoding, content will still be able to kill the framerate of animated gifs through GCs etc.  To ensure we hit the target animated GIF framerate, we need omtc.

In all seriousness, we could pretty easily implement this with the machinery we're building for async video (placeholder layers) and off-main-thread image decoding.

Updated

6 years ago
No longer depends on: 598873

Updated

6 years ago
Blocks: 598873
No longer blocks: 598873
Blocks: 598873
Will the work for this bug include off-main-thread animated PNGs?
It could, but this bug isn't a priority for anyone right now.
(In reply to Jared Wein [:jaws] from comment #1)
> Will the work for this bug include off-main-thread animated PNGs?

There is basically no way to do this for GIF without "accidentally" doing it for APNG as well.
Summary: Off-main-thread animated GIFs → Off-main-thread animated images
Duplicate of this bug: 794489
Blocks: 839889
(I filed this as a half-joke, but) this use case has turned out to important to content.
Whiteboard: [tech-p2]
This should use hardware GIF coprocessor in new ARM cores, too. (fnord)
Assignee: nobody → joe
We'll want to layerize all animated images. That means modifying nsDisplayImage::GetLayerState and nsDisplayBackgroundImage::GetLayerState to choose the ACTIVE state for animated images. (Maybe give up if we have to tile a CSS background image.) Then it's just a matter of making imglib call SetCurrentImage on the mozilla::layers::ImageContainer at the right times --- ImageContainer is a thread-safe object and SetCurrentImage can be called from any thread.
A non-strict prerequisite for this bug is bug 867758, which should help us redraw less and therefore make displaying lots of images on the same page faster.
Depends on: 867770
Depends on: 867774
Current thumbnail sketch. (This is based on the idea of us doing the blending, i.e. RasterImage::DoComposite(), on the composition or some other thread, and thus not making fundamental changes to the way we decode animated images.)

This is in the order I think I'll work in.

* Bug 867758 - Coordinating same-FPS images - Relatively easy, but extend it to a week to be safe.
* Bug 867770 - Layerizing animated images - Could take a couple of weeks. Just the layerization is the easy part; making animated images play well with being layerized, and getting it green for landing, is much more work.
* Bug 867774 - Making DoComposite() threadsafe - A week.
* This bug - Drive animated images off the main thread - Three weeks. Likely the hardest part.
Component: Graphics → ImageLib
Depends on: 869418
The Chromium event loop seems to, when it's possible, use the native event loop of the OS to sleep until the next timer fires, which is exactly what we want. Hooray!
No longer depends on: 869418
Here's my plan of action. I'd love to have any feedback you might want to throw my way.

(1d) is a time estimate, in days.

* Remove the optimization in FrameBlender that mutates the mFrames array (1d)
** If we need such an optimization, add it later, by a followup bug, in local mCompositingFrame storage
* Make a FrameBlender that can share mFrames with another FrameBlender (1d)
** It will have a unique mAnim, but a shared mFrames
** It will therefore have a unique current frame and animation state
* Create an asynchronous, thread-agnostic ImageAnimator class or helper (3d)
** is initialized with an image's initial state, including animation start time and frame#
** Has a FrameBlender that shares the image frames with RasterImage
*** (This implies that the optimization that mutates frames must be removed)
** This will mostly be a duplicate of RasterImage::RequestRefresh
** When the frame or the image animation state changes, we send a message to the RasterImage to tell it that it happened
** When someone requests the current frame (FRAME_CURRENT, not FRAME_FIRST), RasterImage will need to manually retrieve it from its *own* FrameBlender
* Store frames in shmem whem OMTC is enabled (3d)
** The first frame will be allocated in regular memory initially. Upon allocating the second frame, the first frame will need to be reallocated and copied to shmem
* Create a Compositor code path that animates and uploads images that have been registered with it (12d)
** When OMTC is enabled, don't register layerized animated images with the Refresh Driver, register them with the Compositor (2d)
** The Compositor will create its own (FrameBlender, ImageAnimator) pair per registered image (1d)
** There will be an IPDL protocol entry point that registers an image with the compositor that is driven by the same codepaths that currently register animated images with the refresh driver. These codepaths will also tell the RasterImage that it has been registered with the compositor (1.5d)
** There will be a second entry point that registers shmem frames with the compositor, and RasterImage will drive that itself as frames are decoded (1.5d)
** The compositor will set a timer for the nearest timeout for an image, and when that timer fires, it will drive its unique FrameBlender with a ImageAnimator to create the current frame (2d)
** The compositor then uploads that frame to form an Image that backs the animated image's ImageLayer (2d)
** Greening up (2d)
Created attachment 768547 [details] [diff] [review]
part 1: Store a frame's raw image data pointer beside its imgFrame pointer so we can access it without having to lock the frame

This patch makes us store imgFrames in FrameBlender with a new sort-of-tuple,
FrameDataPair, that is smart enough to be able to lock and unlock imgFrames,
and can be transparently cast to an imgFrame, but doesn't do too much else.
The alternative, storing a separate array of uint8_t pointers, seemed too
complicated.
Attachment #768547 - Flags: review?(seth)
Created attachment 768550 [details] [diff] [review]
part 2: remove the imgFrame* helper functions and call the raw data pointer versions explicitly
Attachment #768550 - Flags: review?(seth)
Created attachment 768552 [details] [diff] [review]
part 3: don't change the contents of mFrames

This removes a fast path in favour of keeping mFrames immutable.
Attachment #768552 - Flags: review?(seth)
Created attachment 768553 [details] [diff] [review]
part 4: mark source frames as const so we don't accidentally modify them

This should make part 3 have more teeth.
Attachment #768553 - Flags: review?(seth)
Whiteboard: [tech-p2] → [tech-p2][leave open]
Comment on attachment 768547 [details] [diff] [review]
part 1: Store a frame's raw image data pointer beside its imgFrame pointer so we can access it without having to lock the frame

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

::: image/src/FrameBlender.cpp
@@ +95,5 @@
>      mFrames.InsertElementAt(framenum, aFrame);
> +    if (GetNumFrames() > 1) {
> +      // Whenever we have more than one frame, we always lock *all* our frames
> +      // so we have all the image data pointers.
> +      mFrames[framenum].LockAndGetData();

Could you just call InsertFrame here? The only difference seems to be the call to EnsureAnimExists.

::: image/src/FrameBlender.h
@@ +25,5 @@
> + */
> +class FrameDataPair
> +{
> +public:
> +  FrameDataPair(imgFrame* frame)

This constructor allows implicit conversions, which I think you intend, but just FYI..

@@ +87,5 @@
> +  {
> +    return mFrame;
> +  }
> +
> +  operator imgFrame*() const

I'm not a big fan of implicit conversions but it's up to you.
Attachment #768547 - Flags: review?(seth) → review+
Comment on attachment 768550 [details] [diff] [review]
part 2: remove the imgFrame* helper functions and call the raw data pointer versions explicitly

This needs some revision and a bit of code movement to the earlier patch. New patch coming soon.
Attachment #768550 - Attachment is obsolete: true
Attachment #768550 - Flags: review?(seth)
Created attachment 769090 [details] [diff] [review]
part 1: Store a frame's raw image data pointer beside its imgFrame pointer so we can access it without having to lock the frame

This patch fixes the problems on Try, and also makes the FrameDataPair imgFrame* constructor explicit. (I originally wanted implicit conversions, but then realized that they made it possible for an utterly-unsafe "FrameDataPair = const FrameDataPair" to happen through the implicit operator imgFrame*().

The main change to this over the previous version is fixing SwapFrame() to fix the crashes on try, and rolling parts of part 2 into this patch (i.e., changing mAnim->compositingFrame and compositingPrevFrame to FrameDataPairs instead).
Attachment #768547 - Attachment is obsolete: true
Attachment #769090 - Flags: review?(seth)
Created attachment 769091 [details] [diff] [review]
part 2: remove the imgFrame* helper functions and call the raw data pointer versions explicitly
Attachment #769091 - Flags: review?(seth)
(Parts 3 and 4 remain the same)
Depends on: 888499
Comment on attachment 769090 [details] [diff] [review]
part 1: Store a frame's raw image data pointer beside its imgFrame pointer so we can access it without having to lock the frame

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

::: image/src/FrameBlender.cpp
@@ +139,5 @@
>  
> +  FrameDataPair& prevFrame = mFrames[aPrevFrameIndex];
> +  FrameDataPair& nextFrame = mFrames[aNextFrameIndex];
> +  if (!prevFrame.GetFrame() || !prevFrame.GetFrameData() ||
> +      !nextFrame.GetFrame() || !nextFrame.GetFrameData()) {

Is it a valid state for frame.GetFrameData() to return non-null and frame.GetFrame() to return null? Seems like you can just check GetFrameData here. Or maybe encapsulate this check in a HasFrameData method that asserts FrameDataPair::mFrame is non-null.

::: image/src/FrameBlender.h
@@ +92,5 @@
> +  // of the frame, deleting the old frame (if any).
> +  void SetFrame(imgFrame* frame)
> +  {
> +    mFrame = frame;
> +    mFrameData = nullptr;

Should you call UnlockImageData on the old mFrame here?
Attachment #769090 - Flags: review?(seth) → review+
Comment on attachment 769091 [details] [diff] [review]
part 2: remove the imgFrame* helper functions and call the raw data pointer versions explicitly

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

::: image/src/FrameBlender.cpp
@@ +277,5 @@
>          if (needToBlankComposite) {
>            // If we just created the composite, it could have anything in its
>            // buffer. Clear whole frame
> +          ClearFrame(mAnim->compositingFrame.GetFrameData(),
> +                     mAnim->compositingFrame.GetFrame()->GetRect());

Can ClearFrame just take a reference to a FrameDataPair, since all of its arguments are reachable from one?

@@ +298,5 @@
>          if (mAnim->compositingPrevFrame) {
> +          CopyFrameImage(mAnim->compositingPrevFrame.GetFrameData(),
> +                         mAnim->compositingPrevFrame.GetFrame()->GetRect(),
> +                         mAnim->compositingFrame.GetFrameData(),
> +                         mAnim->compositingFrame.GetFrame()->GetRect());

Same goes for CopyFrameImage - seems like it could just take two FrameDataPair references.
Attachment #769091 - Flags: review?(seth) → review+
Attachment #768552 - Flags: review?(seth) → review+
Attachment #768553 - Flags: review?(seth) → review+
(In reply to Seth Fowler [:seth] from comment #21)
> ::: image/src/FrameBlender.h
> @@ +92,5 @@
> > +  // of the frame, deleting the old frame (if any).
> > +  void SetFrame(imgFrame* frame)
> > +  {
> > +    mFrame = frame;
> > +    mFrameData = nullptr;
> 
> Should you call UnlockImageData on the old mFrame here?

Yes, definitely! Nice catch.
Depends on: 795737
Created attachment 770486 [details] [diff] [review]
part 5: make frames be owned by a separate object, FrameSequence

This moves frame ownership into a separate object, FrameSequence, with the eventual goal being that we are able to share FrameSequences. (And we'll be able to have FrameSequences created by IPDL, backed by shared memory.)
Attachment #770486 - Flags: review?(seth)
Created attachment 770488 [details] [diff] [review]
part 5: make frames be owned by a separate object, FrameSequence

once more, with feeling (and qrefresh)
Attachment #770486 - Attachment is obsolete: true
Attachment #770486 - Flags: review?(seth)
Attachment #770488 - Flags: review?(seth)
Created attachment 770970 [details] [diff] [review]
part 5: make frames be owned by a separate object, FrameSequence

Slight fix: return an empty FrameDataPair when you ask for a frame that doesn't exist.
Attachment #770488 - Attachment is obsolete: true
Attachment #770488 - Flags: review?(seth)
Attachment #770970 - Flags: review?(jmuizelaar)
(In reply to Seth Fowler [:seth] from comment #22)
> Can ClearFrame just take a reference to a FrameDataPair, since all of its
> arguments are reachable from one?

It can, but I really don't want it to. This way we're sure we only operate on the char* (and const char*) and never operate on the imgFrame, which is safer.
Comment on attachment 770970 [details] [diff] [review]
part 5: make frames be owned by a separate object, FrameSequence

er, wtf, self.
Attachment #770970 - Flags: review?(jmuizelaar) → review?(seth)
Created attachment 771017 [details] [diff] [review]
part 6: make FrameSequences be refcounted

This makes it possible for us to share FrameSequences by refcounting them. We don't do anything smart when inserting/removing/swapping frames, but we do carefully handle the "discard" case (by just reallocating a new FrameSequence).

Note that, currently, nothing actually *shares* FrameSequences.
Attachment #771017 - Flags: review?(seth)
Comment on attachment 770488 [details] [diff] [review]
part 5: make frames be owned by a separate object, FrameSequence

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

Nice! The code ends up a lot cleaner.

::: image/src/FrameBlender.cpp
@@ +93,3 @@
>      mAnim->lastCompositedFrameIndex = -1;
> +    mFrames.RemoveFrame(framenum);
> +    mFrames.InsertFrame(framenum, aFrame);

Note that this can also be handled with SwapFrame; one can just drop the return value on the floor. (I bring this up in relation to the changes I recommended in FrameSequence.cpp.)

::: image/src/FrameSequence.cpp
@@ +32,5 @@
> +
> +void
> +FrameSequence::RemoveFrame(uint32_t framenum)
> +{
> +  NS_ABORT_IF_FALSE(framenum < mFrames.Length(), "Deleting invalid frame!");

It's not a big deal, but I suggest we move towards standardizing on MOZ_ASSERT rather than NS_ABORT_IF_FALSE.

@@ +46,5 @@
> +  mFrames.Clear();
> +}
> +
> +void
> +FrameSequence::InsertFrame(uint32_t framenum, imgFrame* aFrame)

Do we need to insert frames in arbitrary order, or would an AppendFrame method be enough? (I realize you do need this for SwapFrame, but see the comment below.)

@@ +77,5 @@
> +  }
> +
> +  mFrames.RemoveElementAt(framenum);
> +  if (aFrame) {
> +    InsertFrame(framenum, aFrame);

This is rather expensive since AFAICT all the array elements after framenum are going to get copied once due to the removal and then copied a second time due to the insertion. Is there a reason to prefer this approach over swapping the _value_ of the element at framenum? Could use e.g. ReplaceElementAt.
Attachment #770488 - Attachment is obsolete: false
Attachment #770488 - Attachment is obsolete: true
Comment on attachment 770970 [details] [diff] [review]
part 5: make frames be owned by a separate object, FrameSequence

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

We had a midair collision with reviewing. I don't see anything substantially different here from what I reviewed, so moving over the r+ from the other review. See the comments there.
Attachment #770970 - Flags: review?(seth) → review+
Comment on attachment 771017 [details] [diff] [review]
part 6: make FrameSequences be refcounted

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

Looks good!

::: image/src/FrameBlender.h
@@ +32,5 @@
> +   * Create a new FrameBlender with a given frame sequence.
> +   *
> +   * If aSequenceToUse is not specified, it will be allocated automatically.
> +   */
> +  FrameBlender(FrameSequence* aSequenceToUse = nullptr);

Not sure how callers will use this; maybe it would be of benefit to have a constructor that takes an already_AddRefed<FrameSequence>?
Attachment #771017 - Flags: review?(seth) → review+
(In reply to Seth Fowler [:seth] from comment #32)
> Not sure how callers will use this; maybe it would be of benefit to have a
> constructor that takes an already_AddRefed<FrameSequence>?

Callers will use it like

nsRefPtr<FrameSequence> seq = someFrameBlender->GetFrameSequence();
FrameBlender* newBlender = new FrameBlender(seq);

newBlender implicitly adds a reference, and then the getter's reference gets dropped when seq goes out of scope.

Passing an already_AddRefed as a parameter is really weird in my gecko experience.
Attachment #768552 - Flags: checkin+
Attachment #768553 - Flags: checkin+
Attachment #769090 - Flags: checkin+
Attachment #769091 - Flags: checkin+
(In reply to Seth Fowler [:seth] from comment #30)

> > +    mFrames.RemoveFrame(framenum);
> > +    mFrames.InsertFrame(framenum, aFrame);
> 
> Note that this can also be handled with SwapFrame; one can just drop the
> return value on the floor.

Almost on the floor - you have to delete it, but nsAutoPtr<imgFrame> toDelete = SwapFrame() works.

> It's not a big deal, but I suggest we move towards standardizing on
> MOZ_ASSERT rather than NS_ABORT_IF_FALSE.

Agree, though this is simply moving existing code.

> Do we need to insert frames in arbitrary order, or would an AppendFrame
> method be enough? (I realize you do need this for SwapFrame, but see the
> comment below.)

Not necessarily in arbitrary order, but we do need to be able to change existing frames. *Only* append isn't enough.


> 
> @@ +77,5 @@
> > +  }
> > +
> > +  mFrames.RemoveElementAt(framenum);
> > +  if (aFrame) {
> > +    InsertFrame(framenum, aFrame);
> 
> This is rather expensive since AFAICT all the array elements after framenum
> are going to get copied once due to the removal and then copied a second
> time due to the insertion. Is there a reason to prefer this approach over
> swapping the _value_ of the element at framenum? Could use e.g.
> ReplaceElementAt.

I think this'll work, yes.
Attachment #770970 - Flags: checkin+
Attachment #771017 - Flags: checkin+
Created attachment 774685 [details] [diff] [review]
part 7: move image animation logic into its own class, FrameAnimator

This patch moves the logic of moving from one frame to another (and tracking what frame is current, etc) to a separate class, FrameAnimator. Deciding *whether* to animate, and actually calling that animation code, is left to RasterImage, but the animation itself is driven by FrameAnimator.
Attachment #774685 - Flags: review?(seth)
Comment on attachment 774685 [details] [diff] [review]
part 7: move image animation logic into its own class, FrameAnimator

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

Love it! Feels much cleaner.

::: image/src/FrameAnimator.cpp
@@ +58,5 @@
> +  if (timeout < 0) {
> +    // We need to return a sentinel value in this case, because our logic
> +    // doesn't work correctly if we have a negative timeout value. The reason
> +    // this positive infinity was chosen was because it works with the loop in
> +    // RequestRefresh() above.

This comment looks like it needs an update.

::: image/src/FrameAnimator.h
@@ +65,5 @@
> +  /**
> +   * Call when this image is finished decoding so we know that there aren't any
> +   * more frames coming.
> +   */
> +  void SetDoneDecoding(bool done);

Nit: aDone.

@@ +143,5 @@
> +   * @returns a RefreshResult that shows whether the frame was successfully
> +   *          advanced. Note: If the frame wasn't advanced, then aDirtyRect
> +   *          will remain unmodified.
> +   */
> +  RefreshResult AdvanceFrame(mozilla::TimeStamp aTime, nsIntRect* aDirtyRect);

Wait, doesn't RefreshResult contain a dirtyRect as well? (Which I note doesn't appear to be touched anywhere except for Accumulate.) Probably should just use that here instead of having an out parameter, but if you can't use RefreshResult::dirtyRect then it looks like it can probably be removed.

::: image/src/imgFrame.cpp
@@ +801,2 @@
>  {
> +  MutexAutoLock lock(mDirtyMutex);

IMO this is the kinda of situation where 'mutable' is appropriate. This method is still logically const, and you could keep it const if mDirtyMutex was mutable. Up to you though.
Attachment #774685 - Flags: review?(seth) → review+
Blocks: 893930
(In reply to Seth Fowler [:seth] from comment #40)
> Wait, doesn't RefreshResult contain a dirtyRect as well? (Which I note
> doesn't appear to be touched anywhere except for Accumulate.) Probably
> should just use that here instead of having an out parameter, but if you
> can't use RefreshResult::dirtyRect then it looks like it can probably be
> removed.

Yikes. And this, boys and girls, is why we have review.
Comment on attachment 774685 [details] [diff] [review]
part 7: move image animation logic into its own class, FrameAnimator

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8d855c4edb
Attachment #774685 - Flags: checkin+
Blocks: 546837

Comment 44

5 years ago
I think these patches broke transparent, animated gifs for me:

https://dl.dropboxusercontent.com/u/34392223/gif-test.html
Created attachment 780051 [details] [diff] [review]
sketch of part 8: create PAnimatedImage protocol, managed by PCompositor

This is a sketch of a patch that would make it so we could register an animated image with every compositor that comes by. It'd also listen for when the compositors go away, by way of overriding PAnimatedImage::ActorDestroy, and deregister them at that point.

Right now I'm just looking to see (from jdm and bent) whether I totally misunderstand anything with IPDL/ipc here, because it's been a while since I've done it, and I've never done anything this complex.
Attachment #780051 - Flags: feedback?(josh)
Attachment #780051 - Flags: feedback?(bent.mozilla)
Why do you need new IPDL stuff?

You should be able to use an ImageContainer and call SetCurrentImage from your animated-image thread, and the existing code should take care of all IPC stuff for you.
Comment on attachment 780051 [details] [diff] [review]
sketch of part 8: create PAnimatedImage protocol, managed by PCompositor

Looks like this is missing some files...
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #46)
> Why do you need new IPDL stuff?
> 
> You should be able to use an ImageContainer and call SetCurrentImage from
> your animated-image thread, and the existing code should take care of all
> IPC stuff for you.

We aren't creating an animated-image thread. A thread is relatively heavyweight, for one, and it's unnecessary in this case. The current plan, as outlined in comment 11, is to tell Compositors about the animated images directly (very analagous to the way CSS animations are driven). This also means that we won't have to share blended animated images from the thread to the compositor; the compositor will just upload them directly.

Using an ImageContainer also implies that we're going to have more memory overhead because of the multiple blended image surfaces it implies.

(In reply to ben turner [:bent] from comment #47)
> Comment on attachment 780051 [details] [diff] [review]
> sketch of part 8: create PAnimatedImage protocol, managed by PCompositor
> 
> Looks like this is missing some files...

Yeah, doh. Coming up.
(In reply to silverwind from comment #44)
> I think these patches broke transparent, animated gifs for me:
> 
> https://dl.dropboxusercontent.com/u/34392223/gif-test.html

Silverwind, can you file a bug showing what is broken about this? For me, on OS X, it looks the same before and after.
Created attachment 780397 [details] [diff] [review]
sketch of part 8: create PAnimatedImage protocol, managed by PCompositor
Attachment #780051 - Attachment is obsolete: true
Attachment #780051 - Flags: feedback?(josh)
Attachment #780051 - Flags: feedback?(bent.mozilla)
Attachment #780397 - Flags: feedback?(josh)
Attachment #780397 - Flags: feedback?(bent.mozilla)

Updated

5 years ago
Depends on: 897488
No longer depends on: 897488
Depends on: 897488
(In reply to Joe Drew (:JOEDREW! \o/) from comment #48)
> We aren't creating an animated-image thread. A thread is relatively
> heavyweight, for one, and it's unnecessary in this case. The current plan,
> as outlined in comment 11, is to tell Compositors about the animated images
> directly (very analagous to the way CSS animations are driven). This also
> means that we won't have to share blended animated images from the thread to
> the compositor; the compositor will just upload them directly.

Oh, I totally missed that, sorry.
Comment on attachment 780397 [details] [diff] [review]
sketch of part 8: create PAnimatedImage protocol, managed by PCompositor

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

::: gfx/layers/ipc/AnimatedImageParent.h
@@ +15,5 @@
> +class AnimatedImageParent : public PAnimatedImageParent
> +{
> +public:
> +  virtual bool
> +  RecvAddFrame(const int& frameNumber, const SurfaceDescriptor& newFrame);

Make sure you add MOZ_OVERRIDE everywhere you can

::: gfx/layers/ipc/AnimatedImageTracker.cpp
@@ +41,5 @@
> +      return;
> +    }
> +  }
> +
> +  AnimatedImageChild* image = static_cast<AnimatedImageChild*>(aCompositor->SendPAnimatedImageConstructor());

This could be made nicer as follows:

  AnimatedImageChild* image = new AnimatedImageChild(this);
  aCompositor->SendPAnimatedImageConstructor(image);

  AnimatedImageCompositorPair pair = { image, aCompositor };
  mImages.AppendElement(pair);

Then you could lose the SetTracker function too.

::: gfx/layers/ipc/PCompositor.ipdl
@@ +67,5 @@
> +  PAnimatedImage();
> +
> +  // Deregister a previously-created animated image that was registered with
> +  // this compositor.
> +  DeregisterAnimatedImage(PAnimatedImage image);

There's no real need for this message, just use the destructor to do this work.
Attachment #780397 - Flags: feedback?(bent.mozilla) → feedback+
Depends on: 899325
Created attachment 783290 [details] [diff] [review]
Part 8: Add PAnimatedImage, a representation of an animated image the compositor can use

This patch adds an IPDL protocol, PAnimatedImage (mostly just stubbed out) that Compositors can use to track animated images. RasterImage creates it when animated image layers are enabled, and tells the compositor about itself. In future patches, RasterImage will send its frames to its PAnimatedImage instances; in still further future patches, compositors with registered PAnimatedImages will drive their animation from their event loop.
Attachment #780397 - Attachment is obsolete: true
Attachment #780397 - Flags: feedback?(josh)
Attachment #783290 - Flags: review?(seth)
Attachment #783290 - Flags: review?(gps)
Attachment #783290 - Flags: review?(bent.mozilla)
This patch doesn't work without the patch in bug 845982. And as for reviews: Seth for imagelib, Ben for IPC/IPDL, gps for Makefile.in.
Depends on: 845982
No longer depends on: 129986, 332973
Comment on attachment 783290 [details] [diff] [review]
Part 8: Add PAnimatedImage, a representation of an animated image the compositor can use

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

Only covers build bits.
Attachment #783290 - Flags: review?(gps) → review+

Updated

5 years ago
Depends on: 899861
Comment on attachment 783290 [details] [diff] [review]
Part 8: Add PAnimatedImage, a representation of an animated image the compositor can use

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

Looks good so far overall.

::: image/src/RasterImage.h
@@ +701,5 @@
>    // we can mark it as stopped if necessary. The ScaleWorker/DrawWorker duo
>    // will inform us when to let go of this pointer.
>    ScaleRequest* mScaleRequest;
>  
> +  mozilla::layers::AnimatedImageTracker mCompositorTracker;

I find the names in use here a bit confusing. Does this thing track compositors or animated images? =) I suggest that the fact that the type name and the variable name you wanted to use are so different means that there is a naming problem here. I suspect the name of the type is the problem but whatever you may decide I think a change is warranted.
Attachment #783290 - Flags: review?(seth) → review+
Created attachment 783996 [details] [diff] [review]
Part 8: Add PAnimatedImage, a representation of an animated image the compositor can use

This is an updated version of part 8. It adds a couple of things we need, namely the fact that frames will need disposal and blend methods, and that images themselves need sizes. I'm also retargeting Ben's review at Matt, since that feels more appropriate anyways.
Attachment #783290 - Attachment is obsolete: true
Attachment #783290 - Flags: review?(bent.mozilla)
Attachment #783996 - Flags: review?(matt.woodrow)
Attachment #783996 - Flags: review?(bent.mozilla)
Comment on attachment 783996 [details] [diff] [review]
Part 8: Add PAnimatedImage, a representation of an animated image the compositor can use

I also renamed AnimatedImageTracker to AnimatedImageRegistrar, and renamed the RasterImage member mAnimatedImageRegistrar.
Attachment #783996 - Flags: review?(bent.mozilla)
Comment on attachment 783996 [details] [diff] [review]
Part 8: Add PAnimatedImage, a representation of an animated image the compositor can use

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

::: gfx/layers/ipc/PAnimatedImage.ipdl
@@ +14,5 @@
> +{
> +  manager PCompositor;
> +
> +parent:
> +  AddFrame(int frameNumber, SurfaceDescriptor newFrame, int disposal, int blend);

Comments about what the parameters mean (especially the last two) would be nice.
Attachment #783996 - Flags: review?(matt.woodrow) → review+
Created attachment 785398 [details] [diff] [review]
(in progress) Make a AnimatedImageFrame, and pass it around
Assignee: joe → nobody
"Move all image animation logic into a new class, FrameAnimator, and use it from RasterImage." seems to have caused bug 899861. I'd suggest backing that part out for now.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #62)
> "Move all image animation logic into a new class, FrameAnimator, and use it
> from RasterImage." seems to have caused bug 899861. I'd suggest backing that
> part out for now.

Probably a good idea. I'll prepare a minimal patch.
Created attachment 790491 [details] [diff] [review]
part 7b: Change the test for the "need to wait to display" frame. Do not land.

The patch needs to be rebased before it lands, as the part 7 above was (mostly) backed out with the bug 899861, but this is basically what the change to part 7 would have been, had that back out not happened.
Attachment #790491 - Flags: review?(seth)
Comment on attachment 774685 [details] [diff] [review]
part 7: move image animation logic into its own class, FrameAnimator

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

::: image/src/FrameAnimator.cpp
@@ +86,5 @@
> +  // If we're done decoding, we know we've got everything we're going to get.
> +  // If we aren't, we only display fully-downloaded frames; everything else
> +  // gets delayed.
> +  bool needToWait = !mDoneDecoding &&
> +                    mFrameBlender.RawGetFrame(nextFrameIndex) &&

I'm not sure this line is useful in protecting us from a crash; FrameBlender::RawGetFrame(uint32_t) calls FrameSequence::GetFrame(uint32) which returns an "empty" const FrameDataPair& if the index value is out of range. That should always be "true", even if the FrameDataPair has a null image pointer.  In that case, however, we would end up calling ImageComplete() on a nullptr on the next line.
Depends on: 909559
(Assignee)

Comment 66

5 years ago
Milan, can you please find an owner for this. We should drive this in.
(Assignee)

Comment 67

5 years ago
Part 1-7 have landed.
(Assignee)

Comment 68

5 years ago
I wonder whether it would be a simpler design to check that a rasterimage satisfies certain criteria (not too large, etc), and if so

1. Allocate a layer that is N times the size of the image, with N == blended frames in the animation.
2. Attach an async transform animation to the layer that shows the right part of the layer using the right timings.
3. Throw away all decoded images since we won't need them.

When we switch away from a document we already destroy the layer tree. We could do that in addition also for low memory situations, which forces it to be rebuilt but frees up the memory used here.

roc, vlad, what do you think?
Flags: needinfo?(vladimir)
Flags: needinfo?(roc)
(Assignee)

Comment 69

5 years ago
I am interested in hacking on this in my (essentially non-existent) spare time. This is an explicit invitation to steal this from me at any time for any reason without asking, since I will likely have very little time to actually work on this.
Assignee: nobody → gal
I don't think it would be much work to just ship all the blended images across and animate changing which one of them is current. Probably easier (and much simpler to understand) than texture atlasing.

However, I believe one of the initial goals was to avoid having all the blended frames around, to save memory. Is this no longer an issue, or do you think that the size/frame count restraints will be enough to prevent excessive memory usage?
(Assignee)

Comment 71

5 years ago
The reason I like my plan is that I basically don't need any changes to the compositor. We already have all the pieces. I don't even need a new layer type. I can use an image layer or a thebes layer and put a transform animation sequence on it. I am a big fan of reusing abstractions instead of further complicating the compositor interface. What do you think?

As for blending, I guess I could go and run some analysis on this but my initial reflex is to do the easy thing until its not good enough. Main main motivation to look for this bug is the desktop loading-the-page throbber on top of the tab. It is animated with an offensively low 30fps that just bugs the shit out of me (we probably chose 30fps to avoid yank). I would sleep a lot sounder if that was off the main thread and 60fps.

I will go and collect some data.
For that particular case, couldn't we just use an OMT animated transform? It's just a rotating image.
Animated transform would likely look smoother animation wise, but we'd want to see what the actual image looked like when transformed.

I don't have a strong opinion on whether the texture atlasing proposal is good or not.  A layers abstraction that acts like a "switch" seems useful in general, and is common in tree-based render graphs, so I think it would get reuse.  But if it's not a lot of work to try the texture atlasing bit, it might be worth doing so.  You'll still have to extend the image layer to know that it's animated and how to get the right frame based on time though.
Flags: needinfo?(vladimir)
(In reply to Matt Woodrow (:mattwoodrow) from comment #72)
> For that particular case, couldn't we just use an OMT animated transform?
> It's just a rotating image.

See bug 759252 which I worked on to do just that. If the issues brought up in that bug are no longer real, then we can reopen it and land the patch that is attached to it.
(Assignee)

Comment 75

5 years ago
If we introduce an animated image layer, we probably want to do blending on the compositor. Also, I would in that case prefer for this to be atomic, so content sends over all decoded images at once, instead of having half-states where only some images are available.
(In reply to Jared Wein [:jaws] from comment #74)
> See bug 759252 which I worked on to do just that. If the issues brought up
> in that bug are no longer real, then we can reopen it and land the patch
> that is attached to it.

We should reland bug 759252 when OMTA is turned on for desktop everywhere.

I suspect the plan in comment #11 gets us to a better place than explicit atlasing. Although it requires code in the compositor, that code should be shared across all backends.
Flags: needinfo?(roc)

Updated

5 years ago
Depends on: 909589
Comment on attachment 783996 [details] [diff] [review]
Part 8: Add PAnimatedImage, a representation of an animated image the compositor can use

Talking with Matt on IRC, we decided that we probably won't want this. Instead we can drive the animation from a dedicated thread of each content process using existing ImageContainer interfaces.
Attachment #783996 - Attachment is obsolete: true
Adding ni?jeff in case he remembers a specific reason we had PAnimatedImage to drive the animation from the compositor side of things.
Flags: needinfo?(jmuizelaar)
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
tracking-e10s: --- → +
tracking-e10s: + → later
Does this bug cover eventually enabling layers.offmainthreadcomposition.async-animations or is this covered by another bug? The reason I ask is because if you set layers.offmainthreadcomposition.async-animations to true in Nightly builds, you can no longer click on any option in the right click menu, you have to use the arrow keys to navigate the menu and then press enter to select an option.
Pretty sure that pref is for CSS animations type things, not for the animated GIFs.
(In reply to Brian Carpenter [:geeknik] from comment #80)
> Does this bug cover eventually enabling
> layers.offmainthreadcomposition.async-animations or is this covered by
> another bug? The reason I ask is because if you set
> layers.offmainthreadcomposition.async-animations to true in Nightly builds,
> you can no longer click on any option in the right click menu, you have to
> use the arrow keys to navigate the menu and then press enter to select an
> option.

That's bug 980770. You should file that and mark it as a dependency.
(In reply to Matt Woodrow (:mattwoodrow) from comment #78)
> Adding ni?jeff in case he remembers a specific reason we had PAnimatedImage
> to drive the animation from the compositor side of things.

No, I don't remember anything.
Flags: needinfo?(jmuizelaar)
Doesn't comment 48 answer this? It sounds like the idea was that we could avoid having a separate thread so we wouldn't have to deal with sharing textures between that thread and the compositor thread.
You need to log in before you can comment on or make changes to this bug.