Attachment 411442 Details for Bug 217752 – Patch

[patch] Patch

bug-217752-20201015164241.patch (text/plain), 17.03 KB, created by Sergio Villar Senin on 2020-10-15 07:42:43 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Sergio Villar Senin

Created: 2020-10-15 07:42:43 PDT

Size: 17.03 KB

patch

obsolete

>Subversion Revision: 268516
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index e1877a3bc9b2a4b35b4f1336d04152057037c5bd..ff5a951322e477496d16b3af32c313b50493dc84 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-14  Sergio Villar Senin  <svillar@igalia.com>
> 
>         [css-flexbox] flex-grow property should be animatable
>diff --git a/Source/WebCore/Modules/webxr/WebXRSystem.cpp b/Source/WebCore/Modules/webxr/WebXRSystem.cpp
>index b47a457c5446d22352ab0c10e66f27dd1a7d32b3..d5b4d4c21e72d8f44858e25cecbf0d9b7d3e4f5a 100644
>--- a/Source/WebCore/Modules/webxr/WebXRSystem.cpp
>+++ b/Source/WebCore/Modules/webxr/WebXRSystem.cpp
>@@ -85,39 +85,40 @@ void WebXRSystem::ensureImmersiveXRDeviceIsSelected(CompletionHandler<void()>&&
>     // https://immersive-web.github.io/webxr/#enumerate-immersive-xr-devices
>     auto& platformXR = PlatformXR::Instance::singleton();
>     bool isFirstXRDevicesEnumeration = !m_immersiveXRDevicesHaveBeenEnumerated;
>-    platformXR.enumerateImmersiveXRDevices([this, protectedThis = makeRef(*this), isFirstXRDevicesEnumeration, callback = WTFMove(callback)](auto& immersiveXRDevices) mutable {
>+    platformXR.enumerateImmersiveXRDevices([this, protectedThis = makeRef(*this), isFirstXRDevicesEnumeration, callback = WTFMove(callback)](auto* immersiveXRDevices) mutable {
>         m_immersiveXRDevicesHaveBeenEnumerated = true;
> 
>+        auto callbackOnExit = makeScopeExit([&]() {
>+            callOnMainThread([callback = WTFMove(callback)]() mutable {
>+                callback();
>+            });
>+        });
>+
>         // https://immersive-web.github.io/webxr/#select-an-immersive-xr-device
>         auto* oldDevice = m_activeImmersiveDevice.get();
>-        if (immersiveXRDevices.isEmpty()) {
>+        if (!immersiveXRDevices) {
>             m_activeImmersiveDevice = nullptr;
>-            callback();
>             return;
>         }
>-        if (immersiveXRDevices.size() == 1) {
>-            m_activeImmersiveDevice = makeWeakPtr(immersiveXRDevices.first().get());
>-            callback();
>+        if (immersiveXRDevices->size() == 1) {
>+            m_activeImmersiveDevice = makeWeakPtr(immersiveXRDevices->first().get());
>             return;
>         }
> 
>-        if (m_activeImmersiveSession && oldDevice && immersiveXRDevices.findMatching([&](auto& entry) { return entry.get() == oldDevice; }) != notFound)
>+        if (m_activeImmersiveSession && oldDevice && immersiveXRDevices->findMatching([&](auto& entry) { return entry.get() == oldDevice; }) != notFound)
>             ASSERT(m_activeImmersiveDevice.get() == oldDevice);
>         else {
>             // FIXME: implement a better UA selection mechanism if required.
>-            m_activeImmersiveDevice = makeWeakPtr(immersiveXRDevices.first().get());
>+            m_activeImmersiveDevice = makeWeakPtr(immersiveXRDevices->first().get());
>         }
> 
>         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..6dbe49bbec47d6388f28972d48980d85837de572 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<std::unique_ptr<Device>>;
>+    void enumerateImmersiveXRDevices(CompletionHandler<void(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..09d0c0b619349a87c57b3f4b99be4ee9c478bca0 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,52 @@ bool Instance::Impl::checkInstanceExtensionProperties() const
> #endif // USE_OPENXR
> 
> Instance::Impl::Impl()
>-{
> #if USE_OPENXR
>-    LOG(XR, "OpenXR: initializing\n");
>+    : m_workQueue(WorkQueue::create("OpenXR queue"))
>+{
>+    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,
>-    };
>-
>-    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
>+        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);
>+    });
> }
>+#else
>+{
>+}
>+#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 +223,68 @@ Instance::Instance()
> {
> }
> 
>-void Instance::enumerateImmersiveXRDevices(CompletionHandler<void(const Vector<std::unique_ptr<Device>>& devices)>&& callback)
>+void Instance::enumerateImmersiveXRDevices(CompletionHandler<void(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(nullptr);
>+            });
>+        });
> 
>-    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());
>+
>+        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(makeUnique<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);
>+                });
>+            });
>+        });
>+
>+        callbackOnExit.release();
>+    });
> 
>-    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 +328,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 +371,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..01cd15937ee3c9f0336a04bded34de463b432d1d 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/Forward.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