Repository navigation
Conversation
Signed-off-by: Devin <devindd@foxmail.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
hsliuustc0106
added
bug
|
@fhfuih PTAL |
There was a problem hiding this comment.
The implementation looks good to me. But I want to double check with your claim
"Video models such as MiniMax-H3 can generate native audio together with video when generate_sound=true."
I cannot find where minimax h3 pipeline recognize this param (that's probably why I didn't pay attention to this param. ) When I tried H3 with ConfyUI, it always generates with sound even without this param.
Now that seeing this PR, I do notice Cosmos recognize this param though
And as that being said, I didn't yet encounter the 2nd problem you mentioned: "bytes_to_video() may drop the audio during decoding.".
So could you help test 2x2=4 cases of generation using (H3, Cosmos) model and with this toggle (on or off), and attach screen recordings---or other test cases that demonstrate both the generate_sound problem and the audio-dropping problem before and after the fix
|
Thanks for the careful review. You're right about MiniMax-H3 — I re-checked the pipeline and my original wording was inaccurate. MiniMax-H3 currently generates video and audio jointly and does not appear to use Cosmos is different: its pipeline does recognize the sound-generation flag, so the expected behavior should be:
I'll update the PR description to avoid implying that For the second issue, the behavior I observed was reproducible locally with H3: the MP4 returned by the vLLM-Omni server contained a valid audio stream, but the ComfyUI The key difference is: # before
container.decode(video_stream)
container.decode(audio_stream)
# after
container.decode(video_stream, audio_stream)My understanding is that after the first decode loop consumes the container to EOF, the second decode may not yield the interleaved audio packets anymore. This explains why the problem may not reproduce for every media layout, but it did reproduce consistently with the H3-generated MP4s in my environment. I won't be able to provide screen recordings, but I can include the exact configurations and whether an audio stream was present in each generated result. |
There was a problem hiding this comment.
Thanks for the double check. I think it now it makes sense. One last thing: AFAIK if you change a node implementation in ComfyUI, when you import a workflow that references to that node, it still shows the old set of toggles. So could you also
- copy/rsync your updated integration source code to your local comfyui installation
- import the example custom workflows that uses video generation
- delete the main video generation node, re-grag it from the node menu, and set all io connections & fields again like before
- same the current state of the workflow
- find the saved workflow JSON in comfyui directory and copy (overwrite) it here, and commit that to this PR
And if you cannot offer screen recording, but still at least have the final video from the comfyui output node, could you upload them here just for everyone's record? (the no sound old video and the new video). I think you can find a download button from the output video preview control, which should be what's outputted by the decoder
Omni ReviewBot: no human activity for 20 days@Devindan this pull request has had no human commit, comment or review since 2026-08-19. 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. |
| height: int, | ||
| fps: int, | ||
| num_frames: int, | ||
| generate_sound: bool, |
There was a problem hiding this comment.
Update the video tests to pass generate_sound; all six existing cases now raise TypeError.
Omni ReviewBot: no human activity for 7 days@Devindan 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 14 days@Devindan 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 zcode (GLM-5.3-Flash) under experiment |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: e71390ce-85f1-47b6-a575-40eec00ca07e) — check |
2 similar comments
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: e71390ce-85f1-47b6-a575-40eec00ca07e) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: e71390ce-85f1-47b6-a575-40eec00ca07e) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 4fbd34e2-eb67-4d68-b02b-39038e167c02) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 9c005501-f887-4af4-84c8-f5fdeea007e3) — check |
vllm-omni-review-bot
left a comment
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
Changes since the previous review
- 1 new inline finding(s); 1 finding(s) below.
CI at
66f18cbe8cfd(2026-10-09T13:41:20.658594+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Note: The assigned review arm
strict/zcode/GLM-5.3-Flashcould not complete this review, so it was produced by the fallback armdirect/cursor/auto. It is excluded from the routing experiment.
Full review analysis
PR description
The ComfyUI Generate Video node gains a generate_sound toggle (default off) and VLLMOmniClient.generate_video() always posts it as a top-level /v1/videos multipart field ("true" or "false"). bytes_to_video() now demuxes the chosen video stream and the last audio stream in one container.decode(...) pass, then builds the ComfyUI VIDEO from those frames, so an MP4 that already contains audio is no longer decoded to EOF on video before audio is read. The flag only changes the request; this diff does not teach MiniMax-H3 to honor it, and audio already present in the response is kept either way.
Change flow
flowchart LR
Widget["[NEW] generate_sound widget"]:::new
Client["[CHANGED] generate_video multipart field"]:::changed
API["[EXISTING] POST /v1/videos MP4"]:::existing
Decode["[CHANGED] bytes_to_video joint decode"]:::changed
Out["[EXISTING] ComfyUI VIDEO"]:::existing
Widget --> Client --> API --> Decode --> Out
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
Findings
- [P1] Required generate_sound breaks existing Generate Video callers —
apps/ComfyUI-vLLM-Omni/comfyui_vllm_omni/nodes.py:199
Existing thread: #5905 (comment)
Evidence for Required generate_sound breaks existing Generate Video callers
VLLMOmniGenerateVideo.generate inserts generate_sound: bool with no default ahead of the previous optional arguments. VLLMOmniClient.generate_video already defaults the same flag to False, but the node method does not. Any existing call that still passes negative_prompt, frame, or later arguments by keyword and omits generate_sound now raises TypeError before the request is built. This three-file diff does not update callers or tests. Give the node argument generate_sound: bool = False so it matches the widget default and the client, and update the video-node tests to pass the flag explicitly.
🤖 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!
|
|
||
| images = torch.stack(frames, dim=0) | ||
| frame_rate = Fraction(video_stream.average_rate) if video_stream.average_rate else Fraction(1) | ||
| for frame in container.decode(*streams): |
There was a problem hiding this comment.
[P2] Audio-preserving decode has no regression pin
Evidence and suggested fix
The bugfix replaces sequential container.decode(video_stream) then container.decode(audio_stream) with one container.decode(*streams) loop at this line. The PR testing section only reports a manual MiniMax-H3 / Cosmos run on 2x H800. The changed-file set adds no test that builds an MP4 with both streams, runs bytes_to_video, and asserts a non-empty waveform, nor a video-only control. Without that pin, a later return to two decode passes can drop audio again while ComfyUI unit tests still pass. Add a local PyAV fixture for both layouts on bytes_to_video.
Omni ReviewBot: finding feedback[p1] Required generate_sound breaks existing Generate Video callers — See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
|
@Devindan Are you planning to continue work on this PR? Please reply by 2026-10-19 10:42:13 UTC. A yes or a brief plan is enough; code changes do not need to be finished within these three days. If you are waiting on a review or other maintainer action, please let us know. If there is no reply, new commit, or maintainer takeover by then, we will recheck and close this PR as inactive. It can be reopened when work resumes. |
5 participants
Summary
Fix audio handling in the
ComfyUI-vLLM-Omnivideo generation workflow.This PR addresses two related issues:
/v1/videosgenerate_soundoption in the ComfyUIGenerate Videonode and forwards it to the vLLM-Omni API.VIDEOobjects.Problem
Video models such as MiniMax-H3 can generate native audio together with video when
generate_sound=true.However, the current ComfyUI integration has two problems:
Generate Videodoes not expose or forward thegenerate_soundrequest parameter.bytes_to_video()may drop the audio during decoding.The second issue is caused by decoding the streams sequentially from the same PyAV container:
followed by:
After the video stream has been fully decoded, the container has already been consumed to EOF, so the subsequent audio decode may return no frames.
As a result, the generated ComfyUI
VIDEOobject contains the video frames but has no audio.Changes
Add
generate_soundsupportAdd a
generate_soundboolean option to theGenerate Videonode and forward it throughVLLMOmniClient.generate_video()as a top-level multipart field for/v1/videos.The default remains
false, matching the API's existing behavior.Preserve audio during video decoding
Update
bytes_to_video()to decode the selected video and audio streams together in a single pass:Video and audio frames are then handled according to their frame type.
This avoids consuming the container before the audio stream is decoded.
Testing
Tested end-to-end with:
Verified the following cases:
generate_sound=trueproduces a ComfyUIVIDEOobject with working generated audio.bytes_to_video().generate_sound=falsecontinues to generate video without audio normally.Scope
The decoding fix is not specific to MiniMax-H3. It should apply to any video generation backend that returns an MP4 containing both video and audio streams through the vLLM-Omni video API.