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

@@ -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 () => {

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<ImmichTags['RegionInfo']> };
+
 @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<JobName.METADATA_EXTRACTION>): Promise<JobStatus> {
-    const { metadata, reverseGeocoding } = await this.getConfig({ withCache: true });
+    const [{ metadata, reverseGeocoding }, [asset]] = await Promise.all([
-    const [asset] = await this.assetRepository.getByIds([data.id], { faces: { person: false } });
+      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 [stats, exifTags] = await Promise.all([
-
+      this.storageRepository.stat(asset.originalPath),
-    const exifTags = await this.getExifTags(asset);
+      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<Exif> = {
       assetId: asset.id,
@@ -197,9 +211,9 @@ export class MetadataService extends BaseService {
       // gps
       latitude,
       longitude,
-      country,
+      country: geo.country,
-      state,
+      state: geo.state,
-      city,
+      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);
+    const promises: Promise<unknown>[] = [
-    await this.applyMotionPhotos(asset, exifTags);
+      this.assetRepository.upsertExif(exifData),
-
+      this.assetRepository.update({
-    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,
-    });
+      }),
+      this.applyTagList(asset, exifTags),
+    ];
 
-    if (exifData.livePhotoCID) {
+    if (this.isMotionPhoto(asset, exifTags)) {
-      await this.linkLivePhotos(asset, exifData);
+      promises.push(this.applyMotionPhotos(asset, exifTags));
     }
 
-    if (isFaceImportEnabled(metadata)) {
+    if (isFaceImportEnabled(metadata) && this.hasTaggedFaces(exifTags)) {
-      await this.applyTaggedFaces(asset, 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<ImmichTags> {
+  private getExifTags(asset: AssetEntity): Promise<ImmichTags> {
-    const mediaTags = await this.metadataRepository.readTags(asset.originalPath);
+    if (!asset.sidecarPath && asset.type === AssetType.IMAGE) {
-    const sidecarTags = asset.sidecarPath ? await this.metadataRepository.readTags(asset.sidecarPath) : {};
+      return this.metadataRepository.readTags(asset.originalPath);
-    const videoTags = asset.type === AssetType.VIDEO ? await this.getVideoTags(asset.originalPath) : {};
+    }
+
+    return this.mergeExifTags(asset);
+  }
+
+  private async mergeExifTags(asset: AssetEntity): Promise<ImmichTags> {
+    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
+    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) {
+  private getTagList(exifTags: ImmichTags): string[] {
-    const tags: string[] = [];
+    let tags: string[];
     if (exifTags.TagsList) {
-      tags.push(...exifTags.TagsList.map(String));
+      tags = exifTags.TagsList.map(String);
     } else if (exifTags.HierarchicalSubject) {
-      tags.push(
+      tags = exifTags.HierarchicalSubject.map((tag) =>
-        ...exifTags.HierarchicalSubject.map((tag) =>
-          String(tag)
         // convert | to /
-            .replaceAll('/', '<PLACEHOLDER>')
+        typeof tag === 'number'
-            .replaceAll('|', '/')
+          ? String(tag)
-            .replaceAll('<PLACEHOLDER>', '|'),
+          : 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) {
+  private isMotionPhoto(asset: AssetEntity, tags: ImmichTags): boolean {
-    if (asset.type !== AssetType.IMAGE) {
+    return asset.type === AssetType.IMAGE && !!(tags.MotionPhoto || tags.MicroVideo);
-      return;
   }
 
+  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(
+        this.logger.debugFn(() => {
-          `Motion photo video with checksum ${checksum.toString(
+          const base64Checksum = checksum.toString('base64');
-            'base64',
+          return `Motion asset with checksum ${base64Checksum} already exists for asset ${asset.id}: ${asset.originalPath}`;
-          )} already exists in the repository 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']) {
+  private hasGeo(tags: ImmichTags): tags is ImmichTags & { GPSLatitude: number; GPSLongitude: number } {
-    let latitude = validate(tags.GPSLatitude);
+    return (
-    let longitude = validate(tags.GPSLongitude);
+      tags.GPSLatitude !== undefined &&
-
+      tags.GPSLongitude !== undefined &&
-    // TODO take ref into account
+      (tags.GPSLatitude !== 0 || tags.GPSLatitude !== 0)
-
+    );
-    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 getAutoStackId(tags: ImmichTags | null): string | null {