|
|
DescriptionDraw WebVR security overlay with a non-reprojected viewport
This prevents the overlay from jittering regardless of the performance of the
WebVR content.
BUG=389343
Committed: https://crrev.com/58290e535d3de7a235e13b6f023b1c8bf06a223a
Cr-Commit-Position: refs/heads/master@{#426912}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Addressed Klaus' feedback #
Total comments: 5
Patch Set 3 : Rebase, Overlay->Headlocked #
Messages
Total messages: 27 (8 generated)
klausw@google.com changed reviewers: + klausw@google.com
Looks good in general, minor suggestions below. https://codereview.chromium.org/2430543002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2430543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:447: buffer_viewport_list_->SetBufferViewport(buffer_viewport_list_->GetSize(), Using GetSize() twice seems weird. Does the SetToRecommendedBufferViewports() above create a two-element list (0=left eye, 1=right eye), and this appends items 2=overlay left, 3=overlay right? https://codereview.chromium.org/2430543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:450: // Bind back to the second default framebuffer. These comments are confusing. Can you rephrase this one and the one above to something like this? // Render to the main viewport set. frame.BindBuffer(0); ... // Render to the overlay viewport set. frame.BindBuffer(1); For bonus points, use a constant instead of a hardcoded "1" here and in SetSourceBufferIndex above? https://codereview.chromium.org/2430543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:669: // Don't need to clear, since we're drawing over the entire render target. Please delete this comment, it conflicts with the glClear immediately below.
bajones@chromium.org changed reviewers: + bshe@chromium.org, klausw@chromium.org - klausw@google.com
Thanks for the review, Klaus! Updated to take your feedback into account and also added some code to lessen the amount of time we're drawing blank content and resized one of the rects to prevent some single-pixel artifacts at the bottom.
LGTM https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:788: dom_contents_loaded_ = true; I'm a bit fuzzy about how the life cycle works, does this need to be set back to false on exiting VR mode, or does that happen implicitly when/if the VrShell object gets destroyed and recreated? Sounds like the latter.
bshe@chromium.org changed reviewers: + cjgrant@chromium.org
+cjgrant in case this conflicts with his refactor
On 2016/10/21 18:18:53, bshe wrote: > +cjgrant in case this conflicts with his refactor It may conflict, but not in a bad way. This change is fixing the jittering. My change is drawing the warnings as part of the UI, rather than using custom native code. So, this change may simply be refactored to draw all UI elements, rather than specific warnings. For reference: https://codereview.chromium.org/2436863002
Also, I'm looking at the GL calls that occur in the 2D and WebVR cases, and seeing things that look odd or broken (I am a GL newb though). After these changes land, we should sort those out.
https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:788: dom_contents_loaded_ = true; On 2016/10/21 17:50:47, klausw wrote: > I'm a bit fuzzy about how the life cycle works, does this need to be set back to > false on exiting VR mode, or does that happen implicitly when/if the VrShell > object gets destroyed and recreated? Sounds like the latter. See previous comments. Native state changes (like WebVR mode and secure origin) will be propagated to the HTML UI, and the HTML UI will trigger the appearance/disappearance of warnings as it would any other UI element. https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:174: bool dom_contents_loaded_ = false; This will probably disappear with my change. Native rendering shouldn't care if the DOM contents have loaded, because the HTML UI will drive the security warnings.
From the perspective of the HTML UI, I think it'd be best to land this change ahead of my pending change to pull all the custom security warning code out of native. I'll refactor my change to use this non-jitter work for the benefit of all UI elements. How does that sound?
cjgrant, refactoring other UI code to use this buffer doesn't sound appropriate. The key reason for this separate overlay is that the warnings are head locked, so we don't want reprojection for them since that's only useful for world-relative content. I've suggested a rename below to make that more obvious. In theory the head locked overlay would be useful for a Cardboard-style fixed gaze reticle which is head locked, but since there's no reprojection for Cardboard headsets there wouldn't be much point in refactoring it to use this. https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:93: static constexpr int kFrameOverlayBuffer = 1; rename to kFrameHeadLockedBuffer ?
On 2016/10/21 18:32:42, klausw wrote: > cjgrant, refactoring other UI code to use this buffer doesn't sound appropriate. > The key reason for this separate overlay is that the warnings are head locked, > so we don't want reprojection for them since that's only useful for > world-relative content. I've suggested a rename below to make that more obvious. > > In theory the head locked overlay would be useful for a Cardboard-style fixed > gaze reticle which is head locked, but since there's no reprojection for > Cardboard headsets there wouldn't be much point in refactoring it to use this. > > https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_shell.cc:93: static constexpr int > kFrameOverlayBuffer = 1; > rename to kFrameHeadLockedBuffer ? More context: UI elements can optionally be head-locked. When my changes land, the HTML UI will be driving head-locked security warnings that look identical to what's currently in native. I think that means that when the new UI code renders elements, it would render head-locked elements using the new path added in this CL. Conventional elements would be rendered as they are today. Does that make sense?
On 2016/10/21 18:40:27, cjgrant wrote: > On 2016/10/21 18:32:42, klausw wrote: > > cjgrant, refactoring other UI code to use this buffer doesn't sound > appropriate. > > The key reason for this separate overlay is that the warnings are head locked, > > so we don't want reprojection for them since that's only useful for > > world-relative content. I've suggested a rename below to make that more > obvious. > > > > In theory the head locked overlay would be useful for a Cardboard-style fixed > > gaze reticle which is head locked, but since there's no reprojection for > > Cardboard headsets there wouldn't be much point in refactoring it to use this. > > > > > https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... > > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > > > > https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... > > chrome/browser/android/vr_shell/vr_shell.cc:93: static constexpr int > > kFrameOverlayBuffer = 1; > > rename to kFrameHeadLockedBuffer ? > > More context: UI elements can optionally be head-locked. When my changes land, > the HTML UI will be driving head-locked security warnings that look identical to > what's currently in native. I think that means that when the new UI code > renders elements, it would render head-locked elements using the new path added > in this CL. Conventional elements would be rendered as they are today. Does > that make sense? Yes, this does make sense. Reprojection works well for static world content, and is still generally helpful for moving in-world objects even though those can look jumpy or duplicated. It's never helpful for head-locked objects, those always look worse with reprojection.
Cool. Let me know how you want to proceed - ie, this change landing first and me refactoring after that, or vice-versa.
lgtm https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2430543002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:265: overlay_left_viewport_.reset( nit: if you rename the above constexpr to HeadLocked, rename this overlay to head_lock too.
On 2016/10/21 18:53:37, cjgrant wrote: > Cool. Let me know how you want to proceed - ie, this change landing first and > me refactoring after that, or vice-versa. I'll land this now, since it doesn't sound like it will be too difficult for you to integrate into your patch.
The CQ bit was checked by bajones@chromium.org
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bajones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2430543002/#ps40001 (title: "Rebase, Overlay->Headlocked")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Draw WebVR security overlay with a non-reprojected viewport This prevents the overlay from jittering regardless of the performance of the WebVR content. BUG=389343 ========== to ========== Draw WebVR security overlay with a non-reprojected viewport This prevents the overlay from jittering regardless of the performance of the WebVR content. BUG=389343 Committed: https://crrev.com/58290e535d3de7a235e13b6f023b1c8bf06a223a Cr-Commit-Position: refs/heads/master@{#426912} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/58290e535d3de7a235e13b6f023b1c8bf06a223a Cr-Commit-Position: refs/heads/master@{#426912} |