[kixelated]

Problem

The program encoder dropped what a pipelined backend still held at three points (#3 follow-up):

• On-demand pause: the gap marker went out without Encoder::flush, so held pictures surfaced after the gap, ahead of the next keyframe.
• Canvas resize: a live resize (#16) retires the rendition's encode thread, which finished the track without draining its encoder.
• Exit: nothing stopped the program clock or its encode threads, so the encoder and its track were dropped with the process: no drain, no clean finish.

Media Foundation, V4L2 and MediaCodec buffer. VideoToolbox, NVENC, VA-API and openh264 do not at the pinned moq (VideoToolbox calls complete_frames on every encode), so this Mac could not show the loss with a real encoder.

Approach

• moq_scene::publish::Encoding owns an encoder and its encode::Producer and drains before every boundary:
  • drain/open: publish finish() before replacing the encoder.
  • pause: publish flush(), queue a cut, then mark the discontinuity. The encoder and its session stay open.
  • discontinuity: drain, then mark the gap, so the next encoder opened starts with a keyframe even on a backend that cannot cut. A replay source's gap uses it, as on main.
  • finish/Drop: drain, then finish the track. A failed drain aborts it with the reason.
• Source Video and each program rendition's encode thread both use it, so there is one path, not two. The source's own drain and Drop move into it unchanged, and the program's paused flag goes away.
• A resize or quit ends a rendition by dropping its frame sender. Its encode thread encodes what was queued, then Encoding::finish drains and finishes the track. The next rendition's probe already waits for that thread, so the old encoder's tail also lands before the new session opens.
• Engine::stop sets a flag that the program clock checks each tick. The program thread then ends the current rendition, joins every retiring encode thread (including those an in-flight probe holds, by joining the probe), and returns. stop joins the program thread. main calls it after the save on quit.
• Encode is a four-method trait (encode, flush, finish, cut), implemented for moq_video::encode::Encoder. Its test double is moq-video's backend trait, which is crate-private.

Tests:

• publish::tests::a_pipelined_encoder_loses_no_pictures: a double wraps openh264 and holds two pictures. Read back through the container consumer, it shows every picture before the pause's end marker, the resumed pictures in their own group, and the tail before a clean finish. Mutation-checked: dropping the pause flush or the finish drain each fails it. Encoding::finish is exactly how a resize retires a rendition, so this covers that case too.
• engine::tests::the_program_reaches_a_relay (nightly, render-sw) now also stops the engine after the resize and checks that the current rendition's video track finishes cleanly at the relay with no output error. Main's part of the test already reads the retired rendition's track to a clean end. It passes locally on macOS (Metal and VideoToolbox).

Impact

• Quitting the studio now ends the program video track cleanly after the encoder drains, instead of dropping it.
• A live canvas resize publishes the old rendition's last pictures before its track ends.
• With --on-demand, a pipelined hardware encoder's last pictures before a pause land before the gap.

Choices made without asking

• No engine-level paused-clock test for the resize drain. The program's encode threads read Pending GPU copies, so a test of that wiring needs a GPU and a real network relay. Making the encode thread generic over its frame source only for a test did not seem worth its weight. The resize path calls the same Encoding::finish the pipelined test proves, and the GPU relay test covers the resize and the stop end to end.
• Replay gaps drain and reopen (Encoding::discontinuity) rather than use pause: a backend without forced keyframes would otherwise fail the replay track. The program keeps pause, since it holds its encode session while unwatched. Main already called cut() there.

Alternatives

• Draining inline in engine.rs: rejected. The quest asks for one path.
• A test-only pipelined backend in moq-video: needs an upstream PR, and moq-side quests are on hold.
• Polling the stop flag vs. a wake channel: the program clock already ticks every frame, so a flag adds at most one frame of latency.

Follow-ups

• Suggested quest: close the relay session gracefully at quit. stop drains and finishes the program video, but main then returns and drops the runtime, so the tail may not reach the relay. moq_tokio::Connection::close drains, bounded at one second, but first needs every track finished: program audio and each source's track (source threads are not stopped at exit). Raised by the OpenAI review; answered in this comment.

Review

Codex (quest review), gpt-6.1-sol, medium effort, against origin/main (1bc0394).

• Round 1 on 12ba5b946eea583ec5836d79632b6a6831b4b491: one finding.
  • P2 publish.rs:427, a replay gap went through pause, whose cut() fails on a backend without forced keyframes: fixed in 70c1efe (Encoding::discontinuity).
• Round 2 on 70c1efe67b2fe79186d185835c01a80d79b39f3a: no findings.

Online: pullfrog found no issues on b33ff61. The OpenAI review's relay-drain finding is declined above as the follow-up.

Local: just ci passed (137 tests). The ignored relay test passed on macOS (Metal and VideoToolbox).

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

[coderabbitai]

Warning

Review limit reached

You'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 44 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 86aa0b92-a44f-4c93-a88b-bdb07caeb37f

Commits

Reviewing files that changed from the base of the PR and between 6510d9e and 6a03546.

Files selected for processing: 5

• quest/m0/README.md
• quest/m0/program-drain.md
• rs/moq-scene/src/publish.rs
• rs/moq-studio/src/engine.rs
• rs/moq-studio/src/main.rs

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed head b33ff61 against base 587cc9f.

P2: Drain the relay connection before exiting (rs/moq-studio/src/main.rs:106–109, rs/moq-studio/src/engine.rs:378–387). stop() joins the encode thread after it drains into the local producer, but it does not wait for the relay session to send that tail. Main then returns and drops the Tokio runtime, so frames queued behind the network and the clean track end can still be lost when the user quits. The new relay test reads to EOF while its runtime remains alive, so it does not cover the actual quit boundary. Finish the program audio and source tracks as well, retain and close the connection through its graceful drain before tearing down the runtime, and test exit with a delayed relay reader. This completes the quit guarantee rather than leaving the transport half to a separate follow-up.

Static GitHub-only review of the encoder, program shutdown and pinned transport close path. I did not run GPU, encoder, relay or shutdown tests.

(Written by review (OpenAI))

[pullfrog]

✅ No new issues found.

Reviewed changes

• Shared Encoding in moq-scene: a new Encode trait plus an Encoding<E> owner drains the encoder before it is replaced, publishes flush() before a pause's discontinuity and gap, and drains before finishing (or aborts with the reason) on end and Drop.
• Source Video delegation: the source drops its own encoder/drain/Drop and uses Encoding, keeping the size/color reopen path unchanged.
• Program encode thread: builds an Encoding, routes Video::Pause through track.pause(), and drains then finishes on the sender ending; the manual paused/cut-on-next-frame logic is gone.
• Exit path: Engine::stop flips a stop flag that ends the program clock, then joins the encode thread so the track finishes; main calls it after the quit save, and watch no longer reports the deliberate end as an output failure.
• Tests: a pipelined double over openh264 proves no pictures cross a pause gap or a clean finish; the nightly relay test stops the engine and asserts the track ends with no output error.
• Quest bookkeeping: quest/m0/program-drain.md and its README row are removed with no dangling references.

I verified the drain/flush/cut contracts against the pinned moq (96244dd): Encoder::flush leaves the encoder usable, cut applies to the next frame, and Producer::finish borrows so the abort path still runs. The stop join cannot deadlock because the program clock runs on its own clocked thread, and the sender-drop chain carries the stop ordering to watch. The relay-session close and the canvas-resize rendition end are recorded as follow-ups in the PR body and were not re-raised.

  ^{｜ View workflow run ｜ Using big-pickle (free) ｜ 𝕏}

[kixelated]

Re the OpenAI review's P2, "Drain the relay connection before exiting": declined for this PR, kept as the follow-up in the description.

This PR's guarantee ends at the producer: every picture the encoder was handed, plus what it holds, is published, and the track finishes rather than dropping. Getting that tail across the network at quit is a different fix. moq_tokio::Connection::close drains only once every track has finished, and today neither the program audio nor any source track finishes at exit, since source threads are never stopped. So it needs a shutdown order for audio, every source, and the connections, all of which Output holds. None of that touches the encoder path here, and putting it in this PR would mix transport lifecycle into an encoder fix.

As of 70c1efe the same drain also covers the rendition a live canvas resize retires (#16): each rendition's encode thread ends through Encoding::finish.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed head 70c1efe.

No additional actionable issue found in the resize integration or replay-gap change. The replay path drains and reopens the encoder at a gap, preserving the previous behavior for backends that cannot force a keyframe. The earlier relay-drain finding remains deferred under the scope explained in the author response, rather than fixed here.

Verification: GitHub-only static inspection of the encoder, resize and shutdown paths. I did not run quest context, tests, GPU or relay execution.

(Written by review (OpenAI))

[pullfrog]

✅ No new issues found.

Reviewed changes

Changes since the prior pullfrog review (b33ff61): two merges of main (live-resize renditions, clips, health grading, browser sources) and the replay-gap fix.

• Stop joins the program clock: Engine::stop sets the stop flag and joins the program thread, which on its next tick ends the current rendition, joins every retiring encode thread, and joins an in-flight probe. clocked now returns its JoinHandle to make that join possible.
• Encode thread uses Encoding: encode wraps the producer in moq_scene::publish::Encoding, opens the encoder, drains through finish, and aborts with the failure reason; main's stats.fail_video/opened_encoder accounting is kept.
• Replay gaps drain and reopen: new Encoding::discontinuity drains the encoder before marking the gap, so a replay gap no longer goes through pause's cut and fails on a backend without forced keyframes.
• Relay test follows the resize: the nightly test now stops the engine, reads the resized rendition's track to its end, and asserts stats.error() stays empty.

The merge resolution keeps the PR's guarantee intact: a rendition retired by a resize is joined by the next probe before it opens, and at quit the program thread joins both the current and the retired encode threads. Producer::publish/finish are synchronous into the local origin, so those joins do not depend on the tokio runtime draining the relay.

  ^{｜ View workflow run ｜ Using big-pickle (free) ｜ 𝕏}