|
|
DescriptionWebVR: flush before requesting sync token for mailbox
This is a speculative fix, but seems to mostly eliminate
black screen flashes during VR presentation when the
"WebVR experimental rendering optimization" flag is enabled.
(It's off by default.)
Since this flush can happen in parallel with the previous
frame's rendering, move it ahead of the "wait for previous
render to finish" wait.
BUG=723962
Review-Url: https://codereview.chromium.org/2891033002
Cr-Commit-Position: refs/heads/master@{#473687}
Committed: https://chromium.googlesource.com/chromium/src/+/83c3cf1e053cbeaa118ab891cb32fc5629a167a6
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix variable name mixup #Patch Set 3 : Tweak trace events #Patch Set 4 : Rebase to 2888313002/#ps40001 (ps3) #Patch Set 5 : Rebase on origin/master #Patch Set 6 : Rebase #Messages
Total messages: 37 (25 generated)
klausw@chromium.org changed reviewers: + bajones@chromium.org, mthiesse@chromium.org
klausw@chromium.org changed required reviewers: + mthiesse@chromium.org
As noted in the description, this is a bit speculative, but it seemed to fix the black screen flashes which made the experimental rendering path mostly unusable. If this works out, making this path the default would significantly reduce Javascript-side latency in submitting frames since we can defer a wait until the next frame where it almost always completes immediately.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
lgtm https://codereview.chromium.org/2891033002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2891033002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:622: auto token = static_image->GetSyncToken(); s/token/sync_token
On 2017/05/18 15:33:53, mthiesse wrote: > lgtm > > https://codereview.chromium.org/2891033002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > https://codereview.chromium.org/2891033002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:622: auto token = > static_image->GetSyncToken(); > s/token/sync_token LGTM as well, especially since this is off the default path.
On 2017/05/18 15:33:53, mthiesse wrote: > lgtm > > https://codereview.chromium.org/2891033002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > https://codereview.chromium.org/2891033002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:622: auto token = > static_image->GetSyncToken(); > s/token/sync_token Done, this was a bad cherry-pick with manual edits.
https://codereview.chromium.org/2891033002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2891033002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:622: auto token = static_image->GetSyncToken(); On 2017/05/18 15:33:53, mthiesse wrote: > s/token/sync_token Done, this was a cherry-pick with a bad manual edit.
The CQ bit was checked by klausw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2891033002/#ps20001 (title: "Fix variable name mixup")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2888313002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
FYI, I made some minor edits to tweak the event tracing so that it focuses on slower calls.
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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...
On 2017/05/18 18:02:17, mthiesse wrote: > lgtm I've rebased this change top of origin/master instead of basing it on crrev.com/2888313002 since that one is still under discussion and missing timing measurements. That way, this change can be applied independently.
Description was changed from ========== WebVR: flush before requesting sync token for mailbox This is a speculative fix, but seems to mostly eliminate black screen flashes during VR presentation when the "WebVR experimental rendering optimization" flag is enabled. (It's off by default.) BUG=723962 ========== to ========== WebVR: flush before requesting sync token for mailbox This is a speculative fix, but seems to mostly eliminate black screen flashes during VR presentation when the "WebVR experimental rendering optimization" flag is enabled. (It's off by default.) Since this flush can happen in parallel with the previous frame's rendering, move it ahead of the "wait for previous render to finish" wait. BUG=723962 ==========
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...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
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...
The CQ bit was unchecked by klausw@chromium.org
The CQ bit was checked by klausw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org, mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2891033002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495482688120050, "parent_rev": "4ecb9563608b9759307678598bdde3923eff0df7", "commit_rev": "83c3cf1e053cbeaa118ab891cb32fc5629a167a6"}
Message was sent while issue was closed.
Description was changed from ========== WebVR: flush before requesting sync token for mailbox This is a speculative fix, but seems to mostly eliminate black screen flashes during VR presentation when the "WebVR experimental rendering optimization" flag is enabled. (It's off by default.) Since this flush can happen in parallel with the previous frame's rendering, move it ahead of the "wait for previous render to finish" wait. BUG=723962 ========== to ========== WebVR: flush before requesting sync token for mailbox This is a speculative fix, but seems to mostly eliminate black screen flashes during VR presentation when the "WebVR experimental rendering optimization" flag is enabled. (It's off by default.) Since this flush can happen in parallel with the previous frame's rendering, move it ahead of the "wait for previous render to finish" wait. BUG=723962 Review-Url: https://codereview.chromium.org/2891033002 Cr-Commit-Position: refs/heads/master@{#473687} Committed: https://chromium.googlesource.com/chromium/src/+/83c3cf1e053cbeaa118ab891cb32... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/83c3cf1e053cbeaa118ab891cb32... |