1337111 - Make animated image decoders generate full frames instead of partial

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Andrew Osmond [:aosmond] (he/him)]

When decoding animated images, we currently generate partial frames that must be composited by FrameAnimator::DoBlend. We should stop using RemoveFrameRectFilter and replace it with a new filter which allows us to spit out the completed frame. Since this will increase our memory footprint (no more paletted images), we will also need to resolve the outstanding memory management issues with animated images (i.e. release them from the cache like normal images, decode frames as needed, etc).

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add BlendAnimationFilter

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add BlendAnimationFilter, v2

Improve filter to use arch-specific Skia methods for performing the OVER operation. Also fix DoResetToFirstRow to work with deinterlace/ADAM7 filter. Note there is significant room for improvement in these cases because we copy the frame data all over again, even when we know only the area inside the frame rect could have changed! (This is an existing issue for RemoveFrameRectFilter as well, it rewrites all of the empty rows.)

Comment 3

[Tzuhao Kuo [:kaku]]

(In reply to Andrew Osmond [:aosmond] from comment #0)
> When decoding animated images, we currently generate partial frames that
> must be composited by FrameAnimator::DoBlend. We should stop using
> RemoveFrameRectFilter and replace it with a new filter which allows us to
> spit out the completed frame. Since this will increase our memory footprint
> (no more paletted images), we will also need to resolve the outstanding
> memory management issues with animated images (i.e. release them from the
> cache like normal images, decode frames as needed, etc).

Hi, Andrew, this is Kaku from Taipei Media team. The media team would like to make the animation images leverage our media playback framework (bug 1187118 and bug 1257388), and I have started to implement bug 1187118. Since the media playback does not cache frames so it should be able to solve the high memory footprint issue you mentioned here, right?

I already have some WIP patches for the bug 1187118, however, while I was working on it, I realized that the nsGIFDecoder2 does not generate full frames. Instead, it makes partial frames. The behavior does not fit our video decoding framework, and then your bug came just in time! So, I'm wondering that are you going to make the nsGIFDecoder2 itself have the ability to decode full frames?

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Tzuhao Kuo [:kaku] from comment #3)
> (In reply to Andrew Osmond [:aosmond] from comment #0)
> > When decoding animated images, we currently generate partial frames that
> > must be composited by FrameAnimator::DoBlend. We should stop using
> > RemoveFrameRectFilter and replace it with a new filter which allows us to
> > spit out the completed frame. Since this will increase our memory footprint
> > (no more paletted images), we will also need to resolve the outstanding
> > memory management issues with animated images (i.e. release them from the
> > cache like normal images, decode frames as needed, etc).
> 
> Hi, Andrew, this is Kaku from Taipei Media team. The media team would like
> to make the animation images leverage our media playback framework (bug
> 1187118 and bug 1257388), and I have started to implement bug 1187118. Since
> the media playback does not cache frames so it should be able to solve the
> high memory footprint issue you mentioned here, right?
> 
> I already have some WIP patches for the bug 1187118, however, while I was
> working on it, I realized that the nsGIFDecoder2 does not generate full
> frames. Instead, it makes partial frames. The behavior does not fit our
> video decoding framework, and then your bug came just in time! So, I'm
> wondering that are you going to make the nsGIFDecoder2 itself have the
> ability to decode full frames?

Tzuhau, yes it solves a big part of my problem :).

nsGIFDecoder2 will produce full frames when this additional surface pipe filter gets integrated. I have a WIP doing this, but as you noted, there are other bits in imagelib that I would need to fix before landing it. I briefly reviewed attachment 8840364 [details] and it looks like you will get what you need once I am ready to land what I have.

Given your need, I will see about potentially landing this sooner rather than later, but behind an additional decoder flag that the regular imagelib path won't use (which you could then pass in from GIFTrackDemuxer::CreateGIFDecoder).

Keep in mind, I think GIFDecoder/etc should be renamed to something like AnimatedImageDecoder (or otherwise have that as the base class), because PNGs can also be animated and the difference between nsGIFDecoder2 and nsPNGDecoder is trivial as far as decoder APIs go (just use Decoder instead of nsGIFDecoder2/nsPNGDecoder). Bug 1294490 also offers the possibility of WebP animated images in the future.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add BlendAnimationFilter, v3

Some minor fixes, dirty rect issues resolved (partly the filter, partly other temp testing code).

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add BlendAnimationFilter, v4

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add decoder flag to create blended frames instead of partial, v1

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Add tests, v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c6e97a13807be60f7244a4995975b8dc6bf687b

try, with a hack to make the regular decoding path use the new filter: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2abb1bda879117cc3149f054b8c1eab429bd5e6e

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add BlendAnimationFilter, v5

Fix broken mochitest, off-by-one row for the clear rect.

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

try, with a hack to make the regular decoding path use the new filter: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31a9e61815952fa0bee46df7e24ccb86199ef1a4

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Add tests, v1.1

Whoops, remove dead code copied from TestRemoveFrameRectFilter.cpp.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add BlendAnimationFilter, v6

Merge in bits from part 2 that should have been in part 1. Fix bugs encountered while improving test coverage in part 3.

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add decoder flag to create blended frames instead of partial, v2

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Add tests, v2

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8851723 [details] [diff] [review]
Part 3. Add tests, v2

Add new tests to cover edge cases.

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add BlendAnimationFilter, v7

Rebase.

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add decoder flag to create blended frames instead of partial, v3

Rebase.

Comment 18

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Add tests, v3

Comment 19

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Add pref to force decoding of full frames, disabled by default., v1

Add a pref to control RasterImage/FrameAnimator behaviour at runtime (requires restart), for ease of testing.

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

Attachment: [WIP] Advance animated image decoding on demand, rather than all at once., v1

This is just a related WIP, tracking here for now. It advances the animation decoding only on an as needed basis -- starts with 3 frames in its queue, and whenever the animation advances on the main thread, it requests another frame from the decoder. This still keeps around the old frames, so it only goes through the loop once. Up next -- add a dynamic threshold for this advancing-on-demand behavior (e.g. look at how much animation time we have advanced or how much memory we have consumed?), recreate the decoder to start over again when we hit the end to allow us to discard the old frames (and keep peak memory consumption low).

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Andrew Osmond [:aosmond] from comment #20)
> Created attachment 8892245 [details] [diff] [review]
> [WIP] Advance animated image decoding on demand, rather than all at once., v1
> 
> This is just a related WIP, tracking here for now. It advances the animation
> decoding only on an as needed basis -- starts with 3 frames in its queue,
> and whenever the animation advances on the main thread, it requests another
> frame from the decoder. This still keeps around the old frames, so it only
> goes through the loop once. Up next -- add a dynamic threshold for this
> advancing-on-demand behavior (e.g. look at how much animation time we have
> advanced or how much memory we have consumed?), recreate the decoder to
> start over again when we hit the end to allow us to discard the old frames
> (and keep peak memory consumption low).

On a related note, I notice on my machine that it constantly just gets one frame and stops decoding, as the decoder is able to keep up fairly well. Maybe batching up requests for multiple frames at a time would be half (want a high watermark of frames in the buffer at any given time, and request enough to get us there when we hit a low watermark?) to minimize task switching and maximize cache hits would be good idea?

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8892245 [details] [diff] [review]
[WIP] Advance animated image decoding on demand, rather than all at once., v1

This WIP is now tracked in bug 523950.

Comment 23

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8892241 [details] [diff] [review]
Part 1. Add BlendAnimationFilter, v7

As requested.

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1337111-Part-1.-Add-BlendAnimationFilter-to-allo.patch, v8

Rebase.

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1337111-Part-2.-Add-decoder-flag-to-request-comp.patch, v4

Rebase.

Comment 26

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1337111-Part-3.-Add-gtests-for-BlendAnimationFil.patch, v4

Rebase.

Comment 27

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0004-Bug-1337111-Part-4.-Add-pref-to-force-decoding-of-fu.patch, v2

Comment 28

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0005-Bug-1337111-Part-5.-Add-telemetry-for-animation-blen.patch, v1

Adds telemetry to track how much time we spend decoding and blending.

try (with patches, but disabled by default): https://treeherder.mozilla.org/#/jobs?repo=try&revision=4893848fb376850c3bb646d59077583c8c91f62f

try (with patches, enabled by default): https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d20160379782c0b14c2d31efed11fed60e105a

Comment 29

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0004-Bug-1337111-Part-4.-Add-pref-to-force-decoding-of-fu.patch, v3

Change pref to be a live pref instead of a once.

Comment 30

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0005-Bug-1337111-Part-5.-Add-telemetry-for-animation-blen.patch, v2

Rebased.

Comment 31

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0006-Bug-1337111-Part-6.-Use-shared-surfaces-for-full-ani.patch, v1

Switch to using shared surfaces for animated frames if WebRender is enabled and we are generating full frames. Since there was only one compositing frame before updated by FrameAnimator during the refresh tick on the main thread, we couldn't use shared memory to avoid copies for animated images. If we are generating full frames, this is no longer a problem, and we can treat them like normal images and only updating the image key in the display item (at least until we get async image pipelines working...).

Comment 32

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1337111-Part-2.-Add-an-image-decoder-flag-to-req.patch, v5

Rebase, it has some merge conflicts.

Comment 33

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0004-Bug-1337111-Part-4.-Add-pref-to-force-decoding-of-fu.patch, v4

Fixed a bug with WebRender. Technically FrameAnimator's compositing frame isn't a full frame, since it has multiple finish calls. This is a problem because full frames will be put into a shared surface, which are expected to be immutable once the initial decode is complete. This was only a problem when blend animation was *disabled* since FrameAnimator blending isn't used otherwise.

Comment 34

[Andrew Osmond [:aosmond] (he/him)]

I thought I had regressed APNGs with this patch, but it seems although it renders differently, it is actually correct. It is the current implementation that is wrong.

https://philip.html5.org/tests/apng/tests.html#apng-blend-op-over-repeatedly-with-nearly-transparent-colours

Chrome and Firefox + these patches + enable pref render the same solid green rectangle. Firefox today renders it with two different colours, one side gradually growing darker (perhaps some error is accumulating?).

Comment 35

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1337111-Part-1.-Add-BlendAnimationFilter-to-allo.patch, v9

Rebased on top of bug 1462355. To avoid adding yet more mutex locks, which the original patch did, I had to rejig some things. You'll need to look at part 2 in conjunction with part 1 because there are some method changes it depends on in the decoder. Before it was cleanly standalone but I had to couple it more tightly to the decoder to avoid the locks. But this forces me to write gtests that also test the decoder's portion, which is good :).

Comment 36

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1337111-Part-2.-Add-an-image-decoder-flag-to-req.patch, v6

Comment 37

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1337111-Part-3.-Land-groundwork-for-new-blended-.patch, v5

I split the original part 3 in two. The new part 3 just contains the changes to the test framework to allow me to write new tests for the blend animation filter. The new part 4 contains the actual tests.

Comment 38

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0004-Bug-1337111-Part-4.-Add-gtests-for-BlendAnimationFil.patch, v5

Comment 39

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0005-Bug-1337111-Part-5.-Add-pref-to-force-decoding-of-fu.patch, v5

Comment 40

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0006-Bug-1337111-Part-6.-Use-shared-surfaces-for-full-ani.patch, v2

I removed the telemetry changes. I've done enough profiling on Linux/Mac to be reasonable confident I know the pain points.

Comment 41

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1337111-Part-1.-Add-BlendAnimationFilter-to-allo.patch, v10

Comment 42

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1337111-Part-1.-Add-BlendAnimationFilter-to-allo.patch, v11

Whoops wrong one.

Comment 43

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1337111-Part-2.-Add-an-image-decoder-flag-to-req.patch, v7

Comment 44

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1337111-Part-3.-Land-groundwork-for-new-blended-.patch, v6

Comment 45

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0004-Bug-1337111-Part-4.-Add-gtests-for-BlendAnimationFil.patch, v6

Comment 46

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0005-Bug-1337111-Part-5.-Add-pref-to-force-decoding-of-fu.patch, v6

Comment 47

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0006-Bug-1337111-Part-6.-Use-shared-surfaces-for-full-ani.patch, v3

Comment 48

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8985458 [details] [diff] [review]
0005-Bug-1337111-Part-5.-Add-pref-to-force-decoding-of-fu.patch, v6

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

::: image/FrameAnimator.cpp
@@ +487,5 @@
>      }
>    }
>  
>    // Advanced to the correct frame, the composited frame is now valid to be drawn.
> +  if (ret.mFrameAdvanced && aState.mCompositedFrameInvalid) {

Why is the change from "currentFrameEndTime > aTime" to "ret.mFrameAdvanced" needed?

::: image/imgFrame.cpp
@@ +235,5 @@
>                           SurfaceFormat aFormat,
>                           uint8_t aPaletteDepth /* = 0 */,
>                           bool aNonPremult /* = false */,
> +                         const Maybe<AnimationParams>& aAnimParams /* = Nothing() */,
> +                         bool aIsFullFrame /* = true */)

Something seems wrong here. In the header you make the default false for aIsFullFrame, but it's true here in the comment. And as far as I can tell you only pass the non default for this new parameter in InitForAnimator.

Comment 49

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8985458 [details] [diff] [review]
0005-Bug-1337111-Part-5.-Add-pref-to-force-decoding-of-fu.patch, v6

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

::: image/imgFrame.h
@@ +321,5 @@
>    uint8_t*     mPalettedImageData;
>    uint8_t      mPaletteDepth;
>  
>    bool mNonPremult;
> +  bool mIsFullFrame;

Is the point of this variable just so FrameAnimator knows not to blend this frame?

Comment 50

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8985452 [details] [diff] [review]
0002-Bug-1337111-Part-2.-Add-an-image-decoder-flag-to-req.patch, v7

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

::: image/imgFrame.h
@@ +302,1 @@
>  

Can we define what these 4 rects are in comments here? Getting a little confusing.

Comment 51

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1337111-Part-1.-Add-BlendAnimationFilter-to-allo.patch, v12

Comment 52

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1337111-Part-2.-Add-an-image-decoder-flag-to-req.patch, v8

(In reply to Timothy Nikkel (:tnikkel) from comment #50)
> Comment on attachment 8985452 [details] [diff] [review]
> 0002-Bug-1337111-Part-2.-Add-an-image-decoder-flag-to-req.patch, v7
> 
> Review of attachment 8985452 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/imgFrame.h
> @@ +302,1 @@
> >  
> 
> Can we define what these 4 rects are in comments here? Getting a little
> confusing.

Done. I think mImageSize is actually redundant but I'll deal with that separately.

Comment 53

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1337111-Part-3.-Land-groundwork-for-new-blended-.patch, v7

Comment 54

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0004-Bug-1337111-Part-4.-Add-gtests-for-BlendAnimationFil.patch, v7

Comment 55

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0005-Bug-1337111-Part-5.-Add-pref-to-force-decoding-of-fu.patch, v7

(In reply to Timothy Nikkel (:tnikkel) from comment #49)
> Comment on attachment 8985458 [details] [diff] [review]
> 0005-Bug-1337111-Part-5.-Add-pref-to-force-decoding-of-fu.patch, v6
> 
> Review of attachment 8985458 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/imgFrame.h
> @@ +321,5 @@
> >    uint8_t*     mPalettedImageData;
> >    uint8_t      mPaletteDepth;
> >  
> >    bool mNonPremult;
> > +  bool mIsFullFrame;
> 
> Is the point of this variable just so FrameAnimator knows not to blend this
> frame?

Yes.

(In reply to Timothy Nikkel (:tnikkel) from comment #48)
> Comment on attachment 8985458 [details] [diff] [review]
> 0005-Bug-1337111-Part-5.-Add-pref-to-force-decoding-of-fu.patch, v6
> 
> Review of attachment 8985458 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/FrameAnimator.cpp
> @@ +487,5 @@
> >      }
> >    }
> >  
> >    // Advanced to the correct frame, the composited frame is now valid to be drawn.
> > +  if (ret.mFrameAdvanced && aState.mCompositedFrameInvalid) {
> 
> Why is the change from "currentFrameEndTime > aTime" to "ret.mFrameAdvanced"
> needed?
> 

I updated this with a better condition (I realized there was a bug) and hopefully a better explanation. Advancing in of itself doesn't mean we updated the composited frame anymore because we might be producing full frames.

> ::: image/imgFrame.cpp
> @@ +235,5 @@
> >                           SurfaceFormat aFormat,
> >                           uint8_t aPaletteDepth /* = 0 */,
> >                           bool aNonPremult /* = false */,
> > +                         const Maybe<AnimationParams>& aAnimParams /* = Nothing() */,
> > +                         bool aIsFullFrame /* = true */)
> 
> Something seems wrong here. In the header you make the default false for
> aIsFullFrame, but it's true here in the comment. And as far as I can tell
> you only pass the non default for this new parameter in InitForAnimator.

Yes the comment was wrong. If you look a little further down, it only uses that parameter when it has the AnimationParams -- the first frame is always considered a full frame, so it will imply aIsFullFrame in that case.

It is not just set to true in InitForAnimator. It is also set in Decoder::AllocateFrameInternal and it is set to ShouldBlendAnimation which is true if the decoder is producing full frames.

Comment 56

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0006-Bug-1337111-Part-6.-Use-shared-surfaces-for-full-ani.patch, v4

Comment 57

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1337111 - Part 6. Use shared surfaces for full animated frames for WebRender.

Comment 58

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #55)
> > ::: image/FrameAnimator.cpp
> > @@ +487,5 @@
> > >      }
> > >    }
> > >  
> > >    // Advanced to the correct frame, the composited frame is now valid to be drawn.
> > > +  if (ret.mFrameAdvanced && aState.mCompositedFrameInvalid) {
> > 
> > Why is the change from "currentFrameEndTime > aTime" to "ret.mFrameAdvanced"
> > needed?
> > 
> 
> I updated this with a better condition (I realized there was a bug) and
> hopefully a better explanation. Advancing in of itself doesn't mean we
> updated the composited frame anymore because we might be producing full
> frames.

Hmm, okay. So the name of mCompositedFrameInvalid may be misleading now. It only tracks whether we should draw the animation frame at mCurrentAnimationFrameIndex (whether it comes from mCompositedFrame or from the surface cache because the image has full frames that don't need to be blended, either way). It might be better to be called mShouldDrawThisImageRightNow (flipping true/false).

Before we switched to pausing non-visible animated images we needed it for the case where an image was in a background tab, then it became the active tab: we would "fast forward" the image until it got to the point in the timeline where it should have been. Which looks bad. It's better to draw nothing.

Was this causing a bug?

Comment 59

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8999025 [details] [diff] [review]
0002-Bug-1337111-Part-2.-Add-an-image-decoder-flag-to-req.patch, v8

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

::: image/Decoder.cpp
@@ +381,5 @@
> +      if (aPreviousFrame->GetDisposalMethod() !=
> +          DisposalMethod::RESTORE_PREVIOUS) {
> +        // If the new restore frame is the direct previous frame, then we know
> +        // the dirty rect is composed only of the current frame's blend rect and
> +        // this frame's clear rect (if applicable) which are handled in filters.

"the current frame's blend rect and this frame's clear rect"

It's confusing because which frame is "this" frame?

@@ +390,5 @@
> +        // have been several frames between us and mRestoreFrame, the only areas
> +        // that changed are the clear rect, the current frame blending rect, and
> +        // the previous frame's blending rect. All else is forgotten due to us
> +        // restoring the same frame again.
> +        mRestoreDirtyRect = aPreviousFrame->GetDirtyRect();

This comment is also confusing. The clear rect from which frame?

The current frame blending rect doesn't need to be added in here because the blend animation filter takes care of that, correct? The comment makes it sound like it needs to be included in mRestoreDirtyRect.

::: image/Decoder.h
@@ +592,5 @@
>  private:
>    RefPtr<RasterImage> mImage;
>    Maybe<SourceBufferIterator> mIterator;
>    RawAccessFrameRef mCurrentFrame;
> +  RawAccessFrameRef mRestoreFrame;

Define the restore frame here please.

@@ +597,2 @@
>    ImageMetadata mImageMetadata;
>    gfx::IntRect mInvalidRect; // Tracks an invalidation region in the current frame.

Since you are adding another dirty rect this becomes less clear. Can you expand the comment to say it tracks new rows as the image is decoded?

::: image/imgFrame.h
@@ +297,5 @@
>    //////////////////////////////////////////////////////////////////////////////
>    // Effectively const data, only mutated in the Init methods.
>    //////////////////////////////////////////////////////////////////////////////
>  
> +  //! The size of the buffer we are decoding to.

This doesn't seem like the right definition. It's the logical size of the image no?

@@ +302,4 @@
>    IntSize      mImageSize;
> +
> +  //! The rect to which we are decoding. The origin of the rect should be 0, 0,
> +  //! and its width/height match mImageSize.

This definition seems wrong, isn't this the same as mBlendRect?

Comment 60

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8999024 [details] [diff] [review]
0001-Bug-1337111-Part-1.-Add-BlendAnimationFilter-to-allo.patch, v12

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

::: image/SurfaceFilters.h
@@ +686,5 @@
> +                                       /// necessary because the frame rect width
> +                                       /// is larger than the image's logical width.
> +  int32_t  mRow;                       /// The row in unclamped frame rect space
> +                                       /// that we're currently writing.
> +  size_t mRowLength;                   /// Length in bytes of a row.

Length of a row of the unclamped framerect?

@@ +688,5 @@
> +  int32_t  mRow;                       /// The row in unclamped frame rect space
> +                                       /// that we're currently writing.
> +  size_t mRowLength;                   /// Length in bytes of a row.
> +  SkBlitRow::Proc32 mOverProc;         /// Function pointer to perform over blending.
> +  const uint8_t* mBaseFrameStartPtr;   /// Starting row pointer to the base frame

It'd be really good if you could define what the base frame is somewhere.

@@ +692,5 @@
> +  const uint8_t* mBaseFrameStartPtr;   /// Starting row pointer to the base frame
> +                                       /// data from which we copy pixel data from.
> +  const uint8_t* mBaseFrameRowPtr;     /// Current row pointer to the base frame
> +                                       /// data.
> +  gfx::IntRect mClearRect;             /// The frame area to clear.

"before blending the current frame"

Comment 61

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1337111-Part-1.-Add-BlendAnimationFilter-to-allo.patch, v13

(In reply to Timothy Nikkel (:tnikkel) from comment #60)
> Comment on attachment 8999024 [details] [diff] [review]
> 0001-Bug-1337111-Part-1.-Add-BlendAnimationFilter-to-allo.patch, v12
> 
> Review of attachment 8999024 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/SurfaceFilters.h
> @@ +686,5 @@
> > +                                       /// necessary because the frame rect width
> > +                                       /// is larger than the image's logical width.
> > +  int32_t  mRow;                       /// The row in unclamped frame rect space
> > +                                       /// that we're currently writing.
> > +  size_t mRowLength;                   /// Length in bytes of a row.
> 
> Length of a row of the unclamped framerect?
> 

mRow is the unclamped frame rect because that is how many rows the decoder is providing. However we actually ignore the extra pixels (or leave them as 0s if too few) if the unclamped row is longer than the row we desire. mRowLength is the latter / the length of the input size to the next filter.

> @@ +688,5 @@
> > +  int32_t  mRow;                       /// The row in unclamped frame rect space
> > +                                       /// that we're currently writing.
> > +  size_t mRowLength;                   /// Length in bytes of a row.
> > +  SkBlitRow::Proc32 mOverProc;         /// Function pointer to perform over blending.
> > +  const uint8_t* mBaseFrameStartPtr;   /// Starting row pointer to the base frame
> 
> It'd be really good if you could define what the base frame is somewhere.
> 

I put it at the top with the BlendAnimationFilter description

> @@ +692,5 @@
> > +  const uint8_t* mBaseFrameStartPtr;   /// Starting row pointer to the base frame
> > +                                       /// data from which we copy pixel data from.
> > +  const uint8_t* mBaseFrameRowPtr;     /// Current row pointer to the base frame
> > +                                       /// data.
> > +  gfx::IntRect mClearRect;             /// The frame area to clear.
> 
> "before blending the current frame"

Done.

Comment 62

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1337111-Part-2.-Add-an-image-decoder-flag-to-req.patch, v9

(In reply to Timothy Nikkel (:tnikkel) from comment #59)
> Comment on attachment 8999025 [details] [diff] [review]
> 0002-Bug-1337111-Part-2.-Add-an-image-decoder-flag-to-req.patch, v8
> 
> Review of attachment 8999025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/Decoder.cpp
> @@ +381,5 @@
> > +      if (aPreviousFrame->GetDisposalMethod() !=
> > +          DisposalMethod::RESTORE_PREVIOUS) {
> > +        // If the new restore frame is the direct previous frame, then we know
> > +        // the dirty rect is composed only of the current frame's blend rect and
> > +        // this frame's clear rect (if applicable) which are handled in filters.
> 
> "the current frame's blend rect and this frame's clear rect"
> 
> It's confusing because which frame is "this" frame?
> 

The restore frame. I updated this.

> @@ +390,5 @@
> > +        // have been several frames between us and mRestoreFrame, the only areas
> > +        // that changed are the clear rect, the current frame blending rect, and
> > +        // the previous frame's blending rect. All else is forgotten due to us
> > +        // restoring the same frame again.
> > +        mRestoreDirtyRect = aPreviousFrame->GetDirtyRect();
> 
> This comment is also confusing. The clear rect from which frame?
> 

The restore frame. I updated this.

> The current frame blending rect doesn't need to be added in here because the
> blend animation filter takes care of that, correct? The comment makes it
> sound like it needs to be included in mRestoreDirtyRect.
> 

Yes. I'm not sure how to rephrase this since it seems clear to me, but I'm open to suggestions :).

> ::: image/Decoder.h
> @@ +592,5 @@
> >  private:
> >    RefPtr<RasterImage> mImage;
> >    Maybe<SourceBufferIterator> mIterator;
> >    RawAccessFrameRef mCurrentFrame;
> > +  RawAccessFrameRef mRestoreFrame;
> 
> Define the restore frame here please.
> 

Done.

> @@ +597,2 @@
> >    ImageMetadata mImageMetadata;
> >    gfx::IntRect mInvalidRect; // Tracks an invalidation region in the current frame.
> 
> Since you are adding another dirty rect this becomes less clear. Can you
> expand the comment to say it tracks new rows as the image is decoded?
> 
> ::: image/imgFrame.h
> @@ +297,5 @@
> >    //////////////////////////////////////////////////////////////////////////////
> >    // Effectively const data, only mutated in the Init methods.
> >    //////////////////////////////////////////////////////////////////////////////
> >  
> > +  //! The size of the buffer we are decoding to.
> 
> This doesn't seem like the right definition. It's the logical size of the
> image no?
> 

I think those are the same thing :). But see below, the situation is sort of confusing at the moment...

> @@ +302,4 @@
> >    IntSize      mImageSize;
> > +
> > +  //! The rect to which we are decoding. The origin of the rect should be 0, 0,
> > +  //! and its width/height match mImageSize.
> 
> This definition seems wrong, isn't this the same as mBlendRect?

Technically if it is an APNG producing partial frames, mFrameRect = 0,0 -> mImageSize, and mBlendRect is what you would expect. But mFrameRect == mBlendRect otherwise, including when it is a paletted frame (so mFrameRect isn't the full frame size, and the comment is wrong, ugh). I would like to consolidate them to eliminate the need for mImageSize and make mFrameRect is always the full image frame size, if it were complete. But ideally we could do that in a new bug as it is important for *this* bug.

Comment 63

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #58)
> (In reply to Andrew Osmond [:aosmond] from comment #55)
> > > ::: image/FrameAnimator.cpp
> > > @@ +487,5 @@
> > > >      }
> > > >    }
> > > >  
> > > >    // Advanced to the correct frame, the composited frame is now valid to be drawn.
> > > > +  if (ret.mFrameAdvanced && aState.mCompositedFrameInvalid) {
> > > 
> > > Why is the change from "currentFrameEndTime > aTime" to "ret.mFrameAdvanced"
> > > needed?
> > > 
> > 
> > I updated this with a better condition (I realized there was a bug) and
> > hopefully a better explanation. Advancing in of itself doesn't mean we
> > updated the composited frame anymore because we might be producing full
> > frames.
> 
> Hmm, okay. So the name of mCompositedFrameInvalid may be misleading now. It
> only tracks whether we should draw the animation frame at
> mCurrentAnimationFrameIndex (whether it comes from mCompositedFrame or from
> the surface cache because the image has full frames that don't need to be
> blended, either way). It might be better to be called
> mShouldDrawThisImageRightNow (flipping true/false).
> 
> Before we switched to pausing non-visible animated images we needed it for
> the case where an image was in a background tab, then it became the active
> tab: we would "fast forward" the image until it got to the point in the
> timeline where it should have been. Which looks bad. It's better to draw
> nothing.
> 
> Was this causing a bug?

I'm not sure I follow exactly, let's discuss this offline and bring the conclusion back to the bug.

The bug I was concerned about was the dirty rect being wrong, that is all. It was always setting the dirty rect to be the full frame, rather than just the aggregate of the dirty rects of the frames iterated past.

Comment 64

[Timothy Nikkel (:tnikkel)]

Talked to Andrew on irc. r+ with the changes we talked about.

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3785cdebe6a3
Part 1. Add BlendAnimationFilter to allow decoders to generate complete frames. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b1e25b0b345
Part 2. Add an image decoder flag to request complete frames. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/f36337c1309b
Part 3. Land groundwork for new blended animation gtests. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/93e956e89a21
Part 4. Add gtests for BlendAnimationFilter. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d7fa868d0d
Part 5. Add pref to force decoding of full frames, disabled by default. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0caa556dc9
Part 6. Use shared surfaces for full animated frames for WebRender. r=tnikkel

Comment 66

[Cristina Coroiu [:ccoroiu]]

Backed out 6 changesets (bug 1337111) for build bustages at builds/worker/workspace/build/src/image/SurfaceFilters.h

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ac47c5ebf333b38629a2ef590628b3a69c4a24

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ca0caa556dc9c92c57f86ea998aae7a3634bf0fd&selectedJob=199736183

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=199736183&repo=mozilla-inbound&lineNumber=18764

[task 2018-09-17T17:32:00.769Z] 17:32:00     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -o GrMSAAPathRenderer.o -c  -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DSKIA_IMPLEMENTATION=1 -DSK_PDF_USE_SFNTLY=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/gfx/skia -I/builds/worker/workspace/build/src/obj-firefox/gfx/skia -I/builds/worker/workspace/build/src/gfx/skia/skia/include/c -I/builds/worker/workspace/build/src/gfx/skia/skia/include/codec -I/builds/worker/workspace/build/src/gfx/skia/skia/include/config -I/builds/worker/workspace/build/src/gfx/skia/skia/include/core -I/builds/worker/workspace/build/src/gfx/skia/skia/include/effects -I/builds/worker/workspace/build/src/gfx/skia/skia/include/encode -I/builds/worker/workspace/build/src/gfx/skia/skia/include/gpu -I/builds/worker/workspace/build/src/gfx/skia/skia/include/pathops -I/builds/worker/workspace/build/src/gfx/skia/skia/include/ports -I/builds/worker/workspace/build/src/gfx/skia/skia/include/private -I/builds/worker/workspace/build/src/gfx/skia/skia/include/utils -I/builds/worker/workspace/build/src/gfx/skia/skia/include/utils/mac -I/builds/worker/workspace/build/src/gfx/skia/skia/include/views -I/builds/worker/workspace/build/src/gfx/skia/skia/src/core -I/builds/worker/workspace/build/src/gfx/skia/skia/src/gpu -I/builds/worker/workspace/build/src/gfx/skia/skia/src/gpu/effects -I/builds/worker/workspace/build/src/gfx/skia/skia/src/gpu/gl -I/builds/worker/workspace/build/src/gfx/skia/skia/src/gpu/glsl -I/builds/worker/workspace/build/src/gfx/skia/skia/src/image -I/builds/worker/workspace/build/src/gfx/skia/skia/src/lazy -I/builds/worker/workspace/build/src/gfx/skia/skia/src/opts -I/builds/worker/workspace/build/src/gfx/skia/skia/src/sfnt -I/builds/worker/workspace/build/src/gfx/skia/skia/src/shaders -I/builds/worker/workspace/build/src/gfx/skia/skia/src/sksl -I/builds/worker/workspace/build/src/gfx/skia/skia/src/utils -I/builds/worker/workspace/build/src/gfx/skia/skia/src/utils/mac -I/builds/worker/workspace/build/src/gfx/skia/skia/src/utils/win -I/builds/worker/workspace/build/src/gfx/sfntly/cpp/src -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Os -fno-omit-frame-pointer -Wno-deprecated-declarations -Wno-overloaded-virtual -Wno-shadow -Wno-sign-compare -Wno-unreachable-code -Wno-unused-function -Wno-implicit-fallthrough -Wno-inconsistent-missing-override -Wno-macro-redefined -Wno-unused-private-field -I/builds/worker/workspace/build/src/obj-firefox/dist/include/cairo -I/usr/include/freetype2 -pthread -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libpng12  -MD -MP -MF .deps/GrMSAAPathRenderer.o.pp   /builds/worker/workspace/build/src/gfx/skia/skia/src/gpu/ops/GrMSAAPathRenderer.cpp
[task 2018-09-17T17:32:00.769Z] 17:32:00     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/skia'
[task 2018-09-17T17:32:00.769Z] 17:32:00     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/skia'
[task 2018-09-17T17:32:00.770Z] 17:32:00     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/skia'
[task 2018-09-17T17:32:00.770Z] 17:32:00     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/skia'
[task 2018-09-17T17:32:00.771Z] 17:32:00     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/skia'
[task 2018-09-17T17:32:01.337Z] 17:32:01     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/image/decoders'
[task 2018-09-17T17:32:01.345Z] 17:32:01     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -o Unified_cpp_image_decoders0.o -c  -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/image/decoders -I/builds/worker/workspace/build/src/obj-firefox/image/decoders -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/gfx/2d -I/builds/worker/workspace/build/src/image -I/builds/worker/workspace/build/src/gfx/skia -I/builds/worker/workspace/build/src/gfx/skia/skia/include/config -I/builds/worker/workspace/build/src/gfx/skia/skia/include/core -I/builds/worker/workspace/build/src/gfx/skia/skia/include/gpu -I/builds/worker/workspace/build/src/gfx/skia/skia/include/utils -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Os -fno-omit-frame-pointer -Werror -Wno-error=shadow  -MD -MP -MF .deps/Unified_cpp_image_decoders0.o.pp   /builds/worker/workspace/build/src/obj-firefox/image/decoders/Unified_cpp_image_decoders0.cpp
[task 2018-09-17T17:32:01.347Z] 17:32:01     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/image/decoders/Unified_cpp_image_decoders0.cpp:20:
[task 2018-09-17T17:32:01.348Z] 17:32:01     INFO -  In file included from /builds/worker/workspace/build/src/image/decoders/nsGIFDecoder2.cpp:49:
[task 2018-09-17T17:32:01.349Z] 17:32:01     INFO -  In file included from /builds/worker/workspace/build/src/image/SurfacePipeFactory.h:11:
[task 2018-09-17T17:32:01.350Z] 17:32:01     INFO -  /builds/worker/workspace/build/src/image/SurfaceFilters.h:373:67: error: Type 'mozilla::image::SurfaceConfig' must not be used as parameter
[task 2018-09-17T17:32:01.351Z] 17:32:01     INFO -    nsresult Configure(const BlendAnimationConfig& aConfig, Rest... aRest)
[task 2018-09-17T17:32:01.352Z] 17:32:01     INFO -                                                                    ^
[task 2018-09-17T17:32:01.353Z] 17:32:01     INFO -  /builds/worker/workspace/build/src/image/SurfaceFilters.h:373:67: note: Please consider passing a const reference instead
[task 2018-09-17T17:32:01.353Z] 17:32:01     INFO -  /builds/worker/workspace/build/src/image/SurfaceFilters.h:322:8: note: The bad argument was passed to 'BlendAnimationFilter' here
[task 2018-09-17T17:32:01.354Z] 17:32:01     INFO -    Next mNext;                    /// The next SurfaceFilter in the chain.
[task 2018-09-17T17:32:01.354Z] 17:32:01     INFO -         ^
[task 2018-09-17T17:32:01.354Z] 17:32:01     INFO -  /builds/worker/workspace/build/src/image/SurfacePipe.h:783:26: note: 'mozilla::image::SurfaceConfig' is a non-param type because member 'mAnimParams' is a non-param type 'Maybe<mozilla::image::AnimationParams>'
[task 2018-09-17T17:32:01.355Z] 17:32:01     INFO -    Maybe<AnimationParams> mAnimParams; /// Given for animated images.
[task 2018-09-17T17:32:01.355Z] 17:32:01     INFO -                           ^
[task 2018-09-17T17:32:01.355Z] 17:32:01     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Maybe.h:167:69: note: 'Maybe<mozilla::image::AnimationParams>' is a non-param type because member 'mStorage' has an alignas(_) annotation
[task 2018-09-17T17:32:01.355Z] 17:32:01     INFO -  class MOZ_NON_PARAM MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS Maybe
[task 2018-09-17T17:32:01.355Z] 17:32:01     INFO -                                                                      ^
[task 2018-09-17T17:32:01.356Z] 17:32:01     INFO -  1 error generated.
[task 2018-09-17T17:32:01.359Z] 17:32:01     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1121: recipe for target 'Unified_cpp_image_decoders0.o' failed
[task 2018-09-17T17:32:01.359Z] 17:32:01     INFO -  make[4]: *** [Unified_cpp_image_decoders0.o] Error 1
[task 2018-09-17T17:32:01.360Z] 17:32:01     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/image/decoders'
[task 2018-09-17T17:32:01.361Z] 17:32:01     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'image/decoders/target' failed
[task 2018-09-17T17:32:01.361Z] 17:32:01     INFO -  make[3]: *** [image/decoders/target] Error 2
[task 2018-09-17T17:32:01.362Z] 17:32:01     INFO -  make[3]: *** Waiting for unfinished jobs....
[task 2018-09-17T17:32:01.363Z] 17:32:01     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/skia'
[task 2018-09-17T17:32:01.364Z] 17:32:01     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/skia'
[task 2018-09-17T17:32:01.365Z] 17:32:01     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/skia'
[task 2018-09-17T17:32:01.366Z] 17:32:01     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/skia'

Comment 67

[Andrew Osmond [:aosmond] (he/him)]

Looks like bug 1462355 landing first bit me. Hopefully this does it:

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ddce7c9b615c3c8fd6e5e34bfde6c1a31594494

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81865752a2f7
Part 1. Add BlendAnimationFilter to allow decoders to generate complete frames. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e72e07baca92
Part 2. Add an image decoder flag to request complete frames. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/a826a94ae5dd
Part 3. Land groundwork for new blended animation gtests. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cd5c4f23ee8
Part 4. Add gtests for BlendAnimationFilter. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8687fe42df8
Part 5. Add pref to force decoding of full frames, disabled by default. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c53523cea45
Part 6. Use shared surfaces for full animated frames for WebRender. r=tnikkel

Comment 69

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/81865752a2f7
https://hg.mozilla.org/mozilla-central/rev/e72e07baca92
https://hg.mozilla.org/mozilla-central/rev/a826a94ae5dd
https://hg.mozilla.org/mozilla-central/rev/8cd5c4f23ee8
https://hg.mozilla.org/mozilla-central/rev/f8687fe42df8
https://hg.mozilla.org/mozilla-central/rev/7c53523cea45