chore(quest): complete the media release review - #3878
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (28)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. WalkthroughThe changes align media documentation, changelogs, and quest plans with current APIs and behavior. They document audio, video, transcode, and NVENC breaking changes; update GPU, CPU, capture, and feature-gating descriptions; mark Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The PR aligns media documentation, changelogs, and quest plans, with the intentional public compatibility change making RateError non-exhaustive. It is merge-ready with normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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. Comment |
MERGEPositive improvement? Yes. This closes the media release review quest the way it was written: record that the audio/video/transcode/NVENC contracts landed on Worth the complexity? Yes. Almost all of the diff is quest/docs/changelog hygiene; there is no second media model and no wire or published FFI layout change. Bundling the justfile fix with the review is a little opportunistic, but the PR already says it can land alone, and the cost of keeping it here is low. Different approach? Splitting the justfile fix into its own PR would make the unblock land faster if this review stalled, but it does not change the verdict. Holding the quest open until NVIDIA/VAAPI/V4L2/MF hardware runs finish would conflate compile evidence with hardware evidence; those lanes already live under Recommendation: MERGE. Good to land as the review record and backlog/docs cleanup; the separate 0.1 bump stays a follow-up. This is an automated review, not the maintainer's decision |
|
No issues found. |
Records that the audio, video, transcode, and NVENC contracts landed on main at f189411 with no release blockers, fixes the stale backlog and docs the review turned up, adds the missing Unreleased changelog entries, and marks RateError non-exhaustive like every other public error enum. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
89403c6 to
822512a
Compare
|
No issues found. |
1 participant
Completes
quest/main/media-release-review.md.Problem
The twelve media API quests landed on
maintoday. Before anyone requests the separate 0.1 version bump formoq-audio,moq-video,moq-transcode, andmoq-nvenc, the quest asks for a record that the agreed contracts are in, that the deferred backlog can extend them without another replacement, and that the release candidate passes the independent feature, refusal, ownership, docs, and dependency-tree gates.Approach
Verdict: landed, no blockers. Release candidate reviewed:
origin/mainatf1894110b7028fefc7420c1a3b088d00e0bcaf16(#3875). This PR deletes the quest, fixes the stale backlog and docs it turned up, and adds one missing attribute.Landed contracts
Encoder::load/LoadError, no publicENCODE_APISubmissionheld through completion,unsaferaw entry pointsEncodeError::cleanupopenh264(default on),renderoff by default,nvidiaonly;just rs media-featuresin CIrate::{Policy, Control}(frommoq_video::encode::rate)Encoder/Decoderare!Send; asyncSink/Consumerown executionFrame/Encodednon-exhaustive with constructors, typedI420, one configured conversion eachRate, capture frames timestamped before queue replacementOutput { Native, Cpu },decode::Config { kind, output, scale_hint },decode::Options,resize::Config::output,--framesencode::Gopenum, falliblecut(),CutUnsupportedat openframe::vulkanimporter,frame::cuda::Converter, bounded pool, NVENC in placeLayout;encode::{Input, Settings, Options};decode::{Config, Output, Options}Producer::demand()replacestrack(), privateResampler,PublicationOptions, non-exhaustive framesfecflag removedaec::Control, exclusive microphone attachment,Error::Busyplayback::Sink::write -> Write { accepted, dropped }Rate,Gop,resize::Config, feature defaults; no second media modelExtension points checked against the deferred lines: intra-refresh extends the non-exhaustive
Gop; audio-codecs and surround extendLayoutanddecode::Kind; color-model extendsColoronencode::Config; the decode seam extendsKind; ladder, #2848, and encoder-feedback consumemoq_mux::rateandencode::Options; NVENC reuse/recovery build on the ownedSubmission. Every deferred quest is phrased as an extension; none needs a replacement of these types.Verification on
f1894110b(macOS host, Nix shell)just rs media-featuresmedia-features(--no-default-featurestest suite:Kind::Softwareand named openh264 refuse)--all-featuresjust rs macoscapturefeaturejust rs capture--all-featuresRUSTDOCFLAGS="-D warnings" cargo doc --locked --no-deps -p moq-audio -p moq-video -p moq-transcode -p moq-nvenc -p moq-muxjust checkandjust teston each merged PR (CICheck,Android,Swift,OBS,WASMgreen on every PR head)cargo tree -e features --workspace --target x86_64-unknown-linux-gnucpalandwgpuabsent from the workspace default graph;openh264andmoq-nvencenter only through the intended defaultopenh264/nvidiafeatures;render,capture,v4l2stay opt-incargo run --locked -p quest -- checkHardware cases not exercised here, recorded separately from compilation: NVENC/NVDEC/CUDA/Vulkan (Linux/NVIDIA;
just rs vulkan-cudastays underquest/main/video-gpu-encode.md), VAAPI and V4L2 (Linux,vaapi/v4l2are nightly--all-featuresonly), Media Foundation (nightlyjust rs windows), AVFoundation/ScreenCaptureKit at runtime (compiled byjust rs macos, not run). The last nightly (2026-09-21,a93fdb8e8) failed its macOS, Windows, uring, and drill lanes; that SHA predates every media PR, and #3877 fixed the macOS capture break they hit.Backlog and docs cleaned
decode::Config(nowdecode::Options) inaudio-jitter-target/{README,native}.md;Config::bitrateand the dev-merge condition in2848-*.md; the completed host-flag paragraph incapture-alsa-link.md; the private resampler insip-stack.md;Layoutinplay-audio-rendition-gap.md; "replacement option" wording inaudio-loss-recovery.md.gpu_framesinvideo-vaapi.md; the pre-cut()state ofkeyframe-trigger.md; the backend keyframe flag inintra-refresh/encode-config.mdand the self-disabling V4L2 fallback inintra-refresh/v4l2-refresh.md; completed rollback and destructor items innvenc-recovery.md; the dev-merge condition inladder/README.md;encode::Configandrate.rsinqos/stats/encoder-feedback.md;Rendition::updateinjitter-flush-clock.md. (The stalemobile/ffi-video-consumer.mdwas already deleted by chore(quest): groom next after the dev merge #3868, which agrees the FFI consumer landed.)moq-videoREADME capture loop returnsFrame;moq-transcodeREADME example uses the currentcreate_broadcast+announce; transcode crate doc anddoc/bin/cli.mdmention--frames;DESIGN-native-codecs.mdnames thenvidia/vaapifeatures;doc/lib/rs/moq-video.mdsays openh264 is a default feature.Unreleasedchangelog entries for the landed breaking changes that had none (moq-video, moq-audio, moq-transcode, moq-nvenc, moq-muxrate), so the 0.1 notes carry the migration detail.Impact
moq_video::RateErrorgains#[non_exhaustive](0.0.xmoq-video;rs/CLAUDE.mdrequires it on public error enums, and adding it after 0.1 would be a breaking change). Nothing else changes; the review is otherwise docs and quests.git diffofrs/libmoqandrs/moq-ffipublic signatures across the range confirms. The only C layout change onmainin the same window is feat(ffi)!: scope announcement streams with patterns #3856 (moq_announce_updategainscaptures), outside this line and already declared breaking there.Alternatives
Keeping the quest open for the hardware runs would conflate compilation evidence with hardware evidence;
quest/main/video-gpu-encode.mdalready owns the NVIDIA validation and the other hardware lanes are nightly.Follow-ups
rs/moq-cli/Cargo.tomlstill carries the silentnvenc/nvdecfeature aliases that Make media backends optional #3839 removed frommoq-video; a maintainer decision on the binary's feature names, not part of the four crates.🤖 Generated with Claude Code