Drop frontbuffers with ui-use-gpu-process, synchronized with browser, decoupled from backbuffer dro…

Drop frontbuffers with ui-use-gpu-process, synchronized with browser, decoupled from backbuffer drop.

On Aura, every time a tab is backgrounded, RenderWidgetHostViewAura will reset its handle to the front surface.  If that tab is foregrounded again, that front surface will not be used until synchronizing with the gpu process to make sure that surface is still available.  By doing this, the gpu process knows when it is safe to discard the front surface.

RWHVA sends a FrontSurfaceIsProtected(bool, int) message to the gpu process to keep it informed about front surface protection.  The int is a state-of-the-world identifier to protect from ABA issues.  RWHVA delays sending FrontSurfaceIsProtected(false) until after the current surface is certain to not be in use, namely after the compositor finishes the current frame and the browser thumbnailer is complete.


BUG=112842
TEST=Manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142408

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142541

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143590

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148657

[mmocny, 2012-04-11 22:58:36]

This patch will drop frontbuffers for texture_image_transport_surface.

I apologize that this patch is doing a few things at once, I started trying to
accomplish the end goal and realized I needed a lot to get there:
* Decoupling drop frontbuffer from drop backbuffer
* Change texture_image_transport_surface to support release/new of either
surface
* Synchronization with browser via AcceleratedSurfaceReleaseACK, with bool
success so that browser can ignore fb release request (if visibility has changed
back to true)
* Updates to EGL and GLX

This patch is not complete:
* I will need to update other image_transport_surface (win, etc)
* I need to synch with @mazda regarding browser thumbnailer to make sure it
doesn't try to thumbnail while we have no frontbuffer

https://chromiumcodereview.appspot.com/10052018/diff/2001/content/common/gpu/...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/2001/content/common/gpu/...
content/common/gpu/texture_image_transport_surface.cc:169:
CreateTexture(front(), textures_[front()].size);
Maybe I should be using back() surface size here

[mmocny, 2012-04-12 16:41:27]

http://codereview.chromium.org/10052018/diff/4001/content/common/gpu/texture_...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/10052018/diff/4001/content/common/gpu/texture_...
content/common/gpu/texture_image_transport_surface.cc:169:
CreateTexture(front(), textures_[front()].size);
I am playing with delaying this recreation until after the next
swap/postsubbuffer.  Otherwise, we tell the browser we have a new surface, but
its contents are empty and we display a blank frame (flash white).
Also, we have an open issue to delay switching tabs until after frontbuffer is
valid -- and since the browser has no way of knowing that contents are empty --
delaying AcceleratedSurfaceNew seems like the best option.

http://codereview.chromium.org/10052018/diff/4001/content/common/gpu/texture_...
content/common/gpu/texture_image_transport_surface.cc:223: previous_damage_rect_
= gfx::Rect(textures_[back()].size);
I think I got this backwards, previous_damage_rect_ should become empty here.

As a coincidence it doesn't matter, because we only recreate frontbuffer before
recreating backbuffer and redrawing a whole frame.

[piman, 2012-04-18 22:12:44]

At a high level I have a couple of suggestions/comments:
- I think you can avoid unscheduling the channel, just delay the actual
destroying of the surface until you receive the ack.
- with this patch, since the release can fail, it seems that we could get into a
situation where we don't release the surface (if switching tabs quickly). But I
think with enough state you can make it never fail:
  - on the UI side, instead of sending back an ack with failure, don't send the
ack yet, until the next swap or it becomes invisible.
  - on the GPU side, keep track of the state we sent to the browser as well as
the corresponding ack we received. We may need to recreate the surface before we
receive the ack, in which case we'll send a new AcceleratedSurfaceNew, which is
fine (provided proper state is kept on the UI side, i.e. that new surface is
conceptually different, even though it has the same surfce_handle). After
swapping to the new surface, we can send the ack to the old one.
- about delaying the creating of the surface for the front buffer, that surface
shouldn't be taken into account anyway until the next swap, so you shouldn't
have to do that.
- In any case, because the renderer context and the UI context are not ordered
wrt one another, calling SetExternalTexture on the window doesn't have an effect
that is guaranteed to be visible in the GPU process until the next swap. You
need to take that into account when sending the ack to avoid releasing "too
early" (while the UI context is still finishing a frame that used that surface).
- to simplify things, you actually never have to tell the browser anything about
the back buffer - except when it becomes the front. So you can freely delete and
recreate the back texture without telling the browser

[mmocny, 2012-05-01 13:59:02]

On 2012/04/18 22:12:44, piman wrote:
> At a high level I have a couple of suggestions/comments:
> - I think you can avoid unscheduling the channel, just delay the actual
> destroying of the surface until you receive the ack.
I already delay destroy until the ACK (the destroy is done in the Ack received
message handler of gpu process).  However, you are right, with a bit more state
I can avoid descheduling the channel.

> - with this patch, since the release can fail, it seems that we could get into
a
> situation where we don't release the surface (if switching tabs quickly). But
I
> think with enough state you can make it never fail:
This isn't really about failure, its about the-state-of-the-world changing such
that we actively want to ignore the request to release the surface.
For this reason, and since the ACK is really a request, I think maybe I should
create a "suggestRelease*" (from gpu to browser) and "discard*" (from browser to
gpu) messages.  This mirrors the current design for backbuffer and more
accurately represents the truth.
The gpu process knows when it would be a good time to discard the frontbuffer
(based on the state of the world), but by the time the suggest message arrives,
that may have changed and so it is only a suggestion.  The browser knows if that
surface is in use upon message arrival (i.e. rapid tab switching).  If it isn't,
browser will clear its handles and only then send a discard request which the
gpu process must process.

>   - on the UI side, instead of sending back an ack with failure, don't send
the
> ack yet, until the next swap or it becomes invisible.
This wont be needed, I think.

>   - on the GPU side, keep track of the state we sent to the browser as well as
> the corresponding ack we received. We may need to recreate the surface before
we
> receive the ack, in which case we'll send a new AcceleratedSurfaceNew, which
is
> fine (provided proper state is kept on the UI side, i.e. that new surface is
> conceptually different, even though it has the same surfce_handle). After
> swapping to the new surface, we can send the ack to the old one.
We won't destroy the surface before the ack, but once we start to not deschedule
the channel, the state of the world may be changing so we will need to track
appropriate state.

> - about delaying the creating of the surface for the front buffer, that
surface
> shouldn't be taken into account anyway until the next swap, so you shouldn't
> have to do that.
Not sure what you mean here?  Is this because we only draw into backbuffer so
there is no sense sending SurfaceNew message until a swap of a full frame?  If
so, yes I agree.

> - In any case, because the renderer context and the UI context are not ordered
> wrt one another, calling SetExternalTexture on the window doesn't have an
effect
> that is guaranteed to be visible in the GPU process until the next swap. You
> need to take that into account when sending the ack to avoid releasing "too
> early" (while the UI context is still finishing a frame that used that
surface).
Also not really sure what you mean here, perhaps you were under the impression
that the browser was releasing the surface resources?  The browser is just
releasing its handles, and the gpu process does not assume anything until it
receives the ACK.
Or, do you mean to say that SetExternalTexture sends a message down to the gpu
process using the ui context and so the ACK message requesting a discard may
come before the browser handle is actually released?  If so, this may possibly
be a concern, but I will point out that we only go down this path if the browser
isn't using this surface, and any frame the ui is finishing will not be using
this surface.. unless due to pipelining it's still finishing some frame from the
past, from a previous visibility state.. but that seems like an unlikely race
condition esp. since frontbuffers are not dropped immediately.

> - to simplify things, you actually never have to tell the browser anything
about
> the back buffer - except when it becomes the front. So you can freely delete
and
> recreate the back texture without telling the browser
Ok.

[mmocny, 2012-05-02 14:13:11]

Pasting Antoines reply from email:

On 2012/05/01 13:59:02, mmocny wrote:
> On 2012/04/18 22:12:44, piman wrote:
> > At a high level I have a couple of suggestions/comments:
> > - I think you can avoid unscheduling the channel, just delay the actual
> > destroying of the surface until you receive the ack.
> I already delay destroy until the ACK (the destroy is done in the Ack received
> message handler of gpu process).  However, you are right, with a bit more
state
> I can avoid descheduling the channel.
> 
> > - with this patch, since the release can fail, it seems that we could get
into
> a
> > situation where we don't release the surface (if switching tabs quickly).
But
> I
> > think with enough state you can make it never fail:
> This isn't really about failure, its about the-state-of-the-world changing
such
> that we actively want to ignore the request to release the surface.
> For this reason, and since the ACK is really a request, I think maybe I should
> create a "suggestRelease*" (from gpu to browser) and "discard*" (from browser
to
> gpu) messages.  This mirrors the current design for backbuffer and more
> accurately represents the truth.
> The gpu process knows when it would be a good time to discard the frontbuffer
> (based on the state of the world), but by the time the suggest message
arrives,
> that may have changed and so it is only a suggestion.  The browser knows if
that
> surface is in use upon message arrival (i.e. rapid tab switching).  If it
isn't,
> browser will clear its handles and only then send a discard request which the
> gpu process must process.
> 
> >   - on the UI side, instead of sending back an ack with failure, don't send
> the
> > ack yet, until the next swap or it becomes invisible.
> This wont be needed, I think.
> 
> >   - on the GPU side, keep track of the state we sent to the browser as well
as
> > the corresponding ack we received. We may need to recreate the surface
before
> we
> > receive the ack, in which case we'll send a new AcceleratedSurfaceNew, which
> is
> > fine (provided proper state is kept on the UI side, i.e. that new surface is
> > conceptually different, even though it has the same surfce_handle). After
> > swapping to the new surface, we can send the ack to the old one.
> We won't destroy the surface before the ack, but once we start to not
deschedule
> the channel, the state of the world may be changing so we will need to track
> appropriate state.
> 
> > - about delaying the creating of the surface for the front buffer, that
> surface
> > shouldn't be taken into account anyway until the next swap, so you shouldn't
> > have to do that.
> Not sure what you mean here?  Is this because we only draw into backbuffer so
> there is no sense sending SurfaceNew message until a swap of a full frame?  If
> so, yes I agree.
Right. The only way the GPU process "creates" a front buffer is by swapping the
back into the front. As much as possible I want to avoid weird states where the
browser side has resources but those are not meaningful (like a texture right
after being reated / resized).

> 
> > - In any case, because the renderer context and the UI context are not
ordered
> > wrt one another, calling SetExternalTexture on the window doesn't have an
> effect
> > that is guaranteed to be visible in the GPU process until the next swap. You
> > need to take that into account when sending the ack to avoid releasing "too
> > early" (while the UI context is still finishing a frame that used that
> surface).
> Also not really sure what you mean here, perhaps you were under the impression
> that the browser was releasing the surface resources?  The browser is just
> releasing its handles, and the gpu process does not assume anything until it
> receives the ACK.
> Or, do you mean to say that SetExternalTexture sends a message down to the gpu
> process using the ui context and so the ACK message requesting a discard may
> come before the browser handle is actually released?  If so, this may possibly
> be a concern, but I will point out that we only go down this path if the
browser
> isn't using this surface, and any frame the ui is finishing will not be using
> this surface.. unless due to pipelining it's still finishing some frame from
the
> past, from a previous visibility state.. but that seems like an unlikely race
> condition esp. since frontbuffers are not dropped immediately.
> 
What I mean is the latter.
The browser must keep track of whether the front buffer it can drop is "in use"
by the compositor. Whenever the compositor draws a frame with the RWHVA's window
visible, it will send commands to the GPU process that use that texture. The IPC
coming from the RWHVA to the GPU process are not ordered WRT the drawing
commands. So what can happen is this:
1- compositor draws a frame with the RWHVA visible
2- event comes in, switches tab, makes RWHVA invisible, lets the renderer know
3- renderer tells the GPU process
4- GPU process hints the RWHVA it can drop the front buffer
5- RWHVA, being invisible, tells the GPU process to actually drop the front
buffer
6- GPU process drops the surface
7- compositor commands get handled, drawing with the deleted / resized texture.
Bang.

At step 5 we need to take into account that a frame is pending and only tell the
GPU process to drop the front buffer after the frame is finished.

Another race to take into account: when the RWHVA becomes visible again, there
is a race between the RHWVA telling the GPU process it should recreate the
surface, and the renderer compositor drawing. If the latter happens before, then
we need to make sure we correctly handle the swap (i.e. we don't drop it to the
floor or something). One way is to always reallocate the front buffer when
either the renderer tells it it's visible again OR the RHWVA tells it to
recreate the surface.

Lastly, on becoming visible again, we want to delay the UI compositor and give a
chance for the renderer to produce a frame to avoid whiteout. Of course we don't
want to wait forever. You can probably do something similar to what we do for
resize, and reuse the GetCompositorLock logic.

> > - to simplify things, you actually never have to tell the browser anything
> about
> > the back buffer - except when it becomes the front. So you can freely delete
> and
> > recreate the back texture without telling the browser
> Ok.

[mmocny, 2012-05-02 14:22:39]

And now my reply:

On 2012/05/02 14:13:11, mmocny wrote:
> Pasting Antoines reply from email:
> 
> On 2012/05/01 13:59:02, mmocny wrote:
> > On 2012/04/18 22:12:44, piman wrote:
> > > At a high level I have a couple of suggestions/comments:
> > > - I think you can avoid unscheduling the channel, just delay the actual
> > > destroying of the surface until you receive the ack.
> > I already delay destroy until the ACK (the destroy is done in the Ack
received
> > message handler of gpu process).  However, you are right, with a bit more
> state
> > I can avoid descheduling the channel.
> > 
> > > - with this patch, since the release can fail, it seems that we could get
> into
> > a
> > > situation where we don't release the surface (if switching tabs quickly).
> But
> > I
> > > think with enough state you can make it never fail:
> > This isn't really about failure, its about the-state-of-the-world changing
> such
> > that we actively want to ignore the request to release the surface.
> > For this reason, and since the ACK is really a request, I think maybe I
should
> > create a "suggestRelease*" (from gpu to browser) and "discard*" (from
browser
> to
> > gpu) messages.  This mirrors the current design for backbuffer and more
> > accurately represents the truth.
> > The gpu process knows when it would be a good time to discard the
frontbuffer
> > (based on the state of the world), but by the time the suggest message
> arrives,
> > that may have changed and so it is only a suggestion.  The browser knows if
> that
> > surface is in use upon message arrival (i.e. rapid tab switching).  If it
> isn't,
> > browser will clear its handles and only then send a discard request which
the
> > gpu process must process.
> > 
> > >   - on the UI side, instead of sending back an ack with failure, don't
send
> > the
> > > ack yet, until the next swap or it becomes invisible.
> > This wont be needed, I think.
> > 
> > >   - on the GPU side, keep track of the state we sent to the browser as
well
> as
> > > the corresponding ack we received. We may need to recreate the surface
> before
> > we
> > > receive the ack, in which case we'll send a new AcceleratedSurfaceNew,
which
> > is
> > > fine (provided proper state is kept on the UI side, i.e. that new surface
is
> > > conceptually different, even though it has the same surfce_handle). After
> > > swapping to the new surface, we can send the ack to the old one.
> > We won't destroy the surface before the ack, but once we start to not
> deschedule
> > the channel, the state of the world may be changing so we will need to track
> > appropriate state.
> > 
> > > - about delaying the creating of the surface for the front buffer, that
> > surface
> > > shouldn't be taken into account anyway until the next swap, so you
shouldn't
> > > have to do that.
> > Not sure what you mean here?  Is this because we only draw into backbuffer
so
> > there is no sense sending SurfaceNew message until a swap of a full frame? 
If
> > so, yes I agree.
> Right. The only way the GPU process "creates" a front buffer is by swapping
the
> back into the front. As much as possible I want to avoid weird states where
the
> browser side has resources but those are not meaningful (like a texture right
> after being reated / resized).
> 
> > 
> > > - In any case, because the renderer context and the UI context are not
> ordered
> > > wrt one another, calling SetExternalTexture on the window doesn't have an
> > effect
> > > that is guaranteed to be visible in the GPU process until the next swap.
You
> > > need to take that into account when sending the ack to avoid releasing
"too
> > > early" (while the UI context is still finishing a frame that used that
> > surface).
> > Also not really sure what you mean here, perhaps you were under the
impression
> > that the browser was releasing the surface resources?  The browser is just
> > releasing its handles, and the gpu process does not assume anything until it
> > receives the ACK.
> > Or, do you mean to say that SetExternalTexture sends a message down to the
gpu
> > process using the ui context and so the ACK message requesting a discard may
> > come before the browser handle is actually released?  If so, this may
possibly
> > be a concern, but I will point out that we only go down this path if the
> browser
> > isn't using this surface, and any frame the ui is finishing will not be
using
> > this surface.. unless due to pipelining it's still finishing some frame from
> the
> > past, from a previous visibility state.. but that seems like an unlikely
race
> > condition esp. since frontbuffers are not dropped immediately.
> > 
> What I mean is the latter.
> The browser must keep track of whether the front buffer it can drop is "in
use"
> by the compositor. Whenever the compositor draws a frame with the RWHVA's
window
> visible, it will send commands to the GPU process that use that texture. The
IPC
> coming from the RWHVA to the GPU process are not ordered WRT the drawing
> commands. So what can happen is this:
> 1- compositor draws a frame with the RWHVA visible
> 2- event comes in, switches tab, makes RWHVA invisible, lets the renderer know
> 3- renderer tells the GPU process
> 4- GPU process hints the RWHVA it can drop the front buffer
> 5- RWHVA, being invisible, tells the GPU process to actually drop the front
> buffer
> 6- GPU process drops the surface
> 7- compositor commands get handled, drawing with the deleted / resized
texture.
> Bang.
> 
> At step 5 we need to take into account that a frame is pending and only tell
the
> GPU process to drop the front buffer after the frame is finished.
Is it sufficient to flush the ui context?

> 
> Another race to take into account: when the RWHVA becomes visible again, there
> is a race between the RHWVA telling the GPU process it should recreate the
> surface, and the renderer compositor drawing. If the latter happens before,
then

I don't think there is.
When RWHVA becomes visible again, we dont tell the gpu process to recreate the
surface.  The renderer calls ensureFramebuffer which will recreate the
backbuffer if it was dropped, and the gpu process "creates" a frontbuffer during
the next swap, just as you mentioned above.
Since we drop the handle to the surface before asking gpu process to discard it,
when RWHVA becomes visible again, it won't try to draw that surface.
Please let me know if I missed something.

> we need to make sure we correctly handle the swap (i.e. we don't drop it to
the
> floor or something). One way is to always reallocate the front buffer when
> either the renderer tells it it's visible again OR the RHWVA tells it to
> recreate the surface.
> 
> Lastly, on becoming visible again, we want to delay the UI compositor and give
a
> chance for the renderer to produce a frame to avoid whiteout. Of course we
don't
> want to wait forever. You can probably do something similar to what we do for
> resize, and reuse the GetCompositorLock logic.

There is a separate bug already filed for smooth tab transitioning so that we
wait for the next frame to be available (or some timeout) before showing an
empty tab contents.  John bates was working on it and saw he was making some
good progress on at least mac/windows, I will touch base with him soon to see
how much of the windows code can be reused for chromeos.

[mmocny, 2012-05-02 21:56:25]

Updated patch to address some of the concerns raised.

Should work:
- No longer descheduling channel
- Waiting for ui composite cycle to finish before sending ack

Big Things missing:
- Smooth tab transitions (wait for frame or timeout before next composite)

Easy Things missing:
- Remove ui_shim usage from RWHVA
- Rename messages to reflect suggestion nature
- Dont inform ui of backbuffer release, only frontbuffer
- Update other platform's Xport surfaces

May be other things left to fix, I'll add them to this list as I remember them.
I am taking an alex home with me overnight to make sure this works and ill try
to do some memory benchmarking on it, too.

[mmocny, 2012-05-02 21:58:26]

On 2012/05/02 21:56:25, mmocny wrote:
> Updated patch to address some of the concerns raised.
> 
> Should work:
> - No longer descheduling channel
> - Waiting for ui composite cycle to finish before sending ack
> 
> Big Things missing:
> - Smooth tab transitions (wait for frame or timeout before next composite)
> 
> Easy Things missing:
> - Remove ui_shim usage from RWHVA
> - Rename messages to reflect suggestion nature
> - Dont inform ui of backbuffer release, only frontbuffer
> - Update other platform's Xport surfaces
> 
> May be other things left to fix, I'll add them to this list as I remember
them.
> I am taking an alex home with me overnight to make sure this works and ill try
> to do some memory benchmarking on it, too.

One more little thing:
- I am requesting a schedule composite, whereas I should be checking if one is
already scheduled and setting callback conditionally (since we can send ack
right away if there is no composite pending).

[mmocny, 2012-05-03 21:45:25]

Okay.  I've fixed all the straightforward stuff that was missing.

The smooth tab transitioning issue is not really about correctness, and since we
have a separate bug for it, I was hoping to include in a subsequent patch (this
one is already a beast).

I am currently seeing an issue where after tab switch (any tab switch) I see a
good frontbuffer, and then a white tab contents swaps in.  If the page continues
to draw it looks like a white flash (we had this bug recently), but if it
doesn't continue to draw, it appears as if the page hangs.  I've verified that
the state of all the buffers is correct when this happens (it happens even with
two tabs, no fb drop, and the bb drop path is now the same as it always was).  I
suspect its  an issue in trunk, going to verify.

I will scan this CL in a little bit and sprinkle some good comments, but I would
appreciate another review since I think it is approaching ready.  Thanks.

[piman, 2012-05-04 00:03:48]

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

http://codereview.chromium.org/10052018/diff/20002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:666: return;
The problem here is that it's racy. We may miss an opportunity to drop the front
buffer when switching tabs rapidly.

http://codereview.chromium.org/10052018/diff/20002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:670:
on_compositing_ended_callbacks_.push_back(
on_compositing_ended_callbacks_ may not be used unless we've added ourselves as
a compositor observer. Right now only do that on Swap/Post and in some resize
cases.
You probably want to change that so that we're always an observer while we're
visible.
The other thing is, you only want to delay the sending if the surface is
actually in use (i.e. between OnCompositingStarted and OnCompositingEnded).

http://codereview.chromium.org/10052018/diff/20002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:671:
base::Bind(&RenderWidgetHostImpl::SendSurfaceDiscard,
By the time that callback is called, we may be visible again, in which case you
shouldn't drop the surface. Better call a method on this object, and only then
send the message if we're still invisible.

http://codereview.chromium.org/10052018/diff/20002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:675:
host_->ScheduleComposite();
This shouldn't be needed. If we don't have a frame pending, we don't want to
kick one just to know when it's done so that we can send the message. See above,
if you take OnCompositingStarted into account, you don't need this.

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

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:160: if
(static_cast<bool>(textures_[back()].info->service_id()) == allocation &&
nit: use !! (or != 0) instead of static_cast<bool>

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:175: if
(static_cast<bool>(textures_[front()].info->service_id()) ==
nit: use !! (or != 0) instead of static_cast<bool>

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:179: return;
should we set textures_[front()].suggested_allocation=allocation here?

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:333: uint64 surface_id) {
Why do we need the id here? This is called by the UI. The UI only cares about
the front buffer.
When the UI calls this, it should be in a state where it will not use any of the
buffers. At that point, we could release whichever is the front (unless the
allocation has changed since then).

TBH I'm having a very hard time following the logic in this file regarding
allocation, especially front buffer.
SetFrontbufferAllocation, OnDiscardSurface and OnBuffersSwappedACK all can
happen in virtually arbitrary time/order relative to the rendering commands that
also affect allocation (mainly SwapBuffers/PostSubBuffers), so the state matrix
can be very big if not done properly, so it's super hard to reason about. Many
of the states are transient, but they exist and must be handled. Right now
there's few comments and no test.

The way I like to reason about it is this way: we have 2 reasons to have a front
buffer. We need to have a front buffer if either is true:
1- the browser says it needs a front buffer.
2- the renderer says it's visible (which I guess comes through
SetFrontbufferAllocation).

In steady state, 1 and 2 are the same, but transitions occur and during that
time they are not. You want to actually drop the front buffer when both of them
become false. While they are both false, any swap/post can be dropped to the
floor. When either one is true, the next swap/post will have regenerated the new
front buffer (that we can then send to the browser).

Right now, (1) is only one-sided (OnDiscardSurface), so we sort-of rely on the
fact that the renderer will create a new frame when the browser becomes visible
again. But it's not an explicit state, so it's hard to reason about. In
particular I don't know how to verify that when switching tabs rapidly, the
OnDiscardSurface won't come before a new frame that got kicked but is not
relevant any more (in which case we'll keep the front buffer, even though we
should really discard it).

I'd much rather see something like:
- a message from the browser indicating whether it wants a surface or not (i.e.
add a bool to the Discard message or something)
- 2 bools here: front_buffer_suggested_allocation, browser_needs_front_buffer
- in OnDiscardSurface (or whatever we rename it to) we set
browser_needs_front_buffer
- in SetFrontbufferAllocation we set front_buffer_suggested_allocation
- in both of them, we handle the transition if (browser_needs_front_buffer ||
front_buffer_suggested_allocation) changes. If it goes from true to false, we
drop the front buffer. If it goes from from false to true, there's nothing to do
yet until the next Swap/Post
- in Swap or Post, if (browser_needs_front_buffer ||
front_buffer_suggested_allocation) is false, we do nothing (i.e. we keep the
back buffer, the front buffer should already have been dropped). If it's true,
we send the Swap/Post to the browser, with the new id.

That's pretty much it (for the front buffer). The buffer we allocate is always
the back one (on Resize or on the Swap ACK), and that's already handled.
Whatever the order between OnDiscardSurface/SetFrontbufferAllocation, we can
verify we do the right thing.

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:400: int id, const
gfx::Size& size) {
AFAICT you only call CreateTexture(back(), ...), which makes sense actually. Can
we leave it as CreateBackTexture to avoid improper usage?

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:420: // release.  We do
this by resettign suggested_allocation and sending an
typo: resettign->resetting

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:470: void
TextureImageTransportSurface::AttachTextureToFBO(int id) {
Having an id here smells fishy to me. Why would we attach anything but the back
buffer to the FBO?
AFAICT This is only called through AttachTextureToFBO(back()) or CreateTexture
which is only called for back() (see above).

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

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.h:72: bool
suggested_allocation;
I don't think suggested_allocation belongs here. Or it doesn't mean what I think
it means

When we call SetBackbufferAllocation(false) we mean, we don't need a back
buffer. It doesn't matter whichever texture that is currently, but if they
change (e.g. because of a swap), it doesn't make sense to move the suggested
allocation from back to front.

It'd be better I think to have a suggested_back_allocation_ and a
suggested_front_allocation_ at the top level.

[mmocny, 2012-05-04 18:55:52]

I'm about to put up a patch that cleans up a bunch of the issues you raised, but
posting my comment replies because I had some questions too

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

http://codereview.chromium.org/10052018/diff/20002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:666: return;
On 2012/05/04 00:03:49, piman wrote:
> The problem here is that it's racy. We may miss an opportunity to drop the
front
> buffer when switching tabs rapidly.

Very sorry, but I am still not understanding why there would be a missed
opportunity.
We will only request a drop if not visible, so if visible here, it must be a
stale request.

Also, stepping back a bit from the specifics here, the overall notion of missing
some opportunity to listen to a suggestion from the GpuMemoryManager is not
generally a problem because it will continue to "Manage" and continue to send
you suggestions if you aren't in the state it expects.  Obviously we want to be
correct and efficient, but if there is a temporary racy window here, it is
better to be cautious and wait for the next suggestion to come in.

http://codereview.chromium.org/10052018/diff/20002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:670:
on_compositing_ended_callbacks_.push_back(
Once I stop scheduling a composite, Yes, I'll need to add as an observer. Thanks
for pointing that out.

On 2012/05/04 00:03:49, piman wrote:
> on_compositing_ended_callbacks_ may not be used unless we've added ourselves
as
> a compositor observer. Right now only do that on Swap/Post and in some resize
> cases.
> You probably want to change that so that we're always an observer while we're
> visible.
> The other thing is, you only want to delay the sending if the surface is
> actually in use (i.e. between OnCompositingStarted and OnCompositingEnded).

http://codereview.chromium.org/10052018/diff/20002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:671:
base::Bind(&RenderWidgetHostImpl::SendSurfaceDiscard,
Correct, thanks!
On 2012/05/04 00:03:49, piman wrote:
> By the time that callback is called, we may be visible again, in which case
you
> shouldn't drop the surface. Better call a method on this object, and only then
> send the message if we're still invisible.

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

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:160: if
(static_cast<bool>(textures_[back()].info->service_id()) == allocation &&
Thanks.

On 2012/05/04 00:03:49, piman wrote:
> nit: use !! (or != 0) instead of static_cast<bool>

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:179: return;
No -- hopefully my previous explanation of what this means helps clear things
up.  Upcoming patch should be a lot nicer.

On 2012/05/04 00:03:49, piman wrote:
> should we set textures_[front()].suggested_allocation=allocation here?

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:333: uint64 surface_id) {
On 2012/05/04 00:03:49, piman wrote:
> Why do we need the id here? This is called by the UI. The UI only cares about
> the front buffer.
Originally, this was here to protect from releasing the wrong surface after a
swap.

After some cleanup and better control of state, this is no longer needed, and
I've removed the id.

> When the UI calls this, it should be in a state where it will not use any of
the
> buffers. At that point, we could release whichever is the front (unless the
> allocation has changed since then).
Yes, exactly, I now make sure the allocation hasn't changed.

> 
> TBH I'm having a very hard time following the logic in this file regarding
> allocation, especially front buffer.
> SetFrontbufferAllocation, OnDiscardSurface and OnBuffersSwappedACK all can
> happen in virtually arbitrary time/order relative to the rendering commands
that
> also affect allocation (mainly SwapBuffers/PostSubBuffers), so the state
matrix
> can be very big if not done properly, so it's super hard to reason about. Many
> of the states are transient, but they exist and must be handled. Right now
> there's few comments and no test.
Sorry, you are correct, this is not very easy to follow or reason about.

I'm working to clean this up.

> 
> The way I like to reason about it is this way: we have 2 reasons to have a
front
> buffer. We need to have a front buffer if either is true:
> 1- the browser says it needs a front buffer.
> 2- the renderer says it's visible (which I guess comes through
> SetFrontbufferAllocation).
I don't understand how being explit about (1) could be useful?  I thought that
the ui does this implicitly by setting via visibility, and waiting for (2) to
happen.  visibility == true is equivalent to browser needs front buffer -- is it
possible this will change?

If the problem is that going through the renderer is too slow/not trustworthy to
relay visibility information, there is already an outstanding issue to have
browser/gpu communicate it directly (its good for other reasons as well).

> 
> In steady state, 1 and 2 are the same, but transitions occur and during that
> time they are not. You want to actually drop the front buffer when both of
them
> become false. While they are both false, any swap/post can be dropped to the
> floor. When either one is true, the next swap/post will have regenerated the
new
> front buffer (that we can then send to the browser).
If I understand correct, you agree that conceptually they both change at the
same time, and the only reason they get "out of sync" is because we have to
communicate the messages.

I think adding a new message for (1) will be equivalent to sending visibility to
gpu process directly (which will set FrontbufferAllocation and thus protect the
frontbuffer).

> 
> Right now, (1) is only one-sided (OnDiscardSurface), so we sort-of rely on the
> fact that the renderer will create a new frame when the browser becomes
visible
> again. But it's not an explicit state, so it's hard to reason about. In
Yes, we do rely on this.  This is not related to frontbuffer protection
correctness.  Even if that is 100% clear and correct we still rely on renderer
pushing a frame when switching back to the tab.  I think this is currently
guaranteed, but its not explicitly requested by the ui.

> particular I don't know how to verify that when switching tabs rapidly, the
> OnDiscardSurface won't come before a new frame that got kicked but is not
> relevant any more (in which case we'll keep the front buffer, even though we
> should really discard it).
If this is possible, that frontbuffer will still be cleared with the next Manage
call, so long as  global state does not get out of sync, memory will eventually
be reclaimed.
I will reason about this a bit longer to make sure.

> 
> I'd much rather see something like:
> - a message from the browser indicating whether it wants a surface or not
(i.e.
> add a bool to the Discard message or something)
> - 2 bools here: front_buffer_suggested_allocation, browser_needs_front_buffer
> - in OnDiscardSurface (or whatever we rename it to) we set
> browser_needs_front_buffer
> - in SetFrontbufferAllocation we set front_buffer_suggested_allocation
> - in both of them, we handle the transition if (browser_needs_front_buffer ||
> front_buffer_suggested_allocation) changes. If it goes from true to false, we
> drop the front buffer. If it goes from from false to true, there's nothing to
do
> yet until the next Swap/Post
> - in Swap or Post, if (browser_needs_front_buffer ||
> front_buffer_suggested_allocation) is false, we do nothing (i.e. we keep the
> back buffer, the front buffer should already have been dropped). If it's true,
> we send the Swap/Post to the browser, with the new id.
Sending visibility directly to gpu would solve the same problem, if (visible ==
browser_needs_front_buffer).

> 
> That's pretty much it (for the front buffer). The buffer we allocate is always
> the back one (on Resize or on the Swap ACK), and that's already handled.
> Whatever the order between OnDiscardSurface/SetFrontbufferAllocation, we can
> verify we do the right thing.

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:400: int id, const
gfx::Size& size) {
done.
On 2012/05/04 00:03:49, piman wrote:
> AFAICT you only call CreateTexture(back(), ...), which makes sense actually.
Can
> we leave it as CreateBackTexture to avoid improper usage?

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:420: // release.  We do
this by resettign suggested_allocation and sending an
done.
On 2012/05/04 00:03:49, piman wrote:
> typo: resettign->resetting

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:470: void
TextureImageTransportSurface::AttachTextureToFBO(int id) {
Yes, this was just a result of CreateTexture taking an id, and can change back
now.
On 2012/05/04 00:03:49, piman wrote:
> Having an id here smells fishy to me. Why would we attach anything but the
back
> buffer to the FBO?
> AFAICT This is only called through AttachTextureToFBO(back()) or CreateTexture
> which is only called for back() (see above).

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

http://codereview.chromium.org/10052018/diff/20002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.h:72: bool
suggested_allocation;
Suggested allocation has evolved with this patch and has become a misnomer, so I
will rename it.

It now means "did send suggest discard frontbuffer message, and that request is
still valid".

I've moved this out into top level and overall cleaned things up significantly.

On 2012/05/04 00:03:49, piman wrote:
> I don't think suggested_allocation belongs here. Or it doesn't mean what I
think
> it means
> 
> When we call SetBackbufferAllocation(false) we mean, we don't need a back
> buffer. It doesn't matter whichever texture that is currently, but if they
> change (e.g. because of a swap), it doesn't make sense to move the suggested
> allocation from back to front.
> 
> It'd be better I think to have a suggested_back_allocation_ and a
> suggested_front_allocation_ at the top level.

[mmocny, 2012-05-04 20:23:02]

Alright, new patch up which hopefully is a bit more clear.

Still working on:
- Only wait for composite if currently in the middle of one

[mmocny, 2012-05-04 20:24:31]

Also note that I have not yet added explicit frontbuffer protection via any ui
message.  Now that things are clearer maybe we can better reason about if it is
necessary.

[mmocny, 2012-05-11 19:49:29]

Just a quick update on this patch:

- Working on landing https://chromiumcodereview.appspot.com/10388010/ which
fixes a beta blocker and is a subset of this patch (related to buffer suggestion
management).
- Adding "frontbuffer protected" messages from browser to gpu process, as
suggested.
- Also adding "discard request ids" to protect from some ABA issues (state is as
we expect, but reply is actually from a request in the past -- I ran some tests
and found this was actually happening).
- Adding state to texture image transport surface so its very clear what the
assumptions about current state are, and so its easier to test those
assumptions.

Patch will probably not be up today, but hope to have something to show on
Monday.

[mmocny, 2012-05-23 16:47:50]

Just another update since its been a while.  Sadly, I am still hunting down the
cause of a black flash I see during rapid tab switching.


Addressing some issues raised before:

* RE: handling the fast 1-hide/2-show/3-hide on the UI side:
* * I still assume visibility messages will be sent from browser to renderer to
gpu process on each transition.  I am open to suggestions on how to
guarantee/add tests for this, but I think there is a lot of code that assumes
this to be true.  However, it seems likely that visibility plumbing will change
soon anyway (for atomic tab switches and for visibility+allocation message
pairs).
* * I did, however, change it so that the texture image transport surface does
not suppress sending a discard request to the ui even if there is already an
outstanding request.  This way, if the first request is ignored (i.e., because
it was received during the 2-show step), the subsequent request will still be
received (1 request for each hide).  I also handle invalidated/stale/needless
requests in the ui.

* RE: UI send a discard message, while the GPU got a frame and sends the swap
message.
* * We now have discard request id's which are passed around and get invalidated
any time state changes (visibility, swaps, etc).  These are used to invalidate
requests, and also solved ABA issues which did happen in practice.

* RE: more state in both of the classes, so it's a lot easier to reason about.
* * Added all the state you suggested, plus the discard request id's, and
ui_may_not_have_frontbuffer_handle_ (since after sending a request, we are not
sure if/when the ui surface handle will be dropped, and the next swap must
assume it has been).
* * Because of the IPC timing, browser-visibility and frontbuffer protection
will get out of synch and cannot be relied upon. Because of this, the
implementation has to be correct no matter what the values are, and so the flag
isn't (cannot be) necessary for correctness.  On the one hand, it occasionally
saves some needless RequestDiscard IPC and may possibly ease debugging, but on
the other hand I had to cache route_id and gpu_host_id in RWHVA (unless there is
another way?).  I consider the flag merely a diagnostic tool and don't consider
the implementation cost worthwhile, but I've left it in for now so we can
discuss.


Remaining TODO:

* Must prevent discarding fb while thumbnailing
* * http://codereview.chromium.org/10349013/ is going to land soon which
prevents starting a thumbnail when we don't have a surface handle.  For aura,
since the readback is asynchronous, I am going to have to delay fb discard until
the thumbnail step is complete.  I plan to do this by observing the
ThumbnailGenerator::AsyncUpdateThumbnailFinish method whenever there is an
outstanding thumbnail task for the requested surface_id.

* Renaming:
* * OnDiscardFrontbuffer is quite a confusing method name on the other image
transport surfaces implementations which have noops but still discard fb in the
SetFrontbufferAllocation step.
* * Also, is it alright to add the no-op implementation to the
ImageTransportSurface base class?  There is already an implementation for
GetRegionsToCopy, though its a protected helper (could have been a non-member)
and not part of the public API.

* Smooth tab switching:
* * I think this should be a subsequent CL.
* * FYI, except for the rapid tab switch case, the flash is very short duration,
even on the alex on a heavy page.

[mmocny, 2012-05-29 15:45:39]

I think I have addressed various (hopefully all) concerns raised so far.

The feature is behind the flag --enable-ui-release-front-surface.

I have tried to add at least a few unit tests to test the major pieces in
isolation (RWHVA and Texture image transport surface -- not going to acronym
that one ;) but they seem to not be written for testability.  Perhaps I should
byte the bullet and make them so.


Aside from the obvious I would appreciate specific attention to my caching of
route_id and gpu_host_id in order to send IsProtected messages later, as well as
to my use of weakptrs and if they are invalidated correctly -- both in RWHVA.

[mmocny, 2012-06-01 21:06:51]

http://codereview.chromium.org/10052018/diff/57011/content/common/gpu/gpu_mem...
File content/common/gpu/gpu_memory_manager.h (right):

http://codereview.chromium.org/10052018/diff/57011/content/common/gpu/gpu_mem...
content/common/gpu/gpu_memory_manager.h:29: enum {
kDefaultMaxSurfacesWithFrontbufferSoftLimit = 3 };
This was checked in accidentally.  I'll fix it in next patch upload.

http://codereview.chromium.org/10052018/diff/57011/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface.h (right):

http://codereview.chromium.org/10052018/diff/57011/content/common/gpu/image_t...
content/common/gpu/image_transport_surface.h:74: bool was_released) { };
Is this default implementation alright here?  Its currently inconsistent with
the IsProtected implementation (which is also a noop on the other platforms), so
I guess I should change one or the other.

[mmocny, 2012-06-05 14:52:35]

http://codereview.chromium.org/10052018/diff/57011/content/common/gpu/gpu_mem...
File content/common/gpu/gpu_memory_manager.h (right):

http://codereview.chromium.org/10052018/diff/57011/content/common/gpu/gpu_mem...
content/common/gpu/gpu_memory_manager.h:29: enum {
kDefaultMaxSurfacesWithFrontbufferSoftLimit = 3 };
On 2012/06/01 21:06:51, mmocny wrote:
> This was checked in accidentally.  I'll fix it in next patch upload.

Done.

http://codereview.chromium.org/10052018/diff/57011/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface.h (right):

http://codereview.chromium.org/10052018/diff/57011/content/common/gpu/image_t...
content/common/gpu/image_transport_surface.h:74: bool was_released) { };
On 2012/06/01 21:06:51, mmocny wrote:
> Is this default implementation alright here?  Its currently inconsistent with
> the IsProtected implementation (which is also a noop on the other platforms),
so
> I guess I should change one or the other.

Done.

[mmocny, 2012-06-11 20:19:47]

Antoine, the most recent patch here is a major simplification thanks to what we
discussed last time.

Overview:
- RWHVA resets current_surface_ to 0 every time it is backgrounded.  It then
sends a IsProtected(false) message to the gpu process *after* it is done current
composite and thumbnail.  Protection against ABA visibility included.
- RWHVA does not actually remove surface from its image_transport_clients_ map
at this point.
- When TextureITS gets an IsProtected(false) message, it knows ui will not use
*either* buffer and so can later release whichever is the frontbuffer once told
to do so.
- TextureITS will send SurfaceRelease message once surface is actually released
and RWHVA can then clean up its mapping.
- TextureITS will send a swap message without actually doing a swap when it gets
IsProtected(true) so that ui can draw without waiting for a new frame to be
generated


Thats about it from the high level.  TextureITS has to be careful about when it
does things and there are a lot of DCHECKS to state assumptions.


Things left to do:
- ui does not differentiate between swap with real new frame and swap just to
reset handle to old one.  We will likely want a new message type so it can
differentiate in order to: (a) not deschedule channel, (b) not require ack, (c)
(maybe) ui use compositor lock to delay until fresh frame.
- gpu_host_id is being cached but may not need to be
- IsProtected message is basically IsVisible message, which we have wanted for
GpuMemoryManager for a long time.  Probably won't add to this patch, but as long
as the delayed IsVisible(false) is acceptable I think I would like to make some
small changes and remove renderer->gpu visibility message.
- This patch include compositor lock work but that is landing separately.
- This patch sends a SwapBuffers when IsProtected(true) but we exclusively use
PostSubBuffer otherwise.  I've heard it is supposed to be safe to interchange
them, but perhaps I should err on the side of caution here?

[piman, 2012-06-11 21:32:24]

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:219:
weak_factory_(this),
ALLOW_THIS_IN_INITIALIZER_LIST

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:244:
weak_factory_.InvalidateWeakPtrs();
Do you need this? weak_factory_ will invalidate the WeakPtrs when it gets
destroyed.
You can actually make this class derive from SupportsWeakPtr instead, which is
equivalent.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:300:
switches::kEnableUIReleaseFrontSurface)) {
Rather than parsing the command line every time this is called, better to store
this as a bool.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:510:
DCHECK(!is_thumbnailing_);
Please check with mazda that this is ensured. That code is pretty far from here,
and baking this assumption may not be safe against refactoring there.
Probably better is to fail if one is already in progress (by calling the
callback with failure).

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:600: // If we are
not visible then this swap is stale. We must still ACK but do not
I think there's still ABA issues.
Say at some point, at around the same time:
- gpu sends swap
- tab got invisible and all other conditions were good, so it sends
IsProtected(false)

A fraction later, the tab got visible again before the swap was received. When
the swap is received, the browser will start using it, and send a
IsProtected(true), but it may be too late and the GPU process may delete the
buffer.

I'm pretty sure you need a state id for the IsProtected message (increasing
every time it is sent) and attach that to the swap/post so that you can verify
that the swap isn't stale (hence the buffer at risk of being destroyed).

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:788:
on_thumbnailing_ended_callback_ =
Do you need to make this a callback? You could just set a bool state and call
SendSurfaceIsProtectedFalseAfterThumbnailerFinished from
CopyFromCompositingSurfaceFinished if it is set.

Similarly to what we do in TextureImageTransportSurface, maybe you could make
that logic as:
- IsProtected is a virtual state of (!visible && !surface_in_use_by_thumbnailer
&& !surface_in_use_by_compositor)
- tracking individual states separately
  * visible through DidBecomeSelected/WasHidden,
  * surface_in_use_by_thumbnailer through
CopyFromCompositingSurface/CopyFromCompositingSurfaceFinished,
  * surface_in_use_by_compositor through the CompositorObserver callbacks
- send the message any time IsProtected() changes (e.g. have a common function
that checks the current state vs the last sent state and send a message when
they're different - and call that function every time any of the above changes).

I think that way it'd be much clearer about the logic (tracing callbacks is
hard).

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:1342:
is_thumbnailing_ = 0;
The thumbnailing callback should be called either way (possibly with failure),
so there shouldn't be a need to do this.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/common/gpu...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/common/gpu...
content/common/gpu/texture_image_transport_surface.cc:376: // TODO: safe to send
swapbuffers when using postsubbuffers usually?
Yes, it's always safe. A Swap is essentially a Post will full damage.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/common/gpu...
content/common/gpu/texture_image_transport_surface.cc:387: if
(textures_[front()].size != textures_[back()].size ||
We should be very careful here.

When restoring the tab, it's possible that the tab has changed size in the mean
time.

On one hand, the browser will send a IsProtected(true), upon which the GPU
process will send a Swap, and that will come back here.
On the other hand, the renderer may have started a new frame, with the new size,
i.e. it will have resized the back buffer.

Depending on timing, when we get here, it's possible that
textures_[front()].size (the old size) != textures_[back()].size (the new size).
However, at this point we do not want to recreate the back texture (it would
resize it to the wrong size, and lose all the rendering that has been done up to
now).

Maybe the best is to keep a flag or something in OnSetFrontSurfaceIsProtected,
so that we tag the Swap as "special", so that we treat is specially here. That
can even help you avoid unscheduling. Just beware of multiple IsProtected in ABA
cases (i.e that flag maybe should be a counter, but special care may need to be
taken if the "real" swap is coming from the renderer before we got an ack from
the "fake" swap).

[mmocny, 2012-06-11 21:45:09]

Thanks for review.  All of your points are addressable so I am happy to know
this is trending in the right direction.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:600: // If we are
not visible then this swap is stale. We must still ACK but do not
Keen observation!  I will add this.

On 2012/06/11 21:32:24, piman wrote:
> I think there's still ABA issues.
> Say at some point, at around the same time:
> - gpu sends swap
> - tab got invisible and all other conditions were good, so it sends
> IsProtected(false)
> 
> A fraction later, the tab got visible again before the swap was received. When
> the swap is received, the browser will start using it, and send a
> IsProtected(true), but it may be too late and the GPU process may delete the
> buffer.
> 
> I'm pretty sure you need a state id for the IsProtected message (increasing
> every time it is sent) and attach that to the swap/post so that you can verify
> that the swap isn't stale (hence the buffer at risk of being destroyed).

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:788:
on_thumbnailing_ended_callback_ =
Sounds great, will do.

On 2012/06/11 21:32:24, piman wrote:
> Do you need to make this a callback? You could just set a bool state and call
> SendSurfaceIsProtectedFalseAfterThumbnailerFinished from
> CopyFromCompositingSurfaceFinished if it is set.
> 
> Similarly to what we do in TextureImageTransportSurface, maybe you could make
> that logic as:
> - IsProtected is a virtual state of (!visible &&
!surface_in_use_by_thumbnailer
> && !surface_in_use_by_compositor)
> - tracking individual states separately
>   * visible through DidBecomeSelected/WasHidden,
>   * surface_in_use_by_thumbnailer through
> CopyFromCompositingSurface/CopyFromCompositingSurfaceFinished,
>   * surface_in_use_by_compositor through the CompositorObserver callbacks
> - send the message any time IsProtected() changes (e.g. have a common function
> that checks the current state vs the last sent state and send a message when
> they're different - and call that function every time any of the above
changes).
> 
> I think that way it'd be much clearer about the logic (tracing callbacks is
> hard).

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/common/gpu...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/common/gpu...
content/common/gpu/texture_image_transport_surface.cc:387: if
(textures_[front()].size != textures_[back()].size ||
Okay, this sounds like it could be potentially hairy.
I'll get something in with counters like you suggest and can evaluate the
concerns once things are cleaned up.

On 2012/06/11 21:32:24, piman wrote:
> We should be very careful here.
> 
> When restoring the tab, it's possible that the tab has changed size in the
mean
> time.
> 
> On one hand, the browser will send a IsProtected(true), upon which the GPU
> process will send a Swap, and that will come back here.
> On the other hand, the renderer may have started a new frame, with the new
size,
> i.e. it will have resized the back buffer.
> 
> Depending on timing, when we get here, it's possible that
> textures_[front()].size (the old size) != textures_[back()].size (the new
size).
> However, at this point we do not want to recreate the back texture (it would
> resize it to the wrong size, and lose all the rendering that has been done up
to
> now).
> 
> Maybe the best is to keep a flag or something in OnSetFrontSurfaceIsProtected,
> so that we tag the Swap as "special", so that we treat is specially here. That
> can even help you avoid unscheduling. Just beware of multiple IsProtected in
ABA
> cases (i.e that flag maybe should be a counter, but special care may need to
be
> taken if the "real" swap is coming from the renderer before we got an ack from
> the "fake" swap).

[mmocny, 2012-06-12 18:26:17]

Alright, fixed up all the previous concerns.  Tested quite a bit on desktop and
seems pretty sweet.  Going to test on device now.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:219:
weak_factory_(this),
Not needed once using SupportsWeakPtr.

On 2012/06/11 21:32:24, piman wrote:
> ALLOW_THIS_IN_INITIALIZER_LIST

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:244:
weak_factory_.InvalidateWeakPtrs();
On 2012/06/11 21:32:24, piman wrote:
> Do you need this? weak_factory_ will invalidate the WeakPtrs when it gets
> destroyed.
> You can actually make this class derive from SupportsWeakPtr instead, which is
> equivalent.

Done.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:300:
switches::kEnableUIReleaseFrontSurface)) {
Done, using static bool inside anon namespace helper so as not to bloat RWHVA
(modeled after ShouldSendPinchGesture).
I know there are issues with static initializers and destruction handlers, but I
suspect static bool is fine?

On 2012/06/11 21:32:24, piman wrote:
> Rather than parsing the command line every time this is called, better to
store
> this as a bool.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:510:
DCHECK(!is_thumbnailing_);
Had a talk with @mazda.  It is not correct to ignore, or callback(false) because
the thumbnailer already handles multiple outstanding thumbnail requests by
ignoring all but the last one.  So, if we fail request B while request A is
pending, the thumbnailer code will ignore request A even when its the only
success one.

Instead, I just track the number of pending thumbnail requests and don't
AdjustSurfaceAllocation until there is 0 pending, and let all of them do what
they always did.

On 2012/06/11 21:32:24, piman wrote:
> Please check with mazda that this is ensured. That code is pretty far from
here,
> and baking this assumption may not be safe against refactoring there.
> Probably better is to fail if one is already in progress (by calling the
> callback with failure).

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:600: // If we are
not visible then this swap is stale. We must still ACK but do not
On 2012/06/11 21:45:09, mmocny wrote:
> Keen observation!  I will add this.
> 
> On 2012/06/11 21:32:24, piman wrote:
> > I think there's still ABA issues.
> > Say at some point, at around the same time:
> > - gpu sends swap
> > - tab got invisible and all other conditions were good, so it sends
> > IsProtected(false)
> > 
> > A fraction later, the tab got visible again before the swap was received.
When
> > the swap is received, the browser will start using it, and send a
> > IsProtected(true), but it may be too late and the GPU process may delete the
> > buffer.
> > 
> > I'm pretty sure you need a state id for the IsProtected message (increasing
> > every time it is sent) and attach that to the swap/post so that you can
verify
> > that the swap isn't stale (hence the buffer at risk of being destroyed).
> 

Done.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:788:
on_thumbnailing_ended_callback_ =
On 2012/06/11 21:45:09, mmocny wrote:
> Sounds great, will do.
> 
> On 2012/06/11 21:32:24, piman wrote:
> > Do you need to make this a callback? You could just set a bool state and
call
> > SendSurfaceIsProtectedFalseAfterThumbnailerFinished from
> > CopyFromCompositingSurfaceFinished if it is set.
> > 
> > Similarly to what we do in TextureImageTransportSurface, maybe you could
make
> > that logic as:
> > - IsProtected is a virtual state of (!visible &&
> !surface_in_use_by_thumbnailer
> > && !surface_in_use_by_compositor)
> > - tracking individual states separately
> >   * visible through DidBecomeSelected/WasHidden,
> >   * surface_in_use_by_thumbnailer through
> > CopyFromCompositingSurface/CopyFromCompositingSurfaceFinished,
> >   * surface_in_use_by_compositor through the CompositorObserver callbacks
> > - send the message any time IsProtected() changes (e.g. have a common
function
> > that checks the current state vs the last sent state and send a message when
> > they're different - and call that function every time any of the above
> changes).
> > 
> > I think that way it'd be much clearer about the logic (tracing callbacks is
> > hard).
> 

Done.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:1342:
is_thumbnailing_ = 0;
On 2012/06/11 21:32:24, piman wrote:
> The thumbnailing callback should be called either way (possibly with failure),
> so there shouldn't be a need to do this.

Done.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/common/gpu...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/common/gpu...
content/common/gpu/texture_image_transport_surface.cc:376: // TODO: safe to send
swapbuffers when using postsubbuffers usually?
On 2012/06/11 21:32:24, piman wrote:
> Yes, it's always safe. A Swap is essentially a Post will full damage.

Done.

https://chromiumcodereview.appspot.com/10052018/diff/71002/content/common/gpu...
content/common/gpu/texture_image_transport_surface.cc:387: if
(textures_[front()].size != textures_[back()].size ||
I got around this issue, as well as having other benefits, by just taking a
SwapBuffers with a "require_ack" bool.

Then, the ui knows this is a special swap type, and the gpu process doesn't need
to do anything fancy in the ACK handler.

On 2012/06/11 21:45:09, mmocny wrote:
> Okay, this sounds like it could be potentially hairy.
> I'll get something in with counters like you suggest and can evaluate the
> concerns once things are cleaned up.
> 
> On 2012/06/11 21:32:24, piman wrote:
> > We should be very careful here.
> > 
> > When restoring the tab, it's possible that the tab has changed size in the
> mean
> > time.
> > 
> > On one hand, the browser will send a IsProtected(true), upon which the GPU
> > process will send a Swap, and that will come back here.
> > On the other hand, the renderer may have started a new frame, with the new
> size,
> > i.e. it will have resized the back buffer.
> > 
> > Depending on timing, when we get here, it's possible that
> > textures_[front()].size (the old size) != textures_[back()].size (the new
> size).
> > However, at this point we do not want to recreate the back texture (it would
> > resize it to the wrong size, and lose all the rendering that has been done
up
> to
> > now).
> > 
> > Maybe the best is to keep a flag or something in
OnSetFrontSurfaceIsProtected,
> > so that we tag the Swap as "special", so that we treat is specially here.
That
> > can even help you avoid unscheduling. Just beware of multiple IsProtected in
> ABA
> > cases (i.e that flag maybe should be a counter, but special care may need to
> be
> > taken if the "real" swap is coming from the renderer before we got an ack
from
> > the "fake" swap).
>

[piman, 2012-06-12 20:59:47]

Some nits, otherwise I think it's almost good to go.

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:305:
++visibility_state_id_;
Why not do this in AdjustSurfaceProtection() before (and only if) sending the
message? The state id needs to increase any time the state changes - even if
it's not for visibility reason. BTW, I think it'd be better named
"protection_state_id" or something like that since that's really what it's
linked to.

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:528:
++pending_thumbnail_tasks_;
do this before kicking the copy, in case the callback gets called synchronously
from the CopyTextureTo

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:621: if
(params_in_pixel.require_ack)
nit: per sytle, needs braces if the body is >1 line

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.h:292: // protected
means that the current_surface_ may be in use by ui and cannot
nit: Protected (cap 1st letter)

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.h:293: // be safely
discarded. Things to consider are thumbnailerm compositor draw
typo: thumbnailer,

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.h:304: int32
route_id_;
nit: maybe rename this as "surface_route_id_" to make it clear this is the
routing id for the surface, not the view.

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/common/gpu...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/common/gpu...
content/common/gpu/gpu_messages.h:65: #endif
the #endif probably should be one line higher otherwise other platforms will
have trouble.

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/common/gpu...
content/common/gpu/gpu_messages.h:212: bool /* is_protected */, uint32 /*
visibility_state_id */)
nit: on messages we usually do 1 parameter per line.

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/common/gpu...
File content/common/gpu/texture_image_transport_surface.h (right):

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/common/gpu...
content/common/gpu/texture_image_transport_surface.h:53: OVERRIDE;
nit: you can write it as:
virtual void OnSetFrontSurfaceIsProtected(
    bool is_protected,
    uint32 visibility_state_id) OVERRIDE;

It's a more conventional style.

[mmocny, 2012-06-12 21:25:50]

See below for comment on visibility state id.  Other other nits are easily
addressable, patch incoming.

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/76003/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:305:
++visibility_state_id_;
I need to increase state id before the Adjust function since the visibility
state id is tested for changes inside SetSurfaceNotInUseByCompositor to make
sure we dont have visibility ABA issues while delaying reset for compositing
callback.

I am assuming we will only flip protection as a result of visibility changes
(since right now, that is the only way we set/reset current_surface_), so it
really is a visibility state id (although visibility state id changes more often
than protection does, it always flips when we need it to).

On 2012/06/12 20:59:48, piman wrote:
> Why not do this in AdjustSurfaceProtection() before (and only if) sending the
> message? The state id needs to increase any time the state changes - even if
> it's not for visibility reason. BTW, I think it'd be better named
> "protection_state_id" or something like that since that's really what it's
> linked to.

[mmocny, 2012-06-12 22:50:09]

Okay, replaced with protection_state_id and addressed minor nits.

I played with patch on device earlier and it worked excellent.  On stumpy with
30" screen with maximized window and poster circles running, the 67ms timeout
was not enough to create a new frame and I do see flashing tab content. 
@reveman was here last week and had suggestions on decreasing latency in
updating frames so hopefully we improve over time. Smaller windows did not
flash, which is great.

I did not test these most recent changes because ToT was not working for me
after rebase.  Filed bugs and will confirm tomorrow morning.

[piman, 2012-06-12 23:05:11]

LGTM+nits
You should probably send try jobs...

https://chromiumcodereview.appspot.com/10052018/diff/81004/content/browser/re...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/81004/content/browser/re...
content/browser/renderer_host/render_widget_host_impl.cc:1839: if (ui_shim)
nit: braces around body

https://chromiumcodereview.appspot.com/10052018/diff/81004/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/81004/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:638: if
(params_in_pixel.require_ack)
nit: braces

https://chromiumcodereview.appspot.com/10052018/diff/81004/content/common/gpu...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/81004/content/common/gpu...
content/common/gpu/texture_image_transport_surface.cc:344: #if defined(USE_AURA)
nit: somehow it feels wrong to have a #ifdef USE_AURA in gpu-process code,
because AURA is a browser-process concept.
Arguably we could use this class on other platforms, and the same asynchronicity
constraints would apply too.
Would it make sense to remove those #ifdef USE_AURA? The parameter can stay
unused on other platforms.

[mmocny, 2012-06-13 14:47:22]

Adding reviewers:

@davemoore for chrome/browser/chromeos/login/login_util.cc

@sky for content/browser/renderer_host/

@joi for content/port/browser/ and content/public/common/

Thanks!

[Jói, 2012-06-13 15:18:39]

LGTM for content/port and content/common.

[sky, 2012-06-13 16:21:23]

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_impl.cc:1834: void
RenderWidgetHostImpl::SendFrontSurfaceIsProtected(
What is aura specific here?

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_impl.h:155: bool IsVisible()
const { return !is_hidden_; }
generally we return the member directly and name the method to match, eg
is_hidden().

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:530:
++pending_thumbnail_tasks_;
Why does this need to be delayed, and why do you need to schedule more than one
at a time?

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:538: void
RenderWidgetHostViewAura::CopyFromCompositingSurfaceFinished(
Position should match that of header.

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:559:
DCHECK(image_transport_clients_.find(current_surface_) !=
DCHECK_NE

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.h:234: void
SetSurfaceNotInUseByCompositor();
Add descriptions for these.

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.h:301: // Protected
means that the current_surface_ may be in use by ui and cannot
Use |s around argument names and field names in documentation.

[mmocny, 2012-06-13 18:40:05]

Updated.

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_impl.cc:1834: void
RenderWidgetHostImpl::SendFrontSurfaceIsProtected(
On other platforms this is not needed, because it is safe for the gpu process to
release its handle to the front surface without invalidating the browser handle.
 It can then send a release/suspend message to the browser to finalize the
release.  If we tab switch during this time, it is still safe to display the old
content.  On aura, the browser needs to reset its handle and promise to no
longer use the surface before the gpu can release.  This message is how we
communicate that fact.

We could add it to all platforms and not use it (since they may potentially need
to use it in the future), but perhaps we can delay doing that until that need
arises?

On 2012/06/13 16:21:23, sky wrote:
> What is aura specific here?

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_impl.h:155: bool IsVisible()
const { return !is_hidden_; }
On 2012/06/13 16:21:23, sky wrote:
> generally we return the member directly and name the method to match, eg
> is_hidden().

Done.

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:530:
++pending_thumbnail_tasks_;
The browser thumbnailer will call this multiple times if we switch tabs during
the duration of the previous async read.  Discussing with @mazda, the browser
thumbnailer is written to ignore all but the last read in such a case, so I
cannot ignore new requests nor cancel current ones, so I hdd to schedule
multiple concurrently.

If we want to fix the browser thumbnailer to not issue multiple concurrent
reads, then we can fix this, but I suspect that should be a follow on.

On 2012/06/13 16:21:23, sky wrote:
> Why does this need to be delayed, and why do you need to schedule more than
one
> at a time?

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:538: void
RenderWidgetHostViewAura::CopyFromCompositingSurfaceFinished(
On 2012/06/13 16:21:23, sky wrote:
> Position should match that of header.

Done.

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:559:
DCHECK(image_transport_clients_.find(current_surface_) !=
DCHECK_NE seems to cause compile failure when values are std::map iterator.  (no
match for 'operator<<').
grepped the code base and could not find another use of DCHECK_NE with iterator
values.

On 2012/06/13 16:21:23, sky wrote:
> DCHECK_NE

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.h:234: void
SetSurfaceNotInUseByCompositor();
On 2012/06/13 16:21:23, sky wrote:
> Add descriptions for these.

Done.

https://chromiumcodereview.appspot.com/10052018/diff/71023/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.h:301: // Protected
means that the current_surface_ may be in use by ui and cannot
On 2012/06/13 16:21:23, sky wrote:
> Use |s around argument names and field names in documentation.

Done.

[sky, 2012-06-13 21:56:21]

LGTM

[glotov, 2012-06-15 15:11:14]

LGTM

[commit-bot: I haz the power, 2012-06-15 15:20:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10052018/74023

[commit-bot: I haz the power, 2012-06-15 16:54:53]

Change committed as 142408

[commit-bot: I haz the power, 2012-06-15 22:40:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10052018/92002

[commit-bot: I haz the power, 2012-06-16 00:35:57]

Change committed as 142541

[Nikita (slow), 2012-06-18 13:18:22]

I had to revert this change as these bots were broken:

http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28amd6...
http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%...

You should run cros_x86 trybot to check whether improved change would work.

After revert bots are back to green:
http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%...
http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28amd6...

My suggestion is that your change somehow may be affecting how we track login
screen WebUI being painted so reporting that sign in WebUI is ready so that
autotests could proceed.
See this change for more details -
https://chromiumcodereview.appspot.com/10476002

[commit-bot: I haz the power, 2012-06-21 19:58:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10052018/104003

[commit-bot: I haz the power, 2012-06-21 21:04:37]

Try job failure for 10052018-104003 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-21 22:11:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10052018/104003

[commit-bot: I haz the power, 2012-06-21 23:41:59]

Try job failure for 10052018-104003 (retry) (retry) on mac_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-22 13:34:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10052018/104003

[commit-bot: I haz the power, 2012-06-22 15:04:41]

Change committed as 143590

[Nikita (slow), 2012-06-22 15:29:13]

What was the issue with CrOS tests?

[piman, 2012-07-25 19:56:51]

http://codereview.chromium.org/10052018/diff/140002/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/10052018/diff/140002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:512: callback);
I believe the thumbnailer expects you to run the callback in all cases, even if
it's to report failure. Maybe double-check with mazda@
The risk here is that if the RWHVA gets deleted, the callback won't be sent. You
may have to keep a list of pending callbacks (add here, remove in
CopyFromCompositingSurfaceFinished) and run them with failure when the RWHVA
gets deleted.

[commit-bot: I haz the power, 2012-07-26 19:24:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10052018/136017

[commit-bot: I haz the power, 2012-07-26 23:42:05]

Change committed as 148657