telemetry: Remove gaps between interactions when calculating smoothness metrics.

telemetry: Remove gaps between interactions when calculating smoothness metrics.

BUG=477757
Committed: https://crrev.com/260051d32fa1d3218a8e8a9ce87cc81d16b0b4ff
Cr-Commit-Position: refs/heads/master@{#325583}

[vmiura, 2015-04-16 21:32:38]

vmiura@chromium.org changed reviewers:
+ nednguyen@google.com

[vmiura, 2015-04-16 21:32:39]

PTAL

[nednguyen, 2015-04-16 22:06:57]

Can you add some tests that would catch the failure without this patch?

[vmiura, 2015-04-16 22:30:52]

On 2015/04/16 22:06:57, nednguyen wrote:
> Can you add some tests that would catch the failure without this patch?

Do we have similar tests I could base on?

smoothness_unittest.py seems to check only for the existence of the metrics,
with a real browser.

[nednguyen, 2015-04-16 22:34:47]

On 2015/04/16 22:30:52, vmiura wrote:
> On 2015/04/16 22:06:57, nednguyen wrote:
> > Can you add some tests that would catch the failure without this patch?
> 
> Do we have similar tests I could base on?
> 
> smoothness_unittest.py seems to check only for the existence of the metrics,
> with a real browser.

SmoothnessMetricUnitTest in smoothness_unittest.py does have tests that assert
the exact metric values.

[vmiura, 2015-04-16 23:11:19]

On 2015/04/16 22:34:47, nednguyen wrote:
> On 2015/04/16 22:30:52, vmiura wrote:
> > On 2015/04/16 22:06:57, nednguyen wrote:
> > > Can you add some tests that would catch the failure without this patch?
> > 
> > Do we have similar tests I could base on?
> > 
> > smoothness_unittest.py seems to check only for the existence of the metrics,
> > with a real browser.
> 
> SmoothnessMetricUnitTest in smoothness_unittest.py does have tests that assert
> the exact metric values.

Thanks.  PTAL.

[nednguyen, 2015-04-17 01:39:57]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-04-17 01:39:57]

lgtm

[commit-bot: I haz the power, 2015-04-17 01:40:34]

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

[commit-bot: I haz the power, 2015-04-17 02:00:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-17 02:01:24]

Patchset 2 (id:??) landed as
https://crrev.com/260051d32fa1d3218a8e8a9ce87cc81d16b0b4ff
Cr-Commit-Position: refs/heads/master@{#325583}

[mithro-old, 2015-04-21 03:45:58]

mithro@mithis.com changed reviewers:
+ mithro@mithis.com

[mithro-old, 2015-04-21 03:45:59]

Hi vmiura,

I know this patch has been submitted but I had two questions about the code
changes after your "PSA: Changes in smoothness metrics" email.

I think either the code should be using stats.frame_times or the unittest change
doesn't need them? What am I missing?

Tim 'mithro' Ansell

https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right):

https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:111: if
self._HasEnoughFrames(stats.frame_timestamps):
Why does this metric use stats.frame_timestamps rather than stats.frame_times?
Looking at your unittest the stats.frame_times contains the already calculated
value you need?

https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py (right):

https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:63:
frame_times=[[10], [10, 10]])
I can't see anything in _ComputeSurfaceFlingerMetric which stats.uses
frame_times at the moment, what am I missing?

[vmiura, 2015-04-21 22:04:46]

https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right):

https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:111: if
self._HasEnoughFrames(stats.frame_timestamps):
On 2015/04/21 03:45:58, mithro wrote:
> Why does this metric use stats.frame_timestamps rather than stats.frame_times?
> Looking at your unittest the stats.frame_times contains the already calculated
> value you need?

I think you're right, it looks like these metrics could be computed from
frame_times.

https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py (right):

https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:63:
frame_times=[[10], [10, 10]])
On 2015/04/21 03:45:58, mithro wrote:
> I can't see anything in _ComputeSurfaceFlingerMetric which stats.uses
> frame_times at the moment, what am I missing?

Nothing uses them.  This change is just in the interest of making frame_times
consistent with the changes to frame_timestamps.  It seems worthwhile to remove
frame_timestamps from RenderingStats.

[nednguyen, 2015-04-21 22:11:16]

On 2015/04/21 22:04:46, vmiura wrote:
>
https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right):
> 
>
https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
> tools/telemetry/telemetry/web_perf/metrics/smoothness.py:111: if
> self._HasEnoughFrames(stats.frame_timestamps):
> On 2015/04/21 03:45:58, mithro wrote:
> > Why does this metric use stats.frame_timestamps rather than
stats.frame_times?
> > Looking at your unittest the stats.frame_times contains the already
calculated
> > value you need?
> 
> I think you're right, it looks like these metrics could be computed from
> frame_times.
> 
>
https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py
(right):
> 
>
https://codereview.chromium.org/1092803002/diff/20001/tools/telemetry/telemet...
> tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:63:
> frame_times=[[10], [10, 10]])
> On 2015/04/21 03:45:58, mithro wrote:
> > I can't see anything in _ComputeSurfaceFlingerMetric which stats.uses
> > frame_times at the moment, what am I missing?
> 
> Nothing uses them.  This change is just in the interest of making frame_times
> consistent with the changes to frame_timestamps.  It seems worthwhile to
remove
> frame_timestamps from RenderingStats.

+1. That would be a good simplification of the smoothness measurement.