Encode frame number in pixel data for pose sync

third_party/WebKit/Source/modules/vr/VRDisplay.cpp

Index: third_party/WebKit/Source/modules/vr/VRDisplay.cpp
diff --git a/third_party/WebKit/Source/modules/vr/VRDisplay.cpp b/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
index b5ad05be54efe74136e4e23bea508c19a7bb62c2..867ffbcc09545f98240a7e3478915eeccc126e96 100644
--- a/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
+++ b/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
@@ -121,6 +121,10 @@ VRPose* VRDisplay::getPose()
 void VRDisplay::updatePose()
 {
     if (m_canUpdateFramePose) {
+        // Increment pose counter. This must stay in sync with the
+        // frameNumStarted_ counter in device/vr/android/gvr/gvr_device.cc.
+        ++m_poseNum;

[bajones, 2016/10/02 20:55:23]

This feels very fragile. Let's add a poseId to the Mojo VRPose structure and
pass it down that way instead. Oculus has alreday asked for that.

[klausw, 2016/10/02 21:25:50]

I agree, that this would be much cleaner. I'll look into adding that. Want to
add a timestamp while we're at it, or save that for later? I think it would be
helpful for performance statistics.

[klausw, 2016/10/04 00:40:41]

Done. I saw it has a timestamp already internally, that's just not exposed
through the JS-visible VRPose object.

+        // LOG << "klausw:VRDisplay::updatePose " << m_poseNum;
         m_framePose = controller()->getPose(m_displayId);
         if (m_isPresenting)
             m_canUpdateFramePose = false;
@@ -346,6 +350,32 @@ HeapVector<VRLayer> VRDisplay::getLayers()
 
 void VRDisplay::submitFrame()
 {
+    // Write the frame number for the pose used into a bottom left pixel block.
+    // It is read by chrome/browser/android/vr_shell/vr_shell.cc to associate
+    // the correct corresponding pose for submission.
+    auto ctx = toWebGLRenderingContextBase(m_layer.source()->renderingContext());

[bajones, 2016/10/02 20:55:23]

Probably just want to cache this on calls to requestPresent. I know we'll need
it for other operations in the future.

[klausw, 2016/10/02 21:25:50]

Sounds good.

[klausw, 2016/10/04 00:40:41]

Done, saving the underlying GL context since that's what I'm using.

+    int frame = m_poseNum;
+    ctx->scissor(0, 0, 4, 4);

[bajones, 2016/10/02 20:55:23]

why 4x4? we're only reading one pixel

[klausw, 2016/10/02 21:25:50]

I need to investigate more. I had tried 2x2 at some point and ended up with
wrong values on reading the pixel back, it's possible there's some blurring
going on at texture edges.

In practical terms it doesn't matter since this area is blacked out by the
vignette effect at the eye view border.

[klausw, 2016/10/04 00:40:41]

I tested, smallest that worked was (0, 0, 1, 3). I think this is due to the
WebGL texture size not matching the output texture size and that this can result
in interpolated colors in texture sampling. Added a comment to explain what's
going on, though I'm not totally sure if I'm understanding it right.

+    ctx->colorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
+    ctx->clearColor(
+        (frame & 255) / 255.0f,
+        ((frame >> 8) & 255) / 255.0f,
+        ((frame >> 16) & 255) / 255.0f,
+        1.0f);
+    ctx->enable(GL_SCISSOR_TEST);
+    ctx->clear(GL_COLOR_BUFFER_BIT);
+
+    // TODO(klausw): restore the previous parameters here.
+    // WebGLRenderingContextBase's getParameter is based on ScriptState and
+    // ScriptValue, I don't know how those work. For now, just restore some
+    // presumably-sane values.
+    //
+    // Alternatively, use a GL context from ctx->drawingBuffer()->contextGL(),
+    // but drawingBuffer() is a protected method.
+    ctx->disable(GL_SCISSOR_TEST);

[bajones, 2016/10/02 20:55:23]

This is definitely not going to fly, as it will break various WebGL apps that
use values other than this. For the script state bit, it's probably best to dig
one level deeper and get the WebGraphicsContext3D that the
WebGLRenderingContextBase forwards calls to.

[klausw, 2016/10/02 21:25:50]

I know it's not acceptable, hence the TODO. I just wasn't sure how best to
proceed since the ScriptState/ScriptValue abstraction seemed unhelpful for this
use case, and the underlying DrawingBuffer was a protected interface which would
need a change to the WebAPI internal interface. I'll look into the
WebRenderingContext3D to see if that helps. Would it be very controversial to
make a public accessor to get the drawing buffer if that turns out to be
necessary?

[klausw, 2016/10/03 16:50:17]

Good news, WebGLRenderingContextBase has a public contextGL() accessor which
bypasses the protected drawingBuffer() accessor. Assuming that works, I'll
switch the code to use that.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webgl/...

gpu::gles2::GLES2Interface* contextGL() const {
    DrawingBuffer* d = drawingBuffer();
    if (!d)
      return nullptr;
    return d->contextGL();
  }

[klausw, 2016/10/04 00:40:41]

This seems to work now, though there's some slightly subtle magic going on where
I'm relying on the rendering context to auto-restore values via
restoreStateAfterClear(). I added comments to explain what's going on.

+    ctx->clearColor(0, 0, 0, 1.0f);
+    // LOG << "klausw:VRDisplay::submitFrame " << frameNum;
+
     controller()->submitFrame(m_displayId, m_framePose.Clone());
     m_canUpdateFramePose = true;
 }