WebVR: Use content CVC size for compositor rendering

WebVR: Use content CVC size for compositor rendering

We can't currently resize the content window used by the
compositor rendering path, so we must use its dimensions
for the WebVR API reported render sizes.

It includes a rather nasty hack to override canvas size to 100% if
it was manually set to something different. This works around
three.js examples not rendering right, apparently a fullscreened
canvas just gets centered if it explicitly sets a size smaller
than the fullscreen dimensions.

BUG=389343, 655722
Committed: https://crrev.com/4026d565687ddbc1f67c4cc04887c339b7a03685
Cr-Commit-Position: refs/heads/master@{#432811}

[klausw, 2016-11-16 20:20:06]

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

[klausw, 2016-11-16 20:20:06]

bshe@chromium.org: Please review changes in vr_shell.cc

bajones@chromium.org: Please review changes elsewhere

[mthiesse, 2016-11-16 20:24:47]

On 2016/11/16 20:20:06, klausw wrote:
> mailto:bshe@chromium.org: Please review changes in vr_shell.cc
> 
> mailto:bajones@chromium.org: Please review changes elsewhere

Can you wait for https://codereview.chromium.org/2428383006/ to land? This is
duplicating a lot of work.

[mthiesse, 2016-11-16 20:37:49]

On 2016/11/16 20:24:47, mthiesse wrote:
> On 2016/11/16 20:20:06, klausw wrote:
> > mailto:bshe@chromium.org: Please review changes in vr_shell.cc
> > 
> > mailto:bajones@chromium.org: Please review changes elsewhere
> 
> Can you wait for https://codereview.chromium.org/2428383006/ to land? This is
> duplicating a lot of work.

As discussed offline, please revert VrShellImpl changes, the size values are
already passed down to native, we don't need more getters.

[klausw, 2016-11-16 20:46:37]

Description was changed from

==========
VrShell/WebVR content viewport refactoring

- don't assume that the ContentCVC and UiCVC are the same size.

- track resolution of WebVR viewport separately and support resize.

BUG=389343
==========

to

==========
WebVR: Use content CVC size for compositor rendering
    
We can't currently resize the content window used by the
compositor rendering path, so we must use its dimensions
for the WebVR API reported render sizes.

It includes a rather nasty hack to override canvas size to 100% if it was
manually set to something different. This works around
three.js examples not rendering right, apparently a fullscreened
canvas just gets centered if it explicitly sets a size smaller
than the fullscreen dimensions.

BUG=389343
==========

[klausw, 2016-11-16 20:47:18]

On 2016/11/16 20:37:49, mthiesse wrote:
> On 2016/11/16 20:24:47, mthiesse wrote:
> > On 2016/11/16 20:20:06, klausw wrote:
> > > mailto:bshe@chromium.org: Please review changes in vr_shell.cc
> > > 
> > > mailto:bajones@chromium.org: Please review changes elsewhere
> > 
> > Can you wait for https://codereview.chromium.org/2428383006/ to land? This
is
> > duplicating a lot of work.
> 
> As discussed offline, please revert VrShellImpl changes, the size values are
> already passed down to native, we don't need more getters.

Done, I've completely reverted the changes to VrShellImpl.

[klausw, 2016-11-16 20:48:05]

Description was changed from

==========
WebVR: Use content CVC size for compositor rendering
    
We can't currently resize the content window used by the
compositor rendering path, so we must use its dimensions
for the WebVR API reported render sizes.

It includes a rather nasty hack to override canvas size to 100% if it was
manually set to something different. This works around
three.js examples not rendering right, apparently a fullscreened
canvas just gets centered if it explicitly sets a size smaller
than the fullscreen dimensions.

BUG=389343
==========

to

==========
WebVR: Use content CVC size for compositor rendering
    
We can't currently resize the content window used by the
compositor rendering path, so we must use its dimensions
for the WebVR API reported render sizes.

It includes a rather nasty hack to override canvas size to 100% if
it was manually set to something different. This works around
three.js examples not rendering right, apparently a fullscreened
canvas just gets centered if it explicitly sets a size smaller
than the fullscreen dimensions.

BUG=389343
==========

[bajones, 2016-11-16 21:23:53]

LGTM with some nits.

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:610: DrawUiView(&head_pose,
world_elements, render_size_primary_, 0);
Would prefer that this used something like kPrimaryViewportOffset instead of
magic number 0

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:615: if (last_viewport <= 2) {
Not an issue with this patch, but I'm curious: Did you find that this was
actually a problem?

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:626: DrawUiView(nullptr,
head_locked_elements, render_size_headlocked_, 2);
Same magic number concern here: kHeadlockedViewportOffset?

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:837: webvr_needs_ui_ =
!secure_origin;
Doesn't look like this is actually used anywhere?

https://codereview.chromium.org/2508703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2508703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:456: if
(!m_fullscreenOrigWidth.isNull()) {
This is all so hacky anyway I don't know how much I care, but we risk resetting
this to an old value if the developer updates it while in VR presentation mode
(which would probably cause their content to break, but at that point it's on
their heads.) Maybe we should just check to see if it's currently set to "100%"
before restoring? Or we can just run with it like this because we intend for the
solution to be short-lived anyway.

[mthiesse, 2016-11-16 21:30:40]

mthiesse@chromium.org changed reviewers:
+ mthiesse@chromium.org

[mthiesse, 2016-11-16 21:30:42]

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:268: specs[specs.size() -
1].SetSize(1024, 1024);
magic numbers to constants with comment

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:268: specs[specs.size() -
1].SetSize(1024, 1024);
nit: specs.back()

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:525: if (html_interface_->GetMode()
== UiInterface::Mode::WEB_VR) {
Do you need to undo this when not in webVR?

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.h:191: bool webvr_needs_ui_ = false;
Seems unused, remove?

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.h:199: int webvr_width_ = 2048;
magic numbers to constants somewhere? Also why these exact values?

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right):

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:36: return gvr::Sizei{2048,
1024}; }
More magic numbers

https://codereview.chromium.org/2508703002/diff/40001/device/vr/android/gvr/g...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2508703002/diff/40001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_device.cc:68: left_eye->renderWidth = 1024;
Maybe move this 1024 value used all over the place to some shared header?

At least keep the comment here about why 1024.

https://codereview.chromium.org/2508703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2508703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:343: m_fullscreenOrigWidth =
inlineStyle->getPropertyValue(CSSPropertyWidth);
why are we changing CSS properties?

[mthiesse, 2016-11-16 21:31:35]

https://codereview.chromium.org/2508703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2508703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:343: m_fullscreenOrigWidth =
inlineStyle->getPropertyValue(CSSPropertyWidth);
On 2016/11/16 21:30:41, mthiesse wrote:
> why are we changing CSS properties?

Just saw your comment in the CL description. Put it here too.

[klausw, 2016-11-16 23:37:39]

Updated, PTAL.

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:268: specs[specs.size() -
1].SetSize(1024, 1024);
On 2016/11/16 21:30:41, mthiesse wrote:
> magic numbers to constants with comment

Done:

// Pixel dimensions and field of view for the head-locked content. This
// is currently sized to fit the WebVR "insecure transport" warnings,
// adjust it as needed if there is additional content.
static constexpr gvr::Sizei kHeadlockedBufferDimensions = {1024, 1024};
static constexpr gvr::Rectf kHeadlockedBufferFov = {20.f, 20.f, 20.f, 20.f};

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:268: specs[specs.size() -
1].SetSize(1024, 1024);
On 2016/11/16 21:30:41, mthiesse wrote:
> nit: specs.back()

Done.

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:525: if (html_interface_->GetMode()
== UiInterface::Mode::WEB_VR) {
On 2016/11/16 21:30:41, mthiesse wrote:
> Do you need to undo this when not in webVR?

I don't think so, I added this comment:

    // If needed, resize the primary buffer for use with WebVR.
    // For now, this is never reverted, at this point there is no
    // transition from WebVR mode to VrShell mode other than
    // destroying/recreating the VrShell instance. Revisit this if
    // that assumption changes.

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:610: DrawUiView(&head_pose,
world_elements, render_size_primary_, 0);
On 2016/11/16 21:23:52, bajones wrote:
> Would prefer that this used something like kPrimaryViewportOffset instead of
> magic number 0

Done, using kViewportListPrimaryOffset.

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:615: if (last_viewport <= 2) {
On 2016/11/16 21:23:52, bajones wrote:
> Not an issue with this patch, but I'm curious: Did you find that this was
> actually a problem?

I don't think it was a problem, SetToRecommendedBufferViewports gets
run every frame and apparently installs the pair of viewports we use
for the primary buffer.

I changed it to avoid the "++" which looked worrying:

    buffer_viewport_list_->SetBufferViewport(
        kViewportListHeadlockedOffset + GVR_LEFT_EYE,
        *headlocked_left_viewport_);
    buffer_viewport_list_->SetBufferViewport(
        kViewportListHeadlockedOffset + GVR_RIGHT_EYE,
        *headlocked_right_viewport_);

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:626: DrawUiView(nullptr,
head_locked_elements, render_size_headlocked_, 2);
On 2016/11/16 21:23:52, bajones wrote:
> Same magic number concern here: kHeadlockedViewportOffset?

Done, using kViewportListHeadlockedOffset

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:837: webvr_needs_ui_ =
!secure_origin;
On 2016/11/16 21:23:52, bajones wrote:
> Doesn't look like this is actually used anywhere?

Correct, removed here and in the .h file.

This used to be a shortcut to avoid UI rendering, I've now added a
"if (!world_elements.empty())" check in DrawVrShell instead along with
some minor rearrangement.

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right):

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:36: return gvr::Sizei{2048,
1024}; }
On 2016/11/16 21:30:41, mthiesse wrote:
> More magic numbers

Replaced with kFallbackRenderTargetSize.

https://codereview.chromium.org/2508703002/diff/40001/device/vr/android/gvr/g...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2508703002/diff/40001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_device.cc:68: left_eye->renderWidth = 1024;
On 2016/11/16 21:30:41, mthiesse wrote:
> Maybe move this 1024 value used all over the place to some shared header?
> 
> At least keep the comment here about why 1024.

I've reverted this part of the change to keep the original
assignment + comment.

I've also added a new kFallbackRenderTargetSize constant to make
this clearer.

https://codereview.chromium.org/2508703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2508703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:343: m_fullscreenOrigWidth =
inlineStyle->getPropertyValue(CSSPropertyWidth);
On 2016/11/16 21:31:35, mthiesse wrote:
> On 2016/11/16 21:30:41, mthiesse wrote:
> > why are we changing CSS properties?
> 
> Just saw your comment in the CL description. Put it here too.

Done, with a bit more detail:

      // THREE.js's VREffect sets explicit style.width/height on its rendering
      // canvas based on the non-fullscreen window dimensions, and it keeps
      // those unchanged when presenting. Unfortunately it appears that a
      // fullscreened canvas just gets centered if it has explicitly set a
      // size smaller than the fullscreen dimensions. Manually set size to
      // 100% in this case and restore it when exiting fullscreen. This is a
      // stopgap measure since THREE.js's usage appears legal according to the
      // WebVR API spec. This will no longer be necessary once we can get rid
      // of this fullscreen hack.

https://codereview.chromium.org/2508703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:456: if
(!m_fullscreenOrigWidth.isNull()) {
On 2016/11/16 21:23:52, bajones wrote:
> This is all so hacky anyway I don't know how much I care, but we risk
resetting
> this to an old value if the developer updates it while in VR presentation mode
> (which would probably cause their content to break, but at that point it's on
> their heads.) Maybe we should just check to see if it's currently set to
"100%"
> before restoring? Or we can just run with it like this because we intend for
the
> solution to be short-lived anyway.

I don't have strong feelings about this, but at this point I don't think making
the logic more complex has much benefit. Hopefully this code will all go away
soon.

[klausw, 2016-11-16 23:42:54]

The CQ bit was checked by klausw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 23:43:50]

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

[klausw, 2016-11-16 23:50:04]

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:525: if (html_interface_->GetMode()
== UiInterface::Mode::WEB_VR) {
On 2016/11/16 23:37:38, klausw wrote:
> On 2016/11/16 21:30:41, mthiesse wrote:
> > Do you need to undo this when not in webVR?
> 
> I don't think so, I added this comment:
> 
>     // If needed, resize the primary buffer for use with WebVR.
>     // For now, this is never reverted, at this point there is no
>     // transition from WebVR mode to VrShell mode other than
>     // destroying/recreating the VrShell instance. Revisit this if
>     // that assumption changes.

On second thought this isn't hard to fix to make it possible
to enter WebVR from VrShell mode and return. I'll send an update.

[klausw, 2016-11-17 00:07:42]

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:525: if (html_interface_->GetMode()
== UiInterface::Mode::WEB_VR) {
On 2016/11/16 23:50:04, klausw wrote:
> On 2016/11/16 23:37:38, klausw wrote:
> > On 2016/11/16 21:30:41, mthiesse wrote:
> > > Do you need to undo this when not in webVR?
> > 
> > I don't think so, I added this comment:
> > 
> >     // If needed, resize the primary buffer for use with WebVR.
> >     // For now, this is never reverted, at this point there is no
> >     // transition from WebVR mode to VrShell mode other than
> >     // destroying/recreating the VrShell instance. Revisit this if
> >     // that assumption changes.
> 
> On second thought this isn't hard to fix to make it possible
> to enter WebVR from VrShell mode and return. I'll send an update.

Done, though it doesn't currently seem to work right at ToT + this patch.

[klausw, 2016-11-17 01:22:57]

The CQ bit was checked by klausw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 01:24:07]

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

[bajones, 2016-11-17 01:26:05]

On 2016/11/17 00:07:42, klausw wrote:
>
https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell.cc (right):
> 
>
https://codereview.chromium.org/2508703002/diff/40001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell.cc:525: if
(html_interface_->GetMode()
> == UiInterface::Mode::WEB_VR) {
> On 2016/11/16 23:50:04, klausw wrote:
> > On 2016/11/16 23:37:38, klausw wrote:
> > > On 2016/11/16 21:30:41, mthiesse wrote:
> > > > Do you need to undo this when not in webVR?
> > > 
> > > I don't think so, I added this comment:
> > > 
> > >     // If needed, resize the primary buffer for use with WebVR.
> > >     // For now, this is never reverted, at this point there is no
> > >     // transition from WebVR mode to VrShell mode other than
> > >     // destroying/recreating the VrShell instance. Revisit this if
> > >     // that assumption changes.
> > 
> > On second thought this isn't hard to fix to make it possible
> > to enter WebVR from VrShell mode and return. I'll send an update.
> 
> Done, though it doesn't currently seem to work right at ToT + this patch.

Updates LGTM

[mthiesse, 2016-11-17 01:30:12]

lgtm

[klausw, 2016-11-17 02:04:35]

The CQ bit was unchecked by klausw@chromium.org

[klausw, 2016-11-17 02:04:36]

The CQ bit was checked by klausw@chromium.org

[commit-bot: I haz the power, 2016-11-17 02:05:57]

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-11-17 03:27:19]

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

[commit-bot: I haz the power, 2016-11-17 03:27:21]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)

[klausw, 2016-11-17 03:30:20]

The CQ bit was checked by klausw@chromium.org

[commit-bot: I haz the power, 2016-11-17 03:31:05]

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-11-17 04:25:31]

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

[commit-bot: I haz the power, 2016-11-17 04:25:33]

Failed to apply patch for chrome/browser/android/vr_shell/vr_shell.cc:
While running git apply --index -p1;
  error: patch failed: chrome/browser/android/vr_shell/vr_shell.cc:212
  error: chrome/browser/android/vr_shell/vr_shell.cc: patch does not apply

Patch:       chrome/browser/android/vr_shell/vr_shell.cc
Index: chrome/browser/android/vr_shell/vr_shell.cc
diff --git a/chrome/browser/android/vr_shell/vr_shell.cc
b/chrome/browser/android/vr_shell/vr_shell.cc
index
9a3bc41fd84365fc5a47f83754c2d5da81fdd369..580a08e8a4c90455d75f86bbb434751886856fc4
100644
--- a/chrome/browser/android/vr_shell/vr_shell.cc
+++ b/chrome/browser/android/vr_shell/vr_shell.cc
@@ -70,9 +70,24 @@ static constexpr float kReticleOffset = 0.99f;
 // adjust according to content quad placement.
 static constexpr float kReticleDistanceMultiplier = 1.5f;
 
+// GVR buffer indices for use with viewport->SetSourceBufferIndex
+// or frame.BindBuffer. We use one for world content (with reprojection)
+// including main VrShell and WebVR content plus world-space UI.
+// The headlocked buffer is for UI that should not use reprojection.
 static constexpr int kFramePrimaryBuffer = 0;
 static constexpr int kFrameHeadlockedBuffer = 1;
 
+// Pixel dimensions and field of view for the head-locked content. This
+// is currently sized to fit the WebVR "insecure transport" warnings,
+// adjust it as needed if there is additional content.
+static constexpr gvr::Sizei kHeadlockedBufferDimensions = {1024, 1024};
+static constexpr gvr::Rectf kHeadlockedBufferFov = {20.f, 20.f, 20.f, 20.f};
+
+// The GVR viewport list has two entries (left eye and right eye) for each
+// GVR buffer.
+static constexpr int kViewportListPrimaryOffset = 0;
+static constexpr int kViewportListHeadlockedOffset = 2;
+
 vr_shell::VrShell* g_instance;
 
 static const char kVrShellUIURL[] = "chrome://vr-shell-ui";
@@ -212,13 +227,10 @@ void VrShell::GvrInit(JNIEnv* env,
   base::AutoLock lock(gvr_init_lock_);
   gvr_api_ =
      
gvr::GvrApi::WrapNonOwned(reinterpret_cast<gvr_context*>(native_gvr_api));
-
-  if (delegate_) {
-    main_thread_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&device::GvrDeviceProvider::OnGvrDelegateReady,
-                              delegate_->GetDeviceProvider(),
-                              weak_ptr_factory_.GetWeakPtr()));
-  }
+  // TODO(klausw,crbug.com/655728): should report OnGvrDelegateReady here once
+  // we switch to using a WebVR render surface. We currently need to wait for
+  // the compositor window's size to be known first. See also
+  // ContentSurfaceChanged.
   controller_.reset(
       new VrController(reinterpret_cast<gvr_context*>(native_gvr_api)));
   content_input_manager_ = new VrInputManager(main_contents_);
@@ -251,30 +263,53 @@ void VrShell::InitializeGl(JNIEnv* env,
   content_texture_id_ = content_texture_handle;
   ui_texture_id_ = ui_texture_handle;
 
+  // While WebVR is going through the compositor path, it shares
+  // the same texture ID. This will change once it gets its own
+  // surface, but store it separately to avoid future confusion.
+  // TODO(klausw,crbug.com/655728): remove this.
+  webvr_texture_id_ = content_texture_id_;
+
   gvr_api_->InitializeGl();
   std::vector<gvr::BufferSpec> specs;
+  // For kFramePrimaryBuffer (primary VrShell and WebVR content)
   specs.push_back(gvr_api_->CreateBufferSpec());
-  render_size_ = specs[0].GetSize();
+  render_size_primary_ = specs[kFramePrimaryBuffer].GetSize();
+  render_size_primary_vrshell_ = render_size_primary_;
 
-  // For WebVR content
+  // For kFrameHeadlockedBuffer (for WebVR insecure content warning).
+  // Set this up at fixed resolution, the (smaller) FOV gets set below.
   specs.push_back(gvr_api_->CreateBufferSpec());
+  specs.back().SetSize(kHeadlockedBufferDimensions);
+  render_size_headlocked_ = specs[kFrameHeadlockedBuffer].GetSize();
 
   swap_chain_.reset(new gvr::SwapChain(gvr_api_->CreateSwapChain(specs)));
 
   vr_shell_renderer_.reset(new VrShellRenderer());
+
+  // Allocate a buffer viewport for use in UI drawing. This isn't
+  // initialized at this point, it'll be set from other viewport list
+  // entries as needed.
+  buffer_viewport_.reset(
+      new gvr::BufferViewport(gvr_api_->CreateBufferViewport()));
+
+  // Set up main content viewports. The list has two elements, 0=left
+  // eye and 1=right eye.
   buffer_viewport_list_.reset(
       new gvr::BufferViewportList(gvr_api_->CreateEmptyBufferViewportList()));
   buffer_viewport_list_->SetToRecommendedBufferViewports();
 
-  buffer_viewport_.reset(
-      new gvr::BufferViewport(gvr_api_->CreateBufferViewport()));
-
+  // Set up head-locked UI viewports, these will be elements 2=left eye
+  // and 3=right eye. For now, use a hardcoded 20-degree-from-center FOV
+  // frustum to reduce rendering cost for this overlay. This fits the
+  // current content, but will need to be adjusted once there's more dynamic
+  // head-locked content that could be larger.
   headlocked_left_viewport_.reset(
       new gvr::BufferViewport(gvr_api_->CreateBufferViewport()));
   buffer_viewport_list_->GetBufferViewport(GVR_LEFT_EYE,
                                            headlocked_left_viewport_.get());
   headlocked_left_viewport_->SetSourceBufferIndex(kFrameHeadlockedBuffer);
   headlocked_left_viewport_->SetReprojection(GVR_REPROJECTION_NONE);
+  headlocked_left_viewport_->SetSourceFov(kHeadlockedBufferFov);
 
   headlocked_right_viewport_.reset(
       new gvr::BufferViewport(gvr_api_->CreateBufferViewport()));
@@ -282,7 +317,10 @@ void VrShell::InitializeGl(JNIEnv* env,
                                            headlocked_right_viewport_.get());
   headlocked_right_viewport_->SetSourceBufferIndex(kFrameHeadlockedBuffer);
   headlocked_right_viewport_->SetReprojection(GVR_REPROJECTION_NONE);
+  headlocked_right_viewport_->SetSourceFov(kHeadlockedBufferFov);
 
+  // Save copies of the first two viewport items for use by WebVR, it
+  // sets its own UV bounds.
   webvr_left_viewport_.reset(
       new gvr::BufferViewport(gvr_api_->CreateBufferViewport()));
   buffer_viewport_list_->GetBufferViewport(GVR_LEFT_EYE,
@@ -496,8 +534,24 @@ uint32_t GetPixelEncodedPoseIndex() {
 }
 
 void VrShell::DrawFrame(JNIEnv* env, const JavaParamRef<jobject>& obj) {
+  // Reset the viewport list to just the pair of viewports for the
+  // primary buffer each frame. Head-locked viewports get added by
+  // DrawVrShell if needed.
   buffer_viewport_list_->SetToRecommendedBufferViewports();
 
+  if (html_interface_->GetMode() == UiInterface::Mode::WEB_VR) {
+    // If needed, resize the primary buffer for use with WebVR.
+    if (render_size_primary_ != render_size_primary_webvr_) {
+      render_size_primary_ = render_size_primary_webvr_;
+      swap_chain_->ResizeBuffer(kFramePrimaryBuffer, render_size_primary_);
+    }
+  } else {
+    if (render_size_primary_ != render_size_primary_vrshell_) {
+      render_size_primary_ = render_size_primary_vrshell_;
+      swap_chain_->ResizeBuffer(kFramePrimaryBuffer, render_size_primary_);
+    }
+  }
+
   gvr::Frame frame = swap_chain_->AcquireFrame();
   gvr::ClockTimePoint target_time = gvr::GvrApi::GetTimePointNow();
   target_time.monotonic_system_time_nanos += kPredictionTimeWithoutVsyncNanos;
@@ -534,8 +588,8 @@ void VrShell::DrawFrame(JNIEnv* env, const
JavaParamRef<jobject>& obj) {
     // buffering in the compositor and SurfaceTexture, we read the pose number
     // from a corner pixel. There's no point in doing this for legacy
     // distortion rendering since that doesn't need a pose, and reading back
-    // pixels is an expensive operation. TODO(klausw): stop doing this once we
-    // have working no-compositor rendering for WebVR.
+    // pixels is an expensive operation. TODO(klausw,crbug.com/655728): stop
+    // doing this once we have working no-compositor rendering for WebVR.
     if (gvr_api_->GetAsyncReprojectionEnabled()) {
       uint32_t webvr_pose_frame = GetPixelEncodedPoseIndex();
       // If we don't get a valid frame ID back we shouldn't attempt to
reproject
@@ -572,39 +626,69 @@ void VrShell::DrawVrShell(const gvr::Mat4f& head_pose,
     }
   }
 
-  bool not_web_vr = html_interface_->GetMode() != UiInterface::Mode::WEB_VR;
+  if (html_interface_->GetMode() == UiInterface::Mode::WEB_VR) {
+    // WebVR is incompatible with 3D world compositing since the
+    // depth buffer was already populated with unknown scaling - the
+    // WebVR app has full control over zNear/zFar. Just leave the
+    // existing content in place in the primary buffer without
+    // clearing. Currently, there aren't any world elements in WebVR
+    // mode, this will need further testing if those get added
+    // later.
+  } else {
+    // Non-WebVR mode, enable depth testing and clear the primary buffers.
+    glEnable(GL_CULL_FACE);
+    glEnable(GL_DEPTH_TEST);
+    glDepthMask(GL_TRUE);
 
-  glEnable(GL_CULL_FACE);
-  glEnable(GL_DEPTH_TEST);
-  glDepthMask(GL_TRUE);
-
-  if (not_web_vr) {
     glClearColor(0.1f, 0.1f, 0.1f, 1.0f);
     glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
   }
 
-  DrawUiView(&head_pose, world_elements);
+  if (!world_elements.empty()) {
+    DrawUiView(&head_pose, world_elements, render_size_primary_,
+               kViewportListPrimaryOffset);
+  }
 
   if (!head_locked_elements.empty()) {
-    // Switch to head-locked viewports.
-    size_t last_viewport = buffer_viewport_list_->GetSize();
-    buffer_viewport_list_->SetBufferViewport(last_viewport++,
+    // Add head-locked viewports. The list gets reset to just
+    // the recommended viewports (for the primary buffer) each frame.
+    buffer_viewport_list_->SetBufferViewport(
+        kViewportListHeadlockedOffset + GVR_LEFT_EYE,
         *headlocked_left_viewport_);
-    buffer_viewport_list_->SetBufferViewport(last_viewport++,
+    buffer_viewport_list_->SetBufferViewport(
+        kViewportListHeadlockedOffset + GVR_RIGHT_EYE,
         *…
(message too large)

[klausw, 2016-11-17 05:06:23]

Description was changed from

==========
WebVR: Use content CVC size for compositor rendering
    
We can't currently resize the content window used by the
compositor rendering path, so we must use its dimensions
for the WebVR API reported render sizes.

It includes a rather nasty hack to override canvas size to 100% if
it was manually set to something different. This works around
three.js examples not rendering right, apparently a fullscreened
canvas just gets centered if it explicitly sets a size smaller
than the fullscreen dimensions.

BUG=389343
==========

to

==========
WebVR: Use content CVC size for compositor rendering
    
We can't currently resize the content window used by the
compositor rendering path, so we must use its dimensions
for the WebVR API reported render sizes.

It includes a rather nasty hack to override canvas size to 100% if
it was manually set to something different. This works around
three.js examples not rendering right, apparently a fullscreened
canvas just gets centered if it explicitly sets a size smaller
than the fullscreen dimensions.

BUG=389343,655722
==========

[klausw, 2016-11-17 05:07:25]

The CQ bit was checked by klausw@chromium.org

[klausw, 2016-11-17 05:07:25]

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/2508703002/#ps140001
(title: "Rebase, use more appropriate crbug/655722 for TODOS")

[commit-bot: I haz the power, 2016-11-17 05:08:04]

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-11-17 08:28:19]

Description was changed from

==========
WebVR: Use content CVC size for compositor rendering
    
We can't currently resize the content window used by the
compositor rendering path, so we must use its dimensions
for the WebVR API reported render sizes.

It includes a rather nasty hack to override canvas size to 100% if
it was manually set to something different. This works around
three.js examples not rendering right, apparently a fullscreened
canvas just gets centered if it explicitly sets a size smaller
than the fullscreen dimensions.

BUG=389343,655722
==========

to

==========
WebVR: Use content CVC size for compositor rendering
    
We can't currently resize the content window used by the
compositor rendering path, so we must use its dimensions
for the WebVR API reported render sizes.

It includes a rather nasty hack to override canvas size to 100% if
it was manually set to something different. This works around
three.js examples not rendering right, apparently a fullscreened
canvas just gets centered if it explicitly sets a size smaller
than the fullscreen dimensions.

BUG=389343,655722
==========

[commit-bot: I haz the power, 2016-11-17 08:28:21]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-17 08:31:11]

Description was changed from

==========
WebVR: Use content CVC size for compositor rendering
    
We can't currently resize the content window used by the
compositor rendering path, so we must use its dimensions
for the WebVR API reported render sizes.

It includes a rather nasty hack to override canvas size to 100% if
it was manually set to something different. This works around
three.js examples not rendering right, apparently a fullscreened
canvas just gets centered if it explicitly sets a size smaller
than the fullscreen dimensions.

BUG=389343,655722
==========

to

==========
WebVR: Use content CVC size for compositor rendering

We can't currently resize the content window used by the
compositor rendering path, so we must use its dimensions
for the WebVR API reported render sizes.

It includes a rather nasty hack to override canvas size to 100% if
it was manually set to something different. This works around
three.js examples not rendering right, apparently a fullscreened
canvas just gets centered if it explicitly sets a size smaller
than the fullscreen dimensions.

BUG=389343,655722

Committed: https://crrev.com/4026d565687ddbc1f67c4cc04887c339b7a03685
Cr-Commit-Position: refs/heads/master@{#432811}
==========

[commit-bot: I haz the power, 2016-11-17 08:31:12]

Patchset 8 (id:??) landed as
https://crrev.com/4026d565687ddbc1f67c4cc04887c339b7a03685
Cr-Commit-Position: refs/heads/master@{#432811}