Refactor AR frame transport and MailboxToSurfaceBridge use

Instead of partially initializing this in arcore_device, just pass the initial
object and let the GL thread take care of setting it up.

This also makes it possible to simplify mailbox_to_surface_bridge, it can now
assume that all post-creation methods are used on a single GL thread.

Add logic to enforce that at most one frame is allowed to be in "processing"
state, deferring the next frame in case of attempted overlap. This matches
the current logic used in GVR's immersive-vr mode.

Also update some stale comments and make simplifications that are now possible
due to the new lifecycle.

Bug: 1012972, 836524
Change-Id: I9b85bb6dd261c0c2fa41f443222e34b76a64c5c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1854506
Commit-Queue: Klaus Weidner <klausw@chromium.org>
Reviewed-by: Piotr Bialecki <bialpio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705317}

chrome/browser/android/vr/arcore_device/ar_image_transport.cc

@@ -26,34 +26,37 @@ namespace device {
 ArImageTransport::ArImageTransport(
     std::unique_ptr<vr::MailboxToSurfaceBridge> mailbox_bridge)
     : gl_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      mailbox_bridge_(std::move(mailbox_bridge)) {}
+      mailbox_bridge_(std::move(mailbox_bridge)) {
+  DVLOG(2) << __func__;
+}
 
-ArImageTransport::~ArImageTransport() {
+ArImageTransport::~ArImageTransport() = default;
+
+void ArImageTransport::DestroySharedBuffers(vr::WebXrPresentationState* webxr) {
+  DVLOG(2) << __func__;
   DCHECK(IsOnGlThread());
 
-  if (webxr_) {
-    std::vector<std::unique_ptr<vr::WebXrSharedBuffer>> buffers =
-        webxr_->TakeSharedBuffers();
-    for (auto& buffer : buffers) {
-      if (!buffer->mailbox_holder.mailbox.IsZero()) {
-        DCHECK(mailbox_bridge_);
-        DVLOG(2) << ": DestroySharedImage, mailbox="
-                 << buffer->mailbox_holder.mailbox.ToDebugString();
-        // Note: the sync token in mailbox_holder may not be accurate. See
-        // comment in TransferFrame below.
-        mailbox_bridge_->DestroySharedImage(buffer->mailbox_holder);
-      }
+  if (!webxr)
+    return;
+
+  std::vector<std::unique_ptr<vr::WebXrSharedBuffer>> buffers =
+      webxr->TakeSharedBuffers();
+  for (auto& buffer : buffers) {
+    if (!buffer->mailbox_holder.mailbox.IsZero()) {
+      DCHECK(mailbox_bridge_);
+      DVLOG(2) << ": DestroySharedImage, mailbox="
+               << buffer->mailbox_holder.mailbox.ToDebugString();
+      // Note: the sync token in mailbox_holder may not be accurate. See
+      // comment in TransferFrame below.
+      mailbox_bridge_->DestroySharedImage(buffer->mailbox_holder);
     }
   }
 }
 
-bool ArImageTransport::Initialize(vr::WebXrPresentationState* webxr) {
-  DVLOG(1) << __func__;
+void ArImageTransport::Initialize(vr::WebXrPresentationState* webxr,
+                                  base::OnceClosure callback) {
   DCHECK(IsOnGlThread());
-
-  webxr_ = webxr;
-
-  mailbox_bridge_->BindContextProviderToCurrentThread();
+  DVLOG(2) << __func__;
 
   glDisable(GL_DEPTH_TEST);
   glDepthMask(GL_FALSE);
@@ -62,14 +65,26 @@ bool ArImageTransport::Initialize(vr::WebXrPresentationState* webxr) {
 
   glGenFramebuffersEXT(1, &camera_fbo_);
 
-  return true;
+  mailbox_bridge_->CreateAndBindContextProvider(
+      base::BindOnce(&ArImageTransport::OnMailboxBridgeReady,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+}
+
+void ArImageTransport::OnMailboxBridgeReady(base::OnceClosure callback) {
+  DVLOG(2) << __func__;
+  DCHECK(IsOnGlThread());
+
+  DCHECK(mailbox_bridge_->IsConnected());
+
+  std::move(callback).Run();
 }
 
 GLuint ArImageTransport::GetCameraTextureId() {
   return camera_texture_id_arcore_;
 }
 
-void ArImageTransport::ResizeSharedBuffer(const gfx::Size& size,
+void ArImageTransport::ResizeSharedBuffer(vr::WebXrPresentationState* webxr,
+                                          const gfx::Size& size,
                                           vr::WebXrSharedBuffer* buffer) {
   DCHECK(IsOnGlThread());
 
@@ -94,7 +109,7 @@ void ArImageTransport::ResizeSharedBuffer(const gfx::Size& size,
   static constexpr gfx::BufferFormat format = gfx::BufferFormat::RGBA_8888;
   static constexpr gfx::BufferUsage usage = gfx::BufferUsage::SCANOUT;
 
-  gfx::GpuMemoryBufferId kBufferId(webxr_->next_memory_buffer_id++);
+  gfx::GpuMemoryBufferId kBufferId(webxr->next_memory_buffer_id++);
   buffer->gmb = gpu::GpuMemoryBufferImplAndroidHardwareBuffer::Create(
       kBufferId, size, format, usage,
       gpu::GpuMemoryBufferImpl::DestructionCallback());
@@ -135,16 +150,17 @@ std::unique_ptr<vr::WebXrSharedBuffer> ArImageTransport::CreateBuffer() {
 }
 
 gpu::MailboxHolder ArImageTransport::TransferFrame(
+    vr::WebXrPresentationState* webxr,
     const gfx::Size& frame_size,
     const gfx::Transform& uv_transform) {
   DCHECK(IsOnGlThread());
 
-  if (!webxr_->GetAnimatingFrame()->shared_buffer) {
-    webxr_->GetAnimatingFrame()->shared_buffer = CreateBuffer();
+  if (!webxr->GetAnimatingFrame()->shared_buffer) {
+    webxr->GetAnimatingFrame()->shared_buffer = CreateBuffer();
   }
   vr::WebXrSharedBuffer* shared_buffer =
-      webxr_->GetAnimatingFrame()->shared_buffer.get();
-  ResizeSharedBuffer(frame_size, shared_buffer);
+      webxr->GetAnimatingFrame()->shared_buffer.get();
+  ResizeSharedBuffer(webxr, frame_size, shared_buffer);
 
   mailbox_bridge_->GenSyncToken(&shared_buffer->mailbox_holder.sync_token);
   return shared_buffer->mailbox_holder;
@@ -169,12 +185,13 @@ void ArImageTransport::CopyCameraImageToFramebuffer(
 }
 
 void ArImageTransport::CopyDrawnImageToFramebuffer(
+    vr::WebXrPresentationState* webxr,
     const gfx::Size& frame_size,
     const gfx::Transform& uv_transform) {
   DVLOG(2) << __func__;
 
   vr::WebXrSharedBuffer* shared_buffer =
-      webxr_->GetRenderingFrame()->shared_buffer.get();
+      webxr->GetRenderingFrame()->shared_buffer.get();
 
   // Set the blend mode for combining the drawn image (source) with the camera
   // image (destination). WebXR assumes that the canvas has premultiplied alpha,

chrome/browser/android/vr/arcore_device/ar_image_transport.h

@@ -7,6 +7,7 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "chrome/browser/android/vr/arcore_device/ar_renderer.h"
 #include "device/vr/public/mojom/vr_service.mojom.h"
@@ -29,30 +30,40 @@ struct WebXrSharedBuffer;
 
 namespace device {
 
-// This class copies the camera texture to a shared image and returns a mailbox
-// holder which is suitable for mojo transport to the Renderer.
+// This class handles transporting WebGL rendered output from the GPU process's
+// command buffer GL context to the local GL context, and compositing WebGL
+// output onto the camera image using the local GL context.
 class ArImageTransport {
  public:
   explicit ArImageTransport(
       std::unique_ptr<vr::MailboxToSurfaceBridge> mailbox_bridge);
   virtual ~ArImageTransport();
 
-  // Initialize() must be called on a valid GL thread.
-  virtual bool Initialize(vr::WebXrPresentationState* webxr);
+  virtual void DestroySharedBuffers(vr::WebXrPresentationState* webxr);
+
+  // All methods must be called on a valid GL thread. Initialization
+  // must happen after the local GL context is ready for use. That
+  // starts the asynchronous setup for the GPU process command buffer
+  // GL context via MailboxToSurfaceBridge, and the callback is called
+  // once that's complete.
+  virtual void Initialize(vr::WebXrPresentationState* webxr,
+                          base::OnceClosure callback);
 
   virtual GLuint GetCameraTextureId();
 
   // This transfers whatever the contents of the texture specified
   // by GetCameraTextureId() is at the time it is called and returns
   // a gpu::MailboxHolder with that texture copied to a shared buffer.
-  virtual gpu::MailboxHolder TransferFrame(const gfx::Size& frame_size,
+  virtual gpu::MailboxHolder TransferFrame(vr::WebXrPresentationState* webxr,
+                                           const gfx::Size& frame_size,
                                            const gfx::Transform& uv_transform);
   virtual void CreateGpuFenceForSyncToken(
       const gpu::SyncToken& sync_token,
       base::OnceCallback<void(std::unique_ptr<gfx::GpuFence>)>);
   virtual void CopyCameraImageToFramebuffer(const gfx::Size& frame_size,
                                             const gfx::Transform& uv_transform);
-  virtual void CopyDrawnImageToFramebuffer(const gfx::Size& frame_size,
+  virtual void CopyDrawnImageToFramebuffer(vr::WebXrPresentationState* webxr,
+                                           const gfx::Size& frame_size,
                                            const gfx::Transform& uv_transform);
   virtual void CopyTextureToFramebuffer(GLuint texture,
                                         const gfx::Size& frame_size,
@@ -61,10 +72,13 @@ class ArImageTransport {
 
  private:
   std::unique_ptr<vr::WebXrSharedBuffer> CreateBuffer();
-  void ResizeSharedBuffer(const gfx::Size& size, vr::WebXrSharedBuffer* buffer);
+  void ResizeSharedBuffer(vr::WebXrPresentationState* webxr,
+                          const gfx::Size& size,
+                          vr::WebXrSharedBuffer* buffer);
   bool IsOnGlThread() const;
+  void OnMailboxBridgeReady(base::OnceClosure callback);
   std::unique_ptr<ArRenderer> ar_renderer_;
-  // samplerExternalOES texture data for WebXR content image.
+  // samplerExternalOES texture for the camera image.
   GLuint camera_texture_id_arcore_ = 0;
   GLuint camera_fbo_ = 0;
 
@@ -72,8 +86,8 @@ class ArImageTransport {
 
   std::unique_ptr<vr::MailboxToSurfaceBridge> mailbox_bridge_;
 
-  vr::WebXrPresentationState* webxr_ = nullptr;
-
+  // Must be last.
+  base::WeakPtrFactory<ArImageTransport> weak_ptr_factory_{this};
   DISALLOW_COPY_AND_ASSIGN(ArImageTransport);
 };
 

chrome/browser/android/vr/arcore_device/arcore_device.cc

@@ -88,12 +88,6 @@ ArCoreDevice::ArCoreDevice(
   // it obvious if we're using this data instead of the actual values we get
   // from the output drawing surface.
   SetVRDisplayInfo(CreateVRDisplayInfo(GetId(), {16, 16}));
-
-  // TODO(https://crbug.com/836524) clean up usage of mailbox bridge
-  // and extract the methods in this class that interact with ARCore API
-  // into a separate class that implements the ArCore interface.
-  mailbox_bridge_->CreateUnboundContextProvider(
-      base::BindOnce(&ArCoreDevice::OnMailboxBridgeReady, GetWeakPtr()));
 }
 
 ArCoreDevice::ArCoreDevice()
@@ -122,33 +116,6 @@ ArCoreDevice::~ArCoreDevice() {
   session_state_->arcore_gl_thread_ = nullptr;
 }
 
-void ArCoreDevice::OnMailboxBridgeReady() {
-  DVLOG(1) << __func__;
-  DCHECK(IsOnMainThread());
-  DCHECK(!session_state_->arcore_gl_thread_);
-  // MailboxToSurfaceBridge's destructor's call to DestroyContext must
-  // happen on the GL thread, so transferring it to that thread is appropriate.
-  // TODO(https://crbug.com/836553): use same GL thread as GVR.
-  session_state_->arcore_gl_thread_ = std::make_unique<ArCoreGlThread>(
-      std::move(ar_image_transport_factory_), std::move(mailbox_bridge_),
-      CreateMainThreadCallback(base::BindOnce(
-          &ArCoreDevice::OnArCoreGlThreadInitialized, GetWeakPtr())));
-  session_state_->arcore_gl_thread_->Start();
-}
-
-void ArCoreDevice::OnArCoreGlThreadInitialized() {
-  DVLOG(1) << __func__;
-  DCHECK(IsOnMainThread());
-
-  session_state_->is_arcore_gl_thread_initialized_ = true;
-
-  if (session_state_->pending_request_session_after_gl_thread_initialized_) {
-    std::move(
-        session_state_->pending_request_session_after_gl_thread_initialized_)
-        .Run();
-  }
-}
-
 void ArCoreDevice::RequestSession(
     mojom::XRRuntimeSessionOptionsPtr options,
     mojom::XRRuntime::RequestSessionCallback callback) {
@@ -166,35 +133,22 @@ void ArCoreDevice::RequestSession(
       options->enabled_features,
       device::mojom::XRSessionFeature::DOM_OVERLAY_FOR_HANDHELD_AR);
 
-  if (session_state_->is_arcore_gl_thread_initialized_) {
-    // First session on a new ArCoreDevice, and it's ready to proceed now.
-    RequestSessionAfterInitialization(
-        options->render_process_id, options->render_frame_id, use_dom_overlay);
-  } else {
-    if (mailbox_bridge_) {
-      // This is a new ArCoreDevice, but its mailbox_bridge_ hasn't finished
-      // initialization yet.
-    } else {
-      // We're reusing a previously constructed ArCoreDevice for a new session.
-      // Restart initialization.
-      mailbox_bridge_ = std::make_unique<vr::MailboxToSurfaceBridge>();
-      mailbox_bridge_->CreateUnboundContextProvider(
-          base::BindOnce(&ArCoreDevice::OnMailboxBridgeReady, GetWeakPtr()));
-    }
-
-    // We're now expecting a call to OnMailboxBridgeReady() which will create
-    // a new GL thread, and at some point after that GL thread initialization
-    // will complete which calls OnArCoreGlThreadInitialized().
-    session_state_->pending_request_session_after_gl_thread_initialized_ =
-        base::BindOnce(&ArCoreDevice::RequestSessionAfterInitialization,
-                       GetWeakPtr(), options->render_process_id,
-                       options->render_frame_id, use_dom_overlay);
-  }
+  // mailbox_bridge_ is either supplied from the constructor, or recreated in
+  // OnSessionEnded().
+  DCHECK(mailbox_bridge_);
+
+  session_state_->arcore_gl_thread_ = std::make_unique<ArCoreGlThread>(
+      std::move(ar_image_transport_factory_), std::move(mailbox_bridge_),
+      CreateMainThreadCallback(
+          base::BindOnce(&ArCoreDevice::OnGlThreadReady, GetWeakPtr(),
+                         options->render_process_id, options->render_frame_id,
+                         use_dom_overlay)));
+  session_state_->arcore_gl_thread_->Start();
 }
 
-void ArCoreDevice::RequestSessionAfterInitialization(int render_process_id,
-                                                     int render_frame_id,
-                                                     bool use_overlay) {
+void ArCoreDevice::OnGlThreadReady(int render_process_id,
+                                   int render_frame_id,
+                                   bool use_overlay) {
   auto ready_callback =
       base::BindRepeating(&ArCoreDevice::OnDrawingSurfaceReady, GetWeakPtr());
   auto touch_callback =
@@ -268,9 +222,9 @@ void ArCoreDevice::OnSessionEnded() {
   // just a factory.)
   ar_image_transport_factory_ = std::make_unique<ArImageTransportFactory>();
 
-  // Shut down the mailbox bridge, this has the side effect of also destroying
-  // GL resources in the GPU process.
-  mailbox_bridge_ = nullptr;
+  // Create a new mailbox bridge for use in the next session. (This is cheap,
+  // the constructor doesn't establish a GL context.)
+  mailbox_bridge_ = std::make_unique<vr::MailboxToSurfaceBridge>();
 }
 
 void ArCoreDevice::CallDeferredRequestSessionCallback(bool success) {
@@ -291,7 +245,6 @@ void ArCoreDevice::CallDeferredRequestSessionCallback(bool success) {
   }
 
   // Success case should only happen after GL thread is ready.
-  DCHECK(session_state_->is_arcore_gl_thread_initialized_);
   auto create_callback =
       base::BindOnce(&ArCoreDevice::OnCreateSessionCallback, GetWeakPtr(),
                      std::move(deferred_callback));
@@ -342,7 +295,6 @@ void ArCoreDevice::RequestArCoreGlInitialization(
     const gfx::Size& frame_size) {
   DVLOG(1) << __func__;
   DCHECK(IsOnMainThread());
-  DCHECK(session_state_->is_arcore_gl_thread_initialized_);
 
   if (!arcore_session_utils_->EnsureLoaded()) {
     DLOG(ERROR) << "ARCore was not loaded properly.";
@@ -372,7 +324,6 @@ void ArCoreDevice::RequestArCoreGlInitialization(
 void ArCoreDevice::OnArCoreGlInitializationComplete(bool success) {
   DVLOG(1) << __func__;
   DCHECK(IsOnMainThread());
-  DCHECK(session_state_->is_arcore_gl_thread_initialized_);
 
   if (!success) {
     CallDeferredRequestSessionCallback(/*success=*/false);

chrome/browser/android/vr/arcore_device/arcore_device.h

@@ -52,11 +52,6 @@ class ArCoreDevice : public VRDeviceBase {
   }
 
  private:
-  // VRDeviceBase implementation
-  void OnMailboxBridgeReady();
-  void OnArCoreGlThreadInitialized();
-  void OnRequestCameraPermissionComplete(bool success);
-
   void OnDrawingSurfaceReady(gfx::AcceleratedWidget window,
                              display::Display::Rotation rotation,
                              const gfx::Size& frame_size);
@@ -84,13 +79,22 @@ class ArCoreDevice : public VRDeviceBase {
 
   bool IsOnMainThread();
 
-  void RequestSessionAfterInitialization(int render_process_id,
-                                         int render_frame_id,
-                                         bool use_overlay);
+  // Called once the GL thread is started. At this point, it doesn't
+  // have a valid GL context yet.
+  void OnGlThreadReady(int render_process_id,
+                       int render_frame_id,
+                       bool use_overlay);
+
+  // Replies to the pending mojo RequestSession request.
   void CallDeferredRequestSessionCallback(bool success);
+
+  // Tells the GL thread to initialize a GL context and other resources,
+  // using the supplied window as a drawing surface.
   void RequestArCoreGlInitialization(gfx::AcceleratedWidget window,
                                      int rotation,
                                      const gfx::Size& size);
+
+  // Called when the GL thread's GL context initialization completes.
   void OnArCoreGlInitializationComplete(bool success);
 
   void OnCreateSessionCallback(
@@ -112,7 +116,6 @@ class ArCoreDevice : public VRDeviceBase {
     ~SessionState();
 
     std::unique_ptr<ArCoreGlThread> arcore_gl_thread_;
-    bool is_arcore_gl_thread_initialized_ = false;
     bool is_arcore_gl_initialized_ = false;
 
     base::OnceClosure start_immersive_activity_callback_;
@@ -121,8 +124,6 @@ class ArCoreDevice : public VRDeviceBase {
     // the callback for replying once that initialization completes. Only one
     // concurrent session is supported, other requests are rejected.
     mojom::XRRuntime::RequestSessionCallback pending_request_session_callback_;
-
-    base::OnceClosure pending_request_session_after_gl_thread_initialized_;
   };
 
   // This object is reset to initial values when ending a session. This helps

chrome/browser/android/vr/arcore_device/arcore_device_unittest.cc

@@ -31,9 +31,10 @@ class StubArImageTransport : public ArImageTransport {
       std::unique_ptr<vr::MailboxToSurfaceBridge> mailbox_bridge)
       : ArImageTransport(std::move(mailbox_bridge)) {}
 
-  // TODO(lincolnfrog): verify this gets called on GL thread.
-  // TODO(lincolnfrog): test what happens if this returns false.
-  bool Initialize(vr::WebXrPresentationState*) override { return true; }
+  void Initialize(vr::WebXrPresentationState*,
+                  base::OnceClosure callback) override {
+    std::move(callback).Run();
+  }
 
   // TODO(lincolnfrog): test verify this somehow.
   GLuint GetCameraTextureId() override { return CAMERA_TEXTURE_ID; }
@@ -42,6 +43,7 @@ class StubArImageTransport : public ArImageTransport {
   // by GetCameraTextureId() is at the time it is called and returns
   // a gpu::MailboxHolder with that texture copied to a shared buffer.
   gpu::MailboxHolder TransferFrame(
+      vr::WebXrPresentationState*,
       const gfx::Size& frame_size,
       const gfx::Transform& uv_transform) override {
     return gpu::MailboxHolder();
@@ -65,14 +67,10 @@ class StubMailboxToSurfaceBridge : public vr::MailboxToSurfaceBridge {
  public:
   StubMailboxToSurfaceBridge() = default;
 
-  MOCK_METHOD1(DoCreateUnboundContextProvider,
-               void(base::OnceClosure callback));
-  void CreateUnboundContextProvider(base::OnceClosure callback) override {
+  void CreateAndBindContextProvider(base::OnceClosure callback) override {
     callback_ = std::move(callback);
   }
 
-  void BindContextProviderToCurrentThread() override {}
-
   bool IsConnected() override { return true; }
 
   void CallCallback() { std::move(callback_).Run(); }
@@ -178,7 +176,6 @@ class ArCoreDeviceTest : public testing::Test {
 
     run_loop = std::make_unique<base::RunLoop>();
     quit_closure = run_loop->QuitClosure();
-    bridge->CallCallback();
     run_loop->Run();
 
     EXPECT_TRUE(environment_provider);

chrome/browser/android/vr/arcore_device/arcore_gl.cc

@@ -80,11 +80,6 @@ gfx::Transform ConvertUvsToTransformMatrix(const std::vector<float>& uvs) {
   return result;
 }
 
-gfx::Transform WebXRImageTransformMatrix() {
-  gfx::Transform result;
-  return result;
-}
-
 const gfx::Size kDefaultFrameSize = {1, 1};
 const display::Display::Rotation kDefaultRotation = display::Display::ROTATE_0;
 
@@ -108,12 +103,12 @@ ArCoreGl::ArCoreGl(std::unique_ptr<ArImageTransport> ar_image_transport)
       webxr_(std::make_unique<vr::WebXrPresentationState>()),
       environment_binding_(this) {
   DVLOG(1) << __func__;
-  webxr_transform_ = WebXRImageTransformMatrix();
 }
 
 ArCoreGl::~ArCoreGl() {
   DVLOG(1) << __func__;
   DCHECK(IsOnGlThread());
+  ar_image_transport_->DestroySharedBuffers(webxr_.get());
   ar_image_transport_.reset();
   CloseBindingsIfOpen();
 }
@@ -155,13 +150,24 @@ void ArCoreGl::Initialize(vr::ArCoreSessionUtils* session_utils,
     return;
   }
 
-  // Set the texture on ArCore to render the camera.
+  DVLOG(3) << "ar_image_transport_->Initialize()...";
+  ar_image_transport_->Initialize(
+      webxr_.get(),
+      base::BindOnce(&ArCoreGl::OnArImageTransportReady,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+
+  // Set the texture on ArCore to render the camera. Must be after
+  // ar_image_transport_->Initialize().
   arcore_->SetCameraTexture(ar_image_transport_->GetCameraTextureId());
   // Set the Geometry to ensure consistent behaviour.
   arcore_->SetDisplayGeometry(kDefaultFrameSize, kDefaultRotation);
+}
 
+void ArCoreGl::OnArImageTransportReady(
+    base::OnceCallback<void(bool)> callback) {
+  DVLOG(3) << __func__;
   is_initialized_ = true;
-
+  webxr_->NotifyMailboxBridgeReady();
   std::move(callback).Run(true);
 }
 
@@ -240,12 +246,6 @@ bool ArCoreGl::InitializeGl(gfx::AcceleratedWidget drawing_widget) {
     return false;
   }
 
-  DVLOG(3) << "ar_image_transport_->Initialize()...";
-  if (!ar_image_transport_->Initialize(webxr_.get())) {
-    DLOG(ERROR) << "ARImageTransport failed to initialize";
-    return false;
-  }
-
   // Assign the surface and context members now that initialization has
   // succeeded.
   surface_ = std::move(surface);
@@ -391,8 +391,8 @@ void ArCoreGl::GetFrameData(
   }
   // Set up a shared buffer for the renderer to draw into, it'll be sent
   // alongside the frame pose.
-  gpu::MailboxHolder buffer_holder =
-      ar_image_transport_->TransferFrame(transfer_size_, uv_transform_);
+  gpu::MailboxHolder buffer_holder = ar_image_transport_->TransferFrame(
+      webxr_.get(), transfer_size_, uv_transform_);
 
   // Create the frame data to return to the renderer.
   frame_data->pose = std::move(pose);
@@ -443,15 +443,23 @@ bool ArCoreGl::IsSubmitFrameExpected(int16_t frame_index) {
 }
 
 void ArCoreGl::CopyCameraImageToFramebuffer() {
+  DVLOG(2) << __func__;
+
   // Draw the current camera texture to the output default framebuffer now, if
   // available.
-  if (!have_camera_image_)
-    return;
+  if (have_camera_image_) {
+    glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER, 0);
+    ar_image_transport_->CopyCameraImageToFramebuffer(camera_image_size_,
+                                                      uv_transform_);
+    have_camera_image_ = false;
+  }
 
-  glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER, 0);
-  ar_image_transport_->CopyCameraImageToFramebuffer(camera_image_size_,
-                                                    uv_transform_);
-  have_camera_image_ = false;
+  // We're done with the camera image for this frame, start the next ARCore
+  // update if we had deferred it. This will get the next frame's camera image
+  // and pose in parallel while we're waiting for this frame's rendered image.
+  if (pending_getframedata_) {
+    std::move(pending_getframedata_).Run();
+  }
 }
 
 void ArCoreGl::SubmitFrameMissing(int16_t frame_index,
@@ -466,13 +474,6 @@ void ArCoreGl::SubmitFrameMissing(int16_t frame_index,
 
   CopyCameraImageToFramebuffer();
 
-  // We're done with the camera image for this frame, start the next ARCore
-  // update if we had deferred it. This will get the next frame's camera image
-  // and pose in parallel while we're waiting for this frame's rendered image.
-  if (pending_getframedata_) {
-    std::move(pending_getframedata_).Run();
-  }
-
   surface_->SwapBuffers(base::DoNothing());
   DVLOG(3) << __func__ << ": frame=" << frame_index << " SwapBuffers";
 }
@@ -496,19 +497,20 @@ void ArCoreGl::SubmitFrameDrawnIntoTexture(int16_t frame_index,
   if (!IsSubmitFrameExpected(frame_index))
     return;
 
-  webxr_->TransitionFrameAnimatingToProcessing();
+  // Start processing the frame now if possible. If there's already a current
+  // processing frame, defer it until that frame calls TryDeferredProcessing.
+  webxr_->ProcessOrDefer(base::BindOnce(&ArCoreGl::ProcessFrameDrawnIntoTexture,
+                                        weak_ptr_factory_.GetWeakPtr(),
+                                        frame_index, sync_token));
+}
 
+void ArCoreGl::ProcessFrameDrawnIntoTexture(int16_t frame_index,
+                                            const gpu::SyncToken& sync_token) {
   TRACE_EVENT0("gpu", "ArCore SubmitFrame");
 
+  DCHECK(webxr_->HaveProcessingFrame());
   CopyCameraImageToFramebuffer();
 
-  // We're done with the camera image for this frame, start the next ARCore
-  // update if we had deferred it. This will get the next frame's camera image
-  // and pose in parallel while we're waiting for this frame's rendered image.
-  if (pending_getframedata_) {
-    std::move(pending_getframedata_).Run();
-  }
-
   ar_image_transport_->CreateGpuFenceForSyncToken(
       sync_token, base::BindOnce(&ArCoreGl::OnWebXrTokenSignaled, GetWeakPtr(),
                                  frame_index));
@@ -518,11 +520,12 @@ void ArCoreGl::OnWebXrTokenSignaled(int16_t frame_index,
                                     std::unique_ptr<gfx::GpuFence> gpu_fence) {
   DVLOG(3) << __func__ << ": frame=" << frame_index;
 
+  DCHECK(webxr_->HaveProcessingFrame());
   webxr_->TransitionFrameProcessingToRendering();
 
   glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER, 0);
-  ar_image_transport_->CopyDrawnImageToFramebuffer(camera_image_size_,
-                                                   webxr_transform_);
+  ar_image_transport_->CopyDrawnImageToFramebuffer(
+      webxr_.get(), camera_image_size_, shared_buffer_transform_);
   surface_->SwapBuffers(base::DoNothing());
   DVLOG(3) << __func__ << ": frame=" << frame_index << " SwapBuffers";
 
@@ -535,6 +538,8 @@ void ArCoreGl::OnWebXrTokenSignaled(int16_t frame_index,
     submit_client_->OnSubmitFrameGpuFence(
         gfx::CloneHandleForIPC(gpu_fence2->GetGpuFenceHandle()));
   }
+  // We finished processing a frame, unblock a potentially waiting next frame.
+  webxr_->TryDeferredProcessing();
 }
 
 void ArCoreGl::UpdateLayerBounds(int16_t frame_index,

chrome/browser/android/vr/arcore_device/arcore_gl.h

@@ -130,6 +130,8 @@ class ArCoreGl : public mojom::XRFrameDataProvider,
   // mojom::XRSessionController
   void SetFrameDataRestricted(bool restricted) override;
 
+  void ProcessFrameDrawnIntoTexture(int16_t frame_index,
+                                    const gpu::SyncToken& sync_token);
   void OnWebXrTokenSignaled(int16_t frame_index,
                             std::unique_ptr<gfx::GpuFence> gpu_fence);
 
@@ -147,6 +149,7 @@ class ArCoreGl : public mojom::XRFrameDataProvider,
                     mojom::XRFrameDataProvider::GetFrameDataCallback callback);
 
   bool InitializeGl(gfx::AcceleratedWidget drawing_widget);
+  void OnArImageTransportReady(base::OnceCallback<void(bool)> callback);
   bool IsOnGlThread() const;
   void CopyCameraImageToFramebuffer();
 
@@ -192,8 +195,14 @@ class ArCoreGl : public mojom::XRFrameDataProvider,
   display::Display::Rotation display_rotation_ = display::Display::ROTATE_0;
   bool should_update_display_geometry_ = true;
 
+  // UV transform for drawing the camera texture, this is supplied by ARCore
+  // and can include 90 degree rotations or other nontrivial transforms.
   gfx::Transform uv_transform_;
-  gfx::Transform webxr_transform_;
+
+  // UV transform for drawing received WebGL content from a shared buffer's
+  // texture, this is simply an identity.
+  gfx::Transform shared_buffer_transform_;
+
   gfx::Transform projection_;
   gfx::Transform inverse_projection_;
   // The first run of ProduceFrame should set uv_transform_ and projection_

chrome/browser/android/vr/arcore_device/arcore_gl_thread.cc

@@ -34,7 +34,6 @@ void ArCoreGlThread::Init() {
 
   arcore_gl_ = std::make_unique<ArCoreGl>(
       ar_image_transport_factory_->Create(std::move(mailbox_bridge_)));
-
   std::move(initialized_callback_).Run();
 }
 

chrome/browser/android/vr/mailbox_to_surface_bridge.cc

@@ -144,8 +144,7 @@ GLuint ConsumeTexture(gpu::gles2::GLES2Interface* gl,
 
 namespace vr {
 
-MailboxToSurfaceBridge::MailboxToSurfaceBridge()
-    : constructor_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()) {
+MailboxToSurfaceBridge::MailboxToSurfaceBridge() {
   DVLOG(1) << __FUNCTION__;
 }
 
@@ -177,20 +176,12 @@ void MailboxToSurfaceBridge::OnContextAvailableOnUiThread(
   // destruction.
   context_provider_ = std::move(provider);
 
-  if (on_context_provider_ready_) {
-    // We have a custom callback from CreateUnboundContextProvider. Run that.
-    // The client is responsible for running BindContextProviderToCurrentThread
-    // before use.
-    constructor_thread_task_runner_->PostTask(
-        FROM_HERE, std::move(on_context_provider_ready_));
-  } else {
-    DCHECK(on_context_bound_);
-    constructor_thread_task_runner_->PostTask(
-        FROM_HERE,
-        base::BindOnce(
-            &MailboxToSurfaceBridge::BindContextProviderToCurrentThread,
-            base::Unretained(this)));
-  }
+  DCHECK(on_context_bound_);
+  gl_thread_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &MailboxToSurfaceBridge::BindContextProviderToCurrentThread,
+          base::Unretained(this)));
 }
 
 void MailboxToSurfaceBridge::BindContextProviderToCurrentThread() {
@@ -234,21 +225,11 @@ void MailboxToSurfaceBridge::CreateSurface(
   ANativeWindow_release(window);
 }
 
-void MailboxToSurfaceBridge::CreateUnboundContextProvider(
-    base::OnceClosure callback) {
-  on_context_provider_ready_ = std::move(callback);
-  DCHECK(!on_context_bound_);
-  CreateContextProviderInternal();
-}
-
 void MailboxToSurfaceBridge::CreateAndBindContextProvider(
     base::OnceClosure on_bound_callback) {
+  gl_thread_task_runner_ = base::ThreadTaskRunnerHandle::Get();
   on_context_bound_ = std::move(on_bound_callback);
-  DCHECK(!on_context_provider_ready_);
-  CreateContextProviderInternal();
-}
 
-void MailboxToSurfaceBridge::CreateContextProviderInternal() {
   // The callback to run in this thread. It is necessary to keep |surface| alive
   // until the context becomes available. So pass it on to the callback, so that
   // it stays alive, and is destroyed on the same thread once done.
@@ -292,17 +273,19 @@ void MailboxToSurfaceBridge::CreateContextProviderInternal() {
 }
 
 void MailboxToSurfaceBridge::ResizeSurface(int width, int height) {
+  surface_width_ = width;
+  surface_height_ = height;
+
   if (!IsConnected()) {
     // We're not initialized yet, save the requested size for later.
     needs_resize_ = true;
-    resize_width_ = width;
-    resize_height_ = height;
     return;
   }
-  DVLOG(1) << __FUNCTION__ << ": resize Surface to " << width << "x" << height;
-  gl_->ResizeCHROMIUM(width, height, 1.f, GL_COLOR_SPACE_UNSPECIFIED_CHROMIUM,
-                      false);
-  gl_->Viewport(0, 0, width, height);
+  DVLOG(1) << __FUNCTION__ << ": resize Surface to " << surface_width_ << "x"
+           << surface_height_;
+  gl_->ResizeCHROMIUM(surface_width_, surface_height_, 1.f,
+                      GL_COLOR_SPACE_UNSPECIFIED_CHROMIUM, false);
+  gl_->Viewport(0, 0, surface_width_, surface_height_);
 }
 
 bool MailboxToSurfaceBridge::CopyMailboxToSurfaceAndSwap(
@@ -315,13 +298,20 @@ bool MailboxToSurfaceBridge::CopyMailboxToSurfaceAndSwap(
   }
 
   TRACE_EVENT0("gpu", __FUNCTION__);
+
   if (needs_resize_) {
-    ResizeSurface(resize_width_, resize_height_);
+    ResizeSurface(surface_width_, surface_height_);
     needs_resize_ = false;
   }
 
   DCHECK(mailbox.mailbox.IsSharedImage());
 
+  // While it's not an error to use a zero-sized Surface, it's not going to
+  // produce any visible output. Show a debug mode warning in that case to avoid
+  // another annoying debugging session.
+  DLOG_IF(WARNING, !surface_width_ || !surface_height_)
+      << "Surface is zero-sized. Missing call to ResizeSurface?";
+
   GLuint sourceTexture = ConsumeTexture(gl_, mailbox);
   gl_->BeginSharedImageAccessDirectCHROMIUM(
       sourceTexture, GL_SHARED_IMAGE_ACCESS_MODE_READ_CHROMIUM);

chrome/browser/android/vr/mailbox_to_surface_bridge.h

@@ -41,6 +41,10 @@ namespace vr {
 
 class MailboxToSurfaceBridge {
  public:
+  // It's OK to create an object instance and pass it to a different thread,
+  // i.e. to enable dependency injection for a unit test, but all methods on it
+  // must be called consistently on a single GL thread. This is verified by
+  // DCHECKs.
   MailboxToSurfaceBridge();
   virtual ~MailboxToSurfaceBridge();
 
@@ -52,40 +56,16 @@ class MailboxToSurfaceBridge {
   // is active. Requires initialization to be complete.
   bool IsGpuWorkaroundEnabled(int32_t workaround);
 
+  // This call is needed for Surface transport, in that case it must be called
+  // on the GL thread with a valid local native GL context. If it's not used,
+  // only the SharedBuffer transport methods are available.
   void CreateSurface(gl::SurfaceTexture*);
 
-  // This class can be used in a couple ways using these sequences:
-  //
-  // To use entirely on the GL thread:
-  // Call CreateAndBindContextProvider(callback) from your thread.
-  // When the callback is invoked, the object is ready for calls that use the
-  // context, such as CreateSharedImage().
-  //
-  // To create on one thread and use GL on another:
-  // Call CreateUnboundContextProvider(callback) and then make sure
-  // to call BindContextProviderToCurrentThread() from your GL
-  // thread afterwards before making a context-related calls.
-
   // Asynchronously create the context using the surface provided by an earlier
   // CreateSurface call, or an offscreen context if that wasn't called. Also
-  // binds the context provider to the thread used for constructing the
-  // MailboxToSurfaceBridge object, and calls the callback on the constructor
-  // thread. Use this if constructing the object on the intended GL thread.
-  void CreateAndBindContextProvider(base::OnceClosure callback);
-
-  // Variant of above, use this if the MailboxToSurfaceBridge constructor
-  // wasn't run on the GL thread. The provided callback is run on the
-  // constructor thread. After that, you can pass the MailboxToSurfaceBridge
-  // to another thread. You must call BindContextProviderToCurrentThread()
-  // on the target GL thread before using any context-related methods.
-  // The context-related methods check that they are called on this thread, so
-  // there will be a DCHECK error if they are not used consistently.
-  virtual void CreateUnboundContextProvider(base::OnceClosure callback);
-
-  // Client must call this on the target (GL) thread after
-  // CreateUnboundContextProvider. It's called automatically when using
-  // CreateAndBindContextProvider.
-  virtual void BindContextProviderToCurrentThread();
+  // binds the context provider to the current thread (making it the GL thread),
+  // and calls the callback on the GL thread.
+  virtual void CreateAndBindContextProvider(base::OnceClosure callback);
 
   // All other public methods below must be called on the GL thread
   // (except when marked otherwise).
@@ -127,7 +107,7 @@ class MailboxToSurfaceBridge {
   void DestroySharedImage(const gpu::MailboxHolder& mailbox_holder);
 
  private:
-  void CreateContextProviderInternal();
+  void BindContextProviderToCurrentThread();
   void OnContextAvailableOnUiThread(
       scoped_refptr<viz::ContextProvider> provider);
   void InitializeRenderer();
@@ -142,19 +122,19 @@ class MailboxToSurfaceBridge {
   // TODO(https://crbug.com/836524): shouldn't have both of these closures
   // in the same class like this.
   base::OnceClosure on_context_bound_;
-  base::OnceClosure on_context_provider_ready_;
 
-  // Saved state for a pending resize, the dimensions are only
-  // valid if needs_resize_ is true.
+  int surface_width_ = 0;
+  int surface_height_ = 0;
+
+  // If true, surface width/height is the intended size that should be applied
+  // to the surface once it's ready for use.
   bool needs_resize_ = false;
-  int resize_width_;
-  int resize_height_;
 
   // A swap ID which is passed to GL swap. Incremented each call.
   uint64_t swap_id_ = 0;
 
-  // A task runner for the thread the object was created on.
-  scoped_refptr<base::SingleThreadTaskRunner> constructor_thread_task_runner_;
+  // A task runner for the GL thread
+  scoped_refptr<base::SingleThreadTaskRunner> gl_thread_task_runner_;
 
   // Must be last.
   base::WeakPtrFactory<MailboxToSurfaceBridge> weak_ptr_factory_{this};