Encode frame number in pixel data for pose sync

chrome/browser/android/vr_shell/vr_shell.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/vr_shell/vr_shell.h"
 
 #include <thread>
 
 #include "chrome/browser/android/vr_shell/ui_scene.h"
 #include "chrome/browser/android/vr_shell/vr_compositor.h"
@@ skipping 138 matching lines @@
   float screen_height = kScreenHeightRatio * desktop_height_;
   std::unique_ptr<ContentRectangle> rect(new ContentRectangle());
   rect->id = kBrowserUiElementId;
   rect->size = {screen_width, screen_height, 1.0f};
   rect->translation = kDesktopPositionDefault;
   scene_.AddUiElement(rect);
 
   desktop_plane_ = scene_.GetUiElementById(kBrowserUiElementId);
 
   LoadUIContent();
+
+  webvr_head_pose_.resize(kPoseRingBufferSize);

[dcheng, 2016/10/05 07:34:44]

resize() takes a second argument, so I think this can just say:

resize(kPoseRingBufferSize, kIdentity)

[klausw, 2016/10/05 17:02:10]

Done.

+  const gvr::Mat4f identity = {{{1.0f, 0.0f, 0.0f, 0.0f},

[dcheng, 2016/10/05 07:34:44]

Nit: prefer constexpr to const (also prefix with k)

[mthiesse, 2016/10/05 15:40:01]

Might as well re-use SetIdentityM in vr_math.cc

Actually, maybe we should just have a constexpr identity matrix in vr_math.cc,
then we can replace this and calls to SetIdentityM with matrix = kIdentityMatrix

[klausw, 2016/10/05 17:02:10]

Acknowledged, but it's no longer const (or constexpr) due to using SetIdentityM
on it.

[klausw, 2016/10/05 17:02:10]

I changed it to use SetIdentityM. Somehow I was under the mistaken impression
that a GVR pose matrix was a different type from what vr_math uses, but both are
just gvr::Mat4f.

I'm a bit hesitant about a shared kIdentityMatrix, will someone manage to cast
away the constness and apply an in-place transform to it? That would lead to
extremely nasty bugs if it's a small transform. I'd feel a bit more comfortable
with a GetIdentityM() function so that you could use it in expressions or
initialization without direct sharing. Or is that overly paranoid? In any case,
I think this should be a separate refactor since it gets used in quite a few
places.

[dcheng, 2016/10/05 17:35:21]

Casting away const-ness would be a Bad Thing™ to do and mutating a const object
is undefined behavior. We shouldn't be doing that: if there is something in
Chrome doing that, we should fix it =)

+                                {0.0f, 1.0f, 0.0f, 0.0f},
+                                {0.0f, 0.0f, 1.0f, 0.0f},
+                                {0.0f, 0.0f, 0.0f, 1.0f}}};
+  for (int i = 0; i < kPoseRingBufferSize; ++i) {
+    webvr_head_pose_[i] = identity;
+  }
 }
 
 void VrShell::UpdateCompositorLayers(JNIEnv* env,
                                      const JavaParamRef<jobject>& obj) {
   content_compositor_->SetLayer(content_cvc_);
   ui_compositor_->SetLayer(ui_cvc_);
 }
 
 void VrShell::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   delete this;
@@ skipping 100 matching lines @@
       bool is_inside = x >= 0.0f && x < 1.0f && y >= 0.0f && y < 1.0f;
       if (is_inside) {
         closest_element = distance_to_plane;
         target_point_ = plane_intersection_point;
         target_element_ = &plane;
       }
     }
   }
 }
 
+void VrShell::SetGvrPoseForWebVr(const gvr::Mat4f& pose, uint32_t pose_num) {
+  webvr_head_pose_[pose_num % kPoseRingBufferSize] = pose;
+}
+
+uint32_t GetPixelEncodedPoseIndex() {
+  // Read the pose index encoded in a bottom left pixel as color values.
+  // See also third_party/WebKit/Source/modules/vr/VRDisplay.cpp which
+  // encodes the pose index, and device/vr/android/gvr/gvr_device.cc
+  // which tracks poses.
+  uint8_t pixels[4];
+  // Assume we're reading from the frambebuffer we just wrote to.
+  // That's true currently, we may need to use glReadBuffer(GL_BACK)
+  // or equivalent if the rendering setup changes in the future.
+  glReadPixels(0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, pixels);

[mthiesse, 2016/10/05 13:14:06]

After you start presenting, aren't the poses and frames delivered in order, and
with a 1:1  mapping? Do you ever expect GetPixelEncodedPoseIndex to not return 1
higher than it previously returned outside of requestPresent/whatever ends
presentation? If not, could you do the pixel read once to synchronize after
requestPresent(), and just assume increasing IDs after?

[mthiesse, 2016/10/05 14:58:42]

Acknowledged.

+  return pixels[0] | (pixels[1] << 8) | (pixels[2] << 16);
+}
+
 void VrShell::DrawFrame(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   buffer_viewport_list_->SetToRecommendedBufferViewports();
 
   gvr::Frame frame = swap_chain_->AcquireFrame();
   gvr::ClockTimePoint target_time = gvr::GvrApi::GetTimePointNow();
   target_time.monotonic_system_time_nanos += kPredictionTimeWithoutVsyncNanos;
   gvr::Mat4f head_pose = gvr_api_->GetHeadSpaceFromStartSpaceRotation(target_time);
   head_pose = gvr_api_->ApplyNeckModel(head_pose, 1.0f);
 
   // Bind back to the default framebuffer.
   frame.BindBuffer(0);
 
   if (webvr_mode_) {
     DrawWebVr();
     if (!webvr_secure_origin_) {
       DrawWebVrOverlay(target_time.monotonic_system_time_nanos);
     }
+
+    // When using async reprojection, we need to know which pose was used in
+    // the WebVR app for drawing this frame. Due to unknown amounts of
+    // 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.
+    if (gvr_api_->GetAsyncReprojectionEnabled()) {
+      int32_t webvr_pose_frame = GetPixelEncodedPoseIndex();

[dcheng, 2016/10/05 07:34:44]

Nit: match types with uint32_t here as well.

[klausw, 2016/10/05 17:02:10]

Done. I'm surprised the compiler didn't complain about this one, I think it
would have caused out-of-bounds array access after half a year of continuous VR
presentation due to the mod result going negative.

[dcheng, 2016/10/05 17:35:21]

We suppress a lot of unsigned / signed conversion warnings.

+      head_pose = webvr_head_pose_[webvr_pose_frame % kPoseRingBufferSize];
+    }
   } else {
     DrawVrShell(head_pose);
   }
 
   frame.Unbind();
   frame.Submit(*buffer_viewport_list_, head_pose);
 }
 
 void VrShell::DrawVrShell(const gvr::Mat4f& head_pose) {
   float screen_tilt = desktop_screen_tilt_ * M_PI / 180.0f;
@@ skipping 368 matching lines @@
       content::WebContents::FromJavaWebContents(content_web_contents));
   content::ContentViewCore* ui_core = content::ContentViewCore::FromWebContents(
       content::WebContents::FromJavaWebContents(ui_web_contents));
   return reinterpret_cast<intptr_t>(new VrShell(
       env, obj, c_core,
       reinterpret_cast<ui::WindowAndroid*>(content_window_android), ui_core,
       reinterpret_cast<ui::WindowAndroid*>(ui_window_android)));
 }
 
 }  // namespace vr_shell