Closed Bug 1116733 Opened 7 years ago Closed 7 years ago
Allocate frames off-main-thread in Image
A serious obstacle to supporting multiple decoders at once for the same RasterImage is the amount of communication between the decoding threads and the main thread. Once the synchronization becomes more complex, it will become increasingly difficult to support without creating situations that can produce deadlock. (Especially given the existence of RasterImage::SyncDecode.) The single most problematic interaction decoding threads have with the main thread currently is the allocation of new frames, because right now there's no easy workaround - if a new frame is needed, the decoding thread will have to wait for the main thread, end of story. There's no reason for this, though! When they're initially allocated, imgFrame objects are stored in main memory - not in a texture or anything else that might be tricky off-main-thread on some platforms. We can, and definitely should, support allocating them off-main-thread. (I've confirmed with Bas, and with my own tests, that this will work fine.) Supporting this will also require that many more methods of imgFrame be made threadsafe. Since more locking is required, I've tried to compensate by making some special accessors that return all the values needed for certain tasks (scaling and animation are the two important ones) at once. Especially combined with some restructuring of lock access patterns that I plan to do in FrameAnimator in a followup bug, I expect that we will be able to avoid paying a substantial penalty for this additional locking - indeed, I expect a net improvement in responsiveness when loading pages with many animated images, since all of those extra frames can be allocated off-main-thread and the decoders can just keep crunching. Single-frame images won't touch the new locks much at all, but they will continue to allocate their first frame on the main thread, so they will neither gain nor lose.
Here's the patch. There's a lot here: - Decoder::Write has the meat of the change, which is actually pretty simple! It's the consequences that make up the rest of the patch. The rest of the changes in Decoder are mostly about ensuring that we don't have to take the imgFrame lock several times in a row for no reason - that's why imgFrame now gets told whether it's going to be premultiplied in InitForDecoder(), and why PostFrameStop now calls a single method, imgFrame::Finish(), which sets all of the values at once. - RasterImage just has changes to run OnAddedFrame and OnSurfaceDiscarded asynchronously when they're called from off-main-thread. (And some efficiency-related changes in the scaling code.) - SurfaceCache needs locking on all the methods in its public interface, but fortunately it was designed with that in mind, so it's easy to add. - imgFrame is where most of the complexity is. AnimationData and ScalingData group together the results of many accessors in a single data structure to reduce the number of times we need to take the lock. There is a lot of other refactoring to ensure that the lock gets taken whenever racy data accesses could occur. You'll see that the member variables are partitioned into mutable data that is accessed from multiple threads, immutable data that can be shared freely, and mutable data that can only be accessed on the main thread. All of the locking changes in imgFrame.cpp have been designed paying attention to which member variables are accessed in each method and which category they fall into. - The other classes (the decoders and FrameAnimator) have just been updated to reduce the number of times we need to take the imgFrame lock.
The second part (and the last for this bug) is a refactoring patch that removes code that's unnecessary after part 1. The two main things being removed are: - The DecodeStrategy enumeration, because whether we're sync decoding or async decoding, Decoder still handles frame allocation, and that was all that DecodeStrategy really controlled. - FrameNeededWorker and related code in DecodePool and RasterImage, because there's no more need to ask the main thread for new frames.
Attachment #8542938 - Flags: review?(tnikkel)
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=d348c8eadb0f
This patch needed a rebase because Opacity::TRANSPARENT got renamed to Opacity::SOME_TRANSPARENCY.
It looks like all those failures are from earlier patches in my patch stack. Here's a new try job now that everything below should be green: https://tbpl.mozilla.org/?tree=Try&rev=86abc012b680
Whoops, just realized that on Android we call GetHasAlpha() even though it's removed in this patch. Fixed that.
New try job here: https://tbpl.mozilla.org/?tree=Try&rev=3c7b9e376608
Whoops. That try job didn't include part 2. One more time: https://tbpl.mozilla.org/?tree=Try&rev=f5ea834d5e31
<none> OK, looks like the only remaining failure is for |2d.drawImage.broken.html|, and it's actually an unexpected pass. I've removed the failure annotation file for that test.
Attachment #8545560 - Flags: review?(tnikkel)
One more try job, for linux only, just to make sure I got everything: https://tbpl.mozilla.org/?tree=Try&rev=c477402ce38a
Comment on attachment 8545560 [details] [diff] [review] (Part 1) - Allocate frames off-main-thread Maybe AnimationData and ScalingData can be integrated with RawAccessFrameRef so that the enforcement of holding a RawAccessFrameRef is automatic? Do we not need the mutex in SurfaceCache::CanHold?
Attachment #8545560 - Flags: review?(tnikkel) → review+
Attachment #8542938 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #11) > Maybe AnimationData and ScalingData can be integrated with RawAccessFrameRef > so that the enforcement of holding a RawAccessFrameRef is automatic? Totally! I would love to move in that direction, especially now that we're getting to the point where there's no legacy code left that deals in raw imgFrame pointers. > Do we not need the mutex in SurfaceCache::CanHold? Yeah, we don't, because it doesn't consider what's currently in the SurfaceCache - it just considers the SurfaceCache's maximum size. Insert does the more precise check, but since RasterImage and VectorImage check CanHold() before they allocate frames, we make sure that we never even try to allocate a truly gigantic frame.
Thanks for the review, Timothy! Looks like this is green on try too, so I went ahead and pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c96ef32cd8a5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b4cdc04f6555
Pushed a followup because I forgot to mark a single-argument constructor explicit: https://hg.mozilla.org/integration/mozilla-inbound/rev/c356ab8b348a
https://hg.mozilla.org/mozilla-central/rev/c96ef32cd8a5 https://hg.mozilla.org/mozilla-central/rev/b4cdc04f6555 https://hg.mozilla.org/mozilla-central/rev/c356ab8b348a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.