Closed
Bug 1291045
Opened 8 years ago
Closed 8 years ago
Add an ISurfaceProvider for single-frame images that also serves as an IDecodingTask
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
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.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
Try job here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0245f8c7498
Attachment #8776773 -
Flags: review?(edwin) → review+
Attachment #8776774 -
Flags: review?(edwin) → review+
Attachment #8776777 -
Flags: review?(edwin) → review+
Attachment #8776778 -
Flags: review?(edwin) → review+
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8776782 -
Attachment is obsolete: true
Attachment #8776782 -
Flags: review?(edwin)
Attachment #8776782 -
Flags: review?(dholbert)
Assignee | ||
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
Bug 1291033 looks good. Here's a new try job for this bug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6565339a1ee
Attachment #8776776 -
Flags: review?(edwin) → review+
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+
Updated•8 years ago
|
Attachment #8776773 -
Flags: review?(dholbert) → review+
Comment 14•8 years ago
|
||
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+
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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+
Comment 17•8 years ago
|
||
(Sorry, I repeated myself a bit in that last comment; poor editing/copypasting on my part.)
Assignee | ||
Comment 18•8 years ago
|
||
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. =)
Assignee | ||
Comment 19•8 years ago
|
||
(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.)
Assignee | ||
Comment 20•8 years ago
|
||
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.)
Assignee | ||
Updated•8 years ago
|
Attachment #8776773 -
Attachment is obsolete: true
Attachment #8776774 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 21•8 years ago
|
||
Assignee | ||
Comment 22•8 years ago
|
||
Assignee | ||
Comment 23•8 years ago
|
||
Assignee | ||
Comment 24•8 years ago
|
||
Assignee | ||
Comment 25•8 years ago
|
||
Assignee | ||
Comment 26•8 years ago
|
||
Assignee | ||
Comment 27•8 years ago
|
||
New try job here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fd086c6db29
Comment 28•8 years ago
|
||
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
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/41ecafc57524 https://hg.mozilla.org/mozilla-central/rev/50ac73a27eb3 https://hg.mozilla.org/mozilla-central/rev/4999ae42eaec https://hg.mozilla.org/mozilla-central/rev/c31142f750f8 https://hg.mozilla.org/mozilla-central/rev/7466366ce986 https://hg.mozilla.org/mozilla-central/rev/a6e140998c35 https://hg.mozilla.org/mozilla-central/rev/b5418ecab2a9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•