WebVR: Add sanity checks for decoded pose index values

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 90456fd60192c92960f1e2210719bb461e36cc6a..26cdca892a17059783d3d227dcc01fe88cccdc04 100644
--- a/chrome/browser/android/vr_shell/vr_shell.cc
+++ b/chrome/browser/android/vr_shell/vr_shell.cc
@@ -542,6 +542,7 @@ void VrShell::SendEventsToTarget(VrInputManager* input_target,
 }
 
 void VrShell::SetGvrPoseForWebVr(const gvr::Mat4f& pose, uint32_t pose_num) {
+  webvr_pose_index_max_ = pose_num;
   webvr_head_pose_[pose_num % kPoseRingBufferSize] = pose;
 }
 
@@ -559,6 +560,19 @@ uint32_t GetPixelEncodedPoseIndex() {
   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
@@ -623,15 +637,18 @@ void VrShell::DrawFrame(JNIEnv* env, const JavaParamRef<jobject>& obj) {
     // 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);
       }
     }
   }