|
|
Created:
4 years, 2 months ago by bajones Modified:
4 years, 1 month ago CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, Eric Willigers, haraken, rjwright, rwlbuis, shans, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow VRDisplay to manage it's own rAF callbacks.
This will be necessary when we begin firing VR rAFs at a different rate than window.rAF, but here is used to prevent misuse of submitFrame, which needs to happen in a VR-specific rAF in order to ensure a good experience across the board.
BUG=389343
Committed: https://crrev.com/8dcae4a8c016b4b284cc550b23bc404a2c439d25
Cr-Commit-Position: refs/heads/master@{#427496}
Patch Set 1 #Patch Set 2 : Fixing commit hook issues #Patch Set 3 : A Few more tweaks #
Total comments: 3
Patch Set 4 : Address review feedback #
Messages
Total messages: 21 (11 generated)
Description was changed from ========== Allow VRDisplay to manage it's own rAF callbacks BUG= ========== to ========== Allow VRDisplay to manage it's own rAF callbacks. This will be necessary when we begin firing VR rAFs at a different rate than window.rAF, but here is used to prevent misuse of submitFrame, which needs to happen in a VR-specific rAF in order to ensure a good experience across the board. BUG=389343 ==========
bajones@chromium.org changed reviewers: + klausw@chromium.org, mkwst@chromium.org
mkwst@: Could you take a look at the use of ScriptedAnimationController? Eventually we'll have a code path that services it independent of the window rAF based on the VR hardware's refresh rate, so the funny chaining of rAF calls here is temporary. klasw@: Mostly FYI, but I'd welcome any comments you have.
mkwst@chromium.org changed reviewers: + jochen@chromium.org, skyostil@chromium.org
I'm a bad reviewer for timing kinds of things. +skyostil@ who knows things about scheduling.
This basically lgtm, although I had some questions mainly because I'm not very familiar with how WebVR has spec'd some of these things. I'd be great if you could point me a design doc of how we're expecting this to ultimately look :) https://codereview.chromium.org/2413903002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2413903002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:167: return ensureScriptedAnimationController(doc).registerCallback(callback); Does it ever make sense to register more than one callback for a given VRDisplay? I'd expect you'd only ever want at most one submitFrame() per VR-vsync? https://codereview.chromium.org/2413903002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:180: m_inAnimationFrame = true; nit: Could use AutoReset here. https://codereview.chromium.org/2413903002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:493: // We need to make sure that we don't start up the animation controller on a Should this be a TODO since we're not resuming the controller anywhere? I guess we'll also need to plumb some signals about visibility to make this work -- or does the VRDisplay manage that automatically?
RS LGTM. Once skyostil@ is happy with the shape of things, I'm happy too.
> This basically lgtm, although I had some questions mainly because I'm not very > familiar with how WebVR has spec'd some of these things. I'd be great if you > could point me a design doc of how we're expecting this to ultimately look :) Spec is here: https://w3c.github.io/webvr/ Explainer here: https://github.com/w3c/webvr/blob/master/explainer.md (explainer is a little out-of-date but still hits the major points.) We could definitely stand to have a bit more explicit information about the rendering loop in there, though! > https://codereview.chromium.org/2413903002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > https://codereview.chromium.org/2413903002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:167: return > ensureScriptedAnimationController(doc).registerCallback(callback); > Does it ever make sense to register more than one callback for a given > VRDisplay? I'd expect you'd only ever want at most one submitFrame() per > VR-vsync? Good question! I don't know of any scenarios off the top of my head that would require multiple callbacks, but we're also trying to stick pretty close to the conventions set by the normal rAF for developer familiarity. You're correct in that you want only one submitFrame per vsync, but that's not the only thing a rAF callback could be used for. > https://codereview.chromium.org/2413903002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:180: m_inAnimationFrame = > true; > nit: Could use AutoReset here. Ah, didn't know about that particular class. Done! > https://codereview.chromium.org/2413903002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:493: // We need to make sure > that we don't start up the animation controller on a > Should this be a TODO since we're not resuming the controller anywhere? I guess > we'll also need to plumb some signals about visibility to make this work -- or > does the VRDisplay manage that automatically? Another good question! I can see that the VR hardware may want to keep working even if you switched tabs (like an audio stream or webcam), so for the moment I've stripped out the suspend call. I do want to bring the issue up with the other implementors and see what they think, though. If we decided that VR devices should only be accessible to a visible tab I'll add the suspend/resume logic in with page visibility tracking.
The CQ bit was checked by bajones@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/24 22:13:01, bajones wrote: > https://codereview.chromium.org/2413903002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:493: // We need to make > sure > > that we don't start up the animation controller on a > > Should this be a TODO since we're not resuming the controller anywhere? I > guess > > we'll also need to plumb some signals about visibility to make this work -- or > > does the VRDisplay manage that automatically? > > Another good question! I can see that the VR hardware may want to keep working > even if you switched tabs (like an audio stream or webcam), so for the moment > I've stripped out the suspend call. I do want to bring the issue up with the > other implementors and see what they think, though. If we decided that VR > devices should only be accessible to a visible tab I'll add the suspend/resume > logic in with page visibility tracking. FWIW, I think keeping VR devices active in the background sounds potentially useful on a desktop machine, i.e. one person using the VR headset/controllers while another is using keyboard/mouse/monitor on a shared machine. Other use cases would include development (being able to switch to a web-based tool without suspending the VR environment), or asymmetric multiplayer. If the current non-suspending behavior is intentional, would it be worth adding a brief comment explaining this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Thanks for the links! lgtm -- although I agree with Klaus that the special background behavior could use a comment as a reminder. On 2016/10/24 22:13:01, bajones wrote: > Good question! I don't know of any scenarios off the top of my head that would > require multiple callbacks, but we're also trying to stick pretty close to the > conventions set by the normal rAF for developer familiarity. You're correct in > that you want only one submitFrame per vsync, but that's not the only thing a > rAF callback could be used for. True. I guess the real difference to normal rAF is that submitFrame() is explicit rather than implicit, but maybe there's a good reason for that. Anyway, that's really only tangentially related to this patch. > Another good question! I can see that the VR hardware may want to keep working > even if you switched tabs (like an audio stream or webcam), so for the moment > I've stripped out the suspend call. I do want to bring the issue up with the > other implementors and see what they think, though. If we decided that VR > devices should only be accessible to a visible tab I'll add the suspend/resume > logic in with page visibility tracking. We currently have a bunch of safeguards for never backgrounding tabs that are playing audio. Perhaps VR-usage should be added as a condition there too? Since backgrounding is driven by the browser, the renderer doesn't need to do anything special -- it'll just think the tab is always in the foreground. FYI some APIs like Gamepad (IIRC) are spec'd to stop working in background tabs which would also be fixed by this safeguard.
The CQ bit was checked by bajones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2413903002/#ps60001 (title: "Address review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allow VRDisplay to manage it's own rAF callbacks. This will be necessary when we begin firing VR rAFs at a different rate than window.rAF, but here is used to prevent misuse of submitFrame, which needs to happen in a VR-specific rAF in order to ensure a good experience across the board. BUG=389343 ========== to ========== Allow VRDisplay to manage it's own rAF callbacks. This will be necessary when we begin firing VR rAFs at a different rate than window.rAF, but here is used to prevent misuse of submitFrame, which needs to happen in a VR-specific rAF in order to ensure a good experience across the board. BUG=389343 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Allow VRDisplay to manage it's own rAF callbacks. This will be necessary when we begin firing VR rAFs at a different rate than window.rAF, but here is used to prevent misuse of submitFrame, which needs to happen in a VR-specific rAF in order to ensure a good experience across the board. BUG=389343 ========== to ========== Allow VRDisplay to manage it's own rAF callbacks. This will be necessary when we begin firing VR rAFs at a different rate than window.rAF, but here is used to prevent misuse of submitFrame, which needs to happen in a VR-specific rAF in order to ensure a good experience across the board. BUG=389343 Committed: https://crrev.com/8dcae4a8c016b4b284cc550b23bc404a2c439d25 Cr-Commit-Position: refs/heads/master@{#427496} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8dcae4a8c016b4b284cc550b23bc404a2c439d25 Cr-Commit-Position: refs/heads/master@{#427496} |