WebVR: Add sanity checks for decoded pose index values

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 "base/metrics/histogram_macros.h"
 #include "chrome/browser/android/vr_shell/ui_elements.h"
 #include "chrome/browser/android/vr_shell/ui_interface.h"
 #include "chrome/browser/android/vr_shell/ui_scene.h"
@@ skipping 524 matching lines @@
           (base::TimeTicks::Now() - base::TimeTicks()).InSecondsF();
     }
     gesture->type = WebInputEvent::GestureTapDown;
     gesture->data.tapDown.width = pixel_x;
     gesture->data.tapDown.height = pixel_y;
     current_input_target_->ProcessUpdatedGesture(*gesture.get());
   }
 }
 
 void VrShell::SetGvrPoseForWebVr(const gvr::Mat4f& pose, uint32_t pose_num) {
+  webvr_pose_index_max_ = pose_num;
   webvr_head_pose_[pose_num % kPoseRingBufferSize] = pose;
 }
 
 uint32_t GetPixelEncodedPoseIndex() {
   TRACE_EVENT0("gpu", "VrShell::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 framebuffer 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);
   return pixels[0] | (pixels[1] << 8) | (pixels[2] << 16);
 }
 
+bool VrShell::WebVrPoseIsValid(uint32_t pose_frame) {

[mthiesse, 2016/12/01 15:39:11]

This doesn't guarantee the pose is valid; we should fill the top 16(?) bits with
a signature of some sort to check if the frame is valid.

[klausw, 2016/12/01 16:10:18]

Will do, in earlier versions I did have a magic number but had removed that for
simplification at some point. I'll add it back. This would also fix the current
issue of missing reprojection for a few frames every 3.2 days when the current
24bit index wraps back to zero.

I think a 16-bit constant magic number should be fairly reliable, and I think
it's a bit simpler and more predictable. A checksum would need to be carefully
chosen to ensure that all-black or all-white pixels don't read as valid, and in
any case there's a 1-in-65536 chance that a completely random pixel would be
considered valid.

In practice I don't think I've ever seen wrong pixels other than pure white or
black.

[klausw, 2016/12/01 19:50:41]

Added a 16-bit magic number. Currently hardcoded since there's no shared header
file with blink's VRDisplay.cpp, but this will all go away soon anyway I hope.

+  // Ignore pose numbers that are zero or larger than any known pose
+  // index. This helps avoid glitches from garbage data in the render
+  // buffer that can appear during initialization or resizing. These
+  // often appear as flashes of all-black or all-white pixels.
+  if (pose_frame == 0 || pose_frame > webvr_pose_index_max_) {
+    VLOG(1) << "WebVR: reject decoded pose index " << pose_frame <<
+        ", highest seen is " << webvr_pose_index_max_;
+    return false;
+  }
+  return true;
+}
+
 void VrShell::DrawFrame(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   TRACE_EVENT0("gpu", "VrShell::DrawFrame");
   // 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_) {
@@ skipping 44 matching lines @@
 
     // 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,crbug.com/655722): 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
-      // by an invalid matrix, so turn of reprojection instead.
-      if (webvr_pose_frame == 0) {
-        webvr_left_viewport_->SetReprojection(GVR_REPROJECTION_NONE);
-        webvr_right_viewport_->SetReprojection(GVR_REPROJECTION_NONE);
-      } else {
+      if (WebVrPoseIsValid(webvr_pose_frame)) {
+        // We have a valid pose, use it for reprojection.
         webvr_left_viewport_->SetReprojection(GVR_REPROJECTION_FULL);
         webvr_right_viewport_->SetReprojection(GVR_REPROJECTION_FULL);
         head_pose = webvr_head_pose_[webvr_pose_frame % kPoseRingBufferSize];

[mthiesse, 2016/12/01 15:39:11]

You should make webvr_head_pose_ a vector of unique pointers, and reset each
pose after we use it. If the pose we're about to set is null, we've clearly
received an invalid pose frame index.

[klausw, 2016/12/01 16:10:18]

Poses do get reused unfortunately. The compositor resends a duplicate copy of an
old frame if the new frame hasn't finished rendering yet at vsync, usually
followed by a dropped frame.

I don't think we can safely skip these frames, we only know it's a duplicate
after binding the GVR framebuffer, rendering it, and reading the pixel, and then
we've done pretty much all the work already. Not sure if it's legal to discard
an acquired frame without submitting it.

I'll add a comment along these lines to the source. I had actually implemented a
parallel pose_valid bool vector, then removed it again when I realized it
doesn't work.

[klausw, 2016/12/01 19:50:41]

Done, I've added a "valid" vector that's set to false on initialization (in
InitializeGl), but still allowing pose reuse. Also added a comment.

+      } else {
+        // If we don't get a valid frame ID back we shouldn't attempt
+        // to reproject by an invalid matrix, so turn off reprojection
+        // instead. Invalid poses can permanently break reprojection
+        // for this GVR instance: http://crbug.com/667327
+        webvr_left_viewport_->SetReprojection(GVR_REPROJECTION_NONE);
+        webvr_right_viewport_->SetReprojection(GVR_REPROJECTION_NONE);
       }
     }
   }
 
   DrawVrShell(head_pose, frame);
 
   frame.Unbind();
   frame.Submit(*buffer_viewport_list_, head_pose);
 }
 
@@ skipping 408 matching lines @@
            const JavaParamRef<jobject>& ui_web_contents,
            jlong ui_window_android) {
   return reinterpret_cast<intptr_t>(new VrShell(
       env, obj, content::WebContents::FromJavaWebContents(content_web_contents),
       reinterpret_cast<ui::WindowAndroid*>(content_window_android),
       content::WebContents::FromJavaWebContents(ui_web_contents),
       reinterpret_cast<ui::WindowAndroid*>(ui_window_android)));
 }
 
 }  // namespace vr_shell