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

chore(server): update exiftool and migrate off deprecated method signatures #10367

Merged

Conversation

stephen304
Copy link
Contributor

@stephen304 stephen304 commented Jun 15, 2024

As a result of the previous exiftool largefilesupport fix and chatting with the exiftool-vendored maintainer, a new version of exiftool-vendored was released with a clearer API which removes the redundancy of exiftool methods accepting both an optionalArgs parameter as well as an options object that also includes optionalArgs. Instead, the updated interface is just the necessary args and an options object that accepts readArgs/writeArgs. This PR updates exiftool-vendored and our use of it to remove the deprecated forms while also moving largefilesupport flags into the constructor, since the addition of readArgs/writeArgs to the options object now allows it to be persistently set in the constructor for each operation instead of needing to pass args on every call.

  • chore(server): update exiftool-vendored to 27.0.0

  • chore(server): switch away from deprecated exiftool method signatures

    • options now includes read/writeArgs making the deprecated signatures with args array redundant
    • switch read call from file,args,options to file,options
    • switch write call from file,tags,args to file,tags,options
  • chore(server): move largefilesupport flags into exiftool constructor

    • options now includes read/writeArgs making it available to be set globally in constructor
    • switches back to instantiating an instance of exiftool

Note: I also removed the merging of DefaultReadTaskOptions into the read call options since it looks like the passed options object is already merged into the default options by exiftool-vendored (validated this with strace by checking that default flags are passed to the processes stdin).

I also added a bit of cleanup that I think should be safe based on my digging around in exiftool:

  • Since there don't seem to be any downsides to enabling largefilesupport always (as discussed in exiftool forums and how the first largefilesupport fix tried to work by passing the flag to the process when launching it), I think it makes sense to just add largefilesupport to writeArgs too in addition to reads, which also makes it clear to any future maintainers that there's no reason for it to be off
  • The writeTags sets -overwrite_original, which as I understand it disables the behavior of keeping an _original copy of modified files. Since immich doesn't have any provisions that I know of to handle _original file copies, I added it to the global writeArgs flags in the constructor.
    • Digging around the exiftool-vendored code it seems the only write tasks that use writeArgs is write itself and deleteAllTags, so I think setting -overwrite_original in writeArgs in the constructor is safe and consolidates exiftool config.
  • readTags sets some config options in its read call: defaultVideosToUTC, backfillTimezones inferTimezoneFromDatestamps, useMWG, an additional numericTags (+ FocalLength) as well as better geoTZ are also added. Since most of these are for reads only, it's safe to put them in the constructor config since they only affect the call that already had them set. Only useMWG would start to affect write if moved to the constructor, but this is already set for reads and the documentation notes that it's recommended to be on and that it "makes ExifTool.write write to "synonymous" MWG tags automatically.", which seems benign especially when any reading already reads using MWG.

Here is a comparison of what flags are sent to exiftool for "refresh metadata" and "write" (editing date to trigger writing an xmp file) from the current main branch and with this branch and all the cleanup - the only change is adding struct=1 (this version of exiftool now passes that from defaults to write where before only a couple specific default fields were picked) and adding MWG and largefilesupport:

@@ -1,29 +1,35 @@
 read(0, "-json
 -api
 largefilesupport=1
 -api
 struct=1
 -use
 MWG
 -*Duration*#
 -GPSAltitude#
 -GPSLatitude#
 -GPSLongitude#
 -GPSPosition#
 -Orientation#
 -FocalLength#
 -all
-/usr/src/app/upload/library/admin/2018/2018-06-14/00J0J_joaONoenBdV_1200x900_cwjpkf.jpg
+/usr/src/app/upload/library/admin/2018/2018-06-29/00J0J_joaONoenBdV_1200x900_cwjpkf.jpg
 -ignoreMinorErrors
 -execute
 ", 65536) = 272
 
 read(0, "-sep
 \37
 -E
--CreationDate=2018-06-16T20:30:00.000-04:00
+-api
+struct=1
+-use
+MWG
+-CreationDate=2018-06-19T20:30:00.000-04:00
+-api
+largefilesupport=1
 -overwrite_original
-/usr/src/app/upload/library/admin/2018/2018-06-15/00J0J_joaONoenBdV_1200x900_cwjpkf.jpg.xmp
+/usr/src/app/upload/library/admin/2018/2018-06-13/00J0J_joaONoenBdV_1200x900_cwjpkf.jpg.xmp
 -ignoreMinorErrors
 -execute
-", 65536) = 194
+", 65536) = 241

…atures

* chore(server): update exiftool-vendored to 27.0.0

* chore(server): switch away from deprecated exiftool method signatures
- options now includes read/writeArgs making the deprecated signatures with
  args array redundant
- switch read call from file,args,options to file,options
- switch write call from file,tags,args to file,tags,options

* chore(server): move largefilesupport flags into exiftool constructor
- options now includes read/writeArgs making it available to be set globally in
  constructor
- switches back to instantiating an instance of exiftool
@stephen304 stephen304 force-pushed the exiftool-deprecated-signatures branch from 085b1a8 to 46870d5 Compare June 15, 2024 20:24
@stephen304 stephen304 marked this pull request as ready for review June 16, 2024 00:56
server/src/repositories/metadata.repository.ts Outdated Show resolved Hide resolved
@stephen304
Copy link
Contributor Author

Moved the instantiation of exiftool into MetadataRepository constructor since I'm pretty sure that's what you meant

@alextran1502
Copy link
Contributor

Hello, can you go over what tests have been done for this PR? I am asking because EXIF extraction has many edge cases related to timezone and if it is the same on main vs this pr

@stephen304
Copy link
Contributor Author

stephen304 commented Jun 16, 2024

@alextran1502 I mainly tested by using strace to see what flags are passed to exiftool to make sure my changes didn't introduce any changed behavior that could matter. At the end of the PR description I included a diff to show that for metadata extraction exiftool is being called the same way (as it should since I simply moved the config options from the individual read call to the constructor) and the write call for writing xmp has the minor flag additions that you can see. For the write case I also tested editing the description, date, and location (I think that covers all the editable data that would be written to xmp) to make sure that those functions aren't impacted by the MWG or struct=1 flags (I couldn't figure out what the struct default is for exiftool but for exiftool-vendored it's supposed to be 1).

In short metadata extraction shouldn't have any changed behavior as a result of this cleanup, especially since all of the exiftool config options are the same, but if there are some other test files with timezone edge cases, I could test with those too in addition to the checking I've done with simple pictures from my phone.

@alextran1502 alextran1502 merged commit 1b67ea2 into immich-app:main Jun 17, 2024
22 checks passed
@stephen304 stephen304 deleted the exiftool-deprecated-signatures branch June 17, 2024 17:13
zackpollard added a commit that referenced this pull request Jul 16, 2024
zackpollard added a commit that referenced this pull request Jul 16, 2024
zackpollard added a commit that referenced this pull request Jul 16, 2024
* Revert "chore(server): update exiftool and migrate off deprecated method signatures (#10367)"

This reverts commit 1b67ea2

* fix: downgrade exiftool-vendored to 26.0.0

* chore: change motionphoto filenames to be kebab-case

* test: add pixel 6 pro motionphoto e2e test case

* test: add pixel 8a motion photo

* chore: update test-assets submodule pointer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants