[cc] Forward BeginFrameAcks through DirectCFS and CFSSupport.

[cc] Forward BeginFrameAcks through DirectCFS and CFSSupport.

Also updates unit tests to check that acks are indeed forwarded.

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

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

[Eric Seckler, 2017-03-01 17:05:49]

Description was changed from

==========
[cc] Forward BeginFrameAcks through DirectCFS and CFSSupport.

Also updates unit tests to check that acks are indeed forwarded.

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

to

==========
[cc] Forward BeginFrameAcks through DirectCFS and CFSSupport.

Also updates unit tests to check that acks are indeed forwarded.

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

[Eric Seckler, 2017-03-01 19:40:04]

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

[commit-bot: I haz the power, 2017-03-01 19:40:48]

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-03-01 19:47:00]

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

[Eric Seckler, 2017-03-01 19:47:01]

Any ideas for alternative testing approaches appreciated :)

[Fady Samuel, 2017-03-01 19:49:06]

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

[Fady Samuel, 2017-03-01 19:49:07]

https://codereview.chromium.org/2727553004/diff/1/cc/surfaces/compositor_fram...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2727553004/diff/1/cc/surfaces/compositor_fram...
cc/surfaces/compositor_frame_sink_support_unittest.cc:156:
surface_manager_.RegisterBeginFrameSource(begin_frame_source_.get(),
I guess cc::Display normally does this? Could you please add a comment about why
this is necessary in this unit test? Thanks!

[Alex Z., 2017-03-01 20:19:09]

lgtm. 

Am I correct in thinking that this CL could also fix the two bugs caused by my
DirectCFS refactor?
(https://bugs.chromium.org/p/chromium/issues/detail?id=696030 and
https://bugs.chromium.org/p/chromium/issues/detail?id=480361#c26)

[Eric Seckler, 2017-03-01 20:22:19]

On 2017/03/01 20:19:09, Alex Z. wrote:
> lgtm. 
> 
> Am I correct in thinking that this CL could also fix the two bugs caused by my
> DirectCFS refactor?
> (https://bugs.chromium.org/p/chromium/issues/detail?id=696030 and
> https://bugs.chromium.org/p/chromium/issues/detail?id=480361#c26)

Possibly, I guess? :)

The BackToBackBFS does use the calls to DidFinishFrame to determine when to send
the next frame, IRRC. So yeah, this could affect your bug. If you want to check,
feel free to patch this in locally and try. :)

[commit-bot: I haz the power, 2017-03-01 20:45:46]

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

[commit-bot: I haz the power, 2017-03-01 20:45:47]

Dry run: This issue passed the CQ dry run.

[Alex Z., 2017-03-01 22:55:54]

On 2017/03/01 20:22:19, Eric Seckler wrote:
> On 2017/03/01 20:19:09, Alex Z. wrote:
> > lgtm. 
> > 
> > Am I correct in thinking that this CL could also fix the two bugs caused by
my
> > DirectCFS refactor?
> > (https://bugs.chromium.org/p/chromium/issues/detail?id=696030 and
> > https://bugs.chromium.org/p/chromium/issues/detail?id=480361#c26)
> 
> Possibly, I guess? :)
> 
> The BackToBackBFS does use the calls to DidFinishFrame to determine when to
send
> the next frame, IRRC. So yeah, this could affect your bug. If you want to
check,
> feel free to patch this in locally and try. :)

I was finally able to build and run with the patch on Windows. 
All performance_browser_tests passed! :D

[Alex Z., 2017-03-01 22:56:59]

Description was changed from

==========
[cc] Forward BeginFrameAcks through DirectCFS and CFSSupport.

Also updates unit tests to check that acks are indeed forwarded.

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

to

==========
[cc] Forward BeginFrameAcks through DirectCFS and CFSSupport.

Also updates unit tests to check that acks are indeed forwarded.

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

[Fady Samuel, 2017-03-01 23:01:12]

On 2017/03/01 22:55:54, Alex Z. wrote:
> On 2017/03/01 20:22:19, Eric Seckler wrote:
> > On 2017/03/01 20:19:09, Alex Z. wrote:
> > > lgtm. 
> > > 
> > > Am I correct in thinking that this CL could also fix the two bugs caused
by
> my
> > > DirectCFS refactor?
> > > (https://bugs.chromium.org/p/chromium/issues/detail?id=696030 and
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=480361#c26)
> > 
> > Possibly, I guess? :)
> > 
> > The BackToBackBFS does use the calls to DidFinishFrame to determine when to
> send
> > the next frame, IRRC. So yeah, this could affect your bug. If you want to
> check,
> > feel free to patch this in locally and try. :)
> 
> I was finally able to build and run with the patch on Windows. 
> All performance_browser_tests passed! :D

aside: I wonder if this is a problem for exo benchmarks too?

[Eric Seckler, 2017-03-01 23:10:27]

On 2017/03/01 23:01:12, Fady Samuel wrote:
> On 2017/03/01 22:55:54, Alex Z. wrote:
> > On 2017/03/01 20:22:19, Eric Seckler wrote:
> > > On 2017/03/01 20:19:09, Alex Z. wrote:
> > > > lgtm. 
> > > > 
> > > > Am I correct in thinking that this CL could also fix the two bugs caused
> by
> > my
> > > > DirectCFS refactor?
> > > > (https://bugs.chromium.org/p/chromium/issues/detail?id=696030 and
> > > > https://bugs.chromium.org/p/chromium/issues/detail?id=480361#c26)
> > > 
> > > Possibly, I guess? :)
> > > 
> > > The BackToBackBFS does use the calls to DidFinishFrame to determine when
to
> > send
> > > the next frame, IRRC. So yeah, this could affect your bug. If you want to
> > check,
> > > feel free to patch this in locally and try. :)
> > 
> > I was finally able to build and run with the patch on Windows. 
> > All performance_browser_tests passed! :D
Fantastic! Let's get this landed soon then. I'll make the changes Fady suggested
tomorrow once back at the workstation ;)

Brian, let me know if you'd like to have a look at this too.

> 
> aside: I wonder if this is a problem for exo benchmarks too?

Not sure, but I think exo's CompositorFrameSink also currently doesn't confirm
the BeginFrames.  It's something relatively high on my list of subsequent
changes to make anyway, as part of my acknowledgement unifications. I can look
into the next steps for that tomorrow as well.

I don't think any other BeginFrameSources aside from the BackToBackBFS actually
use DidFinishFrame at the moment, so you'd probably only ever see an effect in
no-vsync mode benchmarks.

[Eric Seckler, 2017-03-02 09:41:59]

https://codereview.chromium.org/2727553004/diff/1/cc/surfaces/compositor_fram...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2727553004/diff/1/cc/surfaces/compositor_fram...
cc/surfaces/compositor_frame_sink_support_unittest.cc:156:
surface_manager_.RegisterBeginFrameSource(begin_frame_source_.get(),
On 2017/03/01 19:49:07, Fady Samuel wrote:
> I guess cc::Display normally does this? Could you please add a comment about
why
> this is necessary in this unit test? Thanks!

Correct, normally done by the display. The other thing that seems to be missing
here is registering the hierarchy of the frame sinks (which would assign the BFS
to the other frame sinks). But since it doesn't seem to be required for the
tests, I guess we can leave it out.

Added a comment.

[Fady Samuel, 2017-03-02 14:03:53]

lgtm. Since this fixes a regression, let's try to land this as soon as possible.
Thanks!

[Eric Seckler, 2017-03-02 14:11:35]

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

[Eric Seckler, 2017-03-02 14:11:36]

On 2017/03/02 14:03:53, Fady Samuel wrote:
> lgtm. Since this fixes a regression, let's try to land this as soon as
possible.
> Thanks!

Let's see if Sami's happy to stamp this, otherwise we'll wait for Brian.

[brianderson, 2017-03-02 19:30:57]

lgtm

[brianderson, 2017-03-02 19:31:01]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2017-03-02 19:31:02]

The patchset sent to the CQ was uploaded after l-g-t-m from staraz@chromium.org
Link to the patchset: https://codereview.chromium.org/2727553004/#ps20001
(title: "add a comment")

[commit-bot: I haz the power, 2017-03-02 19:31:38]

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 19:37:11]

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

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

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-03-02 20:11:42]

On 2017/03/02 19:53:16, brianderson wrote:

Working on rebasing. Brian, did you have a comment, seems there was an empty
email?

[Eric Seckler, 2017-03-02 20:24:58]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-03-02 20:24:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, staraz@chromium.org, brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2727553004/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-02 20:25:41]

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

[brianderson, 2017-03-02 20:35:47]

lgtm.

I didn't have a comment; just accidentally hit publish on the wrong code review.

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

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488486298350570,
"parent_rev": "76c172851a33d32e6eee07701d9ed8821b0e2a30", "commit_rev":
"d9bbb44814c924c58e80bb0e8b64a9a634fd2198"}

[commit-bot: I haz the power, 2017-03-02 22:13:02]

Description was changed from

==========
[cc] Forward BeginFrameAcks through DirectCFS and CFSSupport.

Also updates unit tests to check that acks are indeed forwarded.

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

to

==========
[cc] Forward BeginFrameAcks through DirectCFS and CFSSupport.

Also updates unit tests to check that acks are indeed forwarded.

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

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

[commit-bot: I haz the power, 2017-03-02 22:13:03]

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