Implemented software output device for Aura.

Implemented software output device for Aura.


BUG=124671, 161008
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191873

[slavi, 2013-03-28 22:18:56]

This is essentially the same patch as https://codereview.chromium.org/13042022/
with all comments addressed.

Note that it depends on https://codereview.chromium.org/13248003/

[piman, 2013-03-29 00:23:53]

https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
File content/renderer/gpu/compositor_software_output_device.cc (right):

https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.cc:66: dibs_.clear();
From previous review: I still don't understand why it's safe. When you call Send
for the SwapCompositorFrame with a reference to a handle, all it does is post a
task to the IO thread. Now, if this thread continues on and calls Resize, you
may delete the transport dib, which will free the handle before it's been sent
over the wire (and used on the other side). So the other side will try to open /
attach to an invalid handle.

I think you only want to resize TransportDIBs that you "own", that is that you
haven't sent yet and/or have received back.

https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.cc:68:
dibs_.push_back(CreateDIB());
From previous review: why not let BeginPaint reallocate the TransportDIB?
There's no much point in pre-allocating, and that lets you get rid of
kNumBuffers which isn't very meaningful (since you can have more buffers than
that). Less code.

https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
File content/renderer/gpu/compositor_software_output_device.h (right):

https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.h:8: #include
"base/memory/scoped_ptr.h"
From previous review: I don't think this is needed. Please remove.

https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.h:18: // to a fixed
thread when bindToClient is called.
From previous review: bindToClient -> BindToClient

[slavi, 2013-03-29 01:48:40]

On 2013/03/29 00:23:53, piman wrote:
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> File content/renderer/gpu/compositor_software_output_device.cc (right):
> 
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> content/renderer/gpu/compositor_software_output_device.cc:66: dibs_.clear();
> From previous review: I still don't understand why it's safe. When you call
Send
> for the SwapCompositorFrame with a reference to a handle, all it does is post
a
> task to the IO thread. Now, if this thread continues on and calls Resize, you
> may delete the transport dib, which will free the handle before it's been sent
> over the wire (and used on the other side). So the other side will try to open
/
> attach to an invalid handle.
> 
> I think you only want to resize TransportDIBs that you "own", that is that you
> haven't sent yet and/or have received back.

It is safe in the sense that the render_widget_host handles correctly if it
receives a DIB that fails to Map (see
content/browser/renderer_host/render_widget_host_view_aura.cc
https://codereview.chromium.org/13042012/ which still haven't landed).

You're right that if I keep the handle lingering open in the renderer, the
browser will probably never receive invalid handles. But since this happens only
after Resize event, the browser would ignore this frame anyway in this case (it
will have the wrong size).

>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> content/renderer/gpu/compositor_software_output_device.cc:68:
> dibs_.push_back(CreateDIB());
> From previous review: why not let BeginPaint reallocate the TransportDIB?
> There's no much point in pre-allocating, and that lets you get rid of
> kNumBuffers which isn't very meaningful (since you can have more buffers than
> that). Less code.

Calling CreateDIB in BeginPaint actually is hack.

When threaded compositing (in the renderer) is enabled, the scheduler guarantees
that there won't be more than 2 frames in flight (hence the kNumBuffers). The
CreateDIB in BeginPaint is never hit this case.

The CreateDIB in BeginPaint is there to support the non-threaded compositing
case which doesn't guarantee anything about how many frames in flight the will
be.

> 
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> File content/renderer/gpu/compositor_software_output_device.h (right):
> 
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> content/renderer/gpu/compositor_software_output_device.h:8: #include
> "base/memory/scoped_ptr.h"
> From previous review: I don't think this is needed. Please remove.

Done.

> 
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> content/renderer/gpu/compositor_software_output_device.h:18: // to a fixed
> thread when bindToClient is called.
> From previous review: bindToClient -> BindToClient

From previous review: Done. =-)

[piman, 2013-04-01 18:18:07]

On 2013/03/29 01:48:40, slavi wrote:
> On 2013/03/29 00:23:53, piman wrote:
> >
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> > File content/renderer/gpu/compositor_software_output_device.cc (right):
> > 
> >
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> > content/renderer/gpu/compositor_software_output_device.cc:66: dibs_.clear();
> > From previous review: I still don't understand why it's safe. When you call
> Send
> > for the SwapCompositorFrame with a reference to a handle, all it does is
post
> a
> > task to the IO thread. Now, if this thread continues on and calls Resize,
you
> > may delete the transport dib, which will free the handle before it's been
sent
> > over the wire (and used on the other side). So the other side will try to
open
> /
> > attach to an invalid handle.
> > 
> > I think you only want to resize TransportDIBs that you "own", that is that
you
> > haven't sent yet and/or have received back.
> 
> It is safe in the sense that the render_widget_host handles correctly if it
> receives a DIB that fails to Map (see
> content/browser/renderer_host/render_widget_host_view_aura.cc
> https://codereview.chromium.org/13042012/ which still haven't landed).

If the handle has been reused, the Map will not fail, but map something
completely unrelated / of a different size / from another renderer etc.

> 
> You're right that if I keep the handle lingering open in the renderer, the
> browser will probably never receive invalid handles. But since this happens
only
> after Resize event, the browser would ignore this frame anyway in this case
(it
> will have the wrong size).

That is, unless the RWHVA is back to the same size. Or some refactoring changes
that logic. Or another user (e.g. a parent renderer in the <webview> case /
out-of-process iframes) starts handling that message.
It's very fragile to rely on another class in another process to discard bogus
data that you sent it. It's much much safer to not send the bogus data in the
first place.

> 
> >
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> > content/renderer/gpu/compositor_software_output_device.cc:68:
> > dibs_.push_back(CreateDIB());
> > From previous review: why not let BeginPaint reallocate the TransportDIB?
> > There's no much point in pre-allocating, and that lets you get rid of
> > kNumBuffers which isn't very meaningful (since you can have more buffers
than
> > that). Less code.
> 
> Calling CreateDIB in BeginPaint actually is hack.

I don't agree. It makes sense to allocate data only when you actually need it.
And it saves code, since you have to do it anyway.

> 
> When threaded compositing (in the renderer) is enabled, the scheduler
guarantees
> that there won't be more than 2 frames in flight (hence the kNumBuffers). The
> CreateDIB in BeginPaint is never hit this case.

That is, unless we change the value through the command-line.

> The CreateDIB in BeginPaint is there to support the non-threaded compositing
> case which doesn't guarantee anything about how many frames in flight the will
> be.
> 
> > 
> >
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> > File content/renderer/gpu/compositor_software_output_device.h (right):
> > 
> >
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> > content/renderer/gpu/compositor_software_output_device.h:8: #include
> > "base/memory/scoped_ptr.h"
> > From previous review: I don't think this is needed. Please remove.
> 
> Done.
> 
> > 
> >
>
https://codereview.chromium.org/13255002/diff/1/content/renderer/gpu/composit...
> > content/renderer/gpu/compositor_software_output_device.h:18: // to a fixed
> > thread when bindToClient is called.
> > From previous review: bindToClient -> BindToClient
> 
> From previous review: Done. =-)

[slavi, 2013-04-02 01:28:18]

All done. PTAL.

[piman, 2013-04-02 01:35:26]

lgtm

[commit-bot: I haz the power, 2013-04-02 02:08:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/13255002/12001

[commit-bot: I haz the power, 2013-04-02 02:33:19]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[slavi, 2013-04-02 17:02:52]

+kbr for a ui/surface change

[Ken Russell (switch to Gerrit), 2013-04-02 17:04:19]

lgtm

[commit-bot: I haz the power, 2013-04-02 17:05:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/13255002/30001

[commit-bot: I haz the power, 2013-04-02 19:15:45]

Change committed as 191873