Browser side changes for software compositing

Browser side changes for software compositing.

BUG=124671, 161008
NOTRY=true

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

[slavi, 2013-03-27 20:16:40]

https://codereview.chromium.org/13042012/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/13042012/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1413:
::DuplicateHandle(host_->GetProcess()->GetHandle(), handle,
BUG: Save the original handle value so that we send correct Acks.

[piman, 2013-04-01 22:25:19]

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

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1425: #endif
You need to check the new dib's size (in bytes) against the frame_size (in
pixels), because it's untrusted data. Below in OnPaint you will access the
shared memory based on the frame_size, but you need to check that it's safe (if
it's not, you can discard the frame or whatever, but the renderer shouldn't be
allowed to crash - or worse - the browser).

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1437: if
(!compositor || !current_dib_ || ShouldSkipFrame(frame_size_in_dip)) {
I don't think we can lump together those 3 cases.

For the !compositor case, we definitely want to keep the new dib anyway, but we
can return the *old* dib immediately (the old dib isn't "in flight").

For the ShouldSkipFrame flow, we can decide that we don't want to keep the new
dib around, and return it immediately, but if that's the case we need to keep
the old dib around. However, we can also keep the new dib around and return the
old dib, i.e. ignore the whole ShouldSkipFrame - it's necessary for GL textures
because they're used on another thread, but here it's only used on the main
thread so there's no race. That is, you could remove the ShouldSkipFrame logic
for the software mode.

For the !current_dib_ case, I assume this happens when we can't map for any
reason, between out-of-address-space or bogus (malicious?) data, and I guess
it's ok to drop the old dib, though you could also keep it.

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1447: AsWeakPtr(),
dib_id));
I think here you need to return the old dib ID, not the new one - the browser
may need to draw multiple frames with the same renderer frame.

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2704: if
(!compositor)
In all the callsites, this has already been checked, so I would DCHECK instead.

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2710:
can_lock_compositor_ = NO_PENDING_COMMIT;
I feel somewhat uneasy at moving this here. I understand it saves code, but it's
mostly unrelated to the commit callback.
If you want to leave it here, maybe change the name of the method to be explicit
about this? E.g. AddOnCommitCallbackAndDisableLocks() ?

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2712: return;
not needed.

[slavi, 2013-04-03 00:29:11]

PTAL

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

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1425: #endif
On 2013/04/01 22:25:19, piman wrote:
> You need to check the new dib's size (in bytes) against the frame_size (in
> pixels), because it's untrusted data. Below in OnPaint you will access the
> shared memory based on the frame_size, but you need to check that it's safe
(if
> it's not, you can discard the frame or whatever, but the renderer shouldn't be
> allowed to crash - or worse - the browser).

Done.

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1437: if
(!compositor || !current_dib_ || ShouldSkipFrame(frame_size_in_dip)) {
On 2013/04/01 22:25:19, piman wrote:
> I don't think we can lump together those 3 cases.
> 
> For the !compositor case, we definitely want to keep the new dib anyway, but
we
> can return the *old* dib immediately (the old dib isn't "in flight").
> 
> For the ShouldSkipFrame flow, we can decide that we don't want to keep the new
> dib around, and return it immediately, but if that's the case we need to keep
> the old dib around. However, we can also keep the new dib around and return
the
> old dib, i.e. ignore the whole ShouldSkipFrame - it's necessary for GL
textures
> because they're used on another thread, but here it's only used on the main
> thread so there's no race. That is, you could remove the ShouldSkipFrame logic
> for the software mode.
> 
> For the !current_dib_ case, I assume this happens when we can't map for any
> reason, between out-of-address-space or bogus (malicious?) data, and I guess
> it's ok to drop the old dib, though you could also keep it.

I split out the check for a valid dib and dib's size.

But I still think that the ShouldSkipFrame check is necessary to make sure that
the frames that hit the screen are always the right size. And when the browser
is being resized, it waits for a frame with the correct frame to received. WDYT?

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1447: AsWeakPtr(),
dib_id));
On 2013/04/01 22:25:19, piman wrote:
> I think here you need to return the old dib ID, not the new one - the browser
> may need to draw multiple frames with the same renderer frame.

Done.

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2704: if
(!compositor)
On 2013/04/01 22:25:19, piman wrote:
> In all the callsites, this has already been checked, so I would DCHECK
instead.

Done.

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2710:
can_lock_compositor_ = NO_PENDING_COMMIT;
On 2013/04/01 22:25:19, piman wrote:
> I feel somewhat uneasy at moving this here. I understand it saves code, but
it's
> mostly unrelated to the commit callback.
> If you want to leave it here, maybe change the name of the method to be
explicit
> about this? E.g. AddOnCommitCallbackAndDisableLocks() ?

Done.

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2712: return;
On 2013/04/01 22:25:19, piman wrote:
> not needed.

Done.

[piman, 2013-04-03 18:31:26]

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

https://codereview.chromium.org/13042012/diff/30001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1335:
base::Bind(ack_callback, false, texture_to_return));
nit: style requires braces for the else clause if it spans more than one line.
That in turn also requires braces on the if clause.

https://codereview.chromium.org/13042012/diff/30001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1444:
DLOG_IF(ERROR, damage_rect != gfx::Rect(frame_size))
nit: braces

https://codereview.chromium.org/13042012/diff/30001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1447: // Update
current_dib_ and friends.
nit: unnecessary comment.

https://codereview.chromium.org/13042012/diff/30001/content/renderer/gpu/comp...
File content/renderer/gpu/compositor_software_output_device.cc (right):

https://codereview.chromium.org/13042012/diff/30001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_software_output_device.cc:146: if (it !=
awaiting_ack_.end())
Is there any case where we get here and this would be false? Should we DCHECK
instead?

https://codereview.chromium.org/13042012/diff/30001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/13042012/diff/30001/content/renderer/render_w...
content/renderer/render_widget.cc:565: #if defined(USE_AURA)
AURA is really a UI concept, which shouldn't have a place in the renderer.
I don't think we should make a decision here based on the ui side. If anything,
because we don't ship any platform with the kEnableSoftwareCompositingGLAdapter
flag, nor ever planning to do so, so I'd say let's just remove the
CompositorSoftwareOutputDeviceGLAdapter, with its flag, and add a new
--enable-software-compositing flag, that would always do the
CompositorSoftwareOutputDevice.

The advantage is that we can make it work for <webview> too, and possibly, in
time, on other platforms if we desire.

[piman, 2013-04-03 18:36:53]

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

https://codereview.chromium.org/13042012/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1437: if
(!compositor || !current_dib_ || ShouldSkipFrame(frame_size_in_dip)) {
On 2013/04/03 00:29:11, slavi wrote:
> On 2013/04/01 22:25:19, piman wrote:
> > I don't think we can lump together those 3 cases.
> > 
> > For the !compositor case, we definitely want to keep the new dib anyway, but
> we
> > can return the *old* dib immediately (the old dib isn't "in flight").
> > 
> > For the ShouldSkipFrame flow, we can decide that we don't want to keep the
new
> > dib around, and return it immediately, but if that's the case we need to
keep
> > the old dib around. However, we can also keep the new dib around and return
> the
> > old dib, i.e. ignore the whole ShouldSkipFrame - it's necessary for GL
> textures
> > because they're used on another thread, but here it's only used on the main
> > thread so there's no race. That is, you could remove the ShouldSkipFrame
logic
> > for the software mode.
> > 
> > For the !current_dib_ case, I assume this happens when we can't map for any
> > reason, between out-of-address-space or bogus (malicious?) data, and I guess
> > it's ok to drop the old dib, though you could also keep it.
> 
> I split out the check for a valid dib and dib's size.
> 
> But I still think that the ShouldSkipFrame check is necessary to make sure
that
> the frames that hit the screen are always the right size.

ShouldSkipFrame has no impact on that though. It's there for the case where:
- the old frame is of the wrong size
- the new frame still isn't of the right size (and so we want to unblock the
renderer so that it can produce the one with the right size)

> And when the browser
> is being resized, it waits for a frame with the correct frame to received.
WDYT?

It only waits with a timeout. If we take the new frame, even though it's not the
right size, at least is more up-to-date than the old one, so that if the timeout
expires and we gutter, we have a more up-to-date frame.
Neither the old nor the new frame will be of the right size, but the new one
would be "better".

[piman, 2013-04-03 18:39:32]

also, please update the CL description at some point before we can land this.

[slavi, 2013-04-03 21:38:22]

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

https://codereview.chromium.org/13042012/diff/30001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1335:
base::Bind(ack_callback, false, texture_to_return));
On 2013/04/03 18:31:26, piman wrote:
> nit: style requires braces for the else clause if it spans more than one line.
> That in turn also requires braces on the if clause.

Done.

https://codereview.chromium.org/13042012/diff/30001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1444:
DLOG_IF(ERROR, damage_rect != gfx::Rect(frame_size))
On 2013/04/03 18:31:26, piman wrote:
> nit: braces

Done.

https://codereview.chromium.org/13042012/diff/30001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1447: // Update
current_dib_ and friends.
On 2013/04/03 18:31:26, piman wrote:
> nit: unnecessary comment.

Done.

https://codereview.chromium.org/13042012/diff/30001/content/renderer/gpu/comp...
File content/renderer/gpu/compositor_software_output_device.cc (right):

https://codereview.chromium.org/13042012/diff/30001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_software_output_device.cc:146: if (it !=
awaiting_ack_.end())
On 2013/04/03 18:31:26, piman wrote:
> Is there any case where we get here and this would be false? Should we DCHECK
> instead?

Done.

https://codereview.chromium.org/13042012/diff/30001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/13042012/diff/30001/content/renderer/render_w...
content/renderer/render_widget.cc:565: #if defined(USE_AURA)
Done. 

I'll rename the flag in a separate CL that only concerns flags.

[piman, 2013-04-03 21:54:42]

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

https://codereview.chromium.org/13042012/diff/39001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1478: return;
Is that for the first frame? The renderer still expects an Ack.
It should be ok to send back an explicit invalid id with the ack (meaning,
you're not returning anything).

https://codereview.chromium.org/13042012/diff/39001/content/renderer/gpu/comp...
File content/renderer/gpu/compositor_software_output_device.cc (right):

https://codereview.chromium.org/13042012/diff/39001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_software_output_device.cc:147: if (it !=
awaiting_ack_.end())
nit: with the DCHECK, the if is unnecessary, right?

Follow-up: from the changes in RWHVA I see now that there is the case for the
"invalid" returned id for the first frame. This should be handled explicitly I
think, so that a logic error would still trigger the DCHECK.

[slavi, 2013-04-05 20:59:00]

PTAL

[piman, 2013-04-05 21:07:28]

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

https://codereview.chromium.org/13042012/diff/39001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:1478: return;
On 2013/04/03 21:54:42, piman wrote:
> Is that for the first frame? The renderer still expects an Ack.
> It should be ok to send back an explicit invalid id with the ack (meaning,
> you're not returning anything).

ping? Since you fixed CompositorSoftwareOutputDevice::ReclaimDIB this is not
needed any more.

https://codereview.chromium.org/13042012/diff/39001/content/renderer/gpu/comp...
File content/renderer/gpu/compositor_software_output_device.cc (right):

https://codereview.chromium.org/13042012/diff/39001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_software_output_device.cc:147: if (it !=
awaiting_ack_.end())
On 2013/04/03 21:54:42, piman wrote:
> nit: with the DCHECK, the if is unnecessary, right?
> 
> Follow-up: from the changes in RWHVA I see now that there is the case for the
> "invalid" returned id for the first frame. This should be handled explicitly I
> think, so that a logic error would still trigger the DCHECK.

ping

https://codereview.chromium.org/13042012/diff/50001/ui/compositor/layer.cc
File ui/compositor/layer.cc (left):

https://codereview.chromium.org/13042012/diff/50001/ui/compositor/layer.cc#ol...
ui/compositor/layer.cc:568: if ((delegate_ || texture_) &&
!damaged_region_.isEmpty()) {
The whole point of this logic is that we don't want to send damage rects if we
don't have a delegate any more. This is needed to avoid black-outs when we close
the window and it fades away (delegate is gone but layer is still there).

How does it affect software?

[slavi, 2013-04-05 21:20:14]

On 2013/04/05 21:07:28, piman wrote:
>
https://codereview.chromium.org/13042012/diff/39001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/13042012/diff/39001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_aura.cc:1478: return;
> On 2013/04/03 21:54:42, piman wrote:
> > Is that for the first frame? The renderer still expects an Ack.
> > It should be ok to send back an explicit invalid id with the ack (meaning,
> > you're not returning anything).
> 
> ping? Since you fixed CompositorSoftwareOutputDevice::ReclaimDIB this is not
> needed any more.

Done.

> 
>
https://codereview.chromium.org/13042012/diff/39001/content/renderer/gpu/comp...
> File content/renderer/gpu/compositor_software_output_device.cc (right):
> 
>
https://codereview.chromium.org/13042012/diff/39001/content/renderer/gpu/comp...
> content/renderer/gpu/compositor_software_output_device.cc:147: if (it !=
> awaiting_ack_.end())
> On 2013/04/03 21:54:42, piman wrote:
> > nit: with the DCHECK, the if is unnecessary, right?
> > 
> > Follow-up: from the changes in RWHVA I see now that there is the case for
the
> > "invalid" returned id for the first frame. This should be handled explicitly
I
> > think, so that a logic error would still trigger the DCHECK.
> 
> ping

Done.
> 
> https://codereview.chromium.org/13042012/diff/50001/ui/compositor/layer.cc
> File ui/compositor/layer.cc (left):
> 
>
https://codereview.chromium.org/13042012/diff/50001/ui/compositor/layer.cc#ol...
> ui/compositor/layer.cc:568: if ((delegate_ || texture_) &&
> !damaged_region_.isEmpty()) {
> The whole point of this logic is that we don't want to send damage rects if we
> don't have a delegate any more. This is needed to avoid black-outs when we
close
> the window and it fades away (delegate is gone but layer is still there).
> 
> How does it affect software?

You're right it doesn't; just a leftover from my lost-damage-chasing debugging
session.

[piman, 2013-04-05 21:22:31]

LGTM, thanks for bearing with me :)

[commit-bot: I haz the power, 2013-04-05 21:23:23]

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

[commit-bot: I haz the power, 2013-04-05 22:48:53]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-04-05 23:02:55]

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

[commit-bot: I haz the power, 2013-04-06 00:50:26]

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

[commit-bot: I haz the power, 2013-04-06 00:59:25]

Change committed as 192678