[exo] Clean up BeginFrame distribution & add acks in Surface.

[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call
to DidFinishFrame(), and passes on the ack in submitted CompositorFrames.

Passing on of no-damage ack through MojoCompositorFrameSink remains TODO.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
Review-Url: https://codereview.chromium.org/2724953007
Cr-Commit-Position: refs/heads/master@{#457038}
Committed: https://chromium.googlesource.com/chromium/src/+/599d86bbcfeb41cd4d48f73d85f62a47918be0f1

[Eric Seckler, 2017-03-02 12:23:05]

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

[commit-bot: I haz the power, 2017-03-02 12:23:23]

Dry run: 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, 2017-03-02 12:44:55]

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

[commit-bot: I haz the power, 2017-03-02 12:44:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Eric Seckler, 2017-03-02 14:02:25]

Description was changed from

==========
[exo] Clean up BeginFrame distribution & add acks in Surface/CFSHolder.

BUG=697086
==========

to

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

Passing on through MojoCompositorFrameSink still TODO.

BUG=697086
==========

[Eric Seckler, 2017-03-02 14:19:49]

Description was changed from

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

Passing on through MojoCompositorFrameSink still TODO.

BUG=697086
==========

to

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

Passing on through MojoCompositorFrameSink still TODO.

BUG=697086, 646774
==========

[Eric Seckler, 2017-03-02 14:29:22]

Patchset #1 (id:1) has been deleted

[Eric Seckler, 2017-03-02 14:29:30]

Patchset #1 (id:20001) has been deleted

[Eric Seckler, 2017-03-02 14:29:48]

Patchset #1 (id:40001) has been deleted

[Eric Seckler, 2017-03-02 15:30:19]

Description was changed from

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

Passing on through MojoCompositorFrameSink still TODO.

BUG=697086, 646774
==========

to

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

Passing on of no-damage ack through MojoCompositorFrameSink still TODO.

BUG=697086, 646774
==========

[Eric Seckler, 2017-03-02 15:31:46]

Description was changed from

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

Passing on of no-damage ack through MojoCompositorFrameSink still TODO.

BUG=697086, 646774
==========

to

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call to
DidFinishFrame() and passes on the ack in submitted CompositorFrames, too.

Passing on of no-damage ack through MojoCompositorFrameSink remains a TODO.

BUG=697086, 646774
==========

[Eric Seckler, 2017-03-02 15:54:36]

Patchset #2 (id:80001) has been deleted

[Eric Seckler, 2017-03-02 16:04:32]

Patchset #2 (id:100001) has been deleted

[Eric Seckler, 2017-03-02 16:26:43]

Patchset #2 (id:120001) has been deleted

[Eric Seckler, 2017-03-02 17:05:56]

Patchset #2 (id:140001) has been deleted

[Eric Seckler, 2017-03-07 12:36:31]

Patchset #2 (id:160001) has been deleted

[Eric Seckler, 2017-03-07 13:59:07]

Description was changed from

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call to
DidFinishFrame() and passes on the ack in submitted CompositorFrames, too.

Passing on of no-damage ack through MojoCompositorFrameSink remains a TODO.

BUG=697086, 646774
==========

to

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call to
DidFinishFrame(), and passes on the ack in submitted CompositorFrames.

Passing on of no-damage ack through MojoCompositorFrameSink remains a TODO.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
==========

[Eric Seckler, 2017-03-07 13:59:19]

Description was changed from

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call to
DidFinishFrame(), and passes on the ack in submitted CompositorFrames.

Passing on of no-damage ack through MojoCompositorFrameSink remains a TODO.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
==========

to

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call to
DidFinishFrame(), and passes on the ack in submitted CompositorFrames.

Passing on of no-damage ack through MojoCompositorFrameSink remains a TODO.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
==========

[Eric Seckler, 2017-03-07 14:01:58]

eseckler@chromium.org changed reviewers:
+ fsamuel@chromium.org, reveman@chromium.org

[Eric Seckler, 2017-03-07 14:01:59]

David, PTAL - let me know if you've got any questions :)

https://codereview.chromium.org/2724953007/diff/180001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2724953007/diff/180001/components/exo/surface...
components/exo/surface.cc:438: begin_frame_source_->DidFinishFrame(this,
current_begin_frame_ack_);
reveman@: Since I'm not very familiar with wayland, I'm wondering if this is the
right (and only) place that we should send the ack.

Generally, an observer should send an ack once it has completed processing a
BeginFrame that was sent to it. Many observers have to do this in two places:
(a) after "drawing" (submitting a CompositorFrame in this case) and (b) if they
decide not to draw during a BeginFrame (for whatever reason).

At the moment, the exo::Surface is acknowledging after Commit(), which I assume
covers (a). From what I can tell, the common behavior of wayland clients is to
request a frame callback, do some work when the callback is executed during the
next BeginFrame, and then perform a commit. I'm also guessing that the commit
can happen asynchronous to the frame callback execution?

Is there some way for the clients to tell the surface that they decided not to
perform a commit after the frame callback is executed, which would cover (b)? Or
is that not common?

[Eric Seckler, 2017-03-07 14:08:52]

Description was changed from

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call to
DidFinishFrame(), and passes on the ack in submitted CompositorFrames.

Passing on of no-damage ack through MojoCompositorFrameSink remains a TODO.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
==========

to

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call
to DidFinishFrame(), and passes on the ack in submitted CompositorFrames.

Passing on of no-damage ack through MojoCompositorFrameSink remains a
TODO.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
==========

[Eric Seckler, 2017-03-07 14:09:08]

Description was changed from

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call
to DidFinishFrame(), and passes on the ack in submitted CompositorFrames.

Passing on of no-damage ack through MojoCompositorFrameSink remains a
TODO.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
==========

to

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call
to DidFinishFrame(), and passes on the ack in submitted CompositorFrames.

Passing on of no-damage ack through MojoCompositorFrameSink remains TODO.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
==========

[reveman, 2017-03-07 14:16:21]

https://codereview.chromium.org/2724953007/diff/180001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2724953007/diff/180001/components/exo/surface...
components/exo/surface.cc:438: begin_frame_source_->DidFinishFrame(this,
current_begin_frame_ack_);
On 2017/03/07 at 14:01:59, Eric Seckler wrote:
> reveman@: Since I'm not very familiar with wayland, I'm wondering if this is
the right (and only) place that we should send the ack.
> 
> Generally, an observer should send an ack once it has completed processing a
BeginFrame that was sent to it. Many observers have to do this in two places:
(a) after "drawing" (submitting a CompositorFrame in this case) and (b) if they
decide not to draw during a BeginFrame (for whatever reason).
> 
> At the moment, the exo::Surface is acknowledging after Commit(), which I
assume covers (a). From what I can tell, the common behavior of wayland clients
is to request a frame callback, do some work when the callback is executed
during the next BeginFrame, and then perform a commit. I'm also guessing that
the commit can happen asynchronous to the frame callback execution?

Yes, all wayland protocol is async. 

> 
> Is there some way for the clients to tell the surface that they decided not to
perform a commit after the frame callback is executed, which would cover (b)? Or
is that not common?

No, there's no way to tell the compositor that the client will not to perform a
commit after the frame callback. It's expected that the client doesn't request a
callback unless needed. However, we can't trust that the client will commit even
if it requests this callback. How are we handling a failure to respond to a
begin frame by a renderer? Same will likely apply to wayland clients as both are
external clients that can be malicious and misbehave.

[Eric Seckler, 2017-03-07 14:36:01]

On 2017/03/07 14:16:21, reveman wrote:
> > Is there some way for the clients to tell the surface that they decided not
to
> perform a commit after the frame callback is executed, which would cover (b)?
Or
> is that not common?
> 
> No, there's no way to tell the compositor that the client will not to perform
a
> commit after the frame callback. It's expected that the client doesn't request
a
> callback unless needed. However, we can't trust that the client will commit
even
> if it requests this callback. How are we handling a failure to respond to a
> begin frame by a renderer? Same will likely apply to wayland clients as both
are
> external clients that can be malicious and misbehave.

We're planning to use the acknowledgements in the DisplayScheduler to decide
whether we can draw&swap before the BeginFrame's deadline - as a sort of "early"
draw.

If a wayland client (or renderer for that matter) doesn't respond to a
BeginFrame, then the normal deadline will still kick in and trigger the draw. It
just means that the early-draw optimization won't work if a wayland client
requested a frame callback but then doesn't actually commit (because the
DisplayScheduler will still wait for the exo::Surface's ack until the deadline).
Given that there's no way for the client to signal this, I'd say this behavior
would be fine. WDYT?

[reveman, 2017-03-07 15:46:45]

On 2017/03/07 at 14:36:01, eseckler wrote:
> On 2017/03/07 14:16:21, reveman wrote:
> > > Is there some way for the clients to tell the surface that they decided
not to
> > perform a commit after the frame callback is executed, which would cover
(b)? Or
> > is that not common?
> > 
> > No, there's no way to tell the compositor that the client will not to
perform a
> > commit after the frame callback. It's expected that the client doesn't
request a
> > callback unless needed. However, we can't trust that the client will commit
even
> > if it requests this callback. How are we handling a failure to respond to a
> > begin frame by a renderer? Same will likely apply to wayland clients as both
are
> > external clients that can be malicious and misbehave.
> 
> We're planning to use the acknowledgements in the DisplayScheduler to decide
whether we can draw&swap before the BeginFrame's deadline - as a sort of "early"
draw.
> 
> If a wayland client (or renderer for that matter) doesn't respond to a
BeginFrame, then the normal deadline will still kick in and trigger the draw. It
just means that the early-draw optimization won't work if a wayland client
requested a frame callback but then doesn't actually commit (because the
DisplayScheduler will still wait for the exo::Surface's ack until the deadline).
Given that there's no way for the client to signal this, I'd say this behavior
would be fine. WDYT?

Yes, all properly working wayland clients will produce a new frame as a result
of receiving the frame callback. The new begin frame behavior SGTM.

[Eric Seckler, 2017-03-10 16:16:06]

On 2017/03/07 15:46:45, reveman wrote:
> On 2017/03/07 at 14:36:01, eseckler wrote:
> > On 2017/03/07 14:16:21, reveman wrote:
> > > > Is there some way for the clients to tell the surface that they decided
> not to
> > > perform a commit after the frame callback is executed, which would cover
> (b)? Or
> > > is that not common?
> > > 
> > > No, there's no way to tell the compositor that the client will not to
> perform a
> > > commit after the frame callback. It's expected that the client doesn't
> request a
> > > callback unless needed. However, we can't trust that the client will
commit
> even
> > > if it requests this callback. How are we handling a failure to respond to
a
> > > begin frame by a renderer? Same will likely apply to wayland clients as
both
> are
> > > external clients that can be malicious and misbehave.
> > 
> > We're planning to use the acknowledgements in the DisplayScheduler to decide
> whether we can draw&swap before the BeginFrame's deadline - as a sort of
"early"
> draw.
> > 
> > If a wayland client (or renderer for that matter) doesn't respond to a
> BeginFrame, then the normal deadline will still kick in and trigger the draw.
It
> just means that the early-draw optimization won't work if a wayland client
> requested a frame callback but then doesn't actually commit (because the
> DisplayScheduler will still wait for the exo::Surface's ack until the
deadline).
> Given that there's no way for the client to signal this, I'd say this behavior
> would be fine. WDYT?
> 
> Yes, all properly working wayland clients will produce a new frame as a result
> of receiving the frame callback. The new begin frame behavior SGTM.

Sounds good. The patch is ready for review then :)

[reveman, 2017-03-13 12:13:35]

lgtm % nit

if you can verify that this works correctly with the wayland_rects_client in
exo/wayland/clients that would be great

https://codereview.chromium.org/2724953007/diff/180001/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/2724953007/diff/180001/components/exo/surface...
components/exo/surface.h:399: bool begin_frame_observer_added_ = false;
nit: s/begin_frame_observer_added_/needs_begin_frame_| as adding the observer is
no longer something we do once

[Eric Seckler, 2017-03-14 09:27:14]

On 2017/03/13 12:13:35, reveman wrote:
> if you can verify that this works correctly with the wayland_rects_client in
> exo/wayland/clients that would be great

I'm not sure how to do that (and I also don't have a build environment setup for
chromeos/wayland)...

Is this not exercised by tests on the bots?

https://codereview.chromium.org/2724953007/diff/180001/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/2724953007/diff/180001/components/exo/surface...
components/exo/surface.h:399: bool begin_frame_observer_added_ = false;
On 2017/03/13 12:13:35, reveman wrote:
> nit: s/begin_frame_observer_added_/needs_begin_frame_| as adding the observer
is
> no longer something we do once

Done.

[Eric Seckler, 2017-03-14 09:27:24]

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

[commit-bot: I haz the power, 2017-03-14 09:27:42]

Dry run: 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, 2017-03-14 10:37:28]

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

[commit-bot: I haz the power, 2017-03-14 10:37:29]

Dry run: This issue passed the CQ dry run.

[reveman, 2017-03-14 12:29:25]

On 2017/03/14 at 09:27:14, eseckler wrote:
> On 2017/03/13 12:13:35, reveman wrote:
> > if you can verify that this works correctly with the wayland_rects_client in
> > exo/wayland/clients that would be great
> 
> I'm not sure how to do that (and I also don't have a build environment setup
for chromeos/wayland)...
> 
> Is this not exercised by tests on the bots?

Only the exo unit tests are exercised by bots. If we want to be more confident
that this is not causing issues with real clients I recommend also running the
rects client. It's easy to do on a workstation. Just build with gn args
'target_os = "chromeos"'. Run chrome with --enable-wayland-server and then build
and run the client with 'ninja -C out/Debug wayland_rects_client' and
'./out/Debug/wayland_rects_client'.

> 
>
https://codereview.chromium.org/2724953007/diff/180001/components/exo/surface.h
> File components/exo/surface.h (right):
> 
>
https://codereview.chromium.org/2724953007/diff/180001/components/exo/surface...
> components/exo/surface.h:399: bool begin_frame_observer_added_ = false;
> On 2017/03/13 12:13:35, reveman wrote:
> > nit: s/begin_frame_observer_added_/needs_begin_frame_| as adding the
observer is
> > no longer something we do once
> 
> Done.

[Eric Seckler, 2017-03-15 08:55:23]

On 2017/03/14 12:29:25, reveman wrote:
> On 2017/03/14 at 09:27:14, eseckler wrote:
> > On 2017/03/13 12:13:35, reveman wrote:
> > > if you can verify that this works correctly with the wayland_rects_client
in
> > > exo/wayland/clients that would be great
> > 
> > I'm not sure how to do that (and I also don't have a build environment setup
> for chromeos/wayland)...
> > 
> > Is this not exercised by tests on the bots?
> 
> Only the exo unit tests are exercised by bots. If we want to be more confident
> that this is not causing issues with real clients I recommend also running the
> rects client. It's easy to do on a workstation. Just build with gn args
> 'target_os = "chromeos"'. Run chrome with --enable-wayland-server and then
build
> and run the client with 'ninja -C out/Debug wayland_rects_client' and
> './out/Debug/wayland_rects_client'.

Thanks, the client seems to work just fine with the change. Frames are produced
smoothly :)

[Eric Seckler, 2017-03-15 08:56:02]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-03-15 08:56:03]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2724953007/#ps200001
(title: "rename instance var.")

[commit-bot: I haz the power, 2017-03-15 08:56:54]

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, 2017-03-15 09:02:59]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489568162704130,
"parent_rev": "33d538d5169ba8ff274c6cccc9985679bb75746a", "commit_rev":
"599d86bbcfeb41cd4d48f73d85f62a47918be0f1"}

[commit-bot: I haz the power, 2017-03-15 09:03:43]

Description was changed from

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call
to DidFinishFrame(), and passes on the ack in submitted CompositorFrames.

Passing on of no-damage ack through MojoCompositorFrameSink remains TODO.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774
==========

to

==========
[exo] Clean up Surface BeginFrame distribution & add acks in Surface.

This refactors exo::Surface to use a BeginFrameSource again, adds a call
to DidFinishFrame(), and passes on the ack in submitted CompositorFrames.

Passing on of no-damage ack through MojoCompositorFrameSink remains TODO.

This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks

BUG=697086, 646774

Review-Url: https://codereview.chromium.org/2724953007
Cr-Commit-Position: refs/heads/master@{#457038}
Committed:
https://chromium.googlesource.com/chromium/src/+/599d86bbcfeb41cd4d48f73d85f6...
==========

[commit-bot: I haz the power, 2017-03-15 09:03:45]

Committed patchset #3 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/599d86bbcfeb41cd4d48f73d85f6...