feat(server): optimize person thumbnail generation (#7513)

* do crop and resize together

* redundant `pipelineColorspace` call

* formatting

* fix rebase

* handle orientation

* remove unused import

* formatting

* use oriented dimensions for half size calculation

* default case for orientation

* simplify orientation code

---------

Co-authored-by: Alex <alex.tran1502@gmail.com>

server/src/interfaces/media.interface.ts

@@ -3,11 +3,19 @@ import { ImageFormat, TranscodeTarget, VideoCodec } from 'src/entities/system-co
 
 export const IMediaRepository = 'IMediaRepository';
 
-export interface ResizeOptions {
+export interface CropOptions {
+  top: number;
+  left: number;
+  width: number;
+  height: number;
+}
+
+export interface ThumbnailOptions {
   size: number;
   format: ImageFormat;
   colorspace: string;
   quality: number;
+  crop?: CropOptions;
 }
 
 export interface VideoStreamInfo {
@@ -45,13 +53,6 @@ export interface VideoInfo {
   audioStreams: AudioStreamInfo[];
 }
 
-export interface CropOptions {
-  top: number;
-  left: number;
-  width: number;
-  height: number;
-}
-
 export interface TranscodeOptions {
   inputOptions: string[];
   outputOptions: string[];
@@ -76,8 +77,7 @@ export interface VideoCodecHWConfig extends VideoCodecSWConfig {
 export interface IMediaRepository {
   // image
   extract(input: string, output: string): Promise<boolean>;
-  resize(input: string | Buffer, output: string, options: ResizeOptions): Promise<void>;
-  crop(input: string, options: CropOptions): Promise<Buffer>;
+  generateThumbnail(input: string | Buffer, output: string, options: ThumbnailOptions): Promise<void>;
   generateThumbhash(imagePath: string): Promise<Buffer>;
   getImageDimensions(input: string): Promise<ImageDimensions>;
 

server/src/repositories/media.repository.ts

@@ -8,10 +8,9 @@ import sharp from 'sharp';
 import { Colorspace } from 'src/entities/system-config.entity';
 import { ILoggerRepository } from 'src/interfaces/logger.interface';
 import {
-  CropOptions,
   IMediaRepository,
   ImageDimensions,
-  ResizeOptions,
+  ThumbnailOptions,
   TranscodeOptions,
   VideoInfo,
 } from 'src/interfaces/media.interface';
@@ -45,23 +44,17 @@ export class MediaRepository implements IMediaRepository {
     return true;
   }
 
-  crop(input: string | Buffer, options: CropOptions): Promise<Buffer> {
-    return sharp(input, { failOn: 'none' })
-      .pipelineColorspace('rgb16')
-      .extract({
-        left: options.left,
-        top: options.top,
-        width: options.width,
-        height: options.height,
-      })
-      .toBuffer();
-  }
-
-  async resize(input: string | Buffer, output: string, options: ResizeOptions): Promise<void> {
-    await sharp(input, { failOn: 'none' })
+  async generateThumbnail(input: string | Buffer, output: string, options: ThumbnailOptions): Promise<void> {
+    const pipeline = sharp(input, { failOn: 'none' })
       .pipelineColorspace(options.colorspace === Colorspace.SRGB ? 'srgb' : 'rgb16')
+      .rotate();
+
+    if (options.crop) {
+      pipeline.extract(options.crop);
+    }
+
+    await pipeline
       .resize(options.size, options.size, { fit: 'outside', withoutEnlargement: true })
-      .rotate()
       .withIccProfile(options.colorspace)
       .toFormat(options.format, {
         quality: options.quality,

server/src/services/media.service.spec.ts

@@ -213,7 +213,7 @@ describe(MediaService.name, () => {
     it('should skip thumbnail generation if asset not found', async () => {
       assetMock.getByIds.mockResolvedValue([]);
       await sut.handleGeneratePreview({ id: assetStub.image.id });
-      expect(mediaMock.resize).not.toHaveBeenCalled();
+      expect(mediaMock.generateThumbnail).not.toHaveBeenCalled();
       expect(assetMock.update).not.toHaveBeenCalledWith();
     });
 
@@ -221,7 +221,7 @@ describe(MediaService.name, () => {
       mediaMock.probe.mockResolvedValue(probeStub.noVideoStreams);
       assetMock.getByIds.mockResolvedValue([assetStub.video]);
       await sut.handleGeneratePreview({ id: assetStub.image.id });
-      expect(mediaMock.resize).not.toHaveBeenCalled();
+      expect(mediaMock.generateThumbnail).not.toHaveBeenCalled();
       expect(assetMock.update).not.toHaveBeenCalledWith();
     });
 
@@ -230,7 +230,7 @@ describe(MediaService.name, () => {
 
       expect(await sut.handleGeneratePreview({ id: assetStub.livePhotoMotionAsset.id })).toEqual(JobStatus.SKIPPED);
 
-      expect(mediaMock.resize).not.toHaveBeenCalled();
+      expect(mediaMock.generateThumbnail).not.toHaveBeenCalled();
       expect(assetMock.update).not.toHaveBeenCalledWith();
     });
 
@@ -242,7 +242,7 @@ describe(MediaService.name, () => {
       await sut.handleGeneratePreview({ id: assetStub.image.id });
 
       expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/thumbs/user-id/as/se');
-      expect(mediaMock.resize).toHaveBeenCalledWith('/original/path.jpg', previewPath, {
+      expect(mediaMock.generateThumbnail).toHaveBeenCalledWith('/original/path.jpg', previewPath, {
         size: 1440,
         format,
         quality: 80,
@@ -269,7 +269,7 @@ describe(MediaService.name, () => {
       await sut.handleGeneratePreview({ id: assetStub.image.id });
 
       expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/thumbs/user-id/as/se');
-      expect(mediaMock.resize).toHaveBeenCalledWith(
+      expect(mediaMock.generateThumbnail).toHaveBeenCalledWith(
         '/original/path.jpg',
         'upload/thumbs/user-id/as/se/asset-id-preview.jpeg',
         {
@@ -369,7 +369,7 @@ describe(MediaService.name, () => {
     it('should skip thumbnail generation if asset not found', async () => {
       assetMock.getByIds.mockResolvedValue([]);
       await sut.handleGenerateThumbnail({ id: assetStub.image.id });
-      expect(mediaMock.resize).not.toHaveBeenCalled();
+      expect(mediaMock.generateThumbnail).not.toHaveBeenCalled();
       expect(assetMock.update).not.toHaveBeenCalledWith();
     });
 
@@ -378,7 +378,7 @@ describe(MediaService.name, () => {
 
       expect(await sut.handleGenerateThumbnail({ id: assetStub.livePhotoMotionAsset.id })).toEqual(JobStatus.SKIPPED);
 
-      expect(mediaMock.resize).not.toHaveBeenCalled();
+      expect(mediaMock.generateThumbnail).not.toHaveBeenCalled();
       expect(assetMock.update).not.toHaveBeenCalledWith();
     });
 
@@ -392,7 +392,7 @@ describe(MediaService.name, () => {
         await sut.handleGenerateThumbnail({ id: assetStub.image.id });
 
         expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/thumbs/user-id/as/se');
-        expect(mediaMock.resize).toHaveBeenCalledWith('/original/path.jpg', thumbnailPath, {
+        expect(mediaMock.generateThumbnail).toHaveBeenCalledWith('/original/path.jpg', thumbnailPath, {
           size: 250,
           format,
           quality: 80,
@@ -419,7 +419,7 @@ describe(MediaService.name, () => {
     await sut.handleGenerateThumbnail({ id: assetStub.image.id });
 
     expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/thumbs/user-id/as/se');
-    expect(mediaMock.resize).toHaveBeenCalledWith(
+    expect(mediaMock.generateThumbnail).toHaveBeenCalledWith(
       assetStub.imageDng.originalPath,
       'upload/thumbs/user-id/as/se/asset-id-thumbnail.webp',
       {
@@ -444,7 +444,7 @@ describe(MediaService.name, () => {
     await sut.handleGenerateThumbnail({ id: assetStub.image.id });
 
     const extractedPath = mediaMock.extract.mock.calls.at(-1)?.[1].toString();
-    expect(mediaMock.resize.mock.calls).toEqual([
+    expect(mediaMock.generateThumbnail.mock.calls).toEqual([
       [
         extractedPath,
         'upload/thumbs/user-id/as/se/asset-id-thumbnail.webp',
@@ -468,7 +468,7 @@ describe(MediaService.name, () => {
 
     await sut.handleGenerateThumbnail({ id: assetStub.image.id });
 
-    expect(mediaMock.resize.mock.calls).toEqual([
+    expect(mediaMock.generateThumbnail.mock.calls).toEqual([
       [
         assetStub.imageDng.originalPath,
         'upload/thumbs/user-id/as/se/asset-id-thumbnail.webp',
@@ -491,7 +491,7 @@ describe(MediaService.name, () => {
 
     await sut.handleGenerateThumbnail({ id: assetStub.image.id });
 
-    expect(mediaMock.resize).toHaveBeenCalledWith(
+    expect(mediaMock.generateThumbnail).toHaveBeenCalledWith(
       assetStub.imageDng.originalPath,
       'upload/thumbs/user-id/as/se/asset-id-thumbnail.webp',
       {
@@ -511,7 +511,7 @@ describe(MediaService.name, () => {
     await sut.handleGenerateThumbnail({ id: assetStub.image.id });
 
     expect(mediaMock.extract).not.toHaveBeenCalled();
-    expect(mediaMock.resize).toHaveBeenCalledWith(
+    expect(mediaMock.generateThumbnail).toHaveBeenCalledWith(
       assetStub.imageDng.originalPath,
       'upload/thumbs/user-id/as/se/asset-id-thumbnail.webp',
       {

server/src/services/media.service.ts

@@ -210,7 +210,8 @@ export class MediaService {
           const colorspace = this.isSRGB(asset) ? Colorspace.SRGB : image.colorspace;
           const imageOptions = { format, size, colorspace, quality: image.quality };
 
-          await this.mediaRepository.resize(useExtracted ? extractedPath : asset.originalPath, path, imageOptions);
+          const outputPath = useExtracted ? extractedPath : asset.originalPath;
+          await this.mediaRepository.generateThumbnail(outputPath, path, imageOptions);
         } finally {
           if (didExtract) {
             await this.storageRepository.unlink(extractedPath);

server/src/services/person.service.spec.ts

@@ -45,8 +45,6 @@ const responseDto: PersonResponseDto = {
 
 const statistics = { assets: 3 };
 
-const croppedFace = Buffer.from('Cropped Face');
-
 const detectFaceMock = {
   assetId: 'asset-1',
   personId: 'person-1',
@@ -104,8 +102,6 @@ describe(PersonService.name, () => {
       cryptoMock,
       loggerMock,
     );
-
-    mediaMock.crop.mockResolvedValue(croppedFace);
   });
 
   it('should be defined', () => {
@@ -862,20 +858,20 @@ describe(PersonService.name, () => {
     it('should skip a person not found', async () => {
       personMock.getById.mockResolvedValue(null);
       await sut.handleGeneratePersonThumbnail({ id: 'person-1' });
-      expect(mediaMock.crop).not.toHaveBeenCalled();
+      expect(mediaMock.generateThumbnail).not.toHaveBeenCalled();
     });
 
     it('should skip a person without a face asset id', async () => {
       personMock.getById.mockResolvedValue(personStub.noThumbnail);
       await sut.handleGeneratePersonThumbnail({ id: 'person-1' });
-      expect(mediaMock.crop).not.toHaveBeenCalled();
+      expect(mediaMock.generateThumbnail).not.toHaveBeenCalled();
     });
 
     it('should skip a person with a face asset id not found', async () => {
       personMock.getById.mockResolvedValue({ ...personStub.primaryPerson, faceAssetId: faceStub.middle.id });
       personMock.getFaceByIdWithAssets.mockResolvedValue(faceStub.face1);
       await sut.handleGeneratePersonThumbnail({ id: 'person-1' });
-      expect(mediaMock.crop).not.toHaveBeenCalled();
+      expect(mediaMock.generateThumbnail).not.toHaveBeenCalled();
     });
 
     it('should skip a person with a face asset id without a thumbnail', async () => {
@@ -883,30 +879,34 @@ describe(PersonService.name, () => {
       personMock.getFaceByIdWithAssets.mockResolvedValue(faceStub.face1);
       assetMock.getByIds.mockResolvedValue([assetStub.noResizePath]);
       await sut.handleGeneratePersonThumbnail({ id: 'person-1' });
-      expect(mediaMock.crop).not.toHaveBeenCalled();
+      expect(mediaMock.generateThumbnail).not.toHaveBeenCalled();
     });
 
     it('should generate a thumbnail', async () => {
       personMock.getById.mockResolvedValue({ ...personStub.primaryPerson, faceAssetId: faceStub.middle.assetId });
       personMock.getFaceByIdWithAssets.mockResolvedValue(faceStub.middle);
-      assetMock.getByIds.mockResolvedValue([assetStub.primaryImage]);
+      assetMock.getById.mockResolvedValue(assetStub.primaryImage);
 
-      await sut.handleGeneratePersonThumbnail({ id: 'person-1' });
+      await sut.handleGeneratePersonThumbnail({ id: personStub.primaryPerson.id });
 
-      expect(assetMock.getByIds).toHaveBeenCalledWith([faceStub.middle.assetId]);
+      expect(assetMock.getById).toHaveBeenCalledWith(faceStub.middle.assetId, { exifInfo: true });
       expect(storageMock.mkdirSync).toHaveBeenCalledWith('upload/thumbs/admin_id/pe/rs');
-      expect(mediaMock.crop).toHaveBeenCalledWith('/uploads/admin-id/thumbs/path.jpg', {
-        left: 95,
-        top: 95,
-        width: 110,
-        height: 110,
-      });
-      expect(mediaMock.resize).toHaveBeenCalledWith(croppedFace, 'upload/thumbs/admin_id/pe/rs/person-1.jpeg', {
-        format: 'jpeg',
-        size: 250,
-        quality: 80,
-        colorspace: Colorspace.P3,
-      });
+      expect(mediaMock.generateThumbnail).toHaveBeenCalledWith(
+        assetStub.primaryImage.originalPath,
+        'upload/thumbs/admin_id/pe/rs/person-1.jpeg',
+        {
+          format: 'jpeg',
+          size: 250,
+          quality: 80,
+          colorspace: Colorspace.P3,
+          crop: {
+            left: 238,
+            top: 163,
+            width: 274,
+            height: 274,
+          },
+        },
+      );
       expect(personMock.update).toHaveBeenCalledWith({
         id: 'person-1',
         thumbnailPath: 'upload/thumbs/admin_id/pe/rs/person-1.jpeg',
@@ -916,43 +916,51 @@ describe(PersonService.name, () => {
     it('should generate a thumbnail without going negative', async () => {
       personMock.getById.mockResolvedValue({ ...personStub.primaryPerson, faceAssetId: faceStub.start.assetId });
       personMock.getFaceByIdWithAssets.mockResolvedValue(faceStub.start);
-      assetMock.getByIds.mockResolvedValue([assetStub.image]);
+      assetMock.getById.mockResolvedValue(assetStub.image);
 
-      await sut.handleGeneratePersonThumbnail({ id: 'person-1' });
+      await sut.handleGeneratePersonThumbnail({ id: personStub.primaryPerson.id });
 
-      expect(mediaMock.crop).toHaveBeenCalledWith('/uploads/user-id/thumbs/path.jpg', {
-        left: 0,
-        top: 0,
-        width: 510,
-        height: 510,
-      });
-      expect(mediaMock.resize).toHaveBeenCalledWith(croppedFace, 'upload/thumbs/admin_id/pe/rs/person-1.jpeg', {
-        format: 'jpeg',
-        size: 250,
-        quality: 80,
-        colorspace: Colorspace.P3,
-      });
+      expect(mediaMock.generateThumbnail).toHaveBeenCalledWith(
+        assetStub.image.originalPath,
+        'upload/thumbs/admin_id/pe/rs/person-1.jpeg',
+        {
+          format: 'jpeg',
+          size: 250,
+          quality: 80,
+          colorspace: Colorspace.P3,
+          crop: {
+            left: 0,
+            top: 428,
+            width: 1102,
+            height: 1102,
+          },
+        },
+      );
     });
 
     it('should generate a thumbnail without overflowing', async () => {
       personMock.getById.mockResolvedValue({ ...personStub.primaryPerson, faceAssetId: faceStub.end.assetId });
       personMock.getFaceByIdWithAssets.mockResolvedValue(faceStub.end);
-      assetMock.getByIds.mockResolvedValue([assetStub.primaryImage]);
+      assetMock.getById.mockResolvedValue(assetStub.primaryImage);
 
-      await sut.handleGeneratePersonThumbnail({ id: 'person-1' });
+      await sut.handleGeneratePersonThumbnail({ id: personStub.primaryPerson.id });
 
-      expect(mediaMock.crop).toHaveBeenCalledWith('/uploads/admin-id/thumbs/path.jpg', {
-        left: 297,
-        top: 297,
-        width: 202,
-        height: 202,
-      });
-      expect(mediaMock.resize).toHaveBeenCalledWith(croppedFace, 'upload/thumbs/admin_id/pe/rs/person-1.jpeg', {
-        format: 'jpeg',
-        size: 250,
-        quality: 80,
-        colorspace: Colorspace.P3,
-      });
+      expect(mediaMock.generateThumbnail).toHaveBeenCalledWith(
+        assetStub.primaryImage.originalPath,
+        'upload/thumbs/admin_id/pe/rs/person-1.jpeg',
+        {
+          format: 'jpeg',
+          size: 250,
+          quality: 80,
+          colorspace: Colorspace.P3,
+          crop: {
+            left: 591,
+            top: 591,
+            width: 408,
+            height: 408,
+          },
+        },
+      );
     });
   });
 

server/src/services/person.service.ts

@@ -40,12 +40,13 @@ import {
 } from 'src/interfaces/job.interface';
 import { ILoggerRepository } from 'src/interfaces/logger.interface';
 import { IMachineLearningRepository } from 'src/interfaces/machine-learning.interface';
-import { CropOptions, IMediaRepository } from 'src/interfaces/media.interface';
+import { CropOptions, IMediaRepository, ImageDimensions } from 'src/interfaces/media.interface';
 import { IMoveRepository } from 'src/interfaces/move.interface';
 import { IPersonRepository, UpdateFacesData } from 'src/interfaces/person.interface';
 import { ISearchRepository } from 'src/interfaces/search.interface';
 import { IStorageRepository } from 'src/interfaces/storage.interface';
 import { ISystemConfigRepository } from 'src/interfaces/system-config.interface';
+import { Orientation } from 'src/services/metadata.service';
 import { CacheControl, ImmichFileResponse } from 'src/utils/file';
 import { mimeTypes } from 'src/utils/mime-types';
 import { usePagination } from 'src/utils/pagination';
@@ -489,11 +490,13 @@ export class PersonService {
 
     const person = await this.repository.getById(data.id);
     if (!person?.faceAssetId) {
+      this.logger.error(`Could not generate person thumbnail: person ${person?.id} has no face asset`);
       return JobStatus.FAILED;
     }
 
     const face = await this.repository.getFaceByIdWithAssets(person.faceAssetId);
     if (face === null) {
+      this.logger.error(`Could not generate person thumbnail: face ${person.faceAssetId} not found`);
       return JobStatus.FAILED;
     }
 
@@ -507,19 +510,29 @@ export class PersonService {
       imageHeight,
     } = face;
 
-    const [asset] = await this.assetRepository.getByIds([assetId]);
-    if (!asset?.previewPath) {
+    const asset = await this.assetRepository.getById(assetId, { exifInfo: true });
+    if (!asset?.exifInfo?.exifImageHeight || !asset.exifInfo.exifImageWidth) {
+      this.logger.error(`Could not generate person thumbnail: asset ${assetId} dimensions are unknown`);
       return JobStatus.FAILED;
     }
+
     this.logger.verbose(`Cropping face for person: ${person.id}`);
     const thumbnailPath = StorageCore.getPersonThumbnailPath(person);
     this.storageCore.ensureFolders(thumbnailPath);
 
-    const halfWidth = (x2 - x1) / 2;
-    const halfHeight = (y2 - y1) / 2;
+    const { width: exifWidth, height: exifHeight } = this.withOrientation(asset.exifInfo.orientation as Orientation, {
+      width: asset.exifInfo.exifImageWidth,
+      height: asset.exifInfo.exifImageHeight,
+    });
 
-    const middleX = Math.round(x1 + halfWidth);
-    const middleY = Math.round(y1 + halfHeight);
+    const widthScale = exifWidth / imageWidth;
+    const heightScale = exifHeight / imageHeight;
+
+    const halfWidth = (widthScale * (x2 - x1)) / 2;
+    const halfHeight = (heightScale * (y2 - y1)) / 2;
+
+    const middleX = Math.round(widthScale * x1 + halfWidth);
+    const middleY = Math.round(heightScale * y1 + halfHeight);
 
     // zoom out 10%
     const targetHalfSize = Math.floor(Math.max(halfWidth, halfHeight) * 1.1);
@@ -528,8 +541,8 @@ export class PersonService {
     const newHalfSize = Math.min(
       middleX - Math.max(0, middleX - targetHalfSize),
       middleY - Math.max(0, middleY - targetHalfSize),
-      Math.min(imageWidth - 1, middleX + targetHalfSize) - middleX,
-      Math.min(imageHeight - 1, middleY + targetHalfSize) - middleY,
+      Math.min(exifWidth - 1, middleX + targetHalfSize) - middleX,
+      Math.min(exifHeight - 1, middleY + targetHalfSize) - middleY,
     );
 
     const cropOptions: CropOptions = {
@@ -539,15 +552,15 @@ export class PersonService {
       height: newHalfSize * 2,
     };
 
-    const croppedOutput = await this.mediaRepository.crop(asset.previewPath, cropOptions);
     const thumbnailOptions = {
       format: ImageFormat.JPEG,
       size: FACE_THUMBNAIL_SIZE,
       colorspace: image.colorspace,
       quality: image.quality,
+      crop: cropOptions,
     } as const;
 
-    await this.mediaRepository.resize(croppedOutput, thumbnailPath, thumbnailOptions);
+    await this.mediaRepository.generateThumbnail(asset.originalPath, thumbnailPath, thumbnailOptions);
     await this.repository.update({ id: person.id, thumbnailPath });
 
     return JobStatus.SUCCESS;
@@ -614,4 +627,18 @@ export class PersonService {
     }
     return person;
   }
+
+  private withOrientation(orientation: Orientation, { width, height }: ImageDimensions): ImageDimensions {
+    switch (orientation) {
+      case Orientation.MirrorHorizontalRotate270CW:
+      case Orientation.Rotate90CW:
+      case Orientation.MirrorHorizontalRotate90CW:
+      case Orientation.Rotate270CW: {
+        return { width: height, height: width };
+      }
+      default: {
+        return { width, height };
+      }
+    }
+  }
 }

server/test/fixtures/asset.stub.ts

@@ -163,6 +163,8 @@ export const assetStub = {
     sidecarPath: null,
     exifInfo: {
       fileSizeInByte: 5000,
+      exifImageHeight: 1000,
+      exifImageWidth: 1000,
     } as ExifEntity,
     stack: assetStackStub('stack-1', [
       { id: 'primary-asset-id' } as AssetEntity,
@@ -207,6 +209,8 @@ export const assetStub = {
     sidecarPath: null,
     exifInfo: {
       fileSizeInByte: 5000,
+      exifImageHeight: 3840,
+      exifImageWidth: 2160,
     } as ExifEntity,
   }),
 

server/test/repositories/media.repository.mock.ts

@@ -3,10 +3,9 @@ import { Mocked, vitest } from 'vitest';
 
 export const newMediaRepositoryMock = (): Mocked<IMediaRepository> => {
   return {
+    generateThumbnail: vitest.fn(),
     generateThumbhash: vitest.fn(),
     extract: vitest.fn().mockResolvedValue(false),
-    resize: vitest.fn(),
-    crop: vitest.fn(),
     probe: vitest.fn(),
     transcode: vitest.fn(),
     getImageDimensions: vitest.fn(),