Add SlidingAverage to FPSMeter, use for WebVR prediction time

Add SlidingAverage to FPSMeter, use for WebVR prediction time

Refactor FPSMeter to extract the "sliding window sum" code
into a separate class, use it in FPSMeter, and separately
use it to implement a SlidingAverage class.

This revealed a pre-existing bug where current_index_ was
not being initialized, leading to wrong FPS calculations.

Use SlidingAverage to calculate WebVR Javascript and rendering
times, and use those to calculate the prediction time offset
for GVR poses instead of hardcoding 50ms for this.

BUG=723962
Review-Url: https://codereview.chromium.org/2901213002
Cr-Commit-Position: refs/heads/master@{#474549}
Committed: https://chromium.googlesource.com/chromium/src/+/0d1a6048d093aeefb6eec350fcef6352326f5029

[klausw, 2017-05-24 00:32:01]

klausw@chromium.org changed reviewers:
+ bajones@chromium.org, cjgrant@chromium.org

[klausw, 2017-05-24 00:36:51]

The CQ bit was checked by klausw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 00:38:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 00:44:46]

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

[commit-bot: I haz the power, 2017-05-24 00:44:47]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[klausw, 2017-05-24 05:04:56]

The CQ bit was checked by klausw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 05:05:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 07:43:55]

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

[commit-bot: I haz the power, 2017-05-24 07:43:56]

Dry run: This issue passed the CQ dry run.

[cjgrant, 2017-05-24 13:37:30]

Klaus, a general question as well: For this application, is the sample history
actually necessary?  What about just maintaining an average value, and updating
it each frame as average = latest_interval * a + (average * (1 - a)), where you
tune 'a' based on how smooth you want things?

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/fps_meter.cc (right):

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/fps_meter.cc:21: SampleQueue::~SampleQueue() {}
does "= default;" work here?

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/fps_meter.cc:66: double FPSMeter::GetFPS() const
{
nit (for future reference): go/style prefers FpsMeter::GetFps()
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/fps_meter.cc:82: if (values_.GetCount() == 0) {
if (values.GetCount() == 0)
  return default_value;
return values_.GetSum()...

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/fps_meter.h (right):

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/fps_meter.h:40: explicit FPSMeter(size_t
window_size);
Just a sanity check - are there no existing tools that give you this for free? 
Eg:
https://cs.chromium.org/chromium/src/cc/trees/frame_rate_counter.cc

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1533: int64_t frame_time_micros =
vsync_interval_.InMicroseconds();
How do you feel about dropping this block into
VrShellGl::GetPredictedFrameTime() or similar?

[klausw, 2017-05-24 16:32:23]

> Klaus, a general question as well: For this application, is the sample history
actually necessary?  What about just maintaining an average value, and updating
it each frame as average = latest_interval * a + (average * (1 - a)), where you
tune 'a' based on how smooth you want things?

Hah. That's actually how I had implemented it in my experimental build, but I
was worried that this decaying average can get skewed by outliers for quite a
long time. I've occasionally seen frame times in the hundreds of milliseconds
for single frames, and with a = 0.2 this would still contribute ~10% after 10
frames and 1% after 100 frames. I thought I may as well use the moving window
average since the fps counter already implements it.

I'm also ok with switching back to the simple decaying average, though in this
case I'd still prefer to keep the SampleQueue refactor + uninitialized member
bugfix from this change.

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/fps_meter.cc (right):

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/fps_meter.cc:21: SampleQueue::~SampleQueue() {}
On 2017/05/24 13:37:30, cjgrant wrote:
> does "= default;" work here?

Done.

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/fps_meter.cc:66: double FPSMeter::GetFPS() const
{
On 2017/05/24 13:37:30, cjgrant wrote:
> nit (for future reference): go/style prefers FpsMeter::GetFps()
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Acknowledged, I wasn't the author of this part.

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/fps_meter.cc:82: if (values_.GetCount() == 0) {
On 2017/05/24 13:37:30, cjgrant wrote:
> if (values.GetCount() == 0)
>   return default_value;
> return values_.GetSum()...

Done.

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/fps_meter.h (right):

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/fps_meter.h:40: explicit FPSMeter(size_t
window_size);
On 2017/05/24 13:37:30, cjgrant wrote:
> Just a sanity check - are there no existing tools that give you this for free?

> Eg:
> https://cs.chromium.org/chromium/src/cc/trees/frame_rate_counter.cc

Can you check with vollick@? He implemented this FPS counter. Not sure if it's a
dependency constraint.

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2901213002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1533: int64_t frame_time_micros =
vsync_interval_.InMicroseconds();
On 2017/05/24 13:37:30, cjgrant wrote:
> How do you feel about dropping this block into
> VrShellGl::GetPredictedFrameTime() or similar?

Done, I've also added TRACE_COUNTER output so that these (+ FPS) get graphed in
event traces.

[klausw, 2017-05-24 17:03:28]

The CQ bit was checked by klausw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 17:04:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cjgrant, 2017-05-24 17:17:14]

lgtm from a VrShell/code perspective.

[bajones, 2017-05-24 17:21:44]

LGTM. Definitely be nice to get away from a static prediction value!

[klausw, 2017-05-24 17:28:57]

The CQ bit was unchecked by klausw@chromium.org

[klausw, 2017-05-24 17:29:25]

The CQ bit was checked by klausw@chromium.org

[commit-bot: I haz the power, 2017-05-24 17:30:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 19:11:39]

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

[commit-bot: I haz the power, 2017-05-24 19:11:39]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[klausw, 2017-05-24 19:24:32]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-05-24 19:24:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2901213002/#ps80001
(title: "Fix "Complex destructor has an inline body" for = default.")

[commit-bot: I haz the power, 2017-05-24 19:25:35]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 22:39:38]

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

[commit-bot: I haz the power, 2017-05-24 22:39:39]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[klausw, 2017-05-24 22:43:11]

The CQ bit was checked by klausw@chromium.org

[commit-bot: I haz the power, 2017-05-24 22:44:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 02:37:58]

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

[commit-bot: I haz the power, 2017-05-25 02:38:00]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[klausw, 2017-05-25 02:47:46]

The CQ bit was checked by klausw@chromium.org

[commit-bot: I haz the power, 2017-05-25 02:49:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 04:33:05]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495680466966290,
"parent_rev": "2094591b5e7cd1fc31f288817608e218ce149947", "commit_rev":
"0d1a6048d093aeefb6eec350fcef6352326f5029"}

[commit-bot: I haz the power, 2017-05-25 04:33:27]

Description was changed from

==========
Add SlidingAverage to FPSMeter, use for WebVR prediction time

Refactor FPSMeter to extract the "sliding window sum" code
into a separate class, use it in FPSMeter, and separately
use it to implement a SlidingAverage class.

This revealed a pre-existing bug where current_index_ was
not being initialized, leading to wrong FPS calculations.

Use SlidingAverage to calculate WebVR Javascript and rendering
times, and use those to calculate the prediction time offset
for GVR poses instead of hardcoding 50ms for this.

BUG=723962
==========

to

==========
Add SlidingAverage to FPSMeter, use for WebVR prediction time

Refactor FPSMeter to extract the "sliding window sum" code
into a separate class, use it in FPSMeter, and separately
use it to implement a SlidingAverage class.

This revealed a pre-existing bug where current_index_ was
not being initialized, leading to wrong FPS calculations.

Use SlidingAverage to calculate WebVR Javascript and rendering
times, and use those to calculate the prediction time offset
for GVR poses instead of hardcoding 50ms for this.

BUG=723962

Review-Url: https://codereview.chromium.org/2901213002
Cr-Commit-Position: refs/heads/master@{#474549}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d1a6048d093aeefb6eec350fcef...
==========

[commit-bot: I haz the power, 2017-05-25 04:33:28]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0d1a6048d093aeefb6eec350fcef...