Merge pull request #11891 from iwubcode/asset_thread_safety

VideoCommon: add additional locks around asset access / usage

Source/Core/VideoCommon/Assets/CustomAsset.cpp

@@ -16,6 +16,7 @@ bool CustomAsset::Load()
   const auto load_information = LoadImpl(m_asset_id);
   if (load_information.m_bytes_loaded > 0)
   {
+    std::lock_guard lk(m_info_lock);
     m_bytes_loaded = load_information.m_bytes_loaded;
     m_last_loaded_time = load_information.m_load_time;
   }
@@ -29,6 +30,7 @@ CustomAssetLibrary::TimeType CustomAsset::GetLastWriteTime() const
 
 const CustomAssetLibrary::TimeType& CustomAsset::GetLastLoadedTime() const
 {
+  std::lock_guard lk(m_info_lock);
   return m_last_loaded_time;
 }
 
@@ -39,6 +41,7 @@ const CustomAssetLibrary::AssetID& CustomAsset::GetAssetId() const
 
 std::size_t CustomAsset::GetByteSizeInMemory() const
 {
+  std::lock_guard lk(m_info_lock);
   return m_bytes_loaded;
 }
 

Source/Core/VideoCommon/Assets/CustomAsset.h

@@ -30,6 +30,7 @@ public:
 
   // Queries the last time the asset was modified or standard epoch time
   // if the asset hasn't been modified yet
+  // Note: not thread safe, expected to be called by the loader
   CustomAssetLibrary::TimeType GetLastWriteTime() const;
 
   // Returns the time that the data was last loaded
@@ -48,8 +49,10 @@ protected:
 private:
   virtual CustomAssetLibrary::LoadInfo LoadImpl(const CustomAssetLibrary::AssetID& asset_id) = 0;
   CustomAssetLibrary::AssetID m_asset_id;
+
+  mutable std::mutex m_info_lock;
   std::size_t m_bytes_loaded = 0;
-  CustomAssetLibrary::TimeType m_last_loaded_time;
+  CustomAssetLibrary::TimeType m_last_loaded_time = {};
 };
 
 // An abstract class that is expected to
@@ -70,7 +73,7 @@ public:
   // they want to handle reloads
   [[nodiscard]] std::shared_ptr<UnderlyingType> GetData() const
   {
-    std::lock_guard lk(m_lock);
+    std::lock_guard lk(m_data_lock);
     if (m_loaded)
       return m_data;
     return nullptr;
@@ -78,7 +81,7 @@ public:
 
 protected:
   bool m_loaded = false;
-  mutable std::mutex m_lock;
+  mutable std::mutex m_data_lock;
   std::shared_ptr<UnderlyingType> m_data;
 };
 

Source/Core/VideoCommon/Assets/CustomAssetLoader.cpp

@@ -30,7 +30,7 @@ void CustomAssetLoader::Init()
 
       std::this_thread::sleep_for(TIME_BETWEEN_ASSET_MONITOR_CHECKS);
 
-      std::lock_guard lk(m_assets_lock);
+      std::lock_guard lk(m_asset_load_lock);
       for (auto& [asset_id, asset_to_monitor] : m_assets_to_monitor)
       {
         if (auto ptr = asset_to_monitor.lock())
@@ -50,11 +50,11 @@ void CustomAssetLoader::Init()
     {
       if (ptr->Load())
       {
-        if (m_max_memory_available >= m_total_bytes_loaded + ptr->GetByteSizeInMemory())
+        std::lock_guard lk(m_asset_load_lock);
+        const std::size_t asset_memory_size = ptr->GetByteSizeInMemory();
+        if (m_max_memory_available >= m_total_bytes_loaded + asset_memory_size)
         {
-          m_total_bytes_loaded += ptr->GetByteSizeInMemory();
-
-          std::lock_guard lk(m_assets_lock);
+          m_total_bytes_loaded += asset_memory_size;
           m_assets_to_monitor.try_emplace(ptr->GetAssetId(), ptr);
         }
         else

Source/Core/VideoCommon/Assets/CustomAssetLoader.h

@@ -52,12 +52,17 @@ private:
   {
     auto [it, inserted] = asset_map.try_emplace(asset_id);
     if (!inserted)
-      return it->second.lock();
+    {
+      auto shared = it->second.lock();
+      if (shared)
+        return shared;
+    }
     std::shared_ptr<AssetType> ptr(new AssetType(std::move(library), asset_id), [&](AssetType* a) {
-      asset_map.erase(a->GetAssetId());
-      m_total_bytes_loaded -= a->GetByteSizeInMemory();
-      std::lock_guard lk(m_assets_lock);
-      m_assets_to_monitor.erase(a->GetAssetId());
+      {
+        std::lock_guard lk(m_asset_load_lock);
+        m_total_bytes_loaded -= a->GetByteSizeInMemory();
+        m_assets_to_monitor.erase(a->GetAssetId());
+      }
       delete a;
     });
     it->second = ptr;
@@ -66,6 +71,7 @@ private:
   }
 
   static constexpr auto TIME_BETWEEN_ASSET_MONITOR_CHECKS = std::chrono::milliseconds{500};
+
   std::map<CustomAssetLibrary::AssetID, std::weak_ptr<RawTextureAsset>> m_textures;
   std::map<CustomAssetLibrary::AssetID, std::weak_ptr<GameTextureAsset>> m_game_textures;
   std::thread m_asset_monitor_thread;
@@ -75,7 +81,10 @@ private:
   std::size_t m_max_memory_available = 0;
 
   std::map<CustomAssetLibrary::AssetID, std::weak_ptr<CustomAsset>> m_assets_to_monitor;
-  std::mutex m_assets_lock;
+
+  // Use a recursive mutex to handle the scenario where an asset goes out of scope while
+  // iterating over the assets to monitor which calls the lock above in 'LoadOrCreateAsset'
+  std::recursive_mutex m_asset_load_lock;
   Common::WorkQueueThread<std::weak_ptr<CustomAsset>> m_asset_load_thread;
 };
 }  // namespace VideoCommon

Source/Core/VideoCommon/Assets/DirectFilesystemAssetLibrary.cpp

@@ -27,6 +27,7 @@ std::size_t GetAssetSize(const CustomTextureData& data)
 CustomAssetLibrary::TimeType
 DirectFilesystemAssetLibrary::GetLastAssetWriteTime(const AssetID& asset_id) const
 {
+  std::lock_guard lk(m_lock);
   if (auto iter = m_assetid_to_asset_map_path.find(asset_id);
       iter != m_assetid_to_asset_map_path.end())
   {
@@ -47,50 +48,47 @@ DirectFilesystemAssetLibrary::GetLastAssetWriteTime(const AssetID& asset_id) con
 CustomAssetLibrary::LoadInfo DirectFilesystemAssetLibrary::LoadTexture(const AssetID& asset_id,
                                                                        CustomTextureData* data)
 {
-  if (auto iter = m_assetid_to_asset_map_path.find(asset_id);
-      iter != m_assetid_to_asset_map_path.end())
+  const auto asset_map = GetAssetMapForID(asset_id);
+
+  // Raw texture is expected to have one asset mapped
+  if (asset_map.empty() || asset_map.size() > 1)
+    return {};
+  const auto& asset_path = asset_map.begin()->second;
+
+  const auto last_loaded_time = std::filesystem::last_write_time(asset_path);
+  auto ext = asset_path.extension().string();
+  Common::ToLower(&ext);
+  if (ext == ".dds")
   {
-    const auto& asset_map_path = iter->second;
-
-    // Raw texture is expected to have one asset mapped
-    if (asset_map_path.empty() || asset_map_path.size() > 1)
+    LoadDDSTexture(data, asset_path.string());
+    if (data->m_levels.empty()) [[unlikely]]
+      return {};
+    if (!LoadMips(asset_path, data))
       return {};
-    const auto& asset_path = asset_map_path.begin()->second;
 
-    const auto last_loaded_time = std::filesystem::last_write_time(asset_path);
-    auto ext = asset_path.extension().string();
-    Common::ToLower(&ext);
-    if (ext == ".dds")
-    {
-      LoadDDSTexture(data, asset_path.string());
-      if (data->m_levels.empty()) [[unlikely]]
-        return {};
-      if (!LoadMips(asset_path, data))
-        return {};
+    return LoadInfo{GetAssetSize(*data), last_loaded_time};
+  }
+  else if (ext == ".png")
+  {
+    // If we have no levels, create one to pass into LoadPNGTexture
+    if (data->m_levels.empty())
+      data->m_levels.push_back({});
 
-      return LoadInfo{GetAssetSize(*data), last_loaded_time};
-    }
-    else if (ext == ".png")
-    {
-      // If we have no levels, create one to pass into LoadPNGTexture
-      if (data->m_levels.empty())
-        data->m_levels.push_back({});
+    if (!LoadPNGTexture(&data->m_levels[0], asset_path.string()))
+      return {};
+    if (!LoadMips(asset_path, data))
+      return {};
 
-      if (!LoadPNGTexture(&data->m_levels[0], asset_path.string()))
-        return {};
-      if (!LoadMips(asset_path, data))
-        return {};
-
-      return LoadInfo{GetAssetSize(*data), last_loaded_time};
-    }
+    return LoadInfo{GetAssetSize(*data), last_loaded_time};
   }
 
   return {};
 }
 
-void DirectFilesystemAssetLibrary::SetAssetIDMapData(
-    const AssetID& asset_id, std::map<std::string, std::filesystem::path> asset_path_map)
+void DirectFilesystemAssetLibrary::SetAssetIDMapData(const AssetID& asset_id,
+                                                     AssetMap asset_path_map)
 {
+  std::lock_guard lk(m_lock);
   m_assetid_to_asset_map_path[asset_id] = std::move(asset_path_map);
 }
 
@@ -145,4 +143,16 @@ bool DirectFilesystemAssetLibrary::LoadMips(const std::filesystem::path& asset_p
 
   return true;
 }
+
+DirectFilesystemAssetLibrary::AssetMap
+DirectFilesystemAssetLibrary::GetAssetMapForID(const AssetID& asset_id) const
+{
+  std::lock_guard lk(m_lock);
+  if (auto iter = m_assetid_to_asset_map_path.find(asset_id);
+      iter != m_assetid_to_asset_map_path.end())
+  {
+    return iter->second;
+  }
+  return {};
+}
 }  // namespace VideoCommon

Source/Core/VideoCommon/Assets/DirectFilesystemAssetLibrary.h

@@ -5,6 +5,7 @@
 #include <chrono>
 #include <filesystem>
 #include <map>
+#include <mutex>
 #include <string>
 
 #include "VideoCommon/Assets/CustomAssetLibrary.h"
@@ -18,6 +19,8 @@ class CustomTextureData;
 class DirectFilesystemAssetLibrary final : public CustomAssetLibrary
 {
 public:
+  using AssetMap = std::map<std::string, std::filesystem::path>;
+
   LoadInfo LoadTexture(const AssetID& asset_id, CustomTextureData* data) override;
 
   // Gets the latest time from amongst all the files in the asset map
@@ -26,12 +29,16 @@ public:
   // Assigns the asset id to a map of files, how this map is read is dependent on the data
   // For instance, a raw texture would expect the map to have a single entry and load that
   // file as the asset.  But a model file data might have its data spread across multiple files
-  void SetAssetIDMapData(const AssetID& asset_id,
-                         std::map<std::string, std::filesystem::path> asset_path_map);
+  void SetAssetIDMapData(const AssetID& asset_id, AssetMap asset_path_map);
 
 private:
   // Loads additional mip levels into the texture structure until _mip<N> texture is not found
   bool LoadMips(const std::filesystem::path& asset_path, CustomTextureData* data);
+
+  // Gets the asset map given an asset id
+  AssetMap GetAssetMapForID(const AssetID& asset_id) const;
+
+  mutable std::mutex m_lock;
   std::map<AssetID, std::map<std::string, std::filesystem::path>> m_assetid_to_asset_map_path;
 };
 }  // namespace VideoCommon

Source/Core/VideoCommon/Assets/TextureAsset.cpp

@@ -9,31 +9,35 @@ namespace VideoCommon
 {
 CustomAssetLibrary::LoadInfo RawTextureAsset::LoadImpl(const CustomAssetLibrary::AssetID& asset_id)
 {
-  std::lock_guard lk(m_lock);
   auto potential_data = std::make_shared<CustomTextureData>();
   const auto loaded_info = m_owning_library->LoadTexture(asset_id, potential_data.get());
   if (loaded_info.m_bytes_loaded == 0)
     return {};
-  m_loaded = true;
-  m_data = std::move(potential_data);
+  {
+    std::lock_guard lk(m_data_lock);
+    m_loaded = true;
+    m_data = std::move(potential_data);
+  }
   return loaded_info;
 }
 
 CustomAssetLibrary::LoadInfo GameTextureAsset::LoadImpl(const CustomAssetLibrary::AssetID& asset_id)
 {
-  std::lock_guard lk(m_lock);
   auto potential_data = std::make_shared<CustomTextureData>();
   const auto loaded_info = m_owning_library->LoadGameTexture(asset_id, potential_data.get());
   if (loaded_info.m_bytes_loaded == 0)
     return {};
-  m_loaded = true;
-  m_data = std::move(potential_data);
+  {
+    std::lock_guard lk(m_data_lock);
+    m_loaded = true;
+    m_data = std::move(potential_data);
+  }
   return loaded_info;
 }
 
 bool GameTextureAsset::Validate(u32 native_width, u32 native_height) const
 {
-  std::lock_guard lk(m_lock);
+  std::lock_guard lk(m_data_lock);
 
   if (!m_loaded)
   {