build(container): restrict the in-tree ffmpeg to a narrow media-codec allowlist (OPS-7665) - #11628
build(container): restrict the in-tree ffmpeg to a narrow media-codec allowlist (OPS-7665)#11628dmitry-tokarev-nv wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
🔍 Pre-existing --disable-bsfs may affect VP9 decode in MP4 containers
The --disable-bsfs flag at container/templates/wheel_builder.Dockerfile:357 was already present before this PR and disables all bitstream filters. Some demuxers/decoders use bitstream filters internally (e.g., vp9_superframe for VP9 in MP4). If VP9-in-MP4 decode fails at runtime due to missing BSFs, this pre-existing flag would be the cause. The test fixtures were regenerated with -g 1 (all keyframes), which may mask the issue since superframe handling is primarily needed for non-keyframe packets. Worth monitoring in integration tests with real-world VP9-in-MP4 content that has B-frames or superframes.
(Refers to line 357)
Was this helpful? React with 👍 or 👎 to provide feedback.
WalkthroughChangesFFmpeg version and codec policy
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/llm/src/preprocessor/media/decoders/video.rs`:
- Around line 318-320: Update the ffmpeg regeneration command in the video
fixture documentation to append an MP4 output filename after the encoding
options, making the example executable while preserving its existing input,
codec, frame-count, and keyframe settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 040fe901-aba8-4e7a-99ee-3261d77e31a7
⛔ Files ignored due to path filters (5)
lib/llm/tests/data/media/2160p_10.mp4is excluded by!**/*.mp4lib/llm/tests/data/media/240p_1.mp4is excluded by!**/*.mp4lib/llm/tests/data/media/240p_10.mp4is excluded by!**/*.mp4lib/llm/tests/data/media/240p_100.mp4is excluded by!**/*.mp4lib/llm/tests/data/media/2p_10.mp4is excluded by!**/*.mp4
📒 Files selected for processing (5)
container/compliance/native_packages.yamlcontainer/context.yamlcontainer/templates/wheel_builder.Dockerfilelib/llm/src/preprocessor/media/README.mdlib/llm/src/preprocessor/media/decoders/video.rs
| /// Fixtures are VP9-in-mp4: the in-tree ffmpeg only decodes the royalty-free | ||
| /// allowlist (VP8/VP9), so H.264 fixtures would not decode. Regenerate with | ||
| /// `ffmpeg -f lavfi -i testsrc2=size=WxH:rate=1 -frames:v N -c:v libvpx-vp9 -g 1 -strict -2`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the cited lines.
sed -n '300,335p' lib/llm/src/preprocessor/media/decoders/video.rs
# Look for similar ffmpeg regeneration instructions elsewhere.
rg -n "ffmpeg -f lavfi|libvpx-vp9|testsrc2|240p_10\.mp4|Fixtures are VP9-in-mp4" lib -SRepository: ai-dynamo/dynamo
Length of output: 2531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '310,330p' lib/llm/src/preprocessor/media/decoders/video.rsRepository: ai-dynamo/dynamo
Length of output: 1124
Add an output path to the ffmpeg example. The regeneration command is incomplete as written; append an MP4 filename so it can actually run.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/llm/src/preprocessor/media/decoders/video.rs` around lines 318 - 320,
Update the ffmpeg regeneration command in the video fixture documentation to
append an MP4 output filename after the encoding options, making the example
executable while preserving its existing input, codec, frame-count, and keyframe
settings.
Source: Path instructions
… allowlist (OPS-7665) The in-tree ffmpeg restricted encoders/muxers but still shipped ffmpeg's full default decoder set (291 video + 212 audio decoders) in the libav*.so copied into every runtime image. Narrow it to the media formats we actually build and use. Switch the wheel_builder configure to an allowlist: blanket --disable-decoders/--disable-demuxers/--disable-parsers/--disable-protocols plus exactly the components the two real paths need: - encode CLI (imageio): rawvideo demux/decode over a pipe -> h264_nvenc/vp9 - Rust media-ffmpeg VideoDecoder: VP8/VP9 in mp4/webm/mkv Image decode does not use ffmpeg (Rust `image` crate), so no still-image decoders are enabled. HW NVDEC (h264_cuvid/hevc_cuvid) can be re-added if a decode feature needs it. Also bump ffmpeg 8.1 -> 8.1.2 (an upstream maintenance release) to pick up security fixes and trim the decode surface further. Test fixtures are re-encoded to VP9-in-mp4; filenames and the Rust/Python tests that consume them are unchanged. Validated by building ffmpeg 8.1.2 with this exact allowlist and decoding every fixture to the expected frame counts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
9ed1a9a to
869ddaf
Compare
Remove codec-bearing Python packages inherited from the SGLang base, build the Dynamo runtime without media-ffmpeg, and fail the image build if FFmpeg, H.264, H.265, or AAC libraries remain. Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
The ARM64 Inkling base installs the distribution as decord2, whose wheel bundles FFmpeg, x264, and x265 shared libraries. Remove that package and its residual files before the final codec guard. Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
|
Harrison — thanks for the SGLang codec removal. It lines up exactly with the rc.10 codec audit I ran: SGLang had the most codec sources of the three images (in-tree ffmpeg + a vendored CI test failures (root-caused):
Open review nits from my original commits:
Devin's Sibling PRs: #11607 (opencv removal) and #11632 (the compliance gate, which should merge last). Glad to take any of the above off your plate — just say the word. |
- Drop internal ticket references from the wheel_builder + media README comments. - Make the fixture-regeneration ffmpeg command in the video.rs docstring executable by adding the output filename. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
The TRT-LLM diffusion encode path (imageio -> ffmpeg -c:v h264_nvenc -> mp4) regressed after the decoder allowlist. The h264 *parser* (not the H.264 decoder) is needed for the mp4 muxer to package the h264_nvenc bitstream, so add it back. A parser carries no codec implementation, so this does not reintroduce any software H.264 encode/decode surface; h264_nvenc remains the NVIDIA hardware encoder (the sanctioned path). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
|
Addressed the review comments on my commits (pushed
Still open (not a review comment — a consequence of the SGLang codec removal): the SGLang |
The vllm-openai base ships PyAV, opencv, decord, torchcodec, and PyNvVideoCodec, each bundling its own full ffmpeg with software H.264/H.265/AAC (PyAV and decord also ship GPL libx264/libx265). Dynamo's vLLM component imports none of them, so they are unused decode-side dead weight. Uninstall + remove them, with a guard that fails the build if cv2/av survive. The in-tree LGPL ffmpeg and imageio-ffmpeg are kept for the omni HW video-encode path (h264_nvenc, the sanctioned NVIDIA hardware encoder). This drops software video *decode* from the vLLM images (mirrors the SGLang codec removal). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
|
Added the vLLM codec cleanup (
Lower risk than the SGLang removal: no vLLM serve/deployment test decodes video (the Note: the #11632 scan also flagged |
|
Combined into #11836 (with the vLLM codec cleanup added on top). Keeping this open for the review thread + CI. Review comments addressed (OPS refs removed, video.rs regen command fixed, Devin |
The narrow in-tree ffmpeg protocol allowlist enabled only file,pipe. On ffmpeg 8.x, `ffmpeg -i -` (reading rawvideo frames from stdin, as the imageio encode path does) resolves stdin through the `fd:` protocol, not `pipe:`. With fd disabled, opening the stdin input fails immediately with "Error opening input: Protocol not found. Did you mean file:fd:?", breaking the video encode path. Add fd to the protocol allowlist. It is pure file-descriptor I/O and carries no codec implementation, so it does not widen the media-format surface. Reproduced and confirmed against a locally built allowlisted ffmpeg: file,pipe fails to open stdin; file,pipe,fd opens it cleanly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
…ages
The vllm and sglang runtime images intentionally omit certain media-decoder
libraries (opencv for vLLM's VideoMediaIO backend decode, decord for SGLang's
video path). The video multimodal serve tests exercise those decode paths, so
without the decoder present they fail at request time ("No module named
'cv2'" / "No module named 'decord'").
Rather than skip that coverage, let a serve config opt into an ephemeral,
test-only decoder install via a reserved env key (DYN_TEST_ONLY_PIP_INSTALL).
common.run_serve_deployment installs the named package(s) into the runtime
container before the server launches, so the worker can import them; the
shipped image is unaffected. Session-deduped and a no-op unless a config sets
the key.
Wired for the three vLLM video topologies (opencv-python-headless) and the two
SGLang video configs (decord2). Verified on faithful codec-stripped rc.11
images: each decode path reproduces the exact failure when stripped and decodes
successfully once the package is installed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
|
superseeded by #11836 |
Summary
The in-tree ffmpeg already restricted encoders/muxers, but it still shipped ffmpeg's full default decoder set (291 video + 212 audio decoders) in the
libav*.socopied into every runtime image. Narrow it to the media formats we actually build and use.This switches the
wheel_builderffmpeg configure to an allowlist: blanket--disable-decoders/--disable-demuxers/--disable-parsers/--disable-protocolsplus exactly the components the two real paths need:h264_nvenc/libvpx_vp9media-ffmpegVideoDecoder: VP8/VP9 in mp4/webm/mkvImage decode does not use ffmpeg (it goes through the Rust
imagecrate), so no still-image decoders are enabled. HW NVDEC (h264_cuvid/hevc_cuvid) can be re-added explicitly if a decode feature ever needs H.264/H.265 (not wired today — noted in the media README).Also bumps ffmpeg 8.1 → 8.1.2 (an upstream maintenance release) to pick up security fixes and trim the decode surface further.
Test fixtures are re-encoded to VP9-in-mp4; filenames and every Rust/Python test that consumes them are unchanged.
Pairs with the compliance-side gate in #11632. See OPS-7665 for background.
Validation
configureaccepts every component token (caught one bug:webmis not a separate demuxer token;matroskacovers it) and the build succeeds LGPL-2.1.frames × w × h × 1.5exactly (2×2/10f, 320×240/1f/10f/100f, 3840×2160/10f).container/render.pyrenders the sglang runtime (theenable_media_ffmpeg=trueconsumer) with the new configure block intact.native_packages.yamlffmpeg version bumped to match.Linear: https://linear.app/nvidia/issue/OPS-7665/disable-ffmpeg-decoders-and-add-image-scan
🤖 Generated with Claude Code