immich
immich copied to clipboard
fix(server): use exiftool decoded values and unify metadata extraction
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
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 |
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
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...
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
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.
Can you help fix the server test? Thank you
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.
This issue (#3287) should be discussed before merging in the changes in this PR that removes the
getExifPropertyfunction 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.
This issue (#3287) should be discussed before merging in the changes in this PR that removes the
getExifPropertyfunction 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.
Hey all, any updates on this PR?
This issue (#3287) should be discussed before merging in the changes in this PR that removes the
getExifPropertyfunction 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.