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

Remove image.copy() op from exif_transpose() #5574

Closed
wants to merge 2 commits into from
Closed

Remove image.copy() op from exif_transpose() #5574

wants to merge 2 commits into from

Conversation

glenn-jocher
Copy link

This version of the function avoids image copy and a dictionary build for the majority use case of missing exif tags / no action needed on call.

As implemented in YOLOv5 in ultralytics/yolov5#3852

This version of the function avoids image copy and a dictionary build for the majority use case of missing exif tags / no action needed on call.

As implemented in YOLOv5 in ultralytics/yolov5#3852
@radarhere
Copy link
Member

I count 3 changes here

  1. Moving method inside a new if block.

It seems to me like the extra code complexity isn't worth the very minor speed increase.

  1. You reverted Do not modify EXIF of original image instance in exif_transpose() #5547, deleting the orientation tag from the original image.

As #5546 explains in full, this means that while original image hasn't been transposed, its orientation tag is gone now.

from PIL import Image
im = Image.open("hopper.jpg")
exif = im.getexif()
orientation_before = exif.get(0x0112)
im.exif_transpose()
orientation_after = exif.get(0x0112)

The orientation_after will be empty, but the image data in im is still the same. I don't think this is desired.

  1. Removing the copy() operation if a transpose is not performed.

Because transpose returns a copy of the image, this keeps the output consistent. Without it,

from PIL import Image
im = Image.open("hopper.jpg")
im2 = im.exif_transpose()

sometimes, im will be the same object as im2, and sometimes it won't. That's a source of confusion for the user.

For example, afterwards, if I rotate im2,

im2.rotate(90)

I don't know whether or not im has been rotated, because it might the same image instance, or it might not.

@glenn-jocher
Copy link
Author

glenn-jocher commented Jul 2, 2021

CI is failing. I think this appears to be due to the CI itself expecting copies of images returned from the function rather than a problem in the function.

EDIT: Thanks for the comments @radarhere. I'll have to think through my changes again it seems.

@radarhere
Copy link
Member

The specific failure seen in AppVeyor at the moment isn't because this isn't copy()ing the image. It's a test from #5547. So it's because this is deleting the orientation tag from the original image's EXIF data - what I'm calling point 2 in these changes.

@glenn-jocher
Copy link
Author

@radarhere thanks for the detailed analysis. I understand the need for continuity of behavior within the PIL package, so in this case it seems we want slightly different behaviors, as in our case we want to prioritize speed, so to use the official function we'd probably want to have an if-statement outside the function to determine if it required transposing first, which would duplicate the code inside the function.

I think probably we'll implement our updated version in YOLOv5 with a comment referencing the source, and then I'll close this PR.

@glenn-jocher glenn-jocher deleted the patch-1 branch July 2, 2021 11:26
@glenn-jocher
Copy link
Author

@radarhere PR is closed. Thank you for your feedback!

@radarhere
Copy link
Member

In Pillow 10.0.0, we added an in_place argument through #7092. This means that you can now call ImageOps.exif_transpose(im, in_place=True) instead of im = ImageOps.exif_transpose(im) to avoid the copy() operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants