[cc] Set BeginFrame sequence numbers on CompositorFrames from Scheduler.

[cc] Set BeginFrame sequence numbers on CompositorFrames from Scheduler.

Other CompositorFrame creators will have to set these, too. But that'll
follow separately.

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

Review-Url: https://codereview.chromium.org/2712983003
Cr-Commit-Position: refs/heads/master@{#455194}
Committed: https://chromium.googlesource.com/chromium/src/+/dde665fa0a7577e619e819c13c9e4af95d8b19b1

[Eric Seckler, 2017-02-23 17:25:29]

Description was changed from

==========
[cc] Set BeginFrame sequence numbers on CompositorFrames from Scheduler.

Other CompositorFrame creators will have to set these, too. But that'll
follow separately.

BUG=646774, 401331
==========

to

==========
[cc] Set BeginFrame sequence numbers on CompositorFrames from Scheduler.

Other CompositorFrame creators will have to set these, too. But that'll
follow separately.

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

[Eric Seckler, 2017-02-23 17:28:05]

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

[commit-bot: I haz the power, 2017-02-23 17:28:23]

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

[Eric Seckler, 2017-02-23 17:30:47]

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

[Eric Seckler, 2017-02-23 17:30:48]

Seemed like the queue was draining, here's another one! :P

Thank you for the reviews, Brian :)

[commit-bot: I haz the power, 2017-02-23 17:31:04]

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

[commit-bot: I haz the power, 2017-02-23 17:31:05]

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

[Eric Seckler, 2017-02-23 17:39:14]

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

[Eric Seckler, 2017-02-23 17:39:22]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-02-23 17:39:37]

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-02-23 18:37:27]

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

[commit-bot: I haz the power, 2017-02-23 18:37:27]

Dry run: This issue passed the CQ dry run.

[brianderson, 2017-02-24 01:08:40]

https://codereview.chromium.org/2712983003/diff/20001/cc/output/compositor_fr...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2712983003/diff/20001/cc/output/compositor_fr...
cc/output/compositor_frame_metadata.h:91: BeginFrameArgs::kInvalidFrameNumber;
Can this be an instance of the BeginFrameAck struct?

https://codereview.chromium.org/2712983003/diff/20001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/2712983003/diff/20001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:147: uint64_t
LatestConfirmedBeginFrameSequenceNumberForActiveTree() const;
Can these be collapsed into a single getter called CurrentBeginFrameAck that
either takes an Ack as an output argument or returns an Ack?

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:232: uint64_t
latest_confirmed_begin_frame_sequence_number;
Can this be an instance of the BeginFrameAck struct?

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:7025: if
(current_begin_frame_args_.IsValid()) {
This if condition probably isn't needed.

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:7038:
layer_tree_host()->SetDeferCommits(true);
I think you can avoid this if you use the BeginCommitOnThread hook, which is
only called while the Main and Impl threads are "locked" to copy the
begin_main_frame_args to the impl-side if it hasn't already been copied.

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:450: BEGINFRAME_FROM_HERE, UINT32_MAX, 1,
frame_begin_time, base::TimeTicks(),
Can you make this a BeginFrameArgs constant with a meaningful name?

[Eric Seckler, 2017-02-27 11:50:32]

Thank you! PTAL :)

https://codereview.chromium.org/2712983003/diff/20001/cc/output/compositor_fr...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2712983003/diff/20001/cc/output/compositor_fr...
cc/output/compositor_frame_metadata.h:91: BeginFrameArgs::kInvalidFrameNumber;
On 2017/02/24 01:08:36, brianderson wrote:
> Can this be an instance of the BeginFrameAck struct?

Done. Was trying to avoid overhead of the unused fields (remaining_frames and
has_damage), but it does look nicer having an ACK in here, and the overhead
should be minimal. And remaining_frames should go away eventually.

https://codereview.chromium.org/2712983003/diff/20001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/2712983003/diff/20001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:147: uint64_t
LatestConfirmedBeginFrameSequenceNumberForActiveTree() const;
On 2017/02/24 01:08:36, brianderson wrote:
> Can these be collapsed into a single getter called CurrentBeginFrameAck that
> either takes an Ack as an output argument or returns an Ack?

Done.

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:232: uint64_t
latest_confirmed_begin_frame_sequence_number;
On 2017/02/24 01:08:36, brianderson wrote:
> Can this be an instance of the BeginFrameAck struct?

Done.

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:7025: if
(current_begin_frame_args_.IsValid()) {
On 2017/02/24 01:08:36, brianderson wrote:
> This if condition probably isn't needed.

Done.

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:7038:
layer_tree_host()->SetDeferCommits(true);
On 2017/02/24 01:08:37, brianderson wrote:
> I think you can avoid this if you use the BeginCommitOnThread hook, which is
> only called while the Main and Impl threads are "locked" to copy the
> begin_main_frame_args to the impl-side if it hasn't already been copied.

Not entirely sure whether I understood this correctly. I'm now requesting
another commit in BeginCommitOnThread, but still setting begin_main_frame_args
in BeginMainFrame. No need for deferring commits anymore, it seems :)

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2712983003/diff/20001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:450: BEGINFRAME_FROM_HERE, UINT32_MAX, 1,
frame_begin_time, base::TimeTicks(),
On 2017/02/24 01:08:38, brianderson wrote:
> Can you make this a BeginFrameArgs constant with a meaningful name?

Done.

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

Description was changed from

==========
[cc] Set BeginFrame sequence numbers on CompositorFrames from Scheduler.

Other CompositorFrame creators will have to set these, too. But that'll
follow separately.

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

to

==========
[cc] Set BeginFrame sequence numbers on CompositorFrames from Scheduler.

Other CompositorFrame creators will have to set these, too. But that'll
follow separately.

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

[brianderson, 2017-03-02 19:53:36]

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

https://codereview.chromium.org/2712983003/diff/40001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:54: static constexpr uint32_t kManualSourceId =
UINT32_MAX;
Can you add a comment explaining what a manual source id is?

https://codereview.chromium.org/2712983003/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2712983003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:7027: current_begin_main_frame_args_ =
args;
current_begin_main_frame_args_ is written on the main thread here and read on
the impl thread from *OnThread methods without a lock, which might result in a
corrupt view and a flaky test.

BeginCommitOnThread occurs while the main thread and impl threads are "locked",
so you can safely transfer the value to the impl thread from that hook.

[Eric Seckler, 2017-03-07 12:26:56]

PTAL :)

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

https://codereview.chromium.org/2712983003/diff/40001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:54: static constexpr uint32_t kManualSourceId =
UINT32_MAX;
On 2017/03/02 19:53:36, brianderson wrote:
> Can you add a comment explaining what a manual source id is?

Done.

https://codereview.chromium.org/2712983003/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2712983003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:7027: current_begin_main_frame_args_ =
args;
On 2017/03/02 19:53:36, brianderson wrote:
> current_begin_main_frame_args_ is written on the main thread here and read on
> the impl thread from *OnThread methods without a lock, which might result in a
> corrupt view and a flaky test.
> 
> BeginCommitOnThread occurs while the main thread and impl threads are
"locked",
> so you can safely transfer the value to the impl thread from that hook.

Ah, I see. Added a copy to impl.

[brianderson, 2017-03-07 18:06:12]

lgtm

[Eric Seckler, 2017-03-07 18:18:35]

The CQ bit was checked by eseckler@chromium.org

[commit-bot: I haz the power, 2017-03-07 18:19:58]

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-07 20:19:31]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488910715570880,
"parent_rev": "e03d6f332a76e587d802634757c3e9e74778d5aa", "commit_rev":
"dde665fa0a7577e619e819c13c9e4af95d8b19b1"}

[commit-bot: I haz the power, 2017-03-07 20:20:16]

Description was changed from

==========
[cc] Set BeginFrame sequence numbers on CompositorFrames from Scheduler.

Other CompositorFrame creators will have to set these, too. But that'll
follow separately.

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

to

==========
[cc] Set BeginFrame sequence numbers on CompositorFrames from Scheduler.

Other CompositorFrame creators will have to set these, too. But that'll
follow separately.

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

Review-Url: https://codereview.chromium.org/2712983003
Cr-Commit-Position: refs/heads/master@{#455194}
Committed:
https://chromium.googlesource.com/chromium/src/+/dde665fa0a7577e619e819c13c9e...
==========

[commit-bot: I haz the power, 2017-03-07 20:20:18]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/dde665fa0a7577e619e819c13c9e...

[chanli1, 2017-03-08 21:13:36]

On 2017/03/07 20:20:18, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:60001) as
>
https://chromium.googlesource.com/chromium/src/+/dde665fa0a7577e619e819c13c9e...

Hello,

Based on
https://findit-for-me.appspot.com/waterfall/check-flake?key=ag9zfmZpbmRpdC1mb...

The test
LayerTreeHostTestBeginFrameSequenceNumbers.RunMultiThread_DelegatingRenderer was
flaky since it's added. Could you help me and check if this change related to
the flakiness?

Thank you,
Chan Li