Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(30)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 days, 9 hours ago by klausw
Modified:
2 days, 15 hours ago
CC:
chromium-reviews, haraken, blink-reviews, feature-vr-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 5

Patch Set 2 : Refactor RequestVSync, add comments and test #

Total comments: 2

Patch Set 3 : Add comments, remove dead code #

Patch Set 4 : Rebase on crrev.com/2891033002#ps80001 (ps5) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+236 lines, -9 lines) Patch
A third_party/WebKit/LayoutTests/vr/requestAnimationFrame_handoff.html View 1 2 1 chunk +65 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/vr/requestAnimationFrame_submitFrame_combinations.html View 1 1 chunk +72 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js View 1 2 2 chunks +20 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/vr/VRDisplay.h View 2 chunks +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/vr/VRDisplay.cpp View 1 2 3 7 chunks +68 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 View 1 1 chunk +3 lines, -1 line 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 29 (18 generated)
klausw
This patch is part of my ongoing refactoring to tweak rAF timing, but I think ...
4 days, 9 hours ago (2017-05-18 05:42:10 UTC) #3
bajones
https://codereview.chromium.org/2888313002/diff/1/third_party/WebKit/Source/modules/vr/VRDisplay.cpp File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2888313002/diff/1/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode153 third_party/WebKit/Source/modules/vr/VRDisplay.cpp:153: } I'm not sure what this call site provides? ...
3 days, 23 hours ago (2017-05-18 15:59:18 UTC) #8
mthiesse
This *especially* needs tests to make sure we don't fail to fire the rAF callback ...
3 days, 23 hours ago (2017-05-18 16:09:01 UTC) #9
klausw
vollick@chromium.org: Please review changes in RuntimeEnabledFeatures.json5 PTAL, added a specific test for the cases involved ...
3 days, 17 hours ago (2017-05-18 22:22:29 UTC) #11
mthiesse
lgtm % nits https://codereview.chromium.org/2888313002/diff/20001/third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js File third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js (right): https://codereview.chromium.org/2888313002/diff/20001/third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js#newcode28 third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:28: // console.log("requestPresent"); nit: remove commented out ...
3 days, 15 hours ago (2017-05-18 23:39:15 UTC) #14
mthiesse
Also we're kind of trusting that this will be better than requesting vsync immediately. Do ...
3 days, 15 hours ago (2017-05-18 23:41:24 UTC) #15
Ian Vollick
On 2017/05/18 23:41:24, mthiesse wrote: > Also we're kind of trusting that this will be ...
3 days, 15 hours ago (2017-05-18 23:48:02 UTC) #16
klausw
I've added some rather verbose comments to the handoff test and mock-vr-service.js to explain the ...
3 days, 11 hours ago (2017-05-19 04:06:47 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888313002/40001
3 days, 11 hours ago (2017-05-19 04:08:46 UTC) #22
commit-bot: I haz the power
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_presubmit/builds/442053)
3 days, 11 hours ago (2017-05-19 04:19:10 UTC) #24
klausw
3 days, 9 hours ago (2017-05-19 05:34:43 UTC) #27
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06