Pillow icon indicating copy to clipboard operation
Pillow copied to clipboard

Add inverse mappings for exif tags

Open melonmouse opened this issue 2 years ago • 7 comments

A nicer solution would be to switch to an enum with the right values, but this is the quick win.

Tested locally, ran through pycodestyle. To me it seems the code is trivial enough to not warrant adding a unit test.

melonmouse avatar Sep 16 '22 09:09 melonmouse

Could you describe an example use case for this feature? The current dictionary allows for the codes to be translated into something that can be understood by humans. What is your particular scenario where you want to go the other way?

radarhere avatar Sep 16 '22 09:09 radarhere

A nicer solution would be to switch to an enum with the right values, but this is the quick win.

Could you expand on this?

radarhere avatar Sep 16 '22 09:09 radarhere

Suppose a user wants to set an exif tag, I presume they would want to set the data using a human readable tag name.

melonmouse avatar Sep 16 '22 09:09 melonmouse

A nicer solution would be to switch to an enum with the right values, but this is the quick win.

Could you expand on this?

Using a dict to store these values feels hacky in the first place.

from enum import IntEnum


class ExifTags(IntEnum):
    InteropIndex = 0x0001
    ProcessingSoftware = 0x000B
    [...]

Which gives us just the right access:

$ExifTags(0x0001).name
InteropIndex

$ExifTags.InteropIndex.value
1

melonmouse avatar Sep 16 '22 09:09 melonmouse

To elaborate a bit more.

Currently

exif_data = Image.Exif()
exif_data[0x123456] = '2020:01:01 00:00:00'
image.save(filename, exif=exif_data.tobytes())

People have to manage how to keep it readable themselves. Their options include adding a comment, rolling their own inverse mapping or accepting less readable code.

After this pull request

exif_data = Image.Exif()
exif_data[TAG_CODES['DateTimeOriginal']] = '2020:01:01 00:00:00'
image.save(filename, exif=exif_data.tobytes())

Here we get the mapping for free, so we don't need to worry about the numerical exif ID (which, to the user, is an implementation detail). The code will crash if there is a typo in the name (which is a good thing) but your favorite IDE may have trouble autocompleting a tag name.

After bigger (breaking?) enum change

exif_data = Image.Exif()
exif_data[ExifTags.DateTimeOriginal] = '2020:01:01 00:00:00'
image.save(filename, exif=exif_data.tobytes())

Here your IDE will autocomplete the tagname. May need to update the code of ExifTags a bit to do this right. Could provide a TAGS dict generated from the Enum for backward compatibility.

melonmouse avatar Sep 16 '22 09:09 melonmouse

Here your IDE will autocomplete the tagname. May need to update the code of ExifTags a bit to do this right. Could provide a TAGS dict generated from the Enum for backward compatibility.

I'm in favour of this, but when we previously had this conversation in #5875, I implemented it, it was suggested that the original attributes be deprecated, and now there's #6537 where a user doesn't like that we changed the API. So I might hold off on this for a moment.

radarhere avatar Sep 16 '22 11:09 radarhere

Whenever adding anything to an API, it is important to think about a potential future cost of deprecating it again.

Deprecation of the old approach would be costly indeed. It is always a balance between quality of a library and the amount of maintenance for existing users. It is never possible to please everyone, but we can try our best. For a change like the one suggested in this thread, backward compatibility should win. Adding the Enum in a backward compatible way is possible however. I'm sorry about not noticing the backward compatibility break in #5954.

In any case, thank you for your hard work on PIL!

melonmouse avatar Sep 17 '22 09:09 melonmouse

I've created PR #6630 to use enums as a possible alternative to this.

radarhere avatar Oct 01 '22 07:10 radarhere

#6630 has been merged.

radarhere avatar Oct 24 '22 21:10 radarhere