Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support of mode and remove channels #3024

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

datumbox
Copy link
Contributor

@datumbox datumbox commented Nov 18, 2020

Fixes #3021

@datumbox datumbox requested review from fmassa and vfdev-5 November 19, 2020 09:07
Copy link
Contributor Author

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

torchvision/csrc/cpu/image/image_read_mode.h Show resolved Hide resolved
#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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: new line

I'll add it along with the other proposed corrections to minimize CI runs.

@datumbox datumbox changed the title [WIP] Add support of mode and remove channels Add support of mode and remove channels Nov 19, 2020
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks a lot!

I only have minor comments which are non-blocking, so I'll be moving forward with merging this PR.

Comment on lines +5 to +9
#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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
break;
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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here about the error message, it would be better to specify that this was not supported for PNG maybe?

torchvision/csrc/cpu/image/image_read_mode.h Show resolved Hide resolved
Comment on lines +98 to +101
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`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we add some of this documentation (or move it) to ImageReadMode?

@fmassa fmassa merged commit a884cb7 into pytorch:master Nov 19, 2020
@datumbox
Copy link
Contributor Author

Thanks Francisco, I agree with all suggestions. I opened a ticket so that we won't forget #3034.

@datumbox datumbox deleted the feature/read_image_mode branch November 19, 2020 19:19
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* Add support of mode and remove channels.

* Replacing integer mode with define constants.
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* Add support of mode and remove channels.

* Replacing integer mode with define constants.
@datumbox datumbox mentioned this pull request Jan 5, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improved format conversion in io.image.read_image
3 participants