From 34592b38f68ccaeadce18c5862c427f4e0ee8178 Mon Sep 17 00:00:00 2001 From: Eli Gao Date: Wed, 4 Dec 2024 05:23:55 +0800 Subject: [PATCH] refactor: tweaks for code review --- e2e/src/api/specs/asset.e2e-spec.ts | 5 +- mobile/openapi/lib/model/path_type.dart | 6 +- open-api/immich-openapi-specs.json | 2 +- open-api/typescript-sdk/src/fetch-client.ts | 2 +- server/src/cores/storage.core.ts | 2 +- server/src/enum.ts | 4 +- server/src/interfaces/asset.interface.ts | 1 - server/src/interfaces/media.interface.ts | 2 +- server/src/repositories/asset.repository.ts | 4 - server/src/repositories/media.repository.ts | 4 +- server/src/services/asset-media.service.ts | 4 +- server/src/services/media.service.spec.ts | 29 +++---- server/src/services/media.service.ts | 80 +++++++++++-------- server/src/utils/asset.util.ts | 2 +- .../repositories/asset.repository.mock.ts | 1 - .../asset-viewer/photo-viewer.svelte | 2 +- 16 files changed, 76 insertions(+), 74 deletions(-) diff --git a/e2e/src/api/specs/asset.e2e-spec.ts b/e2e/src/api/specs/asset.e2e-spec.ts index a0c429a82e224e..3e1b05a078a652 100644 --- a/e2e/src/api/specs/asset.e2e-spec.ts +++ b/e2e/src/api/specs/asset.e2e-spec.ts @@ -1222,7 +1222,7 @@ describe('/asset', () => { }, ]; - it(`should upload and generate a thumbnail for different file types`, async () => { + it(`should upload and generate a thumbnail for different file types`, { timeout: 60_000 }, async () => { // upload in parallel const assets = await Promise.all( tests.map(async ({ input }) => { @@ -1235,7 +1235,8 @@ describe('/asset', () => { for (const { id, status } of assets) { expect(status).toBe(AssetMediaStatus.Created); - await utils.waitForWebsocketEvent({ event: 'assetUpload', id }); + // longer timeout as the thumbnail generation from full-size raw files can take a while + await utils.waitForWebsocketEvent({ event: 'assetUpload', id, timeout: 60_000 }); } for (const [i, { id }] of assets.entries()) { diff --git a/mobile/openapi/lib/model/path_type.dart b/mobile/openapi/lib/model/path_type.dart index e5c469b08a1025..1f0cc9cadf1587 100644 --- a/mobile/openapi/lib/model/path_type.dart +++ b/mobile/openapi/lib/model/path_type.dart @@ -24,7 +24,7 @@ class PathType { String toJson() => value; static const original = PathType._(r'original'); - static const extracted = PathType._(r'extracted'); + static const converted = PathType._(r'converted'); static const preview = PathType._(r'preview'); static const thumbnail = PathType._(r'thumbnail'); static const encodedVideo = PathType._(r'encoded_video'); @@ -35,7 +35,7 @@ class PathType { /// List of all possible values in this [enum][PathType]. static const values = [ original, - extracted, + converted, preview, thumbnail, encodedVideo, @@ -81,7 +81,7 @@ class PathTypeTypeTransformer { if (data != null) { switch (data) { case r'original': return PathType.original; - case r'extracted': return PathType.extracted; + case r'converted': return PathType.converted; case r'preview': return PathType.preview; case r'thumbnail': return PathType.thumbnail; case r'encoded_video': return PathType.encodedVideo; diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index bfe50c1180bae4..20cf69b1856df6 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -10021,7 +10021,7 @@ "PathType": { "enum": [ "original", - "extracted", + "converted", "preview", "thumbnail", "encoded_video", diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts index f3abb015c30d00..bdfc95461077b1 100644 --- a/open-api/typescript-sdk/src/fetch-client.ts +++ b/open-api/typescript-sdk/src/fetch-client.ts @@ -3480,7 +3480,7 @@ export enum PathEntityType { } export enum PathType { Original = "original", - Extracted = "extracted", + Converted = "converted", Preview = "preview", Thumbnail = "thumbnail", EncodedVideo = "encoded_video", diff --git a/server/src/cores/storage.core.ts b/server/src/cores/storage.core.ts index 6f194c275f9ff7..cfd177ec163a36 100644 --- a/server/src/cores/storage.core.ts +++ b/server/src/cores/storage.core.ts @@ -26,7 +26,7 @@ export interface MoveRequest { }; } -export type GeneratedImageType = AssetPathType.PREVIEW | AssetPathType.THUMBNAIL | AssetPathType.EXTRACTED; +export type GeneratedImageType = AssetPathType.PREVIEW | AssetPathType.THUMBNAIL | AssetPathType.CONVERTED; export type GeneratedAssetType = GeneratedImageType | AssetPathType.ENCODED_VIDEO; let instance: StorageCore | null; diff --git a/server/src/enum.ts b/server/src/enum.ts index e23e6d22d1caf9..b665c348834141 100644 --- a/server/src/enum.ts +++ b/server/src/enum.ts @@ -36,7 +36,7 @@ export enum AssetFileType { /** * An full/large-size image extracted/converted from RAW photos */ - EXTRACTED = 'extracted', + CONVERTED = 'converted', PREVIEW = 'preview', THUMBNAIL = 'thumbnail', } @@ -241,7 +241,7 @@ export enum ManualJobName { export enum AssetPathType { ORIGINAL = 'original', - EXTRACTED = 'extracted', + CONVERTED = 'converted', PREVIEW = 'preview', THUMBNAIL = 'thumbnail', ENCODED_VIDEO = 'encoded_video', diff --git a/server/src/interfaces/asset.interface.ts b/server/src/interfaces/asset.interface.ts index 9b2cda147d18c9..1b32c57d4141b2 100644 --- a/server/src/interfaces/asset.interface.ts +++ b/server/src/interfaces/asset.interface.ts @@ -180,7 +180,6 @@ export interface IAssetRepository { updateDuplicates(options: AssetUpdateDuplicateOptions): Promise; update(asset: AssetUpdateOptions): Promise; remove(asset: AssetEntity): Promise; - removeAssetFile(path: string): Promise; findLivePhotoMatch(options: LivePhotoSearchOptions): Promise; getStatistics(ownerId: string, options: AssetStatsOptions): Promise; getTimeBuckets(options: TimeBucketOptions): Promise; diff --git a/server/src/interfaces/media.interface.ts b/server/src/interfaces/media.interface.ts index 468a6ad88d9ec0..be7ff8b10bc434 100644 --- a/server/src/interfaces/media.interface.ts +++ b/server/src/interfaces/media.interface.ts @@ -30,7 +30,7 @@ interface DecodeImageOptions { } export interface DecodeToBufferOptions extends DecodeImageOptions { - size: number; + size?: number; orientation?: ExifOrientation; } diff --git a/server/src/repositories/asset.repository.ts b/server/src/repositories/asset.repository.ts index dcd7fa5354ef0d..ce7d257b40f288 100644 --- a/server/src/repositories/asset.repository.ts +++ b/server/src/repositories/asset.repository.ts @@ -281,10 +281,6 @@ export class AssetRepository implements IAssetRepository { await this.repository.remove(asset); } - async removeAssetFile(path: string): Promise { - await this.fileRepository.delete({ path }); - } - @GenerateSql({ params: [{ ownerId: DummyValue.UUID, libraryId: DummyValue.UUID, checksum: DummyValue.BUFFER }] }) getByChecksum({ ownerId, diff --git a/server/src/repositories/media.repository.ts b/server/src/repositories/media.repository.ts index e4951d1d97e636..0891defa92774f 100644 --- a/server/src/repositories/media.repository.ts +++ b/server/src/repositories/media.repository.ts @@ -103,8 +103,8 @@ export class MediaRepository implements IMediaRepository { pipeline = pipeline.extract(options.crop); } - // Infinity is a special value that means no resizing - if (options.size === Infinity) { + // No size, no resizing + if (options.size !== undefined) { return pipeline; } return pipeline.resize(options.size, options.size, { fit: 'outside', withoutEnlargement: true }); diff --git a/server/src/services/asset-media.service.ts b/server/src/services/asset-media.service.ts index 74c4e7cb63f340..51b7fcfdddfcdc 100644 --- a/server/src/services/asset-media.service.ts +++ b/server/src/services/asset-media.service.ts @@ -208,14 +208,14 @@ export class AssetMediaService extends BaseService { const asset = await this.findOrFail(id); const size = dto.size ?? AssetMediaSize.THUMBNAIL; - const { thumbnailFile, previewFile, extractedFile } = getAssetFiles(asset.files); + const { thumbnailFile, previewFile, convertedFile } = getAssetFiles(asset.files); let filepath = previewFile?.path; if (size === AssetMediaSize.THUMBNAIL && thumbnailFile) { filepath = thumbnailFile.path; } else if (size === AssetMediaSize.ORIGINAL) { // eslint-disable-next-line unicorn/prefer-ternary if (mimeTypes.isRaw(asset.originalPath)) { - filepath = extractedFile?.path ?? previewFile?.path; + filepath = convertedFile?.path ?? previewFile?.path; } else { filepath = asset.originalPath; } diff --git a/server/src/services/media.service.spec.ts b/server/src/services/media.service.spec.ts index a1aa67eae3eb6b..471c6112376262 100644 --- a/server/src/services/media.service.spec.ts +++ b/server/src/services/media.service.spec.ts @@ -628,9 +628,9 @@ describe(MediaService.name, () => { await sut.handleGenerateThumbnails({ id: assetStub.image.id }); - const extractedPath = mediaMock.extract.mock.calls.at(-1)?.[1].toString(); + const convertedPath = mediaMock.extract.mock.calls.at(-1)?.[1].toString(); expect(mediaMock.decodeImage).toHaveBeenCalledOnce(); - expect(mediaMock.decodeImage).toHaveBeenCalledWith(extractedPath, { + expect(mediaMock.decodeImage).toHaveBeenCalledWith(convertedPath, { colorspace: Colorspace.P3, processInvalidImages: false, size: 1440, @@ -645,18 +645,17 @@ describe(MediaService.name, () => { await sut.handleGenerateThumbnails({ id: assetStub.image.id }); - const extractedPath = mediaMock.extract.mock.calls.at(-1)?.[1].toString(); + const convertedPath = mediaMock.extract.mock.calls.at(-1)?.[1].toString(); expect(mediaMock.generateThumbnail).toHaveBeenCalledWith( - assetStub.imageDng.originalPath, - expect.objectContaining({ size: Infinity }), - extractedPath, + rawBuffer, + expect.objectContaining({ size: 1440 }), + convertedPath, ); - expect(extractedPath).toMatch(/-extracted\.jpeg$/); + expect(convertedPath).toMatch(/-converted\.jpeg$/); expect(mediaMock.decodeImage).toHaveBeenCalledOnce(); - expect(mediaMock.decodeImage).toHaveBeenCalledWith(extractedPath, { + expect(mediaMock.decodeImage).toHaveBeenCalledWith(assetStub.imageDng.originalPath, { colorspace: Colorspace.P3, processInvalidImages: false, - size: 1440, }); }); @@ -667,10 +666,9 @@ describe(MediaService.name, () => { await sut.handleGenerateThumbnails({ id: assetStub.image.id }); expect(mediaMock.decodeImage).toHaveBeenCalledOnce(); - expect(mediaMock.decodeImage).toHaveBeenCalledWith('upload/thumbs/user-id/as/se/asset-id-extracted.jpeg', { + expect(mediaMock.decodeImage).toHaveBeenCalledWith(assetStub.imageDng.originalPath, { colorspace: Colorspace.P3, processInvalidImages: false, - size: 1440, }); expect(mediaMock.getImageDimensions).not.toHaveBeenCalled(); }); @@ -683,10 +681,9 @@ describe(MediaService.name, () => { expect(mediaMock.extract).not.toHaveBeenCalled(); expect(mediaMock.decodeImage).toHaveBeenCalledOnce(); - expect(mediaMock.decodeImage).toHaveBeenCalledWith('upload/thumbs/user-id/as/se/asset-id-extracted.jpeg', { + expect(mediaMock.decodeImage).toHaveBeenCalledWith(assetStub.imageDng.originalPath, { colorspace: Colorspace.P3, processInvalidImages: false, - size: 1440, }); expect(mediaMock.getImageDimensions).not.toHaveBeenCalled(); }); @@ -700,15 +697,15 @@ describe(MediaService.name, () => { expect(mediaMock.decodeImage).toHaveBeenCalledOnce(); expect(mediaMock.decodeImage).toHaveBeenCalledWith( - 'upload/thumbs/user-id/as/se/asset-id-extracted.jpeg', + assetStub.imageDng.originalPath, expect.objectContaining({ processInvalidImages: true }), ); expect(mediaMock.generateThumbnail).toHaveBeenCalledTimes(3); expect(mediaMock.generateThumbnail).toHaveBeenCalledWith( - assetStub.imageDng.originalPath, + rawBuffer, expect.objectContaining({ processInvalidImages: true }), - 'upload/thumbs/user-id/as/se/asset-id-extracted.jpeg', + 'upload/thumbs/user-id/as/se/asset-id-converted.jpeg', ); expect(mediaMock.generateThumbnail).toHaveBeenCalledWith( rawBuffer, diff --git a/server/src/services/media.service.ts b/server/src/services/media.service.ts index 63ff4d3a8a6b03..dd0aad8e0fc2a8 100644 --- a/server/src/services/media.service.ts +++ b/server/src/services/media.service.ts @@ -27,7 +27,13 @@ import { JobStatus, QueueName, } from 'src/interfaces/job.interface'; -import { AudioStreamInfo, TranscodeCommand, VideoFormat, VideoStreamInfo } from 'src/interfaces/media.interface'; +import { + AudioStreamInfo, + DecodeToBufferOptions, + TranscodeCommand, + VideoFormat, + VideoStreamInfo, +} from 'src/interfaces/media.interface'; import { BaseService } from 'src/services/base.service'; import { getAssetFiles } from 'src/utils/asset.util'; import { BaseConfig, ThumbnailConfig } from 'src/utils/media'; @@ -130,7 +136,7 @@ export class MediaService extends BaseService { return JobStatus.FAILED; } - await this.storageCore.moveAssetImage(asset, AssetPathType.EXTRACTED, ImageFormat.JPEG); + await this.storageCore.moveAssetImage(asset, AssetPathType.CONVERTED, ImageFormat.JPEG); await this.storageCore.moveAssetImage(asset, AssetPathType.PREVIEW, image.preview.format); await this.storageCore.moveAssetImage(asset, AssetPathType.THUMBNAIL, image.thumbnail.format); await this.storageCore.moveAssetVideo(asset); @@ -154,7 +160,7 @@ export class MediaService extends BaseService { let generated: { previewPath: string; thumbnailPath: string; - extractedPath?: string; + convertedPath?: string; thumbhash: Buffer; }; if (asset.type === AssetType.VIDEO || asset.originalFileName.toLowerCase().endsWith('.gif')) { @@ -166,7 +172,7 @@ export class MediaService extends BaseService { return JobStatus.SKIPPED; } - const { previewFile, thumbnailFile, extractedFile } = getAssetFiles(asset.files); + const { previewFile, thumbnailFile, convertedFile } = getAssetFiles(asset.files); const toUpsert: UpsertFileOptions[] = []; if (previewFile?.path !== generated.previewPath) { toUpsert.push({ assetId: asset.id, path: generated.previewPath, type: AssetFileType.PREVIEW }); @@ -176,8 +182,8 @@ export class MediaService extends BaseService { toUpsert.push({ assetId: asset.id, path: generated.thumbnailPath, type: AssetFileType.THUMBNAIL }); } - if (generated.extractedPath && extractedFile?.path !== generated.extractedPath) { - toUpsert.push({ assetId: asset.id, path: generated.extractedPath, type: AssetFileType.EXTRACTED }); + if (generated.convertedPath && convertedFile?.path !== generated.convertedPath) { + toUpsert.push({ assetId: asset.id, path: generated.convertedPath, type: AssetFileType.CONVERTED }); } if (toUpsert.length > 0) { @@ -195,18 +201,13 @@ export class MediaService extends BaseService { pathsToDelete.push(thumbnailFile.path); } - if (extractedFile && extractedFile.path !== generated.extractedPath) { + if (convertedFile && convertedFile.path !== generated.convertedPath) { this.logger.debug(`Deleting old extracted image for asset ${asset.id}`); - pathsToDelete.push(extractedFile.path); + pathsToDelete.push(convertedFile.path); } if (pathsToDelete.length > 0) { - await Promise.all( - pathsToDelete.map(async (path) => { - await this.storageRepository.unlink(path); - await this.assetRepository.removeAssetFile(path); - }), - ); + await Promise.all(pathsToDelete.map((path) => this.storageRepository.unlink(path))); } if (asset.thumbhash != generated.thumbhash) { @@ -226,48 +227,57 @@ export class MediaService extends BaseService { const processInvalidImages = process.env.IMMICH_PROCESS_INVALID_IMAGES === 'true'; const colorspace = this.isSRGB(asset) ? Colorspace.SRGB : image.colorspace; - const decodeOptions = { colorspace, processInvalidImages, size: image.preview.size }; + const imageIsRaw = mimeTypes.isRaw(asset.originalPath); + const decodeOptions: DecodeToBufferOptions = { + colorspace, + processInvalidImages, + size: image.preview.size, + }; - let fullsizePath: string; - let extractedPath: string | undefined; - if (mimeTypes.isRaw(asset.originalPath)) { + let fullsizePath: string = asset.originalPath; + /** + * Converted or extracted image from RAW + */ + let convertedPath: string | undefined; + let shouldConvertFromRaw = false; + if (imageIsRaw) { let useExtracted = false; - extractedPath = StorageCore.getImagePath(asset, AssetPathType.EXTRACTED, ImageFormat.JPEG); + // extracted image from RAW is always in JPEG format, as implied from the `jpgFromRaw` tag name + convertedPath = StorageCore.getImagePath(asset, AssetPathType.CONVERTED, ImageFormat.JPEG); if (image.extractEmbedded) { // try extracting embedded preview from RAW - // extracted image from RAW is always in JPEG format, as implied from the `jpgFromRaw` tag name - const didExtract = await this.mediaRepository.extract(asset.originalPath, extractedPath); - useExtracted = didExtract && (await this.shouldUseExtractedImage(extractedPath, image.preview.size)); + const didExtract = await this.mediaRepository.extract(asset.originalPath, convertedPath); + useExtracted = didExtract && (await this.shouldUseExtractedImage(convertedPath, image.preview.size)); } if (useExtracted) { - fullsizePath = extractedPath; + fullsizePath = convertedPath; + // proper orientation metadata is missing in extracted images, specify separately + // TODO: remove this when proper EXIF is included in extracted images + decodeOptions.orientation = asset.exifInfo?.orientation ? Number(asset.exifInfo.orientation) : undefined; } else { // did not extract or extracted preview is smaller than target size, // convert a full-sized thumbnail from original instead - extractedPath = StorageCore.getImagePath(asset, AssetPathType.EXTRACTED, image.preview.format); - // const orientation = asset.exifInfo?.orientation ? Number(asset.exifInfo.orientation) : undefined; - await this.mediaRepository.generateThumbnail( - asset.originalPath, - { ...image.preview, colorspace, processInvalidImages, size: Infinity }, - extractedPath, - ); + convertedPath = StorageCore.getImagePath(asset, AssetPathType.CONVERTED, image.preview.format); + // unset size to disable resizing + decodeOptions.size = undefined; + shouldConvertFromRaw = true; } - fullsizePath = extractedPath; - } else { - fullsizePath = asset.originalPath; } const { info, data } = await this.mediaRepository.decodeImage(fullsizePath, decodeOptions); const thumbnailOptions = { colorspace, processInvalidImages, raw: info }; const outputs = await Promise.all([ + this.mediaRepository.generateThumbhash(data, thumbnailOptions), this.mediaRepository.generateThumbnail(data, { ...image.thumbnail, ...thumbnailOptions }, thumbnailPath), this.mediaRepository.generateThumbnail(data, { ...image.preview, ...thumbnailOptions }, previewPath), - this.mediaRepository.generateThumbhash(data, thumbnailOptions), + shouldConvertFromRaw && convertedPath + ? this.mediaRepository.generateThumbnail(data, { ...image.preview, ...thumbnailOptions }, convertedPath) + : undefined, ]); - return { previewPath, thumbnailPath, extractedPath, thumbhash: outputs[2] }; + return { previewPath, thumbnailPath, convertedPath, thumbhash: outputs[0] }; } private async generateVideoThumbnails(asset: AssetEntity) { diff --git a/server/src/utils/asset.util.ts b/server/src/utils/asset.util.ts index 36ee3249735501..a75599cecfe2a7 100644 --- a/server/src/utils/asset.util.ts +++ b/server/src/utils/asset.util.ts @@ -25,7 +25,7 @@ const getFileByType = (files: AssetFileEntity[] | undefined, type: AssetFileType }; export const getAssetFiles = (files?: AssetFileEntity[]) => ({ - extractedFile: getFileByType(files, AssetFileType.EXTRACTED), + convertedFile: getFileByType(files, AssetFileType.CONVERTED), previewFile: getFileByType(files, AssetFileType.PREVIEW), thumbnailFile: getFileByType(files, AssetFileType.THUMBNAIL), }); diff --git a/server/test/repositories/asset.repository.mock.ts b/server/test/repositories/asset.repository.mock.ts index 4d399a8af61207..928a7956c5f0c6 100644 --- a/server/test/repositories/asset.repository.mock.ts +++ b/server/test/repositories/asset.repository.mock.ts @@ -28,7 +28,6 @@ export const newAssetRepositoryMock = (): Mocked => { deleteAll: vitest.fn(), update: vitest.fn(), remove: vitest.fn(), - removeAssetFile: vitest.fn(), findLivePhotoMatch: vitest.fn(), getStatistics: vitest.fn(), getTimeBucket: vitest.fn(), diff --git a/web/src/lib/components/asset-viewer/photo-viewer.svelte b/web/src/lib/components/asset-viewer/photo-viewer.svelte index 1f8a778a90e341..2fca3e4eb37f63 100644 --- a/web/src/lib/components/asset-viewer/photo-viewer.svelte +++ b/web/src/lib/components/asset-viewer/photo-viewer.svelte @@ -149,7 +149,7 @@ }); let isWebCompatible = $derived(isWebCompatibleImage(asset)); - // RAW files may have corresponding extracted JPEGs + // RAW files may have corresponding converted web-compatible original-sized files let isRaw = $derived(isRawImage(asset)); let useOriginalByDefault = $derived(isWebCompatible && $alwaysLoadOriginalFile);