diff --git a/src/spice2x/CMakeLists.txt b/src/spice2x/CMakeLists.txt index 26e2ea9..9659fe1 100644 --- a/src/spice2x/CMakeLists.txt +++ b/src/spice2x/CMakeLists.txt @@ -626,6 +626,7 @@ set(SOURCE_FILES ${SOURCE_FILES} # rawinput rawinput/rawinput.cpp + rawinput/rawinput_handles.cpp rawinput/midi.cpp rawinput/sextet.cpp rawinput/piuio.cpp diff --git a/src/spice2x/overlay/windows/config.cpp b/src/spice2x/overlay/windows/config.cpp index f1607d2..0f62087 100644 --- a/src/spice2x/overlay/windows/config.cpp +++ b/src/spice2x/overlay/windows/config.cpp @@ -1356,8 +1356,9 @@ namespace overlay::windows { if (check_devices) { // iterate updated devices auto updated_devices = RI_MGR->devices_get_updated(); + bool binding_finished = false; for (auto device : updated_devices) { - std::lock_guard lock(*device->mutex); + std::unique_lock lock(*device->mutex); switch (device->type) { case rawinput::MOUSE: { auto mouse = device->mouseInfo; @@ -1374,7 +1375,7 @@ namespace overlay::windows { ImGui::CloseCurrentPopup(); buttons_bind_active = false; inc_buttons_many_index(button_it_max); - RI_MGR->devices_midi_freeze(false); + binding_finished = true; break; } } @@ -1393,7 +1394,7 @@ namespace overlay::windows { buttons_bind_active = false; buttons_many_index = -1; buttons_many_active = false; - RI_MGR->devices_midi_freeze(false); + binding_finished = true; break; } @@ -1406,7 +1407,7 @@ namespace overlay::windows { ImGui::CloseCurrentPopup(); buttons_bind_active = false; inc_buttons_many_index(button_it_max); - RI_MGR->devices_midi_freeze(false); + binding_finished = true; break; } } @@ -1542,7 +1543,7 @@ namespace overlay::windows { ImGui::CloseCurrentPopup(); buttons_bind_active = false; inc_buttons_many_index(button_it_max); - RI_MGR->devices_midi_freeze(false); + binding_finished = true; } break; @@ -1590,7 +1591,7 @@ namespace overlay::windows { ImGui::CloseCurrentPopup(); buttons_bind_active = false; inc_buttons_many_index(button_it_max); - RI_MGR->devices_midi_freeze(false); + binding_finished = true; break; } } @@ -1614,7 +1615,7 @@ namespace overlay::windows { ImGui::CloseCurrentPopup(); buttons_bind_active = false; inc_buttons_many_index(button_it_max); - RI_MGR->devices_midi_freeze(false); + binding_finished = true; break; } } else { @@ -1641,7 +1642,7 @@ namespace overlay::windows { ImGui::CloseCurrentPopup(); buttons_bind_active = false; inc_buttons_many_index(button_it_max); - RI_MGR->devices_midi_freeze(false); + binding_finished = true; break; } } else { @@ -1668,7 +1669,7 @@ namespace overlay::windows { ImGui::CloseCurrentPopup(); buttons_bind_active = false; inc_buttons_many_index(button_it_max); - RI_MGR->devices_midi_freeze(false); + binding_finished = true; break; } } else { @@ -1694,7 +1695,7 @@ namespace overlay::windows { ImGui::CloseCurrentPopup(); buttons_bind_active = false; inc_buttons_many_index(button_it_max); - RI_MGR->devices_midi_freeze(false); + binding_finished = true; break; } @@ -1715,7 +1716,7 @@ namespace overlay::windows { ImGui::CloseCurrentPopup(); buttons_bind_active = false; inc_buttons_many_index(button_it_max); - RI_MGR->devices_midi_freeze(false); + binding_finished = true; break; } } @@ -1745,7 +1746,7 @@ namespace overlay::windows { ImGui::CloseCurrentPopup(); buttons_bind_active = false; inc_buttons_many_index(button_it_max); - RI_MGR->devices_midi_freeze(false); + binding_finished = true; break; } } @@ -1755,6 +1756,13 @@ namespace overlay::windows { default: break; } + lock.unlock(); + if (binding_finished) { + break; + } + } + if (binding_finished) { + RI_MGR->devices_midi_freeze(false); } } diff --git a/src/spice2x/rawinput/device.h b/src/spice2x/rawinput/device.h index 5a080a3..a0eefa8 100644 --- a/src/spice2x/rawinput/device.h +++ b/src/spice2x/rawinput/device.h @@ -197,8 +197,8 @@ namespace rawinput { HANDLE handle = INVALID_HANDLE_VALUE; DeviceType type = UNKNOWN; DeviceInfo info; - std::mutex *mutex; - std::mutex *mutex_out; + std::mutex *mutex = nullptr; + std::mutex *mutex_out = nullptr; bool updated = true; bool output_pending = true; bool output_enabled = false; @@ -213,5 +213,7 @@ namespace rawinput { double input_time = 0.0; double input_hz = 0.f; double input_hz_max = 0.f; + + // when adding or removing a field, update replace_device_slot to match }; } diff --git a/src/spice2x/rawinput/midi.cpp b/src/spice2x/rawinput/midi.cpp index 12bbd13..2d71665 100644 --- a/src/spice2x/rawinput/midi.cpp +++ b/src/spice2x/rawinput/midi.cpp @@ -238,8 +238,6 @@ void rawinput::RawInputManager::devices_scan_midi() { midi_device.name = midi_identifier; midi_device.desc = to_string(midi_device_caps.szPname); midi_device.info = midi_device_info; - midi_device.mutex = new std::mutex(); - midi_device.mutex_out = new std::mutex(); midi_device.midiInfo = midi_device_midi_info; // mutate the shared device list under lock (the slow WinMM calls above @@ -256,10 +254,9 @@ void rawinput::RawInputManager::devices_scan_midi() { // carry over ID midi_device.id = device.id; - // destruct and replace, reusing the slot's existing mutexes + // destruct and replace the slot in place under its locks this->devices_destruct(&device); - reuse_device_mutexes(midi_device, device); - device = midi_device; + replace_device_slot(device, midi_device); // notify change for (auto &cb : this->callback_change) { @@ -275,6 +272,8 @@ void rawinput::RawInputManager::devices_scan_midi() { } // add device to list + midi_device.mutex = new std::mutex(); + midi_device.mutex_out = new std::mutex(); auto &device = this->devices.emplace_back(midi_device); // notify add diff --git a/src/spice2x/rawinput/rawinput.cpp b/src/spice2x/rawinput/rawinput.cpp index 1eef49f..632968a 100644 --- a/src/spice2x/rawinput/rawinput.cpp +++ b/src/spice2x/rawinput/rawinput.cpp @@ -44,18 +44,40 @@ namespace rawinput { bool OS_WINDOW_ACTIVE = false; } -// when replacing a device slot in place, keep the old slot's per-device mutexes -// instead of the freshly allocated pair on `replacement`. their addresses stay -// stable, so a thread still holding a snapshot pointer to the slot (e.g. the -// output thread blocked on mutex_out, which is taken without devices_mutex) -// never locks freed memory. the freshly allocated pair is freed here rather -// than leaking the old one. the slot must already be destructed so both -// mutexes are unlocked -void rawinput::RawInputManager::reuse_device_mutexes(Device &replacement, const Device &existing) { - delete replacement.mutex; - delete replacement.mutex_out; - replacement.mutex = existing.mutex; - replacement.mutex_out = existing.mutex_out; +// serialize replacement with input and output users, matching teardown lock order +void rawinput::RawInputManager::replace_device_slot(Device &existing, Device &replacement) { + if (existing.mutex == nullptr || existing.mutex_out == nullptr) { + log_fatal("rawinput", "existing device slot has no mutexes"); + } + if (replacement.mutex != nullptr || replacement.mutex_out != nullptr) { + log_fatal("rawinput", "replacement device already has mutexes"); + } + std::lock_guard lock_out(*existing.mutex_out); + std::lock_guard lock(*existing.mutex); + + // copy every field except the stable mutex pointers: snapshot users may be blocked + // on those exact locks without devices_mutex, so rewriting the pointers could send a + // waiter to a different mutex. when adding a field, add it here too (see device.h) + existing.id = replacement.id; + existing.name = std::move(replacement.name); + existing.desc = std::move(replacement.desc); + existing.handle = replacement.handle; + existing.type = replacement.type; + existing.info = replacement.info; + existing.updated = replacement.updated; + existing.output_pending = replacement.output_pending; + existing.output_enabled = replacement.output_enabled; + existing.mouseInfo = replacement.mouseInfo; + existing.keyboardInfo = replacement.keyboardInfo; + existing.hidInfo = replacement.hidInfo; + existing.midiInfo = replacement.midiInfo; + existing.sextetInfo = replacement.sextetInfo; + existing.piuioDev = replacement.piuioDev; + existing.smxstageInfo = replacement.smxstageInfo; + existing.smxdedicabInfo = replacement.smxdedicabInfo; + existing.input_time = replacement.input_time; + existing.input_hz = replacement.input_hz; + existing.input_hz_max = replacement.input_hz_max; } rawinput::RawInputManager::RawInputManager() { @@ -277,8 +299,6 @@ void rawinput::RawInputManager::devices_scan_rawinput(RAWINPUTDEVICELIST *device new_device.name = device_name; new_device.desc = device_description; new_device.info = device_info; - new_device.mutex = new std::mutex(); - new_device.mutex_out = new std::mutex(); new_device.input_time = get_performance_seconds(); switch (device->dwType) { case RIM_TYPEMOUSE: @@ -805,10 +825,10 @@ void rawinput::RawInputManager::devices_scan_rawinput(RAWINPUTDEVICELIST *device // carry over old device ID new_device.id = prev_device.id; - // destruct and replace, reusing the slot's existing mutexes + // destruct and replace the slot in place under its locks this->devices_destruct(&prev_device); - reuse_device_mutexes(new_device, prev_device); - prev_device = new_device; + replace_device_slot(prev_device, new_device); + this->rawinput_handles.add(&prev_device); // notify change for (auto &cb : this->callback_change) { @@ -820,7 +840,10 @@ void rawinput::RawInputManager::devices_scan_rawinput(RAWINPUTDEVICELIST *device } // add device to list + new_device.mutex = new std::mutex(); + new_device.mutex_out = new std::mutex(); auto &added_device = this->devices.emplace_back(new_device); + this->rawinput_handles.add(&added_device); if (log) { log_info("rawinput", "added device: {} / {}", added_device.desc, added_device.name); } @@ -951,8 +974,6 @@ void rawinput::RawInputManager::devices_scan_xinput() { device.name = xinput::get_device_desc(player); device.desc = fmt::format("XInput Gamepad P{}", player + 1); device.handle = reinterpret_cast(player); - device.mutex = new std::mutex(); - device.mutex_out = new std::mutex(); return device; }; @@ -969,11 +990,10 @@ void rawinput::RawInputManager::devices_scan_xinput() { log_info("rawinput", "overwriting previously destroyed XInput device: {}", prev_device.name); const auto old_id = prev_device.id; - // replace in place, reusing the slot's existing mutexes + // replace the slot in place under its locks auto replacement = create_device(player); - reuse_device_mutexes(replacement, prev_device); - prev_device = replacement; - prev_device.id = old_id; + replacement.id = old_id; + replace_device_slot(prev_device, replacement); // notify change for (auto &cb : this->callback_change) { @@ -989,6 +1009,8 @@ void rawinput::RawInputManager::devices_scan_xinput() { log_info("rawinput", "adding new XInput device: player {}", player + 1); auto new_xinput_device = create_device(player); new_xinput_device.id = this->devices.size() + 1; + new_xinput_device.mutex = new std::mutex(); + new_xinput_device.mutex_out = new std::mutex(); auto &device = this->devices.emplace_back(new_xinput_device); // notify add @@ -1438,6 +1460,8 @@ void rawinput::RawInputManager::devices_destruct() { void rawinput::RawInputManager::devices_destruct(Device *device, bool log) { + this->rawinput_handles.remove(device); + // check if destroyed if (device->type == DESTROYED) { return; @@ -1454,21 +1478,20 @@ void rawinput::RawInputManager::devices_destruct(Device *device, bool log) { // devices_mutex, so mutex_out is what actually serializes it against us std::lock_guard lock_out(*device->mutex_out); - // mark as destroyed - auto device_type = device->type; - device->type = DESTROYED; + // mark the device destroyed while synchronized with in-flight input + DeviceType device_type; + { + std::lock_guard lock(*device->mutex); + device_type = device->type; + device->type = DESTROYED; + } - // notify change + // callbacks may lock the device mutex for (auto &cb : this->callback_change) { cb.f(cb.data, device); } - /* - * lock device - * note: this is an exception to only locking devices when we acquire them from the list - * callbacks could want to lock the mutex as well and it isn't recursive - * this also means the device must be unlocked before calling this function - */ + // wait for input users before releasing device resources std::lock_guard lock(*device->mutex); // close device handles @@ -1561,21 +1584,13 @@ LRESULT CALLBACK rawinput::RawInputManager::input_wnd_proc( // find device HANDLE device_handle = data->header.hDevice; - // lock the device list so a concurrent scan can't mutate it while we iterate - std::lock_guard devices_lock(ref->devices_mutex); - for (auto &device : ref->devices_get()) { - - // skip if this is the wrong device - if (device.handle != device_handle) { - continue; - } + auto acquired_device = ref->rawinput_handles.acquire(device_handle); + if (acquired_device.device != nullptr) { + auto &device = *acquired_device.device; // get input time const auto input_time = get_performance_seconds(); - // lock device - device.mutex->lock(); - // update hz double diff_time = input_time - device.input_time; if (diff_time > 0.0001) { @@ -1834,7 +1849,7 @@ LRESULT CALLBACK rawinput::RawInputManager::input_wnd_proc( USAGE usage = usages[usage_num] - button_caps.Range.UsageMin; - // guard against some buggy device sending an event for a usage below `UsageMin` + // guard against some buggy device sending an event for a usage below UsageMin if (usage < button_count) { new_states[usage] = true; } @@ -1937,11 +1952,7 @@ LRESULT CALLBACK rawinput::RawInputManager::input_wnd_proc( break; } - // free device - device.mutex->unlock(); - - // don't iterate through the other devices - break; + acquired_device.lock.unlock(); } // update controller state ring buffers (DDR/MDXF) @@ -1973,6 +1984,9 @@ LRESULT CALLBACK rawinput::RawInputManager::input_wnd_proc( void rawinput::RawInputManager::device_write_output(Device *device, bool only_updated) { + // lock device + std::lock_guard lock(*device->mutex_out); + // check if output is enabled if (!device->output_enabled) { return; @@ -1983,9 +1997,6 @@ void rawinput::RawInputManager::device_write_output(Device *device, bool only_up return; } - // lock device - device->mutex_out->lock(); - // mark device as updated device->output_pending = false; @@ -2208,8 +2219,6 @@ void rawinput::RawInputManager::device_write_output(Device *device, bool only_up break; } - // unlock device - device->mutex_out->unlock(); } void rawinput::RawInputManager::devices_flush_output(bool optimized) { diff --git a/src/spice2x/rawinput/rawinput.h b/src/spice2x/rawinput/rawinput.h index 0684863..81a95a4 100644 --- a/src/spice2x/rawinput/rawinput.h +++ b/src/spice2x/rawinput/rawinput.h @@ -12,6 +12,7 @@ #include "device.h" #include "hotplug.h" +#include "rawinput_handles.h" #include "rawinput/xinput.h" #include "util/scope_guard.h" @@ -74,6 +75,9 @@ namespace rawinput { std::list devices; std::recursive_mutex devices_mutex; + // separate lookup isolated from devices_mutex for the latency-sensitive WM_INPUT path + RawInputHandles rawinput_handles; + WNDCLASSEX input_hwnd_class {}; std::thread *input_thread = nullptr; std::thread *midi_thread = nullptr; @@ -134,10 +138,9 @@ namespace rawinput { static void CALLBACK input_midi_proc(HMIDIIN, UINT, DWORD_PTR, DWORD_PTR, DWORD_PTR); static DeviceInfo get_device_info(const std::string &device_name); - // shared by the rawinput / xinput / midi device scans when replacing a slot - // in place; keeps the existing slot's mutex pair so snapshot pointers held by - // other threads stay valid (see the definition in rawinput.cpp) - static void reuse_device_mutexes(Device &replacement, const Device &existing); + // replace an already-destructed slot in place, holding both device locks so + // concurrent pollers and the output thread never see a half-copied Device + static void replace_device_slot(Device &existing, Device &replacement); public: @@ -193,6 +196,7 @@ namespace rawinput { std::lock_guard lock(devices_mutex); for (auto &device : devices_get()) { if (device.type == MIDI) { + std::lock_guard device_lock(*device.mutex); device.midiInfo->freeze = freeze; if (!freeze) { for (unsigned short index = 0; index < device.midiInfo->states.size(); index++) { diff --git a/src/spice2x/rawinput/rawinput_handles.cpp b/src/spice2x/rawinput/rawinput_handles.cpp new file mode 100644 index 0000000..1c692b5 --- /dev/null +++ b/src/spice2x/rawinput/rawinput_handles.cpp @@ -0,0 +1,31 @@ +#include "rawinput_handles.h" + +void rawinput::RawInputHandles::add(Device *device) { + std::lock_guard lock(this->mutex); + this->devices[device->handle] = device; +} + +void rawinput::RawInputHandles::remove(Device *device) { + std::lock_guard lock(this->mutex); + + // teardown is shared by every device type, and handles can be reused; only erase + // an entry that still belongs to this exact RawInput device + auto it = this->devices.find(device->handle); + if (it != this->devices.end() && it->second == device) { + this->devices.erase(it); + } +} + +rawinput::RawInputHandles::AcquiredDevice rawinput::RawInputHandles::acquire(HANDLE handle) { + std::lock_guard index_lock(this->mutex); + auto it = this->devices.find(handle); + if (it == this->devices.end()) { + return {}; + } + + auto *device = it->second; + + // take the device mutex while index_lock is still held so teardown can't free it + // between the lookup and the lock + return {device, std::unique_lock(*device->mutex)}; +} diff --git a/src/spice2x/rawinput/rawinput_handles.h b/src/spice2x/rawinput/rawinput_handles.h new file mode 100644 index 0000000..3b60564 --- /dev/null +++ b/src/spice2x/rawinput/rawinput_handles.h @@ -0,0 +1,28 @@ +#pragma once + +#include +#include + +#include + +#include "device.h" + +namespace rawinput { + + class RawInputHandles { + public: + struct AcquiredDevice { + Device *device = nullptr; + std::unique_lock lock; + }; + + void add(Device *device); + void remove(Device *device); + AcquiredDevice acquire(HANDLE handle); + + private: + // non-owning index; RawInputManager owns the stable device slots + std::unordered_map devices; + std::mutex mutex; + }; +}