From 35e5178f0e50661c1f17ae11067d9205eef2ef8b Mon Sep 17 00:00:00 2001 From: Seungha Yang Date: Sun, 4 Feb 2024 23:25:19 +0900 Subject: [PATCH] d3d12: Fix potential self thread join Fence data could hold GstD3D12Device directly or indirectly. Then if it's holding last refcount, the device object will be released from the device object's internal thread, and will try join self thread. Delegates it to other global background thread to avoid self thread joining. Part-of: --- .../sys/d3d12/gstd3d12-private.h | 3 + .../sys/d3d12/gstd3d12commandqueue.cpp | 4 +- .../sys/d3d12/gstd3d12device.cpp | 310 ++++++++++++------ .../gst-plugins-bad/sys/d3d12/plugin.cpp | 11 + 4 files changed, 235 insertions(+), 93 deletions(-) diff --git a/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12-private.h b/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12-private.h index 9ff1cb2ee4..496b322501 100644 --- a/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12-private.h +++ b/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12-private.h @@ -192,3 +192,6 @@ static const GstD3D12Format g_gst_d3d12_default_format_map[] = { void gst_d3d12_device_clear_yuv_texture (GstD3D12Device * device, GstMemory * mem); +void gst_d3d12_init_background_thread (void); + +void gst_d3d12_sync_background_thread (void); diff --git a/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12commandqueue.cpp b/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12commandqueue.cpp index b52a61d529..3ee4cafff0 100644 --- a/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12commandqueue.cpp +++ b/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12commandqueue.cpp @@ -22,6 +22,7 @@ #endif #include "gstd3d12.h" +#include "gstd3d12-private.h" #include #include #include @@ -396,12 +397,13 @@ gst_d3d12_command_queue_set_notify (GstD3D12CommandQueue * queue, std::lock_guard < std::mutex > lk (priv->lock); if (!priv->gc_thread) { + gst_d3d12_init_background_thread (); priv->gc_thread = g_thread_new ("GstD3D12Gc", (GThreadFunc) gst_d3d12_command_queue_gc_thread, queue); } GST_LOG_OBJECT (queue, "Pushing GC data %" G_GUINT64_FORMAT, fence_value); - priv->gc_list.push (gc_data); + priv->gc_list.push (std::move (gc_data)); priv->cond.notify_one (); } diff --git a/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12device.cpp b/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12device.cpp index a9e74dff68..c552fd4dd7 100644 --- a/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12device.cpp +++ b/subprojects/gst-plugins-bad/sys/d3d12/gstd3d12device.cpp @@ -38,6 +38,7 @@ #include #include #include +#include GST_DEBUG_CATEGORY_STATIC (gst_d3d12_device_debug); GST_DEBUG_CATEGORY_STATIC (gst_d3d12_sdk_debug); @@ -54,12 +55,7 @@ enum PROP_DESCRIPTION, }; -/* d3d12 devices are singtones per adapter. Keep track of live objects and - * reuse already created object if possible */ /* *INDENT-OFF* */ -std::mutex device_list_lock_; -std::vector live_devices_; - using namespace Microsoft::WRL; struct _GstD3D12DevicePrivate @@ -152,8 +148,214 @@ struct _GstD3D12DevicePrivate std::string description; gint64 adapter_luid = 0; }; + +enum GstD3D12DeviceConstructType +{ + GST_D3D12_DEVICE_CONSTRUCT_FOR_INDEX, + GST_D3D12_DEVICE_CONSTRUCT_FOR_LUID, +}; + +struct GstD3D12DeviceConstructData +{ + union + { + guint index; + gint64 luid; + } data; + GstD3D12DeviceConstructType type; +}; + +static GstD3D12Device * +gst_d3d12_device_new_internal (const GstD3D12DeviceConstructData * data); + +struct DeviceCache +{ + ~DeviceCache() + { + if (priv) + delete priv; + } + + GstD3D12Device *device = nullptr; + GstD3D12DevicePrivate *priv = nullptr; + guint64 token = 0; +}; + +/* Because ID3D12Device instance is a singleton per adapter, + * this DeviceCacheManager object will cache GstD3D12Device object and + * will return the same GstD3D12Device object for create request + * if instanted object exists already. + * + * Another role of this object dtor thread management. + * GstD3D12CommandQueue object held by GstD3D12Device will run background + * garbage collection thread and releasing garbage collection data + * could result in releasing GstD3D12Device object, which can cause self-thread + * joining. This manager will run one background thread to avoid it + */ +class DeviceCacheManager +{ +public: + DeviceCacheManager (const DeviceCacheManager &) = delete; + DeviceCacheManager& operator= (const DeviceCacheManager &) = delete; + static DeviceCacheManager * GetInstance() + { + static DeviceCacheManager *inst = nullptr; + GST_D3D12_CALL_ONCE_BEGIN { + inst = new DeviceCacheManager (); + } GST_D3D12_CALL_ONCE_END; + + return inst; + } + + void InitThread () + { + std::lock_guard lk (lock_); + if (!thread_) + thread_ = new std::thread (&DeviceCacheManager::threadFunc, this); + } + + void Sync () + { + guint64 to_wait = 0; + + { + std::lock_guard lk (lock_); + if (!thread_) + return; + + token_++; + to_wait = token_; + + auto empty_item = std::make_shared (); + empty_item->token = to_wait; + + std::lock_guard olk (thread_lock_); + to_remove_.push (std::move (empty_item)); + thread_cond_.notify_one (); + } + + std::unique_lock olk (token_lock_); + while (token_synced_ < to_wait) + token_cond_.wait (olk); + } + + GstD3D12Device * Create (const GstD3D12DeviceConstructData * data) + { + std::lock_guard lk (lock_); + auto it = std::find_if (list_.begin (), list_.end (), + [&] (const auto & cache) { + const auto priv = cache->priv; + if (data->type == GST_D3D12_DEVICE_CONSTRUCT_FOR_INDEX) + return priv->adapter_index == data->data.index; + + return priv->adapter_luid == data->data.luid; + }); + + if (it != list_.end ()) + return (GstD3D12Device *) gst_object_ref ((*it)->device); + + auto device = gst_d3d12_device_new_internal (data); + if (!device) + return nullptr; + + gst_object_ref_sink (device); + + auto item = std::make_shared (); + item->device = device; + item->priv = device->priv; + + g_object_weak_ref (G_OBJECT (device), DeviceCacheManager::NotifyCb, this); + list_.push_back (item); + + return device; + } + + static void NotifyCb (gpointer data, GObject * device) + { + auto self = (DeviceCacheManager *) data; + self->remove ((GstD3D12Device *) device); + } + +private: + DeviceCacheManager () {} + ~DeviceCacheManager () {} + + void threadFunc () + { + while (true) { + std::unique_lock lk (thread_lock_); + while (to_remove_.empty ()) + thread_cond_.wait (lk); + + while (!to_remove_.empty ()) { + guint64 token; + + { + auto item = to_remove_.front (); + to_remove_.pop (); + token = item->token; + } + + std::lock_guard olk (token_lock_); + token_synced_ = token; + token_cond_.notify_all (); + } + } + } + + void remove (GstD3D12Device * device) + { + std::lock_guard lk (lock_); + auto it = std::find_if (list_.begin (), list_.end (), + [&] (const auto & cache) { + return cache->device == device; + }); + + std::shared_ptr cached; + if (it != list_.end ()) { + cached = *it; + list_.erase (it); + } else { + GST_WARNING ("Couldn't find device from cache"); + } + + if (cached && thread_) { + std::lock_guard tlk (thread_lock_); + token_++; + cached->token = token_; + to_remove_.push (std::move (cached)); + thread_cond_.notify_one (); + } + } + +private: + std::mutex lock_; + std::vector> list_; + std::mutex thread_lock_; + std::condition_variable thread_cond_; + std::thread *thread_ = nullptr; + std::queue> to_remove_; + std::mutex token_lock_; + std::condition_variable token_cond_; + guint64 token_ = 0; + guint64 token_synced_ = 0; +}; /* *INDENT-ON* */ +void +gst_d3d12_init_background_thread (void) +{ + auto manager = DeviceCacheManager::GetInstance (); + manager->InitThread (); +} + +void +gst_d3d12_sync_background_thread (void) +{ + auto manager = DeviceCacheManager::GetInstance (); + manager->Sync (); +} + static gboolean gst_d3d12_device_enable_debug (void) { @@ -260,7 +462,7 @@ gst_d3d12_device_finalize (GObject * object) GST_DEBUG_OBJECT (self, "Finalize"); - delete self->priv; + /* Don't delete private struct. DeviceCacheManager will destroy it */ G_OBJECT_CLASS (parent_class)->finalize (object); } @@ -463,33 +665,6 @@ gst_d3d12_device_setup_format_table (GstD3D12Device * self) } } -static void -gst_d3d12_device_weak_ref_notify (gpointer data, GstD3D12Device * device) -{ - std::lock_guard < std::mutex > lk (device_list_lock_); - auto it = std::find (live_devices_.begin (), live_devices_.end (), device); - if (it != live_devices_.end ()) - live_devices_.erase (it); - else - GST_WARNING ("Could not find %p from list", device); -} - -typedef enum -{ - GST_D3D12_DEVICE_CONSTRUCT_FOR_INDEX, - GST_D3D12_DEVICE_CONSTRUCT_FOR_LUID, -} GstD3D12DeviceConstructType; - -typedef struct -{ - union - { - guint index; - gint64 luid; - } data; - GstD3D12DeviceConstructType type; -} GstD3D12DeviceConstructData; - static HRESULT gst_d3d12_device_find_adapter (const GstD3D12DeviceConstructData * data, IDXGIFactory2 * factory, guint * index, IDXGIAdapter1 ** rst) @@ -665,72 +840,23 @@ error: GstD3D12Device * gst_d3d12_device_new (guint adapter_index) { - GstD3D12Device *self = nullptr; - std::lock_guard < std::mutex > lk (device_list_lock_); + auto manager = DeviceCacheManager::GetInstance (); + GstD3D12DeviceConstructData data; + data.data.index = adapter_index; + data.type = GST_D3D12_DEVICE_CONSTRUCT_FOR_INDEX; - /* *INDENT-OFF* */ - for (auto iter: live_devices_) { - if (iter->priv->adapter_index == adapter_index) { - self = (GstD3D12Device *) gst_object_ref (iter); - break; - } - } - /* *INDENT-ON* */ - - if (!self) { - GstD3D12DeviceConstructData data; - data.data.index = adapter_index; - data.type = GST_D3D12_DEVICE_CONSTRUCT_FOR_INDEX; - - self = gst_d3d12_device_new_internal (&data); - if (!self) { - GST_INFO ("Could not create device for index %d", adapter_index); - return nullptr; - } - - gst_object_ref_sink (self); - g_object_weak_ref (G_OBJECT (self), - (GWeakNotify) gst_d3d12_device_weak_ref_notify, nullptr); - live_devices_.push_back (self); - } - - return self; + return manager->Create (&data); } GstD3D12Device * gst_d3d12_device_new_for_adapter_luid (gint64 adapter_luid) { - GstD3D12Device *self = nullptr; - std::lock_guard < std::mutex > lk (device_list_lock_); + auto manager = DeviceCacheManager::GetInstance (); + GstD3D12DeviceConstructData data; + data.data.luid = adapter_luid; + data.type = GST_D3D12_DEVICE_CONSTRUCT_FOR_LUID; - /* *INDENT-OFF* */ - for (auto iter: live_devices_) { - if (iter->priv->adapter_luid == adapter_luid) { - self = (GstD3D12Device *) gst_object_ref (iter); - break; - } - } - /* *INDENT-ON* */ - - if (!self) { - GstD3D12DeviceConstructData data; - data.data.luid = adapter_luid; - data.type = GST_D3D12_DEVICE_CONSTRUCT_FOR_LUID; - - self = gst_d3d12_device_new_internal (&data); - if (!self) { - GST_INFO ("Could not create device for LUID %" G_GINT64_FORMAT, - adapter_luid); - return nullptr; - } - - gst_object_ref_sink (self); - g_object_weak_ref (G_OBJECT (self), - (GWeakNotify) gst_d3d12_device_weak_ref_notify, nullptr); - live_devices_.push_back (self); - } - - return self; + return manager->Create (&data); } ID3D12Device * diff --git a/subprojects/gst-plugins-bad/sys/d3d12/plugin.cpp b/subprojects/gst-plugins-bad/sys/d3d12/plugin.cpp index 5c3848ca47..105310b9cc 100644 --- a/subprojects/gst-plugins-bad/sys/d3d12/plugin.cpp +++ b/subprojects/gst-plugins-bad/sys/d3d12/plugin.cpp @@ -29,6 +29,7 @@ #include #include "gstd3d12.h" +#include "gstd3d12-private.h" #include "gstd3d12convert.h" #include "gstd3d12download.h" #include "gstd3d12upload.h" @@ -59,6 +60,12 @@ GST_DEBUG_CATEGORY (gst_d3d12_utils_debug); #define GST_CAT_DEFAULT gst_d3d12_debug +static void +plugin_deinit (gpointer data) +{ + gst_d3d12_sync_background_thread (); +} + static gboolean plugin_init (GstPlugin * plugin) { @@ -134,6 +141,10 @@ plugin_init (GstPlugin * plugin) "d3d12screencapturedeviceprovider", GST_RANK_PRIMARY, GST_TYPE_D3D12_SCREEN_CAPTURE_DEVICE_PROVIDER); + g_object_set_data_full (G_OBJECT (plugin), + "plugin-d3d12-shutdown", (gpointer) "shutdown-data", + (GDestroyNotify) plugin_deinit); + return TRUE; }