Video handle persistence #64
Conversation
WalkthroughThe changes introduce a Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files selected for processing (2)
- sleap_io/io/video.py (6 hunks)
- tests/io/test_video_backends.py (2 hunks)
Additional comments (Suppressed): 4
tests/io/test_video_backends.py (1)
- 4-7: The import of the
pytestmodule is necessary for the new parameterized test. This change aligns with the goals of the PR.sleap_io/io/video.py (3)
43-56: The new attribute
keep_openand_open_readerhave been added to the class. Ensure that these attributes are properly initialized in the constructor of the class and that they are used correctly throughout the codebase.58-66: Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
84-100: The
from_filenamemethod has been updated to include thekeep_openparameter when creating instances ofMediaVideoandHDF5Video. Make sure that theMediaVideoandHDF5Videoclasses have been updated to accept this new parameter.
sleap_io/io/video.py
Outdated
| `get_frame` method of the `VideoBackend` class instead. | ||
| """ | ||
| if self.plugin == "opencv": | ||
| reader = cv2.VideoCapture(self.filename) | ||
| if self.keep_open: | ||
| if self._open_reader is None: | ||
| self._open_reader = cv2.VideoCapture(self.filename) | ||
| reader = self._open_reader | ||
| else: | ||
| reader = cv2.VideoCapture(self.filename) | ||
| reader.set(cv2.CAP_PROP_POS_FRAMES, frame_idx) | ||
| _, img = reader.read() | ||
| else: | ||
| with iio.imopen(self.filename, "r", plugin=self.plugin) as vid: | ||
| elif self.plugin == "pyav" or self.plugin == "FFMPEG": | ||
| if self.keep_open: | ||
| if self._open_reader is None: | ||
| self._open_reader = iio.imopen( | ||
| self.filename, "r", plugin=self.plugin | ||
| ) | ||
| reader = self._open_reader | ||
| img = vid.read(index=frame_idx) | ||
| else: | ||
| with iio.imopen(self.filename, "r", plugin=self.plugin) as vid: | ||
| img = vid.read(index=frame_idx) | ||
| return img | ||
|
|
There was a problem hiding this comment.
The _read_frame method has been updated to use the cached reader if keep_open is True. This could potentially improve performance when reading multiple frames. However, it's important to ensure that the reader is properly closed when it's no longer needed to avoid resource leaks.
+ if self.keep_open:
+ if self._open_reader is None:
+ self._open_reader = cv2.VideoCapture(self.filename)
+ reader = self._open_reader
+ else:
+ reader = cv2.VideoCapture(self.filename)| `get_frames` method of the `VideoBackend` class instead. | ||
| """ | ||
| if self.plugin == "opencv": | ||
| reader = cv2.VideoCapture(self.filename) | ||
| if self.keep_open: | ||
| if self._open_reader is None: | ||
| self._open_reader = cv2.VideoCapture(self.filename) | ||
| reader = self._open_reader | ||
| else: | ||
| reader = cv2.VideoCapture(self.filename) | ||
|
|
||
| reader.set(cv2.CAP_PROP_POS_FRAMES, frame_inds[0]) | ||
| imgs = [] | ||
| for idx in frame_inds: |
There was a problem hiding this comment.
Similar to the _read_frame method, the _read_frames method has also been updated to use the cached reader if keep_open is True. Again, make sure that the reader is properly closed when it's no longer needed.
+ if self.keep_open:
+ if self._open_reader is None:
+ self._open_reader = cv2.VideoCapture(self.filename)
+ reader = self._open_reader
+ else:
+ reader = cv2.VideoCapture(self.filename)
sleap_io/io/video.py
Outdated
| imgs.append(img) | ||
| imgs = np.stack(imgs, axis=0) | ||
|
|
||
| else: | ||
| with iio.imopen(self.filename, "r", plugin=self.plugin) as vid: | ||
| elif self.plugin == "pyav" or self.plugin == "FFMPEG": | ||
| if self.keep_open: | ||
| if self._open_reader is None: | ||
| self._open_reader = iio.imopen( | ||
| self.filename, "r", plugin=self.plugin | ||
| ) | ||
| reader = self._open_reader | ||
| imgs = np.stack([vid.read(index=idx) for idx in frame_inds], axis=0) | ||
| else: | ||
| with iio.imopen(self.filename, "r", plugin=self.plugin) as vid: | ||
| imgs = np.stack([vid.read(index=idx) for idx in frame_inds], axis=0) | ||
| return imgs |
There was a problem hiding this comment.
The changes in this hunk are similar to the ones in the _read_frame method. The same comments apply here.
+ if self.keep_open:
+ if self._open_reader is None:
+ self._open_reader = iio.imopen(
+ self.filename, "r", plugin=self.plugin
+ )
+ reader = self._open_reader
+ imgs = np.stack([vid.read(index=idx) for idx in frame_inds], axis=0)
+ else:
+ with iio.imopen(self.filename, "r", plugin=self.plugin) as vid:
+ imgs = np.stack([vid.read(index=idx) for idx in frame_inds], axis=0)
Codecov Report
@@ Coverage Diff @@
## main #64 +/- ##
==========================================
+ Coverage 94.64% 94.68% +0.03%
==========================================
Files 12 12
Lines 1195 1241 +46
==========================================
+ Hits 1131 1175 +44
- Misses 64 66 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (3)
- sleap_io/io/video.py (12 hunks)
- sleap_io/model/video.py (2 hunks)
- tests/io/test_video_backends.py (3 hunks)
Additional comments (Suppressed): 15
sleap_io/model/video.py (3)
46-51: The new
keep_openparameter has been added to thefrom_filenamemethod of theVideoclass. This parameter controls whether the video reader should be kept open between calls to read frames or closed after each call. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.56-62: The docstring has been updated to include a description for the new
keep_openparameter. The explanation is clear and concise, providing enough information about what the parameter does and its default value.67-76: The
keep_openparameter is passed to theVideoBackend.from_filenamemethod when initializing the backend. This change aligns with the goal of the PR to allow control over whether the video reader is cached for subsequent calls.tests/io/test_video_backends.py (1)
- 7-7: The import of the
pytestmodule is necessary for the new parameterized tests. This change is appropriate.sleap_io/io/video.py (11)
33-34: The change from using
cls.__attrs_attrs__toattrs.fields(cls)is a good one. It's more readable and maintainable.44-57: The addition of the
keep_openattribute and_open_readerattribute is well implemented. The default value forkeep_openis set toTrue, which aligns with the PR summary. This will potentially improve performance when reading multiple frames.72-78: The docstring for the
from_filenamemethod has been updated to include the newkeep_openparameter. This is a good practice as it keeps the documentation up-to-date with the code changes.86-101: The
from_filenamemethod now passes thekeep_openargument to theMediaVideoandHDF5Videoconstructors. This is consistent with the PR summary and the changes made to these classes.290-299: The
MediaVideoclass now includes thekeep_openattribute in its docstring, keeping the documentation up-to-date with the code changes.321-337: The
readerproperty has been updated to cache the reader object ifkeep_openisTrue. This is consistent with the PR summary and should improve performance when reading multiple frames.359-371: The
_read_framemethod has been updated to use the cached reader object ifkeep_openisTrue. This is consistent with the PR summary and should improve performance when reading multiple frames.384-396: The
_read_framesmethod has been updated to use the cached reader object ifkeep_openisTrue. This is consistent with the PR summary and should improve performance when reading multiple frames.440-449: The
HDF5Videoclass now includes thekeep_openattribute in its docstring, keeping the documentation up-to-date with the code changes.600-625: The
_read_framemethod has been updated to use the cached reader object ifkeep_openisTrue. This is consistent with the PR summary and should improve performance when reading multiple frames.637-665: The
_read_framesmethod has been updated to use the cached reader object ifkeep_openisTrue. This is consistent with the PR summary and should improve performance when reading multiple frames.
tests/io/test_video_backends.py
Outdated
| assert_equal(backend[-3:-1], backend.get_frames(range(1097, 1099))) | ||
|
|
||
|
|
||
| def test_mediavideo(centered_pair_low_quality_path): | ||
| @pytest.mark.parametrize("keep_open", [False, True]) | ||
| def test_mediavideo(centered_pair_low_quality_path, keep_open): | ||
| backend = VideoBackend.from_filename( | ||
| centered_pair_low_quality_path, plugin="FFMPEG" | ||
| centered_pair_low_quality_path, plugin="FFMPEG", keep_open=keep_open | ||
| ) | ||
| assert type(backend) == MediaVideo | ||
| assert backend.filename == centered_pair_low_quality_path | ||
| assert backend.shape == (1100, 384, 384, 1) | ||
| assert backend[0].shape == (384, 384, 1) | ||
| assert backend[:3].shape == (3, 384, 384, 1) | ||
| if keep_open: | ||
| assert backend._open_reader is not None | ||
| assert backend[0].shape == (384, 384, 1) | ||
| assert type(backend._open_reader).__name__ == "LegacyPlugin" | ||
|
|
||
| backend = VideoBackend.from_filename(centered_pair_low_quality_path, plugin="pyav") | ||
| backend = VideoBackend.from_filename( | ||
| centered_pair_low_quality_path, plugin="pyav", keep_open=keep_open | ||
| ) | ||
| assert type(backend) == MediaVideo | ||
| assert backend.filename == centered_pair_low_quality_path | ||
| assert backend.shape == (1100, 384, 384, 1) | ||
| assert backend[0].shape == (384, 384, 1) | ||
| assert backend[:3].shape == (3, 384, 384, 1) | ||
| if keep_open: | ||
| assert backend._open_reader is not None | ||
| assert backend[0].shape == (384, 384, 1) | ||
| assert type(backend._open_reader).__name__ == "PyAVPlugin" | ||
|
|
||
| backend = VideoBackend.from_filename( | ||
| centered_pair_low_quality_path, plugin="opencv" | ||
| centered_pair_low_quality_path, plugin="opencv", keep_open=keep_open | ||
| ) | ||
| assert type(backend) == MediaVideo | ||
| assert backend.filename == centered_pair_low_quality_path | ||
| assert backend.shape == (1100, 384, 384, 1) | ||
| assert backend[0].shape == (384, 384, 1) | ||
| assert backend[:3].shape == (3, 384, 384, 1) | ||
| if keep_open: | ||
| assert backend._open_reader is not None | ||
| assert backend[0].shape == (384, 384, 1) | ||
| assert type(backend._open_reader).__name__ == "VideoCapture" | ||
|
|
||
|
|
||
| def test_hdf5video_rank4(centered_pair_low_quality_path, tmp_path): | ||
| backend = VideoBackend.from_filename(centered_pair_low_quality_path) | ||
| @pytest.mark.parametrize("keep_open", [False, True]) | ||
| def test_hdf5video_rank4(centered_pair_low_quality_path, tmp_path, keep_open): | ||
| backend = VideoBackend.from_filename( | ||
| centered_pair_low_quality_path, keep_open=keep_open | ||
| ) | ||
| imgs = backend[:3] | ||
| assert imgs.shape == (3, 384, 384, 1) | ||
|
|
There was a problem hiding this comment.
Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [59-122]
The addition of the keep_open parameter to the test_mediavideo and test_hdf5video_rank4 functions is a good improvement. It allows testing the behavior of the VideoBackend class when the video reader is kept open between calls to read frames. The assertions added to check the _open_reader attribute when keep_open is True are also correct. However, there should be an else clause to assert that _open_reader is None when keep_open is False. This will ensure that the video reader is indeed closed after each call when keep_open is False.
if keep_open:
assert backend._open_reader is not None
assert backend[0].shape == (384, 384, 1)
+ else:
+ assert backend._open_reader is None
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- tests/io/test_video_backends.py (3 hunks)
Additional comments (Suppressed): 3
tests/io/test_video_backends.py (3)
7-7: The import of
pytestis necessary for the new parameterized tests. This change is appropriate.59-114: The changes to the
test_mediavideoandtest_hdf5video_rank4functions are well done. The use ofpytest.mark.parametrizeallows testing both scenarios wherekeep_openisTrueandFalse. The assertions checking the_open_readerattribute based on thekeep_openvalue are correct and ensure that the video reader caching behavior works as expected.122-128: The addition of the
keep_opencheck in thetest_hdf5video_rank4function is consistent with the changes made in other test functions. It ensures that the video reader caching behavior is tested in this scenario as well.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (2)
- sleap_io/io/video.py (12 hunks)
- tests/io/test_video_backends.py (3 hunks)
Additional comments (Suppressed): 9
tests/io/test_video_backends.py (3)
4-10: The import of
pytesthas been added. Ensure thatpytestis included in the project dependencies.59-104: The test function
test_mediavideohas been updated to include a parameter forkeep_open. The assertions have been updated to check the_open_readerattribute based on the value ofkeep_open. This is a good improvement as it tests both scenarios wherekeep_openisTrueandFalse.56-114: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [107-128]
The test function
test_hdf5video_rank4has been updated similarly totest_mediavideo, with the addition of thekeep_openparameter and corresponding assertions. This ensures that thekeep_openfunctionality is tested for different types of video backends.sleap_io/io/video.py (6)
31-34: The change from using
cls.__attrs_attrs__toattrs.fields(cls)is a good one. The latter is the recommended way to get attribute names in attrs classes, as it returns a list ofAttributeobjects which can be used to extract the name.44-57: The addition of the
keep_openattribute and its default value being set toTrueis a significant change. This attribute controls whether the video reader should be kept open between calls to read frames, potentially improving performance for repeated reads. The reader is cached in the_open_readerattribute for subsequent use. Ensure that this change doesn't introduce any resource leaks where file handles are left open indefinitely.85-101: The
from_filenamemethod now accepts thekeep_openparameter, which is passed down to theMediaVideoandHDF5Videoconstructors. This is consistent with the changes made to these classes.320-337: The new
readerproperty has been added to handle the creation and caching of the video reader object based on thekeep_openattribute. This is a good example of encapsulation, as it hides the complexity of managing the reader object behind a simple property.364-376: The
_read_framemethod has been updated to use the newreaderproperty instead of creating a new reader each time. This should improve performance when reading multiple frames from the same video.406-424: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [389-421]
The
_read_framesmethod has been updated in a similar way to the_read_framemethod, using thereaderproperty to manage the reader object. This is consistent with the changes made elsewhere in the code.
| @property | ||
| def num_frames(self) -> int: | ||
| """Number of frames in the video.""" | ||
| if self.plugin == "opencv": | ||
| return int(cv2.VideoCapture(self.filename).get(cv2.CAP_PROP_FRAME_COUNT)) | ||
| return int(self.reader.get(cv2.CAP_PROP_FRAME_COUNT)) | ||
| else: | ||
| return iio.improps(self.filename, plugin="pyav").shape[0] | ||
| props = iio.improps(self.filename, plugin=self.plugin) | ||
| n_frames = props.n_images | ||
| if np.isinf(n_frames): | ||
| legacy_reader = self.reader.legacy_get_reader() | ||
| # Note: This might be super slow for some videos, so maybe we should | ||
| # defer evaluation of this or give the user control over it. | ||
| n_frames = legacy_reader.count_frames() | ||
| return n_frames |
There was a problem hiding this comment.
The num_frames method has been updated to handle cases where the number of frames is infinite. This is a good improvement as it makes the code more robust to different types of videos. However, there's a comment indicating that counting frames might be slow for some videos. It would be beneficial to provide an option to control this behavior, perhaps by adding a timeout or a maximum number of frames to count.
This PR implements video handle persistence which should improve performance when reading repeatedly.
Addresses #62.
Summary by CodeRabbit
keep_openparameter to theVideoBackendclass and its subclasses insleap_io/io/video.py. This feature allows users to control whether the video reader should be kept open between calls to read frames or closed after each call, potentially improving performance when reading multiple frames.tests/io/test_video_backends.pyto include the newkeep_openparameter. This ensures that the new functionality is adequately tested under different scenarios.