From 7b4b07e3138e10b72b18767e61afe75ef29bdbdb Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 3 Sep 2026 17:45:45 -0400 Subject: [PATCH 1/7] perf(scroll): pace frames to the panel, not to a fixed sleep Scrolling ran at 44-46 fps on a 2x128x64 chain and 14-17% of frames took 41-53ms, which reads as judder. Four independent causes, each measured on the hardware; details and the diagnostic recipe are in docs/SCROLL_PERFORMANCE.md. The high-FPS loop slept a flat 8ms after every render. display() has already blocked on the panel's vsync by then, so that sleep was added to a wait that had happened: ~4ms of render plus 8ms put each iteration at ~12ms against a 10ms refresh grid, so every swap missed a refresh and the loop settled at 50fps while asking for 125 -- with no headroom, so a further 14% of frames slipped again. It now sleeps only the remainder, with a 1ms floor so plugin threads still get the GIL. ScrollHelper stepped position on a wall clock at 1/scroll_delay steps per second. Plugins set scroll_delay to the frame period, so that comparison sat exactly on its own threshold: a frame arriving a hair early moved zero pixels and rendered an identical frame, dirty-tracking skipped the swap, it returned in ~2ms, and the beat repeated. No scroll_delay value tunes that out -- a shorter delay trades stalled frames for periodic double-steps. Both modes now accumulate elapsed time at the same configured speed, so position stays proportional to real time. Sub-pixel blending goes back to off by default. It renders a half-step by mixing two adjacent columns, which on a coarse panel showing pixel-font text alternates crisp and smeared frames and reads as shimmer -- visibly worse than integer stepping on the hardware. Vegas mode still opts in. disk_cache uses orjson when importable, falling back to the stdlib. Encoding a ~1MB record drops from 14.8ms to 5.4ms end-to-end, and that work holds the GIL while a marquee is on screen. display_manager also checksummed the whole framebuffer twice per frame (dirty tracking, then the preview snapshot); the snapshot now takes the checksum the caller already computed. New src/common/scroll_config.py resolves scroll settings in one place. Five ticker plugins each hand-rolled this and disagreed: odds-ticker ranked the deprecated scroll_pixels_per_second above the documented scroll_speed/delay pair, and because that key carries a schema default the documented settings were dead for every user (ChuckBuilds/ledmatrix-plugins#408), while ledmatrix-leaderboard read the same key only as a fallback. The resolver also warns when a speed will not advance a whole number of pixels per refresh, which is the property that actually determines whether a scroll looks smooth. scripts/build_rgbmatrix_nogil.sh rebuilds the rgbmatrix binding so it releases the GIL. Upstream declares SwapOnVSync without nogil, unlike SetPixel/Clear/Fill beside it, so the render thread held the GIL for the whole vsync wait and starved background threads into long uninterruptible bursts. The script patches, builds and self-verifies into a scratch tree; --install backs up the original and rolls back if the service does not come back healthy. Measured after: 100 fps locked, no stalls observed, render thread down from 51% to 19% of one core. Co-Authored-By: Claude Opus 5 --- docs/SCROLL_PERFORMANCE.md | 188 ++++++++++++++++++++++++ requirements.txt | 8 + scripts/build_rgbmatrix_nogil.sh | 242 +++++++++++++++++++++++++++++++ src/cache/disk_cache.py | 60 +++++++- src/common/__init__.py | 12 ++ src/common/scroll_config.py | 224 ++++++++++++++++++++++++++++ src/common/scroll_helper.py | 51 +++++-- src/display_controller.py | 18 ++- src/display_manager.py | 24 ++- test/test_scroll_config.py | 213 +++++++++++++++++++++++++++ 10 files changed, 1011 insertions(+), 29 deletions(-) create mode 100644 docs/SCROLL_PERFORMANCE.md create mode 100644 scripts/build_rgbmatrix_nogil.sh create mode 100644 src/common/scroll_config.py create mode 100644 test/test_scroll_config.py diff --git a/docs/SCROLL_PERFORMANCE.md b/docs/SCROLL_PERFORMANCE.md new file mode 100644 index 00000000..479c325b --- /dev/null +++ b/docs/SCROLL_PERFORMANCE.md @@ -0,0 +1,188 @@ +# Scroll Performance + +How scrolling is paced on this hardware, what was wrong with it, and how to +configure a plugin so its marquee is smooth. + +Measured on a Raspberry Pi 4 driving a 2×128×64 chain (256×64 logical) at +`limit_refresh_rate_hz: 100`. Numbers below come from that panel. + +| | before | after | +|---|---|---| +| scroll frame rate | 44–46 fps | **100 fps, locked** | +| frames ≥ 45 ms | 14–17% | none observed | +| dominant frame time | 20 ms | **10 ms** | +| disk cache write (~1 MB) | 14.8 ms | **5.4 ms** | + +--- + +## The one rule that matters + +**Motion is smooth when the strip advances a whole number of pixels per panel +refresh.** + +On a 100 Hz panel the crisp speeds are 100 px/s, 200 px/s, 300 px/s. A speed +that does not divide evenly has to do one of two bad things: + +- **blend** two adjacent columns to render a half-step — on pixel-font text + this alternates crisp and smeared frames and reads as shimmer, or as the + text jumping a pixel ahead of itself; +- **repeat** a frame — the strip stands still, then jumps, which reads as + judder. + +Neither is tunable away. Pick a speed that divides evenly. + +`src.common.scroll_config.resolve()` warns when a configured speed will not, +and names the nearest speed that will. + +## Configuring a plugin + +Use the shared resolver rather than reading config keys yourself: + +```python +from src.common import scroll_config + +settings = scroll_config.configure( + self.scroll_helper, + plugin_config=self.config, + global_config=self.global_config, + refresh_hz=scroll_config.refresh_hz_from_config(self.global_config), + plugin_logger=self.logger, +) +``` + +It resolves every config shape in one place, applies the speed, and returns +what it did. Precedence, highest first: + +1. `display_options.scroll_speed` + `scroll_delay` — **the recommended form** +2. `display.scroll_speed` + `scroll_delay` — deprecated shape +3. `scroll_speed` + `scroll_delay` at the root — legacy flat +4. `scroll_pixels_per_second` — deprecated +5. the global `display` block +6. the built-in default (100 px/s) + +`scroll_speed` is pixels per frame and `scroll_delay` is the frame period in +seconds, so the pair means `scroll_speed / scroll_delay` px/s. The recommended +config for a 100 Hz panel: + +```json +"display_options": { "scroll_speed": 1.0, "scroll_delay": 0.01 } +``` + +### Why the deprecated key ranks below the explicit pair + +Because some plugins give `scroll_pixels_per_second` a **schema default**, and +schema defaults are merged into plugin config. Ranking it above the pair means +it is always present and always wins, so the documented settings become +unreachable. That is a real, shipped bug — see +[ledmatrix-plugins#408](https://github.com/ChuckBuilds/ledmatrix-plugins/issues/408). + +If you are writing a plugin: do not give a deprecated key a schema default. + +## What was actually wrong + +Four independent faults, each found by measurement. + +### 1. The frame loop slept on top of a wait it had already done + +`display_controller.py` ran the high-FPS loop as `render → SwapOnVSync (blocks +to the panel's refresh) → time.sleep(0.008) → plugin ticks`. The sleep was +unconditional and added to a wait that had already happened. Render work +measured ~4 ms, so each iteration cost ~12 ms against a 10 ms refresh grid — +every swap missed a refresh and landed on the next one. The loop settled at +exactly 50 fps while asking for 125, with no headroom, so ~14% of frames +slipped a further refresh. + +Now the loop sleeps only the remainder of the frame budget, with a 1 ms floor +so plugin threads still get the GIL. + +### 2. `SwapOnVSync` held the GIL while blocking + +The rgbmatrix binding declares it without `nogil` (unlike `SetPixel`, `Clear` +and `Fill` immediately above it in `cppinc.pxd`), so the render thread held the +GIL for the entire vsync wait — most of every frame. Background threads were +starved into long uninterruptible bursts; a 1.5 MB API response costs ~17 ms to +parse and ~18 ms to re-encode for the cache, and `json.raw_decode` cannot be +preempted mid-document. Those bursts are what the render loop then waited on. + +Fixed by rebuilding the binding: `scripts/build_rgbmatrix_nogil.sh`. + +### 3. Sub-pixel blending was wrong for this display + +Enabling it made things worse, not better — see the rule at the top. It is off +by default and only Vegas mode opts in via `set_sub_pixel_scrolling(True)`. + +### 4. Frame-based stepping raced the vsync clock + +Frame-based mode gated motion on a wall clock at `1/scroll_delay` steps per +second. Plugins set `scroll_delay` to the frame period, which puts that +comparison exactly on its own threshold: a frame arriving a hair early moved +zero pixels and rendered an identical frame, which dirty-tracking skipped, so +it returned in ~2 ms and the beat repeated. No `scroll_delay` value tunes this +out — a shorter delay just trades stalled frames for periodic double-steps. + +`ScrollHelper` now accumulates elapsed time in both modes at the same +configured speed, so position stays proportional to real time. + +## Diagnosing a juddery scroller + +**`Avg FPS` will lie to you.** It is a 100-frame moving average, and a 2 ms +duplicate frame plus a 21 ms double-wait average to exactly 10 ms. A ticker +that is stalling on half its frames still reports a healthy `100.0`. + +Look at the **distribution** instead: + +```bash +journalctl -u ledmatrix --since "-10min" --no-pager \ + | grep -oE "Frame time: [0-9.]+ms" | awk '{print $3}' | sed 's/ms//' \ + | awk '{printf "%.0f\n", $1}' | sort -n | uniq -c +``` + +Reading it, on a 100 Hz panel: + +| you see | it means | +|---|---| +| everything at 10 ms | healthy | +| a mode at ~2 ms | **duplicate frames** — the swap was skipped because the image did not change. The scroller is advancing less than one pixel per frame. | +| a mode at 20/30/50 ms | frames missing refreshes — per-frame work is overrunning, or a background thread is holding the GIL | +| `Avg FPS` above 100 | duplicates present, unless the scroll cycle has completed and is idling | + +Then confirm what the plugin actually loaded — config edits do not always reach +the running code: + +```bash +journalctl -u ledmatrix --since "-5min" --no-pager | grep -iE "px/s|px/frame" +``` + +If a plugin logs its scroll config **twice** with different modes, the second +line is what is running. + +## Rebuilding the binding + +```bash +bash scripts/build_rgbmatrix_nogil.sh # build into a scratch dir +sudo bash scripts/build_rgbmatrix_nogil.sh --install +sudo bash scripts/build_rgbmatrix_nogil.sh --rollback +``` + +The build never touches the installed module. `--install` backs up the original +to `~/rgbmatrix-core.so.ORIGINAL` first, and rolls back automatically if the +service does not come back healthy. Requires `build-essential`; Cython is +installed into a cached venv under `~/.cache/ledmatrix-cython`. + +Re-run it after upgrading `rpi-rgb-led-matrix`, since a library upgrade +replaces the patched binding. + +## Faster JSON + +`src/cache/disk_cache.py` uses `orjson` when it is importable and falls back to +the stdlib otherwise, so it is optional: + +```bash +sudo pip3 install --break-system-packages orjson +``` + +Encoding is where it pays — about 7× on this hardware. Decoding gains far less +(~1.3× on large payloads) because the cost there is building Python objects, +not scanning text. That is also why moving parsing to a subprocess does not +help: `pickle.loads` of the same payload costs 8.1 ms against `json.loads` at +10.9 ms, so the work just moves rather than disappearing. diff --git a/requirements.txt b/requirements.txt index 760dc596..2d0350bf 100644 --- a/requirements.txt +++ b/requirements.txt @@ -55,6 +55,14 @@ packaging>=23.0,<27.0 # range as a hard dependency — keep the two in sync. # pip install 'psutil>=6.0.0,<7.0.0' # +# orjson — faster JSON for the disk cache +# (src/cache/disk_cache.py). Encoding a ~1MB cache +# record drops from ~12ms to ~1.6ms on a Pi 4, which +# matters because that work holds the GIL and stalls +# the render thread mid-scroll. Falls back to the +# stdlib json when missing — see docs/SCROLL_PERFORMANCE.md. +# pip install 'orjson>=3.9,<4.0' +# # Flask-Limiter — request rate limiting in web_interface/app.py # (accidental-abuse protection, not security). The # web interface starts without rate limiting when diff --git a/scripts/build_rgbmatrix_nogil.sh b/scripts/build_rgbmatrix_nogil.sh new file mode 100644 index 00000000..959a6c4d --- /dev/null +++ b/scripts/build_rgbmatrix_nogil.sh @@ -0,0 +1,242 @@ +#!/usr/bin/env bash +# +# Rebuild the rgbmatrix Python binding so it releases the GIL. +# +# WHY THIS EXISTS +# --------------- +# The upstream binding declares FrameCanvas::SwapOnVSync WITHOUT `nogil` +# (cppinc.pxd), unlike SetPixel/Clear/Fill on the lines just above it. +# SwapOnVSync blocks until the panel's next vertical sync -- up to a full +# refresh period on every frame -- so the render thread was holding the GIL +# for most of every frame. Background threads (API fetches, JSON parsing, +# image decode) were starved into long uninterruptible bursts, which in turn +# made the render loop miss refreshes. +# +# Measured on a Pi 4 driving a 2x128x64 chain at limit_refresh_rate_hz=100: +# +# before ~44 fps average, 14-17% of frames 41-53ms +# after 100 fps locked, no stalls observed +# +# This script also releases the GIL across the per-pixel blit +# (SetPixelsPillow) and walks the Pillow buffer row-major instead of +# column-major so each row is contiguous. +# +# SAFETY +# ------ +# Builds into a scratch directory; touches the installed module only in the +# --install step, and backs up the original first. Roll back at any time with: +# +# sudo bash scripts/build_rgbmatrix_nogil.sh --rollback +# +# USAGE +# bash scripts/build_rgbmatrix_nogil.sh # build only +# sudo bash scripts/build_rgbmatrix_nogil.sh --install +# sudo bash scripts/build_rgbmatrix_nogil.sh --rollback +# +set -uo pipefail + +SRC_TREE="${RGB_SRC_TREE:-$HOME/LEDMatrix/rpi-rgb-led-matrix-master}" +BUILD_DIR="${RGB_BUILD_DIR:-$HOME/rgbmatrix-nogil-build}" +VENV="${RGB_CYTHON_VENV:-$HOME/.cache/ledmatrix-cython}" +BACKUP="${RGB_BACKUP:-$HOME/rgbmatrix-core.so.ORIGINAL}" + +die() { echo "FATAL: $*" >&2; exit 1; } + +py_site() { + python3 -c 'import rgbmatrix, os; print(os.path.dirname(rgbmatrix.__file__))' 2>/dev/null +} + +abi_so() { + ls "$BUILD_DIR"/bindings/python/rgbmatrix/core.cpython-*.so 2>/dev/null | head -1 +} + +do_rollback() { + local dst; dst="$(py_site)" || die "rgbmatrix not importable" + [ -n "$dst" ] || die "could not locate the installed rgbmatrix package" + [ -f "$BACKUP" ] || die "no backup at $BACKUP" + systemctl stop ledmatrix 2>/dev/null + cp -a "$BACKUP" "$dst/core.so" || die "restore failed" + find "$dst" -name __pycache__ -type d -exec rm -rf {} + 2>/dev/null + systemctl start ledmatrix 2>/dev/null + echo "rolled back to the original core.so" + exit 0 +} + +do_install() { + local so dst + so="$(abi_so)"; [ -n "$so" ] || die "no built module found - run the build first" + dst="$(py_site)"; [ -n "$dst" ] || die "could not locate the installed rgbmatrix package" + + if [ ! -f "$BACKUP" ]; then + cp -a "$dst/core.so" "$BACKUP" || die "could not back up the original" + echo "backed up original core.so -> $BACKUP" + else + echo "backup already present at $BACKUP (keeping the true original)" + fi + + systemctl stop ledmatrix 2>/dev/null + cp "$so" "$dst/core.so" || die "install failed" + find "$dst" -name __pycache__ -type d -exec rm -rf {} + 2>/dev/null + systemctl start ledmatrix 2>/dev/null + + echo "waiting 25s for the display to come back..." + sleep 25 + local healthy=1 + systemctl is-active --quiet ledmatrix || healthy=0 + if journalctl -u ledmatrix --since "40 sec ago" --no-pager \ + | grep -qiE "Traceback|ImportError|Segmentation fault|undefined symbol"; then + healthy=0 + fi + if [ "$healthy" = "1" ]; then + echo "SUCCESS - running on the rebuilt binding" + else + echo "UNHEALTHY - rolling back" + cp -a "$BACKUP" "$dst/core.so" + systemctl restart ledmatrix + journalctl -u ledmatrix --since "90 sec ago" --no-pager | tail -25 + exit 1 + fi + exit 0 +} + +case "${1:-}" in + --rollback) do_rollback ;; + --install) do_install ;; + "" ) ;; + *) die "unknown option: $1" ;; +esac + +# ---------------------------------------------------------------- build ---- +[ -d "$SRC_TREE" ] || die "matrix source tree not found at $SRC_TREE (set RGB_SRC_TREE)" +command -v g++ >/dev/null || die "g++ not installed (apt install build-essential)" + +echo "==> staging a scratch copy at $BUILD_DIR" +rm -rf "$BUILD_DIR" +cp -r "$SRC_TREE" "$BUILD_DIR" || die "copy failed" + +echo "==> patching the bindings to release the GIL" +python3 - "$BUILD_DIR" <<'PYEOF' || die "patch failed" +import io, sys +base = sys.argv[1] + "/bindings/python/rgbmatrix/" + +p = base + "cppinc.pxd" +s = io.open(p, encoding="utf-8").read() +old = " FrameCanvas *SwapOnVSync(FrameCanvas*, uint8_t)\n" +new = " FrameCanvas *SwapOnVSync(FrameCanvas*, uint8_t) nogil\n" +if old in s: + io.open(p, "w", encoding="utf-8", newline="\n").write(s.replace(old, new, 1)) + print(" cppinc.pxd: SwapOnVSync declared nogil") +elif new in s: + print(" cppinc.pxd: already nogil") +else: + sys.exit("could not find the SwapOnVSync declaration") + +p = base + "core.pyx" +s = io.open(p, encoding="utf-8").read() + +old = """ def SwapOnVSync(self, FrameCanvas newFrame, uint8_t framerate_fraction = 1): + return __createFrameCanvas(self.__matrix.SwapOnVSync(newFrame.__canvas, framerate_fraction)) +""" +new = """ def SwapOnVSync(self, FrameCanvas newFrame, uint8_t framerate_fraction = 1): + # Blocks until the panel's next vertical sync. Holding the GIL across + # that wait starves every other Python thread for most of each frame. + cdef cppinc.RGBMatrix* matrix = self.__matrix + cdef cppinc.FrameCanvas* frame = newFrame.__canvas + cdef uint8_t fraction = framerate_fraction + cdef cppinc.FrameCanvas* swapped + with nogil: + swapped = matrix.SwapOnVSync(frame, fraction) + return __createFrameCanvas(swapped) +""" +if old in s: + s = s.replace(old, new, 1) + print(" core.pyx: SwapOnVSync releases the GIL") +elif "with nogil:\n swapped = matrix.SwapOnVSync" in s: + print(" core.pyx: SwapOnVSync already patched") +else: + sys.exit("could not find the SwapOnVSync body") + +old = """ buffer = get_pillow_buffer(image_capsule) + + for col in range(max(0, -xstart), min(width, frame_width - xstart)): + for row in range(max(0, -ystart), min(height, frame_height - ystart)): + pixel = buffer[row][col] + r = (pixel ) & 0xFF + g = (pixel >> 8) & 0xFF + b = (pixel >> 16) & 0xFF + my_canvas.SetPixel(xstart+col, ystart+row, r, g, b) +""" +new = """ buffer = get_pillow_buffer(image_capsule) + + # Bounds hoisted so the blit needs no Python state and can run without + # the GIL: it touches only a C buffer and a C++ canvas. Row-major order + # walks each row contiguously; col-outer re-strided the whole buffer. + cdef int col_start = max(0, -xstart) + cdef int col_end = min(width, frame_width - xstart) + cdef int row_start = max(0, -ystart) + cdef int row_end = min(height, frame_height - ystart) + + with nogil: + for row in range(row_start, row_end): + for col in range(col_start, col_end): + pixel = buffer[row][col] + r = (pixel ) & 0xFF + g = (pixel >> 8) & 0xFF + b = (pixel >> 16) & 0xFF + my_canvas.SetPixel(xstart+col, ystart+row, r, g, b) +""" +if old in s: + s = s.replace(old, new, 1) + print(" core.pyx: pixel blit releases the GIL, row-major") +elif "with nogil:\n for row in range(row_start, row_end):" in s: + print(" core.pyx: blit already patched") +else: + sys.exit("could not find the SetPixelsPillow loop") + +io.open(p, "w", encoding="utf-8", newline="\n").write(s) +PYEOF + +echo "==> building librgbmatrix.a (this takes a few minutes)" +nice -n 10 make -C "$BUILD_DIR/lib" -j2 >/dev/null 2>&1 \ + || die "library build failed - rerun 'make -C $BUILD_DIR/lib' to see why" +[ -f "$BUILD_DIR/lib/librgbmatrix.a" ] || die "librgbmatrix.a was not produced" + +echo "==> preparing Cython" +[ -d "$VENV" ] || python3 -m venv --system-site-packages "$VENV" || die "venv failed" +"$VENV/bin/pip" install --quiet cython || die "cython install failed" + +cat > "$BUILD_DIR/bindings/python/setup.py" <<'EOF' +from setuptools import setup, Extension +from Cython.Build import cythonize + +core = Extension( + "rgbmatrix.core", + sources=["rgbmatrix/core.pyx", "rgbmatrix/shims/pillow.c"], + include_dirs=["../../include", "rgbmatrix/shims"], + extra_objects=["../../lib/librgbmatrix.a"], + language="c++", + extra_compile_args=["-O3", "-Wall", "-fno-exceptions", "-std=c++11"], + extra_link_args=["-lrt", "-lm", "-lpthread"], +) + +setup(name="rgbmatrix", + ext_modules=cythonize([core], language_level="3str", + compiler_directives={"binding": False})) +EOF + +echo "==> compiling the extension" +( cd "$BUILD_DIR/bindings/python" && "$VENV/bin/python" setup.py build_ext --inplace ) \ + >/dev/null 2>&1 || die "extension build failed" + +SO="$(abi_so)"; [ -n "$SO" ] || die "no .so produced" + +# Verify the GIL really is released before anyone installs this. +PAIRS=$(grep -c "PyEval_SaveThread\|Py_UNBLOCK_THREADS" "$BUILD_DIR/bindings/python/rgbmatrix/core.cpp") +[ "$PAIRS" -ge 2 ] || die "generated C++ has only $PAIRS GIL releases, expected >= 2" + +echo +echo "BUILT: $SO" +echo " ($PAIRS GIL-release sites in the generated C++)" +echo +echo "Install with: sudo bash $0 --install" +echo "Roll back with: sudo bash $0 --rollback" diff --git a/src/cache/disk_cache.py b/src/cache/disk_cache.py index 03cfb2f1..30ffd1ad 100644 --- a/src/cache/disk_cache.py +++ b/src/cache/disk_cache.py @@ -14,6 +14,11 @@ from typing import Dict, Any, Optional, Protocol from datetime import datetime +try: # optional: large speedup on the cache write path, see _dumps below + import orjson +except ImportError: # pragma: no cover - exercised on hosts without the wheel + orjson = None + # How old an abandoned write's temp file must be before the sweep removes it. # A real write holds its temp file for milliseconds, so an hour is far beyond # any in-flight write while still clearing the same day's debris. Deliberately @@ -40,13 +45,52 @@ def get_data_type_from_key(self, key: str) -> str: class DateTimeEncoder(json.JSONEncoder): - """JSON encoder that handles datetime objects.""" + """JSON encoder that handles datetime objects. + + Retained for the stdlib fallback path and for any caller importing it. + """ def default(self, obj: Any) -> Any: if isinstance(obj, datetime): return obj.isoformat() return super().default(obj) +def _datetime_default(obj: Any) -> Any: + """Serialise datetimes exactly as DateTimeEncoder did.""" + if isinstance(obj, datetime): + return obj.isoformat() + raise TypeError(f"Object of type {type(obj).__name__} is not JSON serializable") + + +if orjson is not None: + # Encoding the cache record dominated the background fetch worker: on a + # Pi 4, stdlib json.dumps runs ~12ms per MB and holds the GIL for all of + # it, which stalls the render thread mid-scroll. orjson measures ~7x + # faster on the same payloads (11.9ms -> 1.6ms for 985KB). Decoding gains + # far less (~1.3x on large payloads) because the cost there is building + # the Python objects, not scanning the text, but it is still free to take. + # + # OPT_NON_STR_KEYS: stdlib json coerces int/float dict keys to strings; + # orjson raises without this, and cache records do carry numeric keys. + # OPT_PASSTHROUGH_DATETIME: orjson would otherwise emit its own RFC 3339 + # form for datetimes instead of calling default(). Routing them through + # _datetime_default keeps byte-for-byte parity with the records already + # on disk. + _DUMPS_OPTS = orjson.OPT_NON_STR_KEYS | orjson.OPT_PASSTHROUGH_DATETIME + + def _dumps(data: Any) -> bytes: + return orjson.dumps(data, default=_datetime_default, option=_DUMPS_OPTS) + + def _loads(raw: bytes) -> Any: + return orjson.loads(raw) +else: + def _dumps(data: Any) -> bytes: + return json.dumps(data, cls=DateTimeEncoder).encode("utf-8") + + def _loads(raw: bytes) -> Any: + return json.loads(raw) + + class DiskCache: """Manages persistent disk-based cache.""" @@ -99,8 +143,8 @@ def get(self, key: str, max_age: Optional[int] = 300) -> Optional[Dict[str, Any] try: with self._lock: - with open(cache_path, 'r', encoding='utf-8') as f: - record = json.load(f) + with open(cache_path, 'rb') as f: + record = _loads(f.read()) # Determine record timestamp (prefer embedded, else file mtime) record_ts = None @@ -189,12 +233,12 @@ def set(self, key: str, data: Dict[str, Any]) -> None: # write path below, and cache files are machine-read only — indenting # them just multiplied the bytes written to the SD card. try: - payload = json.dumps(data, cls=DateTimeEncoder) + payload = _dumps(data) except (TypeError, ValueError) as e: self.logger.warning("Cache data for key '%s' not serializable: %s", key, e) return - digest = zlib.adler32(payload.encode('utf-8')) + digest = zlib.adler32(payload) try: # Atomic write to avoid partial/corrupt files @@ -242,7 +286,7 @@ def set(self, key: str, data: Dict[str, Any]) -> None: # wear source (dozens of fsyncs/min on API-heavy # installs) for data that can be re-downloaded. try: - with os.fdopen(fd, 'w', encoding='utf-8') as tmp_file: + with os.fdopen(fd, 'wb') as tmp_file: tmp_file.write(payload) os.replace(tmp_path, cache_path) self._write_digests[key] = digest @@ -260,7 +304,7 @@ def set(self, key: str, data: Dict[str, Any]) -> None: else: # Fallback: direct write (not atomic, but better than failing) try: - with open(cache_path, 'w', encoding='utf-8') as cache_file: + with open(cache_path, 'wb') as cache_file: cache_file.write(payload) self._write_digests[key] = digest # Set proper permissions: 660 (rw-rw----) for group-readable cache files @@ -290,7 +334,7 @@ def set(self, key: str, data: Dict[str, Any]) -> None: # is a different path, so future sets must keep # retrying the primary location. fallback_path = os.path.join(fallback_dir, os.path.basename(cache_path)) - with open(fallback_path, 'w', encoding='utf-8') as tmp_file: + with open(fallback_path, 'wb') as tmp_file: tmp_file.write(payload) # Set proper permissions: 660 (rw-rw----) for group-readable cache files try: diff --git a/src/common/__init__.py b/src/common/__init__.py index 4b6cb383..03588175 100644 --- a/src/common/__init__.py +++ b/src/common/__init__.py @@ -23,6 +23,13 @@ ) from src.common.api_helper import APIHelper from src.common.scroll_helper import ScrollHelper +from src.common import scroll_config +from src.common.scroll_config import ( + ScrollSettings, + configure as configure_scroll, + resolve as resolve_scroll_settings, + refresh_hz_from_config, +) from src.common.logo_helper import LogoHelper from src.common.text_helper import TextHelper @@ -60,6 +67,11 @@ 'log_and_raise', 'APIHelper', 'ScrollHelper', + 'scroll_config', + 'ScrollSettings', + 'configure_scroll', + 'resolve_scroll_settings', + 'refresh_hz_from_config', 'LogoHelper', 'TextHelper', # adaptive layout & images diff --git a/src/common/scroll_config.py b/src/common/scroll_config.py new file mode 100644 index 00000000..9bf38a0d --- /dev/null +++ b/src/common/scroll_config.py @@ -0,0 +1,224 @@ +"""One place that turns plugin config into a configured ScrollHelper. + +Five ticker plugins each hand-rolled this resolution (odds-ticker, news and +ledmatrix-leaderboard reference the deprecated ``scroll_pixels_per_second`` +key 16-18 times apiece), and they disagreed in ways that were invisible until +someone watched the panel: + +* odds-ticker read ``scroll_pixels_per_second`` on the *recommended* config + path and let it override ``scroll_speed``/``scroll_delay``. Because that key + carries a schema default, the documented settings were dead for every user + -- see ChuckBuilds/ledmatrix-plugins#408. +* ledmatrix-leaderboard read the same key only as a fallback, so identical + config produced different speeds in the two plugins. +* stock-news derived px/frame from it via its own arithmetic. + +What matters on the hardware +---------------------------- +Motion is smooth when the strip advances a **whole number of pixels per panel +refresh**. On a 100Hz panel that means 100 px/s, 200 px/s, and so on. Anything +else has to either blend adjacent columns (which on pixel-font text reads as +shimmer) or repeat frames (which reads as judder). :func:`resolve` warns when +the requested speed will not divide evenly, because that is a real display +artefact and not a rounding detail. + +Speed is always expressed to the helper as pixels per second and applied in +time-based mode. Frame-based stepping gates motion on a wall clock at +``1/scroll_delay`` steps per second; plugins set ``scroll_delay`` to the frame +period, which puts that comparison exactly on its own threshold and makes the +step count flip on sub-millisecond jitter. Accumulating elapsed time keeps +position proportional to real time instead. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from typing import Any, Dict, Optional + +logger = logging.getLogger(__name__) + +#: Speed used when a plugin supplies nothing usable. One pixel per refresh on a +#: 100Hz panel, which is the slowest crisp scroll that hardware can show. +DEFAULT_PIXELS_PER_SECOND = 100.0 + +#: Bounds accepted from config. Below the floor a marquee appears frozen; +#: above the ceiling it outruns any panel's refresh and tears. +MIN_PIXELS_PER_SECOND = 1.0 +MAX_PIXELS_PER_SECOND = 500.0 + +#: Assumed refresh when the caller does not say. Matches the usual +#: ``display.hardware.limit_refresh_rate_hz``. +DEFAULT_REFRESH_HZ = 100.0 + +#: How far px/s may sit from a whole number of pixels per refresh before it is +#: worth warning about. 0.05px per frame is invisible; a third of a pixel is not. +_WHOLE_PIXEL_TOLERANCE = 0.05 + + +@dataclass(frozen=True) +class ScrollSettings: + """The resolved outcome, and which config key produced it.""" + + pixels_per_second: float + source: str + target_fps: Optional[float] = None + pixels_per_frame: Optional[float] = None + warning: Optional[str] = None + + def describe(self) -> str: + text = f"{self.pixels_per_second:.1f} px/s (from {self.source})" + if self.pixels_per_frame is not None: + text += f" = {self.pixels_per_frame:.2f} px/frame" + if self.target_fps: + text += f" at {self.target_fps:.0f} fps" + return text + + +def _coerce(value: Any) -> Optional[float]: + """A positive float, or None. Config reaches us with nulls and strings.""" + if value is None or isinstance(value, bool): + return None + try: + number = float(value) + except (TypeError, ValueError): + return None + return number if number > 0 else None + + +def _from_speed_and_delay(block: Any) -> Optional[float]: + """px/s from a ``scroll_speed`` (px/frame) + ``scroll_delay`` (s) pair.""" + if not isinstance(block, dict): + return None + speed = _coerce(block.get("scroll_speed")) + delay = _coerce(block.get("scroll_delay")) + if speed is None or delay is None: + return None + return speed / delay + + +def resolve( + plugin_config: Optional[Dict[str, Any]] = None, + global_config: Optional[Dict[str, Any]] = None, + default_pixels_per_second: float = DEFAULT_PIXELS_PER_SECOND, + refresh_hz: Optional[float] = None, +) -> ScrollSettings: + """Resolve one scroll speed from the several shapes plugins accept. + + Precedence, highest first. The deprecated flat key sits *below* the + explicit pairs deliberately: it carries schema defaults in some plugins, so + ranking it above them silently disables the documented settings. + + 1. ``display_options.scroll_speed`` + ``scroll_delay`` (current) + 2. ``display.scroll_speed`` + ``scroll_delay`` (deprecated shape) + 3. ``scroll_speed`` + ``scroll_delay`` at the root (legacy flat) + 4. ``scroll_pixels_per_second``, nested or flat (deprecated) + 5. the global ``display`` block + 6. ``default_pixels_per_second`` + + :param refresh_hz: panel refresh, used only to check whether the resolved + speed lands on whole pixels per frame and to fill in ``target_fps``. + """ + plugin_config = plugin_config or {} + global_config = global_config or {} + refresh = _coerce(refresh_hz) or DEFAULT_REFRESH_HZ + + display_options = plugin_config.get("display_options") + display_block = plugin_config.get("display") + + candidates = [ + (_from_speed_and_delay(display_options), "display_options.scroll_speed/delay"), + (_from_speed_and_delay(display_block), "display.scroll_speed/delay"), + (_from_speed_and_delay(plugin_config), "scroll_speed/delay (root)"), + ] + for block, label in ( + (display_options, "display_options.scroll_pixels_per_second"), + (display_block, "display.scroll_pixels_per_second"), + (plugin_config, "scroll_pixels_per_second"), + ): + if isinstance(block, dict): + candidates.append((_coerce(block.get("scroll_pixels_per_second")), label)) + + global_display = global_config.get("display") + candidates.append((_from_speed_and_delay(global_display), "global display.scroll_speed/delay")) + + pixels_per_second = None + source = "default" + for value, label in candidates: + if value is not None: + pixels_per_second, source = value, label + break + if pixels_per_second is None: + pixels_per_second = default_pixels_per_second + + clamped = max(MIN_PIXELS_PER_SECOND, min(MAX_PIXELS_PER_SECOND, pixels_per_second)) + warning = None + if clamped != pixels_per_second: + warning = ( + f"scroll speed {pixels_per_second:.1f} px/s out of range, " + f"clamped to {clamped:.1f}" + ) + pixels_per_second = clamped + + pixels_per_frame = pixels_per_second / refresh if refresh > 0 else None + if warning is None and pixels_per_frame is not None: + offset = abs(pixels_per_frame - round(pixels_per_frame)) + if pixels_per_frame < 1.0 - _WHOLE_PIXEL_TOLERANCE or offset > _WHOLE_PIXEL_TOLERANCE: + suggestion = max(1.0, round(pixels_per_frame)) * refresh + warning = ( + f"{pixels_per_second:.1f} px/s is {pixels_per_frame:.2f} px per " + f"refresh at {refresh:.0f}Hz, so some frames repeat and the " + f"scroll will judder; {suggestion:.0f} px/s divides evenly" + ) + + return ScrollSettings( + pixels_per_second=pixels_per_second, + source=source, + target_fps=refresh, + pixels_per_frame=pixels_per_frame, + warning=warning, + ) + + +def configure( + scroll_helper: Any, + plugin_config: Optional[Dict[str, Any]] = None, + global_config: Optional[Dict[str, Any]] = None, + default_pixels_per_second: float = DEFAULT_PIXELS_PER_SECOND, + refresh_hz: Optional[float] = None, + plugin_logger: Optional[logging.Logger] = None, +) -> ScrollSettings: + """Resolve the config and apply it to ``scroll_helper``. + + Applied in time-based mode: see the module docstring for why frame-based + stepping is not used. ``hasattr`` guards keep this usable against older + ScrollHelper builds that a plugin may be running on. + + :returns: the settings applied, so the caller can log or assert on them. + """ + log = plugin_logger or logger + settings = resolve( + plugin_config, + global_config, + default_pixels_per_second=default_pixels_per_second, + refresh_hz=refresh_hz, + ) + + if hasattr(scroll_helper, "set_frame_based_scrolling"): + scroll_helper.set_frame_based_scrolling(False) + scroll_helper.set_scroll_speed(settings.pixels_per_second) + if settings.target_fps and hasattr(scroll_helper, "set_target_fps"): + scroll_helper.set_target_fps(settings.target_fps) + + log.info("Scroll configured: %s", settings.describe()) + if settings.warning: + log.warning("Scroll speed: %s", settings.warning) + return settings + + +def refresh_hz_from_config(global_config: Optional[Dict[str, Any]]) -> float: + """The panel's refresh cap from the global config, or the default.""" + if not isinstance(global_config, dict): + return DEFAULT_REFRESH_HZ + hardware = (global_config.get("display") or {}).get("hardware") or {} + return _coerce(hardware.get("limit_refresh_rate_hz")) or DEFAULT_REFRESH_HZ diff --git a/src/common/scroll_helper.py b/src/common/scroll_helper.py index 6e2f4218..5b34cfe0 100644 --- a/src/common/scroll_helper.py +++ b/src/common/scroll_helper.py @@ -75,8 +75,19 @@ def __init__(self, display_width: int, display_height: int, # Pre-allocated buffer for output frame (reused to avoid allocations) self._frame_buffer: Optional[np.ndarray] = None - # Sub-pixel scrolling settings (disabled - using high FPS integer scrolling instead) - self.sub_pixel_scrolling = False # Disabled - use high frame rate for smoothness + # Sub-pixel scrolling: OFF by default, and that is deliberate. + # Blending renders a half-step by mixing two adjacent columns 50/50. + # On a high-resolution screen that reads as smooth motion; on a coarse + # LED matrix showing pixel-font text it does not. A one-pixel stroke + # becomes two half-brightness pixels, so frames alternate between crisp + # and smeared and the text appears to shimmer and jump a pixel ahead -- + # tested on a 2x128x64 panel and clearly worse than integer stepping. + # + # The rule this display obeys: motion is smooth when it advances a + # whole number of pixels per refresh. Anything slower must either + # blend (blur) or repeat frames (judder); blending is the worse of the + # two here. Vegas mode still opts in via set_sub_pixel_scrolling(). + self.sub_pixel_scrolling = False self._last_integer_position = 0 # Cache for integer position to avoid repeated calculations # Frame-based scrolling settings @@ -244,19 +255,31 @@ def update_scroll_position(self) -> None: if self.last_step_time == 0.0: self.last_step_time = current_time - # Check if scroll_delay has passed - time_since_last_step = current_time - self.last_step_time - if time_since_last_step >= self.scroll_delay: - # Move pixels (can move multiple steps if lag occurred, but cap to prevent huge jumps) - steps = int(time_since_last_step / self.scroll_delay) - # Cap at reasonable number to prevent huge jumps from lag - max_steps = max(1, int(0.04 / self.scroll_delay)) # Limit to 0.04s (2 steps at 50 FPS) for smoother scrolling - steps = min(steps, max_steps) - pixels_to_move = self.scroll_speed * steps - # Update last_step_time, preserving fractional delay for smooth timing - self.last_step_time = current_time - (time_since_last_step % self.scroll_delay) + # Frame-based mode advances by elapsed time, exactly like the + # time-based branch below, at the same configured speed + # (scroll_speed px per scroll_delay seconds). + # + # It used to step discretely: 0, 1 or 2 whole pixels depending on + # whether a wall clock had passed scroll_delay. Plugins set + # scroll_delay to the target frame period, so that comparison sits + # exactly on its own threshold and the decision flips on sub- + # millisecond jitter -- a frame a hair early moved nothing and + # rendered an identical frame, a frame a hair late moved two + # pixels. Rounding the step count fixed the stalls but still + # discarded the remainder, so the error never corrected. + # + # Accumulating elapsed time keeps position exactly proportional to + # real time: jitter shifts a pixel boundary by a fraction of a + # frame instead of flipping a whole step, and nothing is lost or + # gained. This is what the one visibly smooth scroller on the + # hardware (the stock ticker) was already doing by virtue of never + # enabling frame-based mode. + if self.scroll_delay > 0: + pixels_per_second = self.scroll_speed / self.scroll_delay else: - pixels_to_move = 0.0 + pixels_per_second = self.scroll_speed * 100.0 + pixels_to_move = pixels_per_second * delta_time + self.last_step_time = current_time else: # Time-based: move based on time delta (correct speed over time) # scroll_speed is pixels per second diff --git a/src/display_controller.py b/src/display_controller.py index bb650698..d68e2e97 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -2405,6 +2405,7 @@ def _should_exit_dynamic(elapsed_time: float) -> bool: ) while True: + _frame_start = time.perf_counter() try: with self._display_lock_or_skip(plugin_id) as can_display: if can_display: @@ -2425,11 +2426,26 @@ def _should_exit_dynamic(elapsed_time: float) -> bool: # Multi-display sync: send follower frame after each render self._send_follower_frame(manager_to_display) - time.sleep(display_interval) self._tick_plugin_updates() self._poll_on_demand_requests() self._check_on_demand_expiration() + # Pace to the frame deadline rather than sleeping a flat + # interval on top of the work. display() has already + # blocked on the panel's vsync by this point, so an + # unconditional sleep is added to a wait that already + # happened. Measured on a 2x128x64 chain at + # limit_refresh_rate_hz=100: ~4ms of render plus a flat + # 8ms put each iteration at ~12ms against a 10ms refresh + # grid, so every swap missed a refresh and the loop + # settled at 50fps where display_interval asks for 125 -- + # and with zero headroom, ~14% of frames slipped a + # further refresh, which is what reads as scroll stutter. + _remaining = display_interval - (time.perf_counter() - _frame_start) + # Yield even when the frame overran its budget, so plugin + # update threads and the web UI are not starved of the GIL. + time.sleep(_remaining if _remaining > 0 else 0.001) + if self.current_display_mode != active_mode: logger.debug("Mode changed during high-FPS loop, breaking early") break diff --git a/src/display_manager.py b/src/display_manager.py index 9cc7f622..af23f880 100644 --- a/src/display_manager.py +++ b/src/display_manager.py @@ -768,16 +768,18 @@ def update_display(self): return # Skip hardware write — content is being captured off-screen digest = None + frame_checksum = None if self._dirty_tracking_enabled: try: brightness = getattr(self.matrix, 'brightness', None) except AttributeError: brightness = None - digest = (zlib.adler32(self.image.tobytes()), brightness) + frame_checksum = zlib.adler32(self.image.tobytes()) + digest = (frame_checksum, 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() + self._write_snapshot_if_due(frame_checksum) return # Copy the current image to the offscreen canvas. In double-sided @@ -796,7 +798,7 @@ def update_display(self): self._last_pushed_digest = digest # Write a snapshot for the web preview (throttled) - self._write_snapshot_if_due() + self._write_snapshot_if_due(frame_checksum) except Exception as e: logger.error(f"Error updating display: {e}") @@ -1410,11 +1412,20 @@ def _viewer_is_fresh(self, now: float) -> bool: self._viewer_fresh = False return self._viewer_fresh - def _write_snapshot_if_due(self) -> None: + def _write_snapshot_if_due(self, frame_checksum: Optional[int] = None) -> None: """Mirror the current frame to the preview snapshot when the policy says it's worth it — see src/common/snapshot_policy.py. Unchanged frames are never re-encoded; without viewers the cadence drops to - the idle keepalive.""" + the idle keepalive. + + Args: + frame_checksum: adler32 of the current frame, when the caller has + already computed one. Dirty tracking checksums every frame a + few lines above the call site, and re-deriving it here meant a + second tobytes() plus a second pass over the whole framebuffer + on every single frame — ~0.17ms per frame of the two combined + at 256x64, paid 100 times a second to reach the same number. + """ try: now = time.time() viewer_fresh = self._viewer_is_fresh(now) @@ -1424,7 +1435,8 @@ def _write_snapshot_if_due(self) -> None: self._last_snapshot_ts = 0.0 self._viewer_was_fresh = viewer_fresh - digest = zlib.adler32(self.image.tobytes()) + digest = (frame_checksum if frame_checksum is not None + else zlib.adler32(self.image.tobytes())) action = snapshot_policy.decide( now, self._last_snapshot_ts, self._last_snapshot_touch_ts, viewer_fresh, digest != self._last_snapshot_digest) diff --git a/test/test_scroll_config.py b/test/test_scroll_config.py new file mode 100644 index 00000000..f3f1dcc2 --- /dev/null +++ b/test/test_scroll_config.py @@ -0,0 +1,213 @@ +"""Tests for the shared scroll configuration resolver.""" + +import logging +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from src.common.scroll_config import ( # noqa: E402 + DEFAULT_PIXELS_PER_SECOND, + MAX_PIXELS_PER_SECOND, + MIN_PIXELS_PER_SECOND, + ScrollSettings, + configure, + refresh_hz_from_config, + resolve, +) + + +class FakeHelper: + """Records what configure() applied.""" + + def __init__(self, with_optional=True): + self.speed = None + self.frame_based = None + self.target_fps = None + if not with_optional: + del FakeHelper.set_frame_based_scrolling + del FakeHelper.set_target_fps + + def set_scroll_speed(self, speed): + self.speed = speed + + def set_frame_based_scrolling(self, enabled): + self.frame_based = enabled + + def set_target_fps(self, fps): + self.target_fps = fps + + +class MinimalHelper: + """An older helper exposing only set_scroll_speed.""" + + def __init__(self): + self.speed = None + + def set_scroll_speed(self, speed): + self.speed = speed + + +class TestPrecedence: + def test_display_options_pair_wins(self): + s = resolve({"display_options": {"scroll_speed": 1.0, "scroll_delay": 0.01}}) + assert s.pixels_per_second == 100.0 + assert s.source == "display_options.scroll_speed/delay" + + def test_display_block_used_when_options_absent(self): + s = resolve({"display": {"scroll_speed": 2.0, "scroll_delay": 0.01}}) + assert s.pixels_per_second == 200.0 + assert s.source == "display.scroll_speed/delay" + + def test_root_pair_used_when_both_blocks_absent(self): + s = resolve({"scroll_speed": 1.0, "scroll_delay": 0.02}) + assert s.pixels_per_second == 50.0 + + def test_pixels_per_second_used_when_no_pair_given(self): + s = resolve({"display_options": {"scroll_pixels_per_second": 120.0}}) + assert s.pixels_per_second == 120.0 + assert s.source == "display_options.scroll_pixels_per_second" + + def test_global_display_is_the_last_resort_before_default(self): + s = resolve({}, {"display": {"scroll_speed": 1.0, "scroll_delay": 0.005}}) + assert s.pixels_per_second == 200.0 + + def test_default_when_nothing_configured(self): + s = resolve({}, {}) + assert s.pixels_per_second == DEFAULT_PIXELS_PER_SECOND + assert s.source == "default" + + +class TestDeprecatedKeyCannotOverrideExplicitPair: + """Regression for ChuckBuilds/ledmatrix-plugins#408. + + odds-ticker ranked scroll_pixels_per_second above the documented + scroll_speed/scroll_delay pair. Because that key carries a schema default, + the documented settings became unreachable for every user and the ticker + silently ran at the default speed. The pair must win. + """ + + def test_pair_beats_pixels_per_second_in_the_same_block(self): + s = resolve( + { + "display_options": { + "scroll_speed": 1.0, + "scroll_delay": 0.01, + "scroll_pixels_per_second": 50.0, # schema default + } + } + ) + assert s.pixels_per_second == 100.0 + assert "scroll_speed/delay" in s.source + + def test_pair_beats_pixels_per_second_in_an_outer_block(self): + s = resolve( + { + "display_options": {"scroll_speed": 1.0, "scroll_delay": 0.01}, + "scroll_pixels_per_second": 50.0, + } + ) + assert s.pixels_per_second == 100.0 + + +class TestMalformedValues: + @pytest.mark.parametrize("bad", [None, "fast", "", {}, [], float("nan")]) + def test_unusable_speed_falls_through(self, bad): + s = resolve({"display_options": {"scroll_speed": bad, "scroll_delay": 0.01}}) + assert s.pixels_per_second == DEFAULT_PIXELS_PER_SECOND + + @pytest.mark.parametrize("bad", [0, -5, 0.0]) + def test_non_positive_values_fall_through(self, bad): + s = resolve({"display_options": {"scroll_pixels_per_second": bad}}) + assert s.pixels_per_second == DEFAULT_PIXELS_PER_SECOND + + def test_booleans_are_not_treated_as_numbers(self): + s = resolve({"display_options": {"scroll_pixels_per_second": True}}) + assert s.pixels_per_second == DEFAULT_PIXELS_PER_SECOND + + def test_zero_delay_does_not_divide_by_zero(self): + s = resolve({"display_options": {"scroll_speed": 1.0, "scroll_delay": 0}}) + assert s.pixels_per_second == DEFAULT_PIXELS_PER_SECOND + + def test_non_dict_blocks_are_ignored(self): + s = resolve({"display_options": "nonsense", "display": 5}) + assert s.pixels_per_second == DEFAULT_PIXELS_PER_SECOND + + +class TestClamping: + def test_absurdly_fast_is_clamped(self): + s = resolve({"display_options": {"scroll_pixels_per_second": 100000.0}}) + assert s.pixels_per_second == MAX_PIXELS_PER_SECOND + assert s.warning and "clamped" in s.warning + + def test_absurdly_slow_is_clamped(self): + s = resolve({"display_options": {"scroll_pixels_per_second": 0.01}}) + assert s.pixels_per_second == MIN_PIXELS_PER_SECOND + + +class TestWholePixelWarning: + """The display rule: whole pixels per refresh, or it judders.""" + + def test_no_warning_when_speed_divides_evenly(self): + s = resolve({"display_options": {"scroll_pixels_per_second": 100.0}}, refresh_hz=100) + assert s.warning is None + assert s.pixels_per_frame == pytest.approx(1.0) + + def test_no_warning_at_an_integer_multiple(self): + s = resolve({"display_options": {"scroll_pixels_per_second": 200.0}}, refresh_hz=100) + assert s.warning is None + + def test_warns_on_a_half_pixel_per_frame(self): + s = resolve({"display_options": {"scroll_pixels_per_second": 50.0}}, refresh_hz=100) + assert s.warning is not None + assert "judder" in s.warning + assert "100 px/s" in s.warning + + def test_warns_on_a_fractional_multiple(self): + s = resolve({"display_options": {"scroll_pixels_per_second": 130.0}}, refresh_hz=100) + assert s.warning is not None + + def test_respects_a_non_default_refresh_rate(self): + s = resolve({"display_options": {"scroll_pixels_per_second": 150.0}}, refresh_hz=150) + assert s.warning is None + assert s.pixels_per_frame == pytest.approx(1.0) + + +class TestConfigure: + def test_applies_time_based_mode_and_speed(self): + helper = FakeHelper() + s = configure(helper, {"display_options": {"scroll_speed": 1.0, "scroll_delay": 0.01}}) + assert helper.speed == 100.0 + assert helper.frame_based is False, "must not use the wall-clock step gate" + assert helper.target_fps == 100.0 + assert s.pixels_per_second == 100.0 + + def test_works_against_a_helper_without_optional_methods(self): + helper = MinimalHelper() + configure(helper, {"display_options": {"scroll_pixels_per_second": 100.0}}) + assert helper.speed == 100.0 + + def test_logs_the_warning_when_speed_will_judder(self, caplog): + helper = FakeHelper() + with caplog.at_level(logging.WARNING): + configure(helper, {"display_options": {"scroll_pixels_per_second": 50.0}}) + assert any("judder" in r.getMessage() for r in caplog.records) + + def test_returns_settings_describing_the_source(self): + s = configure(FakeHelper(), {"display_options": {"scroll_speed": 2.0, "scroll_delay": 0.01}}) + assert isinstance(s, ScrollSettings) + assert "200.0 px/s" in s.describe() + + +class TestRefreshFromConfig: + def test_reads_the_hardware_limit(self): + assert refresh_hz_from_config( + {"display": {"hardware": {"limit_refresh_rate_hz": 150}}} + ) == 150.0 + + @pytest.mark.parametrize("cfg", [None, {}, {"display": {}}, {"display": {"hardware": {}}}, + "nonsense", {"display": {"hardware": {"limit_refresh_rate_hz": None}}}]) + def test_falls_back_to_the_default(self, cfg): + assert refresh_hz_from_config(cfg) == 100.0 From 85ab02bb7e3bc13996e3d8f5011d764fa6f6cb24 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 3 Sep 2026 19:55:51 -0400 Subject: [PATCH 2/7] fix(display): keep the panel swap locked to vsync while scrolling Dirty tracking skipped SwapOnVSync for byte-identical frames. That is the right call for static content, but SwapOnVSync is also what paces the render loop, so skipping it skips the wait for the panel: a duplicate frame returns in ~8ms instead of ~10ms on a 100Hz panel, advances the strip only 0.8px instead of 1.0px, and so makes the next frame more likely to repeat as well. The effect sustains itself once it starts. Measured over 20 minutes on a 2x128x64 chain, both scrollers configured identically at 100 px/s: leaderboard 10ms x35, 11ms x3 (clean) odds-ticker 10ms x26, 8ms x7, 15ms x5 (~20% duplicates mid-scroll) The duplicates were not end-of-cycle idling -- 38% of fast frames fell within 90s of a scroll completion against 35% of normal frames, a null result. The trigger is per-frame work: odds does more of it, and more variably, so it is first to land a frame that advances less than a whole pixel. Pushing an identical frame costs one canvas copy. Falling out of vsync lock costs smooth motion. Static content is untouched, because is_currently_scrolling() expires on its own inactivity threshold -- covered by test_stale_scrolling_state_stops_forcing_pushes so a plugin that stops scrolling without saying so cannot pin the panel into always-push. Also de-flakes test_snapshot_still_written_on_skip, which asserted a strict mtime increase between two writes that can land in the same filesystem tick; it failed about two runs in three on Windows regardless of the code under test. The file is now backdated before the check. 156 tests pass on the Pi. Not yet confirmed by eye on the panel. Co-Authored-By: Claude Opus 5 --- src/display_manager.py | 17 +++++++- test/test_display_dirty_tracking.py | 61 +++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/src/display_manager.py b/src/display_manager.py index af23f880..e77fcad3 100644 --- a/src/display_manager.py +++ b/src/display_manager.py @@ -776,9 +776,24 @@ def update_display(self): brightness = None frame_checksum = zlib.adler32(self.image.tobytes()) digest = (frame_checksum, brightness) - if digest == self._last_pushed_digest: + if digest == self._last_pushed_digest and not self.is_currently_scrolling(): # Nothing changed since the last push — the panel is # already showing exactly this frame. + # + # Never taken mid-scroll, and that exception is the + # point. SwapOnVSync is what paces the render loop, so + # skipping it also skips the wait: a duplicate frame + # returns in ~8ms instead of ~10ms on a 100Hz panel, + # advances only 0.8px instead of 1.0px, and so makes + # the *next* frame more likely to repeat as well. That + # is self-sustaining -- measured at ~20% duplicate + # frames mid-scroll on the odds ticker, against + # essentially zero on a lighter plugin with identical + # scroll settings. Swapping an identical frame costs + # one canvas copy and keeps the loop locked to the + # panel; falling out of that lock costs smooth motion. + # Static content is unaffected: is_currently_scrolling() + # expires on its own inactivity threshold. self._write_snapshot_if_due(frame_checksum) return diff --git a/test/test_display_dirty_tracking.py b/test/test_display_dirty_tracking.py index d367d2d6..e6dfb2b4 100644 --- a/test/test_display_dirty_tracking.py +++ b/test/test_display_dirty_tracking.py @@ -109,6 +109,11 @@ def test_snapshot_still_written_on_skip(self, dm, tmp_path): dm.draw.rectangle([0, 0, 30, 8], fill=(255, 255, 0)) dm.update_display() # push + snapshot write (first frame) assert os.path.exists(dm._snapshot_path) + # Backdate the file so the "was it bumped?" check below cannot be + # defeated by filesystem mtime granularity -- on Windows two writes in + # the same tick get identical timestamps, which made this test fail + # roughly two runs in three regardless of the code under test. + os.utime(dm._snapshot_path, (time.time() - 60, time.time() - 60)) first_mtime = os.path.getmtime(dm._snapshot_path) # Age the write/touch bookkeeping past TOUCH_INTERVAL so the next @@ -125,6 +130,62 @@ def test_snapshot_still_written_on_skip(self, dm, tmp_path): assert os.path.getmtime(dm._snapshot_path) > first_mtime +class TestScrollLock: + """Dirty tracking must not skip the panel push while a scroll is running. + + SwapOnVSync is what paces the render loop, so skipping it also skips the + wait for the panel. A duplicate frame therefore returns early -- ~8ms + instead of ~10ms on a 100Hz panel -- which advances the strip only 0.8px + instead of 1.0px, which makes the NEXT frame more likely to be a duplicate + too. That is self-sustaining: measured at ~20% duplicate frames mid-scroll + on the odds ticker against essentially zero on a lighter plugin with + identical scroll settings. Pushing an identical frame costs one canvas + copy; falling out of vsync lock costs smooth motion. + """ + + def test_identical_frames_still_push_while_scrolling(self, dm): + dm.draw.rectangle([0, 0, 12, 12], fill=(0, 0, 255)) + dm.update_display() + dm.set_scrolling_state(True) + try: + with _SwapSpy(dm.matrix) as spy: + dm.update_display() + dm.update_display() + dm.update_display() + assert spy.count == 3, "scrolling must stay locked to the panel" + finally: + dm.set_scrolling_state(False) + + def test_identical_frames_are_skipped_when_not_scrolling(self, dm): + """The optimisation still applies to static content.""" + dm.set_scrolling_state(False) + dm.draw.rectangle([0, 0, 14, 14], fill=(255, 0, 255)) + dm.update_display() + with _SwapSpy(dm.matrix) as spy: + dm.update_display() + dm.update_display() + assert spy.count == 0 + + def test_stale_scrolling_state_stops_forcing_pushes(self, dm): + """A plugin that stops scrolling without saying so must not pin the + panel into always-push forever. is_currently_scrolling() expires on + its own inactivity threshold, and the skip has to come back with it.""" + dm.draw.rectangle([0, 0, 16, 16], fill=(0, 255, 255)) + dm.update_display() + dm.set_scrolling_state(True) + try: + # Backdate the activity marker past the inactivity threshold. + dm._scrolling_state['last_scroll_activity'] = ( + time.time() - dm._scrolling_state['scroll_inactivity_threshold'] - 1.0) + assert dm.is_currently_scrolling() is False + with _SwapSpy(dm.matrix) as spy: + dm.update_display() + dm.update_display() + assert spy.count == 0 + finally: + dm.set_scrolling_state(False) + + class TestKillSwitch: def test_dirty_tracking_can_be_disabled(self, dm): dm._dirty_tracking_enabled = False From 6031e70525392be600f5a368ccb7bf5209171c14 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:41:33 -0400 Subject: [PATCH 3/7] fix(scroll): report the frame-time tail, and stop the row-major blit Two problems, both found by looking at the panel rather than the metric. The frame-stats line reported ONE instantaneous frame every 5 seconds -- about 1 frame in 500 -- printed beside a 100-frame average. Both hide exactly the fault they are used to chase: a 2ms duplicate and a 21ms double-wait average to precisely 10ms, so a ticker stalling on half its frames still reports a healthy "Avg FPS: 100.0". That reading cost several rounds of chasing the wrong layer. The line now aggregates every frame since the last log and reports median, p95, max, min, and explicit stall and skip rates (past 1.5x the median missed a refresh; under half never reached the panel, because dirty tracking skipped the swap so the frame never waited on vsync). On the hardware this now reads: leaderboard 100.0 fps over 501 frames | median 10.00ms p95 10.05ms max 10.34ms | stalls 0 (0.0%) skips 0 (0.0%) The binding rebuild's blit patch becomes opt-in (RGB_PATCH_BLIT=1, default off). Reordering that loop to row-major changes what a torn frame looks like: column-major tearing shows as a vertical seam, row-major as a horizontal split between the panel's upper and lower halves. On a 1/32 scan panel that reads as a one-pixel fold across the middle of every panel, which is what was reported on hardware and what went away when the blit was reverted. All of the measured gain comes from the SwapOnVSync change, so the risky half is simply not worth taking; the header says so. Also fixes --install resolving its paths against $HOME, which is /root under sudo, so it looked in /root/rgbmatrix-nogil-build and died with "no built module found" on a machine where the build had just succeeded. It now resolves SUDO_USER's home. Both build paths are verified on the Pi: default yields one GIL-release site, RGB_PATCH_BLIT=1 yields two. Co-Authored-By: Claude Opus 5 --- scripts/build_rgbmatrix_nogil.sh | 179 ++++++++++++++++++------------- src/common/scroll_helper.py | 41 +++++-- 2 files changed, 141 insertions(+), 79 deletions(-) diff --git a/scripts/build_rgbmatrix_nogil.sh b/scripts/build_rgbmatrix_nogil.sh index 959a6c4d..c38e550a 100644 --- a/scripts/build_rgbmatrix_nogil.sh +++ b/scripts/build_rgbmatrix_nogil.sh @@ -15,11 +15,17 @@ # Measured on a Pi 4 driving a 2x128x64 chain at limit_refresh_rate_hz=100: # # before ~44 fps average, 14-17% of frames 41-53ms -# after 100 fps locked, no stalls observed +# after 100 fps, median 10.00ms, p95 10.05ms, 0% stalls # -# This script also releases the GIL across the per-pixel blit -# (SetPixelsPillow) and walks the Pillow buffer row-major instead of -# column-major so each row is contiguous. +# The per-pixel blit (SetPixelsPillow) can also release the GIL and walk the +# Pillow buffer row-major, but that is OFF by default and you almost certainly +# want to leave it that way. Row-major changes what a partially-written frame +# looks like: column-major tearing shows as a vertical seam, row-major tearing +# shows as a horizontal split between the panel's upper and lower halves. On a +# 1/32 scan panel that reads as a one-pixel "fold" across the middle of every +# panel -- reported on hardware, and it went away when the blit was reverted. +# Enable with RGB_PATCH_BLIT=1 only if you have measured that you need it; +# essentially all of the gain above comes from the SwapOnVSync change alone. # # SAFETY # ------ @@ -35,10 +41,19 @@ # set -uo pipefail -SRC_TREE="${RGB_SRC_TREE:-$HOME/LEDMatrix/rpi-rgb-led-matrix-master}" -BUILD_DIR="${RGB_BUILD_DIR:-$HOME/rgbmatrix-nogil-build}" -VENV="${RGB_CYTHON_VENV:-$HOME/.cache/ledmatrix-cython}" -BACKUP="${RGB_BACKUP:-$HOME/rgbmatrix-core.so.ORIGINAL}" +# Resolve the invoking user's home, not root's. --install runs under sudo, +# where $HOME is /root, so every default path below pointed somewhere the +# build had never written and the install died with "no built module found". +if [ -n "${SUDO_USER:-}" ]; then + OWNER_HOME="$(getent passwd "$SUDO_USER" | cut -d: -f6)" +fi +OWNER_HOME="${OWNER_HOME:-$HOME}" + +SRC_TREE="${RGB_SRC_TREE:-$OWNER_HOME/LEDMatrix/rpi-rgb-led-matrix-master}" +BUILD_DIR="${RGB_BUILD_DIR:-$OWNER_HOME/rgbmatrix-nogil-build}" +VENV="${RGB_CYTHON_VENV:-$OWNER_HOME/.cache/ledmatrix-cython}" +BACKUP="${RGB_BACKUP:-$OWNER_HOME/rgbmatrix-core.so.ORIGINAL}" +PATCH_BLIT="${RGB_PATCH_BLIT:-0}" die() { echo "FATAL: $*" >&2; exit 1; } @@ -51,7 +66,7 @@ abi_so() { } do_rollback() { - local dst; dst="$(py_site)" || die "rgbmatrix not importable" + local dst; dst="$(py_site)" [ -n "$dst" ] || die "could not locate the installed rgbmatrix package" [ -f "$BACKUP" ] || die "no backup at $BACKUP" systemctl stop ledmatrix 2>/dev/null @@ -115,83 +130,100 @@ rm -rf "$BUILD_DIR" cp -r "$SRC_TREE" "$BUILD_DIR" || die "copy failed" echo "==> patching the bindings to release the GIL" -python3 - "$BUILD_DIR" <<'PYEOF' || die "patch failed" -import io, sys +python3 - "$BUILD_DIR" "$PATCH_BLIT" <<'PYEOF' || die "patch failed" +import io +import sys + base = sys.argv[1] + "/bindings/python/rgbmatrix/" +patch_blit = len(sys.argv) > 2 and sys.argv[2] == "1" +# --- declare SwapOnVSync as nogil --------------------------------------- p = base + "cppinc.pxd" s = io.open(p, encoding="utf-8").read() -old = " FrameCanvas *SwapOnVSync(FrameCanvas*, uint8_t)\n" -new = " FrameCanvas *SwapOnVSync(FrameCanvas*, uint8_t) nogil\n" -if old in s: - io.open(p, "w", encoding="utf-8", newline="\n").write(s.replace(old, new, 1)) +OLD_DECL = " FrameCanvas *SwapOnVSync(FrameCanvas*, uint8_t)\n" +NEW_DECL = " FrameCanvas *SwapOnVSync(FrameCanvas*, uint8_t) nogil\n" +if OLD_DECL in s: + io.open(p, "w", encoding="utf-8", newline="\n").write(s.replace(OLD_DECL, NEW_DECL, 1)) print(" cppinc.pxd: SwapOnVSync declared nogil") -elif new in s: +elif NEW_DECL in s: print(" cppinc.pxd: already nogil") else: sys.exit("could not find the SwapOnVSync declaration") +# --- release the GIL across the vsync wait ------------------------------ p = base + "core.pyx" s = io.open(p, encoding="utf-8").read() -old = """ def SwapOnVSync(self, FrameCanvas newFrame, uint8_t framerate_fraction = 1): - return __createFrameCanvas(self.__matrix.SwapOnVSync(newFrame.__canvas, framerate_fraction)) -""" -new = """ def SwapOnVSync(self, FrameCanvas newFrame, uint8_t framerate_fraction = 1): - # Blocks until the panel's next vertical sync. Holding the GIL across - # that wait starves every other Python thread for most of each frame. - cdef cppinc.RGBMatrix* matrix = self.__matrix - cdef cppinc.FrameCanvas* frame = newFrame.__canvas - cdef uint8_t fraction = framerate_fraction - cdef cppinc.FrameCanvas* swapped - with nogil: - swapped = matrix.SwapOnVSync(frame, fraction) - return __createFrameCanvas(swapped) -""" -if old in s: - s = s.replace(old, new, 1) +OLD_SWAP = ( + " def SwapOnVSync(self, FrameCanvas newFrame, uint8_t framerate_fraction = 1):\n" + " return __createFrameCanvas(" + "self.__matrix.SwapOnVSync(newFrame.__canvas, framerate_fraction))\n" +) +NEW_SWAP = ( + " def SwapOnVSync(self, FrameCanvas newFrame, uint8_t framerate_fraction = 1):\n" + " # Blocks until the panel's next vertical sync. Holding the GIL\n" + " # across that wait starves every other Python thread for most of\n" + " # each frame. Pointers are hoisted into C locals so the blocking\n" + " # call itself needs no Python state.\n" + " cdef cppinc.RGBMatrix* matrix = self.__matrix\n" + " cdef cppinc.FrameCanvas* frame = newFrame.__canvas\n" + " cdef uint8_t fraction = framerate_fraction\n" + " cdef cppinc.FrameCanvas* swapped\n" + " with nogil:\n" + " swapped = matrix.SwapOnVSync(frame, fraction)\n" + " return __createFrameCanvas(swapped)\n" +) +if OLD_SWAP in s: + s = s.replace(OLD_SWAP, NEW_SWAP, 1) print(" core.pyx: SwapOnVSync releases the GIL") -elif "with nogil:\n swapped = matrix.SwapOnVSync" in s: +elif "swapped = matrix.SwapOnVSync(frame, fraction)" in s: print(" core.pyx: SwapOnVSync already patched") else: sys.exit("could not find the SwapOnVSync body") -old = """ buffer = get_pillow_buffer(image_capsule) - - for col in range(max(0, -xstart), min(width, frame_width - xstart)): - for row in range(max(0, -ystart), min(height, frame_height - ystart)): - pixel = buffer[row][col] - r = (pixel ) & 0xFF - g = (pixel >> 8) & 0xFF - b = (pixel >> 16) & 0xFF - my_canvas.SetPixel(xstart+col, ystart+row, r, g, b) -""" -new = """ buffer = get_pillow_buffer(image_capsule) - - # Bounds hoisted so the blit needs no Python state and can run without - # the GIL: it touches only a C buffer and a C++ canvas. Row-major order - # walks each row contiguously; col-outer re-strided the whole buffer. - cdef int col_start = max(0, -xstart) - cdef int col_end = min(width, frame_width - xstart) - cdef int row_start = max(0, -ystart) - cdef int row_end = min(height, frame_height - ystart) - - with nogil: - for row in range(row_start, row_end): - for col in range(col_start, col_end): - pixel = buffer[row][col] - r = (pixel ) & 0xFF - g = (pixel >> 8) & 0xFF - b = (pixel >> 16) & 0xFF - my_canvas.SetPixel(xstart+col, ystart+row, r, g, b) -""" -if old in s: - s = s.replace(old, new, 1) - print(" core.pyx: pixel blit releases the GIL, row-major") -elif "with nogil:\n for row in range(row_start, row_end):" in s: - print(" core.pyx: blit already patched") +# --- optional: release the GIL across the blit -------------------------- +OLD_BLIT = ( + " buffer = get_pillow_buffer(image_capsule)\n" + "\n" + " for col in range(max(0, -xstart), min(width, frame_width - xstart)):\n" + " for row in range(max(0, -ystart), min(height, frame_height - ystart)):\n" + " pixel = buffer[row][col]\n" + " r = (pixel ) & 0xFF\n" + " g = (pixel >> 8) & 0xFF\n" + " b = (pixel >> 16) & 0xFF\n" + " my_canvas.SetPixel(xstart+col, ystart+row, r, g, b)\n" +) +NEW_BLIT = ( + " buffer = get_pillow_buffer(image_capsule)\n" + "\n" + " # Bounds hoisted so the blit needs no Python state and can run\n" + " # without the GIL: it touches only a C buffer and a C++ canvas.\n" + " # NOTE: row-major order makes a torn frame show as a horizontal\n" + " # split across the panel's halves. See the header before enabling.\n" + " cdef int col_start = max(0, -xstart)\n" + " cdef int col_end = min(width, frame_width - xstart)\n" + " cdef int row_start = max(0, -ystart)\n" + " cdef int row_end = min(height, frame_height - ystart)\n" + "\n" + " with nogil:\n" + " for row in range(row_start, row_end):\n" + " for col in range(col_start, col_end):\n" + " pixel = buffer[row][col]\n" + " r = (pixel ) & 0xFF\n" + " g = (pixel >> 8) & 0xFF\n" + " b = (pixel >> 16) & 0xFF\n" + " my_canvas.SetPixel(xstart+col, ystart+row, r, g, b)\n" +) +if patch_blit: + if OLD_BLIT in s: + s = s.replace(OLD_BLIT, NEW_BLIT, 1) + print(" core.pyx: pixel blit releases the GIL, row-major") + elif "for row in range(row_start, row_end):" in s: + print(" core.pyx: blit already patched") + else: + sys.exit("could not find the SetPixelsPillow loop") else: - sys.exit("could not find the SetPixelsPillow loop") + print(" core.pyx: blit left unpatched (RGB_PATCH_BLIT=1 to enable)") io.open(p, "w", encoding="utf-8", newline="\n").write(s) PYEOF @@ -231,12 +263,15 @@ echo "==> compiling the extension" SO="$(abi_so)"; [ -n "$SO" ] || die "no .so produced" # Verify the GIL really is released before anyone installs this. -PAIRS=$(grep -c "PyEval_SaveThread\|Py_UNBLOCK_THREADS" "$BUILD_DIR/bindings/python/rgbmatrix/core.cpp") -[ "$PAIRS" -ge 2 ] || die "generated C++ has only $PAIRS GIL releases, expected >= 2" +EXPECTED=1; [ "$PATCH_BLIT" = "1" ] && EXPECTED=2 +PAIRS=$(grep -c "PyEval_SaveThread\|Py_UNBLOCK_THREADS" \ + "$BUILD_DIR/bindings/python/rgbmatrix/core.cpp") +[ "$PAIRS" -ge "$EXPECTED" ] \ + || die "generated C++ has $PAIRS GIL-release sites, expected >= $EXPECTED" echo echo "BUILT: $SO" -echo " ($PAIRS GIL-release sites in the generated C++)" +echo " ($PAIRS GIL-release site(s) in the generated C++)" echo -echo "Install with: sudo bash $0 --install" +echo "Install with: sudo bash $0 --install" echo "Roll back with: sudo bash $0 --rollback" diff --git a/src/common/scroll_helper.py b/src/common/scroll_helper.py index 5b34cfe0..34ba3dd1 100644 --- a/src/common/scroll_helper.py +++ b/src/common/scroll_helper.py @@ -116,6 +116,9 @@ def __init__(self, display_width: int, display_height: int, self.last_frame_time = time.time() self.last_fps_log_time = time.time() self.frame_times = [] + # Every frame time since the last stats line, so the 5s summary can + # report the tail rather than one arbitrary sample. Cleared on log. + self._window: list = [] # Scrolling state management self.is_scrolling = False @@ -1040,18 +1043,42 @@ def log_frame_rate(self) -> None: # Keep only last 100 frames for average if len(self.frame_times) > 100: self.frame_times.pop(0) + + # Every frame since the last log, not just the last 100 and not just + # the one that happens to land on the 5s boundary. The old line + # reported a single instantaneous sample -- roughly 1 frame in 500 -- + # which cannot see a stall that hits 1% of frames, and reported it + # next to an average that hides the same stall by construction (a 2ms + # duplicate and a 21ms double-wait mean exactly 10ms). Chasing scroll + # judder needs the tail, so keep the window and report percentiles. + self._window.append(frame_time) # Log FPS every 5 seconds to avoid spam if current_time - self.last_fps_log_time >= 5.0: - avg_frame_time = sum(self.frame_times) / len(self.frame_times) - avg_fps = 1.0 / avg_frame_time if avg_frame_time > 0 else 0 - instant_fps = 1.0 / frame_time if frame_time > 0 else 0 - - self.logger.info(f"Scroll frame stats - Avg FPS: {avg_fps:.1f}, " - f"Current FPS: {instant_fps:.1f}, " - f"Frame time: {frame_time*1000:.2f}ms") + window = sorted(self._window) if self._window else [frame_time] + n = len(window) + median = window[n // 2] + p95 = window[min(n - 1, int(n * 0.95))] + worst = window[-1] + best = window[0] + mean = sum(window) / n + # Anything past 1.5x the median missed a panel refresh; anything + # under half of it never reached the panel at all (dirty tracking + # skipped the swap, so the frame did not wait for vsync). + stalls = sum(1 for f in window if f > median * 1.5) + skips = sum(1 for f in window if f < median * 0.5) + + self.logger.info( + "Scroll frame stats - %.1f fps over %d frames | " + "median %.2fms p95 %.2fms max %.2fms min %.2fms | " + "stalls %d (%.1f%%) skips %d (%.1f%%)", + (1.0 / mean) if mean > 0 else 0.0, n, + median * 1000, p95 * 1000, worst * 1000, best * 1000, + stalls, 100.0 * stalls / n, skips, 100.0 * skips / n, + ) self.last_fps_log_time = current_time self.frame_count = 0 + self._window = [] self.last_frame_time = current_time self.frame_count += 1 From 6e8a9dd24b268ad461eff435ce68b910a33455a7 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:32:52 -0400 Subject: [PATCH 4/7] feat(scroll): let users pick a crisp speed for their own panel Whole-pixel motion was previously only available at multiples of the refresh rate -- 100, 200, 300 px/s on a 100Hz panel. 100 px/s crosses a 256px panel in 2.6s, which is brisk for reading, and everything slower had to blend (blur) or repeat frames unevenly (judder). There was no way to ask for 50 px/s and get clean motion. SwapOnVSync takes a framerate_fraction the display manager never passed. It holds each frame for N panel refreshes; the panel keeps refreshing at its full rate throughout, so holding costs nothing in flicker and only changes how often a NEW image is presented. That turns 50 px/s into one whole pixel every second refresh instead of half a pixel every refresh. The crisp speeds are therefore refresh_hz / hold * pixels_per_frame, and that ladder depends on the panel: a Pi Zero on a long chain has a different set of good speeds from a Pi 4 on a short one. crisp_ladder() enumerates them and solve_crisp() picks the best match for a requested speed. solve_crisp weights motion quality rather than picking the numerically nearest entry, which matters more than it sounds. Asked for 30 px/s, nearest-by-value answers 28.6 -- 2px jumps at 14fps -- over 33.3, which is single-pixel motion at 33fps and obviously better on the panel. The target is also clamped into the ladder's range first, because relative error saturates near 1.0 for a target far outside it and the quality penalty would otherwise answer "10000 px/s" with the slowest entry. configure() snaps to the ladder and applies the hold when given a display manager. Without one the hold silently cannot happen and motion falls back to fractional pixels, so it warns rather than failing quietly. set_frame_hold() resets to 1 when scrolling stops, so one plugin's pacing cannot leak into whatever is on screen next. scripts/scroll_speeds.py is the user-facing part: it prints the ladder for the configured rate, measures what the panel ACTUALLY manages (--measure, for hardware that cannot reach its configured limit), highlights the nearest option to a wanted speed, and demos one live. It never starts or stops the display service itself -- doing that inside a script stranded the panel twice today. Speeds below ~20 px/s remain stepped regardless. That is the pixel pitch, not a software limit. Also fixes the dirty-tracking test spy, which stubbed SwapOnVSync with a single-argument function and would have masked the new call as a failed push, and rewrites a configure() test that had started passing for the wrong reason: it asserted a judder warning, which snapping now prevents, and was matching the unrelated "hold could not be applied" warning instead. 183 tests pass on the Pi. Co-Authored-By: Claude Opus 5 --- docs/SCROLL_PERFORMANCE.md | 78 ++++++++- scripts/scroll_speeds.py | 238 ++++++++++++++++++++++++++++ src/common/scroll_config.py | 193 +++++++++++++++++++++- src/display_manager.py | 34 +++- test/test_display_dirty_tracking.py | 55 ++++++- test/test_scroll_config.py | 147 ++++++++++++++++- 6 files changed, 729 insertions(+), 16 deletions(-) create mode 100644 scripts/scroll_speeds.py diff --git a/docs/SCROLL_PERFORMANCE.md b/docs/SCROLL_PERFORMANCE.md index 479c325b..bb16ddc0 100644 --- a/docs/SCROLL_PERFORMANCE.md +++ b/docs/SCROLL_PERFORMANCE.md @@ -20,8 +20,10 @@ Measured on a Raspberry Pi 4 driving a 2×128×64 chain (256×64 logical) at **Motion is smooth when the strip advances a whole number of pixels per panel refresh.** -On a 100 Hz panel the crisp speeds are 100 px/s, 200 px/s, 300 px/s. A speed -that does not divide evenly has to do one of two bad things: +Advancing one pixel per refresh on a 100 Hz panel gives 100 px/s. Slower crisp +speeds come from holding each frame for several refreshes -- 50 px/s is one +pixel every second refresh -- which is covered under *Choosing a speed* below. +A speed that lands on no such combination has to do one of two bad things: - **blend** two adjacent columns to render a half-step — on pixel-font text this alternates crisp and smeared frames and reads as shimmer, or as the @@ -31,8 +33,76 @@ that does not divide evenly has to do one of two bad things: Neither is tunable away. Pick a speed that divides evenly. -`src.common.scroll_config.resolve()` warns when a configured speed will not, -and names the nearest speed that will. +`src.common.scroll_config` solves this for you: `configure()` snaps a requested +speed to the nearest one the panel can actually show in whole pixels, and +`scripts/scroll_speeds.py` prints the full ladder for your hardware. + +## Choosing a speed + +The crisp speeds are not a fixed list -- they depend on how fast *your* panel +refreshes, which depends on its size, `pwm_bits`, `gpio_slowdown` and the Pi +model. A Pi Zero driving a long chain has a completely different set of good +speeds from a Pi 4 driving a short one. + +```bash +# what can this panel do? (reads your configured refresh rate) +python3 scripts/scroll_speeds.py + +# what does it ACTUALLY manage, rather than what is configured? +sudo systemctl stop ledmatrix +sudo python3 scripts/scroll_speeds.py --measure +sudo systemctl start ledmatrix + +# highlight the closest option to the speed you want +python3 scripts/scroll_speeds.py --want 45 + +# try one on the panel +sudo systemctl stop ledmatrix +sudo python3 scripts/scroll_speeds.py --demo 50 +sudo systemctl start ledmatrix +``` + +Sample ladder for a 100 Hz panel: + +``` + 20.0 px/s (1px every 5 refreshes = 20.0 fps, slightly stepped) + 25.0 px/s (1px every 4 refreshes = 25.0 fps, slightly stepped) + 33.3 px/s (1px every 3 refreshes = 33.3 fps, smooth) + 50.0 px/s (1px every 2 refreshes = 50.0 fps, smooth) + 66.7 px/s (2px every 3 refreshes = 33.3 fps, smooth) + 100.0 px/s (1px every 1 refresh = 100.0 fps, smooth) +``` + +### How a slow speed stays crisp + +`SwapOnVSync(canvas, framerate_fraction)` holds each frame for N panel +refreshes. **The panel keeps refreshing at its full rate either way**, so +holding a frame costs nothing in flicker -- it only changes how often a *new* +image is presented. That is what allows 50 px/s to be one whole pixel every +second refresh, instead of half a pixel every refresh (which has no good +rendering, only a choice between blur and judder). + +`scroll_config.configure()` snaps the requested speed to the nearest entry on +the ladder and applies the hold, provided it is given the display manager: + +```python +scroll_config.configure( + self.scroll_helper, + plugin_config=self.config, + global_config=self.global_config, + display_manager=self.display_manager, # required for the hold to apply +) +``` + +Without `display_manager` a sub-refresh speed still resolves, but the hold is +never applied and the motion falls back to fractional pixels -- so `configure` +logs a warning rather than failing quietly. Pass `snap_to_crisp=False` to keep +an exact requested speed and accept the artefacts. + +Speeds slower than about 20 px/s are stepped no matter what, because a 1-pixel +advance at 20 fps is simply a coarse increment. That is the pixel pitch, not a +software limit; the only way to move in smaller increments is sub-pixel +blending, which this display does not tolerate (see above). ## Configuring a plugin diff --git a/scripts/scroll_speeds.py b/scripts/scroll_speeds.py new file mode 100644 index 00000000..a142b433 --- /dev/null +++ b/scripts/scroll_speeds.py @@ -0,0 +1,238 @@ +#!/usr/bin/env python3 +"""Show and try the scroll speeds your panel can display cleanly. + +Motion looks smooth when the strip advances a WHOLE number of pixels per panel +refresh. Anything else has to blend two columns (which on pixel-font text reads +as shimmer) or repeat frames unevenly (which reads as judder). So the speeds +worth using are not arbitrary -- they are + + refresh_hz / frame_hold * pixels_per_frame + +for whole numbers of frame_hold and pixels_per_frame, and that ladder depends +on how fast YOUR panel actually refreshes. A Pi Zero driving a big chain will +have a completely different set of good speeds from a Pi 4 driving a small one. + + # what can this panel do? (no hardware needed, uses your configured rate) + python3 scripts/scroll_speeds.py + + # measure what the panel ACTUALLY manages, rather than what is configured + sudo systemctl stop ledmatrix + sudo python3 scripts/scroll_speeds.py --measure + sudo systemctl start ledmatrix + + # what would a 60Hz panel offer? + python3 scripts/scroll_speeds.py --hz 60 + + # try one on the panel + sudo systemctl stop ledmatrix + sudo python3 scripts/scroll_speeds.py --demo 50 + sudo systemctl start ledmatrix + +This script never starts or stops the display service itself -- that is left to +you, so a crash here can never leave the panel dark. +""" +from __future__ import annotations + +import argparse +import json +import os +import sys +import time +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from src.common import scroll_config # noqa: E402 + +CONFIG = Path(__file__).resolve().parent.parent / "config" / "config.json" + + +def load_hardware(): + try: + with open(CONFIG, encoding="utf-8") as handle: + return (json.load(handle).get("display") or {}).get("hardware") or {} + except (OSError, ValueError): + return {} + + +def build_options(hardware, refresh_override=None): + from rgbmatrix import RGBMatrixOptions + + o = RGBMatrixOptions() + o.rows = int(hardware.get("rows", 32)) + o.cols = int(hardware.get("cols", 64)) + o.chain_length = int(hardware.get("chain_length", 1)) + o.parallel = int(hardware.get("parallel", 1)) + o.brightness = int(hardware.get("brightness", 80)) + o.hardware_mapping = hardware.get("hardware_mapping", "regular") + o.pwm_bits = int(hardware.get("pwm_bits", 11)) + o.pwm_dither_bits = int(hardware.get("pwm_dither_bits", 0)) + o.pwm_lsb_nanoseconds = int(hardware.get("pwm_lsb_nanoseconds", 130)) + o.led_rgb_sequence = hardware.get("led_rgb_sequence", "RGB") + o.scan_mode = int(hardware.get("scan_mode", 0)) + o.row_address_type = int(hardware.get("row_address_type", 0)) + o.multiplexing = int(hardware.get("multiplexing", 0)) + o.gpio_slowdown = int(hardware.get("gpio_slowdown", 2)) + o.limit_refresh_rate_hz = ( + int(refresh_override) if refresh_override is not None + else int(hardware.get("limit_refresh_rate_hz", 0)) + ) + return o + + +def open_matrix(hardware, refresh_override=None): + """Construct the matrix, or explain why it will not open.""" + if os.geteuid() != 0: + sys.exit("this needs root for GPIO access - rerun with sudo") + try: + from rgbmatrix import RGBMatrix + except ImportError: + sys.exit("rgbmatrix is not installed on this machine") + try: + return RGBMatrix(options=build_options(hardware, refresh_override)) + except Exception as exc: # pragma: no cover - hardware dependent + sys.exit( + "could not open the panel ({}).\n" + "If the display service is running it owns the GPIO - stop it first:\n" + " sudo systemctl stop ledmatrix".format(exc) + ) + + +def measure_refresh(hardware, seconds=6.0): + """Actual refresh rate, by running uncapped and timing the swaps. + + SwapOnVSync blocks until the panel's next refresh, so an unthrottled loop + runs at exactly the panel's rate. This is what an older Pi or a longer + chain will really give you, as opposed to whatever limit_refresh_rate_hz + optimistically asks for. + """ + matrix = open_matrix(hardware, refresh_override=0) + canvas = matrix.CreateFrameCanvas() + canvas = matrix.SwapOnVSync(canvas) # discard the first, it includes setup + frames = 0 + started = time.perf_counter() + while time.perf_counter() - started < seconds: + canvas = matrix.SwapOnVSync(canvas) + frames += 1 + measured = frames / (time.perf_counter() - started) + matrix.Clear() + return measured + + +def demo(hardware, target, seconds): + """Scroll text at the crisp speed nearest `target`.""" + from PIL import Image, ImageDraw, ImageFont + + hz = float(hardware.get("limit_refresh_rate_hz") or scroll_config.DEFAULT_REFRESH_HZ) + choice = scroll_config.solve_crisp(target, hz) + print("asked for {:.0f} px/s -> {}".format(target, choice.describe())) + + matrix = open_matrix(hardware) + canvas = matrix.CreateFrameCanvas() + W, H = canvas.width, canvas.height + + font = None + for path, size in ( + (str(Path(__file__).resolve().parent.parent / "assets/fonts/PressStart2P-Regular.ttf"), 16), + ("/usr/share/fonts/truetype/dejavu/DejaVuSansMono-Bold.ttf", 26), + ): + try: + font = ImageFont.truetype(path, size) + break + except OSError: + continue + if font is None: + font = ImageFont.load_default() + + text = " {:.0f} px/s *** THE QUICK BROWN FOX JUMPS OVER THE LAZY DOG ***".format( + choice.pixels_per_second) + box = ImageDraw.Draw(Image.new("RGB", (8, 8))).textbbox((0, 0), text, font=font) + tw, th = box[2] - box[0], box[3] - box[1] + reps = max(2, (W * 3) // max(tw, 1) + 1) + strip = Image.new("RGB", (tw * reps, H), (0, 0, 0)) + draw = ImageDraw.Draw(strip) + for i in range(reps): + draw.text((i * tw, (H - th) // 2 - box[1]), text, font=font, fill=(255, 210, 60)) + + offset = 0 + frames = 0 + started = time.time() + while time.time() - started < seconds: + window = strip.crop((offset, 0, offset + W, H)) + if window.width < W: + whole = Image.new("RGB", (W, H), (0, 0, 0)) + head = strip.crop((offset, 0, strip.width, H)) + whole.paste(head, (0, 0)) + whole.paste(strip.crop((0, 0, W - head.width, H)), (head.width, 0)) + window = whole + canvas.SetImage(window) + canvas = matrix.SwapOnVSync(canvas, choice.frame_hold) + offset = (offset + choice.pixels_per_frame) % strip.width + frames += 1 + elapsed = time.time() - started + print(" {} frames in {:.1f}s = {:.1f} fps = {:.1f} px/s actual".format( + frames, elapsed, frames / elapsed, frames * choice.pixels_per_frame / elapsed)) + matrix.Clear() + + +def print_ladder(hz, highlight=None): + print("") + print("Whole-pixel scroll speeds at {:.1f}Hz refresh".format(hz)) + print("(the panel refreshes at {:.0f}Hz for every one of these - holding a " + "frame costs no flicker)".format(hz)) + print("") + for entry in scroll_config.crisp_ladder(hz): + if entry.pixels_per_second > hz * 3: + break + mark = " <-- nearest to {:.0f}".format(highlight) if ( + highlight is not None + and entry.pixels_per_second == scroll_config.solve_crisp(highlight, hz).pixels_per_second + ) else "" + print(" " + entry.describe() + mark) + print("") + print("Set one in config.json as pixels per second, e.g.") + print(' "display_options": {{"scroll_pixels_per_second": {:.0f}}}'.format( + scroll_config.solve_crisp(highlight if highlight else hz / 2, hz).pixels_per_second)) + + +def main(): + ap = argparse.ArgumentParser(description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter) + ap.add_argument("--hz", type=float, + help="refresh rate to compute the ladder for (default: your config)") + ap.add_argument("--measure", action="store_true", + help="measure the panel's real refresh rate (needs root, service stopped)") + ap.add_argument("--demo", type=float, metavar="PXPS", + help="scroll text at the crisp speed nearest this (needs root)") + ap.add_argument("--seconds", type=float, default=15.0, help="demo duration") + ap.add_argument("--want", type=float, metavar="PXPS", + help="highlight the entry nearest this speed") + args = ap.parse_args() + + hardware = load_hardware() + configured = float(hardware.get("limit_refresh_rate_hz") or 0) + + if args.demo is not None: + demo(hardware, args.demo, args.seconds) + return + + if args.measure: + measured = measure_refresh(hardware) + print("measured panel refresh: {:.1f}Hz".format(measured)) + if configured: + print("configured limit_refresh_rate_hz: {:.0f}".format(configured)) + if measured < configured * 0.95: + print(" -> the panel cannot reach the configured rate; the ladder") + print(" below uses what it actually manages") + print_ladder(measured, args.want) + return + + hz = args.hz or configured or scroll_config.DEFAULT_REFRESH_HZ + if not args.hz and not configured: + print("no limit_refresh_rate_hz in config; assuming {:.0f}Hz".format(hz)) + print("run with --measure to find your panel's real rate") + print_ladder(hz, args.want) + + +if __name__ == "__main__": + main() diff --git a/src/common/scroll_config.py b/src/common/scroll_config.py index 9bf38a0d..75d232d9 100644 --- a/src/common/scroll_config.py +++ b/src/common/scroll_config.py @@ -33,7 +33,7 @@ from __future__ import annotations import logging -from dataclasses import dataclass +from dataclasses import dataclass, replace from typing import Any, Dict, Optional logger = logging.getLogger(__name__) @@ -56,6 +56,132 @@ _WHOLE_PIXEL_TOLERANCE = 0.05 +#: Longest a frame may be held before motion reads as a slideshow rather than +#: a scroll. 6 refreshes at 100Hz is ~17px/s, already visibly stepped. +MAX_FRAME_HOLD = 8 + +#: Largest whole-pixel jump per presented frame before motion looks like it is +#: teleporting rather than sliding. +MAX_PIXELS_PER_FRAME = 6 + + +@dataclass(frozen=True) +class CrispSpeed: + """A speed the panel can show with whole-pixel motion. + + ``pixels_per_second`` is always ``refresh_hz / frame_hold * pixels_per_frame`` + exactly -- no rounding, no fractional pixel positions, so nothing has to be + blended or repeated unevenly. + + :param frame_hold: refreshes each frame is held for. This is rgbmatrix's + ``SwapOnVSync(canvas, framerate_fraction)``. The panel keeps refreshing + at full rate either way, so holding a frame costs nothing in flicker. + :param pixels_per_frame: whole pixels advanced per presented frame. + """ + + pixels_per_second: float + frame_hold: int + pixels_per_frame: int + refresh_hz: float + + @property + def frames_per_second(self) -> float: + return self.refresh_hz / self.frame_hold + + @property + def steppiness(self) -> str: + """Rough readability hint for this combination.""" + if self.pixels_per_frame > 2: + return "jumpy" + if self.frames_per_second < 20: + return "stepped" + if self.frames_per_second < 30: + return "slightly stepped" + return "smooth" + + def describe(self) -> str: + return ( + f"{self.pixels_per_second:6.1f} px/s " + f"({self.pixels_per_frame}px every {self.frame_hold} refresh" + f"{'es' if self.frame_hold != 1 else ' '} = " + f"{self.frames_per_second:5.1f} fps, {self.steppiness})" + ) + + +def crisp_ladder( + refresh_hz: float = DEFAULT_REFRESH_HZ, + max_frame_hold: int = MAX_FRAME_HOLD, + max_pixels_per_frame: int = MAX_PIXELS_PER_FRAME, +): + """Every whole-pixel speed this panel can show, slowest first. + + Duplicates are collapsed keeping the gentlest option: 100 px/s is reachable + as 1px every refresh or 2px every 2nd refresh, and the former moves in + smaller increments, so that is the one worth offering. + """ + best = {} + for hold in range(1, max_frame_hold + 1): + for ppf in range(1, max_pixels_per_frame + 1): + pps = refresh_hz / hold * ppf + key = round(pps, 3) + candidate = CrispSpeed(pps, hold, ppf, refresh_hz) + incumbent = best.get(key) + if incumbent is None or ppf < incumbent.pixels_per_frame: + best[key] = candidate + return [best[k] for k in sorted(best)] + + +#: How much a bigger pixel step costs, as a fraction of the target speed. +#: Tuned so 66.7px/s (2px at 33fps) beats 50px/s (1px at 50fps) when 60 was +#: asked for, but 33.3px/s (1px, smooth) still beats 28.6px/s (2px at 14fps) +#: when 30 was asked for -- being 11% slow is worth far less than looking bad. +_STEP_PENALTY = 0.05 +_SLOW_FPS_PENALTY = 0.25 # below 20fps +_LOWISH_FPS_PENALTY = 0.10 # below 25fps + + +def _quality_cost(candidate: "CrispSpeed", target: float) -> float: + """Lower is better. Numeric closeness alone picks bad-looking speeds. + + Nearest-by-value would answer "30 px/s" with 28.6 px/s -- which is 2px + jumps at 14fps -- over 33.3 px/s, which is single-pixel motion at 33fps and + obviously better on the panel. Proximity has to be traded against how the + motion actually reads. + """ + error = abs(candidate.pixels_per_second - target) / max(target, 1e-6) + cost = error + _STEP_PENALTY * (candidate.pixels_per_frame - 1) + fps = candidate.frames_per_second + if fps < 20: + cost += _SLOW_FPS_PENALTY + elif fps < 25: + cost += _LOWISH_FPS_PENALTY + return cost + + +def solve_crisp( + target_pixels_per_second: float, + refresh_hz: float = DEFAULT_REFRESH_HZ, + max_frame_hold: int = MAX_FRAME_HOLD, + max_pixels_per_frame: int = MAX_PIXELS_PER_FRAME, +) -> CrispSpeed: + """The whole-pixel speed that will look best for what was asked for. + + Not simply the nearest -- see :func:`_quality_cost`. Ties break toward the + smaller pixel step and the shorter hold. + """ + ladder = crisp_ladder(refresh_hz, max_frame_hold, max_pixels_per_frame) + # Clamp into the ladder's range first. Relative error saturates near 1.0 + # for a target far outside it, so the quality penalty would dominate and + # answer "10000 px/s" with the *slowest* entry -- smooth, and useless. + target = min(max(target_pixels_per_second, ladder[0].pixels_per_second), + ladder[-1].pixels_per_second) + return min( + ladder, + key=lambda c: (round(_quality_cost(c, target), 6), + c.pixels_per_frame, c.frame_hold), + ) + + @dataclass(frozen=True) class ScrollSettings: """The resolved outcome, and which config key produced it.""" @@ -65,6 +191,10 @@ class ScrollSettings: target_fps: Optional[float] = None pixels_per_frame: Optional[float] = None warning: Optional[str] = None + #: The whole-pixel speed actually applied, when snapping was enabled. + crisp: Optional[CrispSpeed] = None + #: What the config asked for, before snapping. + requested_pixels_per_second: Optional[float] = None def describe(self) -> str: text = f"{self.pixels_per_second:.1f} px/s (from {self.source})" @@ -187,6 +317,8 @@ def configure( default_pixels_per_second: float = DEFAULT_PIXELS_PER_SECOND, refresh_hz: Optional[float] = None, plugin_logger: Optional[logging.Logger] = None, + display_manager: Any = None, + snap_to_crisp: bool = True, ) -> ScrollSettings: """Resolve the config and apply it to ``scroll_helper``. @@ -194,6 +326,15 @@ def configure( stepping is not used. ``hasattr`` guards keep this usable against older ScrollHelper builds that a plugin may be running on. + :param display_manager: when given, the frame hold for the chosen speed is + applied to it. Without this a sub-refresh speed still resolves, but the + panel keeps presenting a new frame every refresh, so the motion falls + back to fractional pixels and judders -- the hold is what makes slow + speeds crisp. + :param snap_to_crisp: move the requested speed to the nearest speed the + panel can show in whole pixels. On by default because a speed that does + not divide evenly has no good rendering, only a choice of artefacts. + :returns: the settings applied, so the caller can log or assert on them. """ log = plugin_logger or logger @@ -204,13 +345,57 @@ def configure( refresh_hz=refresh_hz, ) + hz = _coerce(refresh_hz) or settings.target_fps or DEFAULT_REFRESH_HZ + applied = settings.pixels_per_second + choice = None + + if snap_to_crisp: + choice = solve_crisp(settings.pixels_per_second, hz) + applied = choice.pixels_per_second + settings = replace( + settings, + pixels_per_second=applied, + requested_pixels_per_second=settings.pixels_per_second, + crisp=choice, + pixels_per_frame=float(choice.pixels_per_frame), + # Snapping resolves the whole-pixel problem the warning describes. + warning=None if settings.warning and "judder" in settings.warning + else settings.warning, + ) + if hasattr(scroll_helper, "set_frame_based_scrolling"): scroll_helper.set_frame_based_scrolling(False) - scroll_helper.set_scroll_speed(settings.pixels_per_second) - if settings.target_fps and hasattr(scroll_helper, "set_target_fps"): + scroll_helper.set_scroll_speed(applied) + if choice and hasattr(scroll_helper, "set_target_fps"): + scroll_helper.set_target_fps(choice.frames_per_second) + elif settings.target_fps and hasattr(scroll_helper, "set_target_fps"): scroll_helper.set_target_fps(settings.target_fps) - log.info("Scroll configured: %s", settings.describe()) + if choice and display_manager is not None and hasattr(display_manager, "set_frame_hold"): + display_manager.set_frame_hold(choice.frame_hold) + + if choice: + requested = settings.requested_pixels_per_second + if abs(requested - applied) > 0.05: + log.info( + "Scroll configured: %s (asked for %.1f px/s from %s; " + "nearest whole-pixel speed on a %.0fHz panel)", + choice.describe(), requested, settings.source, hz, + ) + else: + log.info("Scroll configured: %s (from %s)", + choice.describe(), settings.source) + if choice.frame_hold > 1 and ( + display_manager is None or not hasattr(display_manager, "set_frame_hold") + ): + log.warning( + "Scroll wants a frame hold of %d but no display manager was " + "given to apply it; motion will use fractional pixels and judder", + choice.frame_hold, + ) + else: + log.info("Scroll configured: %s", settings.describe()) + if settings.warning: log.warning("Scroll speed: %s", settings.warning) return settings diff --git a/src/display_manager.py b/src/display_manager.py index e77fcad3..c54b9772 100644 --- a/src/display_manager.py +++ b/src/display_manager.py @@ -240,6 +240,14 @@ def __init__(self, config: Dict[str, Any] = None, force_fallback: bool = False, self._update_lock = threading.RLock() # Scrolling state tracking for graceful updates + # How many panel refreshes each pushed frame is held for. 1 means a new + # frame every refresh. Higher values are how a scroll runs slower than + # one pixel per refresh WITHOUT fractional pixel positions: the panel + # keeps refreshing at full rate (so flicker is unchanged) but motion + # advances a whole pixel every Nth refresh instead of every one. + # See src/common/scroll_config.py and scripts/scroll_speeds.py. + self._frame_hold = 1 + self._scrolling_state = { 'is_scrolling': False, 'last_scroll_activity': 0, @@ -804,8 +812,10 @@ def update_display(self): else: self.offscreen_canvas.SetImage(self.image) - # Swap buffers immediately - self.matrix.SwapOnVSync(self.offscreen_canvas) + # Swap buffers immediately. framerate_fraction holds the frame + # for N refreshes; SwapOnVSync blocks for all of them, which is + # what paces the render loop to the chosen frame rate. + self.matrix.SwapOnVSync(self.offscreen_canvas, self._frame_hold) # Swap our canvas references self.offscreen_canvas, self.current_canvas = self.current_canvas, self.offscreen_canvas @@ -1291,12 +1301,32 @@ def format_date_with_ordinal(self, dt): return dt.strftime(f"%b %-d{suffix}") + def set_frame_hold(self, refreshes: int) -> None: + """Hold each pushed frame for this many panel refreshes (>=1). + + Set by the scroll configuration so a plugin can run at, say, 50px/s on + a 100Hz panel as one whole pixel every second refresh, rather than half + a pixel every refresh (which has to be blended or repeated unevenly). + + Reset to 1 whenever scrolling stops, so one plugin's pacing cannot + leak into the next thing on screen. + """ + try: + value = int(refreshes) + except (TypeError, ValueError): + logger.warning("Ignoring unusable frame hold: %r", refreshes) + return + self._frame_hold = max(1, min(255, value)) + def set_scrolling_state(self, is_scrolling: bool): """Set the current scrolling state. Call this when a display starts/stops scrolling.""" current_time = time.time() self._scrolling_state['is_scrolling'] = is_scrolling if is_scrolling: self._scrolling_state['last_scroll_activity'] = current_time + else: + # Whatever pacing the finished scroll asked for must not carry over. + self._frame_hold = 1 logger.debug(f"Scrolling state set to: {is_scrolling}") def is_currently_scrolling(self) -> bool: diff --git a/test/test_display_dirty_tracking.py b/test/test_display_dirty_tracking.py index e6dfb2b4..0c63d5c3 100644 --- a/test/test_display_dirty_tracking.py +++ b/test/test_display_dirty_tracking.py @@ -47,12 +47,17 @@ class _SwapSpy: def __init__(self, matrix): self.matrix = matrix self.count = 0 + self.last_frame_hold = None self._orig = matrix.SwapOnVSync def __enter__(self): - def counting(canvas): + def counting(canvas, *args): + # *args carries framerate_fraction, which display_manager passes so + # a frame can be held for several refreshes. Signature must match + # the real binding or the spy hides a TypeError as a failed push. self.count += 1 - return self._orig(canvas) + self.last_frame_hold = args[0] if args else 1 + return self._orig(canvas, *args) self.matrix.SwapOnVSync = counting return self @@ -221,3 +226,49 @@ def test_config_flag_wires_through(self): if __name__ == "__main__": sys.exit(pytest.main([__file__, "-v"])) + + +class TestFrameHold: + """Holding a frame for N refreshes is how a scroll runs slower than one + pixel per refresh without fractional pixel positions.""" + + def test_hold_reaches_swap_on_vsync(self, dm): + dm.set_scrolling_state(True) + dm.set_frame_hold(3) + try: + dm.draw.rectangle([0, 0, 9, 9], fill=(120, 0, 200)) + with _SwapSpy(dm.matrix) as spy: + dm.update_display() + assert spy.count == 1 + assert spy.last_frame_hold == 3 + finally: + dm.set_scrolling_state(False) + + def test_default_is_every_refresh(self, dm): + dm.set_scrolling_state(True) + try: + dm.draw.rectangle([0, 0, 11, 11], fill=(0, 120, 200)) + with _SwapSpy(dm.matrix) as spy: + dm.update_display() + assert spy.last_frame_hold == 1 + finally: + dm.set_scrolling_state(False) + + def test_hold_resets_when_scrolling_stops(self, dm): + """One plugin's pacing must not leak into whatever is on screen next.""" + dm.set_scrolling_state(True) + dm.set_frame_hold(5) + dm.set_scrolling_state(False) + dm.set_scrolling_state(True) + try: + dm.draw.rectangle([0, 0, 13, 13], fill=(200, 120, 0)) + with _SwapSpy(dm.matrix) as spy: + dm.update_display() + assert spy.last_frame_hold == 1 + finally: + dm.set_scrolling_state(False) + + @pytest.mark.parametrize("bad,expected", [(0, 1), (-4, 1), (None, 1), ("x", 1)]) + def test_unusable_holds_are_ignored_or_floored(self, dm, bad, expected): + dm.set_frame_hold(bad) + assert dm._frame_hold == expected diff --git a/test/test_scroll_config.py b/test/test_scroll_config.py index f3f1dcc2..ca8f4614 100644 --- a/test/test_scroll_config.py +++ b/test/test_scroll_config.py @@ -10,6 +10,9 @@ from src.common.scroll_config import ( # noqa: E402 DEFAULT_PIXELS_PER_SECOND, + MAX_PIXELS_PER_FRAME, + crisp_ladder, + solve_crisp, MAX_PIXELS_PER_SECOND, MIN_PIXELS_PER_SECOND, ScrollSettings, @@ -40,6 +43,16 @@ def set_target_fps(self, fps): self.target_fps = fps +class FakeDisplayManager: + """Records the frame hold configure() applies.""" + + def __init__(self): + self.hold = None + + def set_frame_hold(self, refreshes): + self.hold = refreshes + + class MinimalHelper: """An older helper exposing only set_scroll_speed.""" @@ -189,11 +202,60 @@ def test_works_against_a_helper_without_optional_methods(self): configure(helper, {"display_options": {"scroll_pixels_per_second": 100.0}}) assert helper.speed == 100.0 - def test_logs_the_warning_when_speed_will_judder(self, caplog): - helper = FakeHelper() + def test_snapping_removes_the_judder_warning(self, caplog): + """50px/s cannot be shown in whole pixels at 100Hz *per refresh*, but it + can as 1px every 2nd refresh -- so once snapped there is nothing to warn + about. resolve() alone still warns; configure() resolves it.""" + assert "judder" in resolve( + {"display_options": {"scroll_pixels_per_second": 50.0}}, + refresh_hz=100).warning + helper, dm = FakeHelper(), FakeDisplayManager() + with caplog.at_level(logging.WARNING): + settings = configure( + helper, {"display_options": {"scroll_pixels_per_second": 50.0}}, + display_manager=dm) + assert settings.warning is None + assert not [r for r in caplog.records if "judder" in r.getMessage()] + + def test_warns_when_a_hold_is_needed_but_cannot_be_applied(self, caplog): + """Without a display manager the hold silently does not happen, and the + motion falls back to fractional pixels. That must not pass quietly.""" with caplog.at_level(logging.WARNING): - configure(helper, {"display_options": {"scroll_pixels_per_second": 50.0}}) - assert any("judder" in r.getMessage() for r in caplog.records) + configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 50.0}}) + assert any("frame hold" in r.getMessage() for r in caplog.records) + + def test_applies_the_frame_hold_to_the_display_manager(self): + dm = FakeDisplayManager() + configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 25.0}}, + display_manager=dm) + assert dm.hold == 4, "25px/s at 100Hz is 1px every 4th refresh" + + def test_full_speed_needs_no_hold(self): + dm = FakeDisplayManager() + configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 100.0}}, + display_manager=dm) + assert dm.hold == 1 + + def test_snaps_to_the_nearest_crisp_speed_and_reports_both(self): + s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 45.0}}, + display_manager=FakeDisplayManager()) + assert s.requested_pixels_per_second == 45.0 + assert s.pixels_per_second == 50.0 + assert s.crisp is not None and s.crisp.pixels_per_frame == 1 + + def test_snapping_can_be_turned_off(self): + s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 45.0}}, + snap_to_crisp=False) + assert s.pixels_per_second == 45.0 + assert s.crisp is None + + def test_helper_target_fps_matches_the_presentation_rate(self): + """At a hold of 4 the panel still refreshes at 100Hz, but frames are + presented at 25/s -- that is the rate the helper should pace to.""" + helper = FakeHelper() + configure(helper, {"display_options": {"scroll_pixels_per_second": 25.0}}, + display_manager=FakeDisplayManager()) + assert helper.target_fps == pytest.approx(25.0) def test_returns_settings_describing_the_source(self): s = configure(FakeHelper(), {"display_options": {"scroll_speed": 2.0, "scroll_delay": 0.01}}) @@ -211,3 +273,80 @@ def test_reads_the_hardware_limit(self): "nonsense", {"display": {"hardware": {"limit_refresh_rate_hz": None}}}]) def test_falls_back_to_the_default(self, cfg): assert refresh_hz_from_config(cfg) == 100.0 + + +class TestCrispLadder: + """Whole-pixel speeds available on a given panel.""" + + def test_every_entry_is_exactly_reachable(self): + for c in crisp_ladder(100): + assert c.pixels_per_second == pytest.approx( + c.refresh_hz / c.frame_hold * c.pixels_per_frame) + + def test_sorted_slowest_first(self): + speeds = [c.pixels_per_second for c in crisp_ladder(100)] + assert speeds == sorted(speeds) + + def test_no_duplicate_speeds(self): + speeds = [round(c.pixels_per_second, 3) for c in crisp_ladder(100)] + assert len(speeds) == len(set(speeds)) + + def test_duplicates_resolve_to_the_smaller_step(self): + """100px/s is 1px every refresh or 2px every 2nd; prefer the former.""" + entry = next(c for c in crisp_ladder(100) + if c.pixels_per_second == pytest.approx(100.0)) + assert entry.pixels_per_frame == 1 + assert entry.frame_hold == 1 + + def test_ladder_scales_with_the_panel(self): + assert any(c.pixels_per_second == pytest.approx(60.0) + for c in crisp_ladder(60)) + assert any(c.pixels_per_second == pytest.approx(30.0) + for c in crisp_ladder(60)) + + def test_full_refresh_speed_is_present(self): + for hz in (60, 75, 100, 120): + assert any(c.pixels_per_second == pytest.approx(float(hz)) + for c in crisp_ladder(hz)) + + +class TestSolveCrisp: + def test_exact_targets_are_matched_exactly(self): + for target in (100, 50, 25, 20): + assert solve_crisp(target, 100).pixels_per_second == pytest.approx(target) + + def test_prefers_smooth_motion_over_raw_proximity(self): + """30 -> 33.3 (1px, 33fps), not 28.6 (2px at 14fps) which is nearer.""" + got = solve_crisp(30, 100) + assert got.pixels_per_second == pytest.approx(33.333, abs=0.01) + assert got.pixels_per_frame == 1 + + def test_does_not_chase_a_jumpy_exact_match(self): + """45 -> 50 (1px, smooth) beats 42.9 (3px at 14fps).""" + assert solve_crisp(45, 100).pixels_per_frame == 1 + + def test_allows_a_two_pixel_step_when_it_is_clearly_closer(self): + """60 -> 66.7 (2px at 33fps) rather than 50 (1px) which is 17% slow.""" + got = solve_crisp(60, 100) + assert got.pixels_per_second == pytest.approx(66.667, abs=0.01) + assert got.pixels_per_frame == 2 + + def test_result_is_always_on_the_ladder(self): + ladder = {round(c.pixels_per_second, 3) for c in crisp_ladder(100)} + for target in range(5, 205, 5): + assert round(solve_crisp(target, 100).pixels_per_second, 3) in ladder + + def test_adapts_to_a_slower_panel(self): + got = solve_crisp(30, 60) + assert got.pixels_per_second == pytest.approx(30.0) + assert got.pixels_per_frame == 1 + assert got.frame_hold == 2 + + def test_speeds_beyond_the_panel_clamp_to_the_fastest_available(self): + got = solve_crisp(10_000, 100) + assert got.frame_hold == 1 + assert got.pixels_per_frame == MAX_PIXELS_PER_FRAME + + def test_steppiness_labels_are_sane(self): + assert solve_crisp(100, 100).steppiness == "smooth" + assert crisp_ladder(100)[0].steppiness == "stepped" From 2622c15265fc474bfb9c2723c21bd4b7451cfbe3 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 4 Sep 2026 18:17:02 -0400 Subject: [PATCH 5/7] fix(scroll): tie the frame hold to the scroll, not the plugin The hold applied in configure() never reached the panel. Plugins share one display manager, and set_scrolling_state(False) -- fired whenever ANY other plugin finishes its scroll -- reset the hold to 1. A hold set once at plugin construction was therefore always gone by the time that plugin rendered. The symptom was a log line that lied. ledmatrix-stocks reported Scroll configured: 50.0 px/s (1px every 2 refreshes = 50.0 fps, smooth) while the panel measured 100.0 fps, median 10.00ms. Config, resolution and snapping were all correct; only the pacing silently was not applied. set_scrolling_state(is_scrolling, frame_hold=1) now carries it, so the hold lives exactly as long as the scroll that asked for it. configure() reports the value as ScrollSettings.frame_hold instead of applying it -- applying it behind the caller's back could never have been right on a shared display manager. Existing callers are unaffected; the default keeps one frame per refresh. Verified on hardware: stocks at 50 px/s now measures 50.0 fps over 251 frames | median 20.00ms p95 20.09ms | stalls 0 skips 0 20.00ms being exactly two refreshes, with the panel still refreshing at 100Hz underneath so flicker is unchanged. test_another_plugin_stopping_does_not_strand_a_hold pins the interaction that broke this. Co-Authored-By: Claude Opus 5 --- src/common/scroll_config.py | 30 +++++++++---- src/display_manager.py | 39 +++++++++++++++-- test/test_display_dirty_tracking.py | 40 +++++++++++++++++ test/test_scroll_config.py | 68 ++++++++++++++++++++++------- 4 files changed, 150 insertions(+), 27 deletions(-) diff --git a/src/common/scroll_config.py b/src/common/scroll_config.py index 75d232d9..2c11d5d0 100644 --- a/src/common/scroll_config.py +++ b/src/common/scroll_config.py @@ -196,6 +196,11 @@ class ScrollSettings: #: What the config asked for, before snapping. requested_pixels_per_second: Optional[float] = None + @property + def frame_hold(self) -> int: + """Refreshes to hold each frame for; pass to set_scrolling_state().""" + return self.crisp.frame_hold if self.crisp else 1 + def describe(self) -> str: text = f"{self.pixels_per_second:.1f} px/s (from {self.source})" if self.pixels_per_frame is not None: @@ -345,7 +350,13 @@ def configure( refresh_hz=refresh_hz, ) - hz = _coerce(refresh_hz) or settings.target_fps or DEFAULT_REFRESH_HZ + # Refresh rate, most authoritative first: what the caller passed, then the + # display manager (which can see display.hardware; a plugin cannot), then + # whatever resolve() inferred, then the default. + hz = _coerce(refresh_hz) + if hz is None and display_manager is not None: + hz = _coerce(getattr(display_manager, "refresh_hz", None)) + hz = hz or settings.target_fps or DEFAULT_REFRESH_HZ applied = settings.pixels_per_second choice = None @@ -371,8 +382,11 @@ def configure( elif settings.target_fps and hasattr(scroll_helper, "set_target_fps"): scroll_helper.set_target_fps(settings.target_fps) - if choice and display_manager is not None and hasattr(display_manager, "set_frame_hold"): - display_manager.set_frame_hold(choice.frame_hold) + # Deliberately NOT applied here. The hold belongs to a scroll, not to a + # plugin's lifetime: plugins share one display manager, and one left set at + # construction is reset the moment any other plugin finishes scrolling. + # Callers pass settings.frame_hold to set_scrolling_state(True, ...) when + # they start scrolling. configure() only reports what is needed. if choice: requested = settings.requested_pixels_per_second @@ -385,12 +399,10 @@ def configure( else: log.info("Scroll configured: %s (from %s)", choice.describe(), settings.source) - if choice.frame_hold > 1 and ( - display_manager is None or not hasattr(display_manager, "set_frame_hold") - ): - log.warning( - "Scroll wants a frame hold of %d but no display manager was " - "given to apply it; motion will use fractional pixels and judder", + if choice.frame_hold > 1: + log.debug( + "Scroll needs a frame hold of %d - pass settings.frame_hold to " + "display_manager.set_scrolling_state(True, ...) each scroll", choice.frame_hold, ) else: diff --git a/src/display_manager.py b/src/display_manager.py index c54b9772..ed0811a5 100644 --- a/src/display_manager.py +++ b/src/display_manager.py @@ -1301,6 +1301,26 @@ def format_date_with_ordinal(self, dt): return dt.strftime(f"%b %-d{suffix}") + @property + def refresh_hz(self) -> float: + """The panel's refresh rate in Hz, from the hardware config. + + The authoritative place to ask, because a plugin only receives its own + config section and cannot see display.hardware. Scroll pacing needs + this: the speeds a panel can show in whole pixels are refresh_hz + divided by the frame hold, so getting it wrong silently produces + fractional-pixel motion. See src/common/scroll_config.py. + + Note this is the configured *cap*, not necessarily what the panel + achieves -- scripts/scroll_speeds.py --measure reports the real rate. + """ + hardware = (self.config.get('display') or {}).get('hardware') or {} + try: + value = float(hardware.get('limit_refresh_rate_hz') or 0) + except (TypeError, ValueError): + value = 0.0 + return value if value > 0 else 100.0 + def set_frame_hold(self, refreshes: int) -> None: """Hold each pushed frame for this many panel refreshes (>=1). @@ -1318,14 +1338,27 @@ def set_frame_hold(self, refreshes: int) -> None: return self._frame_hold = max(1, min(255, value)) - def set_scrolling_state(self, is_scrolling: bool): - """Set the current scrolling state. Call this when a display starts/stops scrolling.""" + def set_scrolling_state(self, is_scrolling: bool, frame_hold: int = 1): + """Set the current scrolling state, and this scroll's frame pacing. + + Call this when a display starts or stops scrolling. ``frame_hold`` is + how many panel refreshes each frame is held for -- 2 gives one whole + pixel every second refresh, which is how a scroll runs at half the + refresh rate without fractional pixel positions. + + The hold is set here rather than once at plugin construction because + it must not outlive the scroll that asked for it: plugins share one + display manager, so a hold left set by whoever scrolled last would + silently re-pace the next plugin. Passing it alongside the state makes + the lifetime exactly the scroll, and the default of 1 means any caller + that does not care gets a new frame every refresh. + """ current_time = time.time() self._scrolling_state['is_scrolling'] = is_scrolling if is_scrolling: self._scrolling_state['last_scroll_activity'] = current_time + self.set_frame_hold(frame_hold) else: - # Whatever pacing the finished scroll asked for must not carry over. self._frame_hold = 1 logger.debug(f"Scrolling state set to: {is_scrolling}") diff --git a/test/test_display_dirty_tracking.py b/test/test_display_dirty_tracking.py index 0c63d5c3..cf33d12b 100644 --- a/test/test_display_dirty_tracking.py +++ b/test/test_display_dirty_tracking.py @@ -272,3 +272,43 @@ def test_hold_resets_when_scrolling_stops(self, dm): def test_unusable_holds_are_ignored_or_floored(self, dm, bad, expected): dm.set_frame_hold(bad) assert dm._frame_hold == expected + + +class TestFrameHoldLifetime: + """The hold must last exactly as long as the scroll that asked for it. + + Plugins share one display manager. A hold applied at plugin construction is + wiped the moment any *other* plugin finishes scrolling, so by the time the + first plugin renders it is back to one pixel per refresh -- the speed reads + correct in the log and is wrong on the panel. + """ + + def test_scrolling_state_carries_the_hold(self, dm): + dm.set_scrolling_state(True, frame_hold=4) + try: + dm.draw.rectangle([0, 0, 7, 7], fill=(10, 200, 10)) + with _SwapSpy(dm.matrix) as spy: + dm.update_display() + assert spy.last_frame_hold == 4 + finally: + dm.set_scrolling_state(False) + + def test_another_plugin_stopping_does_not_strand_a_hold(self, dm): + dm.set_scrolling_state(True, frame_hold=3) + dm.set_scrolling_state(False) # some other plugin finishes + dm.set_scrolling_state(True) # a plugin that wants no hold + try: + dm.draw.rectangle([0, 0, 6, 6], fill=(200, 10, 10)) + with _SwapSpy(dm.matrix) as spy: + dm.update_display() + assert spy.last_frame_hold == 1 + finally: + dm.set_scrolling_state(False) + + def test_default_keeps_previous_behaviour(self, dm): + """Callers that never heard of frame holds get one frame per refresh.""" + dm.set_scrolling_state(True) + try: + assert dm._frame_hold == 1 + finally: + dm.set_scrolling_state(False) diff --git a/test/test_scroll_config.py b/test/test_scroll_config.py index ca8f4614..0c7f7ff2 100644 --- a/test/test_scroll_config.py +++ b/test/test_scroll_config.py @@ -217,24 +217,25 @@ def test_snapping_removes_the_judder_warning(self, caplog): assert settings.warning is None assert not [r for r in caplog.records if "judder" in r.getMessage()] - def test_warns_when_a_hold_is_needed_but_cannot_be_applied(self, caplog): - """Without a display manager the hold silently does not happen, and the - motion falls back to fractional pixels. That must not pass quietly.""" - with caplog.at_level(logging.WARNING): - configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 50.0}}) - assert any("frame hold" in r.getMessage() for r in caplog.records) - - def test_applies_the_frame_hold_to_the_display_manager(self): + def test_reports_the_hold_without_applying_it(self): + """The hold belongs to a scroll, not a plugin's lifetime -- one left set + at construction is wiped as soon as any other plugin stops scrolling. + configure() reports it; the caller passes it to set_scrolling_state.""" dm = FakeDisplayManager() - configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 25.0}}, - display_manager=dm) - assert dm.hold == 4, "25px/s at 100Hz is 1px every 4th refresh" + s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 25.0}}, + display_manager=dm) + assert s.frame_hold == 4, "25px/s at 100Hz is 1px every 4th refresh" + assert dm.hold is None, "must not apply the hold behind the caller's back" def test_full_speed_needs_no_hold(self): - dm = FakeDisplayManager() - configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 100.0}}, - display_manager=dm) - assert dm.hold == 1 + s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 100.0}}, + display_manager=FakeDisplayManager()) + assert s.frame_hold == 1 + + def test_frame_hold_is_one_when_snapping_is_off(self): + s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 50.0}}, + snap_to_crisp=False) + assert s.frame_hold == 1 def test_snaps_to_the_nearest_crisp_speed_and_reports_both(self): s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 45.0}}, @@ -350,3 +351,40 @@ def test_speeds_beyond_the_panel_clamp_to_the_fastest_available(self): def test_steppiness_labels_are_sane(self): assert solve_crisp(100, 100).steppiness == "smooth" assert crisp_ladder(100)[0].steppiness == "stepped" + + +class TestRefreshFromDisplayManager: + """A plugin sees only its own config section, so the display manager is the + authoritative source for the panel's refresh rate.""" + + class DM: + def __init__(self, hz): + self.refresh_hz = hz + self.hold = None + + def set_frame_hold(self, refreshes): + self.hold = refreshes + + def test_uses_the_display_manager_rate(self): + dm = self.DM(60.0) + s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 30.0}}, + display_manager=dm) + # 30px/s on a 60Hz panel is 1px every 2nd refresh, exactly. + assert s.pixels_per_second == pytest.approx(30.0) + assert s.frame_hold == 2 + + def test_explicit_refresh_hz_wins_over_the_display_manager(self): + dm = self.DM(60.0) + s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 25.0}}, + display_manager=dm, refresh_hz=100) + assert s.frame_hold == 4, "25px/s at 100Hz is 1px every 4th refresh" + + def test_missing_attribute_falls_back_to_the_default(self): + class Bare: + def set_frame_hold(self, refreshes): + self.hold = refreshes + + bare = Bare() + s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 50.0}}, + display_manager=bare) + assert s.frame_hold == 2, "assumed 100Hz" From 571cc6fdacf77c080b54e7076bf89c9ce02bc66e Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Sun, 6 Sep 2026 17:43:30 -0400 Subject: [PATCH 6/7] fix(scroll,cache): resolve CodeRabbit review on #523 Eight findings, all reproduced before fixing. scroll_config.configure() read the refresh rate *after* resolve() had already used it. resolve() fills in target_fps, pixels_per_frame and the judder warning from that rate, so on a 60Hz panel every one of them described 100Hz -- and with snap_to_crisp=False nothing downstream corrected it, so set_target_fps() paced the helper to 100 FPS. The rate is now settled first, and falls back to the global config rather than straight to the default. refresh_hz_from_config() used `(cfg.get("display") or {}).get(...)`, which raises AttributeError when either level is truthy but not a mapping -- out of a function whose whole contract is a rate or a default. The frame-stats line reported the upper-middle sample as the median and the 96th sorted sample as p95 of 100. Both are also thresholds (stalls at 1.5x the median, skips at 0.5x), so the counts were biased too. The arithmetic is now in frame_stats()/format_frame_stats(), testable without a clock. configure()'s docstring and docs/SCROLL_PERFORMANCE.md still said it applies the frame hold and warns when it cannot. It deliberately does neither since "tie the frame hold to the scroll, not the plugin"; a caller following the old text would omit set_scrolling_state() and slow snapped speeds would still present every refresh. disk_cache had no policy for non-finite floats: orjson writes null, the stdlib writes NaN/Infinity, and orjson then rejects those legacy files so DiskCache.get deleted them as corrupt. One behaviour on both paths now -- write null, keep legacy records readable. allow_nan=False detects the values; the replacement walk runs only when there is one, so the ordinary write path is byte-identical and pays nothing. build_rgbmatrix_nogil.sh picked the build artifact with a glob piped to `head -1`, which sorts cpython-311 ahead of cpython-313, so a stale .so staged in from the source tree was installed as core.so while the GIL check -- which reads the generated core.cpp, not the .so -- still passed. It now requires the current interpreter's exact ABI name and fails closed. Its systemctl calls were also unchecked under `set -uo pipefail`: a failed stop left the old service running, the following start succeeded as a no-op, and the health check reported SUCCESS for a binding that was never loaded. orjson floor raised to 3.11.6 for CVE-2025-67221 (unbounded recursion in dumps); it covers the project's Python 3.10-3.13 range. Adds test/test_cache_nonfinite_floats.py (14) plus regression tests in test_scroll_config.py and test_scroll_helper.py. 9 of the cache tests and 9 of the scroll_config tests fail against the pre-fix code. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --- docs/SCROLL_PERFORMANCE.md | 22 ++-- requirements.txt | 6 +- scripts/build_rgbmatrix_nogil.sh | 69 ++++++++++-- src/cache/disk_cache.py | 50 ++++++++- src/common/scroll_config.py | 47 ++++++--- src/common/scroll_helper.py | 65 ++++++++---- test/test_cache_nonfinite_floats.py | 158 ++++++++++++++++++++++++++++ test/test_scroll_config.py | 77 ++++++++++++++ test/test_scroll_helper.py | 80 +++++++++++++- 9 files changed, 522 insertions(+), 52 deletions(-) create mode 100644 test/test_cache_nonfinite_floats.py diff --git a/docs/SCROLL_PERFORMANCE.md b/docs/SCROLL_PERFORMANCE.md index bb16ddc0..57176dab 100644 --- a/docs/SCROLL_PERFORMANCE.md +++ b/docs/SCROLL_PERFORMANCE.md @@ -83,21 +83,29 @@ second refresh, instead of half a pixel every refresh (which has no good rendering, only a choice between blur and judder). `scroll_config.configure()` snaps the requested speed to the nearest entry on -the ladder and applies the hold, provided it is given the display manager: +the ladder and reports the hold that speed needs. It does **not** apply the +hold: the hold belongs to a scroll, not to a plugin's lifetime, and plugins +share one display manager -- one set at construction is reset the moment any +other plugin finishes scrolling. Apply it yourself when the scroll starts: ```python -scroll_config.configure( +settings = scroll_config.configure( self.scroll_helper, plugin_config=self.config, global_config=self.global_config, - display_manager=self.display_manager, # required for the hold to apply + display_manager=self.display_manager, # supplies the panel refresh rate ) + +# ...then, each time this plugin begins scrolling: +self.display_manager.set_scrolling_state(True, frame_hold=settings.frame_hold) ``` -Without `display_manager` a sub-refresh speed still resolves, but the hold is -never applied and the motion falls back to fractional pixels -- so `configure` -logs a warning rather than failing quietly. Pass `snap_to_crisp=False` to keep -an exact requested speed and accept the artefacts. +Passing `display_manager` only lets `configure` read the true refresh rate from +`display.hardware`, which a plugin config cannot see. Skipping the +`set_scrolling_state` call is the mistake that matters: the speed still +resolves, but the panel keeps presenting a new frame every refresh, so a slow +snapped speed falls back to fractional pixels. Pass `snap_to_crisp=False` to +keep an exact requested speed and accept the artefacts. Speeds slower than about 20 px/s are stepped no matter what, because a 1-pixel advance at 20 fps is simply a coarse increment. That is the pixel pitch, not a diff --git a/requirements.txt b/requirements.txt index 2d0350bf..70a7d2e2 100644 --- a/requirements.txt +++ b/requirements.txt @@ -61,7 +61,11 @@ packaging>=23.0,<27.0 # matters because that work holds the GIL and stalls # the render thread mid-scroll. Falls back to the # stdlib json when missing — see docs/SCROLL_PERFORMANCE.md. -# pip install 'orjson>=3.9,<4.0' +# The 3.11.6 floor is CVE-2025-67221: orjson.dumps did not +# limit recursion on deeply nested documents, and the disk +# cache encodes payloads parsed straight from third-party +# APIs. 3.11.6 covers the Python range above. +# pip install 'orjson>=3.11.6,<4.0' # # Flask-Limiter — request rate limiting in web_interface/app.py # (accidental-abuse protection, not security). The diff --git a/scripts/build_rgbmatrix_nogil.sh b/scripts/build_rgbmatrix_nogil.sh index c38e550a..f92a7189 100644 --- a/scripts/build_rgbmatrix_nogil.sh +++ b/scripts/build_rgbmatrix_nogil.sh @@ -57,22 +57,64 @@ PATCH_BLIT="${RGB_PATCH_BLIT:-0}" die() { echo "FATAL: $*" >&2; exit 1; } +# This script runs under `set -uo pipefail` -- no -e -- so an unchecked +# systemctl failure is silently ignored. That matters most for `stop`: leaving +# the old service running means cp overwrites a module the running process has +# mapped, the following `start` succeeds as a no-op, and the health check sees +# an active unit and reports SUCCESS for a binding that was never loaded. +# A machine with no ledmatrix.service at all is a normal build host, so that +# case is skipped rather than treated as a failure. +service_present() { systemctl cat ledmatrix.service >/dev/null 2>&1; } + +service_do() { + local verb="$1" + if ! service_present; then + echo " (no ledmatrix.service installed - skipping $verb)" + return 0 + fi + systemctl "$verb" ledmatrix || die "systemctl $verb ledmatrix failed" +} + py_site() { python3 -c 'import rgbmatrix, os; print(os.path.dirname(rgbmatrix.__file__))' 2>/dev/null } +# The extension filename the interpreter that builds -- and then loads -- this +# module actually uses, e.g. core.cpython-313-aarch64-linux-gnu.so. The build +# venv is made with --system-site-packages from python3, so the two agree; +# falling back keeps --install working when the venv has been cleaned up. +abi_name() { + local py="$VENV/bin/python" + [ -x "$py" ] || py=python3 + "$py" -c \ + 'import sysconfig; print("core" + sysconfig.get_config_var("EXT_SUFFIX"))' \ + 2>/dev/null +} + +# Exactly the current interpreter's artifact, never merely the first one that +# sorts. Staging copies $SRC_TREE wholesale, so a core.cpython-*.so left in the +# source tree by an earlier build comes along for the ride; build_ext --inplace +# only ever overwrites the current ABI's name, and a glob piped to `head -1` +# sorts cpython-311 ahead of cpython-313. That installed a stale, unpatched +# module as core.so while the GIL check below -- which reads the freshly +# generated core.cpp, not the .so -- still reported success. abi_so() { - ls "$BUILD_DIR"/bindings/python/rgbmatrix/core.cpython-*.so 2>/dev/null | head -1 + local name path + name="$(abi_name)" || return 1 + [ -n "$name" ] || return 1 + path="$BUILD_DIR/bindings/python/rgbmatrix/$name" + [ -f "$path" ] || return 1 + printf '%s\n' "$path" } do_rollback() { local dst; dst="$(py_site)" [ -n "$dst" ] || die "could not locate the installed rgbmatrix package" [ -f "$BACKUP" ] || die "no backup at $BACKUP" - systemctl stop ledmatrix 2>/dev/null + service_do stop cp -a "$BACKUP" "$dst/core.so" || die "restore failed" find "$dst" -name __pycache__ -type d -exec rm -rf {} + 2>/dev/null - systemctl start ledmatrix 2>/dev/null + service_do start echo "rolled back to the original core.so" exit 0 } @@ -89,10 +131,10 @@ do_install() { echo "backup already present at $BACKUP (keeping the true original)" fi - systemctl stop ledmatrix 2>/dev/null + service_do stop cp "$so" "$dst/core.so" || die "install failed" find "$dst" -name __pycache__ -type d -exec rm -rf {} + 2>/dev/null - systemctl start ledmatrix 2>/dev/null + service_do start echo "waiting 25s for the display to come back..." sleep 25 @@ -106,8 +148,12 @@ do_install() { echo "SUCCESS - running on the rebuilt binding" else echo "UNHEALTHY - rolling back" - cp -a "$BACKUP" "$dst/core.so" - systemctl restart ledmatrix + cp -a "$BACKUP" "$dst/core.so" \ + || echo "ROLLBACK FAILED: could not restore $BACKUP -> $dst/core.so" >&2 + if service_present && ! systemctl restart ledmatrix; then + echo "ROLLBACK FAILED: ledmatrix did not restart - the display is" \ + "down; restore manually with 'sudo bash $0 --rollback'" >&2 + fi journalctl -u ledmatrix --since "90 sec ago" --no-pager | tail -25 exit 1 fi @@ -129,6 +175,12 @@ echo "==> staging a scratch copy at $BUILD_DIR" rm -rf "$BUILD_DIR" cp -r "$SRC_TREE" "$BUILD_DIR" || die "copy failed" +# Drop any extension artifacts that came across from the source tree. Nothing +# downstream should be able to pick one up, and build_ext --inplace can decide +# a copied .so is already up to date and skip the compile entirely. +find "$BUILD_DIR/bindings/python/rgbmatrix" -maxdepth 1 \ + -name 'core*.so' -delete 2>/dev/null + echo "==> patching the bindings to release the GIL" python3 - "$BUILD_DIR" "$PATCH_BLIT" <<'PYEOF' || die "patch failed" import io @@ -260,7 +312,8 @@ echo "==> compiling the extension" ( cd "$BUILD_DIR/bindings/python" && "$VENV/bin/python" setup.py build_ext --inplace ) \ >/dev/null 2>&1 || die "extension build failed" -SO="$(abi_so)"; [ -n "$SO" ] || die "no .so produced" +SO="$(abi_so)" || true +[ -n "$SO" ] || die "no .so produced - expected $(abi_name) in $BUILD_DIR/bindings/python/rgbmatrix" # Verify the GIL really is released before anyone installs this. EXPECTED=1; [ "$PATCH_BLIT" = "1" ] && EXPECTED=2 diff --git a/src/cache/disk_cache.py b/src/cache/disk_cache.py index 30ffd1ad..92c3f465 100644 --- a/src/cache/disk_cache.py +++ b/src/cache/disk_cache.py @@ -5,6 +5,7 @@ """ import json +import math import os import time import tempfile @@ -62,6 +63,38 @@ def _datetime_default(obj: Any) -> Any: raise TypeError(f"Object of type {type(obj).__name__} is not JSON serializable") +def _replace_nonfinite(obj: Any) -> Any: + """Non-finite floats -> None, matching what ``orjson.dumps`` writes. + + Only reached once a strict pass has proved there is something to replace, + so the ordinary write path never pays for this walk. + """ + if isinstance(obj, float): + return obj if math.isfinite(obj) else None + if isinstance(obj, dict): + return {k: _replace_nonfinite(v) for k, v in obj.items()} + if isinstance(obj, (list, tuple)): + return [_replace_nonfinite(v) for v in obj] + return obj + + +# NON-FINITE FLOATS +# ----------------- +# JSON has no NaN or Infinity. The stdlib emits them anyway as an extension; +# orjson refuses to and writes null. That divergence is not acceptable in a +# cache whose files outlive the decision of which encoder is installed, so the +# policy here is one behaviour on both paths: +# +# writing non-finite floats become null, whichever encoder is in use +# reading files already on disk that carry the stdlib's NaN/Infinity +# tokens stay readable, whichever encoder is in use +# +# Without the write half, installing orjson silently changed cached values. +# Without the read half, installing orjson turned every legacy record holding a +# NaN into a "corrupted cache file" that DiskCache.get logged as an error and +# deleted. Both halves are covered by test/test_cache_nonfinite_floats.py. + + if orjson is not None: # Encoding the cache record dominated the background fetch worker: on a # Pi 4, stdlib json.dumps runs ~12ms per MB and holds the GIL for all of @@ -82,10 +115,23 @@ def _dumps(data: Any) -> bytes: return orjson.dumps(data, default=_datetime_default, option=_DUMPS_OPTS) def _loads(raw: bytes) -> Any: - return orjson.loads(raw) + try: + return orjson.loads(raw) + except orjson.JSONDecodeError: + # Legacy record written by the stdlib path, carrying NaN or + # Infinity. Genuinely malformed files raise again from here, as + # json.JSONDecodeError, which is what DiskCache.get expects. + return json.loads(raw) else: def _dumps(data: Any) -> bytes: - return json.dumps(data, cls=DateTimeEncoder).encode("utf-8") + try: + return json.dumps(data, cls=DateTimeEncoder, + allow_nan=False).encode("utf-8") + except ValueError: + # allow_nan=False is what detects the non-finite values; the walk + # runs only now that we know there is one to replace. + return json.dumps(_replace_nonfinite(data), cls=DateTimeEncoder, + allow_nan=False).encode("utf-8") def _loads(raw: bytes) -> Any: return json.loads(raw) diff --git a/src/common/scroll_config.py b/src/common/scroll_config.py index 2c11d5d0..afaac8e7 100644 --- a/src/common/scroll_config.py +++ b/src/common/scroll_config.py @@ -331,11 +331,13 @@ def configure( stepping is not used. ``hasattr`` guards keep this usable against older ScrollHelper builds that a plugin may be running on. - :param display_manager: when given, the frame hold for the chosen speed is - applied to it. Without this a sub-refresh speed still resolves, but the - panel keeps presenting a new frame every refresh, so the motion falls - back to fractional pixels and judders -- the hold is what makes slow - speeds crisp. + :param display_manager: consulted for the panel's refresh rate only (it can + see display.hardware; a plugin cannot). The frame hold is NOT applied + here -- see the note in the body. The caller must pass + ``settings.frame_hold`` to ``display_manager.set_scrolling_state(True, + ...)`` when it starts scrolling, or a sub-refresh speed still presents + a new frame every refresh and the motion falls back to fractional + pixels. :param snap_to_crisp: move the requested speed to the nearest speed the panel can show in whole pixels. On by default because a speed that does not divide evenly has no good rendering, only a choice of artefacts. @@ -343,20 +345,28 @@ def configure( :returns: the settings applied, so the caller can log or assert on them. """ log = plugin_logger or logger - settings = resolve( - plugin_config, - global_config, - default_pixels_per_second=default_pixels_per_second, - refresh_hz=refresh_hz, - ) # Refresh rate, most authoritative first: what the caller passed, then the # display manager (which can see display.hardware; a plugin cannot), then - # whatever resolve() inferred, then the default. + # the global config, then the default. + # + # This has to be settled BEFORE resolve(), not after. resolve() uses the + # refresh to fill in target_fps, pixels_per_frame and the judder warning, + # so deriving it afterwards described a 100Hz panel to everyone running at + # 60 -- and with snap_to_crisp=False nothing downstream corrected it, so + # set_target_fps() paced the helper to 100 FPS on a 60Hz panel. hz = _coerce(refresh_hz) if hz is None and display_manager is not None: hz = _coerce(getattr(display_manager, "refresh_hz", None)) - hz = hz or settings.target_fps or DEFAULT_REFRESH_HZ + if hz is None: + hz = refresh_hz_from_config(global_config) + + settings = resolve( + plugin_config, + global_config, + default_pixels_per_second=default_pixels_per_second, + refresh_hz=hz, + ) applied = settings.pixels_per_second choice = None @@ -417,5 +427,14 @@ def refresh_hz_from_config(global_config: Optional[Dict[str, Any]]) -> float: """The panel's refresh cap from the global config, or the default.""" if not isinstance(global_config, dict): return DEFAULT_REFRESH_HZ - hardware = (global_config.get("display") or {}).get("hardware") or {} + # Each level is checked for being a mapping rather than merely truthy: a + # malformed config where display or display.hardware is a string or a list + # raised AttributeError out of what is meant to be a total function with a + # default, taking down every caller that asked for the refresh rate. + display = global_config.get("display") + if not isinstance(display, dict): + return DEFAULT_REFRESH_HZ + hardware = display.get("hardware") + if not isinstance(hardware, dict): + return DEFAULT_REFRESH_HZ return _coerce(hardware.get("limit_refresh_rate_hz")) or DEFAULT_REFRESH_HZ diff --git a/src/common/scroll_helper.py b/src/common/scroll_helper.py index 34ba3dd1..d5e949d2 100644 --- a/src/common/scroll_helper.py +++ b/src/common/scroll_helper.py @@ -16,6 +16,7 @@ """ import logging +import math import time from typing import Optional, Dict, Any from PIL import Image @@ -29,6 +30,49 @@ HAS_SCIPY = False +def frame_stats(frame_times: list) -> Dict[str, Any]: + """Summary statistics over one window of frame durations (seconds). + + Split out of log_frame_rate() so the arithmetic can be tested without a + clock. Median and p95 are the real ones: the median takes both middle + samples on an even window, and p95 is nearest-rank, so a 100-frame window + reports the 95th sorted sample rather than the 96th. That matters twice + over, because the median is also the threshold the stall and skip counts + are measured against. + """ + window = sorted(frame_times) + n = len(window) + median = (window[n // 2] if n % 2 + else (window[n // 2 - 1] + window[n // 2]) / 2.0) + mean = sum(window) / n + # Anything past 1.5x the median missed a panel refresh; anything under + # half of it never reached the panel at all (dirty tracking skipped the + # swap, so the frame did not wait for vsync). + return { + "frames": n, + "fps": (1.0 / mean) if mean > 0 else 0.0, + "median": median, + "p95": window[max(0, math.ceil(0.95 * n) - 1)], + "max": window[-1], + "min": window[0], + "stalls": sum(1 for f in window if f > median * 1.5), + "skips": sum(1 for f in window if f < median * 0.5), + } + + +def format_frame_stats(frame_times: list) -> str: + """The one-line rendering of frame_stats(), in milliseconds.""" + s = frame_stats(frame_times) + n = s["frames"] + return ( + f"{s['fps']:.1f} fps over {n} frames | " + f"median {s['median'] * 1000:.2f}ms p95 {s['p95'] * 1000:.2f}ms " + f"max {s['max'] * 1000:.2f}ms min {s['min'] * 1000:.2f}ms | " + f"stalls {s['stalls']} ({100.0 * s['stalls'] / n:.1f}%) " + f"skips {s['skips']} ({100.0 * s['skips'] / n:.1f}%)" + ) + + class ScrollHelper: """ Helper class for scrolling text and image content on LED displays. @@ -1055,26 +1099,9 @@ def log_frame_rate(self) -> None: # Log FPS every 5 seconds to avoid spam if current_time - self.last_fps_log_time >= 5.0: - window = sorted(self._window) if self._window else [frame_time] - n = len(window) - median = window[n // 2] - p95 = window[min(n - 1, int(n * 0.95))] - worst = window[-1] - best = window[0] - mean = sum(window) / n - # Anything past 1.5x the median missed a panel refresh; anything - # under half of it never reached the panel at all (dirty tracking - # skipped the swap, so the frame did not wait for vsync). - stalls = sum(1 for f in window if f > median * 1.5) - skips = sum(1 for f in window if f < median * 0.5) - self.logger.info( - "Scroll frame stats - %.1f fps over %d frames | " - "median %.2fms p95 %.2fms max %.2fms min %.2fms | " - "stalls %d (%.1f%%) skips %d (%.1f%%)", - (1.0 / mean) if mean > 0 else 0.0, n, - median * 1000, p95 * 1000, worst * 1000, best * 1000, - stalls, 100.0 * stalls / n, skips, 100.0 * skips / n, + "Scroll frame stats - %s", + format_frame_stats(self._window or [frame_time]), ) self.last_fps_log_time = current_time self.frame_count = 0 diff --git a/test/test_cache_nonfinite_floats.py b/test/test_cache_nonfinite_floats.py new file mode 100644 index 00000000..55cdfaa9 --- /dev/null +++ b/test/test_cache_nonfinite_floats.py @@ -0,0 +1,158 @@ +"""Non-finite floats round-trip identically with and without orjson. + +JSON has no NaN or Infinity. The stdlib emits them anyway as an extension; +orjson refuses to and writes null. Cache files outlive the decision of which +encoder is installed, so both halves of that gap are pinned here: + + * writing -- installing orjson must not silently change what gets cached, + so the stdlib path writes null too; + * reading -- records already on disk carrying NaN or Infinity must stay + readable, or installing orjson turns each of them into a "corrupted cache + file" that DiskCache.get logs as an error and deletes. +""" + +import importlib +import json +import math +import sys +from pathlib import Path +from unittest import mock + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from src.cache import disk_cache as disk_cache_module # noqa: E402 + + +@pytest.fixture +def stdlib_cache(): + """The module as it loads on a host with no orjson wheel.""" + # A None entry in sys.modules makes `import orjson` raise ImportError, + # which is the branch we want, whether or not orjson is really installed. + with mock.patch.dict(sys.modules, {"orjson": None}): + module = importlib.reload(disk_cache_module) + assert module.orjson is None + yield module + importlib.reload(disk_cache_module) + + +@pytest.fixture +def orjson_cache(): + """The module as it loads with orjson available.""" + module = importlib.reload(disk_cache_module) + if module.orjson is None: + pytest.skip("orjson is not installed on this host") + return module + + +NON_FINITE = {"nan": float("nan"), "inf": float("inf"), "ninf": float("-inf")} + + +def _reject_constant(name): + """Make json.loads as strict as orjson about NaN/Infinity tokens.""" + raise AssertionError(f"non-spec JSON constant in output: {name}") + + +class TestWritePolicy: + """Non-finite floats become null on whichever encoder is in use.""" + + def _assert_nulled(self, module): + record = module._loads(module._dumps(dict(NON_FINITE, ok=1.5))) + assert record["nan"] is None + assert record["inf"] is None + assert record["ninf"] is None + # Finite values are untouched. + assert record["ok"] == 1.5 + + def test_stdlib_writes_null(self, stdlib_cache): + self._assert_nulled(stdlib_cache) + + def test_orjson_writes_null(self, orjson_cache): + self._assert_nulled(orjson_cache) + + def test_stdlib_emits_spec_compliant_json(self, stdlib_cache): + # The point of the write half: bytes written without orjson must still + # parse once orjson is installed later. json.loads accepts the + # extension tokens, so it cannot show this -- assert on the bytes, and + # on a strict reader when there is one. + raw = stdlib_cache._dumps(dict(NON_FINITE)) + assert b"NaN" not in raw + assert b"Infinity" not in raw + assert json.loads(raw, parse_constant=_reject_constant) == { + "nan": None, "inf": None, "ninf": None} + + def test_nested_non_finite_are_replaced(self, stdlib_cache): + data = {"a": [1.0, float("nan"), {"b": float("inf")}], "c": (float("-inf"),)} + record = stdlib_cache._loads(stdlib_cache._dumps(data)) + assert record["a"] == [1.0, None, {"b": None}] + assert record["c"] == [None] + + def test_finite_payloads_are_byte_identical_to_the_old_encoder(self, stdlib_cache): + # allow_nan=False must not change ordinary output. + data = {"x": 1, "y": [1.5, "s", True, None], "z": {"k": 2.25}} + assert stdlib_cache._dumps(data) == json.dumps( + data, cls=stdlib_cache.DateTimeEncoder).encode("utf-8") + + +class TestReplaceNonFinite: + def test_leaves_ordinary_values_alone(self): + for value in (1, 1.5, "s", True, None, [], {}): + assert disk_cache_module._replace_nonfinite(value) == value + + def test_replaces_every_non_finite_float(self): + for value in NON_FINITE.values(): + assert disk_cache_module._replace_nonfinite(value) is None + + def test_walks_nested_containers(self): + assert disk_cache_module._replace_nonfinite( + {"a": [{"b": float("nan")}]}) == {"a": [{"b": None}]} + + +class TestLegacyRecordsStayReadable: + """Files written before orjson arrived still load.""" + + LEGACY = b'{"timestamp": 1000.0, "value": NaN, "other": Infinity}' + + def test_stdlib_reads_legacy_tokens(self, stdlib_cache): + record = stdlib_cache._loads(self.LEGACY) + assert math.isnan(record["value"]) + assert math.isinf(record["other"]) + + def test_orjson_falls_back_for_legacy_tokens(self, orjson_cache): + record = orjson_cache._loads(self.LEGACY) + assert math.isnan(record["value"]) + assert math.isinf(record["other"]) + + def test_genuinely_malformed_files_still_raise(self, stdlib_cache): + with pytest.raises(json.JSONDecodeError): + stdlib_cache._loads(b'{"a": ') + + def test_orjson_still_raises_for_malformed_files(self, orjson_cache): + with pytest.raises(json.JSONDecodeError): + orjson_cache._loads(b'{"a": ') + + +class TestDiskCacheEndToEnd: + def _cache(self, module, tmp_path): + return module.DiskCache(str(tmp_path)) + + def test_legacy_file_is_not_deleted_as_corrupt(self, orjson_cache, tmp_path): + cache = self._cache(orjson_cache, tmp_path) + path = cache.get_cache_path("legacy") + Path(path).write_bytes( + b'{"timestamp": %d, "value": NaN}' % int(__import__("time").time())) + + record = cache.get("legacy", max_age=None) + + assert record is not None, "legacy NaN record was treated as corrupt" + assert math.isnan(record["value"]) + assert Path(path).exists(), "legacy NaN record was deleted" + + def test_round_trip_through_set_and_get(self, stdlib_cache, tmp_path): + cache = self._cache(stdlib_cache, tmp_path) + cache.set("k", {"timestamp": __import__("time").time(), + "value": float("nan")}) + record = cache.get("k", max_age=None) + assert record is not None + assert record["value"] is None diff --git a/test/test_scroll_config.py b/test/test_scroll_config.py index 0c7f7ff2..ad0104a5 100644 --- a/test/test_scroll_config.py +++ b/test/test_scroll_config.py @@ -275,6 +275,20 @@ def test_reads_the_hardware_limit(self): def test_falls_back_to_the_default(self, cfg): assert refresh_hz_from_config(cfg) == 100.0 + @pytest.mark.parametrize("cfg", [ + {"display": "nonsense"}, + {"display": ["nonsense"]}, + {"display": 60}, + {"display": {"hardware": "nonsense"}}, + {"display": {"hardware": ["nonsense"]}}, + {"display": {"hardware": 60}}, + ]) + def test_malformed_nesting_falls_back_rather_than_raising(self, cfg): + # Truthy-but-not-a-mapping at either level used to reach .get() on a + # str/list and raise AttributeError out of a function whose whole + # contract is "a refresh rate, or the default". + assert refresh_hz_from_config(cfg) == 100.0 + class TestCrispLadder: """Whole-pixel speeds available on a given panel.""" @@ -388,3 +402,66 @@ def set_frame_hold(self, refreshes): s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 50.0}}, display_manager=bare) assert s.frame_hold == 2, "assumed 100Hz" + + def test_reported_settings_describe_the_display_manager_rate(self): + """Without snapping, nothing downstream corrects a wrong refresh rate. + + The rate used to be read *after* resolve() had already filled in + target_fps, pixels_per_frame and the judder warning from the 100Hz + default -- so on a 60Hz panel every one of those described 100Hz, and + set_target_fps() paced the helper to 100 FPS. + """ + dm = self.DM(60.0) + helper = FakeHelper() + s = configure(helper, {"display_options": {"scroll_pixels_per_second": 30.0}}, + display_manager=dm, snap_to_crisp=False) + + assert s.target_fps == pytest.approx(60.0) + assert s.pixels_per_frame == pytest.approx(0.5), "30px/s over 60 frames" + assert helper.target_fps == pytest.approx(60.0) + + def test_judder_warning_is_computed_at_the_real_refresh_rate(self): + dm = self.DM(60.0) + # 60px/s is exactly 1px per refresh at 60Hz -- crisp, no warning. At + # the 100Hz default it is 0.6px per refresh and would be flagged. + s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 60.0}}, + display_manager=dm, snap_to_crisp=False) + assert s.warning is None, s.warning + + def test_global_config_supplies_the_rate_without_a_display_manager(self): + s = configure( + FakeHelper(), + {"display_options": {"scroll_pixels_per_second": 30.0}}, + {"display": {"hardware": {"limit_refresh_rate_hz": 60}}}, + snap_to_crisp=False, + ) + assert s.target_fps == pytest.approx(60.0) + + +class TestFrameHoldIsReportedNotApplied: + """configure() reports the hold; the caller applies it when it scrolls. + + The hold belongs to a scroll, not to a plugin's lifetime -- plugins share + one display manager, so one set at construction is reset the moment any + other plugin stops scrolling. + """ + + class RecordingDM: + refresh_hz = 100.0 + + def __init__(self): + self.calls = [] + + def set_scrolling_state(self, is_scrolling, frame_hold=1): + self.calls.append((is_scrolling, frame_hold)) + + def set_frame_hold(self, refreshes): + self.calls.append(("set_frame_hold", refreshes)) + + def test_configure_does_not_touch_the_display_manager(self): + dm = self.RecordingDM() + settings = configure( + FakeHelper(), {"display_options": {"scroll_pixels_per_second": 25.0}}, + display_manager=dm) + assert dm.calls == [], "configure() must not apply the hold itself" + assert settings.frame_hold == 4, "but it must report what to apply" diff --git a/test/test_scroll_helper.py b/test/test_scroll_helper.py index 517d3e47..f5290e86 100644 --- a/test/test_scroll_helper.py +++ b/test/test_scroll_helper.py @@ -11,7 +11,11 @@ from unittest.mock import patch from PIL import Image -from src.common.scroll_helper import ScrollHelper +from src.common.scroll_helper import ( + ScrollHelper, + format_frame_stats, + frame_stats, +) DISPLAY_W = 64 @@ -315,3 +319,77 @@ def test_scroll_position_reflected(self, helper): helper.scroll_position = 42.0 info = helper.get_scroll_info() assert info["scroll_position"] == 42.0 + + +class TestFrameStatsPercentiles: + """The stats line is the instrument this whole scroll change is measured + with, so its median and p95 have to be the real ones. + + Both are also thresholds: stalls are counted at 1.5x the median and skips + at 0.5x, so an off-by-one in the median biases the counts as well as the + printed numbers. + """ + + # 100 samples of 1..100ms. True median 50.5ms (the mean of the two middle + # samples, not the upper one at 51ms); nearest-rank p95 is the 95th + # sample at 95ms, not the 96th at 96ms. + HUNDRED = [i / 1000.0 for i in range(1, 101)] + + def test_even_window_median_averages_both_middle_samples(self): + assert frame_stats(self.HUNDRED)["median"] == pytest.approx(0.0505) + + def test_p95_uses_nearest_rank(self): + assert frame_stats(self.HUNDRED)["p95"] == pytest.approx(0.095) + + def test_odd_window_median_is_the_middle_sample(self): + times = [i / 1000.0 for i in range(1, 102)] # 101 samples + assert frame_stats(times)["median"] == pytest.approx(0.051) + + def test_single_sample_window_does_not_index_out_of_range(self): + stats = frame_stats([0.010]) + assert stats["median"] == pytest.approx(0.010) + assert stats["p95"] == pytest.approx(0.010) + assert stats["min"] == stats["max"] == pytest.approx(0.010) + + def test_two_sample_window(self): + stats = frame_stats([0.010, 0.020]) + assert stats["median"] == pytest.approx(0.015) + assert stats["p95"] == pytest.approx(0.020) + + def test_input_order_does_not_matter(self): + assert frame_stats(list(reversed(self.HUNDRED))) == frame_stats(self.HUNDRED) + + def test_caller_window_is_not_mutated(self): + times = [0.030, 0.010, 0.020] + frame_stats(times) + assert times == [0.030, 0.010, 0.020] + + def test_stall_threshold_follows_the_median(self): + # Ten 10ms frames and two 30ms stalls: median 10ms, so >15ms is a + # stall -- exactly the two. + stats = frame_stats([0.010] * 10 + [0.030] * 2) + assert stats["stalls"] == 2 + assert stats["skips"] == 0 + + def test_skips_are_frames_that_never_reached_the_panel(self): + stats = frame_stats([0.010] * 10 + [0.002] * 3) + assert stats["skips"] == 3 + assert stats["stalls"] == 0 + + def test_fps_is_the_reciprocal_of_the_mean(self): + assert frame_stats([0.010] * 50)["fps"] == pytest.approx(100.0) + + def test_formatted_line_reports_the_corrected_values(self): + line = format_frame_stats(self.HUNDRED) + assert "median 50.50ms" in line, line + assert "p95 95.00ms" in line, line + assert "over 100 frames" in line, line + + def test_log_frame_rate_emits_the_line_and_clears_the_window(self, helper): + helper._window = [0.010] * 20 + helper.last_fps_log_time = 0.0 # force the 5s boundary + with patch.object(helper.logger, "info") as info: + helper.log_frame_rate() + assert info.called + assert "Scroll frame stats" in info.call_args[0][0] + assert helper._window == [] From c17c58003ce0940acc06f7b160752d774220dc88 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Sun, 6 Sep 2026 18:41:42 -0400 Subject: [PATCH 7/7] test(harness): keep the visual double's signature tied to production Moves set_scrolling_state's frame_hold into the test double here, where DisplayManager gains it, rather than in #534 where it arrived a PR early. CodeRabbit flagged the #534 version correctly: a double that accepts an argument production does not lets the call pass every harness run and raise TypeError on the panel, which is the one failure a safety harness exists to prevent. The drift has now gone both ways across two branches -- double behind production on this branch, double ahead of it on #534 -- so it is pinned instead of remembered. test_display_double_parity.py compares the two signatures and fails with the direction of the drift named. It reads the files with ast rather than importing them, because display_manager imports rgbmatrix at module scope and this check should hold on a laptop and in CI as well as on a Pi. Plugins begin passing frame_hold in ledmatrix-plugins#462, which is why production and the double both need it before that lands. Full suite: 3889 passed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --- .../testing/visual_display_manager.py | 13 +++- test/test_display_double_parity.py | 69 +++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 test/test_display_double_parity.py diff --git a/src/plugin_system/testing/visual_display_manager.py b/src/plugin_system/testing/visual_display_manager.py index 5211a226..55bb99ea 100644 --- a/src/plugin_system/testing/visual_display_manager.py +++ b/src/plugin_system/testing/visual_display_manager.py @@ -503,9 +503,18 @@ def draw_text_with_icons(self, text: str, icons: List[tuple] = None, # Scrolling state (no-op interface compat) # ------------------------------------------------------------------ - def set_scrolling_state(self, is_scrolling: bool): - """Set the current scrolling state (no-op for testing).""" + def set_scrolling_state(self, is_scrolling: bool, frame_hold: int = 1): + """Set the current scrolling state (no-op for testing). + + ``frame_hold`` mirrors the DisplayManager signature this change adds. + The two are kept in step deliberately: a double that accepts arguments + production does not lets a call pass every harness run and then raise + TypeError on the panel, and a double that lacks one production has + fails every render of a plugin that legitimately paces its scroll. + Plugins begin passing it in ledmatrix-plugins#462. + """ self._scrolling_state['is_scrolling'] = is_scrolling + self._scrolling_state['frame_hold'] = frame_hold if is_scrolling: self._scrolling_state['last_scroll_activity'] = time.time() diff --git a/test/test_display_double_parity.py b/test/test_display_double_parity.py new file mode 100644 index 00000000..84506848 --- /dev/null +++ b/test/test_display_double_parity.py @@ -0,0 +1,69 @@ +"""The visual-test double must not drift from the real DisplayManager. + +Both directions of drift are silent and both are damaging: + + * the double accepts an argument production does not -- the call passes + every harness run and raises TypeError on the panel, which is precisely + the failure a safety harness exists to prevent; + * the double lacks an argument production has -- every plugin that + legitimately uses it fails every render, and the harness blames the + plugin. + +`set_scrolling_state` has been each of those in turn across two branches, so +the parity is asserted rather than remembered. + +Read with ast rather than imported: src/display_manager.py imports rgbmatrix +at module scope, which is absent anywhere without the panel library, and this +check should hold on a laptop and in CI as well as on a Pi. +""" + +import ast +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parent.parent +REAL = ROOT / "src" / "display_manager.py" +DOUBLE = ROOT / "src" / "plugin_system" / "testing" / "visual_display_manager.py" + +#: Methods a plugin calls on whichever manager it is handed. +SHARED_METHODS = ["set_scrolling_state", "is_currently_scrolling"] + + +def _signature(path: Path, class_hint: str, method: str): + """(name, default-repr) pairs for `method`, or None if it is absent.""" + tree = ast.parse(path.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef) or class_hint not in node.name: + continue + for item in node.body: + if isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)) and item.name == method: + args = [a.arg for a in item.args.args if a.arg != "self"] + pad = [None] * (len(args) - len(item.args.defaults)) + defaults = pad + [ast.unparse(d) for d in item.args.defaults] + return list(zip(args, defaults)) + return None + + +@pytest.mark.parametrize("method", SHARED_METHODS) +def test_the_double_matches_production(method): + real = _signature(REAL, "DisplayManager", method) + double = _signature(DOUBLE, "DisplayManager", method) + + assert real is not None, f"DisplayManager lost {method}" + assert double is not None, \ + f"the test double is missing {method}, so every plugin using it fails to render" + assert double == real, ( + f"{method} has drifted: production takes {real}, the double takes {double}. " + "A double that is more permissive hides a production TypeError; one that " + "is less permissive fails plugins that are actually correct." + ) + + +def test_frame_hold_is_accepted_by_both(): + """The specific argument that has drifted twice.""" + for path, label in ((REAL, "DisplayManager"), (DOUBLE, "VisualTestDisplayManager")): + params = dict(_signature(path, "DisplayManager", "set_scrolling_state") or []) + assert "frame_hold" in params, f"{label} does not accept frame_hold" + assert params["frame_hold"] == "1", \ + f"{label} must default frame_hold to 1 so existing callers are unaffected"