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

refactor(server): use exiftool for file date metadata #16453

Open
wants to merge 4 commits into
base: refactor/server-metadata-extraction-group-async-calls
Choose a base branch
from

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Mar 1, 2025

Description

There are two redundant stat calls in metadata extraction that can instead use the exiftool output (tags starting with File 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.

@mertalev mertalev requested a review from danieldietzler as a code owner March 1, 2025 11:52
@mertalev mertalev force-pushed the refactor/server-remove-metadata-extraction-stat-calls branch from b3615aa to f394c7d Compare March 1, 2025 11:54
@mertalev mertalev force-pushed the refactor/server-remove-metadata-extraction-stat-calls branch from ba670be to 0f8e67f Compare March 1, 2025 13:00
@@ -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 () => {
Copy link
Contributor

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?

@zackpollard zackpollard requested a review from Copilot March 3, 2025 12:48
@zackpollard
Copy link
Contributor

Throwing copilot reviewer at this as a test

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'],
@mertalev mertalev force-pushed the refactor/server-metadata-extraction-group-async-calls branch from ecdb914 to 3cbd12e Compare March 3, 2025 20:22
@mertalev mertalev force-pushed the refactor/server-remove-metadata-extraction-stat-calls branch from ef79514 to 5c16c36 Compare March 3, 2025 21:18
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.

4 participants