Add video writing #129
Add video writing #129
Conversation
WalkthroughThe changes in this pull request introduce the Changes
Possibly related PRs
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (17)
docs/formats.md (2)
9-10: LGTM! Documentation updated correctly.The addition of the
sleap_io.save_videoentry is consistent with the existing documentation structure and is placed logically afterload_video. This update aligns well with the PR objective of introducing new video writing capabilities.Consider adding a brief description or example usage of the
save_videofunction to provide more context for users, similar to how other functions might be documented in the project.
Line range hint
1-24: Documentation updated to reflect new video writing capability.The addition of the
sleap_io.save_videoentry in the documentation is consistent with the PR objectives and existing documentation structure. This update effectively communicates the new video writing capability to users.To further improve the documentation:
- Consider grouping related functions (e.g.,
load_videoandsave_video) under a "Video Operations" section for better organization.- If not already present elsewhere in the documentation, consider adding a brief overview of the supported data formats at the beginning of this file to provide context for new users.
tests/io/test_video_writing.py (1)
7-15: LGTM: Test function covers basic functionality. Consider adding more comprehensive tests.The
test_video_writerfunction effectively tests the basic functionality of theVideoWriterclass. It writes frames to a video file and verifies the output's existence and shape. This is a good starting point for ensuring the video writing process works as expected.To enhance the test coverage, consider the following suggestions:
- Verify the content of the written frames, not just the shape.
- Add error handling tests (e.g., writing invalid frames).
- Test edge cases (e.g., writing an empty video, very large frames).
- Test different output formats or encoding options if supported.
Would you like assistance in implementing these additional test cases?
sleap_io/__init__.py (2)
30-31: Consider documenting the purpose of new imports.The new imports of
VideoBackendandVideoWriterclasses align with the PR objectives of adding video writing capabilities. However, they are currently flagged as unused by the static analysis tool.If these imports are intended to be part of the public API, consider one of the following options:
- Add them to the
__all__list if you have one (not shown in this file).- Use a redundant alias to silence the linter warning, e.g.,
VideoBackend as VideoBackend.- Add a comment explaining why they are imported here if they are meant to be accessible but not directly used.
Example:
# Expose VideoBackend and VideoWriter as part of the public API from sleap_io.io.video_reading import VideoBackend as VideoBackend from sleap_io.io.video_writing import VideoWriter as VideoWriter🧰 Tools
🪛 Ruff
30-30:
sleap_io.io.video_reading.VideoBackendimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
31-31:
sleap_io.io.video_writing.VideoWriterimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
Line range hint
1-31: Summary: Changes align with PR objectives, but some imports need clarification.The modifications to
sleap_io/__init__.pysuccessfully introduce new video writing capabilities, aligning with the PR objectives. The addition ofsave_video,VideoBackend, andVideoWriterenhances the package's functionality for handling videos.However, there are a few points that need attention:
- Some imported functions and classes are flagged as unused by the static analysis tool.
- The purpose of exposing
VideoBackendandVideoWriterin the package's public API is not immediately clear.Next steps:
- Review the usage of
save_video,load_file, andsave_filein the project and decide whether to keep them in the imports.- Clarify the intention behind importing
VideoBackendandVideoWriterand consider documenting their purpose if they are meant to be part of the public API.- If these imports are intentional, consider using techniques to silence the linter warnings, such as adding them to
__all__or using redundant aliases.These adjustments will improve the clarity and maintainability of the package's public API.
🧰 Tools
🪛 Ruff
23-23:
sleap_io.io.main.load_jabsimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
24-24:
sleap_io.io.main.save_jabsimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
25-25:
sleap_io.io.main.load_videoimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
26-26:
sleap_io.io.main.save_videoimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
27-27:
sleap_io.io.main.load_fileimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
28-28:
sleap_io.io.main.save_fileimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
30-30:
sleap_io.io.video_reading.VideoBackendimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
31-31:
sleap_io.io.video_writing.VideoWriterimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
tests/io/test_main.py (1)
110-117: LGTM: Test function is well-structured and covers key functionality.The
test_save_videofunction effectively verifies thesave_videofunctionality by:
- Saving a slice of the original video
- Loading and verifying the saved video's shape
- Re-saving the loaded video
- Loading the re-saved video and verifying its shape
This approach ensures that the
save_videofunction works correctly for both original and loaded video data.Consider enhancing the test by verifying the content of the saved video, not just its shape. This could involve comparing pixel values or checksums of the original and saved videos.
tests/io/test_video_reading.py (1)
59-61: LGTM: New test case for IndexError added.The new test case correctly checks for an IndexError when attempting to access an out-of-range frame index, which is consistent with the PR objectives.
Consider adding a comment explaining why 1100 is out of range (e.g., "Video has 1100 frames, indexed 0-1099") to improve readability:
with pytest.raises(IndexError): + # Video has 1100 frames, indexed 0-1099 backend.get_frame(1100)sleap_io/io/main.py (2)
153-178: LGTM: Well-implementedsave_videofunction.The new
save_videofunction is well-designed and implemented. It aligns with the PR objectives and provides a high-level interface for video writing. The docstring is comprehensive, and the implementation is concise and follows good practices.A minor suggestion for improvement:
Consider adding type hinting for the
**kwargsto improve code readability and IDE support. You can useTypedDictto define the expected keyword arguments:from typing import TypedDict class VideoWriterKwargs(TypedDict, total=False): fps: int pixelformat: str codec: str crf: int preset: str output_params: list[str] def save_video(frames: np.ndarray | Video, filename: str | Path, **kwargs: VideoWriterKwargs): ...This change would provide better type information for users of the function.
Line range hint
181-270: Existing functions unaffected, potential for enhancement.The addition of the
save_videofunction doesn't impact existing I/O operations. Theload_fileandsave_filefunctions remain unchanged and can implicitly support the new video format through their existing logic.Consider enhancing the
save_filefunction to explicitly support video saving:def save_file( data: Labels | np.ndarray | Video, filename: str | Path, format: Optional[str] = None, **kwargs ): ... elif format == "video": save_video(data, filename, **kwargs) ...This would provide a consistent interface for saving different types of data, including videos.
sleap_io/io/video_reading.py (2)
215-220: Improved frame index handling and error checking.The changes to the
get_framemethod enhance its robustness and flexibility:
- An
IndexErroris now raised for out-of-range frame indices, providing clear error messages.- Support for negative indices allows for more intuitive frame access, similar to Python's list indexing.
These improvements make the method more user-friendly and less prone to silent errors.
Consider adding a comment explaining the behavior of negative indices for clarity:
if frame_idx < 0: + # Support negative indexing (e.g., -1 for the last frame) frame_idx = frame_idx % len(self)
Line range hint
1-1000: Consider standardizing error handling across backend classes.While reviewing the file, I noticed that the error handling improvements made to the
VideoBackendclass could be beneficial if applied consistently across all backend classes (MediaVideo,HDF5Video, andImageVideo).Consider implementing similar index validation and negative index support in the
_read_framemethods of each backend class. This would ensure consistent behavior across all video backends and improve the overall robustness of the library.For example, in the
MediaVideoclass:def _read_frame(self, frame_idx: int) -> np.ndarray: if frame_idx >= self.num_frames: raise IndexError(f"Frame index {frame_idx} out of range.") if frame_idx < 0: frame_idx = frame_idx % self.num_frames # Existing implementation...Apply similar changes to
HDF5VideoandImageVideoclasses for consistency.sleap_io/io/video_writing.py (2)
66-69: Consider adding exception handling when opening the video writer.If an error occurs during the initialization of the video writer (e.g., invalid codec or file path issues), it will raise an uncaught exception. Adding exception handling can provide more informative error messages and allow for graceful degradation or cleanup.
You might wrap the writer initialization in a try-except block:
def open(self): """Open the video writer.""" self.close() self.filename.parent.mkdir(parents=True, exist_ok=True) + try: self._writer = iio_v2.get_writer( self.filename.as_posix(), format="FFMPEG", fps=self.fps, codec=self.codec, output_params=self.build_output_params(), ) + except Exception as e: + raise RuntimeError(f"Failed to open video writer: {e}")
43-51: Ensure all class attributes are documented in the docstring.Attributes like
output_paramsand_writerare present in the class but not described in the class docstring. Including all attributes in the documentation improves code readability and maintainability.Consider updating the docstring:
Attributes: filename: Path to output video file. fps: Frames per second. Defaults to 30. pixelformat: Pixel format for video. Defaults to "yuv420p". codec: Codec to use for encoding. Defaults to "libx264". crf: Constant rate factor to control lossiness of video. Values go from 2 to 32, with numbers in the 18 to 30 range being most common. Lower values mean less compressed/higher quality. Defaults to 25. No effect if codec is not "libx264". preset: H264 encoding preset. Defaults to "superfast". No effect if codec is not "libx264". + output_params: Additional output parameters for FFMPEG. Defaults to empty list. + _writer: Internal imageio writer object. Initialized in `open` method.sleap_io/io/slp.py (4)
Line range hint
188-213: Refactor Image Encoding Logic inembed_videoFunctionThe current implementation uses different methods to encode images based on the availability of
cv2, leading to code duplication. Consider refactoring to improve maintainability:if image_format == "hdf5": img_data = frame else: if frame.shape[-1] == 1: frame = frame.squeeze(axis=-1) encode_extension = "." + image_format if "cv2" in sys.modules: _, buffer = cv2.imencode(encode_extension, frame) img_data = buffer.astype("int8") else: img_data_bytes = iio.imwrite("<bytes>", frame, extension=encode_extension) img_data = np.frombuffer(img_data_bytes, dtype="int8")Applying this refactor reduces code duplication and unifies the image encoding logic.
Line range hint
223-225: Handle PotentialValueErrorWhen Replacing VideosIn the
embed_framesfunction:video_ind = labels.videos.index(video)If
videois not present inlabels.videos, this line can raise aValueError. To ensure robustness, add a check or exception handling:Apply this diff to handle the potential error:
+ try: video_ind = labels.videos.index(video) + except ValueError: + raise ValueError(f"Video {video} not found in labels.videos.")
Line range hint
265-268: Clarify the Use of theGROUPVariable inwrite_suggestionsThe variable
GROUPis hardcoded to0, and a TODO comment indicates handling extraneous metadata:GROUP = 0 # TODO: Handle storing extraneous metadata.Consider implementing the logic for storing metadata or removing the unused variable if it's not needed.
Line range hint
742-744: Correct Docstring inwrite_lfsFunctionIn the docstring for
write_lfs, there's a minor typo:- labels: A `Labels` object to store the metadata for. + labels: A `Labels` object containing the labeled frames to write.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- docs/formats.md (1 hunks)
- sleap_io/init.py (1 hunks)
- sleap_io/io/init.py (1 hunks)
- sleap_io/io/main.py (2 hunks)
- sleap_io/io/slp.py (1 hunks)
- sleap_io/io/video_reading.py (2 hunks)
- sleap_io/io/video_writing.py (1 hunks)
- sleap_io/model/video.py (1 hunks)
- tests/io/test_main.py (2 hunks)
- tests/io/test_slp.py (1 hunks)
- tests/io/test_video_reading.py (2 hunks)
- tests/io/test_video_writing.py (1 hunks)
- tests/model/test_video.py (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- sleap_io/model/video.py
- tests/io/test_slp.py
🧰 Additional context used
🪛 Ruff
sleap_io/__init__.py
26-26:
sleap_io.io.main.save_videoimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
27-27:
sleap_io.io.main.load_fileimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
28-28:
sleap_io.io.main.save_fileimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
30-30:
sleap_io.io.video_reading.VideoBackendimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
31-31:
sleap_io.io.video_writing.VideoWriterimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
sleap_io/io/__init__.py
3-3:
.video_readingimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
🔇 Additional comments (17)
tests/io/test_video_writing.py (2)
1-5: LGTM: File structure and imports are well-organized.The file structure is clean and follows good practices:
- Clear docstring describing the file's purpose
- Appropriate imports for the test requirements
- Good separation between imports and the test function
1-15: Overall, the test file is well-structured and provides a good foundation for testing video writing functionality.The
test_video_writerfunction effectively tests the basic operation of theVideoWriterclass. It demonstrates the ability to write frames to a video file and verify the output's existence and basic properties.To further strengthen the test suite, consider expanding it with additional test cases as suggested in the previous comment. This will help ensure the robustness and reliability of the video writing functionality across various scenarios.
sleap_io/__init__.py (2)
26-26: LGTM: Newsave_videofunction added to I/O operations.The addition of the
save_videofunction to the list of I/O operations enhances the video handling capabilities of the package. This aligns with the PR objectives of introducing new video writing functionality.🧰 Tools
🪛 Ruff
26-26:
sleap_io.io.main.save_videoimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
26-28: Verify the necessity of imported functions.The static analysis tool flags
save_video,load_file, andsave_fileas unused imports.Let's verify if these functions are used elsewhere in the project:
If these functions are not used elsewhere, consider removing them from the imports or adding them to
__all__if they are intended to be part of the public API.🧰 Tools
🪛 Ruff
26-26:
sleap_io.io.main.save_videoimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
27-27:
sleap_io.io.main.load_fileimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
28-28:
sleap_io.io.main.save_fileimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
tests/io/test_main.py (1)
14-14: LGTM: Import statement addition is correct.The addition of
save_videoto the import list is consistent with the PR objectives and is correctly placed within the existing import structure.tests/model/test_video.py (2)
Line range hint
1-224: Verify consistency ofvideo_readingmodule with previousvideomodule.While the import statement has been updated, the rest of the test file remains unchanged. This suggests that the refactoring maintains backwards compatibility, which is good. However, it's important to ensure that the behavior of
MediaVideoandImageVideoin the newvideo_readingmodule is consistent with their previous implementation in thevideomodule.To verify this consistency, please review the
video_readingmodule implementation:#!/bin/bash # Description: Display the content of the video_reading module cat sleap_io/io/video_reading.pyCompare this with the previous
videomodule implementation:#!/bin/bash # Description: Display the content of the previous video module git show HEAD^:sleap_io/io/video.pyEnsure that the
MediaVideoandImageVideoclasses maintain the same interface and behavior in the new module.
4-4: LGTM! Verify test suite execution.The import statement has been updated to reflect the new module structure mentioned in the PR objectives. This change from
sleap_io.io.videotosleap_io.io.video_readingaligns with the refactoring of the existing module structure.Please ensure that all tests in this file still pass after this change. Run the following command to verify:
tests/io/test_video_reading.py (3)
1-1: LGTM: Module documentation updated correctly.The module documentation has been updated to reflect the new module name
sleap_io.io.video_reading, which is consistent with the PR objectives of refactoring the module structure.
3-3: LGTM: Import statement updated correctly.The import statement has been updated to import from
sleap_io.io.video_reading, which is consistent with the PR objectives of refactoring the module structure.
Line range hint
1-61: Summary: File updates align with PR objectives and improve test coverage.The changes in this file correctly implement the refactoring of the module structure from
sleap_io.io.videotosleap_io.io.video_reading. The new test case for IndexError enhances the error handling coverage. These updates contribute to better organization and robustness of the codebase.sleap_io/io/main.py (2)
5-5: LGTM: Import statements updated correctly.The new imports for
video_writingandnumpyare appropriately added and necessary for the newsave_videofunction.Also applies to: 8-8
Line range hint
1-270: Summary: Excellent addition of video writing capabilities.The changes in this file successfully implement the PR objectives by adding video writing capabilities. The new
save_videofunction is well-designed and doesn't disrupt existing functionality. The code is clean, well-documented, and follows good practices.Key points:
- New imports are correctly added.
- The
save_videofunction provides a high-level interface for video writing.- Existing functions remain intact and can implicitly support the new video format.
Consider the suggested enhancements to further improve type hinting and consistency across I/O operations.
Overall, this is a valuable addition to the codebase that enhances its capabilities while maintaining its structure and readability.
sleap_io/io/video_reading.py (1)
Line range hint
1-1000: Overall improvement in video reading functionality.The changes made to the
sleap_io/io/video_reading.pyfile, particularly in theVideoBackendclass, enhance the robustness and usability of the video reading functionality. The improved frame index handling and error checking in theget_framemethod provide a more user-friendly experience and reduce the likelihood of silent errors.The file maintains its well-structured and comprehensive implementations for various video backends. The suggested improvements for consistency across backend classes and the minor nitpick for code clarity would further enhance the overall quality of the implementation.
These changes align well with the PR objectives of improving the video handling capabilities of the project.
sleap_io/io/video_writing.py (1)
98-110: Verify the handling of exceptions in context manager methods.Ensure that any exceptions raised within the context manager are appropriately handled or propagated. The current implementation returns
Falsein__exit__, which will not suppress exceptions, keeping standard behavior.If you'd like, I can help review the exception handling strategy for your context manager or provide examples of best practices.
sleap_io/io/slp.py (3)
23-23: Import Path Updated to Reflect Module RestructuringThe import statement has been updated to reflect the new module structure:
from sleap_io.io.video_reading import VideoBackend, ImageVideo, MediaVideo, HDF5VideoThis change aligns with the refactoring of
sleap_io.io.videotosleap_io.io.video_reading.
Line range hint
506-526: Potential Misalignment in Node and Edge SerializationIn the
serialize_skeletonsfunction, ensure that node IDs and indices are correctly mapped when serializing skeletons. Misalignment may cause incorrect node references in the serialized data.Run the following script to check for correct node and edge mappings:
#!/bin/bash # Description: Verify node IDs and indices alignment in serialized skeletons. # Test: Extract node IDs and ensure they match node indices in skeletons. ast-grep --lang python --pattern ' def serialize_skeletons(skeletons: list[Skeleton]) -> $_: $$$ ' sleap_io/io/slp.py -A 20 | grep -E 'node_to_id|nodes_dicts.append'
Line range hint
152-164: Verify Correct Handling of Embedded Videos inwrite_videosFunctionIn the
write_videosfunction, the logic for handling already embedded videos has been enhanced. Ensure that the check for existing embedded videos functions correctly, especially when re-embedding is needed:if Path(labels_path).exists(): with h5py.File(labels_path, "r") as f: already_embedded = f"video{video_ind}/video" in fRun the following script to confirm that embedded videos are correctly identified:
| def build_output_params(self) -> list[str]: | ||
| """Build the output parameters for FFMPEG.""" | ||
| output_params = [] | ||
| if self.codec == "libx264": | ||
| output_params.extend( | ||
| [ | ||
| "-crf", | ||
| str(self.crf), | ||
| "-preset", | ||
| self.preset, | ||
| ] | ||
| ) | ||
| return output_params + self.output_params | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor build_output_params for extensibility.
Currently, build_output_params only adjusts parameters for the libx264 codec. To support additional codecs in the future, consider generalizing this method or adding conditions for other codecs.
For example:
def build_output_params(self) -> list[str]:
"""Build the output parameters for FFMPEG."""
output_params = []
- if self.codec == "libx264":
+ if self.codec in ("libx264", "h264"):
output_params.extend(
[
"-crf",
str(self.crf),
"-preset",
self.preset,
]
)
+ # Add additional codec-specific parameters here
return output_params + self.output_params📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def build_output_params(self) -> list[str]: | |
| """Build the output parameters for FFMPEG.""" | |
| output_params = [] | |
| if self.codec == "libx264": | |
| output_params.extend( | |
| [ | |
| "-crf", | |
| str(self.crf), | |
| "-preset", | |
| self.preset, | |
| ] | |
| ) | |
| return output_params + self.output_params | |
| def build_output_params(self) -> list[str]: | |
| """Build the output parameters for FFMPEG.""" | |
| output_params = [] | |
| if self.codec in ("libx264", "h264"): | |
| output_params.extend( | |
| [ | |
| "-crf", | |
| str(self.crf), | |
| "-preset", | |
| self.preset, | |
| ] | |
| ) | |
| # Add additional codec-specific parameters here | |
| return output_params + self.output_params |
| def write_frame(self, frame: np.ndarray): | ||
| """Write a frame to the video. | ||
|
|
||
| Args: | ||
| frame: Frame to write to video. Should be a 2D or 3D numpy array with | ||
| dimensions (height, width) or (height, width, channels). | ||
| """ | ||
| if self._writer is None: | ||
| self.open() | ||
|
|
||
| self._writer.append_data(frame) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for frame in write_frame method.
To enhance robustness, consider validating that frame is a NumPy array with appropriate dimensions and data type before appending it to the writer. This can help catch unexpected inputs early.
You might include checks like:
def write_frame(self, frame: np.ndarray):
"""Write a frame to the video.
Args:
frame: Frame to write to video. Should be a 2D or 3D numpy array with
dimensions (height, width) or (height, width, channels).
"""
if self._writer is None:
self.open()
+ if not isinstance(frame, np.ndarray):
+ raise TypeError("Frame must be a NumPy ndarray.")
+ if frame.ndim not in (2, 3):
+ raise ValueError("Frame must be a 2D or 3D array.")
self._writer.append_data(frame)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def write_frame(self, frame: np.ndarray): | |
| """Write a frame to the video. | |
| Args: | |
| frame: Frame to write to video. Should be a 2D or 3D numpy array with | |
| dimensions (height, width) or (height, width, channels). | |
| """ | |
| if self._writer is None: | |
| self.open() | |
| self._writer.append_data(frame) | |
| def write_frame(self, frame: np.ndarray): | |
| """Write a frame to the video. | |
| Args: | |
| frame: Frame to write to video. Should be a 2D or 3D numpy array with | |
| dimensions (height, width) or (height, width, channels). | |
| """ | |
| if self._writer is None: | |
| self.open() | |
| if not isinstance(frame, np.ndarray): | |
| raise TypeError("Frame must be a NumPy ndarray.") | |
| if frame.ndim not in (2, 3): | |
| raise ValueError("Frame must be a 2D or 3D array.") | |
| self._writer.append_data(frame) |
|
|
||
| filename: Path = attrs.field(converter=Path) | ||
| fps: float = 30 | ||
| pixelformat: str = "yuv420p" |
There was a problem hiding this comment.
The pixelformat attribute is unused in the open method.
The pixelformat attribute is defined but not passed to get_writer in the open method. If specifying the pixel format is important for your video output, consider passing it to ensure the correct format is used.
Apply this diff to include pixelformat in get_writer:
self._writer = iio_v2.get_writer(
self.filename.as_posix(),
format="FFMPEG",
fps=self.fps,
codec=self.codec,
+ pixelformat=self.pixelformat,
output_params=self.build_output_params(),
)Committable suggestion was skipped due to low confidence.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
==========================================
+ Coverage 96.38% 96.48% +0.09%
==========================================
Files 15 17 +2
Lines 2049 2107 +58
==========================================
+ Hits 1975 2033 +58
Misses 74 74 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
sleap_io/io/video_writing.py (1)
80-110: Consider adding input validation towrite_framemethod.The
close,write_frame,__enter__, and__exit__methods are implemented correctly and provide the necessary functionality for theVideoWriterclass. However, as suggested in a previous review comment, consider adding input validation for theframeparameter in thewrite_framemethod to enhance robustness and catch unexpected inputs early.Example of input validation:
def write_frame(self, frame: np.ndarray): if not isinstance(frame, np.ndarray): raise TypeError("Frame must be a NumPy ndarray.") if frame.ndim not in (2, 3): raise ValueError("Frame must be a 2D or 3D array.") if self._writer is None: self.open() self._writer.append_data(frame)This validation will help prevent potential issues with incorrect input types or shapes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- sleap_io/io/video_reading.py (4 hunks)
- sleap_io/io/video_writing.py (1 hunks)
🔇 Additional comments (7)
sleap_io/io/video_writing.py (6)
13-50: LGTM: Well-structured class definition with appropriate attributes.The
VideoWriterclass is well-defined usingattrs.define, which simplifies the class implementation. The attributes cover essential parameters for video writing, and their default values seem reasonable. The use ofPathfor the filename and the lazy initialization of the_writerattribute are good practices.
52-65: Skipping comment generation.An existing review comment already addresses the extensibility concern for the
build_output_paramsmethod.
66-78: LGTM: Improvedopenmethod with addressed concerns.The
openmethod has been implemented well. It correctly handles directory creation and properly closes any existing writer before opening a new one. Thepixelformatattribute is now being passed toget_writer, addressing a previous review comment about its usage.
112-119: LGTM: Well-implemented__call__method.The
__call__method is a good addition to the class. It provides a convenient way to write frames by allowing the class instance to be called directly with a frame. This implementation enhances the usability of theVideoWriterclass and is consistent with its purpose.
1-119: Overall: Well-implemented VideoWriter class with minor improvement opportunities.The
VideoWriterclass provides a comprehensive and user-friendly interface for video writing operations using imageio and FFMPEG. The implementation is generally well-structured, follows good practices, and includes useful features such as context manager support.Key strengths:
- Clear and descriptive class and method docstrings
- Use of
attrsfor simplified class definition- Proper resource management with
openandclosemethods- Context manager support for ease of use
Areas for potential improvement:
- Input validation in the
write_framemethod- Extensibility of the
build_output_paramsmethod for different codecsOverall, this is a solid implementation that provides a valuable addition to the project's video writing capabilities.
7-8: Verify the need for both imageio imports.The code imports both
imageioandimageio.v2 as iio_v2. While this might be intentional, it's worth confirming if both imports are necessary. If possible, consider using only one version of imageio to maintain consistency and avoid potential version conflicts.sleap_io/io/video_reading.py (1)
226-228: Good Addition of Frame Index Validation inget_frameThe addition of the frame index validation in
get_frame(lines 226-228) enhances the robustness of the method by ensuring that anIndexErroris raised when an invalid frame index is requested. This provides clearer error handling and prevents potential downstream errors.
| def has_frame(self, frame_idx: int) -> bool: | ||
| """Check if a frame index is contained in the video. | ||
|
|
||
| Args: | ||
| frame_idx: Index of frame to check. | ||
|
|
||
| Returns: | ||
| `True` if the index is contained in the video, otherwise `False`. | ||
| """ | ||
| if self.frame_map: | ||
| return frame_idx in self.frame_map | ||
| else: | ||
| return frame_idx < len(self) | ||
|
|
There was a problem hiding this comment.
Handle Negative Frame Indices in has_frame Method for HDF5Video
In the HDF5Video subclass, the has_frame method (lines 637-650) does not handle negative frame indices. Negative indices may result in unintended behavior, especially when accessing frames via frame_map.
Consider modifying the method to ensure that frame_idx is non-negative before performing the check:
def has_frame(self, frame_idx: int) -> bool:
"""Check if a frame index is contained in the video.
Args:
frame_idx: Index of frame to check.
Returns:
`True` if the index is contained in the video, otherwise `False`.
"""
+ if frame_idx < 0:
+ return False
if self.frame_map:
return frame_idx in self.frame_map
else:
return frame_idx < len(self)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def has_frame(self, frame_idx: int) -> bool: | |
| """Check if a frame index is contained in the video. | |
| Args: | |
| frame_idx: Index of frame to check. | |
| Returns: | |
| `True` if the index is contained in the video, otherwise `False`. | |
| """ | |
| if self.frame_map: | |
| return frame_idx in self.frame_map | |
| else: | |
| return frame_idx < len(self) | |
| def has_frame(self, frame_idx: int) -> bool: | |
| """Check if a frame index is contained in the video. | |
| Args: | |
| frame_idx: Index of frame to check. | |
| Returns: | |
| `True` if the index is contained in the video, otherwise `False`. | |
| """ | |
| if frame_idx < 0: | |
| return False | |
| if self.frame_map: | |
| return frame_idx in self.frame_map | |
| else: | |
| return frame_idx < len(self) |
| def has_frame(self, frame_idx: int) -> bool: | ||
| """Check if a frame index is contained in the video. | ||
|
|
||
| Args: | ||
| frame_idx: Index of frame to check. | ||
|
|
||
| Returns: | ||
| `True` if the index is contained in the video, otherwise `False`. | ||
| """ | ||
| return frame_idx < len(self) | ||
|
|
There was a problem hiding this comment.
Handle Negative Frame Indices in has_frame Method
The has_frame method (lines 196-206) in VideoBackend does not account for negative frame indices. Since negative indices can lead to unexpected behavior or errors when accessing frames, it's important to ensure that only valid, non-negative indices are considered.
Consider updating the condition to include a check for non-negative indices:
def has_frame(self, frame_idx: int) -> bool:
"""Check if a frame index is contained in the video.
Args:
frame_idx: Index of frame to check.
Returns:
`True` if the index is contained in the video, otherwise `False`.
"""
- return frame_idx < len(self)
+ return 0 <= frame_idx < len(self)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def has_frame(self, frame_idx: int) -> bool: | |
| """Check if a frame index is contained in the video. | |
| Args: | |
| frame_idx: Index of frame to check. | |
| Returns: | |
| `True` if the index is contained in the video, otherwise `False`. | |
| """ | |
| return frame_idx < len(self) | |
| def has_frame(self, frame_idx: int) -> bool: | |
| """Check if a frame index is contained in the video. | |
| Args: | |
| frame_idx: Index of frame to check. | |
| Returns: | |
| `True` if the index is contained in the video, otherwise `False`. | |
| """ | |
| return 0 <= frame_idx < len(self) |
sio.VideoWriter: basicimageio-ffmpegvideo writer with sensible H264 presets. This can be used as a context manager:sio.save_video: high-level video writing. This can be used to quickly write a set of frames or even a wholeVideofor easy (if inefficient) re-encoding:IndexErrorinVideoBackendto enable sequence protocol for iteration overVideos:sio.io.videotosio.io.video_reading.Summary by CodeRabbit
Release Notes
New Features
save_videofunctionality for saving video files.VideoWriterclass for structured video writing using customizable settings.Bug Fixes
Documentation
Tests