Skip to content

fix(llava_vid): call read_video with its actual signature on the pyav path - #1492

Merged
pufanyi merged 1 commit into
EvolvingLMMs-Lab:mainfrom
Anai-Guo:fix/llava-vid-pyav-read-video
Aug 31, 2026
Merged

fix(llava_vid): call read_video with its actual signature on the pyav path#1492
pufanyi merged 1 commit into
EvolvingLMMs-Lab:mainfrom
Anai-Guo:fix/llava-vid-pyav-read-video

Conversation

@Anai-Guo

Copy link
Copy Markdown
Contributor

Problem

Llava_Vid.generate_until dispatches on video_decode_backend. The decord branch calls
the class's own self.load_video(...), which returns (frames, frame_time, video_time).
The pyav branch next to it calls the shared helper read_video as if it had the same
signature and the same return contract — but it has neither:

# lmms_eval/models/simple/llava_vid.py:463
video, frame_time, video_time = read_video(
    visuals[0],
    self.max_frames_num,
    self.fps,
    force_sample=self.force_sample,
)
# lmms_eval/models/model_utils/load_video.py
def read_video(
    video_path: Union[str, tuple, list],
    *,
    num_frm: int = 8,
    fps: Optional[float] = None,
    format="rgb24",
    force_include_last_frame=False,
    backend: Optional[str] = None,
) -> np.ndarray:

Three separate mismatches:

  1. num_frm/fps are keyword-only, so the two positional arguments raise
    TypeError: read_video() takes 1 positional argument but 3 were given.
  2. force_sample is not a parameter of read_video at all.
  3. Even with the arity fixed, read_video returns a single (N, H, W, 3) array, so
    unpacking it into three names raises ValueError: too many values to unpack (expected 3)
    for any video sampled to more than 3 frames.

The whole block sits inside try: ... except Exception as e:, which logs the message and
appends f"Video {video_path} can not load, check the source" to the results. So the
failure is not loud — running llava_vid with video_decode_backend=pyav silently scores
every sample as an unloadable video instead of crashing.

This is the only call site that gets it wrong

Every other pyav branch in the repo calls the helper the way it is declared:

call site call
longva.py:412 read_video(visual[0], num_frm=self.max_frames_num)
llava_onevision.py:322 read_video(visual[0], num_frm=self.max_frames_num)
llava_onevision.py:507 read_video(visual[0], num_frm=self.max_frames_num)
llava_onevision.py:737 read_video(visual[0], num_frm=self.max_frames_num)
llava_vid.py:463 3 positional args + force_sample=

It looks like the branch was copied from the decord branch three lines above it
(which does take (path, max_frames_num, fps, force_sample=...)) without adapting it to
read_video.

Fix

Call read_video with keyword arguments, and rebuild the timing metadata that the
decord branch returns alongside the frames:

  • force_sample=True in self.load_video means "ignore the target fps and take
    max_frames_num frames uniformly". read_video does exactly that when it is given no
    fps, so the knob is preserved as fps=None if self.force_sample else self.fps rather
    than silently dropped.
  • frame_time/video_time are recovered from the container metadata via the module's
    existing _probe_video_metadata. read_video samples with
    np.linspace(0, total_frames - 1, sampled, dtype=int), so len(video) is enough to
    reproduce the exact frame positions it used — no second decode of the frames.

These two values are only consumed under add_time_instruction, but they are unpacked
unconditionally, so the branch has to produce them either way.

(Happy to rename _probe_video_metadata to a public name if you'd rather not import an
underscore-prefixed helper across modules — say the word and I'll fold that in.)

Verification

No GPU/model needed for the failing mechanism — it is pure call-signature and unpacking.
Replayed the real signature from load_video.py with inspect.Signature.bind:

read_video signature: (video_path, *, num_frm=8, fps=None, format='rgb24',
                       force_include_last_frame=False, backend=None) -> np.ndarray

--- BROKEN call: llava_vid.py:463 (verbatim args) ---
  TypeError: too many positional arguments

--- CONTROL GROUP: the sibling pyav call sites ---
  longva.py:412                bind OK
  llava_onevision.py:322       bind OK
  llava_onevision.py:507       bind OK
  llava_onevision.py:737       bind OK

--- FIXED call ---
  bind OK -> {'video_path': ..., 'num_frm': 20, 'fps': 1, 'format': 'rgb24',
              'force_include_last_frame': False, 'backend': None}

--- return arity ---
  simulated unpack of an (20, H, W, 3) array into 3 names:
  ValueError: too many values to unpack (expected 3)

And the frame_time reconstruction is bit-identical to the indices read_video itself
picks, checked against the module's own _compute_uniform_indices for every
(total_frames, sampled) pair in total ∈ {1, 2, 7, 30, 101, 1000} × sampled ∈ {1, 2, 8, 20}:

index-policy equivalence: identical for every pair tested: True

ruff format and ruff check clean at the version pinned in .pre-commit-config.yaml
(v0.16.4), run against the repo's own pyproject.toml. The four pre-existing findings in
this file are unchanged by the patch (identical output before and after).


🤖 Generated with Claude Code

… path

read_video is keyword-only and returns just the frame array, but the
pyav branch passed three positional arguments plus a force_sample
keyword and unpacked a 3-tuple. Rebuild frame_time/video_time locally
so the branch matches the decord path's contract.
@pufanyi

pufanyi commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

lgtm, thanks for your contribution!

@pufanyi
pufanyi self-requested a review August 31, 2026 07:49
@pufanyi
pufanyi merged commit e9ff3ec into EvolvingLMMs-Lab:main Aug 31, 2026
2 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.

2 participants