[cc] Add and use BeginFrameAck for DidFinishFrame.

[cc] Add and use BeginFrameAck for DidFinishFrame.

Adds BeginFrameAck struct and updates the signature and callsites of
BeginFrameSource::DidFinishFrame(). This is split off from
https://codereview.chromium.org/2527283003/.

Observers will be required to use BeginFrameAcks to indicate the
result of a BeginFrame message. The acknowledgments will be used by the
DisplayScheduler to determine when all updates for a BeginFrame have
been received and trigger an early deadline.

A BeginFrame acknowledgment indicates:
1) completion of a specific BeginFrame by the observer,
2) whether or not the observer produced updates, and
3) the oldest frame that was incorporated into the last update from the
observer.

2) and 3) are in preparation for DevTool's BeginFrameControl, see
http://bit.ly/bfc-v1 and https://codereview.chromium.org/2411793008/. 

Follow-up patches will add:
- correct calculation of |latest_confirmed_frame| in
  Scheduler/DisplayScheduler.
- missing DidFinishFrame() calls in observers
- integration of acks into CompositorFrame submission (to allow ack
  propagation from services / renderer compositor)
- modification of DisplayScheduler to rely on BeginFrameAcks.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2591013004
Cr-Commit-Position: refs/heads/master@{#441960}
Committed: https://chromium.googlesource.com/chromium/src/+/05dc9ef49954de90fd62d5a9b7cb60af7b14ff86

[Eric Seckler, 2016-12-21 14:58:16]

Description was changed from

==========
[cc] Add and use BeginFrameAck for DidFinishFrame.

BUG=
==========

to

==========
[cc] Add and use BeginFrameAck for DidFinishFrame.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2016-12-21 15:16:14]

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

[commit-bot: I haz the power, 2016-12-21 15:16: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, 2016-12-21 15:18:57]

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

[commit-bot: I haz the power, 2016-12-21 15:18:58]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Eric Seckler, 2016-12-21 15:36:23]

Patchset #1 (id:1) has been deleted

[Eric Seckler, 2016-12-21 15:36:29]

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

[commit-bot: I haz the power, 2016-12-21 15:36:53]

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

[Eric Seckler, 2016-12-21 15:52:54]

Description was changed from

==========
[cc] Add and use BeginFrameAck for DidFinishFrame.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Add and use BeginFrameAck for DidFinishFrame.

Adds BeginFrameAck struct and updates the signature and callsites of
BeginFrameSource::DidFinishFrame(). This is split off from
https://codereview.chromium.org/2527283003/.

Observers will be required to use BeginFrameAcks to indicate the
result of a BeginFrame message. The acknowledgments will be used by the
DisplayScheduler to determine when all updates for a BeginFrame have
been received and trigger an early deadline.

A BeginFrame acknowledgment indicates:
1) completion of a specific BeginFrame by the observer,
2) whether or not the observer produced updates, and
3) the oldest frame that was incorporated into the last update from the
observer.

2) and 3) are in preparation for DevTool's BeginFrameControl, see
http://bit.ly/bfc-v1 and https://codereview.chromium.org/2411793008/. 

Follow-up patches will add:
- correct calculation of |latest_confirmed_frame| in
  Scheduler/DisplayScheduler.
- missing DidFinishFrame() calls in observers
- integration of acks into CompositorFrame submission (to allow ack
  propagation from services / renderer compositor)
- modification of DisplayScheduler to rely on BeginFrameAcks.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2016-12-21 15:56:29]

eseckler@chromium.org changed reviewers:
+ brianderson@chromium.org

[Eric Seckler, 2016-12-21 15:56:30]

sequence numbers are only waiting on owners now - sending the next part :)

[commit-bot: I haz the power, 2016-12-21 16:23:30]

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

[commit-bot: I haz the power, 2016-12-21 16:23:31]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Sami, 2016-12-21 16:33:33]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2016-12-21 16:33:33]

Looks great! Non-owner lgtm.

[brianderson, 2016-12-21 20:15:06]

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:127: uint64_t source_id;
uint32_t here or uint64_t elsewhere. Don't care which, as long as they are
consistent.

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:157: uint64_t latest_confirmed_frame;
If this is a sequence number, would it be clearer if it had "sequence_number" in
the name?

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:160: uint32_t remaining_frames;
Is this useful for anything anymore? If not, feel free to remove as part of this
patch or a separate one.

I'm not sure we even use it, currently. A quick code search reveals all existing
hard coded calls to DidFinishFrame have a remaining_frames of 0:
https://cs.chromium.org/search/?q=DidFinishFrame&sq=package:chromium&type=cs

Now that this patch adds an ack, I think the BeginFrameSource will have a better
idea of how many BeginFrames are outstanding anyway, since there could be some
in flight that aren't yet visible to the Observer.

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:164: bool has_damage;
Even if there isn't damage, the compositor might request a draw for a readback.
A better name for this might be has_update.

https://codereview.chromium.org/2591013004/diff/20001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_external_begin_frame_source.h (right):

https://codereview.chromium.org/2591013004/diff/20001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.h:46: const
cc::BeginFrameAck& ack) override {}
Are you planning to implement this in a followup patch?

[Eric Seckler, 2016-12-22 11:10:31]

Patchset #2 (id:40001) has been deleted

[Eric Seckler, 2016-12-22 11:11:49]

Turns out that the DisplaySchedulerTests were a little buggy b/c they didn't
provide missed BeginFrames to the DisplayScheduler and didn't advance time. I
fixed this here since this showed up with sequence numbers - and added a DCHECK
to DisplayScheduler::OnBeginFrameDeadline(). But if you'd prefer, I'll move
those changes into a separate patch.

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:127: uint64_t source_id;
On 2016/12/21 20:15:06, brianderson wrote:
> uint32_t here or uint64_t elsewhere. Don't care which, as long as they are
> consistent.

thanks, this should be uint32_t :)

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:157: uint64_t latest_confirmed_frame;
On 2016/12/21 20:15:06, brianderson wrote:
> If this is a sequence number, would it be clearer if it had "sequence_number"
in
> the name?

I guess so :) Changed to "latest_confirmed_sequence_number".

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:160: uint32_t remaining_frames;
On 2016/12/21 20:15:06, brianderson wrote:
> Is this useful for anything anymore? If not, feel free to remove as part of
this
> patch or a separate one.
> 
> I'm not sure we even use it, currently. A quick code search reveals all
existing
> hard coded calls to DidFinishFrame have a remaining_frames of 0:
> https://cs.chromium.org/search/?q=DidFinishFrame&sq=package:chromium&type=cs
> 
> Now that this patch adds an ack, I think the BeginFrameSource will have a
better
> idea of how many BeginFrames are outstanding anyway, since there could be some
> in flight that aren't yet visible to the Observer.

Yeah, I think we can get rid of it. Let's do that separately though, since it'll
require changes to BackToBackBFS and its uses, particularly in tests.

Added a TODO here.

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:164: bool has_damage;
On 2016/12/21 20:15:06, brianderson wrote:
> Even if there isn't damage, the compositor might request a draw for a
readback.
> A better name for this might be has_update.

Hehe, that was the field's original name :)

I think what we're referring to is that the observer has caused damage visible
to its owner. Thinking of the scenario you describe, wouldn't the draw cause
damage on whatever is drawn on? (Irrespective of whether the draw itself was
caused by damage or not.) But maybe damage is a term specific to stuff happening
before the draw (surfaces)?

Btw, we changed it to "has_damage" from "has_updates" to clarify that this
references the "out-of-band" side effect that the observer may have caused
before sending the ack (rather than the ack itself). Another reason for using
"damage" was picking a term that distinguishes this sort of "resulting update"
from the "pending updates" mentioned in the description of
|latest_confirmed_frame| above.

... would "did_draw" maybe capture this better? or is that too specific in a
different way? :)

https://codereview.chromium.org/2591013004/diff/20001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_external_begin_frame_source.h (right):

https://codereview.chromium.org/2591013004/diff/20001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.h:46: const
cc::BeginFrameAck& ack) override {}
On 2016/12/21 20:15:06, brianderson wrote:
> Are you planning to implement this in a followup patch?

Yup, together with ack-tracking within ExternalBFS. Added a TODO.

[Eric Seckler, 2016-12-22 11:12:01]

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

[commit-bot: I haz the power, 2016-12-22 11:12:08]

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, 2016-12-22 13:09:49]

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

[commit-bot: I haz the power, 2016-12-22 13:09:50]

Dry run: This issue passed the CQ dry run.

[brianderson, 2017-01-03 19:30:17]

lgtm, after now_src_.Advance is explicit in DisplaySchedulerTest.

The DisplaySchedulerTests fixes are related to this patch, so don't worry about
splitting them out.

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:160: uint32_t remaining_frames;
On 2016/12/22 11:11:48, Eric Seckler wrote:
> On 2016/12/21 20:15:06, brianderson wrote:
> > Is this useful for anything anymore? If not, feel free to remove as part of
> this
> > patch or a separate one.
> > 
> > I'm not sure we even use it, currently. A quick code search reveals all
> existing
> > hard coded calls to DidFinishFrame have a remaining_frames of 0:
> > https://cs.chromium.org/search/?q=DidFinishFrame&sq=package:chromium&type=cs
> > 
> > Now that this patch adds an ack, I think the BeginFrameSource will have a
> better
> > idea of how many BeginFrames are outstanding anyway, since there could be
some
> > in flight that aren't yet visible to the Observer.
> 
> Yeah, I think we can get rid of it. Let's do that separately though, since
it'll
> require changes to BackToBackBFS and its uses, particularly in tests.
> 
> Added a TODO here.

sg.

https://codereview.chromium.org/2591013004/diff/20001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:164: bool has_damage;
On 2016/12/22 11:11:48, Eric Seckler wrote:
> On 2016/12/21 20:15:06, brianderson wrote:
> > Even if there isn't damage, the compositor might request a draw for a
> readback.
> > A better name for this might be has_update.
> 
> Hehe, that was the field's original name :)
> 
> I think what we're referring to is that the observer has caused damage visible
> to its owner. Thinking of the scenario you describe, wouldn't the draw cause
> damage on whatever is drawn on? (Irrespective of whether the draw itself was
> caused by damage or not.) But maybe damage is a term specific to stuff
happening
> before the draw (surfaces)?
> 
> Btw, we changed it to "has_damage" from "has_updates" to clarify that this
> references the "out-of-band" side effect that the observer may have caused
> before sending the ack (rather than the ack itself). Another reason for using
> "damage" was picking a term that distinguishes this sort of "resulting update"
> from the "pending updates" mentioned in the description of
> |latest_confirmed_frame| above.
> 
> ... would "did_draw" maybe capture this better? or is that too specific in a
> different way? :)

I see. Don't want to bikeshed this, has_damage is good. The comment for the
field explains enough.

https://codereview.chromium.org/2591013004/diff/60001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/2591013004/diff/60001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler_unittest.cc:99:
now_src_.Advance(base::TimeDelta::FromMicroseconds(10000));
To help test readability, can you make it explicit from the call sites that this
method will advance time? Either change the name of this method to
AdvanceTimeAndBeginFrameForTest or manually advance the time before calling
BeginFrameForTest.

[Eric Seckler, 2017-01-04 10:14:48]

eseckler@chromium.org changed reviewers:
+ dcheng@chromium.org, enne@chromium.org, twellington@chromium.org

[Eric Seckler, 2017-01-04 10:14:49]

Thanks Brian!

Adding more folks for ownership:
 +enne@ for content/renderer/gpu
 +twellington@ for ui/android
 +dcheng@ for cc/ipc security

Thank you :)

https://codereview.chromium.org/2591013004/diff/60001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/2591013004/diff/60001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler_unittest.cc:99:
now_src_.Advance(base::TimeDelta::FromMicroseconds(10000));
On 2017/01/03 19:30:16, brianderson wrote:
> To help test readability, can you make it explicit from the call sites that
this
> method will advance time? Either change the name of this method to
> AdvanceTimeAndBeginFrameForTest or manually advance the time before calling
> BeginFrameForTest.

Done.

[Theresa, 2017-01-04 16:07:12]

ui/android lgtm

[enne (OOO), 2017-01-04 22:12:51]

lgtm

[dcheng, 2017-01-05 06:49:54]

As far as I can tell, we're not actually sending these across IPC yet, right?

Can we move the IPC serialization changes to the patch that adds the browser
plumbing? Thanks.

https://codereview.chromium.org/2591013004/diff/80001/cc/ipc/begin_frame_args...
File cc/ipc/begin_frame_args_struct_traits.h (right):

https://codereview.chromium.org/2591013004/diff/80001/cc/ipc/begin_frame_args...
cc/ipc/begin_frame_args_struct_traits.h:83: cc::BeginFrameAck* out) {
Nit: Read() methods should generally out-of-lined, can we do that here?

[Eric Seckler, 2017-01-06 16:06:50]

Patchset #4 (id:100001) has been deleted

[Eric Seckler, 2017-01-06 16:08:51]

On 2017/01/05 06:49:54, dcheng wrote:
> As far as I can tell, we're not actually sending these across IPC yet, right?
> 
> Can we move the IPC serialization changes to the patch that adds the browser
> plumbing? Thanks.
You're right, I guess that makes sense. I'll add them with the later changes.

https://codereview.chromium.org/2591013004/diff/80001/cc/ipc/begin_frame_args...
File cc/ipc/begin_frame_args_struct_traits.h (right):

https://codereview.chromium.org/2591013004/diff/80001/cc/ipc/begin_frame_args...
cc/ipc/begin_frame_args_struct_traits.h:83: cc::BeginFrameAck* out) {
On 2017/01/05 06:49:54, dcheng wrote:
> Nit: Read() methods should generally out-of-lined, can we do that here?

will do when we add it back in a later CL :)

[Eric Seckler, 2017-01-06 16:09:12]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-01-06 16:09:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, brianderson@chromium.org, twellington@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2591013004/#ps120001
(title: "remove ipc struct traits for BeginFrameAck.")

[commit-bot: I haz the power, 2017-01-06 16:09:34]

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-01-06 17:22:51]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483718952757270,
"parent_rev": "3240715025d4a6f689a9c56ef5fdf9a48b0a20e1", "commit_rev":
"05dc9ef49954de90fd62d5a9b7cb60af7b14ff86"}

[commit-bot: I haz the power, 2017-01-06 17:23:27]

Description was changed from

==========
[cc] Add and use BeginFrameAck for DidFinishFrame.

Adds BeginFrameAck struct and updates the signature and callsites of
BeginFrameSource::DidFinishFrame(). This is split off from
https://codereview.chromium.org/2527283003/.

Observers will be required to use BeginFrameAcks to indicate the
result of a BeginFrame message. The acknowledgments will be used by the
DisplayScheduler to determine when all updates for a BeginFrame have
been received and trigger an early deadline.

A BeginFrame acknowledgment indicates:
1) completion of a specific BeginFrame by the observer,
2) whether or not the observer produced updates, and
3) the oldest frame that was incorporated into the last update from the
observer.

2) and 3) are in preparation for DevTool's BeginFrameControl, see
http://bit.ly/bfc-v1 and https://codereview.chromium.org/2411793008/. 

Follow-up patches will add:
- correct calculation of |latest_confirmed_frame| in
  Scheduler/DisplayScheduler.
- missing DidFinishFrame() calls in observers
- integration of acks into CompositorFrame submission (to allow ack
  propagation from services / renderer compositor)
- modification of DisplayScheduler to rely on BeginFrameAcks.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Add and use BeginFrameAck for DidFinishFrame.

Adds BeginFrameAck struct and updates the signature and callsites of
BeginFrameSource::DidFinishFrame(). This is split off from
https://codereview.chromium.org/2527283003/.

Observers will be required to use BeginFrameAcks to indicate the
result of a BeginFrame message. The acknowledgments will be used by the
DisplayScheduler to determine when all updates for a BeginFrame have
been received and trigger an early deadline.

A BeginFrame acknowledgment indicates:
1) completion of a specific BeginFrame by the observer,
2) whether or not the observer produced updates, and
3) the oldest frame that was incorporated into the last update from the
observer.

2) and 3) are in preparation for DevTool's BeginFrameControl, see
http://bit.ly/bfc-v1 and https://codereview.chromium.org/2411793008/. 

Follow-up patches will add:
- correct calculation of |latest_confirmed_frame| in
  Scheduler/DisplayScheduler.
- missing DidFinishFrame() calls in observers
- integration of acks into CompositorFrame submission (to allow ack
  propagation from services / renderer compositor)
- modification of DisplayScheduler to rely on BeginFrameAcks.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2591013004
Cr-Commit-Position: refs/heads/master@{#441960}
Committed:
https://chromium.googlesource.com/chromium/src/+/05dc9ef49954de90fd62d5a9b7cb...
==========

[commit-bot: I haz the power, 2017-01-06 17:23:29]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/05dc9ef49954de90fd62d5a9b7cb...

[Eric Seckler, 2017-01-13 09:09:06]

Patchset #5 (id:140001) has been deleted