-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
refactor(server): use exiftool for file date metadata #16453
base: refactor/server-metadata-extraction-group-async-calls
Are you sure you want to change the base?
refactor(server): use exiftool for file date metadata #16453
Conversation
b3615aa
to
f394c7d
Compare
ba670be
to
0f8e67f
Compare
@@ -149,15 +155,19 @@ describe(MetadataService.name, () => { | |||
id: assetStub.image.id, | |||
duration: null, | |||
fileCreatedAt: fileModifiedAt, | |||
fileModifiedAt, | |||
localDateTime: fileModifiedAt, | |||
}); | |||
}); | |||
|
|||
it('should take the file creation date when missing exif and earliest than modification date', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not from your PR but would you mind changing all these "earliest" to "earlier" while you're here?
Throwing copilot reviewer at this as a test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR refactors the metadata extraction logic to leverage exiftool for file date metadata while reducing redundant stat calls. Key changes include:
- Removing the parallel stat call in favor of using exiftool output and falling back to stat only when file date tags are missing.
- Updating test cases to include new file date fields (FileCreateDate and FileModifyDate) along with changes to asset update expectations.
- Refactoring the motion photo extraction flow to pass file size as a parameter rather than querying file stats.
Reviewed Changes
File | Description |
---|---|
server/test/medium/specs/metadata.service.spec.ts | Adjusted test mocks to include file creation and modification dates |
server/src/services/metadata.service.spec.ts | Updated tests to align with the new exif data requirements and asset update expectations |
server/src/services/metadata.service.ts | Refactored metadata extraction logic to first use exiftool and fallback to stat, and updated motion photo extraction to use a passed fileSize |
server/src/repositories/metadata.repository.ts | Added 'FileSize' to numericTags to ensure proper type conversion in exiftool output |
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
server/src/services/metadata.service.ts:175
- The fallback block updates exifTags with file dates from stat, but the asset object's fileCreatedAt/fileModifiedAt properties are no longer updated directly. Consider whether the asset should be updated from the fallback values to avoid potential inconsistency later in the processing flow.
if (!exifTags.FileCreateDate || !exifTags.FileModifyDate) {
server/src/services/metadata.service.ts:430
- Since video extraction now uses the fileSize parameter for calculating the extraction position, please ensure that exifData.fileSizeInByte is always a valid number and accurately reflects the file's size.
private async applyMotionPhotos(asset: AssetEntity, tags: ImmichTags, fileSize: number) {
server/src/repositories/metadata.repository.ts:76
- Adding 'FileSize' to numericTags is important; please verify that the exiftool output always returns FileSize in a format that can be reliably parsed as a number to prevent potential NaN issues.
numericTags: [...DefaultReadTaskOptions.numericTags, 'FocalLength', 'FileSize'],
ecdb914
to
3cbd12e
Compare
ef79514
to
5c16c36
Compare
Description
There are two redundant
stat
calls in metadata extraction that can instead use the exiftool output (tags starting withFile
are from the file system). Besides generally less file IO, it avoids taking up a thread (or waiting to acquire one) in the all-too-limited libuv threadpool. Based on #16450, which should be merged first.I tested locally and it works as expected. However, there are cases in the library E2E tests where the tags are undefined. I don't know why this is. The PR falls back to calling
.stat
in this case to handle this.This is the last of the PRs split off from #14277.