Closed
Bug 926745
(AvoidManualRemoveTex)
Opened 11 years ago
Closed 11 years ago
Do not call RemoveTextureClient manually in compositable code
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: nical, Assigned: mattwoodrow)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
nical
:
review+
sotaro
:
review+
|
Details | Diff | Splinter Review |
Bug 912040 raised the problem of forcing TextureClient removal regardless of its reference count, and we ended up doing the short-term solution of marking TextureClients invalid when doing that, so that if we try to access the TextureClient's data after sending the OpRemoveTexture message, we fail to lock it which lets the caller know that he is not allowed to access the shared data.
We still need to remove calls to RemoveTextureClient in compositable code though. The main feature of the new TextureClient/Host is that their lifetime is managed by reference counting of the TextureClient, and those calls to RemoveTextureClient break that.
This will be a good step in the direction of fixing bug 912907.
PTexture (Bug 897452) makes it much easier for us to get rid of the RemoveTextureClient calls because it removes some of the lifetime restrictions of Textures.
Reporter | ||
Updated•11 years ago
|
Updated•11 years ago
|
Blocks: BadSurfaceDescriptor
Updated•11 years ago
|
Alias: AvoidManualRemoveTex
Assignee | ||
Comment 1•11 years ago
|
||
What actually needs to be done here?
It is at as simple as just removing the ForceRemove() calls, and letting the reference count decrement do all the work? Or are there cases where we hang on to a reference but still need the IDPL parts destroyed immediately?
Flags: needinfo?(nical.bugzilla)
Reporter | ||
Comment 2•11 years ago
|
||
You just need to keep the TextureClient in an array that you clear after the transaction and let ref count do the rest. otherwise you will see flickering because the texture deletion races with the swap in the transaction.
Except for Flush allImages that only happens at the end of a video stream, where because of the decoding library on b2g, we need to get back all of the gralloc buffers asap so you may want to leave ForceRemove there (I am not 100% sure, Sotaro knows better what needs to be done in this case).
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8370563 -
Flags: review?(nical.bugzilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8370563 -
Flags: review?(sotaro.ikeda.g)
Reporter | ||
Comment 4•11 years ago
|
||
Comment on attachment 8370563 [details] [diff] [review]
ptexture-multi-layers
Review of attachment 8370563 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/client/CanvasClient.cpp
@@ +52,5 @@
> CanvasClient2D::Update(gfx::IntSize aSize, ClientCanvasLayer* aLayer)
> {
> if (mBuffer &&
> (mBuffer->IsImmutable() || mBuffer->GetSize() != aSize)) {
> + GetForwarder()->DelayedRemoveTexture(mBuffer);
I would prefer to name this HoldUntilTransaction to better show what is happening but I don't have a very strong opinion.
Or RemoveAfterTransaction.
Attachment #8370563 -
Flags: review?(nical.bugzilla) → review+
Comment 5•11 years ago
|
||
Comment on attachment 8370563 [details] [diff] [review]
ptexture-multi-layers
Looks good.
Attachment #8370563 -
Flags: review?(sotaro.ikeda.g) → review+
Assignee | ||
Comment 6•11 years ago
|
||
Comment 7•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•11 years ago
|
Assignee: nobody → matt.woodrow
You need to log in
before you can comment on or make changes to this bug.
Description
•