better handling of rgb8, removing some code duplication #101
Conversation
| @pytest.mark.parametrize("pre_encode_images", [False, True]) | ||
| def test_get_sample_filesystem(pre_encode_images: bool): | ||
| @pytest.mark.parametrize( | ||
| ["pre_encode_images", "rgb16", "rgba"], |
There was a problem hiding this comment.
making the test a bit more complete, generate test cases
| // Encode the image if needed | ||
| let mut image_bytes: Vec<u8> = Vec::new(); | ||
| if encode_images { | ||
| if new_image.color() != image::ColorType::Rgb8 { |
There was a problem hiding this comment.
what you were seeing was not a bug per say @tarek-ayed, it's just that the sanitization was coupled with image encoding, this PR decouples the two
| if encode_images { | ||
| if new_image.color() != image::ColorType::Rgb8 { | ||
| new_image = image::DynamicImage::ImageRgb8(new_image.to_rgb8()); | ||
| bit_depth = (new_image.color().bits_per_pixel() |
There was a problem hiding this comment.
there was a small bug here though, in that channels was not updated, so RGBA would return 4 channels. Fixed
| channels, | ||
| bit_depth, | ||
| }) | ||
| Ok(new_image) => { |
There was a problem hiding this comment.
refactored, both codepaths were essentially doing the same thing, now in a dedicated function
| @@ -58,9 +62,16 @@ def test_get_sample_filesystem(pre_encode_images: bool): | |||
| assert data.image.width == 100 | |||
| assert data.image.height == 100 | |||
|
|
|||
| if pre_encode_images: | |||
| if rgb16: | |||
There was a problem hiding this comment.
I got a bit confused by the name of the variable
rgb16 means "convert RGB 16 to RGB 8" right?
There was a problem hiding this comment.
it's "test rgb16 conversion by generating a rgb16 and checking that it's a rgb8 at the end of the pipe" really, sorry that it's not super clear
|
|
||
|
|
||
| def generate_tmp_files(dir, limit): | ||
| def generate_tmp_files(dir: str, limit: int, rgb16: bool = False, rgba: bool = False): |
There was a problem hiding this comment.
nit: This is a good candidate for using pytest fixtures. It allows you to make sure that test resources are cleaned up and you can also parametrize them and have one resource depend on another one etc.
tackle #100 and #93