[Webkit-unassigned] [Bug 222607] Implement WebXR Opaque Framebuffer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=222607

--- Comment #10 from Imanol Fernandez <ifernandez at igalia.com> ---
Comment on attachment 422371
  --> https://bugs.webkit.org/attachment.cgi?id=422371
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210308/c6d15759/attachment-0001.htm>