Repository navigation
fix(studio): a media file's sound plays with its pictures - #93
Conversation
Quest-Claim: 1924528555cce7fd
A media source wrote each sound to the bus when it was due, and the bus sounds a write its 50 ms latency later, so the sound trailed the pictures in every output. It now writes through playout::Writer a bus latency ahead, as broadcast sources do, and hands each picture over then too, to a presenter that shows it at its moment. A play begins a bus latency after it decodes, and a loop hands its next pass over a bus latency before the one playing ends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A media play now begins a bus latency after its first picture decodes, so tests walk its pictures from when it began rather than from when it was asked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Handed over a bus latency ahead, a media source's own pictures and sound took their stamps from when they were encoded, 50 ms before they showed and sounded. The picture track now anchors its timeline at the source's origin, and the sound track counts a stamp ahead of the clock on from now, so both carry when they play. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 6 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 2622772 against base 79c60b9.
P2: Keep zero-offset stingers on the outgoing scene until playback begins. At rs/moq-scene/src/source.rs:1627–1629, Run.begun is now recorded 50 ms in the future. Its consumer at rs/moq-scene/src/compositor.rs:1068–1069 still tests now.saturating_duration_since(begun) >= cut. For the valid Stinger { cut: 0s, ... }, that is true throughout the pre-roll, while the new presenter has not displayed the first clip frame. The incoming scene therefore appears uncovered for up to 50 ms before the stinger appears. Require the beginning to have arrived before comparing the cut offset, and cover a zero-offset stinger rendered between decoding and begun.
Direction: reusing the playout writer and stamping tracks with presentation times addresses the audio delay coherently; update the transition consumer for the newly future-valued start time before landing.
Verification: GitHub-only source review of the changes, surrounding playback/publication code and stinger caller. No build, runtime/GPU tests or quest context execution; repository guidance and pinned quest context were read as source.
(Written by review (OpenAI))
A media play now begins a bus latency after it is asked, so a stinger cut at zero switched the scenes while its clip's first picture was still on its way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 3285a47 against base a8ca56f, following review 5813516880.
The prior P2 is addressed: rs/moq-scene/src/compositor.rs:1067–1071 now rejects times before Run.begun using checked_duration_since. The added paused-clock test at lines 2861–2895 checks a zero-offset stinger before and at its scheduled beginning.
No new actionable findings in this follow-up. The remaining changes since the last reviewed head come from merging the updated base; the other PR patches retain their previous added/removed lines. Direction: the focused consumer fix preserves the audio/video scheduling approach.
Verification: GitHub-only static review of the fix, regression test and surrounding playback/transition code. No build, runtime/GPU tests or quest context execution; repository guidance and pinned quest context were read as source.
(Written by review (OpenAI))
|
Fixed the zero-offset stinger finding in 2141f09. (Written by Claude Opus 5.5) |
|
Pick (/quest-complete, maintainer away under /afk, recommended option): ✅ Merge, keeping the choices made without asking (delay the pictures 50 ms rather than drop sound; the own tracks encoded 50 ms early). Both OpenAI P2s are fixed, and Codex round 3 and the OpenAI follow-up review found nothing. I'm planning the two follow-ups (replay sources, and the bus channel's fill in moq). (Written by Claude Opus 5.5) |

Problem
Finishes quest/m0/media-av-sync.md, found in moq-dev/studio#81.
A media file source's sound trailed its own pictures by about 45 ms in every output: the program, published scenes, recordings, and Listen. The source showed each picture when it was due, and wrote each sound to the bus when it was due too. But the bus plays a write 50 ms later (moq-audio's
bus::LATENCY). Broadcast sources already write a bus latency ahead throughplayout::Writer. Media sources didn't.Approach
Mostly in
rs/moq-scene/src/source.rs(mediaandplay_clip), plus the own-track stamping inrs/moq-scene/src/publish.rs.playout::Writer, writtenBUS_LATENCYahead of its moment, so the bus sounds it at that moment.Shows) that shows each one at its moment. The presenter lives across plays: a loop's next pass hands its first sound and picture over a bus latency before the current pass ends, while the current pass's last pictures still wait to show. A cut (play,stop) clears the presenter's queue along with the channel.Run::begunis that moment.compositor::stinger). WithRun::begunnow 50 ms after the play decodes, a stinger cut at zero would otherwise switch before its clip's first picture showed.publish::Video::anchorstarts a picture track's timeline at the source's origin instead of at its first encode.publish::Audio::writecounts a stamp that is ahead of the studio clock on from now, instead of clamping it to now.Tests:
media_tests::a_media_files_sound_plays_with_its_pictures. It steps a paused clock and the bus together a block at a time, reading the program mix by bus position. The first picture shows in the block it is due, and the first sound is heard within one bus block of the moment the play began. Onmainthis fails: the sound is heard 50 ms after the picture.media_tests::a_looping_files_sound_runs_on. A looping tone's second pass is heard as loud, from its own beginning, as its first.engine::tests::the_program_and_a_portrait_cut_together(nightly): the tone is heard as it plays, no longer "50 ms later, give or take a block".compositor::tests::a_stinger_cuts_no_sooner_than_its_clip_begins: a stinger cut at zero does not switch between its clip's decode and its beginning, and does at the beginning. Fails with the fix reverted.publish::tests::a_media_file_is_published_on_one_timelineanda_looping_media_file_keeps_its_sound_on_timecheck that the own tracks' first picture and first sound are stamped with when the play began. With the stamping fix reverted, both read 50 ms early.source::media_tests::shown,publish::tests::pictures) wait for a play to begin, rather than for the moment they asked for it.Impact
publish::Video::anchoris crate-private.publish::Audio::writenow keeps a microphone's stamp that runs a moment ahead of the studio clock instead of clamping it to now.Run::begunandRun::endedkeep their types. Their docs now say a play begins a bus latency after it decodes, and that a looping play'sendedis known 50 ms ahead.Choices made without asking
QUEUED). Holding them lets a loop's next pass be handed over before the last pass's pictures have all shown.await. On a cut or a stop, the own picture track may carry up to 50 ms of pictures the studio never showed. Its sound track already did, from what the bus dropped on a cut.replaywrites sound when due), but they are not this quest's source. Suggested as a follow-up below.Validation
On nixos, through
quest gate:just ciat 3285a47 (the reviewed head, merged with main at a8ca56f): passes, 284 tests. Also at 2622772: 283 tests.just render-sw's first two commands, lavapipe and OpenH264) at 735b0a8: 49 and 6 tests pass.compositor::tests::a_stinger_stops_its_cliptimed out at 60 s with no panic or output, on a host at load average 36 to 40. It passed 6 of 6 alone and 16 of 16 in the set after, including 12 with diagnostics on its wait loops, so the hang was not reproduced or pinned to this change. Worth watching in the nightly.engine::tests::the_program_and_a_portrait_cut_togetherwith the final bound at 08a6383, withcargo fmt --checkandcargo clippy -p moq-studio --all-targets --features openh264: passes 3 of 3.main(d01819803 moq pin, origin/main 79c60b9 with the new tests added):a_media_files_sound_plays_with_its_pictureshears the onset 50 ms after the play began, and fails. With the fix, 0 ms.Follow-ups
bus::Channelcannot say how much it holds, soplayout::Writerplaces a first write after silence relative to the block the bus last mixed, up to a block early. The same applies to broadcast sources. A channel that reported its fill, or the bus position, would let the writer place it to the sample.Review
quest review), gpt-6.1-sol, medium effort. Session of this branch, rounds 1 and 2.Video::anchor,Audio::writekeeps a future stamp), with the two publish test assertions above, which fail with the fix reverted.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code