-
Notifications
You must be signed in to change notification settings - Fork 34
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
Heic support and make tag #165
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.
I left a few inline comments.
picframe/get_image_meta.py
Outdated
@@ -91,6 +92,9 @@ def get_location(self): | |||
|
|||
def get_orientation(self): | |||
try: | |||
ext = os.path.splitext(self.__filename)[1].lower() | |||
if ext in ('.heif','.heic'): # converting heic to Image object will rotate pic implicitly. So orientation is always 1 | |||
return 1 |
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.
As I mentioned in the issue discussion, I'm not sure we should record "incorrect" orientation data just because (it'll currently) be fixed when the image is opened. Seems like it'd be better to continue to record "accurate" orientation data here. Then, we could just choose to not act on that data for HEIC images at load/display time.
So, store it correctly here, but essentially "ignore" it later for HEIC data.
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.
Might be better. One advantage is, that you don't have to rebuild the cache to display the heic images in correct orientation.
@@ -9,7 +9,7 @@ class ImageCache: | |||
|
|||
EXTENSIONS = ['.png','.jpg','.jpeg','.heif','.heic'] | |||
EXIF_TO_FIELD = {'EXIF FNumber': 'f_number', | |||
'EXIF Make': 'make', | |||
'Image Make': 'make', |
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 which one is correct here (haven't done any research). I will say that my db currently DOES contain some records where the current make
field is defined - so, the EXIF Make
property must be defined in some cases...
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 checked for Apple, Canon, Sony. I only find "Image Make"
DEBUG:exifread:IFD 0 (Image) at offset 8:
DEBUG:exifread: Make: (0x010F) ASCII=SONY @ 146
DEBUG:exifread: Model: (0x0110) ASCII=ILCE-7RM3 @ 152
Never "Make" in EXIF:
DEBUG:exifread:Exif SubIFD at offset 286:
DEBUG:exifread: ExposureTime: (0x829A) Ratio=3/10 @ 664
DEBUG:exifread: FNumber: (0x829D) Ratio=8 @ 672
Do you have an example image for me?
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.
See exifread: Note that the dictionary keys are the IFD name followed by the tag name. For example:
'EXIF DateTimeOriginal', 'Image Orientation', 'MakerNote FocusMode'
If you check exiftool tags description you will find the "Group" column:
- IFD0 = Image
- ExifIFD = EXIF
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.
Do you have an example image for me?
Here's a few examples. Looks like about 70% of my (frame) images have EXIF Make
instead of Image Make
. However, those images also have Exif Model
instead of Image Model
. So, with the current (mismatched) code, these have the db make
defined but not the model
.
Ultimately, we might have to be a bit more clever with some of the info and cascade through a number of tags looking for a definition...
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 example. Checking the standard I would say, that "Make" belongs to 0th IFD (Image) group. And is not defined for Exif IFD (Exif). So in my understanding your sample images are not correct. You are using Picasa, this Software is no longer maintained, isn't it?
What we can easily do is look first in the correct group and as fallback search in the other. Assuming we are focusing only on "Exif" and "Image" group.
exifread is returning everything anyway:
GetImageMeta__tags:{'Image Software': (0x0131) ASCII=Picasa @ 38, 'Image ExifOffset': (0x8769) Long=46 @ 30, 'Thumbnail Compression': (0x0103) Short=JPEG ...yle) @ 558, 'Thumbnail XResolution': (0x011A) Ratio=72 @ 626, 'Thumbnail YResolution': (0x011B) Ratio=72 @ 634, 'Thumbnail ResolutionUnit': (0x0128) Short=Pixel...Inch @ 594, 'Thumbnail JPEGInterc...angeFormat': (0x0201) Long=642 @ 606, 'Thumbnail JPEGInterc...rmatLength': (0x0202) Long=5620 @ 618, 'EXIF ImageWidth': (0x0100) Short=1911 @ 56, 'EXIF ImageLength': (0x0101) Long=2875 @ 68, 'EXIF BitsPerSample': (0x0102) Signed Short=16 @ 80, 'EXIF Make': (0x010F) ASCII=NIKON...TION @ 328, 'EXIF Model': (0x0110) ASCII=NIKON D70 @ 346, 'EXIF Orientation': (0x0112) Short=Horiz...mal) @ 116, ...}
But I wouldn't do this for other Groups like "Thumbnail"
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.
You are using Picasa, this Software is no longer maintained, isn't it?
I'm definitely not a Picasa user (and, yeah, I think it's been gone for a long time), though I did see it noted in the Image Software
tag in one of the samples. I assume Picasa (or some part of it) must still be in use somewhere in the Google image pipeline. I must have downloaded that particular image instance from my Google Photos account instead of locating the original copy locally - that's my guess anyway...
What we can easily do is look first in the correct group and as fallback search in the other. Assuming we are focusing only on "Exif" and "Image" group.
Agreed. That's exactly what I meant with my earlier statement...
Ultimately, we might have to be a bit more clever with some of the info and cascade through a number of tags looking for a definition...
But I wouldn't do this for other Groups like "Thumbnail"
Agreed.
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.
So in my understanding your sample images are not correct.
Yeah, I think I agree with you (although, the EXIF spec doc is not a fun read...)
I'll try to get to the bottom of what's creating my questionable EXIF data, although I suspect it's coming from "ON1 Photo Raw", which is my primary RAW converter and image editor. Although, that's surprising if so, as it's a widely used package with regular updates...
if im is None: | ||
return None | ||
if pics[0].orientation != 1: |
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.
If we wanted to shift the "heic orientation" issue to load/display time, it'd need to happen right here somewhere. Either we wouldn't call __orientate_image()
for HEIC images or we'd need to change that method to return the original image unchanged if it were passed a HEIC image (though, it'd need to know the image type, which it doesn't know today).
I store the original orientation now in the cache. I will prepare a new pull request |
else: | ||
val = self.__get_if_exist(key) | ||
|
||
if val is None: |
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.
This looks like it should work to cascade between the two groups, so maybe this is all we need for now. It might be nice (for future) to have something like get_exif_fuzzy
(or similar) that used a dictionary keyed by a generic tag name with the value being a list of prioritized group/tag names to search for a match. So, something like this, for example:
{'make': ['Image Make', 'EXIF Make'], 'model': ['Image Model', 'EXIF Model']}
This should fix #164