Conversation
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:
|
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Re-reviewed at ace00984. The log line and the duration: int tightening both look right, and the expanded description now covers the video-tail trade and the teardown caveat I raised — thanks for writing those up rather than just dropping them in a comment.
One correction, though: the mitigation reasoning in the new Not covered here paragraph is wrong, and it matters more than it looks. I measured it on a Pi 2.
The teardown is not absorbed by the next tick
The description says:
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.
media_player.stop() is the last statement of view_video, so it runs inside tick N, after the wait and before asset_loop returns. start_loop then calls asset_loop again, and tick N+1's tick_start = monotonic() is taken after that return. The teardown therefore falls between tick N's deadline and tick N+1's anchor — it is inside no deadline at all, and it is added to the cycle on every video rotation rather than being absorbed by anything.
Measured on a Pi 2 (armhf, Qt5 linuxfb, so a real GstFbdevMediaPlayer.stop()), same overlay method as before — the board's pinned image carried a viewer file byte-identical to this PR's base, so this is a clean single-PR delta:
| segment | base (17e30b91) |
this PR (ace00984) |
|---|---|---|
| image (10s) | +12.3ms | +2.1ms |
| web (10s) | +55.2ms | +2.1ms |
| video (20s) | +336.8ms | +312.9ms |
| cycle drift | 36.4 s/hour | 28.5 s/hour |
n=6–7 segments per cell. Image and web collapse to +2.1ms — the fix does exactly what it promises there, and on armhf the win is far bigger than the x86 numbers I posted earlier. Video barely moves: only the ~24ms of pre-play overhead comes off, and 313ms of teardown stays on every rotation.
The decisive bit is the row underneath: if the teardown really were absorbed by the following deadline, the image slot after a video would come up ~313ms short. It doesn't — it's 10.0021s, the same as an image following a web page. The cost is added to the cycle, not borrowed from the next slot.
So on a mixed playlist this board still drifts 28.5 s/hour after the change, down from 36.4. Real improvement, but ~22% rather than the ~94% the x86 measurement suggested, because video dominates the residual. A video-only playlist on armhf would keep essentially all of its drift.
Suggested fix: just correct that paragraph. The code is fine as-is and I don't think this should block the merge — the teardown is pre-existing and predates the PR. But as written the paragraph tells a reviewer the problem is self-correcting when it isn't, and that's the sentence someone will rely on when they decide not to pick up the follow-up. If it helps, 313ms on a Pi 2 is a starting baseline for whoever does.
Verification
The branch is 7 commits behind master; I tested the merge, which is clean.
uv run pytest -m "not integration"— 2166 passed, 3 skippeduv run ruff check ./ruff format --check .— clean, 255 filesuv run mypy .— no issues in 202 source files- On device: no tracebacks or errors across the run, and the new line renders as
Playing video for 20.0 of 20s(it also showed19.9 of 20son one tick, which is the clamp being visible exactly as intended)
The new INFO line adds one log line per video rotation. Given #3268's journal budget I looked at whether that's a problem — it isn't: it fires at rotation rate, the same cadence as the sibling Showing asset and Sleeping for lines, not on an idle spin.
Nothing else changed in scope: one commit, one file, +6/−4, no new imports and nothing outside the viewer.
Still looks good to merge from my side once that paragraph is amended; the formal approval stays with a maintainer.
|
Thanks for the correction and the Pi 2 measurements in the latest review. You're right: I've corrected the original PR description's Not covered here paragraph, added a reference to your measurements, and revised the other timing claims so they no longer imply exact cycle timing or removal of all accumulating drift. Accounting for teardown remains a separate follow-up requiring regression coverage and device validation. No code changes were made. |
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Checked the revised description — the correction is accurate and the surrounding claims now hold up. Nothing further from me.
Verified against the code rather than just read:
- Not covered here now states the mechanism correctly: teardown finishes before
asset_loopreturns, the next tick anchorstick_startafter that return, so the cost adds to every video rotation and isn't absorbed. That matches both the call order and the measurement. GstFbdevMediaPlayer.stop()really is akillpgplus up to two 3 s waits —SIGTERM+wait(timeout=3), thenSIGKILL+wait(timeout=3)on the timeout path (src/anthias_viewer/media_player.py:855).- "Master already truncates a clip's fractional tail through
clamp_duration'sint()" checks out: the serializer storesclamp_duration(video_duration.total_seconds())at upload (src/anthias_server/api/serializers/mixins.py:219), andclamp_durationis anint()(src/anthias_server/app/models.py:171). So the trade really is an extension of a cut that already exists. - The 313 ms and 36.4 → 28.5 s/hour figures match what I measured.
- The closing paragraph no longer claims exact cycle timing, and the Issues Fixed line now scopes the change to pre-wait work. Both accurate.
Code is byte-identical to ace00984, which I reviewed last time — I re-ran the checks anyway since master has moved 15 commits since. None of them touch src/anthias_viewer/ or tests/test_viewer.py, and the merge is clean:
uv run pytest -m "not integration"— 2208 passed, 3 skippeduv run ruff check ./ruff format --check .— clean, 255 filesuv run mypy .— no issues in 202 source files
One loose end for whoever merges: the two device-testing boxes in the checklist are still unticked, correctly so, since the author didn't run them. The validation has been done though — x86 and a Pi 2, numbers in the two reviews above — so if you want those boxes ticked, the evidence is in the thread rather than missing.
Good to merge from my side.
|
@DiasonD Approved, but please sign the commits. I can't merge it w/out that. |
…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
- 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
ace0098 to
9d48599
Compare
|
Commits are now signed (SSH signature, both show as Verified). Code is unchanged from the approved revision, only the hashes moved. |
|
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 reduces the accumulating drift caused by work before each asset's wait; video teardown remains outside the deadline.
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: reducing drift from pre-play overhead at the cost of 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. Teardown finishes beforeasset_loopreturns, and the next tick recordstick_startonly after that return. Its cost therefore adds to every video rotation and is not absorbed by the following asset's deadline. In the reviewer's Pi 2 measurements, approximately 313 ms of video overhead remained per rotation; the measured mixed playlist's drift fell from 36.4 to 28.5 s/hour. This behavior predates the PR. Accounting for teardown is a separate timing change that needs regression coverage for a slowstop()and device validation.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 subtracts work before the wait from the asset's wait budget, reducing accumulated drift. It does not guarantee exact cycle timing: video teardown, work that exceeds an asset's duration, and scheduling delays can still extend the cycle.
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