Skip to content
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

Enhance EXIF handling #6641

Closed
4 of 5 tasks
cobi-s opened this issue Oct 5, 2022 · 16 comments · Fixed by #6749
Closed
4 of 5 tasks

Enhance EXIF handling #6641

cobi-s opened this issue Oct 5, 2022 · 16 comments · Fixed by #6749
Labels

Comments

@cobi-s
Copy link

cobi-s commented Oct 5, 2022

The getexif() method of PIL.Image could use some enhancements, which most likely require an API bump. I'm just listing some ideas/shortcomings of the current implementation here, which I would love to see addressed (and could possibly contribute to, if the changes are agreed upon).

  • The return type of getexif() is Exif. But without further processing, it's really only IFD0. However, already returning an IFD makes the get_ifd() method look out of place. Therefore, I recommend to implement the Exif class as a dict linking to all extracted IFDs.
  • Either providing specialized methods for common IFDs (e.g. Exif.get_ifd0()) or named constants for common IFDs (e.g. Exif.get_ifd(ExifTags.IFD.IFD0)) would improve the usage of the API a lot. (Added IFD enum to ExifTags #6748)
  • Implement tag names for missing IFDs (especially IFD1 and Interop) (Added Interop to ExifTags #6724)
  • Some tags are purely technical and should never be edited by hand (the IFD offsets come to mind here). It would be nice to have a flag not to return them to the user. (Added Exif hide_offsets() #6762)
  • Some integer tag values encode a human readable description, for example Flash and LightSource. It would be great to provide named constants for those as well as methods to return the description as a string. (Added LightSource tag values to ExifTags #6749 for LightSource)

I can't say anything about the feasability regarding licencing, but maybe the tag names could be scraped from the Exiftool website (the quasi reference implementation of the Exif format): https://exiftool.org/TagNames/EXIF.html

There are already other python libraries for Exif handling, but they all suffer from fragmentation. As the Pillow library already does the heavy lifting of extracting the IFDs anyway, I think providing a convenient and nearly complete view of the data can be a significant improvement to the project.

@radarhere radarhere added the Exif label Oct 5, 2022
@radarhere
Copy link
Member

The return type of getexif() is Exif. But without further processing, it's really only IFD0. However, already returning an IFD makes the get_ifd() method look out of place. Therefore, I recommend to implement the Exif class as a dict linking to all extracted IFDs.

Isn't this just part of the unfortunate reality that https://exiftool.org/TagNames/EXIF.html lists EXIF tags, and one of the groups listed is an ExifIFD? I would have thought the names were inherently confusing.

Either providing specialized methods for common IFDs (e.g. Exif.get_ifd0()) or named constants for common IFDs (e.g. Exif.get_ifd(ExifTags.IFD.IFD0)) would improve the usage of the API a lot.

I'm not opposed to the idea of named constants. I'd want to see what happens with #6630 first.

Implement tag names for missing IFDs (especially IFD1 and Interop)

You mean adding more entries to ExifTags? Again, not opposed, just would want to see what happens with #6630.

Some tags are purely technical and should never be edited by hand (the IFD offsets come to mind here). It would be nice to have a flag not to return them to the user.

I find this a bit vague. Some more detail on what other tags you're thinking of could be helpful.

Some integer tag values encode a human readable description, for example Flash and LightSource. It would be great to provide named constants for those as well as methods to return the description as a string.

#6630 again. Rather than methods to return the description as a string, I wonder if returning IntEnums as the values would be a solution? That would allow the names to be inspected without having to add new arguments or methods.

@cobi-s
Copy link
Author

cobi-s commented Oct 5, 2022

Isn't this just part of the unfortunate reality that https://exiftool.org/TagNames/EXIF.html lists EXIF tags, and one of the groups listed is an ExifIFD? I would have thought the names were inherently confusing.

I don't think so. I might be totally wrong here, though. As far as I understand, the Exif segment (which is returned by Image.info.get('exif') is basically a linked list of IFDs. First one is IFD0, which contains the information that getexif() returns. This IFD links to the next one (which is called ExifIFD by ExifTool) which contains most of the camera information. Then on the the next IFD (Interop) and so on. Yes, the names are a bit confusing. What I was trying to say is that getexif() returns an actual IFD, but is semantically closer to a handle to the whole EXIF information segment of the image.

You mean adding more entries to ExifTags? Again, not opposed, just would want to see what happens with #6630.

Yes. I just rechecked. GPSTags, which can only occur in the GPSInfo IFD, can have "duplicate" tag numbers. For all other tags (maybe excluding MakerNotes, not so sure about them), including for example the Interop tags, this does not seem to be the case and Exiftool itself puts all the tag numbers in one big list. So it should be safe for Pillow to just extend the ExifTags dict (or enum in the future) as well.

I find this a bit vague. Some more detail on what other tags you're thinking of could be helpful.

That should only be the offsets to the next IFD (tag 0xa005 InteropOffset, tag 0x8825 GPSInfo, and a few others). It's not very important though. When I have to write code for "Give me all the information about this image" then the GPS long/lat are important, but it's not really important (obviously depending on the use case) at which byte the GPS Information starts. We extract the information anyway to parse the whole Exif segment, but it would be a convenience function to filter out the offsets.

#6630 again. Rather than methods to return the description as a string, I wonder if returning IntEnums as the values would be a solution? That would allow the names to be inspected without having to add new arguments or methods.

#6630 should cover at least 99%, yes. One could argue that "Light Source" is a nicer string representation than what we get from Enum.name "LightSource" (without the space). However, that definitely needn't be a priority.

But additionally, think for example of tag 0x9208 LightSource. Currently, we could only retrieve an integer as the value of this tag. But for example "1" is not very descriptive. "Daylight" (for which the 1 stands) is much nicer. This should also apply for setting this tag value. Currently, we would have to do something similar to exif_tags[0x9208] = 1. It would be great to be able to write exif_tags[TAGS.LightSource] = Exif.LightSource.Daylight

@bigcat88
Copy link
Contributor

What I was trying to say is that getexif() returns an actual IFD, but is semantically closer to a handle to the whole EXIF information segment of the image.

yep, many people get confused by this and use _getexif cause it give more info instead public method.

@radarhere
Copy link
Member

I've created #6724 to add Interop tags.
It's not clear to me what IFD1 should involve, since "IFD1 tags are not listed separately".
I don't know what other missing IFDs you were thinking of.

@olmerg
Copy link

olmerg commented Nov 18, 2022

maybe it can help, I have the problem that the examples to obtain the gpsinfo with getexif() is not working in the last version of pillow, because gpsInfo is a number like is described in this question , the only way to obtain the gpsinfo was using _getexif

@cobi-s
Copy link
Author

cobi-s commented Nov 18, 2022

It's not clear to me what IFD1 should involve, since "IFD1 tags are not listed separately".

Correct. I should check if tag names, that are allowed to appear in IFD0 and IFD1, are still missing. But that would be a minor addition.

the only way to obtain the gpsinfo was using _getexif

@olmerg You can do it by using getexif().get_ifd(0x8825).
That is inconvenient. And I haven't been able to find the IFD tag number for IFD1 and for the raw thumbnail data.

@radarhere
Copy link
Member

@olmerg if you still think that you can't get the GPS info from an image, open a new issue with more details and a copy of the image.

I've created PR #6748 to add "named constants for common IFDs". If that is merged, you would be able to use get_ifd(ExifTags.IFD.GPSInfo)


I've also created PR #6749 to add a "LightSource" enum. "Flash" was also mentioned, but https://www.awaresystems.be/imaging/tiff/tifftags/privateifd/exif/flash.html lists

hex 004D = Flash fired, compulsory flash mode, red-eye reduction mode, return light not detected

as a possible value. I don't think that translates well to an enum.

@radarhere
Copy link
Member

The return type of getexif() is Exif. But without further processing, it's really only IFD0. However, already returning an IFD makes the get_ifd() method look out of place. Therefore, I recommend to implement the Exif class as a dict linking to all extracted IFDs.

As far as I understand, the Exif segment (which is returned by Image.info.get('exif') is basically a linked list of IFDs. First one is IFD0, which contains the information that getexif() returns. This IFD links to the next one (which is called ExifIFD by ExifTool) which contains most of the camera information. Then on the the next IFD (Interop) and so on.

We're agreed that IFD0 comes first, and that the other IFDs are linked from that. That is the reality of the data, and I think Pillow is reflecting that. As an aside, Pillow used to flatten the other IFDs in, but that meant you couldn't tell which tags were from IFD0 and which were from another IFD.

You're instead suggesting that the IFDs be thought of as being 1-dimensional instead, because automatically loading IFD0 might be unintuitive, and trying all IFDs in the same way might be a simpler way to think about them.

I'm personally more inclined to continue to reflect the fact that one IFD links to another, and loading IFD0 automatically seems helpful, rather than forcing the user to specifically choose it.

Regardless of the preferred way of thinking about this is though, Pillow values backwards compatibility. I don't think there's a way to implement this without breaking that. This change wouldn't allow for any additional functionality or fix any bugs, it's just trying to improve the clarity of the API. So I don't think the benefits outweigh the costs.

@radarhere
Copy link
Member

Some tags are purely technical and should never be edited by hand (the IFD offsets come to mind here). It would be nice to have a flag not to return them to the user.

I've created PR #6762 for this.

@bigcat88
Copy link
Contributor

bigcat88 commented Nov 26, 2022

I don't know what other missing IFDs you were thinking of.

There is a getexif() and _getexif()

Now only three: Webp, Png and Jpeg file formats support _getexif method.

In all three the code almost the same:

    def _getexif(self):
        if "exif" not in self.info:
            return None
        return self.getexif()._get_merged_dict()

Proposal: add optional parameter(merged=False) to getexif to return merged dictionary, and in future remove _getexif.

Asking, cause i do not know, need I implement this for my heif plugin or not. I do not want to implement protected method, to not angry linters, and do not want to introduce custom flags that will be used only in one type of images.

Or are there other plans to get full Exif data instead of _getexif?

@olmerg
Copy link

olmerg commented Nov 27, 2022

@radarhere ,I got the GPS information but it was so difficult to find how, the examples that I find in the internet did not work. The only way that I find was to access the private data of the class. I think will be great your proposal about a more easy way to get that type of data.

@cobi-s
Copy link
Author

cobi-s commented Nov 27, 2022

We're agreed that IFD0 comes first, and that the other IFDs are linked from that. That is the reality of the data, and I think Pillow is reflecting that.

Close, but I would still disagree partly here. Pillow provides get_ifd() which "jumps over" any linked structure. And funnily enough, I can jump from IFD0 into IFD0. I'd argue that the raw data is not strictly a list of linked IFDs, because the offsets to the IFDs are found in different places (eg. the GPSInfo tag). And do users really want to traverse a linked list? Or wouldn't they prefer a function that returns them a dict, the IFDs as keys and their respective tags as values?

Proposal: add optional parameter(merged=False) to getexif to return merged dictionary

I disagree. I can't imagine a scenario where I would want a merged dict, because I can never be sure that the same tag was not stored in multiple IFDs (one example would be XResolution, which can be present in IFD0 for the original image and in IFD1 for the embedded thumbnail).

I am still not able to use (only) Pillow to extract the embedded thumbnail and its metadata in IFD1, because I have no way of extracting the offset to IFD1 for use in get_ifd(). Returning a dict with all IFDs in the first place (call it getexif2() to keep backwards compatability) would solve that very easily.

@radarhere
Copy link
Member

Asking, cause i do not know, need I implement this for my heif plugin or not. I do not want to implement protected method, to not angry linters, and do not want to introduce custom flags that will be used only in one type of images.

@bigcat88 you're not currently using _getexif in https://github.com/bigcat88/pillow_heif, so I can't tell exactly what the problem is. I would hope that you can get any information that you need from getexif. If you can't get some information, it would be good if you could open a new issue about it.

@radarhere
Copy link
Member

I am still not able to use (only) Pillow to extract the embedded thumbnail and its metadata in IFD1, because I have no way of extracting the offset to IFD1 for use in get_ifd(). Returning a dict with all IFDs in the first place (call it getexif2() to keep backwards compatibility) would solve that very easily.

At the moment, even if I created a PR with a getexif2() method, I would just be creating a new way of getting the same information. I don't know what you're thinking of when you say that getexif2() would easily allow you to access IFD1.

Would you mind if I tried to solve this problem with the current API? Could you open a new issue about this, attaching a copy of an image and describing what you're trying to extract? What specification are you referring to?

@cobi-s
Copy link
Author

cobi-s commented Dec 4, 2022

@radarhere Thanks for the offer. Please see the linked issue #6777

@radarhere
Copy link
Member

#6777 has now been resolved by #6748. It added im.getexif().get_ifd(ExifTags.IFD.IFD1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants