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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by klausw
Modified:
2 months, 4 weeks 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 Review-Url: https://codereview.chromium.org/2888313002 Cr-Commit-Position: refs/heads/master@{#474053} Committed: https://chromium.googlesource.com/chromium/src/+/7b6d11d65c59a5e13a62942227b5caf16c70993a

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) #

Patch Set 5 : Rebase #

Patch Set 6 : Rebase, depends-on is submitted #

Patch Set 7 : Tweak test to run a few frames in magic window mode #

Unified diffs Side-by-side diffs Delta from patch set Stats (+246 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 2 3 4 5 6 1 chunk +82 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 2 3 4 5 1 chunk +3 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 40 (26 generated)
klausw
This patch is part of my ongoing refactoring to tweak rAF timing, but I think ...
3 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months ago (2017-05-19 04:19:10 UTC) #24
klausw
On 2017/05/19 04:06:47, klausw wrote: > The delayed getVSync now always happen after the step ...
3 months ago (2017-05-19 05:34:43 UTC) #27
bajones
LGTM
3 months ago (2017-05-22 21:26:19 UTC) #30
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/120001
2 months, 4 weeks ago (2017-05-23 21:03:26 UTC) #37
commit-bot: I haz the power
2 months, 4 weeks ago (2017-05-23 21:12:21 UTC) #40
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/7b6d11d65c59a5e13a62942227b5...
Sign in to reply to this message.

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