Recycle TextureClient of ImageClient

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla36
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox34 wontfix, firefox35 wontfix, firefox36 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(2 attachments, 17 obsolete attachments)

27.91 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
28.71 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
+++ This bug was initially created as a clone of Bug #1091240 +++

This bug is created based on Bug 1091240 Comment 20. During animated png rendering, new CairoImage is allocated, copy image data to it and destroyed for every rendering.

copy image seems unavoidable until Bug 980770 is fixed. But TextureClient allocation problem seems to be addressed by recycling.
Assignee

Updated

5 years ago
Assignee: nobody → sotaro.ikeda.g
Severity: critical → normal
Assignee

Comment 1

5 years ago
The patch basically works. But need to add more memory efficiency.
Assignee

Comment 2

5 years ago
[Blocking Requested - why for this release]: This bug blocks Bug 1091240( b2g v2.1+ bug).
blocking-b2g: --- → 2.1?
Assignee

Updated

5 years ago
Blocks: 1043558
blocking-b2g: 2.1? → 2.1+
Assignee

Comment 3

5 years ago
Fix recycling logic problem.
Attachment #8514605 - Attachment is obsolete: true
Assignee

Comment 4

5 years ago
Add comments and asserts.
Attachment #8515134 - Attachment is obsolete: true
Assignee

Comment 5

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> Created attachment 8515177 [details] [diff] [review]

It support only gonk now.
Assignee

Comment 6

5 years ago
Remove platform dependency.
Attachment #8515177 - Attachment is obsolete: true
Assignee

Comment 7

5 years ago
Make TextureClient's reference count stable not to disrupt recycling.
Attachment #8515273 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8515388 - Attachment description: patch - Add TextureClient recycling to CairoImage → patch for b2gv2.1 - Add TextureClient recycling to CairoImage
Assignee

Updated

5 years ago
Attachment #8515388 - Attachment description: patch for b2gv2.1 - Add TextureClient recycling to CairoImage → patch - Add TextureClient recycling to CairoImage
Assignee

Comment 8

5 years ago
Fix debug build failure.
Attachment #8515388 - Attachment is obsolete: true
Assignee

Comment 9

5 years ago
Need to add TextureFlags handling.
Assignee

Comment 10

5 years ago
Enable Shmem alloc/dealloc.
Attachment #8515409 - Attachment is obsolete: true
Assignee

Comment 12

5 years ago
On window, TextureClient does not hold a reference pointer of ISurfaceAllocator, it does not need it. Therefore, current patch does not work on window.

And In current gecko, only gonk implementation ensures that when TextureClient is recycled, the TextureClient is not used on Host side.
Assignee

Updated

5 years ago
Blocks: 1093110
Assignee

Comment 14

5 years ago
Add TextueClient's reset flag capability. Add some assertions.
Attachment #8515418 - Attachment is obsolete: true
Assignee

Comment 15

5 years ago
Fix debug build problem.
Attachment #8516129 - Attachment is obsolete: true
Assignee

Comment 16

5 years ago
Fix incorrect assert.
Attachment #8516218 - Attachment is obsolete: true
Assignee

Comment 17

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=04738eaa3d26

Need to change ASSERT check.
Assignee

Comment 18

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> https://tbpl.mozilla.org/?tree=Try&rev=04738eaa3d26
> 
> Need to change ASSERT check.

TextureClientRecycleAllocator had a problem.
Assignee

Comment 19

5 years ago
Flag handling fix.
Attachment #8516349 - Attachment is obsolete: true
Assignee

Comment 20

5 years ago
Fix recycling flag handling.
Attachment #8516701 - Attachment is obsolete: true
Assignee

Comment 21

5 years ago
attachment 8516741 [details] [diff] [review] has a problem to change flags after TextureClient recycling.
Assignee

Comment 22

5 years ago
Fix CairoImage change.
Attachment #8516741 - Attachment is obsolete: true
Assignee

Comment 23

5 years ago
Address comment 21.
Attachment #8516796 - Attachment is obsolete: true
Assignee

Comment 25

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> https://tbpl.mozilla.org/?tree=Try&rev=cbe5c52959ab

test failures seems addressed. Current patch enabled recycling except windows, but it need to be disabled except gonk. On current gfx layers, TextureClient's recycling does not mean TextureHosts unused except gonk. It need to be handled as a different problem.
Assignee

Comment 26

5 years ago
Limit the recycling only to gonk.
Attachment #8516866 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8517493 - Flags: review?(nical.bugzilla)
Comment on attachment 8517493 [details] [diff] [review]
patch - Add TextureClient recycling to CairoImag

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

First round of comment as I go through the patch.

I would much prefer that we keep certain properties of the textures such as the flags immutable.
We could do this by destroying the TextureClient but not the shared data (underlying GrallocBuffer, or shmem, etc.) and re-create a TextureClient/Host pair around the data. I prefer this because we have a mechanism in place to make sure there is no race when destroying textures and with some of the texture states like the flags, and this patch adds ways to dodge the system. Instead I would prefer that we use the deallocation handshake as a way to prevent races when reusing textures just like it prevents races when destroying them. I also think it would be simpler since we wouldn't need to hack around the ref counting of TextureClient (TextureClientHolder in this patch). What I am proposing would be very easy to do if we had bug 1027602.

That said, we could refactor the recycling mechanism in a followup and land your patch as it is now if this needs to land soon. I am overbooked right now and I don't want to block other people too much. But If we do this I'd like to know that we agree about coming back to refactor it afterwards, and that we don't build other mechanisms on top of things like being able to change the texture flags.
Assignee

Comment 28

5 years ago
(In reply to Nicolas Silva [:nical] from comment #27)
> 
> I would much prefer that we keep certain properties of the textures such as
> the flags immutable.
> We could do this by destroying the TextureClient but not the shared data
> (underlying GrallocBuffer, or shmem, etc.) and re-create a
> TextureClient/Host pair around the data.

I do not prefer this approach, because It means TextureClient/TextureHost and TextureChild/TextureParent recreation. It does not save cpu time enough. nical Can you explain more about why it is prefereble?

When TextureClient is recycled and if the recycling mechanism ensure that TexutreHost is also not used, there should be no race condition to the flag, I think. It is ensured only on gonk in current gecko.
Flags: needinfo?(nical.bugzilla)
Assignee

Comment 29

5 years ago
As in Bug 1039937, just recreating TextureClient for every video frame and reusing gralloc caused the performance problem. Therefore, nical's approach does not work well on b2g devices.
(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> As in Bug 1039937, just recreating TextureClient for every video frame and
> reusing gralloc caused the performance problem. Therefore, nical's approach
> does not work well on b2g devices.

I wasn't aware of this. I retract the proposition about re-creacting the TextureClient without recreating the gralloc buffer, then. I still don't understand your patch fully so I'll get back to you soon.

One of the main reasons I preferred relying on the shutdown handshake is that TextureClient is using it's reference count to drive its lifetime and and prevent races, which makes some of the things that used to be buggy and complex, simple and have the nice property that some things "just work" such as managing the lifetime of a texture used by several layers. So I am a bit resistant to change some of these properties. But again, if there are perf issues with recreating the actor, then it's a different story.
Flags: needinfo?(nical.bugzilla)
Assignee

Comment 31

5 years ago
(In reply to Nicolas Silva [:nical] from comment #30)
> One of the main reasons I preferred relying on the shutdown handshake is
> that TextureClient is using it's reference count to drive its lifetime and
> and prevent races, which makes some of the things that used to be buggy and
> complex, simple and have the nice property that some things "just work" such
> as managing the lifetime of a texture used by several layers. So I am a bit
> resistant to change some of these properties. But again, if there are perf
> issues with recreating the actor, then it's a different story.

This hand shake mechanism between TextureHost and TextureClient for recycling is not correctly implemented yet.

On gonk, some handshake is provided by RemoveTextureFromCompositableTracker. On gonk, RemoveTextureFromCompositableTracker is necessary to correctly deliver android::Fence from TextureHost to TextureClient. But it is used very inefficient way and semantics is different from recycling handshake. I am going to address a part of RemoveTextureFromCompositableTracker's problem in Bug 1093110.
Assignee

Comment 32

5 years ago
(In reply to Nicolas Silva [:nical] from comment #27)
> also think it would be simpler since
> we wouldn't need to hack around the ref counting of TextureClient
> (TextureClientHolder in this patch). What I am proposing would be very easy
> to do if we had bug 1027602.

I do not think that TextureClientHolder is too ugly hack. TextureClient's recycling necessities exist mainly around ImageLayer and media framework. In this area, only reference count is the hint of recycling timing.
Assignee

Comment 33

5 years ago
The Texture flags handlings are most dirty thing in the patch. It seems necessary to be changed better than current one.
Comment on attachment 8517493 [details] [diff] [review]
patch - Add TextureClient recycling to CairoImag

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

I am missing some things, some questions below:

::: gfx/layers/client/TextureClientRecycleAllocator.cpp
@@ +185,5 @@
> +    MOZ_ASSERT(mInUseClients.find(textureHolder->GetTextureClient()) == mInUseClients.end());
> +    // Register TextureClient
> +    mInUseClients[textureHolder->GetTextureClient()] = textureHolder;
> +  }
> +  textureHolder->GetTextureClient()->SetRecycleCallback(TextureClientRecycleAllocatorImp::RecycleCallback, this);

Where is the declaration/implementation of TextureClient::SetRecycleCallback? I don't see it in the patch nor in mozilla-central.

I also haven't found yet what actually invokes the callback, I'd like to understand how this works if a texture is used by several layers.

::: gfx/layers/composite/TextureHost.cpp
@@ +806,5 @@
> +bool
> +TextureParent::RecvResetFlags(const TextureFlags& aTextureFlags)
> +{
> +  if (!mTextureHost) {
> +    return true;

In what case is this branch supposed to be taken?

::: gfx/layers/ipc/PTexture.ipdl
@@ +40,5 @@
>       * Asynchronously tell the Compositor side to remove the texture.
>       */
>      async RemoveTexture();
> +
> +    async ResetFlags(TextureFlags aTextureFlags);

I would prefer that this to be renamed into RecycleTexture, to make it explicit why we are sending this message. Also I don't want code other than the recycling mechanism to change the textrue flags while a texture is used so I'd rather not give the impression that we support changing the flags as a feature of textures (rather its that we reset the texture when recycling it).
(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> I do not think that TextureClientHolder is too ugly hack. TextureClient's
> recycling necessities exist mainly around ImageLayer and media framework. In
> this area, only reference count is the hint of recycling timing.

Yeah sorry about the TextureClientHolder, at a second look it's not what I thought it was at the time I wrote that comment.
Assignee

Comment 36

5 years ago
(In reply to Nicolas Silva [:nical] from comment #34)
> Comment on attachment 8517493 [details] [diff] [review]
> patch - Add TextureClient recycling to CairoImag
> 
> Review of attachment 8517493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am missing some things, some questions below:
> 
> ::: gfx/layers/client/TextureClientRecycleAllocator.cpp
> @@ +185,5 @@
> > +    MOZ_ASSERT(mInUseClients.find(textureHolder->GetTextureClient()) == mInUseClients.end());
> > +    // Register TextureClient
> > +    mInUseClients[textureHolder->GetTextureClient()] = textureHolder;
> > +  }
> > +  textureHolder->GetTextureClient()->SetRecycleCallback(TextureClientRecycleAllocatorImp::RecycleCallback, this);
> 
> Where is the declaration/implementation of
> TextureClient::SetRecycleCallback? I don't see it in the patch nor in
> mozilla-central.

It is implemented by AtomicRefCountedWithFinalize.
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/AtomicRefCountedWithFinalize.h#83

> I also haven't found yet what actually invokes the callback, I'd like to
> understand how this works if a texture is used by several layers.

TextureClient's callback is called when TextureClient's reference count becomes one. The last one reference is by recycler. If multiple layers are using one TexureClient, its reference count is more than one. And when all layers stop to reference TextureClient, the TextureClient is recycled. It works even when multiple layers are using one TextureClient.

> ::: gfx/layers/composite/TextureHost.cpp
> @@ +806,5 @@
> > +bool
> > +TextureParent::RecvResetFlags(const TextureFlags& aTextureFlags)
> > +{
> > +  if (!mTextureHost) {
> > +    return true;
> 
> In what case is this branch supposed to be taken?

Logically, this situation should not happen. It was added to protect from chrome process crash. It is cleared by TextureParent::ClearTextureHost(). The ClearTextureHost() is called from the following. malformed client might call SendClearTextureHostSync().
 - TextureParent::RecvClearTextureHostSync()
 - TextureParent::ActorDestroy()

> 
> ::: gfx/layers/ipc/PTexture.ipdl
> @@ +40,5 @@
> >       * Asynchronously tell the Compositor side to remove the texture.
> >       */
> >      async RemoveTexture();
> > +
> > +    async ResetFlags(TextureFlags aTextureFlags);
> 
> I would prefer that this to be renamed into RecycleTexture, to make it
> explicit why we are sending this message. Also I don't want code other than
> the recycling mechanism to change the textrue flags while a texture is used
> so I'd rather not give the impression that we support changing the flags as
> a feature of textures (rather its that we reset the texture when recycling
> it).

Agree, such kind of name is better.
(In reply to Sotaro Ikeda [:sotaro] from comment #36)
> It is implemented by AtomicRefCountedWithFinalize.
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/
> AtomicRefCountedWithFinalize.h#83

Ah! thanks, I am starting to connect the dots.

> TextureClient's callback is called when TextureClient's reference count
> becomes one. The last one reference is by recycler. If multiple layers are
> using one TexureClient, its reference count is more than one. And when all
> layers stop to reference TextureClient, the TextureClient is recycled. It
> works even when multiple layers are using one TextureClient.
> 

Yes, makes sense now.
Comment on attachment 8517493 [details] [diff] [review]
patch - Add TextureClient recycling to CairoImag

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

Sorry for the long review cycle. R+ with nits, I'd really like that the ResetFlags message get renamed as discussed in previous comments.
Attachment #8517493 - Flags: review?(nical.bugzilla) → review+
Assignee

Comment 39

5 years ago
Apply the comments. Carry "r=nical".
Attachment #8517493 - Attachment is obsolete: true
Assignee

Comment 40

5 years ago
Update nits. Carry "r=nical".
Attachment #8521605 - Attachment is obsolete: true
Attachment #8521615 - Flags: review+
Assignee

Comment 44

5 years ago
Sorry, I did mistake to un-bitrot the patch:-(
Assignee

Comment 45

5 years ago
Fix build failure.
Attachment #8521615 - Attachment is obsolete: true
Assignee

Comment 46

5 years ago
Comment on attachment 8522267 [details] [diff] [review]
patch - Add TextureClient recycling to CairoImag

Carry 'r=nical'.
Attachment #8522267 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/42f47a2b3eb5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Please request b2g34 approval on this when you get a chance.
Flags: needinfo?(sotaro.ikeda.g)
Assignee

Comment 50

5 years ago
Patch for b2g v2.1. Carry "r=nical".
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8523027 - Flags: review+
Assignee

Comment 52

5 years ago
[Blocking Requested - why for this release]: Set from "2.1+" to "2.1?". WebRTC bug (Bug 1091240) was already fixed by other bug with low risk change. Therefor, there seems no necessity to uplift this bug's fix to b2gv1.2 with some risks.
blocking-b2g: 2.1+ → 2.1?
That works for me - I am going to track it as a 2.2+ blocker, because we need it for bug 1043558.
blocking-b2g: 2.1? → 2.2+
Assignee

Updated

5 years ago
Blocks: 1095106
Assignee

Comment 54

5 years ago
[Blocking Requested - why for this release]:
blocking-b2g: 2.2+ → 2.1?
Assignee

Comment 55

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #54)
> [Blocking Requested - why for this release]:

 like Bug 1095106, WebRTC still need to reduce cpu usage.
blocking-b2g: 2.1? → 2.1+
Please request b2g34 approval on the v2.1 patch when you get a chance.
Flags: needinfo?(sotaro.ikeda.g)
Assignee

Comment 57

5 years ago
Comment on attachment 8523027 [details] [diff] [review]
patch for b2gv2.1 - Add TextureClient recycling to CairoImag

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: more high cpu usage during WebRTC.
Testing completed: locally tested.
Risk to taking this patch (and alternatives if risky): low.
String or UUID changes made by this patch:
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8523027 - Flags: approval-mozilla-b2g34?
Attachment #8523027 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
You need to log in before you can comment on or make changes to this bug.