From 3432b4625f8c6d71a260d2c90cd2b8b4c5ff8eb4 Mon Sep 17 00:00:00 2001 From: Daniele Ricci Date: Fri, 8 Sep 2023 08:49:43 +0200 Subject: [PATCH] fix(server): regenerate missing person thumbnails (#3970) * Regenerate missing person thumbnails * Check for empty string instead of zero length * Remember asset used as person face * Define entity relation between person and asset via faceAssetId * Typo * Fix entity relation * Tests * Tests * Fix code formatting * Fix import path * Fix migration * format * Fix entity and migration * Linting * Remove unneeded cast * Conventions * Simplify queries * Simplify queries * Remove unneeded typings from entity * Remove unneeded cast --------- Co-authored-by: Alex --- .../facial-recognition.service.spec.ts | 1 + .../facial-recognition.services.ts | 2 +- server/src/domain/job/job.constants.ts | 4 +- server/src/domain/media/media.service.spec.ts | 74 ++++++++++++++++++- server/src/domain/media/media.service.ts | 29 ++++++++ server/src/domain/person/person.repository.ts | 5 +- .../src/domain/person/person.service.spec.ts | 21 ++++-- server/src/domain/person/person.service.ts | 2 +- server/src/infra/entities/person.entity.ts | 7 ++ .../1693833336881-AddPersonFaceAssetId.ts | 15 ++++ .../infra/repositories/person.repository.ts | 14 +++- server/test/fixtures/person.stub.ts | 19 +++++ .../repositories/person.repository.mock.ts | 3 + 13 files changed, 183 insertions(+), 13 deletions(-) create mode 100644 server/src/infra/migrations/1693833336881-AddPersonFaceAssetId.ts diff --git a/server/src/domain/facial-recognition/facial-recognition.service.spec.ts b/server/src/domain/facial-recognition/facial-recognition.service.spec.ts index 1ca3ed89b9..9621f9e86f 100644 --- a/server/src/domain/facial-recognition/facial-recognition.service.spec.ts +++ b/server/src/domain/facial-recognition/facial-recognition.service.spec.ts @@ -296,6 +296,7 @@ describe(FacialRecognitionService.name, () => { colorspace: Colorspace.P3, }); expect(personMock.update).toHaveBeenCalledWith({ + faceAssetId: 'asset-1', id: 'person-1', thumbnailPath: 'upload/thumbs/user-id/person-1.jpeg', }); diff --git a/server/src/domain/facial-recognition/facial-recognition.services.ts b/server/src/domain/facial-recognition/facial-recognition.services.ts index 60d3503928..2e94273ce0 100644 --- a/server/src/domain/facial-recognition/facial-recognition.services.ts +++ b/server/src/domain/facial-recognition/facial-recognition.services.ts @@ -171,7 +171,7 @@ export class FacialRecognitionService { quality: thumbnail.quality, } as const; await this.mediaRepository.resize(croppedOutput, output, thumbnailOptions); - await this.personRepository.update({ id: personId, thumbnailPath: output }); + await this.personRepository.update({ id: personId, thumbnailPath: output, faceAssetId: data.assetId }); return true; } diff --git a/server/src/domain/job/job.constants.ts b/server/src/domain/job/job.constants.ts index 7062ab86b6..4342911d9e 100644 --- a/server/src/domain/job/job.constants.ts +++ b/server/src/domain/job/job.constants.ts @@ -28,6 +28,7 @@ export enum JobName { GENERATE_JPEG_THUMBNAIL = 'generate-jpeg-thumbnail', GENERATE_WEBP_THUMBNAIL = 'generate-webp-thumbnail', GENERATE_THUMBHASH_THUMBNAIL = 'generate-thumbhash-thumbnail', + GENERATE_FACE_THUMBNAIL = 'generate-face-thumbnail', // metadata QUEUE_METADATA_EXTRACTION = 'queue-metadata-extraction', @@ -50,7 +51,6 @@ export enum JobName { // facial recognition QUEUE_RECOGNIZE_FACES = 'queue-recognize-faces', RECOGNIZE_FACES = 'recognize-faces', - GENERATE_FACE_THUMBNAIL = 'generate-face-thumbnail', PERSON_CLEANUP = 'person-cleanup', // cleanup @@ -97,6 +97,7 @@ export const JOBS_TO_QUEUE: Record = { [JobName.GENERATE_JPEG_THUMBNAIL]: QueueName.THUMBNAIL_GENERATION, [JobName.GENERATE_WEBP_THUMBNAIL]: QueueName.THUMBNAIL_GENERATION, [JobName.GENERATE_THUMBHASH_THUMBNAIL]: QueueName.THUMBNAIL_GENERATION, + [JobName.GENERATE_FACE_THUMBNAIL]: QueueName.THUMBNAIL_GENERATION, // metadata [JobName.QUEUE_METADATA_EXTRACTION]: QueueName.METADATA_EXTRACTION, @@ -115,7 +116,6 @@ export const JOBS_TO_QUEUE: Record = { // facial recognition [JobName.QUEUE_RECOGNIZE_FACES]: QueueName.RECOGNIZE_FACES, [JobName.RECOGNIZE_FACES]: QueueName.RECOGNIZE_FACES, - [JobName.GENERATE_FACE_THUMBNAIL]: QueueName.RECOGNIZE_FACES, // clip [JobName.QUEUE_ENCODE_CLIP]: QueueName.CLIP_ENCODING, diff --git a/server/src/domain/media/media.service.spec.ts b/server/src/domain/media/media.service.spec.ts index 35ba91b2f2..fe280cad22 100644 --- a/server/src/domain/media/media.service.spec.ts +++ b/server/src/domain/media/media.service.spec.ts @@ -9,15 +9,19 @@ import { } from '@app/infra/entities'; import { assetStub, + faceStub, newAssetRepositoryMock, newJobRepositoryMock, newMediaRepositoryMock, + newPersonRepositoryMock, newStorageRepositoryMock, newSystemConfigRepositoryMock, + personStub, probeStub, } from '@test'; import { IAssetRepository, WithoutProperty } from '../asset'; import { IJobRepository, JobName } from '../job'; +import { IPersonRepository } from '../person'; import { IStorageRepository } from '../storage'; import { ISystemConfigRepository } from '../system-config'; import { IMediaRepository } from './media.repository'; @@ -29,6 +33,7 @@ describe(MediaService.name, () => { let configMock: jest.Mocked; let jobMock: jest.Mocked; let mediaMock: jest.Mocked; + let personMock: jest.Mocked; let storageMock: jest.Mocked; beforeEach(async () => { @@ -36,9 +41,10 @@ describe(MediaService.name, () => { configMock = newSystemConfigRepositoryMock(); jobMock = newJobRepositoryMock(); mediaMock = newMediaRepositoryMock(); + personMock = newPersonRepositoryMock(); storageMock = newStorageRepositoryMock(); - sut = new MediaService(assetMock, jobMock, mediaMock, storageMock, configMock); + sut = new MediaService(assetMock, personMock, jobMock, mediaMock, storageMock, configMock); }); it('should be defined', () => { @@ -51,6 +57,8 @@ describe(MediaService.name, () => { items: [assetStub.image], hasNextPage: false, }); + personMock.getAll.mockResolvedValue([personStub.newThumbnail]); + personMock.getFaceById.mockResolvedValue(faceStub.face1); await sut.handleQueueGenerateThumbnails({ force: true }); @@ -60,6 +68,57 @@ describe(MediaService.name, () => { name: JobName.GENERATE_JPEG_THUMBNAIL, data: { id: assetStub.image.id }, }); + + expect(personMock.getAll).toHaveBeenCalled(); + expect(personMock.getAllWithoutThumbnail).not.toHaveBeenCalled(); + expect(jobMock.queue).toHaveBeenCalledWith({ + name: JobName.GENERATE_FACE_THUMBNAIL, + data: { + imageWidth: faceStub.face1.imageWidth, + imageHeight: faceStub.face1.imageHeight, + boundingBox: { + x1: faceStub.face1.boundingBoxX1, + x2: faceStub.face1.boundingBoxX2, + y1: faceStub.face1.boundingBoxY1, + y2: faceStub.face1.boundingBoxY2, + }, + assetId: faceStub.face1.assetId, + personId: personStub.newThumbnail.id, + }, + }); + }); + + it('should queue all people with missing thumbnail path', async () => { + assetMock.getWithout.mockResolvedValue({ + items: [assetStub.image], + hasNextPage: false, + }); + personMock.getAllWithoutThumbnail.mockResolvedValue([personStub.noThumbnail]); + personMock.getRandomFace.mockResolvedValue(faceStub.face1); + + await sut.handleQueueGenerateThumbnails({ force: false }); + + expect(assetMock.getAll).not.toHaveBeenCalled(); + expect(assetMock.getWithout).toHaveBeenCalledWith({ skip: 0, take: 1000 }, WithoutProperty.THUMBNAIL); + + expect(personMock.getAll).not.toHaveBeenCalled(); + expect(personMock.getAllWithoutThumbnail).toHaveBeenCalled(); + expect(personMock.getRandomFace).toHaveBeenCalled(); + expect(jobMock.queue).toHaveBeenCalledWith({ + name: JobName.GENERATE_FACE_THUMBNAIL, + data: { + imageWidth: faceStub.face1.imageWidth, + imageHeight: faceStub.face1.imageHeight, + boundingBox: { + x1: faceStub.face1.boundingBoxX1, + x2: faceStub.face1.boundingBoxX2, + y1: faceStub.face1.boundingBoxY1, + y2: faceStub.face1.boundingBoxY2, + }, + assetId: faceStub.face1.assetId, + personId: personStub.newThumbnail.id, + }, + }); }); it('should queue all assets with missing resize path', async () => { @@ -67,6 +126,7 @@ describe(MediaService.name, () => { items: [assetStub.noResizePath], hasNextPage: false, }); + personMock.getAllWithoutThumbnail.mockResolvedValue([]); await sut.handleQueueGenerateThumbnails({ force: false }); @@ -76,6 +136,9 @@ describe(MediaService.name, () => { name: JobName.GENERATE_JPEG_THUMBNAIL, data: { id: assetStub.image.id }, }); + + expect(personMock.getAll).not.toHaveBeenCalled(); + expect(personMock.getAllWithoutThumbnail).toHaveBeenCalled(); }); it('should queue all assets with missing webp path', async () => { @@ -83,6 +146,7 @@ describe(MediaService.name, () => { items: [assetStub.noWebpPath], hasNextPage: false, }); + personMock.getAllWithoutThumbnail.mockResolvedValue([]); await sut.handleQueueGenerateThumbnails({ force: false }); @@ -92,6 +156,9 @@ describe(MediaService.name, () => { name: JobName.GENERATE_WEBP_THUMBNAIL, data: { id: assetStub.image.id }, }); + + expect(personMock.getAll).not.toHaveBeenCalled(); + expect(personMock.getAllWithoutThumbnail).toHaveBeenCalled(); }); it('should queue all assets with missing thumbhash', async () => { @@ -99,6 +166,7 @@ describe(MediaService.name, () => { items: [assetStub.noThumbhash], hasNextPage: false, }); + personMock.getAllWithoutThumbnail.mockResolvedValue([]); await sut.handleQueueGenerateThumbnails({ force: false }); @@ -108,6 +176,9 @@ describe(MediaService.name, () => { name: JobName.GENERATE_THUMBHASH_THUMBNAIL, data: { id: assetStub.image.id }, }); + + expect(personMock.getAll).not.toHaveBeenCalled(); + expect(personMock.getAllWithoutThumbnail).toHaveBeenCalled(); }); }); @@ -245,6 +316,7 @@ describe(MediaService.name, () => { items: [assetStub.video], hasNextPage: false, }); + personMock.getAll.mockResolvedValue([]); await sut.handleQueueVideoConversion({ force: true }); diff --git a/server/src/domain/media/media.service.ts b/server/src/domain/media/media.service.ts index 76e62f72d9..4ff9f0cc50 100644 --- a/server/src/domain/media/media.service.ts +++ b/server/src/domain/media/media.service.ts @@ -4,11 +4,13 @@ import { join } from 'path'; import { IAssetRepository, WithoutProperty } from '../asset'; import { usePagination } from '../domain.util'; import { IBaseJob, IEntityJob, IJobRepository, JOBS_ASSET_PAGINATION_SIZE, JobName } from '../job'; +import { IPersonRepository } from '../person'; import { IStorageRepository, StorageCore, StorageFolder } from '../storage'; import { ISystemConfigRepository, SystemConfigFFmpegDto } from '../system-config'; import { SystemConfigCore } from '../system-config/system-config.core'; import { AudioStreamInfo, IMediaRepository, VideoCodecHWConfig, VideoStreamInfo } from './media.repository'; import { H264Config, HEVCConfig, NVENCConfig, QSVConfig, ThumbnailConfig, VAAPIConfig, VP9Config } from './media.util'; + @Injectable() export class MediaService { private logger = new Logger(MediaService.name); @@ -17,6 +19,7 @@ export class MediaService { constructor( @Inject(IAssetRepository) private assetRepository: IAssetRepository, + @Inject(IPersonRepository) private personRepository: IPersonRepository, @Inject(IJobRepository) private jobRepository: IJobRepository, @Inject(IMediaRepository) private mediaRepository: IMediaRepository, @Inject(IStorageRepository) private storageRepository: IStorageRepository, @@ -49,6 +52,32 @@ export class MediaService { } } + const people = force ? await this.personRepository.getAll() : await this.personRepository.getAllWithoutThumbnail(); + + for (const person of people) { + // use stored asset for generating thumbnail or pick a random one if not present + const face = person.faceAssetId + ? await this.personRepository.getFaceById({ personId: person.id, assetId: person.faceAssetId }) + : await this.personRepository.getRandomFace(person.id); + if (face) { + await this.jobRepository.queue({ + name: JobName.GENERATE_FACE_THUMBNAIL, + data: { + imageWidth: face.imageWidth, + imageHeight: face.imageHeight, + boundingBox: { + x1: face.boundingBoxX1, + x2: face.boundingBoxX2, + y1: face.boundingBoxY1, + y2: face.boundingBoxY2, + }, + assetId: face.assetId, + personId: person.id, + }, + }); + } + } + return true; } diff --git a/server/src/domain/person/person.repository.ts b/server/src/domain/person/person.repository.ts index 973d940cd6..30315eb69c 100644 --- a/server/src/domain/person/person.repository.ts +++ b/server/src/domain/person/person.repository.ts @@ -13,7 +13,9 @@ export interface UpdateFacesData { } export interface IPersonRepository { - getAll(userId: string, options: PersonSearchOptions): Promise; + getAll(): Promise; + getAllWithoutThumbnail(): Promise; + getAllForUser(userId: string, options: PersonSearchOptions): Promise; getAllWithoutFaces(): Promise; getById(userId: string, personId: string): Promise; @@ -27,4 +29,5 @@ export interface IPersonRepository { deleteAll(): Promise; getFaceById(payload: AssetFaceId): Promise; + getRandomFace(personId: string): Promise; } diff --git a/server/src/domain/person/person.service.spec.ts b/server/src/domain/person/person.service.spec.ts index b75bea23f8..03ba41a9b7 100644 --- a/server/src/domain/person/person.service.spec.ts +++ b/server/src/domain/person/person.service.spec.ts @@ -42,25 +42,31 @@ describe(PersonService.name, () => { describe('getAll', () => { it('should get all people with thumbnails', async () => { - personMock.getAll.mockResolvedValue([personStub.withName, personStub.noThumbnail]); + personMock.getAllForUser.mockResolvedValue([personStub.withName, personStub.noThumbnail]); await expect(sut.getAll(authStub.admin, { withHidden: undefined })).resolves.toEqual({ total: 1, visible: 1, people: [responseDto], }); - expect(personMock.getAll).toHaveBeenCalledWith(authStub.admin.id, { minimumFaceCount: 1, withHidden: false }); + expect(personMock.getAllForUser).toHaveBeenCalledWith(authStub.admin.id, { + minimumFaceCount: 1, + withHidden: false, + }); }); it('should get all visible people with thumbnails', async () => { - personMock.getAll.mockResolvedValue([personStub.withName, personStub.hidden]); + personMock.getAllForUser.mockResolvedValue([personStub.withName, personStub.hidden]); await expect(sut.getAll(authStub.admin, { withHidden: false })).resolves.toEqual({ total: 2, visible: 1, people: [responseDto], }); - expect(personMock.getAll).toHaveBeenCalledWith(authStub.admin.id, { minimumFaceCount: 1, withHidden: false }); + expect(personMock.getAllForUser).toHaveBeenCalledWith(authStub.admin.id, { + minimumFaceCount: 1, + withHidden: false, + }); }); it('should get all hidden and visible people with thumbnails', async () => { - personMock.getAll.mockResolvedValue([personStub.withName, personStub.hidden]); + personMock.getAllForUser.mockResolvedValue([personStub.withName, personStub.hidden]); await expect(sut.getAll(authStub.admin, { withHidden: true })).resolves.toEqual({ total: 2, visible: 1, @@ -75,7 +81,10 @@ describe(PersonService.name, () => { }, ], }); - expect(personMock.getAll).toHaveBeenCalledWith(authStub.admin.id, { minimumFaceCount: 1, withHidden: true }); + expect(personMock.getAllForUser).toHaveBeenCalledWith(authStub.admin.id, { + minimumFaceCount: 1, + withHidden: true, + }); }); }); diff --git a/server/src/domain/person/person.service.ts b/server/src/domain/person/person.service.ts index 69442a1110..ac814d85de 100644 --- a/server/src/domain/person/person.service.ts +++ b/server/src/domain/person/person.service.ts @@ -26,7 +26,7 @@ export class PersonService { ) {} async getAll(authUser: AuthUserDto, dto: PersonSearchDto): Promise { - const people = await this.repository.getAll(authUser.id, { + const people = await this.repository.getAllForUser(authUser.id, { minimumFaceCount: 1, withHidden: dto.withHidden || false, }); diff --git a/server/src/infra/entities/person.entity.ts b/server/src/infra/entities/person.entity.ts index b0da2f63da..2735204e64 100644 --- a/server/src/infra/entities/person.entity.ts +++ b/server/src/infra/entities/person.entity.ts @@ -8,6 +8,7 @@ import { UpdateDateColumn, } from 'typeorm'; import { AssetFaceEntity } from './asset-face.entity'; +import { AssetEntity } from './asset.entity'; import { UserEntity } from './user.entity'; @Entity('person') @@ -36,6 +37,12 @@ export class PersonEntity { @Column({ default: '' }) thumbnailPath!: string; + @Column({ nullable: true }) + faceAssetId!: string | null; + + @ManyToOne(() => AssetEntity, { onDelete: 'SET NULL', nullable: true }) + faceAsset!: AssetEntity | null; + @OneToMany(() => AssetFaceEntity, (assetFace) => assetFace.person) faces!: AssetFaceEntity[]; diff --git a/server/src/infra/migrations/1693833336881-AddPersonFaceAssetId.ts b/server/src/infra/migrations/1693833336881-AddPersonFaceAssetId.ts new file mode 100644 index 0000000000..2e3c914eec --- /dev/null +++ b/server/src/infra/migrations/1693833336881-AddPersonFaceAssetId.ts @@ -0,0 +1,15 @@ +import { MigrationInterface, QueryRunner } from "typeorm" + +export class AddPersonFaceAssetId1693833336881 implements MigrationInterface { + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(`ALTER TABLE "person" ADD "faceAssetId" uuid`); + await queryRunner.query(`ALTER TABLE "person" ADD CONSTRAINT "FK_2bbabe31656b6778c6b87b61023" FOREIGN KEY ("faceAssetId") REFERENCES "assets"("id") ON DELETE SET NULL ON UPDATE NO ACTION`); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(`ALTER TABLE "person" DROP CONSTRAINT "FK_2bbabe31656b6778c6b87b61023"`); + await queryRunner.query(`ALTER TABLE "person" DROP COLUMN "faceAssetId"`); + } + +} diff --git a/server/src/infra/repositories/person.repository.ts b/server/src/infra/repositories/person.repository.ts index 1203bba381..edfc1357f4 100644 --- a/server/src/infra/repositories/person.repository.ts +++ b/server/src/infra/repositories/person.repository.ts @@ -50,7 +50,15 @@ export class PersonRepository implements IPersonRepository { return people.length; } - getAll(userId: string, options?: PersonSearchOptions): Promise { + getAll(): Promise { + return this.personRepository.find(); + } + + getAllWithoutThumbnail(): Promise { + return this.personRepository.findBy({ thumbnailPath: '' }); + } + + getAllForUser(userId: string, options?: PersonSearchOptions): Promise { const queryBuilder = this.personRepository .createQueryBuilder('person') .leftJoin('person.faces', 'face') @@ -118,4 +126,8 @@ export class PersonRepository implements IPersonRepository { async getFaceById({ personId, assetId }: AssetFaceId): Promise { return this.assetFaceRepository.findOneBy({ assetId, personId }); } + + async getRandomFace(personId: string): Promise { + return this.assetFaceRepository.findOneBy({ personId }); + } } diff --git a/server/test/fixtures/person.stub.ts b/server/test/fixtures/person.stub.ts index 2d419425d0..d6e5b741df 100644 --- a/server/test/fixtures/person.stub.ts +++ b/server/test/fixtures/person.stub.ts @@ -1,4 +1,5 @@ import { PersonEntity } from '@app/infra/entities'; +import { assetStub } from '@test/fixtures/asset.stub'; import { userStub } from './user.stub'; export const personStub = { @@ -12,6 +13,8 @@ export const personStub = { birthDate: null, thumbnailPath: '/path/to/thumbnail.jpg', faces: [], + faceAssetId: null, + faceAsset: null, isHidden: false, }), hidden: Object.freeze({ @@ -24,6 +27,8 @@ export const personStub = { birthDate: null, thumbnailPath: '/path/to/thumbnail.jpg', faces: [], + faceAssetId: null, + faceAsset: null, isHidden: true, }), withName: Object.freeze({ @@ -36,6 +41,8 @@ export const personStub = { birthDate: null, thumbnailPath: '/path/to/thumbnail.jpg', faces: [], + faceAssetId: null, + faceAsset: null, isHidden: false, }), noBirthDate: Object.freeze({ @@ -48,6 +55,8 @@ export const personStub = { birthDate: null, thumbnailPath: '/path/to/thumbnail.jpg', faces: [], + faceAssetId: null, + faceAsset: null, isHidden: false, }), withBirthDate: Object.freeze({ @@ -60,6 +69,8 @@ export const personStub = { birthDate: new Date('1976-06-30'), thumbnailPath: '/path/to/thumbnail.jpg', faces: [], + faceAssetId: null, + faceAsset: null, isHidden: false, }), noThumbnail: Object.freeze({ @@ -72,6 +83,8 @@ export const personStub = { birthDate: null, thumbnailPath: '', faces: [], + faceAssetId: null, + faceAsset: null, isHidden: false, }), newThumbnail: Object.freeze({ @@ -84,6 +97,8 @@ export const personStub = { birthDate: null, thumbnailPath: '/new/path/to/thumbnail.jpg', faces: [], + faceAssetId: assetStub.image.id, + faceAsset: assetStub.image, isHidden: false, }), primaryPerson: Object.freeze({ @@ -96,6 +111,8 @@ export const personStub = { birthDate: null, thumbnailPath: '/path/to/thumbnail', faces: [], + faceAssetId: null, + faceAsset: null, isHidden: false, }), mergePerson: Object.freeze({ @@ -108,6 +125,8 @@ export const personStub = { birthDate: null, thumbnailPath: '/path/to/thumbnail', faces: [], + faceAssetId: null, + faceAsset: null, isHidden: false, }), }; diff --git a/server/test/repositories/person.repository.mock.ts b/server/test/repositories/person.repository.mock.ts index 99fa6de3ed..5dfc6b565c 100644 --- a/server/test/repositories/person.repository.mock.ts +++ b/server/test/repositories/person.repository.mock.ts @@ -4,6 +4,8 @@ export const newPersonRepositoryMock = (): jest.Mocked => { return { getById: jest.fn(), getAll: jest.fn(), + getAllWithoutThumbnail: jest.fn(), + getAllForUser: jest.fn(), getAssets: jest.fn(), getAllWithoutFaces: jest.fn(), @@ -13,6 +15,7 @@ export const newPersonRepositoryMock = (): jest.Mocked => { delete: jest.fn(), getFaceById: jest.fn(), + getRandomFace: jest.fn(), prepareReassignFaces: jest.fn(), reassignFaces: jest.fn(), };