Closed Bug 1291045 Opened 3 years ago Closed 3 years ago

Add an ISurfaceProvider for single-frame images that also serves as an IDecodingTask

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(7 files, 8 obsolete files)

5.44 KB, patch
Details | Diff | Splinter Review
6.43 KB, patch
Details | Diff | Splinter Review
11.88 KB, patch
Details | Diff | Splinter Review
3.03 KB, patch
Details | Diff | Splinter Review
3.10 KB, patch
Details | Diff | Splinter Review
10.47 KB, patch
Details | Diff | Splinter Review
9.59 KB, patch
Details | Diff | Splinter Review
For streaming playback of animated images (bug streaming-gif) we need to have an animated image ISurfaceProvider that is also an IDecodingTask. (Perhaps obviously, since streaming playback means that we may need to do some decoding to provide the surface.)

Before we do that, we should make the same change for single-frame images. Here's why:

- There's *lots* of code simplification that we can do if we make both single-frame and animated images behave this way.

- Perhaps really a restating of the above: The SurfaceCache and Decoder code will be a lot easier to understand if we aren't trying to support two different ways of doing things. This will make the life of future maintainers much easier.

- This change will let us fix some long-standing issues which have been very hard to fix with the existing paradigm. In particular, this will make it much easier to avoid displaying almost-but-not-completely-decoded images because of races between painting and the decoders, which hurts performance since we do the work of painting the images and have to repaint them later. It'll also make it possible to cancel image decoders when the surface they're decoding into gets discarded, which is another thing we've wanted for a long time.

- Pragmatically, single-frame images are used more often and we have much more robust test coverage for them. Keeping single-frame images and animated images behaving a similarly as possible means higher code quality for animated images.

- This change is probably the trickiest of the remaining changes needed to implement streaming decoding of animated images, so it makes sense to make it first with single-frame images, which are inherently simpler.
Depends on: 1291054
Depends on: 1291059
Blocks: 1291071
Alright, here we go. This patch series is going to be a bear, but these changes
will be worth it in the long term. (We won't see the biggest benefits until we
land the corresponding change for animated images, unfortunately.)

This part pretty straightforwardly splits DecodingTask in two, so that there is
a version for single-frame images and a version for animated images. This will
allow them to evolve independently from here on out.
Attachment #8776773 - Flags: review?(edwin)
Attachment #8776773 - Flags: review?(dholbert)
Rather than getting the image implicitly from the decoder, let's start passing
it into NotifyProgress() and NotifyDecodeComplete() explicitly. This will let us
store the image on the IDecodingTask directly and let the IDecodingTask be
responsible for its lifetime. This is both part of a general trend of reducing
the number of things that Decoder objects manage (so they can focus on decoding,
and IDecodingTask can focus on orchestrating interactions between the decoders
and other parts of the system) and will enable us to get the lifetimes right
later on when we construct a new IDecodingTask that is also an ISurfaceProvider.
Attachment #8776774 - Flags: review?(edwin)
This patch is a little bit gross but it moves us into the direction we want to
go. Things will look nicer later on in the patch series, don't worry. =) This
isn't the final state.

The idea here is that DecodingTask takes responsibility for inserting the
surfaces generated by the decoder into the SurfaceCache. There are two things
worth noting here:

- At this point in the patch series, this means that DecodingTask has to
  somewhat awkwardly reproduce the error handling that Decoder would've done
  internally when inserting the surfaces into the SurfaceCache. Later in the
  patch series, that won't be an issue.

- This changes *when* a placeholder cache entry is replaced with an available
  cache entry. Now we'll only insert into the surface cache after the decoder
  has yielded (which, for single-frame images, means after it had to block
  because not all the data had arrived from the network yet) or after decoding
  is complete. We used to insert into the surface cache as soon as we allocated
  the frame. This has the good effect that we'll refuse to paint an image that's
  just about to be decoded, only to have to repaint it on the very next refresh
  driver tick; we've wanted to fix that for a while. The tradeoff is that
  certain race conditions involving sync decoding might cause us to decide to
  start a second decoder more often, but in real web browsing sync decoding is
  very rare and race conditions involving it are even rarer, while the double
  paint issue is quite common.
Attachment #8776776 - Flags: review?(edwin)
Attachment #8776776 - Flags: review?(dholbert)
We've fixed all the callers, so we don't need this method anymore. That's good
news, because the new IDecodingTask we're about to implement won't be able to
implement this method. (That's because it'll drop its reference to its decoder
before its lifetime is over.)
Attachment #8776777 - Flags: review?(edwin)
It'll be a lot saner to implement the new, more complicated
IDecodingTask/ISurfaceProvider subclasses in separate files, so let's expose the
notification methods as static methods to make them available to subclasses that
aren't defined in the same translation unit.
Attachment #8776778 - Flags: review?(edwin)
This is the big one. DecodedSurfaceProvider is both an IDecodingTask and an
ISurfaceProvider. Like DecodingTask, it manages a decoder while it runs. It also
owns the resulting surface and makes it accessible via its ISurfaceProvider
interface. Hopefully it's not too complicated to review; it's an evolution of
the version of DecodingTask we created earlier in this sequence of patches,
combined with features from SimpleSurfaceProvider.

One thing that is nice about this approach, and eliminates some of the grossness
from earlier in the patch series, is that we insert the DecodedSurfaceProvider
into the surface cache right away, and just upgrade it from the placeholder
state to the available state later. That means we don't have to do any of that
weird "generating errors in Decoder from outside of Decoder" stuff that we were
doing before.

While decoding, DecodedSurfaceProvider keeps a strong reference to the decoder
(obviously) and to the owning image (so it can send it notifications). Once
decoding is over, though, as an ISurfaceProvider it doesn't need a reference to
either of those things, so we drop those references when decoding is complete.
We have to be somewhat careful about dropping the RasterImage reference, because
DecodedSurfaceProvider's destructor may run because it got evicted from the
surface cache, which would mean that the surface cache lock was currently held.
RasterImage's destructor also calls into the surface cache, so we need to free
the image asynchronously to avoid deadlock.

Another thing worth noting is that, although imgFrame is a threadsafe object,
ISurfaceProviders don't have any internal locking and aren't threadsafe on their
own. That's why it's important that DecodedSurfaceProvider essentially work in
two phases. While it's in the placeholder phase, there is no parallel access of
mutable state; anything that *is* mutable can't be exposed to other threads
during this phase. (e.g. the decoder and the surface may only be touched from
the decoder thread) Once a surface is available and
SurfaceCache::SurfaceAvailable() has been called, it's OK to call methods that
touch the surface (DrawableRef() and IsFinished()) from multiple threads at
once, so it's important that no further changes to |mSurface| occur. (The
underlying imgFrame, being threadsafe, can of course continue to change.)
SetLocked() also touches the surface, although things are a little safer there
since it can only be called from SurfaceCache code. I mention all this to
explain that this has all been thought through and I've put a lot of effort into
making sure that these methods can work with no additional synchronization -
just two periods in which no publicly visible state is mutable, separated by a
call to SurfaceCache::SurfaceAvailable(). Please think it through yourselves and
make sure you don't find any problems I missed. =)
Attachment #8776781 - Flags: review?(edwin)
Attachment #8776781 - Flags: review?(dholbert)
In this patch we just swap out DecodingTask for DecodedSurfaceProvider and do a
little cleanup. As mentioned in the comments for part 6, we're now inserting
DecodedSurfaceProvider into the surface cache right away; there's no more
placeholder in the old sense.
Attachment #8776782 - Flags: review?(edwin)
Attachment #8776782 - Flags: review?(dholbert)
This updated version of part 7 just adds a few more comments in
DecodedSurfaceProvider.h and adds some missing #include's in
DecodedSurfaceProvider.cpp to increase the odds that this will compile without
unified compilation on. =)
Attachment #8777090 - Flags: review?(edwin)
Attachment #8777090 - Flags: review?(dholbert)
Attachment #8776782 - Attachment is obsolete: true
Attachment #8776782 - Flags: review?(edwin)
Attachment #8776782 - Flags: review?(dholbert)
Looking at the try results, all I can say is that I'm genuinely surprised that bug 1291033 seems to have introduced issues, but not this one. Given their relative complexity I would definitely have expected that to go the other way. I'll fix up bug 1291033 and then resubmit a try job for both bugs.
Comment on attachment 8776781 [details] [diff] [review]
(Part 6) - Add DecodedSurfaceProvider to handle both decoding and surface ownership for single-frame images.

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

LGTM on the decoding side of things.
Attachment #8776781 - Flags: review?(edwin) → review+
Comment on attachment 8777090 [details] [diff] [review]
(Part 7) - Replace DecodingTask with DecodedSurfaceProvider.

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

::: image/DecoderFactory.cpp
@@ +140,5 @@
>    if (NS_FAILED(decoder->Init())) {
>      return nullptr;
>    }
>  
> +  // Create a DecodingSurfaceProvider which will manage the decoding process and

nit: DecodedSurfaceProvider
Attachment #8777090 - Flags: review?(edwin) → review+
Comment on attachment 8776776 [details] [diff] [review]
(Part 3) - Handle interactions with the SurfaceCache in DecodingTask.

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

I'll admit to not fully grokking how the pieces of the decoder pipeline fit together (in existing code), so it's possible I'd miss something here; hopefully seth & edwin know that a bit better & would've caught anything that I'd miss here. :)

In any case, this patch seems sensible enough; r=me

::: image/IDecodingTask.h
@@ +82,2 @@
>  
> +  NotNull<RefPtr<RasterImage>> mImage;

Perhaps this new member-var mImage -- and mDecoder (below it) -- should be "const" ("const NotNull<...>"), since they're initialized in the init list, and never change over the lifetime of this object?

Though, looks like this code is getting deleted (moved?) in part 7.  So, maybe better to make this change (if it's applicable) in the final resting-place/replacement for this code.
Attachment #8776776 - Flags: review?(dholbert) → review+
Depends on: 1292505
Comment on attachment 8776781 [details] [diff] [review]
(Part 6) - Add DecodedSurfaceProvider to handle both decoding and surface ownership for single-frame images.

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

r=me on part 6, nits below:

::: image/DecodedSurfaceProvider.cpp
@@ +17,5 @@
> +  , mImage(aImage.get())
> +  , mDecoder(aDecoder.get())
> +  , mSurfaceKey(aSurfaceKey)
> +{
> +  MOZ_ASSERT(!mDecoder->IsMetadataDecode(),

Please make sure this new file includes everything it needs (by putting it in SOURCES instead of UNIFIED_SOURCES, locally, and ensuring it builds)

In particular, you need the following, I think:

#include "Decoder.h"
#include "mozilla/gfx/Point.h" // for mozilla::gfx::IntSize
# include gfxPrefs.h

using mozilla::gfx::IntSize;

@@ +193,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.

I don't entirely understand what this comment is saying.

What's the connection between:
 - us holding vs. dropping this RefPtr<RasterImage>
 - the presence of "live surfaces in the surface cache"?

(The comment seems to be saying there's a direct dependency between those two things, but it's not obvious to me what that is; maybe because I'm forgetting some piece of SurfaceCache-entry-lifetime.)

In any case, consider clarifying this to make that clearer, or adding a parenthetical with a bit of extra context, or something.

::: image/DecodedSurfaceProvider.h
@@ +66,5 @@
> +  void CheckForNewSurface();
> +  void FinishDecoding();
> +
> +  RefPtr<RasterImage> mImage;
> +  RefPtr<Decoder> mDecoder;

Perhaps add a comment here explaining the lifetime of these references? e.g.:
  // non-null until we finish decoding; null thereafter
(I think that's true of both mImage and mDecoder.)

The reason I'm asking is: the .cpp file has various unexplained "MOZ_ASSERT(mImage);" and "MOZ_ASSERT(mDecoder);" assertions, and it's not immediately clear (without a comment like the above) why those assertions are guaranteed to be valid.

Alternately: if you'd rather, perhaps add a message (like the suggested-comment-text above) to the assertions in the .cpp file.
Attachment #8776781 - Flags: review?(dholbert) → review+
Comment on attachment 8777090 [details] [diff] [review]
(Part 7) - Replace DecodingTask with DecodedSurfaceProvider.

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

All of this patch's changes to DecodedSurfaceProvider.cpp / DecodedSurfaceProvider.h belong in the previous patch, I think.  The DecodedSurfaceProvider changes seem to be part of creating/documenting that new class -- they're not part of "Replace DecodingTask" at all.

Seems like this patch here (part 7) should just be about the factory changes and deleting now-unused code.

Nits/observations below. r=me assuming you move the DecodedSurfaceProvider.* changes into part 6.

::: image/DecodedSurfaceProvider.cpp
@@ +9,3 @@
>  #include "nsProxyRelease.h"
>  
> +#include "Decoder.h"

These includes (and another, and a "using" decl) should be added in the previous patch (where this .cpp file is created), technically, per my previous comment.

::: image/DecodedSurfaceProvider.h
@@ +58,5 @@
>    // Full decodes are low priority compared to metadata decodes because they
>    // don't block layout or page load.
>    TaskPriority Priority() const override { return TaskPriority::eLow; }
>  
> +

(random blank line being inserted as part of this patch; intentional?)

@@ +71,3 @@
>    RefPtr<RasterImage> mImage;
> +
> +  /// The decoder that will generate our surface. Dropped after decoding.

Ah, here are the explanatory comments that I was looking for in the previous patch! OK. :)

Really, all of the changes to this file belong in the previous patch, I think... (They're part of creating/documenting this class, not part of "Replace DecodingTask" at all really.)
Attachment #8777090 - Flags: review?(dholbert) → review+
(Sorry, I repeated myself a bit in that last comment; poor editing/copypasting on my part.)
Thanks for the reviews, folks! Going through the comments now that try looks good. (I'll post a new try job, but I want to include the changes from the review comments, too, so let me address those first.)

(In reply to Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) from comment #14)
> Perhaps this new member-var mImage -- and mDecoder (below it) -- should be
> "const" ("const NotNull<...>"), since they're initialized in the init list,
> and never change over the lifetime of this object?
> 
> Though, looks like this code is getting deleted (moved?) in part 7.  So,
> maybe better to make this change (if it's applicable) in the final
> resting-place/replacement for this code.

Yeah, this code doesn't exist by the end of the patch series in this bug, so probably OK to let this slide. =)
(In reply to Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) from comment #16)
> Comment on attachment 8777090 [details] [diff] [review]
> (Part 7) - Replace DecodingTask with DecodedSurfaceProvider.
> 
> Review of attachment 8777090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All of this patch's changes to DecodedSurfaceProvider.cpp /
> DecodedSurfaceProvider.h belong in the previous patch, I think.  The
> DecodedSurfaceProvider changes seem to be part of creating/documenting that
> new class -- they're not part of "Replace DecodingTask" at all.
> 
> Seems like this patch here (part 7) should just be about the factory changes
> and deleting now-unused code.

That's what was intended, yeah. =) When I made the changes in comment 9, looks like I mistakenly put them in part 7 instead of part 6.

So what I'll do is move that stuff into part 6, address your concern about the comment mentioned in your review for part 6 (the one about surfaces in the surface cache keeping images alive), and make sure that the new file builds in non-unified builds. I believe that should take care of everything. (Since it sounds like the other concerns from the part 6 review are addressed in the changes that mistakenly ended up in part 7.)
Reuploading all the patches. All review comments were addressed. I also fixed a
small issue in part 2 and rebased against bug 1292505. (The latter being the
reason I'm just reuploading everything.)
Attachment #8776773 - Attachment is obsolete: true
Attachment #8776774 - Attachment is obsolete: true
Attachment #8776776 - Attachment is obsolete: true
Attachment #8776777 - Attachment is obsolete: true
Attachment #8776778 - Attachment is obsolete: true
Attachment #8776781 - Attachment is obsolete: true
Attachment #8777090 - Attachment is obsolete: true
Pushed by seth.bugzilla@blackhail.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41ecafc57524
(Part 1) - Use a different IDecodingTask for animated images. r=dholbert,edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/50ac73a27eb3
(Part 2) - Pass the image into NotifyProgress() and NotifyDecodeComplete() explicitly. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/4999ae42eaec
(Part 3) - Handle interactions with the SurfaceCache in DecodingTask. r=dholbert,edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/c31142f750f8
(Part 4) - Remove IDecodingTask::GetDecoder(). r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/7466366ce986
(Part 5) - Expose the IDecodingTask notification helpers for use in other files. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e140998c35
(Part 6) - Add DecodedSurfaceProvider to handle both decoding and surface ownership for single-frame images. r=dholbert,edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5418ecab2a9
(Part 7) - Replace DecodingTask with DecodedSurfaceProvider. r=dholbert,edwin
Regressions: 1572672
You need to log in before you can comment on or make changes to this bug.