feat(minimax-h3): integrate native video-audio apps with V2 - #523
feat(minimax-h3): integrate native video-audio apps with V2#523jmccaffrey-nv wants to merge 3 commits into
Conversation
|
/ok to test 9744c5f |
|
/ok to test 227491a |
|
/ok to test 85308e3 |
Greptile SummaryThe PR ports MiniMax H3 text-, first-frame-, and reference-conditioned video-audio generation onto the V2 application/runtime contract.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the available follow-up review scope. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[MiniMax H3 V2 application] --> B[Native model inference]
B --> C[Video frames and synchronized PCM]
C --> D[V2 session and presentation]
D --> E[Transactional MP4 sink]
E --> F[Stage H.264 video]
E --> G[Stage interleaved PCM]
F --> H[External FFmpeg mux]
G --> H
H --> I[Atomic MP4 publication]
Reviews (2): Last reviewed commit: "docs(minimax-h3): record review validati..." | Re-trigger Greptile |
|
5 second t2v outputs : 10-rooftop-robot-jazz.mp412-lunar-greenhouse.mp408-storm-lighthouse.mp4 |
In-depth review: H3 model integrationReviewed at Overall: the V2-side design is right. The audio contract is model-neutral and lands where it belongs ( The findings below are ordered by severity. Sections 3 and 4 answer the SLoC and reuse questions directly. 1. Correctness1.1 —
The repo already has the 3.10-safe idiom for this, twice: 1.2 — FlashVSR silently gains an
This is blast radius outside the H3 scope. Either keep a mediapy fallback when 1.3 —
rate = stream.get("avg_frame_rate") or stream["r_frame_rate"]
fps = float(Fraction(rate))
Try each candidate in turn and take the first that parses to a positive finite rate. 1.4 —
Probe the frame count during 1.5 —
1.6 — The runtime may call
Either document it on the protocol and in the README, or set the flag in a 1.7 — Partial commit is possible across multiple abortable sinks. In 2. Efficiency and smaller notes
3. Can the SLoC be reduced?Where the 14,895 lines actually go:
First, fair context: 7,000 lines of native model source is in band for this repo. flashvsr is 9,979, sana 9,786, lingbot 7,862. Two VAEs, a 50-layer joint transformer, and Qwen3-VL conditioning is simply that much code, and it is not where the reducible mass is. The reducible mass is in four places: (a) The validation layer — realistically 600-900 lines. 595 lines of the native package sit in dedicated Keep the boundary checks — (b) The repeated cleanup idiom — ~60 lines → ~10. (c) Duplicated test doubles — ~180 lines, and two copies already drifting. (d) Realistic total: roughly 1,000-1,400 lines, most of it in tests and validation, without removing a single capability or a single meaningful guard. 4. Could it lean harder on FlashDreams common components?Already done well, and worth saying: The gaps: 4.1 — 4.2 — There is no single host-FFmpeg locator, and this PR makes that worse. 4.3 — 4.4 — Two 4.5 — Worth an explicit answer: should the H3 VAEs sit on 4.6 — 4.7 — A Suggested order
Nice work on the transactional MP4 path and on |
|
/ok to test 81c68f4 |
|
Addressed the actionable findings from the Claude review, then re-reviewed the patch with three independent agents. Fixed in this PR
Intentionally retained or deferred
VerificationAt No new GPU run was needed for this review batch because it does not change native H3 math, weights, scheduling, or device placement. The immutable RTX PRO artifacts/performance remain explicitly anchored to the real-weight validation head in |
ArielG-NV
left a comment
There was a problem hiding this comment.
Suggest splitting PR for simplicity.
Reviewed everything except for integrations/integrations_v2
#517 - We plan to formalize and "make our file-structure less of a mess" - I wonder if the H3 implementation code should move into this now?
| @final | ||
| def presented_model_audio(self) -> AudioOutput | None: | ||
| """Return the current model chunk's audio payload at most once.""" | ||
| return self._presentation_manager.presented_audio() | ||
|
|
||
|
|
||
| def _contains_close(events: UserInputEvents) -> bool: |
There was a problem hiding this comment.
Maybe we should just pull out audio support from this PR, merge that with minimal demo on how to use -- then come back to this PR as a dependency (shrink the monumental amount of changes in this PR by hoisting some out into a sensible separate easier to review PR)
| @runtime_checkable | ||
| class AbortableOutputSink(OutputSink, Protocol): | ||
| """An output sink that can discard an incomplete session atomically. | ||
|
|
||
| Both terminal operations must be idempotent. The runtime calls ``close`` | ||
| only for a successful run. It calls ``abort`` after cancellation or failure, |
There was a problem hiding this comment.
This class seems to be unnecessary?
- Adds special handling for when we fail during
write==> this should just lead to the app closing (close) - Adds special handling for when we fail during
open/close==> If this logic is needed we can consider just addingabort()/reset()toOutputSink
| Only one channel in a model chunk may carry audio. The presentation manager | ||
| makes that payload available once, with the chunk's first presented frame. |
There was a problem hiding this comment.
Only one channel in a model chunk may carry audio.
Why? isn't the data tied to StepResult.audio. We can just... allow all layers to contain
and composite by default (BlitModelOutputToScreenLoop)
| """ | ||
| width, height, fps = _probe_video_stream(path) | ||
| command = [ | ||
| _find_ffmpeg_binary(), |
There was a problem hiding this comment.
we do not embed ffmpeg.
maybe for something core like this we should use something we can embed in our repo like moviepy or opencv.
There was a problem hiding this comment.
Roger, we can't distribute nor link ffmpeg . Call as separate process and users install it is lawerly OK but not ideal, I'll try to find a less encumbered alternative which doesn't itself depend on ffmpeg .
There was a problem hiding this comment.
moviepy and opencv both depend on ffmpeg .
There is an accelerated PyNvVideoCodec , but not all target NV GPUs have the enc/dec HW
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. |
There was a problem hiding this comment.
we only need the first 2 lines of the liscence
| @@ -0,0 +1,31 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
only need short liscence
| def add_exception_note(error: BaseException, note: str) -> None: | ||
| """Add diagnostic context when the running Python supports exception notes. | ||
|
|
||
| ``BaseException.add_note`` was added in Python 3.11, while FlashDreams | ||
| supports Python 3.10. Cleanup must never replace the primary failure merely | ||
| because that compatibility method is absent. | ||
| """ | ||
| add_note = getattr(error, "add_note", None) | ||
| if callable(add_note): | ||
| add_note(note) | ||
|
|
There was a problem hiding this comment.
Do we really need this file/workaround?
- we should maybe consider moving the pytoml to 3.12 - we already use it in most integrations... we just never moved some of them formally to 3.12
Signed-off-by: Jonathan McCaffrey <jmccaffrey@nvidia.com>
Signed-off-by: Jonathan McCaffrey <jmccaffrey@nvidia.com>
Signed-off-by: Jonathan McCaffrey <jmccaffrey@nvidia.com>
|
Stack split in progress. The fully validated pre-split head is 81c68f4 and is being preserved as dev/jmccaffrey/minimax-h3-integ-v2-pre-split-81c68f46. Model-neutral V2 synchronized-audio/runtime changes are moving to a new prerequisite PR. This PR will retain the H3 review thread, be rebuilt on the exact audio prerequisite head, and then be retargeted to that branch. Existing review links and GPU artifact hashes remain valid historical evidence; no H3 math, weights, scheduling, or device-placement code is changing in the split. |
81c68f4 to
3cfdd23
Compare
|
Stack split complete on H3 head 3cfdd23, based on audio prerequisite #526 head 264840e. The visible range is 3 signed-off H3 commits and 43 H3/input/metadata paths. Local: focused H3/media suite 138 passed; full ci_cpu 2,100 passed, 2 skipped, 346 deselected; all pre-commit hooks and all three distribution builds passed. Fresh GitHub: CPU 8m20s, GPU 16m21s, WorldLens 12m54s, OmniDreams 15m14s, LingBot 24m23s, plus docs/REUSE/OSRB—all passed. The PR remains draft and mergeable. |
Summary
Closes #443.
Depends on #526, which contains the model-neutral V2 synchronized-audio and transactional MP4 runtime. This draft is temporarily based on the exact #526 branch head so the visible diff is H3-only.
Split history
The original combined head 81c68f4 passed all local and GitHub CPU/GPU checks. It is preserved at branch dev/jmccaffrey/minimax-h3-integ-v2-pre-split-81c68f46. Generic runtime/audio review fixes moved to #526; H3 and media-input review fixes remain here. The rebuilt combined tree is identical except that one unrelated legacy runner_io write-video -nostdin line was omitted. No H3 math, weights, scheduling, or device-placement behavior changed.
Validation
Split H3 head: 3cfdd23.
The eight Apache-2.0 native components modified from the pinned Hugging Face Diffusers revision retain their applicable upstream copyright inline and are enumerated in both REUSE.toml and THIRD-PARTY-NOTICES. Model weights are downloaded from the pinned upstream revision and are not redistributed. Host FFmpeg/FFprobe executables are invoked externally and are not bundled or linked.
Real-weight RTX PRO results
Eight real-weight MP4s were generated on an NVIDIA RTX PRO 6000 Blackwell Workstation Edition. Every artifact decoded end-to-end through both streams and probed as H.264 High/yuv420p at 24 fps plus AAC-LC 32 kHz stereo. Exact commands, hashes, probes, environment, codec evidence, and the complete gallery record are in integrations_v2/minimax_h3/ROLLOUT.md.
The 15-second run produces 362 frames and aligns the padded AAC stream within 0.667 ms of the written-video duration. Real-weight results remain tied to artifact-validation head 4e91a25, preserved through the archive branch above.
Notes