karlch / vimiv-qt

An image viewer with vim-like keybindings
https://karlch.github.io/vimiv-qt/
GNU General Public License v3.0
178 stars 14 forks source link

Proper exif metadata formatting #214

Closed karlch closed 3 years ago

karlch commented 4 years ago

During #212 we came to the conclusion that proper formatting of the exif metadata is non-trivial. Piexif seems to have no option to give nice correctly formatted information, only the raw data.

We have a few options here:

Any ideas and suggestions are very welcome!

karlch commented 3 years ago

Having some simple proof-of-concept and at least the values of the default keys in the different metadata key-sets properly format before releasing v0.8.0 would make a lot of sense to me. I will probably take a shot at implementing this directly in vimiv and ask you @jcjgraf for some help once the basics are there. Does this sound like a plan to you?

jcjgraf commented 3 years ago

I think it definitely makes sense to get the formatting right for the next release.

Doing this formatting ourselves, based on the raw data provided by piexif seems a lot of work do me. While it may be reasonable to to it for the most frequently used metadata, it still very tedious. Especially, since the the formatter has to be extended whenever someone needs a new certain new metadata tag.

So my question is, does it really make sense to urgently stick to piexif instead of switching to a more elaborated package? exiftool as well as exiv2 are both amazing tools. While they are not written in python themselves, there exist python wrappers for both.

Obviously both provide formatted metadata output by default (besides many other advantages).

Extracting metadata using PyExifTool (wrapper for exiftool) is quite simple

import exiftool

image = "test.CR2"

with exiftool.ExifTool() as et:
    metadata = et.get_metadata(image)

    print(metadata)

metadata is in this case a dict with all metadata found in the image. The metadata tag is the key and formatted metadata is the value.

In order to extract only certain tags, one can use et.get_tags(["firstTag, secondTag"], image). which returns a similarly structured dict.

While exiv2 seems a bit less powerful that exiftool, its wrapper py3exiv2 works similarly to the example above.

In my view, switching to such an elaborated package is a way cleaner solution that bloading vimiv with formatting code. Also, piexifs documentation does not even state what image types are supported (CR2 certainly works, but I have no clue what other formats are supported). Also, the mentioned tools keep receiving updates for newly released cameras/lenses etc.

Concerning possible disadvantages, the only ones I can imagine is heaving two dependencies instead of one (the actual tool + the wrapper), and a larger size of the dependencies (12MB for exiftool and 9MB for exiv2).

Anyways, let me know what you think about it.

And whatever solution we decide on, I propose to into this issue. This way you can deal with #293 and everything else required for v0.8.0.

karlch commented 3 years ago

To me this currently seems like a lose-lose situation. Let me summarize the pros and cons of the different tools in my opinion. Disclaimer: I haven't used any of them extensively.

piexif

PyExifTool

pyexiv2

Just from this I would rule out using PyExifTool. This leaves us with piexif and pyexiv2.

If you feel like the benefits from using a wrapper for exiv2 are large, feel free to go that route in a PR. We would still need to support piexif for at least one release with a deprecation warning to allow users to prepare for a transition though. For this, it might make sense to decouple the usage even more from the underlying library in an additional module, i.e. imutils.exif_wrapper. This will most certainly be quite an ugly effort, especially with our lazy loading and the optional features related to exif.

jcjgraf commented 3 years ago

Now, after #311 has been merged, this issue can be closed I guess. Is there anything else which needs to be done for v0.8.0?

karlch commented 3 years ago

Yes, makes sense! I have started working on #220 as it makes a lot of sense to have this with the new metadata flexibility, I think. I will probably ask you to take a quick look before merging, but this should be straightforward compared to #311. After this, v0.8.0 should be good to go :tada: