971946 - Remove TextureClientData

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

TextureClientData is added by Bug 901224. Since Bug 897452 and Bug 962101 are fixed. We do not have a reason to keep TextureClientData anymore.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove TextureClientData

Comment 2

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #0)
> TextureClientData is added by Bug 901224. Since Bug 897452 and Bug 962101
> are fixed. We do not have a reason to keep TextureClientData anymore.

I think (but I might be wrong) that we still have a use for TextureClientData: right now deallocating textures on the client side is synchronous which is bad. fortunately it is only used on the ImageBridge right now but snorp was talking about needing that for SkiaGL canvas too which is on the main thread and can't afford the sync remove.

Anyway, where I am getting at is that if I understand correctly, removing TextureClientData is based on the assumption that the removal is synchronous, and that we just need to wait for the end of the synchronous IPC call to know when to unlock the GraphicBufferLocked (current user of TextureClientData). If we can make the whole thing asynchronous (which I think is not a lot of work) it is worth keeping TextureClientData, especially if SkiaGL will need client-side deallocation.

Comment 3

[Sotaro Ikeda [:sotaro]]

TextureClientData is originally added to defer calling GraphicBufferLocked::Unlock() until sync transaction complete. By Bug 962101, ImageClientSingle::FlushAllImages() synchronously flush a CompositableHost's reference to the Texture if TEXTURE_DEALLOCATE_CLIENT flag is set. After the function call, CompositableHost does not refer to the Texture. GraphicBufferLocked can be destructed and gralloc buffer can be returned to ANativeWindow. We do not need to wait TextureClient's destruction complete to call GraphicBufferLocked::Unlock() any more.

So, I think there is no necessary to keep TextureClientData anymore. Nical, is there other necessity to TextureClientData yet?

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #2)
> 
> Anyway, where I am getting at is that if I understand correctly, removing
> TextureClientData is based on the assumption that the removal is
> synchronous, and that we just need to wait for the end of the synchronous
> IPC call to know when to unlock the GraphicBufferLocked (current user of
> TextureClientData).

After calling ImageClientSingle::FlushAllImages(), no CompositableHost reference to the Texture. No one uses the TextureHost in compositor side. "Unlock the GraphicBufferLocked" is independent from TextureClient's lifetime.

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #2)
> (In reply to Sotaro Ikeda [:sotaro] from comment #0)
> > TextureClientData is added by Bug 901224. Since Bug 897452 and Bug 962101
> > are fixed. We do not have a reason to keep TextureClientData anymore.
> 
> I think (but I might be wrong) that we still have a use for
> TextureClientData: right now deallocating textures on the client side is
> synchronous which is bad. fortunately it is only used on the ImageBridge
> right now but snorp was talking about needing that for SkiaGL canvas too
> which is on the main thread and can't afford the sync remove.

Skia GL case can also be handle by GetForwarder()->RemoveTextureFromCompositable(). TextureClient's destruction could be asyncronous, if there is no concern to out of memory because of it.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> 
> Skia GL case can also be handle by
> GetForwarder()->RemoveTextureFromCompositable(). TextureClient's destruction
> could be asyncronous, if there is no concern to out of memory because of it.

In android case, Binder objects destruction is basically async. To prevent oom/ out of resource problems, the following function is called if there is a such risk. I am not sure similar kind of api is present to gecko ipc.

  > IPCThreadState::self()->flushCommands();

Comment 7

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> (In reply to Sotaro Ikeda [:sotaro] from comment #5)
> > 
> > Skia GL case can also be handle by
> > GetForwarder()->RemoveTextureFromCompositable(). TextureClient's destruction
> > could be asyncronous, if there is no concern to out of memory because of it.
> 
> In android case, Binder objects destruction is basically async. To prevent
> oom/ out of resource problems, the following function is called if there is
> a such risk. I am not sure similar kind of api is present to gecko ipc.
> 
>   > IPCThreadState::self()->flushCommands();

I suppose our equivalent is to mark the current transaction as synchronous, or to send a synchronous message to make sure all of the async messages we sent have been processed when we get control back.

Comment 8

[Nicolas Silva [:nical]]

Another use of TextureClientData came up recently in bug 966543: with MacIOSurface, we can have a race between the creation of the TextureHost and the deletion of the TextureClient;

* child: Create the TextureClient
 * child: Create the textureChild asynchronously
* child: Destroy the TextureClient [1]
 * child: Send PTexture::RemoveTexture message
* parent: Create the TextureParent+TextureHost [2]
* parent: RecvRemoveTexture
 * parent: Send__delete__
* child: Recv__delete__

When [1] occurs, the MacIOSurface is still just a handle in a message and has no reference on the parent side yet, so the shared resource gets destroyed. when the message is processed on the parent side, the shared resource has been destroyed and we get a crash. MacIOSurface has cross-process refcounting semantics a bit like gralloc so we don't explicitly delete, we just drop the reference. so if we can run into [1] happening before [2] we are in trouble, and we may have the same theoretical problem with gralloc.

A solution to this problem is to keep the client side reference to the MacIOSurface alive in a TextureClientData and give it to the TextureChild when the TextureClient dies, which will be deleted after the host, so we are sure we can't have the client side handle destroyed before [2] happens.

Keeping solutions to lifetime problems in TextureClientData has the nice property that it makes all the logic of knowing when to release/deallocate in TextureClient.cpp and TextureHost.cpp.

Comment 9

[Nicolas Silva [:nical]]

I think we do need a mechanism to register a callback that would get run when the TextureChild receives __delete__. TextureClientData fulfilling this purpose at the moment, but we could maybe design a simpler abstraction for the callback (more importantly, less verbose) and get rid of TextureClientData.

Comment 10

[Sotaro Ikeda [:sotaro]]

I agree about the callback. I thinks that the things that TextureClientData is doing should be handled by TextureClient. TextureClientData could be error prone and not a good class from architecture point of view. The role in the architecture is not clear.

In bug 966543 case, the problem seems to be fixed by the followings.
 - Hold TextureClient until TextureHost creation complete
 - Hold TextureClient until (sync/async) releasing TextureHost from CompositableHost

Comment 11

[Sotaro Ikeda [:sotaro]]

On current TextueClient's implementation, TextureClient's shutdown sequence ensure TextureHost's presence. Form it, there needs another object that exists longer than TextureClient. So it's unavoidable to use TextureClientData from this point. Set to WONTFIX.

Comment 12

[Sotaro Ikeda [:sotaro]]

Reopen this bug. When Bug 1000660 is fixed. We could remove TextureClientData.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Remove TextureClientData

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Remove TextureClientData

Comment 15

[Sotaro Ikeda [:sotaro]]

 https://hg.mozilla.org/integration/b2g-inbound/rev/8629e195de4b

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8629e195de4b