[WebVR][OpenVR] Implement getVRDisplays()

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Villar Senin]

[WebVR][OpenVR] Implement getVRDisplays()

[Sergio Villar Senin]

Created attachment 333589 [details]
Patch

[Sergio Villar Senin]

Created attachment 333598 [details]
Patch

Build fixes for Mac/iOS

[Zan Dobersek]

Comment on attachment 333598 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> [phone]  Sergio Villar Senin  <[e-mail]>
> +
> +        [WebVR][OpenVR] Implement getVRDisplays()
> +        https://bugs.webkit.org/show_bug.cgi?id=182692
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Added platform/vr/ files to the build system.
> +
> +        * WebCore.xcodeproj/project.pbxproj:
> +

Additional ChangeLog entry.

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:44
> +    VRManager::singleton().refreshVRDevices(WTFMove(promise), navigator.frame()->document());

I think navigator.frame() should be null-checked as well, rejecting the promise in that case.

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:55
> -    return false;
> +    return true;

Should this be a RuntimeEnabledFeature? IMO this should return false until then.

> Source/WebCore/Modules/webvr/VRDisplay.cpp:37
> +Ref<VRDisplay> VRDisplay::create(ScriptExecutionContext& context, VRPlatformDisplay* platformDisplay)

I imagine the VRPlatformDisplay object will be used for different things in this class. For that purpose I think the ownership of VRPlatformDisplay objects has to be sorted out.

For instance, the VRPlatformManager class could own all the VRPlatformDisplay objects, but through getVRDisplays() it would serve out WeakPtrs. This allows VRPlatformManager to gracefully handle any failures that would require it to invalidate all the associated VRPlatformDisplay objects, without VRDisplay tripping into a crash when trying to use the associated VRPlatformDisplay.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:4512
> +		E188236C2032037400B42DF3 /* VRPlatformDisplayOpenVR.h in Headers */ = {isa = PBXBuildFile; fileRef = E188236B2032036800B42DF3 /* VRPlatformDisplayOpenVR.h */; settings = {ATTRIBUTES = (Private, ); }; };
> +		E188236D2032038200B42DF3 /* VRPlatformManagerOpenVR.h in Headers */ = {isa = PBXBuildFile; fileRef = E18823692032036700B42DF3 /* VRPlatformManagerOpenVR.h */; settings = {ATTRIBUTES = (Private, ); }; };

Is it necessary to include these files for the Mac port? I think it shouldn't be since USE_OPENVR isn't enabled for them.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:13864
> +		E18823682032036600B42DF3 /* VRPlatformDisplayOpenVR.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = VRPlatformDisplayOpenVR.cpp; sourceTree = "<group>"; };
> +		E18823692032036700B42DF3 /* VRPlatformManagerOpenVR.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = VRPlatformManagerOpenVR.h; sourceTree = "<group>"; };
> +		E188236A2032036700B42DF3 /* VRPlatformManagerOpenVR.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = VRPlatformManagerOpenVR.cpp; sourceTree = "<group>"; };
> +		E188236B2032036800B42DF3 /* VRPlatformDisplayOpenVR.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = VRPlatformDisplayOpenVR.h; sourceTree = "<group>"; };

Ditto.

> Source/WebCore/platform/vr/VRManager.cpp:52
> +VRManager& VRManager::singleton()
> +{
> +    static NeverDestroyed<VRManager> instance;
> +    return instance;
> +}
> +
> +VRManager::VRManager()
> +{
> +#if USE(OPENVR)
> +    m_platformManager = VRPlatformManagerOpenVR::create();
> +#endif
> +}
> +
> +VRManager::~VRManager()
> +{
> +    m_promiseTaskQueue.close();
> +    m_platformManager = nullptr;
> +}

Using the singleton approach is fine, as is the NeverDestroyed<> usage. But as such, the destructor is effectively unnecessary. One issue with that is that with this approach, the OpenVR system won't ever get shut down during the program lifetime. I don't think this is terribly important at this point, but it's something to consider later.

> Source/WebCore/platform/vr/VRManager.cpp:74
> +void VRManager::refreshVRDevices(GetVRDisplaysPromise&& promise, ScriptExecutionContext* context)
> +{
> +    if (!m_platformManager)
> +        promise.reject(InvalidStateError);
> +
> +    m_getDisplaysPromises.append(WTFMove(promise));
> +
> +    if (m_promiseTaskQueue.hasPendingTasks())
> +        return;
> +
> +    // FIXME: protect context???
> +    m_promiseTaskQueue.enqueueTask([this, promises = WTFMove(m_getDisplaysPromises), context] () mutable {
> +        Vector<VRPlatformDisplay*> platformDisplays = m_platformManager->getVRDisplays();
> +        Vector<Ref<VRDisplay>> displays;
> +        for (auto platformDisplay : platformDisplays)
> +            displays.append(VRDisplay::create(*context, platformDisplay));
> +        for (auto& deferredPromise : promises)
> +            deferredPromise.resolve(displays);
> +        });
> +
> +}

Here we're effectively in the platform layer, which means rejecting/resolving the promise here, as well as using a task queue here is a layering violation. All this has to be reworked so that it's done in NavigatorWebVR. From NavigatorWebVR you should somehow be able to query through VRManager all the enumerated VRPlatformDisplay objects, and then create VRDisplay objects off of that info.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:37
> +enum VRDisplayCapabilityFlags {
> +    None = 0,
> +    Position = 1 << 1,
> +    Orientation = 1 << 2,
> +    ExternalDisplay = 1 << 3,
> +    Present = 1 << 4
> +};

IMO this should be `enum class VRDisplayCapabilityFlag`, and then separately you'd have `using VRDisplayCapabilityFlags = unsigned;`. There might be C++ oddities in the way though, making it hard to use enum class values for bitwise operations.

> Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:33
> +    : m_system(VRSystem)
> +    , m_chaperone(VRChaperone)
> +    , m_compositor(VRCompositor)

VRSystem, VRChaperone and VRCompositor names should maybe be shortened to just 'system' etc., or capitalized as 'vrSystem' etc.

> Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:33
> +using namespace vr;

IMO it makes sense to explicitly list what's residing in the vr namespace, like in VRPlatformDisplayOpenVR.cpp.

> Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:45
> +VRPlatformManagerOpenVR::VRPlatformManagerOpenVR()
> +{
> +}

Use `= default`.

> Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:54
> +    HmdError error;

This should be initialied, I guess to VRInitError_None.

> Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:76
> +    HmdError error;

Should be initialized to VRInitError_None.

> Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:87
> +    Vector<VRPlatformDisplay*> displays;
> +    displays.append(new VRPlatformDisplayOpenVR(m_system, chaperone, compositor));
> +    return displays;

This leaks the VRPlatformDisplayOpenVR object. I think with OpenVR it's most optimal to just store a VRPlatformDisplayOpenVR instance in the VRPlatformManagerOpenVR class (manage it through std::unique_ptr<>, but initialize it in initOpenVR()).

Also, instead of passing pointer values in the Vector<> object, use Vector<std::reference_wrapper<VRPlatformDisplay>> instead.

[Sergio Villar Senin]

(In reply to Zan Dobersek from comment #3)
> Comment on attachment 333598 [details]
> Patch
> 

Thanks for the great review.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333598&action=review
> 
> > Source/WebCore/ChangeLog:11
> > [phone]  Sergio Villar Senin  <[e-mail]>
> > +
> > +        [WebVR][OpenVR] Implement getVRDisplays()
> > +        https://bugs.webkit.org/show_bug.cgi?id=182692
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Added platform/vr/ files to the build system.
> > +
> > +        * WebCore.xcodeproj/project.pbxproj:
> > +
> 
> Additional ChangeLog entry.
> 
> > Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:44
> > +    VRManager::singleton().refreshVRDevices(WTFMove(promise), navigator.frame()->document());
> 
> I think navigator.frame() should be null-checked as well, rejecting the
> promise in that case.

Right.

> > Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:55
> > -    return false;
> > +    return true;
> 
> Should this be a RuntimeEnabledFeature? IMO this should return false until
> then.

Yeah we could convert it to a proper runtime feature indeed.

> > Source/WebCore/Modules/webvr/VRDisplay.cpp:37
> > +Ref<VRDisplay> VRDisplay::create(ScriptExecutionContext& context, VRPlatformDisplay* platformDisplay)
> 
> I imagine the VRPlatformDisplay object will be used for different things in
> this class. For that purpose I think the ownership of VRPlatformDisplay
> objects has to be sorted out.
> 
> For instance, the VRPlatformManager class could own all the
> VRPlatformDisplay objects, but through getVRDisplays() it would serve out
> WeakPtrs. This allows VRPlatformManager to gracefully handle any failures
> that would require it to invalidate all the associated VRPlatformDisplay
> objects, without VRDisplay tripping into a crash when trying to use the
> associated VRPlatformDisplay.

Yes that was my idea, the ownership should be in the manager. I'll add the WeakPtr bits.

> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:4512
> > +		E188236C2032037400B42DF3 /* VRPlatformDisplayOpenVR.h in Headers */ = {isa = PBXBuildFile; fileRef = E188236B2032036800B42DF3 /* VRPlatformDisplayOpenVR.h */; settings = {ATTRIBUTES = (Private, ); }; };
> > +		E188236D2032038200B42DF3 /* VRPlatformManagerOpenVR.h in Headers */ = {isa = PBXBuildFile; fileRef = E18823692032036700B42DF3 /* VRPlatformManagerOpenVR.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> Is it necessary to include these files for the Mac port? I think it
> shouldn't be since USE_OPENVR isn't enabled for them.

Not strictly needed but is something that we could do now with very little effort.

> > Source/WebCore/platform/vr/VRManager.cpp:52
> > +VRManager& VRManager::singleton()
> > +{
> > +    static NeverDestroyed<VRManager> instance;
> > +    return instance;
> > +}
> > +
> > +VRManager::VRManager()
> > +{
> > +#if USE(OPENVR)
> > +    m_platformManager = VRPlatformManagerOpenVR::create();
> > +#endif
> > +}
> > +
> > +VRManager::~VRManager()
> > +{
> > +    m_promiseTaskQueue.close();
> > +    m_platformManager = nullptr;
> > +}
> 
> Using the singleton approach is fine, as is the NeverDestroyed<> usage. But
> as such, the destructor is effectively unnecessary. One issue with that is
> that with this approach, the OpenVR system won't ever get shut down during
> the program lifetime. I don't think this is terribly important at this
> point, but it's something to consider later.

Sure. Perhaps we could eventually destroy the object when exiting VR mode.
 
> > Source/WebCore/platform/vr/VRManager.cpp:74
> > +void VRManager::refreshVRDevices(GetVRDisplaysPromise&& promise, ScriptExecutionContext* context)
> > +{
> > +    if (!m_platformManager)
> > +        promise.reject(InvalidStateError);
> > +
> > +    m_getDisplaysPromises.append(WTFMove(promise));
> > +
> > +    if (m_promiseTaskQueue.hasPendingTasks())
> > +        return;
> > +
> > +    // FIXME: protect context???
> > +    m_promiseTaskQueue.enqueueTask([this, promises = WTFMove(m_getDisplaysPromises), context] () mutable {
> > +        Vector<VRPlatformDisplay*> platformDisplays = m_platformManager->getVRDisplays();
> > +        Vector<Ref<VRDisplay>> displays;
> > +        for (auto platformDisplay : platformDisplays)
> > +            displays.append(VRDisplay::create(*context, platformDisplay));
> > +        for (auto& deferredPromise : promises)
> > +            deferredPromise.resolve(displays);
> > +        });
> > +
> > +}
> 
> Here we're effectively in the platform layer, which means
> rejecting/resolving the promise here, as well as using a task queue here is
> a layering violation. All this has to be reworked so that it's done in
> NavigatorWebVR. From NavigatorWebVR you should somehow be able to query
> through VRManager all the enumerated VRPlatformDisplay objects, and then
> create VRDisplay objects off of that info.

OK.

> > Source/WebCore/platform/vr/VRPlatformDisplay.h:37
> > +enum VRDisplayCapabilityFlags {
> > +    None = 0,
> > +    Position = 1 << 1,
> > +    Orientation = 1 << 2,
> > +    ExternalDisplay = 1 << 3,
> > +    Present = 1 << 4
> > +};
> 
> IMO this should be `enum class VRDisplayCapabilityFlag`, and then separately
> you'd have `using VRDisplayCapabilityFlags = unsigned;`. There might be C++
> oddities in the way though, making it hard to use enum class values for
> bitwise operations.

I think I tried that before but it was generating quite weird build errors when using the flags, I can give it another try.

> > Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:33
> > +    : m_system(VRSystem)
> > +    , m_chaperone(VRChaperone)
> > +    , m_compositor(VRCompositor)
> 
> VRSystem, VRChaperone and VRCompositor names should maybe be shortened to
> just 'system' etc., or capitalized as 'vrSystem' etc.

ack

> > Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:33
> > +using namespace vr;
> 
> IMO it makes sense to explicitly list what's residing in the vr namespace,
> like in VRPlatformDisplayOpenVR.cpp.

ack

> > Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:45
> > +VRPlatformManagerOpenVR::VRPlatformManagerOpenVR()
> > +{
> > +}
> 
> Use `= default`.

Sure

> > Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:54
> > +    HmdError error;
> 
> This should be initialied, I guess to VRInitError_None.

ack

> > Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:76
> > +    HmdError error;
> 
> Should be initialized to VRInitError_None.

ack

> > Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:87
> > +    Vector<VRPlatformDisplay*> displays;
> > +    displays.append(new VRPlatformDisplayOpenVR(m_system, chaperone, compositor));
> > +    return displays;
> 
> This leaks the VRPlatformDisplayOpenVR object. I think with OpenVR it's most
> optimal to just store a VRPlatformDisplayOpenVR instance in the
> VRPlatformManagerOpenVR class (manage it through std::unique_ptr<>, but
> initialize it in initOpenVR()).

Ok

> Also, instead of passing pointer values in the Vector<> object, use
> Vector<std::reference_wrapper<VRPlatformDisplay>> instead.

Sure

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #4)
> (In reply to Zan Dobersek from comment #3)
> > Comment on attachment 333598 [details]
> > Patch
 
> > > Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:54
> > > +    HmdError error;
> > 
> > This should be initialied, I guess to VRInitError_None.
> 
> ack
> 
> > > Source/WebCore/platform/vr/openvr/VRPlatformManagerOpenVR.cpp:76
> > > +    HmdError error;
> > 
> > Should be initialized to VRInitError_None.
> 
> ack

BTW these two do not need to be initialized because the following calls do always properly set an error code (None when nothing fails)

[Sergio Villar Senin]

Created attachment 334028 [details]
Patch

[Zan Dobersek]

Comment on attachment 334028 [details]
Patch

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

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:52
> +        for (auto platformDisplay : platformDisplays.value())

auto&, otherwise you're copying the WeakPtr object here. Nothing wrong with that in itself, but it's not efficient.

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:53
> +            displays.append(VRDisplay::create(context, platformDisplay));

The WeakPtr object should be moved into the VRDisplay::create() call here.

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:55
> +        });

This indentation is off.

> Source/WebCore/Modules/webvr/VRDisplay.cpp:39
> +    auto display = adoptRef(*new VRDisplay(context, platformDisplay));

platformDisplay should be moved ...

> Source/WebCore/Modules/webvr/VRDisplay.cpp:44
> +VRDisplay::VRDisplay(ScriptExecutionContext& context, WeakPtr<VRPlatformDisplay> platformDisplay)

... into the WeakPtr<VRPlatformDisplay>&& parameter.

> Source/WebCore/Modules/webvr/VRDisplay.cpp:57
> -    return false;
> +    return m_display->getDisplayInfo().isConnected;

This should check that m_display is still valid.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:34
> +enum VRDisplayCapabilityFlags {
> +    None = 0,
> +    Position = 1 << 1,
> +    Orientation = 1 << 2,
> +    ExternalDisplay = 1 << 3,
> +    Present = 1 << 4
> +};

IMO the enum should be named `VRDisplayCapabilityFlag`, and a `VRDisplayCapabilityFlags` should be aliased to the `unsigned` type, but up to you.

You can take this up in a separate patch, perhaps moving the whole enum and type definition into a separate header so that the whole VRPlatformDisplay.h header isn't included in VRDisplayCapabilities.h.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:57
> +    WeakPtr<VRPlatformDisplay> createWeakPtr() { return m_weakPtrFactory.createWeakPtr(*this); }
> +private:
> +
> +    WeakPtrFactory<VRPlatformDisplay> m_weakPtrFactory;

Space oddity.

> Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.h:40
> +    vr::IVRSystem *m_system;
> +    vr::IVRChaperone *m_chaperone;
> +    vr::IVRCompositor *m_compositor;

Should be using WebKit pointer variable style.

[Sergio Villar Senin]

Created attachment 334151 [details]
Patch

Patch for landing

[Sergio Villar Senin]

Committed r228662: <https://trac.webkit.org/changeset/228662>

[Radar WebKit Bug Importer]

<rdar://problem/37667900>