fix(Video): don't crash on videos with undecodable audio streams#14746
fix(Video): don't crash on videos with undecodable audio streams#14746bigcat88 wants to merge 1 commit into
Conversation
bigcat88
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
📝 WalkthroughWalkthroughChangesVideo stream handling now skips undecodable audio and template streams without a codec context, logging warnings instead of proceeding with invalid streams. Upload conversion errors are also wrapped in a contextual Estimated code review effort: 🎯 2 (Simple) | ⏱️ ~15 minutes Related issues: None specified Related PRs: None specified Suggested labels: bug, video, error-handling Suggested reviewers: Not enough information to suggest specific reviewers 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
comfy_api_nodes/util/upload_helpers.py (1)
161-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad
except Exceptionin new code.Catching generic
Exceptionhere conflicts with the guideline to prefer specific exception types in new code. That said,save_tocan raise a wide variety of errors (PyAV, OS, ValueError), and this is a top-level boundary meant to translate any conversion failure into a friendly message, so the broad catch is arguably justified here.As per coding guidelines: "Prefer specific exception types when changing new code."
🤖 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_nodes/util/upload_helpers.py` around lines 161 - 168, The new upload conversion boundary in the video save path is catching a generic Exception, which conflicts with the preferred exception-handling guideline. Update the try/except around video.save_to in upload_helpers to catch only the specific failure types it can raise here (for example the relevant PyAV/codec/OS exceptions, or a narrowly scoped tuple of known errors) and still re-raise the same friendly ValueError message with the original error attached. Keep the behavior localized to the save_to conversion flow and avoid broad exception handling unless there is no narrower alternative.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@comfy_api_nodes/util/upload_helpers.py`:
- Around line 161-168: The new upload conversion boundary in the video save path
is catching a generic Exception, which conflicts with the preferred
exception-handling guideline. Update the try/except around video.save_to in
upload_helpers to catch only the specific failure types it can raise here (for
example the relevant PyAV/codec/OS exceptions, or a narrowly scoped tuple of
known errors) and still re-raise the same friendly ValueError message with the
original error attached. Keep the behavior localized to the save_to conversion
flow and avoid broad exception handling unless there is no narrower alternative.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df0c7078-7d3b-494e-b1ac-3d5cb0246f18
📒 Files selected for processing (2)
comfy_api/latest/_input_impl/video_types.pycomfy_api_nodes/util/upload_helpers.py
Signed-off-by: bigcat88 <bigcat88@icloud.com>
bigcat88
force-pushed
the
fix/core/mov-spatial-audio
branch
from
0c316c5 to
15f94d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
comfy_api/latest/_input_impl/video_types.py (1)
441-475: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail remux when the primary video stream can't be templated
The reuse path only logs and skips streams with nocodec_context, so an undecodable video stream can still take this branch and come out audio-only (or empty) instead of failing. Raise a clear error when the video track can’t be reused, rather than letting the picture disappear like a bad magic trick.🤖 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 441 - 475, The remux path in the stream-copy logic is currently skipping streams with missing codec_context, which can let the primary video track disappear silently. Update the reuse_streams branch in the save/remux flow so that the main video stream must be templatable: if the video stream (or any required video track) has no codec_context, raise a clear error instead of continuing. Keep the existing skip behavior only for non-required streams, and use the existing stream handling around container.streams and output_container.add_stream_from_template to locate the fix.Source: Coding guidelines
🤖 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_nodes/util/upload_helpers.py`:
- Around line 161-168: The upload helper is using a broad catch-all around the
video remux/encode path in the save_to call, which should be narrowed if PyAV
exposes a reliable failure type. In upload_helpers.py, review the exception
hierarchy for video.save_to in the remux_video_to_container helper and replace
the generic except Exception with the specific PyAV/ffmpeg errors that can occur
there (and any clearly related I/O/ValueError cases), while keeping the same
chained ValueError message for user-facing context. If the API surface is
uncertain, confirm the documented PyAV error types before changing the handler
so unrelated bugs are not swallowed.
---
Outside diff comments:
In `@comfy_api/latest/_input_impl/video_types.py`:
- Around line 441-475: The remux path in the stream-copy logic is currently
skipping streams with missing codec_context, which can let the primary video
track disappear silently. Update the reuse_streams branch in the save/remux flow
so that the main video stream must be templatable: if the video stream (or any
required video track) has no codec_context, raise a clear error instead of
continuing. Keep the existing skip behavior only for non-required streams, and
use the existing stream handling around container.streams and
output_container.add_stream_from_template to locate the fix.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 68e54ad9-a05a-4d5b-a367-76f12dfaad26
📒 Files selected for processing (2)
comfy_api/latest/_input_impl/video_types.pycomfy_api_nodes/util/upload_helpers.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @alexisrolland.
Found 2 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
2 participants
Videos recorded on recent iPhones with Spatial Audio carry two audio tracks: a stereo AAC fallback and Apple Positional Audio Codec(APAC)
FFmpeg usually has no APAC decoder, so PyAV exposes that stream with
codec_context = NoneVideoFromFilepicked the last audio track - the APAC one. Decoding a packet on a stream without a codec context segfaults the whole process with such error:Three changes:
get_components_internalpicks the last decodable audio stream. A file with only undecodable audio decodes as video-only with a warning instead of crashingsave_toskips streams that have no codec context (they can't be used as an output template) instead of failing the whole saveupload_video_to_comfyapiwraps the conversion so a failure surfaces as a readable node error suggesting to re-export as MP4API Node PR Checklist
Scope
Pricing & Billing
If Need pricing update:
QA
Comms