fix(Video): stream the video transcode instead of buffering every frame in RAM (CORE-353) (CORE-351)#14813
Conversation
968f73f to
1d595d4
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds shared helpers for audio stream selection, audio parameter probing, metadata writing, and MP4/H.264 output options. It refactors trim-window handling across frame counting and component extraction, and routes transcoding through 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.
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)
248-267: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
get_frame_count()returns 1 whendurationis 0.With the switch to
get_active_trim_window(),end_ptsis now computed unconditionally (Line 251). When there is no trimduration(the common untrimmed case that falls through to this last-resort decode-and-count path because frame metadata was unusable),end_pts == start_pts, so the second loop at Line 264 breaks on the very first frame and this returnsframe_count = 1for the whole video.Both
_save_transcoded()(Lines 523/625) andget_components_internal()(Line 348) correctly gate the end condition onduration; this path should match them.🐛 Gate the end cap on
durationstart_time, duration = self.get_active_trim_window() frame_count = 1 start_pts = int(start_time / video_stream.time_base) - end_pts = int((start_time + duration) / video_stream.time_base) + end_pts = int((start_time + duration) / video_stream.time_base) if duration else None container.seek(start_pts, stream=video_stream)for frame in frame_iterator: - if frame.pts >= end_pts: + if end_pts is not None and frame.pts >= end_pts: break frame_count += 1 return frame_count🤖 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 248 - 267, get_frame_count() is incorrectly capping the decode loop even when there is no trim duration, which makes untrimmed videos return 1 frame. Update the get_frame_count() logic in the frame-count fallback path to mirror _save_transcoded() and get_components_internal(): compute the end boundary only when duration is present, and otherwise count frames until exhaustion. Keep using get_active_trim_window(), but make the second loop’s stop condition conditional on duration so the untrimmed case is handled correctly.
🤖 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 `@tests-unit/comfy_api_test/video_types_test.py`:
- Around line 260-283: The audio setup in create_transcode_source is hardcoded
to build s16 mono frames, which breaks when
test_save_to_transcode_trims_audio_in_stream_time_base_units passes an AAC
audio_codec. Update the audio frame generation path in create_transcode_source
so it matches each stream’s encoder sample format, using the stream’s format or
an AudioResampler for non-PCM codecs like AAC. Keep the existing audio stream
loop and muxing flow, but ensure the frames handed to stream.encode are
compatible with the selected audio_codec.
---
Outside diff comments:
In `@comfy_api/latest/_input_impl/video_types.py`:
- Around line 248-267: get_frame_count() is incorrectly capping the decode loop
even when there is no trim duration, which makes untrimmed videos return 1
frame. Update the get_frame_count() logic in the frame-count fallback path to
mirror _save_transcoded() and get_components_internal(): compute the end
boundary only when duration is present, and otherwise count frames until
exhaustion. Keep using get_active_trim_window(), but make the second loop’s stop
condition conditional on duration so the untrimmed case is handled correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: af381a80-a01a-4725-afe0-9155b0c4d011
📥 Commits
Reviewing files that changed from the base of the PR and between ffbecff and 1d595d4a703515664157849daeaee88d7ca47a1c.
📒 Files selected for processing (2)
comfy_api/latest/_input_impl/video_types.pytests-unit/comfy_api_test/video_types_test.py
📜 Review details
⚠️ CI failures not shown inline (3)
GitHub Actions: Execution Tests / 0_test (macos-latest).txt: fix(Video): stream the video transcode instead of buffering every frame in RAM
Conclusion: failure
n alembic.autogenerate.tables
�[32m[INFO]�[0m setup plugin alembic.autogenerate.types
�[32m[INFO]�[0m setup plugin alembic.autogenerate.constraints
�[32m[INFO]�[0m setup plugin alembic.autogenerate.defaults
�[32m[INFO]�[0m setup plugin alembic.autogenerate.comments
�[32m[INFO]�[0m Adding extra search path custom_nodes /Users/runner/work/ComfyUI/ComfyUI/tests/execution/testing_nodes
�[32m[INFO]�[0m Setting output directory to: /Users/runner/work/ComfyUI/ComfyUI/tests/inference/samples
�[32m[INFO]�[0m Found comfy_kitchen backend cuda: {'available': False, 'disabled': True, 'unavailable_reason': 'Extension file not found: /Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/site-packages/comfy_kitchen/backends/cuda/_C.abi3.so', 'capabilities': []}
�[32m[INFO]�[0m Found comfy_kitchen backend triton: {'available': False, 'disabled': True, 'unavailable_reason': "ImportError: No module named 'triton'", 'capabilities': []}
�[32m[INFO]�[0m Found comfy_kitchen backend eager: {'available': True, 'disabled': False, 'unavailable_reason': None, 'capabilities': ['adaln', 'apply_rope', 'apply_rope1', 'apply_rope_split_half', 'apply_rope_split_half1', 'dequantize_int8_convrot_weight', 'dequantize_int8_convrot_weight_dtype', 'dequantize_int8_simple', 'dequantize_int8_simple_dtype', 'dequantize_mxfp8', 'dequantize_nvfp4', 'dequantize_per_tensor_fp8', 'gemv_awq_w4a16', 'int8_linear', 'quantize_and_rotate_rowwise', 'quantize_int8_convrot_weight', 'quantize_int8_rowwise', 'quantize_int8_tensorwise', 'quantize_mxfp8', 'quantize_nvfp4', 'quantize_per_tensor_fp8', 'quantize_svdquant_w4a4', 'scaled_mm_mxfp8', 'scaled_mm_nvfp4', 'scaled_mm_svdquant_w4a4', 'stochastic_rounding_fp8']}
�[32m[INFO]�[0m Checkpoint files will always be loaded safely.
�[32m[INFO]�[0m Total VRAM 7168 MB, total RAM 7168 MB
�[32m[INFO]�[0m pytorch version: 2.12.1
�[32m[INFO]�[0m Mac Version (26, 4)
�[32m[INFO]�[0m Set vram state to: DISABLED
�[32m[INFO]�[0m Device: cpu
�[32m[INFO]�[0m Us...
GitHub Actions: Execution Tests / 1_test (windows-latest).txt: fix(Video): stream the video transcode instead of buffering every frame in RAM
Conclusion: failure
_not_found[server0] PASSED
�[32m[INFO]�[0m setup plugin alembic.autogenerate.schemas
�[32m[INFO]�[0m setup plugin alembic.autogenerate.tables
�[32m[INFO]�[0m setup plugin alembic.autogenerate.types
�[32m[INFO]�[0m setup plugin alembic.autogenerate.constraints
�[32m[INFO]�[0m setup plugin alembic.autogenerate.defaults
�[32m[INFO]�[0m setup plugin alembic.autogenerate.comments
�[32m[INFO]�[0m Adding extra search path custom_nodes D:\a\ComfyUI\ComfyUI\tests\execution\testing_nodes
�[32m[INFO]�[0m Setting output directory to: D:\a\ComfyUI\ComfyUI\tests\inference\samples
�[32m[INFO]�[0m Found comfy_kitchen backend cuda: {'available': False, 'disabled': True, 'unavailable_reason': 'CUDA not available on this system', 'capabilities': []}
�[32m[INFO]�[0m Found comfy_kitchen backend triton: {'available': False, 'disabled': True, 'unavailable_reason': "ImportError: No module named 'triton'", 'capabilities': []}
�[32m[INFO]�[0m Found comfy_kitchen backend eager: {'available': True, 'disabled': False, 'unavailable_reason': None, 'capabilities': ['adaln', 'apply_rope', 'apply_rope1', 'apply_rope_split_half', 'apply_rope_split_half1', 'dequantize_int8_convrot_weight', 'dequantize_int8_convrot_weight_dtype', 'dequantize_int8_simple', 'dequantize_int8_simple_dtype', 'dequantize_mxfp8', 'dequantize_nvfp4', 'dequantize_per_tensor_fp8', 'gemv_awq_w4a16', 'int8_linear', 'quantize_and_rotate_rowwise', 'quantize_int8_convrot_weight', 'quantize_int8_rowwise', 'quantize_int8_tensorwise', 'quantize_mxfp8', 'quantize_nvfp4', 'quantize_per_tensor_fp8', 'quantize_svdquant_w4a4', 'scaled_mm_mxfp8', 'scaled_mm_nvfp4', 'scaled_mm_svdquant_w4a4', 'stochastic_rounding_fp8']}
�[32m[INFO]�[0m Checkpoint files will always be loaded safely.
�[32m[INFO]�[0m Total VRAM 16379 MB, total RAM 16379 MB
�[32m[INFO]�[0m pytorch version: 2.12.1+cpu
�[32m[INFO]�[0m Set vram state to: DISABLED
�[32m[INFO]�[0m Device: cpu
�[32m[INFO]�[0m Using sub quadratic optimization for attention, if you hav...
GitHub Actions: Execution Tests / 2_test (ubuntu-latest).txt: fix(Video): stream the video transcode instead of buffering every frame in RAM
Conclusion: failure
[0m �[32mPrompt executed in 0.00 seconds�[0m
�[32m[INFO]�[0m got prompt
�[32m[INFO]�[0m �[32mPrompt executed in 0.00 seconds�[0m
tests/execution/test_execution.py::TestExecution::test_jobs_api_pagination[server0] PASSED
�[32m[INFO]�[0m got prompt
�[32m[INFO]�[0m �[32mPrompt executed in 0.00 seconds�[0m
�[32m[INFO]�[0m got prompt
�[32m[INFO]�[0m �[32mPrompt executed in 0.00 seconds�[0m
�[32m[INFO]�[0m got prompt
�[32m[INFO]�[0m �[32mPrompt executed in 0.00 seconds�[0m
tests/execution/test_execution.py::TestExecution::test_jobs_api_sorting[server0] PASSED
�[32m[INFO]�[0m got prompt
�[32m[INFO]�[0m �[32mPrompt executed in 0.00 seconds�[0m
tests/execution/test_execution.py::TestExecution::test_jobs_api_status_filter[server0] PASSED
�[32m[INFO]�[0m got prompt
�[32m[INFO]�[0m �[32mPrompt executed in 0.00 seconds�[0m
tests/execution/test_execution.py::TestExecution::test_get_job_by_id[server0] PASSED
tests/execution/test_execution.py::TestExecution::test_get_job_not_found[server0] PASSED
�[32m[INFO]�[0m setup plugin alembic.autogenerate.schemas
�[32m[INFO]�[0m setup plugin alembic.autogenerate.tables
�[32m[INFO]�[0m setup plugin alembic.autogenerate.types
�[32m[INFO]�[0m setup plugin alembic.autogenerate.constraints
�[32m[INFO]�[0m setup plugin alembic.autogenerate.defaults
�[32m[INFO]�[0m setup plugin alembic.autogenerate.comments
�[32m[INFO]�[0m Adding extra search path custom_nodes /home/runner/work/ComfyUI/ComfyUI/tests/execution/testing_nodes
�[32m[INFO]�[0m Setting output directory to: /home/runner/work/ComfyUI/ComfyUI/tests/inference/samples
�[32m[INFO]�[0m Found comfy_kitchen backend triton: {'available': False, 'disabled': True, 'unavailable_reason': "ImportError: No module named 'triton'", 'capabilities': []}
�[32m[INFO]�[0m Found comfy_kitchen backend eager: {'available': True, 'disabled': False, 'unavailable_reason': None, 'capabilities': ['adaln', 'apply_rope', 'apply_rope1', 'apply_rope_split_half', 'apply_rope_split_half1', 'deq...
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Keep imports at module scope; avoid inline imports unless they are already part of an established optional-backend probe or are needed to avoid an import cycle.
Do not add unnecessarytry/exceptblocks; use them only for optional dependency, platform, or backend capability detection when the program has a useful fallback, and prefer specific exception types when changing new code.
Remove workarounds for PyTorch versions that ComfyUI no longer officially supports; if a workaround does not have a comment naming the exact supported PyTorch version(s), remove it.
Let unsupported model formats, invalid quantization metadata, and bad states fail with clear errors instead of silently producing lower quality output.
Match the existing local style in the file you edit; this codebase tolerates long lines, simple helper functions, module-level state, and direct tensor operations when they make the code easier to follow.
Keep comments sparse and useful; strip useless comments that restate the code or describe obvious behavior, while allowing short TODOs that name the concrete missing follow-up.
Treat dtype, device placement, VRAM usage, and offloading behavior as core correctness concerns; check CPU, CUDA, ROCm, MPS, DirectML, XPU, NPU, and low VRAM implications when touching shared execution or loading code.
Prefer native ComfyUI formats and existing quantization/offload helpers over adding parallel code paths; usecomfy.quant_ops,comfy.model_management,comfy.memory_management,comfy.pinned_memory,comfy_aimdo, andcomfy-kitchenhelpers where they already solve the problem.
Use optimized comfy-kitchen ops where they improve performance without changing expected dtype, device, memory, or interface behavior.
All models should use the optimized attention function selected by ComfyUI; higher-level code must not inspect function identity, names, modules, or implementation details to decide behavior.
Apply the same opacity rule to similar patterns beyond...
Files:
comfy_api/latest/_input_impl/video_types.pytests-unit/comfy_api_test/video_types_test.py
**
⚙️ CodeRabbit configuration file
**: ## Engineering Style
- Keep changes small and direct. Most fixes should touch the narrowest code path
that explains the bug, performance issue, dtype issue, model-format issue, or
user-facing behavior.- Change the least amount of files possible. A change that touches many files is
more likely to be a bad change than a good one unless the broader scope is
directly required.- Prefer practical fixes over broad architecture work. Add abstractions only
when they remove real repeated logic or match an existing ComfyUI pattern.- Prefer fewer dependencies. Do not add new dependencies to ComfyUI unless they
are absolutely necessary.- Delete obsolete code aggressively when newer infrastructure makes it useless.
Remove dead fallbacks, migration paths, unused options, debug prints, and
compatibility branches that are no longer needed. Do not leave dead branches,
unreachable code, or functions that are never called. If code is not
necessary for the current behavior, remove it.- Revert or disable problematic behavior quickly when it breaks users. It is
better to remove a broken feature path than keep a complicated partial fix.- Preserve existing APIs, node names, model-loading behavior, file layout, and
workflow compatibility unless the change is explicitly about replacing them.- Code must look hand-written for this repository. Changes that read like
generic AI-generated code will be rejected automatically: unnecessary helper
layers, vague names, boilerplate comments, defensive branches without a real
failure mode, broad rewrites, or code that ignores the local style.Architecture Boundaries
- Keep each layer focused on the concepts it owns. Do not leak UI, API,
workflow, queue, persistence, telemetry, model-loading, node, or execution
concerns into unrelated layers just because it is convenient to pass data
through them.- Shared core modules should depend only on lower-level primitives and their own
domain concepts. Highe...
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
🪛 ast-grep (0.44.1)
comfy_api/latest/_input_impl/video_types.py
[info] 97-97: use jsonify instead of json.dumps for JSON output
Context: json.dumps(value)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (7)
comfy_api/latest/_input_impl/video_types.py (2)
63-113: LGTM!
509-756: LGTM!tests-unit/comfy_api_test/video_types_test.py (5)
5-10: LGTM!
285-300: LGTM!
303-327: LGTM!
330-596: LGTM!
598-623: LGTM!
![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 @bigcat88.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 6 |
| 🟢 Low | 3 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
There was a problem hiding this comment.
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)
248-251: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
end_ptsopen-ended when no duration is requested.Line 251 makes
end_pts == start_ptsfor the default untrimmed case (duration == 0), so the streaming decode fallback reports only the first frame for sources without usable frame/duration metadata.Proposed fix
- end_pts = int((start_time + duration) / video_stream.time_base) + end_pts = int((start_time + duration) / video_stream.time_base) if duration else None @@ - if frame.pts >= end_pts: + if end_pts is not None and frame.pts >= end_pts: break🤖 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 248 - 251, In `get_active_trim_window` usage inside the video decode path, `end_pts` is being set equal to `start_pts` when `duration == 0`, which causes the untrimmed case to stop after the first frame. Update the logic around `start_time, duration`, `start_pts`, and `end_pts` so that when no duration is requested, `end_pts` is left open-ended instead of clamping to the start timestamp, while preserving the existing bounded trim behavior for nonzero durations.
🤖 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.
Outside diff comments:
In `@comfy_api/latest/_input_impl/video_types.py`:
- Around line 248-251: In `get_active_trim_window` usage inside the video decode
path, `end_pts` is being set equal to `start_pts` when `duration == 0`, which
causes the untrimmed case to stop after the first frame. Update the logic around
`start_time, duration`, `start_pts`, and `end_pts` so that when no duration is
requested, `end_pts` is left open-ended instead of clamping to the start
timestamp, while preserving the existing bounded trim behavior for nonzero
durations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 590f233a-5506-488c-9433-e78d4802efad
📥 Commits
Reviewing files that changed from the base of the PR and between 1d595d4a703515664157849daeaee88d7ca47a1c and 64f73d40799808431a2e2c900025ece6a60a6f4f.
📒 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. (5)
- GitHub Check: test (windows-2022)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: test (windows-latest)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Keep imports at module scope; avoid inline imports unless they are already part of an established optional-backend probe or are needed to avoid an import cycle.
Do not add unnecessarytry/exceptblocks; use them only for optional dependency, platform, or backend capability detection when the program has a useful fallback, and prefer specific exception types when changing new code.
Remove workarounds for PyTorch versions that ComfyUI no longer officially supports; if a workaround does not have a comment naming the exact supported PyTorch version(s), remove it.
Let unsupported model formats, invalid quantization metadata, and bad states fail with clear errors instead of silently producing lower quality output.
Match the existing local style in the file you edit; this codebase tolerates long lines, simple helper functions, module-level state, and direct tensor operations when they make the code easier to follow.
Keep comments sparse and useful; strip useless comments that restate the code or describe obvious behavior, while allowing short TODOs that name the concrete missing follow-up.
Treat dtype, device placement, VRAM usage, and offloading behavior as core correctness concerns; check CPU, CUDA, ROCm, MPS, DirectML, XPU, NPU, and low VRAM implications when touching shared execution or loading code.
Prefer native ComfyUI formats and existing quantization/offload helpers over adding parallel code paths; usecomfy.quant_ops,comfy.model_management,comfy.memory_management,comfy.pinned_memory,comfy_aimdo, andcomfy-kitchenhelpers where they already solve the problem.
Use optimized comfy-kitchen ops where they improve performance without changing expected dtype, device, memory, or interface behavior.
All models should use the optimized attention function selected by ComfyUI; higher-level code must not inspect function identity, names, modules, or implementation details to decide behavior.
Apply the same opacity rule to similar patterns beyond...
Files:
comfy_api/latest/_input_impl/video_types.pytests-unit/comfy_api_test/video_types_test.py
**
⚙️ CodeRabbit configuration file
**: ## Engineering Style
- Keep changes small and direct. Most fixes should touch the narrowest code path
that explains the bug, performance issue, dtype issue, model-format issue, or
user-facing behavior.- Change the least amount of files possible. A change that touches many files is
more likely to be a bad change than a good one unless the broader scope is
directly required.- Prefer practical fixes over broad architecture work. Add abstractions only
when they remove real repeated logic or match an existing ComfyUI pattern.- Prefer fewer dependencies. Do not add new dependencies to ComfyUI unless they
are absolutely necessary.- Delete obsolete code aggressively when newer infrastructure makes it useless.
Remove dead fallbacks, migration paths, unused options, debug prints, and
compatibility branches that are no longer needed. Do not leave dead branches,
unreachable code, or functions that are never called. If code is not
necessary for the current behavior, remove it.- Revert or disable problematic behavior quickly when it breaks users. It is
better to remove a broken feature path than keep a complicated partial fix.- Preserve existing APIs, node names, model-loading behavior, file layout, and
workflow compatibility unless the change is explicitly about replacing them.- Code must look hand-written for this repository. Changes that read like
generic AI-generated code will be rejected automatically: unnecessary helper
layers, vague names, boilerplate comments, defensive branches without a real
failure mode, broad rewrites, or code that ignores the local style.Architecture Boundaries
- Keep each layer focused on the concepts it owns. Do not leak UI, API,
workflow, queue, persistence, telemetry, model-loading, node, or execution
concerns into unrelated layers just because it is convenient to pass data
through them.- Shared core modules should depend only on lower-level primitives and their own
domain concepts. Highe...
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)
comfy_api/latest/_input_impl/video_types.py (1)
75-88: LGTM!Also applies to: 306-348, 415-438, 481-491, 527-630, 632-757
tests-unit/comfy_api_test/video_types_test.py (1)
598-650: LGTM!
…me in RAM Signed-off-by: bigcat88 <bigcat88@icloud.com>
Signed-off-by: bigcat88 <bigcat88@icloud.com>
…stead of dropping stream immediately Signed-off-by: bigcat88 <bigcat88@icloud.com>
64f73d4 to
2330be6
Compare
Signed-off-by: bigcat88 <bigcat88@icloud.com>
2330be6 to
524414b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 248-251: The open-ended trim handling in get_active_trim_window()
is forcing end_pts to match start_pts when duration is 0, which makes the later
frame.pts cutoff stop after the first frame for metadata-poor untrimmed videos.
Update the frame-count logic in video_types.py so the no-duration-cap path
leaves end_pts unset/optional and only applies the frame.pts >= end_pts check
when an active duration cap exists.
- Around line 101-111: The MP4 kwargs builder in mp4_output_open_kwargs
currently only sets the output format when format is a VideoContainer instance,
so string-like enum values such as "mp4" can pass validation but still skip the
format passed to av.open(). Normalize the accepted format input after the
validation checks and always set open_kwargs["format"] to the MP4 container
value for supported outputs, while preserving the BytesIO-specific handling; use
mp4_output_open_kwargs, VideoContainer, and VideoCodec as the key locations to
update.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a6e9a176-a0c0-4328-94cc-6cf440c4777e
📥 Commits
Reviewing files that changed from the base of the PR and between 64f73d40799808431a2e2c900025ece6a60a6f4f and 524414b.
📒 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: Run Pylint
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: test (windows-latest)
- GitHub Check: test
- GitHub Check: test (windows-2022)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: Run Pylint
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Keep imports at module scope; avoid inline imports unless they are already part of an established optional-backend probe or are needed to avoid an import cycle.
Do not add unnecessarytry/exceptblocks; use them only for optional dependency, platform, or backend capability detection when the program has a useful fallback, and prefer specific exception types when changing new code.
Remove workarounds for PyTorch versions that ComfyUI no longer officially supports; if a workaround does not have a comment naming the exact supported PyTorch version(s), remove it.
Let unsupported model formats, invalid quantization metadata, and bad states fail with clear errors instead of silently producing lower quality output.
Match the existing local style in the file you edit; this codebase tolerates long lines, simple helper functions, module-level state, and direct tensor operations when they make the code easier to follow.
Keep comments sparse and useful; strip useless comments that restate the code or describe obvious behavior, while allowing short TODOs that name the concrete missing follow-up.
Treat dtype, device placement, VRAM usage, and offloading behavior as core correctness concerns; check CPU, CUDA, ROCm, MPS, DirectML, XPU, NPU, and low VRAM implications when touching shared execution or loading code.
Prefer native ComfyUI formats and existing quantization/offload helpers over adding parallel code paths; usecomfy.quant_ops,comfy.model_management,comfy.memory_management,comfy.pinned_memory,comfy_aimdo, andcomfy-kitchenhelpers where they already solve the problem.
Use optimized comfy-kitchen ops where they improve performance without changing expected dtype, device, memory, or interface behavior.
All models should use the optimized attention function selected by ComfyUI; higher-level code must not inspect function identity, names, modules, or implementation details to decide behavior.
Apply the same opacity rule to similar patterns beyond...
Files:
tests-unit/comfy_api_test/video_types_test.pycomfy_api/latest/_input_impl/video_types.py
**
⚙️ CodeRabbit configuration file
**: ## Engineering Style
- Keep changes small and direct. Most fixes should touch the narrowest code path
that explains the bug, performance issue, dtype issue, model-format issue, or
user-facing behavior.- Change the least amount of files possible. A change that touches many files is
more likely to be a bad change than a good one unless the broader scope is
directly required.- Prefer practical fixes over broad architecture work. Add abstractions only
when they remove real repeated logic or match an existing ComfyUI pattern.- Prefer fewer dependencies. Do not add new dependencies to ComfyUI unless they
are absolutely necessary.- Delete obsolete code aggressively when newer infrastructure makes it useless.
Remove dead fallbacks, migration paths, unused options, debug prints, and
compatibility branches that are no longer needed. Do not leave dead branches,
unreachable code, or functions that are never called. If code is not
necessary for the current behavior, remove it.- Revert or disable problematic behavior quickly when it breaks users. It is
better to remove a broken feature path than keep a complicated partial fix.- Preserve existing APIs, node names, model-loading behavior, file layout, and
workflow compatibility unless the change is explicitly about replacing them.- Code must look hand-written for this repository. Changes that read like
generic AI-generated code will be rejected automatically: unnecessary helper
layers, vague names, boilerplate comments, defensive branches without a real
failure mode, broad rewrites, or code that ignores the local style.Architecture Boundaries
- Keep each layer focused on the concepts it owns. Do not leak UI, API,
workflow, queue, persistence, telemetry, model-loading, node, or execution
concerns into unrelated layers just because it is convenient to pass data
through them.- Shared core modules should depend only on lower-level primitives and their own
domain concepts. Highe...
Files:
tests-unit/comfy_api_test/video_types_test.pycomfy_api/latest/_input_impl/video_types.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:
tests-unit/comfy_api_test/video_types_test.pycomfy_api/latest/_input_impl/video_types.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:
tests-unit/comfy_api_test/video_types_test.pycomfy_api/latest/_input_impl/video_types.py
🪛 ast-grep (0.44.1)
comfy_api/latest/_input_impl/video_types.py
[info] 97-97: use jsonify instead of json.dumps for JSON output
Context: json.dumps(value)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (4)
comfy_api/latest/_input_impl/video_types.py (2)
3-13: LGTM!Also applies to: 63-100
306-348: LGTM!Also applies to: 415-438, 484-491, 509-761, 809-814
tests-unit/comfy_api_test/video_types_test.py (2)
5-10: LGTM!Also applies to: 243-300, 303-565, 604-684
590-593: 🎯 Functional CorrectnessPatch durations on flushed packets too, if this path can emit delayed packets.
video_stream.encode(None)skips theduration_by_ptshandling, so any delayed packet would keep its default duration and miss the exact VFRsttsmapping.
Signed-off-by: bigcat88 <bigcat88@icloud.com>
Signed-off-by: bigcat88 <bigcat88@icloud.com>
alexisrolland
changed the title
alexisrolland
changed the title
2 participants
VideoFromFile.save_to()has two paths: remux (stream copy) when source already matches the requested container/codec, and a transcode otherwiseThe transcode first materialized the entire video as float32 tensors (
get_components-->VideoFromComponents.save_to), with peak memory:of the file actual size. Every Partner Node that uploads video hits this path whenever the source is not already H.264
Note: The video cropping operation also forces the transcoding path (for any codec), so this PR also improves the situation when the
Video Slicenode is usedChanges in this PR
Transcode branch is rewritten as a single streaming pass: demux --> decode --> reformat --> H.264/AAC encode --> mux, one frame at a time (
_save_transcoded). Peak memory no longer scales with video lengthPreserved behavior, now verified by tests:
Trim Video) - video and audio, sample-exact, including chained trimsyuvj/MJPEG) are compressed to limited range as beforeyuv420p10leBehavior changes (transcode path only)
swscaledithering instead of float truncationmax_b_frames = 0): with B-frame reordering the mp4 muxer derives sample durations from decode order, which broke irregular-VFR spans and trim windows; output is a few percent larger for itLoad Video --> Save Video benchmarks
The speedup has the same root as the memory win: ~75 % of the old wall time was spent converting YUV --> float RGB --> torch tensors --> RGB --> YUV
Fixes CORE-353
Related to CORE-351 (the
Get Video Componentseager tensor materialization is a separate issue, not addressed here)