Repository navigation
[Bugfix][ComfyUI] Fix audio track silently dropped in bytes_to_video - #6782
amchen2310 wants to merge 1 commit into
Conversation
Decoding the video stream first exhausts the PyAV demuxer, so the subsequent container.decode(audio_stream) call always yields zero frames and the audio track is silently dropped from generated videos. Decode the selected video and audio streams in a single demux pass instead. Only the chosen streams are decoded, so containers with extra audio tracks or cover-art video tracks are handled safely. Verified with an H3 t2va mp4 (h264 + aac 32kHz): before the fix the returned VideoInput had audio=None; after the fix the audio waveform (1, 2, 165888) @ 32 kHz is present and SaveVideo muxes the aac track.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Omni ReviewBot: no human activity for 9 days@amchen2310 this pull request has had no human commit, comment or review since 2026-08-30. Per repository policy it may be closed if it stays inactive. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Add remote H3 generation with frame upscaling and preservation of the returned audio and FPS. Include UI/API workflows, aspect-ratio forwarding, validation commands and CPU regression coverage. The bytes_to_video audio fix is taken from YichenLiu (@amchen2310), PR vllm-project#6782, commit f1ab717. Its original implementation is retained with the project-required SPDX header. Refs: vllm-project#7380 Signed-off-by: xuezhihao <89690814+xuexueligao@users.noreply.github.com>
|
I validated this fix E2E with MiniMax-H3 Ref2VA through the ComfyUI workflow. Before the fix, the generated MP4 files had no audio stream even though the H3 API reported audio_present=True. After applying the single-pass PyAV demux fix, two new videos were generated successfully:
Both videos were generated through the ComfyUI workflow and audibly contain audio. This confirms that the fix preserves H3 audio in the ComfyUI output path. |
Omni ReviewBot: no human activity for 8 days@amchen2310 this pull request has had no human commit, comment or review since 2026-09-15. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Omni ReviewBot: no human activity for 15 days@amchen2310 this pull request has had no human commit, comment or review since 2026-09-15. Please consider marking this PR as draft until work can resume. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
1 actionable finding(s).
CI at
f1ab717a9692(2026-10-10T03:12:53.781182+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 1 finding(s) below |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [claim-verified] 165888/32000 = 5.184s, consistent with the PR/LinzeShi ~5.17s H3 clip; not re-run here.
- [claim-verified] 165888/32000=5.184s ~ reviewer 5.17s; 165888/1024=162 matches the PR's expected audio-frame count
- [validated] format.py:155-166: two-pass EOF root cause is addressed by decoding video_stream and audio_stream together; residual this does NOT cover: CI pin, and audio_stream-set/audio_frames-empty still returns audio=None with no log.
- [validated] producer/consumer: /v1/videos mux_av_video_audio_bytes writes AAC fltp then all video packets first (media_utils.py:250-269); consumer now demuxes both streams and builds waveform (B,C,T)+sample_rate matching AudioInput.
- [validated] consumers still hold: api_client.py:406 generate_video -> bytes_to_video; format.py:214 base64_to_video. New non-None audio is the intended SaveVideo behavior, not a stale caller.
- [sweep] rebase/merge: single commit f1ab717, no merge commit on the timeline.
1 actionable finding(s).
Verdict: REQUEST CHANGES
Findings
- **[P1] This head's only production change is the single-pass
container.decode(*stream…** —apps/ComfyUI-vLLM-Omni/comfyui_vllm_omni/utils/format.py`
Evidence for This head's only production change is the single-pass `container.decode(*stream…
This head's only production change is the single-pass container.decode(*streams) in bytes_to_video (format.py:166). A revert to two-pass decode(video) then decode(audio) would drop audio at demux EOF and still pass CI: no test*.py file calls bytes_to_video. Unchanged by this diff: tests/e2e/features/comfyui/conftest.py:69 VideoFromComponents=lambda _: VideoInput(b"mock_video_from_components") discards components, and _build_diffusion_video_output (test_comfyui_integration.py:237) muxes images=[video_frames] only. Those tests are marked core_model and cpu (test_comfyui_integration.py:52) and run in Simple · Other Test (.buildkite/cuda/test-ready.yml:32). Add a CPU test that muxes a tiny h264+aac payload (mux_av_video_audio_bytes already used at test_video_api_utils.py:458-466), calls bytes_to_video, and asserts a non-empty waveform — capture VideoFromComponents kwargs instead of discarding them (the original two-pass path yields audio=None).
Evidence: Trigger: A/V bytes through bytes_to_video; two-pass decode(video) then decode(audio) hits demux EOF and yields audio=None with no exception. This head fixes that with a single pass, but CI cannot catch a revert. format.py:166 for frame in container.decode(*streams): (the untested fix). format.py:202 return InputImpl.VideoFromComponents(components) — audio lives only in these kwargs. Unchanged by this diff, present in the PR-time tree: tests/e2e/features/comfyui/conftest.py:69 VideoFromComponents=lambda _: VideoInput(b"mock_video_from_components") — discards components, so waveform is never asserted. Unchanged: test_comfyui_integration.py:237 images=[video_frames], — helper muxes frames only, no audio. Unchanged: test_comfyui_integration.py:52 pytestmark = [pytest.mark.core_model, pytest.mark.cpu] plus .buildkite/cuda/test-ready.yml:32 pytest -sv tests/ -m 'core_model and cpu' --ignore=tests/diffusion --ignore=tests/model_executor --ignore=tests/entrypoints --ignore=tests/engine — that lane runs these tests and would stay green. Repo-wide test*.py grep for bytes_to_video is empty. Unchanged helper already builds h264+aac: test_video_api_utils.py:458-463 with_audio = media_utils.mux_av_video_audio_bytes( / audio_waveform=np.zeros((1, 1024), dtype=np.float32),.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
Omni ReviewBot: finding feedback**[p1] This head's only production change is the single-pass See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: no human activity for 32 days@amchen2310 this pull request has had no human commit, comment or review since 2026-09-15. Please consider closing this PR if the work is no longer planned. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Summary
In
apps/ComfyUI-vLLM-Omni,bytes_to_video()decodes the video stream first and only then callscontainer.decode(audio_stream). Once the video decode has run to completion, the PyAV demuxer position is at EOF, so the seconddecode()yields zero packets — the audio branch collects no frames and the returnedVideoInputalways hasaudio=None. As a result, videos generated by omni-modal models (e.g. MiniMax H3, which synthesizes a native audio track) lose their sound when saved via the ComfyUISaveVideonode.Reproduction
Decode any mp4 that contains both video and audio streams:
Fix
Decode the selected video and audio streams in a single demux pass. Only the chosen streams are decoded, so containers with extra audio tracks or cover-art video tracks stay safe (unlike a bare
container.decode()over all streams).Verification
With an H3 t2va mp4 (h264 832×480 + aac 32 kHz stereo, produced by vllm-omni
/v1/videos):audio=None, saved mp4 has no audio streamaudio={'waveform': (1, 2, 165888), 'sample_rate': 32000}, andSaveVideomuxes the aac track into the outputaudio=Nonewithout errors