Add options to set background color when exporting video #1328
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1328 +/- ##
========================================
Coverage 73.11% 73.11%
========================================
Files 134 134
Lines 23963 23972 +9
========================================
+ Hits 17521 17528 +7
- Misses 6442 6444 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
roomrys
changed the title
|
Hi Liezl, I refractor and simplified the implementation with one option of |
There was a problem hiding this comment.
What we have right now looks good. To add,
- we can create a
sleap-rendercli argument forbackgroundand pass that in our call tosave_labeled_videoin thesleap.io.visuals.mainfunction. - Document the new optional argument in the cli.md
- Then, we can add a test function to run through the code via a psuedo
sleap-rendercall.
WalkthroughThis pull request introduces a new feature that allows users to specify the background color when saving videos. The changes span across several files, including modifications to function signatures, addition of command-line arguments, and updates to tests to cover the new functionality. 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: 1
Files ignored due to filter (1)
- sleap/config/labeled_clip_form.yaml
Files selected for processing (3)
- sleap/gui/commands.py (2 hunks)
- sleap/io/video.py (1 hunks)
- sleap/io/visuals.py (6 hunks)
Files skipped from review due to trivial changes (1)
- sleap/io/video.py
Additional comments (Suppressed): 5
sleap/gui/commands.py (2)
1295-1301: Ensure that all calls to this function throughout the codebase have been updated to include the new "background" parameter. Also, verify that the "background" parameter is always included in the
paramsdictionary before it's used.1355-1361: The new "background" parameter is being correctly extracted from the
export_optionsdictionary. However, ensure that the "background" key is always present in theexport_optionsdictionary before this code is executed to avoid a potential KeyError.sleap/io/visuals.py (3)
30-36: The
readerfunction has been updated to include a new parameter, "background". Ensure that all calls to this function throughout the codebase have been updated to match the new signature.514-518: The
save_labeled_videofunction has been updated to include a new parameter, "background". Ensure that all calls to this function throughout the codebase have been updated to match the new signature.559-560: The
readerfunction is being called with the new "background" parameter in theThreadconstructor. This looks good as long as all calls toreaderhave been updated to include this parameter.
sleap/io/visuals.py
Outdated
| loaded_chunk_idxs, video_frame_images = video.get_frames_safely( | ||
| frames_idx_chunk | ||
| ) | ||
| if background != "Original": | ||
| # fill the frame with the color | ||
| if background == "Black": | ||
| fill = 0 | ||
| elif background == "Grey": | ||
| fill = 127 | ||
| else: | ||
| # background == White | ||
| fill = 255 | ||
| video_frame_images = video_frame_images * 0 + fill |
There was a problem hiding this comment.
This block of code changes the background color of the video frames based on the value of the "background" parameter. However, it assumes that the "background" parameter will always be one of "Original", "Black", "Grey", or "White". If an unexpected value is passed, the code may behave unpredictably. Consider adding error handling for unexpected values.
if background != "Original":
# fill the frame with the color
if background == "Black":
fill = 0
elif background == "Grey":
fill = 127
elif background == "White":
fill = 255
else:
raise ValueError(f"Invalid background color: {background}")
video_frame_images = video_frame_images * 0 + fill
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (3)
- docs/guides/cli.md (1 hunks)
- sleap/io/visuals.py (8 hunks)
- tests/io/test_visuals.py (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/guides/cli.md
Additional comments (Suppressed): 5
tests/io/test_visuals.py (1)
- 76-78: Ensure that all calls to the
test_sleap_renderfunction throughout the codebase have been updated to match the new signature.sleap/io/visuals.py (4)
30-36: The function signature of
readerhas been updated to include a new parameter, "background". Ensure that all calls to this function throughout the codebase have been updated to match the new signature.517-520: The function signature of
save_labeled_videohas been updated to include a new parameter, "background". Ensure that all calls to this function throughout the codebase have been updated to match the new signature.720-728: A new command-line argument, "--background", has been added for setting the background color when saving videos. This is a good addition as it allows users to customize the output video's background color from the command line.
761-765: The call to
save_labeled_videonow includes the "background" argument. This is consistent with the changes made to the function signature.
tests/io/test_visuals.py
Outdated
| @pytest.mark.parametrize("background", ["original", "black", "white", "grey"]) | ||
| def test_sleap_render_with_different_backgrounds(background): | ||
| args = ( | ||
| f"-o test_{background}.avi -f 2 --scale 1.2 --frames 1,2 --video-index 0 --background {background} " | ||
| "tests/data/json_format_v2/centered_pair_predictions.json".split() | ||
| ) | ||
| sleap_render(args) | ||
| assert os.path.exists(f"test_{background}.avi") |
There was a problem hiding this comment.
The new test function test_sleap_render_with_different_backgrounds is a good addition to the test suite. It tests the rendering functionality with different background options, which aligns with the changes made in this PR. However, it would be better to also check if the output video has the correct background color. This can be done by reading the first frame of the output video and checking its color. If this is not feasible due to the complexity of the video processing library, at least ensure that the videos are not only created but also have non-zero size, indicating that they contain some data.
- assert os.path.exists(f"test_{background}.avi")
+ assert os.path.exists(f"test_{background}.avi") and os.path.getsize(f"test_{background}.avi") > 0
sleap/io/visuals.py
Outdated
| loaded_chunk_idxs, video_frame_images = video.get_frames_safely( | ||
| frames_idx_chunk | ||
| ) | ||
| if background != "original": | ||
| # fill the frame with the color | ||
| if background == "black": | ||
| fill = 0 | ||
| elif background == "grey": | ||
| fill = 127 | ||
| elif background == "white": | ||
| # background == White | ||
| fill = 255 | ||
| else: | ||
| raise ValueError(f"Invalid background color: {background}") | ||
| video_frame_images = video_frame_images * 0 + fill |
There was a problem hiding this comment.
This block of code is used to fill the frame with the specified color. However, it only supports three colors: black, grey, and white. If an unsupported color is passed, it raises a ValueError. Consider adding support for more colors or providing a better error message to guide users on what values are acceptable.
- raise ValueError(f"Invalid background color: {background}")
+ raise ValueError(f"Invalid background color: {background}. Supported colors are 'black', 'grey', and 'white'.")
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- tests/io/test_visuals.py (2 hunks)
Additional comments (Suppressed): 1
tests/io/test_visuals.py (1)
- 1-7: The import of
cv2is new in this hunk. Ensure that the OpenCV library (cv2) is included in your project dependencies.
tests/io/test_visuals.py
Outdated
| @pytest.mark.parametrize("background", ["original", "black", "white", "grey"]) | ||
| def test_sleap_render_with_different_backgrounds(background): | ||
| args = ( | ||
| f"-o test_{background}.avi -f 2 --scale 1.2 --frames 1,2 --video-index 0 --background {background} " | ||
| "tests/data/json_format_v2/centered_pair_predictions.json".split() | ||
| ) | ||
| sleap_render(args) | ||
| assert ( | ||
| os.path.exists(f"test_{background}.avi") | ||
| and os.path.getsize(f"test_{background}.avi") > 0 | ||
| ) | ||
|
|
||
| # Check if the background is set correctly if not original background | ||
| if background != "original": | ||
| saved_video_path = f"test_{background}.avi" | ||
| cap = cv2.VideoCapture(saved_video_path) | ||
| ret, frame = cap.read() | ||
|
|
||
| # Accumulate color channels | ||
| b, g, r = cv2.split(frame) | ||
| mean_b = np.mean(b) | ||
| mean_g = np.mean(g) | ||
| mean_r = np.mean(r) | ||
|
|
||
| # Set threshold values. | ||
| white_threshold = 240 | ||
| grey_threshold = 40 | ||
|
|
||
| # Check if the average color is white, grey, or black | ||
| if all(val > white_threshold for val in [mean_b, mean_g, mean_r]): | ||
| background_color = "white" | ||
| elif all(val < grey_threshold for val in [mean_b, mean_g, mean_r]): | ||
| background_color = "black" | ||
| else: | ||
| background_color = "grey" | ||
| assert background_color == background |
There was a problem hiding this comment.
This test function checks if the video rendering with different background colors works as expected. It's a good practice to test with various inputs, and the use of pytest's parametrize decorator is appropriate here. However, the way you're checking the background color could be improved. Instead of splitting the frame into color channels and calculating the mean for each channel, you can calculate the mean color of the entire frame using np.mean(frame, axis=(0, 1)). This will give you the mean color for all pixels in the frame, which should be more accurate.
- # Accumulate color channels
- b, g, r = cv2.split(frame)
- mean_b = np.mean(b)
- mean_g = np.mean(g)
- mean_r = np.mean(r)
+ # Calculate mean color of the frame
+ mean_color = np.mean(frame, axis=(0, 1))
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- tests/io/test_visuals.py (2 hunks)
Additional comments (Suppressed): 1
tests/io/test_visuals.py (1)
- 107-109: The existing test function
test_sleap_renderhas been modified to remove the background argument. This change is fine if the default behavior (without specifying a background) is being tested here. Ensure that this modification aligns with the intended testing strategy.
| @pytest.mark.parametrize("background", ["original", "black", "white", "grey"]) | ||
| def test_sleap_render_with_different_backgrounds(background): | ||
| args = ( | ||
| f"-o test_{background}.avi -f 2 --scale 1.2 --frames 1,2 --video-index 0 " | ||
| f"--background {background} " | ||
| "tests/data/json_format_v2/centered_pair_predictions.json".split() | ||
| ) | ||
| sleap_render(args) | ||
| assert ( | ||
| os.path.exists(f"test_{background}.avi") | ||
| and os.path.getsize(f"test_{background}.avi") > 0 | ||
| ) | ||
|
|
||
| # Check if the background is set correctly if not original background | ||
| if background != "original": | ||
| saved_video_path = f"test_{background}.avi" | ||
| cap = cv2.VideoCapture(saved_video_path) | ||
| ret, frame = cap.read() | ||
|
|
||
| # Calculate mean color of the channels | ||
| b, g, r = cv2.split(frame) | ||
| mean_b = np.mean(b) | ||
| mean_g = np.mean(g) | ||
| mean_r = np.mean(r) | ||
|
|
||
| # Set threshold values. Color is white if greater than white threshold, black | ||
| # if less than grey threshold and grey if in between both threshold values. | ||
| white_threshold = 240 | ||
| grey_threshold = 40 | ||
|
|
||
| # Check if the average color is white, grey, or black | ||
| if all(val > white_threshold for val in [mean_b, mean_g, mean_r]): | ||
| background_color = "white" | ||
| elif all(val < grey_threshold for val in [mean_b, mean_g, mean_r]): | ||
| background_color = "black" | ||
| else: | ||
| background_color = "grey" | ||
| assert background_color == background |
There was a problem hiding this comment.
This new test function test_sleap_render_with_different_backgrounds is a good addition to the test suite. It checks whether the video rendering with different background colors works as expected. However, there's a potential issue with the way the average color of the frame is calculated and compared to the threshold values. The mean color calculation does not take into account the distribution of the colors in the frame. A frame with a majority of white pixels and some black pixels could still have an average color that is grey. Consider using a more robust method for determining the predominant color in the frame.
- # Calculate mean color of the channels
- b, g, r = cv2.split(frame)
- mean_b = np.mean(b)
- mean_g = np.mean(g)
- mean_r = np.mean(r)
+ # Calculate the most frequent color of the channels
+ b, g, r = cv2.split(frame)
+ mode_b = scipy.stats.mode(b, axis=None)[0][0]
+ mode_g = scipy.stats.mode(g, axis=None)[0][0]
+ mode_r = scipy.stats.mode(r, axis=None)[0][0]
sleap/config/labeled_clip_form.yaml
Outdated
| - name: background | ||
| label: Video Background | ||
| type: list | ||
| options: Original,Black,White,Grey |
There was a problem hiding this comment.
These options should match case sensitivity, unless we handle this in visuals.py.
There was a problem hiding this comment.
Sure I'll make it lowercase in the docstrings and the option for GUI.
We could keep it in lowercase to be consistent.
sleap/io/visuals.py
Outdated
| @@ -515,6 +536,7 @@ def save_labeled_video( | |||
| fps: Frames per second for output video. | |||
| scale: scale of image (so we can scale point locations to match) | |||
| crop_size_xy: size of crop around instances, or None for full images | |||
| background: output video background. Either Original, Black, White, Grey | |||
There was a problem hiding this comment.
Match case sensitivity
| background: output video background. Either Original, Black, White, Grey | |
| background: output video background. Either original, black, white, grey |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files ignored due to filter (1)
- sleap/config/labeled_clip_form.yaml
Files selected for processing (1)
- sleap/io/visuals.py (8 hunks)
Additional comments (Suppressed): 4
sleap/io/visuals.py (4)
30-36: The function signature for
readerhas been updated to include a new parameter, "background". Ensure that all calls to this function throughout the codebase have been updated to match the new signature.72-86: This block of code is used to fill the frame with the specified color. However, it's not clear how this interacts with the original video frames. If the original frames are completely overwritten by the fill color, then the video content will be lost. Please verify if this is the intended behavior.
516-519: The function signature for
save_labeled_videohas been updated to include a new parameter, "background". Ensure that all calls to this function throughout the codebase have been updated to match the new signature.716-727: A new command-line argument, "--background", has been added. Ensure that the documentation has been updated to reflect this change.
sleap/io/visuals.py
Outdated
| # fill the frame with the color | ||
| if background == "black": | ||
| fill = 0 | ||
| elif background == "grey": | ||
| fill = 127 | ||
| elif background == "white": | ||
| fill = 255 | ||
| else: | ||
| raise ValueError(f"Invalid background color: {background}") |
There was a problem hiding this comment.
This block of code checks the value of the background variable and assigns a corresponding fill value. This could be simplified and made more maintainable by using a dictionary to map background names to fill values. Here's an example:
- if background == "black":
- fill = 0
- elif background == "grey":
- fill = 127
- elif background == "white":
- fill = 255
- else:
- raise ValueError(f"Invalid background color: {background}")
+ fill_values = {"black": 0, "grey": 127, "white": 255}
+ try:
+ fill = fill_values[background]
+ except KeyError:
+ raise ValueError(f"Invalid background color: {background}")
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- sleap/io/visuals.py (8 hunks)
Additional comments (Suppressed): 5
sleap/io/visuals.py (5)
30-36: The function signature of
readerhas been updated to include a new parameterbackground. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.515-518: The function signature of
save_labeled_videohas been updated to include a new parameterbackground. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.560-561: The
readerfunction is being called with the newbackgroundargument. This is consistent with the changes made to the function signature.718-726: A new command-line argument
--backgroundhas been added to specify the background color when saving videos. The help text clearly explains the purpose of this argument and the available options.762-763: The
save_labeled_videofunction is being called with the newbackgroundargument. This is consistent with the changes made to the function signature.
| loaded_chunk_idxs, video_frame_images = video.get_frames_safely( | ||
| frames_idx_chunk | ||
| ) | ||
| if background != "original": | ||
| # fill the frame with the color | ||
| fill_values = {"black": 0, "grey": 127, "white": 255} | ||
| try: | ||
| fill = fill_values[background] | ||
| except KeyError: | ||
| raise ValueError( | ||
| f"Invalid background color: {background}. Options include: {', '.join(fill_values.keys())}" | ||
| ) | ||
| video_frame_images = video_frame_images * 0 + fill |
There was a problem hiding this comment.
This block of code introduces a new feature to set the background color of the video. It checks if the provided background is not "original", and if so, it fills the frame with the specified color. However, there's no validation for the case when background is "original". If an invalid value is passed, the error will only be raised after some computation has already been done (i.e., after frames are loaded). To improve efficiency, consider validating the background argument at the beginning of the function.
+ fill_values = {"black": 0, "grey": 127, "white": 255, "original": None}
+ try:
+ fill = fill_values[background]
+ except KeyError:
+ raise ValueError(
+ f"Invalid background color: {background}. Options include: {', '.join(fill_values.keys())}"
+ )
loaded_chunk_idxs, video_frame_images = video.get_frames_safely(
frames_idx_chunk
)
- if background != "original":
- # fill the frame with the color
- fill_values = {"black": 0, "grey": 127, "white": 255}
- try:
- fill = fill_values[background]
- except KeyError:
- raise ValueError(
- f"Invalid background color: {background}. Options include: {', '.join(fill_values.keys())}"
- )
- video_frame_images = video_frame_images * 0 + fill
Description
Added background color configuration in video output. It can output with videos, without videos using (white, grey, black) backgrounds.
Types of changes
Does this address any currently open issues?
#921
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
--backgroundto the command-line interface, allowing users to specify the background color when saving videos. The available options are "original", "black", "white", and "grey".do_action,ask,reader,save_labeled_video) to accept an additional parameterbackgroundfor specifying the output video background color.