SurfaceStats / Telemetry: calculate FPS in different frame buckets.

SurfaceStats / Telemetry: calculate FPS in different frame buckets.

Instead of just calculating FPS over all frames returned by SurfaceFlinger,
use the last 90% and the last 50% as well.
This allow us to have a better representation and skip the "setup" stage.

BUG=256340
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210583

[Sami, 2013-07-03 15:11:50]

Thanks, looks like this gives a little more realistic numbers. Would 99% be a
too high threshold for the first bucket?

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
File build/android/pylib/surface_stats_collector.py (right):

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
build/android/pylib/surface_stats_collector.py:90: sliced = timestamps[int(-pct
* len(timestamps)) + 3 : ]
Probably want to bail out here if there were too few samples.

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
build/android/pylib/surface_stats_collector.py:94: 'avg_surface_fps_' +
str(pct).replace('.', '_'),
int(pct * 100) would gives nicer names I think :)

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
build/android/pylib/surface_stats_collector.py:134:
self._CalcFpsInBuckets(timestamps)
If we decide to go ahead with this, let's include jank_count, max_frame_delay
and frame_lengths too.

https://codereview.chromium.org/18341006/diff/2001/build/android/surface_stat...
File build/android/surface_stats.py (right):

https://codereview.chromium.org/18341006/diff/2001/build/android/surface_stat...
build/android/surface_stats.py:26: 'avg_surface_fps_0.9 (fps)': '%.2f',
I think we should just hide the windowed results in this tool, because they
don't really make any sense with continuous sampling.

[bulach, 2013-07-03 18:09:15]

thanks sami! questions inline:

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
File build/android/pylib/surface_stats_collector.py (right):

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
build/android/pylib/surface_stats_collector.py:90: sliced = timestamps[int(-pct
* len(timestamps)) + 3 : ]
On 2013/07/03 15:11:50, Sami wrote:
> Probably want to bail out here if there were too few samples.

when is the "legacy" method used? the non-legacy guarantees there'll be at least
three samples.. I can check it in here too, or...remove the legacy method ? :)

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
build/android/pylib/surface_stats_collector.py:94: 'avg_surface_fps_' +
str(pct).replace('.', '_'),
On 2013/07/03 15:11:50, Sami wrote:
> int(pct * 100) would gives nicer names I think :)

indeed! done.

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
build/android/pylib/surface_stats_collector.py:134:
self._CalcFpsInBuckets(timestamps)
On 2013/07/03 15:11:50, Sami wrote:
> If we decide to go ahead with this, let's include jank_count, max_frame_delay
> and frame_lengths too.

I need to understand this better though: they all depend on "refresh_period",
which seems to be for the entire set?
that is, jank_count depends on normalized_changes which depends on this
"refresh_period"... can we derive "refresh_period" from the trimmed timestamps
instead, i.e., "seconds"?

[Sami, 2013-07-03 18:21:37]

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
File build/android/pylib/surface_stats_collector.py (right):

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
build/android/pylib/surface_stats_collector.py:90: sliced = timestamps[int(-pct
* len(timestamps)) + 3 : ]
On 2013/07/03 18:09:15, bulach wrote:
> On 2013/07/03 15:11:50, Sami wrote:
> > Probably want to bail out here if there were too few samples.
> 
> when is the "legacy" method used? the non-legacy guarantees there'll be at
least
> three samples.. I can check it in here too, or...remove the legacy method ? :)

Ah, good point. The legacy path is only used on pre-JB devices. I think we want
to keep that around since devices like S3 are still on ICS. I don't think we
need to implement windowing there, though.

https://codereview.chromium.org/18341006/diff/2001/build/android/pylib/surfac...
build/android/pylib/surface_stats_collector.py:134:
self._CalcFpsInBuckets(timestamps)
On 2013/07/03 18:09:15, bulach wrote:
> I need to understand this better though: they all depend on "refresh_period",
> which seems to be for the entire set?
> that is, jank_count depends on normalized_changes which depends on this
> "refresh_period"... can we derive "refresh_period" from the trimmed timestamps
> instead, i.e., "seconds"?

|refresh_period| is a constant given by the display panel and is used to
normalize/threshold the data in |timestamps|. That is, it's used to derive these
metrics and not the other way around.

One way this could work is to move the loop here:

  for pct in [0.5, 0.9]:
    trimmed_timestamps = timestamps[int(-pct * len(timestamps)) + 3 : ]
    seconds = trimmed_timestamps[-1] - trimmed_timestamps[0]
    frame_count = len(trimmed_timestamps)
    self._results.append(SurfaceStatsCollector.Result(
        'avg_surface_fps' + suffix, int(round(frame_count / seconds)), 'fps'))
    frame_lengths, normalized_frame_lengths = \
        self._GetNormalizedDeltas(trimmed_timestamps, refresh_period)
    ...
    self._results.append(SurfaceStatsCollector.Result(
          'frame_lengths' + suffix, normalized_frame_lengths, 'vsyncs'))
    [etc.]

[bulach, 2013-07-04 10:12:31]

thanks for the clarifications! added all the measurements for all buckets, ptal.

[Sami, 2013-07-04 10:26:51]

Great, lgtm!

[bulach, 2013-07-04 12:34:00]

+marja for OWNERS in tools/perf/measurements/smoothness.py

thanks!

[bulach, 2013-07-08 17:37:04]

+tonyg for OWNERS in tools/perf/measurements/smoothness.py

[tonyg, 2013-07-09 02:50:13]

lgtm

[commit-bot: I haz the power, 2013-07-09 08:07:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/18341006/12001

[commit-bot: I haz the power, 2013-07-09 08:12:02]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-07-09 10:34:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/18341006/12001

[commit-bot: I haz the power, 2013-07-09 17:04:17]

Change committed as 210583

[nduca, 2013-07-09 17:17:16]

this patch should not have been lgtmd without reconciling the changes made
against mean_frame_time as well.

this is like defining page load time differently on android vs the rest of
chrome. :(

[bulach, 2013-07-09 17:37:57]

sorry nat, feel free to revert if you strongly disagree.

however, please let me give you a few £0.02:
- firstly, this _is_ already android-specific, fwiw.. the SurfaceFlinger is the
only way to measure the "jank"... comparing with measuring page load time is an
exaggeration imho..

- secondly, this did not _change_ existing metrics. on the contrary, I was
reluctant to do that, and instead, _added_ more metrics...

- finally: I agree we could improve the mean_frame_time too, but there's nothing
in this patch that would prevent (or help) that...

wdyt?