-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix process_video and add test case #2011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
|
||
| create_test_video(str(source_path), num_frames=5) | ||
|
|
||
| process_video( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any assertion or other validation, then it runs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a critical bug in the process_video function where a for-else construct was left after an if statement was removed, and improves the handling of max_frames values that exceed the total frames in a video. It also adds a test case to verify the fix.
Key changes:
- Removed the orphaned
elseblock that was executing duplicate frame processing logic - Added logic to cap
max_framesto the actual video frame count, preventing exceptions when requesting more frames than available - Simplified the progress bar total calculation by using the adjusted
max_framesvalue directly - Added a test case to verify the fix for max_frames exceeding total frames
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| supervision/utils/video.py | Fixed the for-else bug by removing the orphaned else block, and added logic to properly handle max_frames values larger than total_frames |
| test/utils/test_video.py | Added a new test file with a test case for the max_frames scenario and a helper function to create test videos |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| process_video( | ||
| source_path=str(source_path), | ||
| target_path=str(target_path), | ||
| callback=lambda frame, _: frame, | ||
| max_frames=10, | ||
| ) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test lacks assertions to verify the expected behavior. The test should validate that:
- The target video file was created successfully
- The video was processed without raising an exception
- The target video contains the expected number of frames (5, not 10)
Consider adding assertions such as checking that the target file exists and verifying the frame count of the output video.
| ``` | ||
| """ | ||
| source_video_info = VideoInfo.from_video_path(video_path=source_path) | ||
| max_frames = max_frames or source_video_info.total_frames |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the 'or' operator here will incorrectly handle the case when max_frames is 0. If a user passes max_frames=0 (meaning process zero frames), this line will replace it with source_video_info.total_frames instead of respecting the 0 value.
Consider using an explicit None check instead, such as:
max_frames = source_video_info.total_frames if max_frames is None else max_frames
| max_frames = max_frames or source_video_info.total_frames | |
| max_frames = ( | |
| source_video_info.total_frames if max_frames is None else max_frames | |
| ) |
This PR fixes the
process_videofunction.There used to be an
if-elsebut somewhere theiffell off the truck resulting in afor-elsethat doesn't make sense.This PR also fixes the following case of passing a
max_framesthat is larger than the frames in the video.Before this PR is would give
Exception: Requested frames are outboundand with it it will simply process the whole video, i.e. treat "max" frames as a max but OK if there are less frames (I think what most users expect/want).I also added a test case (the agent wrote it). Not sure if you want such tests or if/how you want to treat temp dirs and example asset video files in this repo. Let me know whatever you prefer and I can remove that or adjust.