-
Notifications
You must be signed in to change notification settings - Fork 170
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
Add basic support for .heic files #519
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and sorry about the delay. The PR looks good but it will need a rebase. Also would be great if you can add a test and test image (with an open source license).
src/sigal/image.py
Outdated
if hasattr(img, "_getexif") and callable(img._getexif): | ||
exif = img._getexif() | ||
elif "exif" in img.info: | ||
exif = img.getexif()._get_merged_dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
img._getexif()
is an alias for img.getexif()._get_merged_dict()
so we can probably switch to the public methods, which I think are supported since a long time ? (Our min version of pillow is 8.0 currently)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gladly!
I just wasn't sure if I would break any weird combination, because both variants use some private API.
But let tox find out :-)
@@ -264,7 +264,7 @@ def exif(self): | |||
datetime_format = self.settings["datetime_format"] | |||
return ( | |||
get_exif_tags(self.raw_exif, datetime_format=datetime_format) | |||
if self.raw_exif and self.src_ext in (".jpg", ".jpeg") | |||
if self.raw_exif and self.src_ext in (".jpg", ".jpeg", ".heic") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extension list here and below should also include ".heif" I guess ?
Also, those extensions should be recognized only if pillow-heif is available, so might be better to store this list somewhere where it can be imported (in the image
module ?) and reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about .heif
files. As I understand it, this is more of a container format and may contain different image-formats.
I guess pillow-heif
should be able to handle them all, but I have no way to test that yet.
I would actually prefer it if the program errors out if it encounters .heic
images without pillow-heif
installed, instead of just silently ignoring and skipping those pictures.
I mean, they are in fact Images - no matter if supported or not - and the user probably intended for them to be included in the gallery.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #519 +/- ##
==========================================
- Coverage 90.72% 90.55% -0.17%
==========================================
Files 24 24
Lines 2026 2033 +7
==========================================
+ Hits 1838 1841 +3
- Misses 188 192 +4 ☔ View full report in Codecov by Sentry. |
No description provided.