1385101 - Asynchronous paints should hold a ref to backing TextureClients

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

When OMTP is enabled, DrawTargets might outlive their underlying TextureClients. Rather than force a paint flush every time a TextureClient goes away, we should just hold the TextureClient alive asynchronously, and make sure it is released on the main thread.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

TextureClient's destruction process is extremely convoluted, but as far as I can read into it, destruction of the underlying TextureData is initiated in one of two ways. The first way is if the TextureClient has no more outstanding references. The second way is because (for some reason) we forcefully zap all TextureClients when the CompositorBridgeChild shuts down. We probably don't need that code anymore, given work we had to do for the compositor process, but I don't want to change it here.

These two cases both call TextureClient::Destroy, which is the only way to invoke DeallocateTextureClient. This function can destroy the TextureData immediately, or hand off ownership to the associated IPDL actor. The actor then decides whether to free it immediately or later. All of these decisions are based on a bunch of complicated factors.

Anyway, to prevent TextureData from being prematurely freed, we can just make sure TextureClient::Destroy doesn't run during async painting. For the general case, this just means keeping a ref alive. For the latter case, we can keep a count of how many outstanding refs are held by the paint thread, and refuse to run Destroy otherwise if it's non-zero. As an added measure of protection we can flush async paints before shutting down CompositorBridgeChild.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, refactor CapturedPaintState

We're going to need to store TextureClients in CapturedPaintState, but they're only available from ContentClientRemote. There's no clean way to get them from ClientPaintedLayer. I think the easiest thing to do is to make BorrowDrawTargetForRecording responsible for creating the CapturedPaintState. It ends up being a small simplification and we could even move the "captureDT" creation and setup into that function as well. We might even need to do so when it comes time to support component-alpha.

I moved the inline functions into the .cpp file since otherwise we'd have to #include PaintThread.h from RotatedBuffer.h.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, keepalive TextureClients

This patch attaches a separate refcount to TextureClients, intended to track the number of async paint references in progress. It must be incremented on the main thread and decremented on the paint thread. It is only used for assertions, some of which are release asserts but after shipping we should consider dialing them down. We assert that when TextureClient::Destroy is called, there are no outstanding paint thread refs.

In addition we also keep TextureClients alive on the main thread via an array on CompositorBridgeChild. It's cleared when paints are flushed, which is before the next paint begins and whenever the CompositorBridgeChild is shut down. This should ensure that Destroy() is never called with outstanding paint thread refs.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8893241 [details] [diff] [review]
part 2, keepalive TextureClients

Matt, does this patch + approach seem like it will solve the problem? (Problem being that, TextureData needs to be protected while painting asynchronously.)

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

One more note: BorrowDrawTargetForRecording is exposed to ContentClientBasic as well as ContentClientRemote. Meaning in theory, async painting would work if we added it to BasicPaintedLayer. So I made CapturedPaintState::mTextureClient optional.

But probably, we should just make ContentClientBasic not support recording, and require a texture client in CapturedPaintState. I don't think we plan to paint basic layers asynchronously anytime soon?

Comment 6

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8893238 [details] [diff] [review]
part 1, refactor CapturedPaintState

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

Actually yeah, I think this is the right approach. Probably should've done this from the beginning! This also simplifies it for component alpha as we can just throw in the RotatedBuffer::mWhiteBuffer and be done. Also fixed the bug with the wrong region to paint in bug 1383916, so all around yay.

::: gfx/layers/RotatedBuffer.h
@@ +302,5 @@
>     * to the returned DrawTarget, BUT it is required to be painting in the right
>     * location whenever drawing does happen.
>     */
> +  RefPtr<CapturedPaintState> BorrowDrawTargetForRecording(
> +    PaintState& aPaintState,

nit: Whitespace and indenting

::: gfx/layers/client/ContentClient.h
@@ +155,5 @@
>    {
>      return RotatedContentBuffer::BorrowDrawTargetForPainting(aPaintState, aIter);
>    }
> +  virtual RefPtr<CapturedPaintState> BorrowDrawTargetForRecording(
> +    PaintState& aPaintState,

nit: Whitespace

@@ +242,5 @@
>    {
>      return RotatedContentBuffer::BorrowDrawTargetForPainting(aPaintState, aIter);
>    }
> +  virtual RefPtr<CapturedPaintState> BorrowDrawTargetForRecording(
> +    PaintState& aPaintState,

nit: Whitespace

Comment 7

[Mason Chang [Inactive] [:mchang]]

(In reply to David Anderson [:dvander] from comment #5)
> One more note: BorrowDrawTargetForRecording is exposed to ContentClientBasic
> as well as ContentClientRemote. Meaning in theory, async painting would work
> if we added it to BasicPaintedLayer. So I made
> CapturedPaintState::mTextureClient optional.
> 
> But probably, we should just make ContentClientBasic not support recording,
> and require a texture client in CapturedPaintState. I don't think we plan to
> paint basic layers asynchronously anytime soon?

That sounds good to me.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8893241 [details] [diff] [review]
part 2, keepalive TextureClients

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

::: gfx/layers/PaintThread.cpp
@@ +24,5 @@
>  
>  // RAII make sure we clean up and restore our draw targets
>  // when we paint async.
> +struct MOZ_STACK_CLASS AutoCapturedPaintSetup
> +{

TIL MOZ_STACK_CLASS

@@ +55,4 @@
>      }
>    }
>  
> +  CapturedPaintState* mState;

nit: Should this just be a RefPtr just for safety? Any particular reason to hold a raw pointer?

::: gfx/layers/client/ContentClient.cpp
@@ +190,5 @@
> +    return nullptr;
> +  }
> +
> +  cps->mTextureClient = mTextureClient;
> +  cps->mTextureClientOnWhite = mTextureClientOnWhite;

hmm maybe component alpha just works now

::: gfx/layers/client/TextureClient.cpp
@@ +381,5 @@
>  
>  void TextureClient::Destroy()
>  {
> +  // Async paints should have been flushed by now.
> +  MOZ_RELEASE_ASSERT(mPaintThreadRefs == 0);

nit: ASsert Main Thread

@@ +608,5 @@
>  }
>  
>  TextureClient::~TextureClient()
>  {
> +  // TextureClients should be kept alive while there are references on the

nit: Assert Main Thread

::: gfx/layers/ipc/CompositorBridgeChild.cpp
@@ +1169,5 @@
>    mOutstandingAsyncPaints--;
>  
> +  // These textures should be held alive on the main thread. The ref we
> +  // captured should not be the final ref.
> +  MOZ_RELEASE_ASSERT(!aState->mTextureClient || !aState->mTextureClient->HasOneRef());

Are these mixed up? Should they have the !? Don't you mean we have to have a texture client and we should have more than one ref? I don't see where we clear the references to these.

@@ +1175,5 @@
> +  // It's now safe to drop the paint thread ref we're holding, since we've
> +  // flushed writes to the underlying TextureData. Note that we keep the
> +  // main thread ref around until FlushAsyncPaints is called, lazily ensuring
> +  // the Release occurs on the main thread (versus a message in the event
> +  // loop).

Are we guaranteed this during ClientLayerManager shutdown? If someone closes a tab and we don't get another paint call, is it expected that it will cleaned up then? Do we flush in those cases somewhere?

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Mason Chang [:mchang] from comment #8)
> Comment on attachment 8893241 [details] [diff] [review]
> part 2, keepalive TextureClients
>
> @@ +1169,5 @@
> >    mOutstandingAsyncPaints--;
> >  
> > +  // These textures should be held alive on the main thread. The ref we
> > +  // captured should not be the final ref.
> > +  MOZ_RELEASE_ASSERT(!aState->mTextureClient || !aState->mTextureClient->HasOneRef());
> 
> Are these mixed up? Should they have the !? Don't you mean we have to have a
> texture client and we should have more than one ref? I don't see where we
> clear the references to these.

It's asserting that if there is a TextureClient, we should not be holding the last ref. So I think it's right, albeit confusing. I'll simplify it in the next patch.

> @@ +1175,5 @@
> > +  // It's now safe to drop the paint thread ref we're holding, since we've
> > +  // flushed writes to the underlying TextureData. Note that we keep the
> > +  // main thread ref around until FlushAsyncPaints is called, lazily ensuring
> > +  // the Release occurs on the main thread (versus a message in the event
> > +  // loop).
> 
> Are we guaranteed this during ClientLayerManager shutdown? If someone closes
> a tab and we don't get another paint call, is it expected that it will
> cleaned up then? Do we flush in those cases somewhere?

We're not guaranteed this during ClientLayerManager shutdown, but we don't necessarily need to (unless you have a scenario in mind). Textures are tied to the PCompositorBridge, not PLayerTransaction.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, refactor CapturedPaintState

w/ nits picked, also drops ContentClientBasic support for OMTP. Carrying R+ forward.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Mason Chang [:mchang] from comment #8)
> Comment on attachment 8893241 [details] [diff] [review]
> part 2, keepalive TextureClients
>
> ::: gfx/layers/client/TextureClient.cpp
> @@ +381,5 @@
> >  
> >  void TextureClient::Destroy()
> >  {
> > +  // Async paints should have been flushed by now.
> > +  MOZ_RELEASE_ASSERT(mPaintThreadRefs == 0);
> 
> nit: ASsert Main Thread
> 
> @@ +608,5 @@
> >  }
> >  
> >  TextureClient::~TextureClient()
> >  {
> > +  // TextureClients should be kept alive while there are references on the
> 
> nit: Assert Main Thread

The mPaintThreadRefs assertion, combined with the HasOneRef assertion, are together supposed to be approximating an NS_IsMainThread assertion here. It's a little tricky to be more precise since by design TextureClients can be freed on any thread. The assert would trip for video, for example.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, keepalive TextureClients

W/ nits picked and rebased on the previous patch

Comment 13

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8893603 [details] [diff] [review]
part 2, keepalive TextureClients

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

::: gfx/layers/client/TextureClient.cpp
@@ +1719,5 @@
> +TextureClient::DropPaintThreadRef()
> +{
> +  MOZ_RELEASE_ASSERT(PaintThread::IsOnPaintThread());
> +  MOZ_RELEASE_ASSERT(mPaintThreadRefs >= 1);
> +  mPaintThreadRefs -= 1;

nit: Comment with big letters for DRopPaintThreadRef and AddPaintThreadRef, that the callers are expected to own and release the locks.

Comment 14

[Mason Chang [Inactive] [:mchang]]

Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=f06e1971867dca71bcf223e506f7a59cf8273521

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/888580289a11
Part 1 - Create CapturedPaintState in ContentClient rather than ClientPaintedLayer. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/62346afea053
Part 2 Hold TextureClients alive during async painting. r=mattwoodrow,mchang

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/888580289a11
https://hg.mozilla.org/mozilla-central/rev/62346afea053