1462355 - Avoid mutex contention between FrameAnimator/Decoders

Status: RESOLVED FIXED

(Core :: ImageLib, enhancement, P3)

Description

[Andrew Osmond [:aosmond]]

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.

Comment 1

[Andrew Osmond [:aosmond]]

Attachment: 0001-Bug-1462355-Part-1a.-Make-imgFrame-animation-paramet.patch, v1

Comment 2

[Andrew Osmond [:aosmond]]

Attachment: 0002-Bug-1462355-Part-1b.-Update-Decoder-and-SurfacePipe-.patch, v1

Comment 3

[Andrew Osmond [:aosmond]]

Attachment: 0003-Bug-1462355-Part-1c.-Make-individual-image-decoders-.patch, v1

Comment 4

[Andrew Osmond [:aosmond]]

Attachment: 0004-Bug-1462355-Part-2.-Expose-imgFrame-s-data-pointers-.patch, v1

Comment 5

[Andrew Osmond [:aosmond]]

Attachment: 0005-Bug-1462355-Part-3.-Make-FrameAnimator-use-the-new-i.patch, v1

Comment 6

[Andrew Osmond [:aosmond]]

Attachment: 0006-Bug-1462355-Part-4.-Remove-imgFrame-GetAnimationData.patch, v1

Comment 7

[Andrew Osmond [:aosmond]]

Attachment: 0007-Bug-1462355-Part-5.-Avoid-converting-from-DrawableFr.patch, v1

Comment 8

[Andrew Osmond [:aosmond]]

Attachment: 0008-Bug-1462355-Part-6.-Reuse-RawAccessFrameRef-in-Frame.patch, v1

Comment 9

[Andrew Osmond [:aosmond]]

Attachment: 0009-Bug-1462355-Part-7.-Don-t-hit-the-SurfaceCache-in-Fr.patch, v1

Comment 10

[Andrew Osmond [:aosmond]]

Attachment: 0010-Bug-1462355-Part-8.-Avoid-allocating-on-the-heap-in-.patch, v1

Comment 11

[Andrew Osmond [:aosmond]]

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

Comment 12

[Andrew Osmond [:aosmond]]

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

Comment 13

[Andrew Osmond [:aosmond]]

Attachment: 0001-Bug-1462355-Part-1a.-Make-imgFrame-animation-paramet.patch, v2

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

Comment 14

[Andrew Osmond [:aosmond]]

Attachment: 0002-Bug-1462355-Part-1b.-Update-Decoder-and-SurfacePipe-.patch, v2

Comment 15

[Andrew Osmond [:aosmond]]

Attachment: 0003-Bug-1462355-Part-1c.-Make-individual-image-decoders-.patch, v2

Comment 16

[Timothy Nikkel (: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.

Comment 17

[Andrew Osmond [:aosmond]]

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

Comment 18

[Timothy Nikkel (:tnikkel)]

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?

Comment 19

[Andrew Osmond [:aosmond]]

Attachment: 0001-Bug-1462355-Part-1a.-Make-imgFrame-animation-paramet.patch, v3 [carries r=tnikkel]

Incorporate review feedback.

Comment 20

[Andrew Osmond [:aosmond]]

Attachment: 0004-Bug-1462355-Part-2.-Expose-imgFrame-s-data-pointers-.patch, v2

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

Comment 21

[Andrew Osmond [:aosmond]]

Attachment: 0011-Bug-1462355-Part-9.-Lock-animated-imgFrame-objects-a.patch, v1

Comment 22

[Timothy Nikkel (:tnikkel)]

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.

Comment 23

[Andrew Osmond [:aosmond]]

Attachment: 0008-Bug-1462355-Part-6.-Reuse-RawAccessFrameRef-in-Frame.patch, v2

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

Comment 24

[Andrew Osmond [:aosmond]]

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

Comment 25

[Andrew Osmond [:aosmond]]

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

Comment 26

[Andrew Osmond [:aosmond]]

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/259ee94de92c
https://hg.mozilla.org/mozilla-central/rev/87a0a6e103d8
https://hg.mozilla.org/mozilla-central/rev/eb8ec97c5f5f
https://hg.mozilla.org/mozilla-central/rev/3ea8add8bacc
https://hg.mozilla.org/mozilla-central/rev/45406c2b9e9c
https://hg.mozilla.org/mozilla-central/rev/93bdeed04a6b
https://hg.mozilla.org/mozilla-central/rev/c67a6f1315b4
https://hg.mozilla.org/mozilla-central/rev/a7331d229cc8
https://hg.mozilla.org/mozilla-central/rev/baeab7da1768
https://hg.mozilla.org/mozilla-central/rev/8d412f560489
https://hg.mozilla.org/mozilla-central/rev/9b516954e103