--fps override silently truncates frame coverage to the head of the video

Component: watch skill v0.2.0 File: skills/watch/scripts/frames.py → extract(), on main @ 83da59f Severity: Low frequency, total impact when hit — coverage loss is silent and near-complete, and the report gives no indication anything was missed.

Summary

When extract() receives an fps high enough that fps * duration exceeds max_frames, the ffmpeg invocation combines an fps= filter with -frames:v max_frames. Those two together only cover max_frames / fps seconds of source — ffmpeg reaches the frame count and stops decoding. The entire frame budget is spent on the opening seconds and the rest of the video is never sampled.

Nothing in the report signals this. The header still reads full range, and the frame list looks well-formed — just clustered at the start.

Environment

• watch skill 0.2.0
• Windows 11, Python 3.x, ffmpeg/ffprobe on PATH
• Also reachable on macOS/Linux; nothing platform-specific in the affected code path

Reproduction

Fully self-contained — the clip is generated by ffmpeg, no assets needed:

ffmpeg -f lavfi -i "color=c=navy:s=320x240:r=10:d=600" \
       -c:v libx264 -preset ultrafast -pix_fmt yuv420p static600.mp4

# Affected
python scripts/watch.py static600.mp4 \
  --detail balanced --max-frames 20 --fps 2 --no-dedup --no-whisper

# Control (same clip, no --fps)
python scripts/watch.py static600.mp4 \
  --detail balanced --max-frames 20 --no-dedup --no-whisper

--no-dedup is required for the repro to be legible: on a fully static clip the dedup pass would otherwise collapse the output to a single frame and hide the distribution. --no-whisper just skips transcription. Any real screen recording or slide deck reproduces it without either flag.

Observed vs expected

The stderr line from the affected run shows the mismatch directly:

[watch] extracting scene-aware frames over full 600.0s (target 1200, cap 20)…

target 1200, cap 20 — the cap constrains the frame count, but nothing lowers the rate to match.

Root cause

watch.py:160 recomputes target from the override without reconciling it against the cap:

if args.fps is not None:
    fps = min(args.fps, MAX_FPS)
    target = max(1, int(round(fps * effective_duration)))   # 1200

That flows into extract_scene_or_uniform's uniform fallback (frames.py:554):

fallback_cap = target_frames if max_frames is None else min(max_frames, target_frames)
frames = extract(..., fps=fps, max_frames=fallback_cap, ...)

extract() then builds -vf fps=2,… alongside -frames:v 20. 20 frames at 2 fps is 10 seconds of coverage.

Scope

The trigger needs both conditions:

1. A near-static clip — fewer than SCENE_MIN_FRAMES (8) detected cuts, so extract_scene_or_uniform falls through to uniform sampling; and
2. An explicit --fps override.

Cut-heavy footage stays on the scene path, where extract_scene_candidates(max_frames=None) + _even_sample already guarantee full-range coverage (that design is well documented in the extract_scene_or_uniform docstring and works correctly — verified separately on a 2h21m feature film: 200 frames evenly drawn from 2,693 candidates spanning the entire runtime).

frames.py.__main__ reaches the same defect independently via its own --fps handling at line 735.

--detail efficient is structurally immune, which suggests this is an oversight rather than intended behavior: extract_keyframes() takes no fps parameter at all, and its uniform fallback recomputes the rate itself via auto_fps(eff_duration, max_frames=budget). The balanced path is the only one that accepts a caller-supplied fps and then caps the count without adjusting it.

Suggested fix

Guarding inside extract() covers all three call sites at once — extract_scene_or_uniform, extract_keyframes, and frames.py.__main__ — two of which can pass a truncating combination:

 if shutil.which("ffmpeg") is None:
     raise SystemExit("ffmpeg is not installed. Install with: brew install ffmpeg")

+# `fps` and `-frames:v max_frames` together only cover max_frames/fps seconds:
+# ffmpeg stops decoding there, so a cap smaller than fps*duration spends the whole
+# budget at the head of the range and leaves the tail unwatched. Lower fps instead
+# so the budget spans the requested range. Costs one ffprobe only when the range
+# has no explicit end.
+if fps > 0 and max_frames:
+    span_end = end_seconds if end_seconds is not None else get_metadata(video_path)["duration_seconds"]
+    span = max(0.0, span_end - (start_seconds or 0.0))
+    if span > 0 and max_frames < int(round(fps * span)):
+        fps = max_frames / span
+        print(
+            f"[watch] fps lowered to {fps:.4f} so {max_frames} frames span the full "
+            f"{span:.0f}s (the requested fps would have covered only the opening seconds)",
+            file=sys.stderr,
+        )
+
 out_dir.mkdir(parents=True, exist_ok=True)

Notes on the shape of the fix:

• int(round(fps * span)) deliberately mirrors how watch.py computes target, so the exactly-at-budget case (the normal auto-fps path, where fps * duration == max_frames) does not trip on floating-point noise and emit a spurious notice. Verified — the control run above prints nothing.
• The ffprobe call only happens when end_seconds is None, on a path that is already the rare fallback.
• It silently downgrades an impossible --fps, hence the stderr notice so the override doesn't disappear without explanation. If you'd rather preserve the requested rate and warn that coverage is partial, that's a reasonable alternative — but the current silent truncation seems clearly wrong either way.

Verified locally against main @ 83da59f: with the guard, the affected case spans t=00:00…09:30; control and efficient unchanged. The repro above was re-run today on an unpatched tree — affected t=00:00…00:10, control t=00:00…09:30.