fix(metadata): best-effort nb_frames cascade + count_frames utility

[AmitMY]

Closes #4.

Problem

video_metadata(path).nb_frames trusted the container header, but headers can lie: some MOV/MP4 files declare more frames than actually decode (the fixture in #4 declares 41, only 33 decode — 24% off). This caused false-positive frame-count-mismatch reports in sign/data's pose validation.

Approach

Per discussion, no nb_frames_certain flag — nb_frames just does its best, escalating only when needed:

1. candidate = header (stream.frames), else packet count (one demux pass, no decoding) when the header is absent (Matroska/WebM)
2. cross-check candidate against round(duration × fps) — agreement within 1 frame → trust it (zero extra cost; healthy files take the same path as before)
3. disagreement → full decode count = ground truth (only for the rare suspicious files; ~0.2 s on the fixture)
4. non-seekable streams (read_frames_from_stream on a pipe) can't rewind, so they keep header-else-estimate

On the #4 fixture: header=41, derived=31 → disagreement → decode → 33 ✓

Also adds the public count_frames(source) from #4 — explicit full-decode ground truth for callers that need certainty regardless of the header.

Known limitation

The cH27PiYX6gI class from #4 (header and duration×fps agree at 4,340; truth 4,089) is not caught: when both cheap signals agree we trust them. Catching it would require demuxing every file even when signals agree — a full I/O pass on all healthy files. Can be revisited as an opt-in if that class proves common.

Tests

• New fixture tests/assets/buggy_mov_header.mov (1.7 MB, trimmed from the 5.5 MB original with -c copy, bug preserved: header declares 11 frames, only 3 decode)
• test_nb_frames_matches_decoded_frames parametrized over the missing-header mkv/webm and the lying-header mov, asserting nb_frames == count_frames(path) — no hardcoded counts

🤖 Generated with Claude Code

---

Note

Medium Risk
Frame-count logic now may full-decode some seekable files when metadata disagrees, changing nb_frames for buggy headers; behavior is intentional but affects downstream validation that relied on header counts.

Overview
video_metadata().nb_frames no longer trusts the container header alone. Seekable inputs use _best_effort_nb_frames: header or packet demux as a cheap candidate, cross-check against round(duration × fps) (within 1 frame → keep candidate), and full decode only when those signals disagree. Non-seekable sources still get header or duration×fps only.

Adds public count_frames(source) for an explicit full-decode ground truth when callers need certainty.

Tests drop read_frames_exact in favor of count_frames, add buggy_mov_header.mov, and parametrize test_nb_frames_matches_decoded_frames over missing-header Matroska/WebM and lying-header MOV.

^{Reviewed by Cursor Bugbot for commit 844a5bd. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• New Features

  • New frame counting utility that calculates exact frame counts for video files.

• Improvements

  • Enhanced frame detection algorithm that intelligently combines multiple sources of frame information for greater accuracy.
  • Improved robustness when handling videos with missing or incorrect metadata headers.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR refactors frame counting in VideoMetadata to use a best-effort strategy that compares header, demux-packet, and duration×fps signals and falls back to full decoding when seekable, while adding a new public count_frames() helper that returns the decoded frame count.

Changes

Frame Counting Strategy Overhaul

Layer / File(s)	Summary
Best-effort frame counting strategy simple_video_utils/metadata.py	Introduces _count_decoded_frames (full-stream decoding) and _best_effort_nb_frames (multi-signal comparison with conditional fallback); updates _count_video_packets docstring for clarity; replaces prior branching logic in video_metadata_from_container with unified best-effort call.
Public count_frames API and validation tests simple_video_utils/metadata.py, tests/test_metadata.py	Adds public count_frames(source) function for ground-truth frame counts; imports and uses it in new parameterized test that validates nb_frames against actual decoded count across multiple header/container scenarios.

Sequence Diagram

sequenceDiagram
  participant video_metadata_from_container
  participant _best_effort_nb_frames
  participant _count_video_packets
  participant _count_decoded_frames
  video_metadata_from_container->>_best_effort_nb_frames: (stream, duration, fps, seekable)
  _best_effort_nb_frames->>_best_effort_nb_frames: candidate_header = stream.frames
  _best_effort_nb_frames->>_count_video_packets: get packet count
  _best_effort_nb_frames->>_best_effort_nb_frames: estimate = round(duration * fps)
  alt Not seekable
    _best_effort_nb_frames->>_best_effort_nb_frames: return best cheap signal
  else Seekable and signals disagree
    _best_effort_nb_frames->>_count_decoded_frames: decode full stream
    _count_decoded_frames->>_best_effort_nb_frames: return decoded count
  end
  _best_effort_nb_frames->>video_metadata_from_container: nb_frames

Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly Related Issues

• nb_frames can be inaccurate: cascade improvements + ground-truth count_frames() #4: This PR directly implements the proposed best-effort cascade with fallback-to-decode logic and the new public count_frames(source) API described in the related issue.

Poem

🐰 A frame-counting quest, once fraught with doubt,
Now settles signals with a clever route—
Header, packet, duration aligned,
Then decode when seekable signs conflict, find
The truth at last! Ground zero to the end. 🎬

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main changes: a best-effort cascade strategy for computing nb_frames and a new count_frames utility function.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/nb-frames-best-effort

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}