Allocate frames off-main-thread in ImageLib

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

4 years ago
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.
Assignee

Updated

4 years ago
Depends on: 1116735
Assignee

Comment 1

4 years ago
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.
Attachment #8542936 - Flags: review?(tnikkel)
Assignee

Comment 2

4 years ago
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)
Assignee

Updated

4 years ago
Blocks: 1116742
Assignee

Updated

4 years ago
Blocks: 1117607
Assignee

Comment 4

4 years ago
This patch needed a rebase because Opacity::TRANSPARENT got renamed to Opacity::SOME_TRANSPARENCY.
Attachment #8545478 - Flags: review?(tnikkel)
Assignee

Updated

4 years ago
Attachment #8542936 - Attachment is obsolete: true
Attachment #8542936 - Flags: review?(tnikkel)
Assignee

Comment 5

4 years ago
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
Assignee

Comment 6

4 years ago
Whoops, just realized that on Android we call GetHasAlpha() even though it's removed in this patch. Fixed that.
Attachment #8545483 - Flags: review?(tnikkel)
Assignee

Updated

4 years ago
Attachment #8545478 - Attachment is obsolete: true
Attachment #8545478 - Flags: review?(tnikkel)
Assignee

Comment 8

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

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

Comment 9

4 years ago
<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)
Assignee

Updated

4 years ago
Attachment #8545483 - Attachment is obsolete: true
Attachment #8545483 - Flags: review?(tnikkel)
Assignee

Comment 10

4 years ago
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+
Assignee

Comment 12

4 years ago
(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.
Assignee

Comment 13

4 years ago
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
Assignee

Comment 14

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

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

Updated

4 years ago
Depends on: 1120036

Updated

4 years ago
Depends on: 1119938
Depends on: 1120279
Assignee

Updated

4 years ago
Depends on: 1121297
Assignee

Updated

4 years ago
Depends on: 1126739
You need to log in before you can comment on or make changes to this bug.