josch/img2pdf
3
6
Fork 10
Code Issues 22 Pull requests 5 Projects Releases 3 Wiki Activity

Slightly simplify the getexif procedure #202

Open
mara0004 wants to merge 2 commits from mara0004/img2pdf:getexif into main
pull from: mara0004/img2pdf:getexif
merge into: josch:main
Conversation 0 Commits 2 Files changed 1 +37 -36
1 changed files with 37 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 4fa160259d - Show all commits

9
src/img2pdf.py
View file

@ -22,7 +22,7 @@ import sys
import os
import zlib
import argparse
from PIL import Image, TiffImagePlugin, GifImagePlugin, ImageCms
from PIL import Image, TiffImagePlugin, GifImagePlugin, ImageCms, ExifTags
if hasattr(GifImagePlugin, "LoadingStrategy"):
# Pillow 9.0.0 started emitting all frames but the first as RGB instead of
@ -1372,11 +1372,12 @@ def get_imgmetadata(
rotation = 0
if rotreq in (None, Rotation.auto, Rotation.ifvalid):
if hasattr(imgdata, "_getexif") and imgdata._getexif() is not None:
josch commented 2024-08-27 08:13:34 +00:00
Review
Copy link

checks like these for _getexif are there for a reason. Could you try and find out what changed that does not make these necessary anymore and since when it is like that?

checks like these for `_getexif` are there for a reason. Could you try and find out what changed that does not make these necessary anymore and since when it is like that?
mara0004 commented 2024-08-27 11:57:40 +00:00
Review
Copy link

getexif() (without underscore) is available since Pillow 6.0.0, that is more than 5 years ago.
Is requiring that acceptable to you?

Otherwise, I could add a fallback to _getexif(), provided that also returns a subscriptable dict which can be treated the same way as getexif().
But IMHO there's not much point in supporting far outdated versions of Pillow, as that makes the code harder to read.

`getexif()` (without underscore) is available since Pillow [6.0.0](https://pillow.readthedocs.io/en/stable/releasenotes/6.0.0.html#added-exif-class), that is more than 5 years ago. Is requiring that acceptable to you? Otherwise, I could add a fallback to `_getexif()`, provided that also returns a subscriptable dict which can be treated the same way as `getexif()`. But IMHO there's not much point in supporting far outdated versions of Pillow, as that makes the code harder to read.
josch commented 2024-09-15 05:54:40 +00:00
Review
Copy link

It makes the code harder to read, yes. But the advantage is, that this way, recent versions of img2pdf keep working on older platforms and distributions that ship Pillow before version 6 are still receiving support and security updates. I do not think it's smart to break img2pdf on a platform just to have 5 lines less of code. I mean imagine somebody from a library, archive or school (which at least where i'm from are known to update rarely) reports a bug that img2pdf broke for them. Will you explain them that this is because we wanted to have a few lines of code less?

It makes the code harder to read, yes. But the advantage is, that this way, recent versions of img2pdf keep working on older platforms and distributions that ship Pillow before version 6 are still receiving support and security updates. I do not think it's smart to break img2pdf on a platform just to have 5 lines less of code. I mean imagine somebody from a library, archive or school (which at least where i'm from are known to update rarely) reports a bug that img2pdf broke for them. Will you explain them that this is because we wanted to have a few lines of code less?
mara0004 commented 2024-09-15 13:57:13 +00:00
Review
Copy link

Indeed, I see Debian Buster is still on Pillow 5.4 :/
I understand your BW compat concerns and respect your decision, but would not do it that way in my own projects.

IMHO, if you are on a legacy base system, you should be prepared to accept that newer software may not be easily installable. If the institutions you mention do not update the base system, why should they update img2pdf? Sure, new version provide fixes and add features. But that applies to many, many packages throughout the whole system, so if you care about these improvements, you should upgrade your distribution.
I think it's legitimate to break compatibility with legacy versions of dependencies at some point and ask people to upgrade, and many (most?) other projects are doing so.
Sure, one specific case may not be relevant, but if you removed all workarounds for older versions, I bet it'll be more than 5 lines ;)

Anyway, I will look into adding the fallback.

Indeed, I see Debian Buster is still on Pillow 5.4 :/ I understand your BW compat concerns and respect your decision, but would not do it that way in my own projects. IMHO, if you are on a legacy base system, you should be prepared to accept that newer software may not be easily installable. If the institutions you mention do not update the base system, why should they update img2pdf? Sure, new version provide fixes and add features. But that applies to many, many packages throughout the whole system, so if you care about these improvements, you should upgrade your distribution. I think it's legitimate to break compatibility with legacy versions of dependencies at some point and ask people to upgrade, and many (most?) other projects are doing so. Sure, one specific case may not be relevant, but if you removed all workarounds for older versions, I bet it'll be more than 5 lines ;) Anyway, I will look into adding the fallback.
for tag, value in imgdata._getexif().items():
if TAGS.get(tag, tag) == "Orientation":
exif_dict = imgdata.getexif()
o_key = ExifTags.Base.Orientation.value # 274 rsp. 0x112
if exif_dict and o_key in exif_dict:
# Detailed information on EXIF rotation tags:
# http://impulseadventure.com/photo/exif-orientation.html
value = exif_dict[o_key]
if value == 1:
rotation = 0
elif value == 6: