Implement TextureImageTransportSurface using texture mailbox

Implement TextureImageTransportSurface using texture mailbox

This has a couple of advantages:
- allow tearing down and recreating the UI parent context without
losing the renderer contexts
- do not require a context to be able to generate textures when
creating the GLSurfaceHandle
- clearer ownership semantics that potentially allows for more
robust and easier lost context handling/thumbnailing/etc., since a texture is at
any given time owned by either: UI parent, mailbox, or
TextureImageTransportSurface
- simplify frontbuffer protection logic;
the frontbuffer textures are now owned by RWHV where they are refcounted

The TextureImageTransportSurface informs RenderWidgetHostView of the
mailbox names for the front- and backbuffer textures by
associating them with a surface_handle (1 or 2) in the AcceleratedSurfaceNew message.
During SwapBuffers() or PostSubBuffer() cycles, it then uses 
produceTextureCHROMIUM() and consumeTextureCHROMIUM()
to transfer ownership between renderer and browser compositor.

RWHV sends back the surface_handle of the buffer being returned with the Swap ACK
(or 0 if no buffer is being returned in which case TextureImageTransportSurface will
allocate a new texture - note that this could be used to 
simply keep textures for thumbnailing).

BUG=154815, 139616
[email protected]
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172087

[no sievers, 2012-10-17 23:11:56]

Far from ready for review!

Antoine, I hacked this up just to see what it could look like and wouldn't mind
some early feedback to see whether I'm going in the right direction. I think it
would help with some problems I am trying to solve.

[piman, 2012-10-17 23:47:20]

yes! at a high level that's more or less how I thought it would go.

I would invite you to implement this with aura as well, to see if there's any
gotcha there, but I can't think of anything right now.

There is one thing that we'll need to pay attention to, is potential races
between renderer context destruction and the UI trying to put textures into the
mailbox (and vice versa). I want to make sure we don't end up leaking textures
(the lifetime of a texture in a mailbox is linked to the group of the context
that put it into the mailbox).

https://codereview.chromium.org/11194042/diff/1/content/common/gpu/texture_im...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/1/content/common/gpu/texture_im...
content/common/gpu/texture_image_transport_surface.cc:67: texture.identifier =
GenerateId();
They don't need to be globally unique, you can just use 0 and 1.

https://codereview.chromium.org/11194042/diff/1/content/common/gpu/texture_im...
content/common/gpu/texture_image_transport_surface.cc:80: // channel.
The way I imagine this, is to move it the channel creation (it's really a
relationship between channels).

The only thing is the race between the browser and the renderer's channels,
especially after a GPU process crash, but maybe that's solvable.

[no sievers, 2012-10-19 18:43:57]

On 2012/10/17 23:47:20, piman wrote:
> yes! at a high level that's more or less how I thought it would go.
> 
> I would invite you to implement this with aura as well, to see if there's any
> gotcha there, but I can't think of anything right now.
> 
> There is one thing that we'll need to pay attention to, is potential races
> between renderer context destruction and the UI trying to put textures into
the
> mailbox (and vice versa). I want to make sure we don't end up leaking textures
> (the lifetime of a texture in a mailbox is linked to the group of the context
> that put it into the mailbox).
> 

Ah right, it only gets removed from the mailbox when the group goes away, not
when the stub goes away (which would be nicer in this case). Although (also from
what it sounded like in yesterday's thread), this is maybe a general problem,
and it might be better to lose all contexts in a share group simultaneously.

>
https://codereview.chromium.org/11194042/diff/1/content/common/gpu/texture_im...
> File content/common/gpu/texture_image_transport_surface.cc (right):
> 
>
https://codereview.chromium.org/11194042/diff/1/content/common/gpu/texture_im...
> content/common/gpu/texture_image_transport_surface.cc:67: texture.identifier =
> GenerateId();
> They don't need to be globally unique, you can just use 0 and 1.
> 

On a related note, I wonder if it's better to use two fixed mailbox names too,
since using new names would otherwise create more leak-a-texture-in-the-mailbox
scenarios similar to the one you mention above.
However, since the MailboxManager guards a Produce() collision with
NOTREACHED(), maybe the surface should simply attempt to consume the texture
first whenever it recreated a texture. 

With this patch basically any time CreateBackTexture() gets called while the
swap ack is pending, it would attempt to create the texture (since the
service_id is zero now after producing it). It seems like this only should
happen for cases where we wanted to reallocate anyway
(AdjustBackBufferAllocation and resize). Or is there anything else we need to
worry about?


>
https://codereview.chromium.org/11194042/diff/1/content/common/gpu/texture_im...
> content/common/gpu/texture_image_transport_surface.cc:80: // channel.
> The way I imagine this, is to move it the channel creation (it's really a
> relationship between channels).
> 

I should probably do that in a separate patch.

> The only thing is the race between the browser and the renderer's channels,
> especially after a GPU process crash, but maybe that's solvable.

And finally: AdjustFrontBufferAllocation() would somewhat break, because the
RWHV or mailbox will likely be holding the front buffer at that time.

[piman, 2012-10-19 18:59:39]

On Fri, Oct 19, 2012 at 11:43 AM, <[email protected]> wrote:

> On 2012/10/17 23:47:20, piman wrote:
>
>> yes! at a high level that's more or less how I thought it would go.
>>
>
>  I would invite you to implement this with aura as well, to see if there's
>> any
>> gotcha there, but I can't think of anything right now.
>>
>
>  There is one thing that we'll need to pay attention to, is potential races
>> between renderer context destruction and the UI trying to put textures
>> into
>>
> the
>
>> mailbox (and vice versa). I want to make sure we don't end up leaking
>> textures
>> (the lifetime of a texture in a mailbox is linked to the group of the
>> context
>> that put it into the mailbox).
>>
>
>
> Ah right, it only gets removed from the mailbox when the group goes away,
> not
> when the stub goes away (which would be nicer in this case). Although
> (also from
> what it sounded like in yesterday's thread), this is maybe a general
> problem,
> and it might be better to lose all contexts in a share group
> simultaneously.


Note: I'm not talking about context lost, but normal, expected behavior of
the renderer being closed. We very certainly don't want to leak in that
case.

>
>
>
> https://codereview.chromium.**org/11194042/diff/1/content/**
>
common/gpu/texture_image_**transport_surface.cc<https://codereview.chromium.org/11194042/diff/1/content/common/gpu/texture_image_transport_surface.cc>
>
>> File content/common/gpu/texture_**image_transport_surface.cc (right):
>>
>
>
> https://codereview.chromium.**org/11194042/diff/1/content/**
>
common/gpu/texture_image_**transport_surface.cc#newcode67<https://codereview.chromium.org/11194042/diff/1/content/common/gpu/texture_image_transport_surface.cc#newcode67>
>
>> content/common/gpu/texture_**image_transport_surface.cc:67:
>> texture.identifier =
>> GenerateId();
>> They don't need to be globally unique, you can just use 0 and 1.
>>
>
>
> On a related note, I wonder if it's better to use two fixed mailbox names
> too,
> since using new names would otherwise create more
> leak-a-texture-in-the-mailbox
> scenarios similar to the one you mention above.
> However, since the MailboxManager guards a Produce() collision with
> NOTREACHED(), maybe the surface should simply attempt to consume the
> texture
> first whenever it recreated a texture.
>

Using 2 fixed mailbox names sounds good.
Since the service side is the one creating/deleting textures, I don't think
there's a chance of Produce-ing a texture into a "used" mailbox - assuming
the browser sends back the texture in the same mailbox it received it.


> With this patch basically any time CreateBackTexture() gets called while
> the
> swap ack is pending, it would attempt to create the texture (since the
> service_id is zero now after producing it). It seems like this only should
> happen for cases where we wanted to reallocate anyway
> (AdjustBackBufferAllocation and resize). Or is there anything else we need
> to
> worry about?


I don't think CreateBackTexture can be called while the swap ack is
pending, because during that time, the channel is unscheduled so we won't
process command buffer commands. Note that we still process IPCs coming
from the process channel, and post tasks to trigger memory allocation
changes, but I don't think any of them would lead to CreateBackTexture
being called.


>
>
>
> https://codereview.chromium.**org/11194042/diff/1/content/**
>
common/gpu/texture_image_**transport_surface.cc#newcode80<https://codereview.chromium.org/11194042/diff/1/content/common/gpu/texture_image_transport_surface.cc#newcode80>
>
>> content/common/gpu/texture_**image_transport_surface.cc:80: // channel.
>> The way I imagine this, is to move it the channel creation (it's really a
>> relationship between channels).
>>
>
>
> I should probably do that in a separate patch.
>
>
>  The only thing is the race between the browser and the renderer's
>> channels,
>> especially after a GPU process crash, but maybe that's solvable.
>>
>
> And finally: AdjustFrontBufferAllocation() would somewhat break, because
> the
> RWHV or mailbox will likely be holding the front buffer at that time.
>

The logic for the front buffer allocation is meant to ensure we never
delete the front texture unless we're guaranteed it's "owned" by the
service side. That being said, if we rely on service_id==0 to track some of
the state, we'll have to change it so something explicit, yes.


>
>
http://codereview.chromium.**org/11194042/<http://codereview.chromium.org/111...
>

[no sievers, 2012-10-19 19:44:29]

On 2012/10/19 18:59:39, piman wrote:
> 
> Note: I'm not talking about context lost, but normal, expected behavior of
> the renderer being closed. We very certainly don't want to leak in that
> case.
> 

So it seems like textures should only ever be in the mailbox in between Swap and
BufferPresentedImpl. But I guess in that window either texture can or cannot be
in the mailbox, and either side can be going away. Maybe either side should just
attempt to consume the last texture it produced and then free it.

> 
> Using 2 fixed mailbox names sounds good.
> Since the service side is the one creating/deleting textures, I don't think
> there's a chance of Produce-ing a texture into a "used" mailbox - assuming
> the browser sends back the texture in the same mailbox it received it.
> 
> 
> > With this patch basically any time CreateBackTexture() gets called while
> > the
> > swap ack is pending, it would attempt to create the texture (since the
> > service_id is zero now after producing it). It seems like this only should
> > happen for cases where we wanted to reallocate anyway
> > (AdjustBackBufferAllocation and resize). Or is there anything else we need
> > to
> > worry about?
> 
> 
> I don't think CreateBackTexture can be called while the swap ack is
> pending, because during that time, the channel is unscheduled so we won't
> process command buffer commands. Note that we still process IPCs coming
> from the process channel, and post tasks to trigger memory allocation
> changes, but I don't think any of them would lead to CreateBackTexture
> being called.
> 

So resize goes through the command buffer, but ont sure whether
SetBackbufferAllocation could still occur.

> >
> > And finally: AdjustFrontBufferAllocation() would somewhat break, because
> > the
> > RWHV or mailbox will likely be holding the front buffer at that time.
> >
> 
> The logic for the front buffer allocation is meant to ensure we never
> delete the front texture unless we're guaranteed it's "owned" by the
> service side. That being said, if we rely on service_id==0 to track some of
> the state, we'll have to change it so something explicit, yes.
> 
> 

Actually we never allocate the front buffer (only the backbuffer). So the logic
only deals with freeing it. However, if the service_id is zero it means it was
never allocated or produced to the RWHV. And it seems in the latter case, this
would imply that it is protected and we would not want to free it anway.
Correct?

[piman, 2012-10-19 20:44:08]

On Fri, Oct 19, 2012 at 12:44 PM, <[email protected]> wrote:

> On 2012/10/19 18:59:39, piman wrote:
>
>  Note: I'm not talking about context lost, but normal, expected behavior of
>> the renderer being closed. We very certainly don't want to leak in that
>> case.
>>
>
>
> So it seems like textures should only ever be in the mailbox in between
> Swap and
> BufferPresentedImpl. But I guess in that window either texture can or
> cannot be
> in the mailbox, and either side can be going away. Maybe either side
> should just
> attempt to consume the last texture it produced and then free it.


That's probably OK, as long as we squash errors.


>
>
>
>  Using 2 fixed mailbox names sounds good.
>> Since the service side is the one creating/deleting textures, I don't
>> think
>> there's a chance of Produce-ing a texture into a "used" mailbox - assuming
>> the browser sends back the texture in the same mailbox it received it.
>>
>
>
>  > With this patch basically any time CreateBackTexture() gets called while
>> > the
>> > swap ack is pending, it would attempt to create the texture (since the
>> > service_id is zero now after producing it). It seems like this only
>> should
>> > happen for cases where we wanted to reallocate anyway
>> > (AdjustBackBufferAllocation and resize). Or is there anything else we
>> need
>> > to
>> > worry about?
>>
>
>
>  I don't think CreateBackTexture can be called while the swap ack is
>> pending, because during that time, the channel is unscheduled so we won't
>> process command buffer commands. Note that we still process IPCs coming
>> from the process channel, and post tasks to trigger memory allocation
>> changes, but I don't think any of them would lead to CreateBackTexture
>> being called.
>>
>
>
> So resize goes through the command buffer, but ont sure whether
> SetBackbufferAllocation could still occur.


 SetBackbufferAllocation also comes from a command buffer command.


>
>  >
>> > And finally: AdjustFrontBufferAllocation() would somewhat break, because
>> > the
>> > RWHV or mailbox will likely be holding the front buffer at that time.
>> >
>>
>
>  The logic for the front buffer allocation is meant to ensure we never
>> delete the front texture unless we're guaranteed it's "owned" by the
>> service side. That being said, if we rely on service_id==0 to track some
>> of
>> the state, we'll have to change it so something explicit, yes.
>>
>
>
>
> Actually we never allocate the front buffer (only the backbuffer).


Correct.


> So the logic
> only deals with freeing it. However, if the service_id is zero it means it
> was
> never allocated or produced to the RWHV.


Oh, right. I was thinking service_id would become 0 when you produce, but
there's no reason for it.


> And it seems in the latter case, this
> would imply that it is protected and we would not want to free it anway.
> Correct?
>

Right.

We may want to add some state tracking (debug only?), so that we can check
we don't destroy textures that have been sent to the RWHV.


>
>
>
http://codereview.chromium.**org/11194042/<http://codereview.chromium.org/111...
>

[no sievers, 2012-10-24 21:38:05]

Ok, since consuming textures, esp. through the GL api produces errors if the
texture is not there, here is another idea:

We can actually use a single mailbox name (front- and backbuffer don't have to
be in the mailbox at the same time ever, i.e. RWHV can consume first and then
produce).
Then we can avoid leaks by never tearing down the TextureImageTransportSurface
until the ACK is received (or 'fake ack' if the RWHV went away). It can then
consume the texture back before destroying.

Here's somewhat of a hack to achieve that:
- Keep an extra ref on the TransportSurface while ACK is pending.
- When it arrives and the stub already went away, use the channel manager to get
to a share group context and clean up the texture.

[no sievers, 2012-11-02 01:24:22]

I removed all the frontbuffer protection logic *cough*

I think this works simpler now because the RWHV takes ownership of the texture.
It creates owned ui::Textures now, which are refcounted, and I let it hold an
extra ref while waiting for a thumbnailer task or pending commit (when the gpu
thread tells us the frontbuffer should be freed).

The TextureImageTransportSurface also now only needs to know about its current
backbuffer.

One caveat: For partial swaps, the copy logic would now have to move to RWHV,
because that's the place where we have access to both during the swap.

[no sievers, 2012-11-02 01:29:14]

Adding backer too for comments.
I updated RWHVAura, but did not update Android again yet.

http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.h:338: // The buffer
pending to become our new frontbuffer during a swap
Actually this is the previous one, the one to be released back to the producer
when we ack the swap.

[apatrick_chromium, 2012-11-05 20:58:56]

I've only skimmed this. Is this still a work in progress or do you want a full
review?

High level question. I am about to change the semantics of mailboxes. Once a
texture has been associated with a mailbox name, it will be an invalid operation
to ever associate it with a different mailbox name. This is so that the service
ID of a texture object never changes service side. Will that cause your patch to
break and if so can you change things so it doesn't?

http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_android.h:182: : id(0) {}
nit: fits on one line!

[no sievers, 2012-11-05 21:10:37]

On 2012/11/05 20:58:56, apatrick_chromium wrote:
> I've only skimmed this. Is this still a work in progress or do you want a full
> review?
> 

Actually I wouldn't mind already getting feedback since it's a bigger patch (and
I'm expecting to break stuff :) while I clean up the lose ends that I know of:
- need to fix PostSubBuffer to actually track previous damage and use
CopyTexSubImage() (please ignore GLHelper hacks in the current patch)
- need to adjust RenderWidgetHostViewAndroid further
- need to rebase RenderWidgetHostViewAura around backer's recent changes





> High level question. I am about to change the semantics of mailboxes. Once a
> texture has been associated with a mailbox name, it will be an invalid
operation
> to ever associate it with a different mailbox name. This is so that the
service
> ID of a texture object never changes service side. Will that cause your patch
to
> break and if so can you change things so it doesn't?
> 

That should be fine. The TextureImageTransportSurface never changes the mailbox
name it uses. And the RWHV frees it's current textures if it ever receives a new
mailbox name with an AcceleratedSurfaceNew message (the surface will create a
new texture if it is not able to consume one from the mailbox with an ACK).


>
http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
> File content/browser/renderer_host/render_widget_host_view_android.h (right):
> 
>
http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host_view_android.h:182: : id(0)
{}
> nit: fits on one line!

[no sievers, 2012-11-05 21:14:47]

On 2012/11/05 21:10:37, Daniel Sievers wrote:
> On 2012/11/05 20:58:56, apatrick_chromium wrote:
> > I've only skimmed this. Is this still a work in progress or do you want a
full
> > review?
> > 
> 
> Actually I wouldn't mind already getting feedback since it's a bigger patch
(and
> I'm expecting to break stuff :) while I clean up the lose ends that I know of:
> - need to fix PostSubBuffer to actually track previous damage and use
> CopyTexSubImage() (please ignore GLHelper hacks in the current patch)
> - need to adjust RenderWidgetHostViewAndroid further
> - need to rebase RenderWidgetHostViewAura around backer's recent changes
> 
> 
> 
> 
> 
> > High level question. I am about to change the semantics of mailboxes. Once a
> > texture has been associated with a mailbox name, it will be an invalid
> operation
> > to ever associate it with a different mailbox name. This is so that the
> service
> > ID of a texture object never changes service side. Will that cause your
patch
> to
> > break and if so can you change things so it doesn't?
> > 
> 
> That should be fine. The TextureImageTransportSurface never changes the
mailbox
> name it uses. And the RWHV frees it's current textures if it ever receives a
new
> mailbox name with an AcceleratedSurfaceNew message (the surface will create a
> new texture if it is not able to consume one from the mailbox with an ACK).
> 

Wait actually that is a problem for the latter case. But also more generally
since I use the same mailbox name for both textures (i.e. front- and
backbuffer). So I'd have to change it back to use unique names for each texture.



> 
> >
>
http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
> > File content/browser/renderer_host/render_widget_host_view_android.h
(right):
> > 
> >
>
http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
> > content/browser/renderer_host/render_widget_host_view_android.h:182: : id(0)
> {}
> > nit: fits on one line!

[no sievers, 2012-11-05 22:24:42]

On 2012/11/05 21:14:47, Daniel Sievers wrote:
> On 2012/11/05 21:10:37, Daniel Sievers wrote:
> > On 2012/11/05 20:58:56, apatrick_chromium wrote:
> > > I've only skimmed this. Is this still a work in progress or do you want a
> full
> > > review?
> > > 
> > 
> > Actually I wouldn't mind already getting feedback since it's a bigger patch
> (and
> > I'm expecting to break stuff :) while I clean up the lose ends that I know
of:
> > - need to fix PostSubBuffer to actually track previous damage and use
> > CopyTexSubImage() (please ignore GLHelper hacks in the current patch)
> > - need to adjust RenderWidgetHostViewAndroid further
> > - need to rebase RenderWidgetHostViewAura around backer's recent changes
> > 
> > 
> > 
> > 
> > 
> > > High level question. I am about to change the semantics of mailboxes. Once
a
> > > texture has been associated with a mailbox name, it will be an invalid
> > operation
> > > to ever associate it with a different mailbox name. This is so that the
> > service
> > > ID of a texture object never changes service side. Will that cause your
> patch
> > to
> > > break and if so can you change things so it doesn't?
> > > 
> > 
> > That should be fine. The TextureImageTransportSurface never changes the
> mailbox
> > name it uses. And the RWHV frees it's current textures if it ever receives a
> new
> > mailbox name with an AcceleratedSurfaceNew message (the surface will create
a
> > new texture if it is not able to consume one from the mailbox with an ACK).
> > 
> 
> Wait actually that is a problem for the latter case. But also more generally
> since I use the same mailbox name for both textures (i.e. front- and
> backbuffer). So I'd have to change it back to use unique names for each
texture.
> 
> 

Or would it be possible that I could override that behavior to the current one,
since I'm generating the mailbox name through calling into the manager directly
rather than use the GL api?


> 
> > 
> > >
> >
>
http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
> > > File content/browser/renderer_host/render_widget_host_view_android.h
> (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
> > > content/browser/renderer_host/render_widget_host_view_android.h:182: :
id(0)
> > {}
> > > nit: fits on one line!

[apatrick_chromium, 2012-11-05 22:35:30]

The semantic change is going to go pretty deep. We're eventually going to change
the API so produce and consume (to renamed release and acquire) do not even take
a mailbox name because it can never change. There's actually a bug in the
current implementation because the service ID can change and we also want to
remove the restriction that textures cannot be produced and consumed while
attached to an FBO. Mailboxes are cheap. Better to create two if that fixes it.

[no sievers, 2012-11-05 22:53:38]

Ok, sounds good then. Seems like I'd be changing it back then to have a map of
textures in RWHV and send New and Release messages every time a new texture is
created or freed. When the surface is being freed while it is waiting for a
final ACK, it should still only need to try consume the single mailbox texture
it had sent previously. So it probably doesn't change things too much. 


On 2012/11/05 22:35:30, apatrick_chromium wrote:
> The semantic change is going to go pretty deep. We're eventually going to
change
> the API so produce and consume (to renamed release and acquire) do not even
take
> a mailbox name because it can never change. There's actually a bug in the
> current implementation because the service ID can change and we also want to
> remove the restriction that textures cannot be produced and consumed while
> attached to an FBO. Mailboxes are cheap. Better to create two if that fixes
it.

[apatrick_chromium, 2012-11-05 23:36:24]

Okay thanks. I'm doing this as well. It's a faster way to generate mailbox
names. glGenMailbox isn't going away though.

https://codereview.chromium.org/11362053/

[no sievers, 2012-11-09 01:37:00]

Ok here we go. This should be ready for review now.
I yet have to update RenderWidgetHostViewAndroid, but it'll be similar to Aura.

Here is what it does:
SurfaceNew messages register unique mailbox names to identifier mappings (id is
always 1 or 2 for now).

The PostSubBuffer and Swap messages use the id to lookup the mailbox.

With the BufferPresented message they send back the id of the buffer they are
returning (or 0). If the returned id is 0, the surface allocates a new buffer.

Frontbuffer textures are owned by RWHV and don't need to handshake with
TextureImageTransportSurface anymore to free the frontbuffer. The SurfaceRelease
message is used to indicate the frontbuffer should be freed, so that the
management still goes through the gpu thread. But the textures are refcounted
there, so that they wouldn't get deleted until e.g. the thumbnailer is done (or
the compositor has committed).

PostSubBuffer handling is now taking place in RWHV, since that's where we see
both buffers at the same time. I think I fixed a bug where we weren't
accumulating damage correctly for skipped frames (black flashes during resizing
poster circle).

[apatrick_chromium, 2012-11-09 19:19:04]

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:36: #include
"third_party/khronos/GLES2/gl2.h"
For historical reasons, we've been using Mesa's GL headers. I intend to switch
them over to the Khronos one's at some point but haven't got around to it. In
the meantime, it's safer to be consistent in case there are any conflicts, like
extensions in one but not the other.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (left):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
Deleted this line?

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:122:
glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_id_);
This changes the FBO binding for the context and it is not restored. Is that
okay? Elsewhere, ScopedFrameBufferBinder is used to restore the binding.

http://codereview.chromium.org/11194042/diff/23001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

http://codereview.chromium.org/11194042/diff/23001/ipc/ipc_message_utils.cc#n...
ipc/ipc_message_utils.cc:429: void ParamTraits<std::vector<signed char>
>::Write(Message* m,
Are you sure you need explicit ParamTraits for std::vector? I think it will just
work?

Also, I've been using std::string to shuttle mailbox names around by IPC. It's
less typing :) std::string can hold null characters so it works.

[no sievers, 2012-11-09 21:53:01]

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:36: #include
"third_party/khronos/GLES2/gl2.h"
On 2012/11/09 19:19:04, apatrick_chromium wrote:
> For historical reasons, we've been using Mesa's GL headers. I intend to switch
> them over to the Khronos one's at some point but haven't got around to it. In
> the meantime, it's safer to be consistent in case there are any conflicts,
like
> extensions in one but not the other.

Youmean third_party/angle/include?

So that means I would just include:
 #include <GLES2/gl2.h>
 #include <GLES2/gl2ext.h>
because the gyp files put angle in the include path?

However it does not have GL_MAILBOX_SIZE_CHROMIUM :(

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (left):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/11/09 19:19:04, apatrick_chromium wrote:
> Deleted this line?

Oops done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:122:
glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_id_);
Hmm if it's a problem, it should exist before my patch :)
I assume we rely on the compositor to issue a Bind() every time it uses an
intermediate render surface.

On 2012/11/09 19:19:04, apatrick_chromium wrote:
> This changes the FBO binding for the context and it is not restored. Is that
> okay? Elsewhere, ScopedFrameBufferBinder is used to restore the binding.

http://codereview.chromium.org/11194042/diff/23001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

http://codereview.chromium.org/11194042/diff/23001/ipc/ipc_message_utils.cc#n...
ipc/ipc_message_utils.cc:429: void ParamTraits<std::vector<signed char>
>::Write(Message* m,
It has the ipc traits for char and unsigned char.
But ut did not compile with gcc. I think maybe because strictly speaking char,
signed char and unsigned char are different types?

I initially used string but then changed it because passing it around and
casting pointers become a little ugly.


On 2012/11/09 19:19:04, apatrick_chromium wrote:
> Are you sure you need explicit ParamTraits for std::vector? I think it will
just
> work?
> 
> Also, I've been using std::string to shuttle mailbox names around by IPC. It's
> less typing :) std::string can hold null characters so it works.

[piman, 2012-11-09 22:02:01]

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:168: if (host_context_)
{
I would make that a DCHECK. Bad things would happen if this silently failed,
wouldn't it?

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:172: GL_TEXTURE_2D,
mailbox_name_.data());
As you mentioned on chat, you need a flush() here.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:689: container);
// Hold a reference to the texture
I think we don't need this. By the time CropScaleReadbackAndCleanTexture
returns, the texture won't be used any more (it internally copies/scales it
before running the asynchronous part).
I think the thumbnailer-related asynchronous code is overly conservative and can
be simplified.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:835: if
(!skipped_damage_.IsEmpty())
The test isn't useful, since you always end up with the empty rect in either
case.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:877:
scoped_refptr<ui::Texture>& current_texture =
scoped_refptr<ui::Texture>& -> ui::Texture*? You're not modifying the map so no
need for the extra indirection.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:886:
DCHECK(damage_rect == surface_rect);
mmh, you're really asserting something that's coming from a (potentially
malicious) renderer.
What happens if it's not true? Sounds like we only get bad pixels (which is ok
for a malicious renderer). change to a DLOG_IF?

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:891: damage_rect =
UnionRects(skipped_damage_, damage_rect);
So the problem here (and when we union the skipped_damage_ on fast acks), is
that we keep the bounding box of the damage instead of the actual rects. So when
we try to copy previous_damage-damage, we may end up forgetting to copy some
pixels  that were not made up-to-date.
Should we make all the damage tracking structs regions?

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:897: if
(compositor) {
I think you should do this always. At least the copy. You may not have a
compositor if you're not in the tree, but in this case you still want to ensure
that the damage is copied so that when we come back into the tree the texture is
sane.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:968: if
(current_surface_ && ShouldReleaseFrontSurface() &&
We should only do this if current_surface_==surface_handle

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:969:
host_->is_accelerated_compositing_active()) {
Why is this test needed? Note, it's almost positively racy (it's a state that
comes from the renderer whereas AcceleratedSurfaceRelease comes from the GPU
process).

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:985:
image_transport_clients_.erase(current_surface_);
This should still happen always

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/gpu_mes...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/gpu_mes...
content/common/gpu/gpu_messages.h:281: uint64 /* surface_handle */,
Please document above what this surface handle represents (i.e. AFAIU it's a
buffer that gets "recycled" but I don't know that until I read the
implementation of TextureImageTransportSurface)

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (left):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:565:
helper_->SendAcceleratedSurfaceRelease(params);
This should still happen somewhere. If we switch off compositing (it can still
happen under some conditions), we need to tell the RWHV* to release its data
structures associated with the surface.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:122:
glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_id_);
On 2012/11/09 19:19:04, apatrick_chromium wrote:
> This changes the FBO binding for the context and it is not restored. Is that
> okay? Elsewhere, ScopedFrameBufferBinder is used to restore the binding.

It's probably not a problem, since this codes executes on the first MakeCurrent.
It might even be actually needed (I'm not sure the Decoder will explicitly set
GetBackingFrameBufferObject() as the current FBO on initialization).

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:170:
DCHECK(!is_swap_buffers_pending_);
I don't think this check is valid. This can be called from
SetFrontbufferAllocation which can be called from a posted task at any time.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:171: if
(!helper_->MakeCurrent())
We don't need this anymore.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:223: AddRef();
I'm not sure you'll actually be getting an ACK if the renderer gets closed
(which then would be a leak). Have you verified this?

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:324: glDeleteTextures(1,
&backbuffer_.service_id);
Al is working on changes that will make it so that the service id is owned by
the mailbox, that will be owned by the client ids in the respective contexts, so
this can go away eventually. Add a TODO to remove?
As is, it could in theory leak GPU data if the GPU process gets in a state where
it doesn't have contexts any more (not in the Aura case, but you may want to
check Android behavior. I'm also planning on reusing this for Pepper in non-Aura
cases where it could happen).

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:378: Texture& texture =
backbuffer_;
nit: you have a single texture now, so that indirection isn't really necessary
anymore (i.e. can use backbuffer_ and current_size_ directly below).

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:383: if
(!texture.service_id) {
nit: no need for braces any more.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:407:
mailbox_names_[texture.identifier] = mailbox_name;
Al can confirm but I don't think it's ok to change the mailbox name associated
with a service id, so you probably want to do this lazily.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:433: DCHECK(false);
nit: just make the above DLOG(FATAL)?

http://codereview.chromium.org/11194042/diff/23001/gpu/command_buffer/service...
File gpu/command_buffer/service/mailbox_manager.cc (right):

http://codereview.chromium.org/11194042/diff/23001/gpu/command_buffer/service...
gpu/command_buffer/service/mailbox_manager.cc:39: if (it == textures_.end()) {
nit: no need for braces any more.

http://codereview.chromium.org/11194042/diff/23001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

http://codereview.chromium.org/11194042/diff/23001/ipc/ipc_message_utils.cc#n...
ipc/ipc_message_utils.cc:429: void ParamTraits<std::vector<signed char>
>::Write(Message* m,
On 2012/11/09 19:19:04, apatrick_chromium wrote:
> Are you sure you need explicit ParamTraits for std::vector? I think it will
just
> work?

It will work though use an int for each char (i.e. conversion + extra size).

> 
> Also, I've been using std::string to shuttle mailbox names around by IPC. It's
> less typing :) std::string can hold null characters so it works.

This sounds like a good idea.

[jonathan.backer, 2012-11-12 16:52:14]

http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

http://codereview.chromium.org/11194042/diff/13001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:183: GL_TEXTURE_2D,
mailbox_name_.data());
cppreference.com says that std::vector::data is a C++11 feature. Is this OK?

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:182: GL_TEXTURE_2D,
mailbox_name_.data());
No flush() because of sync point?

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:812: if
(ShouldFastACK(params_in_pixel.size)) {
I'm torn as to handle the detached (no compositor state). The decision on how to
handle it was made before the current implementation of fast-ACKing (when the UI
compositor was single-threaded).

It's clear from RWHVA::WasShown that we're expecting a new frame to come in when
a tab was made visible (we're reattached). Here are the advantages of fast-ACK
when detached:
- no copy on PostSubBuffer
- no insertion of SyncPoint
- no commit (in threaded case)

WDYT?

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:891: damage_rect =
UnionRects(skipped_damage_, damage_rect);
On 2012/11/09 22:02:01, piman wrote:
> So the problem here (and when we union the skipped_damage_ on fast acks), is
> that we keep the bounding box of the damage instead of the actual rects. So
when
> we try to copy previous_damage-damage, we may end up forgetting to copy some
> pixels  that were not made up-to-date.
> Should we make all the damage tracking structs regions?

Good catch.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:897: if
(compositor) {
On 2012/11/09 22:02:01, piman wrote:
> I think you should do this always. At least the copy. You may not have a
> compositor if you're not in the tree, but in this case you still want to
ensure
> that the damage is copied so that when we come back into the tree the texture
is
> sane.

As I've mentioned above, I have a slight preference for going the other way:
treat the lack of compositor as an opportunity for a fast-ACK.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:909:
previous_damage_rect_);
Which context is current here? Maybe it doesn't matter because they are all in
the same share group?

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/client/...
File content/common/gpu/client/gl_helper.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/client/...
content/common/gpu/client/gl_helper.cc:761: region.op(RectToSkIRect(new_damage),
SkRegion::kDifference_Op);
Can we early exit if the region is empty? A common case is flash video, where
the damage rects are always the same (no need to copy). I'd prefer not to even
flush in this case.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:72: // TODO: Move this
somewhere else.
Not sure it belongs anywhere else. We only want pre-emption for TITSurfaces with
a parent channel.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:122:
glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_id_);
On 2012/11/09 19:19:04, apatrick_chromium wrote:
> This changes the FBO binding for the context and it is not restored. Is that
> okay? Elsewhere, ScopedFrameBufferBinder is used to restore the binding.

So this is the first time that MakeCurrent is ever called. In this case, we'd
have the default fbo (0) active no?

We've got a slight of hand here: for TITSurface, the default fbo is meant to be
|fbo_id|. This remapping is usually handled by
|GLES2DecoderImpl::DoBindFramebuffer|. I think that it's appropriate here.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:223: AddRef();
On 2012/11/09 22:02:01, piman wrote:
> I'm not sure you'll actually be getting an ACK if the renderer gets closed
> (which then would be a leak). Have you verified this?

We have a default ACK in GpuProcessHostUIShim. I think this is safe. Still ugly.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:364:
ScopedFrameBufferBinder fbo_binder(fbo_id_);
Why the framebuffer binder? Seems like a no-op. Maybe we also want a
glFramebufferTexture2D to release the backbuffer from the fbo?

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:374: // reallocating the
backbuffer now.
comment nit: "If |is_swap_buffers_pending|, we are waiting..."

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:377: gfx::Size& size =
current_size_;
Got tired of typing?

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:388: texture.size = size;
nit: no if necessary.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.h (left):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.h:22: public
base::SupportsWeakPtr<TextureImageTransportSurface> {
Why can we lose this?

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.h (right):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.h:62: struct Texture {
nit: We only ever have one instance of this class.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.h:82: void
ProduceTexture(Texture& texture);
nit: Only operates on the same Texture.

[no sievers, 2012-11-19 20:30:43]

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:168: if (host_context_)
{
On 2012/11/09 22:02:01, piman wrote:
> I would make that a DCHECK. Bad things would happen if this silently failed,
> wouldn't it?

Done.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:172: GL_TEXTURE_2D,
mailbox_name_.data());
On 2012/11/09 22:02:01, piman wrote:
> As you mentioned on chat, you need a flush() here.

Done.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:182: GL_TEXTURE_2D,
mailbox_name_.data());
On 2012/11/12 16:52:15, jonathan.backer wrote:
> No flush() because of sync point?

Yes. I *think* implying this makes sense in this context, since for texture
image transport the producer context lives on a different channel, and needs to
wait for that sync point.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:689: container);
// Hold a reference to the texture
On 2012/11/09 22:02:01, piman wrote:
> I think we don't need this. By the time CropScaleReadbackAndCleanTexture
> returns, the texture won't be used any more (it internally copies/scales it
> before running the asynchronous part).
> I think the thumbnailer-related asynchronous code is overly conservative and
can
> be simplified.

Right it does the copy first on the UI thread. Removed.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:812: if
(ShouldFastACK(params_in_pixel.size)) {
When exactly would this happen?
It sounds though like it would be harmless then to skip the frame also when
!compositor?

On 2012/11/12 16:52:15, jonathan.backer wrote:
> I'm torn as to handle the detached (no compositor state). The decision on how
to
> handle it was made before the current implementation of fast-ACKing (when the
UI
> compositor was single-threaded).
> 
> It's clear from RWHVA::WasShown that we're expecting a new frame to come in
when
> a tab was made visible (we're reattached). Here are the advantages of fast-ACK
> when detached:
> - no copy on PostSubBuffer
> - no insertion of SyncPoint
> - no commit (in threaded case)
> 
> WDYT?

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:835: if
(!skipped_damage_.IsEmpty())
On 2012/11/09 22:02:01, piman wrote:
> The test isn't useful, since you always end up with the empty rect in either
> case.

Done.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:877:
scoped_refptr<ui::Texture>& current_texture =
On 2012/11/09 22:02:01, piman wrote:
> scoped_refptr<ui::Texture>& -> ui::Texture*? You're not modifying the map so
no
> need for the extra indirection.

Done.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:886:
DCHECK(damage_rect == surface_rect);
On 2012/11/09 22:02:01, piman wrote:
> mmh, you're really asserting something that's coming from a (potentially
> malicious) renderer.
> What happens if it's not true? Sounds like we only get bad pixels (which is ok
> for a malicious renderer). change to a DLOG_IF?

Done.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:891: damage_rect =
UnionRects(skipped_damage_, damage_rect);
On 2012/11/12 16:52:15, jonathan.backer wrote:
> On 2012/11/09 22:02:01, piman wrote:
> > So the problem here (and when we union the skipped_damage_ on fast acks), is
> > that we keep the bounding box of the damage instead of the actual rects. So
> when
> > we try to copy previous_damage-damage, we may end up forgetting to copy some
> > pixels  that were not made up-to-date.
> > Should we make all the damage tracking structs regions?
> 
> Good catch.

Done.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:897: if
(compositor) {
On 2012/11/12 16:52:15, jonathan.backer wrote:
> On 2012/11/09 22:02:01, piman wrote:
> > I think you should do this always. At least the copy. You may not have a
> > compositor if you're not in the tree, but in this case you still want to
> ensure
> > that the damage is copied so that when we come back into the tree the
texture
> is
> > sane.
> 
> As I've mentioned above, I have a slight preference for going the other way:
> treat the lack of compositor as an opportunity for a fast-ACK.

Done, I let it skip the frame if !GetCompositor(). Anything I should test for
this?

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:909:
previous_damage_rect_);
On 2012/11/12 16:52:15, jonathan.backer wrote:
> Which context is current here? Maybe it doesn't matter because they are all in
> the same share group?

Yes, the thumbnailer also uses the GLHelper and relies on having access to the
frontbuffer texture in the context the helper was initialized in. I put a
flush() in that routine to make sure the copy is issued before the compositor
uses the texture.

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:968: if
(current_surface_ && ShouldReleaseFrontSurface() &&
On 2012/11/09 22:02:01, piman wrote:
> We should only do this if current_surface_==surface_handle

Well, I guess I changed the semantics of the message now to indicate that we
should drop the frontbuffer. I'm not sure if it's necessary for the surface to
tell RWHVA when it freed the backbuffer, because the actual GL texture is owned
by the surface and the 'other' texture here should not refer to any real GL
texture (with the exception of the transition - I probably assumed that the
surface would not send a release message while an ack is pending, but maybe I
have to revisit that assumption and see if it's a problem).

http://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:969:
host_->is_accelerated_compositing_active()) {
On 2012/11/09 22:02:01, piman wrote:
> Why is this test needed? Note, it's almost positively racy (it's a state that
> comes from the renderer whereas AcceleratedSurfaceRelease comes from the GPU
> process).

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/client/...
File content/common/gpu/client/gl_helper.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/client/...
content/common/gpu/client/gl_helper.cc:761: region.op(RectToSkIRect(new_damage),
SkRegion::kDifference_Op);
On 2012/11/12 16:52:15, jonathan.backer wrote:
> Can we early exit if the region is empty? A common case is flash video, where
> the damage rects are always the same (no need to copy). I'd prefer not to even
> flush in this case.

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/gpu_mes...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/gpu_mes...
content/common/gpu/gpu_messages.h:281: uint64 /* surface_handle */,
On 2012/11/09 22:02:01, piman wrote:
> Please document above what this surface handle represents (i.e. AFAIU it's a
> buffer that gets "recycled" but I don't know that until I read the
> implementation of TextureImageTransportSurface)

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:72: // TODO: Move this
somewhere else.
On 2012/11/12 16:52:15, jonathan.backer wrote:
> Not sure it belongs anywhere else. We only want pre-emption for TITSurfaces
with
> a parent channel.

I've removed the TODO for now. But maybe a more generic mechanism to prioritize
channels would be nice.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:170:
DCHECK(!is_swap_buffers_pending_);
On 2012/11/09 22:02:01, piman wrote:
> I don't think this check is valid. This can be called from
> SetFrontbufferAllocation which can be called from a posted task at any time.

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:171: if
(!helper_->MakeCurrent())
On 2012/11/09 22:02:01, piman wrote:
> We don't need this anymore.

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:223: AddRef();
On 2012/11/12 16:52:15, jonathan.backer wrote:
> On 2012/11/09 22:02:01, piman wrote:
> > I'm not sure you'll actually be getting an ACK if the renderer gets closed
> > (which then would be a leak). Have you verified this?
> 
> We have a default ACK in GpuProcessHostUIShim. I think this is safe. Still
ugly.

Yes, agreed that it's not pretty. Although I think conceptually it's nice to
avoid mailbox leaks through simply guaranteeing ACK cycles to be finished. I
didn't want to mess with a ton of plumbing or other objects' lifetimes.

Will the planned changes to mailbox (where the mailbox owns the gl texture id)
somehow make it easier to avoid leaks in the mailbox and allow me to remove this
hack?

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:324: glDeleteTextures(1,
&backbuffer_.service_id);
On 2012/11/09 22:02:01, piman wrote:
> Al is working on changes that will make it so that the service id is owned by
> the mailbox, that will be owned by the client ids in the respective contexts,
so
> this can go away eventually. Add a TODO to remove?
> As is, it could in theory leak GPU data if the GPU process gets in a state
where
> it doesn't have contexts any more (not in the Aura case, but you may want to
> check Android behavior. I'm also planning on reusing this for Pepper in
non-Aura
> cases where it could happen).


Ok, sounds good and like it also will render the AddRef() hack unnecessary then.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:364:
ScopedFrameBufferBinder fbo_binder(fbo_id_);
On 2012/11/12 16:52:15, jonathan.backer wrote:
> Why the framebuffer binder? Seems like a no-op. Maybe we also want a
> glFramebufferTexture2D to release the backbuffer from the fbo?

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:374: // reallocating the
backbuffer now.
On 2012/11/12 16:52:15, jonathan.backer wrote:
> comment nit: "If |is_swap_buffers_pending|, we are waiting..."

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:377: gfx::Size& size =
current_size_;
On 2012/11/12 16:52:15, jonathan.backer wrote:
> Got tired of typing?

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:378: Texture& texture =
backbuffer_;
On 2012/11/09 22:02:01, piman wrote:
> nit: you have a single texture now, so that indirection isn't really necessary
> anymore (i.e. can use backbuffer_ and current_size_ directly below).

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:383: if
(!texture.service_id) {
On 2012/11/09 22:02:01, piman wrote:
> nit: no need for braces any more.

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:388: texture.size = size;
On 2012/11/12 16:52:15, jonathan.backer wrote:
> nit: no if necessary.

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:407:
mailbox_names_[texture.identifier] = mailbox_name;
On 2012/11/09 22:02:01, piman wrote:
> Al can confirm but I don't think it's ok to change the mailbox name associated
> with a service id, so you probably want to do this lazily.

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:433: DCHECK(false);
On 2012/11/09 22:02:01, piman wrote:
> nit: just make the above DLOG(FATAL)?

Done.

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.h (left):

http://codereview.chromium.org/11194042/diff/23001/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.h:22: public
base::SupportsWeakPtr<TextureImageTransportSurface> {
On 2012/11/12 16:52:15, jonathan.backer wrote:
> Why can we lose this?

It was only needed to post the BufferPresentedImpl() task. Now I bind the task
to the refcounted object (refcounted through the base class GLSurface), since I
always want to finish the ACK cycle.

http://codereview.chromium.org/11194042/diff/23001/gpu/command_buffer/service...
File gpu/command_buffer/service/mailbox_manager.cc (right):

http://codereview.chromium.org/11194042/diff/23001/gpu/command_buffer/service...
gpu/command_buffer/service/mailbox_manager.cc:39: if (it == textures_.end()) {
On 2012/11/09 22:02:01, piman wrote:
> nit: no need for braces any more.

Done.

http://codereview.chromium.org/11194042/diff/23001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

http://codereview.chromium.org/11194042/diff/23001/ipc/ipc_message_utils.cc#n...
ipc/ipc_message_utils.cc:429: void ParamTraits<std::vector<signed char>
>::Write(Message* m,
On 2012/11/09 22:02:01, piman wrote:
> On 2012/11/09 19:19:04, apatrick_chromium wrote:
> > Are you sure you need explicit ParamTraits for std::vector? I think it will
> just
> > work?
> 
> It will work though use an int for each char (i.e. conversion + extra size).
> 
> > 
> > Also, I've been using std::string to shuttle mailbox names around by IPC.
It's
> > less typing :) std::string can hold null characters so it works.
> 
> This sounds like a good idea.

I switched it to using strings.

[piman, 2012-11-19 22:09:52]

https://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:812: if
(ShouldFastACK(params_in_pixel.size)) {
On 2012/11/12 16:52:15, jonathan.backer wrote:
> I'm torn as to handle the detached (no compositor state). The decision on how
to
> handle it was made before the current implementation of fast-ACKing (when the
UI
> compositor was single-threaded).
> 
> It's clear from RWHVA::WasShown that we're expecting a new frame to come in
when
> a tab was made visible (we're reattached). Here are the advantages of fast-ACK
> when detached:
> - no copy on PostSubBuffer
> - no insertion of SyncPoint
> - no commit (in threaded case)
> 
> WDYT?

When detached, there'll be no commit either since you don't have a compositor to
commit to.

On tab switch, prior to this patch, we have the case where we don't wait for the
renderer to redraw everything (including uploading textures), where the GPU
process had kept the front buffer valid, and sends it immediately. It's critical
for fast tab switch.
On this new version, it requires us to handle the swaps coming from the GPU
process, even when not in the tree.

https://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:909:
previous_damage_rect_);
On 2012/11/12 16:52:15, jonathan.backer wrote:
> Which context is current here? Maybe it doesn't matter because they are all in
> the same share group?

On the client side, makeContextCurrent is essentially a noop after the first one
(which creates the context), so I think we've been sloppy with it. If anything
I'd expect the GLHelper to take care of it.

https://codereview.chromium.org/11194042/diff/23001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/23001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:72: // TODO: Move this
somewhere else.
On 2012/11/12 16:52:15, jonathan.backer wrote:
> Not sure it belongs anywhere else. We only want pre-emption for TITSurfaces
with
> a parent channel.

The preemption concept is orthogonal to whether we use
TextureImageTransportSurfaces. In the ÜC world, we won't use them any more, but
still want the preemption feature.

https://codereview.chromium.org/11194042/diff/23001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:122:
glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_id_);
On 2012/11/12 16:52:15, jonathan.backer wrote:
> On 2012/11/09 19:19:04, apatrick_chromium wrote:
> > This changes the FBO binding for the context and it is not restored. Is that
> > okay? Elsewhere, ScopedFrameBufferBinder is used to restore the binding.
> 
> So this is the first time that MakeCurrent is ever called. In this case, we'd
> have the default fbo (0) active no?

Yes.

> We've got a slight of hand here: for TITSurface, the default fbo is meant to
be
> |fbo_id|. This remapping is usually handled by
> |GLES2DecoderImpl::DoBindFramebuffer|. I think that it's appropriate here.

Yeah, that's more or less what I meant above.

[no sievers, 2012-11-19 22:26:18]

Please take a look at #7 vs. #6.

I noticed that SetBackbufferAllocation() is racing with the swap ack returning a
buffer. I changed this to:
- ignore SetBackbufferAllocation(true) and always allocate the buffer lazily in
OnMakeCurrent() instead
- however bail out of CreateBackTexture() if an ACK is pending (because of the
deferred descheduling logic we might be made current for non-draw GL commands)
- free the backbuffer when the ACK arrives, we consume it, and the allocation
was changed to !allocated in the meantime

[piman, 2012-11-19 22:31:53]

https://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_...
content/browser/renderer_host/image_transport_factory.cc:182: GL_TEXTURE_2D,
mailbox_name_.data());
On 2012/11/19 20:30:44, Daniel Sievers wrote:
> On 2012/11/12 16:52:15, jonathan.backer wrote:
> > No flush() because of sync point?
> 
> Yes. I *think* implying this makes sense in this context, since for texture
> image transport the producer context lives on a different channel, and needs
to
> wait for that sync point.

My thoughts as well. Inserting the sync point will implicitly flush anyway.

https://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11194042/diff/23001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:812: if
(ShouldFastACK(params_in_pixel.size)) {
On 2012/11/19 20:30:44, Daniel Sievers wrote:
> When exactly would this happen?
> It sounds though like it would be harmless then to skip the frame also when
> !compositor?

GetCompositor() walks the window tree to find the compositor. If it's not in the
tree, it'll return NULL, but we still need to handle updates for when we get
back into the tree. I'm not 100% sure which UI operations exactly cause this to
happen, but they happen at a higher level. Among others, I believe instant and
prerender do this. Also, we can't strongly associate a RWHVA with a compositor
because of multiple screens.

At a high level, I've been considering this:
1- if we have a compositor, changing textures has some implications to that
compositor: it needs to know which area to redraw, and we need to synchronize
with the rendering logic (on another thread or not) so that we don't modify the
texture that is rendered from.
2- whether we have a compositor or not, the RWHVA still needs to keep track of
the renderer state, whether coming from the renderer or the GPU process on
behalf of it. That state includes the "front buffer", i.e. which texture it is,
and whether we need to copy dirty rects (with this patch).

I think executing the parts in (2) only when we have a compositor would be
wrong.
> 
> On 2012/11/12 16:52:15, jonathan.backer wrote:
> > I'm torn as to handle the detached (no compositor state). The decision on
how
> to
> > handle it was made before the current implementation of fast-ACKing (when
the
> UI
> > compositor was single-threaded).
> > 
> > It's clear from RWHVA::WasShown that we're expecting a new frame to come in
> when
> > a tab was made visible (we're reattached). Here are the advantages of
fast-ACK
> > when detached:
> > - no copy on PostSubBuffer
> > - no insertion of SyncPoint
> > - no commit (in threaded case)
> > 
> > WDYT?
>

[no sievers, 2012-11-20 00:03:03]

https://codereview.chromium.org/11194042/diff/31018/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11194042/diff/31018/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1746:
image_transport_clients_.clear();
This is a problem if we lose the UI/helper context here, but do not lose the
renderer context. In that case we would not be getting a SurfaceNew message,
i.e. the owned texture is lost.

[jonathan.backer, 2012-11-20 21:21:10]

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:826:
skipped_damage_.setEmpty();
I like the idea of the sanity check, but we're not using
|previous_surface_size_| anywhere else. I noticed that we're doing a similar
check on |AcceleratedSurfacePostSubBuffers|. Maybe drop this?

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:882:
ui::Compositor* compositor = GetCompositor();
Don't we know we have a compositor here? If not |SwapBuffersPrepare| would have
returned false and we would have bailed earlier.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:908: damage_rect =
gfx::SkIRectToRect(damage.getBounds());
Why reset |damage_rect|? Just for the following DCHECK?

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:915:
ui::Compositor* compositor = GetCompositor();
Same as above. We know that we have a compositor here.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:932:
previous_damage_ = damage;
This seems like too much damage! We're flipping and copying here. So the next
frame should be relative to damage rect passed in to this function, no?

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:998:
surface_ref));
Isn't this a no-op now?

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1915:
ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
nit: put this in the scope with the call to InsertSyncPoint().

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:368: SkRegion
previous_damage_;
nit: I think that we can get away with just storing a rect here.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/gpu_me...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/gpu_me...
content/common/gpu/gpu_messages.h:86: IPC_STRUCT_MEMBER(int32, surface_id)
If we're not going to use the |surface_id| on USE_AURA, should we drop it?

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:132:
DCHECK(helper_->stub());
Seems a little late for this. Probably would have failed to get a |context_|
above.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:311: Release();
Hmm... do we want |BufferPresentedImpl| to do the |Release()|? Otherwise we
might not hit the code there that deletes a texture.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:377: if
(is_swap_buffers_pending_)
I'm confused (you may have addressed this before), but isn't this an error
condition?

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:384:
backbuffer_.identifier));
nit: indentation

[jonathan.backer, 2012-11-21 05:47:05]

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:932:
previous_damage_ = damage;
On 2012/11/20 21:21:11, jonathan.backer wrote:
> This seems like too much damage! We're flipping and copying here. So the next
> frame should be relative to damage rect passed in to this function, no?

Please ignore this comment.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:368: SkRegion
previous_damage_;
On 2012/11/20 21:21:11, jonathan.backer wrote:
> nit: I think that we can get away with just storing a rect here.

I think I got this wrong.

We have to handle the case where we collect damage, present, collect damage, and
present. Using a region does this. FWIW, I think that a bounding box does too
(for the |previous_damage_| not |skipped_damage_|). But the region is simpler
and obviously correct.

Please ignore previous comment.

[piman, 2012-11-29 04:31:11]

I still owe you a full review, but I posted
https://codereview.chromium.org/11316238/ and it should simplify this a bit.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:162: // a swap ACK
pending. Either way, we will either retrieve the texture when
note: with https://codereview.chromium.org/11316238/ this shouldn't happen any
more. Would it simplify this CL given that?

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:311: Release();
On 2012/11/20 21:21:11, jonathan.backer wrote:
> Hmm... do we want |BufferPresentedImpl| to do the |Release()|? Otherwise we
> might not hit the code there that deletes a texture.

This looks fine, in the first case (sync_point==0) it's done, in the second case
the callback kept a reference, that will be released after the
BufferPresentedImpl.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:377: if
(is_swap_buffers_pending_)
On 2012/11/20 21:21:11, jonathan.backer wrote:
> I'm confused (you may have addressed this before), but isn't this an error
> condition?

I think with https://codereview.chromium.org/11316238/ it now should be.

[no sievers, 2012-11-29 19:57:00]

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:162: // a swap ACK
pending. Either way, we will either retrieve the texture when
On 2012/11/29 04:31:11, piman wrote:
> note: with https://codereview.chromium.org/11316238/ this shouldn't happen any
> more. Would it simplify this CL given that?

We could allocate it now if allocation == true, or do it lazily from
OnMakeCurrent(). Also, line 342 could go away.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:377: if
(is_swap_buffers_pending_)
On 2012/11/29 04:31:11, piman wrote:
> On 2012/11/20 21:21:11, jonathan.backer wrote:
> > I'm confused (you may have addressed this before), but isn't this an error
> > condition?
> 
> I think with https://codereview.chromium.org/11316238/ it now should be.

We could still get non-drawing GL commands, right? (In particular allowing
texture uploads I guess were the main reason for defering the descheduling to
draw commands.)
Do we have to defer other commands that touch the framebuffer, such as
CopyTexImage and ReadPixels?

I.e. the difference with mailbox vs. before is that we don't have a backbuffer
here (the RWHV has it) while ack_pending.

[piman, 2012-11-30 03:54:31]

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/image_transport_factory.cc:159: :
OwnedTexture(host_context,
nit: indent (':' should be at +4, i.e. under the 'c' of const).

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:87: if
(texture_id_in_layer_)
nit: braces around body

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:818:
BufferPresentedParams& params) {
nit: style says no non-const reference as parameter (use a pointer).

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:830: if
(ShouldSkipFrame(surface_rect.size()) || !GetCompositor()) {
As mentioned before, even if we don't have a compositor (e.g. if we're not in
the tree) we should keep the frame that was given, so that we can display it
when we get it back.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:843:
params.surface_handle = previous_surface;
nit: how about std::swap?

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:998:
surface_ref));
On 2012/11/20 21:21:11, jonathan.backer wrote:
> Isn't this a no-op now?

My understanding is that it's just there to keep a refcount. Arguably we could
do it explicitly (a list of ui::Texture to release on commit) but I'm not sure
it'd be a net win.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1915:
ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
On 2012/11/20 21:21:11, jonathan.backer wrote:
> nit: put this in the scope with the call to InsertSyncPoint().

Agreed, and you can combine the bodies of the 2 ifs.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1921: sync_point =
factory->InsertSyncPoint();
nit: indent

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/gpu_me...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/gpu_me...
content/common/gpu/gpu_messages.h:49: IPC_STRUCT_MEMBER(std::string,
mailbox_name)
note: considering the GPU process as untrusted, we should verify that the
mailbox_name's size is 64 bytes (or 0) and refuse the IPC if not.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:317:
backbuffer_.identifier = surface_handle;
nit: DCHECK it's 1 or 2?

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:329: glDeleteTextures(1,
&backbuffer_.service_id);
nit: indent

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:344: CreateBackTexture();
If we're going to recreate the backbuffer lazily in OnMakeCurrent, we could just
move all this over there now, and save the MakeCurrent on line 340. Also note
the interesting thing, the MakeCurrent may already have recreated the
backbuffer.

The only thing is to ensure we do rebind the correct texture, so we may want to
set a "need_swap_in_makecurrent" flag so that we do it on the next one.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:377: if
(is_swap_buffers_pending_)
On 2012/11/29 19:57:00, Daniel Sievers wrote:
> On 2012/11/29 04:31:11, piman wrote:
> > On 2012/11/20 21:21:11, jonathan.backer wrote:
> > > I'm confused (you may have addressed this before), but isn't this an error
> > > condition?
> > 
> > I think with https://codereview.chromium.org/11316238/ it now should be.
> 
> We could still get non-drawing GL commands, right? (In particular allowing
> texture uploads I guess were the main reason for defering the descheduling to
> draw commands.)
> Do we have to defer other commands that touch the framebuffer, such as
> CopyTexImage and ReadPixels?
> 
> I.e. the difference with mailbox vs. before is that we don't have a backbuffer
> here (the RWHV has it) while ack_pending.

So, yes, we may run some GL commands, which will call OnMakeCurrent. That said,
I'd rather add logic there and put a DCHECK here.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:388:
mailbox_names_[backbuffer_.identifier] = new_mailbox_name;
Should we check that the mailbox_names_ slot is empty? That should ensure we
don't generate more than 2 textures and that we don't try to associate mailboxes
with the wrong texture etc.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:414:
params.mailbox_name.append((char*)&mailbox_name, sizeof(mailbox_name));
nit: no c-style cast (use reinterpret_cast).

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.h (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:90: // The current
backbuffer
nit: .

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:119: std::map<uint64,
gpu::gles2::MailboxName> mailbox_names_;
Because the ids are only 1 and 2, maybe just make this a 2-entries array? Maybe
with an accessor gpu::gles2::MailboxName& mailbox_name(uint64 id) {
mailbox_names_[id-1]; } to hide the offset?

[no sievers, 2012-12-01 00:58:38]

Addressed all comments.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:826:
skipped_damage_.setEmpty();
On 2012/11/20 21:21:11, jonathan.backer wrote:
> I like the idea of the sanity check, but we're not using
> |previous_surface_size_| anywhere else. I noticed that we're doing a similar
> check on |AcceleratedSurfacePostSubBuffers|. Maybe drop this?

But I do want to reset the skipped_damage_ here.
The main problem is that it causes real errors if I use a rect too large after
the surface shrunk during the damage copy.
I think I also had it clip it to the surface size at some point, but for reasons
I don't remember exactly ended up keeping the skipped_damage_ precise here.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:830: if
(ShouldSkipFrame(surface_rect.size()) || !GetCompositor()) {
On 2012/11/30 03:54:32, piman wrote:
> As mentioned before, even if we don't have a compositor (e.g. if we're not in
> the tree) we should keep the frame that was given, so that we can display it
> when we get it back.

Ok, so I've changed it to *not* do the fast ack here if there is no compositor,
and also do the damage copy, but do not schedule a paint. Is that good?

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:882:
ui::Compositor* compositor = GetCompositor();
On 2012/11/20 21:21:11, jonathan.backer wrote:
> Don't we know we have a compositor here? If not |SwapBuffersPrepare| would
have
> returned false and we would have bailed earlier.

Since I changed it in the latest patch to copy the damage still, we could have
one now.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:998:
surface_ref));
On 2012/11/30 03:54:32, piman wrote:
> On 2012/11/20 21:21:11, jonathan.backer wrote:
> > Isn't this a no-op now?
> 
> My understanding is that it's just there to keep a refcount. Arguably we could
> do it explicitly (a list of ui::Texture to release on commit) but I'm not sure
> it'd be a net win.

Yes, it's there to make sure we don't delete the texture until did_commit.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:368: SkRegion
previous_damage_;
On 2012/11/21 05:47:05, jonathan.backer wrote:
> On 2012/11/20 21:21:11, jonathan.backer wrote:
> > nit: I think that we can get away with just storing a rect here.
> 
> I think I got this wrong.
> 
> We have to handle the case where we collect damage, present, collect damage,
and
> present. Using a region does this. FWIW, I think that a bounding box does too
> (for the |previous_damage_| not |skipped_damage_|). But the region is simpler
> and obviously correct.
> 
> Please ignore previous comment.

yes, i thought about a rect too. I guess it would maybe simplify the math a bit,
but cause us to copy too conservatively. i'll leave it as a region then for now.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/gpu_me...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/gpu_me...
content/common/gpu/gpu_messages.h:49: IPC_STRUCT_MEMBER(std::string,
mailbox_name)
On 2012/11/30 03:54:32, piman wrote:
> note: considering the GPU process as untrusted, we should verify that the
> mailbox_name's size is 64 bytes (or 0) and refuse the IPC if not.

Done in gpu_process_host_ui_shim.cc

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/gpu_me...
content/common/gpu/gpu_messages.h:86: IPC_STRUCT_MEMBER(int32, surface_id)
On 2012/11/20 21:21:11, jonathan.backer wrote:
> If we're not going to use the |surface_id| on USE_AURA, should we drop it?

Actually, we don't really use the identifier either. I changed the semantics to
'release the frontbuffer'. I was a little wary of adding complications and races
if we also tell the RWHV to release the other texture which is currently the
backbuffer. I assume even when leaving compositing it's harmless to keep the
texture until RWHV destruction time, since we know it's not backed by a real GL
texture.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:388:
mailbox_names_[backbuffer_.identifier] = new_mailbox_name;
On 2012/11/30 03:54:32, piman wrote:
> Should we check that the mailbox_names_ slot is empty? That should ensure we
> don't generate more than 2 textures and that we don't try to associate
mailboxes
> with the wrong texture etc.

We do replace the entry though when we reallocate a buffer (e.g. resize). But I
added a check in the accessor that the id is 1 or 2.

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (left):

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:285:
DCHECK(textures_[back()].info->service_id());
I'm removing the DCHECK(textures_[back()].info->service_id()) here.
ProduceTexture() has the same check, and we would hit this with the mailbox
version, if we check it before the early-out. I.e. I do see us getting
PostSubBuffer() calls with empty damage rects, and while
is_swap_buffers_pending_ we do have no backbuffer in the new model.

Ideally we should also defer scheduling for Swap and PostSubBuffers calls with
no draw in between.

[piman, 2012-12-03 21:08:43]

Only couple of questions/minor things.

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:830: if
(ShouldSkipFrame(surface_rect.size()) || !GetCompositor()) {
On 2012/12/01 00:58:38, Daniel Sievers wrote:
> On 2012/11/30 03:54:32, piman wrote:
> > As mentioned before, even if we don't have a compositor (e.g. if we're not
in
> > the tree) we should keep the frame that was given, so that we can display it
> > when we get it back.
> 
> Ok, so I've changed it to *not* do the fast ack here if there is no
compositor,
> and also do the damage copy, but do not schedule a paint. Is that good?

Yep, great.

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:388:
mailbox_names_[backbuffer_.identifier] = new_mailbox_name;
On 2012/12/01 00:58:38, Daniel Sievers wrote:
> On 2012/11/30 03:54:32, piman wrote:
> > Should we check that the mailbox_names_ slot is empty? That should ensure we
> > don't generate more than 2 textures and that we don't try to associate
> mailboxes
> > with the wrong texture etc.
> 
> We do replace the entry though when we reallocate a buffer (e.g. resize).

Why? resize shouldn't cause us to gen a new texture name. It'll just do the
glTexImage2D below. There's the Release path, though arguably we should be
resetting the mailbox name anyway.

 But I
> added a check in the accessor that the id is 1 or 2.

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:165: if
(!backbuffer_suggested_allocation_ && helper_->MakeCurrent())
I think the MakeCurrent is superfluous (as was before, but I think is a
leftover): SetBackbufferAllocation is called from an IPC message, and when
handling any of them (not limited to command buffer's Flush), we make the
context current (see GpuCommandBufferStub::OnMessageReceived)

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:166: ReleaseBackBuffer();
How are we going to recreate the backbuffer when allocation is true? Are we
relying on MakeCurrent being called before the next commands? It's worth at
least a comment - while it'll happen today because EnsureBackbuffer is a
separate IPC, it wouldn't be true if it was a command buffer command (and I was
thinking of changing it to that).

[no sievers, 2012-12-03 23:20:38]

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/37039/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:388:
mailbox_names_[backbuffer_.identifier] = new_mailbox_name;
On 2012/12/03 21:08:43, piman wrote:
> On 2012/12/01 00:58:38, Daniel Sievers wrote:
> > On 2012/11/30 03:54:32, piman wrote:
> > > Should we check that the mailbox_names_ slot is empty? That should ensure
we
> > > don't generate more than 2 textures and that we don't try to associate
> > mailboxes
> > > with the wrong texture etc.
> > 
> > We do replace the entry though when we reallocate a buffer (e.g. resize).
> 
> Why? resize shouldn't cause us to gen a new texture name. It'll just do the
> glTexImage2D below. There's the Release path, though arguably we should be
> resetting the mailbox name anyway.
> 
>  But I
> > added a check in the accessor that the id is 1 or 2.
> 

Done.

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:165: if
(!backbuffer_suggested_allocation_ && helper_->MakeCurrent())
On 2012/12/03 21:08:43, piman wrote:
> I think the MakeCurrent is superfluous (as was before, but I think is a
> leftover): SetBackbufferAllocation is called from an IPC message, and when
> handling any of them (not limited to command buffer's Flush), we make the
> context current (see GpuCommandBufferStub::OnMessageReceived)

Done.

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:166: ReleaseBackBuffer();
On 2012/12/03 21:08:43, piman wrote:
> How are we going to recreate the backbuffer when allocation is true? Are we
> relying on MakeCurrent being called before the next commands? It's worth at
> least a comment - while it'll happen today because EnsureBackbuffer is a
> separate IPC, it wouldn't be true if it was a command buffer command (and I
was
> thinking of changing it to that).

It's probably harmless to call CreateBackTexture() here also (although it might
already have gotten allocated from OnMakeCurrent()). Is it maybe better to do it
lazily though once it's actually needed? Or are you thinking it might be better
to do it early, since the call to TexImage2D() will likely not be free?

[piman, 2012-12-03 23:33:55]

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:166: ReleaseBackBuffer();
On 2012/12/03 23:20:38, Daniel Sievers wrote:
> On 2012/12/03 21:08:43, piman wrote:
> > How are we going to recreate the backbuffer when allocation is true? Are we
> > relying on MakeCurrent being called before the next commands? It's worth at
> > least a comment - while it'll happen today because EnsureBackbuffer is a
> > separate IPC, it wouldn't be true if it was a command buffer command (and I
> was
> > thinking of changing it to that).
> 
> It's probably harmless to call CreateBackTexture() here also (although it
might
> already have gotten allocated from OnMakeCurrent())

It thought it wouldn't have, because at the time of the MakeCurrent,
backbuffer_suggested_allocation_ wouldn't be set.
Turns out we don't look at it in either OnMakeCurrent or CreateBackTexture,
which is a problem (e.g. the compositor may try to upload textures without
recreating the back buffer, which would implicitly reallocate a back buffer when
we don't want one.)

. Is it maybe better to do it
> lazily though once it's actually needed? Or are you thinking it might be
better
> to do it early, since the call to TexImage2D() will likely not be free?

I don't have a strong opinion on lazily vs immediate. I'm not necessarily asking
for a fix in this CL if it's complex (is it?), since today the code ensures that
MakeCurrent will allocate it, just maybe asking for a comment. But thinking
about it more it uncovered the above...

If we want to do it lazily, another possible place to do it is in DeferDraws,
since it should now be called for all commands that need a back buffer.

[no sievers, 2012-12-04 00:05:36]

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/45001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:166: ReleaseBackBuffer();
On 2012/12/03 23:33:56, piman wrote:
> On 2012/12/03 23:20:38, Daniel Sievers wrote:
> > On 2012/12/03 21:08:43, piman wrote:
> > > How are we going to recreate the backbuffer when allocation is true? Are
we
> > > relying on MakeCurrent being called before the next commands? It's worth
at
> > > least a comment - while it'll happen today because EnsureBackbuffer is a
> > > separate IPC, it wouldn't be true if it was a command buffer command (and
I
> > was
> > > thinking of changing it to that).
> > 
> > It's probably harmless to call CreateBackTexture() here also (although it
> might
> > already have gotten allocated from OnMakeCurrent())
> 
> It thought it wouldn't have, because at the time of the MakeCurrent,
> backbuffer_suggested_allocation_ wouldn't be set.
> Turns out we don't look at it in either OnMakeCurrent or CreateBackTexture,
> which is a problem (e.g. the compositor may try to upload textures without
> recreating the back buffer, which would implicitly reallocate a back buffer
when
> we don't want one.)
> 

Good point. I added a check for backbuffer_suggested_allocation_ in
OnMakeCurrent(). I also simply added the call to CreateBackTexture() here to
keep it simple for now.

> . Is it maybe better to do it
> > lazily though once it's actually needed? Or are you thinking it might be
> better
> > to do it early, since the call to TexImage2D() will likely not be free?
> 
> I don't have a strong opinion on lazily vs immediate. I'm not necessarily
asking
> for a fix in this CL if it's complex (is it?), since today the code ensures
that
> MakeCurrent will allocate it, just maybe asking for a comment. But thinking
> about it more it uncovered the above...
> 
> If we want to do it lazily, another possible place to do it is in DeferDraws,
> since it should now be called for all commands that need a back buffer.

[piman, 2012-12-04 00:09:22]

LGTM. Thanks for bearing with all the back-and-forth. I think we're ending up in
a good place!

I think backer@ wanted to take a last look too, so please wait for his comments.
Lastly, since you're touching *_messages.h, you'll need a security person to
take a look.
It would be a good idea to send try runs already tough.

[no sievers, 2012-12-04 00:11:48]

+cevans for gpu_messages.h

[jonathan.backer, 2012-12-04 19:15:12]

One concern about leaking backbuffer textures on teardown, otherwise just nits.

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:62: using
gfx::SkIRectToRect;
What does this do? Prevents you from having to prefix with gfx:: on these calls?
AFAICT, you've already got the gfx:: at the call sites...

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:919: if
(ack_params.texture_to_produce && !previous_damage_.isEmpty() &&
nit: Is |previous_damage_.isEmpty()| ever true? I thought we dropped the
PostSubBuffer in the GPU process if |damage_rect| was empty.

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:277: static void
InsertSyncPointAndACK(const BufferPresentedParams& params);
nit: Maybe this should be in the anonymous namespace in the .cc? I don't see
refs outside of that file.

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:369: // The damage
in the previous buffer.
nit: |previous_damage_| and |previous_surface_size_| sound really similar, but
|previous_damage_| refers to the last presented frame whereas
|previous_surface_size_| refers to the last received frame. Maybe clarify this
(in name or comment)?

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/client...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:23: #include "ui/gfx/skia_util.h"
nit: necessary?

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/gpu_me...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/gpu_me...
content/common/gpu/gpu_messages.h:87: IPC_STRUCT_MEMBER(uint64, identifier)
Can we nix |identifier|? AFAICT, we're only using this for CrOS (in RWHVA) and
with this CL it is completely ignored.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:88: 
Do we need a |ReleaseBackTexture()| here or in OnWillDestroyStub()? AFAICT, we
may leak otherwise. I think that we may have been able to get away with this
before because the browser would destroy the textures (maybe?).

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:181: void
TextureImageTransportSurface::AdjustFrontBufferAllocation() {
Can we nuke this separate method? AFAICT, it's only called in one place now (it
used to be called in OnSetFrontSurfaceIsProtected as well).

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:227: const uint64
identifier = backbuffer_.identifier;
nit: nix identifier

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:236: params.size =
current_size_;
|backbuffer_.size| instead (they should be equal)

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:257: const uint64
identifier = backbuffer_.identifier;
nit: nix identifier

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:266: params.surface_size =
current_size_;
|backbuffer_.size| instead?

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:352: if (backbuffer_.size
!= current_size_ || !backbuffer_.service_id)
AFAICT, it's only in this method and OnResize that |backbuffer_.size| could not
be equal |current_size_|. I propose nixing |current_size_| and using
|backbuffer_.size| wherever possible. IIRC, this is how it used to work. It's
less state that could get out of sync. I don't feel strongly about this.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:370: void
TextureImageTransportSurface::ReleaseBackTexture() {
Modulo the comment I made about leaking before, currently this is called from
only one place. I'd move the code there.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.h (right):

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:8: #include <map>
nit: necessary?

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:74: uint64 identifier;
nit: s/identifier/surface_handle?

This would disambiguate it with other identifiers (like service_id). Plus this
is what we call it in the IPCs.

In other places you are using id instead of identifier. Maybe do this there too?

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:98: gfx::Size
current_size_;
Maybe a comment on why this differs from |backbuffer_.size|? Or nix it all
together.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:123: // identifier as the
key.
nit: s/texture identifier/surface_handle?

[Chris Evans, 2012-12-05 20:43:00]

On 2012/12/04 19:15:12, jonathan.backer wrote:
> One concern about leaking backbuffer textures on teardown, otherwise just
nits.
> 
>
https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_aura.cc:62: using
> gfx::SkIRectToRect;
> What does this do? Prevents you from having to prefix with gfx:: on these
calls?
> AFAICT, you've already got the gfx:: at the call sites...
> 
>
https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_aura.cc:919: if
> (ack_params.texture_to_produce && !previous_damage_.isEmpty() &&
> nit: Is |previous_damage_.isEmpty()| ever true? I thought we dropped the
> PostSubBuffer in the GPU process if |damage_rect| was empty.
> 
>
https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_view_aura.h (right):
> 
>
https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_aura.h:277: static void
> InsertSyncPointAndACK(const BufferPresentedParams& params);
> nit: Maybe this should be in the anonymous namespace in the .cc? I don't see
> refs outside of that file.
> 
>
https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_aura.h:369: // The
damage
> in the previous buffer.
> nit: |previous_damage_| and |previous_surface_size_| sound really similar, but
> |previous_damage_| refers to the last presented frame whereas
> |previous_surface_size_| refers to the last received frame. Maybe clarify this
> (in name or comment)?
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/client...
> File content/common/gpu/client/gl_helper.cc (right):
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/client...
> content/common/gpu/client/gl_helper.cc:23: #include "ui/gfx/skia_util.h"
> nit: necessary?
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/gpu_me...
> File content/common/gpu/gpu_messages.h (right):
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/gpu_me...
> content/common/gpu/gpu_messages.h:87: IPC_STRUCT_MEMBER(uint64, identifier)
> Can we nix |identifier|? AFAICT, we're only using this for CrOS (in RWHVA) and
> with this CL it is completely ignored.
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> File content/common/gpu/texture_image_transport_surface.cc (right):
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.cc:88: 
> Do we need a |ReleaseBackTexture()| here or in OnWillDestroyStub()? AFAICT, we
> may leak otherwise. I think that we may have been able to get away with this
> before because the browser would destroy the textures (maybe?).
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.cc:181: void
> TextureImageTransportSurface::AdjustFrontBufferAllocation() {
> Can we nuke this separate method? AFAICT, it's only called in one place now
(it
> used to be called in OnSetFrontSurfaceIsProtected as well).
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.cc:227: const uint64
> identifier = backbuffer_.identifier;
> nit: nix identifier
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.cc:236: params.size =
> current_size_;
> |backbuffer_.size| instead (they should be equal)
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.cc:257: const uint64
> identifier = backbuffer_.identifier;
> nit: nix identifier
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.cc:266: params.surface_size
=
> current_size_;
> |backbuffer_.size| instead?
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.cc:352: if
(backbuffer_.size
> != current_size_ || !backbuffer_.service_id)
> AFAICT, it's only in this method and OnResize that |backbuffer_.size| could
not
> be equal |current_size_|. I propose nixing |current_size_| and using
> |backbuffer_.size| wherever possible. IIRC, this is how it used to work. It's
> less state that could get out of sync. I don't feel strongly about this.
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.cc:370: void
> TextureImageTransportSurface::ReleaseBackTexture() {
> Modulo the comment I made about leaking before, currently this is called from
> only one place. I'd move the code there.
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> File content/common/gpu/texture_image_transport_surface.h (right):
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.h:8: #include <map>
> nit: necessary?
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.h:74: uint64 identifier;
> nit: s/identifier/surface_handle?
> 
> This would disambiguate it with other identifiers (like service_id). Plus this
> is what we call it in the IPCs.
> 
> In other places you are using id instead of identifier. Maybe do this there
too?
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.h:98: gfx::Size
> current_size_;
> Maybe a comment on why this differs from |backbuffer_.size|? Or nix it all
> together.
> 
>
https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
> content/common/gpu/texture_image_transport_surface.h:123: // identifier as the
> key.
> nit: s/texture identifier/surface_handle?

I'm not a gpu reviewer, but looks ok from security if that's what you're asking
:)
[I do have some tweak suggestions but for things that exist independently of
this CL, so I'll follow up separately]

[no sievers, 2012-12-05 22:02:17]

On 2012/12/05 20:43:00, Chris Evans wrote:
> 
> I'm not a gpu reviewer, but looks ok from security if that's what you're
asking
> :)
> [I do have some tweak suggestions but for things that exist independently of
> this CL, so I'll follow up separately]

Thanks! 

I've also added an extra check in image_transport_factory.cc for empty
mailbox_name strings (if it's not 0 or 64 gpu_process_host_ui_shim.cc already
rejects the message), so we don't access invalid memory from
glConsume/ProduceTextureCHROMIUM.

[no sievers, 2012-12-05 22:02:34]

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:62: using
gfx::SkIRectToRect;
On 2012/12/04 19:15:13, jonathan.backer wrote:
> What does this do? Prevents you from having to prefix with gfx:: on these
calls?
> AFAICT, you've already got the gfx:: at the call sites...

Done.

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:919: if
(ack_params.texture_to_produce && !previous_damage_.isEmpty() &&
On 2012/12/04 19:15:13, jonathan.backer wrote:
> nit: Is |previous_damage_.isEmpty()| ever true? I thought we dropped the
> PostSubBuffer in the GPU process if |damage_rect| was empty.

It can probably only be empty for the very first frame.

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:277: static void
InsertSyncPointAndACK(const BufferPresentedParams& params);
On 2012/12/04 19:15:13, jonathan.backer wrote:
> nit: Maybe this should be in the anonymous namespace in the .cc? I don't see
> refs outside of that file.

I left it here for now, because if I moved it to the anonymous namespace I'd
have to make BufferPresentedParams public (which is also used below).

https://codereview.chromium.org/11194042/diff/45001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.h:369: // The damage
in the previous buffer.
On 2012/12/04 19:15:13, jonathan.backer wrote:
> nit: |previous_damage_| and |previous_surface_size_| sound really similar, but
> |previous_damage_| refers to the last presented frame whereas
> |previous_surface_size_| refers to the last received frame. Maybe clarify this
> (in name or comment)?

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/client...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:23: #include "ui/gfx/skia_util.h"
On 2012/12/04 19:15:13, jonathan.backer wrote:
> nit: necessary?

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/gpu_me...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/gpu_me...
content/common/gpu/gpu_messages.h:87: IPC_STRUCT_MEMBER(uint64, identifier)
On 2012/12/04 19:15:13, jonathan.backer wrote:
> Can we nix |identifier|? AFAICT, we're only using this for CrOS (in RWHVA) and
> with this CL it is completely ignored.

Good call. Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:88: 
On 2012/12/04 19:15:13, jonathan.backer wrote:
> Do we need a |ReleaseBackTexture()| here or in OnWillDestroyStub()? AFAICT, we
> may leak otherwise. I think that we may have been able to get away with this
> before because the browser would destroy the textures (maybe?).

Good catch. I moved it (and the SendAcceleratedSurfaceRelease()) into
OnWillDestroyStub(), because here we won't have a context/stub anymore.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:181: void
TextureImageTransportSurface::AdjustFrontBufferAllocation() {
On 2012/12/04 19:15:13, jonathan.backer wrote:
> Can we nuke this separate method? AFAICT, it's only called in one place now
(it
> used to be called in OnSetFrontSurfaceIsProtected as well).

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:227: const uint64
identifier = backbuffer_.identifier;
On 2012/12/04 19:15:13, jonathan.backer wrote:
> nit: nix identifier

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:236: params.size =
current_size_;
On 2012/12/04 19:15:13, jonathan.backer wrote:
> |backbuffer_.size| instead (they should be equal)

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:257: const uint64
identifier = backbuffer_.identifier;
On 2012/12/04 19:15:13, jonathan.backer wrote:
> nit: nix identifier

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:266: params.surface_size =
current_size_;
On 2012/12/04 19:15:13, jonathan.backer wrote:
> |backbuffer_.size| instead?

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:352: if (backbuffer_.size
!= current_size_ || !backbuffer_.service_id)
On 2012/12/04 19:15:13, jonathan.backer wrote:
> AFAICT, it's only in this method and OnResize that |backbuffer_.size| could
not
> be equal |current_size_|. I propose nixing |current_size_| and using
> |backbuffer_.size| wherever possible. IIRC, this is how it used to work. It's
> less state that could get out of sync. I don't feel strongly about this.

But we used to have the frontbuffer size also which we would compare against the
backbuffer size.
I need the separate variable in case the buffer we last sent had been resized.
Then the buffer we get back here (which is the other buffer) still has the old
size.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.h (right):

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:8: #include <map>
On 2012/12/04 19:15:13, jonathan.backer wrote:
> nit: necessary?

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:8: #include <map>
On 2012/12/04 19:15:13, jonathan.backer wrote:
> nit: necessary?

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:74: uint64 identifier;
On 2012/12/04 19:15:13, jonathan.backer wrote:
> nit: s/identifier/surface_handle?
> 
> This would disambiguate it with other identifiers (like service_id). Plus this
> is what we call it in the IPCs.
> 
> In other places you are using id instead of identifier. Maybe do this there
too?

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:98: gfx::Size
current_size_;
On 2012/12/04 19:15:13, jonathan.backer wrote:
> Maybe a comment on why this differs from |backbuffer_.size|? Or nix it all
> together.

Done.

https://codereview.chromium.org/11194042/diff/43074/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.h:123: // identifier as the
key.
On 2012/12/04 19:15:13, jonathan.backer wrote:
> nit: s/texture identifier/surface_handle?

Done.

[jonathan.backer, 2012-12-06 14:33:31]

LGTM

This is a very nice cleanup. Thank-you for seeing this through.

[commit-bot: I haz the power, 2012-12-06 16:59:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11194042/52016

[commit-bot: I haz the power, 2012-12-06 17:00:02]

Presubmit check for 11194042-52016 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/common/gpu/gpu_messages.h
    ui/gfx

Presubmit checks took 4.7s to calculate.

[no sievers, 2012-12-06 17:10:20]

TBR sky for ui/gfx/native_widget_types.h

[commit-bot: I haz the power, 2012-12-06 17:10:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11194042/52016

[commit-bot: I haz the power, 2012-12-06 20:10:55]

Change committed as 171569

[no sievers, 2012-12-07 01:20:45]

On 2012/12/06 20:10:55, I haz the power (commit-bot) wrote:
> Change committed as 171569

This got reverted since it seems to make some win aura browser_tests flaky.
I think what all the backtraces there have in common that the ui compositor
context gets lost:
[5308:6360:1206/162634:3538040:ERROR:image_transport_factory.cc(496)] Lost UI
compositor context.

[commit-bot: I haz the power, 2012-12-10 00:01:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11194042/73001

[commit-bot: I haz the power, 2012-12-10 02:39:46]

Retried try job too often on linux_chromeos for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-10 03:47:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11194042/73001

[commit-bot: I haz the power, 2012-12-10 17:58:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11194042/73001

[commit-bot: I haz the power, 2012-12-10 18:00:40]

Change committed as 172087

[Sami, 2012-12-11 16:43:16]

With --enable-virtual-gl-contexts I'm seeing a lot of flicker and what looks
like out of order frames with this patch. Are we perhaps missing some
synchronization between the browser and renderer compositors?