Implement minimal EXIF and IPTC tag support #40
Conversation
sebastiaan-lampo
force-pushed
the
photo_exif
branch
from
c20d43c
to
cb39946
Compare
Use PIL's TAGS to deal with EXIF. Use iptcinfo3 to deal with IPTC tags. Allows to use these as fallback options for copyright and caption of photos. Will not change the default behaviour without specifying the setting tag_map, exif or iptc to the configuration file.
sebastiaan-lampo
force-pushed
the
photo_exif
branch
from
cb39946
to
e393a61
Compare
There was a problem hiding this comment.
Thanks for adding this @sebastiaan-lampo! Very cool addition. I have some small comments from looking through the code and testing it, but hopefully these should be easy to fix. Thanks again and sorry for the delay! :)
hugophotoswipe/photo.py
Outdated
| def free(self): | ||
| """Manually clean up the cached image""" | ||
| if hasattr(self, "_original_img") and self._original_img: | ||
| self._original_img.close() | ||
| del self._original_img | ||
| self._original_img = None | ||
|
|
||
| @property | ||
| def iptc(self): |
There was a problem hiding this comment.
What's the rationale for not having a _load_iptc method, similar to the _load_exif method?
There was a problem hiding this comment.
Good point & implemented.
The only downside is that it now always attempts to load IPTC. We always needed exif for orientation but we could lazy-load IPTC on first use. Right now it will simply load both.
The benefit of the code alignment may outweigh the performance penalty for users that don't use iptc data. I haven't tested the performance penalty yet.
There was a problem hiding this comment.
I'm not sure I understand why you can't lazy load IPTC anyway. In _get_tag_value, you can load IPTC when needed.
Instead of:
o = getattr(self, obj.lower(), {})
you could use:
if obj.lower() == 'exif':
return self.exif.get(t)
return self.iptc.get(t)
Then, in def iptc(self), you can have:
if self._iptc is None:
self._load_iptc()
return self._iptc
This means you won't have _load_iptc in the same place as _load_exif, but I think that's acceptable.
Let me know if you're interested in making this change. This has been open for a long time so if you're not, we could also just merge it and I can handle it instead.
hugophotoswipe/photo.py
Outdated
| return str(self._get_tag_value(settings.tag_map.get('copyright'))) | ||
| return "" | ||
|
|
||
| def __getattribute__(self, attr): |
There was a problem hiding this comment.
I'm not a huge fan of overloading __getattribute__ like this, especially because you're catching an AttributeError and then looking in the settings map. What if in the future I unintentionally introduce a class variable that's also an exif tag name? I'm not entirely sure where you're using this, but perhaps a get_tag method can work instead? Happy to discuss this if you'd like.
There was a problem hiding this comment.
The reason for overloading __getattribute__ is so that you can access those tags from within the hugo shortcode. That would allow you to expand the shortcode with, for example, {{ .Get "aperture"}}. I couldn't figure out another way to make that data accessible when processing the shortcode, except for explicitly defining each as a property. I'm definitely open to other ideas.
I do understand your objection though and as it isn't quite immediately related to the exif/iptc feature I'm removing this from this branch until we can find out a better solution.
There was a problem hiding this comment.
Sorry for the slow response!
This makes sense, Here's one solution I can think of: require a prefix when used in a shortcode: {{ .Get "exit:ApertureValue" }} or {{ .Get "iptc:ReleaseTime" }}. Then you can overload the __getattribute__ method by first checking for the exif or iptc prefixes, and retrieving them using _get_tag_value. If they don't start with either prefix, return getattr(self, attr).
Also add test cases for improperly formatted tags.
Need a more robust solution to extract arbitrary attributes. For now, removing this from this branch.
iptcinfo spews many warnings about missing IPTC data, reading before image or decoding error. Most of the time this just seems to mean that the desired tags aren't present.
Note: Was not detected in test suite because all test images have exif['Orientation'] = None.
Note: Was not detected in test suite because all test images have exif['Orientation'] = None.
Updated version of PR #32.
Rebased on the updated version of HPS. I've added tests to cover the EXIF and IPTC additions.
Minor changes to the configuration tests are required as this PR introduces 3 potential new config file entries. These have been added to the config tests.
This feature allows: