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

[bugzilla-daemon at webkit.org]

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

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

-- 
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/20210305/5a08554f/attachment.htm>