WebVR: Defer GetVSync calls until the current frame is submitted.

WebVR: Defer GetVSync calls until the current frame is submitted.

In order to help the VR device with scheduling, never request a new VSync
until the current frame is either submitted or abandoned. If vrDisplay.rAF
is called earlier, defer the GetVSync until vrDisplay.submitFrame is called.
If the rAF callback exits without submitting a frame, call it at that time.

This helps ensure that the VR device knows how long the rendering portion
of the rAF callback takes, while making sure not to expect a frame that
won't arrive because the client failed to submit one.

BUG=723962
Review-Url: https://codereview.chromium.org/2888313002
Cr-Commit-Position: refs/heads/master@{#474053}
Committed: https://chromium.googlesource.com/chromium/src/+/7b6d11d65c59a5e13a62942227b5caf16c70993a

[klausw, 2017-05-18 05:42:09]

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

[klausw, 2017-05-18 05:42:09]

klausw@chromium.org changed required reviewers:
+ mthiesse@chromium.org

[klausw, 2017-05-18 05:42:10]

This patch is part of my ongoing refactoring to tweak rAF timing, but I think it
offers some benefit by itself already without other changes since it reduces the
current tendency to request poses much earlier than needed.

[klausw, 2017-05-18 05:42:18]

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

[commit-bot: I haz the power, 2017-05-18 05:42:58]

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-18 06:50:54]

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

[commit-bot: I haz the power, 2017-05-18 06:50:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bajones, 2017-05-18 15:59:18]

https://codereview.chromium.org/2888313002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2888313002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:153: }
I'm not sure what this call site provides? Seems like it can't be called unless
the frame has been submitted, which calls RequestVSync anyway?

[mthiesse, 2017-05-18 16:09:01]

This *especially* needs tests to make sure we don't fail to fire the rAF
callback under all of these various cases.

https://codereview.chromium.org/2888313002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2888313002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:153: }
On 2017/05/18 15:59:18, bajones wrote:
> I'm not sure what this call site provides? Seems like it can't be called
unless
> the frame has been submitted, which calls RequestVSync anyway?

This code is all really subtle and kind of scary.

We need this here in case the page calls rAF after calling SubmitFrame, because
then RequestVSync() inside SubmitFrame will do nothing.

Klaus please add a comment here explaining how all possible cases are covered
and we won't fail to request a VSync in response to rAF.

https://codereview.chromium.org/2888313002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:777: 
Add a DCHECK here that if pending_vrdisplay_raf_ is true, that either the VSync
has been requested, or that the vsync provider isn't connected.

[klausw, 2017-05-18 22:22:29]

klausw@chromium.org changed reviewers:
+ vollick@chromium.org

[klausw, 2017-05-18 22:22:29]

vollick@chromium.org: Please review changes in RuntimeEnabledFeatures.json5

PTAL, added a specific test for the cases involved here, plus a bonus test for
window.rAF to vrDisplay.rAF handoff.

https://codereview.chromium.org/2888313002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2888313002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:153: }
On 2017/05/18 16:09:01, mthiesse wrote:
> On 2017/05/18 15:59:18, bajones wrote:
> > I'm not sure what this call site provides? Seems like it can't be called
> unless
> > the frame has been submitted, which calls RequestVSync anyway?
> 
> This code is all really subtle and kind of scary.
> 
> We need this here in case the page calls rAF after calling SubmitFrame,
because
> then RequestVSync() inside SubmitFrame will do nothing.
> 
> Klaus please add a comment here explaining how all possible cases are covered
> and we won't fail to request a VSync in response to rAF.

I've added a detailed explanation in RequestVSync, pointing to it from here.
Also added a layout test that covers all four cases.

https://codereview.chromium.org/2888313002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:777: 
On 2017/05/18 16:09:01, mthiesse wrote:
> Add a DCHECK here that if pending_vrdisplay_raf_ is true, that either the
VSync
> has been requested, or that the vsync provider isn't connected.

Done.

[klausw, 2017-05-18 22:25:51]

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

[commit-bot: I haz the power, 2017-05-18 22:27:00]

Dry run: CQ is trying da patch.

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

[mthiesse, 2017-05-18 23:39:15]

lgtm % nits

https://codereview.chromium.org/2888313002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js (right):

https://codereview.chromium.org/2888313002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:28: //
console.log("requestPresent");
nit: remove commented out log statements here and below.

[mthiesse, 2017-05-18 23:41:24]

Also we're kind of trusting that this will be better than requesting vsync
immediately. Do we have evidence that this doesn't introduce extra delays/lower
throughput? Presumably it mostly benefits pose latency?

[Ian Vollick, 2017-05-18 23:48:02]

On 2017/05/18 23:41:24, mthiesse wrote:
> Also we're kind of trusting that this will be better than requesting vsync
> immediately. Do we have evidence that this doesn't introduce extra
delays/lower
> throughput? Presumably it mostly benefits pose latency?

platform/ lgtm.

[commit-bot: I haz the power, 2017-05-19 01:17:49]

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

[commit-bot: I haz the power, 2017-05-19 01:17:51]

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

[klausw, 2017-05-19 04:06:47]

I've added some rather verbose comments to the handoff test and
mock-vr-service.js to explain the limitations and gotchas for running this in
the test framework.

mthiesse wrote:
> Also we're kind of trusting that this will be better than requesting vsync
> immediately. Do we have evidence that this doesn't introduce extra
delays/lower
> throughput? Presumably it mostly benefits pose latency?

I'll look to get some numbers for comparison.

I don't expect that this would cause any noticeable delays, the Javascript
execution is single-threaded anyway and the "getVSync" should be ready to be
handled in time for triggering the next VSync.

The main goal for this is to reduce queue stuffing and thereby pose latency.
It's not complete yet, but I think the core issue is that many applications need
more time for rendering (post-Javascript-submit to GVR submit) than for JS
execution (rAF start to Javascript submit). In that case, it over-aggressively
schedules JS execution and ends up stuffing the pre-GVR queues (always) and also
the GVR queue (if running at 60fps).

The delayed getVSync now always happen after the step where submitFrame waits
for the previous frame's rendering to complete, which should already reduce
queue stuffing and ensure that poses are fresher. My planned next step is to
move the "render complete" callback past GVR submit to correctly reflect the
overall render time, so that the next submitted frame can immediately start
rendering instead of overlapping with the pre-GVR render steps of the previous
frame.

Anyway, I hope to get some numbers soon for this, it's based on experiments and
timing graphs I'd been doing in a separate branch, but I am reasonably confident
that this much simpler approach should still provide most of the benefit. Apps
should still have ~16ms for JS execution and ~16ms for rendering (including
pre-GVR steps, so less for the app itself) available per frame.

https://codereview.chromium.org/2888313002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js (right):

https://codereview.chromium.org/2888313002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:28: //
console.log("requestPresent");
On 2017/05/18 23:39:14, mthiesse wrote:
> nit: remove commented out log statements here and below.

Done.

[klausw, 2017-05-19 04:07:58]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-05-19 04:07:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2888313002/#ps40001
(title: "Add comments, remove dead code")

[commit-bot: I haz the power, 2017-05-19 04:08:46]

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-19 04:19:08]

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

[commit-bot: I haz the power, 2017-05-19 04:19:10]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[klausw, 2017-05-19 04:45:06]

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

[commit-bot: I haz the power, 2017-05-19 04:45:48]

Dry run: CQ is trying da patch.

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

[klausw, 2017-05-19 05:34:43]

On 2017/05/19 04:06:47, klausw wrote:
> The delayed getVSync now always happen after the step where submitFrame waits
> for the previous frame's rendering to complete, which should already reduce
> queue stuffing and ensure that poses are fresher. My planned next step is to
> move the "render complete" callback past GVR submit to correctly reflect the
> overall render time, so that the next submitted frame can immediately start
> rendering instead of overlapping with the pre-GVR render steps of the previous
> frame.

FYI, an initial draft of planned next step is in http://crrev.com/2897583002.
It'll take a while for me to test that since I'm now building Chrome on my
laptop :-/

In any case, I do want to see some numbers and do sanity checks before
committing any of the patches in this series.

[commit-bot: I haz the power, 2017-05-19 10:37:45]

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

[commit-bot: I haz the power, 2017-05-19 10:37:46]

Dry run: This issue passed the CQ dry run.

[bajones, 2017-05-22 21:26:19]

LGTM

[klausw, 2017-05-23 18:20:27]

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

[commit-bot: I haz the power, 2017-05-23 18:21: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-05-23 20:31:17]

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

[commit-bot: I haz the power, 2017-05-23 20:31:19]

Dry run: This issue passed the CQ dry run.

[klausw, 2017-05-23 21:01:58]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-05-23 21:01:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2888313002/#ps120001
(title: "Tweak test to run a few frames in magic window mode")

[commit-bot: I haz the power, 2017-05-23 21:03:26]

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-23 21:12:08]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495573318039910,
"parent_rev": "ac50ea80f469353abd39abf4bea74a9b14065877", "commit_rev":
"7b6d11d65c59a5e13a62942227b5caf16c70993a"}

[commit-bot: I haz the power, 2017-05-23 21:12:20]

Description was changed from

==========
WebVR: Defer GetVSync calls until the current frame is submitted.

In order to help the VR device with scheduling, never request a new VSync
until the current frame is either submitted or abandoned. If vrDisplay.rAF
is called earlier, defer the GetVSync until vrDisplay.submitFrame is called.
If the rAF callback exits without submitting a frame, call it at that time.

This helps ensure that the VR device knows how long the rendering portion
of the rAF callback takes, while making sure not to expect a frame that
won't arrive because the client failed to submit one.

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

to

==========
WebVR: Defer GetVSync calls until the current frame is submitted.

In order to help the VR device with scheduling, never request a new VSync
until the current frame is either submitted or abandoned. If vrDisplay.rAF
is called earlier, defer the GetVSync until vrDisplay.submitFrame is called.
If the rAF callback exits without submitting a frame, call it at that time.

This helps ensure that the VR device knows how long the rendering portion
of the rAF callback takes, while making sure not to expect a frame that
won't arrive because the client failed to submit one.

BUG=723962

Review-Url: https://codereview.chromium.org/2888313002
Cr-Commit-Position: refs/heads/master@{#474053}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b6d11d65c59a5e13a62942227b5...
==========

[commit-bot: I haz the power, 2017-05-23 21:12:21]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/7b6d11d65c59a5e13a62942227b5...