Draw WebVR security overlay with a non-reprojected viewport

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}

[klausw (use chromium instead), 2016-10-17 23:03:17]

klausw@google.com changed reviewers:
+ klausw@google.com

[klausw (use chromium instead), 2016-10-17 23:03:18]

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, 2016-10-21 17:34:45]

bajones@chromium.org changed reviewers:
+ bshe@chromium.org, klausw@chromium.org
- klausw@google.com

[bajones, 2016-10-21 17:34:45]

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.

[klausw, 2016-10-21 17:50:47]

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, 2016-10-21 18:18:52]

bshe@chromium.org changed reviewers:
+ cjgrant@chromium.org

[bshe, 2016-10-21 18:18:53]

+cjgrant in case this conflicts with his refactor

[cjgrant, 2016-10-21 18:24:31]

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

[cjgrant, 2016-10-21 18:25:38]

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.

[cjgrant, 2016-10-21 18:29:03]

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.

[cjgrant, 2016-10-21 18:31:11]

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?

[klausw, 2016-10-21 18:32:42]

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 ?

[cjgrant, 2016-10-21 18:40:27]

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?

[klausw, 2016-10-21 18:51:26]

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.

[cjgrant, 2016-10-21 18:53:37]

Cool.  Let me know how you want to proceed - ie, this change landing first and
me refactoring after that, or vice-versa.

[bshe, 2016-10-21 19:00:04]

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.

[bajones, 2016-10-21 20:19:06]

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.

[bajones, 2016-10-21 20:19:10]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-10-21 20:20:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 20:23:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 20:23:36]

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_...)

[bajones, 2016-10-21 20:51:59]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-10-21 20:51:59]

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

[commit-bot: I haz the power, 2016-10-21 20:52:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 22:16:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 22:26:23]

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}
==========

[commit-bot: I haz the power, 2016-10-21 22:26:24]

Patchset 3 (id:??) landed as
https://crrev.com/58290e535d3de7a235e13b6f023b1c8bf06a223a
Cr-Commit-Position: refs/heads/master@{#426912}