graphics: move captures off-thread when streaming (#885)

## Link to GitHub Issue or related Pull Request, if one exists
#0

## Description of change

Moves the API capture readback off the game's Present thread while a
video stream client is connected.

The readback is a `LockRect` plus a memcpy of the whole back buffer,
roughly 635us at 720p and 1270us at 1080p. On the Present thread that
comes out of the game's frame budget: TDJ (at 120Hz) dropped to 117fps
with a 60fps stream running, and reading on a pool thread instead gave
the full 120 back.

Only streaming takes the off-thread path, gated on a new
`capture_pump::screen_claimed()`.

Screenshots, one-off API captures, and `THREAD_BAN` games all keep the
existing inline read for compat reasons. A pool thread in `LockRect`
while the Present thread sat inside `GetRenderTargetData` deadlocks DDR
X2 for example.

`CLAIMED[]` becomes `std::atomic<bool>` so the capture path does not
take a lock on the Present thread. The read pool has a single worker so
frames cannot be enqueued out of order, and both capture pools are never
destroyed so a late read cannot queue onto a torn-down pool.

The capture pipeline itself is unchanged: `GetRenderTargetData` is still
synchronous on the Present thread.

## Testing
DDR X2
World
IIDX TDJ
SDVX VM

CONTRIBUTING.md

@@ -22,6 +22,7 @@ To contribute, fork the repo (just the main branch), make changes in your fork,
 * Any additions to built-in patches (modifications / removals are OK if we have a good reason).
 * Changes to the software license terms.
 * Localization / translation for UI text. It becomes too difficult to manage by the maintainers.
+* Bundling third party DLL binary; e.g., Discord Social SDK, PC light control integration, and so on. These should be built as hook DLLs instead, optionally with [Spice SDK](https://github.com/spice2x/spice2x.github.io/wiki/Spice-SDK).
 
 ### Avoiding regressions
 

src/spice2x/api/capture_pump.cpp

@@ -1,6 +1,7 @@
 #include "capture_pump.h"
 
 #include <array>
+#include <atomic>
 #include <mutex>
 
 #include "hooks/graphics/graphics.h"
@@ -11,8 +12,8 @@ namespace api::capture_pump {
 
         std::array<std::mutex, GRAPHICS_CAPTURE_SCREEN_NO> CONSUMER_M;
 
-        std::mutex CLAIMED_M;
-        std::array<bool, GRAPHICS_CAPTURE_SCREEN_NO> CLAIMED {};
+        // read once per capture from the present thread, so it stays lock free
+        std::atomic<bool> CLAIMED[GRAPHICS_CAPTURE_SCREEN_NO] {};
 
         bool valid_screen(int screen) {
             return 0 <= screen && screen < static_cast<int>(GRAPHICS_CAPTURE_SCREEN_NO);
@@ -37,14 +38,7 @@ namespace api::capture_pump {
             return false;
         }
 
-        std::lock_guard<std::mutex> lock(CLAIMED_M);
-
-        if (CLAIMED[screen]) {
-            return false;
-        }
-
-        CLAIMED[screen] = true;
-        return true;
+        return !CLAIMED[screen].exchange(true);
     }
 
     void release_screen(int screen) {
@@ -52,7 +46,14 @@ namespace api::capture_pump {
             return;
         }
 
-        std::lock_guard<std::mutex> lock(CLAIMED_M);
         CLAIMED[screen] = false;
     }
+
+    bool screen_claimed(int screen) {
+        if (!valid_screen(screen)) {
+            return false;
+        }
+
+        return CLAIMED[screen];
+    }
 }

src/spice2x/api/capture_pump.h

@@ -21,4 +21,7 @@ namespace api::capture_pump {
     // a screen carries one stream at a time; false when another connection already holds it
     bool claim_screen(int screen);
     void release_screen(int screen);
+
+    // true while a video stream client holds this screen
+    bool screen_claimed(int screen);
 }

src/spice2x/api/stream_server.cpp

@@ -14,6 +14,7 @@
 
 #include "capture_pump.h"
 #include "hooks/graphics/graphics.h"
+#include "overlay/notifications.h"
 #include "stream_format.h"
 #include "util/logging.h"
 #include "util/utils.h"
@@ -178,6 +179,13 @@ namespace api {
         // carry it too, or the client sees an opaque failure instead of the status.
         constexpr const char *cors_header = "Access-Control-Allow-Origin: *\r\n";
 
+        // the port is unauthenticated, so a scanner hammering a busy/missing screen could
+        // otherwise flood the overlay; throttle failure toasts per distinct cause. kept under
+        // a second so it only swallows that, not a legitimate reconnect - substream itself
+        // switches screens with a 300ms gap, and only backs off to a full second once a
+        // retry has actually failed
+        constexpr double notification_throttle_seconds = 0.5;
+
         void send_error(SOCKET socket, const char *status) {
             const std::string response =
                     std::string("HTTP/1.0 ") + status + "\r\n"
@@ -334,6 +342,11 @@ namespace api {
 
             if (slot < 0) {
                 log_warning("api::stream", "client limit of {} hit", client_limit);
+                overlay::notifications::add_throttled(
+                        overlay::notifications::Severity::Warning,
+                        "api::stream.client_limit",
+                        notification_throttle_seconds,
+                        fmt::format("Video stream refused: client limit reached ({})", address));
                 send_error(client, "503 Service Unavailable");
                 closesocket(client);
                 continue;
@@ -410,16 +423,32 @@ namespace api {
                     if (!streamable(screen)) {
                         log_warning("api::stream",
                                 "screen {} is not available, refusing {}", screen, address);
+                        overlay::notifications::add_throttled(
+                                overlay::notifications::Severity::Warning,
+                                fmt::format("api::stream.screen_unavailable.{}", screen),
+                                notification_throttle_seconds,
+                                fmt::format("Video stream refused: screen {} not available ({})",
+                                        screen, address));
                         send_error(socket, "404 Not Found");
                     } else if (!capture_pump::claim_screen(screen)) {
                         log_warning("api::stream",
                                 "screen {} is already being streamed, refusing {}",
                                 screen, address);
+                        overlay::notifications::add_throttled(
+                                overlay::notifications::Severity::Warning,
+                                fmt::format("api::stream.screen_claimed.{}", screen),
+                                notification_throttle_seconds,
+                                fmt::format("Video stream refused: screen {} already streaming ({})",
+                                        screen, address));
                         send_error(socket, "503 Service Unavailable");
                     } else {
                         log_info("api::stream",
                                 "client connected: {} ({}, screen={}, fps={}, quality={})",
                                 address, request.path, screen, fps, quality);
+                        overlay::notifications::add(
+                                overlay::notifications::Severity::Success,
+                                fmt::format("Video stream client connected ({}, screen {})",
+                                        address, screen));
 
                         const std::string header =
                                 "HTTP/1.0 200 OK\r\n"
@@ -460,6 +489,9 @@ namespace api {
 
                         capture_pump::release_screen(screen);
                         log_info("api::stream", "client disconnected: {}", address);
+                        overlay::notifications::add(
+                                overlay::notifications::Severity::Info,
+                                fmt::format("Video stream client disconnected ({})", address));
                     }
                 }
             }

src/spice2x/api/websocket.cpp

@@ -37,6 +37,15 @@ namespace api {
     private:
         ClientState *state = nullptr;
 
+        // headsocket doesn't expose the peer address on its own client API, but the
+        // sockaddr_in captured at accept time is sitting right there in the impl
+        std::string remote_address() const {
+            char address_data[INET_ADDRSTRLEN] {};
+            inet_ntop(AF_INET, &this->_p->conn.impl()->from.sin_addr,
+                    address_data, INET_ADDRSTRLEN);
+            return std::string(address_data);
+        }
+
     protected:
         bool async_received_data(const data_block &db, uint8_t *ptr, size_t length) override;
 
@@ -120,19 +129,21 @@ namespace api {
         srv->websocket->controller->init_state(state);
 
         // log connection
-        log_info("api::websocket", "client connected");
+        const auto address = this->remote_address();
+        log_info("api::websocket", "client connected: {}", address);
         overlay::notifications::add(
                 overlay::notifications::Severity::Success,
-                "API websocket client connected");
+                fmt::format("API websocket client connected ({})", address));
     }
 
     void WebSocketClient::on_disconnect() {
 
         // log disconnection
-        log_info("api::websocket", "client disconnected");
+        const auto address = this->remote_address();
+        log_info("api::websocket", "client disconnected: {}", address);
         overlay::notifications::add(
                 overlay::notifications::Severity::Info,
-                "API websocket client disconnected");
+                fmt::format("API websocket client disconnected ({})", address));
 
         // get pointer to server
         auto srv = reinterpret_cast<WebSocketServer *>(server().get());

src/spice2x/external/docker/Dockerfile

@@ -25,10 +25,11 @@ ENV PATH="$PATH:/opt/llvm-mingw-xp/bin"
 RUN curl -fsSL "https://github.com/mon/windows-dll-compat-checker/releases/download/v1.3/windows_dll_compat_checker-linux-x86_64.tar.xz" \
     | tar -xJ -C /usr/local/bin
 
-# libjpeg-turbo for JPEG encoding. Only the mingw-w64 toolchains get it;
-# the WinXP targets build without JPEG support entirely.
-RUN su user -c "yay --noconfirm -S mingw-w64-libjpeg-turbo"
+# Stock makepkg.conf builds serially; this makes the AUR compiles below parallel.
+RUN printf '%s\n' 'MAKEFLAGS="-j$(nproc)"' > /home/user/.makepkg.conf \
+    && chown user: /home/user/.makepkg.conf
 
-# x264 for the API H.264 video stream. Only the mingw-w64 toolchains need it;
-# the WinXP targets build without the stream encoder.
-RUN su user -c "yay --noconfirm -S mingw-w64-x264"
+# libjpeg-turbo for JPEG encoding, x264 for the API H.264 video stream. Only the
+# mingw-w64 toolchains get these; the WinXP targets build without JPEG support
+# and without the stream encoder.
+RUN su user -c "yay --noconfirm -S mingw-w64-libjpeg-turbo mingw-w64-x264"

src/spice2x/external/headsocket.h

@@ -1450,7 +1450,10 @@ void basic_tcp_server::accept_thread()
   while (_p->isRunning)
   {
     detail::connection_impl conn_impl;
-    conn_impl.socket = ::accept(_p->serverSocket, reinterpret_cast<struct sockaddr *>(&conn_impl.from), nullptr);
+    // addrlen must be a valid in/out pointer or the OS leaves conn_impl.from untouched,
+    // so the peer address silently reads back as 0.0.0.0
+    int from_len = sizeof(conn_impl.from);
+    conn_impl.socket = ::accept(_p->serverSocket, reinterpret_cast<struct sockaddr *>(&conn_impl.from), &from_len);
     conn_impl.id = _p->nextClientID++;
 
     if (!_p->nextClientID)

src/spice2x/hooks/graphics/backends/d3d9/d3d9_screenshot.cpp

@@ -15,6 +15,7 @@
 #include <external/robin_hood.h>
 #include <external/fpng/fpng.h>
 
+#include "api/capture_pump.h"
 #include "avs/game.h"
 #include "hooks/graphics/graphics.h"
 #include "misc/clipboard.h"
@@ -169,6 +170,13 @@ ThreadPool &encode_pool() {
     return *instance;
 }
 
+// where a capture's pixels are converted and handed to the api. never destroyed: the read
+// pool below can still be working at process exit, and it queues onto this one
+ThreadPool &capture_save_pool() {
+    static auto *instance = new ThreadPool(2);
+    return *instance;
+}
+
 // normalize the supported D3D formats to packed 24bpp RGB. callers screen the
 // format through surface_pixel_size first, so the black fill below is a fallback
 void surface_to_rgb(
@@ -437,11 +445,49 @@ static void dispatch_capture_save(PendingCapture capture) {
     if (image_processing_must_be_inline()) {
         capture_process();
     } else {
-        static auto pool = ThreadPool(2);
-        pool.add(std::move(capture_process));
+        capture_save_pool().add(std::move(capture_process));
     }
 }
 
+// destroying the BackbufferCopy returns its surface to the pool, which is a device call, so
+// it has to happen on whichever thread was cleared to do the read
+static void read_and_dispatch_capture(int screen, BackbufferCopy copy) {
+    PendingCapture capture;
+    if (!read_capture_surface(copy, capture)) {
+        graphics_capture_skip(screen);
+        return;
+    }
+
+    dispatch_capture_save(std::move(capture));
+}
+
+// Whether the readback runs on the present thread or a pool thread trades the game's frame
+// time against the risk of two threads being inside the device at once.
+//
+// The read is a LockRect plus a row by row memcpy of the whole back buffer: roughly 635us at
+// 720p and 1270us at 1080p. On the present thread that comes straight out of the game's frame
+// budget, and at 120Hz with a 60fps stream running it measured as a drop to 117fps. Moving it
+// to a pool thread gave the full 120 back.
+//
+// Only streaming is worth that trade. It is the only path that pays the cost on every frame,
+// and it is the only one the user has opted into by connecting a client. Screenshots and the
+// one off api captures stay inline: they are rare enough that a single slow frame does not
+// matter, and the hazard being avoided is reproduced rather than theoretical, since a pool
+// thread in LockRect while the present thread sat inside GetRenderTargetData deadlocked
+// DDR X2, whose device has no internal locking. Games already known to dislike threaded image
+// processing are excluded as well, on the assumption that whatever breaks them applies here.
+static bool capture_read_off_thread(int screen) {
+    return api::capture_pump::screen_claimed(screen) && !image_processing_must_be_inline();
+}
+
+ThreadPool &capture_read_pool() {
+    // one worker, so reads finish in the order they were submitted: a second worker could
+    // overtake a descheduled one and enqueue a stale frame over a newer one. never destroyed,
+    // so a read still running at process exit cannot touch a dead pool
+    static auto *instance = new ThreadPool(1);
+    return *instance;
+}
+
 // by this point the pixels are plain memory, so none of this needs the device
 static void dispatch_screenshot_save(std::vector<PendingWrite> writes, size_t screen_count) {
     auto screenshot_process = [writes = std::move(writes), screen_count]() mutable {
@@ -623,14 +669,34 @@ static void process_image_request(
     }
 
     if (!screenshot) {
-        PendingCapture capture;
-        if (!read_capture_surface(copies.front(), capture)) {
-            graphics_capture_skip(request.screen);
+        auto copy = std::move(copies.front());
+        copies.clear();
+
+        if (capture_read_off_thread(request.screen)) {
+            try {
+                capture_read_pool().add(
+                        [screen = request.screen, copy = std::move(copy)]() mutable {
+                    // an escape from here would cross a thread boundary and terminate
+                    try {
+                        read_and_dispatch_capture(screen, std::move(copy));
+                    } catch (const std::exception &error) {
+                        log_warning("graphics::d3d9", "capture read failed: {}", error.what());
+                        graphics_capture_skip(screen);
+                    } catch (...) {
+                        log_warning("graphics::d3d9", "capture read failed");
+                        graphics_capture_skip(screen);
+                    }
+                });
+            } catch (const std::exception &) {
+                // the copy went into the lambda before the queue could fail, so there is
+                // nothing left to read here and the client misses this frame
+                graphics_capture_skip(request.screen);
+            }
+
             return;
         }
 
-        copies.clear();
-        dispatch_capture_save(std::move(capture));
+        read_and_dispatch_capture(request.screen, std::move(copy));
         return;
     }
 

src/spice2x/launcher/options.cpp

@@ -1745,7 +1745,7 @@ static const std::vector<OptionDefinition> OPTION_DEFINITIONS = {
     },
     {
         // APIStreamEnable
-        .title = "API Video Stream Server Enable",
+        .title = "API Video Stream Server Enable (EXPERIMENTAL)",
         .name = "apistream",
         .desc = "Serves the mirrored screen as a video stream, on the API port plus two; "
             "alternative to API screen capture. Requires -api.\n\n"
@@ -1754,7 +1754,8 @@ static const std::vector<OptionDefinition> OPTION_DEFINITIONS = {
             "Parameters: screen (0-3), fps (1-60, default 30), q (1-100, default 70).\n\n"
             "Example with -api 1337: http://host:1339/stream.h264?fps=30&q=70\n\n"
             "VIEW ONLY - touch input still requires -api. "
-            "No password protection or encryption of any kind; video sent in the clear!",
+            "No password protection or encryption of any kind; video sent in the clear!\n\n"
+            "Streaming is known to cause older games to hang and crash.",
         .type = OptionType::Bool,
         .category = "Companion & API",
     },

src/spice2x/overlay/imgui/extensions.cpp

@@ -21,12 +21,36 @@ namespace ImGui {
                 base.y + overlay::apply_scaling(1.0f));
     }
 
+    // Tooltips follow the mouse and only avoid a small box around the cursor, so
+    // they can cover the option they describe. Anchor to the hovered item and let
+    // ImGui's popup placer pick a side that clears it and stays on-screen. The
+    // size is taken from the tooltip window, so it covers any content.
+    static void SetNextTooltipPos() {
+        ImGuiContext& g = *GImGui;
+        char name[32];
+        ImFormatString(name, IM_COUNTOF(name), "##Tooltip_%02d", g.TooltipOverrideCount);
+        ImGuiWindow *tooltip = ImGui::FindWindowByName(name);
+        if (tooltip == nullptr || !tooltip->WasActive) {
+            // Reopened tooltips are hidden for one frame while measuring size.
+            return;
+        }
+        const ImRect item(ImGui::GetItemRectMin(), ImGui::GetItemRectMax());
+        ImGui::SetNextWindowPos(ImGui::FindBestWindowPosForPopupEx(
+                item.Min,
+                ImGui::CalcWindowNextAutoFitSize(tooltip),
+                &tooltip->AutoPosLastDirection,
+                ImGui::GetPopupAllowedExtentRect(tooltip),
+                item,
+                ImGuiPopupPositionPolicy_Tooltip));
+    }
+
     void HelpTooltip(const char* desc) {
         ImGui::PushStyleColor(ImGuiCol_Border, bg);
         ImGui::PushStyleColor(ImGuiCol_BorderShadow, bg);
         ImGui::PushStyleColor(ImGuiCol_PopupBg, bg);
         ImGui::PushStyleColor(ImGuiCol_Text, fg);
 
+        SetNextTooltipPos();
         ImGui::BeginTooltip();
         ImGui::PushTextWrapPos(ImGui::GetFontSize() * 35.0f);
         ImGui::TextUnformatted(desc);
@@ -49,6 +73,7 @@ namespace ImGui {
         ImGui::PushStyleColor(ImGuiCol_PopupBg, bg);
         ImGui::PushStyleColor(ImGuiCol_Text, fg);
 
+        SetNextTooltipPos();
         ImGui::BeginTooltip();
         ImGui::PushTextWrapPos(ImGui::GetFontSize() * 35.0f);
         if (desc && desc[0]) {

src/spice2x/overlay/notifications.cpp

@@ -172,7 +172,9 @@ namespace overlay::notifications {
             // small gutter past the accent bar, then wrapped text
             ImGui::Dummy(ImVec2(apply_scaling(2.0f), 0.f));
             ImGui::SameLine();
-            ImGui::PushTextWrapPos(win_pos.x + win_size.x - apply_scaling(TOAST_PAD_X));
+            // wrap pos is in window-local space, not screen space - adding win_pos.x here
+            // pushed the boundary far past the window's own width, so it never wrapped
+            ImGui::PushTextWrapPos(win_size.x - apply_scaling(TOAST_PAD_X));
             ImGui::TextUnformatted(n.text.c_str());
             ImGui::PopTextWrapPos();
 

src/spice2x/overlay/windows/config.cpp

@@ -450,6 +450,8 @@ namespace overlay::windows {
                     ImGuiInputTextFlags_EscapeClearsAll)) {
                 this->search_filter_in_lower_case = strtolower(this->search_filter);
             }
+
+            // clear search terms
             if (!this->search_filter.empty()) {
                 ImGui::SameLine();
                 if (ImGui::Button("Clear")) {
@@ -457,15 +459,22 @@ namespace overlay::windows {
                     this->search_filter_in_lower_case.clear();
                 }
             }
+
+            // active only checkbox
+            ImGui::SameLine();
+            ImGui::Checkbox("Active Only", &this->search_active_only);
+
             ImGui::Spacing();
 
             // draw matching options
-            if (!this->search_filter.empty()) {
+            if (!this->search_filter.empty() || this->search_active_only) {
                 for (auto category : launcher::get_categories(launcher::Options::OptionsCategory::Everything)) {
                     this->build_options(
                         options,
                         category,
-                        const_cast<std::string *>(&this->search_filter_in_lower_case));
+                        const_cast<std::string *>(&this->search_filter_in_lower_case),
+                        false,
+                        this->search_active_only);
                 }
             }
         } else if (this->options_group_selected == OPTIONS_TAB_QUICK) {
@@ -5041,7 +5050,8 @@ namespace overlay::windows {
     }
 
     void Config::build_options(
-        std::vector<Option> *options, const std::string &category, const std::string *filter, bool quick_only) {
+        std::vector<Option> *options, const std::string &category, const std::string *filter,
+        bool quick_only, bool active_only) {
 
         // collect the options that match the current filters. doing this once lets us
         // skip rendering an empty header + table for categories with no matches, and
@@ -5066,8 +5076,11 @@ namespace overlay::windows {
             if (!definition.game_name.empty() && definition.game_name != this->games_selected_name) {
                 continue;
             }
-            if (filter != nullptr) {
-                if (filter->empty() || !option.search_match(*filter)) {
+            if (active_only && !option.is_active()) {
+                continue;
+            }
+            if (filter != nullptr && !filter->empty()) {
+                if (!option.search_match(*filter)) {
                     continue;
                 }
                 // limit to 30 results

src/spice2x/overlay/windows/config.h

@@ -141,6 +141,7 @@ namespace overlay::windows {
         int options_category = 0;
         std::string search_filter = "";
         std::string search_filter_in_lower_case = "";
+        bool search_active_only = false;
 
         // Options tab left-nav: selected group, currently highlighted category, and a pending scroll
         std::string options_group_selected = "";
@@ -216,7 +217,7 @@ namespace overlay::windows {
         void build_option_value_picker(Option& option);
         void build_options(
             std::vector<Option> *options, const std::string &category, const std::string *filter=nullptr,
-            bool quick_only=false);
+            bool quick_only=false, bool active_only=false);
         void build_options_tab(float page_offset);
         void build_controller_tab(float page_offset, ControllerPage *page_selected_new);
         bool build_nav_header(const char *label, bool active);

src/spice2x/overlay/windows/patch_manager.cpp

@@ -7,6 +7,7 @@
 #include <unordered_set>
 #include "avs/game.h"
 #include "cfg/configurator.h"
+#include "external/imgui/imgui_internal.h"
 #include "games/io.h"
 #include "launcher/launcher.h"
 #include "misc/clipboard.h"
@@ -215,19 +216,14 @@ namespace overlay::windows {
             return name;
         }
 
-        // draw a square checkmark for mixed-state groups (neither checked or unchecked)
-        void render_patch_group_mixed_checkbox_mark() {
-            const auto check_min = ImGui::GetItemRectMin();
-            const float check_size = ImGui::GetFrameHeight();
-            const float calculated_padding = check_size / 3.6f;
-            const float padding = calculated_padding < 1.0f ? 1.0f : calculated_padding;
-            ImGui::GetWindowDrawList()->AddRectFilled(
-                ImVec2(check_min.x + padding, check_min.y + padding),
-                ImVec2(
-                    check_min.x + check_size - padding,
-                    check_min.y + check_size - padding),
-                ImGui::GetColorU32(ImGuiCol_CheckMark),
-                ImGui::GetStyle().FrameRounding);
+        // the state text next to a toggle is the checkbox label, so it has to dim itself instead
+        // of being wrapped in BeginDisabled (which would also kill the click area)
+        void push_toggle_label_color(bool checked) {
+            auto color = ImGui::GetStyleColorVec4(ImGuiCol_Text);
+            if (!checked) {
+                color.w *= ImGui::GetStyle().DisabledAlpha;
+            }
+            ImGui::PushStyleColor(ImGuiCol_Text, color);
         }
 
         // render a tri-state checkbox for groups
@@ -237,18 +233,24 @@ namespace overlay::windows {
             const std::vector<size_t>& members,
             bool& checked) {
             const bool mixed = state.status == PatchGroupStatus::Mixed;
-            ImGui::BeginDisabled(state.status == PatchGroupStatus::Error);
+            const bool error = state.status == PatchGroupStatus::Error;
+            ImGui::BeginDisabled(error);
             if (mixed) {
-                ImGui::PushStyleColor(ImGuiCol_CheckMark, ImVec4(0.f, 0.f, 0.f, 0.f));
+                ImGui::PushItemFlag(ImGuiItemFlags_MixedValue, true);
             }
-            const bool changed = ImGui::Checkbox("##group_checked_checkbox", &checked);
+            // the state text doubles as the checkbox label so clicking it toggles the group;
+            // errored groups show the failing patch there instead
+            const char *label = "##group_checked_checkbox";
+            if (!error) {
+                label = mixed ? "mixed" : (checked ? "ON" : "off");
+            }
+            push_toggle_label_color(checked);
+            const bool changed = ImGui::Checkbox(label, &checked);
+            ImGui::PopStyleColor();
             if (mixed) {
-                ImGui::PopStyleColor();
+                ImGui::PopItemFlag();
             }
             ImGui::EndDisabled();
-            if (mixed && !changed) {
-                render_patch_group_mixed_checkbox_mark();
-            }
             if (!changed) {
                 return false;
             }
@@ -266,11 +268,11 @@ namespace overlay::windows {
             return true;
         }
 
-        // render aggregate status or error text beside the group checkbox
-        void render_patch_group_status(const PatchGroupState& state, bool checked) {
-            ImGui::SameLine();
-            ImGui::AlignTextToFramePadding();
+        // render error text beside the group checkbox; other states are shown as its label
+        void render_patch_group_status(const PatchGroupState& state) {
             if (state.status == PatchGroupStatus::Error) {
+                ImGui::SameLine();
+                ImGui::AlignTextToFramePadding();
                 const auto& error_patch = patcher::patches[state.first_error_index];
                 const auto error_reason = error_patch.error_reason.empty()
                     ? "Unknown error"
@@ -279,12 +281,6 @@ namespace overlay::windows {
                 ImGui::PushStyleColor(ImGuiCol_Text, ImVec4(1.f, 0.f, 0.f, 1.f));
                 ImGui::TextUnformatted(error_text.c_str());
                 ImGui::PopStyleColor();
-            } else if (state.status == PatchGroupStatus::Mixed) {
-                ImGui::TextUnformatted("mixed");
-            } else {
-                ImGui::BeginDisabled(!checked);
-                ImGui::TextUnformatted(checked ? "ON" : "off");
-                ImGui::EndDisabled();
             }
         }
 
@@ -311,7 +307,7 @@ namespace overlay::windows {
             if (ImGui::IsItemHovered(ImGui::TOOLTIP_FLAGS)) {
                 show_patch_group_tooltip(group);
             }
-            render_patch_group_status(group_state, group_checked);
+            render_patch_group_status(group_state);
 
             ImGui::TableSetColumnIndex(0);
             const auto group_name = get_patch_group_display_name(
@@ -874,8 +870,18 @@ namespace overlay::windows {
                         render_patch_group_child_gutter(last_group_child);
                         ImGui::SameLine();
                     }
+                    // plain on/off patches have no extra widget, so the state text doubles as the
+                    // checkbox label - clicking the text toggles it, like the Options tab
+                    const bool has_extra_widget =
+                        patch_status == patcher::PatchStatus::Error
+                        || patch.type == patcher::PatchType::Union
+                        || patch.type == patcher::PatchType::Integer;
+                    const char *checkbox_label = has_extra_widget
+                        ? "##patch_checked_checkbox"
+                        : (patch_checked ? "ON" : "off");
+                    push_toggle_label_color(patch_checked);
                     ImGui::BeginDisabled(patch_status == patcher::PatchStatus::Error);
-                    if (ImGui::Checkbox("##patch_checked_checkbox", &patch_checked)) {
+                    if (ImGui::Checkbox(checkbox_label, &patch_checked)) {
                         patcher::config_dirty = true;
                         switch (patch_status) {
                             case patcher::PatchStatus::Enabled:
@@ -901,13 +907,14 @@ namespace overlay::windows {
                         patch.last_status = patcher::is_patch_active(patch);
                     }
                     ImGui::EndDisabled();
+                    ImGui::PopStyleColor();
                     if (ImGui::IsItemHovered(ImGui::TOOLTIP_FLAGS)) {
                         show_patch_tooltip(patch);
                     }
 
                     // second column, part 2: additional options UI (dropdown, text input)
-                    ImGui::SameLine();
                     if (patch_status == patcher::PatchStatus::Error){
+                        ImGui::SameLine();
                         ImGui::AlignTextToFramePadding();
                         ImGui::PushStyleColor(ImGuiCol_Text, ImVec4(1.f, 0.f, 0.f, 1.f));
                         if (patch.error_reason.empty()) {
@@ -917,6 +924,7 @@ namespace overlay::windows {
                         }
                         ImGui::PopStyleColor();
                     } else if (patch.type == patcher::PatchType::Union || patch.type == patcher::PatchType::Integer) {
+                        ImGui::SameLine();
                         if (patch_status == patcher::PatchStatus::Enabled) {
                             if (patch.type == patcher::PatchType::Union) {
                                 set_patch_option_width();
@@ -970,11 +978,6 @@ namespace overlay::windows {
                                 show_patch_tooltip(patch);
                             }
                         }
-                    } else {
-                        ImGui::AlignTextToFramePadding();
-                        ImGui::BeginDisabled(!patch_checked);
-                        ImGui::TextUnformatted(patch_checked ? "ON" : "off");
-                        ImGui::EndDisabled();
                     }
 
                     ImGui::HighlightTableRowOnHover();