Call DidPostSwapBuffers and SwapBuffersComplete for browser software surface

Call DidPostSwapBuffers and SwapBuffersComplete for browser software surface

Committed: https://crrev.com/289d16f7aa916e8b1145c51244b647f74f348dda
Cr-Commit-Position: refs/heads/master@{#298895}

[no sievers, 2014-10-07 00:41:07]

sievers@chromium.org changed reviewers:
+ ccameron@chromium.org, danakj@chromium.org, enne@chromium.org

[no sievers, 2014-10-07 00:43:15]

@enne, dana:

Can't tell if this is intentional or happens to work because
Compositor::swap_state_ never gets updated.
Won't it confuse the cc scheduler?

[no sievers, 2014-10-07 00:51:16]

On 2014/10/07 00:43:15, sievers wrote:
> @enne, dana:
> 
> Can't tell if this is intentional or happens to work because
> Compositor::swap_state_ never gets updated.
> Won't it confuse the cc scheduler?

udpate ps2 for Mac. Looks like only in hw mode we call OnSurfaceDisplayed() so I
moved the Mac logic to defer the swap complete into
GpuBrowserCompositorOutputSurface.

[no sievers, 2014-10-07 22:14:55]

How about this? Looks all green.

[ccameron, 2014-10-07 23:31:17]

This lgtm from the Mac perspective

[danakj, 2014-10-08 19:32:29]

LGTM

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
content/browser/compositor/browser_compositor_output_surface.cc:93:
NOTREACHED();
how about making it pure virtual and NOTREACHED in the software implementation
with a comment about why?

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:61:
cc::OutputSurface::OnSwapBuffersComplete();
you could just call OnSwapBuffersComplete (no class name)?

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
File content/browser/compositor/software_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
content/browser/compositor/software_browser_compositor_output_surface.cc:55:
client_->DidSwapBuffers();
Ooh this seems really important if we're going to use the scheduler. In the
software case we'd hit max pending swaps and just stop swapping?

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
content/browser/compositor/software_browser_compositor_output_surface.cc:56:
PostSwapBuffersComplete();
can you post first before DidSwap like we do in other places?

[no sievers, 2014-10-08 19:59:00]

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
content/browser/compositor/browser_compositor_output_surface.cc:93:
NOTREACHED();
On 2014/10/08 19:32:29, danakj wrote:
> how about making it pure virtual and NOTREACHED in the software implementation
> with a comment about why?

Done.

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:61:
cc::OutputSurface::OnSwapBuffersComplete();
On 2014/10/08 19:32:29, danakj wrote:
> you could just call OnSwapBuffersComplete (no class name)?

Then it would call the above and not do anything for Mac.
I'll be moving this to the base class though anyways with another patch (where
it also gets the latency info).

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
File content/browser/compositor/software_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
content/browser/compositor/software_browser_compositor_output_surface.cc:55:
client_->DidSwapBuffers();
On 2014/10/08 19:32:29, danakj wrote:
> Ooh this seems really important if we're going to use the scheduler. In the
> software case we'd hit max pending swaps and just stop swapping?

Currently in compositor.cc without DidSwapBuffers() we never change the
swap_state_ i.e. don't think we ever swapped so we also don't end up limiting
ourselves.

I think it worked because both calls were missing. If I add only either one it
will trigger DCHECKs().

https://codereview.chromium.org/629293004/diff/60001/content/browser/composit...
content/browser/compositor/software_browser_compositor_output_surface.cc:56:
PostSwapBuffersComplete();
On 2014/10/08 19:32:29, danakj wrote:
> can you post first before DidSwap like we do in other places?

Done.

[no sievers, 2014-10-08 19:59:38]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-10-08 20:03:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/629293004/80001

[danakj, 2014-10-08 20:36:10]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-10-08 20:36:54]

https://codereview.chromium.org/629293004/diff/80001/content/browser/composit...
File content/browser/compositor/software_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/629293004/diff/80001/content/browser/composit...
content/browser/compositor/software_browser_compositor_output_surface.cc:56:
client_->DidSwapBuffers();
wei has confirmed that we already do get to DidSwapBuffers code somehow in
software compositor, so I unchecked the CQ while she figures out what that stack
trace looks like, we don't wanna call it twice!

[danakj, 2014-10-08 20:48:56]

https://codereview.chromium.org/629293004/diff/80001/content/browser/composit...
File content/browser/compositor/software_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/629293004/diff/80001/content/browser/composit...
content/browser/compositor/software_browser_compositor_output_surface.cc:56:
client_->DidSwapBuffers();
On 2014/10/08 20:36:53, danakj wrote:
> wei has confirmed that we already do get to DidSwapBuffers code somehow in
> software compositor, so I unchecked the CQ while she figures out what that
stack
> trace looks like, we don't wanna call it twice!

Ahhh ok so right now we don't tell the scheduler DidSwap or DidSwapComplete, but
it somehow just works anyways. This will add both, so that's great.

[danakj, 2014-10-08 20:49:00]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-10-08 20:53:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/629293004/80001

[no sievers, 2014-10-08 21:09:47]

On 2014/10/08 20:48:56, danakj wrote:
>
https://codereview.chromium.org/629293004/diff/80001/content/browser/composit...
> File content/browser/compositor/software_browser_compositor_output_surface.cc
> (right):
> 
>
https://codereview.chromium.org/629293004/diff/80001/content/browser/composit...
> content/browser/compositor/software_browser_compositor_output_surface.cc:56:
> client_->DidSwapBuffers();
> On 2014/10/08 20:36:53, danakj wrote:
> > wei has confirmed that we already do get to DidSwapBuffers code somehow in
> > software compositor, so I unchecked the CQ while she figures out what that
> stack
> > trace looks like, we don't wanna call it twice!
> 
> Ahhh ok so right now we don't tell the scheduler DidSwap or DidSwapComplete,
but
> it somehow just works anyways. This will add both, so that's great.

Yes I double-checked that Compositor::DidPostSwapBuffers() and
Compositor::DidCompleteSwapBuffers() were not getting called before this patch.

[danakj, 2014-10-08 21:15:47]

On 2014/10/08 21:09:47, sievers wrote:
> On 2014/10/08 20:48:56, danakj wrote:
> >
>
https://codereview.chromium.org/629293004/diff/80001/content/browser/composit...
> > File
content/browser/compositor/software_browser_compositor_output_surface.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/629293004/diff/80001/content/browser/composit...
> > content/browser/compositor/software_browser_compositor_output_surface.cc:56:
> > client_->DidSwapBuffers();
> > On 2014/10/08 20:36:53, danakj wrote:
> > > wei has confirmed that we already do get to DidSwapBuffers code somehow in
> > > software compositor, so I unchecked the CQ while she figures out what that
> > stack
> > > trace looks like, we don't wanna call it twice!
> > 
> > Ahhh ok so right now we don't tell the scheduler DidSwap or DidSwapComplete,
> but
> > it somehow just works anyways. This will add both, so that's great.
> 
> Yes I double-checked that Compositor::DidPostSwapBuffers() and
> Compositor::DidCompleteSwapBuffers() were not getting called before this
patch.

Ooh ya that's the same as SingleThreadProxy::DidSwapBuffersOnImplThread. Cool :D

[commit-bot: I haz the power, 2014-10-08 22:16:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-08 22:16:14]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[no sievers, 2014-10-08 23:02:45]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-10-08 23:05:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/629293004/80001

[commit-bot: I haz the power, 2014-10-09 00:27:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 00:27:01]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[no sievers, 2014-10-09 15:32:52]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-10-09 15:34:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/629293004/80001

[commit-bot: I haz the power, 2014-10-09 16:20:10]

Committed patchset #5 (id:80001) as d6b25d289a700f86c5fdd7bcdb3fed51ab80a3d0

[commit-bot: I haz the power, 2014-10-09 16:22:02]

Patchset 5 (id:??) landed as
https://crrev.com/289d16f7aa916e8b1145c51244b647f74f348dda
Cr-Commit-Position: refs/heads/master@{#298895}