Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848483003:
WebVR: fix initial vsync (Closed)
DescriptionWebVR: fix initial vsync
Applications sometimes use window.rAF while not presenting, then switch to
vrDisplay.rAF after presentation starts. Depending on the animation loop's
timing, this can cause a race condition where presentation has been started
but there's no vrDisplay.rAF pending yet. Ensure there's at least vsync
being processed after presentation starts so that a queued window.rAF
can run and schedule a vrDisplay.rAF.
BUG=711789
Review-Url: https://codereview.chromium.org/2848483003
Cr-Commit-Position: refs/heads/master@{#468167}
Committed: https://chromium.googlesource.com/chromium/src/+/262e77a72493e36e8006aeeba1c7497a42ee5ad9
Patch Set 1 #
Total comments: 5
Patch Set 2 : move iframe focus to separate patch #Patch Set 3 : Logging fixes as suggested. #
Total comments: 3
Patch Set 4 : Use simple scheduling call instead of initializing vsync. #
Total comments: 2
Messages
Total messages: 28 (10 generated)
klausw@chromium.org changed reviewers: + bajones@chromium.org, mthiesse@chromium.org
Can you split this up into two CLs please?
LGTM, but I have some nits on a couple of your console messages https://codereview.chromium.org/2848483003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2848483003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:457: "Scheduling missing vsync.")); Is this something that we feel pages should be architected to avoid? If so I think the message should be a bit more instructive about how to avoid hitting this condition, if not then I don't feel it's appropriate to log a warning to the console for expected behavior and a DVLOG is probably more appropriate. https://codereview.chromium.org/2848483003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:771: "VRDisplay.requestAnimationFrame not called, presentation is broken.")); This seems drastic. o_O If the developer calls VRDisplay.requestAnimationFrame again at some other point outside the rAF they will continue getting new frames, right? If so, I would expect the warning should read more like "No VR frame callback scheduled. Presentation will not resume until VRDisplay.requestAnimationFrame is called."
To be clear, not lgtm, this contains subtle fixes for two different issues that should be reviewed and landed separately.
On 2017/04/27 22:10:48, mthiesse wrote: > To be clear, not lgtm, this contains subtle fixes for two different issues that > should be reviewed and landed separately. Fair enough. Let's separate out the focus fixes from the vsync bits.
Posting this here so that I don't forget/miss the split CLs: We should add tests to make sure these issues don't regress in the future. The rAF issue should be testable with a fairly simple layout test. The Cardboard click issue might be testable with layout tests?
Description was changed from ========== WebVR: fix initial vsync and focus while presenting Applications sometimes use window.rAF while not presenting, then switch to vrDisplay.rAF after presentation starts. Depending on the animation loop's timing, this can cause a race condition where presentation has been started but there's no vrDisplay.rAF pending yet. Ensure there's at least vsync being processed after presentation starts so that a queued window.rAF can run and schedule a vrDisplay.rAF. Also, if the WebVR document loses focus, check if the new focused element is an embedding local parent frame. If that's the case, continue presenting. This fixes issues with Cardboard-style touch events that are reported as a click at viewport (0, 0) where the resulting focus loss stopped presentation. BUG=710863,711789 ========== to ========== WebVR: fix initial vsync Applications sometimes use window.rAF while not presenting, then switch to vrDisplay.rAF after presentation starts. Depending on the animation loop's timing, this can cause a race condition where presentation has been started but there's no vrDisplay.rAF pending yet. Ensure there's at least vsync being processed after presentation starts so that a queued window.rAF can run and schedule a vrDisplay.rAF. BUG=711789 ==========
PTAL. The iframe focus patch is now in crrev.com/2847233002 as an independent patch, they can be applied in any order. Sorry about that, in hindsight I should have kept them separate. The investigation was related but the actual changes didn't really overlap. https://codereview.chromium.org/2848483003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2848483003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:457: "Scheduling missing vsync.")); On 2017/04/27 22:02:46, bajones wrote: > Is this something that we feel pages should be architected to avoid? If so I > think the message should be a bit more instructive about how to avoid hitting > this condition, if not then I don't feel it's appropriate to log a warning to > the console for expected behavior and a DVLOG is probably more appropriate. Changed to DVLOG, this was only there for investigating the issue. Sorry about that. https://codereview.chromium.org/2848483003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:771: "VRDisplay.requestAnimationFrame not called, presentation is broken.")); On 2017/04/27 22:02:46, bajones wrote: > This seems drastic. o_O If the developer calls VRDisplay.requestAnimationFrame > again at some other point outside the rAF they will continue getting new frames, > right? If so, I would expect the warning should read more like "No VR frame > callback scheduled. Presentation will not resume until > VRDisplay.requestAnimationFrame is called." I've changed it to a DVLOG. We should continue discussions here in the bug, IMHO it shouldn't be legal to stop frame production while waiting for a timeout or external event. (That's the only way it can resume presenting, right?)
The CQ bit was checked by klausw@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...
https://codereview.chromium.org/2848483003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2848483003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:771: "VRDisplay.requestAnimationFrame not called, presentation is broken.")); On 2017/04/28 18:43:14, klausw wrote: > On 2017/04/27 22:02:46, bajones wrote: > > This seems drastic. o_O If the developer calls VRDisplay.requestAnimationFrame > > again at some other point outside the rAF they will continue getting new > frames, > > right? If so, I would expect the warning should read more like "No VR frame > > callback scheduled. Presentation will not resume until > > VRDisplay.requestAnimationFrame is called." > > I've changed it to a DVLOG. We should continue discussions here in the bug, IMHO > it shouldn't be legal to stop frame production while waiting for a timeout or > external event. (That's the only way it can resume presenting, right?) To clarify, "the bug" in this context meant crbug.com/716087 as marked in the TODO comment for identifying unrecoverable presentation issues, it's not one of those addressed by these patches.
https://codereview.chromium.org/2848483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2848483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:413: // uses window.rAF while not yet presenting. It needs to get a chance So, why not just fire window.rAF once after presentChange? Why all this complexity about adding another path to initializing the VSync provider? Sure, they'll 'miss' one frame, but that's their fault. Also I'm really really itching to refactor how focus and vsync provider work and shift everything into the browser, but I'm not being allowed to do so until we ship :P
https://codereview.chromium.org/2848483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2848483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:407: // TODO(klausw,crbug.com/710863): there appear to be cases where an embedded If you simply replace all of the code here with: if (doc && doc->GetPage()) { doc->GetPage()->Animator().ServiceScriptedAnimations( WTF::MonotonicallyIncreasingTime()); } it seems to fix at least the quake site that was broken. Then you don't need the rest of the changes here.
PTAL. https://codereview.chromium.org/2848483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2848483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:413: // uses window.rAF while not yet presenting. It needs to get a chance On 2017/04/28 19:13:21, mthiesse wrote: > So, why not just fire window.rAF once after presentChange? Why all this > complexity about adding another path to initializing the VSync provider? > > Sure, they'll 'miss' one frame, but that's their fault. > > Also I'm really really itching to refactor how focus and vsync provider work and > shift everything into the browser, but I'm not being allowed to do so until we > ship :P I'm doing something similar now, and have confirmed that the MonotonicallyIncreasingTimestamp is compatible via some extra temporary logging: 04-28 13:17:10.490 25461 25476 I chromium: [INFO:VRDisplay.cpp(708)] ProcessScheduledWindowAnimations: timestamp=211870.831677ms 04-28 13:17:10.490 25461 25476 I chromium: [INFO:VRDisplay.cpp(711)] ProcessScheduledWindowAnimations: window.rAF fallback successfully scheduled VRDisplay.rAF 04-28 13:17:10.575 25461 25476 I chromium: [INFO:VRDisplay.cpp(708)] ProcessScheduledWindowAnimations: timestamp=211870.989914ms
LGTM again, thanks! You'll need to wait for @mthiesse to approve though, to clear his previous flag.
The CQ bit was checked by klausw@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...
lgtm https://codereview.chromium.org/2848483003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2848483003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:735: if (pending_vrdisplay_raf_ && scripted_animation_controller_) { nit: Checking pending_vrdisplay_raf_ is not really necessary because ProcessScheduledAnimations is only called from OnVSync, which can only be scheduled when there's a pending raf.
https://codereview.chromium.org/2848483003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2848483003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:735: if (pending_vrdisplay_raf_ && scripted_animation_controller_) { On 2017/04/28 20:35:39, mthiesse wrote: > nit: Checking pending_vrdisplay_raf_ is not really necessary because > ProcessScheduledAnimations is only called from OnVSync, which can only be > scheduled when there's a pending raf. True, but seems a bit subtle and possibly fragile. I'd prefer to keep the check even if it's technically redundant in this case.
On 2017/04/28 21:10:17, klausw wrote: > https://codereview.chromium.org/2848483003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > https://codereview.chromium.org/2848483003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:735: if > (pending_vrdisplay_raf_ && scripted_animation_controller_) { > On 2017/04/28 20:35:39, mthiesse wrote: > > nit: Checking pending_vrdisplay_raf_ is not really necessary because > > ProcessScheduledAnimations is only called from OnVSync, which can only be > > scheduled when there's a pending raf. > > True, but seems a bit subtle and possibly fragile. I'd prefer to keep the check > even if it's technically redundant in this case. Oh, please add a test before submitting. Should be pretty simple.
The CQ bit was unchecked by klausw@chromium.org
The CQ bit was checked by klausw@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/28 21:15:27, mthiesse wrote: > On 2017/04/28 21:10:17, klausw wrote: > > > https://codereview.chromium.org/2848483003/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > > > > https://codereview.chromium.org/2848483003/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:735: if > > (pending_vrdisplay_raf_ && scripted_animation_controller_) { > > On 2017/04/28 20:35:39, mthiesse wrote: > > > nit: Checking pending_vrdisplay_raf_ is not really necessary because > > > ProcessScheduledAnimations is only called from OnVSync, which can only be > > > scheduled when there's a pending raf. > > > > True, but seems a bit subtle and possibly fragile. I'd prefer to keep the > check > > even if it's technically redundant in this case. > > Oh, please add a test before submitting. Should be pretty simple. OK to defer the test for a followup? I need to head out, this would delay the patch until Mon. I'd prefer to see it get some canarying ASAP.
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493415891430060,
"parent_rev": "a396a09fc66ad70d5b12e0466d49e8670104b20b", "commit_rev":
"262e77a72493e36e8006aeeba1c7497a42ee5ad9"}
Message was sent while issue was closed.
Description was changed from ========== WebVR: fix initial vsync Applications sometimes use window.rAF while not presenting, then switch to vrDisplay.rAF after presentation starts. Depending on the animation loop's timing, this can cause a race condition where presentation has been started but there's no vrDisplay.rAF pending yet. Ensure there's at least vsync being processed after presentation starts so that a queued window.rAF can run and schedule a vrDisplay.rAF. BUG=711789 ========== to ========== WebVR: fix initial vsync Applications sometimes use window.rAF while not presenting, then switch to vrDisplay.rAF after presentation starts. Depending on the animation loop's timing, this can cause a race condition where presentation has been started but there's no vrDisplay.rAF pending yet. Ensure there's at least vsync being processed after presentation starts so that a queued window.rAF can run and schedule a vrDisplay.rAF. BUG=711789 Review-Url: https://codereview.chromium.org/2848483003 Cr-Commit-Position: refs/heads/master@{#468167} Committed: https://chromium.googlesource.com/chromium/src/+/262e77a72493e36e8006aeeba1c7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/262e77a72493e36e8006aeeba1c7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
