1293472 - Add an ISurfaceProvider for animated images that also serves as an IDecodingTask

Status: RESOLVED FIXED

(Core :: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Just as bug 1291045 did for single-frame images, let's add an ISurfaceProvider for animated images that also serves as an IDecodingTask. This is a massive step in the direction of completing bug streaming-gif, because once we have all the frames of an animated image managed by a single ISurfaceProvider, we can start producing them dynamically using streaming decoding instead of the current model where we keep them all in memory at once. We won't do that in this bug, though - for now we just want to get the ISurfaceProvider in place and otherwise more or less maintain the status quo.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add AnimationSurfaceProvider.

This part adds the new AnimationSurfaceProvider that we'll start using in part
2. The pattern here is quite similar to DecodedSurfaceProvider, so I'll assume
that most of this is familiar and just point out a few differences:

- We need to add a temporary method, ISurfaceProvider::FrameAtIndex(), to let
  FrameAnimator get the frame it needs out of an AnimationSurfaceProvider. Right
  now calling DrawableRef() on an AnimationSurfaceProvider will MOZ_CRASH(), and
  calling FrameAtIndex() on anything *but* an AnimationSurfaceProvider will
  MOZ_CRASH(). As mentioned in the code, this will be fixed rapidly, because as
  soon as AnimationSurfaceProvider owns the FrameAnimator we won't need this
  anymore. But per comment 0, I'm trying to maintain the status quo as much as I
  can in this bug.

- We can't rely on SurfaceAvailable() to provide synchronization here, because
  that gets called after we have *one* frame available, but we'll continue to
  decode more frames and add them to the frames list, which could result in
  mutation of the frames list at the same time that another thread is reading
  it. Because of this, AnimationSurfaceProvider has a mutex.

- It's explained in the comments, but I think it's worth noting explicitly on
  the bug that we have to lie to the surface cache about the size of the
  AnimationSurfaceProvider for now. There's really no way around this if we
  want to do this refactoring in reasonable-sized bugs. We'll fix this quickly,
  though.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2a) - Modify SurfaceKey to key on whether a surface is static or animated, and not on its specific frame number.

OK, now we need to rework how the surface cache views frames of animated images.

Previously, each cache entry in the surface cache was associated with an
animation time - for raster images, in practice that meant a frame number. Now a
single ISurfaceProvider holds all frames of an animated image, though, so that
doesn't make sense anymore. Instead, this patch introduces a PlaybackType
enumeration. An ISurfaceProvider either has PlaybackType::eStatic, meaning that
it contains a single surface that does not dynamically change, or
PlaybackType::eAnimated, meaning that it "contains" an animated, dynamically
changing surface. Single-frame images will store all their surfaces using
PlaybackType::eStatic. Animated images will generally use
PlaybackType::eAnimated, but may also have a non-animated "view" of themselves
stored as PlaybackType::eStatic. (This would be for cases like
canvas.drawImage() or WebGL.)

One note: we don't store cached surfaces for animated vector images in the
surface cache, and since it's not clear the new scheme would be a good fit for
vector images in any case, I've simply removed the animation time argument from
VectorSurfaceKey.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2b) - Don't update AnimationState for single-frame decodes.

OK, so now a single-frame decode is fundamentally different than an animated
decode. If we want the first frame of an image, we *need* a single-frame decode
(i.e. a DecodedSurfaceProvider), which holds just that frame and always has it
available. If we want to display the image as animated, we *need* an animated
decode (i.e. an AnimationSurfaceProvider), which stores the frames in a format
appropriate for animation (and will soon store the composited frame and so
forth, too).

Since animated images potentially have to deal with both of those cases (though
in the common case they'll only use an AnimationSurfaceProvider) we need to
support running both types of decodes for the same image. This can trigger
issues, though, because we currently assume that we never run a single-frame
decode for an animated image. Most seriously, we don't want to tell our
AnimationState that we're done decoding an animated image just because we've
finished a single-frame decode - that will result in bugs! - but in general
updating AnimationState due to single-frame decodes breaks several of our
assertions. There's no reason to do it, so this patch makes us stop.

Note that the issues described above are actually kinda hard to hit in our
existing tests, as they're quite sensitive to timing in a real browser. You need
a situation where you'd get both types of decodes (e.g. an image that is visible
in an <img> element as well as being drawn to a canvas) and some lucky timing.
For that reason, part 3 adds tests that verify we handle this stuff correctly.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2c) - Update RasterImage to use PlaybackType in surface cache lookups.

This patch updates RasterImage to use the new RasterSurfaceKey for lookups,
which takes a PlaybackType rather than a frame number. This eliminates some
RasterImage helper methods, since we don't need to convert
imgIContainer::FRAME_FIRST/FRAME_CURRENT flags to specific frame numbers, but
rather just to PlaybackType::eStatic/PlaybackType::eAnimated. (The mapping
between those two things is direct, perhaps obviously; see PlaybackType.h.)

A subtle thing is that we don't treat the first frame specially for
PlaybackType::eAnimated: if the first frame is the current frame, we still get
it from FrameAnimator. Only the FRAME_FIRST, where the caller is *insisting* on
the first frame (and hence a non-animated view of the image), doesn't go through
FrameAnimator. We use PlaybackType::eStatic in that case, so the code in
RasterImage::LookupFrameInternal() will direct that lookup to the surface cache
directly. This is a change in the status quo; we used to always direct the first
frame to the surface cache, regardless of animation. You'll see the other side
of this change in part 2f where we update FrameAnimator.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2d) - Trigger single-frame decodes for PlaybackType::eStatic and animated decodes for PlaybackType::eAnimated.

This is the last of the changes to RasterImage. RasterImage::Decode() needs a
PlaybackType argument to know what kind of decoder to create, since now animated
images can have either type, so we just need to set that up.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2e) - Update VectorImage to use the new VectorSurfaceKey signature.

This is a straightforward update to switch to the new VectorSurfaceKey signature
in VectorImage.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2f) - Update FrameAnimator to interact with AnimationSurfaceProvider.

This patch updates FrameAnimator to interact with AnimationSurfaceProvider. The
changes aren't too complicated; they really boil down to two things:

- Rather than looking up specific frames in the SurfaceCache, we now use
  PlaybackType::eAnimated to retrieve a single ISurfaceProvider and then call
  ISurfaceProvider::FrameAtIndex() to get the specific frames.

- As I mentioned in the writeup for an earlier part, we now call into
  FrameAnimator even for the first frame, so the assumption that we don't has
  been eliminated.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2g) - Use AnimationSurfaceProvider for animated images in DecoderFactory.

In this part we actually swap out AnimationDecodingTask and swap in
AnimationSurfaceProvider. AnimationDecodingTask is now unused, so we also remove
it in this patch.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2h) - Remove all interactions with the surface cache from Decoder.

Since Decoder has code to insert surfaces into the surface cache, we'd also have
to update its code to handle the new PlaybackType stuff. However, since both
single-frame and animated images now rely on an ISurfaceProvider to manage their
surfaces, no Decoder actually uses this code. So instead of updating it, let's
just rip it out.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2i) - Update memory reporting and test code to use PlaybackType.

This patch just takes care of a few other places where we need to use
PlaybackType in the memory reporting and test code.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Test that single-frame and animated decodes can coexist for the same image.

As promised in an earlier part, this patch adds tests that single-frame and
animated decodes can run for the same image without interfering with each other.
There are two tests, one that runs the single-frame decode first and the
animated decode second, and one that does the reverse.

Comment 12

[Seth Fowler [:seth] [:s2h]]

I swear this isn't as bad as it looks, guys. I just broke it into smaller pieces this time. =p

Here's a try job:

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

Comment 13

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8779172 [details] [diff] [review]
(Part 2f) - Update FrameAnimator to interact with AnimationSurfaceProvider.

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

::: image/FrameAnimator.cpp
@@ +322,5 @@
> +    return LookupResult(MatchType::NOT_FOUND);
> +  }
> +
> +  // We got the frame. Wrap it in a surface provider and return it to the caller.
> +  MOZ_ASSERT(!rawFrame->GetIsPaletted(), "About to return a paletted frame");

How bad is it to return a paletted frame here? Is it worth returning nullptr in this case (and asserting unreachable)?

Comment 14

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8779176 [details] [diff] [review]
(Part 3) - Test that single-frame and animated decodes can coexist for the same image.

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

::: image/test/gtest/TestDecoders.cpp
@@ +520,5 @@
> +  image->LockImage();
> +
> +  // Use GetFrame() to force a sync decode of the image, specifying
> +  // FRAME_CURRENT to ensure we get an animated decode.
> +  printf_stderr("*** GetFrame FRAME_CURRENT\n");

nit: printf

@@ +568,5 @@
> +  }
> +
> +  // Use GetFrame() to force a sync decode of the image, this time specifying
> +  // FRAME_FIRST to ensure that we get a single-frame decode.
> +  printf_stderr("*** GetFrame FRAME_FIRST\n");

nit: printf

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8779110 [details] [diff] [review]
(Part 1) - Add AnimationSurfaceProvider.

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

r=me with the following addressed:

::: image/AnimationSurfaceProvider.cpp
@@ +84,5 @@
> +    MOZ_ASSERT_UNREACHABLE("Calling IsFinished() on a placeholder");
> +    return false;
> +  }
> +
> +  if (mFrames.Length() == 0) {

Nit: reviewers have told me (and I've internalized) that "mFrames.IsEmpty()" is preferred over "mFrames.Length() == 0", because:
 - it's a little more concise
 - it's more human-readable/declarative
 - why bother having the IsEmpty() convenience method at all, unless we use it in cases like this

@@ +90,5 @@
> +    return false;
> +  }
> +
> +  // As long as we have at least one finished frame, we're finished.
> +  return mFrames[0]->IsFinished();

Superficially, this doesn't seem right.  The IsFinished() documentation says:
  @return true if DrawableRef() will return a completely decoded surface.
https://dxr.mozilla.org/mozilla-central/source/image/ISurfaceProvider.h#43

...and we're (potentially) returning true here -- BUT -- DrawableRef() does *not* return a completely-decoded surface.  It insta-crashes instead. (as you noted in comment 1.  Until a forthcoming patch in a separate bug, at least.)

SO: for now, please broaden this comment to clarify...
 (1) what we actually intend to mean, if/when we return true here. (Do we really aim to mean that DrawableRef can return a completely-decoded surface? or something else?)
 (2) (if needed, in an XXX comment): whether we actually match that intended meaning at this point.

@@ +182,5 @@
> +  MOZ_ASSERT(mDecoder);
> +
> +  RawAccessFrameRef frame = mDecoder->GetCurrentFrameRef();
> +  if (!frame) {
> +    MOZ_ASSERT(mFrames.Length() == 0, "Previous frame disappeared?");

mFrames.IsEmpty()

@@ +186,5 @@
> +    MOZ_ASSERT(mFrames.Length() == 0, "Previous frame disappeared?");
> +    return;
> +  }
> +
> +  if (mFrames.Length() > 0 && mFrames.LastElement().get() == frame.get()) {

!mFrames.IsEmpty()

@@ +226,5 @@
> +  mDecoder = nullptr;
> +
> +  // We don't need a reference to our image anymore, either. Drop it so the
> +  // image doesn't stay alive just because it has live surfaces in the
> +  // surface cache.

Probably best to copypaste (or point to) your updated/clarified version of this comment from DecodedSurfaceProvider::FinishDecoding().  (The version here is the less-clear text that I didn't understand on the other bug, which you clarified nicely over there.)
That nice version of this comment is here, FWIW:
  https://dxr.mozilla.org/mozilla-central/source/image/DecodedSurfaceProvider.cpp#200

::: image/AnimationSurfaceProvider.h
@@ +44,5 @@
> +  bool IsFinished() const override;
> +  size_t LogicalSizeInBytes() const override;
> +
> +protected:
> +  // Animation frames are always locked.

Could you extend this comment slightly, to hint at *why* they are (or need to be) always locked?

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8779111 [details] [diff] [review]
(Part 2a) - Modify SurfaceKey to key on whether a surface is static or animated, and not on its specific frame number.

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

r=me on 2a, assuming we have a reasonable way to avoid the VectorImage issue noted below.

::: image/PlaybackType.h
@@ +20,5 @@
> +enum class PlaybackType : uint8_t
> +{
> +  eStatic,   // Calls to DrawableRef() will always return the same surface.
> +  eAnimated  // An animation; calls to DrawableRef() may return different
> +             // surfaces at different times. 

Nit: drop trailing whitespace on this line.

::: image/SurfaceCache.h
@@ +112,5 @@
>  {
>    // We don't care about aFlags for VectorImage because none of the flags we
>    // have right now influence VectorImage's rendering. If we add a new flag that
>    // *does* affect how a VectorImage renders, we'll have to change this.
> +  return SurfaceKey(aSize, aSVGContext, PlaybackType::eStatic,

If we're treating VectorImages as static now (and only varying based on size + context), won't that mean we'll incorrectly pull stale snapshots of animated VectorImages out of the surface cache?  Or do we have some other mechanism that prevents that (e.g. perhaps to make us bypass the surfacecache entirely for animated VectorImages)?

Probably worth mentioning that mechanism here, since otherwise the "PlaybackType::eStatic" usage looks naive / insufficiently-general-for-all-VectorImages.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8779171 [details] [diff] [review]
(Part 2e) - Update VectorImage to use the new VectorSurfaceKey signature.

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

As for part 2a, r=me on this part as long as we don't end up pulling stale animated-vectorimage surfaces out of the surface cache.

(Perhaps we can/should assert that |this|, the VectorImage, isn't animated, just before each VectorSurfaceKey() instantiation here?)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8779173 [details] [diff] [review]
(Part 2g) - Use AnimationSurfaceProvider for animated images in DecoderFactory.

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

r=me on 2g

Comment 19

[Seth Fowler [:seth] [:s2h]]

Thanks for the reviews! Very helpful.

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #16)
> If we're treating VectorImages as static now (and only varying based on size
> + context), won't that mean we'll incorrectly pull stale snapshots of
> animated VectorImages out of the surface cache?  Or do we have some other
> mechanism that prevents that (e.g. perhaps to make us bypass the
> surfacecache entirely for animated VectorImages)?

It's the latter:

https://dxr.mozilla.org/mozilla-central/source/image/VectorImage.cpp?q=VectorImage.cpp&redirect_type=direct#929

We've talked about supporting it, but support has so far not materialized. =)

Comment 20

[Seth Fowler [:seth] [:s2h]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #13)
> How bad is it to return a paletted frame here? Is it worth returning nullptr
> in this case (and asserting unreachable)?

It's not too bad. It'll trip another assertion in RasterImage::LookupFrame() in debug builds, but in opt builds what'll ultimately happen is we'll just refuse to draw it in imgFrame::Draw():

https://dxr.mozilla.org/mozilla-central/source/image/imgFrame.cpp?q=imgFrame.cpp&redirect_type=direct#544

That's probably the best thing to do, really, because returning nullptr will trigger a redecode that won't fix the problem.

The good news is we will shortly remove support for paletted images totally, and this won't be a concern anymore. =)

Comment 21

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #15)
> Superficially, this doesn't seem right.  The IsFinished() documentation says:
>   @return true if DrawableRef() will return a completely decoded surface.
> https://dxr.mozilla.org/mozilla-central/source/image/ISurfaceProvider.h#43
> 
> ...and we're (potentially) returning true here -- BUT -- DrawableRef() does
> *not* return a completely-decoded surface.  It insta-crashes instead. (as
> you noted in comment 1.  Until a forthcoming patch in a separate bug, at
> least.)
> 
> SO: for now, please broaden this comment to clarify...
>  (1) what we actually intend to mean, if/when we return true here. (Do we
> really aim to mean that DrawableRef can return a completely-decoded surface?
> or something else?)
>  (2) (if needed, in an XXX comment): whether we actually match that intended
> meaning at this point.

I'll update the patch with an improved comment, but just to explain in the bug: FrameAtIndex() is basically DrawableRef(), but hacked to expose the fact that AnimationSurfaceProvider contains many frames. Once we move some code around, we won't need to expose that fact anymore, and then calling code will use DrawableRef() just like all the other ISurfaceProviders. So returning true from IsFinished() is OK. It's the same thing; it's just that we needed a different DrawableRef() API temporarily.

Comment 22

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #17)
> (Perhaps we can/should assert that |this|, the VectorImage, isn't animated,
> just before each VectorSurfaceKey() instantiation here?)

What I ended up doing was adding an explicit check when we check for a cached surface so that we don't even consult the surface cache if the image is animated. This also lets us avoid the expense of taking the lock and traversing the surface cache's data structures in that case, so it's a win-win.

Comment 23

[Seth Fowler [:seth] [:s2h]]

OK, I've resolved all the review issues locally. What remains is to fix the try failures. They're almost all a failure of this assertion in DecodedSurfaceProvider:

MOZ_ASSERT_UNREACHABLE: Unexpected yield for single-frame image

This is probably happening because we're now allowing single-frame decodes of animated images, in the case where we want an ISurfaceProvider with PlaybackType::eStatic, so the assertion isn't valid anymore. I'm puzzled as to why this didn't happen in the ImageLib GTests, as I would've expected the new tests in part 3 to catch this. I'll investigate.

Comment 24

[Seth Fowler [:seth] [:s2h]]

So here are the issues I discovered:

- The "unexpected yield for single-frame image" problem was caused by a bug in the PNG decoder; it yields even for the first frame of an animated image, which may be "hidden" (i.e., not intended to be displayed to the user). This is a backwards compatibility thing, so that browsers which don't support APNG can display something different than browsers which do. At any rate, I've got a patch that makes us handle that situation correctly, so that issue is fixed.

- A large number of the failures were crashes in the memory reporting code. It turns out that the memory reporting code was causing ISurfaceProvider::DrawableRef() even on AnimationSurfaceProviders, causing a crash. I've fixed this by letting ISurfaceProviders handle their memory reporting directly. This is necessary anyway, as with the new model we can't get accurate memory reporting for animated images otherwise.

- One test, test_synchronized_animation.html, relied on canvas.drawImage() synchronously decoding every frame of an animated image, even though canvas only uses the first frame. The patches in this bug make us not do that anymore, which is a performance and memory win but, alas, broke this test. I've filed bug 1295501 to fix this in a followup; we'll disable it for now. You can read the details in that bug, but the gist is that this will get fixed when bug streaming-gif is complete anyway, as a byproduct of further refactoring.

Comment 25

[Seth Fowler [:seth] [:s2h]]

I filed bug 1295506 about the PNG decoder issue, and the test_synchronized_animation.html thing will be folded into part 2i since it's a one-liner. The memory reporting changes will require a new part. To keep all the memory reporting stuff in one patch, I'm moving the existing memory reporting changes *out* of part 2i; they'll all be in the new part 2j.

Comment 26

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add AnimationSurfaceProvider.

OK, here comes a mass reupload. If I don't say anything about a patch, assume
that the only changes are addressing review comments, rebasing, or an OCD need
to keep the patches in the bug in the right order. =)

In part 1, in addition to addressing review comments, I've added additional
locking to prevent reentrant calls to AnimationDecodingTask::Run(), to avoid the
problems seen in bug 1293577.

Comment 27

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2a) - Modify SurfaceKey to key on whether a surface is static or animated, and not on its specific frame number.

Comment 28

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2b) - Don't update AnimationState for single-frame decodes.

Comment 29

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2c) - Update RasterImage to use PlaybackType in surface cache lookups.

Comment 30

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2d) - Trigger single-frame decodes for PlaybackType::eStatic and animated decodes for PlaybackType::eAnimated.

Comment 31

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2e) - Update VectorImage to use the new VectorSurfaceKey signature.

Comment 32

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2f) - Update FrameAnimator to interact with AnimationSurfaceProvider.

Comment 33

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2g) - Use AnimationSurfaceProvider for animated images in DecoderFactory.

Comment 34

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2h) - Remove all interactions with the surface cache from Decoder.

Comment 35

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2i) - Update test code to use PlaybackType.

Per comment 25, I've moved the changes related to memory reporting out of this
patch. Everything left is already r+'d by Edwin, but I'll go ahead and have Nick
rereview the memory reporting changes from the old part 2i as part of part 2j.

Also, somewhat arbitrarily, this is where test_synchronized_animation.html gets
disabled.

Comment 36

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2j) - Delegate memory reporting to the ISurfaceProvider.

This is the new part. The idea is to add a new method,
ISurfaceProvider::AddSizeOfExcludingThis(), which will let ISurfaceProvider
implementations decide how to compute their own size. The default
implementation, used by all ISurfaceProviders except the new
AnimationSurfaceProvider added in this bug, just delegates to
DrawableRef()->AddSizeOfExcludingThis(), just as the code in SurfaceCache.cpp
used to do. AnimationSurfaceProvider instead walks over its list of frames and
calls AddSizeOfExcludingThis() for each one.

There are two other changes, both related to the fact that all of the animation
frames for an image are now owned by a single ISurfaceProvider. Rather than give
frame numbers for individual surfaces, we now just expose that single
ISurfaceProvider in the memory report, tagged with " (animation)". Also, we no
longer expose subframe sizes, because there's no place to put them in the memory
report anymore.

You might say to yourself "Why not have AnimationSurfaceProvider fill out
SurfaceMemoryCounters for each surface it contains? Then we could continue
displaying things like subframe sizes!" In theory, for now, we could. But it's
really not worth the effort, because when bug streaming-gif is complete, we'll
only keep a constant number of frames in memory, and as they'll all be
composited frames (i.e., the "sum" of that frame with all prior frames of the
animation), they'll all be full size and subframe sizes won't be interesting
anymore. So in the end this information just won't be useful to have in the
memory report, even if we set up infrastructure to record it.

Comment 37

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Test that single-frame and animated decodes can coexist for the same image.

Comment 38

[Seth Fowler [:seth] [:s2h]]

Here's a try job with the updated patches. The issues that showed up on the previous try job should all be fixed. Hopefully there are no new ones. =)

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

Comment 39

[Nicholas Nethercote [inactive]]

Comment on attachment 8781430 [details] [diff] [review]
(Part 2j) - Delegate memory reporting to the ISurfaceProvider.

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

Seems plausible, though there's clearly a lot going on around this patch that I'm barely aware of :/

::: image/imgLoader.cpp
@@ +299,5 @@
>        if (counter.Type() == SurfaceMemoryCounterType::NORMAL) {
> +        PlaybackType playback = counter.Key().Playback();
> +        surfacePathPrefix.Append(playback == PlaybackType::eAnimated
> +                                   ? " (animation)"
> +                                   : "");

Nit: I'd align the '?' and ':' with the 'p'. Or introduce an intermediate variable for that string.

Comment 40

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! There is definitely a lot going on in this bug, but hopefully it won't have a big impact on memory reporting. (Other than the need to dive into the ISurfaceProvider to get an accurate report.)

Comment 41

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Make it possible to seek DecodedSurfaces.

Alright, because this whole thing needs to be rebased on top of bug 1296147,
which is actually a bit of a conceptual change, some of this code needs
re-review. Sorry. =\ Part 1 is new, so all the other patches have had their part
number bumped by 1.

Part 1 adds a new method, Seek(), to DrawableSurface. This method enables the
caller to seek a dynamic DrawableSurface (associated with an animation) to a
specific frame. While a DrawableSurface which isn't empty will always have
*some* surface available, Seek() can fail, which means that dereferencing the
DrawableSurface will produce some frame other than the expected one. (In
practice, the first frame.) Having this fallback at least makes it harder for
things to explode, and lets us maintain our guarantee that a DrawableSurface
which isn't empty is dereferencable and drawable.

The tradeoff between the old approach (which used FrameAtIndex(), but the
intention was to eventually merge that with DrawableRef(), so that DrawableRef()
would take a frame number as a parameter) and this one is that the old approach
didn't have a failure mode where you got the wrong frame at all; you either got
the frame you wanted, or none. However, this approach maintains the old behavior
of the surface cache code as much as possible, and lets code which doesn't care
about animation totally ignore the concept. There are advantages and
disadvantages to both approaches, but making the old one work proved to be more
complex than I expected, and we're dealing with enough complexity here as it is.

It might seem like an alternative here would be to pass a frame number to the
surface cache lookup functions, but this is not nearly as simple as it appears.
We don't want to actually do the computation to produce the frame inside the
surface cache lock; laziness is the whole point of DrawableSurface. However, if
we maintained laziness and just stored the desired frame number in the
DrawableSurface, we'd *still* have the failure mode where we couldn't seek to
the desired frame and ended up with the wrong frame. Since callers need to know
about that, they'd still have to call some sort of "IsValid()" method to make
sure the desired frame was available, and since they might want to call it
before dereferencing the DrawableSurface, IsValid() would have to be able to
actually perform the seeking, and at that point we've pretty much reinvented
Seek() but with more complexity and less intuitive performance characteristics.
It seemed to me that this wasn't a good solution.

Comment 42

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add AnimationSurfaceProvider.

Part 2 hasn't changed drastically, but because of the switch to the
DrawableSurface model, FrameAtIndex() is gone and DrawableRef() now takes a
frame number as an argument. This is all expected based upon the previous patch,
and the change isn't huge.  I think it's worth having someone at least glance
over the patch again, though, since this patch *has* changed somewhat and it's
the keystone of this whole bug.

Comment 43

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3a) - Modify SurfaceKey to key on whether a surface is static or animated, and not on its specific frame number.

Starting here there are a bunch of patches which either literally haven't
changed at all or have changed only trivially to be updated to the new API. I
don't think it's worthwhile to rerequest review for them.

Comment 44

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3b) - Don't update AnimationState for single-frame decodes.

Comment 45

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3c) - Update RasterImage to use PlaybackType in surface cache lookups.

Comment 46

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3d) - Trigger single-frame decodes for PlaybackType::eStatic and animated decodes for PlaybackType::eAnimated.

Comment 47

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3e) - Update VectorImage to use the new VectorSurfaceKey signature.

Comment 48

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3f) - Update FrameAnimator to interact with AnimationSurfaceProvider.

Given the switch from the FrameAtIndex() model to the DecodedSurface model, this
patch has changed enough that I think re-review is wise. Edwin, you should
probably take a look at part 1 to get an idea of what the updated API looks
like.

Comment 49

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3g) - Use AnimationSurfaceProvider for animated images in DecoderFactory.

Comment 50

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3h) - Remove all interactions with the surface cache from Decoder.

Comment 51

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3i) - Update test code to use PlaybackType.

Comment 52

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3j) - Delegate memory reporting to the ISurfaceProvider.

Comment 53

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Test that single-frame and animated decodes can coexist for the same image.

Comment 54

[Seth Fowler [:seth] [:s2h]]

New try job here:

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

Comment 55

[Daniel Holbert [:dholbert]]

Comment on attachment 8782328 [details] [diff] [review]
(Part 1) - Make it possible to seek DecodedSurfaces.

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

r=me with nits addressed:

::: image/DecodedSurfaceProvider.cpp
@@ +57,3 @@
>  {
> +    MOZ_ASSERT(aFrame == 0,
> +               "Requesting an animation frame from a DecodedSurfaceProvider?");

Nit: there's bonus indentation here. De-indent these lines by 2 spaces, to align with the rest of the function/file.

::: image/ISurfaceProvider.h
@@ +65,5 @@
>  
> +  /// @return an eagerly computed drawable reference to a surface. For
> +  /// dynamically generated animation surfaces, @aFrame specifies the desired
> +  /// frame.
> +  virtual DrawableFrameRef DrawableRef(size_t aFrame) = 0;

Please broaden the comments:
  s/desired frame/0-based index of the desired frame/

(Note that we still have FRAME_FIRST and FRAME_CURRENT constants defined in imgIContainer; it'd be good to remove any ambiguity about whether this "desired frame" param is supposed to be one of those. I'm pretty sure it's not meant to correspond to those at all.)

@@ +130,5 @@
>    }
>  
> +  /**
> +   * If this DrawableSurface is dynamically generated from an animation,
> +   * attempt to seek to frame @aFrame. Otherwise, nothing will blow up at

As above, please add a parenthetical like "(a 0-based frame index)" here after @aFrame for clarity.

@@ +141,5 @@
> +   * but while nothing will blow up, the frame won't be the one they expect.
> +   */
> +  nsresult Seek(size_t aFrame)
> +  {
> +    MOZ_ASSERT(mHaveSurface);

Please add an explanatory comment here; e.g. "Cannot seek an empty DrawableSurface" (if that's the correct explanation?)

Comment 56

[Daniel Holbert [:dholbert]]

Comment on attachment 8782329 [details] [diff] [review]
(Part 2) - Add AnimationSurfaceProvider.

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

r=me

Comment 57

[Seth Fowler [:seth] [:s2h]]

Thanks so much for those reviews, everyone. I know this bug has produced a huge amount of review work. Thankfully it looks like we've shaken out all of the problems; try is as green as try ever is. =) Just need to fix those comments in part 1 and we should be more or less ready to go.

Comment 58

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Make it possible to seek DecodedSurfaces.

Here's an updated version of part 1 with improved comments. I interpreted your
comment about FRAME_FIRST and FRAME_CURRENT as an explanation of why we should
explicitly state that the @aFrame arguments to these methods are 0-based
indexes, so I didn't actually mention FRAME_FIRST and FRAME_CURRENT in the
updated comments; let me know if I misinterpreted what you meant there.

Comment 59

[Daniel Holbert [:dholbert]]

Yup, you interpreted my comment correctly. :) Thanks!

Comment 60

[Seth Fowler [:seth] [:s2h]]

Alright, let's give this a try. Seems like the sort of bug that's gonna get backed out a couple of times before sticking. Maybe I'll get lucky though. =)

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4e430fbd45
(Part 1) - Make it possible to seek DecodedSurfaces. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/02f9e27b988d
(Part 2) - Add AnimationSurfaceProvider. r=dholbert,edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfb1f1eeceb3
(Part 3) - Store animated images in the surface cache as a sequence of frames, rather than each frame getting its own cache entry. r=dholbert,edwin,njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/856b1b82372a
(Part 4) - Test that single-frame and animated decodes can coexist for the same image. r=edwin

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d4e430fbd45
https://hg.mozilla.org/mozilla-central/rev/02f9e27b988d
https://hg.mozilla.org/mozilla-central/rev/cfb1f1eeceb3
https://hg.mozilla.org/mozilla-central/rev/856b1b82372a