1116733 - Allocate frames off-main-thread in ImageLib

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

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.

Comment 1

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

Attachment: (Part 1) - Allocate frames off-main-thread

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.

Comment 2

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

Attachment: (Part 2) - Remove DecodeStrategy and frame allocation handling outside of Decoder

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.

Comment 3

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

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=d348c8eadb0f

Comment 4

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

Attachment: (Part 1) - Allocate frames off-main-thread

This patch needed a rebase because Opacity::TRANSPARENT got renamed to Opacity::SOME_TRANSPARENCY.

Comment 5

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

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

Comment 6

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

Attachment: (Part 1) - Allocate frames off-main-thread

Whoops, just realized that on Android we call GetHasAlpha() even though it's removed in this patch. Fixed that.

Comment 7

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

New try job here:

https://tbpl.mozilla.org/?tree=Try&rev=3c7b9e376608

Comment 8

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

Whoops. That try job didn't include part 2. One more time:

https://tbpl.mozilla.org/?tree=Try&rev=f5ea834d5e31

Comment 9

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

Attachment: (Part 1) - Allocate frames off-main-thread

<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.

Comment 10

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

One more try job, for linux only, just to make sure I got everything:

https://tbpl.mozilla.org/?tree=Try&rev=c477402ce38a

Comment 11

[Timothy Nikkel (:tnikkel)]

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?

Comment 12

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

(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.

Comment 13

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

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

Comment 14

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

Pushed a followup because I forgot to mark a single-argument constructor explicit:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c356ab8b348a

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c96ef32cd8a5
https://hg.mozilla.org/mozilla-central/rev/b4cdc04f6555
https://hg.mozilla.org/mozilla-central/rev/c356ab8b348a