Add support of mode and remove channels#3024
Conversation
datumbox
left a comment
datumbox
left a comment
There was a problem hiding this comment.
I left a couple of comments to highlight specific details.
| default: | ||
| jpeg_destroy_decompress(&cinfo); | ||
| TORCH_CHECK(false, "Invalid number of output channels."); | ||
| TORCH_CHECK(false, "Provided mode not supported"); |
There was a problem hiding this comment.
Note that if an unsupported conversion operation is requested, for instance CMYK to RGB, this check is not going to trigger. Instead we will get a RuntimeError: Unsupported color conversion request exception on the call of jpeg_start_decompress() below. I think it's better to leave libjpeg throw its own exception for its limitations than having extra code on our side to check for the same thing.
There was a problem hiding this comment.
Maybe it would be good to be explicit here and mention that it's not supported because the input is jpeg? Otherwise it could be confusing to the user, wdyt?
| #define IMAGE_READ_MODE_GRAY 1 | ||
| #define IMAGE_READ_MODE_GRAY_ALPHA 2 | ||
| #define IMAGE_READ_MODE_RGB 3 | ||
| #define IMAGE_READ_MODE_RGB_ALPHA 4 No newline at end of file |
There was a problem hiding this comment.
nit: new line
I'll add it along with the other proposed corrections to minimize CI runs.
datumbox
changed the title
fmassa
left a comment
fmassa
left a comment
There was a problem hiding this comment.
Looks great, thanks a lot!
I only have minor comments which are non-blocking, so I'll be moving forward with merging this PR.
| #define IMAGE_READ_MODE_UNCHANGED 0 | ||
| #define IMAGE_READ_MODE_GRAY 1 | ||
| #define IMAGE_READ_MODE_GRAY_ALPHA 2 | ||
| #define IMAGE_READ_MODE_RGB 3 | ||
| #define IMAGE_READ_MODE_RGB_ALPHA 4 No newline at end of file |
There was a problem hiding this comment.
nit: this is fine as is, but I wonder if a more modern pattern now is to use constexpr ImageReadMode kModeUnchanged = 0; or something like that, maybe within a namespace
| default: | ||
| jpeg_destroy_decompress(&cinfo); | ||
| TORCH_CHECK(false, "Invalid number of output channels."); | ||
| TORCH_CHECK(false, "Provided mode not supported"); |
There was a problem hiding this comment.
Maybe it would be good to be explicit here and mention that it's not supported because the input is jpeg? Otherwise it could be confusing to the user, wdyt?
| default: | ||
| png_destroy_read_struct(&png_ptr, &info_ptr, nullptr); | ||
| TORCH_CHECK(false, "Invalid number of output channels."); | ||
| TORCH_CHECK(false, "Provided mode not supported"); |
There was a problem hiding this comment.
Same comment here about the error message, it would be better to specify that this was not supported for PNG maybe?
| the image as-is, `ImageReadMode.GRAY` for converting to grayscale, | ||
| `ImageReadMode.GRAY_ALPHA` for grayscale with transparency, | ||
| `ImageReadMode.RGB` for RGB and `ImageReadMode.RGB_ALPHA` for | ||
| RGB with transparency. Default: `ImageReadMode.UNCHANGED` |
There was a problem hiding this comment.
nit: should we add some of this documentation (or move it) to ImageReadMode?
|
Thanks Francisco, I agree with all suggestions. I opened a ticket so that we won't forget #3034. |
* Add support of mode and remove channels. * Replacing integer mode with define constants.
* Add support of mode and remove channels. * Replacing integer mode with define constants.
3 participants
Fixes #3021