Refactoring the way begin frame sources inside scheduler work.

Refactoring the way begin frame sources inside scheduler work.

This change;
 * Makes non-vsync aligned rendering just another begin frame source.
 * Makes it easier to add vsync/begin frame stabilisation / filtering in the future.

This CL no longer moves background ticking into the scheduler rather than the LayerTreeHostImpl, that will occur in a later CL.

BUG=345459
Committed: https://crrev.com/c34fc0b1428211293c19944db1bac41a7a7d0401
Cr-Commit-Position: refs/heads/master@{#297389}

[mithro-old, 2014-05-05 16:16:35]

Hi Brian,

This CL doesn't work yet and still needs things like tests and such but wanted
to get it in front of you before I go to much further.

Tim

[brianderson, 2014-05-05 16:50:05]

It would be really nice if we could interact with all the BeginFrame sources
with a single interface, so I like where this patch is going. Thanks for putting
this patch together.

When I was moving logic from OutputSurface to the Scheduler recently, I couldn't
figure out how to share an the interface with the unthrottled case. If we change
the interface a bit, it should be possible though!

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
cc/scheduler/frame_source.cc:90: class ThrottledFrameSource : public
FrameSource, FrameSink {
In what cases would this class be used? Is this to throttle down to 30 fps or 20
fps for example?

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
cc/scheduler/frame_source.cc:149: class BackToBackFrameSource : public
TaskRunnerFrameSource {
Would this be used as the "vsync disabled" BeginFrame source? 

In this case, one issue we'd need to solve with the interface between the
Scheduler and a FrameSource is that the Scheduler expects a stream of
BeginFrames after a single call to SetNeedsBeginFrame(true). As in,
SetNeedsBeginFrame is level triggered and not edge triggered.

The problem with a "vsync disabled" source is that, for it to behave like other
sources, we should receive an infinite stream of BeginFrames. Maybe we should
add a method to the FrameSource, in addition to the SetNeedsBeginFrame, called
ReadyToReceiveNextBeginFrame.

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
cc/scheduler/frame_source.cc:245: class DualFrameSource : public FrameSource,
public FrameSink {
Is this going to be used to switch between background and foreground ticking? I
was hoping we could use a single BeginFrame source in that case and throttle the
SetNeedsBeginFrame instead to avoid the monotonic issues.

[mithro-old, 2014-05-05 17:10:52]

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
cc/scheduler/frame_source.cc:90: class ThrottledFrameSource : public
FrameSource, FrameSink {
On 2014/05/05 16:50:05, brianderson wrote:
> In what cases would this class be used? Is this to throttle down to 30 fps or
20
> fps for example?

Yes, examples I can think of are;
 * Throttle an ExternalFrameSource (or the synthetic version) down to
30fps/15fps etc
 * Throttling the BackToBackFrameSource to a maximum fps rate.

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
cc/scheduler/frame_source.cc:149: class BackToBackFrameSource : public
TaskRunnerFrameSource {
On 2014/05/05 16:50:05, brianderson wrote:
> Would this be used as the "vsync disabled" BeginFrame source? 
> 
> In this case, one issue we'd need to solve with the interface between the
> Scheduler and a FrameSource is that the Scheduler expects a stream of
> BeginFrames after a single call to SetNeedsBeginFrame(true). As in,
> SetNeedsBeginFrame is level triggered and not edge triggered.
> 
> The problem with a "vsync disabled" source is that, for it to behave like
other
> sources, we should receive an infinite stream of BeginFrames. Maybe we should
> add a method to the FrameSource, in addition to the SetNeedsBeginFrame, called
> ReadyToReceiveNextBeginFrame.

Is there any reason we want to keep it level triggered rather then edge
triggered? (Maybe because of some sort of pipelining?)

From looking at the various code, it seems we calculate if we actually want to
keep rendering frames every frame (as an animation could finish).

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
cc/scheduler/frame_source.cc:245: class DualFrameSource : public FrameSource,
public FrameSink {
On 2014/05/05 16:50:05, brianderson wrote:
> Is this going to be used to switch between background and foreground ticking?
I
> was hoping we could use a single BeginFrame source in that case and throttle
the
> SetNeedsBeginFrame instead to avoid the monotonic issues.

Yeah. The thought here was to allow switching between background / foreground.

Using throttling has the issue that we are still getting woken up every 16ms
when we only want to do something every second (which is bad for battery).

In theory we could do something smart like the background ticking frequency be a
exact multiple of the foreground ticking and have the phase match. That gets
complicated when vsync is constantly being adjusted though.

[brianderson, 2014-05-05 17:37:49]

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
cc/scheduler/frame_source.cc:90: class ThrottledFrameSource : public
FrameSource, FrameSink {
On 2014/05/05 17:10:53, mithro wrote:
> On 2014/05/05 16:50:05, brianderson wrote:
> > In what cases would this class be used? Is this to throttle down to 30 fps
or
> 20
> > fps for example?
> 
> Yes, examples I can think of are;
>  * Throttle an ExternalFrameSource (or the synthetic version) down to
> 30fps/15fps etc
>  * Throttling the BackToBackFrameSource to a maximum fps rate.

Cool. What would the interface look like to change the rate of throttling? Would
we set a decimation factor or some kind of minimum interval?

https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
cc/scheduler/frame_source.cc:149: class BackToBackFrameSource : public
TaskRunnerFrameSource {
On 2014/05/05 17:10:53, mithro wrote:
> On 2014/05/05 16:50:05, brianderson wrote:
> > Would this be used as the "vsync disabled" BeginFrame source? 
> > 
> > In this case, one issue we'd need to solve with the interface between the
> > Scheduler and a FrameSource is that the Scheduler expects a stream of
> > BeginFrames after a single call to SetNeedsBeginFrame(true). As in,
> > SetNeedsBeginFrame is level triggered and not edge triggered.
> > 
> > The problem with a "vsync disabled" source is that, for it to behave like
> other
> > sources, we should receive an infinite stream of BeginFrames. Maybe we
should
> > add a method to the FrameSource, in addition to the SetNeedsBeginFrame,
called
> > ReadyToReceiveNextBeginFrame.
> 
> Is there any reason we want to keep it level triggered rather then edge
> triggered? (Maybe because of some sort of pipelining?)

It is level triggered at the interface between the Renderer and Browser for
pipelining / latency hiding reasons.

Keeping it level triggered within the Renderer allows us to use the
BeginRetroFrame logic since we can receive new BeginFrames while still handling
the previous BeginFrame. Using an edge triggered interface with the retroactive
logic gets messy.

We don't use the BeginRetro frame logic for the unthrottled case though, since
there would be an infinite number of retroactive frames and we only care about
the most recent one.

> 
> From looking at the various code, it seems we calculate if we actually want to
> keep rendering frames every frame (as an animation could finish).

Yeah. We probably calculate this many times per frame even, but we need some way
to figure out when to toggle the level.

[mithro-old, 2014-05-05 17:58:58]

So your in general agreement that this seems like the right approach? If so,
I'll get to making it all actually work.


On 2014/05/05 17:37:49, brianderson wrote:
> https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc
> File cc/scheduler/frame_source.cc (right):
> 
>
https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
> cc/scheduler/frame_source.cc:90: class ThrottledFrameSource : public
> FrameSource, FrameSink {
> On 2014/05/05 17:10:53, mithro wrote:
> > On 2014/05/05 16:50:05, brianderson wrote:
> > > In what cases would this class be used? Is this to throttle down to 30 fps
> or
> > 20
> > > fps for example?
> > 
> > Yes, examples I can think of are;
> >  * Throttle an ExternalFrameSource (or the synthetic version) down to
> > 30fps/15fps etc
> >  * Throttling the BackToBackFrameSource to a maximum fps rate.
> 
> Cool. What would the interface look like to change the rate of throttling?
Would
> we set a decimation factor or some kind of minimum interval?

The great thing with this type of approach is that we could have both and see
which one works better.

This implementation uses a minimum interval. Would be trivial to have "drop
every X BeginFrame" type decimation too (or even something more complicated).

The interface to change the interval is current the "UpdateFrameSource" with a
null timebase (method could use a better name).

>
https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
> cc/scheduler/frame_source.cc:149: class BackToBackFrameSource : public
> TaskRunnerFrameSource {
> On 2014/05/05 17:10:53, mithro wrote:
> > On 2014/05/05 16:50:05, brianderson wrote:
> > > Would this be used as the "vsync disabled" BeginFrame source? 
> > > 
> > > In this case, one issue we'd need to solve with the interface between the
> > > Scheduler and a FrameSource is that the Scheduler expects a stream of
> > > BeginFrames after a single call to SetNeedsBeginFrame(true). As in,
> > > SetNeedsBeginFrame is level triggered and not edge triggered.
> > > 
> > > The problem with a "vsync disabled" source is that, for it to behave like
> > other
> > > sources, we should receive an infinite stream of BeginFrames. Maybe we
> should
> > > add a method to the FrameSource, in addition to the SetNeedsBeginFrame,
> called
> > > ReadyToReceiveNextBeginFrame.
> > 
> > Is there any reason we want to keep it level triggered rather then edge
> > triggered? (Maybe because of some sort of pipelining?)
> 
> It is level triggered at the interface between the Renderer and Browser for
> pipelining / latency hiding reasons.

At the moment this boundary is only crossed in the "external" frame source which
happens only on Android from what I can see?

> Keeping it level triggered within the Renderer allows us to use the
> BeginRetroFrame logic since we can receive new BeginFrames while still
handling
> the previous BeginFrame. Using an edge triggered interface with the
retroactive
> logic gets messy.

True. I was pondering if the retro frame stuff should be inside the FrameSource
in some way but felt it would make things like doing "catch up" frames much
harder.

> We don't use the BeginRetro frame logic for the unthrottled case though, since
> there would be an infinite number of retroactive frames and we only care about
> the most recent one.

Yeah, I think you are correct. The scheduler needs to tell the frame source that
it is "ready for" / "wants" the next frame.

> > From looking at the various code, it seems we calculate if we actually want
to
> > keep rendering frames every frame (as an animation could finish).
> 
> Yeah. We probably calculate this many times per frame even, but we need some
way
> to figure out when to toggle the level.

Isn't it bad from the Browser/Render boundary to be sending many
SetNeedsBeginFrame many times a frame?

[mithro-old, 2014-05-05 18:26:27]

On 2014/05/05 17:58:58, mithro wrote:
> So your in general agreement that this seems like the right approach? If so,
> I'll get to making it all actually work.
> 
> 
> On 2014/05/05 17:37:49, brianderson wrote:
> >
https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc
> > File cc/scheduler/frame_source.cc (right):
> > 
> >
>
https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
> > cc/scheduler/frame_source.cc:90: class ThrottledFrameSource : public
> > FrameSource, FrameSink {
> > On 2014/05/05 17:10:53, mithro wrote:
> > > On 2014/05/05 16:50:05, brianderson wrote:
> > > > In what cases would this class be used? Is this to throttle down to 30
fps
> > or
> > > 20
> > > > fps for example?
> > > 
> > > Yes, examples I can think of are;
> > >  * Throttle an ExternalFrameSource (or the synthetic version) down to
> > > 30fps/15fps etc
> > >  * Throttling the BackToBackFrameSource to a maximum fps rate.
> > 
> > Cool. What would the interface look like to change the rate of throttling?
> Would
> > we set a decimation factor or some kind of minimum interval?
> 
> The great thing with this type of approach is that we could have both and see
> which one works better.
> 
> This implementation uses a minimum interval. Would be trivial to have "drop
> every X BeginFrame" type decimation too (or even something more complicated).
> 
> The interface to change the interval is current the "UpdateFrameSource" with a
> null timebase (method could use a better name).
> 
> >
>
https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
> > cc/scheduler/frame_source.cc:149: class BackToBackFrameSource : public
> > TaskRunnerFrameSource {
> > On 2014/05/05 17:10:53, mithro wrote:
> > > On 2014/05/05 16:50:05, brianderson wrote:
> > > > Would this be used as the "vsync disabled" BeginFrame source? 
> > > > 
> > > > In this case, one issue we'd need to solve with the interface between
the
> > > > Scheduler and a FrameSource is that the Scheduler expects a stream of
> > > > BeginFrames after a single call to SetNeedsBeginFrame(true). As in,
> > > > SetNeedsBeginFrame is level triggered and not edge triggered.
> > > > 
> > > > The problem with a "vsync disabled" source is that, for it to behave
like
> > > other
> > > > sources, we should receive an infinite stream of BeginFrames. Maybe we
> > should
> > > > add a method to the FrameSource, in addition to the SetNeedsBeginFrame,
> > called
> > > > ReadyToReceiveNextBeginFrame.
> > > 
> > > Is there any reason we want to keep it level triggered rather then edge
> > > triggered? (Maybe because of some sort of pipelining?)
> > 
> > It is level triggered at the interface between the Renderer and Browser for
> > pipelining / latency hiding reasons.
> 
> At the moment this boundary is only crossed in the "external" frame source
which
> happens only on Android from what I can see?
> 
> > Keeping it level triggered within the Renderer allows us to use the
> > BeginRetroFrame logic since we can receive new BeginFrames while still
> handling
> > the previous BeginFrame. Using an edge triggered interface with the
> retroactive
> > logic gets messy.
> 
> True. I was pondering if the retro frame stuff should be inside the
FrameSource
> in some way but felt it would make things like doing "catch up" frames much
> harder.

I was thinking a bit more.

Maybe the scheduler could have a "max pending frames" setting. When the number
of frames in the retro queue + 1 is bigger then we ask the FrameSource to stop
sending events. For the BackToBack render the max pending frames would be 1.

It would then allow us to control the pipeline length a little?

[brianderson, 2014-05-06 00:21:48]

On 2014/05/05 17:58:58, mithro wrote:
> So your in general agreement that this seems like the right approach? If so,
> I'll get to making it all actually work.

A unified interface to BeginFrame sources would definitely be welcome.

> 
> 
> On 2014/05/05 17:37:49, brianderson wrote:
> >
https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc
> > File cc/scheduler/frame_source.cc (right):
> > 
> >
>
https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
> > cc/scheduler/frame_source.cc:90: class ThrottledFrameSource : public
> > FrameSource, FrameSink {
> > On 2014/05/05 17:10:53, mithro wrote:
> > > On 2014/05/05 16:50:05, brianderson wrote:
> > > > In what cases would this class be used? Is this to throttle down to 30
fps
> > or
> > > 20
> > > > fps for example?
> > > 
> > > Yes, examples I can think of are;
> > >  * Throttle an ExternalFrameSource (or the synthetic version) down to
> > > 30fps/15fps etc
> > >  * Throttling the BackToBackFrameSource to a maximum fps rate.
> > 
> > Cool. What would the interface look like to change the rate of throttling?
> Would
> > we set a decimation factor or some kind of minimum interval?
> 
> The great thing with this type of approach is that we could have both and see
> which one works better.
> 
> This implementation uses a minimum interval. Would be trivial to have "drop
> every X BeginFrame" type decimation too (or even something more complicated).
> 
> The interface to change the interval is current the "UpdateFrameSource" with a
> null timebase (method could use a better name).
> 
> >
>
https://codereview.chromium.org/267783004/diff/1/cc/scheduler/frame_source.cc...
> > cc/scheduler/frame_source.cc:149: class BackToBackFrameSource : public
> > TaskRunnerFrameSource {
> > On 2014/05/05 17:10:53, mithro wrote:
> > > On 2014/05/05 16:50:05, brianderson wrote:
> > > > Would this be used as the "vsync disabled" BeginFrame source? 
> > > > 
> > > > In this case, one issue we'd need to solve with the interface between
the
> > > > Scheduler and a FrameSource is that the Scheduler expects a stream of
> > > > BeginFrames after a single call to SetNeedsBeginFrame(true). As in,
> > > > SetNeedsBeginFrame is level triggered and not edge triggered.
> > > > 
> > > > The problem with a "vsync disabled" source is that, for it to behave
like
> > > other
> > > > sources, we should receive an infinite stream of BeginFrames. Maybe we
> > should
> > > > add a method to the FrameSource, in addition to the SetNeedsBeginFrame,
> > called
> > > > ReadyToReceiveNextBeginFrame.
> > > 
> > > Is there any reason we want to keep it level triggered rather then edge
> > > triggered? (Maybe because of some sort of pipelining?)
> > 
> > It is level triggered at the interface between the Renderer and Browser for
> > pipelining / latency hiding reasons.
> 
> At the moment this boundary is only crossed in the "external" frame source
which
> happens only on Android from what I can see?

Yes, although we are going to use it with Aura once we have the
BufferedInputRouter (i.e. input prediction).

> 
> > Keeping it level triggered within the Renderer allows us to use the
> > BeginRetroFrame logic since we can receive new BeginFrames while still
> handling
> > the previous BeginFrame. Using an edge triggered interface with the
> retroactive
> > logic gets messy.
> 
> True. I was pondering if the retro frame stuff should be inside the
FrameSource
> in some way but felt it would make things like doing "catch up" frames much
> harder.
> 
> > We don't use the BeginRetro frame logic for the unthrottled case though,
since
> > there would be an infinite number of retroactive frames and we only care
about
> > the most recent one.
> 
> Yeah, I think you are correct. The scheduler needs to tell the frame source
that
> it is "ready for" / "wants" the next frame.

Whether the retroactive logic gets pulled into the BeginFrame source or we add a
"ready for" signal, we need a common way to handle the vsync enabled and
disabled cases.

> 
> > > From looking at the various code, it seems we calculate if we actually
want
> to
> > > keep rendering frames every frame (as an animation could finish).
> > 
> > Yeah. We probably calculate this many times per frame even, but we need some
> way
> > to figure out when to toggle the level.
> 
> Isn't it bad from the Browser/Render boundary to be sending many
> SetNeedsBeginFrame many times a frame?

Although we calculate it every frame, we only send a message to the Browser when
the level changes.

[brianderson, 2014-05-06 00:22:21]

> I was thinking a bit more.
> 
> Maybe the scheduler could have a "max pending frames" setting. When the number
> of frames in the retro queue + 1 is bigger then we ask the FrameSource to stop
> sending events. For the BackToBack render the max pending frames would be 1.
> 
> It would then allow us to control the pipeline length a little?

I'm not sure this will work since we'll want the new BeginFrames to take
precedence over expired BeginFrames usually.

[mithro-old, 2014-05-06 16:48:24]

The code now compiles and the unittest runs but fail to pass.

Still need to finish hooking up the frame sources inside the scheduler and the
plumbing inside the LayerTreeHostImpl+ThreadProxy.

Tim

[mithro-old, 2014-05-07 08:06:51]

On 2014/05/06 16:48:24, mithro wrote:
> The code now compiles and the unittest runs but fail to pass.
> 
> Still need to finish hooking up the frame sources inside the scheduler and the
> plumbing inside the LayerTreeHostImpl+ThreadProxy.
> 
> Tim

Can you please take another look? While this CL is not finished yet, the frame
sources are now used inside the scheduler and I'm slowly making all the tests in
scheduler_unittest pass.

Tim

[brianderson, 2014-05-07 17:20:16]

Still need to look at more of the patch, but wanted to post what I have so far.

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

https://codereview.chromium.org/267783004/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:45: state->SetString("type", "BeginFrameArgs");
I don't think we use this "type" pattern anywhere else. Would it be more
flexible to have the caller clarify it if needed?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/delay_based...
File cc/scheduler/delay_based_time_source.cc (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/delay_based...
cc/scheduler/delay_based_time_source.cc:75:
DCHECK_NE(interval.ToInternalValue(), 0);
DCHECK_GE? Below too.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.cc:147: if (needs_begin_frame) {
Which solution are you leaning towards for solving when to post the next
BackToBack BeginFrame?

I personally like the solution of a FrameSource::ReadyForNextBeginFrame.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.cc:278: DCHECK(new_source == SourcePrimary() ||
new_source == SourceSecondary());
Can we avoid exposing the underlying frame sources to the here with an enum or
two separate methods?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_source.h
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:18: class CC_EXPORT FrameSink {
Please rename this BeginFrameSink so we have consistent naming and avoid
overloading the word Frame.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:23: class CC_EXPORT FrameSource {
Please rename this BeginFrameSource.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:31: virtual inline std::string FrameSourceType()
const = 0;
Is this for testing purposes? Can it be removed in the final version of this
patch?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:35: class CC_EXPORT BaseFrameSource : public
FrameSource {
I need to think about this a bit. But can you provide reasoning for why
implementation inheritance is better than a composition solution?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:63: class CC_EXPORT ProxyFrameSource : public
BaseFrameSource, public FrameSink {
This is getting a bit confusing for me to follow. Is there any way we can do
without the ProxyFrameSource?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:86: class CC_EXPORT ThrottledFrameSource : public
BaseFrameSource,
I was thinking about this more and think a wrapper based solution like this for
throttling might be too restrictive. For example, when running the compositor at
30 fps, we will want to run the main thread at 30 fps but maybe at an offset
phase to reduce latency. Hiding BeginFrames from the Scheduler would prevent it
from being able to control the phase in a meaningful way.

Let's add throttling related logic for later.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:112: class CC_EXPORT TaskRunnerFrameSource : public
BaseFrameSource {
Does this extra level of inheritance buy us much?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:171: class CC_EXPORT DualFrameSource : public
BaseFrameSource, public FrameSink {
Tim and I talked offline comparing this solution for background ticking and a
solution that pulses SetNeedsBeginFrame in the background from a single
BeginFrameSource instead.

A summary of the tradeoffs:

DualFrameSource:
* Although not a common occurrence, a background tick that arrives right before
we become visible will never delay us from drawing until the next BeginFrame.
* The Browser could eventually control the background FrameSource and coordinate
multiple Renderers so they are more power efficient.

Pulse SetNeedsBeginFrame:
* Doesn't need logic to avoid non monotonic timestamps.
* Retroactively drawing when a background tick arrives right before we become
visible might be complicated.

I wonder if a third solution might be to request foreground or background
BeginFrames from the FrameSource directly.

===================

In any case, I'd be happy with landing the DualFrameSource solution first to
unblock animation work - it reflects the system we have today pretty well.

If we really want to improve the corner cases when becoming visible and improve
power, we can revisit how background ticking is implemented.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:178: // FrameSource
I think DualFrameSource needs to override SetTimebaseAndInterval.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:190: const FrameSource* SourceSecondary() const;
Should we rename these to Foreground and Background? We can generalize in the
future if we need to, but I don't see us needing to.

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

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:56: TRACE_EVENT0("frame_time", "Scheduler::Scheduler()
ProxyFrameSource");
Should the category be "cc" instead of "frame_time"?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:59: } else if (!settings_.throttle_frame_production) {
throttle_frame_production should take precidence, such that the if/else order
is:

if (throttle_frame_production) {}
else if (begin_frame_scheduling_enabled) {}
else { // synthetic }

[brianderson, 2014-05-07 18:09:59]

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

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:236: return;
I don't think we can't return early here, otherwise we'll skip calling
SetupPOllingMechanisms and PostBeginRetroFrameIfNeeded.

https://codereview.chromium.org/267783004/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:969: bool enabled = should_background_tick &&
needs_animate_layers();
Do we need to communicate the needs_animate_layer() to the DualFrameSource
somehow?

https://codereview.chromium.org/267783004/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1470: void
LayerTreeHostImpl::SetSink(FrameSink* sink) {
Missing?

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:457: void
SingleThreadProxy::UpdateBackgroundAnimateTicking() {
Well this is annoying. SingleThreadProxy is going to need a separate solution to
background ticking (outside the Scheduler) until it becomes a SchedulerClient.
Maybe we can't delete the old background ticking logic yet?

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.h:62: // FIXME(mithro): Why is this an empty???
@enne is working on a patch to make SingleThreadProxy a SchedulerClient. Right
now drawing is completely main thread triggered.

[mithro-old, 2014-05-07 22:02:32]

The comments I haven't replied to yet I intend to actually implement in code.

The comments I have replied to need more discussion / guidance.

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

https://codereview.chromium.org/267783004/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:45: state->SetString("type", "BeginFrameArgs");
On 2014/05/07 17:20:16, brianderson wrote:
> I don't think we use this "type" pattern anywhere else. Would it be more
> flexible to have the caller clarify it if needed?

I'd really like to have a type field because then in telemetry we can then find
"all dictionaries of type BeginFrameArgs" easily.

Should this be moved into it's own change? This function (with a little bit of
extra code) can be used to make it possible to use BeginFrameArgs in
TRACE_EVENTs...

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.cc:147: if (needs_begin_frame) {
On 2014/05/07 17:20:16, brianderson wrote:
> Which solution are you leaning towards for solving when to post the next
> BackToBack BeginFrame?
> 
> I personally like the solution of a FrameSource::ReadyForNextBeginFrame.

Yeah, I'm leaning towards the ReadyForNextBeginFrame system too.

If we go this route, I'm wondering if ReadyForNextBeginFrame should also be a
signal to "commit" the "SetNeedsBeginFrame" state?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.cc:278: DCHECK(new_source == SourcePrimary() ||
new_source == SourceSecondary());
On 2014/05/07 17:20:16, brianderson wrote:
> Can we avoid exposing the underlying frame sources to the here with an enum or
> two separate methods?

I originally started with an enum.

After adding the methods to allow access the internal FrameSources, it seemed
like it was just easier to use the pointers. This interface does extends to the
case of a "ManyFrameSource" which could have an arbitrary number of internal
frame source. 

Happy to go with whichever you think is a better interface.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_source.h
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:31: virtual inline std::string FrameSourceType()
const = 0;
On 2014/05/07 17:20:16, brianderson wrote:
> Is this for testing purposes? Can it be removed in the final version of this
> patch?

Having which frame source is in use as part of the scheduler debug state would
be pretty useful.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:63: class CC_EXPORT ProxyFrameSource : public
BaseFrameSource, public FrameSink {
On 2014/05/07 17:20:16, brianderson wrote:
> This is getting a bit confusing for me to follow. Is there any way we can do
> without the ProxyFrameSource?

The sole reason that ProxyFrameSource exists at the moment is because
ThrottleFrameSource/DualFrameSource both take ownership of the frame source that
they are given (via the scope_ptr). Most of the time I think we *do* want these
sources to own the object but it doesn't work with LayerTreeHost as that is
owned via a different route.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:178: // FrameSource
On 2014/05/07 17:20:16, brianderson wrote:
> I think DualFrameSource needs to override SetTimebaseAndInterval.

Yes it does.

At the moment I think we want to forward SetTimebaseAndInterval to the primary
source only?

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

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:56: TRACE_EVENT0("frame_time", "Scheduler::Scheduler()
ProxyFrameSource");
On 2014/05/07 17:20:16, brianderson wrote:
> Should the category be "cc" instead of "frame_time"?

Good question. 

I was pondering introducing "frame_time" as a subcategory of cc disabled by
default which gives more information about the frame time stuff.

I see the options are;
  * Convert to cc
  * Own sub-category under cc
  * Remove before landing

https://codereview.chromium.org/267783004/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:969: bool enabled = should_background_tick &&
needs_animate_layers();
On 2014/05/07 18:09:59, brianderson wrote:
> Do we need to communicate the needs_animate_layer() to the DualFrameSource
> somehow?

The needs_animate_layer is part of the question if we need BeginFrames right?

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:457: void
SingleThreadProxy::UpdateBackgroundAnimateTicking() {
On 2014/05/07 18:09:59, brianderson wrote:
> Well this is annoying. SingleThreadProxy is going to need a separate solution
to
> background ticking (outside the Scheduler) until it becomes a SchedulerClient.
> Maybe we can't delete the old background ticking logic yet?

Unsure yet. Do you know if there are unit tests which make sure this all works?

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.h:62: // FIXME(mithro): Why is this an empty???
On 2014/05/07 18:09:59, brianderson wrote:
> @enne is working on a patch to make SingleThreadProxy a SchedulerClient. Right
> now drawing is completely main thread triggered.

I'll have to figure out how this interacts with this change.

[mithro-old, 2014-05-07 23:42:27]

I think I've replied to all your comments now, please poke anything that I
haven't replied too.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/delay_based...
File cc/scheduler/delay_based_time_source.cc (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/delay_based...
cc/scheduler/delay_based_time_source.cc:75:
DCHECK_NE(interval.ToInternalValue(), 0);
On 2014/05/07 17:20:16, brianderson wrote:
> DCHECK_GE? Below too.

Done.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_source.h
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:18: class CC_EXPORT FrameSink {
On 2014/05/07 17:20:16, brianderson wrote:
> Please rename this BeginFrameSink so we have consistent naming and avoid
> overloading the word Frame.

Done.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:23: class CC_EXPORT FrameSource {
On 2014/05/07 17:20:16, brianderson wrote:
> Please rename this BeginFrameSource.

Done.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:35: class CC_EXPORT BaseFrameSource : public
FrameSource {
On 2014/05/07 17:20:16, brianderson wrote:
> I need to think about this a bit. But can you provide reasoning for why
> implementation inheritance is better than a composition solution?

The code BaseFrameSource saves is a relatively small (but not zero). Feels like
an inheritance type situation but have no other reasoning.

I've fixed the method order and a little more documentation to make the
inheritance easier to understand.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:86: class CC_EXPORT ThrottledFrameSource : public
BaseFrameSource,
On 2014/05/07 17:20:16, brianderson wrote:
> I was thinking about this more and think a wrapper based solution like this
for
> throttling might be too restrictive. For example, when running the compositor
at
> 30 fps, we will want to run the main thread at 30 fps but maybe at an offset
> phase to reduce latency. Hiding BeginFrames from the Scheduler would prevent
it
> from being able to control the phase in a meaningful way.
> 
> Let's add throttling related logic for later.

I'm going to leave the ThrottledFrameSource here for the moment. Will remove it
once the unit tests all pass if we don't end up using it. (It is also useful at
the moment to compare to dual frame sources as they are both cases where you are
wrapping a frame source).

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:112: class CC_EXPORT TaskRunnerFrameSource : public
BaseFrameSource {
On 2014/05/07 17:20:16, brianderson wrote:
> Does this extra level of inheritance buy us much?

Not really - I thought there would be more shared code for posting tasks /
creating BeginFrameArgs but it hasn't turned out that way yet.

I can remove it if you want?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:171: class CC_EXPORT DualFrameSource : public
BaseFrameSource, public FrameSink {
On 2014/05/07 17:20:16, brianderson wrote:
> Tim and I talked offline comparing this solution for background ticking and a
> solution that pulses SetNeedsBeginFrame in the background from a single
> BeginFrameSource instead.
> 
> A summary of the tradeoffs:
> 
> DualFrameSource:
> * Although not a common occurrence, a background tick that arrives right
before
> we become visible will never delay us from drawing until the next BeginFrame.
> * The Browser could eventually control the background FrameSource and
coordinate
> multiple Renderers so they are more power efficient.
> 
> Pulse SetNeedsBeginFrame:
> * Doesn't need logic to avoid non monotonic timestamps.
> * Retroactively drawing when a background tick arrives right before we become
> visible might be complicated.

We are currently 

> 
> I wonder if a third solution might be to request foreground or background
> BeginFrames from the FrameSource directly.
> 
> ===================
> 
> In any case, I'd be happy with landing the DualFrameSource solution first to
> unblock animation work - it reflects the system we have today pretty well.
> 
> If we really want to improve the corner cases when becoming visible and
improve
> power, we can revisit how background ticking is implemented.

Good summary.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:190: const FrameSource* SourceSecondary() const;
On 2014/05/07 17:20:16, brianderson wrote:
> Should we rename these to Foreground and Background? We can generalize in the
> future if we need to, but I don't see us needing to.

Done.

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

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:59: } else if (!settings_.throttle_frame_production) {
On 2014/05/07 17:20:16, brianderson wrote:
> throttle_frame_production should take precidence, such that the if/else order
> is:
> 
> if (throttle_frame_production) {}
> else if (begin_frame_scheduling_enabled) {}
> else { // synthetic }

Done.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:236: return;
On 2014/05/07 18:09:59, brianderson wrote:
> I don't think we can't return early here, otherwise we'll skip calling
> SetupPOllingMechanisms and PostBeginRetroFrameIfNeeded.

Will look at this further as I fix the unittests.

https://codereview.chromium.org/267783004/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1470: void
LayerTreeHostImpl::SetSink(FrameSink* sink) {
On 2014/05/07 18:09:59, brianderson wrote:
> Missing?

Done.

[brianderson, 2014-05-08 00:55:58]

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

https://codereview.chromium.org/267783004/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:45: state->SetString("type", "BeginFrameArgs");
On 2014/05/07 22:02:32, mithro wrote:
> On 2014/05/07 17:20:16, brianderson wrote:
> > I don't think we use this "type" pattern anywhere else. Would it be more
> > flexible to have the caller clarify it if needed?
> 
> I'd really like to have a type field because then in telemetry we can then
find
> "all dictionaries of type BeginFrameArgs" easily.
> 
> Should this be moved into it's own change? This function (with a little bit of
> extra code) can be used to make it possible to use BeginFrameArgs in
> TRACE_EVENTs...

Please split it out to a separate patch, yes. If you feel strongly about keeping
the type, I won't push it - I see value too in being able to search for such an
important type in traces.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.cc:278: DCHECK(new_source == SourcePrimary() ||
new_source == SourceSecondary());
On 2014/05/07 22:02:32, mithro wrote:
> On 2014/05/07 17:20:16, brianderson wrote:
> > Can we avoid exposing the underlying frame sources to the here with an enum
or
> > two separate methods?
> 
> I originally started with an enum.
> 
> After adding the methods to allow access the internal FrameSources, it seemed
> like it was just easier to use the pointers. This interface does extends to
the
> case of a "ManyFrameSource" which could have an arbitrary number of internal
> frame source. 
> 
> Happy to go with whichever you think is a better interface.

Do we need to expose the underlying sources at all?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_source.h
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:63: class CC_EXPORT ProxyFrameSource : public
BaseFrameSource, public FrameSink {
On 2014/05/07 22:02:32, mithro wrote:
> On 2014/05/07 17:20:16, brianderson wrote:
> > This is getting a bit confusing for me to follow. Is there any way we can do
> > without the ProxyFrameSource?
> 
> The sole reason that ProxyFrameSource exists at the moment is because
> ThrottleFrameSource/DualFrameSource both take ownership of the frame source
that
> they are given (via the scope_ptr). Most of the time I think we *do* want
these
> sources to own the object but it doesn't work with LayerTreeHost as that is
> owned via a different route.

I feel like there must be a solution to this problem that requires less
boilerplate and is more straight forward.

For the classes that we might want to have take ownership, could we make the
constructors take in raw pointers and then have separate methods to transfer
ownership? Or have additional constructors that take in raw pointers?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:112: class CC_EXPORT TaskRunnerFrameSource : public
BaseFrameSource {
On 2014/05/07 23:42:28, mithro wrote:
> On 2014/05/07 17:20:16, brianderson wrote:
> > Does this extra level of inheritance buy us much?
> 
> Not really - I thought there would be more shared code for posting tasks /
> creating BeginFrameArgs but it hasn't turned out that way yet.
> 
> I can remove it if you want?

Please remove it unless you find there is a significant benefit.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:178: // FrameSource
On 2014/05/07 22:02:32, mithro wrote:
> On 2014/05/07 17:20:16, brianderson wrote:
> > I think DualFrameSource needs to override SetTimebaseAndInterval.
> 
> Yes it does.
> 
> At the moment I think we want to forward SetTimebaseAndInterval to the primary
> source only?

To the primary source only sounds good.

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

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:56: TRACE_EVENT0("frame_time", "Scheduler::Scheduler()
ProxyFrameSource");
On 2014/05/07 22:02:32, mithro wrote:
> On 2014/05/07 17:20:16, brianderson wrote:
> > Should the category be "cc" instead of "frame_time"?
> 
> Good question. 
> 
> I was pondering introducing "frame_time" as a subcategory of cc disabled by
> default which gives more information about the frame time stuff.
> 
> I see the options are;
>   * Convert to cc
>   * Own sub-category under cc
>   * Remove before landing
> 

If we don't assign it to cc or a new category, there's also a disabled by
default category called cc.debug.scheduler.

https://codereview.chromium.org/267783004/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:969: bool enabled = should_background_tick &&
needs_animate_layers();
On 2014/05/07 22:02:32, mithro wrote:
> On 2014/05/07 18:09:59, brianderson wrote:
> > Do we need to communicate the needs_animate_layer() to the DualFrameSource
> > somehow?
> 
> The needs_animate_layer is part of the question if we need BeginFrames right?

Yes.

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:457: void
SingleThreadProxy::UpdateBackgroundAnimateTicking() {
On 2014/05/07 22:02:32, mithro wrote:
> On 2014/05/07 18:09:59, brianderson wrote:
> > Well this is annoying. SingleThreadProxy is going to need a separate
solution
> to
> > background ticking (outside the Scheduler) until it becomes a
SchedulerClient.
> > Maybe we can't delete the old background ticking logic yet?
> 
> Unsure yet. Do you know if there are unit tests which make sure this all
works?

The LayerTreeHost/Impl unit tests should test background ticking. Most of them
run in both threaded and non-threaded modes.

[brianderson, 2014-05-08 21:08:15]

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.cc:147: if (needs_begin_frame) {
On 2014/05/07 22:02:32, mithro wrote:
> On 2014/05/07 17:20:16, brianderson wrote:
> > Which solution are you leaning towards for solving when to post the next
> > BackToBack BeginFrame?
> > 
> > I personally like the solution of a FrameSource::ReadyForNextBeginFrame.
> 
> Yeah, I'm leaning towards the ReadyForNextBeginFrame system too.
> 
> If we go this route, I'm wondering if ReadyForNextBeginFrame should also be a
> signal to "commit" the "SetNeedsBeginFrame" state?
> 

I've changed my mind here. Let's just keep the single
SetNeedsBeginFrame(needs_begin_frame) entry point and force the scheduler to
call it whenever it's ready for the next BeginFrame. It'll work just like having
a ReadyForNextBeginFrame, but we don't really need an extra entry point.

When vsync is disabled, we can SetNeedsBeginFrame when we are finished with a
frame. When vsync is enabled, we can SetNeedsBeginFrame as part of the
BeginFrame.

[mithro-old, 2014-05-09 02:17:47]

On 2014/05/08 21:08:15, brianderson wrote:
>
https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
> File cc/scheduler/frame_source.cc (right):
> 
>
https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
> cc/scheduler/frame_source.cc:147: if (needs_begin_frame) {
> On 2014/05/07 22:02:32, mithro wrote:
> > On 2014/05/07 17:20:16, brianderson wrote:
> > > Which solution are you leaning towards for solving when to post the next
> > > BackToBack BeginFrame?
> > > 
> > > I personally like the solution of a FrameSource::ReadyForNextBeginFrame.
> > 
> > Yeah, I'm leaning towards the ReadyForNextBeginFrame system too.
> > 
> > If we go this route, I'm wondering if ReadyForNextBeginFrame should also be
a
> > signal to "commit" the "SetNeedsBeginFrame" state?
> > 
> 
> I've changed my mind here. Let's just keep the single
> SetNeedsBeginFrame(needs_begin_frame) entry point and force the scheduler to
> call it whenever it's ready for the next BeginFrame. It'll work just like
having
> a ReadyForNextBeginFrame, but we don't really need an extra entry point.
> 
> When vsync is disabled, we can SetNeedsBeginFrame when we are finished with a
> frame. When vsync is enabled, we can SetNeedsBeginFrame as part of the
> BeginFrame.

Are you saying that SetNeedsBeginFrame should be edge triggered? Or it is still
level triggered but the level should be updated whenever the scheduler is ready
for a new frame?

What about the pipelining from the browser issue?

[mithro-old, 2014-05-09 02:45:58]

Uploaded the latest version of this patch which has all the debugging stuff
removed.

Still working on fixing the scheduler_unittest,

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

https://codereview.chromium.org/267783004/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:45: state->SetString("type", "BeginFrameArgs");
On 2014/05/08 00:55:58, brianderson wrote:
> On 2014/05/07 22:02:32, mithro wrote:
> > On 2014/05/07 17:20:16, brianderson wrote:
> > > I don't think we use this "type" pattern anywhere else. Would it be more
> > > flexible to have the caller clarify it if needed?
> > 
> > I'd really like to have a type field because then in telemetry we can then
> find
> > "all dictionaries of type BeginFrameArgs" easily.
> > 
> > Should this be moved into it's own change? This function (with a little bit
of
> > extra code) can be used to make it possible to use BeginFrameArgs in
> > TRACE_EVENTs...
> 
> Please split it out to a separate patch, yes. If you feel strongly about
keeping
> the type, I won't push it - I see value too in being able to search for such
an
> important type in traces.

Done.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.cc:147: if (needs_begin_frame) {
On 2014/05/08 21:08:15, brianderson wrote:
> On 2014/05/07 22:02:32, mithro wrote:
> > On 2014/05/07 17:20:16, brianderson wrote:
> > > Which solution are you leaning towards for solving when to post the next
> > > BackToBack BeginFrame?
> > > 
> > > I personally like the solution of a FrameSource::ReadyForNextBeginFrame.
> > 
> > Yeah, I'm leaning towards the ReadyForNextBeginFrame system too.
> > 
> > If we go this route, I'm wondering if ReadyForNextBeginFrame should also be
a
> > signal to "commit" the "SetNeedsBeginFrame" state?
> > 
> 
> I've changed my mind here. Let's just keep the single
> SetNeedsBeginFrame(needs_begin_frame) entry point and force the scheduler to
> call it whenever it's ready for the next BeginFrame. It'll work just like
having
> a ReadyForNextBeginFrame, but we don't really need an extra entry point.
> 
> When vsync is disabled, we can SetNeedsBeginFrame when we are finished with a
> frame. When vsync is enabled, we can SetNeedsBeginFrame as part of the
> BeginFrame.

Are you saying that SetNeedsBeginFrame should be edge triggered? Or it is still
level triggered but the level should be updated whenever the scheduler is ready
for a new frame?

What about the pipelining from the browser issue?

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.cc:278: DCHECK(new_source == SourcePrimary() ||
new_source == SourceSecondary());
On 2014/05/08 00:55:58, brianderson wrote:
> On 2014/05/07 22:02:32, mithro wrote:
> > On 2014/05/07 17:20:16, brianderson wrote:
> > > Can we avoid exposing the underlying frame sources to the here with an
enum
> or
> > > two separate methods?
> > 
> > I originally started with an enum.
> > 
> > After adding the methods to allow access the internal FrameSources, it
seemed
> > like it was just easier to use the pointers. This interface does extends to
> the
> > case of a "ManyFrameSource" which could have an arbitrary number of internal
> > frame source. 
> > 
> > Happy to go with whichever you think is a better interface.
> 
> Do we need to expose the underlying sources at all?

As the DualFrameSource takes ownership of the sources it seems like a good idea?

We might want to pass a DualFrameSource around without also passing pointers to
it's internal sources?

The below seems kinda of bad?
------------------------------------------------------------
class A {

 FrameSource* frame_f_;
 FrameSource* frame_b_;
 scoped_ptr<FrameSource> frame_source_;

 A() : {
   frame_f_ = new FrameSourceA();
   frame_b_ = new FrameSourceB();
   frame_source_ make_scoped_ptr(new DualFrameSource(frame_f_, frame_b_));
 }

 void do() {
   frame_f_->XXXX();
 }
}
------------------------------------------------------------
verses
------------------------------------------------------------

class A {

 FrameSource* frame_f_;
 FrameSource* frame_b_;
 scoped_ptr<FrameSource> frame_source_;

 A() : {
   frame_source_ make_scoped_ptr(new DualFrameSource(new FrameSourceA(), new
FrameSourceB());
 }

 void do() {
   frame_source->SourceForeground()->XXXX();
 }
}
------------------------------------------------------------

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_source.h
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:63: class CC_EXPORT ProxyFrameSource : public
BaseFrameSource, public FrameSink {
> I feel like there must be a solution to this problem that requires less
> boilerplate and is more straight forward.
> 
> For the classes that we might want to have take ownership, could we make the
> constructors take in raw pointers and then have separate methods to transfer
> ownership? Or have additional constructors that take in raw pointers?

This seems like mostly a style question? 

I guess we could do the following, but we probably should then have constructors
for all the combinations (O, O) (U, U) (O, U) (U, O)...
----------------------------------------------------------
class DualFrameSource {
  FrameSource* source_foreground_;
  scoped_ptr<FrameSource> source_foreground_owner_;
  FrameSource* source_background_;
  scoped_ptr<FrameSource> source_background_owner_;

  DualFrameSource(FrameSource* foreground, FrameSource* background)
    : source_foreground_(foreground), 
      source_background_(background)
   {}

  DualFrameSource(scoped_ptr<FrameSource> foreground, scoped_ptr<FrameSource>
background)
    : source_foreground_(foreground.get()),
source_foreground_owner_(foreground),
      source_background_(background.get()),
source_background_owner_(background), 
   {}
}
----------------------------------------------------------

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:112: class CC_EXPORT TaskRunnerFrameSource : public
BaseFrameSource {
On 2014/05/08 00:55:58, brianderson wrote:
> On 2014/05/07 23:42:28, mithro wrote:
> > On 2014/05/07 17:20:16, brianderson wrote:
> > > Does this extra level of inheritance buy us much?
> > 
> > Not really - I thought there would be more shared code for posting tasks /
> > creating BeginFrameArgs but it hasn't turned out that way yet.
> > 
> > I can remove it if you want?
> 
> Please remove it unless you find there is a significant benefit.
> 

Done.

https://codereview.chromium.org/267783004/diff/40001/cc/scheduler/frame_sourc...
cc/scheduler/frame_source.h:178: // FrameSource
On 2014/05/08 00:55:58, brianderson wrote:
> On 2014/05/07 22:02:32, mithro wrote:
> > On 2014/05/07 17:20:16, brianderson wrote:
> > > I think DualFrameSource needs to override SetTimebaseAndInterval.
> > 
> > Yes it does.
> > 
> > At the moment I think we want to forward SetTimebaseAndInterval to the
primary
> > source only?
> 
> To the primary source only sounds good.

Done.

How do we set / change the interval for the background source then?

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/267783004/diff/40001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:457: void
SingleThreadProxy::UpdateBackgroundAnimateTicking() {
On 2014/05/08 00:55:58, brianderson wrote:
> On 2014/05/07 22:02:32, mithro wrote:
> > On 2014/05/07 18:09:59, brianderson wrote:
> > > Well this is annoying. SingleThreadProxy is going to need a separate
> solution
> > to
> > > background ticking (outside the Scheduler) until it becomes a
> SchedulerClient.
> > > Maybe we can't delete the old background ticking logic yet?
> > 
> > Unsure yet. Do you know if there are unit tests which make sure this all
> works?
> 
> The LayerTreeHost/Impl unit tests should test background ticking. Most of them
> run in both threaded and non-threaded modes.

Guess we will find out shortly.

[brianderson, 2014-05-21 00:29:38]

Tim, is this the next patch on deck? It would be nice to get this in and re-use
the frame_source logic in Pepper.

[brianderson, 2014-05-21 00:29:38]

Tim, is this the next patch on deck? It would be nice to get this in and re-use
the frame_source logic in Pepper.

[mithro-old, 2014-05-21 00:39:38]

On 2014/05/21 00:29:38, brianderson wrote:
> Tim, is this the next patch on deck? It would be nice to get this in and
re-use
> the frame_source logic in Pepper.

I was focused on BlinkOn last week and only just got back to Sydney this
morning. I will be working on this patch as first priority now and hope to land
it ASAP.

There are currently a couple of things I need to do before we can land this;
 * Making it pass all the existing scheduler unit tests to prove it doesn't
regress the current functionality.
 * Fixing the background ticking tests to understand the scheduler is doing the
ticking now.

My current process for the first part is that I'm diffing the output from the
scheduler unittests with and without the patch using the following command line;
---------------------------------
  ninja -C out-fast/Debug cc_unittests && DISPLAY=:1.0
out-fast/Debug/cc_unittests --single-process-tests --gtest_filter=*Scheduler*
--trace-to-console=frame_time,cc,disabled-by-default-cc.debug.scheduler 2>&1 |
sed -e's/^.*ERROR://' -e's/([0-9.]\+ ms/(xx ms/g'
-e's/\(-\?\)[0-9][0-9][0-9]*\.[0-9][0-9]\?[0-9]\?/\110XXXXXXX\.XXX/g' | awk
--re-interval '{ if ($i ~ /[[:digit:]]*\.[[:digit:]]{1,3}/) {
gsub(/\.[[:digit:]]{1,3}/, ".X", $i); gsub(/[[:digit:]]/, "X", $i) } }1' >
output-new
---------------------------------

How can we reuse this in pepper?

Tim

[brianderson, 2014-05-21 00:57:28]

> How can we reuse this in pepper?

I'm not quite sure myself yet. We may have to move the frame_source.xx files to
a directory usable by both cc an pepper (likely), or we may allow pepper to
include files from cc (less likely). We can figure out where exactly to put it
after you land this.

For Pepper, the idea is to use the SyntheticBeginFrameSource in the Pepper
process initially, which will be easy, and then transition to using a real
BeginFrame message later, which will be a routing headache.

[mithro-old, 2014-05-21 03:39:34]

We will want to refactor the delay_based_time_source.cc before we do that?


On 21 May 2014 10:57, <brianderson@chromium.org> wrote:

> How can we reuse this in pepper?
>>
>
> I'm not quite sure myself yet. We may have to move the frame_source.xx
> files to
> a directory usable by both cc an pepper (likely), or we may allow pepper to
> include files from cc (less likely). We can figure out where exactly to
> put it
> after you land this.
>
> For Pepper, the idea is to use the SyntheticBeginFrameSource in the Pepper
> process initially, which will be easy, and then transition to using a real
> BeginFrame message later, which will be a routing headache.
>
> https://codereview.chromium.org/267783004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2014-05-21 17:29:53]

> We will want to refactor the delay_based_time_source.cc before we do that?

I was hoping we could just move it to wherever the frame_time.cc goes. Is there
anything holding us back from doing that?

[brianderson, 2014-06-09 23:36:38]

Friendly ping.

[mithro-old, 2014-06-10 03:58:23]

On 2014/06/09 at 23:36:38, brianderson wrote:
> Friendly ping.

Hi Brian,

Sorry for the delay, I was working on web-animation performance with the team
here locally last week. I've rebased this patch onto master (with the
EXPECT_NO_ACTION and Delay Source AsValue() patches being a dependencies) but it
currently doesn't pass the scheduler unit tests (Dana landed a change which
changes a couple of the tests). 

It should be okay for review while I work on trying to get all the tests passing
again (will update this thread when the tests pass and again when the
dependencies land). As mentioned previously we need to figure out;
 * If using SetGenerateFrames/IsGeneratingFrames is the right naming, or if we
should use SetBeginFrame>s< / IsSendingBeginFrame>s< (notice the extra 's' to
show it is level triggered rather than edge triggered).
 * If the PendingFrames back pressure is the right way to do it. I do think we
want to separate the back pressure mechanism from the frame generation message.

Tim 'mithro' Ansell

[mithro-old, 2014-06-11 17:00:23]

On 2014/06/10 03:58:23, mithro wrote:
> On 2014/06/09 at 23:36:38, brianderson wrote:
> > Friendly ping.
> 
> Hi Brian,
> 
> Sorry for the delay, I was working on web-animation performance with the team
> here locally last week. I've rebased this patch onto master (with the
> EXPECT_NO_ACTION and Delay Source AsValue() patches being a dependencies) but
it
> currently doesn't pass the scheduler unit tests (Dana landed a change which
> changes a couple of the tests). 
> 
> It should be okay for review while I work on trying to get all the tests
passing
> again (will update this thread when the tests pass and again when the
> dependencies land). As mentioned previously we need to figure out;
>  * If using SetGenerateFrames/IsGeneratingFrames is the right naming, or if we
> should use SetBeginFrame>s< / IsSendingBeginFrame>s< (notice the extra 's' to
> show it is level triggered rather than edge triggered).
>  * If the PendingFrames back pressure is the right way to do it. I do think we
> want to separate the back pressure mechanism from the frame generation
message.
> 
> Tim 'mithro' Ansell

Friendly ping.

[simonhong, 2014-06-12 13:47:40]

Hi mithro,
I did some some style check.
After reading this CL, I felt that there are much inheritance than we need.
IMO,  If Scheduler has background_source and foreground_source directly and
references one of them via active_source instead of using DualBeginFrameSource,
we can implement more briefly.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:9: #include
"cc/scheduler/delay_based_time_source.h"
All above includes are duplicated with frame_source.h

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:10: #include "cc/scheduler/scheduler.h"
Scheduler class is not referenced in this file.
Can be removed.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:11: #include "ui/gfx/frame_time.h"
ditto.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:21: timebase_(),
can be removed.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:108: DCHECK(source_);
DCHECK can be removed because we don't erase or change source_ after it is set
in the ctor.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:113: DCHECK(source_);
ditto.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:120: DCHECK(source_);
ditto.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:228: time_source_->SetActive(false);
time_source_ is set to inactive when it is initialized.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:259: if (!missed_tick_time.is_null()) {
Generally, curly braces are not required for single-line statements.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:68: base::TimeDelta interval_;
Variable declaration should be followed by Method declaration.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:83: */
How about using // instead of /* */?
Style guide says both are ok but be consistent.
All of other sources in cc/ uses // style.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Commen...

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:190: scoped_ptr<BeginFrameSource> source_secondary);
How about foreground_source and background_source?

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:200: const BeginFrameSource* SourceBackground()
const;
How about ForegroundSource() and BackgroundSource()?

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:206: scoped_ptr<BeginFrameSource>
source_background_;
How about foreground_source_ and background_source_?

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:54: scoped_ptr<BeginFrameSource> primary_frame_source;
foreground_frame_source?

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:103: }
I think Scheduler should manage and switch fourground and background source and
active source, too without DualBeginFrameSource.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:28: class Thread;
We don't need this.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:129: virtual void BeginFrame(const BeginFrameArgs&
args) OVERRIDE;
Blank line is needed because this is overrided from BeginFrameSink.
Also needs comments like "//BeginFrameSink implementation".

https://codereview.chromium.org/267783004/diff/210001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/267783004/diff/210001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.h:62: // TODO(mithro): Why is this an empty???
In the single thread mode, BeginFrame() is not used because new frame is not
triggered by LayerTreeHostImpl.
Instead, it triggers new frame immediately when new commit is requested.

[mithro-old, 2014-06-13 04:20:40]

Hi Simon,

Thanks for doing the review. I'll get to your comments shortly but wanted to
give you some extra context that is currently just stuck in myself and Brian's
head regarding where this code is going.

I'm currently working on trying to make Chrome's usage for time for animation
sane. The time to animate for should be generated somewhere and then be passed
down the stack. Nowhere in the animation stack should we ever use the usage of
Now() as it means you are not rendering for the right time. Eventually we want
to make a betters decisions on the time to animate for (such as using the time
things will hit the display).

To reach this goal I'm trying to make the scheduler agnostic to the source of
BeginFrame messages (and make it always use BeginFrame messages, even when doing
things like un-throttled rendering). We want this frame code to be generic
enough that we can move it to another part of the stack (For example, hoisting
it out of the scheduler into the browser process or sharing it between things
like pepper/mojo/video code.) 

We also want to abstract away the background ticking that happens as eventually
we want to be much smarter in doing background ticking (for example coalescing
background ticking of all tabs together, reducing or not even background ticking
during periods of high load, etc). 

Lastly, we want to introduce filtering and stabilisation of the BeginFrame
messages so that we can accurately provide the time of the next vsync signal
within a very small margin of error. 

Having actually enumerated these goals, I do notice there are a couple of places
the abstraction is leaking. The scheduler clearly knows to much about frame
sources and the dual frame source which I will try and fix.

If you have ideas on how this code could work, I'd love to hear them. I'm almost
always on IRC (as mithro, say my name to get it to ping me) and on
GTalk/Hangouts at mithro@mithis.com and tansell@google.com if you want to chat
in real time. Otherwise I try and respond to emails promptly (if you haven't
heard back from me in over 3 days, please do ping).

Thank you once again!

Tim 'mithro' Ansell

[Sami, 2014-06-13 11:20:02]

message: Thanks for the summary Tim.

On 2014/06/13 04:20:40, mithro wrote:
> Lastly, we want to introduce filtering and stabilisation of the BeginFrame
> messages so that we can accurately provide the time of the next vsync signal
> within a very small margin of error. 

Would this be something that the BeginFrame does by itself or something that the
scheduler infers from the BeginFrames it is getting? It feels like the
BeginFrame source (in the browser) can do the best job at this.

[brianderson, 2014-06-17 06:24:43]

Thanks for working through this Tim. Coming up with the right abstractions is
turning out to be a difficult task.

All the sources behave slightly differently, which seems to be where most of the
complexity is coming from. I'm hoping we can simplify the abstract interfaces
and localize the complexity to the Scheduler where ever possible though.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:8: #include <string>
Is this only needed for the TypeString? Can we just use const char * instead to
avoid pulling in this dependency?

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:26: virtual BeginFrameSink* GetBeginFrameSink()
const = 0;
Is GetBeginFrameSink needed for anything? Can we get rid of it?

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:28: virtual void SetGenerateFrames(bool
generate_frames) = 0;
Good idea using the 's' to signify a level request. I think we should rename
this to SetNeedsBeginFrames to be more consistent with the rest of cc's codebase
though.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:34: virtual void PendingFrames(size_t count) = 0;
I think the unthrottled BeginFrame source is really making the attempt to
generalize all sources too difficult by forcing BeginFrameSources that shouldn't
have to care about the unthrottled case to worry about it.

Maybe we should go back to an edge triggered SetNeedsBeginFrame (without the
's'). Then, in the throttled case, the Scheduler could call
SetNeedsBeginFrame(true) within a BeginFrame to request a stream of BeginFrames.
In the unthrottled case, the Scheduler can call SetNeedsBeginFrame(true) only in
the deadline. This would mirror the behavior of SetNeedsCommit more closely and
localize the behavioral differences to the Scheduler rather than leaking it to
all BeginFrameSources.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:39: virtual base::TimeDelta Interval() const = 0;
Not all BeginFrameSources necessarily need a TimeBase and interval. Can these
methods be moved into only the sources that need it.

Maybe have a separate pure virtual class called TimeBaseAndIntervalSink and have
a member variable of that type in the Scheduler assigned to the
SyntheticBeginFrameSource or BackToBackBeginFrameSource when they are active?

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:46: class CC_EXPORT BaseBeginFrameSource : public
BeginFrameSource {
This BaseBeginFrameSource makes understanding the logic a bit convoluted. If it
really helps classes within this file, that's maybe ok; but I'd really like to
prevent either the LayerTreeHostImpl or OutputSurfaces from inheriting it.

I'm hoping that reduceing the API surface of BeginFrameSource will make reduce
the need for BaseBeginFrameSource.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:84: class CC_EXPORT ProxyBeginFrameSource : public
BaseBeginFrameSource,
I understand the desire to have the DualBeginFrameSource own its child
BeginFrameSoures. Do any other sources need to own child sources that we want to
prevent them from actually owning?

I think it would be easier to prevent DualBeginFrameSource from owning its
children and have the Scheduler take ownership of the sources it creates. We'd
just need to be extra careful about destruction order.

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:185: class CC_EXPORT DualBeginFrameSource : public
BaseBeginFrameSource,
Do we need this class anymore since the background ticking change isn't part of
this patch?

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:460: DCHECK(settings_.throttle_frame_production);
Why is this DCHECK removed?

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:100:
frame_source_->SwitchSource(frame_source_->SourceForeground());
SwitchToForeground() and SwitchToBackground()?

https://codereview.chromium.org/267783004/diff/210001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:587:
frame_source_->BeginFrameSourceAsValue().release());
BeginFrameSourceAsValue -> AsValue()?

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

https://codereview.chromium.org/267783004/diff/210001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:113: public BaseBeginFrameSource,
How difficult would it be to make OutputSurface inherit from BeginFrameSource
instead? We'd have to set the sink every time a new OutputSurface is created.

[mithro-old, 2014-09-12 15:08:22]

mithro@mithis.com changed reviewers:
+ skyostil@chromium.org

[mithro-old, 2014-09-12 15:08:23]

Hi Brian / Sami,

I have done a pretty major rewrite of this CL trying to take onboard Brian's
feedback and make it compatible with Simon Hong's CL too.

Any chance you can have a look today so I can start fixing issues on Monday?

Thanks!

Tim

[brianderson, 2014-09-13 01:38:13]

With the knowledge of where the unified BeginFrame patch is headed, this patch
is looking great and is a major improvement.

I didn't get to look at everything in detail yet, but am posting what I have so
far.

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:230: SendBeginFrame(args);
We probably shouldn't send BeginFrames from Browser to Renderer in the
unthrottled case.

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:23: class CC_EXPORT VSyncObserverImpl
Maybe call this VSyncParameterObserver so it's clear this isn't the platform's
vsync signal.

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:83: class CC_EXPORT BeginFrameSource {
Would it make sense to split this into a BeginFrameSource and a
BeginFrameSender?

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:109: class CC_EXPORT ThrottledBeginFrameSource :
public BeginFrameSource,
Let's keep this class out of this patch and add it if it makes sense when we
implement throttling. Although elegant, I'm not sure if this is the best
solution. Let's keep discussion about how to proceed with throttling in a
separate patch/bug.

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:106:
vsync_observer_->OnUpdateVSyncParameters(timebase, interval);
How does this additional indirection help?

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:256:
SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE);
I remember intentionally not including the idle state here, although I forget
the original reason atm. Why do you need it now?

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:34: virtual BeginFrameSource* GetBeginFrameSource() =
0;
How about GetExternalBeginFrameSource()?

https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:141: scoped_ptr<BeginFrameSource>
primary_frame_source_store_;
primary_frame_source_internal_?

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

https://codereview.chromium.org/267783004/diff/230001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:115: public BeginFrameSource,
Can the OutputSurface be the BeginFrameSource instead?

[Sami, 2014-09-15 13:17:00]

https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:140: base::TimeDelta::FromInternalValue(-1));
Can we use a valid interval (say, BeginFrameArgs::DefaultInterval) here? Or if
you want an invalid one then please use zero. Otherwise, base::TimeDelta::Max()
is probably what you want here.

https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:70: virtual bool NeedsBeginFrames() = 0;
nit: Should be const.

https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:75: virtual void FinishedFrame(size_t
remaining_frames) = 0;
nit: Call this something like DidFinishFrame() to match existing naming
conventions.

https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:77: void AddObserver(BeginFrameObserver* obs);
Should we call these SetObserver/ClearObserver since there can only be one at a
time?

[mithro-old, 2014-09-15 14:42:40]

On 2014/09/13 01:38:13, brianderson wrote:
> With the knowledge of where the unified BeginFrame patch is headed, this patch
> is looking great and is a major improvement.
> 
> I didn't get to look at everything in detail yet, but am posting what I have
so
> far.
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> File cc/scheduler/frame_source.cc (right):
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.cc:230: SendBeginFrame(args);
> We probably shouldn't send BeginFrames from Browser to Renderer in the
> unthrottled case.
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> File cc/scheduler/frame_source.h (right):
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.h:23: class CC_EXPORT VSyncObserverImpl
> Maybe call this VSyncParameterObserver so it's clear this isn't the platform's
> vsync signal.
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.h:83: class CC_EXPORT BeginFrameSource {
> Would it make sense to split this into a BeginFrameSource and a
> BeginFrameSender?
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.h:109: class CC_EXPORT ThrottledBeginFrameSource :
> public BeginFrameSource,
> Let's keep this class out of this patch and add it if it makes sense when we
> implement throttling. Although elegant, I'm not sure if this is the best
> solution. Let's keep discussion about how to proceed with throttling in a
> separate patch/bug.
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:106:
> vsync_observer_->OnUpdateVSyncParameters(timebase, interval);
> How does this additional indirection help?
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:256:
> SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE);
> I remember intentionally not including the idle state here, although I forget
> the original reason atm. Why do you need it now?
> 
> https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler.h
> File cc/scheduler/scheduler.h (right):
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.h:34: virtual BeginFrameSource* GetBeginFrameSource() =
> 0;
> How about GetExternalBeginFrameSource()?
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.h:141: scoped_ptr<BeginFrameSource>
> primary_frame_source_store_;
> primary_frame_source_internal_?
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.h (right):
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.h:115: public BeginFrameSource,
> Can the OutputSurface be the BeginFrameSource instead?

Just a FYI for everyone. Myself and Brian chatted quite a bit about Simon Hong's
"Unified BeginFrame scheduling for Chrome"
(https://codereview.chromium.org/423773002/) patch and how to make that
compatible with this change. I came up with the following diagram based on that
discussion -
https://docs.google.com/a/google.com/drawings/d/1Wy5SlG1gVdUIhhkaymraVRJSFi1M...

On 2014/09/13 01:38:13, brianderson wrote:
> With the knowledge of where the unified BeginFrame patch is headed, this patch
> is looking great and is a major improvement.
> 
> I didn't get to look at everything in detail yet, but am posting what I have
so
> far.

Thanks for posting these initial thoughts, it has allowed me to continue with
progress on this patch today.

> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> File cc/scheduler/frame_source.cc (right):
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.cc:230: SendBeginFrame(args);
> We probably shouldn't send BeginFrames from Browser to Renderer in the
> unthrottled case.

I agree.

If the unthrottled frame sources has the same API as the browser to render frame
source it becomes an easy case to swap in / out the different frame sources.

Did you see the update to the Browser / Render diagram at
https://docs.google.com/a/google.com/drawings/d/1Wy5SlG1gVdUIhhkaymraVRJSFi1M...
 which tries to capture this?


>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> File cc/scheduler/frame_source.h (right):
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.h:23: class CC_EXPORT VSyncObserverImpl
> Maybe call this VSyncParameterObserver so it's clear this isn't the platform's
> vsync signal.

Done.

>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.h:83: class CC_EXPORT BeginFrameSource {
> Would it make sense to split this into a BeginFrameSource and a
> BeginFrameSender?

I'm not sure I understand what the difference between a BeginFrameSource and a
BeginFrameSender?

> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.h:109: class CC_EXPORT ThrottledBeginFrameSource :
> public BeginFrameSource,
> Let's keep this class out of this patch and add it if it makes sense when we
> implement throttling. Although elegant, I'm not sure if this is the best
> solution. Let's keep discussion about how to proceed with throttling in a
> separate patch/bug.

After your feedback on the original patch I spent a lot of time removing the
other proposed frame sources. The only reason the ThrottlingBeginFrameSource
still exists the multiplexer is just a more advanced version of the throttler
and it kind of make it slightly easier to test.

I've removed it for now and just included the functionality directly into the
multiplexer.


> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:106:
> vsync_observer_->OnUpdateVSyncParameters(timebase, interval);
> How does this additional indirection help?

The idea was to prevent the needs to read the scheduler settings and a
static_cast<> in this method.

As well, I'm hoping to shortly make the OutputSurface the VSync source and
bypass the scheduler all together.


>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:256:
> SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE);
> I remember intentionally not including the idle state here, although I forget
> the original reason atm. Why do you need it now?

The "void Scheduler::SetupNextBeginFrameWhenVSyncThrottlingDisabled(" includes
the IDLE state, which is where this came from.


> 
> https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler.h
> File cc/scheduler/scheduler.h (right):
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.h:34: virtual BeginFrameSource* GetBeginFrameSource() =
> 0;
> How about GetExternalBeginFrameSource()?
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.h:141: scoped_ptr<BeginFrameSource>
> primary_frame_source_store_;
> primary_frame_source_internal_?

Done.

>
https://codereview.chromium.org/267783004/diff/230001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.h (right):
> 
>
https://codereview.chromium.org/267783004/diff/230001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.h:115: public BeginFrameSource,
> Can the OutputSurface be the BeginFrameSource instead?

I agree that the OutputSurface should be the BeginFrameSource and we should
connect / disconnect it on the surface initalized / losted case. However as
mentioned above this patch was getting a little big in the process of me doing
that as it requires changing a lot of tests.

Will upload that patch shortly.

[mithro-old, 2014-09-15 14:48:17]

On 2014/09/15 13:17:00, Sami wrote:
>
https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
> File cc/scheduler/frame_source.cc (right):
> 
>
https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.cc:140: base::TimeDelta::FromInternalValue(-1));
> Can we use a valid interval (say, BeginFrameArgs::DefaultInterval) here? Or if
> you want an invalid one then please use zero. Otherwise,
base::TimeDelta::Max()
> is probably what you want here.

Changed to base::TimeDelta::Max()

> 
>
https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
> File cc/scheduler/frame_source.h (right):
> 
>
https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.h:70: virtual bool NeedsBeginFrames() = 0;
> nit: Should be const.

Done.

>
https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.h:75: virtual void FinishedFrame(size_t
> remaining_frames) = 0;
> nit: Call this something like DidFinishFrame() to match existing naming
> conventions.

Done

>
https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.h:77: void AddObserver(BeginFrameObserver* obs);
> Should we call these SetObserver/ClearObserver since there can only be one at
a
> time?

AddObserver / RemoveObserver seem to be the naming conventions. It's likely that
when this is moved into the browser process it'll want to support multiple
observers. I just wanted to keep it simple for now.

[mithro-old, 2014-09-16 14:36:48]

Hi,

Can you PTAL?

All tests pass except for
LayerTreeHostTestAbortedCommitDoesntStallDisabledVsync.RunMultiThread_DelegatingRenderer_MainThreadPaint
which seems to pass about 85% of the time. It appears to be some type of race
condition, because enabling tracing prevents the error from ever occurring! Any
ideas?

Tim

[Sami, 2014-09-16 17:09:09]

On 2014/09/16 14:36:48, mithro wrote:
> Hi,
> 
> Can you PTAL?
> 
> All tests pass except for
>
LayerTreeHostTestAbortedCommitDoesntStallDisabledVsync.RunMultiThread_DelegatingRenderer_MainThreadPaint
> which seems to pass about 85% of the time. It appears to be some type of race
> condition, because enabling tracing prevents the error from ever occurring!
Any
> ideas?

If you're desperate, you could try this ancient patch that lets you make a real
trace file from a unit test -- with much less overhead than tracing to the
console:

https://codereview.chromium.org/67073002/

It has probably bit-rotted a bit :(

[danakj, 2014-09-16 17:12:25]

Or this which will hopefully land when enne gets back
https://codereview.chromium.org/542893002/

On Tue, Sep 16, 2014 at 1:09 PM, <skyostil@chromium.org> wrote:

> On 2014/09/16 14:36:48, mithro wrote:
>
>> Hi,
>>
>
>  Can you PTAL?
>>
>
>  All tests pass except for
>>
>
> LayerTreeHostTestAbortedCommitDoesntStallDisabledVsync.RunMultiThread_
> DelegatingRenderer_MainThreadPaint
>
>> which seems to pass about 85% of the time. It appears to be some type of
>> race
>> condition, because enabling tracing prevents the error from ever
>> occurring!
>>
> Any
>
>> ideas?
>>
>
> If you're desperate, you could try this ancient patch that lets you make a
> real
> trace file from a unit test -- with much less overhead than tracing to the
> console:
>
> https://codereview.chromium.org/67073002/
>
> It has probably bit-rotted a bit :(
>
> https://codereview.chromium.org/267783004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2014-09-16 23:55:04]

> I'm not sure I understand what the difference between a BeginFrameSource and a
BeginFrameSender?

Nevermind, I wrote this before I realized your patch makes the Scheduler an
Observer.

> I've removed it for now and just included the functionality directly into the
multiplexer.

Thanks.

> The idea was to prevent the needs to read the scheduler settings and a
static_cast<> in this method.

Ah, I see. Since the SyntheticBeginFrameSource is the only possible vsync
observer, can we rename vsync_observer_ to synthetic_begin_frame_source_? Or do
you plan to have more types of vsync observers in the near future?

> The "void Scheduler::SetupNextBeginFrameWhenVSyncThrottlingDisabled(" includes
the IDLE state, which is where this came from.

Need to think about this some more. I forget why
SetupNextBeginFrameWhenVSyncThrottlingDisabled takes into account the IDLE
state, but SetupNextBeginFrameWhenVSyncThrottlingEnabled does not. If it
actually matters, I guess it's my fault for not writing a test or a comment
explaining why.

> I agree that the OutputSurface should be the BeginFrameSource...

Ok. Thanks for splitting that out into a separate patch. This one is already
geting pretty big.

https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:139: now + base::TimeDelta::FromSeconds(1000),
The current logic sets the deadline to be now+interval in order to make sure the
impl thread runs at at least 60fps without too much of a delay. Having the
deadline so far out will hurt the impl-thread responsiveness.

[mithro-old, 2014-09-17 09:30:03]

On 2014/09/16 23:55:04, brianderson wrote:
> > The idea was to prevent the needs to read the scheduler settings and a
> static_cast<> in this method.
> 
> Ah, I see. Since the SyntheticBeginFrameSource is the only possible vsync
> observer, can we rename vsync_observer_ to synthetic_begin_frame_source_? Or
do
> you plan to have more types of vsync observers in the near future?

The idea was to try and separate the VSyncParameterObserver concept from begin
SyntheticBeginFrameSource specific (if we had other frame sources that needed
vsync information). I can change it if you prefer?

This part might change in https://codereview.chromium.org/577643002/

> > The "void Scheduler::SetupNextBeginFrameWhenVSyncThrottlingDisabled("
includes
> the IDLE state, which is where this came from.
> 
> Need to think about this some more. I forget why
> SetupNextBeginFrameWhenVSyncThrottlingDisabled takes into account the IDLE
> state, but SetupNextBeginFrameWhenVSyncThrottlingEnabled does not. If it
> actually matters, I guess it's my fault for not writing a test or a comment
> explaining why.

I've remove the IDLE stuff and haven't seen any test break locally. Currently
investigating further.

Maybe we should try landing and see what breaks?

> > I agree that the OutputSurface should be the BeginFrameSource...
> 
> Ok. Thanks for splitting that out into a separate patch. This one is already
> geting pretty big.

Hopefully it makes it easier to review so I can land this patch soon.

>
https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
> File cc/scheduler/frame_source.cc (right):
> 
>
https://codereview.chromium.org/267783004/diff/290001/cc/scheduler/frame_sour...
> cc/scheduler/frame_source.cc:139: now + base::TimeDelta::FromSeconds(1000),
> The current logic sets the deadline to be now+interval in order to make sure
the
> impl thread runs at at least 60fps without too much of a delay. Having the
> deadline so far out will hurt the impl-thread responsiveness.

I've changed this to be identical to the original implementation. We can discuss
better values in a separate CL.

Tim

[mithro-old, 2014-09-17 13:11:30]

As far as I can tell, everything seems to work now.

PTAL, anything more needed before landing?

Tim

[brianderson, 2014-09-18 00:27:26]

Still need to look at the tests. But from what I've looked at so far, the patch
is getting very close.

https://codereview.chromium.org/267783004/diff/430001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/267783004/diff/430001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:400: Now(),
Now() -> now ?

https://codereview.chromium.org/267783004/diff/430001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:402: begin_retro_frame_args_.front().frame_time);
begin_retro_frame_args_.front().AsValue()?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:64: void BeginFrameSource::SendBeginFrame(const
BeginFrameArgs& args) {
"Send" implies that we're sending a message. Should we just call this
OnBeginFrame too?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:112: void
BackToBackBeginFrameSource::ScheduleSendBeginFrameArgs() {
ScheduleBeginFrame?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:126: void
BackToBackBeginFrameSource::SendBeginFrameArgs() {
SendBeginFrame -> BeginFrame?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:131: // if (!generate_frames_) {
Should this be !needs_begin_frames_?

Why do the tests depend on this?
Are we sure this isn't hiding real issues?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:136: // Set deadline somewhere a long time in the
future.
Comment doesn't match code anymore here.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:210: void
SyntheticBeginFrameSource::SendBeginFrameFromTick(
Please roll this into OnTimerTick.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:16: #include "ui/gfx/frame_time.h"
Don't think frame_time.h needs to be included in this header.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:23: class CC_EXPORT VSyncParameterObserver
Would it be less code to just use ui::CompositorVSyncManager::Observer rather
than having another class that does almost the same thing?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:72: // This method provides backpressure to a frame
source rather then toggling
then -> than

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:73: // SetGenerateFrames. It is used by systems like
the BackToBackFrameSource to
Don't think you need to reference SetGenerateFrames since it's a concept that no
longer exists in your newest patch.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:155: virtual void OnTimeBaseAndIntervalChange(
// VSyncParameterObserver

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:182: // BeginFrameObserver
It isn't immediately obvious why this needs to be an Observer. Can you add a
comment indicating that by acting as a observer proxy it can make sure the frame
times are monotonic.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:202: std::set<BeginFrameSource*> source_list_;
Since this class doesn't take ownership anymore, it seems funny to track a
source_list.

Could you make this work without AddSource, RemoveSource, HasSource, and
source_list_?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:279:
frame_source_->SetNeedsBeginFrames(needs_begin_frame);
Was worried a bit that you don't pass in the begin_retro_frame_args_ here
anymore, but I see that the SyntheticBeginFrame source gets around this by
triggering a BeginFrame.

The behavior ends up being slightly different though because a BeginFrame is
never skipped, but a BeginRetroFrame can be.

I really want to avoid the situation where we BeginFrame twice very fast when
coming out of idle, since that will put us straight into a high latency mode.

Passing begin_retro_frame_args_ to all sources seems like overkill. Instead,
maybe the Scheduler can track if the BeginFrame is the first BeginFrame and
treat that as if it were a BeginRetroFrame? Then the anti double ticking logic
would apply to all sources.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:144: BeginFrameSource* background_frame_source_ = NULL;
Should these be initialized in the constructor instead?
Also, why are they public rather than protected now?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:156: // Two phase setup needed because C++ don't call
the right virtual functions.
typo:
"because constructors" or "C++ doesn't"

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:156: // Two phase setup needed because C++ don't call
the right virtual functions.
Is there no way around this? FinalizeSetup seems very hacky. For testing
purposes, would it be okay to just undo what the Scheduler constructor does and
replace the frame sources? Or maybe add a new base constructor only used by
tests that skip initializing the frame sources? That way, the hackiness is only
exposed to tests.

[brianderson, 2014-09-18 00:31:26]

Sami, if you have time today can you look at the testing code?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:210: void
SyntheticBeginFrameSource::SendBeginFrameFromTick(
On 2014/09/18 00:27:25, brianderson wrote:
> Please roll this into OnTimerTick.

Nevermind about this one, I see how it's reused when coming out of idle.

[mithro-old, 2014-09-18 13:30:16]

Patchset #32 (id:630001) has been deleted

[mithro-old, 2014-09-18 13:33:38]

Hi Brian,

Think I fixed all your comments except the retro-frames bit.

Do you think an implementation which does something like "If frame_time is more
than XXX from now (IE quite 'late'), push to reto frames"? This could also
happen if the message queue got backed up or similar type of issue....

I'll also add a test for this case / situation.

Tim

https://codereview.chromium.org/267783004/diff/430001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/267783004/diff/430001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:400: Now(),
On 2014/09/18 00:27:24, brianderson wrote:
> Now() -> now ?

Done.

Move this bit into https://codereview.chromium.org/579083002/.

https://codereview.chromium.org/267783004/diff/430001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:402: begin_retro_frame_args_.front().frame_time);
On 2014/09/18 00:27:24, brianderson wrote:
> begin_retro_frame_args_.front().AsValue()?

Done.

Move this bit into https://codereview.chromium.org/579083002/.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:64: void BeginFrameSource::SendBeginFrame(const
BeginFrameArgs& args) {
On 2014/09/18 00:27:25, brianderson wrote:
> "Send" implies that we're sending a message. Should we just call this
> OnBeginFrame too?

It can't actually be named OnBeginFrame otherwise you can't implement both a
BeginFrameSource and BeginFrameObserver.

I changed to "CallOnBeginFrame", is that better?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:112: void
BackToBackBeginFrameSource::ScheduleSendBeginFrameArgs() {
On 2014/09/18 00:27:25, brianderson wrote:
> ScheduleBeginFrame?

Done.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:126: void
BackToBackBeginFrameSource::SendBeginFrameArgs() {
On 2014/09/18 00:27:25, brianderson wrote:
> SendBeginFrame -> BeginFrame?

Done.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:131: // if (!generate_frames_) {
On 2014/09/18 00:27:25, brianderson wrote:
> Should this be !needs_begin_frames_?

Fixed.

> Why do the tests depend on this?
> Are we sure this isn't hiding real issues?

Good catch.

The tests use to execute tasks out of order which meant this was needed. 
Now we have proper ordering

Removed (plus test updated to check this behavior works correctly).

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:136: // Set deadline somewhere a long time in the
future.
On 2014/09/18 00:27:25, brianderson wrote:
> Comment doesn't match code anymore here.

Removed.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:210: void
SyntheticBeginFrameSource::SendBeginFrameFromTick(
On 2014/09/18 00:31:25, brianderson wrote:
> On 2014/09/18 00:27:25, brianderson wrote:
> > Please roll this into OnTimerTick.
> 
> Nevermind about this one, I see how it's reused when coming out of idle.

Acknowledged.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:210: void
SyntheticBeginFrameSource::SendBeginFrameFromTick(
On 2014/09/18 00:27:25, brianderson wrote:
> Please roll this into OnTimerTick.

Acknowledged.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:16: #include "ui/gfx/frame_time.h"
On 2014/09/18 00:27:25, brianderson wrote:
> Don't think frame_time.h needs to be included in this header.

Done.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:23: class CC_EXPORT VSyncParameterObserver
On 2014/09/18 00:27:25, brianderson wrote:
> Would it be less code to just use ui::CompositorVSyncManager::Observer rather
> than having another class that does almost the same thing?

Yeah, I think we can probably ditch this class now. It originally existed
because the scheduler used the vsync interval but that seems to have gone away
now.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:72: // This method provides backpressure to a frame
source rather then toggling
On 2014/09/18 00:27:25, brianderson wrote:
> then -> than

Done.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:73: // SetGenerateFrames. It is used by systems like
the BackToBackFrameSource to
On 2014/09/18 00:27:25, brianderson wrote:
> Don't think you need to reference SetGenerateFrames since it's a concept that
no
> longer exists in your newest patch.

Done.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:155: virtual void OnTimeBaseAndIntervalChange(
On 2014/09/18 00:27:25, brianderson wrote:
> // VSyncParameterObserver

Done.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:182: // BeginFrameObserver
On 2014/09/18 00:27:25, brianderson wrote:
> It isn't immediately obvious why this needs to be an Observer. Can you add a
> comment indicating that by acting as a observer proxy it can make sure the
frame
> times are monotonic.

Added comment.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:202: std::set<BeginFrameSource*> source_list_;
On 2014/09/18 00:27:25, brianderson wrote:
> Since this class doesn't take ownership anymore, it seems funny to track a
> source_list.
> 
> Could you make this work without AddSource, RemoveSource, HasSource, and
> source_list_?

I think having AddSource/RemoveSource/HasSource is kind of a nice sanity check
that you are using the right sources with the right Mux. We could making them
NOP operations for NDEBUG builds?

Can remove them if you feel strongly.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:279:
frame_source_->SetNeedsBeginFrames(needs_begin_frame);
On 2014/09/18 00:27:26, brianderson wrote:
> Was worried a bit that you don't pass in the begin_retro_frame_args_ here
> anymore, but I see that the SyntheticBeginFrame source gets around this by
> triggering a BeginFrame.
> 
> The behavior ends up being slightly different though because a BeginFrame is
> never skipped, but a BeginRetroFrame can be.
> 
> I really want to avoid the situation where we BeginFrame twice very fast when
> coming out of idle, since that will put us straight into a high latency mode.
> 
> Passing begin_retro_frame_args_ to all sources seems like overkill. Instead,
> maybe the Scheduler can track if the BeginFrame is the first BeginFrame and
> treat that as if it were a BeginRetroFrame? Then the anti double ticking logic
> would apply to all sources. 

I still need to do this, but wanted to upload before I finished for today. Will
do it tomorrow.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:144: BeginFrameSource* background_frame_source_ = NULL;
On 2014/09/18 00:27:26, brianderson wrote:
> Should these be initialized in the constructor instead?

After chatting with the C++ experts here, it turns out there is no actual
difference between doing it initializer list verses here.

The Chrome idiom is to use the constructor, so I changed it as you requested.

> Also, why are they public rather than protected now?

Opps! Fixed.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:156: // Two phase setup needed because C++ don't call
the right virtual functions.
On 2014/09/18 00:27:26, brianderson wrote:
> Is there no way around this? FinalizeSetup seems very hacky. For testing
> purposes, would it be okay to just undo what the Scheduler constructor does
and
> replace the frame sources? Or maybe add a new base constructor only used by
> tests that skip initializing the frame sources? That way, the hackiness is
only
> exposed to tests.

I had chat to the local C++ experts and came up with the following solution. Do
you think it's better?

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:156: // Two phase setup needed because C++ don't call
the right virtual functions.
On 2014/09/18 00:27:26, brianderson wrote:
> typo:
> "because constructors" or "C++ doesn't"

Removed.

[Sami, 2014-09-18 13:55:09]

Test coverage looks pretty good to me. One naive question about
LastBeginFrameArgs(). Sorry if I missed any previous debate about it.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:5: #ifndef CC_SCHEDULER_FRAME_SOURCE_H_
nit: I'd call this begin_frame_source.h since that matches the classes.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:20: /**
Missing some documentation here?

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:51: *
Ditto.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:55: virtual const BeginFrameArgs&
LastBeginFrameArgs() const = 0;
If I understood correctly, this is mostly used for testing and tracing. Only the
multiplexer seems to use this for actual functionality, right? If this is the
case, it feels like this shouldn't be part of the observer interface and the
multiplexer should just keep a local copy of the most recent BeginFrame it got.
I think this may help to simplify the tests since the mock can just have the
last args as a field. WDYT?

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:63: *
Ditto.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source_unittest.cc (right):

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source_unittest.cc:45: // MOCK_CONST_METHOD0(AsValueInto,
void(base::debug::TracedValue*));
Unused?

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source_unittest.cc:136: virtual void OnBeginFrame(const
BeginFrameArgs& args) OVERRIDE{};
nit: space before {}.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source_unittest.cc:180: static int64_t kDeadline;
const?

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:173: BeginFrameArgs last_begin_frame_args_;
Is this ever set?

https://codereview.chromium.org/267783004/diff/610001/cc/test/ordered_simple_...
File cc/test/ordered_simple_task_runner.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/test/ordered_simple_...
cc/test/ordered_simple_task_runner.h:77: bool HasPendingTasks();
nit: const

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:30: bool tick_called_ = false;
I think this is a C++11-ism which we don't allow yet.

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:77: bool needs_begin_frames_ = false;
Ditto.

https://codereview.chromium.org/267783004/diff/610001/cc/trees/thread_proxy.h
File cc/trees/thread_proxy.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/trees/thread_proxy.h...
cc/trees/thread_proxy.h:212: virtual BeginFrameSource*
GetExternalBeginFrameSource() OVERRIDE;
nit: Drop the |Get|, it's cleaner :)

[brianderson, 2014-09-18 21:54:19]

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:64: void BeginFrameSource::SendBeginFrame(const
BeginFrameArgs& args) {
On 2014/09/18 13:33:37, mithro wrote:
> On 2014/09/18 00:27:25, brianderson wrote:
> > "Send" implies that we're sending a message. Should we just call this
> > OnBeginFrame too?
> 
> It can't actually be named OnBeginFrame otherwise you can't implement both a
> BeginFrameSource and BeginFrameObserver.
> 
> I changed to "CallOnBeginFrame", is that better?

CallOnBeginFrame sgtm.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:202: std::set<BeginFrameSource*> source_list_;
On 2014/09/18 13:33:37, mithro wrote:
> On 2014/09/18 00:27:25, brianderson wrote:
> > Since this class doesn't take ownership anymore, it seems funny to track a
> > source_list.
> > 
> > Could you make this work without AddSource, RemoveSource, HasSource, and
> > source_list_?
> 
> I think having AddSource/RemoveSource/HasSource is kind of a nice sanity check
> that you are using the right sources with the right Mux. We could making them
> NOP operations for NDEBUG builds?
> 
> Can remove them if you feel strongly.

I don't feel strongly. Don't worry about NDEBUG.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:156: // Two phase setup needed because C++ don't call
the right virtual functions.
On 2014/09/18 13:33:38, mithro wrote:
> On 2014/09/18 00:27:26, brianderson wrote:
> > Is there no way around this? FinalizeSetup seems very hacky. For testing
> > purposes, would it be okay to just undo what the Scheduler constructor does
> and
> > replace the frame sources? Or maybe add a new base constructor only used by
> > tests that skip initializing the frame sources? That way, the hackiness is
> only
> > exposed to tests.
> 
> I had chat to the local C++ experts and came up with the following solution.
Do
> you think it's better?

Neat. I like the SchedulerFrameSourcesConstructor solution.

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:82: void TestSendBeginFrame(const
BeginFrameArgs& args) {
TestOnBeginFrame.

https://codereview.chromium.org/267783004/diff/650001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/650001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:140: * to preserves the monoticity of the
BeginFrameArgs when switching sources.
to preserve

https://codereview.chromium.org/267783004/diff/650001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/650001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:56: class CC_EXPORT SchedulerFrameSourcesConstructor {
Can you add a comment explaining that this allows tests to inject custom
BeginFrameSources? That way we save people from trying to get rid of this only
to find out they can't. =)

[mithro-old, 2014-09-19 02:45:40]

Hi Brian / Sami,

I think I've gotten all your review comments.

Sami, I had a discussion with Brian this morning and there were a couple of
ideas;
 * Brian wants the old retro-frame behaviour for the SyntheticBeginFrame to be
preserved in this CL. (He also promised to land a test for that functionality
after this one lands). To make this happen we went with adding a
"OnMissedBeginFrame" method to the observer. We discussed some alternative
approaches which would allow us to remove this method but want to wait until
after this patch to do them.

 * See the bit about LastBeginFrameArgs.

 * According to pinman cc/scheduler isn't allow to depend on ui/compositor (see
https://codereview.chromium.org/577643002/). Neither Brian or myself have come
up with a better solution then just cloning the interface - open to ideas.

Fixing your comments seems to have broken some of the tests, hopefully I'll
upload a newer version which makes them all pass before either of you get a
chance to see this CL again.

Tim 'mithro' Ansell

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:64: void BeginFrameSource::SendBeginFrame(const
BeginFrameArgs& args) {
On 2014/09/18 21:54:18, brianderson wrote:
> On 2014/09/18 13:33:37, mithro wrote:
> > On 2014/09/18 00:27:25, brianderson wrote:
> > > "Send" implies that we're sending a message. Should we just call this
> > > OnBeginFrame too?
> > 
> > It can't actually be named OnBeginFrame otherwise you can't implement both a
> > BeginFrameSource and BeginFrameObserver.
> > 
> > I changed to "CallOnBeginFrame", is that better?
> 
> CallOnBeginFrame sgtm.

Acknowledged.

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:202: std::set<BeginFrameSource*> source_list_;
On 2014/09/18 21:54:18, brianderson wrote:
> On 2014/09/18 13:33:37, mithro wrote:
> > On 2014/09/18 00:27:25, brianderson wrote:
> > > Since this class doesn't take ownership anymore, it seems funny to track a
> > > source_list.
> > > 
> > > Could you make this work without AddSource, RemoveSource, HasSource, and
> > > source_list_?
> > 
> > I think having AddSource/RemoveSource/HasSource is kind of a nice sanity
check
> > that you are using the right sources with the right Mux. We could making
them
> > NOP operations for NDEBUG builds?
> > 
> > Can remove them if you feel strongly.
> 
> I don't feel strongly. Don't worry about NDEBUG.

Actually, should these be some type of weak_ptr? I don't quite understand when
the correct time/place to use a weak_ptr is...

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/570001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:156: // Two phase setup needed because C++ don't call
the right virtual functions.
On 2014/09/18 21:54:19, brianderson wrote:
> Neat. I like the SchedulerFrameSourcesConstructor solution.

Glad you approve. I personally don't, but then I don't like C++ very much :P

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:5: #ifndef CC_SCHEDULER_FRAME_SOURCE_H_
On 2014/09/18 13:55:08, Sami wrote:
> nit: I'd call this begin_frame_source.h since that matches the classes.

Is this suggestion to rename the file?

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:20: /**
On 2014/09/18 13:55:08, Sami wrote:
> Missing some documentation here?

This class has gone.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:51: *
On 2014/09/18 13:55:08, Sami wrote:
> Ditto.

Done.

I've expanded the documentation substantially to document all the preconditions
that an Observer can expect.

From what I can see, Chromium style prefers // style comments, so I also made
all comments of that format.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:55: virtual const BeginFrameArgs&
LastBeginFrameArgs() const = 0;
On 2014/09/18 13:55:08, Sami wrote:
> If I understood correctly, this is mostly used for testing and tracing. Only
the
> multiplexer seems to use this for actual functionality, right? If this is the
> case, it feels like this shouldn't be part of the observer interface and the
> multiplexer should just keep a local copy of the most recent BeginFrame it
got.
> I think this may help to simplify the tests since the mock can just have the
> last args as a field. WDYT?

So, I had a chat with Brian about this topic this morning. Initially he was
skeptical but I believe he has come around to the idea.

I've added a bunch of documentation to this class which should explain why this
method exists. Can you read that and see if your convinced? (It not then I need
to update the documentation I guess).

I agree that this method does make the mock a lot more complicated and wish
there was an easier way but C++ does really limits your options. (I long for the
days of just quickly monkey patching something for testing :)

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:63: *
On 2014/09/18 13:55:08, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source_unittest.cc (right):

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source_unittest.cc:45: // MOCK_CONST_METHOD0(AsValueInto,
void(base::debug::TracedValue*));
On 2014/09/18 13:55:08, Sami wrote:
> Unused?

Removed.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source_unittest.cc:136: virtual void OnBeginFrame(const
BeginFrameArgs& args) OVERRIDE{};
On 2014/09/18 13:55:08, Sami wrote:
> nit: space before {}.

Done.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source_unittest.cc:180: static int64_t kDeadline;
On 2014/09/18 13:55:08, Sami wrote:
> const?

Done.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:173: BeginFrameArgs last_begin_frame_args_;
On 2014/09/18 13:55:08, Sami wrote:
> Is this ever set?

It should have been, but has been removed now.

https://codereview.chromium.org/267783004/diff/610001/cc/test/ordered_simple_...
File cc/test/ordered_simple_task_runner.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/test/ordered_simple_...
cc/test/ordered_simple_task_runner.h:77: bool HasPendingTasks();
On 2014/09/18 13:55:09, Sami wrote:
> nit: const

Done.

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:30: bool tick_called_ = false;
On 2014/09/18 13:55:09, Sami wrote:
> I think this is a C++11-ism which we don't allow yet.

As Chrome style doesn't seem to used this, I've removed it.

Just FYI, this is actually perfectly valid C++98 if the type is "int compatible"
(IE bool, int, pointer, etc - but not double!). What changes in C++11 is that
this syntax is extended to support more than "int compatible" values through the
use of constexpr.

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:77: bool needs_begin_frames_ = false;
On 2014/09/18 13:55:09, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:82: void TestSendBeginFrame(const
BeginFrameArgs& args) {
On 2014/09/18 21:54:19, brianderson wrote:
> TestOnBeginFrame.

Done.

https://codereview.chromium.org/267783004/diff/610001/cc/trees/thread_proxy.h
File cc/trees/thread_proxy.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/trees/thread_proxy.h...
cc/trees/thread_proxy.h:212: virtual BeginFrameSource*
GetExternalBeginFrameSource() OVERRIDE;
On 2014/09/18 13:55:09, Sami wrote:
> nit: Drop the |Get|, it's cleaner :)

Done.

https://codereview.chromium.org/267783004/diff/650001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/650001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:140: * to preserves the monoticity of the
BeginFrameArgs when switching sources.
On 2014/09/18 21:54:19, brianderson wrote:
> to preserve

Done.

https://codereview.chromium.org/267783004/diff/650001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/650001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:56: class CC_EXPORT SchedulerFrameSourcesConstructor {
On 2014/09/18 21:54:19, brianderson wrote:
> Can you add a comment explaining that this allows tests to inject custom
> BeginFrameSources? That way we save people from trying to get rid of this only
> to find out they can't. =)

Done.

[Sami, 2014-09-19 13:44:22]

Thanks Tim, comments below.

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:5: #ifndef CC_SCHEDULER_FRAME_SOURCE_H_
On 2014/09/19 02:45:39, mithro wrote:
> On 2014/09/18 13:55:08, Sami wrote:
> > nit: I'd call this begin_frame_source.h since that matches the classes.
> 
> Is this suggestion to rename the file?

Yes, sorry for not being clear. In my mind BeginFrame and frame are very
different concepts but that may just be me :)

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:30: bool tick_called_ = false;
On 2014/09/19 02:45:39, mithro wrote:
> On 2014/09/18 13:55:09, Sami wrote:
> > I think this is a C++11-ism which we don't allow yet.
> 
> As Chrome style doesn't seem to used this, I've removed it.
> 
> Just FYI, this is actually perfectly valid C++98 if the type is "int
compatible"
> (IE bool, int, pointer, etc - but not double!). What changes in C++11 is that
> this syntax is extended to support more than "int compatible" values through
the
> use of constexpr.

Interesting, I didn't know that.

https://codereview.chromium.org/267783004/diff/750001/cc/DEPS
File cc/DEPS (right):

https://codereview.chromium.org/267783004/diff/750001/cc/DEPS#newcode15
cc/DEPS:15: "+ui/compositor/compositor_vsync_manager.h",
Do we still need this dep?

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:30: virtual void OnBeginFrame(const BeginFrameArgs&
frame_info) = 0;
Please call this |args| or |begin_frame_args| since that's done elsewhere in the
codebase.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:42: //    BeginFramArgs on which IsValid() returns
false.
typo: BeginFrameArgs

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:57: virtual const BeginFrameArgs
LastBeginFrameArgs() const = 0;
Is it so that the return value from LastBeginFrameArgs will only ever change
inside calls to OnBeginFrame? If so, how about we give OnBeginFrame a return
enum or an out parameter that conveys the same information? Otherwise I think
this should be called something like BeginFrameSourceClient to highlight it's
not a simpler observer.

Also, LastUsedBeginFrameArgs() might make the relationship more obvious.

[brianderson, 2014-09-23 01:31:06]

https://codereview.chromium.org/267783004/diff/690001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/690001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:73: class CC_EXPORT BeginFrameObserverImpl : public
BeginFrameObserver {
Is the BeginFrameObserverImpl really needed?

https://codereview.chromium.org/267783004/diff/690001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/267783004/diff/690001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:352: bool Scheduler::OnBeginFrameImpl(const
BeginFrameArgs& args) {
Why did this become OnBeginFrameImpl? A little confusing, since we have
BeginImplFrame too...

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:30: virtual void OnBeginFrame(const BeginFrameArgs&
frame_info) = 0;
On 2014/09/19 13:44:22, Sami wrote:
> Please call this |args| or |begin_frame_args| since that's done elsewhere in
the
> codebase.

+1

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:54: // These requirements are design to allow
chaining and nesting of
design -> designed

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:112: class CC_EXPORT BeginFrameSource {
Do you want to land this version of BeginFrameSource first and then make it pure
virtual when we make the "OutputSurface" implement BeginFrameSource?

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:187: public ui::CompositorVSyncManager::Observer,
BrowserCompositorOutputSurface implements ui::CompositorVSyncManager::Observer.
We should be able to translate that to
OutputSurfaceClient::CommitVSyncParameters and thread it to the
SyntheticBeginFrameSource without depending on
ui::CompositorVSyncManager::Observer here.

Then, after the unified BeginFrame patch, I think we can get rid of
ui::CompositorVSyncManager::Observer since it becomes an unnecessary middle man
between BrowserCompositorOutputSurface::OnUpdateVSyncParametersFromGpu and
OutputSurfaceClient::CommitVSyncParameters.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:263: std::set<BeginFrameSource*> source_list_;
The frame sources should outlive the multiplexer, so I think we are good with
raw pointers here.

You only need weak pointers when you are afraid you might try to access the
pointer after it's been deallocated and it's okay to turn any calls on the weak
pointer into no-ops if they occur after deallocation. Usually this happens when
you post a task and then deallocate a dependency before the task actually
executes.

[simonhong, 2014-09-23 04:39:09]

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:38: DCHECK(args.frame_time >=
last_begin_frame_args_.frame_time);
Is it possible to equal?

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:60: : observer_(NULL), inside_as_value_into_(false)
{
I think below DCHECKs can be removed if you don't suspect
compiler ;)

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:21: class CC_EXPORT BeginFrameObserver {
How about using BeginFrameSourceObserver instead of BeginFrameObserver?
I think this can make more readable.
(But, not a strong opinion.)

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:107: // Base classes should:
Sub-classes instead of Base classes?

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:204: // ui::CompositorVSyncManager::Observer
If use use ui::CompositorVSyncManager::Observer for code re-use and want to use
observer pattern here, I think new observer class should be better than re-using
that. That observer class is used in the ui compositor layer.

Or,calling directly like primary_frame_source_->UpdateVSyncParameters() also
looks fine.

[mithro-old, 2014-09-23 12:41:14]

Patchset #40 (id:810001) has been deleted

[mithro-old, 2014-09-23 12:43:55]

Hi,

PTAL. I believe I have gotten all your review comments.

The biggest change to the code is from Sami's idea to have OnBeginFrame methods
return an enum value. Having it working now, I think it was probably a pretty
good idea as it meant I could remove a bunch of comments from the tests because
they became self explanatory (which I believe is always a good sign).

Is there anything else before we can land this patch?

Tim 'mithro' Ansell

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:5: #ifndef CC_SCHEDULER_FRAME_SOURCE_H_
On 2014/09/19 13:44:22, Sami wrote:
> On 2014/09/19 02:45:39, mithro wrote:
> > On 2014/09/18 13:55:08, Sami wrote:
> > > nit: I'd call this begin_frame_source.h since that matches the classes.
> > 
> > Is this suggestion to rename the file?
> 
> Yes, sorry for not being clear. In my mind BeginFrame and frame are very
> different concepts but that may just be me :)

I'm going to regret this but,

I was just pondering if the "BeginFrameSource/BeginFrameObserver" interface
should be in cc/output with BeginFrameArgs?

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/267783004/diff/610001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:30: bool tick_called_ = false;
On 2014/09/19 13:44:22, Sami wrote:
> On 2014/09/19 02:45:39, mithro wrote:
> > On 2014/09/18 13:55:09, Sami wrote:
> > > I think this is a C++11-ism which we don't allow yet.
> > 
> > As Chrome style doesn't seem to used this, I've removed it.
> > 
> > Just FYI, this is actually perfectly valid C++98 if the type is "int
> compatible"
> > (IE bool, int, pointer, etc - but not double!). What changes in C++11 is
that
> > this syntax is extended to support more than "int compatible" values through
> the
> > use of constexpr.
> 
> Interesting, I didn't know that.

It has the nice feature that if you forget to initialize the value in one of the
constructors, it still gets a value rather than it being randomly determined by
what the contents of the memory was before (and hence spend hours trying to
figure out why your test is non-deterministic but never fails when you enable
tracing :/).

https://codereview.chromium.org/267783004/diff/690001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/690001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:73: class CC_EXPORT BeginFrameObserverImpl : public
BeginFrameObserver {
On 2014/09/23 01:31:05, brianderson wrote:
> Is the BeginFrameObserverImpl really needed?

I've renamed this class to BeginFrameObserverMixIn to be clearer what it is for.

It saves quite a bit of common code between classes which implement the Observer
(there is the Scheduler and a bunch are found in testing).

As requested I've also made the OnBeginFrameImpl method name not confusing with
the OnBeginImplFrame method naming.

https://codereview.chromium.org/267783004/diff/690001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/267783004/diff/690001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:352: bool Scheduler::OnBeginFrameImpl(const
BeginFrameArgs& args) {
On 2014/09/23 01:31:06, brianderson wrote:
> Why did this become OnBeginFrameImpl? A little confusing, since we have
> BeginImplFrame too...

Fixed. Should be significantly clearer now.

https://codereview.chromium.org/267783004/diff/750001/cc/DEPS
File cc/DEPS (right):

https://codereview.chromium.org/267783004/diff/750001/cc/DEPS#newcode15
cc/DEPS:15: "+ui/compositor/compositor_vsync_manager.h",
On 2014/09/19 13:44:22, Sami wrote:
> Do we still need this dep?

Removed.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.cc:38: DCHECK(args.frame_time >=
last_begin_frame_args_.frame_time);
On 2014/09/23 04:39:09, simonhong wrote:
> Is it possible to equal?

Currently this occurs in tests (and I don't want to fix that issue in this CL
too). When they are eventually fixed to create frame times which increase, then
we can move to the more strict greater than.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:30: virtual void OnBeginFrame(const BeginFrameArgs&
frame_info) = 0;
On 2014/09/19 13:44:22, Sami wrote:
> Please call this |args| or |begin_frame_args| since that's done elsewhere in
the
> codebase.

I original had |args| but a co-worker pointed out that it could be easily
confused with "all the arguments to this function" (hence the change to
frame_info name).

I've changed it back to |args| now.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:30: virtual void OnBeginFrame(const BeginFrameArgs&
frame_info) = 0;
On 2014/09/23 01:31:06, brianderson wrote:
> On 2014/09/19 13:44:22, Sami wrote:
> > Please call this |args| or |begin_frame_args| since that's done elsewhere in
> the
> > codebase.
> 
> +1

Acknowledged.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:42: //    BeginFramArgs on which IsValid() returns
false.
On 2014/09/19 13:44:22, Sami wrote:
> typo: BeginFrameArgs

Done.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:54: // These requirements are design to allow
chaining and nesting of
On 2014/09/23 01:31:06, brianderson wrote:
> design -> designed

Done.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:57: virtual const BeginFrameArgs
LastBeginFrameArgs() const = 0;
On 2014/09/19 13:44:22, Sami wrote:
> Is it so that the return value from LastBeginFrameArgs will only ever change
> inside calls to OnBeginFrame?

Yes, that is correct - LastUsedBeginFrameArgs should only ever change inside the
OnBeginFrame method.

> If so, how about we give OnBeginFrame a return
> enum or an out parameter that conveys the same information?

I'm not sure I fully understand but I gave this idea a go, does it look okay?

> Otherwise I think
> this should be called something like BeginFrameSourceClient to highlight it's
> not a simpler observer.

The patch original called it a BeginFrameSink, but I changed it to
BeginFrameObserver to match the AddObserver/RemoveObserver methods.

> Also, LastUsedBeginFrameArgs() might make the relationship more obvious.

Using this name now.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:107: // Base classes should:
On 2014/09/23 04:39:09, simonhong wrote:
> Sub-classes instead of Base classes?

Done.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:112: class CC_EXPORT BeginFrameSource {
On 2014/09/23 01:31:06, brianderson wrote:
> Do you want to land this version of BeginFrameSource first and then make it
pure
> virtual when we make the "OutputSurface" implement BeginFrameSource?

I think we are leaning towards abandoning this idea now. See further discussion
at http://crbug.com/416760

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:187: public ui::CompositorVSyncManager::Observer,
On 2014/09/23 01:31:06, brianderson wrote:
> BrowserCompositorOutputSurface implements
ui::CompositorVSyncManager::Observer.
> We should be able to translate that to
> OutputSurfaceClient::CommitVSyncParameters and thread it to the
> SyntheticBeginFrameSource without depending on
> ui::CompositorVSyncManager::Observer here.
> 
> Then, after the unified BeginFrame patch, I think we can get rid of
> ui::CompositorVSyncManager::Observer since it becomes an unnecessary middle
man
> between BrowserCompositorOutputSurface::OnUpdateVSyncParametersFromGpu and
> OutputSurfaceClient::CommitVSyncParameters.

I'm not sure I understand what you are saying correctly. I've created the new
VSyncParamaterObserver interface in cc/output/vsync_parameter_observer.h file.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:204: // ui::CompositorVSyncManager::Observer
On 2014/09/23 04:39:09, simonhong wrote:
> If use use ui::CompositorVSyncManager::Observer for code re-use and want to
use
> observer pattern here, I think new observer class should be better than
re-using
> that. That observer class is used in the ui compositor layer.
> 
> Or,calling directly like primary_frame_source_->UpdateVSyncParameters() also
> looks fine.

See my response to Brian's comment.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:263: std::set<BeginFrameSource*> source_list_;
On 2014/09/23 01:31:06, brianderson wrote:
> The frame sources should outlive the multiplexer, so I think we are good with
> raw pointers here.
> 
> You only need weak pointers when you are afraid you might try to access the
> pointer after it's been deallocated and it's okay to turn any calls on the
weak
> pointer into no-ops if they occur after deallocation. Usually this happens
when
> you post a task and then deallocate a dependency before the task actually
> executes.

Okay.

[brianderson, 2014-09-24 06:03:06]

> Is there anything else before we can land this patch?

Mostly small things left, but the two big open things I see are:
1) Are others comfortable with the implementation inheritance from
BeginFrameSource and BeginFrameObserverMixIn? They reduce code duplication, but
add a bit of indirection.
2) I think we should either use LastUsedBeginFrameArgs or enums, but not both.

I took a look at the scheduler unit tests and those look good. The only thing I
haven't had time to grok yet are the begin_frame_source_unittest.

> I was just pondering if the "BeginFrameSource/BeginFrameObserver" interface
should be in cc/output with BeginFrameArgs?

Or maybe move BeginFrameArgs to cc/scheduler. =) It makes sense either way.
Sometimes they are tied to an OutputSurface and other times they aren't though,
so maybe they make sense more often in cc/scheduler.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:57: virtual const BeginFrameArgs
LastBeginFrameArgs() const = 0;
On 2014/09/23 12:43:55, mithro wrote:
> On 2014/09/19 13:44:22, Sami wrote:
> > Is it so that the return value from LastBeginFrameArgs will only ever change
> > inside calls to OnBeginFrame?
> 
> Yes, that is correct - LastUsedBeginFrameArgs should only ever change inside
the
> OnBeginFrame method.
> 
> > If so, how about we give OnBeginFrame a return
> > enum or an out parameter that conveys the same information?
> 
> I'm not sure I fully understand but I gave this idea a go, does it look okay?
> 
> > Otherwise I think
> > this should be called something like BeginFrameSourceClient to highlight
it's
> > not a simpler observer.
> 
> The patch original called it a BeginFrameSink, but I changed it to
> BeginFrameObserver to match the AddObserver/RemoveObserver methods.
> 
> > Also, LastUsedBeginFrameArgs() might make the relationship more obvious.
> 
> Using this name now.
> 

Sami, were you suggesting the enums should replace LastUsedBeginFrameArgs()?
There seems to be a lot of overlap between the two. I like the
LastUsedBeginFrameArgs() since the Observer/Client is the one who really decides
that and is going to store a copy of it anyway; no need to store it twice.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:63: return OnBeginFrame(args);
Looks like this is redirecting to the wrong method.  Should have something like
OnMissedBeginFrameMixInDelegate.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:518: // If the last begin frame is invalid,
than any new begin frame is valid.
than -> then

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:524: return (  // args.frame_time >
observer_->LastUsedBeginFrameArgs().frame_time
Remove commented out code.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:65: virtual const BeginFrameArgs
LastUsedBeginFrameArgs() const = 0;
const BeginFrameArgs&

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:13: #include
"testing/gmock/include/gmock/gmock.h"
I don't understand gmock well enough to review this file quickly. I'll try to
look at it more tomorrow.

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

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:59:
client_->actions_.push_back("SetNeedsBeginFrames(true)");
Thanks for differentiating between the pos/neg edge.

https://codereview.chromium.org/267783004/diff/830001/cc/test/scheduler_test_...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/267783004/diff/830001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:30: bool tick_called_ = false;
Initialize in constructor.

[Sami, 2014-09-24 11:12:54]

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:57: virtual const BeginFrameArgs
LastBeginFrameArgs() const = 0;
On 2014/09/24 06:03:05, brianderson wrote:
> On 2014/09/23 12:43:55, mithro wrote:
> > On 2014/09/19 13:44:22, Sami wrote:
> > > Is it so that the return value from LastBeginFrameArgs will only ever
change
> > > inside calls to OnBeginFrame?
> > 
> > Yes, that is correct - LastUsedBeginFrameArgs should only ever change inside
> the
> > OnBeginFrame method.
> > 
> > > If so, how about we give OnBeginFrame a return
> > > enum or an out parameter that conveys the same information?
> > 
> > I'm not sure I fully understand but I gave this idea a go, does it look
okay?
> > 
> > > Otherwise I think
> > > this should be called something like BeginFrameSourceClient to highlight
> it's
> > > not a simpler observer.
> > 
> > The patch original called it a BeginFrameSink, but I changed it to
> > BeginFrameObserver to match the AddObserver/RemoveObserver methods.
> > 
> > > Also, LastUsedBeginFrameArgs() might make the relationship more obvious.
> > 
> > Using this name now.
> > 
> 
> Sami, were you suggesting the enums should replace LastUsedBeginFrameArgs()?
> There seems to be a lot of overlap between the two. I like the
> LastUsedBeginFrameArgs() since the Observer/Client is the one who really
decides
> that and is going to store a copy of it anyway; no need to store it twice.

Yes, let's have one or the other but not both because that is redundant. If
Brian's happy with just having LastUsedBeginFrameArgs() then I am too :)

https://codereview.chromium.org/267783004/diff/830001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/267783004/diff/830001/cc/BUILD.gn#newcode433
cc/BUILD.gn:433: "scheduler/delay_based_time_source.cc",
Is vsync_parameter_observer.h missing from this file?

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:16: #define EXPECT_USED
cc::BeginFrameObserver::BeginFrameArgsStatus::USED
I think "using" declarations would be cleaner than these. You could add a few
for gmock too to get rid of the ::testing:: noise.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:22: switch (status) {               
                                      \
Maybe it's just me but this feels a little too clever for its own good. I'm
picturing the errors from gmock when an expectation fails and wondering if
anyone can make sense out of them.

Could you at least split this into EXPECT_BEGIN_FRAME_USED and _DROPPED because
the two branches aren't sharing any code.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:58: #define P(pointer) (*pointer)
I think just using (*pointer) would be more readable. If you worry about the
parens then let's handle that inside the macro instead.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:61: class CC_EXPORT SchedulerFrameSourcesConstructor {
This seems more elaborate than it needs to be. How about adding a second
constructor for the Scheduler which takes the begin frame sources as parameters?
We can make that constructor hidden to everyone but tests if necessary.

[mithro-old, 2014-09-24 17:14:55]

On 2014/09/24 06:03:06, brianderson wrote:
> > Is there anything else before we can land this patch?
> 
> Mostly small things left, but the two big open things I see are:
> 1) Are others comfortable with the implementation inheritance from
> BeginFrameSource and BeginFrameObserverMixIn? They reduce code duplication,
but
> add a bit of indirection.

I think the benefit of the extra sanity checks these methods give are the most
useful feature of them. Making sure the BeginFrameSources actually obey the
contract will save us so many problems in the future (and hopefully prevent
people from hacking round the problem by using Now()!).

> 2) I think we should either use LastUsedBeginFrameArgs or enums, but not both.

Enums have been removed in favour of LastUsedBeginFrameArgs

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
File cc/scheduler/frame_source.h (right):

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:57: virtual const BeginFrameArgs
LastBeginFrameArgs() const = 0;
On 2014/09/24 06:03:05, brianderson wrote:
> On 2014/09/23 12:43:55, mithro wrote:
> > On 2014/09/19 13:44:22, Sami wrote:
> > > Is it so that the return value from LastBeginFrameArgs will only ever
change
> > > inside calls to OnBeginFrame?
> > 
> > Yes, that is correct - LastUsedBeginFrameArgs should only ever change inside
> the
> > OnBeginFrame method.
> > 
> > > If so, how about we give OnBeginFrame a return
> > > enum or an out parameter that conveys the same information?
> > 
> > I'm not sure I fully understand but I gave this idea a go, does it look
okay?
> > 
> > > Otherwise I think
> > > this should be called something like BeginFrameSourceClient to highlight
> it's
> > > not a simpler observer.
> > 
> > The patch original called it a BeginFrameSink, but I changed it to
> > BeginFrameObserver to match the AddObserver/RemoveObserver methods.
> > 
> > > Also, LastUsedBeginFrameArgs() might make the relationship more obvious.
> > 
> > Using this name now.
> > 
> 
> Sami, were you suggesting the enums should replace LastUsedBeginFrameArgs()?
> There seems to be a lot of overlap between the two. I like the
> LastUsedBeginFrameArgs() since the Observer/Client is the one who really
decides
> that and is going to store a copy of it anyway; no need to store it twice.

Enum is removed in favour of LastUsedBeginFrameArgs now.

https://codereview.chromium.org/267783004/diff/750001/cc/scheduler/frame_sour...
cc/scheduler/frame_source.h:57: virtual const BeginFrameArgs
LastBeginFrameArgs() const = 0;
On 2014/09/24 11:12:53, Sami wrote:
> On 2014/09/24 06:03:05, brianderson wrote:
> > On 2014/09/23 12:43:55, mithro wrote:
> > > On 2014/09/19 13:44:22, Sami wrote:
> > > > Is it so that the return value from LastBeginFrameArgs will only ever
> change
> > > > inside calls to OnBeginFrame?
> > > 
> > > Yes, that is correct - LastUsedBeginFrameArgs should only ever change
inside
> > the
> > > OnBeginFrame method.
> > > 
> > > > If so, how about we give OnBeginFrame a return
> > > > enum or an out parameter that conveys the same information?
> > > 
> > > I'm not sure I fully understand but I gave this idea a go, does it look
> okay?
> > > 
> > > > Otherwise I think
> > > > this should be called something like BeginFrameSourceClient to highlight
> > it's
> > > > not a simpler observer.
> > > 
> > > The patch original called it a BeginFrameSink, but I changed it to
> > > BeginFrameObserver to match the AddObserver/RemoveObserver methods.
> > > 
> > > > Also, LastUsedBeginFrameArgs() might make the relationship more obvious.
> > > 
> > > Using this name now.
> > > 
> > 
> > Sami, were you suggesting the enums should replace LastUsedBeginFrameArgs()?
> > There seems to be a lot of overlap between the two. I like the
> > LastUsedBeginFrameArgs() since the Observer/Client is the one who really
> decides
> > that and is going to store a copy of it anyway; no need to store it twice.
> 
> Yes, let's have one or the other but not both because that is redundant. If
> Brian's happy with just having LastUsedBeginFrameArgs() then I am too :)

Acknowledged.

https://codereview.chromium.org/267783004/diff/830001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/267783004/diff/830001/cc/BUILD.gn#newcode433
cc/BUILD.gn:433: "scheduler/delay_based_time_source.cc",
On 2014/09/24 11:12:53, Sami wrote:
> Is vsync_parameter_observer.h missing from this file?

Done.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:63: return OnBeginFrame(args);
On 2014/09/24 06:03:05, brianderson wrote:
> Looks like this is redirecting to the wrong method.  Should have something
like
> OnMissedBeginFrameMixInDelegate.

Done.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:518: // If the last begin frame is invalid,
than any new begin frame is valid.
On 2014/09/24 06:03:05, brianderson wrote:
> than -> then

Done.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:524: return (  // args.frame_time >
observer_->LastUsedBeginFrameArgs().frame_time
On 2014/09/24 06:03:05, brianderson wrote:
> Remove commented out code.

Done.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:65: virtual const BeginFrameArgs
LastUsedBeginFrameArgs() const = 0;
On 2014/09/24 06:03:05, brianderson wrote:
> const BeginFrameArgs&

Can't be a reference until C++11 as we need to be able to do "return
BeginFrameArgs();"

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:13: #include
"testing/gmock/include/gmock/gmock.h"
On 2014/09/24 06:03:06, brianderson wrote:
> I don't understand gmock well enough to review this file quickly. I'll try to
> look at it more tomorrow.

Acknowledged.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:16: #define EXPECT_USED
cc::BeginFrameObserver::BeginFrameArgsStatus::USED
On 2014/09/24 11:12:53, Sami wrote:
> I think "using" declarations would be cleaner than these. You could add a few
> for gmock too to get rid of the ::testing:: noise.

So it appears that the using statements and macros don't mix very well. 

1.      ../../cc/scheduler/begin_frame_source_unittest.cc:94:3
<Spelling=../../cc/scheduler/begin_frame_source_unittest.cc:44:29>: current
parser token 'USED'
2.      ../../cc/scheduler/begin_frame_source_unittest.cc:57:1: parsing
namespace 'cc'
3.      ../../cc/scheduler/begin_frame_source_unittest.cc:58:1: parsing
namespace
4.      ../../cc/scheduler/begin_frame_source_unittest.cc:92:54: parsing
function body 'TestBody'
5.      ../../cc/scheduler/begin_frame_source_unittest.cc:92:54: in compound
statement ('{}')
6.      ../../cc/scheduler/begin_frame_source_unittest.cc:94:3
<Spelling=../../cc/scheduler/begin_frame_source_unittest.cc:17:3>: in compound
statement ('{}')
clang: error: unable to execute command: Aborted (core dumped)
clang: error: clang frontend command failed due to signal (use -v to see
invocation)
clang version 3.6.0 (216630)

But it's gone now.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:22: switch (status) {               
                                      \
On 2014/09/24 11:12:53, Sami wrote:
> Maybe it's just me but this feels a little too clever for its own good. I'm
> picturing the errors from gmock when an expectation fails and wondering if
> anyone can make sense out of them.

The GMOCK error output is actually very readable. It says which expectation
failed and prints out all the arguments and stuff.

> Could you at least split this into EXPECT_BEGIN_FRAME_USED and _DROPPED
because
> the two branches aren't sharing any code.

Done.

https://codereview.chromium.org/267783004/diff/830001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:58: #define P(pointer) (*pointer)
On 2014/09/24 11:12:53, Sami wrote:
> I think just using (*pointer) would be more readable. If you worry about the
> parens then let's handle that inside the macro instead.

Done.

https://codereview.chromium.org/267783004/diff/830001/cc/test/scheduler_test_...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/267783004/diff/830001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:30: bool tick_called_ = false;
On 2014/09/24 06:03:06, brianderson wrote:
> Initialize in constructor.

Removed.

[brianderson, 2014-09-24 22:45:38]

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:60: bool used = ProcessBeginFrameArgs(args);
used = ProcessMissedBeginFrameArgs

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:92: virtual bool
ProcessMissedBeginFrameArgs(const BeginFrameArgs& args);
Was imagining this would be pure virtual.

Also, I feel like Process***Args doesn't imply as strong a link between
BeginFrame/OnBeginFrame as there is. Maybe OnBeginFrameMixInDelegate or
OnBeginFrameDelegate would imply a stronger link without conflicting?

[danakj, 2014-09-24 23:35:24]

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:92: virtual bool
ProcessMissedBeginFrameArgs(const BeginFrameArgs& args);
On 2014/09/24 22:45:38, brianderson wrote:
> Was imagining this would be pure virtual.

+1. Please avoid situations where we have non-pure virtual methods that are
overridden by another class.

[danakj, 2014-09-24 23:59:25]

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:92: virtual bool
ProcessMissedBeginFrameArgs(const BeginFrameArgs& args);
On 2014/09/24 23:35:24, danakj wrote:
> On 2014/09/24 22:45:38, brianderson wrote:
> > Was imagining this would be pure virtual.
> 
> +1. Please avoid situations where we have non-pure virtual methods that are
> overridden by another class.

(Or classes that mix pure virtual and concrete virtual methods, if possible.
Pure virtual interfaces are great, but things get more complex when they
implement methods/have members also.)

[danakj, 2014-09-24 23:59:42]

danakj@chromium.org changed reviewers:
- danakj@chromium.org

[mithro-old, 2014-09-25 01:53:00]

Hi Brian,

Hopefully today's chat was useful in wrapping up any the questions about the
unit tests.

Anything else that you think is currently missing or needs to be finished before
we can land?

Tim 'mithro' Ansell

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:60: bool used = ProcessBeginFrameArgs(args);
On 2014/09/24 22:45:38, brianderson wrote:
> used = ProcessMissedBeginFrameArgs

Done.

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:92: virtual bool
ProcessMissedBeginFrameArgs(const BeginFrameArgs& args);
On 2014/09/24 22:45:38, brianderson wrote:
> Was imagining this would be pure virtual.

I'd really like implementation of this method to be totally optional to help
reinforce that this functionality may not be enabled for a given frame source.

> Also, I feel like Process***Args doesn't imply as strong a link between
> BeginFrame/OnBeginFrame as there is. Maybe OnBeginFrameMixInDelegate or
> OnBeginFrameDelegate would imply a stronger link without conflicting?

Naming things is hard, these names are long but better. Changed.

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:92: virtual bool
ProcessMissedBeginFrameArgs(const BeginFrameArgs& args);
On 2014/09/24 23:35:24, danakj wrote:
> On 2014/09/24 22:45:38, brianderson wrote:
> > Was imagining this would be pure virtual.
> 
> +1. Please avoid situations where we have non-pure virtual methods that are
> overridden by another class.

How do you provide "default" implementations without doing this? For example, we
want missed begin frame support to be totally optional.

I also split the dropped count for each type of message.

[chromium-reviews, 2014-09-25 02:25:11]

On Sep 24, 2014 9:53 PM, <mithro@mithis.com> wrote:
>
> Hi Brian,
>
> Hopefully today's chat was useful in wrapping up any the questions about
the
> unit tests.
>
> Anything else that you think is currently missing or needs to be finished
before
> we can land?
>
> Tim 'mithro' Ansell
>
>
>
>
https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
> File cc/scheduler/begin_frame_source.cc (right):
>
>
https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_source.cc:60: bool used =
> ProcessBeginFrameArgs(args);
> On 2014/09/24 22:45:38, brianderson wrote:
>>
>> used = ProcessMissedBeginFrameArgs
>
>
> Done.
>
>
>
https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
> File cc/scheduler/begin_frame_source.h (right):
>
>
https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_source.h:92: virtual bool
> ProcessMissedBeginFrameArgs(const BeginFrameArgs& args);
> On 2014/09/24 22:45:38, brianderson wrote:
>>
>> Was imagining this would be pure virtual.
>
>
> I'd really like implementation of this method to be totally optional to
> help reinforce that this functionality may not be enabled for a given
> frame source.
>
>
>> Also, I feel like Process***Args doesn't imply as strong a link
>
> between
>>
>> BeginFrame/OnBeginFrame as there is. Maybe OnBeginFrameMixInDelegate
>
> or
>>
>> OnBeginFrameDelegate would imply a stronger link without conflicting?
>
>
> Naming things is hard, these names are long but better. Changed.
>
>
>
https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_source.h:92: virtual bool
> ProcessMissedBeginFrameArgs(const BeginFrameArgs& args);
> On 2014/09/24 23:35:24, danakj wrote:
>>
>> On 2014/09/24 22:45:38, brianderson wrote:
>> > Was imagining this would be pure virtual.
>
>
>> +1. Please avoid situations where we have non-pure virtual methods
>
> that are
>>
>> overridden by another class.
>
>
> How do you provide "default" implementations without doing this? For
> example, we want missed begin frame support to be totally optional.

Use composition instead of inheritance. One way is put the default
implementation somewhere else and let impls of the interface own and use
that if they want. Would that work?

> I also split the dropped count for each type of message.
>
> https://codereview.chromium.org/267783004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2014-09-25 06:19:47]

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:43: // DCHECK(args.frame_time >
last_begin_frame_args_.frame_time);
Remove commented code.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:60: // DCHECK(args.frame_time >
last_begin_frame_args_.frame_time);
ditto.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:205: // BackToBackBeginFrameSource
--------------------------------------------
Can be removed.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:429: args.AsValue());
return?

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:70: // Advance observers may also wish to
override the OnMissedBeginFrame method.
Advance -> Advanced?

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:73: // BeginFrameObserver
wrong comment.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:128: virtual void DidFinishFrame(size_t
remaining_frames) = 0;
Does we really need this function?
I'm not sure exposing scheduler's information(the number of remained frame) to
Source is good design.
I think we can provide back pressure with SetNeedsBeginFrames()?

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:130: // Add/Remove and observer from the
source.
and -> an.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:364: TRACE_EVENT1("cc", "Scheduler::BeginFrame",
"args", args.AsValue());
"Scheduler::OnBeginFrameMixInDelegate"?

[mithro-old, 2014-09-25 13:32:20]

Hi everyone,

Can people take another look?

After Simon discovered the bug in the
BeginFrameSourceMultiplexer::OnMissedBeginFrame message I realised we had no
coverage of that functionality. This also lead me to discover that the
SyntheticBeginFrameSource wasn't even using it! (The SyntheticBeginFrameSource
was the whole reason we added the OnMissedBeginFrame message.)

I'm a little worried that there is too much "magic" around the
OnBeginFrame/OnMissedBeginFrame unit tests now. However, it's significantly
reduced from my first working attempt which used function pointers and duplicate
all the test code seems a sure way to let bugs slip in.

In the future, I think we should refactor the scheduler to not need the missed
begin frames to be special cased and remove that whole duplicate path. Really,
we could think of this frame as just one that has been very delayed by the
message queue.

Tim

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:43: // DCHECK(args.frame_time >
last_begin_frame_args_.frame_time);
On 2014/09/25 06:19:46, simonhong wrote:
> Remove commented code.

Done.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:60: // DCHECK(args.frame_time >
last_begin_frame_args_.frame_time);
On 2014/09/25 06:19:46, simonhong wrote:
> ditto.

Done.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:205: // BackToBackBeginFrameSource
--------------------------------------------
On 2014/09/25 06:19:46, simonhong wrote:
> Can be removed.

Fixed.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:429: args.AsValue());
On 2014/09/25 06:19:46, simonhong wrote:
> return?

Fixed with test added to catch this issue.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:70: // Advance observers may also wish to
override the OnMissedBeginFrame method.
On 2014/09/25 06:19:46, simonhong wrote:
> Advance -> Advanced?

Done.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:73: // BeginFrameObserver
On 2014/09/25 06:19:46, simonhong wrote:
> wrong comment.

Done.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:128: virtual void DidFinishFrame(size_t
remaining_frames) = 0;
On 2014/09/25 06:19:46, simonhong wrote:
> Does we really need this function?
> I'm not sure exposing scheduler's information(the number of remained frame) to
> Source is good design.
> I think we can provide back pressure with SetNeedsBeginFrames()?

We went back and forth on this problem and eventually settled on
SetNeedsBeginFrames being level triggered (IE it's on/off). This means that we
needed another back-pressure method.

This is required for the BackToBackFrameSource and also enabled a "dynamic
BeginFrame throttler" which takes into account how many frames are pending
before sending more.

https://codereview.chromium.org/267783004/diff/930001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:130: // Add/Remove and observer from the
source.
On 2014/09/25 06:19:46, simonhong wrote:
> and -> an.

Done.

[mithro-old, 2014-09-26 04:10:12]

Hello everyone,

Last night I was dreaming about BeginFrame messaging (it's been a long week....)
and came up with the idea that instead of using a separate OnMissedBeginFrame
message we could instead just tag the BeginFrameArgs. This significantly reduces
the duplication in the BeginFrame class types as OnMissedBeginFrame goes away,
but increases the size of the BeginFrameArgs type. It also removes all the magic
I was concerned about in unit tests. I do have an evil plan to remove interval
from BeginFrameArgs in the future which would get us back to the current size.

Please compare the two patches below and tell me what you think;
 * Patchset 47 - https://codereview.chromium.org/267783004/#ps970001
 * Patchset 48 - https://codereview.chromium.org/267783004/#ps990001
 
Differences can be seen at
https://codereview.chromium.org/267783004/diff2/970001:990001/cc/output/begin...

Tim

[danakj, 2014-09-27 00:05:30]

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

[danakj, 2014-09-27 00:05:34]

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/267783004/diff/890001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:92: virtual bool
ProcessMissedBeginFrameArgs(const BeginFrameArgs& args);
On 2014/09/25 01:53:00, mithro wrote:
> On 2014/09/24 23:35:24, danakj wrote:
> > On 2014/09/24 22:45:38, brianderson wrote:
> > > Was imagining this would be pure virtual.
> > 
> > +1. Please avoid situations where we have non-pure virtual methods that are
> > overridden by another class.
> 
> How do you provide "default" implementations without doing this? For example,
we
> want missed begin frame support to be totally optional.
> 
> I also split the dropped count for each type of message.

Brian and I talked about this some more the other day and I think we got to a
pretty reasonable conclusion. Maybe an is-a relationship really makes sense here
so you want to use inheritance instead of composition. In that case please keep
the interface (class with pure virtual methods) as entirely an interface, don't
add anything concrete to it, virtual or not. You can have a concrete subclass
add some default implementations for other things to inherit and use if needed,
but I think it will help a lot to keep our interfaces pure, with no concrete
methods in them.

[brianderson, 2014-09-27 00:41:40]

I like the new flag to BeginFrameArgs. It's a cleaner solution and allows us to
more easily extend the concept to other BeginFrame sources.

Thanks for walking me through the testing code too.

lgtm, but please make BeginFrameSource abstract and remove the default arguments
before landing.

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

https://codereview.chromium.org/267783004/diff/990001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:38: BeginFrameArgsType type = NORMAL);
Default values aren't allowed, but overloading is. Please make this a separate
function.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

https://codereview.chromium.org/267783004/diff/990001/cc/test/begin_frame_arg...
File cc/test/begin_frame_args_test.h (right):

https://codereview.chromium.org/267783004/diff/990001/cc/test/begin_frame_arg...
cc/test/begin_frame_args_test.h:24: BeginFrameArgs::BeginFrameArgsType type =
BeginFrameArgs::NORMAL);
Default arguments not allowed.

[danakj, 2014-09-27 01:09:45]

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

https://codereview.chromium.org/267783004/diff/990001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:38: BeginFrameArgsType type = NORMAL);
On 2014/09/27 00:41:40, brianderson wrote:
> Default values aren't allowed, but overloading is. Please make this a separate
> function.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over...

But if you're going to overload consider making the name slightly different is
the advice of the guide, which I think really helps.

[mithro-old, 2014-09-30 08:30:02]

Windows wasn't happy with the way I was using vardic macros, so I ended up
having to remove them to make the windows builder happy.

On 2014/09/27 00:41:40, brianderson wrote:
> lgtm, but please make BeginFrameSource abstract

Done. 

Made BeginFrameSource be pure abstract and the current implementation become
BeginFrameSourceMixIn.

> and remove the default arguments

Done.

> before landing.

I'll try landing shortly.

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

https://codereview.chromium.org/267783004/diff/990001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:38: BeginFrameArgsType type = NORMAL);
On 2014/09/27 01:09:44, danakj wrote:
> On 2014/09/27 00:41:40, brianderson wrote:
> > Default values aren't allowed, but overloading is. Please make this a
separate
> > function.
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over...
> 
> But if you're going to overload consider making the name slightly different is
> the advice of the guide, which I think really helps.

Done.

https://codereview.chromium.org/267783004/diff/990001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:38: BeginFrameArgsType type = NORMAL);
On 2014/09/27 00:41:40, brianderson wrote:
> Default values aren't allowed, but overloading is. Please make this a separate
> function.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

Someone should fix CreateForSynchronousCompositor below.

https://codereview.chromium.org/267783004/diff/990001/cc/test/begin_frame_arg...
File cc/test/begin_frame_args_test.h (right):

https://codereview.chromium.org/267783004/diff/990001/cc/test/begin_frame_arg...
cc/test/begin_frame_args_test.h:24: BeginFrameArgs::BeginFrameArgsType type =
BeginFrameArgs::NORMAL);
On 2014/09/27 00:41:40, brianderson wrote:
> Default arguments not allowed.

Done.

[mithro-old, 2014-09-30 08:31:02]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-09-30 08:31:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/267783004/1130001

[simonhong, 2014-09-30 08:38:34]

On 2014/09/30 08:31:30, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/267783004/1130001

Wow! thanks for landing!

[commit-bot: I haz the power, 2014-09-30 09:10:49]

Committed patchset #55 (id:1130001) as 1900bc86a7a8dd912374e9aecc72c20d10598733

[commit-bot: I haz the power, 2014-09-30 09:11:30]

Patchset 55 (id:??) landed as
https://crrev.com/c34fc0b1428211293c19944db1bac41a7a7d0401
Cr-Commit-Position: refs/heads/master@{#297389}

[Sami, 2014-09-30 14:58:40]

lgtm, yay!

https://codereview.chromium.org/267783004/diff/1130001/cc/output/begin_frame_...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/267783004/diff/1130001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:59: bool IsValid() const { return interval >=
base::TimeDelta(); }
This should probably be updated to look at the type instead?

https://codereview.chromium.org/267783004/diff/1130001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/267783004/diff/1130001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:299: //
EXPECT_FALSE(client->needs_begin_frames());
Dead code?

[brianderson, 2014-09-30 23:53:34]

Before I leave for the day, I would also like to say "whoo hoo!" =)