{{ message }}
Quality: Invalid frame index calculation can seek to -1 or out-of-range frames#1790
Open
Nam0101 wants to merge 1 commit into
Conversation
…nge frames `get_video_frame()` computes the seek position as `min(frame_total, frame_number - 1)`. With the default `frame_number=0`, this becomes `-1` (invalid frame index). For large requested frame numbers, it can also become `frame_total` (also invalid, since last valid frame is `frame_total - 1`). This causes intermittent failed reads (`has_frame=False`) and breaks callers that expect a valid first frame by default. Affected files: capturer.py Signed-off-by: Nguyen Van Nam <nam.nv205106@gmail.com>
Contributor
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
frame_numberparameter is effectively 1-based after this change (0 and 1 both map to the first frame); consider either making the default explicitly1or clarifying/standardizing its semantics across callers to avoid confusion. - The
target_indexcalculation can be simplified to a single clamp expression liketarget_index = max(0, min(frame_total - 1, frame_number - 1)), which makes the valid range and intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `frame_number` parameter is effectively 1-based after this change (0 and 1 both map to the first frame); consider either making the default explicitly `1` or clarifying/standardizing its semantics across callers to avoid confusion.
- The `target_index` calculation can be simplified to a single clamp expression like `target_index = max(0, min(frame_total - 1, frame_number - 1))`, which makes the valid range and intent clearer.
## Individual Comments
### Comment 1
<location path="modules/capturer.py" line_range="17" />
<code_context>
- frame_total = capture.get(cv2.CAP_PROP_FRAME_COUNT)
- capture.set(cv2.CAP_PROP_POS_FRAMES, min(frame_total, frame_number - 1))
+ frame_total = int(capture.get(cv2.CAP_PROP_FRAME_COUNT))
+ if frame_total <= 0:
+ capture.release()
</code_context>
<issue_to_address>
**issue:** Casting CAP_PROP_FRAME_COUNT to int directly can raise if the backend returns NaN.
Some OpenCV backends return NaN or other non-finite values for `CAP_PROP_FRAME_COUNT` on failure, and `int(float('nan'))` raises `ValueError`. To preserve the previous graceful `None` behavior instead of raising, read the raw value first, check for `None`/NaN/non-finite/negative (e.g. via `math.isfinite`), and only cast to `int` when it’s a valid non-negative frame count.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
issue: Casting CAP_PROP_FRAME_COUNT to int directly can raise if the backend returns NaN.
Some OpenCV backends return NaN or other non-finite values for CAP_PROP_FRAME_COUNT on failure, and int(float('nan')) raises ValueError. To preserve the previous graceful None behavior instead of raising, read the raw value first, check for None/NaN/non-finite/negative (e.g. via math.isfinite), and only cast to int when it’s a valid non-negative frame count.
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
1 participant
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.
✨ Code Quality
Problem
get_video_frame()computes the seek position asmin(frame_total, frame_number - 1). With the defaultframe_number=0, this becomes-1(invalid frame index). For large requested frame numbers, it can also becomeframe_total(also invalid, since last valid frame isframe_total - 1). This causes intermittent failed reads (has_frame=False) and breaks callers that expect a valid first frame by default.Severity:
highFile:
modules/capturer.pySolution
Clamp to a valid range explicitly and avoid negative/out-of-bounds indices:
frame_total = int(capture.get(cv2.CAP_PROP_FRAME_COUNT))if frame_total <= 0:capture.release()return None# keep 1-based API: frame_number=1 -> first frametarget_index = 0 if frame_number <= 1 else min(frame_total - 1, frame_number - 1)capture.set(cv2.CAP_PROP_POS_FRAMES, target_index)Changes
modules/capturer.py(modified)Testing
🤖 About this PR
This pull request was generated by ContribAI, an AI agent
that helps improve open source projects. The change was:
If you have questions or feedback about this PR, please comment below.
We appreciate your time reviewing this contribution!
Closes #1789
Summary by Sourcery
Bug Fixes: