[kixelated]

Summary

A video encoder's groups now stay within encode::Config::join when the source runs slower than Config::framerate, such as a camera dropping to 20 fps in low light. Before, every backend keyed every join's worth of frames at the configured rate, so a 2 s join at 30 fps stretched to 3 s on a 20 fps source, and moq-mux warned that the track's groups were longer than the declared interval (HLS lists them as overruns). Found by moq-dev/studio#70.

Encoder::encode now forces a keyframe, through the same cut flag Encoder::cut and capture's Cuts already use, on the first frame whose distance from the last keyframe, counted in frames at the configured rate and rounded to the nearest, reaches Config::keyframe_interval():

• A source at the configured rate keys on the same frame as the backend's own count, whatever its timestamp jitter (up to a quarter frame each side), so nominal streams are unchanged.
• A slower source keys every join, overshooting by less than one of its own frames.
• A faster source reaches the backend's count first. Every keyframe in the encoder's output, including ones the backend placed itself, restarts the clock, so the encoder adds none in between.
• A timestamp behind the last keyframe starts a new timeline, measured from there.
• A backend that cannot cut (V4L2 without the force-keyframe control) is never asked to; its groups stay on the frame count, as Encoder::cut's CutUnsupported docs now say.
• VideoToolbox carries a cut whose frame it dropped (no packet, no error) to the next frame, as MediaCodec already does, so a dropped cut costs one frame instead of a whole join. This also fixes a caller's Encoder::cut lost the same way. openh264 needs no carry: under a starved 2 kbps rate it skipped 269 of 300 frames, but never one with a forced IDR. NVENC and VA-API read each frame back synchronously.

Docs updated: Config::join, Config::keyframe_interval, Encoder::cut, Backend::encode (which now requires a dropped frame's cut to carry over), and the moq-video README.

Choices made without asking

• Backend GOP stays, at the configured frame count (the quest left "ceiling beyond join, or goes" to the PR). It keeps every backend's configuration untouched, keeps faster sources keying on the count as today (shorter groups, never longer), and is the only placer on a backend that cannot cut. Rejected: an infinite backend GOP, which would make the encoder the sole placer but changes per-API semantics (AVEncMPVGOPSize, MediaCodec's negative i-frame-interval) on platforms this PR cannot test; and a ceiling at some multiple of the interval, an arbitrary factor that buys nothing over the count.
• No VideoToolbox kVTCompressionPropertyKey_MaxKeyFrameIntervalDuration. The encoder's time cut already covers VideoToolbox through the forced-keyframe path; a second, time-based placer inside the codec would only add a rounding rule to reconcile.
• Rounded to the nearest configured frame, not "first frame at or past join". The quest's plan said the latter, but it keys one frame late on any jittery camera at the nominal rate (a frame landing 1 ms early pushes the cut to the next one), so every group would overrun the declared interval. Rounding at the configured rate reproduces the frame count exactly at that rate and still bounds a slower source to one frame of overshoot.
• Timestamps only, no wall clock. The encoder's inputs are local captures, decodes, or caller pixels in presentation order; the transcode path already mirrors its source's groups via cut.

Tests

Mocked timestamps, no sleeps, all in rs/moq-video/src/encode/encoder.rs:

• a_slow_source_keys_by_time: a source drifting through 30, 20, 12.5, and 24 fps never holds a frame a join past its group's keyframe and never cuts more than half a configured frame early. A no-cut backend is never asked.
• a_jittery_source_at_the_configured_rate_keys_on_the_count: keys exactly on frames 30, 60, 90.
• a_fast_source_keys_on_the_backend_count: 60 fps on a 30 fps config with a backend keying on its own count; the encoder forces none.
• a_jump_back_restarts_the_join.
• openh264_keys_a_slow_source_by_time: the real codec keys a 20 fps source every 20 frames with no stray IDR on its 30-frame count, which shows openh264 restarts its GOP at a forced keyframe. Runs in the merge gate.

Each new test fails with the time cut disabled or with the output-keyframe re-anchor removed. just check origin/main passes. The VideoToolbox carry is not compiled or run here (Linux host without an Apple target); macOS CI covers it.

Impact

• Public API: none. Config::keyframe_interval stays crate-private; no signatures change.
• Behavior: encoders on cut-capable backends emit extra IDRs only when the source is slower than Config::framerate. The declared effective_join is unchanged.
• Wire: none.

Caused by

No single PR: frame-count GOPs are as old as the encoder backends.

Suggested follow-ups

• Verify on hardware that VideoToolbox, Media Foundation, MediaCodec, NVENC, and VA-API restart their own GOP count at a forced keyframe, as openh264 does. A backend that does not would add a stray keyframe at its count position, producing shorter groups but never longer ones. Recommendation: drop as a quest. The Goal holds either way, and the nightly hardware suites would show it if it matters.

Models

Size: M
Implementer: Claude Code, Claude Opus 5.5, high effort
Reviewer: Codex, gpt-6.1-sol, xhigh effort

🤖 Generated with Claude Code

(Written by Claude Opus 5.5 with high)

Review

Rewritten by quest review after each round.

• Reviewer: Codex, gpt-6.1-sol, xhigh effort
• Session: 01a1b9da-cf3f-72c1-a164-c54097c70502, round 2
• Head: 745560a
• Base: origin/main at 979a794
• Score: 10/10, The incremental changes fix the dropped-cut issue with no new findings; builds and tests were not run as instructed.
• Findings: none
• The last round's findings, at 8b93151:
  • Fixed: medium, rs/moq-video/src/encode/encoder.rs:625: Retain timed cuts when their frame is dropped; VideoToolbox retains cuts after empty output at rs/moq-video/src/encode/backend/videotoolbox.rs:274 and reapplies them at line 241 until a frame is emitted, whose confirmed keyframe updates the join anchor through Join::observe.

[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 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews 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: 777b50e3-ba1c-4590-adea-f905c0b50c77

Commits

Reviewing files that changed from the base of the PR and between a358f6a and 745560a.

Files selected for processing: 6

• quest/m1/README.md
• quest/m1/keyframe-by-time.md
• rs/moq-video/README.md
• rs/moq-video/src/encode/backend/mod.rs
• rs/moq-video/src/encode/backend/videotoolbox.rs
• rs/moq-video/src/encode/encoder.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 745560a.

No actionable findings. The timestamp-based cut supplements the retained backend GOP, honors can_cut, and re-anchors on output keyframes, including those returned by flush. The VideoToolbox change carries a forced cut across empty output, while MediaCodec already carries an unaccepted input's request. The deterministic tests cover slow, nominal/jittery, fast and backward-moving timestamps, with a real openh264 regression included.

Direction: keeping the existing GOP as a ceiling while using the shared cut path for slower inputs is a contained approach, with the no-cut backend limitation explicitly documented.

Verification limits: GitHub-only static review of the complete PR diff and relevant encoder/backend context. No builds, tests, native codec or hardware runs performed; other backends' GOP restart behavior remains unverified here. Rechecked open/non-draft state, full head and published reviews immediately before this COMMENT.

[pullfrog]

ℹ️ Two minor notes: the moq-mux join warning this PR cites can still fire on a drifting slow source, and the moq-video changelog has no entry for a user-visible fix.

Reviewed changes

Reviewed the full diff at 745560a (6 files): the time-based Join, the dropped-cut contract on Backend::encode, the VideoToolbox carry, the doc updates, and the completed quest.

• Time-based join — Encoder::encode now forces a cut through the existing cut flag when the frames' timestamps advance keyframe_interval's worth of time at Config::framerate, and Join::observe raises the anchor to every keyframe in the backend's output (including a flush). A slower source keys every join, a faster one keeps the backend's frame count, and a no-cut backend is never asked.
• VideoToolbox cut carry — a forced cut whose frame yields no packet is reapplied to the next frame, matching MediaCodec, so a dropped cut costs one frame rather than a whole join. This also restores a caller's Encoder::cut in that case.
• Docs — Config::join, Config::keyframe_interval, Encoder::cut, Backend::encode, and the moq-video README now describe the timestamp placement and the carry contract.
• Quest — quest/m1/keyframe-by-time.md and its m1 entry are deleted, and no references to the old path remain in the tree or sh/gh/test-inventory.allow.
• Tests — mocked-timestamp slow, nominal-jitter, fast, and jump-back cases over recording and Counting backends, plus a real openh264 slow-source case in the merge gate.

ℹ️ The cited moq-mux warning can still fire for a drifting slow source

The encoder now cuts within one source frame of the join, but the catalog still declares effective_join, and moq_mux's interval check warns on a strict measured > declared. A drifting camera whose rate does not divide the join evenly can still produce a keyframe-to-keyframe span a fraction of a frame over the declaration and trip the same one-shot "groups are longer than the declared join interval" warning. This may be the accepted one-frame allowance; worth confirming that leaving the warning in place is intended.

Technical details

# Residual join overrun still trips the declared-interval warning

## Affected sites
- `rs/moq-video/src/encode/producer.rs:416` — the capture pipeline declares `probe_config.effective_join()` as the interval hint (`with_interval`).
- `rs/moq-mux/src/container/producer.rs:350` — `member.report(reading.interval)` feeds the longest measured group span into the check.
- `rs/moq-mux/src/catalog/interval.rs:63` — `interval > declared` has no tolerance for the rounding the time cut accepts.

## Required outcome
- A decision on whether a sub-frame overrun should still warn. If not, either the declared hint or `check` must allow the overshoot the time cut permits.

ℹ️ No moq-video changelog entry for a user-visible fix

rs/moq-video/CHANGELOG.md is hand-maintained on main, and recent fixes (e.g. #7878) added ### Fixed bullets under [Unreleased]. This PR changes encoder output (extra IDRs on slow sources, and a VideoToolbox cut that is no longer lost), so it warrants an entry.

Technical details

# Missing Unreleased changelog entry

## Affected sites
- `rs/moq-video/CHANGELOG.md:10` — `[Unreleased] / ### Fixed` has no bullet for this PR.
- `rs/moq-video/README.md:118` — already documents the behavior, so the change is user-visible.

## Required outcome
- One `### Fixed` bullet under `[Unreleased]` in `rs/moq-video/CHANGELOG.md`, e.g. that a video encoder keys by timestamps so a source slower than `Config::framerate` still opens a group every `Config::join`.

(Written by DeepSeek V4.1 Flash 1017)

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}