Bug 216925

Summary: [WebXR] Initial implemention of device initialization/shutdown with OpenXR
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, darin, dino, peng.liu6, pnormand, svillar, webkit-bug-importer, youennf, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 217752    
Bug Blocks: 208988    
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+, ews-feeder: commit-queue-

Description Sergio Villar Senin 2020-09-24 04:58:26 PDT
[WebXR] Initial implemention of device initialization/shutdown with OpenXR
Comment 1 Sergio Villar Senin 2020-09-24 05:06:56 PDT
Created attachment 409560 [details]
Patch
Comment 2 youenn fablet 2020-09-24 06:57:09 PDT
Comment on attachment 409560 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRSession.cpp:62
> +    m_device->initializeTrackingAndRendering(mode);

This seems like this could be done in device constructor and shutdownTrackingAndRendering in device destructor.
Are we sure there might not be two sessions using the same device at the same time?
Comment 3 Sergio Villar Senin 2020-09-24 07:11:20 PDT
(In reply to youenn fablet from comment #2)
> Comment on attachment 409560 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409560&action=review
> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:62
> > +    m_device->initializeTrackingAndRendering(mode);
> 
> This seems like this could be done in device constructor and
> shutdownTrackingAndRendering in device destructor.

The spec mentions the exact points when this should be done. Note that there could be multiple calls to initialize/shutdown if the user enters/leaves XR sessions in a row.

> Are we sure there might not be two sessions using the same device at the
> same time?

There is no device concept in OpenXR API, so our XRDevice is basically an abstraction for a session and some other additional data.
Comment 4 Sergio Villar Senin 2020-09-24 07:23:47 PDT
(In reply to Sergio Villar Senin from comment #3)
> (In reply to youenn fablet from comment #2)
> > Comment on attachment 409560 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=409560&action=review
> > 
> > > Source/WebCore/Modules/webxr/WebXRSession.cpp:62
> > > +    m_device->initializeTrackingAndRendering(mode);
> > 
> > This seems like this could be done in device constructor and
> > shutdownTrackingAndRendering in device destructor.
> 
> The spec mentions the exact points when this should be done. Note that there
> could be multiple calls to initialize/shutdown if the user enters/leaves XR
> sessions in a row.

I noticed the comment lacks some context. Device objects are created in a previous state even before the user requests a session. The author will firstly typically call XRSystem::isSessionSupported() to check whether a particular session (immersive VR, AR, inline) is supported. That call will end up creating the XR devices on demand.

Then, after checking that the session is supported the author might request a XRSession on a particular device. That's the point were we would need to create the OpenXR session. There is no need to unconditionally create OpenXR sessions for any available device. Also, if no session is ever requested then no OpenXR session should be created even though the devices were there.

Hope this clarifies a bit the implementation decision.
Comment 5 Sergio Villar Senin 2020-09-25 13:23:03 PDT
Comment on attachment 409560 [details]
Patch

Removing r?. I'm going first to make the current platform code fully asynchronous and then reupload this patch rebased on top of that work.
Comment 6 Radar WebKit Bug Importer 2020-10-01 04:59:20 PDT
<rdar://problem/69832601>
Comment 7 Sergio Villar Senin 2020-10-27 06:14:08 PDT
Created attachment 412416 [details]
Patch
Comment 8 Peng Liu 2020-10-28 15:05:06 PDT
Comment on attachment 412416 [details]
Patch

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

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:416
> +    };

Nit. Maybe we can write it like:
return (mode == SessionMode::ImmersiveVr) ? XR_VIEW_CONFIGURATION_TYPE_PRIMARY_STEREO : XR_VIEW_CONFIGURATION_TYPE_PRIMARY_MONO;
Comment 9 youenn fablet 2020-10-29 03:58:22 PDT
Comment on attachment 412416 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRSession.cpp:339
> +        m_device->shutdownTrackingAndRendering();

Is there a guarantee shutdown will not be called twice.
Should we ASSERT(!m_ended) at the start of shutdown()? or return early?

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:414
> +    default:

Usually we try to not add default and use all explicit values.
Can we change it to: 'case SessionMode::ImmersiveAr'?

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:431
> +        xrCreateSession(m_instance, &sessionCreateInfo, &m_session);

Other call sites of xrCreateSession do check for the result.
Should we add some logging and/or reset in case the session cannot be created?

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:438
> +        xrDestroySession(m_session);

Is it ok to pass  XR_NULL_HANDLE to xrDestroySession?
Comment 10 Sergio Villar Senin 2020-12-04 11:02:56 PST
Comment on attachment 412416 [details]
Patch

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

Thanks for waiting so much for my replies, I had several higher prio tasks to tackle before this.

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:339
>> +        m_device->shutdownTrackingAndRendering();
> 
> Is there a guarantee shutdown will not be called twice.
> Should we ASSERT(!m_ended) at the start of shutdown()? or return early?

Not really, the caller might invoke session.end() from JS multiple times (although it does not make sense from the functional POV). So shutdown() might be indeed be called multiple times.

The shutdown algorithm from specs seems to assume that the session is !ended so I'll add the condition to the calling place.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:414
>> +    default:
> 
> Usually we try to not add default and use all explicit values.
> Can we change it to: 'case SessionMode::ImmersiveAr'?

Sure.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:416
>> +    };
> 
> Nit. Maybe we can write it like:
> return (mode == SessionMode::ImmersiveVr) ? XR_VIEW_CONFIGURATION_TYPE_PRIMARY_STEREO : XR_VIEW_CONFIGURATION_TYPE_PRIMARY_MONO;

Yeah, but I think it's a good idea to have the switch because this way static analysis tools could easily detect missing handling of some values.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:431
>> +        xrCreateSession(m_instance, &sessionCreateInfo, &m_session);
> 
> Other call sites of xrCreateSession do check for the result.
> Should we add some logging and/or reset in case the session cannot be created?

The session will be XR_NULL_HANDLE whenever we get a failure, but we can indeed add some logging here.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:438
>> +        xrDestroySession(m_session);
> 
> Is it ok to pass  XR_NULL_HANDLE to xrDestroySession?

No, the OpenXR runtimes handle it properly but the standard says that it must be a valid session. I'll change it.
Comment 11 Sergio Villar Senin 2020-12-04 11:02:58 PST
Comment on attachment 412416 [details]
Patch

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

Thanks for waiting so much for my replies, I had several higher prio tasks to tackle before this.

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:339
>> +        m_device->shutdownTrackingAndRendering();
> 
> Is there a guarantee shutdown will not be called twice.
> Should we ASSERT(!m_ended) at the start of shutdown()? or return early?

Not really, the caller might invoke session.end() from JS multiple times (although it does not make sense from the functional POV). So shutdown() might be indeed be called multiple times.

The shutdown algorithm from specs seems to assume that the session is !ended so I'll add the condition to the calling place.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:414
>> +    default:
> 
> Usually we try to not add default and use all explicit values.
> Can we change it to: 'case SessionMode::ImmersiveAr'?

Sure.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:416
>> +    };
> 
> Nit. Maybe we can write it like:
> return (mode == SessionMode::ImmersiveVr) ? XR_VIEW_CONFIGURATION_TYPE_PRIMARY_STEREO : XR_VIEW_CONFIGURATION_TYPE_PRIMARY_MONO;

Yeah, but I think it's a good idea to have the switch because this way static analysis tools could easily detect missing handling of some values.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:431
>> +        xrCreateSession(m_instance, &sessionCreateInfo, &m_session);
> 
> Other call sites of xrCreateSession do check for the result.
> Should we add some logging and/or reset in case the session cannot be created?

The session will be XR_NULL_HANDLE whenever we get a failure, but we can indeed add some logging here.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:438
>> +        xrDestroySession(m_session);
> 
> Is it ok to pass  XR_NULL_HANDLE to xrDestroySession?

No, the OpenXR runtimes handle it properly but the standard says that it must be a valid session. I'll change it.
Comment 12 Sergio Villar Senin 2020-12-04 11:23:33 PST
Created attachment 415435 [details]
Patch
Comment 13 Darin Adler 2021-01-05 12:52:23 PST
Comment on attachment 415435 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRSession.cpp:61
> +    // If no other features of the user agent have done so already, perform the necessary platform-specific steps to
>      // initialize the device's tracking and rendering capabilities, including showing any necessary instructions to the user.

This seems like a comment that should be in the initializeTrackingAndRendering function, not here at the call site.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:37
> -template<typename T, XrStructureType StructureType>
> +template <typename T, XrStructureType StructureType>

Not sure we should make this change. I, for one, prefer the other form.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:65
>      WTF_MAKE_FAST_ALLOCATED;
> +
>  public:

Should use WTF_MAKE_STRUCT_FAST_ALLOCATED so we don’t have to explicitly use public here.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:243
> +    , m_session(XR_NULL_HANDLE)

Can we do this in the class definition instead of here in the constructor?

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:397
> +    return XR_VIEW_CONFIGURATION_TYPE_MAX_ENUM;

Why this value? I think it would be better to return a valid value in this unreachable case.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h:59
> +    void shutdownTrackingAndRendering() final;

The verb "shut down" is two words, so normally we’d want the "d" in "shutDown" here to be capitalized.

The word "shutdown" is a noun or adjective.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h:74
> +    XrViewConfigurationType m_currentViewConfiguration;

Not sure a "type" should be named "configuration".
Comment 14 Sergio Villar Senin 2021-01-08 01:59:34 PST
Committed r271288: <https://trac.webkit.org/changeset/271288>
Comment 15 Dean Jackson 2021-01-20 13:36:02 PST
Comment on attachment 415435 [details]
Patch

What happens if the device disconnects or shuts down the session itself? Does that go through ::end()?

Also, I'm not sure we should fire the "end" event until we've really shutdown the device, which might not be immediate.