523950 - Long animated images (GIF, APNG) makes Firefox consume all available memory

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Zack Weinberg (:zwol)]

The attached animated GIF (a network traffic visualization; I have redacted all identifying information) is an 800x800 image with 8639 frames.  It's 16MB on disk.  When loaded in Firefox (I tried 3.5 and trunk), the browser spins for about 30 seconds and ends up allocating approximately 2.5GB of RAM.  The un-redacted version took 3.5GB and crashed a 32-bit build of the browser (unsurprisingly).

By way of contrast, GNOME's epiphany (using Webkit 1.1.15.2) needs only 50MB of RAM to display the image.  I suspect we are trying to hold all of the uncompressed frames in RAM at once -- 800x800x8369 = 4.98GB, and that's at one byte per pixel.  [The difference is probably down to partial frames in the file, which I think we take advantage of in the in-memory representation.]

Comment 1

[Zack Weinberg (:zwol)]

Attachment: test case (part 1)

This test case is so huge I have to split it into pieces.  Here's part 1.

Comment 2

[Zack Weinberg (:zwol)]

... rather than upload another ten fragments and make people glue them back together again, I've put the complete file on people.mozilla.org.  See URL.

Comment 3

[Jo Hermans]

(In reply to comment #0)
> RAM to display the image.  I suspect we are trying to hold all of the
> uncompressed frames in RAM at once -- 800x800x8369 = 4.98GB, and that's at one
> byte per pixel.  [The difference is probably down to partial frames in the
> file, which I think we take advantage of in the in-memory representation.]

Animated frames stay in memory until the entire page is replaced (caused by bug 414259). Bug 500402 was filed for a solution.

Comment 4

[Joe Drew (not getting mail)]

This argues a little bit for a more video-like representation of animated images, but I'm not sure it's worthwhile, given how few huge, long animated images there are.

Discarding whole animated images won't really help this example, as any time we go to re-render it we'd have to redecode the whole thing to memory. What we might need is to discard individual frames of an animation once we go over a threshold, and decode certain frames on demand. Being able to decode only certain frames probably requires some of the work that will be done in bug 500402, though.

Comment 5

[Zack Weinberg (:zwol)]

I wonder how feasible it would be to estimate the amount of memory an animated image will require if we keep all its frames decoded in RAM, and abort display (or only show the first frame, with some sort of failure tag) if it's going to be huge.

Comment 6

[Joe Drew (not getting mail)]

Impossible. We don't know how many frames there will be until we decode them. :( (True for both APNG and GIF.)

Comment 8

[Marius Hudea]

I've had this issue today. 
I've created a 4 MB gif animation (1374px × 552px) with Virtualdub and saved it on Grayscale. The picture in question is at this time here: definethis.org/temp/torrents.ro/film.gif (copy and paste in new browser window if you wish)

Firefox goes up to about 980 MB. I've right clicked on the picture, clicked with the scroll wheel on the "view image" to open it in new tab and memory usage raised to about 1.56 GB.

Back with no image loaded in the browser, I typed the address in an image tag on a vBulletin forum, clicked on preview post and when page reloaded and image was being downloaded i scrolled down the page. As the image started to get out of view but still being downloaded, Firefox crashed.

It's true that it's not possible to know the number of frames before it's completely downloaded, but should each frame be converted and held in memory as a truecolor bitmap especially if the gif itself is grayscale? I remember a long time ago there was a bug about grayscale gif images being converted to truecolor but I can't find it.

IE 8 has the same problem, though it needs only about 900 MB. Google Chrome however runs the page with the picture at about 45 MB of memory, maybe it decodes each frame each time it's needed, i don't know...

Comment 9

[Marius Hudea]

Second post, sorry about that.. I just copied and pasted the address above to make sure I entered it right and I forgot I already had it open in another tab... 20 seconds later Firefox crashed, even though I have 4 GB of memory. I guess it went over the 2GB per process limit on 32 bit operating systems.

Now that I see it crashes so easily, I really think this should be considered as a security issue. It's really not hard for pranksters to just create a 2MB 3000x3000 blank gif and post it somewhere and have people crash their browsers and potentially losing what they typed in other tabs.

Comment 10

[Zack Weinberg (:zwol)]

I no longer have anywhere I can host the very large image file that was a test case for this bug, without risking nasty bandwidth charges.  I am happy to provide it to anyone who wants to see it, just send me a note.

Comment 11

[Jesse Ruderman]

Zack sent me the testcase. Now it lives at:
http://www.squarefree.com/bug523950/523950.gif

Comment 12

[Chris Lord [:cwiiis]]

I can confirm that this bug still exists on today's Nightly.

Comment 13

[Elbart]

Current Nightly, goes from ~300MB to crash (hits virtual-memory-limit).
Chrome 31, task for the tab with the gif goes from ~200MB to ~950MB after minimizing and restoring the window.

Comment 14

[Elbart]

With HWA off, Nightly maxes out at ~1900MB but doesn't crash. After a few minutes graphic-errors appear and the browser becomes unusable.

Here's the regression-window for the oom-crash:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ef3f5669b53e&tochange=a80dce1126db

And Chrome actually goes to only 250MB. No idea why it spiked to 950 earlier.

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

Elbart you should file your issue as a separate bug. Your issue points to change in behavior recently. In addition getting a tighter regression range on using your platform's mozilla-inbound- folder from http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/

Comment 17

[Caspy7]

Here is a report recently from a reddit user asking why a gif caused their memory to jump to 1.2GB. Other users demonstrated more (1.9GB-2GB).

https://www.reddit.com/r/firefox/comments/5b9rpi/why_does_this_62mb_gif_increase_ram_usage_by_12gb/

This was the gif in question:
https://i.redd.it/i6do3edhxnvx.gif

Comment 18

[Jan Henning [:JanH]]

On a phone with 2 GB RAM (Moto G4 Play), various pages on pokewiki.de (e.g. http://www.pokewiki.de/Metronom) which use multiple animated GIFs are relatively reliably causing OOM crashes on Fennec.

Comment 19

[Caspy7]

This might make a good addition to memshrink.

I'm not sure if we have any idea how frequently users are hitting this, but despite it being 2017, some sites are still using gifs prolifically - and I've run into a few users who recognized that the browser got unbearable or crashed because of it.

Comment 20

[(Away)]

e10s seems to make this worse: I see both parent and content processes allocating 1GB+ of memory at https://pastebin.mozilla.org/9023176. And it's particularly curious that it's at exactly the same stack for both processes. I would have figured they'd be different halves of an IPC or something.

At a minimum can we reduce the duplication? And even better, could we discard frames we no longer need? I suppose for looping images we'll always "need" the frames, but above some threshold it might make more sense to drop the data and re-decode.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

(In reply to David Major [:dmajor] from comment #20)
> e10s seems to make this worse: I see both parent and content processes
> allocating 1GB+ of memory at https://pastebin.mozilla.org/9023176. And it's
> particularly curious that it's at exactly the same stack for both processes.
> I would have figured they'd be different halves of an IPC or something.
> 
> At a minimum can we reduce the duplication? And even better, could we
> discard frames we no longer need? I suppose for looping images we'll always
> "need" the frames, but above some threshold it might make more sense to drop
> the data and re-decode.

This is not expected. What page were you using to reproduce this?

Comment 22

[(Away)]

This one:

(In reply to Caspy7 from comment #17)
> https://i.redd.it/i6do3edhxnvx.gif

The growth in the parent process seems to start a few seconds later than the child, but eventually both reach >1GB.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

(In reply to David Major [:dmajor] from comment #22)
> This one:
> 
> (In reply to Caspy7 from comment #17)
> > https://i.redd.it/i6do3edhxnvx.gif
> 
> The growth in the parent process seems to start a few seconds later than the
> child, but eventually both reach >1GB.

Ah, it's the favicon. We're loading the image in both processes, so everything's working as expected here.

Comment 24

[Jan Henning [:JanH]]

(In reply to Caspy7 from comment #19)
> I'm not sure if we have any idea how frequently users are hitting this, but
> despite it being 2017, some sites are still using gifs prolifically - and
> I've run into a few users who recognized that the browser got unbearable or
> crashed because of it.

It's even more relevant on Android, where memory is much more limited. According to about:memory, that (https://i.redd.it/i6do3edhxnvx.gif) GIF takes up 1,284.48 MB, which is huge, but on most desktops (at least with 64-bit Firefox) probably not immediately catastrophic.

The animated images on e.g. http://www.pokewiki.de/Metronom on the other hand "only" take up a combined total of around 205 MB (measured on desktop), but this is already enough to quite reliably crash Firefox on a phone with 2 GB RAM.

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to David Major [:dmajor] from comment #20)
>...
> At a minimum can we reduce the duplication? And even better, could we
> discard frames we no longer need? 

We do this in 55 as of bug 686905.

Comment 26

[Caspy7]

(In reply to Milan Sreckovic [:milan] from comment #25)
> (In reply to David Major [:dmajor] from comment #20)
> >...
> > At a minimum can we reduce the duplication? And even better, could we
> > discard frames we no longer need? 
> 
> We do this in 55 as of bug 686905.

If we're doing this already, why does this issue remain?

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

You're right, there are two separate and related issues here, and I answered about the wrong one.

The bug 686905 only deals with removing "all the frames of unused animated images".  In this bug, it looks like we need the animated image, so we keep it around, and with it all of its frames.

The second issue is "remove individual frames from animated images if the memory is low", and we don't quite do that.  We've started the work by adding functionality like this bug 1337111, which would have us not keep any frames around (except the latest one, or perhaps a few to make things faster).  A follow up bug to that one would have to use this new API.

Comment 28

[(Away)]

I would argue that a third issue here is the favicon. Why does the parent process need to allocate a gig of memory to display a favicon?

Comment 29

[Zack Weinberg (:zwol)]

(In reply to David Major [:dmajor] from comment #28)
> I would argue that a third issue here is the favicon. Why does the parent
> process need to allocate a gig of memory to display a favicon?

If we dropped the ability for sites to have animated favicons, would anyone even notice?

Comment 30

[Zack Weinberg (:zwol)]

... And the moment I push "save changes", it occurs to me that decoding favicons in the chrome process is a sandboxing fail, too.

Comment 31

[Timothy Nikkel (:tnikkel)]

(In reply to Zack Weinberg (:zwol) from comment #29)
> (In reply to David Major [:dmajor] from comment #28)
> > I would argue that a third issue here is the favicon. Why does the parent
> > process need to allocate a gig of memory to display a favicon?
> 
> If we dropped the ability for sites to have animated favicons, would anyone
> even notice?

There's a very old bug on that, bug 111373.

Comment 32

[Marius Hudea]

I'm not sure how many of these gif files have transparency or not (they're used as basically movies on a page) but.. here's a thought.

Implement a basic MPEG-1 intra frame only encoder inside Firefox. MPEG1 as subset of MPEG2 should be hardware decoded/accelerated by most video cards. MPEG-1 decoder should be equally easy to add, and hardware accelerated/decoded by video cards. 

And, since it's so old/ancient you can basically say for certain there's no patent issues anymore for MPEG1 and it shouldn't use a lot of processor time to do the conversion.
  
I was going to initially suggest vp9 and webm but it uses a lot of cpu to encode and there's still legal/patent issues about it (probably). Maybe AOM when it comes would be a good alternative. 

Also a mpeg1 intra frame only encoder could probably be written in javascript or something already used inside firefox and you wouldn't have to include a huge amount of third party code (like ffmpeg for example) and increase the risks of vulnerabilities/exploits etc
 

If an animated gif exceeds some parameters, convert it to mpeg1 intra frame only (to decode any random frame without depending on other frames and for simplicity) at highest quality possible while keeping the mpeg-1 bitstream valid.

Wikipedia says mpeg-1 supports up to 4095 x 4095 and up to 100 mbps bitrate, which should be fine for animated images.


Any gif that's let's say 
* bigger than 128 pixels wide or 128 pixels tall (so that you won't process menu buttons, logos, favicons etc)
* AND gets decoded to more than 64-128 MB of memory
* doesn't have transparency (if it's too hard to do this)
* has infinite loop so you won't just discard previous frames and show the last frame making conversion pointless

When you reach a threshold while decoding the gif or mng or whatever, go back and convert already decoded frames (which should still be in memory) to mpeg1 intra and continue decoding frames and encoding them in mpeg1, and throw away the original decoded frames to clear the memory.

Then replace the img tag with a html 5 video tag and serve a locally cached mpv (mpeg1-video only) video file and user should also automatically have the video tag controls (play stop seek in the video) 

You could have some kind of CSS custom tag that developers could add to some images (don't ever convert to video, or if you default to not doing this, make css rule to allow this image to be converted/cached etc)
Could also have an option in the configuration to disable automatic conversion of large animations or have a bar show up on top saying something like "For performance reasons, a few animated images were automatically converted to video. Click here to show original animations" - in case the encoding produces visible artifacts in the animations.

Only issue I can see with this is that MPEG1 is YCbCr 4:2:0 so for some animations that colors would be different if the animations are heavy on red or with lots of colors (gradients) or are meant to be overlayed over some background with lots of colors. Maybe a custom mpeg1 decoder could support YCbCr 4:4:4 as well (but then it's unlikely it would be hardware accelerated/decoded by video cards which could be a big deal on phones).

Comment 33

[Eric Rahm [:erahm]]

Timothy, is there anything actionable here?

Comment 34

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

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #33)
> Timothy, is there anything actionable here?

I'm working on a patch that will solve this, it only keeps the current frame and some buffer ahead of it for really large images when we cross some threshold. Nearly ready, worked out the last kinks today, just writing more tests and then breaking up for the review.

Comment 35

[Eric Rahm [:erahm]]

(In reply to Andrew Osmond [:aosmond] from comment #34)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #33)
> > Timothy, is there anything actionable here?
> 
> I'm working on a patch that will solve this, it only keeps the current frame
> and some buffer ahead of it for really large images when we cross some
> threshold. Nearly ready, worked out the last kinks today, just writing more
> tests and then breaking up for the review.

That's great! I'll go ahead and mark you as the assignee.

Comment 36

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

Attachment: Part 1. Do some unified build accounting, missing headers and namespaces., v1

Comment 37

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

Attachment: Part 2. Expose image decoder type and SourceBuffer to owners., v1

Comment 38

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

Attachment: Part 3. Add preferences to control animated image decoding behaviour., v1

Comment 39

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

Attachment: Part 4. Expose new surface provider APIs that will help drive animation decoding., v1

Comment 40

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

Attachment: Part 5. Pass the currently displayed frame of an animation to its decoder., v1

Comment 41

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

Attachment: Part 6. Add DecoderFactory::CloneAnimationDecoder to clone an existing image decoder., v1

Comment 42

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

Attachment: Part 7. Free AnimationSurfaceProvider::mDecoder outside the current execution context., v1

Comment 43

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

Attachment: Part 8. Add AnimatedFrameBuffer to manage storage and decoding of frames in an animation., v1

Comment 44

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

Attachment: Part 9. Add gtests for AnimationFrameBuffer., v1

Comment 45

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

Attachment: Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17fc3358e60c4dc400df646f33cbcadf73202d96

Comment 47

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

(In reply to Andrew Osmond [:aosmond] from comment #45)
> Created attachment 8895821 [details] [diff] [review]
> Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v1
> 
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=17fc3358e60c4dc400df646f33cbcadf73202d96

I see what is happening with the failures here. Looks like the system group dispatches aren't happening in time for leak check (or they are dropped...). It only does the dispatches for AnimationSurfaceProvider::mImage and mDecoder (when itself is freed on the main thread) to avoid a deadlock scenario with SurfaceCache which might be called into indirectly (but SurfaceCache itself might be responsible for our freeing). Fixing it is a pain, I think we can avoid the deadlock by collecting the objects to free in SurfaceCache API calls, and releasing them outside the lock. Patch to follow... shouldn't affect the attached patches too badly though (save part 7).

Comment 48

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

(In reply to Andrew Osmond [:aosmond] from comment #47)
> (In reply to Andrew Osmond [:aosmond] from comment #45)
> > Created attachment 8895821 [details] [diff] [review]
> > Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v1
> > 
> > try:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=17fc3358e60c4dc400df646f33cbcadf73202d96
> 
> I see what is happening with the failures here. Looks like the system group
> dispatches aren't happening in time for leak check (or they are dropped...).
> It only does the dispatches for AnimationSurfaceProvider::mImage and
> mDecoder (when itself is freed on the main thread) to avoid a deadlock
> scenario with SurfaceCache which might be called into indirectly (but
> SurfaceCache itself might be responsible for our freeing). Fixing it is a
> pain, I think we can avoid the deadlock by collecting the objects to free in
> SurfaceCache API calls, and releasing them outside the lock. Patch to
> follow... shouldn't affect the attached patches too badly though (save part
> 7).

Combining with the patches in bug 1389479 (and removing part 7) fixes the leaks (locally).

Comment 49

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

Comment on attachment 8895817 [details] [diff] [review]
Part 7. Free AnimationSurfaceProvider::mDecoder outside the current execution context., v1

No longer needed.

Comment 50

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8895814 [details] [diff] [review]
Part 4. Expose new surface provider APIs that will help drive animation decoding., v1


>     // If mHasBeenDecoded is true then we know the true total frame count and
>     // we can use it to determine if we have all the frames now so we know if
>     // we are currently fully decoded.
>     // If mHasBeenDecoded is false then we'll get another UpdateState call
>     // when the decode finishes.
>     if (mHasBeenDecoded) {
>       Maybe<uint32_t> frameCount = FrameCount();
>       MOZ_ASSERT(frameCount.isSome());
>-      if (NS_SUCCEEDED(aResult.Surface().Seek(*frameCount - 1)) &&
>-          aResult.Surface()->IsFinished()) {
>-        mIsCurrentlyDecoded = true;
>-      } else {
>-        mIsCurrentlyDecoded = false;
>-      }
>+      mIsCurrentlyDecoded = aResult.Surface().IsFullyDecoded();
>     }
>   }

Finding the implementation of IsFullyDecoded a few patches later, this doesn't seem to produce the same result. IsFullyDecoded returns true if we know the length of the animation and it is less than the threshold to use a partial buffer of animation frames.

Comment 51

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

(In reply to Timothy Nikkel (:tnikkel) from comment #50)
> Comment on attachment 8895814 [details] [diff] [review]
> Part 4. Expose new surface provider APIs that will help drive animation
> decoding., v1
> 
> 
> >     // If mHasBeenDecoded is true then we know the true total frame count and
> >     // we can use it to determine if we have all the frames now so we know if
> >     // we are currently fully decoded.
> >     // If mHasBeenDecoded is false then we'll get another UpdateState call
> >     // when the decode finishes.
> >     if (mHasBeenDecoded) {
> >       Maybe<uint32_t> frameCount = FrameCount();
> >       MOZ_ASSERT(frameCount.isSome());
> >-      if (NS_SUCCEEDED(aResult.Surface().Seek(*frameCount - 1)) &&
> >-          aResult.Surface()->IsFinished()) {
> >-        mIsCurrentlyDecoded = true;
> >-      } else {
> >-        mIsCurrentlyDecoded = false;
> >-      }
> >+      mIsCurrentlyDecoded = aResult.Surface().IsFullyDecoded();
> >     }
> >   }
> 
> Finding the implementation of IsFullyDecoded a few patches later, this
> doesn't seem to produce the same result. IsFullyDecoded returns true if we
> know the length of the animation and it is less than the threshold to use a
> partial buffer of animation frames.

I was going to say that knowing the length of the animation was previously the same thing as having all of the frames before, whereas now you but I was unable to reproduce the assert I was thinking I hit by changing it to return mFrames.Complete() only (which is the same as it was before).

Posting updated part 4 and part 10 patches. I renamed IsFullyDecoded to HasFullyDecoded.

Comment 52

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

Attachment: Part 4. Expose new surface provider APIs that will help drive animation decoding., v2

Comment 53

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

Attachment: Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v2

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81b0289080cede84b10e87ad0f6b4d923468764e

Comment 54

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8903734 [details] [diff] [review]
Part 4. Expose new surface provider APIs that will help drive animation decoding., v2

>+  /// @return true if the underlying decoder is still active.
>+  virtual bool HasFullyDecoded() const { return IsFinished(); }

The comment seems wrong.

Comment 55

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8895815 [details] [diff] [review]
Part 5. Pass the currently displayed frame of an animation to its decoder., v1

By the time the decoder gets to the current frame as passed when we create the decoder we could have advanced past it, correct? So later patches deal with that? I guess I'll see :)

Comment 56

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8895818 [details] [diff] [review]
Part 8. Add AnimatedFrameBuffer to manage storage and decoding of frames in an animation., v1

>+bool
>+AnimationFrameBuffer::Insert(RawAccessFrameRef&& aFrame)
>+{
>+  // We should only insert new frames if we actually asked for them.
>+  MOZ_ASSERT(mPending > 0);
>+
>+  if (mSizeKnown) {
>+    // We only insert after the size is known if we are repeating the animation
>+    // and we did not keep all of the frames. Replace whatever is there
>+    // (probably an empty frame) with the new frame.
>+    MOZ_ASSERT(MayDiscard());
>+    MOZ_ASSERT(mInsertIndex < mFrames.Length());
>+
>+    if (mInsertIndex > 0) {
>+      MOZ_ASSERT(!mFrames[mInsertIndex]);
>+      mFrames[mInsertIndex] = Move(aFrame);
>+    }

Should we assert that insertindex isn't 0? Or if it is that its the same frame?

>+  } else {
>+    if (mInsertIndex <= mFrames.Length()) {
>+      // We are still on the first pass of the animation decoding, so this is
>+      // the first time we have seen this frame.
>+      mFrames.AppendElement(Move(aFrame));

We're assuming that mInsertIndex == mFrames.Length() here?

>+    } else if (mInsertIndex > 0) {
>+      // We were forced to restart an animation before we decoded the last
>+      // frame. Thus we might need to insert, even on a "first pass."
>+      MOZ_ASSERT(!mFrames[mInsertIndex]);
>+      mFrames[mInsertIndex] = Move(aFrame);
>+    }

Since this is the else block we know that mInsertIndex > mFrames.Length(), so mFrames[mInsertIndex] should be out of bounds?

>+
>+    if (mFrames.Length() - 1 == mThreshold) {
>+      // We just tripped over the threshold, and on the first pass of the
>+      // decoding; this is our chance to do any clearing of already displayed
>+      // frames. After this, we only need to release as we advance.
>+      MOZ_ASSERT(MayDiscard());
>+      for (size_t i = 1; i < mGetIndex; ++i) {
>+        RawAccessFrameRef discard = Move(mFrames[i]);
>+      }
>+    }
>+  }
>+
>+  MOZ_ASSERT(mFrames[mInsertIndex]);
>+  ++mInsertIndex;
>+
>+  // Ensure we only request more decoded frames if we actually need them. If we
>+  // need to advance to a certain point in the animation on behalf of the owner,
>+  // then do so. This ensures we keep decoding. If the batch size is really
>+  // small (i.e. 1), it is possible advancing will request the decoder to
>+  // "restart", but we haven't told it to stop yet. Note that we skip the first
>+  // insert because we actually start "advanced" to the first frame anyways.
>+  bool continueDecoding = --mPending > 0;
>+  if (mAdvance > 0 && mInsertIndex > 1) {
>+    continueDecoding |= AdvanceInternal();
>+    --mAdvance;
>+  }

What if we restarted an animation (after decoding a few frames) then we don't want to decrement mAdvance until we have re-inserted all the frames we had before we restarted, no?

>+  /**
>+   * Access a specific frame from the frame buffer. It should generally access
>+   * frames in sequential order, increasing in tandem with AdvanceTo calls. The
>+   * first frame may be accessed at any time. The access order should start with
>+   * the same value as that given in Initialize (aStartFrame).

So besides the first frame we should always access in sequential order? What happens if these requirements aren't followed? Illegal or just slow?

>+  /**
>+   * Inserts a frame into the frame buffer. If it has yet to fully decode the
>+   * animated image yet, then it will append the frame to its internal buffer.
>+   * If it has been fully decoded, it will replace the next frame in its buffer
>+   * with the given frame.

Does "fully decode" here mean we have a full buffer of frames? Or we've decoded every frame of the image?

>+  /**
>+   * Resets the currently displayed frame of the frame buffer to the beginning.
>+   * If the buffer is discarding old frames, it will actually discard all frames
>+   * besides the first.
>+   *
>+   * @param aAdvanceOnly Must be false if it is discarding frames, true if it
>+   *                     retains them.

This is confusing, so aAdvanceOnly is only for asserts then?

>+private:
>+  bool AdvanceInternal();

What does the return value mean?

>+
>+  nsTArray<RawAccessFrameRef> mFrames;
>+  size_t mThreshold;
>+  size_t mBatch;
>+  size_t mPending;
>+  size_t mAdvance;
>+  size_t mInsertIndex;
>+  size_t mGetIndex;
>+  bool mSizeKnown;

Comment all of these.

Not sure I totally grok this yet. Waiting for an updated patch.

A lot of this code feels really delicate, like there are a lot of corner cases we are dealing with, but it doesn't feel like there is strong evidence that we are handling all the relevant edges cases. Since bugs in this code are likely to be timing dependant that could make for hard to debug bugs. We should hopefully at least beef up the asserts so that we can catch the problems as soon as possible rather than after things have gone off the rails for a bit.

Comment 57

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

(In reply to Timothy Nikkel (:tnikkel) from comment #54)
> Comment on attachment 8903734 [details] [diff] [review]
> Part 4. Expose new surface provider APIs that will help drive animation
> decoding., v2
> 
> >+  /// @return true if the underlying decoder is still active.
> >+  virtual bool HasFullyDecoded() const { return IsFinished(); }
> 
> The comment seems wrong.

Will fix.

(In reply to Timothy Nikkel (:tnikkel) from comment #55)
> Comment on attachment 8895815 [details] [diff] [review]
> Part 5. Pass the currently displayed frame of an animation to its decoder.,
> v1
> 
> By the time the decoder gets to the current frame as passed when we create
> the decoder we could have advanced past it, correct? So later patches deal
> with that? I guess I'll see :)

When we get a non-zero current frame passed in, it is because we discarded the animation surface provider and we want to show the animation again. The only frame we will have in that case is the compositing frame (if it exists) in FrameAnimator. It won't be able to advance past it, as it won't have the frame data itself, or the metadata (like timeouts).

(In reply to Timothy Nikkel (:tnikkel) from comment #56)
> Comment on attachment 8895818 [details] [diff] [review]
> Part 8. Add AnimatedFrameBuffer to manage storage and decoding of frames in
> an animation., v1
> 
> >+bool
> >+AnimationFrameBuffer::Insert(RawAccessFrameRef&& aFrame)
> >+{
> >+  // We should only insert new frames if we actually asked for them.
> >+  MOZ_ASSERT(mPending > 0);
> >+
> >+  if (mSizeKnown) {
> >+    // We only insert after the size is known if we are repeating the animation
> >+    // and we did not keep all of the frames. Replace whatever is there
> >+    // (probably an empty frame) with the new frame.
> >+    MOZ_ASSERT(MayDiscard());
> >+    MOZ_ASSERT(mInsertIndex < mFrames.Length());
> >+
> >+    if (mInsertIndex > 0) {
> >+      MOZ_ASSERT(!mFrames[mInsertIndex]);
> >+      mFrames[mInsertIndex] = Move(aFrame);
> >+    }
> 
> Should we assert that insertindex isn't 0? Or if it is that its the same
> frame?
> 

The first frame will come again from the decoder on the next pass, since it doesn't particularly care about what frames we currently have buffered. So we want to accept it, and advance our insertion point, but there isn't any point to updating the buffer as it should be identical.

> >+  } else {
> >+    if (mInsertIndex <= mFrames.Length()) {
> >+      // We are still on the first pass of the animation decoding, so this is
> >+      // the first time we have seen this frame.
> >+      mFrames.AppendElement(Move(aFrame));
> 
> We're assuming that mInsertIndex == mFrames.Length() here?
> 

Hm. Yes. This should be ==.

> >+    } else if (mInsertIndex > 0) {
> >+      // We were forced to restart an animation before we decoded the last
> >+      // frame. Thus we might need to insert, even on a "first pass."
> >+      MOZ_ASSERT(!mFrames[mInsertIndex]);
> >+      mFrames[mInsertIndex] = Move(aFrame);
> >+    }
> 
> Since this is the else block we know that mInsertIndex > mFrames.Length(),
> so mFrames[mInsertIndex] should be out of bounds?
> 

Also true. I will fix and add a new test case covering this.

> >+
> >+    if (mFrames.Length() - 1 == mThreshold) {
> >+      // We just tripped over the threshold, and on the first pass of the
> >+      // decoding; this is our chance to do any clearing of already displayed
> >+      // frames. After this, we only need to release as we advance.
> >+      MOZ_ASSERT(MayDiscard());
> >+      for (size_t i = 1; i < mGetIndex; ++i) {
> >+        RawAccessFrameRef discard = Move(mFrames[i]);
> >+      }
> >+    }
> >+  }
> >+
> >+  MOZ_ASSERT(mFrames[mInsertIndex]);
> >+  ++mInsertIndex;
> >+
> >+  // Ensure we only request more decoded frames if we actually need them. If we
> >+  // need to advance to a certain point in the animation on behalf of the owner,
> >+  // then do so. This ensures we keep decoding. If the batch size is really
> >+  // small (i.e. 1), it is possible advancing will request the decoder to
> >+  // "restart", but we haven't told it to stop yet. Note that we skip the first
> >+  // insert because we actually start "advanced" to the first frame anyways.
> >+  bool continueDecoding = --mPending > 0;
> >+  if (mAdvance > 0 && mInsertIndex > 1) {
> >+    continueDecoding |= AdvanceInternal();
> >+    --mAdvance;
> >+  }
> 
> What if we restarted an animation (after decoding a few frames) then we
> don't want to decrement mAdvance until we have re-inserted all the frames we
> had before we restarted, no?
> 

Okay, so aStartFrame > 0 in AnimationFrameBuffer::AnimationFrameBuffer and after decoding a few frames, we actually call AnimationFrameBuffer::Reset. This will set mAdvance to 0, and thus stop any auto-advancing in its tracks, as the decrementing requires mAdvance be > 0.

> >+  /**
> >+   * Access a specific frame from the frame buffer. It should generally access
> >+   * frames in sequential order, increasing in tandem with AdvanceTo calls. The
> >+   * first frame may be accessed at any time. The access order should start with
> >+   * the same value as that given in Initialize (aStartFrame).
> happens if these requirements aren't followed? Illegal or just slow?
> 

It is legal, but it won't always return a valid DrawableFrameRef. It promises to always return a valid ref for any frame from the first to the currently displayed *unless* it has crossed the threshold and starts discarding previous frames. Then its promise weakens to only have the first and current frame. In practice it may have a few frames ahead as well if the batch decoding is keeping ahead of the display.

> >+  /**
> >+   * Inserts a frame into the frame buffer. If it has yet to fully decode the
> >+   * animated image yet, then it will append the frame to its internal buffer.
> >+   * If it has been fully decoded, it will replace the next frame in its buffer
> >+   * with the given frame.
> 
> Does "fully decode" here mean we have a full buffer of frames? Or we've
> decoded every frame of the image?
> 

The latter.

> >+  /**
> >+   * Resets the currently displayed frame of the frame buffer to the beginning.
> >+   * If the buffer is discarding old frames, it will actually discard all frames
> >+   * besides the first.
> >+   *
> >+   * @param aAdvanceOnly Must be false if it is discarding frames, true if it
> >+   *                     retains them.
> 
> This is confusing, so aAdvanceOnly is only for asserts then?
> 

Hm, you are right. It isn't actually necessary because I can check MayDiscard inside of Reset. This was probably an artifact of a few refactoring efforts as I created the class. I will remove the parameter. It was tricky because I needed to hold either the frames mutex (advance only case, no discarding to worry about) or both the decoding mutex and the frames mutex (might have discarded frames we need already), and of course, it is costly to lock both, and the one I need to check the frame buffer state is the *second* mutex ;).

> >+private:
> >+  bool AdvanceInternal();
> 
> What does the return value mean?
> 

It is the same as AdvanceTo. The decoder may have stopped producing new frames at our request. When we advance sufficiently far in the animation, the number of buffered frames ahead of the currently displayed frame will shink. If that buffer is too small, we need to signal to the caller that the decoder should produce more frames again.

Similarly Insert will tell the caller when it doesn't need more frames (yet).

> >+
> >+  nsTArray<RawAccessFrameRef> mFrames;
> >+  size_t mThreshold;
> >+  size_t mBatch;
> >+  size_t mPending;
> >+  size_t mAdvance;
> >+  size_t mInsertIndex;
> >+  size_t mGetIndex;
> >+  bool mSizeKnown;
> 
> Comment all of these.
> 

Will do.

> Not sure I totally grok this yet. Waiting for an updated patch.
> 
> A lot of this code feels really delicate, like there are a lot of corner
> cases we are dealing with, but it doesn't feel like there is strong evidence
> that we are handling all the relevant edges cases. Since bugs in this code
> are likely to be timing dependant that could make for hard to debug bugs. We
> should hopefully at least beef up the asserts so that we can catch the
> problems as soon as possible rather than after things have gone off the
> rails for a bit.

There are gtests in part 9, which should help. We can add more if there are areas I'm missing. For sure, I'll be adding a test case for the bug you pointed out earlier in the comment.

I thought I was pretty good with the number of asserts, but I'll try to think of more that make sense...

Comment 58

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

Attachment: Part 4. Expose new surface provider APIs that will help drive animation decoding., v3 [carries r=tnikkel]

Fix comment.

Comment 59

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

Attachment: Part 8. Add AnimatedFrameBuffer to manage storage and decoding of frames in an animation., v2

Incorporate review feedback.

Comment 60

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

Attachment: Part 9. Add gtests for AnimationFrameBuffer., v2

Add another test as per review feedback. Do a little bit of test cleanup.

Comment 61

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

Attachment: Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v3

Incorporate review feedback (due to changes in part 8).

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

Comment 62

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8905622 [details] [diff] [review]
Part 8. Add AnimatedFrameBuffer to manage storage and decoding of frames in an animation., v2

I think I put my finger on what bothers me about this. When we Insert we are relying strictly on timing and the interactions of AnimatedFrameBuffer and AnimationSurfaceProvider happening in exactly the right order. If they got out of sync or confused we might end up inserting frames for the wrong index into the wrong place. If we passed the absolute frame index when inserted we could at least assert that we are getting the expected frame and putting it in the right place.

Comment 63

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8905624 [details] [diff] [review]
Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v3

>+  // Calculate how many frames we need to decode in this animation before we
>+  // enter frames-on-demand mode. We are willing to buffer as many frames that
>+  // can fit inside the given size threshold, bounded by at least as many frames
>+  // as we are expected to buffer.
>+  IntSize frameSize = aSurfaceKey.Size();
>+  size_t threshold =
>+    (size_t(gfxPrefs::ImageAnimatedDecodeOnDemandThresholdKB()) * 1024) /
>+    (sizeof(uint8_t) * frameSize.width * frameSize.height);

So this is assuming we are an 8bit image?

tion, which is not worth the complexity of
>@@ -74,33 +78,33 @@ public:
>-  void CheckForNewFrameAtYield();
>-  void CheckForNewFrameAtTerminalState();
>+  bool CheckForNewFrameAtYield();
>+  bool CheckForNewFrameAtTerminalState();

Comment what the return value means

Comment 64

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8905621 [details] [diff] [review]
Part 4. Expose new surface provider APIs that will help drive animation decoding., v3 [carries r=tnikkel]

>@@ -66,22 +66,17 @@ AnimationState::UpdateStateInternal(LookupResult& aResult,
>     // If mHasBeenDecoded is true then we know the true total frame count and
>     // we can use it to determine if we have all the frames now so we know if
>     // we are currently fully decoded.
>     // If mHasBeenDecoded is false then we'll get another UpdateState call
>     // when the decode finishes.
>     if (mHasBeenDecoded) {
>       Maybe<uint32_t> frameCount = FrameCount();
>       MOZ_ASSERT(frameCount.isSome());
>-      if (NS_SUCCEEDED(aResult.Surface().Seek(*frameCount - 1)) &&
>-          aResult.Surface()->IsFinished()) {
>-        mIsCurrentlyDecoded = true;
>-      } else {
>-        mIsCurrentlyDecoded = false;
>-      }
>+      mIsCurrentlyDecoded = aResult.Surface().HasFullyDecoded();
>     }
>   }

So this will change the meaning of mIsCurrentlyDecoded. Currently it means we have all frames of the animation and can access them. After your changes it just means we have decoded the animation frames that are currently in the surface cache once.

This means that basically every assert referencing mIsCurrentlyDecoded (or GetIsCurrentlyDecoded) will now be able to be hit when the decoder is unable to keep up.

This also means that we can hit the case where we can't keep up with the animation when we are decoding frames _after_ the first time we decode an animation. So we'll probably want this code

https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/image/FrameAnimator.cpp#289

to handle that case as well so we don't jerkily jump through the animation when that happens.

Comment 65

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

(In reply to Timothy Nikkel (:tnikkel) from comment #62)
> Comment on attachment 8905622 [details] [diff] [review]
> Part 8. Add AnimatedFrameBuffer to manage storage and decoding of frames in
> an animation., v2
> 
> I think I put my finger on what bothers me about this. When we Insert we are
> relying strictly on timing and the interactions of AnimatedFrameBuffer and
> AnimationSurfaceProvider happening in exactly the right order. If they got
> out of sync or confused we might end up inserting frames for the wrong index
> into the wrong place. If we passed the absolute frame index when inserted we
> could at least assert that we are getting the expected frame and putting it
> in the right place.

Yes. I envisioned AnimatedFrameBuffer and AnimationSurfaceProvider as a cohesive state machine, where AnimatedFrameBuffer tells AnimationSurfaceProvider when to make state transitions based on its return value from operations.

Comment 66

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

(In reply to Timothy Nikkel (:tnikkel) from comment #63)
> Comment on attachment 8905624 [details] [diff] [review]
> Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v3
> 
> >+  // Calculate how many frames we need to decode in this animation before we
> >+  // enter frames-on-demand mode. We are willing to buffer as many frames that
> >+  // can fit inside the given size threshold, bounded by at least as many frames
> >+  // as we are expected to buffer.
> >+  IntSize frameSize = aSurfaceKey.Size();
> >+  size_t threshold =
> >+    (size_t(gfxPrefs::ImageAnimatedDecodeOnDemandThresholdKB()) * 1024) /
> >+    (sizeof(uint8_t) * frameSize.width * frameSize.height);
> 
> So this is assuming we are an 8bit image?
> 

I will correct this. It will check the decoder type and use the appropriate size.

> tion, which is not worth the complexity of
> >@@ -74,33 +78,33 @@ public:
> >-  void CheckForNewFrameAtYield();
> >-  void CheckForNewFrameAtTerminalState();
> >+  bool CheckForNewFrameAtYield();
> >+  bool CheckForNewFrameAtTerminalState();
> 
> Comment what the return value means

Will do.

Comment 67

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

(In reply to Timothy Nikkel (:tnikkel) from comment #64)
> Comment on attachment 8905621 [details] [diff] [review]
> Part 4. Expose new surface provider APIs that will help drive animation
> decoding., v3 [carries r=tnikkel]
> 
> >@@ -66,22 +66,17 @@ AnimationState::UpdateStateInternal(LookupResult& aResult,
> >     // If mHasBeenDecoded is true then we know the true total frame count and
> >     // we can use it to determine if we have all the frames now so we know if
> >     // we are currently fully decoded.
> >     // If mHasBeenDecoded is false then we'll get another UpdateState call
> >     // when the decode finishes.
> >     if (mHasBeenDecoded) {
> >       Maybe<uint32_t> frameCount = FrameCount();
> >       MOZ_ASSERT(frameCount.isSome());
> >-      if (NS_SUCCEEDED(aResult.Surface().Seek(*frameCount - 1)) &&
> >-          aResult.Surface()->IsFinished()) {
> >-        mIsCurrentlyDecoded = true;
> >-      } else {
> >-        mIsCurrentlyDecoded = false;
> >-      }
> >+      mIsCurrentlyDecoded = aResult.Surface().HasFullyDecoded();
> >     }
> >   }
> 
> So this will change the meaning of mIsCurrentlyDecoded. Currently it means
> we have all frames of the animation and can access them. After your changes
> it just means we have decoded the animation frames that are currently in the
> surface cache once.
> 
> This means that basically every assert referencing mIsCurrentlyDecoded (or
> GetIsCurrentlyDecoded) will now be able to be hit when the decoder is unable
> to keep up.
> 
> This also means that we can hit the case where we can't keep up with the
> animation when we are decoding frames _after_ the first time we decode an
> animation. So we'll probably want this code
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/image/FrameAnimator.cpp#289
> 
> to handle that case as well so we don't jerkily jump through the animation
> when that happens.

Are you saying I should revert to the original behaviour? Or something else ? I changed this as per comment 51. What was there originally (IIRC) was mIsCurrentlyDecoded was only if we had fully decoded once, and we did not cross the threshold to discard frames. This, while different, was sufficient to avoid the assert tripping, as it was in a way a weaker condition. The alternative to that (as I see it) is to split depending on what you want to know -- the frame count, or if you expect the frame in question to be there.

Comment 68

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

Attachment: Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v4

Incorporate review feedback.

Comment 70

[Caspy7]

Am I seeing correctly that the thing holding this back are month old patches waiting for review?

Hate to see this die on the vine for such a reason.

Comment 71

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

Attachment: Part 1. Do some unified build accounting, missing headers and namespaces., v2 [carries r=tnikkel]

Comment 72

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

Attachment: Part 2. Expose image decoder type and SourceBuffer to owners., v2 [carries r=tnikkel]

Comment 73

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

Attachment: Part 3. Add preferences to control animated image decoding behaviour., v2 [carries r=tnikkel]

Comment 74

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

Attachment: Part 4. Expose new surface provider APIs that will help drive animation decoding., v4 [carries r=tnikkel]

This incorporates the change discussed, where FrameAnimator::AdvanceFrame needs to update the current time when the frame isn't finished, even if we know the frame count.

Comment 75

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

Attachment: Part 5. Pass the currently displayed frame of an animation to its decoder., v2 [carries r=tnikkel]

Comment 76

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

Attachment: Part 6. Add DecoderFactory::CloneAnimationDecoder to clone an existing image decoder., v2 [carries r=tnikkel]

Comment 77

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

Attachment: Part 8. Add AnimatedFrameBuffer to manage storage and decoding of frames in an animation., v3

Comment 78

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

Attachment: Part 9. Add gtests for AnimationFrameBuffer., v3

Comment 79

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

Attachment: Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v5

Comment 80

[Caspy7]

It's been over a month since patches were submitted.

Are we just waiting on reviews?
At this point has it bitrotted?

Comment 81

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8937791 [details] [diff] [review]
Part 8. Add AnimatedFrameBuffer to manage storage and decoding of frames in an animation., v3

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

::: image/AnimationFrameBuffer.cpp
@@ +89,5 @@
> +      MOZ_ASSERT(!mFrames[mInsertIndex]);
> +      mFrames[mInsertIndex] = Move(aFrame);
> +    }
> +
> +    if (mFrames.Length() - 1 == mThreshold) {

Doesn't the |mFrames.Length() - 1 == mThreshold| check need to be inside the if where we AppendElement? Otherwise we could potentially execute it if we go through the "else if" branch after taking the "if" branch the previous time through and hence doing it twice.

@@ +93,5 @@
> +    if (mFrames.Length() - 1 == mThreshold) {
> +      // We just tripped over the threshold, and on the first pass of the
> +      // decoding; this is our chance to do any clearing of already displayed
> +      // frames. After this, we only need to release as we advance.
> +      MOZ_ASSERT(MayDiscard());

Can we get a sanity assert on mGetIndex here. That it's strictly less than mInsertIndex, correct? And anything else you can think of that should hold.

::: image/AnimationFrameBuffer.h
@@ +126,5 @@
> +   * @returns True if the frame buffer was ever marked as complete. This implies
> +   *          that the total number of frames is known and may be gotten from
> +   *          Frames().Length().
> +   */
> +  bool Complete() const { return mSizeKnown; }

If we just called this SizeKnown that would be clearer all around then no?

Comment 82

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8937795 [details] [diff] [review]
Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v5

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

::: image/AnimationSurfaceProvider.cpp
@@ +38,5 @@
> +                     ? sizeof(uint8_t) : sizeof(uint32_t);
> +
> +  // Calculate how many frames we need to decode in this animation before we
> +  // enter frames-on-demand mode. We are willing to buffer as many frames that
> +  // can fit inside the given size threshold, bounded by at least as many frames

Not sure if this sentence makes sense.

@@ +157,5 @@
> +  return mFrames.Frames()[0]->IsFinished();
> +}
> +
> +bool
> +AnimationSurfaceProvider::IsFullyDecoded() const

Can you document in ISurfaceProvider.h what this function means for animated images? Otherwise it'll be pretty hard to understand it's meaning by having to look at all overrides of it.

@@ +279,3 @@
>  
>      // Append the new frame to the list.
> +    continueDecoding = mFrames.Insert(Move(frame));

The defintion of Insert allows us to Insert the first frame more than once (it is ignored after the first time). So we could hit the frameCount == 1 case more than once, and I don't think we want to call AnnounceSurfaceAvailable more than once.

@@ +319,1 @@
>  

Same comment about Insert and justGotFirstFrame.

@@ +333,5 @@
>  AnimationSurfaceProvider::AnnounceSurfaceAvailable()
>  {
>    mFramesMutex.AssertNotCurrentThreadOwns();
> +  if (!mImage) {
> +    // Not the first pass for the animation decoder.

Ah, okay, so we rely on this to handle the above two comments then.

Comment 83

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8937793 [details] [diff] [review]
Part 9. Add gtests for AnimationFrameBuffer., v3

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

In addition to this I think we need an integration test that tests this code. Something like image/test/mochitest/test_discardAnimatedImage.html, if the prefs are live you can put them suitably low so they activate this code for a smallish gif. I'd like to look at that test, but this testing code is fine so I'll r+ it now.

Also, have you tried a full try server run with the threshold prefs set to very low values so basically all animated images use this new code to see if there are any issues?

Comment 84

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

Attachment: Part 8. Add AnimatedFrameBuffer to manage storage and decoding of frames in an animation., v4 [carries r=tnikkel]

Incorporate review feedback.

Comment 85

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

Attachment: Part 9. Add gtests for AnimationFrameBuffer., v4 [carries r=tnikkel]

Add an additional check for changes in part 8.

Comment 86

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

Attachment: Part 10. Integrate AnimationSurfaceProvider with AnimationFrameBuffer., v6 [carries r=tnikkel]

Incorporate review feedback.

Comment 87

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

Attachment: Part 11. Add integration mochitest. v1

try (default prefs): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ef4886c8a799e6371ee4a98ae39788ce55a5b5c

try (batch size of 1, threshold of 0kB): https://treeherder.mozilla.org/#/jobs?repo=try&revision=66c019e77addf8155b80281d14a63ee54d5fb182

Comment 88

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8954508 [details] [diff] [review]
Part 11. Add integration mochitest. v1

Looks good. Thanks for writing this!

Since there is extra code to request and track discards (of the entire image) you could add another step to this test that discards (the whole image) and then checks that we get the frame updates again to test that image discarding works in conjunction with frame discarding.

Comment 89

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

Attachment: Part 11. Add integration mochitest. v2 [carries r=tnikkel]

Updated test to discard the entire image as well, and repeat the test.

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

I also fixed how the image observers weren't getting removed properly at the test end. This was causing mochitest failures.

Comment 90

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e19edf401e9
Part 1. Do some unified build accounting, missing headers and namespaces. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae96eb62198
Part 2. Expose image decoder type and SourceBuffer to owners. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/17802c1f71ac
Part 3. Add preferences to control animated image decoding behaviour. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b33a66cf6b
Part 4. Expose new surface provider APIs that will help drive animation decoding. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/88ff3387dd49
Part 5. Pass the currently displayed frame of an animation to its decoder. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e96c578f2a9e
Part 6. Add DecoderFactory::CloneAnimationDecoder to clone an existing image decoder. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/209a5b6b89b9
Part 7. Add AnimatedFrameBuffer to manage storage and decoding of frames in an animation. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8170b0d61fc8
Part 8. Add gtests for AnimationFrameBuffer. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf2391a0e6a
Part 9. Integrate AnimationSurfaceProvider with AnimationFrameBuffer. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/58a2ebce44c3
Part 10. Add mochitest for when we discard frames from an animated image. r=tnikkel

Comment 91

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7e19edf401e9
https://hg.mozilla.org/mozilla-central/rev/3ae96eb62198
https://hg.mozilla.org/mozilla-central/rev/17802c1f71ac
https://hg.mozilla.org/mozilla-central/rev/32b33a66cf6b
https://hg.mozilla.org/mozilla-central/rev/88ff3387dd49
https://hg.mozilla.org/mozilla-central/rev/e96c578f2a9e
https://hg.mozilla.org/mozilla-central/rev/209a5b6b89b9
https://hg.mozilla.org/mozilla-central/rev/8170b0d61fc8
https://hg.mozilla.org/mozilla-central/rev/5bf2391a0e6a
https://hg.mozilla.org/mozilla-central/rev/58a2ebce44c3

Comment 94

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #81)
> Comment on attachment 8937791 [details] [diff] [review]
> Part 8. Add AnimatedFrameBuffer to manage storage and decoding of frames in
> an animation., v3
> 
> Review of attachment 8937791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/AnimationFrameBuffer.cpp
> @@ +89,5 @@
> > +      MOZ_ASSERT(!mFrames[mInsertIndex]);
> > +      mFrames[mInsertIndex] = Move(aFrame);
> > +    }
> > +
> > +    if (mFrames.Length() - 1 == mThreshold) {
> 
> Doesn't the |mFrames.Length() - 1 == mThreshold| check need to be inside the
> if where we AppendElement? Otherwise we could potentially execute it if we
> go through the "else if" branch after taking the "if" branch the previous
> time through and hence doing it twice.

It doesn't look like this comment got addressed?

Comment 96

[Ryan VanderMeulen [:RyanVM]]

This was disabled for 60/61 in bug 1454824.

Comment 97

[Ryan VanderMeulen [:RyanVM]]

Bug 1454824 disabled this while 61 was still on trunk, so this was disabled for all future versions going forward as well.

Comment 98

[Caspy7]

Wait, is it intended to be enabled now that trunk is 62?
What's the next step? Who's got the ball?

(I bring this up because I've seen too many weird situations where bugs with patches linger or get forgotten.)

Comment 99

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

(In reply to Caspy7 from comment #98)
> Wait, is it intended to be enabled now that trunk is 62?
> What's the next step? Who's got the ball?
> 
> (I bring this up because I've seen too many weird situations where bugs with
> patches linger or get forgotten.)

I've re-enabled it on inbound today, as well as landing the other fix I needed. I'm committed to seeing these animation improvements through to release, so I'm the one to poke if it stops moving ;).