From 1167f0f2b7a9481104b7fa63cae34870e83ec44a Mon Sep 17 00:00:00 2001 From: Mert <101130780+mertalev@users.noreply.github.com> Date: Wed, 8 May 2024 09:09:34 -0400 Subject: [PATCH] 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 | 20 ++-- server/src/repositories/media.repository.ts | 27 ++--- server/src/services/media.service.spec.ts | 26 ++--- server/src/services/media.service.ts | 3 +- server/src/services/person.service.spec.ts | 110 ++++++++++-------- server/src/services/person.service.ts | 49 ++++++-- server/test/fixtures/asset.stub.ts | 4 + .../repositories/media.repository.mock.ts | 3 +- 8 files changed, 137 insertions(+), 105 deletions(-) diff --git a/server/src/interfaces/media.interface.ts b/server/src/interfaces/media.interface.ts index a82b38b6de..092536b026 100644 --- a/server/src/interfaces/media.interface.ts +++ b/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>; diff --git a/server/src/repositories/media.repository.ts b/server/src/repositories/media.repository.ts index 434fb585f8..71c58fd91e 100644 --- a/server/src/repositories/media.repository.ts +++ b/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, diff --git a/server/src/services/media.service.spec.ts b/server/src/services/media.service.spec.ts index e58dc1d013..637e5fa248 100644 --- a/server/src/services/media.service.spec.ts +++ b/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', { diff --git a/server/src/services/media.service.ts b/server/src/services/media.service.ts index b7f4a1a3ad..00623cecbe 100644 --- a/server/src/services/media.service.ts +++ b/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); diff --git a/server/src/services/person.service.spec.ts b/server/src/services/person.service.spec.ts index a9d96bbcea..3a1d76388e 100644 --- a/server/src/services/person.service.spec.ts +++ b/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, + }, + }, + ); }); }); diff --git a/server/src/services/person.service.ts b/server/src/services/person.service.ts index 721f2586ee..c8a16f71c7 100644 --- a/server/src/services/person.service.ts +++ b/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 }; + } + } + } } diff --git a/server/test/fixtures/asset.stub.ts b/server/test/fixtures/asset.stub.ts index 2491406785..e5d30f72fa 100644 --- a/server/test/fixtures/asset.stub.ts +++ b/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, }), diff --git a/server/test/repositories/media.repository.mock.ts b/server/test/repositories/media.repository.mock.ts index da3e05fe81..4c344a9866 100644 --- a/server/test/repositories/media.repository.mock.ts +++ b/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(),