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

Added a flag 'process_exif: bool = False' to 'read_image' #8109

Closed
wants to merge 3 commits into from

Conversation

Zekrom-7780
Copy link

Description

This pull request addresses issue #7977 by introducing a new feature to read and display EXIF metadata from images using the PyTorch Vision library.

Changes Made

  • I didn't know if Torch had any way to read the image's EXIF information, so I used PIL to do that
  • I added a bool process_exifand set it's default value to False.
  • If it is set to True, then the user can see the EXIF details of the image, and that is present in the function process_exif_metadata.
  • In the description of the function, I added what process_exif does.
  • I have formatted the code using ufmt

Reviewers

cc @pmeier @NicolasHug

P.S.

Sorry if this PR is written unprofessionally, and I didn't know if I had to add unit-tests for it, so I haven't added them here.

Copy link

pytorch-bot bot commented Nov 10, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8109

Note: Links to docs will display an error until the docs builds have been completed.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot
Copy link

Hi @Zekrom-7780!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@pmeier
Copy link
Collaborator

pmeier commented Nov 10, 2023

I didn't know if Torch had any way to read the image's EXIF information, so I used PIL to do that

Torch does not have such functionality yet. However, the whole point of torchvision.io is to be independent of PIL. So using PIL here to process the EXIF data is an antipattern. I guess this needs to happen on the C level likely as part of

torch::Tensor decode_png(
const torch::Tensor& data,
ImageReadMode mode,
bool allow_16_bits) {

and

torch::Tensor decode_jpeg(const torch::Tensor& data, ImageReadMode mode) {

I don't know if libpng and libjpeg-turbo provide utilities to process the EXIF data or if need to pull in more dependencies. If its the latter, this is a discussion on its own with the likely outcome that we cannot hard-depend on it.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@Zekrom-7780
Copy link
Author

@pmeier ,I looked at libpng and libpng_turbo, and I found that libpng does have the feature to read the EXIF data of the image

So, sould i completely erase the changes that I made here?

@pmeier
Copy link
Collaborator

pmeier commented Nov 13, 2023

So, sould i completely erase the changes that I made here?

Mostly yes I'm afraid. To avoid this in the future, please talk to us to get guidance if needed before you start implementing something

I found that libpng does have the feature to read the EXIF data of the image

What about libjpeg-turbo? Not sure if it makes sense to only have this handling for PNG images. If we go for only PNG, it should probably only go into decode_png and not read_image, since JPEG doesn't support it.

We have similar situation for JPEG already:

decode_jpeg has a device parameter

def decode_jpeg(
input: torch.Tensor, mode: ImageReadMode = ImageReadMode.UNCHANGED, device: str = "cpu"
) -> torch.Tensor:

that allows decoding on GPU, which is not available for decode_png

def decode_png(input: torch.Tensor, mode: ImageReadMode = ImageReadMode.UNCHANGED) -> torch.Tensor:

If you want to use the device parameter, you cannot use the generic decode_image or read_image though:

def decode_image(input: torch.Tensor, mode: ImageReadMode = ImageReadMode.UNCHANGED) -> torch.Tensor:

def read_image(path: str, mode: ImageReadMode = ImageReadMode.UNCHANGED) -> torch.Tensor:

Thoughts @NicolasHug?

@NicolasHug
Copy link
Member

I think we should prioritize support for jpeg over png (it's more common, and this is what the original feature request was using. As for where we expose the new parameter, exposing it at the highest level i.e. read_image() is fine IMO (along with decode_jpeg() of course). We can just ignore the parameter for png until it gets implemented, as long as it's documented.

@Zekrom-7780
Copy link
Author

Thank you soo much for your comments, I'll start working on only the .jpeg first, and then I'll try working on .png

@NicolasHug
Copy link
Member

Thanks for the PR @Zekrom-7780 , native C++ support for exif was added in (#8303, #8279, #8342, #8302)

@NicolasHug NicolasHug closed this May 31, 2024
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.

4 participants