[fix] Fixes Qwen3-VL prompt expansion when a message contains multiple videos/multiple round chat#10518
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the multi-modal plugin to properly support processing multiple videos. It overrides the get_mm_inputs method, updates process_messages to dynamically index video grids and metadata using the current video token count rather than the message index, and uses temporal_patch_size as a fallback for merge_size. Additionally, the test suite has been updated to include multi-video test cases and verify the new get_mm_inputs functionality. There are no review comments, so I have no further feedback to provide.
08abd61 to
2dde55e
Compare
2dde55e to
3282346
Compare
3282346 to
6e8e7e5
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the video processing logic in mm_plugin.py to dynamically retrieve num_frames and metadata based on num_video_tokens when expanding video tokens, rather than using hardcoded values or message indices. It also updates the test suite to verify multi-video processing. The review feedback suggests using getattr with a default value when accessing temporal_patch_size from video_processor to improve robustness and maintain consistency with other parts of the codebase.
| timestamps = processor._calculate_timestamps( | ||
| metadata.frames_indices, | ||
| metadata.fps, | ||
| video_processor.merge_size, | ||
| video_processor.temporal_patch_size, | ||
| ) |
There was a problem hiding this comment.
For better robustness and consistency with other parts of the codebase (such as lines 2074 and 2351), it is recommended to use getattr with a default value when accessing temporal_patch_size from video_processor.
| timestamps = processor._calculate_timestamps( | |
| metadata.frames_indices, | |
| metadata.fps, | |
| video_processor.merge_size, | |
| video_processor.temporal_patch_size, | |
| ) | |
| timestamps = processor._calculate_timestamps( | |
| metadata.frames_indices, | |
| metadata.fps, | |
| getattr(video_processor, "temporal_patch_size", 2), | |
| ) |
Co-authored-by: gemini-code-assist <200291788+gemini-code-assist@users.noreply.github.com>
|
For Qwen3-VL, should we treat multiple videos across both of these forms as supported?
I also noticed a similar first-video assumption in |
|
thanks, we will look through this. |
Kuangdd01
left a comment
There was a problem hiding this comment.
Thanks for fixing, LGTM!
What does this PR do?
Fixes #9704
This PR fixes Qwen3-VL prompt expansion when a single message contains multiple
<video>placeholders.Previously, Qwen3-VL video metadata was selected by message index during prompt expansion. This breaks when one message contains more than one video, because later
<video>placeholders can reuse the wrong metadata and frame count.The fix changes video expansion to consume
video_grid_thwandvideo_metadataby video placeholder order instead of message index. Each<video>placeholder now uses its corresponding video grid, frame count, timestamps, and per-frame token length.This PR also updates Qwen3-VL timestamp calculation to pass
temporal_patch_sizetoprocessor._calculate_timestamps, matching the upstream Qwen3-VL processor behavior.merge_sizeis still used for spatial token merging, whiletemporal_patch_sizecontrols timestamp grouping across frames.A regression test is added for a user message with two videos of different lengths.
Before submitting