From 01a9cda15dcc5440f0f9ce647c8a42d38ef6e5dc Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Wed, 16 Oct 2024 18:20:44 -0400 Subject: [PATCH] fix(server): never try to parse Duration from exif data (#13497) --- server/src/services/metadata.service.spec.ts | 114 ++++++++++--------- server/src/services/metadata.service.ts | 6 +- 2 files changed, 66 insertions(+), 54 deletions(-) diff --git a/server/src/services/metadata.service.spec.ts b/server/src/services/metadata.service.spec.ts index cd7f68ab1dd8c..cc6eae6e3b511 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -44,6 +44,12 @@ describe(MetadataService.name, () => { let tagMock: Mocked; let userMock: Mocked; + const mockReadTags = (exifData?: Partial, sidecarData?: Partial) => { + metadataMock.readTags.mockReset(); + metadataMock.readTags.mockResolvedValueOnce(exifData ?? {}); + metadataMock.readTags.mockResolvedValueOnce(sidecarData ?? {}); + }; + beforeEach(() => { ({ sut, @@ -62,6 +68,8 @@ describe(MetadataService.name, () => { userMock, } = newTestService(MetadataService)); + mockReadTags(); + delete process.env.TZ; }); @@ -258,13 +266,7 @@ describe(MetadataService.name, () => { const originalDate = new Date('2023-11-21T16:13:17.517Z'); const sidecarDate = new Date('2022-01-01T00:00:00.000Z'); assetMock.getByIds.mockResolvedValue([assetStub.sidecar]); - metadataMock.readTags.mockImplementation((path) => { - const map = { - [assetStub.sidecar.originalPath]: originalDate.toISOString(), - [assetStub.sidecar.sidecarPath as string]: sidecarDate.toISOString(), - }; - return Promise.resolve({ CreationDate: map[path] ?? new Date().toISOString() }); - }); + mockReadTags({ CreationDate: originalDate.toISOString() }, { CreationDate: sidecarDate.toISOString() }); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.sidecar.id], { faces: { person: false } }); @@ -280,9 +282,7 @@ describe(MetadataService.name, () => { it('should account for the server being in a non-UTC timezone', async () => { process.env.TZ = 'America/Los_Angeles'; assetMock.getByIds.mockResolvedValue([assetStub.sidecar]); - metadataMock.readTags.mockResolvedValueOnce({ - DateTimeOriginal: '2022:01:01 00:00:00', - }); + mockReadTags({ DateTimeOriginal: '2022:01:01 00:00:00' }); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.upsertExif).toHaveBeenCalledWith( @@ -300,7 +300,7 @@ describe(MetadataService.name, () => { it('should handle lists of numbers', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ ISO: [160] }); + mockReadTags({ ISO: [160] }); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id], { faces: { person: false } }); @@ -317,7 +317,7 @@ describe(MetadataService.name, () => { assetMock.getByIds.mockResolvedValue([assetStub.withLocation]); systemMock.get.mockResolvedValue({ reverseGeocoding: { enabled: true } }); mapMock.reverseGeocode.mockResolvedValue({ city: 'City', state: 'State', country: 'Country' }); - metadataMock.readTags.mockResolvedValue({ + mockReadTags({ GPSLatitude: assetStub.withLocation.exifInfo!.latitude!, GPSLongitude: assetStub.withLocation.exifInfo!.longitude!, }); @@ -337,7 +337,7 @@ describe(MetadataService.name, () => { it('should discard latitude and longitude on null island', async () => { assetMock.getByIds.mockResolvedValue([assetStub.withLocation]); - metadataMock.readTags.mockResolvedValue({ + mockReadTags({ GPSLatitude: 0, GPSLongitude: 0, }); @@ -349,7 +349,7 @@ describe(MetadataService.name, () => { it('should extract tags from TagsList', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ TagsList: ['Parent'] }); + mockReadTags({ TagsList: ['Parent'] }); tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -359,7 +359,7 @@ describe(MetadataService.name, () => { it('should extract hierarchy from TagsList', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ TagsList: ['Parent/Child'] }); + mockReadTags({ TagsList: ['Parent/Child'] }); tagMock.upsertValue.mockResolvedValueOnce(tagStub.parent); tagMock.upsertValue.mockResolvedValueOnce(tagStub.child); @@ -375,7 +375,7 @@ describe(MetadataService.name, () => { it('should extract tags from Keywords as a string', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ Keywords: 'Parent' }); + mockReadTags({ Keywords: 'Parent' }); tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -385,7 +385,7 @@ describe(MetadataService.name, () => { it('should extract tags from Keywords as a list', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ Keywords: ['Parent'] }); + mockReadTags({ Keywords: ['Parent'] }); tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -395,7 +395,7 @@ describe(MetadataService.name, () => { it('should extract tags from Keywords as a list with a number', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ Keywords: ['Parent', 2024] }); + mockReadTags({ Keywords: ['Parent', 2024] }); tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -406,7 +406,7 @@ describe(MetadataService.name, () => { it('should extract hierarchal tags from Keywords', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ Keywords: 'Parent/Child' }); + mockReadTags({ Keywords: 'Parent/Child' }); tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -421,7 +421,7 @@ describe(MetadataService.name, () => { it('should ignore Keywords when TagsList is present', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ Keywords: 'Child', TagsList: ['Parent/Child'] }); + mockReadTags({ Keywords: 'Child', TagsList: ['Parent/Child'] }); tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -436,7 +436,7 @@ describe(MetadataService.name, () => { it('should extract hierarchy from HierarchicalSubject', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ HierarchicalSubject: ['Parent|Child', 'TagA'] }); + mockReadTags({ HierarchicalSubject: ['Parent|Child', 'TagA'] }); tagMock.upsertValue.mockResolvedValueOnce(tagStub.parent); tagMock.upsertValue.mockResolvedValueOnce(tagStub.child); @@ -453,7 +453,7 @@ describe(MetadataService.name, () => { it('should extract tags from HierarchicalSubject as a list with a number', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ HierarchicalSubject: ['Parent', 2024] }); + mockReadTags({ HierarchicalSubject: ['Parent', 2024] }); tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -464,7 +464,7 @@ describe(MetadataService.name, () => { it('should extract ignore / characters in a HierarchicalSubject tag', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ HierarchicalSubject: ['Mom/Dad'] }); + mockReadTags({ HierarchicalSubject: ['Mom/Dad'] }); tagMock.upsertValue.mockResolvedValueOnce(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -478,7 +478,7 @@ describe(MetadataService.name, () => { it('should ignore HierarchicalSubject when TagsList is present', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ HierarchicalSubject: ['Parent2|Child2'], TagsList: ['Parent/Child'] }); + mockReadTags({ HierarchicalSubject: ['Parent2|Child2'], TagsList: ['Parent/Child'] }); tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -493,7 +493,7 @@ describe(MetadataService.name, () => { it('should remove existing tags', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({}); + mockReadTags({}); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -518,7 +518,7 @@ describe(MetadataService.name, () => { it('should handle an invalid Directory Item', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ + mockReadTags({ MotionPhoto: 1, ContainerDirectory: [{ Foo: 100 }], }); @@ -529,7 +529,7 @@ describe(MetadataService.name, () => { it('should extract the correct video orientation', async () => { assetMock.getByIds.mockResolvedValue([assetStub.video]); mediaMock.probe.mockResolvedValue(probeStub.videoStreamVertical2160p); - metadataMock.readTags.mockResolvedValue({}); + mockReadTags({}); await sut.handleMetadataExtraction({ id: assetStub.video.id }); @@ -541,7 +541,7 @@ describe(MetadataService.name, () => { it('should extract the MotionPhotoVideo tag from Samsung HEIC motion photos', async () => { assetMock.getByIds.mockResolvedValue([{ ...assetStub.livePhotoWithOriginalFileName, livePhotoVideoId: null }]); - metadataMock.readTags.mockResolvedValue({ + mockReadTags({ Directory: 'foo/bar/', MotionPhotoVideo: new BinaryField(0, ''), // The below two are included to ensure that the MotionPhotoVideo tag is extracted @@ -589,7 +589,7 @@ describe(MetadataService.name, () => { it('should extract the EmbeddedVideo tag from Samsung JPEG motion photos', async () => { assetMock.getByIds.mockResolvedValue([{ ...assetStub.livePhotoWithOriginalFileName, livePhotoVideoId: null }]); - metadataMock.readTags.mockResolvedValue({ + mockReadTags({ Directory: 'foo/bar/', EmbeddedVideoFile: new BinaryField(0, ''), EmbeddedVideoType: 'MotionPhoto_Data', @@ -634,7 +634,7 @@ describe(MetadataService.name, () => { it('should extract the motion photo video from the XMP directory entry ', async () => { assetMock.getByIds.mockResolvedValue([{ ...assetStub.livePhotoWithOriginalFileName, livePhotoVideoId: null }]); - metadataMock.readTags.mockResolvedValue({ + mockReadTags({ Directory: 'foo/bar/', MotionPhoto: 1, MicroVideo: 1, @@ -680,7 +680,7 @@ describe(MetadataService.name, () => { it('should delete old motion photo video assets if they do not match what is extracted', async () => { assetMock.getByIds.mockResolvedValue([assetStub.livePhotoWithOriginalFileName]); - metadataMock.readTags.mockResolvedValue({ + mockReadTags({ Directory: 'foo/bar/', MotionPhoto: 1, MicroVideo: 1, @@ -705,7 +705,7 @@ describe(MetadataService.name, () => { it('should not create a new motion photo video asset if the hash of the extracted video matches an existing asset', async () => { assetMock.getByIds.mockResolvedValue([assetStub.livePhotoStillAsset]); - metadataMock.readTags.mockResolvedValue({ + mockReadTags({ Directory: 'foo/bar/', MotionPhoto: 1, MicroVideo: 1, @@ -727,7 +727,7 @@ describe(MetadataService.name, () => { it('should link and hide motion video asset to still asset if the hash of the extracted video matches an existing asset', async () => { assetMock.getByIds.mockResolvedValue([{ ...assetStub.livePhotoStillAsset, livePhotoVideoId: null }]); - metadataMock.readTags.mockResolvedValue({ + mockReadTags({ Directory: 'foo/bar/', MotionPhoto: 1, MicroVideo: 1, @@ -753,7 +753,7 @@ describe(MetadataService.name, () => { assetMock.getByIds.mockResolvedValue([ { ...assetStub.livePhotoStillAsset, livePhotoVideoId: null, isExternal: true }, ]); - metadataMock.readTags.mockResolvedValue({ + mockReadTags({ Directory: 'foo/bar/', MotionPhoto: 1, MicroVideo: 1, @@ -796,7 +796,7 @@ describe(MetadataService.name, () => { Rating: 3, }; assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue(tags); + mockReadTags(tags); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id], { faces: { person: false } }); @@ -854,7 +854,7 @@ describe(MetadataService.name, () => { tz: undefined, }; assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue(tags); + mockReadTags(tags); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id], { faces: { person: false } }); @@ -887,7 +887,7 @@ describe(MetadataService.name, () => { ); }); - it('only extracts duration for videos', async () => { + it('should only extract duration for videos', async () => { assetMock.getByIds.mockResolvedValue([{ ...assetStub.image }]); mediaMock.probe.mockResolvedValue({ ...probeStub.videoStreamH264, @@ -908,7 +908,7 @@ describe(MetadataService.name, () => { ); }); - it('omits duration of zero', async () => { + it('should omit duration of zero', async () => { assetMock.getByIds.mockResolvedValue([{ ...assetStub.video }]); mediaMock.probe.mockResolvedValue({ ...probeStub.videoStreamH264, @@ -930,7 +930,7 @@ describe(MetadataService.name, () => { ); }); - it('handles duration of 1 week', async () => { + it('should a handle duration of 1 week', async () => { assetMock.getByIds.mockResolvedValue([{ ...assetStub.video }]); mediaMock.probe.mockResolvedValue({ ...probeStub.videoStreamH264, @@ -952,9 +952,17 @@ describe(MetadataService.name, () => { ); }); - it('trims whitespace from description', async () => { + it('should ignore duration from exif data', async () => { + assetMock.getByIds.mockResolvedValue([assetStub.image]); + mockReadTags({}, { Duration: { Value: 123 } }); + + await sut.handleMetadataExtraction({ id: assetStub.image.id }); + expect(assetMock.update).toHaveBeenCalledWith(expect.objectContaining({ duration: null })); + }); + + it('should trim whitespace from description', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ Description: '\t \v \f \n \r' }); + mockReadTags({ Description: '\t \v \f \n \r' }); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.upsertExif).toHaveBeenCalledWith( @@ -963,7 +971,7 @@ describe(MetadataService.name, () => { }), ); - metadataMock.readTags.mockResolvedValue({ ImageDescription: ' my\n description' }); + mockReadTags({ ImageDescription: ' my\n description' }); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.upsertExif).toHaveBeenCalledWith( expect.objectContaining({ @@ -972,9 +980,9 @@ describe(MetadataService.name, () => { ); }); - it('handles a numeric description', async () => { + it('should handle a numeric description', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ Description: 1000 }); + mockReadTags({ Description: 1000 }); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.upsertExif).toHaveBeenCalledWith( @@ -987,7 +995,7 @@ describe(MetadataService.name, () => { it('should skip importing metadata when the feature is disabled', async () => { assetMock.getByIds.mockResolvedValue([assetStub.primaryImage]); systemMock.get.mockResolvedValue({ metadata: { faces: { import: false } } }); - metadataMock.readTags.mockResolvedValue(metadataStub.withFace); + mockReadTags(metadataStub.withFace); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(personMock.getDistinctNames).not.toHaveBeenCalled(); }); @@ -995,7 +1003,7 @@ describe(MetadataService.name, () => { it('should skip importing metadata face for assets without tags.RegionInfo', async () => { assetMock.getByIds.mockResolvedValue([assetStub.primaryImage]); systemMock.get.mockResolvedValue({ metadata: { faces: { import: true } } }); - metadataMock.readTags.mockResolvedValue(metadataStub.empty); + mockReadTags(metadataStub.empty); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(personMock.getDistinctNames).not.toHaveBeenCalled(); }); @@ -1003,7 +1011,7 @@ describe(MetadataService.name, () => { it('should skip importing faces without name', async () => { assetMock.getByIds.mockResolvedValue([assetStub.primaryImage]); systemMock.get.mockResolvedValue({ metadata: { faces: { import: true } } }); - metadataMock.readTags.mockResolvedValue(metadataStub.withFaceNoName); + mockReadTags(metadataStub.withFaceNoName); personMock.getDistinctNames.mockResolvedValue([]); personMock.createAll.mockResolvedValue([]); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -1015,7 +1023,7 @@ describe(MetadataService.name, () => { it('should skip importing faces with empty name', async () => { assetMock.getByIds.mockResolvedValue([assetStub.primaryImage]); systemMock.get.mockResolvedValue({ metadata: { faces: { import: true } } }); - metadataMock.readTags.mockResolvedValue(metadataStub.withFaceEmptyName); + mockReadTags(metadataStub.withFaceEmptyName); personMock.getDistinctNames.mockResolvedValue([]); personMock.createAll.mockResolvedValue([]); await sut.handleMetadataExtraction({ id: assetStub.image.id }); @@ -1027,7 +1035,7 @@ describe(MetadataService.name, () => { it('should apply metadata face tags creating new persons', async () => { assetMock.getByIds.mockResolvedValue([assetStub.primaryImage]); systemMock.get.mockResolvedValue({ metadata: { faces: { import: true } } }); - metadataMock.readTags.mockResolvedValue(metadataStub.withFace); + mockReadTags(metadataStub.withFace); personMock.getDistinctNames.mockResolvedValue([]); personMock.createAll.mockResolvedValue([personStub.withName.id]); personMock.update.mockResolvedValue(personStub.withName); @@ -1064,7 +1072,7 @@ describe(MetadataService.name, () => { it('should assign metadata face tags to existing persons', async () => { assetMock.getByIds.mockResolvedValue([assetStub.primaryImage]); systemMock.get.mockResolvedValue({ metadata: { faces: { import: true } } }); - metadataMock.readTags.mockResolvedValue(metadataStub.withFace); + mockReadTags(metadataStub.withFace); personMock.getDistinctNames.mockResolvedValue([{ id: personStub.withName.id, name: personStub.withName.name }]); personMock.createAll.mockResolvedValue([]); personMock.update.mockResolvedValue(personStub.withName); @@ -1095,7 +1103,7 @@ describe(MetadataService.name, () => { it('should handle invalid modify date', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ ModifyDate: '00:00:00.000' }); + mockReadTags({ ModifyDate: '00:00:00.000' }); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.upsertExif).toHaveBeenCalledWith( @@ -1107,7 +1115,7 @@ describe(MetadataService.name, () => { it('should handle invalid rating value', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ Rating: 6 }); + mockReadTags({ Rating: 6 }); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.upsertExif).toHaveBeenCalledWith( @@ -1119,7 +1127,7 @@ describe(MetadataService.name, () => { it('should handle valid rating value', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); - metadataMock.readTags.mockResolvedValue({ Rating: 5 }); + mockReadTags({ Rating: 5 }); await sut.handleMetadataExtraction({ id: assetStub.image.id }); expect(assetMock.upsertExif).toHaveBeenCalledWith( diff --git a/server/src/services/metadata.service.ts b/server/src/services/metadata.service.ts index a81d1b4904c27..38c86bcdb13ca 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -339,7 +339,7 @@ export class MetadataService extends BaseService { const sidecarTags = asset.sidecarPath ? await this.metadataRepository.readTags(asset.sidecarPath) : {}; const videoTags = asset.type === AssetType.VIDEO ? await this.getVideoTags(asset.originalPath) : {}; - // make sure dates comes from sidecar + // prefer dates from sidecar tags const sidecarDate = firstDateTime(sidecarTags as Tags, EXIF_DATE_TAGS); if (sidecarDate) { for (const tag of EXIF_DATE_TAGS) { @@ -347,6 +347,10 @@ export class MetadataService extends BaseService { } } + // prefer duration from video tags + delete mediaTags.Duration; + delete sidecarTags.Duration; + return { ...mediaTags, ...videoTags, ...sidecarTags }; }