Conversation
…duration The image/web wait and the video wait were both a flat `duration` on top of everything the tick did first (scheduler refresh, DB reads, the D-Bus load call), so every rotation ran `duration` plus a device- specific overhead. Co-located players cycling the same playlist drifted apart by seconds within hours. - Take `tick_start` as the first statement of `asset_loop` and wait until `tick_start + duration` in both wait sites via `_seconds_until` - Clamp at zero so a mid-tick browser respawn just moves on - Keep the configured integer duration in the "Sleeping for" log line - Tests drive `asset_loop` on a fake monotonic clock for image and video, plus the zero clamp; pin `monotonic` in the max-duration test Claude-Session: https://claude.ai/code/session_015NGKXmXtrjLX2M3Uv7CaKy
There was a problem hiding this comment.
🔵 Needs a closer look
Video teardown can still add substantial rotation drift; its cost needs to be accounted for with regression coverage.
Pull request overview
This PR anchors per-asset waits to each tick’s monotonic start to reduce playlist drift.
Changes:
- Adds deadline-based waits for image, webpage, and video assets.
- Clamps overdue waits to zero.
- Adds fake-clock timing regression tests.
File summaries
| File | Summary |
|---|---|
tests/test_viewer.py |
Tests deadline timing, video behavior, and zero-duration clamping. |
src/anthias_viewer/__init__.py |
Implements tick-anchored asset timing and shared remaining-time waits. |
Review details
Suppressed comments (1)
src/anthias_viewer/init.py:1704
- This deadline only covers the wait;
view_videostill callsmedia_player.stop()after the wait returns. In particular,GstFbdevMediaPlayer.stop()can block up to 3 seconds waiting for SIGTERM and another 3 seconds after SIGKILL, and the nextasset_looptick cannot start until that teardown completes. Video playlists can therefore retain substantial per-rotation drift despite this change; the teardown needs to be accounted for or moved in the timing design, with a regression test for its cost.
if skip_event.wait(timeout=_seconds_until(deadline)):
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis is correct, the fix is minimal and well-targeted, and the three new tests are genuine regression tests (I confirmed each one fails against master's viewer and passes with the change). I ran the suite locally and also measured the change on real hardware, since the device-testing boxes are unticked. Details below; three non-blocking comments at the end.
Local verification
Branch is current with master (17e30b91).
uv run pytest -m "not integration"— 2058 passed, 3 skippeduv run ruff check .— cleanuv run ruff format --check .— 249 files already formatteduv run mypy .— no issues in 196 source files
Hardware measurement (x86 testbed)
The board's pinned image carried a viewer file byte-identical to origin/master, so the baseline is exactly master; I then overlaid only src/anthias_viewer/__init__.py extracted from commit 911152de (md5 verified in-container) and re-measured the same playlist. Three assets — video 20s, image 10s, web 10s — timed from Showing asset log timestamps.
| segment | master | this PR | drift/hour, before → after |
|---|---|---|---|
| image (10s) | 10.0152s (+15.2ms) | 10.0017s (+1.7ms) | 5.5s → 0.6s |
| web (10s) | 10.0221s (+22.1ms) | 10.0007s (+0.7ms) | 8.0s → 0.3s |
| video (20s) | 20.0410s (+41.0ms) | 20.0025s (+2.5ms) | 7.4s → 0.5s |
| full cycle (40s) | 40.0784s (+78.4ms) | 40.0049s (+4.9ms) | — |
n = 9–12 segments per cell. Per-cycle overhead drops ~94%, which is the accumulating component the PR set out to remove. No tracebacks or behaviour changes in the viewer log across the run, and skip still works at both wait sites under the new deadline — assets/control/next was honoured in 20ms during video playback and 9ms during an image.
Note this board is close to the best case: x86 on Wayland with the QtMultimedia player. The absorbed overhead is device-specific and larger on the SBCs — this board alone emits a wlr-randr could not apply rotation retry at the top of every tick, which is exactly the kind of work the anchor now soaks up.
Comments (none blocking)
1. The video path still carries unabsorbed per-cycle overhead. view_video calls media_player.stop() after the deadline wait, so that cost falls outside the deadline and still accumulates. It shows up in the numbers above: video's residual (+2.5ms) is the largest of the three despite having twice the nominal slot. On x86 that's a cheap D-Bus stopVideo, but GstFbdevMediaPlayer.stop() on the Qt5 linuxfb boards is killpg + wait(timeout=3), with a second 3s budget after SIGKILL — a process teardown that can dwarf everything this PR absorbs. Copilot's review flagged the same thing independently. Not a regression (master has it too), and I don't think it should hold up the merge, but it does mean "the rotation takes exactly duration" holds for image/web and only approximately for video. The PR's own video test can't catch it because the player is mocked.
2. For video, duration is the clip length, so the deadline trims the clip's tail. AssetCreationMixin rejects a caller-supplied duration for video and sets it from get_video_duration (src/anthias_server/api/serializers/mixins.py:210), so a video slot is exactly one pass of the clip. The rationale in the description — the D-Bus load call returns before rendering, so the flat wait never bought on-screen time — holds for loadImage/loadPage, but not for playVideo, where the wait is the playback window. Absorbing the pre-play() work therefore cuts the last N ms off every clip: tens of ms on this board, proportionally more on a slow board or when a browser respawn lands mid-tick. I think it's the right trade, but it's a user-visible semantic change worth stating explicitly in the description.
3. _seconds_until's docstring over-claims on observability. It justifies not logging the zero-clamp on the grounds that the caller's Sleeping for 0.0 line already shows it. That's true for image/web, but view_video has no such line — it logs only Displaying video %s for %s at debug level, and with the configured duration rather than the computed wait. A video asset whose wait clamps to zero therefore leaves no diagnostic at all at the default log level. Either soften the docstring or give view_video an equivalent line.
Nit: view_video's duration: int | str annotation is now stale — the value only feeds logging and set_asset, and the sole caller passes a clamp_duration-ed int.
One thing I could not exercise on hardware: the zero-clamp path, where tick overhead exceeds a short duration. No device reproduced it naturally, so it rests on the new unit test.
Leaving the formal approval to a maintainer, but from my side this looks good to merge.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3328 +/- ##
=========================================
Coverage ? 90.47%
=========================================
Files ? 85
Lines ? 10075
Branches ? 1126
=========================================
Hits ? 9115
Misses ? 707
Partials ? 253 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- view_video now logs the deadline-clamped wait next to the configured duration, matching the image/web "Sleeping for" line, so a zero-clamp during video playback is visible at the default log level - drop the stale `int | str` annotation on view_video; the only caller passes a clamped int - _seconds_until docstring no longer claims the image/web log line covers the video path Claude-Session: https://claude.ai/code/session_01Gos7GxdfJQkjapBvmXzGQS
|
Issues Fixed
Related to the discussion in issue 3319 (wall-clock playlist alignment). This does not implement that feature and does not close the issue. It fixes the accumulating part of the drift described there.
Description
asset_loopwaited a flatdurationafter everything the tick did first (scheduler refresh, DB reads, the D-Bus load call), so every rotation tookdurationplus a device-specific overhead. Two players cycling the same playlist drifted apart by seconds within a few hours. The docs already promise an asset "is displayed for the specified duration", and the D-BusloadImage/loadPagecalls return before rendering, so the flat wait never bought on-screen time; it only lengthened the cycle.tick_start = monotonic()is now the first statement ofasset_loop, and both wait sites (image/web inasset_loop, video inview_video) wait untiltick_start + durationthrough one helper,_seconds_until.Video semantics. For a video asset,
durationis the clip length probed at upload, so the wait is the playback window rather than a display timer. Anchoring the wait on the tick start therefore trims the tick's pre-play overhead off the end of every clip: a few tens of ms on x86, proportionally more on a slow board or when a browser respawn lands mid-tick. Master already truncates a clip's fractional tail throughclamp_duration'sint(), so this is a small extension of an existing cut. It is the intended trade: a fixed cycle time for the playlist over the last few frames of a clip.Not covered here.
view_videostill callsmedia_player.stop()after the wait returns, so player teardown falls outside the deadline. On x86 that is a cheap D-BusstopVideo; on the Qt5 linuxfb boardsGstFbdevMediaPlayer.stop()is akillpgplus up to two 3 s waits. That cost lands in the next tick, where the next asset's deadline absorbs it, so it only leaks into the cycle when teardown exceeds the following slot. Not a regression against master. Moving or budgeting the teardown needs a Pi 3 measurement and is a separate change.What this does not do: it does not phase-align players (each still starts its counter wherever it was at boot) and it does not remove the residual crystal-rate drift between devices. It makes the cycle time equal to
durationinstead ofdurationplus overhead.Tests: three new cases drive
asset_loopon a fake monotonic clock (image, video, and the zero clamp), and the existing max-duration test pinsmonotonicso its exact-timeout assertion stays valid.ruff check,ruff format --check,mypyand the viewer test files pass.Checklist
https://claude.ai/code/session_01Gos7GxdfJQkjapBvmXzGQS