Allow cc::Scheduler begin frame source to be changed

Allow cc::Scheduler begin frame source to be changed

Also, add back throttling tests to check changing the source.

Depends on https://codereview.chromium.org/1765723002
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/061ea9c84412147d772b91e138413cfd2a515eab
Cr-Commit-Position: refs/heads/master@{#381779}

[enne (OOO), 2016-03-09 00:00:37]

Description was changed from

==========
Allow cc::Scheduler begin frame source to be changed

Also, add back throttling tests to check changing the source.

Depends on https://codereview.chromium.org/1765723002
==========

to

==========
Allow cc::Scheduler begin frame source to be changed

Also, add back throttling tests to check changing the source.

Depends on https://codereview.chromium.org/1765723002
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-03-09 00:03:55]

enne@chromium.org changed reviewers:
+ skyostil@chromium.org, sunnyps@chromium.org

[enne (OOO), 2016-03-09 00:03:56]

Just another incremental refactoring patch towards allowing output surfaces to
provide a begin frame source for the scheduler.

The followup is to figure out whether or not cc::Scheduler can support a null
begin frame source.  I think it can, but just need to write a few more tests.

[Sami, 2016-03-09 17:09:15]

https://codereview.chromium.org/1774323003/diff/40001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/1774323003/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:117: void
Scheduler::SetBeginFrameSource(BeginFrameSource* source) {
Not sure how often this would actually be used, but is it worth adding a check
for trying to switch to the same |source| we already have? I think
Add/RemoveObserver might involve IPC in some cases.

https://codereview.chromium.org/1774323003/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:124: begin_frame_source_->SetClientReady();
One problem here is that some begin frame sources (e.g., 
CompositorExternalBeginFrameSource) don't like it if you call SetClientReady
multiple times. Maybe we should change the API contract to allow that. Or maybe
SetClientReady could be folded into AddObserver.

https://codereview.chromium.org/1774323003/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/1774323003/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:3059: TEST_F(SchedulerTest,
SwitchFrameSourceToUnthrottled) {
General thought: it's good that you're testing all these cases but do we
actually need this level of flexibility? I can't really imagine a need for a
compositor to switch from one begin frame source to another several times within
a single frame. Maybe we could say that you are allowed to switch only when the
compositor is idle? (I'm probably missing something here.)

[enne (OOO), 2016-03-09 19:34:01]

Re: flexibility.  It's a really good question.  I think my hope is that if it
doesn't make the scheduler more complicated to support switching a begin frame
source at any point, I think it's better to just allow that rather than trying
to be more restrictive and brittle about when it's ok to change it.  Or, I'd
rather have a bunch of test complicated test cases that prove flexibility if the
implementation in the scheduler itself is simple.  :)

I think there are some cases where the begin frame source will change when the
output surface is lost.  However, I do think that there are other cases where a
window is dragged from one display to another and the begin frame source will
just change arbitrarily.  I think it'd be more complicated to wait until there
was a good time to change it than to just change it immediately.

Some of those tests cases are also the test cases I removed when I removed
throttling (it was harder to convert them in place than it was to remove,
refactor, and then add back).  So, if we ever wanted to add back runtime
changing of the begin frame source from throttled to unthrottled, then we'd
already be in a good place to do that.

[enne (OOO), 2016-03-09 19:46:10]

https://codereview.chromium.org/1774323003/diff/40001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/1774323003/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:117: void
Scheduler::SetBeginFrameSource(BeginFrameSource* source) {
On 2016/03/09 at 17:09:15, Sami wrote:
> Not sure how often this would actually be used, but is it worth adding a check
for trying to switch to the same |source| we already have? I think
Add/RemoveObserver might involve IPC in some cases.

Yeah, I think something needs to change here, as the scheduler has no idea
whether or not a source has had that called on it before.  I'll put up a patch
to address that separately.

[enne (OOO), 2016-03-09 23:58:04]

Ok, rebased on the previous patch and addressed one of those nits.

The one thing that worries me a little is the logic inside of
SetupNextBeginFrameIfNeeded.  That logic adds an observer as quickly as possible
if the state machine needs a begin frame and also only tries to remove the
observer in between frames.  I think this patch preserves the first property. 
I'm not sure what the second state is trying to do.  I don't know why it'd be
problematic to remove the observer in between frames.  The multiplexer certainly
used to do this at one level removed when changing throttling, so it can't be
that problematic.  Maybe this just a legacy comment and doesn't really apply
anymore.

[Sami, 2016-03-10 19:43:02]

On 2016/03/09 23:58:04, enne wrote:
> Ok, rebased on the previous patch and addressed one of those nits.

Thanks, lgtm.

> The one thing that worries me a little is the logic inside of
> SetupNextBeginFrameIfNeeded.  That logic adds an observer as quickly as
possible
> if the state machine needs a begin frame and also only tries to remove the
> observer in between frames.  I think this patch preserves the first property. 
> I'm not sure what the second state is trying to do.  I don't know why it'd be
> problematic to remove the observer in between frames.  The multiplexer
certainly
> used to do this at one level removed when changing throttling, so it can't be
> that problematic.  Maybe this just a legacy comment and doesn't really apply
> anymore.

I think the case that's trying to address is quickly toggling the
needs-begin-frame bit on, off and back on again within a single frame. We want
that to not result in an extra frame of latency if an in-flight BeginFrame gets
dropped on the floor because of poor timing. I *think* we have sufficient test
coverage (both unit and perf) to make sure that isn't happening so I don't think
we can break that without noticing.

[enne (OOO), 2016-03-17 18:47:22]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-17 18:47:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1774323003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1774323003/80001

[commit-bot: I haz the power, 2016-03-17 19:50:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-17 19:53:03]

Description was changed from

==========
Allow cc::Scheduler begin frame source to be changed

Also, add back throttling tests to check changing the source.

Depends on https://codereview.chromium.org/1765723002
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow cc::Scheduler begin frame source to be changed

Also, add back throttling tests to check changing the source.

Depends on https://codereview.chromium.org/1765723002
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/061ea9c84412147d772b91e138413cfd2a515eab
Cr-Commit-Position: refs/heads/master@{#381779}
==========

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

Patchset 5 (id:??) landed as
https://crrev.com/061ea9c84412147d772b91e138413cfd2a515eab
Cr-Commit-Position: refs/heads/master@{#381779}