Fixes issues when switching between software and compositing.

Fixes issues when switching between software and compositing.

Some buffers were sent back to the texture_image_transport_surface
when it wasn't expecting them. This patch only sends and ACK when needed
and destroys the buffer.

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

[alexst (slow to review), 2013-01-16 16:12:41]

This should address the navigation coruption issue.

[Fady Samuel, 2013-01-16 16:22:22]

Before I start looking at nits, could you please describe what you're doing here
more thoroughly.

https://codereview.chromium.org/11967013/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11967013/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:63:
RenderThread::Get()->Send(
Please use BrowserPluginManager to send messages for testability.

https://codereview.chromium.org/11967013/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/11967013/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:95: Send(new
BrowserPluginHostMsg_BuffersSwappedACK(
Why is this in BrowserPluginManager and not in BrowserPlugin?

[alexst (slow to review), 2013-01-16 16:58:10]

> Before I start looking at nits, could you please describe what you're doing
here
> more thoroughly.
 
Some background, texture_image_transport_surface expects one ACK for every
buffer it sends to the embedder, no more, no less. It asserts if an extra ack
arrives and waits for the final ack before shutting down.

My original implementation just sent an ack for every buffer released from the
compositor, which worked fine until you switch back to software and a buffer
gets freed up from the compositor before a new one arrives from the guest
renderer.

The solution is to add some state to the callback so that it knows if the guest
is expecting an ack and delete the freed buffer otherwise (once the mailbox
arrives, we own it and can delete it).

It also cleans up the destruction sequence because once we know container is
going away, we can delete the layer and just fast ack any new buffers since we
won't display them anyway.

[alexst (slow to review), 2013-01-16 16:58:21]

https://codereview.chromium.org/11967013/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/11967013/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:95: Send(new
BrowserPluginHostMsg_BuffersSwappedACK(
Because the browser plugin is already gone at this point. It's the final buffer
from texture_image_transport_surface and needs to be ACK'd, otherwise it won't
shut down.

On 2013/01/16 16:22:22, Fady Samuel wrote:
> Why is this in BrowserPluginManager and not in BrowserPlugin?

[Fady Samuel, 2013-01-17 19:18:45]

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:44:
WebKit::WebGraphicsContext3D *context =
I don't really understand what this is doing. Could you please describe this?

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:60: if
(!ack_pending_) {
More description here as well.

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:110:
scoped_refptr<BrowserPluginCompositingHelper> helper(this);
Why do we have this here?

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:94: int
gpu_host_id) {
Please add a comment explaining why this exists and why it's here. Please file a
bug to solve this more generally and put the link in a comment here.

[alexst (slow to review), 2013-01-18 18:49:43]

New  patch.

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:44:
WebKit::WebGraphicsContext3D *context =
On 2013/01/17 19:18:45, Fady Samuel wrote:
> I don't really understand what this is doing. Could you please describe this?

Done.

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:60: if
(!ack_pending_) {
On 2013/01/17 19:18:45, Fady Samuel wrote:
> More description here as well. 

Done.

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:110:
scoped_refptr<BrowserPluginCompositingHelper> helper(this);
On 2013/01/17 19:18:45, Fady Samuel wrote:
> Why do we have this here?

I left it there to make it explicit that this is meant to be passed as a
scoped_refptr and remain alive for as long as the callback object is around.

I'll clean up by moving into the function call.

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/11967013/diff/7001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:94: int
gpu_host_id) {
On 2013/01/17 19:18:45, Fady Samuel wrote:
> Please add a comment explaining why this exists and why it's here. Please file
a
> bug to solve this more generally and put the link in a comment here.

Done.

[Fady Samuel, 2013-01-18 18:57:01]

LGTM after you add some more comments, please.

https://codereview.chromium.org/11967013/diff/11001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(left):

https://codereview.chromium.org/11967013/diff/11001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:55: if
(buffer_size_ != size) {
I realize this is already upstream but could you please add a comment here
regarding what's going on? Is this related to gutter?

https://codereview.chromium.org/11967013/diff/11001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11967013/diff/11001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:58: if
(sync_point)
This sync point represents when it's safe to consume a texture from the mailbox,
right? Could you please explain this in a comment.

[alexst (slow to review), 2013-01-18 19:15:42]

Sure thing, more commends added.

https://codereview.chromium.org/11967013/diff/11001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(left):

https://codereview.chromium.org/11967013/diff/11001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:55: if
(buffer_size_ != size) {
On 2013/01/18 18:57:01, Fady Samuel wrote:
> I realize this is already upstream but could you please add a comment here
> regarding what's going on? Is this related to gutter?

Done.

https://codereview.chromium.org/11967013/diff/11001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11967013/diff/11001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:58: if
(sync_point)
On 2013/01/18 18:57:01, Fady Samuel wrote:
> This sync point represents when it's safe to consume a texture from the
mailbox,
> right? Could you please explain this in a comment.

Done.

[Fady Samuel, 2013-01-18 20:07:47]

https://codereview.chromium.org/11967013/diff/21001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/11967013/diff/21001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:122: //
including autosize and a resize in process.
..in progress.

https://codereview.chromium.org/11967013/diff/21001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:124: //
later, a new buffer with updated size will arrive. During this process
During this process,

https://codereview.chromium.org/11967013/diff/21001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:126: // We
accoplish this by modifying texture coordinates in the layer,
We accomplish

https://codereview.chromium.org/11967013/diff/21001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:128: // to
also update texture coordinates.
What does this do visually? This is for gutter right? Could you please explain
that?

[alexst (slow to review), 2013-01-18 20:35:36]

> What does this do visually? This is for gutter right? Could you please explain
> that?

Yeah, it either displays a sub-region or introduces a gutter. Updated.

[commit-bot: I haz the power, 2013-01-21 13:23:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/11967013/18003

[commit-bot: I haz the power, 2013-01-21 15:42:24]

Change committed as 177926