[vr] Add a frame rate counter for the vr UI

[vr] Add a frame rate counter for the vr UI

With this CL, we only gather the info needed for an
FPS meter. It does not currently output anything (this
is future work.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2804943003
Cr-Commit-Position: refs/heads/master@{#463931}
Committed: https://chromium.googlesource.com/chromium/src/+/47619f8e9971f86a0478b32190a004b9a53141c1

[Ian Vollick, 2017-04-06 15:26:31]

Description was changed from

==========
[vr] WIP - Use cc's frame rate counter for the vr UI

I'm not convinced that this counter is appropriate for our usecase, but
it does give us a reliable framerate. I will ask brianderson.

BUG=
==========

to

==========
[vr] WIP - Use cc's frame rate counter for the vr UI

I'm not convinced that this counter is appropriate for our usecase, but
it does give us a reliable framerate. I will ask brianderson.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Ian Vollick, 2017-04-06 20:57:02]

Description was changed from

==========
[vr] WIP - Use cc's frame rate counter for the vr UI

I'm not convinced that this counter is appropriate for our usecase, but
it does give us a reliable framerate. I will ask brianderson.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[vr] Use cc's frame rate counter for the vr UI

With this CL, we only gather the info needed for an
FPS meter. It does not currently output anything (this
is future work.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Ian Vollick, 2017-04-06 20:57:03]

vollick@chromium.org changed reviewers:
+ brianderson@chromium.org, mthiesse@chromium.org

[Ian Vollick, 2017-04-06 20:58:06]

On 2017/04/06 20:57:03, Ian Vollick wrote:
> mailto:vollick@chromium.org changed reviewers:
> + mailto:brianderson@chromium.org, mailto:mthiesse@chromium.org

My impression, after chatting with Brian offline, was that this wasn't crazy.

wdyt of this approach, folks?

[Ian Vollick, 2017-04-07 01:53:30]

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

[commit-bot: I haz the power, 2017-04-07 01:53:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ian Vollick, 2017-04-07 02:03:08]

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

[commit-bot: I haz the power, 2017-04-07 02:03:28]

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-04-07 03:04:43]

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

[commit-bot: I haz the power, 2017-04-07 03:04:44]

Dry run: This issue passed the CQ dry run.

[mthiesse, 2017-04-07 15:36:33]

This counter assumes that our framerate is exactly 60fps, which it approximately
is at the moment (though who knows, it could be 59.94 or something). However, in
the future we'll be supporting 90+ fps displays, so we should probably add a
parameter for the framerate to the counter constructor.

Unrelated to this CL:
brianderson, why is this counter always on for every LayerTreeHost? I'm sure
it's not a lot of overhead, but there's some non-zero memory and cpu cost, and
it looks like it would be fairly easy to only create it when the HUD comes up?

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

https://codereview.chromium.org/2804943003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:851:
frame_rate_counter_->SaveTimeStamp(current_time,
I know this is fairly lightweight, but we should probably only be generating
this data when using it, so putting it behind a debug flag for now might be
reasonable?

https://codereview.chromium.org/2804943003/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:852:
unused_is_a_software_compositor);
nit: Definitely personal preference, but I think this is overly verbose.
I would have written 
frame_rate_counter_->SaveTimeStamp(current_time, false /* unused */);
but feel free to ignore this.

[Ian Vollick, 2017-04-07 15:44:12]

On 2017/04/07 15:36:33, mthiesse wrote:
> This counter assumes that our framerate is exactly 60fps, which it
approximately
> is at the moment (though who knows, it could be 59.94 or something). However,
in
> the future we'll be supporting 90+ fps displays, so we should probably add a
> parameter for the framerate to the counter constructor.

Good point. The logic for invalid frames in cc's framecounter doesn't seem
applicable in our use case. I think I'll write up a targeted vr meter instead.

Posting this now before I start on the patchset in case either of you have
objections.
> 
> Unrelated to this CL:
> brianderson, why is this counter always on for every LayerTreeHost? I'm sure
> it's not a lot of overhead, but there's some non-zero memory and cpu cost, and
> it looks like it would be fairly easy to only create it when the HUD comes up?
> 
>
https://codereview.chromium.org/2804943003/diff/80001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
> 
>
https://codereview.chromium.org/2804943003/diff/80001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_gl.cc:851:
> frame_rate_counter_->SaveTimeStamp(current_time,
> I know this is fairly lightweight, but we should probably only be generating
> this data when using it, so putting it behind a debug flag for now might be
> reasonable?
> 
>
https://codereview.chromium.org/2804943003/diff/80001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_gl.cc:852:
> unused_is_a_software_compositor);
> nit: Definitely personal preference, but I think this is overly verbose.
> I would have written 
> frame_rate_counter_->SaveTimeStamp(current_time, false /* unused */);
> but feel free to ignore this.

[Ian Vollick, 2017-04-08 15:55:10]

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

[commit-bot: I haz the power, 2017-04-08 15:55:18]

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-04-08 16:43:48]

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

[commit-bot: I haz the power, 2017-04-08 16:43:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ian Vollick, 2017-04-10 13:23:57]

vollick@chromium.org changed reviewers:
- brianderson@chromium.org

[Ian Vollick, 2017-04-10 13:25:26]

On 2017/04/07 15:44:12, Ian Vollick wrote:
> On 2017/04/07 15:36:33, mthiesse wrote:
> > This counter assumes that our framerate is exactly 60fps, which it
> approximately
> > is at the moment (though who knows, it could be 59.94 or something).
However,
> in
> > the future we'll be supporting 90+ fps displays, so we should probably add a
> > parameter for the framerate to the counter constructor.
> 
> Good point. The logic for invalid frames in cc's framecounter doesn't seem
> applicable in our use case. I think I'll write up a targeted vr meter instead.
> 
> Posting this now before I start on the patchset in case either of you have
> objections.

New meter does not make framerate assumptions.

> >
>
https://codereview.chromium.org/2804943003/diff/80001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/vr_shell_gl.cc:851:
> > frame_rate_counter_->SaveTimeStamp(current_time,
> > I know this is fairly lightweight, but we should probably only be generating
> > this data when using it, so putting it behind a debug flag for now might be
> > reasonable?

Sg. Done.
> > 
> >
>
https://codereview.chromium.org/2804943003/diff/80001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/vr_shell_gl.cc:852:
> > unused_is_a_software_compositor);
> > nit: Definitely personal preference, but I think this is overly verbose.
> > I would have written 
> > frame_rate_counter_->SaveTimeStamp(current_time, false /* unused */);
> > but feel free to ignore this.

N/A now that I've switched to a different meter.

[mthiesse, 2017-04-11 19:14:36]

lgtm

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

https://codereview.chromium.org/2804943003/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/fps_meter.cc:52: for (const auto& delta :
frame_times_)
Instead of recomputing the sum each time you read, why not update the sum as you
go?

eg.
sum -= frame_times_[current_index_].InSecondsF();
sum += delta.InSecondsF();
frame_times_[current_index_] = delta;

[mthiesse, 2017-04-11 19:17:13]

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

https://codereview.chromium.org/2804943003/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:151: #ifndef NDEBUG
Also, consider using DCHECK_ALWAYS_ON? Might be useful to be able to enable this
for release builds.

[billorr, 2017-04-12 00:40:02]

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

[billorr, 2017-04-12 00:40:03]

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

https://codereview.chromium.org/2804943003/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/fps_meter.cc:52: for (const auto& delta :
frame_times_)
On 2017/04/11 19:14:36, mthiesse wrote:
> Instead of recomputing the sum each time you read, why not update the sum as
you
> go?
> 
> eg.
> sum -= frame_times_[current_index_].InSecondsF();
> sum += delta.InSecondsF();
> frame_times_[current_index_] = delta;

floating point error will accumulate

[mthiesse, 2017-04-12 01:11:18]

On 2017/04/12 00:40:03, billorr wrote:
>
https://codereview.chromium.org/2804943003/diff/140001/chrome/browser/android...
> File chrome/browser/android/vr_shell/fps_meter.cc (right):
> 
>
https://codereview.chromium.org/2804943003/diff/140001/chrome/browser/android...
> chrome/browser/android/vr_shell/fps_meter.cc:52: for (const auto& delta :
> frame_times_)
> On 2017/04/11 19:14:36, mthiesse wrote:
> > Instead of recomputing the sum each time you read, why not update the sum as
> you
> > go?
> > 
> > eg.
> > sum -= frame_times_[current_index_].InSecondsF();
> > sum += delta.InSecondsF();
> > frame_times_[current_index_] = delta;
> 
> floating point error will accumulate

It's double precision... I expect we could get away with this for a *very* long
time. If we care we could also recompute the sum from scratch every few million
frames :P

[Ian Vollick, 2017-04-12 01:12:47]

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

[commit-bot: I haz the power, 2017-04-12 01:13:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ian Vollick, 2017-04-12 01:25:06]

billorr suggested storing total microseconds as int64_t which seems nice to me.
how do you folks feel about that?

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

https://codereview.chromium.org/2804943003/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:151: #ifndef NDEBUG
On 2017/04/11 19:17:13, mthiesse wrote:
> Also, consider using DCHECK_ALWAYS_ON? Might be useful to be able to enable
this
> for release builds.

Sure thing. My hope is that when this is ready (i.e., when we've got a proper
UI), we'll have a nicer way to turn this on in release (via a flag or similar).

[Ian Vollick, 2017-04-12 01:26:43]

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

[commit-bot: I haz the power, 2017-04-12 01:27:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ian Vollick, 2017-04-12 01:52:58]

Description was changed from

==========
[vr] Use cc's frame rate counter for the vr UI

With this CL, we only gather the info needed for an
FPS meter. It does not currently output anything (this
is future work.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[vr] Add a frame rate counter for the vr UI

With this CL, we only gather the info needed for an
FPS meter. It does not currently output anything (this
is future work.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-04-12 02:26:27]

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

[commit-bot: I haz the power, 2017-04-12 02:26:29]

Dry run: 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...)

[mthiesse, 2017-04-12 02:39:00]

Avoiding floats sgtm

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

https://codereview.chromium.org/2804943003/diff/220001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:151: #if !defined(NDEBUG) ||
defined(DCHECK_ALWAYS_ON)
Oops, I meant to suggest DCHECK_IS_ON. Then you don't need to check two flags.

[Ian Vollick, 2017-04-12 03:07:23]

On 2017/04/12 02:39:00, mthiesse wrote:
> Avoiding floats sgtm
> 
>
https://codereview.chromium.org/2804943003/diff/220001/chrome/browser/android...
> File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
> 
>
https://codereview.chromium.org/2804943003/diff/220001/chrome/browser/android...
> chrome/browser/android/vr_shell/vr_shell_gl.cc:151: #if !defined(NDEBUG) ||
> defined(DCHECK_ALWAYS_ON)
> Oops, I meant to suggest DCHECK_IS_ON. Then you don't need to check two flags.

Done. Is ok?

[mthiesse, 2017-04-12 03:15:30]

lgtm

[Ian Vollick, 2017-04-12 03:40:57]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2017-04-12 03:41:13]

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-04-12 04:41:55]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491968457781070,
"parent_rev": "e83f086321c0dab4f9562a8d0140ea3b878d535f", "commit_rev":
"47619f8e9971f86a0478b32190a004b9a53141c1"}

[commit-bot: I haz the power, 2017-04-12 04:42:51]

Description was changed from

==========
[vr] Add a frame rate counter for the vr UI

With this CL, we only gather the info needed for an
FPS meter. It does not currently output anything (this
is future work.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[vr] Add a frame rate counter for the vr UI

With this CL, we only gather the info needed for an
FPS meter. It does not currently output anything (this
is future work.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2804943003
Cr-Commit-Position: refs/heads/master@{#463931}
Committed:
https://chromium.googlesource.com/chromium/src/+/47619f8e9971f86a0478b32190a0...
==========

[commit-bot: I haz the power, 2017-04-12 04:42:52]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/47619f8e9971f86a0478b32190a0...