From fa3087609d4d815bf07ba91757c4067f8672ede7 Mon Sep 17 00:00:00 2001 From: Chuck Date: Sun, 12 Jul 2026 19:43:06 -0400 Subject: [PATCH 1/2] perf(display): dirty tracking in update_display + plugin FPS declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit update_display now skips SetImage+SwapOnVSync when the frame is byte-identical to the last pushed one (adler32 digest) AND brightness is unchanged — brightness is part of the digest, and set_brightness additionally resets it, so a dim-schedule change can never be skipped. clear() resets the digest (it writes to the matrix directly). Skipping a swap is hardware-safe: the panel refreshes the current frame from the driver's own thread; swaps only change content. Kill switch: display.dirty_tracking: false restores always-push. display_controller's high-FPS decision gains a precedence step: a plugin exposing needs_high_fps is honored first (so static-image can declare False for still PNGs and stop burning a 125fps loop on them); static-image without the attribute keeps its historical forced high-FPS (GIF back-compat); scrolling logic is otherwise unchanged. Verified with 7 tests against the real DisplayManager on RGBMatrixEmulator (identical-frame skip, pixel-change push, clear and brightness invalidation, snapshot-through-skip, kill switch) plus the 202-test display/controller/vegas suites, and a clean devpi deploy. Audit: every SetImage/SwapOnVSync/Clear/brightness call site is inside display_manager — no external writer can bypass the digest. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam --- src/display_controller.py | 17 +++- src/display_manager.py | 46 +++++++++- test/test_display_dirty_tracking.py | 135 ++++++++++++++++++++++++++++ 3 files changed, 193 insertions(+), 5 deletions(-) create mode 100644 test/test_display_dirty_tracking.py diff --git a/src/display_controller.py b/src/display_controller.py index 8ae64f34..795e4873 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -2072,10 +2072,21 @@ def run(self): # For plugins, call display multiple times to allow game rotation if manager_to_display and hasattr(manager_to_display, 'display'): - # Check if plugin needs high FPS (like stock ticker) - # Always enable high-FPS for static-image plugin (for GIF animation support) + # High-FPS decision, in precedence order: + # 1. A plugin that declares needs_high_fps knows best + # (e.g. static-image sets it False for still PNGs, + # True for animated GIFs). + # 2. Back-compat: older static-image versions without + # the attribute keep the historical forced high-FPS + # (GIF support). + # 3. Otherwise scrolling plugins get high FPS. plugin_id = getattr(manager_to_display, 'plugin_id', None) - if plugin_id == 'static-image': + declared = getattr(manager_to_display, 'needs_high_fps', None) + if declared is not None: + needs_high_fps = bool(declared) + logger.debug("FPS check for %s - plugin declares needs_high_fps=%s", + active_mode, needs_high_fps) + elif plugin_id == 'static-image': needs_high_fps = True logger.debug("FPS check - static-image plugin: forcing high-FPS mode for GIF support") else: diff --git a/src/display_manager.py b/src/display_manager.py index 019c05c6..818815fa 100644 --- a/src/display_manager.py +++ b/src/display_manager.py @@ -36,6 +36,7 @@ from typing import Dict, Any, List, Optional import logging import math +import zlib import freetype # Get logger without configuring @@ -188,6 +189,12 @@ def __init__(self, config: Dict[str, Any] = None, force_fallback: bool = False, self._snapshot_path = "/tmp/led_matrix_preview.png" # nosec B108 - fixed path intentional; web UI reads same path self._snapshot_min_interval_sec = 0.2 # max ~5 fps self._last_snapshot_ts = 0.0 + # Dirty tracking: (image digest, brightness) of the last frame pushed + # to the panel; update_display() skips identical pushes. Kill switch: + # display.dirty_tracking: false. + self._dirty_tracking_enabled = bool( + self.config.get('display', {}).get('dirty_tracking', True)) + self._last_pushed_digest = None # Scrolling state tracking for graceful updates self._scrolling_state = { @@ -418,6 +425,10 @@ def set_brightness(self, brightness: int) -> bool: try: # RGBMatrix accepts brightness as a property self.matrix.brightness = brightness + # Brightness applies on the next swap — force a re-push even if + # the image itself is unchanged (belt-and-braces: brightness is + # also part of the dirty-tracking digest when readable). + self._last_pushed_digest = None logger.info(f"[BRIGHTNESS] Display brightness set to {brightness}%") return True except AttributeError as e: @@ -509,7 +520,20 @@ def _composite_double_sided(self): return phys def update_display(self): - """Update the display using double buffering with proper sync.""" + """Update the display using double buffering with proper sync. + + Skips the panel push entirely when the frame is byte-identical to + the last pushed one (same image digest AND same brightness) — static + content re-rendered every second, and 125 fps loops between actual + scroll steps, otherwise re-walk the full framebuffer for nothing. + The panel keeps refreshing the current frame from its own thread, + so skipping a swap never blanks or freezes the hardware. + + Correctness hinges on invalidation: clear() resets the digest (it + writes to the matrix directly), and brightness is PART of the digest + so a dim-schedule change is never skipped. Disable via config + ``display.dirty_tracking: false`` if a redraw issue is ever suspected. + """ try: if self.matrix is None: # Fallback mode - no actual hardware to update @@ -521,6 +545,19 @@ def update_display(self): if self._capture_mode_active: return # Skip hardware write — content is being captured off-screen + digest = None + if self._dirty_tracking_enabled: + try: + brightness = getattr(self.matrix, 'brightness', None) + except Exception: + brightness = None + digest = (zlib.adler32(self.image.tobytes()), brightness) + if digest == self._last_pushed_digest: + # Nothing changed since the last push — the panel is + # already showing exactly this frame. + self._write_snapshot_if_due() + return + # Copy the current image to the offscreen canvas. In double-sided # mode the logical screen is first tiled across the full chain. if self._double_sided is not None: @@ -530,10 +567,12 @@ def update_display(self): # Swap buffers immediately self.matrix.SwapOnVSync(self.offscreen_canvas) - + # Swap our canvas references self.offscreen_canvas, self.current_canvas = self.current_canvas, self.offscreen_canvas + self._last_pushed_digest = digest + # Write a snapshot for the web preview (throttled) self._write_snapshot_if_due() except Exception as e: @@ -569,6 +608,9 @@ def clear(self): # Clear both canvases and the underlying matrix to ensure no artifacts. # Failures are non-fatal — the image buffer is already black above, so # the next update_display() call will push clean content regardless. + # The matrix content no longer matches the last pushed digest, + # so dirty tracking must not skip the next push. + self._last_pushed_digest = None try: self.offscreen_canvas.Clear() except (RuntimeError, OSError) as e: diff --git a/test/test_display_dirty_tracking.py b/test/test_display_dirty_tracking.py new file mode 100644 index 00000000..dcd394d9 --- /dev/null +++ b/test/test_display_dirty_tracking.py @@ -0,0 +1,135 @@ +"""Tests for update_display dirty tracking (src/display_manager.py). + +Runs against RGBMatrixEmulator (EMULATOR=true), exercising the REAL +DisplayManager — not a mock — so the skip logic, its invalidation hooks, +and the kill switch are verified off-Pi. + +The invariants: +- identical frames are pushed exactly once (SwapOnVSync not re-called) +- ANY pixel change pushes +- clear() and set_brightness() invalidate (the two paths that alter panel + state outside the digest's view) +- the kill switch (display.dirty_tracking: false) restores always-push +""" + +import os +import sys + +os.environ["EMULATOR"] = "true" + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + + +@pytest.fixture(scope="module") +def dm(): + """One real DisplayManager on the emulator (it's a process singleton).""" + from src.display_manager import DisplayManager + DisplayManager._instance = None + DisplayManager._initialized = False + manager = DisplayManager({ + "display": { + "hardware": {"rows": 32, "cols": 64, "chain_length": 2, + "parallel": 1, "brightness": 90}, + "runtime": {"gpio_slowdown": 0}, + }, + }, suppress_test_pattern=True) + yield manager + + +class _SwapSpy: + """Counts SwapOnVSync calls through the real matrix object.""" + + def __init__(self, matrix): + self.matrix = matrix + self.count = 0 + self._orig = matrix.SwapOnVSync + + def __enter__(self): + def counting(canvas): + self.count += 1 + return self._orig(canvas) + self.matrix.SwapOnVSync = counting + return self + + def __exit__(self, *exc): + self.matrix.SwapOnVSync = self._orig + + +class TestDirtyTracking: + def test_identical_frames_push_once(self, dm): + dm.draw.rectangle([0, 0, 10, 10], fill=(255, 0, 0)) + with _SwapSpy(dm.matrix) as spy: + dm.update_display() + dm.update_display() + dm.update_display() + assert spy.count == 1 + + def test_pixel_change_pushes(self, dm): + dm.update_display() + with _SwapSpy(dm.matrix) as spy: + dm.draw.point((5, 5), fill=(0, 255, 0)) + dm.update_display() + dm.update_display() # unchanged again + assert spy.count == 1 + + def test_clear_invalidates(self, dm): + dm.draw.rectangle([0, 0, 20, 20], fill=(0, 0, 255)) + dm.update_display() + dm.clear() # writes to the matrix directly; digest must reset + with _SwapSpy(dm.matrix) as spy: + dm.update_display() # black frame after clear must still push + assert spy.count == 1 + + def test_brightness_change_forces_push(self, dm): + dm.draw.rectangle([0, 0, 20, 20], fill=(200, 200, 200)) + dm.update_display() + with _SwapSpy(dm.matrix) as spy: + dm.update_display() # identical -> skipped + assert spy.count == 0 + dm.set_brightness(40) # dim schedule scenario + dm.update_display() # same image, new brightness -> push + assert spy.count == 1 + dm.set_brightness(90) + + def test_snapshot_still_written_on_skip(self, dm, tmp_path): + """The web preview path must keep working through skipped pushes.""" + dm._snapshot_path = str(tmp_path / "snap.png") + dm._last_snapshot_ts = 0.0 + dm.draw.rectangle([0, 0, 30, 8], fill=(255, 255, 0)) + dm.update_display() # push + snapshot + assert os.path.exists(dm._snapshot_path) + + +class TestKillSwitch: + def test_dirty_tracking_can_be_disabled(self, dm): + dm._dirty_tracking_enabled = False + try: + dm.draw.rectangle([0, 0, 10, 10], fill=(1, 2, 3)) + with _SwapSpy(dm.matrix) as spy: + dm.update_display() + dm.update_display() + dm.update_display() + assert spy.count == 3 # always-push, exactly the old behavior + finally: + dm._dirty_tracking_enabled = True + dm._last_pushed_digest = None + + def test_config_flag_wires_through(self): + from src.display_manager import DisplayManager + DisplayManager._instance = None + DisplayManager._initialized = False + manager = DisplayManager({ + "display": { + "hardware": {"rows": 32, "cols": 64, "chain_length": 1, + "parallel": 1}, + "runtime": {"gpio_slowdown": 0}, + "dirty_tracking": False, + }, + }, suppress_test_pattern=True) + assert manager._dirty_tracking_enabled is False + + +if __name__ == "__main__": + sys.exit(pytest.main([__file__, "-v"])) From 16a79cb9a8f9eb33b5faef975cc2c1325d86ad78 Mon Sep 17 00:00:00 2001 From: Chuck Date: Mon, 13 Jul 2026 10:19:56 -0400 Subject: [PATCH 2/2] fix(display): serialize update_display, narrow brightness exception, log fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on #406, verified against current code: - update_display() can genuinely be called from background threads (some sports base classes call it directly from inside update() for an immediate "live" refresh), not just the render loop — confirmed via the existing follower-mode gating wrapper in display_controller.py, which exists specifically because "background plugin threads" can reach it. Without a lock, two callers could both pass the digest check before either writes _last_pushed_digest back, causing a redundant push, or interleave the offscreen/current canvas swap. Added self._update_lock (RLock, in case of re-entrant callers) around the full method body so every call site is automatically covered — no caller changes needed. (No prior lock existed to reuse on DisplayManager; this adds one.) - Narrowed the brightness-read exception handler to AttributeError, matching the established pattern in get_brightness()/set_brightness() — a getattr() with a default already swallows AttributeError, so the only case this guards is the property getter itself raising, and the established pattern treats that as an expected, specific failure mode rather than something to blanket-catch. - FPS-check debug log now includes the plugin_id already in scope (previously only active_mode) and a "[DisplayController]" prefix for grep-ability, matching the sibling log two lines below it. - test_display_dirty_tracking.py: dm fixture and test_config_flag_wires_through now reset the DisplayManager singleton on teardown, matching the pattern test_display_manager.py already uses elsewhere in the same file family. - test_snapshot_still_written_on_skip previously only exercised the non-skip (push) path despite its name; now performs a second update that meets the skip conditions (identical frame) and asserts the snapshot is still written even though the panel push itself is skipped. All 7 dirty-tracking tests pass, plus the full display_manager/ display_controller/vegas suite (140 passed). Full repo suite has only the 5 known pre-existing failures (double-sided config x2, state_reconciliation x2, and test_circuit_breaker's conftest.py mock signature drift — the latter fixed in #400, which this branch's base predates). --- src/display_controller.py | 6 ++- src/display_manager.py | 81 +++++++++++++++++------------ test/test_display_dirty_tracking.py | 34 ++++++++---- 3 files changed, 77 insertions(+), 44 deletions(-) diff --git a/src/display_controller.py b/src/display_controller.py index 795e4873..75a2e5dd 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -2084,8 +2084,10 @@ def run(self): declared = getattr(manager_to_display, 'needs_high_fps', None) if declared is not None: needs_high_fps = bool(declared) - logger.debug("FPS check for %s - plugin declares needs_high_fps=%s", - active_mode, needs_high_fps) + logger.debug( + "[DisplayController] FPS check for %s (plugin=%s) - " + "plugin declares needs_high_fps=%s", + active_mode, plugin_id, needs_high_fps) elif plugin_id == 'static-image': needs_high_fps = True logger.debug("FPS check - static-image plugin: forcing high-FPS mode for GIF support") diff --git a/src/display_manager.py b/src/display_manager.py index 818815fa..c218d97e 100644 --- a/src/display_manager.py +++ b/src/display_manager.py @@ -32,6 +32,7 @@ from rgbmatrix import RGBMatrix, RGBMatrixOptions from contextlib import contextmanager from PIL import Image, ImageDraw, ImageFont +import threading import time from typing import Dict, Any, List, Optional import logging @@ -195,6 +196,11 @@ def __init__(self, config: Dict[str, Any] = None, force_fallback: bool = False, self._dirty_tracking_enabled = bool( self.config.get('display', {}).get('dirty_tracking', True)) self._last_pushed_digest = None + # Serializes update_display(): plugins can call it directly from + # background threads (see docstring on update_display), not just the + # render loop. RLock in case a caller within the critical section + # ever re-enters (e.g. via a nested draw callback). + self._update_lock = threading.RLock() # Scrolling state tracking for graceful updates self._scrolling_state = { @@ -533,48 +539,57 @@ def update_display(self): writes to the matrix directly), and brightness is PART of the digest so a dim-schedule change is never skipped. Disable via config ``display.dirty_tracking: false`` if a redraw issue is ever suspected. + + Serialized via ``_update_lock``: plugins can call this directly from + background threads (e.g. sports base classes push an immediate + "live" refresh from inside update()), so without a lock two callers + could both pass the digest check before either writes it back, + double-pushing a frame, or interleave the offscreen/current canvas + swap below. The lock is scoped to this method, so callers never + need to know about it. """ try: - if self.matrix is None: - # Fallback mode - no actual hardware to update - logger.debug("Update display called in fallback mode (no hardware)") - # Still write a snapshot so the web UI can preview - self._write_snapshot_if_due() - return - - if self._capture_mode_active: - return # Skip hardware write — content is being captured off-screen - - digest = None - if self._dirty_tracking_enabled: - try: - brightness = getattr(self.matrix, 'brightness', None) - except Exception: - brightness = None - digest = (zlib.adler32(self.image.tobytes()), brightness) - if digest == self._last_pushed_digest: - # Nothing changed since the last push — the panel is - # already showing exactly this frame. + with self._update_lock: + if self.matrix is None: + # Fallback mode - no actual hardware to update + logger.debug("Update display called in fallback mode (no hardware)") + # Still write a snapshot so the web UI can preview self._write_snapshot_if_due() return - # Copy the current image to the offscreen canvas. In double-sided - # mode the logical screen is first tiled across the full chain. - if self._double_sided is not None: - self.offscreen_canvas.SetImage(self._composite_double_sided()) - else: - self.offscreen_canvas.SetImage(self.image) + if self._capture_mode_active: + return # Skip hardware write — content is being captured off-screen + + digest = None + if self._dirty_tracking_enabled: + try: + brightness = getattr(self.matrix, 'brightness', None) + except AttributeError: + brightness = None + digest = (zlib.adler32(self.image.tobytes()), brightness) + if digest == self._last_pushed_digest: + # Nothing changed since the last push — the panel is + # already showing exactly this frame. + self._write_snapshot_if_due() + return + + # Copy the current image to the offscreen canvas. In double-sided + # mode the logical screen is first tiled across the full chain. + if self._double_sided is not None: + self.offscreen_canvas.SetImage(self._composite_double_sided()) + else: + self.offscreen_canvas.SetImage(self.image) - # Swap buffers immediately - self.matrix.SwapOnVSync(self.offscreen_canvas) + # Swap buffers immediately + self.matrix.SwapOnVSync(self.offscreen_canvas) - # Swap our canvas references - self.offscreen_canvas, self.current_canvas = self.current_canvas, self.offscreen_canvas + # Swap our canvas references + self.offscreen_canvas, self.current_canvas = self.current_canvas, self.offscreen_canvas - self._last_pushed_digest = digest + self._last_pushed_digest = digest - # Write a snapshot for the web preview (throttled) - self._write_snapshot_if_due() + # Write a snapshot for the web preview (throttled) + self._write_snapshot_if_due() except Exception as e: logger.error(f"Error updating display: {e}") diff --git a/test/test_display_dirty_tracking.py b/test/test_display_dirty_tracking.py index dcd394d9..6f207f2b 100644 --- a/test/test_display_dirty_tracking.py +++ b/test/test_display_dirty_tracking.py @@ -36,6 +36,8 @@ def dm(): }, }, suppress_test_pattern=True) yield manager + DisplayManager._instance = None + DisplayManager._initialized = False class _SwapSpy: @@ -101,6 +103,16 @@ def test_snapshot_still_written_on_skip(self, dm, tmp_path): dm.update_display() # push + snapshot assert os.path.exists(dm._snapshot_path) + # Re-open the snapshot's own throttle and remove the file, then push + # the identical frame again: dirty tracking must skip the panel + # write but the snapshot must still be (re-)written on that path. + os.remove(dm._snapshot_path) + dm._last_snapshot_ts = 0.0 + with _SwapSpy(dm.matrix) as spy: + dm.update_display() # identical frame -> panel push skipped + assert spy.count == 0 + assert os.path.exists(dm._snapshot_path) + class TestKillSwitch: def test_dirty_tracking_can_be_disabled(self, dm): @@ -120,15 +132,19 @@ def test_config_flag_wires_through(self): from src.display_manager import DisplayManager DisplayManager._instance = None DisplayManager._initialized = False - manager = DisplayManager({ - "display": { - "hardware": {"rows": 32, "cols": 64, "chain_length": 1, - "parallel": 1}, - "runtime": {"gpio_slowdown": 0}, - "dirty_tracking": False, - }, - }, suppress_test_pattern=True) - assert manager._dirty_tracking_enabled is False + try: + manager = DisplayManager({ + "display": { + "hardware": {"rows": 32, "cols": 64, "chain_length": 1, + "parallel": 1}, + "runtime": {"gpio_slowdown": 0}, + "dirty_tracking": False, + }, + }, suppress_test_pattern=True) + assert manager._dirty_tracking_enabled is False + finally: + DisplayManager._instance = None + DisplayManager._initialized = False if __name__ == "__main__":