Avoid mutex contention between FrameAnimator/Decoders

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

({perf})

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(11 attachments, 6 obsolete attachments)

21.30 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.31 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
18.60 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.50 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
9.11 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
42.81 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
10.62 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
14.28 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
6.88 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.11 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
11.69 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
Mutex contention was showing up in my profiling of animated images. We do a lot of unnecessary locking and the following should be fixed:

1) imgFrame's animation parameters are effectively write once, but we set them in imgFrame::Finish when we should be doing it in imgFrame::InitForDecoder so we can remove the lock on them entirely.

2) We want a RawAccessFrameRef in FrameAnimator but the way DrawableSurface works is that we can only get DrawableFrameRef from it. That is an extra round of locking for no real reason.

3) RawAccessFrameRef guarantees there is a data pointer available; it should just save it in the object itself, to avoid locking again just to get that pointer. If all of the other parameters are lockfree, then it just boils down to getting the first reference.

4) We ask for a new RawAccessFrameRef a lot more than we need to, such as when getting the timeout for a frame we will just ask for again later. These should be consolidated.

5) On a related papercut note, I've also observed DrawableFrameRef's malloc appearing in submitted profiles from others. While I doubt it is really significant, we can easily remove it by making ScopedMap work properly with Maybe.

6) We hit the SurfaceCache in FrameAnimator::GetCompositedFrame, even when we don't use the result at all, which is most of the time. We should avoid that.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P3
Whiteboard: [gfx-noted]
Comment on attachment 8976557 [details] [diff] [review]
0006-Bug-1462355-Part-4.-Remove-imgFrame-GetAnimationData.patch, v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39b438c1ff2f9dab757fe85c151013139a5b834a
Attachment #8976557 - Flags: review?(tnikkel)
(In reply to Andrew Osmond [:aosmond] from comment #11)
> Comment on attachment 8976557 [details] [diff] [review]
> 0006-Bug-1462355-Part-4.-Remove-imgFrame-GetAnimationData.patch, v1
> 
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=39b438c1ff2f9dab757fe85c151013139a5b834a

The QR reftest failures are WebRender specific. I was originally allocating the first frame of an animated image in a SourceSurfaceSharedData buffer, which doesn't need to be uploaded to the compositor process via TextureHost. I changed this (I recall thinking this, but not believing it would matter) because it now decides it is an animated frame if it gets animated parameters, rather than being a frame other than the first. Using the fallback path, TextureHost doesn't support such large images, and thus it doesn't appear in the reftest. I can switch this back with a bit of bit fiddling.

However the investigation into this revealed downscaling can be improved. Instead of allocating the image into a 32k x 100 for a 100x100 display size, we really ought to downscale to 100x100. More generally, we should support downscaling as long as one of the dimensions fits (e.g. if 200x200 was requested, we should downscale to 200x100). Any stretching done by the drawing should not look any worse as a result of the downscaling, we can reduce the amount of work drawing has to do, and we can reduce our memory footprint to boot. I'll take this as part of a new bug.
Fix reftest failure by moving the frame number into AnimationParams. This allows imgFrame to access it easily and make the same decision regarding type of SourceSurface to store the data in (as well as abstracting the notion of frame numbers away from decoders that don't support animations).
Attachment #8976552 - Attachment is obsolete: true
Attachment #8976552 - Flags: review?(tnikkel)
Attachment #8976688 - Flags: review?(tnikkel)
Attachment #8976553 - Attachment is obsolete: true
Attachment #8976553 - Flags: review?(tnikkel)
Attachment #8976689 - Flags: review?(tnikkel)
Attachment #8976554 - Attachment is obsolete: true
Attachment #8976554 - Flags: review?(tnikkel)
Attachment #8976690 - Flags: review?(tnikkel)
Comment on attachment 8976688 [details] [diff] [review]
0001-Bug-1462355-Part-1a.-Make-imgFrame-animation-paramet.patch, v2

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

::: image/imgFrame.h
@@ +100,5 @@
>                             SurfaceFormat aFormat)
>    {
> +    nsIntRect frameRect(0, 0, aSize.width, aSize.height);
> +    AnimationParams animParams { frameRect, FrameTimeout::Forever(),
> +                                 /* aFrameNum */ 0, BlendMethod::OVER,

Passing AnimationParams with framenum == 0 means we change the value of the bool we pass  AllocateBufferForImage. Before we passed aIsAnimated (== true in this situation), now we pass false because we are pretending this is the first frame.
(In reply to Timothy Nikkel (:tnikkel) from comment #16)
> Comment on attachment 8976688 [details] [diff] [review]
> 0001-Bug-1462355-Part-1a.-Make-imgFrame-animation-paramet.patch, v2
> 
> Review of attachment 8976688 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/imgFrame.h
> @@ +100,5 @@
> >                             SurfaceFormat aFormat)
> >    {
> > +    nsIntRect frameRect(0, 0, aSize.width, aSize.height);
> > +    AnimationParams animParams { frameRect, FrameTimeout::Forever(),
> > +                                 /* aFrameNum */ 0, BlendMethod::OVER,
> 
> Passing AnimationParams with framenum == 0 means we change the value of the
> bool we pass  AllocateBufferForImage. Before we passed aIsAnimated (== true
> in this situation), now we pass false because we are pretending this is the
> first frame.

Right you are; I will change it to 1. It isn't super harmful since InitForAnimator is just used by FrameAnimator for two frames, but it will unnecessarily chew up an extra file handle on Android, and use a shared surface with WebRender that is never used by the compositor process.
Attachment #8976688 - Flags: review?(tnikkel) → review+
Attachment #8976689 - Flags: review?(tnikkel) → review+
Attachment #8976690 - Flags: review?(tnikkel) → review+
Comment on attachment 8976555 [details] [diff] [review]
0004-Bug-1462355-Part-2.-Expose-imgFrame-s-data-pointers-.patch, v1

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

::: image/imgFrame.h
@@ +128,5 @@
>                              uint32_t aImageFlags,
>                              gfx::BackendType aBackend);
>  
>    DrawableFrameRef DrawableRef();
> +  RawAccessFrameRef RawAccessRef(bool aOnlyFinished = false);

Comment what aOnlyFinished means please.

@@ +508,4 @@
>    }
>  
> +  uint8_t* Data() const { return mData; }
> +  uint32_t DataLength() const { return mFrame->GetImageDataLength(); }

GetImageDataLength calls GetImageBytesPerRow which asserts we are in the monitor, but what guarantees that we have the monitor here?
Attachment #8976557 - Flags: review?(tnikkel) → review+
Incorporate review feedback.
Attachment #8976688 - Attachment is obsolete: true
Attachment #8976936 - Flags: review+
(In reply to Timothy Nikkel (:tnikkel) from comment #18)
> Comment on attachment 8976555 [details] [diff] [review]
> 0004-Bug-1462355-Part-2.-Expose-imgFrame-s-data-pointers-.patch, v1
> 
> Review of attachment 8976555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/imgFrame.h
> @@ +128,5 @@
> >                              uint32_t aImageFlags,
> >                              gfx::BackendType aBackend);
> >  
> >    DrawableFrameRef DrawableRef();
> > +  RawAccessFrameRef RawAccessRef(bool aOnlyFinished = false);
> 
> Comment what aOnlyFinished means please.
> 

Done.

> @@ +508,4 @@
> >    }
> >  
> > +  uint8_t* Data() const { return mData; }
> > +  uint32_t DataLength() const { return mFrame->GetImageDataLength(); }
> 
> GetImageDataLength calls GetImageBytesPerRow which asserts we are in the
> monitor, but what guarantees that we have the monitor here?

Hm correct. I removed DataLength() as it wasn't used. My brain thought it made sense in a completionist sense.
Attachment #8976938 - Flags: review?(tnikkel)
Attachment #8976555 - Attachment is obsolete: true
Attachment #8976555 - Flags: review?(tnikkel)
Blocks: 1337111
Attachment #8976556 - Flags: review?(tnikkel) → review+
Attachment #8976938 - Flags: review?(tnikkel) → review+
Attachment #8976558 - Flags: review?(tnikkel) → review+
Comment on attachment 8976559 [details] [diff] [review]
0008-Bug-1462355-Part-6.-Reuse-RawAccessFrameRef-in-Frame.patch, v1

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

::: image/FrameAnimator.h
@@ +347,5 @@
>     *          advanced, and its resulting dirty rect.
>     */
>    RefreshResult AdvanceFrame(AnimationState& aState,
>                               DrawableSurface& aFrames,
> +                             RawAccessFrameRef& aCurrentFrame,

Comment the in/out param behaviour of this please.
(In reply to Timothy Nikkel (:tnikkel) from comment #22)
> Comment on attachment 8976559 [details] [diff] [review]
> 0008-Bug-1462355-Part-6.-Reuse-RawAccessFrameRef-in-Frame.patch, v1
> 
> Review of attachment 8976559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/FrameAnimator.h
> @@ +347,5 @@
> >     *          advanced, and its resulting dirty rect.
> >     */
> >    RefreshResult AdvanceFrame(AnimationState& aState,
> >                               DrawableSurface& aFrames,
> > +                             RawAccessFrameRef& aCurrentFrame,
> 
> Comment the in/out param behaviour of this please.

Done.
Attachment #8976559 - Attachment is obsolete: true
Attachment #8976559 - Flags: review?(tnikkel)
Attachment #8980679 - Flags: review?(tnikkel)
Attachment #8980679 - Flags: review?(tnikkel) → review+
Attachment #8976560 - Flags: review?(tnikkel) → review+
Attachment #8976561 - Flags: review?(tnikkel) → review+
Attachment #8980258 - Flags: review?(tnikkel) → review+
Lots of parts here, so let's try not to need to back this out ;).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e41490f947ec1e4bb98e08ff72b8b7fe18262c2d
(In reply to Andrew Osmond [:aosmond] from comment #24)
> Lots of parts here, so let's try not to need to back this out ;).
> 
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e41490f947ec1e4bb98e08ff72b8b7fe18262c2d

Ugh. Speaking of failures. Maybe cannot be copied if it is in a function parameter? Ugh. This may be a tricky one to fix.
Fixed this by passing the filter config structs by const reference. Seems to have done the trick, in addition to saving some unnecessary copies on the stack :).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccf9f30b0cf2272df1db7c0ec61a69ce7d2e2700
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/259ee94de92c
Part 1a. Make imgFrame animation parameters threadsafe. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a0a6e103d8
Part 1b. Update Decoder and SurfacePipe plumbing to use updated imgFrame methods. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8ec97c5f5f
Part 1c. Make individual image decoders to use updated Decoder/SurfacePipe methods. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea8add8bacc
Part 2. Expose imgFrame's data pointers via RawAccessFrameRef. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/45406c2b9e9c
Part 3. Make FrameAnimator use the new imgFrame/RawAccessFrameRef methods. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/93bdeed04a6b
Part 4. Remove imgFrame::GetAnimationData as it is no longer used. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/c67a6f1315b4
Part 5. Avoid converting from DrawableFrameRef to RawAccessFrameRef. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7331d229cc8
Part 6. Reuse RawAccessFrameRef in FrameAnimator where possible. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/baeab7da1768
Part 7. Don't hit the SurfaceCache in FrameAnimator::GetCompositedFrame if possible. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d412f560489
Part 8. Avoid allocating on the heap in DrawableFrameRef. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b516954e103
Part 9. Lock animated imgFrame objects at creation rather than deferring. r=tnikkel
You need to log in before you can comment on or make changes to this bug.