diff --git a/src/display_manager.py b/src/display_manager.py index 7a33b1ff..6dfc42c2 100644 --- a/src/display_manager.py +++ b/src/display_manager.py @@ -199,6 +199,10 @@ class DisplayManager: self._snapshot_cond = threading.Condition() self._snapshot_pending: Optional[Image.Image] = None self._snapshot_thread: Optional[threading.Thread] = None + self._snapshot_stop = False + # Held for the whole of each PNG write, by the writer thread and by + # the inline static path, so the two land on disk in order. + self._snapshot_write_lock = threading.Lock() self._viewer_check_ts = 0.0 self._viewer_fresh = False self._viewer_was_fresh = False @@ -1274,6 +1278,8 @@ class DisplayManager: def cleanup(self): """Clean up resources.""" + if hasattr(self, '_snapshot_cond'): + self._stop_snapshot_writer() if hasattr(self, 'matrix') and self.matrix is not None: try: self.matrix.Clear() @@ -1611,7 +1617,12 @@ class DisplayManager: if self.is_currently_scrolling(): self._queue_snapshot(self.image.copy()) else: - self._save_snapshot(self.image) + # A scroll that just ended can leave its last frame queued or + # mid-write; it must not land on top of this newer one. + with self._snapshot_write_lock: + with self._snapshot_cond: + self._snapshot_pending = None + self._save_snapshot(self.image) self._last_snapshot_ts = now self._last_snapshot_touch_ts = now self._last_snapshot_digest = digest @@ -1696,10 +1707,35 @@ class DisplayManager: def _snapshot_writer(self) -> None: while True: with self._snapshot_cond: - while self._snapshot_pending is None: + while self._snapshot_pending is None and not self._snapshot_stop: self._snapshot_cond.wait() - image, self._snapshot_pending = self._snapshot_pending, None - try: - self._save_snapshot(image) - except Exception as e: - self._log_snapshot_failure(e) \ No newline at end of file + if self._snapshot_stop: + return # shutting down: a pending frame is dropped + # The write lock before the frame: whichever of this and an inline + # static save gets it first also writes first, and a static save + # clears the slot, so an older frame never lands on a newer one. + with self._snapshot_write_lock: + with self._snapshot_cond: + image, self._snapshot_pending = self._snapshot_pending, None + if image is None: + continue + try: + self._save_snapshot(image) + except Exception as e: + # The frame was recorded as written when it was queued. + # Forget that, so an unchanged frame is written again + # rather than only mtime-touching a stale file into + # looking healthy. + self._last_snapshot_digest = None + self._log_snapshot_failure(e) + + def _stop_snapshot_writer(self, timeout: float = 1.0) -> None: + """Stop the writer thread, dropping any frame it has not started.""" + with self._snapshot_cond: + self._snapshot_stop = True + self._snapshot_pending = None + self._snapshot_cond.notify_all() + thread = self._snapshot_thread + if thread is not None and thread is not threading.current_thread(): + thread.join(timeout) + self._snapshot_thread = None \ No newline at end of file diff --git a/src/vegas_mode/render_pipeline.py b/src/vegas_mode/render_pipeline.py index d1af0c79..7bb0ca12 100644 --- a/src/vegas_mode/render_pipeline.py +++ b/src/vegas_mode/render_pipeline.py @@ -137,6 +137,8 @@ class RenderPipeline: # a 95Hz panel, p99 21-28ms -- a visible hitch every few frames. self._crisp = None self._frame_hold = 1 + # Gaps timed under the old hold would be divided by the new one. + self._swap_times.clear() if self.config.smooth_scroll and not self.config.sub_pixel_blend: self._crisp = solve_crisp(self.config.scroll_speed, self._refresh_hz()) self._frame_hold = self._crisp.frame_hold @@ -197,10 +199,15 @@ class RenderPipeline: refresh the panel never delivers -- 90px/s at "120Hz" is 3px every 4 refreshes, visibly jumpy, where the real 95Hz allows 1px every refresh. - SwapOnVSync blocks for frame_hold refreshes, so the median gap between - swaps is frame_hold refresh periods. The median ignores the odd frame - that missed its vsync or waited on a recompose. Measured once: the - refresh only changes with the hardware config, which restarts us. + SwapOnVSync blocks for frame_hold refreshes, and a swap can only come + back late -- a missed vsync lengthens its gap by whole refreshes, never + shortens one -- so the low end of the gaps is frame_hold refresh + periods: the 10th percentile, as src/common/frame_timing.py uses. The + median would track the render loop instead once most frames in the + window were late (startup, a prefetch, a recompose), lock in a rate + too low, and scroll faster than configured until restart. Measured + once: the refresh only changes with the hardware config, which + restarts us. """ if self._crisp is None or self._measured_hz is not None: return @@ -214,11 +221,11 @@ class RenderPipeline: times = list(self._swap_times) gaps = sorted(b - a for a, b in zip(times, times[1:])) - median = gaps[len(gaps) // 2] + period = gaps[len(gaps) // 10] self._swap_times.clear() - if median <= 0: + if period <= 0: return - measured = self._frame_hold / median + measured = self._frame_hold / period cap = self._cap_hz() if measured >= cap * (1.0 - self.REFRESH_TOLERANCE): self._measured_hz = cap diff --git a/test/test_display_dirty_tracking.py b/test/test_display_dirty_tracking.py index 954637bb..c431430a 100644 --- a/test/test_display_dirty_tracking.py +++ b/test/test_display_dirty_tracking.py @@ -399,6 +399,74 @@ class TestSnapshotOffRenderThread: assert threads[0] != threading.current_thread().name assert os.path.exists(dm._snapshot_path) + def test_a_failed_background_write_is_retried_not_touched( + self, dm, tmp_path, monkeypatch): + # Queuing records the frame as written. If the writer then fails, an + # unchanged frame must be written again, not mtime-touched: touching + # would make a stale preview look healthy. + import threading + import time + failed = threading.Event() + + def failing(image): + failed.set() + raise OSError("disk full") + + monkeypatch.setattr(dm, "_save_snapshot", failing) + self._due(dm, tmp_path, (0, 255, 0)) + dm.set_scrolling_state(True) + try: + dm.update_display() + assert failed.wait(5) + deadline = time.time() + 5 + while dm._last_snapshot_digest is not None and time.time() < deadline: + time.sleep(0.01) + finally: + dm.set_scrolling_state(False) + assert dm._last_snapshot_digest is None + + def test_a_static_frame_lands_after_a_queued_one_still_being_written( + self, dm, tmp_path, monkeypatch): + # The last frame of a scroll can still be encoding when the first + # static frame is due; the older one must not land on top. + import threading + written, started, release = [], threading.Event(), threading.Event() + real = dm._save_snapshot + + def slow_then_record(image): + if threading.current_thread().name == "snapshot-writer": + started.set() + release.wait(5) + written.append((threading.current_thread().name, image.getpixel((0, 0)))) + real(image) + + monkeypatch.setattr(dm, "_save_snapshot", slow_then_record) + self._due(dm, tmp_path, (0, 0, 255)) + dm.set_scrolling_state(True) + dm.update_display() # queued: the writer blocks mid-write + assert started.wait(5) + dm.set_scrolling_state(False) + self._due(dm, tmp_path, (255, 0, 0)) + static = threading.Thread(target=dm.update_display) + static.start() + static.join(0.2) + assert static.is_alive(), "the static save must wait for the write in flight" + release.set() + static.join(5) + assert [colour for _, colour in written] == [(0, 0, 255), (255, 0, 0)] + + def test_cleanup_stops_the_writer(self, dm, tmp_path, monkeypatch): + threads, done = self._record_saves(dm, monkeypatch) + self._due(dm, tmp_path, (0, 255, 255)) + dm.set_scrolling_state(True) + dm.update_display() + assert done.wait(5) + writer = dm._snapshot_thread + dm.set_scrolling_state(False) + dm._stop_snapshot_writer() + writer.join(2) + assert not writer.is_alive() + def test_static_frames_are_still_written_inline( self, dm, tmp_path, monkeypatch): import threading diff --git a/test/test_vegas_crisp_pacing.py b/test/test_vegas_crisp_pacing.py index 696754a3..c5b87005 100644 --- a/test/test_vegas_crisp_pacing.py +++ b/test/test_vegas_crisp_pacing.py @@ -9,6 +9,7 @@ import sys from pathlib import Path from unittest.mock import patch +import pytest from PIL import Image sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) @@ -122,6 +123,31 @@ def test_a_panel_that_keeps_up_with_its_cap_is_left_alone(): assert p._crisp == crisp +def test_a_window_of_mostly_late_frames_still_measures_the_panel(): + # A late swap only lengthens its gap, by whole refreshes. A window where + # most frames missed a vsync (startup, a prefetch) must not read as a + # slower panel: with 7 frames in 10 a refresh late, the median would say + # 76Hz here and the speed would be solved for a panel that isn't there. + p = _pipeline(FakeDM(refresh_hz=120.0), scroll_speed=90) + period, hold = 1 / 95.0, p._frame_hold + gaps = [(hold + 1) * period if i % 10 < 7 else hold * period for i in range(400)] + clock = [1000.0] + with patch.object(rp_module.time, 'monotonic', lambda: clock[0]): + for gap in gaps: + p.render_frame() + clock[0] += gap + if p._measured_hz is not None: + break + assert p._measured_hz == pytest.approx(95.0) + + +def test_re_solving_the_pacing_drops_samples_timed_under_the_old_hold(): + p = _pipeline(FakeDM(refresh_hz=120.0), scroll_speed=90) + p._swap_times.extend([1.0, 1.01, 1.02]) + p._configure_scroll_helper() + assert len(p._swap_times) == 0 + + def test_no_measurement_without_hardware(): # Nothing blocks in the emulator, so swap gaps say nothing about a panel. p = _pipeline(FakeDM(refresh_hz=120.0, hardware=False), scroll_speed=90)