Closed Bug 1091777 Opened 10 years ago Closed 10 years ago

Recycle TextureClient of ImageClient

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36
Project Flags:
blocking-b2g 2.1+
Tracking Flags:
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 17 obsolete files)

patch - Add TextureClient recycling to CairoImag
27.91 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
patch for b2gv2.1 - Add TextureClient recycling to CairoImag
28.71 KB, patch
sotaro
: review+
bajaj
: approval-mozilla-b2g34+
Details | Diff | Splinter Review 
+++ 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: nobody → sotaro.ikeda.g
Severity: critical → normal
The patch basically works. But need to add more memory efficiency.
[Blocking Requested - why for this release]: This bug blocks Bug 1091240( b2g v2.1+ bug).
blocking-b2g: --- → 2.1?
Blocks: 1043558
blocking-b2g: 2.1? → 2.1+
Fix recycling logic problem.
Attachment #8514605 - Attachment is obsolete: true
Add comments and asserts.
Attachment #8515134 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> Created attachment 8515177 [details] [diff] [review]

It support only gonk now.
Remove platform dependency.
Attachment #8515177 - Attachment is obsolete: true
Make TextureClient's reference count stable not to disrupt recycling.
Attachment #8515273 - Attachment is obsolete: true
Attachment #8515388 - Attachment description: patch - Add TextureClient recycling to CairoImage → patch for b2gv2.1 - Add TextureClient recycling to CairoImage
Attachment #8515388 - Attachment description: patch for b2gv2.1 - Add TextureClient recycling to CairoImage → patch - Add TextureClient recycling to CairoImage
Fix debug build failure.
Attachment #8515388 - Attachment is obsolete: true
Need to add TextureFlags handling.
Enable Shmem alloc/dealloc.
Attachment #8515409 - Attachment is obsolete: true
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.
Blocks: 1093110
Add TextueClient's reset flag capability. Add some assertions.
Attachment #8515418 - Attachment is obsolete: true
Fix debug build problem.
Attachment #8516129 - Attachment is obsolete: true
Fix incorrect assert.
Attachment #8516218 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=04738eaa3d26

Need to change ASSERT check.
(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.
Flag handling fix.
Attachment #8516349 - Attachment is obsolete: true
Fix recycling flag handling.
Attachment #8516701 - Attachment is obsolete: true
attachment 8516741 [details] [diff] [review] has a problem to change flags after TextureClient recycling.
Fix CairoImage change.
Attachment #8516741 - Attachment is obsolete: true
Address comment 21.
Attachment #8516796 - Attachment is obsolete: true
(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.
Limit the recycling only to gonk.
Attachment #8516866 - Attachment is obsolete: true
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.
(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)
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)
(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.
(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.
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.
(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+
Apply the comments. Carry "r=nical".
Attachment #8517493 - Attachment is obsolete: true
Update nits. Carry "r=nical".
Attachment #8521605 - Attachment is obsolete: true
Attachment #8521615 - Flags: review+
Sorry, I did mistake to un-bitrot the patch:-(
Fix build failure.
Attachment #8521615 - Attachment is obsolete: true
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Please request b2g34 approval on this when you get a chance.
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox34: --- → wontfix
status-firefox35: --- → wontfix
status-firefox36: --- → fixed
Flags: needinfo?(sotaro.ikeda.g)
Patch for b2g v2.1. Carry "r=nical".
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8523027 - Flags: review+
[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+
Blocks: 1095106
[Blocking Requested - why for this release]:
blocking-b2g: 2.2+ → 2.1?
(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)
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.

Attachment

General

Creator:
Created:
Updated:
Size: