Adding FPS metadata to Video formats
#105
Conversation
WalkthroughThe recent updates enhance the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
==========================================
- Coverage 96.04% 96.03% -0.01%
==========================================
Files 17 17
Lines 2022 2044 +22
==========================================
+ Hits 1942 1963 +21
- Misses 80 81 +1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- sleap_io/io/video.py (9 hunks)
- sleap_io/model/video.py (4 hunks)
- tests/io/test_video_backends.py (1 hunks)
Additional comments not posted (8)
tests/io/test_video_backends.py (1)
17-17: LGTM! Assertion forframe_rateis correctly added.The assertion verifies that the
frame_rateattribute is correctly set to 15.0.sleap_io/model/video.py (2)
65-65: LGTM!_frame_rateparameter is correctly added.The
_frame_rateparameter is correctly passed to theVideoBackend.from_filenamemethod.Also applies to: 83-83, 98-98
113-121: LGTM!frame_rateproperty is correctly added.The
frame_rateproperty correctly retrieves the frame rate from the backend.sleap_io/io/video.py (5)
52-52: LGTM!_frame_rateattribute andframe_rateproperty are correctly added.The
_frame_rateattribute andframe_rateproperty are correctly implemented.Also applies to: 58-58, 197-201
69-69: LGTM!_frame_rateparameter and frame rate calculation are correctly added.The
_frame_rateparameter is correctly passed, and the frame rate is correctly calculated using OpenCV.Also applies to: 83-83, 88-92, 113-113, 122-122
327-327: LGTM!_frame_rateattribute is correctly added.The
_frame_rateattribute is correctly implemented.
483-483: LGTM!_frame_rateattribute is correctly added.The
_frame_rateattribute is correctly implemented.
725-725: LGTM!_frame_rateattribute is correctly added.The
_frame_rateattribute is correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap_io/io/video.py (8 hunks)
Additional context used
Ruff
sleap_io/io/video.py
89-90: Use a single
ifstatement instead of nestedifstatements(SIM102)
Additional comments not posted (5)
sleap_io/io/video.py (5)
52-52: Addition of_frame_rateattribute in class docstringThe addition of the
_frame_rateattribute in the class docstring ofVideoBackendis well-documented, providing clear information about its purpose.
58-58: Initialization of_frame_rateattributeThe
_frame_rateattribute is properly initialized toNone, which is appropriate for optional values. This allows for flexibility in cases where the frame rate might not be applicable or available.
83-83: Documentation for_frame_rateparameterThe documentation for the
_frame_rateparameter in thefrom_filenamemethod is clear and concise, explaining its purpose effectively.
122-122: Passing_frame_ratetoHDF5VideoconstructorThe
_frame_rateis correctly passed to theHDF5Videoconstructor, ensuring that the frame rate information is preserved across different video backends.
197-201: Property method forframe_rateThe
frame_rateproperty method is correctly implemented to return the_frame_rateattribute. This provides a clean API for accessing the frame rate of a video.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap_io/io/video.py (7 hunks)
Additional context used
Ruff
sleap_io/io/video.py
194-195: Use a single
ifstatement instead of nestedifstatements(SIM102)
Additional comments not posted (2)
sleap_io/io/video.py (2)
324-324: Addition of_frame_rateattribute toMediaVideo.The addition of the
_frame_rateattribute to theMediaVideoclass is consistent with the PR objectives and aligns with the changes made to theVideoBackendclass.
480-480: Addition of_frame_rateattribute toHDF5Video.The addition of the
_frame_rateattribute to theHDF5Videoclass is consistent with the PR objectives and aligns with the changes made to theVideoBackendclass.
|
I am getting the error |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
sleap_io/io/video.py (1)
58-58: Ensure_frame_rateis correctly handled inVideoBackendclass.The
_frame_rateattribute is initialized toNoneand is returned by a method, but there are no other assignments or method calls involving_frame_ratefound. This suggests that_frame_ratemight not be properly set to a meaningful value before being used.
- File:
sleap_io/io/video.py- Lines:
_frame_rateinitialization:sleap_io/io/video.py: _frame_rate = None- Method returning
_frame_rate:sleap_io/io/video.py: return self._frame_ratePlease ensure
_frame_rateis correctly set to a valid float value before being used in any operations.Analysis chain
Ensure
_frame_rateis correctly handled inVideoBackendclass.The
_frame_rateattribute has been initialized toNone. Ensure that it is correctly set and used in the class methods.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `_frame_rate` is correctly set and used in the `VideoBackend` class. # Test: Search for the `_frame_rate` usage. Expect: Correct handling in the class methods. rg --type python '_frame_rate' -A 5Length of output: 66
Script:
#!/bin/bash # Description: Verify that `_frame_rate` is correctly set and used in the `VideoBackend` class. # Test: Search for the `_frame_rate` usage. Expect: Correct handling in the class methods. rg --type py '_frame_rate' -A 5Length of output: 3490
Script:
#!/bin/bash # Description: Verify that `_frame_rate` is correctly set and used in the `VideoBackend` class. # Test: Search for assignments to `_frame_rate` and method calls involving `_frame_rate`. rg --type py '_frame_rate\s*=' -A 5 rg --type py 'self\._frame_rate' -A 5Length of output: 942
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap_io/io/video.py (6 hunks)
- sleap_io/model/video.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap_io/model/video.py
Additional comments not posted (5)
sleap_io/io/video.py (5)
52-52: Addition of_frame_rateattribute toVideoBackendclass.The addition of the
_frame_rateattribute aligns with the PR objectives and enhances the functionality of theVideoBackendclass by storing the frame rate of the video.
330-330: Addition of_frame_rateattribute toMediaVideoclass.The addition of the
_frame_rateattribute in the docstring aligns with the PR objectives and enhances the functionality of theMediaVideoclass by storing the frame rate of the video.
486-486: Addition of_frame_rateattribute toHDF5Videoclass.The addition of the
_frame_rateattribute in the docstring aligns with the PR objectives and enhances the functionality of theHDF5Videoclass by storing the frame rate of the video.
728-728: Addition of_frame_rateattribute toImageVideoclass.The addition of the
_frame_rateattribute aligns with the PR objectives and enhances the functionality of theImageVideoclass by storing the frame rate of the video.
195-203: Optimize conditional checks for file extension inframe_rateproperty.The nested
ifstatements checking for file extensions can be optimized into a single conditional expression. This simplifies the code and improves readability.- if isinstance(self.filename, str) and any( - self.filename.endswith(ext) for ext in video_extensions - ): + if isinstance(self.filename, str) and any(self.filename.endswith(ext) for ext in video_extensions):Likely invalid or redundant comment.
| @@ -77,6 +79,7 @@ def from_filename( | |||
| frames. If False, will close the reader after each call. If True (the | |||
| default), it will keep the reader open and cache it for subsequent calls | |||
| which may enhance the performance of reading multiple frames. | |||
| _frame_rate: Frame rate of the video. | |||
There was a problem hiding this comment.
Update type hint for _frame_rate parameter in from_filename method.
The type hint for _frame_rate should be Optional[float] instead of Optional[bool].
- _frame_rate: Optional[bool] = None,
+ _frame_rate: Optional[float] = None,Committable suggestion was skipped due to low confidence.
sleap_io/io/video.py
Outdated
| video_extensions = ["mp4", "avi", "mov", "mj2", "mkv", "slp"] | ||
| if isinstance(self.filename, str) and any( | ||
| self.filename.endswith(ext) for ext in video_extensions | ||
| ): | ||
| return cv2.VideoCapture(self.filename).get(cv2.CAP_PROP_FPS) |
There was a problem hiding this comment.
SUBGROUP REVIEW: We don't need to check the video extension or filename here since this property of frame_rate would only ever be accessed if filename pointed to a VIdeo object anyways
…oding/io_fork into kloding/add_video_fps_data
| def frame_rate(self) -> Optional[float]: | ||
| """Frames per second of the video.""" | ||
| video_extensions = ["mp4", "avi", "mov", "mj2", "mkv"] | ||
| if not any(self.filename.endswith(ext) for ext in video_extensions): |
There was a problem hiding this comment.
This conditional is necessary because ImageVideo inherits from VideoBackend. If we try to check the frame rate of ImageVideo objects, which are stored as image files, we will get an error.
Description
Right now, our video classes such as
VideoandMediaVideocan't store information about the fps of the video. Adding attributes to these classes that store this data will allow the user to set the frame rate, rather than setting it to a default value. I will write new tests and load videos with different frame rates in the GUI. I will also update the documentation, as necessary.Types of changes
Does this address any currently open issues?
#103
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit