Repository navigation
feat(moq-vaapi)!: vendor moq-dev/vaapi in-tree and trim unused codecs - #8235
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AV1, VP8, VP9, MPEG-2, and JPEG buffer wrappers and their bindings: the encode and decode drivers only speak H.264 and H.265. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 2 minutes. View limit details
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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed c51c63f against the in-tree integration and moq-dev/vaapi at 24c755965066d57cda542a8e4a638e6665c8ccb4.
No actionable findings. The move preserves the H.264/H.265 driver behavior from the 0.2.1 source; the substantive wrapper deletion matches the explicitly documented unsupported-codec API removal. Workspace dependency/logging changes retain runtime libva loading, and the regeneration script carries the libloading 0.9 constructor-bound adjustment without adding a build-time bindgen requirement. Bringing this dependency in-tree removes the separate publish/bump cycle while retaining the upstream audit record.
Verification limits: GitHub-only static review, including upstream blob/content comparison, non-generated source delta, buffer dispatch, regeneration script and integration/docs. I did not regenerate or exhaustively re-audit generated bindings, run builds/tests, inspect a published crate artifact, or reproduce the Intel/AMD hardware results. Rechecked open/non-draft state, full head and published reviews immediately before this COMMENT.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No code issues found — one follow-up worth tracking so it survives the merge.
Reviewed changes
- Vendors
moq-dev/vaapiat24c7559(0.2.1) asrs/moq-vaapi, adds it to the workspace anddefault-members, and turns the rootmoq-vaapidependency into a path dependency at the last published version. - Trims the AV1, VP8, VP9, MPEG-2, and JPEG buffer wrappers along with their
bindgenallowlist entries, leaving H.264/H.265 only;src/bindings/libva.rsdrops from 37436 to 22389 lines. - Replaces the standalone repo's
just vendor/just bindgenrecipes withrs/moq-vaapi/bindgen.sh, which regenerates from the vendoredlibva/headers and carries the libloading 0.9AsFilenamebound. - Switches logging from
logtotracing, adopts workspace dependencies and lints, and documents the allow blocks' reasons. - Repoints the docs,
rs/moq-videodesign/module notes, and the vendor-sync quest at the in-tree crate, and deletes the completed radeonsi quest.
I checked the vendored tree against upstream 24c7559: libva/, libva-wrapper.h, and the license files are identical, and src/ differs only in the intended edits (tracing, trimmed buffer codecs, reason on the allow blocks). The regenerated bindings only lose items, so the trim did not perturb code it keeps, and the pointer-width #[cfg] matches libva's __x86_64__ || __LP64__ gate on every target, Windows x64 included.
ℹ️ The anyhow public API has no tracked follow-up
With the crate in-tree, rs/AGENTS.md's "thiserror for libraries" rule now applies to it, yet its public decode, encode, vpp, and dmabuf entry points still return anyhow::Result. The deferral exists only in this PR's body, which does not survive the merge, and CONTRIBUTING.md asks that out-of-scope findings become follow-up quests.
Technical details
# Track the anyhow -> thiserror conversion for moq-vaapi
## Affected sites
- `rs/moq-vaapi/src/decode.rs:290,318,341,377,383` — `Decoder::new/decode/decode_exported/flush*`
- `rs/moq-vaapi/src/encode.rs:228,346,386,433,573` — `Encoder::new/encode_nv12/encode_dmabuf/set_bitrate` and `probe`
- `rs/moq-vaapi/src/vpp.rs:58,91,103,118,169,294` — `probe` and `Processor` constructors/transforms
- `rs/moq-vaapi/src/dmabuf.rs:151,194` — `from_prime`/`import`
- `rs/moq-vaapi/src/decode.rs:1406`, `src/vpp.rs:324` — helpers that propagate `anyhow::Error`
## Required outcome
A quest under `quest/m3/vendor-sync/` (or another m3 owner) records the conversion, or the conversion lands with this PR. Whichever holds, the origin (`rs/AGENTS.md`'s thiserror rule) and the current `moq-video` callers that map these errors need to be named, since only the crate's shape changes.
## Open questions for the human
- Whether the conversion should replace the `VaError` wrappers with a single `Error` enum, or keep `VaError` and add context variants for the encode/decode layers.deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
|
Re the pullfrog note on moq-vaapi's (Written by Claude Opus 5.5) |
Problem
moq-vaapi lived in moq-dev/vaapi and shipped through crates.io. That made every fix a publish-then-bump round trip (the radeonsi fix waited a quest on it). The separate repo existed because the crate's
build.rsneeded libclang. moq-dev/vaapi#8 checked the bindings in, so nothing keeps it out of the monorepo. moq-nvenc and moq-v4l already moved in-tree the same way.Approach
rs/moq-vaapicomes from moq-dev/vaapi at24c7559(0.2.1, which includes the radeonsi fix from moq-dev/vaapi#13). The rootmoq-vaapidependency becomes a path dependency, version 0.2.1.tracinginstead oflog. It also takes the workspace lints; the crate-level allow for vendored code now states its reason.vendorandbindgenrecipes becomers/moq-vaapi/bindgen.sh, like moq-v4l's script. It reproduces the old bindings byte for byte. It also patches bindgen'sVa::newbound, because libloading 0.9 no longer acceptsAsRef<OsStr>.libva.rsgoes from 37k to 22k lines.encode/backend/vaapi.rs.Validation
just cipasses.vaapitests, ignored ones included, withopenh264), using libva 2.24.1, iHD 26.3.5, and Mesa 26.2.6 from nixpkgs: 62/62 pass on both an Intel iGPU (renderD128) and an AMD RX 9070 (renderD129). The only skips are a test that skips by design whenMOQ_VAAPI_DEVICEis set, and H.265 at 320x240 on radeonsi, which quest/m3/h265-min-width.md tracks.Impact
PictureParameter,SliceParameter,IQMatrix,EncSequenceParameter,EncPictureParameter, andEncSliceParametervariants;BufferType::{HuffmanTable, Probability, QMatrix}with theHuffmanTableandQMatrixenums; and those codecs' bindgen types. There is no replacement; the crate only drives H.264 and H.265.tracinginstead oflog.Alternatives
Follow-ups
anyhowerrors in its public API, againstrs/AGENTS.md's thiserror-for-libraries rule. That was already true before the move and is left alone here.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Review
Rewritten by
quest reviewafter each round.01a1b8bb-c209-78a2-b32b-a38f650f68b1, round 1Source review found no actionable regressions or blockers in the scoped diff, but builds and tests were not run as instructed.