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 @@ Handles persistent disk-based caching with atomic writes and error recovery. """ 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 @@ if orjson is not None: 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 + + # Refresh rate, most authoritative first: what the caller passed, then the + # display manager (which can see display.hardware; a plugin cannot), then + # 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)) + 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=refresh_hz, + refresh_hz=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 @@ -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 @@ Features: """ import logging +import math import time from typing import Optional, Dict, Any from PIL import Image @@ -29,6 +30,49 @@ except ImportError: 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 @@ class ScrollHelper: # 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 @@ class TestRefreshFromConfig: 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 @@ class TestRefreshFromDisplayManager: 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 @@ import time 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 @@ class TestGetScrollInfo: 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 == []