From 43649509577ba9cbeff7ba2dfc3eab5dd6607a95 Mon Sep 17 00:00:00 2001
From: Kevin Blackburn-Matzen <kmatzen@gmail.com>
Date: Sat, 18 Jul 2026 16:19:53 -0700
Subject: [PATCH] Stop fabricating a 30 fps fallback in seek()

#35 made getFPS() report 0.0 when the container declares no frame rate,
rather than silently claiming 30. seek() kept its own fallback and still
assumed {30, 1} to compute a target timestamp, so the class gave two
different answers to "what is this file's frame rate".

Since #51 the fabricated rate could no longer mis-position the reader --
the position-recovery code recomputes the duration from the real rate,
finds it unusable and fails -- so the fallback was only ever producing a
target timestamp that was then guaranteed to be discarded.

Keyframe seeking needs the rate twice: once to turn the requested index
into a timestamp, once to turn the landing timestamp back into an index.
Neither is possible without a declared rate, so the rate is now selected
once, up front, and seek() fails cleanly if there is none. That also
removes the duplicated avg/r_frame_rate selection, which had drifted --
the first copy checked num and den, the second only num.

No test: constructing a fixture whose container declares no frame rate is
awkward, since ffmpeg infers one for essentially every format it writes.
The change is exercised indirectly by the existing seek tests, which all
take the rate-available path.

Fixes #52

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 src/VideoReader.cpp | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/src/VideoReader.cpp b/src/VideoReader.cpp
index 3eb20c1..9e86453 100644
--- a/src/VideoReader.cpp
+++ b/src/VideoReader.cpp
@@ -338,19 +338,28 @@ bool VideoReader::seek(int64_t frame_number) {
         (frame_number < current_frame_) || (frame_number - current_frame_ > 50);
 
     if (need_keyframe_seek) {
-        int64_t target_ts;
-        if (stream->avg_frame_rate.num != 0 && stream->avg_frame_rate.den != 0) {
-            target_ts = av_rescale_q(frame_number,
-                                     av_inv_q(stream->avg_frame_rate),
-                                     stream->time_base);
-        } else {
-            AVRational fps = stream->r_frame_rate;
-            if (fps.num == 0 || fps.den == 0) {
-                fps = {30, 1};
-            }
-            target_ts = av_rescale_q(frame_number, av_inv_q(fps), stream->time_base);
+        // Keyframe seeking needs a frame rate twice: to turn the requested
+        // frame index into a target timestamp, and to turn the landing
+        // timestamp back into an index. If the container declares none there
+        // is no honest way to do either, so fail here rather than proceed on a
+        // fabricated rate -- getFPS() reports 0.0 in exactly this case, and the
+        // two must not disagree.
+        AVRational seek_fps =
+            (stream->avg_frame_rate.num != 0 && stream->avg_frame_rate.den != 0)
+                ? stream->avg_frame_rate
+                : stream->r_frame_rate;
+
+        if (seek_fps.num == 0 || seek_fps.den == 0) {
+            detail::log(LogLevel::Error)
+                << "framewright::VideoReader: Cannot seek by frame number without a frame rate"
+                << std::endl;
+            return false;
         }
 
+        const double frame_duration = av_q2d(av_inv_q(seek_fps));
+        const int64_t target_ts =
+            av_rescale_q(frame_number, av_inv_q(seek_fps), stream->time_base);
+
         auto seek_to_keyframe = [&]() -> bool {
             int ret =
                 av_seek_frame(formatCtx_, videoStreamIndex_, target_ts, AVSEEK_FLAG_BACKWARD);
@@ -376,11 +385,6 @@ bool VideoReader::seek(int64_t frame_number) {
             return false;
         }
 
-        AVRational seek_fps =
-            stream->avg_frame_rate.num != 0 ? stream->avg_frame_rate : stream->r_frame_rate;
-        double frame_duration =
-            (seek_fps.num != 0 && seek_fps.den != 0) ? av_q2d(av_inv_q(seek_fps)) : 0.0;
-
         // Without a usable timestamp there is no way to know where we landed.
         // Rewind to the start so the reader is left in a position it can
         // honestly report, then fail.