Skip to content

fix(reasoner): preserve VideoPhy2 timeline after frame limiting - #200

Merged
pengcuo merged 2 commits into
NVIDIA:mainfrom
PeiPei233:fix/videophy2-effective-fps
Aug 28, 2026
Merged

fix(reasoner): preserve VideoPhy2 timeline after frame limiting#200
pengcuo merged 2 commits into
NVIDIA:mainfrom
PeiPei233:fix/videophy2-effective-fps

Conversation

@PeiPei233

@PeiPei233 PeiPei233 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Problem

VideoPhy2 first samples videos toward 2 FPS, then caps clips with more than 32 sampled frames. After that second downsampling step, the processor still reports source_fps / stride, so Qwen timestamp tokens see a compressed timeline for long videos.

Fix

Compute the effective FPS from the final number of sampled frames:

effective_fps = len(indices) / total_frames * source_fps

This matches _decode_reasoner_video and keeps timestamp tokens aligned with the source video duration after the 32-frame cap. A regression test covers a 900-frame, 30 FPS video capped to 32 frames.

Testing

  • uvx ruff==0.12.7 check <changed files>
  • uvx ruff==0.12.7 format --check <changed files>
  • uvx pre-commit run -c ci/.pre-commit-config-base.yaml --files <changed files>
  • pytest cosmos_framework/configs/base/reasoner/experiment/videophy2_dataflow_roles_test.py (1 passed)

Signed-off-by: PeiPei233 <2503585731@qq.com>
@PeiPei233
PeiPei233 marked this pull request as ready for review August 14, 2026 11:09
Copilot AI lite review requested due to automatic review settings August 14, 2026 11:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes VideoPhy2 video timeline/FPS reporting after applying the 32-frame cap so Qwen timestamp tokens remain aligned with the source video duration, and adds a regression test covering a long-video capped case.

Changes:

  • Update _decode_video_to_pil_frames to compute effective_fps from the final sampled-frame count relative to the original video duration.
  • Add a unit test that simulates a 900-frame, 30 FPS video capped to 32 frames and asserts the corrected effective_fps.
  • Minor formatting adjustment to a TypeError raise.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cosmos_framework/configs/base/reasoner/experiment/videophy2_dataflow_roles.py Computes effective_fps based on final indices to preserve the correct timeline after frame limiting.
cosmos_framework/configs/base/reasoner/experiment/videophy2_dataflow_roles_test.py Adds regression coverage for capped-frame videos and validates the updated FPS calculation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +49 to +54
decoders_module = ModuleType("torchcodec.decoders")
decoders_module.VideoDecoder = FakeVideoDecoder # type: ignore[attr-defined]
torchcodec_module = ModuleType("torchcodec")
torchcodec_module.decoders = decoders_module # type: ignore[attr-defined]
monkeypatch.setitem(sys.modules, "torchcodec", torchcodec_module)
monkeypatch.setitem(sys.modules, "torchcodec.decoders", decoders_module)

@pengcuo pengcuo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! Verified the logic: the old source_fps / stride always reported ~2.0 fps regardless of the 32-frame cap, compressing the perceived timeline of any video longer than 16s (a 30s clip showed up as 16s downstream, where fps feeds the Qwen3-VL video_metadata timestamps and the Nemotron bridge duration). The new len(indices) / total_frames * source_fps matches the qwen_vl_utils / i4 video_decoder_qwen convention exactly, and degenerates to the old value when the cap is not hit — confirmed both cases by running the decode against a fake 900-frame/30fps and 300-frame/30fps decoder. LGTM.

@pengcuo
pengcuo enabled auto-merge (squash) August 28, 2026 08:02
@pengcuo
pengcuo merged commit 5050411 into NVIDIA:main Aug 28, 2026
16 of 17 checks passed
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.

4 participants