Add reference-counting for mailbox textures.

Add reference-counting for mailbox textures.

Reference mailbox textures from each client texture.
When a texture is produced into a mailbox, it becomes immutable (size 0x0), but can still be used to consume back to a mailbox texture.

This makes it impossible to leak textures in the mailbox.

Next is to actually reference the same texture from all client textures, but make them sort-of read-only (except for the active one).

BUG=181640

[no sievers, 2013-03-19 04:25:24]

Maybe not fully polished yet, but probably good to get some feedback on the
semantics - in particular see modified unit tests.

Antoine and I were discussing this as a solution to both problems with leaks in
--composite-to-mailbox (and the new WebGL path being worked on - in both cases
it's tricky and requires complicated handshakes to handle both parties going
away independently, incl. the renderer crashing), and some other cases where it
might be nice for a produced texture to still be readable.

If this is agreeable, I would make these changes separately:
- modify MailboxOutputSurface to keep around and reuse client textures
- modify TextureImageTransportSurface (I think the main change would be to have
it use TextureDefinitions for the front- and backbuffer which can also be done
separately)

[apatrick_chromium, 2013-03-19 18:50:18]

I think I like the direction here. There are some other tests that will make the
new semantics clearer.

http://src.chromium.org/viewvc/chrome/trunk/src/gpu/command_buffer/tests/gl_t...

Also, can you change the extension spec?

http://src.chromium.org/viewvc/chrome/trunk/src/gpu/GLES2/extensions/CHROMIUM...

https://codereview.chromium.org/12717013/diff/2001/gpu/command_buffer/service...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/12717013/diff/2001/gpu/command_buffer/service...
gpu/command_buffer/service/texture_manager.cc:119: if (mailbox_ &&
mailbox_->HasOneRef()) {
HasOneRef is a bit dangerous if there are potentially ever going to be other
scoped_refptrs to TextureDefinition.

https://codereview.chromium.org/12717013/diff/2001/gpu/command_buffer/service...
gpu/command_buffer/service/texture_manager.cc:973: texture->min_filter(),
indentation

https://codereview.chromium.org/12717013/diff/2001/gpu/command_buffer/service...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/12717013/diff/2001/gpu/command_buffer/service...
gpu/command_buffer/service/texture_manager.h:356: // A reference to a texture
definition ater putting it into a mailbox.
ater -> after

[no sievers, 2013-03-19 19:06:58]

Should I simply disallow producing a texture more than once? so there is
somewhat of an owner (or rather one mutable reference at most). otherwise it
gets funky if you can have the same definition in the mailbox with different
names. need to add a test for this.

[apatrick_chromium, 2013-03-19 19:20:58]

On 2013/03/19 19:06:58, Daniel Sievers wrote:
> Should I simply disallow producing a texture more than once? so there is
> somewhat of an owner (or rather one mutable reference at most). otherwise it
> gets funky if you can have the same definition in the mailbox with different
> names. need to add a test for this.

That seems like a good simplification. Would it be possible to make them all
immutable? What happens if a texture is in two different context groups and it
is redefined in the one where it is mutable? How is the housekeeping info in the
other context group updated such that it will correctly determine that there is
potentially an incomplete texture and revalidate? Likewise, texture memory usage
stats?

[no sievers, 2013-03-19 20:36:48]

Per chat with Al:

- make shared textures always immutable
- fail SetTexParamter() while Texture::mailbox_ != NULL (i.e. texture was
shared) (add unit test for this)
- then it would also be nice to optimize it so that when all other refs go away,
a Texture releases its last ref to the TextureDefinition and takes back
ownership. Q: now changes to texture parameters would be allowed again. But
should the texture also become mutable again if it was in the first place?
- add unit test and make sure the black fill logic still works as expected with
these changes and we don't accidentally blackfill a texture unnecessarily

[no sievers, 2013-03-19 20:56:48]

On 2013/03/19 20:36:48, Daniel Sievers wrote:
> Per chat with Al:
> 
> - make shared textures always immutable
> - fail SetTexParamter() while Texture::mailbox_ != NULL (i.e. texture was
> shared) (add unit test for this)
> - then it would also be nice to optimize it so that when all other refs go
away,
> a Texture releases its last ref to the TextureDefinition and takes back
> ownership. Q: now changes to texture parameters would be allowed again. But
> should the texture also become mutable again if it was in the first place?
> - add unit test and make sure the black fill logic still works as expected
with
> these changes and we don't accidentally blackfill a texture unnecessarily

- require texture to be complete for Produce() to succeed. add unit test

piman: How does that all sound?

The one drawback of making mailbox textures immutable is that it requires an
extra roundtrip (for example from renderer to browser) during resizing, if one
wanted to make sure the consumer never renders a texture that has disappeared
(renderer compositor going away).

[no sievers, 2013-03-19 21:15:37]

On 2013/03/19 20:56:48, Daniel Sievers wrote:
> > - then it would also be nice to optimize it so that when all other refs go
> away,
> > a Texture releases its last ref to the TextureDefinition and takes back
> > ownership

On second thought, this does not seem very useful, because there is no good way
for a client to know whether it's the only client referencing a texture. So
maybe it's good enough to stay immutable w/unmodifiable tex params.

[alexst (slow to review), 2013-03-19 21:56:57]

> On second thought, this does not seem very useful, because there is no good
way
> for a client to know whether it's the only client referencing a texture. So
> maybe it's good enough to stay immutable w/unmodifiable tex params.

Immutable with unmodifiable tex params seems reasonable to me. I don't think the
resize/param change case is frequent enough to warrant added complexity.

[no sievers, 2013-03-19 22:10:02]

another thing:
Should Texture::CanRenderTo() only return |true| for shared textures when the
client is the one who last consumed it?  What about (copy)texSubimage etc.?

[piman, 2013-03-19 22:50:49]

On Tue, Mar 19, 2013 at 1:36 PM, <sievers@chromium.org> wrote:

> Per chat with Al:
>
> - make shared textures always immutable
> - fail SetTexParamter() while Texture::mailbox_ != NULL (i.e. texture was
> shared) (add unit test for this)
>

I'm pretty sure we can't do that. We change filtering modes on the reader
side.


> - then it would also be nice to optimize it so that when all other refs go
> away,
> a Texture releases its last ref to the TextureDefinition and takes back
> ownership. Q: now changes to texture parameters would be allowed again. But
> should the texture also become mutable again if it was in the first place?
> - add unit test and make sure the black fill logic still works as expected
> with
> these changes and we don't accidentally blackfill a texture unnecessarily
>
>
>
https://codereview.chromium.**org/12717013/<https://codereview.chromium.org/1...
>

[piman, 2013-03-19 22:56:11]

On Tue, Mar 19, 2013 at 1:56 PM, <sievers@chromium.org> wrote:

> On 2013/03/19 20:36:48, Daniel Sievers wrote:
>
>> Per chat with Al:
>>
>
>  - make shared textures always immutable
>> - fail SetTexParamter() while Texture::mailbox_ != NULL (i.e. texture was
>> shared) (add unit test for this)
>> - then it would also be nice to optimize it so that when all other refs go
>>
> away,
>
>> a Texture releases its last ref to the TextureDefinition and takes back
>> ownership. Q: now changes to texture parameters would be allowed again.
>> But
>> should the texture also become mutable again if it was in the first place?
>> - add unit test and make sure the black fill logic still works as expected
>>
> with
>
>> these changes and we don't accidentally blackfill a texture unnecessarily
>>
>
> - require texture to be complete for Produce() to succeed. add unit test
>
> piman: How does that all sound?
>

At a high level it sounds reasonable.


> The one drawback of making mailbox textures immutable is that it requires
> an
> extra roundtrip (for example from renderer to browser) during resizing, if
> one
> wanted to make sure the consumer never renders a texture that has
> disappeared
> (renderer compositor going away).
>

I think we can have higher-level constructs that would allow this without
adding complexity to this (e.g. a "pool" like we discussed, that's only
there to add a refcount, that could be created by the browser side ahead of
time).


>
>
https://codereview.chromium.**org/12717013/<https://codereview.chromium.org/1...
>

[alexst (slow to review), 2013-03-20 15:06:45]

> Should Texture::CanRenderTo() only return |true| for shared textures when the
> client is the one who last consumed it?  What about (copy)texSubimage etc.?

I was reminded of an issue related to this. I see another use case for enabling
partial swaps in webview compositing.

I want to hold onto the client id of the previous buffer to reconstruct the full
image when a partial swap arrives. I'll need to texSubImage into the new buffer,
which would require binding the previous id to the framebuffer, and I imagine
that could hit a CanRenderTo() assert.

[alexst (slow to review), 2013-03-20 15:24:47]

> which would require binding the previous id to the framebuffer

In fact WebGL paintCompositedResultsToCanvas does the same thing.

[piman, 2013-03-20 17:06:29]

On Wed, Mar 20, 2013 at 8:06 AM, <alexst@chromium.org> wrote:

>  Should Texture::CanRenderTo() only return |true| for shared textures when
>> the
>> client is the one who last consumed it?  What about (copy)texSubimage
>> etc.?
>>
>
> I was reminded of an issue related to this. I see another use case for
> enabling
> partial swaps in webview compositing.
>
> I want to hold onto the client id of the previous buffer to reconstruct
> the full
> image when a partial swap arrives. I'll need to texSubImage into the new
> buffer,
> which would require binding the previous id to the framebuffer, and I
> imagine
> that could hit a CanRenderTo() assert.
>

There are tricky things wrt FBO completeness which is why today you can't
produce a texture that's bound to a FBO. So I suspect if you could unbind,
produce, and then rebind those problems would still exist.
That is, unless the immutability property makes the issue go away entirely
(I haven't thought about the details).

Otherwise, since you only need to read from the produced texture, you can
always blit with a quad, instead of glFramebufferBlit, which shouldn't
require binding to a FBO.


>
https://codereview.chromium.**org/12717013/<https://codereview.chromium.org/1...
>

[alexst (slow to review), 2013-03-20 21:11:17]

> Otherwise, since you only need to read from the produced texture, you can
> always blit with a quad, instead of glFramebufferBlit, which shouldn't
> require binding to a FBO.

If we go down this road, I think it might be useful to create something akin to
copyTextureCHROMIUM, but with ability to do subrects since the quad blitter
would need to exist in WebKit for webgl, for webview compositing and maybe for
the thumbnailer too?

Is there such an API already maybe?

[no sievers, 2013-04-02 03:07:20]

Ok, I have taken a step back in that a texture after 'produce' does not actually
reference the same GL texture (that will be next with tex params to be sorted
out and such).

At the same time I have made the reference-counting more explicit:
TextureDefinition::Client::Start/StopSharing().

(I think eventually TextureDefinition can be renamed to SharedTexture.)

For backwards compatibility I had to make TextureImageTransportSurface keep
references on the textures in flight so that they don't get deleted before the
browser consumes them (or also before the surface consumes them back during
ack).

I removed the old AddRef() hack there, but if we cannot safely convince
ourselves that there are no cases where after the stub is destroyed the browser
might still try to consume and swap a pending frame, I might add it back.

https://codereview.chromium.org/12717013/diff/2001/gpu/command_buffer/service...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/12717013/diff/2001/gpu/command_buffer/service...
gpu/command_buffer/service/texture_manager.cc:973: texture->min_filter(),
On 2013/03/19 18:50:18, apatrick_chromium wrote:
> indentation

Done.

https://codereview.chromium.org/12717013/diff/2001/gpu/command_buffer/service...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/12717013/diff/2001/gpu/command_buffer/service...
gpu/command_buffer/service/texture_manager.h:356: // A reference to a texture
definition ater putting it into a mailbox.
On 2013/03/19 18:50:18, apatrick_chromium wrote:
> ater -> after

Done.

[no sievers, 2013-04-02 03:08:55]

On 2013/04/02 03:07:20, Daniel Sievers wrote:
> At the same time I have made the reference-counting more explicit:
> TextureDefinition::Client::Start/StopSharing().
> 

Oh I did it this way (with each shared texture keeping a list of backpointers to
the active clients using it), so that this can be used later to validate texture
params or whatever across clients, if necessary.

[no sievers, 2013-04-02 18:27:37]

So the surface needs more logic to release the reference to the frontbuffer when
it's being freed. But I might have an idea for simplifying the logic needed in
ImageTransportSurface...

[no sievers, 2013-04-02 21:38:39]

On 2013/04/02 18:27:37, Daniel Sievers wrote:
> So the surface needs more logic to release the reference to the frontbuffer
when
> it's being freed. But I might have an idea for simplifying the logic needed in
> ImageTransportSurface...

Ok I've hacked on it a bit to show the general idea:

By default shared textures are only referenced from client textures. The mailbox
manager only has a weak ptr (with an observer callback for when the texture gets
destroyed). But optionally it can hold a reference to the shared texture if it
is stored there with a texture pool id.

The pool id for the transport surface is generated from the transport surface
id, so it can start putting textures into the pool without having to wait for
anything from the browser.

When RWHV (or the transport surface browser-side) goes away, it sends an IPC to
gpu process to remove all textures in that pool.