879681 - Remove PGrallocBuffer protocol

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Instead we should only use a protocol to create/delete the gralloc buffer. The parent/child don't necessarily want the same life time of a gralloc buffer so it shouldn't be tied to an actor.

The plan is to

1. Remove PGrallocBuffer
2. Create a protocol to create/delete gralloc buffer
3. Wrap the buffer in a SurfaceDescriptorXXX when we need to pass it to the other side.

I think this could simplify the life time handling of gralloc buffers.

Comment 1

[Nicolas Silva [:nical]]

I agree that we should simplify gralloc buffer handling. My understanding is hat the lifetime of gralloc buffers is somewhat tied to their IPDL actor, although if someone keeps a sp<android::GraphicBuffer> around they can outlive the actor. Plus, the actor themselves are not reference counted so we have to manually ensure that we don't leak them, which kinda defeats the purpose of ref counting GraphicBuffer (since the actor has a ref pointer to the GraphicBuffer, leaking the actor leaks the bufer as well...).

My problem with gralloc is that we manage it the same way we do for other memory types (which don't have IPDL actors), Which forces us to special case B2G code paths more than we should and write code that works everywhere but B2G if we aren't very careful.

That said having an actor has one nice advantage: if the content process crashes the actor gets notified and we can handle it at the level of the texture and free memory properly. If textures don't have their own IPDL actors we have to propagate cleaning up from the Compositable (which is not so bad right now).
In the future we will need to share textures between compositables which makes things a bit trickier without an actor per texture. Plus, enforcing that a texture doesn't outlive it's compositable(s) is a bit fiddly in some cases.

So, I agree that we should remove PGrallocBuffer.
If we decide to add an IPDL actor in the future, it will be for all texture types (not just gralloc), and it should be an implementation detail of TextureClient/TextureHost.

I don't think we should add a new protocol, though. I think it should be a message in PImageBridge and PLayers (other wise you'd have to add a protocol managed by both PImageBurdge and PLayers but I don't see what this would get us).

Comment 2

[Nicolas Silva [:nical]]

actually, I take back what i said about adding a new protocol. I didn't realize it when i commented earlier, but I suppose you propose this so that we can allocate gralloc buffers in the compositor process but not in the compositor thread so as to not be delayed when the compositor thread is busy. So adding a protocol on a different thread makes a lot of sense.

Comment 3

[Chiajung Hung [:chiajung]]

I think the target of this issue is to remove the burden to manage PGrallocBufferActors as they do not really reflect the life time of GraphicBuffer.

Since we can retrieve the sp<GraphicBuffer> after IPC communication done, delete the PGrallocBufferActor does not reduce the underlying reference count at all, but cause various problem.

The idea here is to make GraphicBuffer keep track the life time by android::sp directly, which should make things simpler and avoid most PGrallocBufferActors related crash ideally.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

Createing a separate protocol for gralloc has the advantage of not cluttering the PImageBridge and PLayers protocol. Separating it from the compositor thread was not my primary concern since 1. we still need to wait the compositor to get the image back 2. we cannot allocate infinite gralloc buffers 3. given 1 and 2 the decoder will still be throttled by the compositor.

Comment 5

[Nicolas Silva [:nical]]

(In reply to Kan-Ru Chen (:kanru) (Mozilla Corporation) from comment #4)
> Createing a separate protocol for gralloc has the advantage of not
> cluttering the PImageBridge and PLayers protocol.

Adding one method per protocol is not too bad (especially since these methods would only have to call a function implemented in ShadowLayersUtilGralloc.cpp), adding a new protocol on the other hand is a lot of boilerplate code. I'd rather avoid it if it doesn't get us actual technical benefits.

> Separating it from the
> compositor thread was not my primary concern since 1. we still need to wait
> the compositor to get the image back 2. we cannot allocate infinite gralloc
> buffers 3. given 1 and 2 the decoder will still be throttled by the
> compositor.

My understanding from a discussion with pchang on irc is that we suffer from the fact that requesting gralloc buffers takes too long when the compositor is busy.
So I thought your proposition was also partly aiming at mitigating this.

Anyway, never my misunderstanding, this is a discussion for a separate bug.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

unassign myself as I won't be able to work on this recently.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Kan-Ru Chen [:kanru] from comment #0)
> 3. Wrap the buffer in a SurfaceDescriptorXXX when we need to pass it to the
> other side.

You might already know this, but to do this you'll need to have some kind of non-IPC-managed way of referring to GraphicBuffers cross-process.  Right now, at cross-process creation time, the fd-passing dance happens and two GraphicBuffer objects referring to the same underlying memory show up in the two different processes.  But on subsequent passes, only an integer id is passed which is then looked up by the IPC system.  This id is allocated on a per-protocol basis, so you can't use one protocol's objects in another (you'll get errors at runtime).

You'll need a way to refer to the GraphicBuffers in a generic way to be able to pass them in other protocols.  Perhaps as easy as just an integer token that gets passed along with the fds, so that both parent/child have the same ID that they can pass around and look up.

Comment 8

[Nicolas Silva [:nical]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #7)
> (In reply to Kan-Ru Chen [:kanru] from comment #0)
> > 3. Wrap the buffer in a SurfaceDescriptorXXX when we need to pass it to the
> > other side.
> 
> You might already know this, but to do this you'll need to have some kind of
> non-IPC-managed way of referring to GraphicBuffers cross-process.  Right
> now, at cross-process creation time, the fd-passing dance happens and two
> GraphicBuffer objects referring to the same underlying memory show up in the
> two different processes.  But on subsequent passes, only an integer id is
> passed which is then looked up by the IPC system.  This id is allocated on a
> per-protocol basis, so you can't use one protocol's objects in another
> (you'll get errors at runtime).
> 
> You'll need a way to refer to the GraphicBuffers in a generic way to be able
> to pass them in other protocols.  Perhaps as easy as just an integer token
> that gets passed along with the fds, so that both parent/child have the same
> ID that they can pass around and look up.

I am working this out on top of the new TextureClient/TextureHost stuff, so I need to serialize the gralloc buffer only once (in opAddTexture that creates the TextureHost on the other side). Except that right now I create the gralloc buffer before OpAddTexture so I guess I am doing the fd-passing dance twice (when creating the buffer and when creating the texture host). Would that be a big performance hit?

If it is a big problem I will need to do some more work to make textures more independent from compositables and then I'll be able to create the TextureClient, TextureHost and GraphicBuffer at once, which is planned but I hoped to do that later.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Nicolas Silva [:nical] from comment #8)
> I am working this out on top of the new TextureClient/TextureHost stuff, so
> I need to serialize the gralloc buffer only once (in opAddTexture that
> creates the TextureHost on the other side). Except that right now I create
> the gralloc buffer before OpAddTexture so I guess I am doing the fd-passing
> dance twice (when creating the buffer and when creating the texture host).
> Would that be a big performance hit?

So the fds go in one direction the first time, and the other direction the second time?  Shouldn't be a big perf hit, though it does mean that the compositor will have the same buffer opened twice (and associated memory, perhaps VM, cost).

It won't actually do anything with the first one, right (the one that it actually created to hand back)?  I suspect the right thing is to actually notify the compositor that it doesn't need to hold on to that one any more, once it's opened properly on the child side.  Maybe.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

I don't see this blocking bug 900012 as such - removing PGrallocActor is not the only way to stop that crash.