Unified OutputSurface::SwapBuffers.

Unified OutputSurface::SwapBuffers.

This patch introduces a hard contract that CC will always call
OutputSurface::SwapBuffers(), and OutputSurface will always respond with
OutputSurfaceClient::OnSwapBuffersComplete(), in all rendering modes.  I
deleted the methods SendFrameToParentCompositor, PostSubBuffer, and
OnSendFrameToParentCompositorAck, subsuming them into SwapBuffers. I also
deleted all the settings and capabilities specifying which variant needed to be
called. 

This should be a no-op for all graphics modes except for Android WebView, where
it has the benefits of ensuring OnSwapBuffersComplete is called and that
CompositorFrameMetadata is available even though no parent compositor exists. 

NOTRY=true
BUG=237006
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205501

[aelias_OOO_until_Jul13, 2013-06-10 08:14:39]

Hi Antoine, PTAL.  I realized last week that this cleanup would also fix several
WebView bugs, so I figured it's a good time to unify the SwapBuffers flow
instead of adding more complexity.  As far as I know this should be a no-op for
everyone else, but I'll make sure to smoke test every rendering mode before
trying to land this.

[joth, 2013-06-10 18:43:32]

Thanks for the sweeping clean up Alex, it's going to really make our lives
easier.

Only some minor comments / suggestions from me. (I expect you'll have a followup
iteration to get unit tests passing?)

https://codereview.chromium.org/16304003/diff/9001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16304003/diff/9001/cc/output/output_surface.c...
cc/output/output_surface.cc:195: }
fwiw, I found I had to wrap this lot in "if context3d_" (and remove the DCHECK)
to have the SW renderer LTHI unit tests not timeout, as they will now need the
OnSwapBuffersComplete() below.

The alternative is write a dedicated FakeOutputSurface (just adds test
clutter?), or a deeper refactoring of this class to make context3d  optional by
design which is probably a nice idea, but beyond scope of this patch.

https://codereview.chromium.org/16304003/diff/9001/content/browser/android/in...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/16304003/diff/9001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:139:
did_swap_buffer_ = true;
// TODO(joth): Route page scale to the client, see http://crbug.com/237006

https://codereview.chromium.org/16304003/diff/9001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/16304003/diff/9001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:96: if (swap_with_ipc_) {
nit: swap_buffers_via_ipc_ maybe clearer?

https://codereview.chromium.org/16304003/diff/9001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:101: if
(frame->software_frame_data)
do you need include the base-call here, to get the default
client_->OnSwapBuffersComplete() invocation? (or, just call it straight from
here)

https://codereview.chromium.org/16304003/diff/9001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/16304003/diff/9001/content/renderer/render_wi...
content/renderer/render_widget.cc:634: }
nit: indent

[aelias_OOO_until_Jul13, 2013-06-10 18:57:57]

https://codereview.chromium.org/16304003/diff/9001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16304003/diff/9001/cc/output/output_surface.c...
cc/output/output_surface.cc:195: }
cc_unittests package is already passing as of Patch Set 2.  It did need some
extra logic in FakeOutputSurface.  I agree the OutputSurface hierarchy needs
more refactoring but it was biting off too much to try to add that to this
patch.

https://codereview.chromium.org/16304003/diff/9001/content/browser/android/in...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/16304003/diff/9001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:139:
did_swap_buffer_ = true;
On 2013/06/10 18:43:32, joth wrote:
> // TODO(joth): Route page scale to the client, see http://crbug.com/237006

Done.

https://codereview.chromium.org/16304003/diff/9001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/16304003/diff/9001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:96: if (swap_with_ipc_) {
On 2013/06/10 18:43:32, joth wrote:
> nit: swap_buffers_via_ipc_ maybe clearer?

Done.

https://codereview.chromium.org/16304003/diff/9001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:101: if
(frame->software_frame_data)
On 2013/06/10 18:43:32, joth wrote:
> do you need include the base-call here, to get the default
> client_->OnSwapBuffersComplete() invocation? (or, just call it straight from
> here)

Good catch, changed to call directly from here.

https://codereview.chromium.org/16304003/diff/9001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/16304003/diff/9001/content/renderer/render_wi...
content/renderer/render_widget.cc:634: }
On 2013/06/10 18:43:32, joth wrote:
> nit: indent

Done.

[joth, 2013-06-10 19:04:49]

lgtm

https://codereview.chromium.org/16304003/diff/9001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16304003/diff/9001/cc/output/output_surface.c...
cc/output/output_surface.cc:195: }
On 2013/06/10 18:57:57, aelias wrote:
> cc_unittests package is already passing as of Patch Set 2.  It did need some
> extra logic in FakeOutputSurface.  I agree the OutputSurface hierarchy needs
> more refactoring but it was biting off too much to try to add that to this
> patch.

oh yes! Nice. I totally missed that somehow :)

[joth, 2013-06-10 19:06:42]

Ben just pointed out this causes SW draw to think it failed to draw... simple
fix.

https://codereview.chromium.org/16304003/diff/18001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/16304003/diff/18001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:210:
return finished_draw;
ah, need to return did_swap_buffer_  here now (so you can delete the
finished_draw local, but still need to reset the canvas pointer to NULL)

[aelias_OOO_until_Jul13, 2013-06-10 19:19:03]

https://codereview.chromium.org/16304003/diff/18001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/16304003/diff/18001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:210:
return finished_draw;
On 2013/06/10 19:06:42, joth wrote:
> ah, need to return did_swap_buffer_  here now (so you can delete the
> finished_draw local, but still need to reset the canvas pointer to NULL)

Done.

[joth, 2013-06-10 19:28:01]

lgtm (and ben confirmed that works)

[piman, 2013-06-10 19:52:25]

Very nice cleanup, me likes.

There's a couple of things. There's also
https://codereview.chromium.org/16730003/ that's in the pipe, they interact with
each other (though they're moving in the same direction) we need to figure out
how to stage them.

https://codereview.chromium.org/16304003/diff/24001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/16304003/diff/24001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1998: swap_buffer_rect_.height());
Is it at all possible that the rect here would be empty? That would cause us to
Swap which would be bad (if we didn't draw anything).
I think we should DCHECK.

https://codereview.chromium.org/16304003/diff/24001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16304003/diff/24001/cc/output/output_surface....
cc/output/output_surface.cc:198: client_->OnSwapBuffersComplete(NULL);
I don't know if there could be some issues with re-entering here - see below. If
there are, maybe we want to post a task for this?

https://codereview.chromium.org/16304003/diff/24001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/16304003/diff/24001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:176:
state_machine_.DidDrawIfPossibleCompleted(result.did_draw);
In the case were we will call OnSwapBuffersComplete directly from SwapBuffers,
can we get in trouble here? It will be called before we update the state
machine.

https://codereview.chromium.org/16304003/diff/24001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16304003/diff/24001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1224: active_tree_->ClearLatencyInfo();
Here as well, is there any potential trouble if we call OnSwapBuffersComplete
re-entrantly from SwapBuffers() ?

https://codereview.chromium.org/16304003/diff/24001/content/renderer/gpu/comp...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/16304003/diff/24001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_output_surface.cc:102:
client_->OnSwapBuffersComplete(NULL);
similar to concerns in the base OutputSurface: any potential trouble here if we
re-enter?

https://codereview.chromium.org/16304003/diff/24001/content/renderer/gpu/comp...
File content/renderer/gpu/compositor_output_surface.h (right):

https://codereview.chromium.org/16304003/diff/24001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_output_surface.h:94: bool swap_buffers_via_ipc_;
nit: naming was confusing to me at first, because WGC3D::prepareTexture also
goes to IPC (through command buffers, to the GPU). How about
use_swap_compositor_frame_message_ or something?

[aelias_OOO_until_Jul13, 2013-06-10 23:18:06]

https://codereview.chromium.org/16304003/diff/24001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/16304003/diff/24001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1998: swap_buffer_rect_.height());
On 2013/06/10 19:52:25, piman wrote:
> Is it at all possible that the rect here would be empty? That would cause us
to
> Swap which would be bad (if we didn't draw anything).
> I think we should DCHECK.

OK, switched to full rect.  And I introduced another GLFrameData boolean
partial_swap_allowed for now to avoid changing postSubBuffer/prepareTexture
behavior as part of this patch (since I'd I'd prefer backer@ took ownership of
managing any fallout).

https://codereview.chromium.org/16304003/diff/24001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16304003/diff/24001/cc/output/output_surface....
cc/output/output_surface.cc:198: client_->OnSwapBuffersComplete(NULL);
On 2013/06/10 19:52:25, piman wrote:
> I don't know if there could be some issues with re-entering here - see below.
If
> there are, maybe we want to post a task for this?

I switched to always post task for onSwapBuffersComplete, thanks for
highlighting this problem.

https://codereview.chromium.org/16304003/diff/24001/content/renderer/gpu/comp...
File content/renderer/gpu/compositor_output_surface.h (right):

https://codereview.chromium.org/16304003/diff/24001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_output_surface.h:94: bool swap_buffers_via_ipc_;
On 2013/06/10 19:52:25, piman wrote:
> nit: naming was confusing to me at first, because WGC3D::prepareTexture also
> goes to IPC (through command buffers, to the GPU). How about
> use_swap_compositor_frame_message_ or something?

Renamed.

[piman, 2013-06-10 23:30:05]

lgtm

https://codereview.chromium.org/16304003/diff/36002/content/renderer/gpu/comp...
File content/renderer/gpu/compositor_output_surface.h (right):

https://codereview.chromium.org/16304003/diff/36002/content/renderer/gpu/comp...
content/renderer/gpu/compositor_output_surface.h:48: bool swap_buffers_via_ipc);
nit: I would rename this parameter too...

[aelias_OOO_until_Jul13, 2013-06-11 05:59:48]

FYI, a few of the Aura layer_unittests were relying on the old behavior that a
commit always caused a draw notification, even if there was no damage and
therefore no draw.  I fixed those tests to observe commits instead of observing
draws (or changed the order of operations so that there's always damage).

[jbauman, 2013-06-11 06:52:28]

https://codereview.chromium.org/16304003/diff/26017/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16304003/diff/26017/cc/output/output_surface....
cc/output/output_surface.cc:187: DCHECK(context3d_);
This won't necessarily be the case in the software renderer path, will it?

[piman, 2013-06-11 06:56:56]

On Mon, Jun 10, 2013 at 10:59 PM, <aelias@chromium.org> wrote:

> FYI, a few of the Aura layer_unittests were relying on the old behavior
> that a
> commit always caused a draw notification, even if there was no damage and
> therefore no draw.  I fixed those tests to observe commits instead of
> observing
> draws (or changed the order of operations so that there's always damage).
>

What in your patch changes that behavior?


>
>
https://codereview.chromium.**org/16304003/<https://codereview.chromium.org/1...
>

[aelias_OOO_until_Jul13, 2013-06-11 07:02:07]

On 2013/06/11 06:56:56, piman wrote:
> On Mon, Jun 10, 2013 at 10:59 PM, <mailto:aelias@chromium.org> wrote:
> 
> > FYI, a few of the Aura layer_unittests were relying on the old behavior
> > that a
> > commit always caused a draw notification, even if there was no damage and
> > therefore no draw.  I fixed those tests to observe commits instead of
> > observing
> > draws (or changed the order of operations so that there's always damage).
> >
> 
> What in your patch changes that behavior?

The lines I deleted from ui::Compositor::DidCommitAndDrawFrame().

[aelias_OOO_until_Jul13, 2013-06-11 07:12:45]

https://codereview.chromium.org/16304003/diff/26017/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16304003/diff/26017/cc/output/output_surface....
cc/output/output_surface.cc:187: DCHECK(context3d_);
On 2013/06/11 06:52:28, jbauman wrote:
> This won't necessarily be the case in the software renderer path, will it?

Hmm, you're right, thanks.  I added an early out here.

What are the latest flags to try out end-to-end software compositing on Linux? 
I had only tested renderer-side software composting.

[piman, 2013-06-11 07:27:46]

On 2013/06/11 07:02:07, aelias wrote:
> On 2013/06/11 06:56:56, piman wrote:
> > On Mon, Jun 10, 2013 at 10:59 PM, <mailto:aelias@chromium.org> wrote:
> > 
> > > FYI, a few of the Aura layer_unittests were relying on the old behavior
> > > that a
> > > commit always caused a draw notification, even if there was no damage and
> > > therefore no draw.  I fixed those tests to observe commits instead of
> > > observing
> > > draws (or changed the order of operations so that there's always damage).
> > >
> > 
> > What in your patch changes that behavior?
> 
> The lines I deleted from ui::Compositor::DidCommitAndDrawFrame().

Mmh, if we don't call OnCompositingEnded with test contexts, I think this could
break some browser_tests on aura.

[aelias_OOO_until_Jul13, 2013-06-11 07:34:34]

On 2013/06/11 07:27:46, piman wrote:
> On 2013/06/11 07:02:07, aelias wrote:
> > On 2013/06/11 06:56:56, piman wrote:
> > > On Mon, Jun 10, 2013 at 10:59 PM, <mailto:aelias@chromium.org> wrote:
> > > 
> > > > FYI, a few of the Aura layer_unittests were relying on the old behavior
> > > > that a
> > > > commit always caused a draw notification, even if there was no damage
and
> > > > therefore no draw.  I fixed those tests to observe commits instead of
> > > > observing
> > > > draws (or changed the order of operations so that there's always
damage).
> > > >
> > > 
> > > What in your patch changes that behavior?
> > 
> > The lines I deleted from ui::Compositor::DidCommitAndDrawFrame().
> 
> Mmh, if we don't call OnCompositingEnded with test contexts, I think this
could
> break some browser_tests on aura.

In Patch Set 8 all the win_aura and linux_chromeos browser_tests passed, so I
think we're probably good here.  OnCompositingEnded still gets called as long as
there's actually a redraw after the commit, which is very likely the case in an
integration test.

If a test relied on this behavior before, that was incorrect and it would have
broken if its context ever got on_swap_complete_callback property.

[piman, 2013-06-11 07:59:03]

On Tue, Jun 11, 2013 at 12:34 AM, <aelias@chromium.org> wrote:

> On 2013/06/11 07:27:46, piman wrote:
>
>> On 2013/06/11 07:02:07, aelias wrote:
>> > On 2013/06/11 06:56:56, piman wrote:
>> > > On Mon, Jun 10, 2013 at 10:59 PM, <mailto:aelias@chromium.org> wrote:
>> > >
>> > > > FYI, a few of the Aura layer_unittests were relying on the old
>> behavior
>> > > > that a
>> > > > commit always caused a draw notification, even if there was no
>> damage
>>
> and
>
>> > > > therefore no draw.  I fixed those tests to observe commits instead
>> of
>> > > > observing
>> > > > draws (or changed the order of operations so that there's always
>>
> damage).
>
>> > > >
>> > >
>> > > What in your patch changes that behavior?
>> >
>> > The lines I deleted from ui::Compositor::**DidCommitAndDrawFrame().
>>
>
>  Mmh, if we don't call OnCompositingEnded with test contexts, I think this
>>
> could
>
>> break some browser_tests on aura.
>>
>
> In Patch Set 8 all the win_aura and linux_chromeos browser_tests passed,
> so I
> think we're probably good here.  OnCompositingEnded still gets called as
> long as
> there's actually a redraw after the commit, which is very likely the case
> in an
> integration test.
>
> If a test relied on this behavior before, that was incorrect and it would
> have
> broken if its context ever got on_swap_complete_callback property.
>

Oh, I see... DidCompleteSwapBuffers still gets called, now by LTHI instead
of explicitly.

LGTM then.


>
https://codereview.chromium.**org/16304003/<https://codereview.chromium.org/1...
>

[aelias_OOO_until_Jul13, 2013-06-11 09:17:56]

OK, all the trybots went green in one of the recent patchsets, let's try to land
this.

[commit-bot: I haz the power, 2013-06-11 09:18:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/16304003/78001

[commit-bot: I haz the power, 2013-06-11 12:19:31]

Change committed as 205501

[slavi, 2013-06-11 16:43:47]

On 2013/06/11 07:12:45, aelias wrote:
>
https://codereview.chromium.org/16304003/diff/26017/cc/output/output_surface.cc
> File cc/output/output_surface.cc (right):
> 
>
https://codereview.chromium.org/16304003/diff/26017/cc/output/output_surface....
> cc/output/output_surface.cc:187: DCHECK(context3d_);
> On 2013/06/11 06:52:28, jbauman wrote:
> > This won't necessarily be the case in the software renderer path, will it?
> 
> Hmm, you're right, thanks.  I added an early out here.
> 
> What are the latest flags to try out end-to-end software compositing on Linux?

> I had only tested renderer-side software composting.

Still --ui-enable-software-compositing --enable-software-compositing-gl-adapter
--enable-compositor-frame-message --disable-accelerated-2d-canvas
--force-compositing-mode --allow-webui-compositing  --no-sandbox.
Soon to be --disable-gpu mode.