1027088 - Ensure fence delivery for TiledContentClient on gonk

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #1022167 +++

gecko deliver fence object by best effort for TiledContentClient. There is a cases that it is not enough. We need to ensure fence delivery for TiledContentClient.

Basics of fence delivery ensuring is already implemented by Bug 1006957. I am going to use it.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Ensure fence delivery for TiledContentClient on gonk

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Ensure fence delivery for TiledContentClient on gonk

Add OMTC check.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Ensure fence delivery for TiledContentClient on gonk

Fix infinite loop at AsyncTransactionTracker::WaitComplete().

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> Created attachment 8442257 [details] [diff] [review]
> patch v3 - Ensure fence delivery for TiledContentClient on gonk
> 
> Fix infinite loop at AsyncTransactionTracker::WaitComplete().

TextureClientPool::ReturnTextureClient() with RemoveTextureFromCompositableTracker caused infinite loop at AsyncTransactionTracker::WaitComplete(). RemoveTextureFromCompositableTracker needs Compositor side's response. If the WaitComplete() is called before the layer transaction complete, compositor side do not receive a necessary message. attachment 8442257 [details] [diff] [review] fix this problem.

Comment 5

[Sotaro Ikeda [:sotaro]]

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

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 8442257 [details] [diff] [review]
patch v3 - Ensure fence delivery for TiledContentClient on gonk

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

::: gfx/layers/client/TiledContentClient.cpp
@@ +539,5 @@
> +                                                                          mCompositableClient,
> +                                                                          mBackBuffer);
> +      }
> +      // TextureClient can be reused after transaction complete,
> +      // when RemoveTextureFromCompositableTracker is used. 

nit: trailing space

::: gfx/layers/client/TiledContentClient.h
@@ +228,5 @@
>    RefPtr<TextureClient> mFrontBuffer;
>    RefPtr<gfxSharedReadLock> mBackLock;
>    RefPtr<gfxSharedReadLock> mFrontLock;
>    RefPtr<ClientLayerManager> mManager;
> +  RefPtr<CompositableClient> mCompositableClient;

This is creating a cycle, can you prove that it will not leak? If so you should add a comment here about it

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #6)
> Comment on attachment 8442257 [details] [diff] [review]
> patch v3 - Ensure fence delivery for TiledContentClient on gonk
> 
> Review of attachment 8442257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/TiledContentClient.cpp
> @@ +539,5 @@
> > +                                                                          mCompositableClient,
> > +                                                                          mBackBuffer);
> > +      }
> > +      // TextureClient can be reused after transaction complete,
> > +      // when RemoveTextureFromCompositableTracker is used. 
> 
> nit: trailing space
> 
> ::: gfx/layers/client/TiledContentClient.h
> @@ +228,5 @@
> >    RefPtr<TextureClient> mFrontBuffer;
> >    RefPtr<gfxSharedReadLock> mBackLock;
> >    RefPtr<gfxSharedReadLock> mFrontLock;
> >    RefPtr<ClientLayerManager> mManager;
> > +  RefPtr<CompositableClient> mCompositableClient;
> 
> This is creating a cycle, can you prove that it will not leak? If so you
> should add a comment here about it

We can change from Reference pointer to raw pointer. The pointer is used only when the TiledContentClient exist. All TileClients are released during TiledContentClient's destructor.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/TiledContentClient.h#469

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> > 
> > This is creating a cycle, can you prove that it will not leak? If so you
> > should add a comment here about it
> 
> We can change from Reference pointer to raw pointer. The pointer is used
> only when the TiledContentClient exist. All TileClients are released during
> TiledContentClient's destructor.
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> TiledContentClient.h#469

Oh, destructor is too late.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch v4 - Ensure fence delivery for TiledContentClient on gonk

Apply the comment. Confirmed that the patch works on master flame.

Comment 10

[Sotaro Ikeda [:sotaro]]

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

Comment 11

[Sotaro Ikeda [:sotaro]]

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

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e9118a2be9b3