fix(moq-video): make moq-video compile on dev again - #3423
Conversation
`resolved_bitrate()` returns `moq_net::bandwidth::Rate` on dev, but the V4L2 encoder was written on main against the older `u64` and merged in by 175b477 without being recompiled. Every other backend already calls `.as_bps()`; v4l2 passed the `Rate` straight to `set_bitrate` and called `div_ceil` on it, and its `the_level_clears_the_bitrate` test assigned a bare integer to `Config::bitrate`. `moq-video` therefore does not compile on dev, which fails `just check` for any branch whose changed-file scope selects it. That is why a moq-net change such as #3419 goes red while a moq-sock or moq-relay one does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DLtoPv9B3779kGxLFTUGHR
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. |
`DmaBufExport::inner` carries `expect(dead_code)` without `render`, but `into_parts` was only lint-suppressed, not compiled out. The function therefore still existed and still read `inner`, which kept the field live and made the field's own expectation unfulfilled. `-D warnings` promotes `unfulfilled_lint_expectations` to an error, so moq-video failed to compile for any crate selecting dmabuf without render, moq-ffi and libmoq included. `render/dmabuf.rs` is the only caller, so gate the function on `render`. The field is then genuinely dead without it and its expectation holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DLtoPv9B3779kGxLFTUGHR
kixelated
changed the title
|
@codex review The earlier review covered (written by claude-opus-5[1m]) |
|
@coderabbitai review (written by claude-opus-5[1m]) |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR metadata and changed files =="
gh pr view 3423 --repo moq-dev/moq --json number,title,baseRefName,headRefName,commits,files
echo
echo "== PR diff =="
gh pr diff 3423 --repo moq-dev/moq --patch
echo
echo "== Locate relevant source files =="
fd -t f . rs | rg '(v4l2|dma.?buf|dmabuf)' || true
echo
echo "== Outline V4L2 backend =="
ast-grep outline rs/moq-video/src/encode/backend/v4l2.rs --items all 2>/dev/null || true
echo
echo "== DmaBufExport definitions and into_parts call sites =="
rg -n -C 5 -P '\b(struct|impl)\s+DmaBufExport\b|\binto_parts\s*\(' rsLength of output: 13530 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Locate moq-video manifest =="
fd -t f '^Cargo\.toml$' rs/moq-video -x sh -c 'echo "--- $1"; sed -n "/^\[features\]/,/^\[/p" "$1"' sh {}
echo
echo "== frame.rs module outline and conditional compilation =="
ast-grep outline rs/moq-video/src/frame.rs --items all
rg -n -C 4 -P 'cfg|DmaBufExport|DmaBufFrame|fn export\s*\(' rs/moq-video/src/frame.rs
echo
echo "== Render module gates and DmaBufExport call context =="
fd -t f . rs/moq-video/src/render -x sh -c 'echo "--- $1"; rg -n -C 4 -P "cfg|mod dmabuf|into_parts\\s*\\(" "$1"' sh {}
echo
echo "== V4L2 bitrate APIs and Config bitrate type =="
sed -n '125,165p;650,850p;875,945p' rs/moq-video/src/encode/backend/v4l2.rs
rg -n -C 4 -P 'struct Config\b|bitrate\s*:|fn resolved_bitrate\s*\(' rs/moq-video/src/encode
rg -n -C 3 -P 'struct Rate\b|impl Rate\b|fn (as_bps|from_bps)\s*\(' rs/moq-netLength of output: 40250 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Crate render module declaration =="
fd -t f 'lib.rs' rs/moq-video -x sh -c 'echo "--- $1"; rg -n -C 4 -P "(pub\\s+)?mod\\s+render\\b|cfg.*render" "$1"' sh {}
echo
echo "== Encoder Config bitrate API =="
fd -t f 'encoder.rs' rs/moq-video/src/encode -x sh -c 'echo "--- $1"; rg -n -C 6 -P "struct Config\\b|bitrate\\s*:|fn resolved_bitrate\\s*\\(" "$1"' sh {}
echo
echo "== Rate conversion API =="
rg -n -C 5 -P 'struct Rate\b|impl Rate\b|pub\s+(const\s+)?fn\s+(as_bps|from_bps)\s*\(' rs/moq-net
echo
echo "== All into_parts definitions and calls, excluding comments =="
ast-grep run --lang rust --pattern '$VALUE.into_parts()' rsLength of output: 8514
The ✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. WalkthroughThe V4L2 backend now converts resolved Merge Risk: ⚪ Minimal · up to This change restores V4L2 compilation by converting resolved bitrate values to bits per second and limits DMA-BUF splitting to render builds. The updated behavior preserves bitrate boundaries and render functionality, with no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Both reviews are clean on Declining the one failed pre-merge check (Docstring Coverage, 75% vs an 80% threshold). The undocumented function it counts is fn set_bitrate(device: &Device, bitrate: u64) -> Result<(), Error> {
device.set_control(V4L2_CID_MPEG_VIDEO_BITRATE, bitrate.min(i32::MAX as u64) as i32)
}Private, two lines, and a direct wrapper over the control it names. CLAUDE.md asks for the opposite of what the threshold wants here:
Worth noting the diff does not change (written by claude-opus-5[1m]) |
1 participant
Root cause
Config::resolved_bitrate()returnsmoq_net::bandwidth::Rateondev. The V4L2 stateful M2M encoder (#3332) was written onmain, where that method still returnedu64, and arrived ondevthrough the merge175b477a0. Nothing recompiledmoq-videoafterwards, so three call sites never type-checked:v4l2.rs:148passes theRatestraight intofn set_bitrate(_, bitrate: u64)v4l2.rs:832calls.div_ceil(1_000)on it, whichRatedoes not havethe_level_clears_the_bitrateassigns a bare integer toConfig::bitrate, nowOption<Rate>moq-videodoes not compile ondevtoday. Every other backend (mediafoundation, nvenc, openh264, vaapi, videotoolbox) already goes through.as_bps(); v4l2 is the only one that does not.Why it was not caught
just checkscopes to the crates the branch changed plus their dependents, so this only fires for a branch whose scope selectsmoq-video. Amoq-netchange does (#3419 is red on exactly these two errors); amoq-sockormoq-relaychange does not. So the break has been sitting ondevfailing an arbitrary subset of PRs for reasons unrelated to their diffs.Fix
Add
.as_bps()at both source sites and construct aRatein the test, matching what every other backend already does. No behavior change:Rateis a newtype overu64bits per second, andfrom_bps/as_bpsround-trip it.Verification
just rs check-changed rs/moq-video/src/encode/backend/v4l2.rsexits 0 (clippy-D warnings, all targets).cargo nextest run -p moq-video: 29 tests pass, includingthe_level_clears_the_bitrate, which is the test that stopped compiling.origin/dev.Not run:
just rs macos/just rs windows(no such host, and V4L2 is Linux-only), and no hardware V4L2 encoder is present on this machine, so the level-table logic is covered by its unit tests rather than a live encode. Cross-Package Sync has no row formoq-video.🤖 Generated with Claude Code
https://claude.ai/code/session_01DLtoPv9B3779kGxLFTUGHR
(written by claude-opus-5[1m])