Reland "Add new latency_info for tracking browser composite time."

Reland "Add new latency_info for tracking browser composite time."

This change originally landed in crrev.com/577273003, but was reverted
in crrev.com/750093009 due to poor scrolling performance during
browser-composited animations. The issue has since been resolved, so
reland the change with some minor refactoring.

BUG=437603, 437932
Committed: https://crrev.com/83083a322a41d8de740442530d4ba9a7bb7cf0cc
Cr-Commit-Position: refs/heads/master@{#310193}

[jdduke (slow), 2015-01-05 23:07:12]

brianderson@: PTAL. There was one small revert conflict (due to LatencyInfo
sizing), easily resolved.

[brianderson, 2015-01-05 23:19:56]

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

[brianderson, 2015-01-05 23:19:56]

lgtm
+sievers

[brianderson, 2015-01-05 23:20:55]

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

[brianderson, 2015-01-05 23:20:55]

+miletus too

[jdduke (slow), 2015-01-05 23:24:53]

Changed the for loops to range for (because fun), and changed the TODO to
mention brianderson instead of orglofch.

[Yufeng Shen (Slow to review), 2015-01-06 00:31:28]

On 2015/01/05 23:24:53, jdduke wrote:
> Changed the for loops to range for (because fun), and changed the TODO to
> mention brianderson instead of orglofch.

LatencyInfo lgtm.

[Sami, 2015-01-06 12:21:39]

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

[Sami, 2015-01-06 12:21:40]

lgtm

https://codereview.chromium.org/833283003/diff/40001/cc/base/rolling_time_del...
File cc/base/rolling_time_delta_history.h (right):

https://codereview.chromium.org/833283003/diff/40001/cc/base/rolling_time_del...
cc/base/rolling_time_delta_history.h:26: size_t SampleCount();
drive-by nit: const

[jdduke (slow), 2015-01-06 16:01:43]

https://codereview.chromium.org/833283003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/833283003/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:86:
ui::INPUT_EVENT_BROWSER_SWAP_BUFFER_COMPONENT, 0, 0);
Hmm, I actually don't see this component used anywhere. The deadline estimation
only uses BROWSER_COMPOSITE_COMPONENT and GPU_SWAP_BUFFER_COMPONENT. Can we get
rid of this?

[jdduke (slow), 2015-01-06 20:09:18]

I got rid of INPUT_EVENT_BROWSER_SWAP_BUFFER_COMPONENT because it's unused, and
moved the additional latency-related logic from RWHImpl to RWHLatencyTracker.

sievers@: Anything else you want to see here?

[no sievers, 2015-01-06 22:28:46]

lgtm

https://codereview.chromium.org/833283003/diff/60001/cc/base/rolling_time_del...
File cc/base/rolling_time_delta_history.h (right):

https://codereview.chromium.org/833283003/diff/60001/cc/base/rolling_time_del...
cc/base/rolling_time_delta_history.h:26: size_t SampleCount();
nit: const

or actually... this looks unused.

https://codereview.chromium.org/833283003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/833283003/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:268:
latency.AddLatencyNumber(ui::INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, 0, 0);
We didn't actually start compositing yet. We only received a new frame.

What would happen if the browser scheduler decided to composite a little later
(for whatever reason), since that then causes the render compositor to draw
earlier (because it thinks that compositing took longer although really it
didn't - we just delayed it)? It seems like it might do the opposite of what we
want in that case.

[jdduke (slow), 2015-01-06 23:13:14]

https://codereview.chromium.org/833283003/diff/60001/cc/base/rolling_time_del...
File cc/base/rolling_time_delta_history.h (right):

https://codereview.chromium.org/833283003/diff/60001/cc/base/rolling_time_del...
cc/base/rolling_time_delta_history.h:26: size_t SampleCount();
On 2015/01/06 22:28:46, sievers wrote:
> nit: const
> 
> or actually... this looks unused.

Yup, totally right, removed.

https://codereview.chromium.org/833283003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/833283003/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:268:
latency.AddLatencyNumber(ui::INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, 0, 0);
On 2015/01/06 22:28:46, sievers wrote:
> We didn't actually start compositing yet. We only received a new frame.
> 
> What would happen if the browser scheduler decided to composite a little later
> (for whatever reason), since that then causes the render compositor to draw
> earlier (because it thinks that compositing took longer although really it
> didn't - we just delayed it)? It seems like it might do the opposite of what
we
> want in that case.

That's a good point. Brian, is that why he originally had a
ui::INPUT_EVENT_BROWSER_SWAP_BUFFER_COMPONENT? It wasn't used anywhere, but I
wonder if it would help inform the estimate?

[brianderson, 2015-01-06 23:35:13]

https://codereview.chromium.org/833283003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/833283003/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:268:
latency.AddLatencyNumber(ui::INPUT_EVENT_BROWSER_COMPOSITE_COMPONENT, 0, 0);
On 2015/01/06 23:13:14, jdduke wrote:
> On 2015/01/06 22:28:46, sievers wrote:
> > We didn't actually start compositing yet. We only received a new frame.
> > 
> > What would happen if the browser scheduler decided to composite a little
later
> > (for whatever reason), since that then causes the render compositor to draw
> > earlier (because it thinks that compositing took longer although really it
> > didn't - we just delayed it)? It seems like it might do the opposite of what
> we
> > want in that case.
> 
> That's a good point. Brian, is that why he originally had a
> ui::INPUT_EVENT_BROWSER_SWAP_BUFFER_COMPONENT? It wasn't used anywhere, but I
> wonder if it would help inform the estimate?

Summary: I think we should keep this patch as is and rename the component so it
makes sense.

I remember that we would sometimes see tasks get scheduled between
OnSwapCompositorFrame and the Browser composite time, in which case this gives a
more realistic estimate.

If the Browser is intentionally delaying draw, then it would be less accurate as
Daniel points out, but I think it's a case we can optimize for later. The
Browser would only delay draw during self updates after the Renderer has missed
a deadline and is in a high latency mode.

Let's optimize for accuracy of low latency mode for now and improve high latency
mode later. High latency mode already sucks and needs a number of other
improvements anyway.

[jdduke (slow), 2015-01-06 23:46:28]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-01-06 23:47:15]

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

[commit-bot: I haz the power, 2015-01-07 00:42:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-07 00:43:53]

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