Unified BeginFrame scheduling

Unified BeginFrame scheduling

This CL is initial implementation for "Unified BeginFrame scheduling on chrome".
Scheduler is divided into publisher(live in browser compositor)
and receiver(live in renderer compositor).
Newly created BeginFrameManager(which owned by browser compositor) will act like router
between Scheduler and RWHV for delivering BeginFrame messages.
Browser compositor's scheduler(publisher) triggers next BeginFrame for itself(ui)
and renderers(receiver).

Please refer to below docs for overall design.
(https://docs.google.com/a/chromium.org/document/d/13xtO-_NSSnNZRRS1Xq3xGNKZawKc8HQxOid5boBUyX8/edit#)

R=brianderson@chromium.org, sky@chromium.org
BUG=372086
TEST=NONE(Not yet used)

[simonhong, 2014-07-28 02:24:32]

Dear brian and sky,
Please take a look!

@sky, would you please review overall design as
UI owner?

[sky, 2014-07-28 15:30:25]

+piman, he's a better reviewer for this.

[brianderson, 2014-07-28 21:13:48]

Will this end up sending BeginFrames to all Renderers/Observers even if only one
Renderer needs BeginFrames?

https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
File ui/base/compositor/begin_frame_manager.h (right):

https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
ui/base/compositor/begin_frame_manager.h:38: void SendBeginFrame(base::TimeTicks
timebase, base::TimeDelta interval);
Would BeginFrameArgs work better than timebase+interval?
If not, where will the deadline be set?

[simonhong, 2014-07-28 23:37:41]

BeginFrameManager will send to its all observer.
Then observer(mostly RWHV) will determine whether to send or not to
corresponding renderer process because RWHV have the
information(ViewHostMsg_SetNeedsBeginFrame) received from renderer.
Also RWHV will remove itself from observer list when it is not shown.

https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
File ui/base/compositor/begin_frame_manager.h (right):

https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
ui/base/compositor/begin_frame_manager.h:38: void SendBeginFrame(base::TimeTicks
timebase, base::TimeDelta interval);
On 2014/07/28 21:13:47, brianderson wrote:
> Would BeginFrameArgs work better than timebase+interval?
> If not, where will the deadline be set?

IMO, RWHV is more better place to calculate deadline because we will need
estimated draw-time.

[brianderson, 2014-07-29 00:21:32]

On 2014/07/28 23:37:41, simonhong wrote:
> BeginFrameManager will send to its all observer.
> Then observer(mostly RWHV) will determine whether to send or not to
> corresponding renderer process because RWHV have the
> information(ViewHostMsg_SetNeedsBeginFrame) received from renderer.
> Also RWHV will remove itself from observer list when it is not shown.

Ok, that should work.

> 
>
https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
> File ui/base/compositor/begin_frame_manager.h (right):
> 
>
https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
> ui/base/compositor/begin_frame_manager.h:38: void
SendBeginFrame(base::TimeTicks
> timebase, base::TimeDelta interval);
> On 2014/07/28 21:13:47, brianderson wrote:
> > Would BeginFrameArgs work better than timebase+interval?
> > If not, where will the deadline be set?
> 
> IMO, RWHV is more better place to calculate deadline because we will need
> estimated draw-time.

Each Renderer will track its own draw time in the Renderer process, so we don't
need to track it in the Browser too.

Every RWHV should be given the same Browser deadline, which is what we are
trying to communicate here. The Browser's draw time will be known by the
Browser's Scheduler, not the RWHV.

Would the following work well:
1) Have the Browser's Scheduler trigger BeginFrameManager::SendBeginFrame for
all platforms.
2) Have the platform-specific stuff hooked up to Scheduler::BeginFrame in the
Browser.

[simonhong, 2014-07-29 01:40:33]

On 2014/07/29 00:21:32, brianderson wrote:
> On 2014/07/28 23:37:41, simonhong wrote:
> > BeginFrameManager will send to its all observer.
> > Then observer(mostly RWHV) will determine whether to send or not to
> > corresponding renderer process because RWHV have the
> > information(ViewHostMsg_SetNeedsBeginFrame) received from renderer.
> > Also RWHV will remove itself from observer list when it is not shown.
> 
> Ok, that should work.
> 
> > 
> >
>
https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
> > File ui/base/compositor/begin_frame_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
> > ui/base/compositor/begin_frame_manager.h:38: void
> SendBeginFrame(base::TimeTicks
> > timebase, base::TimeDelta interval);
> > On 2014/07/28 21:13:47, brianderson wrote:
> > > Would BeginFrameArgs work better than timebase+interval?
> > > If not, where will the deadline be set?
> > 
> > IMO, RWHV is more better place to calculate deadline because we will need
> > estimated draw-time.
> 
> Each Renderer will track its own draw time in the Renderer process, so we
don't
> need to track it in the Browser too.
> 
> Every RWHV should be given the same Browser deadline, which is what we are
> trying to communicate here. The Browser's draw time will be known by the
> Browser's Scheduler, not the RWHV.

Yep. I got it.

> 
> Would the following work well:
> 1) Have the Browser's Scheduler trigger BeginFrameManager::SendBeginFrame for
> all platforms.

On Aura, Browser's Scheduler will trigger BeginFrame.
On Mac, I thought to use DisplayLink as vsync source and requests directly
BeginFrame when vsync callback is called.
Do you mean that using DisplayLink in Scheduler as a BeginFrame source instead
of SyntheticBeginFrameSource?
If so, I think this would be good.

> 2) Have the platform-specific stuff hooked up to Scheduler::BeginFrame in the
> Browser.

[brianderson, 2014-07-30 19:41:48]

On 2014/07/29 01:40:33, simonhong wrote:
> On 2014/07/29 00:21:32, brianderson wrote:
> > On 2014/07/28 23:37:41, simonhong wrote:
> > > BeginFrameManager will send to its all observer.
> > > Then observer(mostly RWHV) will determine whether to send or not to
> > > corresponding renderer process because RWHV have the
> > > information(ViewHostMsg_SetNeedsBeginFrame) received from renderer.
> > > Also RWHV will remove itself from observer list when it is not shown.
> > 
> > Ok, that should work.
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
> > > File ui/base/compositor/begin_frame_manager.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
> > > ui/base/compositor/begin_frame_manager.h:38: void
> > SendBeginFrame(base::TimeTicks
> > > timebase, base::TimeDelta interval);
> > > On 2014/07/28 21:13:47, brianderson wrote:
> > > > Would BeginFrameArgs work better than timebase+interval?
> > > > If not, where will the deadline be set?
> > > 
> > > IMO, RWHV is more better place to calculate deadline because we will need
> > > estimated draw-time.
> > 
> > Each Renderer will track its own draw time in the Renderer process, so we
> don't
> > need to track it in the Browser too.
> > 
> > Every RWHV should be given the same Browser deadline, which is what we are
> > trying to communicate here. The Browser's draw time will be known by the
> > Browser's Scheduler, not the RWHV.
> 
> Yep. I got it.
> 
> > 
> > Would the following work well:
> > 1) Have the Browser's Scheduler trigger BeginFrameManager::SendBeginFrame
for
> > all platforms.
> 
> On Aura, Browser's Scheduler will trigger BeginFrame.
> On Mac, I thought to use DisplayLink as vsync source and requests directly
> BeginFrame when vsync callback is called.
> Do you mean that using DisplayLink in Scheduler as a BeginFrame source instead
> of SyntheticBeginFrameSource?
> If so, I think this would be good.

Yep. Hook DisplayLink up to the Scheduler as the BeginFrame source. Similarly,
hook WindowAndroid::OnVSync up to the Scheduler too. Then all platforms can have
the Browser's Scheduler trigger BeginFrameManager::SendBeginFrame.

> 
> > 2) Have the platform-specific stuff hooked up to Scheduler::BeginFrame in
the
> > Browser.

[brianderson, 2014-07-30 19:44:29]

+jdduke for fyi.

https://codereview.chromium.org/423773002/diff/20001/ui/base/compositor/begin...
File ui/base/compositor/begin_frame_manager.h (right):

https://codereview.chromium.org/423773002/diff/20001/ui/base/compositor/begin...
ui/base/compositor/begin_frame_manager.h:39: void SendBeginFrame(base::TimeTicks
frame_time,
Please use BeginFrameArgs instead.

[simonhong, 2014-07-31 00:17:51]

On 2014/07/30 19:41:48, brianderson wrote:
> On 2014/07/29 01:40:33, simonhong wrote:
> > On 2014/07/29 00:21:32, brianderson wrote:
> > > On 2014/07/28 23:37:41, simonhong wrote:
> > > > BeginFrameManager will send to its all observer.
> > > > Then observer(mostly RWHV) will determine whether to send or not to
> > > > corresponding renderer process because RWHV have the
> > > > information(ViewHostMsg_SetNeedsBeginFrame) received from renderer.
> > > > Also RWHV will remove itself from observer list when it is not shown.
> > > 
> > > Ok, that should work.
> > > 
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
> > > > File ui/base/compositor/begin_frame_manager.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/423773002/diff/1/ui/base/compositor/begin_fra...
> > > > ui/base/compositor/begin_frame_manager.h:38: void
> > > SendBeginFrame(base::TimeTicks
> > > > timebase, base::TimeDelta interval);
> > > > On 2014/07/28 21:13:47, brianderson wrote:
> > > > > Would BeginFrameArgs work better than timebase+interval?
> > > > > If not, where will the deadline be set?
> > > > 
> > > > IMO, RWHV is more better place to calculate deadline because we will
need
> > > > estimated draw-time.
> > > 
> > > Each Renderer will track its own draw time in the Renderer process, so we
> > don't
> > > need to track it in the Browser too.
> > > 
> > > Every RWHV should be given the same Browser deadline, which is what we are
> > > trying to communicate here. The Browser's draw time will be known by the
> > > Browser's Scheduler, not the RWHV.
> > 
> > Yep. I got it.
> > 
> > > 
> > > Would the following work well:
> > > 1) Have the Browser's Scheduler trigger BeginFrameManager::SendBeginFrame
> for
> > > all platforms.
> > 
> > On Aura, Browser's Scheduler will trigger BeginFrame.
> > On Mac, I thought to use DisplayLink as vsync source and requests directly
> > BeginFrame when vsync callback is called.
> > Do you mean that using DisplayLink in Scheduler as a BeginFrame source
instead
> > of SyntheticBeginFrameSource?
> > If so, I think this would be good.
> 
> Yep. Hook DisplayLink up to the Scheduler as the BeginFrame source. Similarly,
> hook WindowAndroid::OnVSync up to the Scheduler too. Then all platforms can
have
> the Browser's Scheduler trigger BeginFrameManager::SendBeginFrame.

Yep! Using scheduler as the sole source of BeginFrame looks much better design.
Thanks for advice!

> 
> > 
> > > 2) Have the platform-specific stuff hooked up to Scheduler::BeginFrame in
> the
> > > Browser.

[simonhong, 2014-07-31 00:18:03]

https://codereview.chromium.org/423773002/diff/20001/ui/base/compositor/begin...
File ui/base/compositor/begin_frame_manager.h (right):

https://codereview.chromium.org/423773002/diff/20001/ui/base/compositor/begin...
ui/base/compositor/begin_frame_manager.h:39: void SendBeginFrame(base::TimeTicks
frame_time,
On 2014/07/30 19:44:29, brianderson wrote:
> Please use BeginFrameArgs instead.

Done.

[simonhong, 2014-07-31 09:43:05]

The CQ bit was checked by simonhong@chromium.org

[commit-bot: I haz the power, 2014-07-31 09:45:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/simonhong@chromium.org/423773002/40001

[commit-bot: I haz the power, 2014-07-31 09:45:21]

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

[commit-bot: I haz the power, 2014-07-31 09:45:23]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[jdduke (slow), 2014-07-31 15:11:52]

On 2014/07/30 19:44:29, brianderson wrote:
> +jdduke for fyi.

Is there a WIP parent or tracking patch that has this change in an end-to-end
working state on one of the platforms?

[simonhong, 2014-07-31 15:20:53]

On 2014/07/31 15:11:52, jdduke wrote:
> On 2014/07/30 19:44:29, brianderson wrote:
> > +jdduke for fyi.
> 
> Is there a WIP parent or tracking patch that has this change in an end-to-end
> working state on one of the platforms?

@jdduke, this is first step patch for BeginFrame scheduling.
After landing this, I'll make next patches in the scheduler side, RWHV and etc..

[sky, 2014-07-31 16:04:27]

Do not land this until one of piman and/or jamesr has responded. They have a
higher level picture of the compositor and would know better if this fits in.

[brianderson, 2014-07-31 19:27:14]

On 2014/07/31 15:11:52, jdduke wrote:
> On 2014/07/30 19:44:29, brianderson wrote:
> > +jdduke for fyi.
> 
> Is there a WIP parent or tracking patch that has this change in an end-to-end
> working state on one of the platforms?

+1.

Before landing this patch, I would like to see:
1) a patch that hooks up the BeginFrameManager up to all RWHVs so that
ViewHostMsg_SetNeedsBeginFrame and ViewMsg_BeginFrame are exposed like
RenderWidgetHostViewAndroid already exposes them.
2) a patch that hooks the Scheduler up to BeginFrameManager as the
BeginFrameManager::Delegate
3) a patch that routes WindowAndroid::OnVSync and Mac's DisplayLink to the
Scheduler.

For 2 and 3, you'll want to base your changes on top of enne's following patches
so you only need to worry about implementing it for the SingleThreadProxy.
https://codereview.chromium.org/134623005
https://codereview.chromium.org/344743002

Once those 3 are up, even if they are incomplete, I think we'll have a better
idea if this patch fits in well or needs to change at all.

[piman, 2014-07-31 19:42:41]

On 2014/07/31 16:04:27, sky wrote:
> Do not land this until one of piman and/or jamesr has responded. They have a
> higher level picture of the compositor and would know better if this fits in.

I will review once Brian is happy with the direction. As he indicated, I think
we should look at the big picture, because this patch in isolation doesn't
really do anything.

[simonhong, 2014-08-11 07:20:28]

@brianderson, I updated initial version only for aura platform.
Please review overall big picture in your free time.
(I need one more issue not yet implemented.
Current cl only handles main frame's RWHV.
I'm finding a nice way for handling all RWHV for child frame, guest frame..)

[jdduke (slow), 2014-08-15 22:58:13]

On 2014/08/11 07:20:28, simonhong wrote:
> @brianderson, I updated initial version only for aura platform.
> Please review overall big picture in your free time.
> (I need one more issue not yet implemented.
> Current cl only handles main frame's RWHV.
> I'm finding a nice way for handling all RWHV for child frame, guest frame..)

I think Brian was away at a conference, but he should be back next week.

[simonhong, 2014-08-20 03:03:50]

On 2014/08/15 22:58:13, jdduke wrote:
> On 2014/08/11 07:20:28, simonhong wrote:
> > @brianderson, I updated initial version only for aura platform.
> > Please review overall big picture in your free time.
> > (I need one more issue not yet implemented.
> > Current cl only handles main frame's RWHV.
> > I'm finding a nice way for handling all RWHV for child frame, guest frame..)
> 
> I think Brian was away at a conference, but he should be back next week.

Just a remind to brian.
(If you busy you can review this later :))
I updated with working implementation on aura with
--enable-begin-frame-scheduling.

[mithro-old, 2014-08-20 03:43:02]

Just a FYI for everyone, I think this change has a lot of synergy with my change
at https://codereview.chromium.org/267783004/ which tries and refactor the way
BeginFrame messages work in the Scheduler (currently blocked on making the
scheduler tests determinstic to prove it is truely a no-op change). 

The eventual idea with my refactor is that most of the code would end up being
hoisted into the browser level, kind of similar to what this change is
attempting.

I'm chatting with Simon to figure out the best way to make sure our two changes
don't conflict.

Tim

[brianderson, 2014-08-21 00:14:00]

https://codereview.chromium.org/423773002/diff/160001/ash/display/display_con...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/423773002/diff/160001/ash/display/display_con...
ash/display/display_controller.cc:130: * Should update this to somewhere.
Let's route it to the cc::Scheduler. Only the Browser will have a
SyntheticBeginFrameSource, which is the only class that will really need to know
about the authoritative vsync interval.

Scheduler::CommitVSyncParameters can then override the interval the same way
CompositorVSyncManager::UpdateVSyncParameters used to.

https://codereview.chromium.org/423773002/diff/160001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/423773002/diff/160001/cc/base/switches.cc#new...
cc/base/switches.cc:74: // Use a eginFrame signal from browser to renderer to
scheduling rendering.
egin -> Begin

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:306: !begin_frame_requested_);
Instead of taking begin_frame_requested_ into account here, would it make sense
to take it into account from inside SchedulerStateMachine::BeginFrameNeeded?

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:419: begin_frame_requested_ = false;
The child Schedulers should be setting begin_frame_requested explicitly to false
and the parent Scheduler should not set it to false implicitly like this. That
way we reduce the number of messages between the Renderer and Browser.

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:636:
synthetic_begin_frame_source_->SetNeedsBeginFrame(true,
Instead of calling SetNeedsBeginFrame direcly here, please call
ProcessScheduledActions instead, which will go through the common way of setting
up the value for SetNeedsBeginFrame.

That will properly handle the vsync disabled case.

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:214: bool begin_frame_requested_;
Store this in the state machine as children_need_begin_frames_ instead.

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.h:27: bool begin_frame_scheduling_enabled;
Maybe we should rename this to begin_frame_receiver to more clearly
differentiate it's meaning from begin_frame_publisher.

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.h:35: bool begin_frame_publisher;
Group this next to begin_frame_scheduling_enabled.

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:302: void SendBeginFrame(const BeginFrameArgs& args);
SendBeginFrameToChildren?

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:463: bool begin_frame_requested_;
I don't think LTH needs to store this. If early outs are needed, it should be
done at a higher level.

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_client.h:49: virtual void SendBeginFrame(const
BeginFrameArgs& args) {}
Can you make the default implementation call NOT_REACHED() until it becomes pure
virtual?

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.h:84: bool begin_frame_publisher;
Group with begin_frame_scheduling_enabled.

https://codereview.chromium.org/423773002/diff/160001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/proxy.h#newcod...
cc/trees/proxy.h:103: virtual void BeginFrameRequested() = 0;
SetChildrenNeedBeginFrames(bool enable)?

The 's' in "Frames" indicate that it's level triggered. "Children"
differentiates the request from main thread requests.

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

https://codereview.chromium.org/423773002/diff/160001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:556: if (scheduler_on_impl_thread_)
In what case would a Renderer request a BeginFrame when the Browser doesn't have
a scheduler?

https://codereview.chromium.org/423773002/diff/160001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1194: Proxy::MainThreadTaskRunner()->PostTask(
It would be nice if we could send directly Renderers from the Impl thread but,
since we are moving to SingleThreadProxy Browsers everywhere, this is fine.

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

https://codereview.chromium.org/423773002/diff/160001/cc/trees/thread_proxy.h...
cc/trees/thread_proxy.h:72: bool begin_frame_requested;
ThreadProxy shouldn't have to cache this.

https://codereview.chromium.org/423773002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/423773002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2493: return;
I think it would be better for the RWHV to add or remove itself as an observer
in this method, rather than in WasShown or WasHidden.

https://codereview.chromium.org/423773002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/423773002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:92: public
ui::BeginFrameManager::Observer,
Is the plan to land this first as Aura-only and then unify it with all other
platforms as a separate patch?

https://codereview.chromium.org/423773002/diff/160001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/423773002/diff/160001/content/common/view_mes...
content/common/view_messages.h:962: #if !defined(OS_MACOSX)
Should this be:
#if defined(OS_ANDROID) || defined(USE_AURA)

https://codereview.chromium.org/423773002/diff/160001/ui/base/compositor/begi...
File ui/base/compositor/begin_frame_manager.cc (right):

https://codereview.chromium.org/423773002/diff/160001/ui/base/compositor/begi...
ui/base/compositor/begin_frame_manager.cc:25: void
BeginFrameManager::RequestBeginFrame() const {
I think this method can be removed.

Instead, delegate_->SetChildrenNeedBeginFrames(true/false) can be called from
AddObserver when the number of observers goes from 0 to 1, and from
RemoveObserver when the number of observers goes from 1 to 0.

https://codereview.chromium.org/423773002/diff/160001/ui/base/compositor/begi...
ui/base/compositor/begin_frame_manager.cc:31: void
BeginFrameManager::AddObserver(Observer* observer) {
You may want to make this:
void BeginFrameManager::AddObserver(Observer* observer, BeginFrameArgs
last_begin_frame_args_sent_to_observer)

And remember a last_begin_frame_args_sent in BeginFrameManager::SendBeginFrame.

Then, if last_begin_frame_args_sent_to_observer != last_begin_frame_args_sent
and last_begin_frame_args_sent.deadline hasn't passed, send
last_begin_frame_args_sent to the new observer immediately.

[simonhong, 2014-08-21 00:25:03]

Thanks for review, brian.
I'm trying to make BeginFrameManager as a thread safe class.
With this, scheduler can become a BeginFrameManager::delegate and it can
access BeginFrameManager directly on either thread(main or compositor
safely)
I think my current implementation that sending all messages between
BeginFrameManager and scheduler through proxy and LTH is not efficient.
Does this also make sense?
With this, authoritative interval also could be updated easily, too.


On Thu, Aug 21, 2014 at 9:13 AM, <brianderson@chromium.org> wrote:

>
> https://codereview.chromium.org/423773002/diff/160001/ash/
> display/display_controller.cc
> File ash/display/display_controller.cc (right):
>
> https://codereview.chromium.org/423773002/diff/160001/ash/
> display/display_controller.cc#newcode130
> ash/display/display_controller.cc:130: * Should update this to
> somewhere.
> Let's route it to the cc::Scheduler. Only the Browser will have a
> SyntheticBeginFrameSource, which is the only class that will really need
> to know about the authoritative vsync interval.
>
> Scheduler::CommitVSyncParameters can then override the interval the same
> way CompositorVSyncManager::UpdateVSyncParameters used to.
>
> https://codereview.chromium.org/423773002/diff/160001/cc/base/switches.cc
> File cc/base/switches.cc (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> base/switches.cc#newcode74
> cc/base/switches.cc:74: // Use a eginFrame signal from browser to
> renderer to scheduling rendering.
> egin -> Begin
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> scheduler/scheduler.cc#newcode306
> cc/scheduler/scheduler.cc:306: !begin_frame_requested_);
> Instead of taking begin_frame_requested_ into account here, would it
> make sense to take it into account from inside
> SchedulerStateMachine::BeginFrameNeeded?
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> scheduler/scheduler.cc#newcode419
> cc/scheduler/scheduler.cc:419: begin_frame_requested_ = false;
> The child Schedulers should be setting begin_frame_requested explicitly
> to false and the parent Scheduler should not set it to false implicitly
> like this. That way we reduce the number of messages between the
> Renderer and Browser.
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> scheduler/scheduler.cc#newcode636
> cc/scheduler/scheduler.cc:636:
> synthetic_begin_frame_source_->SetNeedsBeginFrame(true,
> Instead of calling SetNeedsBeginFrame direcly here, please call
> ProcessScheduledActions instead, which will go through the common way of
> setting up the value for SetNeedsBeginFrame.
>
> That will properly handle the vsync disabled case.
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> scheduler/scheduler.h
> File cc/scheduler/scheduler.h (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> scheduler/scheduler.h#newcode214
> cc/scheduler/scheduler.h:214: bool begin_frame_requested_;
> Store this in the state machine as children_need_begin_frames_ instead.
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> scheduler/scheduler_settings.h
> File cc/scheduler/scheduler_settings.h (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> scheduler/scheduler_settings.h#newcode27
> cc/scheduler/scheduler_settings.h:27: bool
> begin_frame_scheduling_enabled;
> Maybe we should rename this to begin_frame_receiver to more clearly
> differentiate it's meaning from begin_frame_publisher.
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> scheduler/scheduler_settings.h#newcode35
> cc/scheduler/scheduler_settings.h:35: bool begin_frame_publisher;
> Group this next to begin_frame_scheduling_enabled.
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/layer_tree_host.h
> File cc/trees/layer_tree_host.h (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/layer_tree_host.h#newcode302
> cc/trees/layer_tree_host.h:302: void SendBeginFrame(const
> BeginFrameArgs& args);
> SendBeginFrameToChildren?
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/layer_tree_host.h#newcode463
> cc/trees/layer_tree_host.h:463: bool begin_frame_requested_;
> I don't think LTH needs to store this. If early outs are needed, it
> should be done at a higher level.
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/layer_tree_host_client.h
> File cc/trees/layer_tree_host_client.h (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/layer_tree_host_client.h#newcode49
> cc/trees/layer_tree_host_client.h:49: virtual void SendBeginFrame(const
> BeginFrameArgs& args) {}
> Can you make the default implementation call NOT_REACHED() until it
> becomes pure virtual?
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/layer_tree_settings.h
> File cc/trees/layer_tree_settings.h (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/layer_tree_settings.h#newcode84
> cc/trees/layer_tree_settings.h:84: bool begin_frame_publisher;
> Group with begin_frame_scheduling_enabled.
>
> https://codereview.chromium.org/423773002/diff/160001/cc/trees/proxy.h
> File cc/trees/proxy.h (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/proxy.h#newcode103
> cc/trees/proxy.h:103: virtual void BeginFrameRequested() = 0;
> SetChildrenNeedBeginFrames(bool enable)?
>
> The 's' in "Frames" indicate that it's level triggered. "Children"
> differentiates the request from main thread requests.
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/single_thread_proxy.cc
> File cc/trees/single_thread_proxy.cc (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/single_thread_proxy.cc#newcode556
> cc/trees/single_thread_proxy.cc:556: if (scheduler_on_impl_thread_)
> In what case would a Renderer request a BeginFrame when the Browser
> doesn't have a scheduler?
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/thread_proxy.cc#newcode1194
> cc/trees/thread_proxy.cc:1194: Proxy::MainThreadTaskRunner()->PostTask(
> It would be nice if we could send directly Renderers from the Impl
> thread but, since we are moving to SingleThreadProxy Browsers
> everywhere, this is fine.
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/thread_proxy.h
> File cc/trees/thread_proxy.h (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/thread_proxy.h#newcode72
> cc/trees/thread_proxy.h:72: bool begin_frame_requested;
> ThreadProxy shouldn't have to cache this.
>
> https://codereview.chromium.org/423773002/diff/160001/
> content/browser/renderer_host/render_widget_host_view_aura.cc
> File content/browser/renderer_host/render_widget_host_view_aura.cc
> (right):
>
> https://codereview.chromium.org/423773002/diff/160001/
> content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2493
> content/browser/renderer_host/render_widget_host_view_aura.cc:2493:
> return;
> I think it would be better for the RWHV to add or remove itself as an
> observer in this method, rather than in WasShown or WasHidden.
>
> https://codereview.chromium.org/423773002/diff/160001/
> content/browser/renderer_host/render_widget_host_view_aura.h
> File content/browser/renderer_host/render_widget_host_view_aura.h
> (right):
>
> https://codereview.chromium.org/423773002/diff/160001/
> content/browser/renderer_host/render_widget_host_view_aura.h#newcode92
> content/browser/renderer_host/render_widget_host_view_aura.h:92: public
> ui::BeginFrameManager::Observer,
> Is the plan to land this first as Aura-only and then unify it with all
> other platforms as a separate patch?
>
> https://codereview.chromium.org/423773002/diff/160001/
> content/common/view_messages.h
> File content/common/view_messages.h (right):
>
> https://codereview.chromium.org/423773002/diff/160001/
> content/common/view_messages.h#newcode962
> content/common/view_messages.h:962: #if !defined(OS_MACOSX)
> Should this be:
> #if defined(OS_ANDROID) || defined(USE_AURA)
>
> https://codereview.chromium.org/423773002/diff/160001/ui/
> base/compositor/begin_frame_manager.cc
> File ui/base/compositor/begin_frame_manager.cc (right):
>
> https://codereview.chromium.org/423773002/diff/160001/ui/
> base/compositor/begin_frame_manager.cc#newcode25
> ui/base/compositor/begin_frame_manager.cc:25: void
> BeginFrameManager::RequestBeginFrame() const {
> I think this method can be removed.
>
> Instead, delegate_->SetChildrenNeedBeginFrames(true/false) can be called
> from AddObserver when the number of observers goes from 0 to 1, and from
> RemoveObserver when the number of observers goes from 1 to 0.
>
> https://codereview.chromium.org/423773002/diff/160001/ui/
> base/compositor/begin_frame_manager.cc#newcode31
> ui/base/compositor/begin_frame_manager.cc:31: void
> BeginFrameManager::AddObserver(Observer* observer) {
> You may want to make this:
> void BeginFrameManager::AddObserver(Observer* observer, BeginFrameArgs
> last_begin_frame_args_sent_to_observer)
>
> And remember a last_begin_frame_args_sent in
> BeginFrameManager::SendBeginFrame.
>
> Then, if last_begin_frame_args_sent_to_observer !=
> last_begin_frame_args_sent and last_begin_frame_args_sent.deadline
> hasn't passed, send last_begin_frame_args_sent to the new observer
> immediately.
>
> https://codereview.chromium.org/423773002/
>

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

[simonhong, 2014-08-21 00:36:49]

https://codereview.chromium.org/423773002/diff/160001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1194: Proxy::MainThreadTaskRunner()->PostTask(
On 2014/08/21 00:14:00, brianderson wrote:
> It would be nice if we could send directly Renderers from the Impl thread but,
> since we are moving to SingleThreadProxy Browsers everywhere, this is fine.

Is ChromeOS also a target for SingleThreadProxy?
It is currently set to threaded mode explicitly.

[brianderson, 2014-08-21 00:37:34]

> I'm trying to make BeginFrameManager as a thread safe class. With this,
scheduler can become a BeginFrameManager::delegate and it can access
BeginFrameManager directly on either thread(main or compositor safely)
> I think my current implementation that sending all messages between
BeginFrameManager and scheduler through proxy and LTH is not efficient.
> Does this also make sense?
> With this, authoritative interval also could be updated easily, too.

Don't worry about optimizing ThreadProxy right now since BeginFrameManager won't
be used with ThreadProxy soon. We can optimize ThreadProxy in the future if we
need to. What you have now with the PostTasks is good enough.

[piman, 2014-08-21 00:38:57]

On Wed, Aug 20, 2014 at 5:36 PM, <simonhong@chromium.org> wrote:

>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
>
> https://codereview.chromium.org/423773002/diff/160001/cc/
> trees/thread_proxy.cc#newcode1194
> cc/trees/thread_proxy.cc:1194: Proxy::MainThreadTaskRunner()->PostTask(
> On 2014/08/21 00:14:00, brianderson wrote:
>
>> It would be nice if we could send directly Renderers from the Impl
>>
> thread but,
>
>> since we are moving to SingleThreadProxy Browsers everywhere, this is
>>
> fine.
>
> Is ChromeOS also a target for SingleThreadProxy?
> It is currently set to threaded mode explicitly.
>

Yes, we want to move back to SingleThreadProxy for the UI compositor on
Chrome OS (the renderer will keep a ThreadProxy).


> https://codereview.chromium.org/423773002/
>

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

[simonhong, 2014-08-21 00:58:44]

brian@, Could I assume that BeginFrameManager will only be used with
SingleThreadProxy?
I spent some time to consider how this works well with ThreadProxy, too.
If I don't need to consider ThreadProxy, implementation can be more simpler.


On Thu, Aug 21, 2014 at 9:37 AM, <brianderson@chromium.org> wrote:

> I'm trying to make BeginFrameManager as a thread safe class. With this,
>>
> scheduler can become a BeginFrameManager::delegate and it can access
> BeginFrameManager directly on either thread(main or compositor safely)
>
>> I think my current implementation that sending all messages between
>>
> BeginFrameManager and scheduler through proxy and LTH is not efficient.
>
>> Does this also make sense?
>> With this, authoritative interval also could be updated easily, too.
>>
>
> Don't worry about optimizing ThreadProxy right now since BeginFrameManager
> won't
> be used with ThreadProxy soon. We can optimize ThreadProxy in the future
> if we
> need to. What you have now with the PostTasks is good enough.
>
> https://codereview.chromium.org/423773002/
>

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

[brianderson, 2014-08-21 01:15:13]

Yes, you can assume BeginFrameManager will only be used with SingleThreadProxy.

On 2014/08/21 00:58:44, simonhong wrote:
> brian@, Could I assume that BeginFrameManager will only be used with
> SingleThreadProxy?
> I spent some time to consider how this works well with ThreadProxy, too.
> If I don't need to consider ThreadProxy, implementation can be more simpler.
> 
> 
> On Thu, Aug 21, 2014 at 9:37 AM, <mailto:brianderson@chromium.org> wrote:
> 
> > I'm trying to make BeginFrameManager as a thread safe class. With this,
> >>
> > scheduler can become a BeginFrameManager::delegate and it can access
> > BeginFrameManager directly on either thread(main or compositor safely)
> >
> >> I think my current implementation that sending all messages between
> >>
> > BeginFrameManager and scheduler through proxy and LTH is not efficient.
> >
> >> Does this also make sense?
> >> With this, authoritative interval also could be updated easily, too.
> >>
> >
> > Don't worry about optimizing ThreadProxy right now since BeginFrameManager
> > won't
> > be used with ThreadProxy soon. We can optimize ThreadProxy in the future
> > if we
> > need to. What you have now with the PostTasks is good enough.
> >
> > https://codereview.chromium.org/423773002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2014-08-26 08:24:48]

brianderson@, Please take a look.
I made Scheduler as a subclass of BeginFrameManager::Delegate.
Thanks for review!

https://codereview.chromium.org/423773002/diff/160001/ash/display/display_con...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/423773002/diff/160001/ash/display/display_con...
ash/display/display_controller.cc:130: * Should update this to somewhere.
On 2014/08/21 00:13:59, brianderson wrote:
> Let's route it to the cc::Scheduler. Only the Browser will have a
> SyntheticBeginFrameSource, which is the only class that will really need to
know
> about the authoritative vsync interval.
> 
> Scheduler::CommitVSyncParameters can then override the interval the same way
> CompositorVSyncManager::UpdateVSyncParameters used to.

Done.

https://codereview.chromium.org/423773002/diff/160001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/423773002/diff/160001/cc/base/switches.cc#new...
cc/base/switches.cc:74: // Use a eginFrame signal from browser to renderer to
scheduling rendering.
On 2014/08/21 00:13:59, brianderson wrote:
> egin -> Begin

Done.

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:306: !begin_frame_requested_);
On 2014/08/21 00:13:59, brianderson wrote:
> Instead of taking begin_frame_requested_ into account here, would it make
sense
> to take it into account from inside SchedulerStateMachine::BeginFrameNeeded?

Yeap, handling this state by SchdulerStateMachine would make sense!

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:419: begin_frame_requested_ = false;
On 2014/08/21 00:13:59, brianderson wrote:
> The child Schedulers should be setting begin_frame_requested explicitly to
false
> and the parent Scheduler should not set it to false implicitly like this. That
> way we reduce the number of messages between the Renderer and Browser.

Yeap, calling SetChildrenNeedBeginFrames() from BeginFrameManager can make much
simpler :)

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:636:
synthetic_begin_frame_source_->SetNeedsBeginFrame(true,
On 2014/08/21 00:13:59, brianderson wrote:
> Instead of calling SetNeedsBeginFrame direcly here, please call
> ProcessScheduledActions instead, which will go through the common way of
setting
> up the value for SetNeedsBeginFrame.
> 
> That will properly handle the vsync disabled case.

Done.

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:214: bool begin_frame_requested_;
On 2014/08/21 00:13:59, brianderson wrote:
> Store this in the state machine as children_need_begin_frames_ instead.

Done.

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.h:27: bool begin_frame_scheduling_enabled;
On 2014/08/21 00:13:59, brianderson wrote:
> Maybe we should rename this to begin_frame_receiver to more clearly
> differentiate it's meaning from begin_frame_publisher.

Done.

https://codereview.chromium.org/423773002/diff/160001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.h:35: bool begin_frame_publisher;
On 2014/08/21 00:13:59, brianderson wrote:
> Group this next to begin_frame_scheduling_enabled.

Done.

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:302: void SendBeginFrame(const BeginFrameArgs& args);
On 2014/08/21 00:13:59, brianderson wrote:
> SendBeginFrameToChildren?

Removed this and RequestBeginFrame().
Changed Scheduler as a subclass of BeginFrameManager::Delegate.

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:463: bool begin_frame_requested_;
On 2014/08/21 00:13:59, brianderson wrote:
> I don't think LTH needs to store this. If early outs are needed, it should be
> done at a higher level.

Removed.

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_client.h:49: virtual void SendBeginFrame(const
BeginFrameArgs& args) {}
On 2014/08/21 00:13:59, brianderson wrote:
> Can you make the default implementation call NOT_REACHED() until it becomes
pure
> virtual?

Removed.
Scheduler will call BeginFrameManager::SendBeginFrame directly.

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.h:84: bool begin_frame_publisher;
On 2014/08/21 00:13:59, brianderson wrote:
> Group with begin_frame_scheduling_enabled.

Done.

https://codereview.chromium.org/423773002/diff/160001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/proxy.h#newcod...
cc/trees/proxy.h:103: virtual void BeginFrameRequested() = 0;
On 2014/08/21 00:14:00, brianderson wrote:
> SetChildrenNeedBeginFrames(bool enable)?
> 
> The 's' in "Frames" indicate that it's level triggered. "Children"
> differentiates the request from main thread requests.

This also removed.

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

https://codereview.chromium.org/423773002/diff/160001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:556: if (scheduler_on_impl_thread_)
On 2014/08/21 00:14:00, brianderson wrote:
> In what case would a Renderer request a BeginFrame when the Browser doesn't
have
> a scheduler?

This function also removed. :)

https://codereview.chromium.org/423773002/diff/160001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/423773002/diff/160001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1194: Proxy::MainThreadTaskRunner()->PostTask(
On 2014/08/21 00:36:49, simonhong wrote:
> On 2014/08/21 00:14:00, brianderson wrote:
> > It would be nice if we could send directly Renderers from the Impl thread
but,
> > since we are moving to SingleThreadProxy Browsers everywhere, this is fine.
> 
> Is ChromeOS also a target for SingleThreadProxy?
> It is currently set to threaded mode explicitly.

This also removed.

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

https://codereview.chromium.org/423773002/diff/160001/cc/trees/thread_proxy.h...
cc/trees/thread_proxy.h:72: bool begin_frame_requested;
On 2014/08/21 00:14:00, brianderson wrote:
> ThreadProxy shouldn't have to cache this.

Removed.

https://codereview.chromium.org/423773002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/423773002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2493: return;
On 2014/08/21 00:14:00, brianderson wrote:
> I think it would be better for the RWHV to add or remove itself as an observer
> in this method, rather than in WasShown or WasHidden.
> 

It's much much better than now!
Thanks :)

https://codereview.chromium.org/423773002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/423773002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:92: public
ui::BeginFrameManager::Observer,
On 2014/08/21 00:14:00, brianderson wrote:
> Is the plan to land this first as Aura-only and then unify it with all other
> platforms as a separate patch?

I'll implement mac and android also in this CL,
then split this CL into other smaller CLs with test codes.

https://codereview.chromium.org/423773002/diff/160001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/423773002/diff/160001/content/common/view_mes...
content/common/view_messages.h:962: #if !defined(OS_MACOSX)
On 2014/08/21 00:14:00, brianderson wrote:
> Should this be:
> #if defined(OS_ANDROID) || defined(USE_AURA) 

Done.

https://codereview.chromium.org/423773002/diff/160001/ui/base/compositor/begi...
File ui/base/compositor/begin_frame_manager.cc (right):

https://codereview.chromium.org/423773002/diff/160001/ui/base/compositor/begi...
ui/base/compositor/begin_frame_manager.cc:25: void
BeginFrameManager::RequestBeginFrame() const {
On 2014/08/21 00:14:00, brianderson wrote:
> I think this method can be removed.
> 
> Instead, delegate_->SetChildrenNeedBeginFrames(true/false) can be called from
> AddObserver when the number of observers goes from 0 to 1, and from
> RemoveObserver when the number of observers goes from 1 to 0.

Yes! This also can make code more simpler :)
Removed.

https://codereview.chromium.org/423773002/diff/160001/ui/base/compositor/begi...
ui/base/compositor/begin_frame_manager.cc:31: void
BeginFrameManager::AddObserver(Observer* observer) {
On 2014/08/21 00:14:00, brianderson wrote:
> You may want to make this:
> void BeginFrameManager::AddObserver(Observer* observer, BeginFrameArgs
> last_begin_frame_args_sent_to_observer)
> 
> And remember a last_begin_frame_args_sent in
BeginFrameManager::SendBeginFrame.
> 
> Then, if last_begin_frame_args_sent_to_observer != last_begin_frame_args_sent
> and last_begin_frame_args_sent.deadline hasn't passed, send
> last_begin_frame_args_sent to the new observer immediately.

Does we need more heuristics for this like "use last args if deadline is greater
than now + interval/3"?

[brianderson, 2014-08-27 01:34:08]

Still need to review a lot of this patch, but wanted to send what I have before
I head out for the day.

> Does we need more heuristics for this like "use last args if deadline is
greater
than now + interval/3"?

No, don't worry about it. The Renderer can decide to ignore it if it wants to.

https://codereview.chromium.org/423773002/diff/260001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/423773002/diff/260001/cc/base/switches.cc#new...
cc/base/switches.cc:74: // Use a BeginFrame signal from browser to renderer to
scheduling rendering.
scheduling -> schedule

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_manager.cc (right):

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_manager.cc:8: bool IsSameBeginFrameArgs(const
cc::BeginFrameArgs arg1,
Can you move this to begin_frame_args.h, take in const references rather than
just consts, and overload ==?

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_manager.h (right):

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_manager.h:31: // Subclass should implement howto
schedule next BeginFrame.
howto -> how to

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_manager.h:53: // Request BeginFrame to observers.
Request -> Send

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_manager.h:71: // Used to send to the new observer
immediately.
"the new observer" -> "any new observers"

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:153: // the publisher.
the -> as the

[simonhong, 2014-08-27 01:39:55]

On 2014/08/27 01:34:08, brianderson wrote:
> Still need to review a lot of this patch, but wanted to send what I have
before
> I head out for the day.

Thanks brian :)

> 
> > Does we need more heuristics for this like "use last args if deadline is
> greater
> than now + interval/3"?
> 
> No, don't worry about it. The Renderer can decide to ignore it if it wants to.
> 
> https://codereview.chromium.org/423773002/diff/260001/cc/base/switches.cc
> File cc/base/switches.cc (right):
> 
>
https://codereview.chromium.org/423773002/diff/260001/cc/base/switches.cc#new...
> cc/base/switches.cc:74: // Use a BeginFrame signal from browser to renderer to
> scheduling rendering.
> scheduling -> schedule
> 
>
https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
> File cc/scheduler/begin_frame_manager.cc (right):
> 
>
https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_manager.cc:8: bool IsSameBeginFrameArgs(const
> cc::BeginFrameArgs arg1,
> Can you move this to begin_frame_args.h, take in const references rather than
> just consts, and overload ==?
> 
>
https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
> File cc/scheduler/begin_frame_manager.h (right):
> 
>
https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_manager.h:31: // Subclass should implement howto
> schedule next BeginFrame.
> howto -> how to
> 
>
https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_manager.h:53: // Request BeginFrame to observers.
> Request -> Send
> 
>
https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_manager.h:71: // Used to send to the new observer
> immediately.
> "the new observer" -> "any new observers"
> 
> https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/scheduler.h
> File cc/scheduler/scheduler.h (right):
> 
>
https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.h:153: // the publisher.
> the -> as the

[simonhong, 2014-08-27 07:56:13]

brianderson@, thanks for review!
Mac and Android part is in early stage.
You can skip that parts.

https://codereview.chromium.org/423773002/diff/260001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/423773002/diff/260001/cc/base/switches.cc#new...
cc/base/switches.cc:74: // Use a BeginFrame signal from browser to renderer to
scheduling rendering.
On 2014/08/27 01:34:07, brianderson wrote:
> scheduling -> schedule

Done.

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_manager.cc (right):

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_manager.cc:8: bool IsSameBeginFrameArgs(const
cc::BeginFrameArgs arg1,
On 2014/08/27 01:34:07, brianderson wrote:
> Can you move this to begin_frame_args.h, take in const references rather than
> just consts, and overload ==?

Done.

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_manager.h (right):

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_manager.h:31: // Subclass should implement howto
schedule next BeginFrame.
On 2014/08/27 01:34:08, brianderson wrote:
> howto -> how to

Done.

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_manager.h:53: // Request BeginFrame to observers.
On 2014/08/27 01:34:08, brianderson wrote:
> Request -> Send

Done.

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_manager.h:71: // Used to send to the new observer
immediately.
On 2014/08/27 01:34:08, brianderson wrote:
> "the new observer" -> "any new observers"

Done.

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/423773002/diff/260001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:153: // the publisher.
On 2014/08/27 01:34:08, brianderson wrote:
> the -> as the

Done.

[simonhong, 2014-08-27 07:56:18]

brianderson@, thanks for review!
Mac and Android part is in early stage.
You can skip that parts.

[brianderson, 2014-08-28 02:48:07]

Very excited about this patch. Looking forward to how Android and Mac progress.

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:137: if (interval == base::TimeDelta())
Might as well roll this condition into your if/else chain below.

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:429: // |arg.deadline| should be adjusted by child
Scheduler.
Today, the deadline the Browser sends to the Renderers should subtract all of
the following:
1) BeginMainFrameToCommitDurationEstimate
2) CommitToActivateDurationEstimate
3) DrawDurationEstimate
4) EstimatedParentDrawTime (this will include GPU Service and driver latency
eventually).

Once we have commitless updates, we can get rid of 1 and 2, but we aren't there
yet. Put in a TODO regarding the possibility of removing 1 and 2.

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.h (right):

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.h:29: bool using_platform_vsync;
Can you use begin_frame_receiver to represent using_platform_vsync, instead of
having two settings?

I think it makes sense for us to consider Android's and Mac's Browser as both a
BeginFrame receiver and publisher.

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:709: return
(BeginFrameNeededToAnimateOrDraw() || BeginFrameNeededToChildren());
This should not include BeginFrameNeededToChildren. In this if block, we should
DCHECK(!BeginFrameNeedeToChildren()) since the synchronous compositor doesn't
have a Browser.

If we ever need to support such a configuration we'd need to figure out a way to
send BeginFrames to children without triggering a Browser draw, which would get
complicated. Luckily, there are no plans to do support that config.

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:712: BeginFrameNeededToChildren() ||
BeginFrameNeededToChildren belongs in ProactiveBeginFrameWanted since the
Browser might not draw immediately if the Renderer is slow.

https://codereview.chromium.org/423773002/diff/280001/content/browser/composi...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/423773002/diff/280001/content/browser/composi...
content/browser/compositor/browser_compositor_output_surface.cc:78:
CommitVSyncParameters(timebase, interval);
OnUpdateVSyncParametersFromGpu calls CommitVSyncParameters no when it didn't
before. Is that okay?

https://codereview.chromium.org/423773002/diff/280001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/423773002/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2157:
base::TimeDelta estimated_browser_composite_time =
We should be able to remove this adjustment here and the cc::Scheduler should
adjust the deadline instead since it has all the timing estimates.

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/composito...
ui/compositor/compositor.cc:240:
begin_frame_manager_->SetAuthoritativeVSyncInterval(interval);
I think it'll be better to have SetAuthoritativeVSyncInterval go through the LTH
and Proxy, rather than the BeginFrameManager. Even though the BeginFrameManager
will only exist with SingleThreadProxy, the Scheduler conceptually lives on the
Impl-side, and communicating to the Scheduler directly like this, without going
through the Proxy, seems a bit off.

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/composito...
ui/compositor/compositor.h:190: cc::BeginFrameManager* begin_frame_manager()
const {
Instead of exposing the BeginFrameManager to everyone, I think it would be
cleaner to only expose it through forwarding methods instead.

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/composito...
ui/compositor/compositor.h:275: scoped_ptr<cc::BeginFrameManager>
begin_frame_manager_;
Would it make sense for the LTH or Proxy to own the BeginFrameManager instead? I
don't think all platforms use ui::Compositor.

[simonhong, 2014-08-29 00:03:02]

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/composito...
ui/compositor/compositor.h:275: scoped_ptr<cc::BeginFrameManager>
begin_frame_manager_;
On 2014/08/28 02:48:07, brianderson wrote:
> Would it make sense for the LTH or Proxy to own the BeginFrameManager instead?
I
> don't think all platforms use ui::Compositor.

My plan was making BeginFrameManager as a member variable of each
compositor(ui::Compositor and CompositorImpl).
After seeing your this and above comment, each compositor only needs to have
BeginFrameObserver that manage RWHV.
After removing BeginFrameManager, current BeginFrameManger::Delegate's api
should
move into LTH. Then, only interaction between compositor and LTH is needed.
With this, we don't need to expose Scheduler to compositor as you said and all
requests
(ex, SetChildrenNeedsBeginFrame, authoritative, StartBeginFrame) will be
delivered to scheduler
only through LTH. (StartBeginFrame will be used to call scheduler's BeginFrame()
when vsync is triggered on Mac and Android)
What do you think about this change?

[brianderson, 2014-08-29 02:01:18]

On 2014/08/29 00:03:02, simonhong wrote:
>
https://codereview.chromium.org/423773002/diff/280001/ui/compositor/compositor.h
> File ui/compositor/compositor.h (right):
> 
>
https://codereview.chromium.org/423773002/diff/280001/ui/compositor/composito...
> ui/compositor/compositor.h:275: scoped_ptr<cc::BeginFrameManager>
> begin_frame_manager_;
> On 2014/08/28 02:48:07, brianderson wrote:
> > Would it make sense for the LTH or Proxy to own the BeginFrameManager
instead?
> I
> > don't think all platforms use ui::Compositor.
> 
> My plan was making BeginFrameManager as a member variable of each
> compositor(ui::Compositor and CompositorImpl).
> After seeing your this and above comment, each compositor only needs to have
> BeginFrameObserver that manage RWHV.
> After removing BeginFrameManager, current BeginFrameManger::Delegate's api
> should
> move into LTH. Then, only interaction between compositor and LTH is needed.
> With this, we don't need to expose Scheduler to compositor as you said and all
> requests
> (ex, SetChildrenNeedsBeginFrame, authoritative, StartBeginFrame) will be
> delivered to scheduler
> only through LTH. (StartBeginFrame will be used to call scheduler's
BeginFrame()
> when vsync is triggered on Mac and Android)
> What do you think about this change?

That sounds like a good plan.

A couple other things:

1) Would it be difficult to wire your StartBeginFrame logic to go through
OutputSurface->LTHI->Proxy->Scheduler instead of LTH? The OutputSurface is the
BeginFrame source for the Renderers and it would be nice to have the Browser
behave the same way. Also, the plan for Ozone is to expose the BeginFrame
through the OutputSurface eventually. (See
https://code.google.com/p/chromium/issues/detail?id=370524)

2) Since you are touching the --enable-begin-frame-scheduling command-line flag
already, do you think it would be easy to remove the command-line aspect of it
and set begin_frame_receiver flag directly in the code instead? We've been
intending to remove it as a command-line option since it's really just a
platform-specific setting.

[simonhong, 2014-09-03 01:13:57]

Patchset #18 (id:360001) has been deleted

[simonhong, 2014-09-03 01:14:20]

Patchset #17 (id:320001) has been deleted

[simonhong, 2014-09-03 01:14:38]

Patchset #16 (id:300001) has been deleted

[simonhong, 2014-09-03 20:58:45]

Patchset #20 (id:460001) has been deleted

[simonhong, 2014-09-03 21:02:23]

brian, would you please review again?
Command line is removed and OutputSurface is wired with compositor
for requesting BeginFrame to scheduler.
(But, I still need to debug Mac implementation. you can skip mac stuff)

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:137: if (interval == base::TimeDelta())
On 2014/08/28 02:48:07, brianderson wrote:
> Might as well roll this condition into your if/else chain below.

Done.

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:429: // |arg.deadline| should be adjusted by child
Scheduler.
On 2014/08/28 02:48:07, brianderson wrote:
> Today, the deadline the Browser sends to the Renderers should subtract all of
> the following:
> 1) BeginMainFrameToCommitDurationEstimate
> 2) CommitToActivateDurationEstimate
> 3) DrawDurationEstimate
> 4) EstimatedParentDrawTime (this will include GPU Service and driver latency
> eventually).
> 
> Once we have commitless updates, we can get rid of 1 and 2, but we aren't
there
> yet. Put in a TODO regarding the possibility of removing 1 and 2.

Done.

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.h (right):

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.h:29: bool using_platform_vsync;
On 2014/08/28 02:48:07, brianderson wrote:
> Can you use begin_frame_receiver to represent using_platform_vsync, instead of
> having two settings?
> 
> I think it makes sense for us to consider Android's and Mac's Browser as both
a
> BeginFrame receiver and publisher.

I didn't think of using both option!
Using both in Android and Mac looks good.
Removed.

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:709: return
(BeginFrameNeededToAnimateOrDraw() || BeginFrameNeededToChildren());
On 2014/08/28 02:48:07, brianderson wrote:
> This should not include BeginFrameNeededToChildren. In this if block, we
should
> DCHECK(!BeginFrameNeedeToChildren()) since the synchronous compositor doesn't
> have a Browser.
> 
> If we ever need to support such a configuration we'd need to figure out a way
to
> send BeginFrames to children without triggering a Browser draw, which would
get
> complicated. Luckily, there are no plans to do support that config.

Done.

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:712: BeginFrameNeededToChildren() ||
On 2014/08/28 02:48:07, brianderson wrote:
> BeginFrameNeededToChildren belongs in ProactiveBeginFrameWanted since the
> Browser might not draw immediately if the Renderer is slow.

How Browser knows whether Renderer is slow or not in
ProactiveBeginFrameWanted()?
I think it only checks Browser-side scheduler status.

https://codereview.chromium.org/423773002/diff/280001/content/browser/composi...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/423773002/diff/280001/content/browser/composi...
content/browser/compositor/browser_compositor_output_surface.cc:78:
CommitVSyncParameters(timebase, interval);
On 2014/08/28 02:48:07, brianderson wrote:
> OnUpdateVSyncParametersFromGpu calls CommitVSyncParameters no when it didn't
> before. Is that okay?

I can't catch the point you care about.
This function will be always called when BrowserCompositorOutputSurfaceProxy
received GpuHostMsg_UpdateVSyncParameters from gpu process.

Question)
Do we need to maintain Unified BeginFrame mode and previous mode?
In this cl, I removed CompositorVSyncManager.
If this cl is landed when chromeos's browser compositor is still running on the
threaded mode, it will be a problem.
Or does this cl can be landed after chromeos's browser compositor is changed to
use single threaded mode?

https://codereview.chromium.org/423773002/diff/280001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/423773002/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2157:
base::TimeDelta estimated_browser_composite_time =
On 2014/08/28 02:48:07, brianderson wrote:
> We should be able to remove this adjustment here and the cc::Scheduler should
> adjust the deadline instead since it has all the timing estimates.

Yep, did it in the scheduler.

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/composito...
ui/compositor/compositor.cc:240:
begin_frame_manager_->SetAuthoritativeVSyncInterval(interval);
On 2014/08/28 02:48:07, brianderson wrote:
> I think it'll be better to have SetAuthoritativeVSyncInterval go through the
LTH
> and Proxy, rather than the BeginFrameManager. Even though the
BeginFrameManager
> will only exist with SingleThreadProxy, the Scheduler conceptually lives on
the
> Impl-side, and communicating to the Scheduler directly like this, without
going
> through the Proxy, seems a bit off.

Done.

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/423773002/diff/280001/ui/compositor/composito...
ui/compositor/compositor.h:190: cc::BeginFrameManager* begin_frame_manager()
const {
On 2014/08/28 02:48:07, brianderson wrote:
> Instead of exposing the BeginFrameManager to everyone, I think it would be
> cleaner to only expose it through forwarding methods instead.

BeginFrameManager is removed.
Instead, {Add|Remove}BeginFrameObserver() is added and will be used
by RWHV.

[brianderson, 2014-09-04 18:45:43]

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

[brianderson, 2014-09-04 18:45:47]

+ccameron for feedback on the Mac-specific parts.

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/423773002/diff/280001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:712: BeginFrameNeededToChildren() ||
Nevermind about my comment here. What you have is good.

I wanted to think of children wanting BeginFrames as a proactive signal (and
have the logic equal to what you already have), but I think that was just me
trying to fit a new concept into an old way of thinking.

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

https://codereview.chromium.org/423773002/diff/500001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:54: return (frame_time == other.frame_time &
& -> &&

https://codereview.chromium.org/423773002/diff/500001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/423773002/diff/500001/cc/output/output_surfac...
cc/output/output_surface.h:140: void BeginFrame(const BeginFrameArgs& args)
const;
Should this be OnBeginFrame and protected? Can you add a comment about what this
is for?

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:411: adjusted_args.deadline -=
EstimatedParentDrawTime();
We'll want two different adjusted_args: one for the children and one for the
Browser.

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:136: if (authoritative_vsync_interval_ ==
base::TimeDelta()) {
nit: I was imagining a form that doesn't require nested ifs:

if (authoritative_vsync_interval_ != base::TimeDelta()) {
  vsync_interval_ = authoritative_vsync_interval;
} else if (interval == base::TimeDelta()) {
  vsync_interval_ = BeginFrameArgs::DefaultInterval();
} else {
  vsync_interval_ = interval_;
}

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:432: client_->SendBeginFrameToChildren(args);
You need to send the adjusted_args rather than the args now.

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.cc:40: DCHECK(!begin_frame_publisher);
Let's not worry about this DCHECK since it is artificially limiting other
platforms. Aura shouldn't need this DCHECK either for example.

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:709:
DCHECK(!BeginFrameNeededToChildren());
Can you add a comment regarding why we added this DCHECK?

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:743: #if !defined(OS_ANDROID) &&
!defined(OS_MACOSX)
We try to avoid platforms-specific defines in cc if possible.
Maybe just DCHECK(settings_.begin_frame_publisher) for all platforms?

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.h:243: bool BeginFrameNeededToChildren()
const;
BeginFrameNeededToChildren -> BeginFrameNeededForChildren

https://codereview.chromium.org/423773002/diff/500001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/423773002/diff/500001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:442: client_->DidLoseOutputSurface();
Why is this needed?

https://codereview.chromium.org/423773002/diff/500001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/423773002/diff/500001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.h:25: // When we use BeginFrame scheduling,
Android's and Mac's Browser should act
nit: I feel like these comments belong more in SchedulerSettings since they
affect the Scheduler the most.

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:470:
begin_frame_scheduling_is_enabled_ = true;
Can we assume this is true for all Aura platforms?

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.h:596: void
OnSetNeedsBeginFrame(bool enabled);
Should this be grouped with OnSendBeginFrame?

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:660:
delegated_frame_host_->WasShown(ui::LatencyInfo());
Why pass ui::LatencyInfo() now?

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:896:
DCHECK(!GetCompsoitr());
RemoveBeginFrameObserver?

https://codereview.chromium.org/423773002/diff/500001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/423773002/diff/500001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:172: #if defined(OS_ANDROID) ||
(defined(USE_AURA) &&!defined(OS_CHROMEOS))
Why !OS_CHROMEOS?
For platforms other than Mac, can we always set settings.begin_frame_receiver to
true in the Renderer?

https://codereview.chromium.org/423773002/diff/500001/ui/base/android/window_...
File ui/base/android/window_android.cc (right):

https://codereview.chromium.org/423773002/diff/500001/ui/base/android/window_...
ui/base/android/window_android.cc:71: // Makes |compositor_| NULL after all
observers finishes their work.
This comment would be more helpful if it were a "why" instead of a "what".

https://codereview.chromium.org/423773002/diff/500001/ui/base/compositor/comp...
File ui/base/compositor/compositor_begin_frame_observer.h (right):

https://codereview.chromium.org/423773002/diff/500001/ui/base/compositor/comp...
ui/base/compositor/compositor_begin_frame_observer.h:14: class
CompositorBeginFrameObserver {
Could this class go in any other header?

https://codereview.chromium.org/423773002/diff/500001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/423773002/diff/500001/ui/compositor/composito...
ui/compositor/compositor.cc:89: #if defined(USE_AURA) && !defined(OS_CHROMEOS)
Why !OS_CHROMOS?

https://codereview.chromium.org/423773002/diff/500001/ui/compositor/composito...
ui/compositor/compositor.cc:249: if (output_surface_lost_)
Hmm. Having to expose output_surface_lost_ to the compositor is unfortunate. I
wonder if we can get rid of StartBeginFrame from Compositor and only have the
OutputSurface interact with the platform-specific bits.

https://codereview.chromium.org/423773002/diff/500001/ui/compositor/composito...
ui/compositor/compositor.cc:251: task_runner_->PostTask(
Does this need to be a post task?

https://codereview.chromium.org/423773002/diff/500001/ui/compositor/composito...
ui/compositor/compositor.cc:287: bool need_begin_frame = false;
bool need_begin_frame = begin_frame_observer_list_.might_have_observers();

https://codereview.chromium.org/423773002/diff/500001/ui/compositor/composito...
ui/compositor/compositor.cc:295: task_runner_->PostTask(
Does this need to be a PostTask?

[ccameron, 2014-09-04 19:29:23]

Looks good overall, but perhaps some of the code in RWHVMac and RWHVAura can be
merged?

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:616: // True when
RenderWidget needs a BeginFrame.
This has an identical bit in RWHVMac -- would it be possible to move all of this
to DelegatedFrameHost and put any RenderWidgetHostView-specific bits against
DelegatedFrameHostClient?

[piman, 2014-09-04 20:34:25]

https://codereview.chromium.org/423773002/diff/500001/cc/trees/proxy.cc
File cc/trees/proxy.cc (right):

https://codereview.chromium.org/423773002/diff/500001/cc/trees/proxy.cc#newco...
cc/trees/proxy.cc:91: NOTREACHED() << "Only used by SingleThreadProxy";
This is not true - on Chrome OS we use the ThreadProxy (currently) and we will
end up calling this.

[piman, 2014-09-04 20:56:38]

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.cc:38: #if !defined(OS_ANDROID) &&
!defined(OS_MACOSX)
Please no #ifdef like this. It prevents testing.

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:875: //
destruction/cleanup code should happen there, not here.
Beware of this comment!

https://codereview.chromium.org/423773002/diff/500001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2430: }
A lot of this logic is common between mac and aura. Can it be moved into
DelegatedFrameHost?

https://codereview.chromium.org/423773002/diff/500001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/423773002/diff/500001/ui/compositor/composito...
ui/compositor/compositor.cc:93: if
(command_line->HasSwitch(switches::kEnableDelegatedRenderer)) {
This is a layering violation.

Can this configuration be passed to the constructor instead of ifdefs?

Either way, I think we want to make this a flag until we ship it. We need to be
able to disable it on a branch if it causes problems.

https://codereview.chromium.org/423773002/diff/500001/ui/compositor/composito...
ui/compositor/compositor.cc:347: output_surface_weak_ptr_ =
output_surface->GetWeakPtr();
This can't work when we use the compositor thread. The output surface is
destroyed on the compositor thread, which means you can only dereference the
weak pointer on that thread.

[mithro-old, 2014-09-09 03:22:37]

Just a couple of very small comments.

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

https://codereview.chromium.org/423773002/diff/500001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:53: bool operator==(const BeginFrameArgs& other)
const {
We have an operator== used just for testing in cc/test/begin_frame_args_test.h

Could you please remove that copy of the function?

It would probably be worth doing a minor refactoring the tests in
cc/output/begin_frame_args_unittest.cc to pull the equality tests out into their
own function.

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/423773002/diff/500001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:195: base::TimeDelta authoritative_vsync_interval_;
What is the difference between "authoritative_vsync_interval" and
"vsync_interval"?

[mithro-old, 2014-09-15 10:56:23]

Hi Simon,

In an effort to better understand this patch I have taken your patch, rebased it
onto master and made all the test on Linux compile (but they do not yet pass).
You can see the results in https://codereview.chromium.org/548153004/

I'll update that CL if I do any more work on it and mention it here.

Tim

[jdduke (slow), 2014-09-17 15:11:01]

On 2014/09/15 10:56:23, mithro wrote:
> Hi Simon,
> 
> In an effort to better understand this patch I have taken your patch, rebased
it
> onto master and made all the test on Linux compile (but they do not yet pass).
> You can see the results in https://codereview.chromium.org/548153004/
> 
> I'll update that CL if I do any more work on it and mention it here.
> 
> Tim

Are there any portions of this patch we can split out and start landing? What's
the best way to make progress here?

[simonhong, 2014-09-17 18:35:34]

Sorry for the delay.
I'm still on vacation until 9/22.
I think cc-side can land first then ui or content side.
Maybe, I can re-start this work next week.
2014. 9. 17. 오후 6:11에 <jdduke@chromium.org>님이 작성:

> On 2014/09/15 10:56:23, mithro wrote:
>
>> Hi Simon,
>>
>
>  In an effort to better understand this patch I have taken your patch,
>> rebased
>>
> it
>
>> onto master and made all the test on Linux compile (but they do not yet
>> pass).
>> You can see the results in https://codereview.chromium.org/548153004/
>>
>
>  I'll update that CL if I do any more work on it and mention it here.
>>
>
>  Tim
>>
>
> Are there any portions of this patch we can split out and start landing?
> What's
> the best way to make progress here?
>
> https://codereview.chromium.org/423773002/
>

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

[simonhong, 2014-09-26 06:57:13]

There are many pending issues before landing this cl.
1) Makes ui::Compositor use cc::Scheduler
2) Makes SingleThreadProxy use cc::Scheduler (Implementation is over, but turned
off)
3) Decouple BeginFrame mechanism from OutputSurface
(https://code.google.com/p/chromium/issues/detail?id=416760)

enne@ is still working for (1) and (2).
New issue(3) is created during the working on this CL.

This cl will be suspended until above three items are cleared.
I was trying to land cc::Scheduler part of this cl, but there is ongoing work
for refactoring
BeginFrame mechanism by Tim.

So, I will start 3). then back to here again.

[brianderson, 2014-09-27 04:49:54]

On 2014/09/26 06:57:13, simonhong wrote:
> There are many pending issues before landing this cl.
> 1) Makes ui::Compositor use cc::Scheduler
> 2) Makes SingleThreadProxy use cc::Scheduler (Implementation is over, but
turned
> off)
> 3) Decouple BeginFrame mechanism from OutputSurface
> (https://code.google.com/p/chromium/issues/detail?id=416760)
> 
> enne@ is still working for (1) and (2).
> New issue(3) is created during the working on this CL.
> 
> This cl will be suspended until above three items are cleared.
> I was trying to land cc::Scheduler part of this cl, but there is ongoing work
> for refactoring
> BeginFrame mechanism by Tim.
> 
> So, I will start 3). then back to here again.

Thanks Simon. That sounds like a good plan if you think you are totally blocked
on this. The unified BeginFrame is blocking progress in other areas though, so
let me know if you can start landing parts of this in parallel. I anticipate
Tim's refactoring work will land next week, which should help unblock you.

[jdduke (slow), 2014-10-13 15:06:56]

On 2014/09/27 04:49:54, brianderson wrote:
> On 2014/09/26 06:57:13, simonhong wrote:
> > There are many pending issues before landing this cl.
> > 1) Makes ui::Compositor use cc::Scheduler
> > 2) Makes SingleThreadProxy use cc::Scheduler (Implementation is over, but
> turned
> > off)
> > 3) Decouple BeginFrame mechanism from OutputSurface
> > (https://code.google.com/p/chromium/issues/detail?id=416760)
> > 
> > enne@ is still working for (1) and (2).
> > New issue(3) is created during the working on this CL.
> > 
> > This cl will be suspended until above three items are cleared.
> > I was trying to land cc::Scheduler part of this cl, but there is ongoing
work
> > for refactoring
> > BeginFrame mechanism by Tim.
> > 
> > So, I will start 3). then back to here again.
> 
> Thanks Simon. That sounds like a good plan if you think you are totally
blocked
> on this. The unified BeginFrame is blocking progress in other areas though, so
> let me know if you can start landing parts of this in parallel. I anticipate
> Tim's refactoring work will land next week, which should help unblock you.

Is there nothing from this patch that we can land in the meantime?

[simonhong, 2014-10-13 21:40:02]

On 2014/10/13 15:06:56, jdduke wrote:
> On 2014/09/27 04:49:54, brianderson wrote:
> > On 2014/09/26 06:57:13, simonhong wrote:
> > > There are many pending issues before landing this cl.
> > > 1) Makes ui::Compositor use cc::Scheduler
> > > 2) Makes SingleThreadProxy use cc::Scheduler (Implementation is over, but
> > turned
> > > off)
> > > 3) Decouple BeginFrame mechanism from OutputSurface
> > > (https://code.google.com/p/chromium/issues/detail?id=416760)
> > > 
> > > enne@ is still working for (1) and (2).
> > > New issue(3) is created during the working on this CL.
> > > 
> > > This cl will be suspended until above three items are cleared.
> > > I was trying to land cc::Scheduler part of this cl, but there is ongoing
> work
> > > for refactoring
> > > BeginFrame mechanism by Tim.
> > > 
> > > So, I will start 3). then back to here again.
> > 
> > Thanks Simon. That sounds like a good plan if you think you are totally
> blocked
> > on this. The unified BeginFrame is blocking progress in other areas though,
so
> > let me know if you can start landing parts of this in parallel. I anticipate
> > Tim's refactoring work will land next week, which should help unblock you.
> 
> Is there nothing from this patch that we can land in the meantime?

jdduke@, I'm trying to land another cl related with this
(https://codereview.chromium.org/619843002/).
That would be landed in one or two day.
I will try to find and land cc part of this cl in parallel.