Add a switch to control whether image.copy() or not in ImageOps.exif_transpose

[yantaozhao]

What did you expect to happen?

• Python: 3.9+
• Pillow: 9.5

Add a switch flag to control whether image.copy() or not if no orientation or orientation=1 in exif to the experimental function ImageOps.exif_transpose.

As pointed out in #5574 (comment) , pillow wants to keeps the output consistent .
However there's unnecessary computation&time cost, and even in many cases where the copy() is not needed, for example:

# Just open image with correct look as human eyes'

from PIL import Image
im = Image.open("hopper.jpg")
im = ImageOps.exif_transpose(im)  #assign to the same variable

#or simplified by:
im = ImageOps.exif_transpose(Image.open("hopper.jpg"))

It's better to let the user decide explicitly when he/she knows what's happening under the hood and wants more control. For lazy user just keep the old consistent manner.

the code maybe like this or something similar:

# Defined in ImageOps.py
def exif_transpose(image, copy_when_no_transpose=True):
    # check orientation in exif
    if method is not None:
        # do something
        return transposed_image
    else:
        return image.copy() if copy_when_no_transpose else image


----------------------------------------

# Called in user code
im = exif_transpose(im, copy_when_no_transpose=False)

[Yay295]

Here's a function I wrote for myself to do this:

from PIL import ExifTags, ImageOps

def apply_exif_orientation(img):
    orientation = img.getexif().get(ExifTags.Base.Orientation)
    if orientation and 2 <= orientation <= 8:
        return ImageOps.exif_transpose(img)
    return img

[yantaozhao]

Your code certainly works. That is the workaround now for there's no copy() switch in the official function.

But read and check exif twice in both user code and ImageOps.exif_transpose, with additional importing ExifTags and defining a function separately which also requires knowledge of exif, is not as simple as just one switch flag.

[radarhere]

from PIL import Image
im = Image.open("hopper.jpg")
im = ImageOps.exif_transpose(im)  #assign to the same variable

#or simplified by:
im = ImageOps.exif_transpose(Image.open("hopper.jpg"))

The first example here doesn't feel like an intuitive API to me.
The second example potentially loses the reference to the original image, which means you can't close the underlying file (this also applies to the first example, but you could wrap it in a context manager to avoid that).

If we wanted to do this, I think a better solution would be to follow the example of ImageCms.applyTransform(), and add an inPlace argument, defaulting to False, that if set to True, modifies the image in place and doesn't return anything.

[yantaozhao]

The second example potentially loses the reference to the original image, which means you can't close the underlying file (this also applies to the first example, but you could wrap it in a context manager to avoid that).

How about call Image.Image.load() inside ImageOps.exif_transpose if there's no transpose applied? return the loaded image or image.copy() depending on the switch flag.
The load() function will do the right thing, and the returned stuff which are loaded pixels is consistent as it is now.

[radarhere]

load() doesn't close the file if there are multiple frames.

[yantaozhao]

load() doesn't close the file if there are multiple frames.

Yes, the file is not closed.
And that is the current Pillow behaves. Compare 2 code snippets below:

Current ImageOps.exif_transpose in Pillow version 9.5:

from PIL import Image, ImageOps

### For single-frame image ###
# case 1: assign returned value to another variable
im = Image.open("hopper.jpg")
im2 = ImageOps.exif_transpose(im)  # the associated file is closed

# case 2: assign returned value to the same name variable
#   similar to `im=ImageOps.exif_transpose(Image.open("hopper.jpg"))`
im = Image.open("hopper.jpg")
im = ImageOps.exif_transpose(im)  # the associated file is closed


### For multi-frame image ###
# case 1:
im = Image.open("hopper.jpg")
im2 = ImageOps.exif_transpose(im)  # the associated file is still open
im.close()  # ok

# case 2:
im = Image.open("hopper.jpg")
im = ImageOps.exif_transpose(im)  # the associated file is still open
# then, resource leak because the file can not be closed later!!!

---

Expected function or similar thing might be:

# Defined in ImageOps.py
def exif_transpose(image, copy_when_no_transpose=True):
    # check orientation in exif
    if method is not None:
        # do something
        return transposed_image
    else:
        # ★★★ call load() here, return the loaded_image or a copy ★★★
        return image.copy() if copy_when_no_transpose else loaded_image

The imaginary code will be:

from PIL import Image, ImageOps

### For single-frame image ###
# case 1:
im = Image.open("hopper.jpg")
im2 = ImageOps.exif_transpose(im, copy_when_no_transpose=False)  # the associated file is closed

# case 2:
im = Image.open("hopper.jpg")
im = ImageOps.exif_transpose(im, copy_when_no_transpose=False)  # the associated file is closed


### For multi-frame image ###
# case 1:
im = Image.open("hopper.jpg")
im2 = ImageOps.exif_transpose(im, copy_when_no_transpose=False)  # the associated file is still open
im.close()  # ok

# case 2:
im = Image.open("hopper.jpg")
im = ImageOps.exif_transpose(im, copy_when_no_transpose=False)  # the associated file is still open
# then, resource leak because the file can not be closed later!!!

The behaviors are consistent in both situations of real code and imaginary code.
So call load() inside exif_transpose is ok. At least it complies current Pillow API's behavior.

Correct me if i am wrong.

[radarhere]

To be clear, I'm not suggesting that ImageOps.exif_transpose() should close the underlying file.

I'm saying that suggesting that users run

im = Image.open("hopper.gif")
im = ImageOps.exif_transpose(im, copy_when_no_transpose=False)

prevents them from closing the underlying file.

In describing Pillow's current behaviour, you've explained that case 2

im = ImageOps.exif_transpose(Image.open("hopper.gif"))

could leave the underlying file open if the image is a multiframe image, yes. However, the user could choose to run case 1 instead.

im = Image.open("hopper.gif")
im2 = ImageOps.exif_transpose(im)
im.close()

We don't tell users to run case 2. What you've outlined sounds to me like a good reason why you shouldn't use case 2. It goes against what we ask users to do in https://pillow.readthedocs.io/en/stable/releasenotes/7.0.0.html#image-del

Your proposed API doesn't allow for the user to close the underlying file in a simple way. After running

im = Image.open("hopper.gif")
im2 = ImageOps.exif_transpose(im, copy_when_no_transpose=False)

if I want to ensure that the original image is closed, but not the new one, that is tricky, because I don't know if they're the same or not. I would have to

im = Image.open("hopper.gif")
im2 = ImageOps.exif_transpose(im, copy_when_no_transpose=False)
if im is not im2:
  im.close()

That feels complicated.

Instead, my idea for an alternative was to use an inPlace argument.

im = Image.open("hopper.gif")
ImageOps.exif_transpose(im, inPlace=True)
im.close()

which seems much nicer.

[yantaozhao]

if I want to ensure that the original image is closed, but not the new one, that is tricky, because I don't know if they're the same or not. I would have to

im = Image.open("hopper.gif")
im2 = ImageOps.exif_transpose(im, copy_when_no_transpose=False)
if im is not im2:
  im.close()

That feels complicated.

You're right, things are becoming complicated.

Instead, my idea for an alternative was to use an inPlace argument.

im = Image.open("hopper.gif")
ImageOps.exif_transpose(im, inPlace=True)
im.close()

which seems much nicer.

So, you mean to create a new API alongside the exif_transpose?

[radarhere]

inPlace would be a new argument to the exif_transpose() method.

See #7092 for a demonstration of this.