Bug 225896

Summary: [WebXR] Attach IOSurface to WebXROpaqueFramebuffer
Product: WebKit Reporter: Dean Jackson <dino>
Component: New BugsAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: adachan, ews-watchlist, graouts, kondapallykalyan, sam, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
WIP - Just the GCGL bits
none
Patch
sam: review+, ews-feeder: commit-queue-
WIP
ews-feeder: commit-queue-
WIP 2 none

Description Dean Jackson 2021-05-17 16:38:00 PDT 
[WebXR] Attach IOSurface to WebXROpaqueFramebuffer
Comment 1 Radar WebKit Bug Importer 2021-05-17 16:38:46 PDT 
<rdar://problem/78128289>
Comment 2 Dean Jackson 2021-05-17 16:39:28 PDT 
Created attachment 428890 [details]
WIP
Comment 3 Dean Jackson 2021-05-17 16:41:37 PDT 
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.
Comment 4 Dean Jackson 2021-05-21 21:56:30 PDT 
Created attachment 429400 [details]
WIP - Just the GCGL bits
Comment 5 Dean Jackson 2021-05-29 16:51:30 PDT 
Created attachment 430116 [details]
Patch
Comment 6 Sam Weinig 2021-05-29 17:07:13 PDT 
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?
Comment 7 Dean Jackson 2021-05-31 16:14:13 PDT 
Created attachment 430213 [details]
WIP
Comment 8 Dean Jackson 2021-05-31 16:43:15 PDT 
Created attachment 430215 [details]
WIP 2
Comment 9 Dean Jackson 2021-05-31 21:48:58 PDT 
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.
Comment 10 Dean Jackson 2021-05-31 21:53:10 PDT 
Committed r278295 (238332@main): <https://commits.webkit.org/238332@main>
Comment 11 Sam Weinig 2021-06-01 09:40:05 PDT 
(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.
Comment 12 Truitt Savell 2021-06-01 09:55:13 PDT 
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
Comment 13 Dean Jackson 2021-06-01 10:03:42 PDT 
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
Comment 14 Dean Jackson 2021-06-01 10:19:30 PDT 
(In reply to Dean Jackson from comment #13)
> I'll fix this right away. Thanks Truitt.

Hopefully fixed in r278312