Bug 225896 – [WebXR] Attach IOSurface to WebXROpaqueFramebuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

[WebXR] Attach IOSurface to WebXROpaqueFramebuffer

[Radar WebKit Bug Importer]

<rdar://problem/78128289>

[Dean Jackson]

Created attachment 428890 [details]
WIP

[Dean Jackson]

This patch compiles but still needs some work. Some code has been copied from GraphicsContextGLCVANGLE, but it probably should be moved.

Nothing has been tested yet.

[Dean Jackson]

Created attachment 429400 [details]
WIP - Just the GCGL bits

[Dean Jackson]

Created attachment 430116 [details]
Patch

[Sam Weinig]

Comment on attachment 430116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430116&action=review

> Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp:189
> +    if (m_ioSurfaceTextureHandle) {
> +        auto gCGL = static_cast<GraphicsContextGLOpenGL*>(&gl);
> +        gCGL->destroyPbufferAndDetachIOSurface(m_ioSurfaceTextureHandle);
> +    }

Is there a smart pointer we can use for this? This seems extremely easy to get wrong.

> Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp:248
> +    bool supportsPackedDepthStencil = true;

Seems like this should probably only be true is useDepthStencil is true? (all the other #if sections check useDepthStencil here.

> Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp:267
> +        // Cap the maxiumum multisample count at 4. Any more than this is likely overkill and will impact performance.

Is this observable and/or in the spec?

[Dean Jackson]

Created attachment 430213 [details]
WIP

[Dean Jackson]

Created attachment 430215 [details]
WIP 2

[Dean Jackson]

Comment on attachment 430116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430116&action=review

>> Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp:189
>> +    }
> 
> Is there a smart pointer we can use for this? This seems extremely easy to get wrong.

Agreed. I've filed https://bugs.webkit.org/show_bug.cgi?id=226480

>> Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp:248
>> +    bool supportsPackedDepthStencil = true;
> 
> Seems like this should probably only be true is useDepthStencil is true? (all the other #if sections check useDepthStencil here.

This is a platform test - ANGLE always supports a packed depth stencil (although it gets a bit complicated because WebGL 1 doesn't).

>> Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp:267
>> +        // Cap the maxiumum multisample count at 4. Any more than this is likely overkill and will impact performance.
> 
> Is this observable and/or in the spec?

This is not easily observable and is copied from our WebGL implementation. The GPU software folks said that anything above this is unnecessary and impacts old GPUs.

Actually, I guess it is observable if you craft your geometry just right. You'd be able to detect small differences in the antialiasing. However, the location of the samples within any texel are implementation/device specific.

[Dean Jackson]

Committed r278295 (238332@main): <https://commits.webkit.org/238332@main>

[Sam Weinig]

(In reply to Dean Jackson from comment #9)
> Comment on attachment 430116 [details]
> Patch
> 
> 
> >> Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp:248
> >> +    bool supportsPackedDepthStencil = true;
> > 
> > Seems like this should probably only be true is useDepthStencil is true? (all the other #if sections check useDepthStencil here.
> 
> This is a platform test - ANGLE always supports a packed depth stencil
> (although it gets a bit complicated because WebGL 1 doesn't).


Why do the other ports check useDepthStencil?

> 
> >> Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp:267
> >> +        // Cap the maxiumum multisample count at 4. Any more than this is likely overkill and will impact performance.
> > 
> > Is this observable and/or in the spec?
> 
> This is not easily observable and is copied from our WebGL implementation.
> The GPU software folks said that anything above this is unnecessary and
> impacts old GPUs.
> 
> Actually, I guess it is observable if you craft your geometry just right.
> You'd be able to detect small differences in the antialiasing. However, the
> location of the samples within any texel are implementation/device specific.

Let's avoid copying it. Let's put this in one place.

[Truitt Savell]

The changes in https://trac.webkit.org/changeset/278295/webkit

broke imported/w3c/web-platform-tests/webxr/xrBoundedReferenceSpace_updates.https.html causing it to crash constantly

history:
imported/w3c/web-platform-tests/webxr/xrBoundedReferenceSpace_updates.https.html

log:
0   com.apple.WebCore             	0x000000010fc6a880 size + 0 (IOSurface.h:144) [inlined]
1   com.apple.WebCore             	0x000000010fc6a880 WebCore::WebXROpaqueFramebuffer::startFrame(PlatformXR::Device::FrameData::LayerData const&) + 128 (WebXROpaqueFramebuffer.cpp:122)
2   com.apple.WebCore             	0x000000010fc7051f operator() + 317 (WebXRSession.cpp:547) [inlined]
3   com.apple.WebCore             	0x000000010fc7051f WTF::Detail::CallableWrapper<WebCore::WebXRSession::onFrame(PlatformXR::Device::FrameData&&)::$_4, void>::call() + 367 (Function.h:52)
4   com.apple.WebCore             	0x000000010ee61ae1 WebCore::EventLoop::run() + 337 (EventLoop.cpp:123)

https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r278306%20(3020)/imported/w3c/web-platform-tests/webxr/xrBoundedReferenceSpace_updates.https-crash-log.txt

[Dean Jackson]

I'll fix this right away. Thanks Truitt.

(In reply to Truitt Savell from comment #12)
> The changes in https://trac.webkit.org/changeset/278295/webkit
> 
> broke
> imported/w3c/web-platform-tests/webxr/xrBoundedReferenceSpace_updates.https.
> html causing it to crash constantly
> 
> history:
> imported/w3c/web-platform-tests/webxr/xrBoundedReferenceSpace_updates.https.
> html
> 
> log:
> 0   com.apple.WebCore             	0x000000010fc6a880 size + 0
> (IOSurface.h:144) [inlined]
> 1   com.apple.WebCore             	0x000000010fc6a880
> WebCore::WebXROpaqueFramebuffer::startFrame(PlatformXR::Device::FrameData::
> LayerData const&) + 128 (WebXROpaqueFramebuffer.cpp:122)
> 2   com.apple.WebCore             	0x000000010fc7051f operator() + 317
> (WebXRSession.cpp:547) [inlined]
> 3   com.apple.WebCore             	0x000000010fc7051f
> WTF::Detail::CallableWrapper<WebCore::WebXRSession::onFrame(PlatformXR::
> Device::FrameData&&)::$_4, void>::call() + 367 (Function.h:52)
> 4   com.apple.WebCore             	0x000000010ee61ae1
> WebCore::EventLoop::run() + 337 (EventLoop.cpp:123)
> 
> https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/
> r278306%20(3020)/imported/w3c/web-platform-tests/webxr/
> xrBoundedReferenceSpace_updates.https-crash-log.txt

[Dean Jackson]

(In reply to Dean Jackson from comment #13)
> I'll fix this right away. Thanks Truitt.

Hopefully fixed in r278312