cc: Add BeginFrameArgs

cc: Add BeginFrameArgs

In addition to the frame_time, include a deadline and interval
with BeginFrame. Values used are placeholders for now, but will
be used in the near future.

BUG=240945

[nduca, 2013-06-13 00:16:51]

yuhsss lgtm

[brianderson, 2013-06-13 00:16:58]

PTAL.

Depends on:
https://codereview.chromium.org/16833003

This sets and sends BeginFrameArgs everywhere, but doesn't yet use it.

[brianderson, 2013-06-13 01:03:03]

+jschuh for IPC security review.

[brianderson, 2013-06-13 01:10:18]

Also +apatrick for content/renderer/gpu review.

[no sievers, 2013-06-13 01:17:47]

Looks fine for Android with two questions

https://codereview.chromium.org/16863005/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/16863005/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:1235: 
Is this function still getting called with begin frame notifications enabled?

https://codereview.chromium.org/16863005/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/16863005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/compositor_impl_android.cc:50:
capabilities_.has_parent_compositor = false;
This is for the soon-to-be-deprecated WebView w/browser compositor path....

https://codereview.chromium.org/16863005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/compositor_impl_android.cc:374: return
make_scoped_ptr(new cc::OutputSurface(
...while this is created for Chrome, ContentShell etc. for the browser
compositor. Do we also need to set has_parent_compositor to false here?

[Sami, 2013-06-13 10:01:07]

Sweet, lgtm with some nits.

https://codereview.chromium.org/16863005/diff/1/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/16863005/diff/1/cc/output/begin_frame_args.h#...
cc/output/begin_frame_args.h:16: BeginFrameArgs();
Can we make this default constructor private/protected? Or maybe drop it
completely (and the explicit destructor too)?

https://codereview.chromium.org/16863005/diff/1/cc/output/begin_frame_args.h#...
cc/output/begin_frame_args.h:21: static BeginFrameArgs
CreateBeginFrame(base::TimeTicks frame_time,
Bikeshed: drop the "BeginFrame", it's cleaner :)
Create/CreateForSynchronousCompositor/CreateForTesting.

https://codereview.chromium.org/16863005/diff/1/cc/output/begin_frame_args.h#...
cc/output/begin_frame_args.h:29: static base::TimeDelta
DefaultDeadlineAdjustment();
Could this be a static constant or do you think we'll have some fancier
algorithm here in the future?

https://codereview.chromium.org/16863005/diff/1/cc/scheduler/frame_rate_contr...
File cc/scheduler/frame_rate_controller.cc (right):

https://codereview.chromium.org/16863005/diff/1/cc/scheduler/frame_rate_contr...
cc/scheduler/frame_rate_controller.cc:40:
interval_(base::TimeDelta::FromMicroseconds(16666)),
It might be worth making the default frame interval a static constant in
BeginFrameArgs too to avoid sprinkling it around the codebase.

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

https://codereview.chromium.org/16863005/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:154: last_begin_frame_args_ = args;
Dupe assignment?

https://codereview.chromium.org/16863005/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/16863005/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:1235: 
On 2013/06/13 01:17:47, Daniel Sievers wrote:
> Is this function still getting called with begin frame notifications enabled?

Yes, we call when attaching the view for the first time to establish the vsync
timebase and interval.

[brianderson, 2013-06-14 20:12:02]

https://codereview.chromium.org/16863005/diff/1/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/16863005/diff/1/cc/output/begin_frame_args.h#...
cc/output/begin_frame_args.h:16: BeginFrameArgs();
On 2013/06/13 10:01:07, Sami wrote:
> Can we make this default constructor private/protected? Or maybe drop it
> completely (and the explicit destructor too)?

Sure. To do so, we will also need to add a CreateEmpty for initialization of
member variables.

https://codereview.chromium.org/16863005/diff/1/cc/output/begin_frame_args.h#...
cc/output/begin_frame_args.h:21: static BeginFrameArgs
CreateBeginFrame(base::TimeTicks frame_time,
On 2013/06/13 10:01:07, Sami wrote:
> Bikeshed: drop the "BeginFrame", it's cleaner :)
> Create/CreateForSynchronousCompositor/CreateForTesting.

Sounds good.

https://codereview.chromium.org/16863005/diff/1/cc/output/begin_frame_args.h#...
cc/output/begin_frame_args.h:29: static base::TimeDelta
DefaultDeadlineAdjustment();
On 2013/06/13 10:01:07, Sami wrote:
> Could this be a static constant or do you think we'll have some fancier
> algorithm here in the future?

I wanted to make this a function in case we wanted to have it return different
values depending on the platform.

https://codereview.chromium.org/16863005/diff/1/cc/scheduler/frame_rate_contr...
File cc/scheduler/frame_rate_controller.cc (right):

https://codereview.chromium.org/16863005/diff/1/cc/scheduler/frame_rate_contr...
cc/scheduler/frame_rate_controller.cc:40:
interval_(base::TimeDelta::FromMicroseconds(16666)),
On 2013/06/13 10:01:07, Sami wrote:
> It might be worth making the default frame interval a static constant in
> BeginFrameArgs too to avoid sprinkling it around the codebase.

Good idea.

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

https://codereview.chromium.org/16863005/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:154: last_begin_frame_args_ = args;
On 2013/06/13 10:01:07, Sami wrote:
> Dupe assignment?

Will fix.

https://codereview.chromium.org/16863005/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/16863005/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:1235: 
On 2013/06/13 10:01:07, Sami wrote:
> On 2013/06/13 01:17:47, Daniel Sievers wrote:
> > Is this function still getting called with begin frame notifications
enabled?
> 
> Yes, we call when attaching the view for the first time to establish the vsync
> timebase and interval.

It is also called now when we did not draw after a BeginFrame and we want a
newer BeginFrame.

https://codereview.chromium.org/16863005/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/16863005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/compositor_impl_android.cc:374: return
make_scoped_ptr(new cc::OutputSurface(
On 2013/06/13 01:17:47, Daniel Sievers wrote:
> ...while this is created for Chrome, ContentShell etc. for the browser
> compositor. Do we also need to set has_parent_compositor to false here?

Ok, will change this.

[brianderson, 2013-06-14 21:07:17]

https://codereview.chromium.org/16863005/diff/1/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/16863005/diff/1/cc/output/begin_frame_args.h#...
cc/output/begin_frame_args.h:16: BeginFrameArgs();
On 2013/06/14 20:12:02, Brian Anderson wrote:
> On 2013/06/13 10:01:07, Sami wrote:
> > Can we make this default constructor private/protected? Or maybe drop it
> > completely (and the explicit destructor too)?
> 
> Sure. To do so, we will also need to add a CreateEmpty for initialization of
> member variables.

Actually, to send this as a message, we need a default constructor. I will have
the default constructor create an invalid set of values.

[no sievers, 2013-06-14 21:15:32]

lgtm for Android.

+piman for Aura

There might be some overlap with has_parent_compositor and other existing flags,
might be worth at some point to step back and look at our different modes and
decisions being made based on that and to see how we can simplify that.

https://codereview.chromium.org/16863005/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/16863005/diff/14001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:370: cc::OutputSurface*
output_surface = new cc::OutputSurface(
nit:
scoped_ptr<OutputSurface> output_surface(new ...());
return output_surface.Pass();

[piman, 2013-06-14 22:53:45]

I'm confused about what this does, so I have some questions first...

https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_arg...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_arg...
cc/output/begin_frame_args.cc:46: return
base::TimeDelta::FromMicroseconds(-8888);
Can you explain to me what this means, and why it's a hard-coded value that
applies to all platforms, even the ones that haven't switched to the BeginFrame
flow yet?

Why half a frame? Does it mean that the renderer only has half a frame to
produce its output from the time it receives the BeginFrame?

https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_arg...
cc/output/begin_frame_args.h:10: #include "ipc/ipc_param_traits.h"
nit: layering violation, cc doesn't depend on IPC

https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_arg...
cc/output/begin_frame_args.h:49: friend struct IPC::ParamTraits<BeginFrameArgs>;
nit: you just want this to be a struct. You have here a value class, that's
copiable and that you pass through IPCs. In chrome we make those structs.

https://codereview.chromium.org/16863005/diff/25001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16863005/diff/25001/cc/output/output_surface....
cc/output/output_surface.cc:108: capabilities_.has_parent_compositor ?
What does has_parent_compositor really mean? Is the adjustment here taking into
account more than 2 levels of compositing (e.g. <webview> tag)?
Is the adjustment something that should be plumbed from the parent compositor?

[brianderson, 2013-06-14 23:32:12]

https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_arg...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_arg...
cc/output/begin_frame_args.cc:46: return
base::TimeDelta::FromMicroseconds(-8888);
On 2013/06/14 22:53:46, piman wrote:
> Can you explain to me what this means, and why it's a hard-coded value that
> applies to all platforms, even the ones that haven't switched to the
BeginFrame
> flow yet?
> 
> Why half a frame? Does it mean that the renderer only has half a frame to
> produce its output from the time it receives the BeginFrame?



I will add a comment for what this is used for.

For platforms that support BeginFrame natively, the BeginFrame message from the
Browser will include a deadline that incorporates the expected draw time of the
Browser.

All other platforms use BeginFrame emulation as of
(https://chromiumcodereview.appspot.com/16833003/) by incorporating the
FrameRateController in the OutputSurface rather than the Scheduler.  In this
case, the deadline has to be set within the OutputSurface without a message from
a parent compositor. Currently, this is set to different values depending on
whether or not there is a parent compositor.

As a first stab, all code paths, whether they support BeginFrame natively or
not, will use this default deadline adjustment. Ali has a patch to actually
estimate the draw time in the threaded compositor that we can hook up when it
lands (https://chromiumcodereview.appspot.com/16574002).

In this patch, I chose half a frame arbitrarily since this doesn't affect
anything yet. It is a function so that we can potentially choose different
values for different platforms. In the worst case, we use some huge negative
number causing us to always be past the deadline, which will effectively revert
us to the current behavior of the scheduler.

An adjustment of half a frame means the renderer will have half a frame to
produce it's output if it wants to make *this* frame. If it does not, it can
still try to make the next frame.

https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_arg...
cc/output/begin_frame_args.h:10: #include "ipc/ipc_param_traits.h"
On 2013/06/14 22:53:46, piman wrote:
> nit: layering violation, cc doesn't depend on IPC

Will fix.

https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_arg...
cc/output/begin_frame_args.h:49: friend struct IPC::ParamTraits<BeginFrameArgs>;
On 2013/06/14 22:53:46, piman wrote:
> nit: you just want this to be a struct. You have here a value class, that's
> copiable and that you pass through IPCs. In chrome we make those structs.

Ok, I'll just make it a struct with public members.

https://codereview.chromium.org/16863005/diff/25001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/16863005/diff/25001/cc/output/output_surface....
cc/output/output_surface.cc:108: capabilities_.has_parent_compositor ?
On 2013/06/14 22:53:46, piman wrote:
> What does has_parent_compositor really mean? Is the adjustment here taking
into
> account more than 2 levels of compositing (e.g. <webview> tag)?
> Is the adjustment something that should be plumbed from the parent compositor?

On platforms that support BeginFrame natively, this code path will not be used
and the adjustment will be included in the BeginFrame itself.

This code path is used for all other platforms to emulate BeginFrame until we
get the BeginFrame message from the parent compositor.

This adjustment does not take into account more than 2 levels of compositing.
This code path will not be used by WebView. Are there other cases where would
need more than 2 levels?

[piman, 2013-06-15 00:14:28]

On Fri, Jun 14, 2013 at 4:32 PM, <brianderson@chromium.org> wrote:

>
> https://codereview.chromium.**org/16863005/diff/25001/cc/**
>
output/begin_frame_args.cc<https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_args.cc>
> File cc/output/begin_frame_args.cc (right):
>
> https://codereview.chromium.**org/16863005/diff/25001/cc/**
>
output/begin_frame_args.cc#**newcode46<https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_args.cc#newcode46>
> cc/output/begin_frame_args.cc:**46: return
> base::TimeDelta::**FromMicroseconds(-8888);
> On 2013/06/14 22:53:46, piman wrote:
>
>> Can you explain to me what this means, and why it's a hard-coded value
>>
> that
>
>> applies to all platforms, even the ones that haven't switched to the
>>
> BeginFrame
>
>> flow yet?
>>
>
>  Why half a frame? Does it mean that the renderer only has half a frame
>>
> to
>
>> produce its output from the time it receives the BeginFrame?
>>
>
>
>
> I will add a comment for what this is used for.
>
> For platforms that support BeginFrame natively, the BeginFrame message
> from the Browser will include a deadline that incorporates the expected
> draw time of the Browser.
>
> All other platforms use BeginFrame emulation as of
>
(https://chromiumcodereview.**appspot.com/16833003/<https://chromiumcodereview...)
> by incorporating the
> FrameRateController in the OutputSurface rather than the Scheduler.  In
> this case, the deadline has to be set within the OutputSurface without a
> message from a parent compositor. Currently, this is set to different
> values depending on whether or not there is a parent compositor.
>
> As a first stab, all code paths, whether they support BeginFrame
> natively or not, will use this default deadline adjustment. Ali has a
> patch to actually estimate the draw time in the threaded compositor that
> we can hook up when it lands
>
(https://chromiumcodereview.**appspot.com/16574002<https://chromiumcodereview....
> ).
>
> In this patch, I chose half a frame arbitrarily since this doesn't
> affect anything yet. It is a function so that we can potentially choose
> different values for different platforms. In the worst case, we use some
> huge negative number causing us to always be past the deadline, which
> will effectively revert us to the current behavior of the scheduler.
>

Shouldn't we start with this then, so that we can implement the mechanism
first, without changing the policy? And then a follow-up CL that sets
reasonably tuned values?
That way you decouple the code refactoring from the policy change, so that
you can keep on iterating on the implementation, and if the policy causes
problems on some platforms we can revert it easily on the branch. Branch
point is close (just about one week away), and I'd like to avoid a panic.


>
> An adjustment of half a frame means the renderer will have half a frame
> to produce it's output if it wants to make *this* frame. If it does not,
> it can still try to make the next frame.


What's the tradeoff here? Won't trying too hard to rush a frame increase
checkerboarding or low-res tiles? How do we tune it?
I think in most workloads that are latency sensitive, the browser
compositor has nothing to do but to blit what the renderer gives it, and we
want to give as much time as possible to the renderer for being able to
produce a good frame.


>
>
> https://codereview.chromium.**org/16863005/diff/25001/cc/**
>
output/begin_frame_args.h<https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_args.h>
> File cc/output/begin_frame_args.h (right):
>
> https://codereview.chromium.**org/16863005/diff/25001/cc/**
>
output/begin_frame_args.h#**newcode10<https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_args.h#newcode10>
> cc/output/begin_frame_args.h:**10: #include "ipc/ipc_param_traits.h"
> On 2013/06/14 22:53:46, piman wrote:
>
>> nit: layering violation, cc doesn't depend on IPC
>>
>
> Will fix.
>
>
> https://codereview.chromium.**org/16863005/diff/25001/cc/**
>
output/begin_frame_args.h#**newcode49<https://codereview.chromium.org/16863005/diff/25001/cc/output/begin_frame_args.h#newcode49>
> cc/output/begin_frame_args.h:**49: friend struct
> IPC::ParamTraits<**BeginFrameArgs>;
> On 2013/06/14 22:53:46, piman wrote:
>
>> nit: you just want this to be a struct. You have here a value class,
>>
> that's
>
>> copiable and that you pass through IPCs. In chrome we make those
>>
> structs.
>
> Ok, I'll just make it a struct with public members.
>
>
> https://codereview.chromium.**org/16863005/diff/25001/cc/**
>
output/output_surface.cc<https://codereview.chromium.org/16863005/diff/25001/cc/output/output_surface.cc>
> File cc/output/output_surface.cc (right):
>
> https://codereview.chromium.**org/16863005/diff/25001/cc/**
>
output/output_surface.cc#**newcode108<https://codereview.chromium.org/16863005/diff/25001/cc/output/output_surface.cc#newcode108>
> cc/output/output_surface.cc:**108: capabilities_.has_parent_**compositor ?
> On 2013/06/14 22:53:46, piman wrote:
>
>> What does has_parent_compositor really mean? Is the adjustment here
>>
> taking into
>
>> account more than 2 levels of compositing (e.g. <webview> tag)?
>> Is the adjustment something that should be plumbed from the parent
>>
> compositor?
>
> On platforms that support BeginFrame natively, this code path will not
> be used and the adjustment will be included in the BeginFrame itself.
>
> This code path is used for all other platforms to emulate BeginFrame
> until we get the BeginFrame message from the parent compositor.
>
> This adjustment does not take into account more than 2 levels of
> compositing. This code path will not be used by WebView. Are there other
> cases where would need more than 2 levels?
>

<webview> tag, aka "browser plugin", precursor to out-of-process iframes
(not Android WebView - sorry for the confusion, I didn't pick the names).
<webview> tag is already shipping, and introduces a third level of
compositing.


>
https://codereview.chromium.**org/16863005/<https://codereview.chromium.org/1...
>

[brianderson, 2013-06-15 00:39:55]

On 2013/06/15 00:14:28, piman wrote:
> On Fri, Jun 14, 2013 at 4:32 PM, <mailto:brianderson@chromium.org> wrote:
> >
> > In this patch, I chose half a frame arbitrarily since this doesn't
> > affect anything yet. It is a function so that we can potentially choose
> > different values for different platforms. In the worst case, we use some
> > huge negative number causing us to always be past the deadline, which
> > will effectively revert us to the current behavior of the scheduler.
> >
> 
> Shouldn't we start with this then, so that we can implement the mechanism
> first, without changing the policy? And then a follow-up CL that sets
> reasonably tuned values?
> That way you decouple the code refactoring from the policy change, so that
> you can keep on iterating on the implementation, and if the policy causes
> problems on some platforms we can revert it easily on the branch. Branch
> point is close (just about one week away), and I'd like to avoid a panic.
> 

That is a good suggestion. I will commit this patch with a large negative number
and test locally for individual platforms.

> 
> >
> > An adjustment of half a frame means the renderer will have half a frame
> > to produce it's output if it wants to make *this* frame. If it does not,
> > it can still try to make the next frame.
> 
> 
> What's the tradeoff here? Won't trying too hard to rush a frame increase
> checkerboarding or low-res tiles? How do we tune it?
> I think in most workloads that are latency sensitive, the browser
> compositor has nothing to do but to blit what the renderer gives it, and we
> want to give as much time as possible to the renderer for being able to
> produce a good frame.
> 

Having a deadline in the future will allow the renderer to draw later rather
than now, which should actually reduce checkerboarding / low-res tiles if we
wait up until the deadline to draw. The trade-off is, if we delay the draw too
long, we risk janking when we don't actually make a deadline as predicted. In
*theory*, we can have that first jank cause us to increase our latency by 1 and
buffer future janks from occurring though.

output/output_surface.cc#**newcode108<https://codereview.chromium.org/16863005/diff/25001/cc/output/output_surface.cc#newcode108>
> > cc/output/output_surface.cc:**108: capabilities_.has_parent_**compositor ?
> > On 2013/06/14 22:53:46, piman wrote:
> >
> >> What does has_parent_compositor really mean? Is the adjustment here
> >>
> > taking into
> >
> >> account more than 2 levels of compositing (e.g. <webview> tag)?
> >> Is the adjustment something that should be plumbed from the parent
> >>
> > compositor?
> >
> > On platforms that support BeginFrame natively, this code path will not
> > be used and the adjustment will be included in the BeginFrame itself.
> >
> > This code path is used for all other platforms to emulate BeginFrame
> > until we get the BeginFrame message from the parent compositor.
> >
> > This adjustment does not take into account more than 2 levels of
> > compositing. This code path will not be used by WebView. Are there other
> > cases where would need more than 2 levels?
> >
> 
> <webview> tag, aka "browser plugin", precursor to out-of-process iframes
> (not Android WebView - sorry for the confusion, I didn't pick the names).
> <webview> tag is already shipping, and introduces a third level of
> compositing.
> 

I see. I need to understand better how <webview> is implemented and will discuss
this offline.

[piman, 2013-06-15 01:04:02]

On Fri, Jun 14, 2013 at 5:39 PM, <brianderson@chromium.org> wrote:

> On 2013/06/15 00:14:28, piman wrote:
>
>  On Fri, Jun 14, 2013 at 4:32 PM,
<mailto:brianderson@chromium.**org<brianderson@chromium.org>>
>> wrote:
>> >
>> > In this patch, I chose half a frame arbitrarily since this doesn't
>> > affect anything yet. It is a function so that we can potentially choose
>> > different values for different platforms. In the worst case, we use some
>> > huge negative number causing us to always be past the deadline, which
>> > will effectively revert us to the current behavior of the scheduler.
>> >
>>
>
>  Shouldn't we start with this then, so that we can implement the mechanism
>> first, without changing the policy? And then a follow-up CL that sets
>> reasonably tuned values?
>> That way you decouple the code refactoring from the policy change, so that
>> you can keep on iterating on the implementation, and if the policy causes
>> problems on some platforms we can revert it easily on the branch. Branch
>> point is close (just about one week away), and I'd like to avoid a panic.
>>
>
>
> That is a good suggestion. I will commit this patch with a large negative
> number
> and test locally for individual platforms.


Ok, that sounds good.

>
>
>
>  >
>> > An adjustment of half a frame means the renderer will have half a frame
>> > to produce it's output if it wants to make *this* frame. If it does not,
>> > it can still try to make the next frame.
>>
>
>
>  What's the tradeoff here? Won't trying too hard to rush a frame increase
>> checkerboarding or low-res tiles? How do we tune it?
>> I think in most workloads that are latency sensitive, the browser
>> compositor has nothing to do but to blit what the renderer gives it, and
>> we
>> want to give as much time as possible to the renderer for being able to
>> produce a good frame.
>>
>
>
> Having a deadline in the future will allow the renderer to draw later
> rather
> than now, which should actually reduce checkerboarding / low-res tiles if
> we
> wait up until the deadline to draw. The trade-off is, if we delay the draw
> too
> long, we risk janking when we don't actually make a deadline as predicted.
> In
> *theory*, we can have that first jank cause us to increase our latency by
> 1 and
> buffer future janks from occurring though.
>

Right, my worry is that having a single hard-coded value here, compared to
the cross product of platforms (and devices) times the number of sites,
will mean everyone will want to pull this number in every direction, and
we'll end up at best with something like the bunch of ifdefs
in GpuMemoryManager.
What is going to be the process for tuning this? How will we input and
manage the various values? Who will do the work to find the good ones on
all relevant platforms?


>
> output/output_surface.cc#****newcode108<https://codereview.**
> chromium.org/16863005/diff/**25001/cc/output/output_**
>
surface.cc#newcode108<https://codereview.chromium.org/16863005/diff/25001/cc/output/output_surface.cc#newcode108>
> >
>
>> > cc/output/output_surface.cc:****108:
capabilities_.has_parent_****compositor
>> ?
>>
>> > On 2013/06/14 22:53:46, piman wrote:
>> >
>> >> What does has_parent_compositor really mean? Is the adjustment here
>> >>
>> > taking into
>> >
>> >> account more than 2 levels of compositing (e.g. <webview> tag)?
>> >> Is the adjustment something that should be plumbed from the parent
>> >>
>> > compositor?
>> >
>> > On platforms that support BeginFrame natively, this code path will not
>> > be used and the adjustment will be included in the BeginFrame itself.
>> >
>> > This code path is used for all other platforms to emulate BeginFrame
>> > until we get the BeginFrame message from the parent compositor.
>> >
>> > This adjustment does not take into account more than 2 levels of
>> > compositing. This code path will not be used by WebView. Are there other
>> > cases where would need more than 2 levels?
>> >
>>
>
>  <webview> tag, aka "browser plugin", precursor to out-of-process iframes
>> (not Android WebView - sorry for the confusion, I didn't pick the names).
>> <webview> tag is already shipping, and introduces a third level of
>> compositing.
>>
>
>
> I see. I need to understand better how <webview> is implemented and will
> discuss
> this offline.
>

alexst@ is a good person to talk to.
In a nutshell, the "guest" renderer works pretty much like an Aura
renderer, swaps to the browser, which forwards the frame to the "embedder"
renderer, which puts the frame into a TextureLayer (or a
DelegatedRendererLayer for ÜC). See RenderWidgetHostViewGuest which is the
RWHV for the guest - receives swap/post messages and forwards them to the
embedder. BrowserPluginCompositingHelper and interacts with the embedder
(parent) compositor.


>
>
>
>
>
https://codereview.chromium.**org/16863005/<https://codereview.chromium.org/1...
>

[brianderson, 2013-06-18 19:05:55]

Note: I am moving this review to https://codereview.chromium.org/17391006
because I keep getting the "base files missing error" due to
https://code.google.com/p/rietveld/issues/detail?id=357.

Regarding potential explosion of #ifdefs for the deadline adjustment:
The plan is to avoid hard-coded values as much as possible with Ali's estimation
patch. It'll be a necessary crutch in the short-term however.

For <webview>, I will be working with alexst to figure out how to proceed. In
the short term, we can keep the deadline adjustment large to avoid changing the
scheduling behavior until <webview> gets BeginFrames natively.