diff --git a/CHANGELOG.md b/CHANGELOG.md index 35ac9182..44cfe81e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,21 @@ accepts both, but the store flags the old spelling as deprecated ## Unreleased +### Fixed + +- The web preview and `/api/v3/display/current` no longer stay black for a + whole screen that draws its card once and then holds it. The snapshot is + written from `update_display()` at most once per write interval, so a frame + pushed inside that interval was skipped and left for the next + `update_display()` -- which such a screen never makes. Soccer's + recent/upcoming cards skip redundant redraws, and the first one after an + on-demand start lands a few milliseconds after the start's clear wrote a + black frame: on ledpi the preview showed 0 lit pixels for the whole 15 s + while the panel showed the card. `DisplayManager` now remembers a skipped + changed frame, and the render loop writes it (`write_owed_snapshot()`) + once the interval has passed. The cadence is unchanged, and nothing extra + runs when no frame is owed. + ### Cheap per-frame and per-fetch savings - `BaseOddsManager.get_odds()` no longer pretty-prints every odds response diff --git a/src/display_controller.py b/src/display_controller.py index 8756518a..31234caf 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -1201,6 +1201,13 @@ class DisplayController: note = getattr(self.plugin_manager, 'note_display_duration', None) if note is not None and plugin_id: note(plugin_id, time.monotonic() - started) + # A screen that drew once and holds makes no more + # update_display() calls, so a frame the preview throttle + # skipped would otherwise never reach the snapshot. + write_owed = getattr(getattr(self, 'display_manager', None), + 'write_owed_snapshot', None) + if write_owed is not None: + write_owed() def _health_tracker(self): """The plugin circuit breaker, or None when it is not enabled.""" diff --git a/src/display_manager.py b/src/display_manager.py index 3e211e9f..07471f5b 100644 --- a/src/display_manager.py +++ b/src/display_manager.py @@ -317,6 +317,11 @@ class DisplayManager: # is handed to the writer; this only once it has been saved, so an # mtime touch never vouches for a frame still waiting to be written. self._saved_snapshot_digest: Optional[int] = None + # A changed frame reached _write_snapshot_if_due() inside the write + # interval and was skipped. Nothing writes it unless update_display() + # runs again, and a screen that draws once and holds never calls it + # again -- see write_owed_snapshot(). + self._snapshot_owed = False self._snapshot_dir_prepared = False # Background writer used mid-scroll; see _write_snapshot_if_due. self._snapshot_cond = threading.Condition() @@ -1788,9 +1793,10 @@ class DisplayManager: if frame_checksum is not None: digest = frame_checksum + frame_changed = digest != self._last_snapshot_digest action = snapshot_policy.decide( now, self._last_snapshot_ts, self._last_snapshot_touch_ts, - viewer_fresh, digest != self._last_snapshot_digest) + viewer_fresh, frame_changed) else: # Ask as if the frame had changed before paying to find out. # decide() is monotone in frame_changed -- a SKIP for a @@ -1802,15 +1808,24 @@ class DisplayManager: now, self._last_snapshot_ts, self._last_snapshot_touch_ts, viewer_fresh, True) if action is snapshot_policy.SnapshotAction.SKIP: + # Not hashed, so not known to be unchanged: owed until a + # later look finds it written or unchanged. + self._snapshot_owed = True return digest = zlib.adler32(self.image.tobytes()) - if digest == self._last_snapshot_digest: + frame_changed = digest != self._last_snapshot_digest + if not frame_changed: # Unchanged after all: the decision an unchanged frame gets. action = snapshot_policy.decide( now, self._last_snapshot_ts, self._last_snapshot_touch_ts, viewer_fresh, False) if action is snapshot_policy.SnapshotAction.SKIP: + # A changed frame inside the write interval stays owed: the + # next update_display() would write it, but a static screen + # may not make one -- write_owed_snapshot() covers that. + self._snapshot_owed = frame_changed return + self._snapshot_owed = False if (action is snapshot_policy.SnapshotAction.TOUCH and self._saved_snapshot_digest == digest): # mtime bump only: keeps the health check (snapshot age) @@ -1845,6 +1860,35 @@ class DisplayManager: except Exception as e: self._log_snapshot_failure(e) + def write_owed_snapshot(self) -> None: + """Write a frame the snapshot throttle skipped, once it is due. + + The preview snapshot is only ever written from update_display(), and + at most once per write interval (snapshot_policy). A frame pushed + inside that interval is skipped, and is written by the next + update_display() that comes after it -- but a screen that draws its + card once and then holds it makes no further call. Its frame was on + the panel and never in the preview: soccer's recent/upcoming cards + skip redundant redraws, and the first one after an on-demand start + (pushed a few milliseconds after the controller's clear) left + /api/v3/display/current and the web preview black for the whole + screen while the panel showed the card. + + The render loop calls this after each frame. Cheap when nothing is + owed (one attribute read); otherwise the usual policy decides, so + the write still waits out the interval and an unchanged frame is + never re-encoded. + """ + if not self._snapshot_owed: + return + try: + if self._writes_suppressed(): + return + with self._update_lock: + self._write_snapshot_if_due() + except Exception as e: # pylint: disable=broad-except + self._log_snapshot_failure(e) + def _log_snapshot_failure(self, error: Exception) -> None: # Snapshot failures must never break display — but they must not # be silent either: the snapshot's mtime is the web UI's display diff --git a/test/test_snapshot_owed_frame.py b/test/test_snapshot_owed_frame.py new file mode 100644 index 00000000..fe027e6e --- /dev/null +++ b/test/test_snapshot_owed_frame.py @@ -0,0 +1,162 @@ +"""A frame the preview throttle skipped still reaches the snapshot. + +The preview snapshot (/api/v3/display/current, the web UI's live preview) is +only written from update_display(), at most once per write interval. A screen +that draws its card once and then holds it -- soccer's recent/upcoming cards +skip redundant redraws -- pushes exactly one frame. When that push lands inside +the interval, e.g. a few milliseconds after the on-demand start's clear wrote a +black frame, the throttle skips it and nothing ever writes it: on ledpi the +preview stayed black for soccer's whole 15 s screen while the panel showed the +card, and the next screen "rendered immediately". + +Runs the real DisplayManager on the emulator, like test_display_dirty_tracking. +""" + +import os +import sys +import types + +os.environ["EMULATOR"] = "true" + +import pytest +from PIL import Image + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + + +@pytest.fixture(scope="module") +def dm(tmp_path_factory): + from src.display_manager import DisplayManager + DisplayManager._instance = None + manager = DisplayManager({ + "display": { + "hardware": {"rows": 32, "cols": 64, "chain_length": 2, + "parallel": 1, "brightness": 90}, + "runtime": {"gpio_slowdown": 0}, + }, + }, suppress_test_pattern=True) + manager._snapshot_path = str( + tmp_path_factory.mktemp("owed_snapshot") / "led_matrix_preview.png") + yield manager + DisplayManager._instance = None + + +@pytest.fixture +def viewer(dm, monkeypatch, tmp_path): + """A preview is open (1 s write interval); fresh snapshot bookkeeping.""" + monkeypatch.setattr(dm, "_viewer_is_fresh", lambda now: True) + dm._viewer_was_fresh = True + dm._snapshot_path = str(tmp_path / "snap.png") + dm._last_snapshot_ts = 0.0 + dm._last_snapshot_touch_ts = 0.0 + dm._last_snapshot_digest = None + dm._saved_snapshot_digest = None + dm._snapshot_owed = False + dm.set_scrolling_state(False) + return dm + + +def _lit(path): + with Image.open(path) as img: + return sum(1 for p in img.convert("RGB").getdata() if max(p) > 20) + + +def _age_last_write(dm, seconds=2.0): + """As if `seconds` had passed since the last snapshot write.""" + dm._last_snapshot_ts -= seconds + dm._last_snapshot_touch_ts -= seconds + + +def _clear_then_draw_card(dm): + """The on-demand start's clear, then the card a few ms later.""" + dm.clear() + dm.update_display() # black frame: written + assert _lit(dm._snapshot_path) == 0 + dm.draw.rectangle([4, 4, 40, 20], fill=(255, 255, 0)) + dm.update_display() # the card: inside the interval + + +def _controller(dm): + from src import display_controller as dc_module + controller = dc_module.DisplayController.__new__(dc_module.DisplayController) + controller.plugin_manager = None + controller.display_manager = dm + return controller + + +class _HoldingPlugin: + """Already showing its card: display() returns True and draws nothing.""" + + plugin_id = "holding" + + def __init__(self): + self.calls = 0 + + def display(self, display_mode=None, force_clear=False): + self.calls += 1 + return True + + +def test_a_held_card_reaches_the_preview_on_the_next_frame(viewer): + dm = viewer + _clear_then_draw_card(dm) + assert _lit(dm._snapshot_path) == 0 # the throttle skipped the card + + controller = _controller(dm) + plugin = _HoldingPlugin() + _age_last_write(dm) + # The render loop's next frame: the plugin draws nothing and makes no + # update_display() call, as soccer's switch cards do. + assert controller._display_once(plugin, "soccer_eng.1_recent", True) is True + assert plugin.calls == 1 + assert _lit(dm._snapshot_path) > 0 + + +def test_the_owed_write_still_waits_out_the_interval(viewer, monkeypatch): + dm = viewer + _clear_then_draw_card(dm) + saves = [] + monkeypatch.setattr(dm, "_save_snapshot", lambda image: saves.append(image)) + dm.write_owed_snapshot() # still inside the interval + assert saves == [] + _age_last_write(dm) + dm.write_owed_snapshot() + assert len(saves) == 1 + # Written: nothing is owed, so later frames do no work and the unchanged + # frame is not encoded again. + assert dm._snapshot_owed is False + _age_last_write(dm) + dm.write_owed_snapshot() + assert len(saves) == 1 + + +def test_nothing_owed_after_a_frame_that_was_written(viewer, monkeypatch): + dm = viewer + dm.draw.rectangle([0, 0, 8, 8], fill=(0, 255, 0)) + dm.update_display() # due: written at once + assert dm._snapshot_owed is False + calls = [] + monkeypatch.setattr(dm, "_write_snapshot_if_due", + lambda *a, **k: calls.append(a)) + dm.write_owed_snapshot() + assert calls == [] + + +def test_an_unchanged_frame_inside_the_interval_is_not_owed(viewer): + dm = viewer + dm.draw.rectangle([0, 0, 8, 8], fill=(0, 0, 255)) + dm.update_display() + dm.update_display() # same frame, inside the interval + assert dm._snapshot_owed is False + + +def test_a_controller_without_the_hook_still_draws(): + """Controllers built without a display manager (tests) are unaffected.""" + from src import display_controller as dc_module + controller = dc_module.DisplayController.__new__(dc_module.DisplayController) + controller.plugin_manager = None + plugin = _HoldingPlugin() + assert controller._display_once(plugin, "x", True) is True + controller.display_manager = types.SimpleNamespace() + assert controller._display_once(plugin, "x", True) is True + assert plugin.calls == 2