From 52fb530488585a62ad85868604e8556c90013149 Mon Sep 17 00:00:00 2001
From: Kevin Blackburn-Matzen <kmatzen@gmail.com>
Date: Sat, 18 Jul 2026 16:58:52 -0700
Subject: [PATCH] Reject out-of-range seeks without decoding to EOF

frame_count_ was populated in open() and never read in seek(), so a
knowably impossible target was discovered only by scanning forward and
decoding every remaining frame until read() failed -- O(frames remaining)
of full decode and colour conversion for a request that could be rejected
immediately.

The guard stays conditional on frame_count_ > 0 because -1 means the
container reports no count (common with MKV and some MP4), and then the
scan really is the only way to find out.

The observable difference is position, which is what the new test asserts:
rejecting early leaves the reader where it was, whereas the old scan
consumed the rest of the file first. Without the fix it reports frame 60
on a 60-frame fixture after a failed seek from frame 5.

Also fixes the sentinel: cleanup() set frame_count_ to 0 while open() uses
-1 and getFrameCount() documents -1 for "unknown", so a closed reader
reported a legitimate-looking count of zero.

Fixes #67

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 src/VideoReader.cpp         | 14 +++++++++++++-
 tests/test_video_reader.cpp | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/src/VideoReader.cpp b/src/VideoReader.cpp
index 9e86453..bed37f5 100644
--- a/src/VideoReader.cpp
+++ b/src/VideoReader.cpp
@@ -327,6 +327,16 @@ bool VideoReader::seek(int64_t frame_number) {
         return false;
     }
 
+    // Reject a knowably out-of-range target before doing any work. Without
+    // this the seek scans forward decoding every remaining frame only to fail
+    // at EOF, which is O(frames remaining) of full decode for a request that
+    // cannot succeed. frame_count_ is -1 when the container does not report
+    // one (common with MKV and some MP4), and then the scan really is the only
+    // way to find out, so the guard stays conditional.
+    if (frame_count_ > 0 && frame_number >= frame_count_) {
+        return false;
+    }
+
     if (current_frame_ == frame_number) {
         return true;
     }
@@ -469,7 +479,9 @@ void VideoReader::cleanup() {
     width_ = 0;
     height_ = 0;
     fps_ = 0.0;
-    frame_count_ = 0;
+    // -1 means "unknown", matching open() and what getFrameCount() documents.
+    // 0 would be a legitimate-looking count for a closed reader.
+    frame_count_ = -1;
     current_frame_ = 0;
     current_timestamp_ = 0.0;
 }
diff --git a/tests/test_video_reader.cpp b/tests/test_video_reader.cpp
index 6bc97f8..5091cb5 100644
--- a/tests/test_video_reader.cpp
+++ b/tests/test_video_reader.cpp
@@ -258,6 +258,43 @@ TEST_CASE("VideoReader seek past the end fails", "[reader][seek]") {
     CHECK_FALSE(reader.seek(kSeekFixtureFrames + 100));
 }
 
+// A seek that cannot succeed should be rejected outright, not by scanning to
+// EOF and discovering the failure there. The observable difference is the
+// position: rejecting early leaves the reader where it was, whereas the old
+// scan consumed the rest of the file first. See #67.
+TEST_CASE("VideoReader rejects out-of-range seek without moving", "[reader][seek]") {
+    const std::string path = fixtures + "/seek_numbered.mp4";
+    const std::vector<int> ref = seekReference(path);
+
+    framewright::VideoReader reader;
+    REQUIRE(reader.open(path));
+    REQUIRE(reader.getFrameCount() == kSeekFixtureFrames);
+
+    cv::Mat frame;
+    for (int i = 0; i < 5; i++) REQUIRE(reader.read(frame));
+    REQUIRE(reader.getCurrentFrameNumber() == 5);
+
+    CHECK_FALSE(reader.seek(kSeekFixtureFrames));       // one past the last valid index
+    CHECK_FALSE(reader.seek(kSeekFixtureFrames + 100));
+
+    // Still at frame 5, and the next read still returns frame 5.
+    CHECK(reader.getCurrentFrameNumber() == 5);
+    REQUIRE(reader.read(frame));
+    CHECK(identifyFrame(frame, ref) == 5);
+}
+
+TEST_CASE("VideoReader reports unknown frame count after close", "[reader]") {
+    framewright::VideoReader reader;
+    REQUIRE(reader.open(fixtures + "/seek_numbered.mp4"));
+    CHECK(reader.getFrameCount() > 0);
+
+    reader.close();
+
+    // -1 is "unknown", as documented on getFrameCount(). 0 would read as a
+    // real count of zero frames.
+    CHECK(reader.getFrameCount() == -1);
+}
+
 #endif // HAVE_SEEK_FIXTURE
 
 TEST_CASE("VideoReader close and reopen", "[reader]") {