Repository navigation
Conversation
Hardware codecs come first, so the vendored OpenH264 C++ is no longer a default of moq-video or moq-transcode, and no workspace crate turns it on unconditionally. moq-ffi, moq-c, moq-boy, and moq-cli each forward an `openh264` feature instead, and every release path enables it: rs/moq-ffi/build.sh (all language bindings), maturin, the C++ CMake build, nix's moq-c and moq-boy, and moq-c's Windows build. The host builds behind the Go, Kotlin, and Swift test suites and `just rs android` match. Tests that encode keep running everywhere: moq-c, moq-ffi, and moq-transcode turn the feature on through a dev-dependency (GPU-less CI has no other encoder), and `just rs play` passes it for moq-cli. One moq-video test that encodes in software was gated on `vaapi` alone; it now also requires `openh264`. BREAKING CHANGE: moq-video and moq-transcode no longer enable `openh264` by default. A consumer that relies on the software fallback (a GPU-less host, the Apple simulators, API-24 Android) must enable it explicitly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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 27 seconds. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughOpenH264 is no longer enabled by default in several Rust crates. Consumer crates expose or forward an opt-in feature, and selected build and packaging targets now enable it explicitly. Documentation describes the feature as opt-in. Test commands and a Linux decoder-tail test condition are updated to account for the feature. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 9 files. (13 skipped: 1… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly summarizes the main change: OpenH264 becomes opt-in and remains enabled for release builds. |
| Description check | |
The description directly explains the feature changes, release paths, test strategy, breaking behavior, and validation results. |
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 9 files. (13 skipped: 13 unsupported.)
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- 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.
Comment @coderabbitai help to get the list of available commands.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7b8255d7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # The software H.264 encoder. moq-boy needs it at runtime (a 160x144 frame is | ||
| # below every hardware encoder's minimum), but it is vendored C++, so it stays | ||
| # off for development builds; the nix package turns it on. | ||
| openh264 = ["moq-video/openh264"] |
There was a problem hiding this comment.
The documented cargo install moq-boy uses these default features and therefore never enables the new openh264 feature. VideoEncoder::spawn unconditionally forces encode::Kind::Software for its 160x144 frames, so without OpenH264 the software candidate list is empty and startup panics while probing the encoder on every platform. Ensure the normal Cargo installation enables this required feature, rather than only enabling it in the Nix package.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Kept openh264 opt-in rather than defaulting it on in moq-boy: a moq-boy default would, through feature unification, compile OpenH264 into every default cargo build --workspace and undo this PR. Instead moq-boy now refuses to start without it, naming cargo install moq-boy --features openh264, and the README, demo/boy, and MIGRATE.md say so. Fixed in eb1ef4e.
(Written by Claude Opus 5.5)
| # explicitly. Workspace consumers opt out at the root manifest | ||
| # (`default-features = false`) and pick per crate, so the relay and the language | ||
| # bindings never build device or rendering stacks. | ||
| default = ["mediacodec", "nvidia"] |
There was a problem hiding this comment.
Downstream users are still told in rs/moq-video/README.md that OpenH264 is a default feature (both the backend tables and the default-feature paragraph), and the crate-level rustdoc in src/lib.rs makes the same claim. With this new default list, following that documentation on a GPU-less host produces a build with no H.264 encoder, so these user-facing references need to describe openh264 as opt-in.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the README backend tables, default-feature paragraph, and cargo add examples, plus the src/lib.rs rustdoc, now call openh264 opt-in. MIGRATE.md has an entry.
(Written by Claude Opus 5.5)
Both encode H.264 in software on GPU-less runners, which now needs the opt-in openh264 feature. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # rs/justfile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e93d7ac867
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| moq-transcode = { workspace = true, optional = true } | ||
| moq-video = { workspace = true, optional = true } |
There was a problem hiding this comment.
When users follow doc/bin/obs.md:43 (cargo install --locked moq-cli --features transcode) on GPU-less Linux, these optional dependencies no longer enable OpenH264, leaving only the default NVENC backend and causing encoder probing to fail at runtime. doc/bin/cli.md:186-222 also still promises an H.264 software fallback and support for software-only hosts, so update the CLI build examples and related claims to include openh264 wherever that fallback is expected.
AGENTS.md reference: AGENTS.md:L120-L120
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: doc/bin/obs.md installs with --features transcode,openh264, and doc/bin/cli.md names openh264 for the software fallback under Play, Capture, and Transcode. moq-cli's MIGRATE.md has an entry.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69510a3a71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # moq-video's: release builds enable it. `vaapi` stays opt-in because its | ||
| # bindgen needs libclang; `v4l2` mirrors moq-video's opt-in too, though it costs | ||
| # the build nothing. | ||
| default = ["nvidia"] |
There was a problem hiding this comment.
With this default reduced to nvidia, the published crate-level documentation in rs/moq-transcode/src/lib.rs:20-22 still tells direct users that the default openh264 feature supplies the software fallback. A user relying on that rustdoc on a GPU-less host now builds a transcoder with no usable encoder, so update the rustdoc alongside the README to state that openh264 must be enabled explicitly.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the crate rustdoc now calls openh264 opt-in and says a GPU-less host needs it. MIGRATE.md has an entry.
(Written by Claude Opus 5.5)
# Conflicts: # cpp/moq/CMakeLists.txt # rs/moq-c/Cargo.toml # rs/moq-ffi/Cargo.toml
…t opt-in moq-boy refuses to start without OpenH264 and its README, demo, and MIGRATE name the feature. The Dart hook and moq-c CMake source builds enable it like the other binding builds. The moq-video and moq-transcode docs, the CLI and OBS build instructions, and each affected MIGRATE.md describe it as opt-in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # rs/moq-cli/MIGRATE.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e649d23198
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| moq-transcode = { workspace = true, optional = true, features = ["openh264"] } | ||
| moq-video = { workspace = true, optional = true, features = ["openh264"] } | ||
| moq-transcode = { workspace = true, optional = true } | ||
| moq-video = { workspace = true, optional = true } |
There was a problem hiding this comment.
On a GPU-less Linux host, following doc/concept/transport.md:86 still builds moq-cli with only iroh,play. These changed dependency lines no longer activate OpenH264, while NVIDIA is the only default Linux decoder, so the documented moq ... play command cannot open an H.264 stream. A repo-wide invocation search found this remaining recipe after the other CLI docs were updated; include openh264 in its feature list. Local cargo build --help confirms that --features accepts a comma-separated list.
AGENTS.md reference: AGENTS.md:L122-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: doc/concept/transport.md now builds with iroh,play,openh264.
(Written by Claude Opus 5.5)
| # moq-video's: release builds enable it. `vaapi` stays opt-in because its | ||
| # bindgen needs libclang; `v4l2` mirrors moq-video's opt-in too, though it costs | ||
| # the build nothing. | ||
| default = ["nvidia"] |
There was a problem hiding this comment.
On a GPU-less host, the runnable example in rs/moq-transcode/README.md:81-82 now builds with the default nvidia feature only, so an H.264 source fails when the example tries to open its decoder or encoder. The updated prose immediately above correctly says such hosts require --features openh264, but both this command and the copy in examples/transcode.rs:6-7 omit it; update those examples to keep the documented workflow runnable.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining: examples build with dev-dependencies, and moq-transcode's dev-dependency enables moq-video/openh264. cargo tree -p moq-transcode -e normal,dev -i openh264-sys2 reaches it, so cargo run -p moq-transcode --example transcode already has the software fallback on a GPU-less host.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
1 participant
Summary
We prioritize hardware encoding, so the vendored OpenH264 C++ is now opt-in everywhere except releases.
moq-videoandmoq-transcodedropopenh264fromdefault. No workspace crate enables it unconditionally any more.moq-ffi,moq-c,moq-boy, andmoq-clieach forward anopenh264feature.rs/moq-ffi/build.sh, which builds the libraries for every language binding. Under--no-default-featuresit's a no-op.[tool.maturin] features.MOQ_FFI_DIRinput comes frombuild.sh, so it already has it).add_subdirectory(rs/moq-c)), so it stays "the samemoq::ctarget" as the release archive.moq-candmoq-boy. moq-boy encodes 160x144, which is below every hardware encoder's minimum.sh/dart/generate.sh), and Python (just py test, whose--featuresreplaces pyproject's list) test suites,just rs android, andsh/swift/simulator.shnow build the same feature set that releases ship.openh264, naming the install command, instead of panicking while probing an encoder. Its README,demo/boy, and MIGRATE name the feature.Tests stay in the merge gate
GPU-less CI has no encoder besides OpenH264. Gating the 38 affected tests would have pulled them out of the gate, which
rs/AGENTS.mdasks us not to do. Instead:moq-c,moq-ffi, andmoq-transcodeenablemoq-video/openh264through a dev-dependency. Their test targets get the encoder; library and release builds still choose it explicitly.just rs playpassesopenh264. moq-cli's moq-video dependency is optional, so a dev-dependency would drag moq-video into every moq-cli test build.decode::consumer::tests::the_track_ending_drains_the_decoderencodes in software but was gated onvaapialone. It now also requiresopenh264.Decisions
Made unattended by the iterate agent; each picked the recommended option.
moq-boy and
cargo install(Codex P1).openh264opt-in; refuse to start without it and documentcargo install moq-boy --features openh264. Defaulting it on in moq-boy would, through feature unification, compile OpenH264 into every defaultcargo build --workspace, undoing this PR.openh264to moq-boy'sdefault. Fixes barecargo install, at the cost above.What a CMake source build of moq-c ships.
openh264, like the C++ CMake build and the release archive.Breaking
moq-videoandmoq-transcodeno longer enableopenh264by default. A downstream crate that needs the software fallback must name the feature. moq-cli'scapture,transcode, andplay, a barecargo buildof moq-ffi or moq-c, andcargo install moq-boyneed--features openh264for it too. Each of those packages has aMIGRATE.mdentry. Published bindings, wheels, archives, and nix packages are unchanged.No public API or wire change.
Expected effect on build time
Small. On a cold
cargo check, openh264's build script is about 16 s of one core and sits off the critical path; mbx caches the C++ on warm builds. The main win is that hardware-only consumers and plain library builds stop compiling it.Test plan
just check(Nix dev shell) on the merged head, including the Dart suite's software-encode test through the build hookcargo clippy --workspace --all-targets -D warnings(without openh264)just rs lint --workspace(fmt, doc, shear, sort)just rs media-features --all(dependency-shape assertions)just rs testfor moq-video, moq-transcode, moq-ffi, moq-c, moq-boy, moq-cli: 723 passed (38 failed before the dev-dependencies)just rs play,just rs capture-testcargo check --workspace --all-targets --no-default-featurescargo tree -e normal: each package lacksopenh264-sys2by default and has it with--features openh264(Written by Claude Opus 5.5)
🤖 Generated with Claude Code