Reject out-of-range seeks without decoding to EOF#69
Merged
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #67.
frame_count_was populated inopen()and never read inseek(). A knowably impossible target was therefore discovered only by scanning forward and decoding every remaining frame untilread()failed — O(frames remaining) of full decode and colour conversion for a request that could be rejected on arrival.The guard stays conditional on
frame_count_ > 0because-1means the container reports no count (common with MKV and some MP4), and there the scan really is the only way to find out.The test asserts position, not speed
Timing assertions are flaky, so the new test checks the observable consequence instead: a rejected seek should leave the reader where it was, whereas the old scan consumed the rest of the file first.
Verified it catches the old behaviour — reverting
VideoReader.cppfails with:The reader had walked to frame 60 on a 60-frame fixture as a side effect of a failed seek from frame 5.
Sentinel fix
cleanup()setframe_count_ = 0whileopen()uses-1andgetFrameCount()documents-1for "unknown". A closed reader reported a legitimate-looking count of zero. Now-1in both places; covered by a test that also fails on the old code (0 == -1).No existing code read
getFrameCount()apart from the Python binding'sframe_countproperty, so nothing depended on the old value.Full suite: 41 cases, 1135 assertions, all passing locally.
🤖 Generated with Claude Code