WebXR: Workaround for system ANGLE issue on S22 Exynos

On the Samsung Galaxy S22 Exynos, the system GLES driver is implemented
via an ANGLE wrapper on top of Vulkan. This ANGLE version appears to
have overly-strict validation which makes it impossible to use
AHardwareBuffer-backed external RGB textures as a framebuffer render
target attachment.

As a workaround, force-disable ArImageTransport::UseSharedBuffer
if this issue is detected and retry session creation.

Bug: 1355946
Change-Id: If026b61af5a5bdaaa1ff6f1d06c1beb2cfbbb605
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3998663
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Auto-Submit: Klaus Weidner <klausw@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067722}

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

@@ -35,8 +35,8 @@ class StubArImageTransport : public ArImageTransport {
       : ArImageTransport(std::move(mailbox_bridge)) {}
 
   void Initialize(WebXrPresentationState*,
-                  base::OnceClosure callback) override {
-    std::move(callback).Run();
+                  XrInitStatusCallback callback) override {
+    std::move(callback).Run(true);
   }
 
   // TODO(lincolnfrog): test verify this somehow.

device/vr/android/arcore/DEPS

@@ -2,6 +2,7 @@ include_rules = [
   "+components/viz/common",
   "+components/viz/host",
   "+gpu/command_buffer/service",
+  "+gpu/config/gpu_driver_bug_workaround_type.h",
   "+services/viz/privileged/mojom/compositing",
   "+services/viz/public/mojom/compositing",
   "+skia/ext",

device/vr/android/arcore/ar_image_transport.cc

@@ -13,6 +13,7 @@
 #include "device/vr/android/web_xr_presentation_state.h"
 #include "gpu/command_buffer/common/shared_image_usage.h"
 #include "gpu/command_buffer/service/ahardwarebuffer_utils.h"
+#include "gpu/config/gpu_driver_bug_workaround_type.h"
 #include "gpu/ipc/common/gpu_memory_buffer_impl_android_hardware_buffer.h"
 #include "ui/gfx/color_space.h"
 #include "ui/gfx/gpu_fence.h"
@@ -26,11 +27,14 @@
 
 namespace device {
 
+// static
+bool ArImageTransport::disable_shared_buffer_ = false;
+
 bool ArImageTransport::UseSharedBuffer() {
   // When available (Android O and up), use AHardwareBuffer-based shared
-  // images for frame transport.
+  // images for frame transport, unless disabled due to bugs.
   static bool val = base::AndroidHardwareBufferCompat::IsSupportAvailable();
-  return val;
+  return val && !ArImageTransport::disable_shared_buffer_;
 }
 
 ArImageTransport::ArImageTransport(
@@ -64,7 +68,7 @@ void ArImageTransport::DestroySharedBuffers(WebXrPresentationState* webxr) {
 }
 
 void ArImageTransport::Initialize(WebXrPresentationState* webxr,
-                                  base::OnceClosure callback) {
+                                  XrInitStatusCallback callback) {
   DCHECK(IsOnGlThread());
   DVLOG(2) << __func__;
 
@@ -93,13 +97,42 @@ void ArImageTransport::Initialize(WebXrPresentationState* webxr,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
-void ArImageTransport::OnMailboxBridgeReady(base::OnceClosure callback) {
+void ArImageTransport::OnMailboxBridgeReady(XrInitStatusCallback callback) {
   DVLOG(2) << __func__;
   DCHECK(IsOnGlThread());
 
   DCHECK(mailbox_bridge_->IsConnected());
 
-  std::move(callback).Run();
+  bool success = true;
+  if (UseSharedBuffer()) {
+    bool shared_buffer_not_usable = mailbox_bridge_->IsGpuWorkaroundEnabled(
+        gpu::DISABLE_RENDERING_TO_RGB_EXTERNAL_TEXTURE);
+    DVLOG(1) << __func__
+             << ": shared_buffer_not_usable=" << shared_buffer_not_usable;
+    if (shared_buffer_not_usable) {
+      // This is a bug workaround for shared buffer mode being unusable due to
+      // device GLES driver bugs, see https://crbug.com/1355946 for an example.
+      // We want to retry initialization with UseSharedBuffers forced to false.
+      //
+      // It would be nice if we could avoid this retry and just do the right
+      // thing on the first attempt, but unfortunately that's difficult due to
+      // the initialization order. We only know that the GPU bug workaround is
+      // necessary after the GPU process connection is established (this is when
+      // OnMailboxBridgeReady gets called). However, in order to establish the
+      // GPU process connection, we already have to decide if we want to use
+      // shared buffers or not - when not using shared buffers, we need to set
+      // up a Surface/SurfaceTexture pair first, and doing that requires a local
+      // GL context. However, setting up the local GL context wants to know if
+      // we'll be using compositor mode which requires knowing if we're using
+      // shared buffer mode. This is a circular dependency. Instead, just fail
+      // the first initialization attempt, force UseSharedBuffers to false,
+      // and try again a single time.
+      ArImageTransport::disable_shared_buffer_ = true;
+      DCHECK(!UseSharedBuffer());
+      success = false;
+    }
+  }
+  std::move(callback).Run(success);
 }
 
 void ArImageTransport::SetFrameAvailableCallback(

device/vr/android/arcore/ar_image_transport.h

@@ -35,6 +35,7 @@ class WebXrPresentationState;
 struct WebXrSharedBuffer;
 
 using XrFrameCallback = base::RepeatingCallback<void(const gfx::Transform&)>;
+using XrInitStatusCallback = base::OnceCallback<void(bool success)>;
 
 // This class handles transporting WebGL rendered output from the GPU process's
 // command buffer GL context to the local GL context, and compositing WebGL
@@ -61,7 +62,7 @@ class COMPONENT_EXPORT(VR_ARCORE) ArImageTransport {
   // GL context via MailboxToSurfaceBridge, and the callback is called
   // once that's complete.
   virtual void Initialize(WebXrPresentationState* webxr,
-                          base::OnceClosure callback);
+                          XrInitStatusCallback callback);
 
   virtual GLuint GetCameraTextureId();
 
@@ -100,14 +101,20 @@ class COMPONENT_EXPORT(VR_ARCORE) ArImageTransport {
   void ServerWaitForGpuFence(std::unique_ptr<gfx::GpuFence> gpu_fence);
 
  private:
+  // Used to disable UseSharedBuffer on platforms where the feature is available
+  // but unusable due to driver bugs. Must be mutable so that it can be switched
+  // to true persistently before retrying session creation, so it can't be
+  // constexpr or inline.
+  static bool disable_shared_buffer_;
+
   std::unique_ptr<WebXrSharedBuffer> CreateBuffer();
   // Returns true if the buffer was resized and its sync token updated.
   bool ResizeSharedBuffer(WebXrPresentationState* webxr,
                           const gfx::Size& size,
                           WebXrSharedBuffer* buffer);
   void ResizeSurface(const gfx::Size& size);
   bool IsOnGlThread() const;
-  void OnMailboxBridgeReady(base::OnceClosure callback);
+  void OnMailboxBridgeReady(XrInitStatusCallback callback);
   void OnFrameAvailable();
   std::unique_ptr<ArRenderer> ar_renderer_;
   // samplerExternalOES texture for the camera image.

device/vr/android/arcore/arcore_device.cc

@@ -87,7 +87,8 @@ ArCoreDevice::ArCoreDevice(
 
 ArCoreDevice::~ArCoreDevice() {
   // If there's still a pending session request, reject it.
-  CallDeferredRequestSessionCallback(absl::nullopt);
+  CallDeferredRequestSessionCallback(
+      base::unexpected(ArCoreGlInitializeError::kFailure));
 
   // Ensure that any active sessions are terminated. Terminating the GL thread
   // would normally do so via its session_shutdown_callback_, but that happens
@@ -110,6 +111,11 @@ void ArCoreDevice::RequestSession(
     mojom::XRRuntime::RequestSessionCallback callback) {
   DVLOG(1) << __func__;
   DCHECK(IsOnMainThread());
+
+  if (session_state_->allow_retry_) {
+    session_state_->options_clone_for_retry_ = options.Clone();
+  }
+
   DCHECK(options->mode == device::mojom::XRSessionMode::kImmersiveAr);
 
   if (HasExclusiveSession()) {
@@ -217,7 +223,31 @@ void ArCoreDevice::OnDrawingSurfaceTouch(bool is_primary,
 void ArCoreDevice::OnDrawingSurfaceDestroyed() {
   DVLOG(1) << __func__;
 
-  CallDeferredRequestSessionCallback(absl::nullopt);
+  if (session_state_->initiate_retry_) {
+    // If we get here, the drawing surface was destroyed intentionally in
+    // OnArCoreGlInitializationComplete due to a driver bug where we want to
+    // retry with workarounds applied.
+    DVLOG(1) << __func__ << ": initiating retry";
+
+    // Grab the options and callback before they are cleared by OnSessionEnded.
+    mojom::XRRuntimeSessionOptionsPtr options =
+        std::move(session_state_->options_clone_for_retry_);
+    mojom::XRRuntime::RequestSessionCallback callback =
+        std::move(session_state_->pending_request_session_callback_);
+
+    // Reset session_state_ back to defaults.
+    OnSessionEnded();
+
+    // Update the freshly-reset session state to not allow further retries. We
+    // don't want an infinite loop in case of logic errors.
+    session_state_->allow_retry_ = false;
+
+    RequestSession(std::move(options), std::move(callback));
+    return;
+  }
+
+  CallDeferredRequestSessionCallback(
+      base::unexpected(ArCoreGlInitializeError::kFailure));
 
   OnSessionEnded();
 }
@@ -278,7 +308,7 @@ void ArCoreDevice::OnSessionEnded() {
 }
 
 void ArCoreDevice::CallDeferredRequestSessionCallback(
-    absl::optional<ArCoreGlInitializeResult> initialize_result) {
+    ArCoreGlInitializeStatus initialize_result) {
   DVLOG(1) << __func__ << " success=" << initialize_result.has_value();
   DCHECK(IsOnMainThread());
 
@@ -290,7 +320,7 @@ void ArCoreDevice::CallDeferredRequestSessionCallback(
   mojom::XRRuntime::RequestSessionCallback deferred_callback =
       std::move(session_state_->pending_request_session_callback_);
 
-  if (!initialize_result) {
+  if (!initialize_result.has_value()) {
     TRACE_EVENT_WITH_FLOW0(
         "xr",
         "ArCoreDevice::CallDeferredRequestSessionCallback: GL initialization "
@@ -380,7 +410,8 @@ void ArCoreDevice::RequestArCoreGlInitialization(
 
   if (!arcore_session_utils_->EnsureLoaded()) {
     DLOG(ERROR) << "ARCore was not loaded properly.";
-    OnArCoreGlInitializationComplete(absl::nullopt);
+    OnArCoreGlInitializationComplete(
+        base::unexpected(ArCoreGlInitializeError::kFailure));
     return;
   }
 
@@ -412,21 +443,36 @@ void ArCoreDevice::RequestArCoreGlInitialization(
 }
 
 void ArCoreDevice::OnArCoreGlInitializationComplete(
-    absl::optional<ArCoreGlInitializeResult> arcore_initialization_result) {
+    ArCoreGlInitializeStatus arcore_initialization_result) {
   DVLOG(1) << __func__ << ": arcore_initialization_result.has_value()="
-           << arcore_initialization_result.has_value();
+           << arcore_initialization_result.has_value()
+           << " session_state_->allow_retry_=" << session_state_->allow_retry_;
   DCHECK(IsOnMainThread());
 
   session_state_->is_arcore_gl_initialized_ =
       arcore_initialization_result.has_value();
 
-  if (arcore_initialization_result) {
+  if (arcore_initialization_result.has_value()) {
     session_state_->enabled_features_ =
         arcore_initialization_result->enabled_features;
     session_state_->depth_configuration_ =
         arcore_initialization_result->depth_configuration;
     session_state_->frame_sink_id_ =
         arcore_initialization_result->frame_sink_id;
+    // Clear the cloned options now that we know we don't need a retry. The
+    // object size could be substantial, i.e. if it contains images for the
+    // image tracking feature.
+    session_state_->options_clone_for_retry_.reset();
+  } else if (arcore_initialization_result.error() ==
+                 ArCoreGlInitializeError::kRetryableFailure &&
+             session_state_->allow_retry_) {
+    session_state_->initiate_retry_ = true;
+    // Exit the current incomplete session, this will destroy the drawing
+    // surface.
+    arcore_session_utils_->EndSession();
+    // The retry will happen in OnDrawingSurfaceDestroyed, so skip calling
+    // the deferred callback.
+    return;
   } else {
     session_state_->enabled_features_ = {};
     session_state_->depth_configuration_ = absl::nullopt;

device/vr/android/arcore/arcore_device.h

@@ -103,7 +103,7 @@ class COMPONENT_EXPORT(VR_ARCORE) ArCoreDevice : public VRDeviceBase {
 
   // Replies to the pending mojo RequestSession request.
   void CallDeferredRequestSessionCallback(
-      absl::optional<ArCoreGlInitializeResult> arcore_initialization_result);
+      ArCoreGlInitializeStatus arcore_initialization_result);
 
   // Tells the GL thread to initialize a GL context and other resources,
   // using the supplied window as a drawing surface.
@@ -115,7 +115,7 @@ class COMPONENT_EXPORT(VR_ARCORE) ArCoreDevice : public VRDeviceBase {
 
   // Called when the GL thread's GL context initialization completes.
   void OnArCoreGlInitializationComplete(
-      absl::optional<ArCoreGlInitializeResult> arcore_initialization_result);
+      ArCoreGlInitializeStatus arcore_initialization_result);
 
   void OnCreateSessionCallback(
       mojom::XRRuntime::RequestSessionCallback deferred_callback,
@@ -171,6 +171,14 @@ class COMPONENT_EXPORT(VR_ARCORE) ArCoreDevice : public VRDeviceBase {
     // Trace ID of the requestSession() call that resulted in creating this
     // session state.
     uint64_t request_session_trace_id_;
+
+    // In case of driver bugs that need workarounds (see
+    // ArImageTransport::OnSurfaceBridgeReady), allow a one-time
+    // retry of session creation. This needs a copy of the original
+    // session creation options.
+    bool allow_retry_ = true;
+    mojom::XRRuntimeSessionOptionsPtr options_clone_for_retry_;
+    bool initiate_retry_ = false;
   };
 
   // This object is reset to initial values when ending a session. This helps

device/vr/android/arcore/arcore_gl.cc

@@ -151,7 +151,8 @@ ArCoreGl::~ArCoreGl() {
   // If anyone is still waiting for our initialization to finish, let them know
   // that it failed.
   if (initialized_callback_)
-    std::move(initialized_callback_).Run(absl::nullopt);
+    std::move(initialized_callback_)
+        .Run(base::unexpected(ArCoreGlInitializeError::kFailure));
 
   // Make sure mojo bindings are closed before proceeding with member
   // destruction. Specifically, destroying pending_getframedata_
@@ -214,7 +215,8 @@ void ArCoreGl::Initialize(
     drawing_widget = gfx::kNullAcceleratedWidget;
   }
   if (!InitializeGl(drawing_widget)) {
-    std::move(callback).Run(absl::nullopt);
+    std::move(callback).Run(
+        base::unexpected(ArCoreGlInitializeError::kFailure));
     return;
   }
 
@@ -223,7 +225,8 @@ void ArCoreGl::Initialize(
       session_utils->GetApplicationContext();
   if (!application_context.obj()) {
     DLOG(ERROR) << "Unable to retrieve the Java context/activity!";
-    std::move(callback).Run(absl::nullopt);
+    std::move(callback).Run(
+        base::unexpected(ArCoreGlInitializeError::kFailure));
     return;
   }
 
@@ -252,7 +255,8 @@ void ArCoreGl::Initialize(
                           std::move(depth_sensing_config));
   if (!maybe_initialize_result) {
     DLOG(ERROR) << "ARCore failed to initialize";
-    std::move(callback).Run(absl::nullopt);
+    std::move(callback).Run(
+        base::unexpected(ArCoreGlInitializeError::kFailure));
     return;
   }
 
@@ -331,16 +335,22 @@ void ArCoreGl::InitializeArCompositor(gpu::SurfaceHandle surface_handle,
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
-void ArCoreGl::OnArImageTransportReady() {
-  DVLOG(1) << __func__;
+void ArCoreGl::OnArImageTransportReady(bool success) {
+  DVLOG(1) << __func__ << ": success=" << success;
+  if (!success) {
+    std::move(initialized_callback_)
+        .Run(base::unexpected(ArCoreGlInitializeError::kRetryableFailure));
+    return;
+  }
   is_image_transport_ready_ = true;
   OnInitialized();
 }
 
 void ArCoreGl::OnArCompositorInitialized(bool initialized) {
   DVLOG(1) << __func__ << " intialized=" << initialized;
   if (!initialized) {
-    std::move(initialized_callback_).Run(absl::nullopt);
+    std::move(initialized_callback_)
+        .Run(base::unexpected(ArCoreGlInitializeError::kFailure));
     return;
   }
 

device/vr/android/arcore/arcore_gl.h

@@ -15,6 +15,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/task/single_thread_task_runner.h"
 #include "base/time/time.h"
+#include "base/types/expected.h"
 #include "device/vr/android/arcore/ar_compositor_frame_sink.h"
 #include "device/vr/public/cpp/xr_frame_sink_client.h"
 #include "device/vr/public/mojom/isolated_xr_service.mojom.h"
@@ -83,8 +84,15 @@ struct ArCoreGlInitializeResult {
   ~ArCoreGlInitializeResult();
 };
 
+enum class ArCoreGlInitializeError {
+  kFailure,
+  kRetryableFailure,
+};
+
+using ArCoreGlInitializeStatus =
+    base::expected<ArCoreGlInitializeResult, ArCoreGlInitializeError>;
 using ArCoreGlInitializeCallback =
-    base::OnceCallback<void(absl::optional<ArCoreGlInitializeResult>)>;
+    base::OnceCallback<void(ArCoreGlInitializeStatus)>;
 
 // All of this class's methods must be called on the same valid GL thread with
 // the exception of GetGlThreadTaskRunner() and GetWeakPtr().
@@ -214,7 +222,7 @@ class ArCoreGl : public mojom::XRFrameDataProvider,
                               ui::WindowAndroid* root_window,
                               XrFrameSinkClient* xr_frame_sink_client,
                               device::DomOverlaySetup dom_setup);
-  void OnArImageTransportReady();
+  void OnArImageTransportReady(bool success);
   void OnArCompositorInitialized(bool initialized);
   void OnInitialized();
   bool IsOnGlThread() const;