Transcode when the output container cannot store the source codec - #15367
Transcode when the output container cannot store the source codec#15367christian-byrne wants to merge 4 commits into
Conversation
Saving a VP8 WebM with format=auto named the file .mp4 and then stream copied VP8 into it, which PyAV rejects. Check the destination muxer's supported codecs before copying and fall back to re-encoding when any source stream cannot be stored.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
🚥 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: 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 `@comfy_api/latest/_input_impl/video_types.py`:
- Around line 508-513: Update the automatic fallback in the surrounding video
save method before calling _save_transcoded: either preserve all subtitle
streams by passing them through in a supported target format, or reject the
fallback with a clear error before truncating or writing path. Ensure
subtitle-bearing inputs cannot silently lose streams, and add coverage for a
subtitle stream combined with an incompatible audio or video codec.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 08ef9ab3-bf8f-4bca-8981-2bc3a7e39206
📒 Files selected for processing (2)
comfy_api/latest/_input_impl/video_types.pytests-unit/comfy_api_test/video_types_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: Run Pylint
- GitHub Check: test (windows-2022)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test
- GitHub Check: Run Pylint
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep changes small, direct, and limited to the narrowest necessary code path and smallest number of files.
Prefer practical fixes, minimal dependencies, and existing repository patterns; remove obsolete, dead, unreachable, or unused code.
Preserve existing APIs, node names, model-loading behavior, file layout, and workflow compatibility unless replacement is explicitly intended.
Core ComfyUI must not add outbound internet requests, telemetry, tracking, reporting, remote configuration, or background network activity. User-authorized model downloads are limited to the requested artifact and must exclude telemetry and unrelated metadata.
Files:
comfy_api/latest/_input_impl/video_types.pytests-unit/comfy_api_test/video_types_test.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Keep state and capability flags on the object that owns the behavior. Prefer explicit parent-owned attributes over probing child objects withgetattr; use child checks only when the child owns the delegated behavior.
Preserve shared method signatures, argument order, return shapes, side effects, and error behavior unless every affected caller and interface is intentionally updated.
Do not add unused compatibility parameters, flags, attributes, constructor options, or model-specific options to shared helpers; keep one-off behavior at the integration boundary.
Normalize third-party return conventions at integration boundaries so core code receives the expected type and shape; avoid undocumented caller-side unwrapping.
Do not addtorch.no_grad,torch.inference_mode, or inference-mode wrappers. Do not add model freeze/unfreeze toggles; only disable globally enabled inference mode when a training path requires gradients.
Remove inference-only training behavior such as dropout while preserving checkpoint and state-dict compatibility; usenn.Identitywhen deleting a module would alter keys or ordering.
Keep imports at module scope except established optional-backend probes or imports required to avoid cycles; avoid unnecessarytry/exceptblocks and use specific exceptions with useful fallbacks.
Do not add workarounds for unsupported library versions, especially PyTorch exception-and-float-cast retries, unless a comment names the exact versions still requiring them.
Let unsupported model formats, invalid quantization metadata, and bad states fail with clear errors instead of silently degrading output.
Match local style, keep comments sparse and useful, and remove comments that merely restate obvious code.
Treat dtype, device placement, VRAM use, and offloading as correctness concerns across CPU, CUDA, ROCm, MPS, DirectML, XPU, NPU, and low-VRAM environments.
Prefer existing ComfyUI and Comfy Kitchen operations, quantization helpers, cast/offload helpe...
Files:
comfy_api/latest/_input_impl/video_types.pytests-unit/comfy_api_test/video_types_test.py
**/*.{py,json}
📄 CodeRabbit inference engine (AGENTS.md)
Treat legacy combo,
io.Combo, andio.DynamicCombovalues affecting filesystem access as untrusted; revalidate them at load/save boundaries withfolder_paths, containment checks, or fixed allowlists.
Files:
comfy_api/latest/_input_impl/video_types.pytests-unit/comfy_api_test/video_types_test.py
**/*.{py,md,txt,json}
📄 CodeRabbit inference engine (AGENTS.md)
Keep warning and info messages short and actionable, remove noisy or misleading logging, and make documentation edits concise, factual, and tied to changed behavior.
Files:
comfy_api/latest/_input_impl/video_types.pytests-unit/comfy_api_test/video_types_test.py
**
⚙️ CodeRabbit configuration file
**: IMPORTANT: Only comment on issues directly introduced by this PR's code changes.
Treat AGENTS.md as mandatory repository policy, not optional style guidance.
Flag PR changes that violate AGENTS.md even when the code is otherwise functional.
In particular, enforce architecture boundaries, dtype/device/memory rules,
interface contracts, import style, no unnecessary try/except blocks, no inline
imports, no outbound internet paths in core ComfyUI, and narrow scoped fixes.
Prefer direct findings over suggestions when a rule is violated. Only ignore
AGENTS.md when it clearly conflicts with a newer explicit maintainer instruction
in the PR.
Do NOT flag pre-existing issues in code that was merely moved, re-indented,
de-indented, or reformatted without logic changes. If code appears in the diff
only due to whitespace or structural reformatting (e.g., removing awith:block),
treat it as unchanged. Contributors should not feel obligated to address
pre-existing issues outside the scope of their contribution.
Files:
comfy_api/latest/_input_impl/video_types.pytests-unit/comfy_api_test/video_types_test.py
🧠 Learnings (1)
📚 Learning: 2026-02-21T14:01:41.482Z
Learnt from: pythongosssss
Repo: Comfy-Org/ComfyUI PR: 12555
File: comfy_extras/nodes_glsl.py:719-724
Timestamp: 2026-02-21T14:01:41.482Z
Learning: In PyOpenGL, bare Python scalars can be accepted for 1-element array parameters by NumberHandler. This means you can pass an int/float directly to OpenGL texture deletion (e.g., glDeleteTextures(tex)) without wrapping in a list. Verify function-specific expectations and ensure types match what the OpenGL call expects; use explicit lists only when the API requires an array.
Applied to files:
comfy_api/latest/_input_impl/video_types.pytests-unit/comfy_api_test/video_types_test.py
🔇 Additional comments (2)
tests-unit/comfy_api_test/video_types_test.py (1)
273-304: LGTM!Also applies to: 307-315, 318-328, 331-339, 341-349
comfy_api/latest/_input_impl/video_types.py (1)
104-104: 🩺 Stability & AvailabilityNo dependency change is required.
The repository already declares
av>=16.0.0, which satisfies the statedOutputContainer.supported_codecsrequirement.> Likely an incorrect or invalid review comment.
| if bit_depth is None: | ||
| bit_depth = source_bit_depth | ||
| if isinstance(path, io.BytesIO): | ||
| path.seek(0) | ||
| path.truncate() | ||
| return self._save_transcoded(container, path, format=format, codec=codec, metadata=metadata, bit_depth=bit_depth, crf=crf) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve subtitles when automatic fallback transcodes.
If any video or audio codec is unsupported, this path calls _save_transcoded. _save_transcoded has no subtitle stream output path. The fallback therefore drops every subtitle stream, including compatible subtitle streams, without an error.
Preserve subtitles in a supported target format, or reject this automatic fallback with a clear error before writing output. Add coverage for a subtitle stream when an incompatible audio or video stream triggers fallback.
🤖 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 `@comfy_api/latest/_input_impl/video_types.py` around lines 508 - 513, Update
the automatic fallback in the surrounding video save method before calling
_save_transcoded: either preserve all subtitle streams by passing them through
in a supported target format, or reject the fallback with a clear error before
truncating or writing path. Ensure subtitle-bearing inputs cannot silently lose
streams, and add coverage for a subtitle stream combined with an incompatible
audio or video codec.
Source: Coding guidelines
OutputContainer.supported_codecs only keeps codecs for which avformat_query_codec returns 1, while add_stream_from_template rejects only a falsy result. Muxers that implement neither a query_codec callback nor a codec_tag table return AVERROR_PATCHWELCOME, so they accept a stream copy that supported_codecs reports as impossible: an h264 mpegts source saved with format=auto was fully re-encoded and silently rehoused in mp4. Attempt the copy and let PyAV answer instead. All streams are added before the first packet is muxed, so a stream the destination rejects is caught before anything is written. Handling it per stream also means a subtitle track the destination cannot store is dropped on its own rather than diverting the video alongside it into a needless re-encode. Log the whole-file re-encode at warning, since it is lossy and discards the extra audio and subtitle streams the transcode path does not carry.
The transcode fallback needs a video stream, so an audio-only source whose codec the destination cannot store failed with "No video stream found in file", hiding the codec that actually stopped the save.
A WebM source with format=MP4 never entered the remux path: "mp4" is absent from the source demuxer's name, so the pre-existing reuse_streams guard already forced a transcode and the test passed against an unfixed tree. A mov source carrying pcm_u8 keeps reuse_streams true and is refused by the mp4 muxer, which is the case the test was meant to describe, and it is the only test that reaches the BytesIO truncate. Also decode the outputs rather than trusting stream headers, encode the audio-fallback source as mpeg4 so the h264 assertion distinguishes a re-encode from a copy, stop pinning the mp4 demuxer's full alias list, and skip rather than error when a build lacks an encoder the sources need.
1 participant
Load Video -> Save Video on a VP8 WebM crashes, because
format=autonames the output after the destination extension (.mp4) but skips the container check that decides whether the source streams can be copied, so VP8 packets get aimed at a muxer that refuses them. Every stream is added before the first packet is muxed, so this attempts the copy and treats PyAV's own refusal as the answer: nothing has been written yet, and the save falls back to re-encoding. Handling the refusal per stream also means a subtitle track the destination cannot store is dropped on its own instead of dragging an otherwise copyable video into a needless re-encode, and codecs the destination does accept still remux losslessly. One caveat: when the refused stream is audio, the whole file is re-encoded and extra audio and subtitle tracks are dropped, since the transcode path carries one video and one audio stream; that is now logged at warning rather than passing silently. Repro is any VP8 WebM through Load Video into Save Video with the defaultformat=auto: