From bc61497461c7a41fd6655c0fcba0d8de12f7c765 Mon Sep 17 00:00:00 2001 From: Mert <101130780+mertalev@users.noreply.github.com> Date: Thu, 6 Mar 2025 09:56:35 -0500 Subject: [PATCH] refactor(server): group async calls in metadata extraction (#16450) * group async calls use debugFn no need to change mock * check call count in tests --- server/src/services/metadata.service.spec.ts | 16 +- server/src/services/metadata.service.ts | 181 +++++++++++-------- 2 files changed, 116 insertions(+), 81 deletions(-) diff --git a/server/src/services/metadata.service.spec.ts b/server/src/services/metadata.service.spec.ts index e8ffd4a04f..765c6bb936 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -454,6 +454,7 @@ describe(MetadataService.name, () => { mocks.asset.getByIds.mockResolvedValue([{ ...assetStub.livePhotoWithOriginalFileName, livePhotoVideoId: null }]); mockReadTags({ Directory: 'foo/bar/', + MotionPhoto: 1, MotionPhotoVideo: new BinaryField(0, ''), // The below two are included to ensure that the MotionPhotoVideo tag is extracted // instead of the EmbeddedVideoFile, since HEIC MotionPhotos include both @@ -491,10 +492,11 @@ describe(MetadataService.name, () => { }); expect(mocks.user.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512); expect(mocks.storage.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); - expect(mocks.asset.update).toHaveBeenNthCalledWith(1, { + expect(mocks.asset.update).toHaveBeenCalledWith({ id: assetStub.livePhotoWithOriginalFileName.id, livePhotoVideoId: fileStub.livePhotoMotion.uuid, }); + expect(mocks.asset.update).toHaveBeenCalledTimes(2); }); it('should extract the EmbeddedVideo tag from Samsung JPEG motion photos', async () => { @@ -503,6 +505,7 @@ describe(MetadataService.name, () => { Directory: 'foo/bar/', EmbeddedVideoFile: new BinaryField(0, ''), EmbeddedVideoType: 'MotionPhoto_Data', + MotionPhoto: 1, }); mocks.crypto.hashSha1.mockReturnValue(randomBytes(512)); mocks.asset.create.mockResolvedValue(assetStub.livePhotoMotionAsset); @@ -535,10 +538,11 @@ describe(MetadataService.name, () => { }); expect(mocks.user.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512); expect(mocks.storage.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); - expect(mocks.asset.update).toHaveBeenNthCalledWith(1, { + expect(mocks.asset.update).toHaveBeenCalledWith({ id: assetStub.livePhotoWithOriginalFileName.id, livePhotoVideoId: fileStub.livePhotoMotion.uuid, }); + expect(mocks.asset.update).toHaveBeenCalledTimes(2); }); it('should extract the motion photo video from the XMP directory entry ', async () => { @@ -580,10 +584,11 @@ describe(MetadataService.name, () => { }); expect(mocks.user.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512); expect(mocks.storage.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); - expect(mocks.asset.update).toHaveBeenNthCalledWith(1, { + expect(mocks.asset.update).toHaveBeenCalledWith({ id: assetStub.livePhotoWithOriginalFileName.id, livePhotoVideoId: fileStub.livePhotoMotion.uuid, }); + expect(mocks.asset.update).toHaveBeenCalledTimes(2); }); it('should delete old motion photo video assets if they do not match what is extracted', async () => { @@ -648,14 +653,15 @@ describe(MetadataService.name, () => { mocks.storage.readFile.mockResolvedValue(video); await sut.handleMetadataExtraction({ id: assetStub.livePhotoStillAsset.id }); - expect(mocks.asset.update).toHaveBeenNthCalledWith(1, { + expect(mocks.asset.update).toHaveBeenCalledWith({ id: assetStub.livePhotoMotionAsset.id, isVisible: false, }); - expect(mocks.asset.update).toHaveBeenNthCalledWith(2, { + expect(mocks.asset.update).toHaveBeenCalledWith({ id: assetStub.livePhotoStillAsset.id, livePhotoVideoId: assetStub.livePhotoMotionAsset.id, }); + expect(mocks.asset.update).toHaveBeenCalledTimes(3); }); it('should not update storage usage if motion photo is external', async () => { diff --git a/server/src/services/metadata.service.ts b/server/src/services/metadata.service.ts index 14513c738a..46add68bae 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -6,7 +6,6 @@ import _ from 'lodash'; import { Duration } from 'luxon'; import { constants } from 'node:fs/promises'; import path from 'node:path'; -import { SystemConfig } from 'src/config'; import { JOBS_ASSET_PAGINATION_SIZE } from 'src/constants'; import { StorageCore } from 'src/cores/storage.core'; import { Exif } from 'src/db'; @@ -76,6 +75,8 @@ const validateRange = (value: number | undefined, min: number, max: number): Non return val; }; +type ImmichTagsWithFaces = ImmichTags & { RegionInfo: NonNullable }; + @Injectable() export class MetadataService extends BaseService { @OnEvent({ name: 'app.bootstrap', workers: [ImmichWorker.MICROSERVICES] }) @@ -161,15 +162,19 @@ export class MetadataService extends BaseService { @OnJob({ name: JobName.METADATA_EXTRACTION, queue: QueueName.METADATA_EXTRACTION }) async handleMetadataExtraction(data: JobOf): Promise { - const { metadata, reverseGeocoding } = await this.getConfig({ withCache: true }); - const [asset] = await this.assetRepository.getByIds([data.id], { faces: { person: false } }); + const [{ metadata, reverseGeocoding }, [asset]] = await Promise.all([ + this.getConfig({ withCache: true }), + this.assetRepository.getByIds([data.id], { faces: { person: false } }), + ]); + if (!asset) { return JobStatus.FAILED; } - const stats = await this.storageRepository.stat(asset.originalPath); - - const exifTags = await this.getExifTags(asset); + const [stats, exifTags] = await Promise.all([ + this.storageRepository.stat(asset.originalPath), + this.getExifTags(asset), + ]); this.logger.verbose('Exif Tags', exifTags); @@ -182,9 +187,18 @@ export class MetadataService extends BaseService { } const { dateTimeOriginal, localDateTime, timeZone, modifyDate } = this.getDates(asset, exifTags); - const { latitude, longitude, country, state, city } = await this.getGeo(exifTags, reverseGeocoding); const { width, height } = this.getImageDimensions(exifTags); + let geo: ReverseGeocodeResult, latitude: number | null, longitude: number | null; + if (reverseGeocoding.enabled && this.hasGeo(exifTags)) { + latitude = exifTags.GPSLatitude; + longitude = exifTags.GPSLongitude; + geo = await this.mapRepository.reverseGeocode({ latitude, longitude }); + } else { + latitude = null; + longitude = null; + geo = { country: null, state: null, city: null }; + } const exifData: Insertable = { assetId: asset.id, @@ -197,9 +211,9 @@ export class MetadataService extends BaseService { // gps latitude, longitude, - country, - state, - city, + country: geo.country, + state: geo.state, + city: geo.city, // image/file fileSizeInByte: stats.size, @@ -230,25 +244,29 @@ export class MetadataService extends BaseService { autoStackId: this.getAutoStackId(exifTags), }; - await this.applyTagList(asset, exifTags); - await this.applyMotionPhotos(asset, exifTags); + const promises: Promise[] = [ + this.assetRepository.upsertExif(exifData), + this.assetRepository.update({ + id: asset.id, + duration: exifTags.Duration?.toString() ?? null, + localDateTime, + fileCreatedAt: exifData.dateTimeOriginal ?? undefined, + fileModifiedAt: stats.mtime, + }), + this.applyTagList(asset, exifTags), + ]; - await this.assetRepository.upsertExif(exifData); - - await this.assetRepository.update({ - id: asset.id, - duration: exifTags.Duration?.toString() ?? null, - localDateTime, - fileCreatedAt: exifData.dateTimeOriginal ?? undefined, - fileModifiedAt: stats.mtime, - }); - - if (exifData.livePhotoCID) { - await this.linkLivePhotos(asset, exifData); + if (this.isMotionPhoto(asset, exifTags)) { + promises.push(this.applyMotionPhotos(asset, exifTags)); } - if (isFaceImportEnabled(metadata)) { - await this.applyTaggedFaces(asset, exifTags); + if (isFaceImportEnabled(metadata) && this.hasTaggedFaces(exifTags)) { + promises.push(this.applyTaggedFaces(asset, exifTags)); + } + + await Promise.all(promises); + if (exifData.livePhotoCID) { + await this.linkLivePhotos(asset, exifData); } await this.assetRepository.upsertJobStatus({ assetId: asset.id, metadataExtractedAt: new Date() }); @@ -347,48 +365,66 @@ export class MetadataService extends BaseService { return { width, height }; } - private async getExifTags(asset: AssetEntity): Promise { - const mediaTags = await this.metadataRepository.readTags(asset.originalPath); - const sidecarTags = asset.sidecarPath ? await this.metadataRepository.readTags(asset.sidecarPath) : {}; - const videoTags = asset.type === AssetType.VIDEO ? await this.getVideoTags(asset.originalPath) : {}; + private getExifTags(asset: AssetEntity): Promise { + if (!asset.sidecarPath && asset.type === AssetType.IMAGE) { + return this.metadataRepository.readTags(asset.originalPath); + } + + return this.mergeExifTags(asset); + } + + private async mergeExifTags(asset: AssetEntity): Promise { + const [mediaTags, sidecarTags, videoTags] = await Promise.all([ + this.metadataRepository.readTags(asset.originalPath), + asset.sidecarPath ? this.metadataRepository.readTags(asset.sidecarPath) : null, + asset.type === AssetType.VIDEO ? this.getVideoTags(asset.originalPath) : null, + ]); // prefer dates from sidecar tags - const sidecarDate = firstDateTime(sidecarTags as Tags, EXIF_DATE_TAGS); - if (sidecarDate) { - for (const tag of EXIF_DATE_TAGS) { - delete mediaTags[tag]; + if (sidecarTags) { + const sidecarDate = firstDateTime(sidecarTags as Tags, EXIF_DATE_TAGS); + if (sidecarDate) { + for (const tag of EXIF_DATE_TAGS) { + delete mediaTags[tag]; + } } } // prefer duration from video tags delete mediaTags.Duration; - delete sidecarTags.Duration; + delete sidecarTags?.Duration; return { ...mediaTags, ...videoTags, ...sidecarTags }; } - private async applyTagList(asset: AssetEntity, exifTags: ImmichTags) { - const tags: string[] = []; + private getTagList(exifTags: ImmichTags): string[] { + let tags: string[]; if (exifTags.TagsList) { - tags.push(...exifTags.TagsList.map(String)); + tags = exifTags.TagsList.map(String); } else if (exifTags.HierarchicalSubject) { - tags.push( - ...exifTags.HierarchicalSubject.map((tag) => - String(tag) - // convert | to / - .replaceAll('/', '') - .replaceAll('|', '/') - .replaceAll('', '|'), - ), + tags = exifTags.HierarchicalSubject.map((tag) => + // convert | to / + typeof tag === 'number' + ? String(tag) + : tag + .split('|') + .map((tag) => tag.replaceAll('/', '|')) + .join('/'), ); } else if (exifTags.Keywords) { let keywords = exifTags.Keywords; if (!Array.isArray(keywords)) { keywords = [keywords]; } - tags.push(...keywords.map(String)); + tags = keywords.map(String); + } else { + tags = []; } + return tags; + } + private async applyTagList(asset: AssetEntity, exifTags: ImmichTags) { + const tags = this.getTagList(exifTags); const results = await upsertTags(this.tagRepository, { userId: asset.ownerId, tags }); await this.tagRepository.replaceAssetTags( asset.id, @@ -396,11 +432,11 @@ export class MetadataService extends BaseService { ); } - private async applyMotionPhotos(asset: AssetEntity, tags: ImmichTags) { - if (asset.type !== AssetType.IMAGE) { - return; - } + private isMotionPhoto(asset: AssetEntity, tags: ImmichTags): boolean { + return asset.type === AssetType.IMAGE && !!(tags.MotionPhoto || tags.MicroVideo); + } + private async applyMotionPhotos(asset: AssetEntity, tags: ImmichTags) { const isMotionPhoto = tags.MotionPhoto; const isMicroVideo = tags.MicroVideo; const videoOffset = tags.MicroVideoOffset; @@ -415,7 +451,7 @@ export class MetadataService extends BaseService { if (isMotionPhoto && directory) { for (const entry of directory) { - if (entry?.Item?.Semantic == 'MotionPhoto') { + if (entry?.Item?.Semantic === 'MotionPhoto') { length = entry.Item.Length ?? 0; padding = entry.Item.Padding ?? 0; break; @@ -462,11 +498,10 @@ export class MetadataService extends BaseService { checksum, }); if (motionAsset) { - this.logger.debug( - `Motion photo video with checksum ${checksum.toString( - 'base64', - )} already exists in the repository for asset ${asset.id}: ${asset.originalPath}`, - ); + this.logger.debugFn(() => { + const base64Checksum = checksum.toString('base64'); + return `Motion asset with checksum ${base64Checksum} already exists for asset ${asset.id}: ${asset.originalPath}`; + }); // Hide the motion photo video asset if it's not already hidden to prepare for linking if (motionAsset.isVisible) { @@ -531,6 +566,12 @@ export class MetadataService extends BaseService { } } + private hasTaggedFaces(tags: ImmichTags): tags is ImmichTagsWithFaces { + return ( + tags.RegionInfo !== undefined && tags.RegionInfo.AppliedToDimensions && tags.RegionInfo.RegionList.length > 0 + ); + } + private async applyTaggedFaces(asset: AssetEntity, tags: ImmichTags) { if (!tags.RegionInfo?.AppliedToDimensions || tags.RegionInfo.RegionList.length === 0) { return; @@ -572,7 +613,7 @@ export class MetadataService extends BaseService { } if (missing.length > 0) { - this.logger.debug(`Creating missing persons: ${missing.map((p) => `${p.name}/${p.id}`)}`); + this.logger.debugFn(() => `Creating missing persons: ${missing.map((p) => `${p.name}/${p.id}`)}`); const newPersonIds = await this.personRepository.createAll(missing); const jobs = newPersonIds.map((id) => ({ name: JobName.GENERATE_PERSON_THUMBNAIL, data: { id } }) as const); await this.jobRepository.queueAll(jobs); @@ -650,24 +691,12 @@ export class MetadataService extends BaseService { return new Date(Math.min(a.valueOf(), b.valueOf())); } - private async getGeo(tags: ImmichTags, reverseGeocoding: SystemConfig['reverseGeocoding']) { - let latitude = validate(tags.GPSLatitude); - let longitude = validate(tags.GPSLongitude); - - // TODO take ref into account - - if (latitude === 0 && longitude === 0) { - this.logger.debug('Latitude and longitude of 0, setting to null'); - latitude = null; - longitude = null; - } - - let result: ReverseGeocodeResult = { country: null, state: null, city: null }; - if (reverseGeocoding.enabled && longitude && latitude) { - result = await this.mapRepository.reverseGeocode({ latitude, longitude }); - } - - return { ...result, latitude, longitude }; + private hasGeo(tags: ImmichTags): tags is ImmichTags & { GPSLatitude: number; GPSLongitude: number } { + return ( + tags.GPSLatitude !== undefined && + tags.GPSLongitude !== undefined && + (tags.GPSLatitude !== 0 || tags.GPSLatitude !== 0) + ); } private getAutoStackId(tags: ImmichTags | null): string | null {