Bug 222607

Created attachment 421979 [details] Patch

Created attachment 421982 [details] Video of WebXR sample running in HTC Vive

:dino r? Some comments before your review. For a follow-up it may make sense to abstract out some of the common code bits between default WebGL framebuffer and XR OpaqueFramebuffer into a base class. For example Gecko has a MozFramebuffer class that's used between both, I believe that chromium also has a similar DrawingBuffer class. That would be a big change that affects all the default FBO context code, and is out of the scope of this patch. Anyway this is a very specific use case of a Framebuffer, and it might not worth it to work on the common class that if we don't have more places in mind where "Default framebuffer" logic will be reused. IMO the future of WebXR will be more based on the WebXR layers to take advantage of multiview, and they use a Opaque Texture or TextureArray instead of a Opaque Framebuffer. Also it would be nice to implement GL_EXT_multisampled_render_to_texture as a follow-up to support more performant antialias on devices not supporting the similar Imagination extension.

Comment on attachment 421979 [details] Patch LGTM overall, a few questions. View in context: https://bugs.webkit.org/attachment.cgi?id=421979&action=review > Source/WebCore/Modules/webxr/WebXRSession.h:75 > + const WeakPtr<PlatformXR::Device>& device() const { return m_device; } Probably better to expose const Device* > Source/WebCore/Modules/webxr/WebXRViewport.h:46 > + const IntRect& rect() const { return m_viewport; } s/const IntRect&/IntRect/ > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:102 > + uint32_t height = recommendedSize.height() * init.framebufferScaleFactor; Is framebufferScaleFactor provided by JS? Should we check this does not overflow? > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:132 > + // 9.2 Initialize layer's framebuffer to null. This method starts to be long. We could add a function that would do step 9 maybe. > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:404 > + return; You have two styles for the same pattern here and in startFrame. I would use a single one. > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:539 > + } else if (useDepthStencil) { I would write it as: return gl.checkFramebufferStatus(GL::FRAMEBUFFER) == GL::FRAMEBUFFER_COMPLETE; } if (useDepthStencil) { .... > Source/WebCore/Modules/webxr/WebXRWebGLLayer.h:113 > + std::unique_ptr<WebXROpaqueFramebuffer> m_framebuffer; Should we use a UniqueRef<> here? > Source/WebCore/html/canvas/WebGLFramebuffer.cpp:314 > +Ref<WebGLFramebuffer> WebGLFramebuffer::createOpaque(WebGLRenderingContextBase& ctx) context > Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:62 > + : m_swapchain(WTFMove(swapchain)) Could be a UniqueRef as well > Source/WebCore/platform/xr/openxr/OpenXRSwapchain.cpp:54 > + imageHeaders.append(reinterpret_cast<XrSwapchainImageBaseHeader*>(&image)); imageHeaders do not seem to be used. Can we remove it? It is not clear what we plan to do with them and if the pointers will be valid when we will use them. Also, you could create the vector using WTF::map > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:292 > + m_queue.dispatch([this, width, height, alpha, &handle, &semaphore]() mutable { Why do we need to dispatch to a queue? If we need to, why do we need to wait on a semaphore? Could we refactor code to take a completion handler to not block? It is not really clear to me whether m_configurationViews is only supposed to be accessed in the queue? It seems m_layers is accessed from both, the semaphore helps making things simpler.

Comment on attachment 421979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421979&action=review >> Source/WebCore/Modules/webxr/WebXRSession.h:75 >> + const WeakPtr<PlatformXR::Device>& device() const { return m_device; } > > Probably better to expose const Device* done >> Source/WebCore/Modules/webxr/WebXRViewport.h:46 >> + const IntRect& rect() const { return m_viewport; } > > s/const IntRect&/IntRect/ done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:102 >> + uint32_t height = recommendedSize.height() * init.framebufferScaleFactor; > > Is framebufferScaleFactor provided by JS? > Should we check this does not overflow? good catch, it's a good idea to clamp the value. I added two functions to get the nativeResolution and maxResolution scale factors from device. For now they default to 1.0 but it adds flexibility to query the best maximum value from each XR runtime. We can address querying the real values from platform code in a follow-up issue. >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:132 >> + // 9.2 Initialize layer's framebuffer to null. > > This method starts to be long. > We could add a function that would do step 9 maybe. done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:404 >> + return; > > You have two styles for the same pattern here and in startFrame. > I would use a single one. done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:539 >> + } else if (useDepthStencil) { > > I would write it as: > return gl.checkFramebufferStatus(GL::FRAMEBUFFER) == GL::FRAMEBUFFER_COMPLETE; > } > if (useDepthStencil) { > .... done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.h:113 >> + std::unique_ptr<WebXROpaqueFramebuffer> m_framebuffer; > > Should we use a UniqueRef<> here? Not here because m_framebuffer is null for inline non-immservive sessions. This gave me the idea to use it in PlatformOpenXR::m_layers but it seems that UniqueRef doesn't work well with a HashMap because of the lack of empty constructor. >> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:314 >> +Ref<WebGLFramebuffer> WebGLFramebuffer::createOpaque(WebGLRenderingContextBase& ctx) > > context The similar existing create method is using ctx (WebGLFramebuffer::create(WebGLRenderingContextBase& ctx)) >> Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:62 >> + : m_swapchain(WTFMove(swapchain)) > > Could be a UniqueRef as well done >> Source/WebCore/platform/xr/openxr/OpenXRSwapchain.cpp:54 >> + imageHeaders.append(reinterpret_cast<XrSwapchainImageBaseHeader*>(&image)); > > imageHeaders do not seem to be used. > Can we remove it? > It is not clear what we plan to do with them and if the pointers will be valid when we will use them. > Also, you could create the vector using WTF::map We need imageHeaders to call xrEnumerateSwapchainImages but we don't need them later. I have removed them from the OpenXRSwapchain members and used WTF::map to create the temporal vector. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:292 >> + m_queue.dispatch([this, width, height, alpha, &handle, &semaphore]() mutable { > > Why do we need to dispatch to a queue? If we need to, why do we need to wait on a semaphore? Could we refactor code to take a completion handler to not block? > It is not really clear to me whether m_configurationViews is only supposed to be accessed in the queue? > It seems m_layers is accessed from both, the semaphore helps making things simpler. I tried to use a completion handler when implementing this but the WebXR spec doesn't use the promise pattern for XRWebGLLayer construction. The problem is not the m_configurationViews but the OpenXRLayerProjection creation which initializes a Swapchain internally that could fail, and JS expects to receive this error synchronously (OperationError when calling XRWebGLLayer::create() I'm going to open a issue in the WebXR spec about this, but I guess it's too late to change it if it breaks backwards compatibility. We could improve it for WebXR Layers though, which uses a similar creation pattern for other layers.

Created attachment 422371 [details] Patch Address review feedback

I opened https://github.com/immersive-web/webxr/issues/1177 to discuss async WebXR Layer GPU-side resource allocation

Comment on attachment 422371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422371&action=review Not confident doing a formal review here so just a few drive-by comments. > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:60 > +static constexpr double MinFramebufferScalingFactor = 0.2; static is not needed for constants like these. > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:66 > + return Exception { OperationError }; Please pass an exception message as second parameter. This is not Web-developer friendly. > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:84 > + return Exception { OperationError }; ditto about exception message. > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:95 > + return Exception { OperationError }; ditto about exception message. > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:347 > + auto opaque = std::unique_ptr<WebXROpaqueFramebuffer>(new WebXROpaqueFramebuffer(handle, WTFMove(framebuffer), context, WTFMove(attributes), width, height)); Why isn't this using makeUnique<>()? > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:366 > + if (gl) { if (auto gl = m_context.graphicsContextGL()) { > Source/WebCore/Modules/webxr/WebXRWebGLLayer.h:129 > + Nit: extra line > Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:33 > +{ Can we ASSERT(isMainThread()) since you are incrementing a global thread-unsafe integer? > Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:58 > + return std::unique_ptr<OpenXRLayerProjection>(new OpenXRLayerProjection(makeUniqueRefFromNonNullUniquePtr(WTFMove(swapchain)))); makeUnique<>()? > Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:76 > +Optional<XrCompositionLayerBaseHeader*> OpenXRLayerProjection::endFrame(const Device::Layer& layer, XrSpace space, const Vector<XrView>& frameViews) Seems weird to have an optional of a pointer. Do you really need to distinguish WTF::nullopt and nullptr? Looking at the implementation, it does not seem like it. > Source/WebCore/platform/xr/openxr/OpenXRSwapchain.h:32 > +class OpenXRSwapchain final { Why final? There is no base class. > Source/WebCore/platform/xr/openxr/OpenXRSwapchain.h:45 > + OpenXRSwapchain(XrInstance, XrSession, XrSwapchain, const XrSwapchainCreateInfo&, Vector<XrSwapchainImageOpenGLKHR>&&); Seems this should be private given we have a create() function? > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:246 > + m_queue.dispatch([this, layers = WTFMove(layers)]() mutable { What guarantees |this| is still alive when the lambda runs on the work queue. This looks unsafe.

Comment on attachment 422371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422371&action=review > Source/WebCore/platform/xr/PlatformXR.h:102 > + virtual Optional<LayerHandle> createLayerProjection(uint32_t width, uint32_t height, bool alpha) = 0; > + virtual void deleteLayer(LayerHandle) = 0; We're trying to find a better way to do this so that it is picked up by our EWS bots, but could you add empty implementations of these to PlatformXRCocoa.h? Otherwise the Apple builds fail when we have ENABLE_WEBXR turned on. It's off by default, so you don't see the compilation errors - we might have to enable it on the bots to avoid this.

Comment on attachment 422371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422371&action=review >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:60 >> +static constexpr double MinFramebufferScalingFactor = 0.2; > > static is not needed for constants like these. done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:66 >> + return Exception { OperationError }; > > Please pass an exception message as second parameter. This is not Web-developer friendly. done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:84 >> + return Exception { OperationError }; > > ditto about exception message. done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:95 >> + return Exception { OperationError }; > > ditto about exception message. done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:347 >> + auto opaque = std::unique_ptr<WebXROpaqueFramebuffer>(new WebXROpaqueFramebuffer(handle, WTFMove(framebuffer), context, WTFMove(attributes), width, height)); > > Why isn't this using makeUnique<>()? We are using private constructor now >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:366 >> + if (gl) { > > if (auto gl = m_context.graphicsContextGL()) { done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.h:129 >> + > > Nit: extra line done >> Source/WebCore/platform/xr/PlatformXR.h:102 >> + virtual void deleteLayer(LayerHandle) = 0; > > We're trying to find a better way to do this so that it is picked up by our EWS bots, but could you add empty implementations of these to PlatformXRCocoa.h? Otherwise the Apple builds fail when we have ENABLE_WEBXR turned on. It's off by default, so you don't see the compilation errors - we might have to enable it on the bots to avoid this. I have added the empty methods. Youenn also suggested that we enable WebXR tests cross-platform, I started this bug for that: https://bugs.webkit.org/show_bug.cgi?id=222317 >> Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:33 >> +{ > > Can we ASSERT(isMainThread()) since you are incrementing a global thread-unsafe integer? Done. The correct assert would be ASSERT(&RunLoop::current() == &m_queue.runLoop()); But instead of adding the assert (we don't have access to queue instance there) I got rid of the static variable and moved the handle generation to PlatformOpenXR. >> Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:58 >> + return std::unique_ptr<OpenXRLayerProjection>(new OpenXRLayerProjection(makeUniqueRefFromNonNullUniquePtr(WTFMove(swapchain)))); > > makeUnique<>()? I couldn't use makeUnique because of the private constructor >> Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:76 >> +Optional<XrCompositionLayerBaseHeader*> OpenXRLayerProjection::endFrame(const Device::Layer& layer, XrSpace space, const Vector<XrView>& frameViews) > > Seems weird to have an optional of a pointer. Do you really need to distinguish WTF::nullopt and nullptr? Looking at the implementation, it does not seem like it. We don't need to distinguish between both, I changed the code to use a pointer. >> Source/WebCore/platform/xr/openxr/OpenXRSwapchain.h:32 >> +class OpenXRSwapchain final { > > Why final? There is no base class. done >> Source/WebCore/platform/xr/openxr/OpenXRSwapchain.h:45 >> + OpenXRSwapchain(XrInstance, XrSession, XrSwapchain, const XrSwapchainCreateInfo&, Vector<XrSwapchainImageOpenGLKHR>&&); > > Seems this should be private given we have a create() function? yes, we can make it private >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:246 >> + m_queue.dispatch([this, layers = WTFMove(layers)]() mutable { > > What guarantees |this| is still alive when the lambda runs on the work queue. This looks unsafe. Good point. I used the same pattern in this PR as existing code in other calls. Checking this more, PlatformXR::Instance is LazyNeverDestroyed and it's holding both the queue and the device instance. In the case the device is destroyed the queue would be destroyed at the same time, with supposedly cancelled queue callbacks. But I see some other risks too if we implement device enumeration changes in the future, reusing the queue but with a different device. To make the code safer I added a weakThis check in the queue lambda. If you like this approach I'll create a follow-up issue to apply the same pattern in all the other m_queue.dispatch methods and to revisit the destruction of the instance.

> > What guarantees |this| is still alive when the lambda runs on the work queue. This looks unsafe. > > Good point. I used the same pattern in this PR as existing code in other > calls. Checking this more, PlatformXR::Instance is LazyNeverDestroyed and > it's holding both the queue and the device instance. In the case the device > is destroyed the queue would be destroyed at the same time, with supposedly > cancelled queue callbacks. But I see some other risks too if we implement > device enumeration changes in the future, reusing the queue but with a > different device. > > To make the code safer I added a weakThis check in the queue lambda. If you > like this approach I'll create a follow-up issue to apply the same pattern > in all the other m_queue.dispatch methods and to revisit the destruction of > the instance. The usual pattern is to have the object owning the WorkQueue. Can we do that here or is there a need for a single queue encompassing all objects? Weak pointers and work queues do not tend to work well together, checking a weak pointer is usually only safe in one specific thread.

Created attachment 422562 [details] Patch Address review feddback

(In reply to youenn fablet from comment #11) > > > What guarantees |this| is still alive when the lambda runs on the work queue. This looks unsafe. > > > > Good point. I used the same pattern in this PR as existing code in other > > calls. Checking this more, PlatformXR::Instance is LazyNeverDestroyed and > > it's holding both the queue and the device instance. In the case the device > > is destroyed the queue would be destroyed at the same time, with supposedly > > cancelled queue callbacks. But I see some other risks too if we implement > > device enumeration changes in the future, reusing the queue but with a > > different device. > > > > To make the code safer I added a weakThis check in the queue lambda. If you > > like this approach I'll create a follow-up issue to apply the same pattern > > in all the other m_queue.dispatch methods and to revisit the destruction of > > the instance. > > The usual pattern is to have the object owning the WorkQueue. Can we do that > here or is there a need for a single queue encompassing all objects? > > Weak pointers and work queues do not tend to work well together, checking a > weak pointer is usually only safe in one specific thread. PlatformOpenXR has a single queue (In reply to youenn fablet from comment #11) > > > What guarantees |this| is still alive when the lambda runs on the work queue. This looks unsafe. > > > > Good point. I used the same pattern in this PR as existing code in other > > calls. Checking this more, PlatformXR::Instance is LazyNeverDestroyed and > > it's holding both the queue and the device instance. In the case the device > > is destroyed the queue would be destroyed at the same time, with supposedly > > cancelled queue callbacks. But I see some other risks too if we implement > > device enumeration changes in the future, reusing the queue but with a > > different device. > > > > To make the code safer I added a weakThis check in the queue lambda. If you > > like this approach I'll create a follow-up issue to apply the same pattern > > in all the other m_queue.dispatch methods and to revisit the destruction of > > the instance. > > The usual pattern is to have the object owning the WorkQueue. Can we do that > here or is there a need for a single queue encompassing all objects? > > Weak pointers and work queues do not tend to work well together, checking a > weak pointer is usually only safe in one specific thread. We need to initalize the OpenXR XrInstance in the queue first, before device enumeration and device instance creation. We can take a Ref<WorkQueue> in the device to protect it from destruction while the device is alive

Created attachment 422564 [details] Patch Use Ref<WorkQueue> instead of WorkQueue& in OpenXR device

<rdar://problem/75223815>

Created attachment 423181 [details] Patch Rebase onto main. Fix GL error in setupFramebuffer() makeScopeExit.

Comment on attachment 423181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423181&action=review > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:43 > + , m_queue(WTFMove(queue)) As it stands, the WorkQueue may outlive OpenXRDevice so m_queue->dispatch([this] { /*do something with this*/}); is unsafe. On the other hand, if OpenXRDevice creates its own WorkQueue in constructor and will not ref it elsewhere, it becomes ok.

Comment on attachment 423181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423181&action=review >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:43 >> + , m_queue(WTFMove(queue)) > > As it stands, the WorkQueue may outlive OpenXRDevice so m_queue->dispatch([this] { /*do something with this*/}); is unsafe. > On the other hand, if OpenXRDevice creates its own WorkQueue in constructor and will not ref it elsewhere, it becomes ok. I don't think this statement is accurate. A WorkQueue ref's itself when dispatching so it does not matter if it "looks" like the OpenXRDevice "owns" the WorkQueue, it really doesn't and capturing |this| without protection is not safe. I will add that: 1. You cannot capture a WeakPtr and check if |this| is alive because more than one thread is involved here. 2. You cannot capture protectedThis in the lambda because you are in the constructor (the lambda could finish running on the other thread very quickly and deref |this|, which would call the destructor before even returning from the constructor. A common solution is usually to capture protectedThis in the lambda but move the dispatch logic from the constructor to the create() factory function (so that you are holding a RefPtr<> to the OpenXRDevice when dispatching). > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:47 > + m_queue->dispatch([this, callback = WTFMove(callback)]() mutable { I already mentioned in a previous review that this is not safe. The only case this would be safe would be if OpenXRDevice is a singleton, which doesn't seem to be the case. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:76 > + m_queue->dispatch([this, mode]() { ditto.

I created a separate patch to fix the unsafe queue issue: https://bugs.webkit.org/show_bug.cgi?id=223211 I'll rebase this one when we land bug 223211

Created attachment 423319 [details] Patch Rebase onto main with Bug 223211 landed

Comment on attachment 423319 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423319&action=review > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:302 > + Vector<PlatformXR::Device::LayerView> views = { s/ = //

Created attachment 423590 [details] Patch for landing

Committed r274644: <https://commits.webkit.org/r274644> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423590 [details].

The changes in https://trac.webkit.org/changeset/274644/webkit broke our internal Mac builds with this Error: WebCore/Modules/webxr/WebXRWebGLLayer.cpp:31:10: fatal error: 'ExtensionsGLOpenGLCommon.h' file not found

Reverted r274644 for reason: Broke internal Mac Builds Committed r274648 (235473@main): <https://commits.webkit.org/235473@main>

I'm looking into fixing the internal build so we can land this again.

Comment on attachment 423590 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=423590&action=review > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:31 > +#include "ExtensionsGLOpenGLCommon.h" I'm going to do some hacky things to get it to compile. Someone will need to come by later and correct what I'm doing, but I'll get this so it can land. > Source/WebCore/html/canvas/WebGLFramebuffer.h:166 > + bool m_opaqueHasStencil { false }; I'm going to remove this. It's unused, and it's generating a warning for an unused private member. > Source/WebCore/platform/xr/cocoa/PlatformXRCocoa.h:40 > + Optional<LayerHandle> createLayerProjection(uint32_t width, uint32_t height, bool alpha) override { return WTF::nullopt; }; I'm going to remove the variable names to avoid a warning from unused variables.

Created attachment 423673 [details] Patch

Committed r274695: <https://commits.webkit.org/r274695> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423673 [details].

Build fix in https://trac.webkit.org/changeset/274704/webkit