immich icon indicating copy to clipboard operation
immich copied to clipboard

fix(server): use exiftool decoded values and unify metadata extraction

Open uhthomas opened this issue 2 years ago • 7 comments

There was a lot of unnecessary stuff happening which exiftool should have already taken care of. It's been replaced with the values provided by the exiftool package and the code is much cleaner as a result. Further, exiftool can be used to extract metadata from videos which greatly simplifies things.

Fixes: https://github.com/immich-app/immich/issues/2786, https://github.com/immich-app/immich/issues/2859, https://github.com/immich-app/immich/issues/2953

uhthomas avatar Jun 22 '23 00:06 uhthomas

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
immich ⬜️ Ignored (Inspect) Visit Preview Sep 14, 2023 2:03pm

vercel[bot] avatar Jun 22 '23 00:06 vercel[bot]

I've tested this and it works quite well, though I did notice that video files are missing their time zone. I've asked about this on their forum.

https://exiftool.org/forum/index.php?topic=14946.0

uhthomas avatar Jun 22 '23 00:06 uhthomas

I've tested this and it works quite well, though I did notice that video files are missing their time zone. I've asked about this on their forum.

https://exiftool.org/forum/index.php?topic=14946.0

Interestingly, OriginalCreateDateTime does actually return the timezone correctly. I'd like to avoid using this tag though as the package reports it as 0 stars, extremely uncommon and does not even provide an example...

uhthomas avatar Jun 22 '23 01:06 uhthomas

I've tested this and it works quite well, though I did notice that video files are missing their time zone. I've asked about this on their forum.

https://exiftool.org/forum/index.php?topic=14946.0

Perhaps this will fix https://github.com/immich-app/immich/issues/2786

evanphilip avatar Jun 29 '23 18:06 evanphilip

I've tested this and it works quite well, though I did notice that video files are missing their time zone. I've asked about this on their forum. https://exiftool.org/forum/index.php?topic=14946.0

Perhaps this will fix #2786

I believe it will.

uhthomas avatar Jun 30 '23 00:06 uhthomas

Can you help fix the server test? Thank you

alextran1502 avatar Jul 01 '23 04:07 alextran1502

This issue (https://github.com/immich-app/immich/issues/3287) should be discussed before merging in the changes in this PR that removes the getExifProperty function and assuming exiftool returns all valid data without us doing any sort of checking.

alex-phillips avatar Jul 15 '23 17:07 alex-phillips

This issue (#3287) should be discussed before merging in the changes in this PR that removes the getExifProperty function and assuming exiftool returns all valid data without us doing any sort of checking.

It would be good to investigate and I hope this PR can solve this, but it should not be a blocker given the current implementation doesn't catch this either.

uhthomas avatar Jul 15 '23 23:07 uhthomas

This issue (#3287) should be discussed before merging in the changes in this PR that removes the getExifProperty function and assuming exiftool returns all valid data without us doing any sort of checking.

It would be good to investigate and I hope this PR can solve this, but it should not be a blocker given the current implementation doesn't catch this either.

That's true. And this does quite a bit more. The primary logic for the tag values is still centralized into a single function call, so any specific handling that may be done in the future should still be fairly contained.

alex-phillips avatar Jul 16 '23 00:07 alex-phillips

Hey all, any updates on this PR?

aeozyalcin avatar Aug 21 '23 03:08 aeozyalcin

This issue (#3287) should be discussed before merging in the changes in this PR that removes the getExifProperty function and assuming exiftool returns all valid data without us doing any sort of checking.

It would be good to investigate and I hope this PR can solve this, but it should not be a blocker given the current implementation doesn't catch this either.

That's true. And this does quite a bit more. The primary logic for the tag values is still centralized into a single function call, so any specific handling that may be done in the future should still be fairly contained.

This PR does streamline a few things, especially video exif parsing. I think that it is OK to merge it and incorporate groups afterwards. You are probably right in that we'll end up with another version of getExifValue(), but that should be easy enough to add back.

jrasm91 avatar Sep 14 '23 14:09 jrasm91