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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: nical, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Blocks: 912897
Depends on: PTexture
Alias: AvoidManualRemoveTex
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)
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)
Attached patch ptexture-multi-layers (deleted) — Splinter Review
Attachment #8370563 - Flags: review?(nical.bugzilla)
Attachment #8370563 - Flags: review?(sotaro.ikeda.g)
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 on attachment 8370563 [details] [diff] [review] ptexture-multi-layers Looks good.
Attachment #8370563 - Flags: review?(sotaro.ikeda.g) → review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Assignee: nobody → matt.woodrow
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: