Bug 222607 - Implement WebXR Opaque Framebuffer
Summary: Implement WebXR Opaque Framebuffer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebXR (show other bugs)
Version: WebKit Nightly Build
Hardware: All Unspecified
: P2 Normal
Assignee: Imanol Fernandez
URL:
Keywords: InRadar
Depends on:
Blocks: 208988
  Show dependency treegraph
 
Reported: 2021-03-02 10:29 PST by Imanol Fernandez
Modified: 2021-03-18 22:35 PDT (History)
13 users (show)

See Also:


Attachments
Patch (87.89 KB, patch)
2021-03-02 12:33 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Video of WebXR sample running in HTC Vive (22.04 MB, video/x-matroska)
2021-03-02 12:41 PST, Imanol Fernandez
no flags Details
Patch (89.76 KB, patch)
2021-03-05 07:37 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (91.98 KB, patch)
2021-03-08 06:36 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (95.75 KB, patch)
2021-03-08 07:26 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (95.83 KB, patch)
2021-03-15 07:55 PDT, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (92.09 KB, patch)
2021-03-16 04:48 PDT, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch for landing (92.11 KB, patch)
2021-03-18 06:12 PDT, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (92.72 KB, patch)
2021-03-18 17:34 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Imanol Fernandez 2021-03-02 12:33:19 PST
Created attachment 421979 [details]
Patch
Comment 2 Imanol Fernandez 2021-03-02 12:41:08 PST
Created attachment 421982 [details]
Video of WebXR sample running in HTC Vive
Comment 3 Imanol Fernandez 2021-03-02 13:17:15 PST
: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 4 youenn fablet 2021-03-05 04:35:21 PST
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 5 Imanol Fernandez 2021-03-05 07:16:53 PST
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.
Comment 6 Imanol Fernandez 2021-03-05 07:37:03 PST
Created attachment 422371 [details]
Patch

Address review feedback
Comment 7 Imanol Fernandez 2021-03-05 07:52:03 PST
I opened https://github.com/immersive-web/webxr/issues/1177 to discuss async WebXR Layer GPU-side resource allocation
Comment 8 Chris Dumez 2021-03-05 07:58:39 PST
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 9 Dean Jackson 2021-03-06 05:15:45 PST
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 10 Imanol Fernandez 2021-03-08 06:22:48 PST
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.
Comment 11 youenn fablet 2021-03-08 06:26:28 PST
> > 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.
Comment 12 Imanol Fernandez 2021-03-08 06:36:32 PST
Created attachment 422562 [details]
Patch

Address review feddback
Comment 13 Imanol Fernandez 2021-03-08 06:48:28 PST
(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
Comment 14 Imanol Fernandez 2021-03-08 07:26:11 PST
Created attachment 422564 [details]
Patch

Use Ref<WorkQueue> instead of WorkQueue& in OpenXR device
Comment 15 Radar WebKit Bug Importer 2021-03-09 10:30:22 PST
<rdar://problem/75223815>
Comment 16 Imanol Fernandez 2021-03-15 07:55:05 PDT
Created attachment 423181 [details]
Patch

Rebase onto main. Fix GL error in setupFramebuffer() makeScopeExit.
Comment 17 youenn fablet 2021-03-15 07:59:02 PDT
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 18 Chris Dumez 2021-03-15 08:29:39 PDT
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.
Comment 19 Imanol Fernandez 2021-03-15 13:40:33 PDT
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
Comment 20 Imanol Fernandez 2021-03-16 04:48:22 PDT
Created attachment 423319 [details]
Patch

Rebase onto main with Bug 223211 landed
Comment 21 youenn fablet 2021-03-18 05:26:16 PDT
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/ = //
Comment 22 Imanol Fernandez 2021-03-18 06:12:23 PDT
Created attachment 423590 [details]
Patch for landing
Comment 23 EWS 2021-03-18 06:59:09 PDT
Committed r274644: <https://commits.webkit.org/r274644>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423590 [details].
Comment 24 Truitt Savell 2021-03-18 09:47:15 PDT
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
Comment 25 Truitt Savell 2021-03-18 09:51:19 PDT
Reverted r274644 for reason:

Broke internal Mac Builds

Committed r274648 (235473@main): <https://commits.webkit.org/235473@main>
Comment 26 Alex Christensen 2021-03-18 17:07:26 PDT
I'm looking into fixing the internal build so we can land this again.
Comment 27 Alex Christensen 2021-03-18 17:34:19 PDT
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.
Comment 28 Alex Christensen 2021-03-18 17:34:40 PDT
Created attachment 423673 [details]
Patch
Comment 29 EWS 2021-03-18 18:27:44 PDT
Committed r274695: <https://commits.webkit.org/r274695>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423673 [details].
Comment 30 Simon Fraser (smfr) 2021-03-18 22:35:06 PDT
Build fix in https://trac.webkit.org/changeset/274704/webkit