Refactor VideoProcessor class - #3358
Conversation
Convert entries in self.files to strings and sort them before use. This ensures a deterministic, predictable ordering of videos (e.g., when files are Path objects or come from an unordered collection) and avoids nondeterministic processing order.
Cast video and output path arguments to str before passing them to vp to ensure pathlib.Path or other path-like objects don't cause errors. Updated calls in proc_video, create_video, create_video_with_all_detections (and related CreateVideoSlow usage) to use str(video) and str(outputname/output_path) for improved compatibility with Path inputs.
There was a problem hiding this comment.
Pull request overview
This PR fixes labeled-video creation failures when GUI-selected videos are stored as a set and/or when video paths are provided as Path objects that OpenCV cannot consume directly. It ensures deterministic, list-like ordering for GUI video selection and normalizes video path inputs to str before passing them into the OpenCV video processor.
Changes:
- Convert
videoarguments tostrbefore constructingVideoProcessorCVinstances in labeled-video generation paths. - Convert GUI-selected videos from a
setto a sortedlist[str]before callingdeeplabcut.create_labeled_video.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
deeplabcut/utils/make_labeled_video.py |
Normalizes video/outputname to str when initializing the OpenCV video processor to avoid OpenCV type errors. |
deeplabcut/gui/tabs/create_videos.py |
Passes a deterministic, sorted list[str] of videos (instead of a set) into deeplabcut.create_labeled_video. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
@C-Achard, thanks for picking this up. A few remarks:
The GUI commit seems a bit misplaced to me, since collect_video_paths sorts as Paths right after.
More general, to keep a clear internal policy (i.e. prefere Path over str), I would prefer to solve it at the boundaries of our own package rather than in intermediate modules like create_videos.py and make_labeled_video.py. Let me know if you agree.
I think a good alternative would be to solve this in the VideoProcessor base class:
self.fname = str(fname) if fname else fname
self.sname = str(sname) if sname else sname
Same for VideoReader and VideoWriter
This keeps the solution to small isolated boundaries, where it is expected to fail (i.e. OpenCV can't handle pathlib, pass str to OpenCV, use Path elsewere).
Also let's add a test to pick this up in CI.
|
BTW, let me know if you prefer me to implement these suggestions directly in the PR |
This reverts commit 3e47c96.
|
@deruyter92 I think you're right, the fix is a bit too minimal and does not go well with your recent efforts for internal standardization to pathlib. I think I will instead spend a bit of time in the VideoProcessor class, I think it could use a careful overhaul and likely some tests. |
Introduce tests/utils/test_video_processor.py which adds comprehensive unit tests for deeplabcut.utils.video_processor.VideoProcessorCV. Includes a helper _make_test_video to produce temporary OpenCV videos and tests for metadata reading, frame loading and EOF behavior, nframes cap and -1 handling, fps override, video writing with default and explicit dimensions, idempotent close, context-manager cleanup, and invalid-input error handling.
Make VideoProcessor an abstract base class by importing ABC and abstractmethod and inheriting from ABC. Replace placeholder pass methods with @abstractmethod-decorated methods that raise NotImplementedError for get_video, get_info, create_video, _read_frame, save_frame, and close. Expose internal attributes via @Property for height, width, fps, counter, and frame_count. These changes formalize the interface and enforce implementation in subclasses.
Replace calls to clip.height(), clip.width(), and clip.fps() with attribute access (clip.height, clip.width, clip.fps) in make_labeled_video.py. This updates CreateVideo, CreateVideoSlow, proc_video, and create_video_with_all_detections to match the video object API and avoid method-call errors when these values are provided as properties.
Introduce fname/sname properties on VideoProcessor (with _fname/_sname initialization) to ensure string coercion and clearer attribute access. Expand VideoProcessorCV with comprehensive docstrings, clarify RGB<->BGR conversion behavior, add safety checks when reading frames, and make close() null out released OpenCV handles. Update tests: remove the context-manager close test and mark the invalid-input-video test as xfail for backwards compatibility.
Remove redundant str() conversions when creating vp clips in make_labeled_video.py now that the property coerces. Pass video and outputname/path variables directly (e.g. vp(fname=video), vp(video), vp(fname=video, sname=outputname)) to improve compatibility with pathlib.Path and avoid potential type issues. Updates applied in proc_video, create_video, and create_video_with_all_detections.
In deeplabcut/utils/video_processor.py initialize commonly used instance attributes (FPS, vid, svid, sh, sw) in the constructor to avoid missing-attribute errors. Replace a stray print() in the exception handler with a module logger and add logging import/logger setup. Also add a brief note about FPS handling to clarify behavior when overriding video FPS.
C-Achard
changed the title
Add a unit test that creates a temporary video file and passes a pathlib.Path to VideoProcessorCV. The test asserts that no exception is raised, that clip.fname is stored as a string, and ensures the clip is closed. This prevents regressions where Path objects might not be handled or converted to str by the processor.
Introduce Python type hints to the VideoProcessor.__init__ signature: fname: str, sname: str, nframes: int, fps: float, codec: str, sh: int, sw: int. This improves code clarity and static typing.
deruyter92
left a comment
deruyter92
left a comment
There was a problem hiding this comment.
Overall great improvements and good fix to the issue!
Some suggestions about consistency for types + private/public attributes
If those are addressed looks good to me!
|
@C-Achard, regarding the API breakage, maybe it would actually be a good idea to keep transitional backward compatibility, since it is relatively cheap to implement, e.g.: use properties with a new name (e.g. @property
def h(self):
return self._h
@deprecated(replacement="VideoProcessor.h", since="3.1")
def height(self):
return self._h |
Annotate fps, sh, and sw with None-aware union types and change sh/sw defaults from empty strings to None. Update logic to check for None rather than empty strings, and make fname/sname setters convert None or empty input to an empty string to avoid storing literal 'None'. Also guard frame writes by ensuring svid is set before calling write (prevents attempts to write when output video is not initialized). These changes improve type correctness and robustness when optional values are omitted.
Introduce a clearer VideoProcessor API: replace legacy FPS/width/height/frame_count attributes with video_fps, h, w, and nframes, add setters/getters and legacy compatibility methods (fps(), height(), width(), frame_count(), FPS). Improve docstrings and typing (Literal import), tighten handling of empty output sizes, and ensure video writers use video_fps. Update VideoProcessorCV to set/read video_fps from OpenCV and to warn when writing without an open writer; avoid writing None frames. Update make_labeled_video calls to use clip.video_fps and clip.h/clip.w, and adjust tests accordingly to reflect the new attribute names.
It should now be more of a middle ground. I'm not sure we really gain clarity from it but it is compatible. |
Add deprecation wrappers to legacy VideoProcessor methods and properties (counter, height, width, fps, FPS getter/setter) so callers are warned to migrate. Each decorator provides a replacement hint (i, h, w, video_fps) and a since="3.1" note. Also import the deprecated helper from deeplabcut.utils.deprecation to support these annotations. This preserves backward compatibility while guiding users toward the new attributes.
|
Thanks for addressing all the comments. Looks good to me!
Yes, for me this is also still a difficult balance - to what extent backward compatibility is favored over clarity. I think the middle ground you took is both clear and backward compatible. If you think we are overdoing it in this case, feel free to revert to your |
|
I think here problems from external use are less likely to occur compared to a higher level API. This is not really meant to be user facing, so I think I would favor maintainability for the more private API. I will re-adapt this a bit, as discussed, to be more towards what I think is the (c)lean side. |
Replace the legacy video_fps attribute with a slimmer fps property across the codebase. Updated VideoProcessor to expose fps (getter/setter) and changed VideoProcessorCV to initialize and use fps for reading and writing; callers in make_labeled_video now reference clip.fps. Removed several legacy/deprecated compatibility methods related to old accessors (counter, height, width, FPS, frame_count, etc.). Tests were adjusted to assert clip.fps instead of clip.video_fps. This consolidates FPS handling and simplifies the VideoProcessor API.
|
yes good consideration. I agree. Looks good now! Feel free to name the properties |
Replace ambiguous 'h' and 'w' properties with explicit 'height' and 'width' on VideoProcessor and its subclasses. Update all internal references (e.g. VideoProcessorCV initialization and make_labeled_video bbox/cropping logic) and adjust tests to assert clip.width and clip.height. Improves readability and consistency of video dimension access across the codebase.
Motivation
Fixes #3357.
The utils/make_labeled_video file uses the provided
videoas-is, butvideomay now be provided as a Path, which may not always be supported by OpenCV calls.Otherwise, the GUI stores selected videos in
self.files, which is backed by asetto avoid duplicates.However, labeled video creation expects a list-like sequence of video path strings.
Fix
Convert all uses ofvideotostrin utils/make_labeled_videoConvert the selected videos to a sortedlist[str]before callingdeeplabcut.create_labeled_videostris now immediate at the property level forfnameandsname