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 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]") {