Use vsync manager regardless of begin frame settings

Use vsync manager regardless of begin frame settings

There was some hope that the ui::CompositorVSyncManager could be removed
once --enable-begin-frame-scheduling was turned on.  Unfortunately, the
vsync manager is used by two additional systems that can't be easily
removed: DelegatedFrameHost::AttemptFrameSubscriberCapture as well as
components/exo/wayland/server.cc.  The latter may go away eventually.
If so, then the frame capture could then be changed to more simple
polling as needed instead of vsync manager and observer system that
needs to be plumbed everywhere.

In the medium term, the vsync manager needs to live on in all paths.
This means that BrowserCompositorOutputSurface and ui::Compositor always
need to update the vsync manager with changes.  Conditionals for
whether or not to send vsync information to renderers just get moved
down the pipeline from the output surface to DelegatedFrameHost.

Committed: https://crrev.com/0875b32a9ef74d66951b77930ca46f1a3bf08043
Cr-Commit-Position: refs/heads/master@{#414186}

[enne (OOO), 2016-08-24 18:44:49]

enne@chromium.org changed reviewers:
+ piman@chromium.org

[enne (OOO), 2016-08-24 18:52:23]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 18:52:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2016-08-24 18:55:03]

After the branch point, I'll clean up the --disable-begin-frame-scheduling
paths, but figured I'd keep that alive until then.

https://codereview.chromium.org/2277883002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

https://codereview.chromium.org/2277883002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:726:
delegated_frame_host_->SetVSyncParameters(args.frame_time, args.interval);
Deleted this line while cleaning up vsync functions in DFH because it seemed
spurious.  The DFH already gets vsync updates from the vsync manager and doesn't
need another path to redundantly do it.

[commit-bot: I haz the power, 2016-08-24 19:17:08]

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

[commit-bot: I haz the power, 2016-08-24 19:17:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[enne (OOO), 2016-08-24 20:14:30]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 20:15:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-08-24 21:23:59]

lgtm

https://codereview.chromium.org/2277883002/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2277883002/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:405: base::TimeDelta interval) {
This is only called on mac, by BrowserCompositorMac::UpdateVSyncParameters,
right?
In particular, BrowserCompositorOutputSurface skips it and goes directly to the
BeginFrameSource and the VSyncManager.

There doesn't seem like there's a good way to make it more consistent, but it
would be nice to document this fact.

[enne (OOO), 2016-08-24 21:48:37]

https://codereview.chromium.org/2277883002/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2277883002/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:405: base::TimeDelta interval) {
On 2016/08/24 at 21:23:58, piman wrote:
> This is only called on mac, by BrowserCompositorMac::UpdateVSyncParameters,
right?
> In particular, BrowserCompositorOutputSurface skips it and goes directly to
the BeginFrameSource and the VSyncManager.
> 
> There doesn't seem like there's a good way to make it more consistent, but it
would be nice to document this fact.

Do you want more than the comment I added in compositor.h?

And yeah, I felt sad about adding this at the time, but I couldn't come up with
a better way to route this.  This isn't a great alternate route, but it's not
that terrible either.  One possibility is that I could go Compositor ->
GpuProcessTransportFactory -> Display -> BrowserCompositorOutputSurface if I
made Display use BrowserCompositorOutputSurface instead of just plain
OutputSurface, but I'm not sure if I like Display knowing that.

[piman, 2016-08-24 22:18:49]

On 2016/08/24 21:48:37, enne wrote:
>
https://codereview.chromium.org/2277883002/diff/60001/ui/compositor/composito...
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/2277883002/diff/60001/ui/compositor/composito...
> ui/compositor/compositor.cc:405: base::TimeDelta interval) {
> On 2016/08/24 at 21:23:58, piman wrote:
> > This is only called on mac, by BrowserCompositorMac::UpdateVSyncParameters,
> right?
> > In particular, BrowserCompositorOutputSurface skips it and goes directly to
> the BeginFrameSource and the VSyncManager.
> > 
> > There doesn't seem like there's a good way to make it more consistent, but
it
> would be nice to document this fact.
> 
> Do you want more than the comment I added in compositor.h?

Oh, LGTM, somehow I blanked out the comment.

> And yeah, I felt sad about adding this at the time, but I couldn't come up
with
> a better way to route this.  This isn't a great alternate route, but it's not
> that terrible either.  One possibility is that I could go Compositor ->
> GpuProcessTransportFactory -> Display -> BrowserCompositorOutputSurface if I
> made Display use BrowserCompositorOutputSurface instead of just plain
> OutputSurface, but I'm not sure if I like Display knowing that.

Yeah, I wouldn't like that either.
I think one of the roots of the issue is that VSyncManager doesn't really fit
into ui::Compositor, especially since ui::Compositor is not involved anymore at
all in scheduling decision. It was a convenient place at the time it was added
(IIRC ash is what calls SetAuthoritativeVSyncInterval, but ash doesn't know
about content).
Maybe it could be simpler if the VSyncManager was owned by GPTF and exposed via
CompositorFactory. Though the way those parameters are routed on Mac are a bit
odd. Dunno.

[enne (OOO), 2016-08-24 22:23:48]

On 2016/08/24 at 22:18:49, piman wrote:

> I think one of the roots of the issue is that VSyncManager doesn't really fit
into ui::Compositor, especially since ui::Compositor is not involved anymore at
all in scheduling decision. It was a convenient place at the time it was added
(IIRC ash is what calls SetAuthoritativeVSyncInterval, but ash doesn't know
about content).
> Maybe it could be simpler if the VSyncManager was owned by GPTF and exposed
via CompositorFactory. Though the way those parameters are routed on Mac are a
bit odd. Dunno.

Thanks.  These are good thoughts.  Definitely agreed that VSyncManager no longer
fits into ui::Compositor, but ui::Compositor is definitely on the plumbing route
for some vsync parameters still.  I'll think about hoisting the vsync manager up
to GPTF and see what that looks like.  ui::Compositor already forwards the
authoritative vsync interval to the context factory, so keeping the vsync
manager in GPTF wouldn't be bad on that front either.  At that point, maybe
SyntheticBeginFrameSource should just grow vsync parameter observers instead of
needing a separate class to track vsync parameters and authoritative values too.

[enne (OOO), 2016-08-24 22:23:58]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2016-08-24 22:24:00]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-08-24 22:25:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 22:58:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-24 23:00:41]

Description was changed from

==========
Use vsync manager regardless of begin frame settings

There was some hope that the ui::CompositorVSyncManager could be removed
once --enable-begin-frame-scheduling was turned on.  Unfortunately, the
vsync manager is used by two additional systems that can't be easily
removed: DelegatedFrameHost::AttemptFrameSubscriberCapture as well as
components/exo/wayland/server.cc.  The latter may go away eventually.
If so, then the frame capture could then be changed to more simple
polling as needed instead of vsync manager and observer system that
needs to be plumbed everywhere.

In the medium term, the vsync manager needs to live on in all paths.
This means that BrowserCompositorOutputSurface and ui::Compositor always
need to update the vsync manager with changes.  Conditionals for
whether or not to send vsync information to renderers just get moved
down the pipeline from the output surface to DelegatedFrameHost.
==========

to

==========
Use vsync manager regardless of begin frame settings

There was some hope that the ui::CompositorVSyncManager could be removed
once --enable-begin-frame-scheduling was turned on.  Unfortunately, the
vsync manager is used by two additional systems that can't be easily
removed: DelegatedFrameHost::AttemptFrameSubscriberCapture as well as
components/exo/wayland/server.cc.  The latter may go away eventually.
If so, then the frame capture could then be changed to more simple
polling as needed instead of vsync manager and observer system that
needs to be plumbed everywhere.

In the medium term, the vsync manager needs to live on in all paths.
This means that BrowserCompositorOutputSurface and ui::Compositor always
need to update the vsync manager with changes.  Conditionals for
whether or not to send vsync information to renderers just get moved
down the pipeline from the output surface to DelegatedFrameHost.

Committed: https://crrev.com/0875b32a9ef74d66951b77930ca46f1a3bf08043
Cr-Commit-Position: refs/heads/master@{#414186}
==========

[commit-bot: I haz the power, 2016-08-24 23:00:42]

Patchset 4 (id:??) landed as
https://crrev.com/0875b32a9ef74d66951b77930ca46f1a3bf08043
Cr-Commit-Position: refs/heads/master@{#414186}