Apply suggestions from code review

Co-authored-by: Mai <mathew1800@gmail.com>
Co-authored-by: BhaaL <bhaalsen@gmail.com>
Co-authored-by: iwubcode <iwubcode@users.noreply.github.com>

Source/Core/Common/EventHook.h

@@ -8,10 +8,13 @@
 
 #include <functional>
 #include <memory>
+#include <mutex>
 #include <string>
 #include <string_view>
 #include <vector>
 
+namespace Common
+{
 // A hookable event system.
 
 // Define Events in a header as:
@@ -43,10 +46,13 @@ public:
   using CallbackType = std::function<void(CallbackArgs...)>;
 
 private:
-  struct HookImpl : public HookBase
+  struct HookImpl final : public HookBase
   {
     ~HookImpl() override { Event::Remove(this); }
-    HookImpl(CallbackType callback, std::string name) : m_fn(std::move(callback)), m_name(std::move(name)) {}
+    HookImpl(CallbackType callback, std::string name)
+        : m_fn(std::move(callback)), m_name(std::move(name))
+    {
+    }
     CallbackType m_fn;
     std::string m_name;
   };
@@ -55,6 +61,7 @@ public:
   // Returns a handle that will unregister the listener when destroyed.
   static EventHook Register(CallbackType callback, std::string name)
   {
+    std::lock_guard lock(m_mutex);
     DEBUG_LOG_FMT(COMMON, "Registering {} handler at {} event hook", name, EventName.value);
     auto handle = std::make_unique<HookImpl>(callback, std::move(name));
     m_listeners.push_back(handle.get());
@@ -63,6 +70,7 @@ public:
 
   static void Trigger(const CallbackArgs&... args)
   {
+    std::lock_guard lock(m_mutex);
     for (auto& handle : m_listeners)
       handle->m_fn(args...);
   }
@@ -70,10 +78,12 @@ public:
 private:
   static void Remove(HookImpl* handle)
   {
-    auto it = std::find(m_listeners.begin(), m_listeners.end(), handle);
+    std::lock_guard lock(m_mutex);
-    if (it != m_listeners.end())
+    std::erase(m_listeners, handle);
-      m_listeners.erase(it);
   }
 
   inline static std::vector<HookImpl*> m_listeners = {};
+  inline static std::mutex m_mutex;
 };
+
+}  // namespace Common

Source/Core/Common/StringLiteral.h

@@ -5,6 +5,8 @@
 
 #include <algorithm>
 
+namespace Common
+{
 // A useful template for passing string literals as arguments to templates
 // from: https://ctrpeach.io/posts/cpp20-string-literal-template-parameters/
 template <size_t N>
@@ -14,3 +16,5 @@ struct StringLiteral
 
   char value[N];
 };
+
+}  // namespace Common

Source/Core/Core/Core.cpp

@@ -132,7 +132,7 @@ static thread_local bool tls_is_gpu_thread = false;
 
 static void EmuThread(std::unique_ptr<BootParameters> boot, WindowSystemInfo wsi);
 
-static EventHook s_frame_presented = AfterPresentEvent::Register(
+static Common::EventHook s_frame_presented = AfterPresentEvent::Register(
     [](auto& present_info) {
       const double last_speed_denominator = g_perf_metrics.GetLastSpeedDenominator();
       // The denominator should always be > 0 but if it's not, just return 1

Source/Core/Core/FifoPlayer/FifoRecorder.h

@@ -76,5 +76,5 @@ private:
   std::vector<u8> m_Ram;
   std::vector<u8> m_ExRam;
 
-  EventHook m_end_of_frame_event;
+  Common::EventHook m_end_of_frame_event;
 };

Source/Core/VideoBackends/D3D/D3DGfx.h

@@ -16,7 +16,7 @@ class SwapChain;
 class DXTexture;
 class DXFramebuffer;
 
-class Gfx : public ::AbstractGfx
+class Gfx final : public ::AbstractGfx
 {
 public:
   Gfx(std::unique_ptr<SwapChain> swap_chain, float backbuffer_scale);

Source/Core/VideoBackends/Metal/MTLGfx.h

@@ -69,7 +69,6 @@ public:
   void BindBackbuffer(const ClearColor& clear_color = {}) override;
   void PresentBackbuffer() override;
 
-  // Returns info about the main surface (aka backbuffer)
   SurfaceInfo GetSurfaceInfo() const override;
 
 private:

Source/Core/VideoBackends/Vulkan/VKGfx.h

@@ -19,7 +19,7 @@ class VKFramebuffer;
 class VKPipeline;
 class VKTexture;
 
-class VKGfx : public ::AbstractGfx
+class VKGfx final : public ::AbstractGfx
 {
 public:
   VKGfx(std::unique_ptr<SwapChain> swap_chain, float backbuffer_scale);
@@ -77,7 +77,6 @@ public:
   void SetFullscreen(bool enable_fullscreen) override;
   bool IsFullscreen() const override;
 
-  // Returns info about the main surface (aka backbuffer)
   virtual SurfaceInfo GetSurfaceInfo() const override;
 
   // Completes the current render pass, executes the command buffer, and restores state ready for

Source/Core/VideoCommon/FrameDumper.h

@@ -98,7 +98,7 @@ private:
   std::mutex m_screenshot_lock;
   std::string m_screenshot_name;
 
-  EventHook m_frame_end_handle;
+  Common::EventHook m_frame_end_handle;
 };
 
 extern std::unique_ptr<FrameDumper> g_frame_dumper;

Source/Core/VideoCommon/FramebufferManager.h

@@ -238,7 +238,7 @@ protected:
   std::vector<EFBPokeVertex> m_color_poke_vertices;
   std::vector<EFBPokeVertex> m_depth_poke_vertices;
 
-  EventHook m_end_of_frame_event;
+  Common::EventHook m_end_of_frame_event;
 };
 
 extern std::unique_ptr<FramebufferManager> g_framebuffer_manager;

Source/Core/VideoCommon/GraphicsModSystem/Runtime/GraphicsModManager.h

@@ -54,7 +54,7 @@ private:
 
   std::unordered_set<std::string> m_groups;
 
-  EventHook m_end_of_frame_event;
+  Common::EventHook m_end_of_frame_event;
 };
 
 extern std::unique_ptr<GraphicsModManager> g_graphics_mod_manager;

Source/Core/VideoCommon/ShaderCache.h

@@ -252,7 +252,7 @@ private:
   // Texture decoding shaders
   std::map<std::pair<u32, u32>, std::unique_ptr<AbstractShader>> m_texture_decoding_shaders;
 
-  EventHook m_frame_end_handler;
+  Common::EventHook m_frame_end_handler;
 };
 
 }  // namespace VideoCommon

Source/Core/VideoCommon/Statistics.cpp

@@ -18,10 +18,10 @@
 
 Statistics g_stats;
 
-static EventHook s_before_frame_event =
+static Common::EventHook s_before_frame_event =
     BeforeFrameEvent::Register([] { g_stats.ResetFrame(); }, "Statistics::ResetFrame");
 
-static EventHook s_after_frame_event = AfterFrameEvent::Register(
+static Common::EventHook s_after_frame_event = AfterFrameEvent::Register(
     [] {
       DolphinAnalytics::PerformanceSample perf_sample;
       perf_sample.speed_ratio = SystemTimers::GetEstimatedEmulationPerformance();

Source/Core/VideoCommon/TextureCacheBase.cpp

@@ -780,10 +780,10 @@ void TextureCacheBase::OnFrameEnd()
     // Flush any outstanding EFB copies to RAM, in case the game is running at an uncapped frame
     // rate and not waiting for vblank. Otherwise, we'd end up with a huge list of pending
     // copies.
-    g_texture_cache->FlushEFBCopies();
+    FlushEFBCopies();
   }
 
-  g_texture_cache->Cleanup(g_presenter->FrameCount());
+  Cleanup(g_presenter->FrameCount());
 }
 
 void TCacheEntry::DoState(PointerWrap& p)

Source/Core/VideoCommon/TextureCacheBase.h

@@ -123,8 +123,8 @@ struct TCacheEntry
   bool is_efb_copy = false;
   bool is_custom_tex = false;
   bool may_have_overlapping_textures = true;
-  bool has_arbitrary_mips = false;         // indicates that the mips in this texture are arbitrary
+  // indicates that the mips in this texture are arbitrary content, aren't just downscaled
-                                           // content, aren't just downscaled
+  bool has_arbitrary_mips = false;
   bool should_force_safe_hashing = false;  // for XFB
   bool is_xfb_copy = false;
   bool is_xfb_container = false;
@@ -438,7 +438,8 @@ private:
   void OnFrameEnd();
 
   Common::Flag m_force_reload_textures;
-  EventHook m_frame_event = AfterFrameEvent::Register([this] { OnFrameEnd(); }, "TextureCache");
+  Common::EventHook m_frame_event =
+      AfterFrameEvent::Register([this] { OnFrameEnd(); }, "TextureCache");
 };
 
 extern std::unique_ptr<TextureCacheBase> g_texture_cache;

Source/Core/VideoCommon/VertexManagerBase.h

@@ -230,7 +230,7 @@ private:
   std::vector<u32> m_scheduled_command_buffer_kicks;
   bool m_allow_background_execution = true;
 
-  EventHook m_frame_end_event;
+  Common::EventHook m_frame_end_event;
 };
 
 extern std::unique_ptr<VertexManagerBase> g_vertex_manager;

Source/Core/VideoCommon/VideoConfig.cpp

@@ -355,5 +355,5 @@ void CheckForConfigChanges()
   // TODO: Move everything else to the ConfigChanged event
 }
 
-static EventHook s_check_config_event =
+static Common::EventHook s_check_config_event =
     AfterFrameEvent::Register([] { CheckForConfigChanges(); }, "CheckForConfigChanges");

Source/Core/VideoCommon/VideoEvents.h

@@ -7,16 +7,16 @@
 #include "Common/EventHook.h"
 
 // Called when certain video config setting are changed
-using ConfigChangedEvent = Event<"ConfigChanged", u32>;
+using ConfigChangedEvent = Common::Event<"ConfigChanged", u32>;
 
 // An event called just before the first draw call of a frame
-using BeforeFrameEvent = Event<"BeforeFrame">;
+using BeforeFrameEvent = Common::Event<"BeforeFrame">;
 
 // An event called after the frame XFB copy begins processing on the host GPU.
 // Useful for "once per frame" usecases.
 // Note: In a few rare cases, games do multiple XFB copies per frame and join them while presenting.
 //       If this matters to your usecase, you should use BeforePresent instead.
-using AfterFrameEvent = Event<"AfterFrame">;
+using AfterFrameEvent = Common::Event<"AfterFrame">;
 
 struct PresentInfo
 {
@@ -76,8 +76,8 @@ struct PresentInfo
 // frame.
 //
 // frame_count: The number of frames
-using BeforePresentEvent = Event<"BeforePresent", PresentInfo&>;
+using BeforePresentEvent = Common::Event<"BeforePresent", PresentInfo&>;
 
 // An event that is triggered after a frame is presented.
 // The exact timing of this event depends on backend/driver support.
-using AfterPresentEvent = Event<"AfterPresent", PresentInfo&>;
+using AfterPresentEvent = Common::Event<"AfterPresent", PresentInfo&>;

Source/Core/VideoCommon/Widescreen.h

@@ -27,8 +27,8 @@ private:
   bool m_is_game_widescreen = false;
   bool m_was_orthographically_anamorphic = false;
 
-  EventHook m_update_widescreen;
+  Common::EventHook m_update_widescreen;
-  EventHook m_config_changed;
+  Common::EventHook m_config_changed;
 };
 
 extern std::unique_ptr<WidescreenManager> g_widescreen;