Encode frame number in pixel data for pose sync

chrome/browser/android/vr_shell/vr_shell.h

 // 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.
 
 #ifndef CHROME_BROWSER_ANDROID_VR_SHELL_VR_SHELL_H_
 #define CHROME_BROWSER_ANDROID_VR_SHELL_VR_SHELL_H_
 
 #include <jni.h>
 #include <memory>
 #include <vector>
@@ skipping 55 matching lines @@
   UiScene* GetScene();
   void OnDomContentsLoaded();
   void SetUiTextureSize(int width, int height);
 
   // device::GvrDelegate implementation
   void SetWebVRSecureOrigin(bool secure_origin) override;
   void SubmitWebVRFrame() override;
   void UpdateWebVRTextureBounds(
       int eye, float left, float top, float width, float height) override;
   gvr::GvrApi* gvr_api() override;
+  void SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t pose_num) override;

[mthiesse, 2016/10/04 21:19:16]

This function name feels wrong...

How about SetPoseForWebVrFrame(pose, frame_index).

[klausw, 2016/10/04 23:56:42]

I think it does need "GvrPose" in the name since that's distinct from WebVR's
VRPose. Submitting frames needs a GVR pose.

 
   void ContentSurfaceChanged(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& object,
       jint width,
       jint height,
       const base::android::JavaParamRef<jobject>& surface);
   void UiSurfaceChanged(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& object,
@@ skipping 63 matching lines @@
   bool controller_active_ = false;
 
   gvr::Vec3f target_point_;
   const ContentRectangle* target_element_ = nullptr;
   int ui_tex_width_ = 0;
   int ui_tex_height_ = 0;
 
   bool webvr_mode_ = false;
   bool webvr_secure_origin_ = false;
   int64_t webvr_warning_end_nanos_ = 0;
+  // The pose queue size must be a power of two to avoid glitches when
+  // the pose counter wraps around. Should be large enough to handle
+  // the current backlog of poses which is 2-3 frames.
+  static constexpr int POSE_QUEUE_SIZE = 8;

[dcheng, 2016/10/04 23:17:53]

Nit: Chromium-style naming for constants is kPoseQueueSize

[klausw, 2016/10/04 23:56:42]

Done, using kPoseRingBufferSize since it's not actually a queue.

+  gvr::Mat4f webvr_head_pose_[POSE_QUEUE_SIZE];

[mthiesse, 2016/10/04 21:19:16]

Mild preference for std::array, or std::vector over c-style arrays. Smaller
stack sizes are also generally preferable.

[dcheng, 2016/10/04 23:17:53]

It looks like this isn't explicitly initialized, so I think it'll get default
initialized. In this case, I think that means that there will be arbitrary
values in here. Do we want to zero-initialize it or initialize it to something
sensible, since a renderer can specify arbitrary pose numbers?

[klausw, 2016/10/04 23:56:42]

Done, using std::vector.

[klausw, 2016/10/04 23:56:42]

Good point, added explicit initialization to identity matrices.

+  int32_t webvr_newest_pose_num_ = 0;

[mthiesse, 2016/10/04 21:19:16]

Remove unused variable.

[klausw, 2016/10/04 23:56:42]

Done.

 
   base::WeakPtrFactory<VrShell> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(VrShell);
 };
 
 bool RegisterVrShell(JNIEnv* env);
 
 }  // namespace vr_shell
 
 #endif  // CHROME_BROWSER_ANDROID_VR_SHELL_VR_SHELL_H_