From 1a34b8bdb2cfec5e1a5e02dd6c7dc80894de28fd Mon Sep 17 00:00:00 2001
From: Piotr Bialecki
Date: Fri, 30 Jul 2021 23:51:21 +0000
Subject: [PATCH] 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
Reviewed-by: Alexander Cooper
Reviewed-by: Kenneth Russell
Reviewed-by: Brian Sheedy
Commit-Queue: Piotr Bialecki
Cr-Commit-Position: refs/heads/master@{#907320}
---
.../browser/vr/WebXrArCameraAccessTest.java | 2 -
.../html/webxr_test_camera_access.html | 22 ++--
.../modules/webgl/webgl_unowned_texture.cc | 11 ++
.../modules/webgl/webgl_unowned_texture.h | 6 +
.../blink/renderer/modules/xr/xr_view.cc | 3 +
.../renderer/modules/xr/xr_webgl_binding.cc | 16 +--
.../renderer/modules/xr/xr_webgl_layer.cc | 47 ++++++--
.../renderer/modules/xr/xr_webgl_layer.h | 19 ++-
.../proposals/camera-access-barebones.html | 112 ++++++++++++++++--
9 files changed, 194 insertions(+), 44 deletions(-)
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrArCameraAccessTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrArCameraAccessTest.java
index e3349c1a4c972c..f64e10f7db496b 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrArCameraAccessTest.java
+++ b/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"})
diff --git a/chrome/test/data/xr/e2e_test_files/html/webxr_test_camera_access.html b/chrome/test/data/xr/e2e_test_files/html/webxr_test_camera_access.html
index 3c8df74df29617..c0f2985153bb19 100644
--- a/chrome/test/data/xr/e2e_test_files/html/webxr_test_camera_access.html
+++ b/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);
diff --git a/third_party/blink/renderer/modules/webgl/webgl_unowned_texture.cc b/third_party/blink/renderer/modules/webgl/webgl_unowned_texture.cc
index bd29e7e814187a..a7f3b06eed5ac8 100644
--- a/third_party/blink/renderer/modules/webgl/webgl_unowned_texture.cc
+++ b/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;
}
diff --git a/third_party/blink/renderer/modules/webgl/webgl_unowned_texture.h b/third_party/blink/renderer/modules/webgl/webgl_unowned_texture.h
index ee694dd148a52e..53eed52b42e694 100644
--- a/third_party/blink/renderer/modules/webgl/webgl_unowned_texture.h
+++ b/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;
};
diff --git a/third_party/blink/renderer/modules/xr/xr_view.cc b/third_party/blink/renderer/modules/xr/xr_view.cc
index 286c3e64b90b4c..965e0055fc109a 100644
--- a/third_party/blink/renderer/modules/xr/xr_view.cc
+++ b/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.
diff --git a/third_party/blink/renderer/modules/xr/xr_webgl_binding.cc b/third_party/blink/renderer/modules/xr/xr_webgl_binding.cc
index b4991add61243c..a29bb2cc413bbd 100644
--- a/third_party/blink/renderer/modules/xr/xr_webgl_binding.cc
+++ b/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 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(
- webgl_context_, texture_id, GL_TEXTURE_2D);
- return texture;
+ return base_layer->GetCameraTexture();
}
XRWebGLDepthInformation* XRWebGLBinding::getDepthInformation(
diff --git a/third_party/blink/renderer/modules/xr/xr_webgl_layer.cc b/third_party/blink/renderer/modules/xr/xr_webgl_layer.cc
index 7e052b85c159b0..c4d5c12aa88b2d 100644
--- a/third_party/blink/renderer/modules/xr/xr_webgl_layer.cc
+++ b/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 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(
+ 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& 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);
}
diff --git a/third_party/blink/renderer/modules/xr/xr_webgl_layer.h b/third_party/blink/renderer/modules/xr/xr_webgl_layer.h
index be839021fb7b26..23a954d94aeb5a 100644
--- a/third_party/blink/renderer/modules/xr/xr_webgl_layer.h
+++ b/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 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& buffer_mailbox_holder,
@@ -82,7 +90,7 @@ class XRWebGLLayer final : public XRLayer {
uint32_t GetBufferTextureId(
const absl::optional& buffer_mailbox_holder);
- void BindBufferTexture(
+ void BindCameraBufferTexture(
const absl::optional& buffer_mailbox_holder);
Member 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 camera_image_texture_;
+
absl::optional camera_image_mailbox_holder_;
};
diff --git a/third_party/webxr_test_pages/webxr-samples/proposals/camera-access-barebones.html b/third_party/webxr_test_pages/webxr-samples/proposals/camera-access-barebones.html
index fe46f05c6899d2..008e592e051ff8 100644
--- a/third_party/webxr_test_pages/webxr-samples/proposals/camera-access-barebones.html
+++ b/third_party/webxr_test_pages/webxr-samples/proposals/camera-access-barebones.html
@@ -29,9 +29,13 @@
Barebones WebXR Camera Access
@@ -43,16 +47,19 @@
This sample demonstrates extremely simple use of WebXR's camera access by creating a rotating cube and applying the camera's image as a texture to it.
Back