WebXR raw camera access: fix texture lifetime issue, reenable test

Also expands the camera-access-barebones.html to include more debugging
output and conditionally perform readback in case appropriate parameter
is present in the URI.

Fixed: 1115167
Change-Id: I8d6805528f2d269c3986e63e54ff3c2436cbfb8d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3060057
Auto-Submit: Piotr Bialecki <bialpio@chromium.org>
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Commit-Queue: Piotr Bialecki <bialpio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#907320}

chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrArCameraAccessTest.java

@@ -22,7 +22,6 @@
 import org.chromium.base.test.params.ParameterSet;
 import org.chromium.base.test.params.ParameterizedRunner;
 import org.chromium.base.test.util.CommandLineFlags;
-import org.chromium.base.test.util.DisabledTest;
 import org.chromium.base.test.util.MinAndroidSdkLevel;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
 import org.chromium.chrome.browser.vr.rules.ArPlaybackFile;
@@ -127,7 +126,6 @@ public void setUp() {
      */
     @Test
     @MediumTest
-    @DisabledTest(message = "https://www.crbug.com/1115167")
     @XrActivityRestriction({XrActivityRestriction.SupportedActivity.ALL})
     @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
             "enable-features=WebXRIncubations,LogJsConsoleMessages"})

chrome/test/data/xr/e2e_test_files/html/webxr_test_camera_access.html

@@ -153,7 +153,7 @@
 
         // Assign pixels array non-zero values.
         pixels = new Uint8Array(gl.drawingBufferWidth * gl.drawingBufferHeight * 4);
-        pixels.fill(1);
+        pixels.fill(0);
 
         // Counter of rAFcb calls in which readPixels() call was performed:
         let numIterations = 0;
@@ -167,6 +167,7 @@
             // texture before attempting to get the camera image texture.
             if (numPosesFound >= MIN_NUM_FRAMES_WITH_POSES
              && numPosesFound < MAX_NUM_FRAMES_WITH_POSES) {
+
               const view = pose.views.find(v => v.camera != null);
               if (view == undefined) return;
 
@@ -177,17 +178,24 @@
               gl.framebufferTexture2D(gl.FRAMEBUFFER, attachmentPoint, gl.TEXTURE_2D,
                                       cameraImageTexture, level);
 
-              const fbComplete = readCameraImageTexturePixels();
+              const fbComplete = readCameraImageTexturePixels(pixels);
+
+              console.log("Framebuffer complete? " + fbComplete);
               if (!fbComplete) return;
 
               numIterations++;
 
               const numZeroedRGBAValues = countZeros(pixels);
+
+              // We should get at least some non-zero pixels.
               assert_not_equals(numZeroedRGBAValues, pixels.length,
-                                "Camera image texture was already empty.");
+                                "Camera image texture was empty.");
+
+              // Clean up after ourselves:
+              pixels.fill(0);
             }
 
-            if (numPosesFound > MAX_NUM_FRAMES_WITH_POSES) {
+            if (numPosesFound >= MAX_NUM_FRAMES_WITH_POSES) {
               onARFrameCallback = null;
 
               assert_not_equals(numIterations, 0, "We should have read the pixels at least once!");
@@ -199,7 +207,7 @@
               gl.framebufferTexture2D(gl.FRAMEBUFFER, attachmentPoint, gl.TEXTURE_2D,
                                       cameraImageTexture, level);
 
-              const fbComplete = readCameraImageTexturePixels();
+              const fbComplete = readCameraImageTexturePixels(pixels);
               if (fbComplete) {
                 const numZeroedRGBAValues = countZeros(pixels);
                 assert_equals(numZeroedRGBAValues, pixels.length,
@@ -212,11 +220,11 @@
         };
       }
 
-      // Reads pixels from the framebuffer into the `pixels` variable.
+      // Reads pixels from the framebuffer into the |pixels| parameter.
       // Returns true if framebuffer status was FRAMEBUFFER_COMPLETE, false
       // otherwise. If the framebuffer was incomplete, readPixels() call was not
       // performed.
-      function readCameraImageTexturePixels () {
+      function readCameraImageTexturePixels (pixels) {
         if(gl.checkFramebufferStatus(gl.FRAMEBUFFER) == gl.FRAMEBUFFER_COMPLETE){
           gl.readPixels(0, 0, gl.drawingBufferWidth, gl.drawingBufferHeight, gl.RGBA, gl.UNSIGNED_BYTE, pixels);
           assert_equals(gl.getError(), gl.NO_ERROR);

third_party/blink/renderer/modules/webgl/webgl_unowned_texture.cc

@@ -14,7 +14,18 @@ WebGLUnownedTexture::WebGLUnownedTexture(WebGLRenderingContextBase* ctx,
                                          GLenum target)
     : WebGLTexture(ctx, texture, target) {}
 
+void WebGLUnownedTexture::OnGLDeleteTextures() {
+  // The owner of the texture name notified us that it is no longer valid.
+  // Just zero it out so we're not going to use it somewhere.
+  // Note that this will suppress the rest of the logic found in
+  // WebGLObject::DeleteObject(), since one of the first things that the method
+  // does is a check to see if |object_| is valid.
+  object_ = 0;
+}
+
 void WebGLUnownedTexture::DeleteObjectImpl(gpu::gles2::GLES2Interface* gl) {
+  // Normally, we would invoke gl->DeleteTextures() here, but
+  // WebGLUnownedTexture does not own its texture name. Just zero it out.
   object_ = 0;
 }
 

third_party/blink/renderer/modules/webgl/webgl_unowned_texture.h

@@ -14,6 +14,8 @@ namespace blink {
 // provide a camera image textures until it's decided how to best expose
 // the texture to the WebXR API.
 // TODO(https://bugs.chromium.org/p/chromium/issues/detail?id=1104340).
+// The texture does not own its texture name - it relies on being notified that
+// the texture name has been deleted by whoever owns it.
 class WebGLUnownedTexture final : public WebGLTexture {
  public:
   // The provided GLuint must have been created in the same
@@ -24,6 +26,10 @@ class WebGLUnownedTexture final : public WebGLTexture {
                                GLenum target);
   ~WebGLUnownedTexture() override;
 
+  // Used to notify the unowned texture that the owner has removed the texture
+  // name and so that it should not be used anymore.
+  void OnGLDeleteTextures();
+
  private:
   void DeleteObjectImpl(gpu::gles2::GLES2Interface*) override;
 };

third_party/blink/renderer/modules/xr/xr_view.cc

@@ -190,6 +190,9 @@ XRCamera* XRView::camera() const {
       frame_->session()->mode() ==
       device::mojom::blink::XRSessionMode::kImmersiveAr;
 
+  DVLOG(3) << __func__ << ": camera_access_enabled=" << camera_access_enabled
+           << ", is_immersive_ar_session=" << is_immersive_ar_session;
+
   if (camera_access_enabled && is_immersive_ar_session) {
     // The feature is enabled and we're in immersive-ar session, so let's return
     // a camera object if the camera image was received in the current frame.

third_party/blink/renderer/modules/xr/xr_webgl_binding.cc

@@ -149,6 +149,8 @@ WebGLTexture* XRWebGLBinding::getReflectionCubeMap(
 
 WebGLTexture* XRWebGLBinding::getCameraImage(XRCamera* camera,
                                              ExceptionState& exception_state) {
+  DVLOG(3) << __func__;
+
   XRFrame* frame = camera->Frame();
   DCHECK(frame);
 
@@ -188,20 +190,8 @@ WebGLTexture* XRWebGLBinding::getCameraImage(XRCamera* camera,
   XRWebGLLayer* base_layer = session->renderState()->baseLayer();
   DCHECK(base_layer);
 
-  absl::optional<gpu::MailboxHolder> camera_image_mailbox_holder =
-      base_layer->CameraImageMailboxHolder();
-
-  if (!camera_image_mailbox_holder) {
-    DVLOG(3) << __func__ << ": camera image mailbox holder is not set";
-    return nullptr;
-  }
-
-  GLuint texture_id = base_layer->CameraImageTextureId();
-
   // This resource is owned by the XRWebGLLayer, and is freed in OnFrameEnd();
-  WebGLUnownedTexture* texture = MakeGarbageCollected<WebGLUnownedTexture>(
-      webgl_context_, texture_id, GL_TEXTURE_2D);
-  return texture;
+  return base_layer->GetCameraTexture();
 }
 
 XRWebGLDepthInformation* XRWebGLBinding::getDepthInformation(

third_party/blink/renderer/modules/xr/xr_webgl_layer.cc

@@ -290,13 +290,25 @@ HTMLCanvasElement* XRWebGLLayer::output_canvas() const {
   return nullptr;
 }
 
-uint32_t XRWebGLLayer::CameraImageTextureId() const {
-  return camera_image_texture_id_;
-}
+WebGLTexture* XRWebGLLayer::GetCameraTexture() {
+  DVLOG(1) << __func__;
+
+  // We already have a WebGL texture for the camera image - return it:
+  if (camera_image_texture_) {
+    return camera_image_texture_;
+  }
+
+  // We don't have a WebGL texture, and we cannot create it - return null:
+  if (!camera_image_texture_id_) {
+    return nullptr;
+  }
 
-absl::optional<gpu::MailboxHolder> XRWebGLLayer::CameraImageMailboxHolder()
-    const {
-  return camera_image_mailbox_holder_;
+  // We don't have a WebGL texture, but we can create it, so create, store and
+  // return it:
+  camera_image_texture_ = MakeGarbageCollected<WebGLUnownedTexture>(
+      webgl_context_, camera_image_texture_id_, GL_TEXTURE_2D);
+
+  return camera_image_texture_;
 }
 
 void XRWebGLLayer::OnFrameStart(
@@ -320,7 +332,9 @@ void XRWebGLLayer::OnFrameStart(
       camera_image_mailbox_holder_ = camera_image_mailbox_holder;
       camera_image_texture_id_ =
           GetBufferTextureId(camera_image_mailbox_holder_);
-      BindBufferTexture(camera_image_mailbox_holder_);
+      DVLOG(3) << __func__
+               << ": camera_image_texture_id_=" << camera_image_texture_id_;
+      BindCameraBufferTexture(camera_image_mailbox_holder_);
     }
   }
 }
@@ -333,10 +347,11 @@ uint32_t XRWebGLLayer::GetBufferTextureId(
            << buffer_mailbox_holder->sync_token.ToDebugString();
   GLuint texture_id = gl->CreateAndTexStorage2DSharedImageCHROMIUM(
       buffer_mailbox_holder->mailbox.name);
+  DVLOG(3) << __func__ << ": texture_id=" << texture_id;
   return texture_id;
 }
 
-void XRWebGLLayer::BindBufferTexture(
+void XRWebGLLayer::BindCameraBufferTexture(
     const absl::optional<gpu::MailboxHolder>& buffer_mailbox_holder) {
   gpu::gles2::GLES2Interface* gl = drawing_buffer_->ContextGL();
 
@@ -385,10 +400,23 @@ void XRWebGLLayer::OnFrameEnd() {
       session()->xr()->frameProvider()->SubmitWebGLLayer(this,
                                                          framebuffer_dirty);
       if (camera_image_mailbox_holder_ && camera_image_texture_id_) {
-        DVLOG(3) << __func__ << "Deleting camera image texture";
+        DVLOG(3) << __func__
+                 << ": deleting camera image texture, camera_image_texture_id_="
+                 << camera_image_texture_id_;
         gpu::gles2::GLES2Interface* gl = drawing_buffer_->ContextGL();
+
         gl->EndSharedImageAccessDirectCHROMIUM(camera_image_texture_id_);
         gl->DeleteTextures(1, &camera_image_texture_id_);
+
+        // Notify our WebGLUnownedTexture (created from
+        // camera_image_texture_id_) that we have deleted it. Also, release the
+        // reference since we no longer need it (note that it could still be
+        // kept alive by the JS application, but should be a defunct object).
+        if (camera_image_texture_) {
+          camera_image_texture_->OnGLDeleteTextures();
+          camera_image_texture_ = nullptr;
+        }
+
         camera_image_texture_id_ = 0;
         camera_image_mailbox_holder_ = absl::nullopt;
       }
@@ -424,6 +452,7 @@ void XRWebGLLayer::Trace(Visitor* visitor) const {
   visitor->Trace(right_viewport_);
   visitor->Trace(webgl_context_);
   visitor->Trace(framebuffer_);
+  visitor->Trace(camera_image_texture_);
   XRLayer::Trace(visitor);
 }
 

third_party/blink/renderer/modules/xr/xr_webgl_layer.h

@@ -10,6 +10,7 @@
 #include "third_party/blink/renderer/bindings/modules/v8/v8_xr_webgl_layer_init.h"
 #include "third_party/blink/renderer/modules/webgl/webgl2_rendering_context.h"
 #include "third_party/blink/renderer/modules/webgl/webgl_rendering_context.h"
+#include "third_party/blink/renderer/modules/webgl/webgl_unowned_texture.h"
 #include "third_party/blink/renderer/modules/xr/xr_layer.h"
 #include "third_party/blink/renderer/modules/xr/xr_utils.h"
 #include "third_party/blink/renderer/modules/xr/xr_view.h"
@@ -61,8 +62,15 @@ class XRWebGLLayer final : public XRLayer {
   void UpdateViewports();
 
   HTMLCanvasElement* output_canvas() const;
-  uint32_t CameraImageTextureId() const;
-  absl::optional<gpu::MailboxHolder> CameraImageMailboxHolder() const;
+
+  // Returns WebGLTexture (actually a WebGLUnownedTexture instance)
+  // corresponding to the camera image.
+  // The texture is owned by the XRWebGLLayer and will be freed in OnFrameEnd().
+  // When the texture is deleted by the layer, the returned object will have its
+  // texture name set to 0 to avoid using stale texture names in case the user
+  // code still holds references to this object.
+  // The consumers should not attempt to delete the texture themselves.
+  WebGLTexture* GetCameraTexture();
 
   void OnFrameStart(
       const absl::optional<gpu::MailboxHolder>& buffer_mailbox_holder,
@@ -82,7 +90,7 @@ class XRWebGLLayer final : public XRLayer {
   uint32_t GetBufferTextureId(
       const absl::optional<gpu::MailboxHolder>& buffer_mailbox_holder);
 
-  void BindBufferTexture(
+  void BindCameraBufferTexture(
       const absl::optional<gpu::MailboxHolder>& buffer_mailbox_holder);
 
   Member<XRViewport> left_viewport_;
@@ -100,6 +108,11 @@ class XRWebGLLayer final : public XRLayer {
   uint32_t clean_frame_count = 0;
 
   uint32_t camera_image_texture_id_;
+  // WebGL texture that points to the |camera_image_texture_|. Must be notified
+  // via a call to |WebGLUnownedTexture::OnGLDeleteTextures()| when
+  // |camera_image_texture_id_| is deleted.
+  Member<WebGLUnownedTexture> camera_image_texture_;
+
   absl::optional<gpu::MailboxHolder> camera_image_mailbox_holder_;
 };