fix: improve video processing fps assignment logic

[Xqle]

What does this PR do?

1. Improve fps handling in Qwen2_5_VLProcessor.__call__()

• Previously, the __call__() function always defaulted to fps=2, regardless of the input video.

• Now, when fps is not explicitly provided and cannot be read from the video metadata, it falls back to 24 by default. Otherwise, the provided or metadata fps is used. It follows a priority order:

  kwargs["fps"] → video_metadata["fps"] → 24 (default)

  The default value 24 is chosen as it represents a common frame rate for most videos.

• This prevents potential errors in second_per_grid_ts calculation when the actual video frame rate differs from the assumed default.

2. More robust do_sample_frames behavior

• If fps is manually provided but do_sample_frames is not specified, do_sample_frames will now be automatically set to True.
• This avoids cases where custom fps is ignored due to missing sampling flag.

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline,
  Pull Request section?
• Was this discussed/approved via a Github issue or the forum? Please add a link
  to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

Who can review?

@zucchini-nlp

[Rocketknight1]

cc @molbap @yonigozlan for processors

[zucchini-nlp]

Hey @Xqle , thanks for the PR though I am not convinced we have to automatically change the flag depending on input args. Please see my comments below

[zucchini-nlp]

i don't think this is a good idea. We can't automatically change the flag only for a single model. If we are to change to all models, it will be much harder because the sampling can be set either with num_frames or fps and optionally other args. Checking for what required args are present per model is not as straightforward always

[Xqle]

For this part, we can just revert the change.

[zucchini-nlp]

i don't think this is the fps if the original video here we are using. It's more probably the sampling fps rate used which is in videos_kwargs

The sampling fps however isn't always there, we can sample with number of frames, with default values of video processor or do not sample at all. So the 2 here is just the default value of the video processor

[Xqle]

@zucchini-nlp

Qwen2VL Video Processor Default Parameters

Thanks for the feedback.

In Qwen2.5-VL, the video processor we are using is the Qwen2-VL video processor, which does not initialize fps or num_frames by default, see above. So the model will not sample the input video by default.

And I think the fps in this context is not only used for frame sampling, but is later involved in computing seconds_per_grid_ts, which directly affects the temporal RoPE index inside get_rope_index. So, the fps used here should correspond to the effective fps of the processed video (i.e., the fps after potential frame sampling).

Specifically:

• When fps and do_sample_frames are passed in video_kwargs, the processor will sample the video according to that fps, so the processed video’s fps should match the sampling fps in video_kwargs.

• When eitherfps ordo_sample_frames is not provided, no sampling occurs, and the processed video keeps its original fps, which is metadata.fps. In that case, using metadata.fps ensures that seconds_per_grid_ts and thus the temporal RoPE positions remain consistent with the actual video duration.

• Finally, if no sampling occurs and metadata.fps is unavailable, it’s reasonable to fall back to a typical video frame rate such as 24 fps, to maintain temporal consistency in downstream computations.

So, I think we can revert the part of automatic change of flag do_sample_frames, and change this part to something like:

fps = [metadata.fps if metadata.fps is not None else 24 for metadata in video_metadata]
if output_kwargs["videos_kwargs"].get("do_sample_frames") is not None:
    if output_kwargs["videos_kwargs"].get("fps") is not None:
        fps = output_kwargs["videos_kwargs"]["fps"]
    elif output_kwargs["videos_kwargs"].get("num_frames") is not None:
        fps = [output_kwargs["videos_kwargs"]["num_frames"] / metadata.duration for metadata in video_metadata]

[zucchini-nlp]

Thanks for detailed explanation. I looked through the qwen paper and the m-rope documentation and it looks like the FPS is indeed the video's fps after the optional sampling. Though it will be breaking wrt the current main branch because the default fps when not provided is 24 now. I want to also ask the authors @BakerBunker to take a look and confirm this is how QwenVL should work

[zucchini-nlp]

For impl, imo it's better if we add a new field in metadata for sampled_fps and use it later. The field might be useful not only in QwenVL but other models also

[Xqle]

@zucchini-nlp Sounds good. I can try to work on it later.

[BakerBunker]

Currently, fps refers to the frame rate expected by the language model, not the actual frame rate of the video itself.

[Xqle]

Currently, fps refers to the frame rate expected by the language model, not the actual frame rate of the video itself.

Thanks for the reply. I am wondering if that is the case, will the rope index be inconsistent with the input video since videos with different fps, assuming no sampling is applied, will be using the same temporl rope index.

[BakerBunker]

The input video should be resampled by video processor, so we assume the videos have same fps

[Xqle]

@zucchini-nlp

Thanks for the review. But I think the fps should align with the fps of video after video processing, no matter the video is sampled during video processing or not.
Please see my reply for your comments above.

[Xqle]

@zucchini-nlp I've added sampled_fps to the metadata as discussed. If this implementation looks good to you, I can also update the other models that use FPS in a similar way. From what I’ve checked, only qwen3_omni_moe and qwen2_5_omni would need similar changes.

Also, I noticed a potential inconsistency regarding the previously discussed do_sample_frames flag.

• When calling processor.apply_chat_template(..., tokenize=True, fps=1), do_sample_frames is automatically set to True (see code in processor_utils.py).
• However, if we call processor.apply_chat_template(..., tokenize=False) and then use processor(..., fps=1) manually for processing, do_sample_frames remains unset. So the video will not be sampled and there’s no warning or error about it.

I understand your previous concern that adding this logic inside the processor might require updating every model’s processor implementation, which could be non-trivial. Maybe we could move the flag assignment from processor_utils.apply_chat_template to video_processing_utils.preprocess?

If that sounds reasonable to you, I can open a separate PR to address this specifically.

[zucchini-nlp]

Also, I noticed a potential inconsistency regarding the previously discussed do_sample_frames flag.

ah yeah, we had to do the automatic switch trick for BC. Prev we didn't have a dedicated video processor and thus no flag to control the sampling behavior. Sampling was done only when applying chat template by helper functions. So we had to keep it and always same frames when the processor is called via apply_chat_template

Though when simply calling, I still think it should not be automatically changing. For all new models it is set to True anyway so it will be sampling by default. For old model we have the flag set to False (again for BC) and will require users to toggle it if they want sampling

[zucchini-nlp]

it could be in kwargs but set to False, let's check it's value if the key exists

[Xqle]

Thanks for pointing that out. I will udpateqwen2_5_vl in this PR.
I also checked other models and found that a few of them have the same issue.
To keep the changes clean and consistent, I think it might be better to handle other models in a separate PR.

[zucchini-nlp]

personally I don't like examining the function to get the sampling FPS. Looks a bit overengineered, so maybe we could obtain it by returning directly from sample_indices_fn? Or have it as a property of VideoMetadata and infer from total frames and length of sampled frames?

[Xqle]

Yep, I think it can be inferred directly from indices, and I’ve updated the code accordingly. The logic looks much cleaner now, thanks for the suggestion.

[zucchini-nlp]

we infer it in the same way everywhere so imo we can move it as a property of class VideoMetadata, similar to how timestamps work

[zucchini-nlp]

Thanks! The CI is failing for unrelated reasons, should be fixed soon. Then we'll merge

[github-actions]

[For maintainers] Suggested jobs to run (before merge)

run-slow: qwen2_5_vl

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.