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

[bugzilla-daemon at webkit.org]

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

--- Comment #4 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 421979
  --> https://bugs.webkit.org/attachment.cgi?id=421979
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.

-- 
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/069665d6/attachment.htm>