Skip to content

fix: keep video movflags scoped to container#14787

Open
ahfoysal wants to merge 1 commit into
Comfy-Org:masterfrom
ahfoysal:fix/video-components-audio-movflags
Open

fix: keep video movflags scoped to container#14787
ahfoysal wants to merge 1 commit into
Comfy-Org:masterfrom
ahfoysal:fix/video-components-audio-movflags

Conversation

@ahfoysal

@ahfoysal ahfoysal commented Jul 6, 2026

Copy link
Copy Markdown

Fixes #14755.

VideoFromComponents.save_to() was passing movflags=use_metadata_tags through PyAV's global options argument. PyAV applies options to the container and streams, so the AAC encoder can see movflags while opening the audio stream and reject it.

This switches the shared write kwargs helper to container_options, then has VideoFromComponents.save_to() use that same helper instead of keeping its own av.open() kwargs. That keeps metadata tags on the MP4 container without leaking the flag into stream codec options.

Tests:

  • uv run --with-requirements requirements.txt --with-requirements tests-unit/requirements.txt pytest tests-unit/comfy_api_test/video_types_test.py -q
  • uv run --with ruff ruff check comfy_api/latest/_input_impl/video_types.py tests-unit/comfy_api_test/video_types_test.py
  • python3 -m py_compile comfy_api/latest/_input_impl/video_types.py tests-unit/comfy_api_test/video_types_test.py
  • git diff --check

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d6975c92-b452-42db-82c9-781e6335e288

📥 Commits

Reviewing files that changed from the base of the PR and between b08debc and a77158e.

📒 Files selected for processing (2)
  • comfy_api/latest/_input_impl/video_types.py
  • tests-unit/comfy_api_test/video_types_test.py
📜 Recent review details
🧰 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 unnecessary try/except blocks; 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; use comfy.quant_ops, comfy.model_management, comfy.memory_management, comfy.pinned_memory, comfy_aimdo, and comfy-kitchen helpers 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.py
  • comfy_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.py
  • comfy_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 a with: 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.py
  • comfy_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.py
  • comfy_api/latest/_input_impl/video_types.py
🔇 Additional comments (2)
comfy_api/latest/_input_impl/video_types.py (1)

33-52: LGTM!

Also applies to: 511-529

tests-unit/comfy_api_test/video_types_test.py (1)

8-9: LGTM!

Also applies to: 242-262


📝 Walkthrough

Walkthrough

Changes

This change modifies how PyAV container write options are configured when saving video output. The movflags setting for MP4 metadata tag preservation is moved from the options dictionary to container_options within the kwargs returned by get_open_write_kwargs. The save_to method for MP4 output now delegates to get_open_write_kwargs rather than manually constructing av.open parameters. Corresponding unit tests were added to verify the new kwargs structure, including format handling for BytesIO targets and explicit container requests.

Sequence Diagram(s)

See hidden artifact diagram above.

Related Issues: Not specified in provided context.

Related PRs: Not specified in provided context.

Suggested labels: bug, video, tests

Suggested reviewers: Not specified in provided context.

Poem
A rabbit tweaks a movie flag,
From "options" to a container bag,
Metadata tags now safely stay,
Tests confirm it, hip hip hooray!
🐇🎬

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly reflects the main change: scoping movflags to the video container.
Description check ✅ Passed The description accurately explains the bug and the container_options fix in this PR.
Linked Issues check ✅ Passed The code changes address #14755 by keeping movflags container-scoped and updating save_to accordingly.
Out of Scope Changes check ✅ Passed The changes stay focused on the movflags fix and matching tests, with no unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: av.error.ArgumentError in video_types.py when muxing audio on macOS (AAC encoder rejects container movflags)

1 participant