Attachment 411853 Details for Bug 217752 – Patch

[patch] Patch

bug-217752-20201020135913.patch (text/plain), 16.33 KB, created by Sergio Villar Senin on 2020-10-20 04:59:14 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Sergio Villar Senin

Created: 2020-10-20 04:59:14 PDT

Size: 16.33 KB

patch

obsolete

>Subversion Revision: 268724
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 835a861e46ca20a0a9bafde5d117c6e37cd688ea..ec43d0e54d94a7cea536aabf5e538adcd1f84737 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,36 @@
>+2020-10-15  Sergio Villar Senin  <svillar@igalia.com>
>+
>+        [WebXR] Move OpenXR calls off the main thread
>+        https://bugs.webkit.org/show_bug.cgi?id=217752
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The OpenXR API is synchronous. Many of the calls involve dealing with external hardware devices
>+        meaning that they have to potential to block the main thread. They should be moved to a different
>+        thread in order to avoid that.
>+
>+        The PlatformXR::Instance creates a WorkQueue which is going to be used by the OpenXR devices to
>+        issue OpenXR calls and also serialize them to ensure that they are executed sequentially. The
>+        OpenXRDevice's are created in the main thread anyway because we need to get weak pointers from
>+        them in the main thread.
>+
>+        * Modules/webxr/WebXRSystem.cpp:
>+        (WebCore::WebXRSystem::ensureImmersiveXRDeviceIsSelected): Use a scoped exit to call callback. Also
>+        the Vector of immersive devices is now a pointer which might be null, meaning no available devices.
>+        * platform/xr/PlatformXR.h: Added a "using" for the Vector of devices.
>+        * platform/xr/openxr/PlatformXROpenXR.cpp:
>+        (PlatformXR::Instance::Impl::queue const): New getter returning the WorkQueue.
>+        (PlatformXR::Instance::Impl::enumerateApiLayerProperties const): Added an ASSERT.
>+        (PlatformXR::Instance::Impl::checkInstanceExtensionProperties const): Ditto.
>+        (PlatformXR::Instance::Impl::Impl): Create the OpenXR WorkQueue and dispatch a task to perform the
>+        OpenXR system initialization in the queue.
>+        (PlatformXR::Instance::Impl::~Impl): Delete the instance in the WorkQueue
>+        (PlatformXR::Instance::enumerateImmersiveXRDevices): Moved the code to a task in the WorkQueue.
>+        (PlatformXR::OpenXRDevice::OpenXRDevice): Ditto.
>+        (PlatformXR::OpenXRDevice::collectSupportedSessionModes): Added an ASSERT.
>+        (PlatformXR::OpenXRDevice::collectConfigurationViews): Ditto.
>+        * platform/xr/openxr/PlatformXROpenXR.h: Added WorkQueue attribute and parameter to device constructor.
>+
> 2020-10-19  Sergio Villar Senin  <svillar@igalia.com>
> 
>         [css-flexbox] flex-shrink property should be animatable
>diff --git a/Source/WebCore/Modules/webxr/WebXRSystem.cpp b/Source/WebCore/Modules/webxr/WebXRSystem.cpp
>index 5ba47e2d9a0655eada4355d807aa2525e6db639f..12a9f21e724a12ddc15da9a84c1b82eb4d17d4d8 100644
>--- a/Source/WebCore/Modules/webxr/WebXRSystem.cpp
>+++ b/Source/WebCore/Modules/webxr/WebXRSystem.cpp
>@@ -88,20 +88,22 @@ void WebXRSystem::ensureImmersiveXRDeviceIsSelected(CompletionHandler<void()>&&
>     platformXR.enumerateImmersiveXRDevices([this, protectedThis = makeRef(*this), isFirstXRDevicesEnumeration, callback = WTFMove(callback)](auto& immersiveXRDevices) mutable {
>         m_immersiveXRDevicesHaveBeenEnumerated = true;
> 
>+        auto callbackOnExit = makeScopeExit([&]() {
>+            callOnMainThread(WTFMove(callback));
>+        });
>+
>         // https://immersive-web.github.io/webxr/#select-an-immersive-xr-device
>         auto* oldDevice = m_activeImmersiveDevice.get();
>         if (immersiveXRDevices.isEmpty()) {
>             m_activeImmersiveDevice = nullptr;
>-            callback();
>             return;
>         }
>         if (immersiveXRDevices.size() == 1) {
>             m_activeImmersiveDevice = makeWeakPtr(immersiveXRDevices.first().get());
>-            callback();
>             return;
>         }
> 
>-        if (m_activeImmersiveSession && oldDevice && immersiveXRDevices.findMatching([&](auto& entry) { return entry.get() == oldDevice; }) != notFound)
>+        if (m_activeImmersiveSession && oldDevice && immersiveXRDevices.findMatching([&](auto& entry) { return &entry == oldDevice; }) != notFound)
>             ASSERT(m_activeImmersiveDevice.get() == oldDevice);
>         else {
>             // FIXME: implement a better UA selection mechanism if required.
>@@ -109,15 +111,12 @@ void WebXRSystem::ensureImmersiveXRDeviceIsSelected(CompletionHandler<void()>&&
>         }
> 
>         if (isFirstXRDevicesEnumeration || m_activeImmersiveDevice.get() == oldDevice) {
>-            callback();
>             return;
>         }
> 
>         // FIXME: 7. Shut down any active XRSessions.
>         // FIXME: 8. Set the XR compatible boolean of all WebGLRenderingContextBase instances to false.
>         // FIXME: 9. Queue a task to fire an event named devicechange on the context object.
>-
>-        callback();
>     });
> }
> 
>diff --git a/Source/WebCore/platform/xr/PlatformXR.h b/Source/WebCore/platform/xr/PlatformXR.h
>index a71623ae4336dab07537fce7a2c6c4d09aa46326..e54e2624f25c9b6804b67bcd929ed6f9cdab7779 100644
>--- a/Source/WebCore/platform/xr/PlatformXR.h
>+++ b/Source/WebCore/platform/xr/PlatformXR.h
>@@ -75,7 +75,8 @@ class Instance {
> public:
>     static Instance& singleton();
> 
>-    void enumerateImmersiveXRDevices(CompletionHandler<void(const Vector<std::unique_ptr<Device>>&)>&&);
>+    using DeviceList = Vector<UniqueRef<Device>>;
>+    void enumerateImmersiveXRDevices(CompletionHandler<void(const DeviceList&)>&&);
> 
> private:
>     friend LazyNeverDestroyed<Instance>;
>@@ -85,7 +86,7 @@ private:
>     struct Impl;
>     UniqueRef<Impl> m_impl;
> 
>-    Vector<std::unique_ptr<Device>> m_immersiveXRDevices;
>+    DeviceList m_immersiveXRDevices;
> };
> 
> #endif // ENABLE(WEBXR)
>diff --git a/Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp b/Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp
>index f86b37ea2630bd922df2ad57fae4748b34991911..ecfed36558411a28edc3045c8af0183cf675063d 100644
>--- a/Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp
>+++ b/Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp
>@@ -73,6 +73,7 @@ public:
> 
> #if USE_OPENXR
>     XrInstance xrInstance() const { return m_instance; }
>+    WorkQueue& queue() const { return m_workQueue; }
> #endif
> 
> private:
>@@ -81,12 +82,14 @@ private:
>     bool checkInstanceExtensionProperties() const;
> 
>     XrInstance m_instance { XR_NULL_HANDLE };
>+    Ref<WorkQueue> m_workQueue;
> #endif // USE_OPENXR
> };
> 
> #if USE_OPENXR
> void Instance::Impl::enumerateApiLayerProperties() const
> {
>+    ASSERT(&RunLoop::current() == &m_workQueue->runLoop());
>     uint32_t propertyCountOutput { 0 };
>     XrResult result = xrEnumerateApiLayerProperties(0, &propertyCountOutput, nullptr);
>     RETURN_IF_FAILED(result, "xrEnumerateApiLayerProperties()", m_instance);
>@@ -119,6 +122,7 @@ static bool isExtensionSupported(const char* extensionName, Vector<XrExtensionPr
> 
> bool Instance::Impl::checkInstanceExtensionProperties() const
> {
>+    ASSERT(&RunLoop::current() == &m_workQueue->runLoop());
>     uint32_t propertyCountOutput { 0 };
>     XrResult result = xrEnumerateInstanceExtensionProperties(nullptr, 0, &propertyCountOutput, nullptr);
>     RETURN_IF_FAILED(result, "xrEnumerateInstanceExtensionProperties", m_instance, false);
>@@ -154,44 +158,51 @@ bool Instance::Impl::checkInstanceExtensionProperties() const
> #endif // USE_OPENXR
> 
> Instance::Impl::Impl()
>+#if USE_OPENXR
>+    : m_workQueue(WorkQueue::create("OpenXR queue"))
>+#endif
> {
> #if USE_OPENXR
>-    LOG(XR, "OpenXR: initializing\n");
>+    m_workQueue->dispatch([this]() {
>+        LOG(XR, "OpenXR: initializing\n");
> 
>-    enumerateApiLayerProperties();
>+        enumerateApiLayerProperties();
> 
>-    if (!checkInstanceExtensionProperties())
>-        return;
>+        if (!checkInstanceExtensionProperties())
>+            return;
>+
>+        static const char* s_applicationName = "WebXR (WebKit)";
>+        static const uint32_t s_applicationVersion = 1;
>+
>+        const char* const enabledExtensions[] = {
>+            XR_MND_HEADLESS_EXTENSION_NAME,
>+        };
> 
>-    static const char* s_applicationName = "WebXR (WebKit)";
>-    static const uint32_t s_applicationVersion = 1;
>-
>-    const char* const enabledExtensions[] = {
>-        XR_MND_HEADLESS_EXTENSION_NAME,
>-    };
>-
>-    auto createInfo = createStructure<XrInstanceCreateInfo, XR_TYPE_INSTANCE_CREATE_INFO>();
>-    createInfo.createFlags = 0;
>-    std::memcpy(createInfo.applicationInfo.applicationName, s_applicationName, XR_MAX_APPLICATION_NAME_SIZE);
>-    createInfo.applicationInfo.apiVersion = XR_CURRENT_API_VERSION;
>-    createInfo.applicationInfo.applicationVersion = s_applicationVersion;
>-    createInfo.enabledApiLayerCount = 0;
>-    createInfo.enabledExtensionCount = WTF_ARRAY_LENGTH(enabledExtensions);
>-    createInfo.enabledExtensionNames = enabledExtensions;
>-
>-    XrInstance instance;
>-    XrResult result = xrCreateInstance(&createInfo, &instance);
>-    RETURN_IF_FAILED(result, "xrCreateInstance()", m_instance);
>-    m_instance = instance;
>-    LOG(XR, "xrCreateInstance(): using instance %p\n", m_instance);
>+        auto createInfo = createStructure<XrInstanceCreateInfo, XR_TYPE_INSTANCE_CREATE_INFO>();
>+        createInfo.createFlags = 0;
>+        std::memcpy(createInfo.applicationInfo.applicationName, s_applicationName, XR_MAX_APPLICATION_NAME_SIZE);
>+        createInfo.applicationInfo.apiVersion = XR_CURRENT_API_VERSION;
>+        createInfo.applicationInfo.applicationVersion = s_applicationVersion;
>+        createInfo.enabledApiLayerCount = 0;
>+        createInfo.enabledExtensionCount = WTF_ARRAY_LENGTH(enabledExtensions);
>+        createInfo.enabledExtensionNames = enabledExtensions;
>+
>+        XrInstance instance;
>+        XrResult result = xrCreateInstance(&createInfo, &instance);
>+        RETURN_IF_FAILED(result, "xrCreateInstance()", m_instance);
>+        m_instance = instance;
>+        LOG(XR, "xrCreateInstance(): using instance %p\n", m_instance);
>+    });
> #endif // USE_OPENXR
> }
> 
> Instance::Impl::~Impl()
> {
> #if USE_OPENXR
>-    if (m_instance != XR_NULL_HANDLE)
>-        xrDestroyInstance(m_instance);
>+    m_workQueue->dispatch([this] {
>+        if (m_instance != XR_NULL_HANDLE)
>+            xrDestroyInstance(m_instance);
>+    });
> #endif
> }
> 
>@@ -211,51 +222,68 @@ Instance::Instance()
> {
> }
> 
>-void Instance::enumerateImmersiveXRDevices(CompletionHandler<void(const Vector<std::unique_ptr<Device>>& devices)>&& callback)
>+void Instance::enumerateImmersiveXRDevices(CompletionHandler<void(const DeviceList& devices)>&& callback)
> {
> #if USE_OPENXR
>-    auto callbackOnExit = makeScopeExit([&]() {
>-        callback({ });
>-    });
>-    if (m_impl->xrInstance() == XR_NULL_HANDLE) {
>-        LOG(XR, "%s Unable to enumerate XR devices. No XrInstance present\n", __FUNCTION__);
>-        return;
>-    }
>+    m_impl->queue().dispatch([this, callback = WTFMove(callback)]() mutable {
>+        auto callbackOnExit = makeScopeExit([&]() {
>+            callOnMainThread([callback = WTFMove(callback)]() mutable {
>+                callback({ });
>+            });
>+        });
> 
>-    auto systemGetInfo = createStructure<XrSystemGetInfo, XR_TYPE_SYSTEM_GET_INFO>();
>-    systemGetInfo.formFactor = XR_FORM_FACTOR_HEAD_MOUNTED_DISPLAY;
>+        m_immersiveXRDevices.clear();
> 
>-    XrSystemId systemId;
>-    XrResult result = xrGetSystem(m_impl->xrInstance(), &systemGetInfo, &systemId);
>-    RETURN_IF_FAILED(result, "xrGetSystem", m_impl->xrInstance());
>+        if (m_impl->xrInstance() == XR_NULL_HANDLE) {
>+            LOG(XR, "%s Unable to enumerate XR devices. No XrInstance present\n", __FUNCTION__);
>+            return;
>+        }
> 
>-#if !LOG_DISABLED
>-    auto systemProperties = createStructure<XrSystemProperties, XR_TYPE_SYSTEM_PROPERTIES>();
>-    result = xrGetSystemProperties(m_impl->xrInstance(), systemId, &systemProperties);
>-    if (result == XR_SUCCESS)
>-        LOG(XR, "Found XRSystem %lu: \"%s\", vendor ID %d\n", systemProperties.systemId, systemProperties.systemName, systemProperties.vendorId);
>-#endif
>+        auto systemGetInfo = createStructure<XrSystemGetInfo, XR_TYPE_SYSTEM_GET_INFO>();
>+        systemGetInfo.formFactor = XR_FORM_FACTOR_HEAD_MOUNTED_DISPLAY;
>+
>+        XrSystemId systemId;
>+        XrResult result = xrGetSystem(m_impl->xrInstance(), &systemGetInfo, &systemId);
>+        RETURN_IF_FAILED(result, "xrGetSystem", m_impl->xrInstance());
>+
>+        callbackOnExit.release();
>+
>+        callOnMainThread([this, callback = WTFMove(callback), systemId]() mutable {
>+            // Create devices in the main thread but do not call the callback immediately as the device needs to initialize
>+            // some stuff in the work queue. That's why we need to queue another task to call the callback after the
>+            // device initialization is completed.
>+            m_immersiveXRDevices.append(makeUniqueRef<OpenXRDevice>(systemId, m_impl->xrInstance(), m_impl->queue()));
>+            m_impl->queue().dispatch([this, callback = WTFMove(callback)]() mutable {
>+                callOnMainThread([this, callback = WTFMove(callback)]() mutable {
>+                    callback(m_immersiveXRDevices);
>+                });
>+            });
>+        });
>+    });
> 
>-    m_immersiveXRDevices.append(makeUnique<OpenXRDevice>(systemId, m_impl->xrInstance()));
>-    callback(m_immersiveXRDevices);
>-    callbackOnExit.release();
> #endif // USE_OPENXR
> }
> 
> #if USE_OPENXR
>-OpenXRDevice::OpenXRDevice(XrSystemId id, XrInstance instance)
>+OpenXRDevice::OpenXRDevice(XrSystemId id, XrInstance instance, WorkQueue& queue)
>     : m_systemId(id)
>     , m_instance(instance)
>+    , m_queue(queue)
> {
>-    auto systemProperties = createStructure<XrSystemProperties, XR_TYPE_SYSTEM_PROPERTIES>();
>-    XrResult result = xrGetSystemProperties(instance, m_systemId, &systemProperties);
>-    if (result == XR_SUCCESS)
>-        m_supportsOrientationTracking = systemProperties.trackingProperties.orientationTracking == XR_TRUE;
>-    else
>-        LOG(XR, "xrGetSystemProperties(): error %s\n", resultToString(result, m_instance).utf8().data());
>+    m_queue.dispatch([this]() {
>+        auto systemProperties = createStructure<XrSystemProperties, XR_TYPE_SYSTEM_PROPERTIES>();
>+        auto result = xrGetSystemProperties(m_instance, m_systemId, &systemProperties);
>+        if (!XR_FAILED(result))
>+            m_supportsOrientationTracking = systemProperties.trackingProperties.orientationTracking == XR_TRUE;
>+#if !LOG_DISABLED
>+        else
>+            LOG(XR, "xrGetSystemProperties(): error %s\n", resultToString(result, m_instance).utf8().data());
>+        LOG(XR, "Found XRSystem %lu: \"%s\", vendor ID %d\n", systemProperties.systemId, systemProperties.systemName, systemProperties.vendorId);
>+#endif
> 
>-    collectSupportedSessionModes();
>-    collectConfigurationViews();
>+        collectSupportedSessionModes();
>+        collectConfigurationViews();
>+    });
> }
> 
> Device::ListOfEnabledFeatures OpenXRDevice::enumerateReferenceSpaces(XrSession& session) const
>@@ -299,6 +327,7 @@ Device::ListOfEnabledFeatures OpenXRDevice::enumerateReferenceSpaces(XrSession&
> 
> void OpenXRDevice::collectSupportedSessionModes()
> {
>+    ASSERT(&RunLoop::current() == &m_queue.runLoop());
>     uint32_t viewConfigurationCount;
>     auto result = xrEnumerateViewConfigurations(m_instance, m_systemId, 0, &viewConfigurationCount, nullptr);
>     RETURN_IF_FAILED(result, "xrEnumerateViewConfigurations", m_instance);
>@@ -341,6 +370,7 @@ void OpenXRDevice::collectSupportedSessionModes()
> 
> void OpenXRDevice::collectConfigurationViews()
> {
>+    ASSERT(&RunLoop::current() == &m_queue.runLoop());
>     for (auto& config : m_viewConfigurationProperties.values()) {
>         uint32_t viewCount;
>         auto configType = config.viewConfigurationType;
>diff --git a/Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h b/Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h
>index c2815023a272a342ff2a9ac482569d503615837b..549a505d5ef13e7c17571d03da122b1df418d3a3 100644
>--- a/Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h
>+++ b/Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h
>@@ -26,6 +26,7 @@
> 
> #if USE_OPENXR
> #include <openxr/openxr.h>
>+#include <wtf/WorkQueue.h>
> 
> namespace PlatformXR {
> 
>@@ -43,7 +44,7 @@ namespace PlatformXR {
> // the XRSystem is basically the entry point for the WebXR API available via the Navigator object.
> class OpenXRDevice final : public Device {
> public:
>-    OpenXRDevice(XrSystemId, XrInstance);
>+    OpenXRDevice(XrSystemId, XrInstance, WorkQueue&);
>     XrSystemId xrSystemId() const { return m_systemId; }
> private:
>     void collectSupportedSessionModes();
>@@ -61,6 +62,8 @@ private:
>     XrSystemId m_systemId;
>     XrInstance m_instance;
>     XrSession m_session;
>+
>+    WorkQueue& m_queue;
> };
> 
> } // namespace PlatformXR

Actions: View | Formatted Diff | Diff

Attachments on bug 217752: 411442 | 411443 | 411747 | 411853 | 411967 | 412120