feat(media): actionable errors for unsupported video/audio codecs (OPS-7779) - #12725
Conversation
Reimplements PR #11872 against the post-codec-removal, post-NVDEC tree (every file it touched was rewritten since; the old branch carries the merged removal PR's history). What a user hits today on a shipped image: vLLM: Failed to load video from ...: No module named 'cv2' SGLang: Failed to process mm items: Error while loading data b'\x00\x00\x00 ftyp...<entire video payload repr>...': No module named 'decord' TRT-LLM: the vendor loader's bare cv2 ImportError frontend (source builds with media-ffmpeg): "Decoder not found" None name the codec or a remedy, and the docstrings in video_loader.py and nvdec_decoder.py claimed an actionable "unsupported codec" error that nothing raised. A new common/multimodal/codec_errors.py builds the messages in one place: the probed codec, the missing package at its validated version bounds (imported from the explicit installer's VALIDATED_SPECS so message and documented install cannot drift), the installer command, and the hardware alternative. H.264/H.265 with NVDEC unavailable is distinguished from genuinely software-only codecs: the former leads with the 'video' driver capability, not a pip install. Per decode entry point: - vLLM video: the two software-decode call sites gain the wrap; the codec probe is hoisted so the message can name it. The error type (RuntimeError, deliberately not ValueError) survives load_video's generic wrap so a deployment gap is not blamed on the request. - SGLang: a codec-aware preflight where the NVDEC fallback hands bytes to SGLang -- H.264/H.265 requests never require decord, so the old blanket preflight would now be wrong. Also exempted from the broad fallback except-clause that would otherwise swallow it and reproduce the deep failure it replaces. Fails before the payload bytes can be embedded in an error message. - TRT-LLM video: wraps both vendor-loader call sites (new coverage; the original PR predates knowing TRT-LLM decodes video at all). - vLLM audio: replaces vLLM's "pip install vllm[audio]" hint, which drags an unpinned stack, with the bounded PyAV install; states that NVDEC never decodes audio. Covers the audio half of the ticket title that the original PR never delivered. - Rust frontend decoder: Decoder::new keeps the original FFmpeg error prominent and adds conditional VP8/VP9 guidance (re-encode command, or send to the backend where H.264/H.265 decode on NVDEC), so malformed input is not misreported as a codec problem. Ships in no image (enable_media_ffmpeg is false everywhere); matters for source builds. An H.264-fixture test pins the message. The three findings from the original PR's review are carried as design constraints: preflight only after payload validation, never imply VP8/VP9 avoids the backend decoder, keep the original error prominent with guidance conditional. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
The actionable unsupported-codec errors single-source their version bounds from the installer's VALIDATED_SPECS, so production files now legitimately contain the module name and the whole-tree string sweep would flag exactly the drift-prevention it exists to protect. Split it by what each part actually guards: no __main__.py may reference the installer at all (entrypoints are where startup happens), nothing outside the installer and its test may CALL install_media_decoders(), and the retired DYN_ENABLE_MEDIA_DECODERS switch may appear nowhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
github-actions
Bot
added
backend::sglang
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
WalkthroughThe change adds typed, actionable decoder errors for missing audio and video dependencies. Common loaders and backend handlers preserve these errors across NVDEC and software fallback paths. Native video decoding adds FFmpeg failure guidance. Tests cover dependency detection, fallback, propagation, and installation messaging. ChangesMedia decoder error handling
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
lib/llm/src/preprocessor/media/decoders/video.rs (1)
231-246: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreserve the original FFmpeg error in the error chain.
anyhow::anyhow!formatseinto a new message. It keeps the display text, but it drops the original error type and source chain. Downstream callers cannot inspect the FFmpeg cause.Wrap the result with
anyhow::Contextoranyhow::Error::new(e).context(...). Keep the existing guidance text in the context. Verify that the repository’s declaredanyhowversion supports the selected wrapper.🤖 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 231 - 246, The Decoder::new error mapping currently formats the FFmpeg error into a new anyhow message and loses its source chain. Update the map_err handling around video_rs::decode::Decoder::new to wrap the original error with anyhow::Context or anyhow::Error::new(e).context(...), preserving the existing codec guidance as context and confirming compatibility with the declared anyhow version.
🤖 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 `@components/src/dynamo/common/multimodal/audio_loader.py`:
- Around line 159-166: Preserve MissingMediaDecoderError in batch APIs: in
components/src/dynamo/common/multimodal/audio_loader.py lines 159-166, import
the exception, retain the first instance during load_audio_batch aggregation,
and re-raise it before the generic aggregate; apply the same retention and
re-raise behavior in components/src/dynamo/common/multimodal/video_loader.py
lines 206-211 for load_video_batch. Add regression tests covering both batch
loaders and their preserved exception type.
In `@components/src/dynamo/common/tests/test_install_media_decoders.py`:
- Around line 489-490: Update the static scan in the test around the offender
collection to parse imports and call nodes, resolve aliases imported from the
installer module, and flag calls through those aliases as well as direct
install_media_decoders calls. Preserve detection of DYN_ENABLE_MEDIA_DECODERS
references and ensure aliased invocations are added to offenders.
In `@components/src/dynamo/sglang/tests/test_sglang_multimodal_video.py`:
- Around line 208-216: Move the importlib.util import used by
_selective_find_spec and the affected test functions to module scope, defining
one shared alias there. Update all references at the noted locations to reuse
that alias and remove function-local import statements, while preserving the
existing find_spec behavior.
In `@components/src/dynamo/trtllm/multimodal_processor.py`:
- Around line 502-529: Handle MissingMediaDecoderError before the generic HTTP
error handler in multimodal video loading, preserving it or mapping it to a
server-side status rather than HTTP 400; update the tests covering the
decoder-missing paths to assert both the actionable message and corrected
server-side classification. Apply the implementation change in
components/src/dynamo/trtllm/multimodal_processor.py (lines 502-529) and the
assertions in
components/src/dynamo/trtllm/tests/test_trtllm_multimodal_processor.py (lines
255-291).
---
Nitpick comments:
In `@lib/llm/src/preprocessor/media/decoders/video.rs`:
- Around line 231-246: The Decoder::new error mapping currently formats the
FFmpeg error into a new anyhow message and loses its source chain. Update the
map_err handling around video_rs::decode::Decoder::new to wrap the original
error with anyhow::Context or anyhow::Error::new(e).context(...), preserving the
existing codec guidance as context and confirming compatibility with the
declared anyhow version.
🪄 Autofix
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: 584ea554-4cc7-4a03-b641-c72f31602628
📒 Files selected for processing (13)
components/src/dynamo/common/multimodal/audio_loader.pycomponents/src/dynamo/common/multimodal/codec_errors.pycomponents/src/dynamo/common/multimodal/nvdec_decoder.pycomponents/src/dynamo/common/multimodal/video_loader.pycomponents/src/dynamo/common/tests/multimodal/test_audio_loader.pycomponents/src/dynamo/common/tests/multimodal/test_codec_errors.pycomponents/src/dynamo/common/tests/multimodal/test_video_loader.pycomponents/src/dynamo/common/tests/test_install_media_decoders.pycomponents/src/dynamo/sglang/request_handlers/multimodal/encode_worker_handler.pycomponents/src/dynamo/sglang/tests/test_sglang_multimodal_video.pycomponents/src/dynamo/trtllm/multimodal_processor.pycomponents/src/dynamo/trtllm/tests/test_trtllm_multimodal_processor.pylib/llm/src/preprocessor/media/decoders/video.rs
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Six findings from the bot review, five of them real: - The TRT-LLM wrap discarded the vendor loader's own text, violating this PR's stated "original error stays prominent" constraint and breaking the pre-existing test that pins it. The builders now take a `cause` appended as "(decoder reported: ...)" -- `raise ... from` preserves it for tracebacks, but HTTP handlers ship only str(exc), so it belongs in the message. Applied at every wrap site (vLLM video/audio, TRT-LLM). - A missing decoder reached clients as HTTP 400 on TRT-LLM. It is deployment configuration, not a bad request: an explicit handler maps MissingMediaDecoderError to 500 before the generic 400 wrap. The pre-existing VP9 test is updated to the new contract (500, vendor text still asserted verbatim, plus the installer guidance). - load_video_batch/load_audio_batch collected failures into a generic Exception, erasing the actionable type. Both now track and re-raise MissingMediaDecoderError after the client-error verdicts, mirroring the existing typed-error priority pattern. - The SGLang preflight used find_spec, so a present-but-broken decoder passed preflight only to fail deep in SGLang -- the same find_spec-vs-real-import lesson the installer already encodes. The probe now really imports (success cached in sys.modules; first request only). - The no-call sweep matched only the literal `install_media_decoders(`, missing aliased imports. It now walks the AST for calls through any binding of the installer module or its functions, with detector tests covering direct, aliased, module-alias and constants-only forms. - Minor: the audio loader's new import was isort-misordered; importlib.util usage in the sglang tests moved behind the reworked probe fake. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
Review analysis on the actionable-error PR: _maybe_nvdec_decoder's preflight is reachable only through the NVDEC-enabled branch of _build_encode_inputs, so deployments with NVDEC off -- a CPU image, DYN_DISABLE_NVDEC, or a gated model type -- passed raw URLs to SGLang unchecked. Those are exactly the deployments most likely to have no decoder at all, and the ones where the deep "No module named 'decord'" with the payload repr embedded would still surface. Preflight video URLs on the disabled path as well; no bytes are fetched there, so the codec cannot be named, but the actionable install guidance beats the blob error. With a software decoder present the URLs pass through unchanged, as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
…ionable-errors-for-unsupported-videoaudio-codecs Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com> # Conflicts: # components/src/dynamo/sglang/request_handlers/multimodal/encode_worker_handler.py
Stdlib imports belong at the top of the file; the selective-import fake only needs the module reference, not a function-local import. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
…pported-videoaudio-codecs
furionw
left a comment
furionw
left a comment
There was a problem hiding this comment.
sorry. mislicked -- reviewing
…ecoder gating Two review findings, both reproduced on the real runtime images first. A data: URI carries the whole media payload inline, so echoing one into an error serialized it to the client and to every log sink: measured on the TRT-LLM image, a 250 KB inline video produced a 683,213-character message -- the payload twice, once from our text and once from HttpStatusError's own format string -- against 457 characters of actual guidance. Add describe_media_source, which labels a data: URI by media type and size and bounds every other source, and use it for each error raised by the video loop. With NVDEC disabled -- the stock configuration, since the embedding cache is off by default -- _build_encode_inputs handed URLs straight back for SGLang to fetch with its own session, so our url policy never applied; and when no decoder imported it answered with deployment configuration before the request's URL was ever checked. Reproduced both on the SGLang image with a link-local metadata URL. Run the policy first, then the decoder gate, and return the validated URLs. Raised by rmccorm4 on #12725. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
…eoaudio-codecs' of https://github.com/ai-dynamo/dynamo into dtokarev/ops-7779-actionable-errors-for-unsupported-videoaudio-codecs Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
…ionable-errors-for-unsupported-videoaudio-codecs Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
video_e_pd_qwen serves its clip from the local test HTTP server over plain http, which the url policy refuses by default. Its model is gated out of NVDEC (_NVDEC_UNSAFE_MODEL_TYPES), so it took the NVDEC-disabled path -- the one that until now handed URLs to SGLang without consulting the policy, which is why this test alone never needed the opt-in. Its sibling video_e_pd_qwen_nvdec runs the same clips on a non-gated model, takes the NVDEC path that already validated, and has carried DYN_MM_ALLOW_INTERNAL=1 all along for exactly this reason. The two now agree. Reproduced and verified on GPU hardware against the real sglang runtime image: without the flag the deployment returns the CI failure verbatim (400 'http:// URLs are not allowed'); with it, both video E/PD variants pass in 84s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
|
Pushed That test serves its clip from the local test HTTP server over plain Its sibling
So the suite had documented the asymmetry without anyone noticing the second half was a policy bypass. The two configs now agree. Reproduced and verified on GPU hardware against the real sglang runtime image (run as root, as CI does, since the config pip-installs decord2): without the flag the deployment returns the CI failure verbatim — I have added a "Behavior change worth knowing on upgrade" section to the PR body: a deployment feeding video over internal
|
All three backend multimodal pages listed http://example.com/image.jpg as a supported media URL, but the url policy denies by default: only https:// and data: pass, and hostnames resolving to private or loopback addresses are refused. Nothing outside the v1.2.0 release notes mentioned DYN_MM_ALLOW_INTERNAL, so an operator serving media from inside the cluster had no documented way to find it. Correct the example to https:// and note the policy and the opt-in. This matters more now that the SGLang NVDEC-disabled video path validates too, where it previously passed URLs through unchecked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
github-actions
Bot
added
the
documentation
Validating a URL resolves its hostname (loop.getaddrinfo) to check the address against the blocked ranges. Running the policy on the NVDEC-disabled path therefore gave three cache tests a DNS lookup for example.com that they never had before, and the CPU test container has no outbound resolution: in CI the lookup blocked until the step's cap killed the container, which read as an infrastructure failure rather than a test one -- the session stopped at 96% with no reported failure, twice, on the same test. Stub validate_media_url in the cache_handler fixture, the way the decoder preflight is already stubbed there: these tests are about caching, not URL policy. The validation-ordering test restores the real function, and its URL is refused on scheme, so it still performs no lookup. Verified on the real sglang image: before, the file fails 3 tests with no network; after, all 25 pass with no network at all, and 32 pass with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
|
Pushed The job stopped at 96% with no reported failure and then That test uses The history lines up exactly: the CPU stage passed on Fix: stub Verified on the real sglang image: before, the file fails 3 tests with no network (all three Worth recording why my GPU verification missed it: I ran those suites with |
Giving the fixture handler a real url policy removed the AttributeError that used to abort _maybe_nvdec_decoder before it fetched. On an NVDEC-capable host the cache tests then issued a real request for their example.com URL, and the event loop waited on it at teardown: measured 400.00s of teardown for test_video_requests_reuse_cached_embeddings against a 0.05s call. In CI that exceeded the step budget, so the container was killed at 96% with no test reported as failing -- the 'custom container implementation failed' signature. Stub fetch_bytes in the cache_handler fixture. The broad except in _maybe_nvdec_decoder turns it into the URL passthrough these tests always expected, which is the behaviour they had before; the seven tests that exercise fetching stub it with their own payload and are unaffected. Also cap the module at 60s. These are sub-second tests, and a stalled teardown should fail one test rather than consume the job's whole step budget -- the failure mode that made this look like a runner fault twice. Measured on the real sglang image, full pre_merge/sglang/gpu_0 selection: 409.47s -> 10.58s, and the 400s teardown no longer appears at all. Suites: 32 passed network-isolated, 32 with network, 52 common. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
|
@rmccorm4 CI is green on Both P1s are implemented and verified on real images:
The three CI failures, all mine:
I called (2) infrastructure and then (3) DNS before measuring properly — a main-vs-branch control and One thing still deliberately out of scope: routing the NVDEC-disabled path through our own fetch (rather than only validating) is filed as OPS-8124. It changes what SGLang receives on the default configuration — the embedding cache is off by default — so it wants its own GPU validation instead of riding along here. Happy to fold it in if you'd rather have it in one PR. Also documented the operator-visible consequence: plain |
7 participants
Summary
Reimplements and supersedes #11872 against the post-codec-removal, post-NVDEC, post-#12051 tree: every decode entry point now raises an error that names the probed codec, the missing package at its validated version bounds (single-sourced from the installer's
VALIDATED_SPECS, so message and documented install cannot drift), thepython -m dynamo.common.utils.install_media_decoders <backend>command, and the hardware alternative.Linear OPS-7779 — the last open prerequisite of the codec-removal un-defer triad (e2e coverage and the explicit installer, #12051, have landed).
Details
Before (reproduced on the real runtime images, GPU box):
ValueError: Failed to load video from …: No module named 'cv2'— codec unnamed, no remedy._maybe_nvdec_decodersilently returned 17,557 raw bytes, which fail inside SGLang asFailed to process mm items: Error while loading data b'\x00\x00\x00 ftyp…<entire payload repr>…': No module named 'decord'.video_loader.py,nvdec_decoder.py) claimed an actionable "unsupported codec" error that nothing raised.After (verified on the same images):
Per entry point:
common/multimodal/codec_errors.py— one message builder for all backends; distinguishes "H.264/H.265 but NVDEC unavailable → grant thevideodriver capability" from software-only codecs.MissingMediaDecoderErroris deliberately RuntimeError, not ValueError: a missing decoder is deployment configuration, and handlers that map ValueError to a client 4xx must not blame the request._decode_video_bytesso both software-decode call sites can name the codec; error type survivesload_video's generic wrap; both lying docstrings fixed.pip install vllm[audio]hint (unpinned stack) with the bounded PyAV spec; the audio half of the ticket title feat(media): actionable errors for unsupported video/audio codecs (OPS-7779) #11872 never delivered.Decoder::newkeeps the original FFmpeg error prominent with conditional VP8/VP9 guidance (ships in no image; source builds only), plus an H.264-fixture test pinning the message.__main__.pyreference / noinstall_media_decoders(call / no retired env var), since error builders now legitimately importVALIDATED_SPECS.All three bot-review findings from #11872 are carried as design constraints: preflight only after payload validation; never imply VP8/VP9 avoids the backend decoder; original error stays prominent with guidance conditional.
Where should the reviewer start?
components/src/dynamo/common/multimodal/codec_errors.py, then the SGLang preflight inencode_worker_handler.py(the one place with re-raise subtleties).Validation
.codec-audit/gpu_validate_codec_errors.sh): repro mode captured both "before" behaviors verbatim on unmodified images; fixed mode shows the messages above end-to-end through the realVideoLoaderand the real SGLang handler (no mocks), plus in-container unit suites: 31 passed in the vLLM image (codec_errors + both loaders), SGLang preflight pair, TRT-LLM wrap test under--gpus all.codec_errorsunit tests also pass locally; suites for the loaders/handlers extended (missing-decoder actionability, type preservation through the wrap chains, preflight positive/negative, codec named + spec quoted fromVALIDATED_SPECS).ruffclean; the Rust change compiles under CI'smedia-ffmpegfeature job (no local ffmpeg dev libs).find_specto real imports (broken native installs = absent); fallback-leg and NVDEC-disabled-path preflights added with unit coverage (test_disabled_nvdec_*,test_decode_failure_without_software_decoder_is_actionable); batch APIs preserveMissingMediaDecoderError; TRT-LLM maps it to HTTP 500 with the vendor text carried in the message; installer-call sweep resolves aliases via AST.origin/main(incl. feat(sglang): support frontend image decoding in multimodal EPD #11880 frontend decoding): the disabled-path preflight now counts only str URL items — pre-decoded frontend variants need no decoder;pre-commit run --all-filespasses on the merged head.Behavior change worth knowing on upgrade
With NVDEC disabled or gated (
_NVDEC_UNSAFE_MODEL_TYPES, e.g. Qwen3-VL), the SGLang encode worker now applies the URL policy to video inputs before handing them on. Previously that path skipped validation entirely, so a plainhttp://or private-address video URL was passed to SGLang and fetched. Such a request now returns 400 naming the knob (set DYN_MM_ALLOW_INTERNAL=1 to enable).This makes the gated path agree with the NVDEC path, which already enforced the same rule — the inconsistency was the defect. Our own suite demonstrated it:
video_e_pd_qwen(gated model, local http clips) passed only because validation was skipped, while its siblingvideo_e_pd_qwen_nvdechas always carriedDYN_MM_ALLOW_INTERNAL=1for exactly this reason. That config is updated in35cdfa5605; deployments serving video over internal http need the same opt-in.Related Issues
Supersedes #11872.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests