refactor(server): group async calls in metadata extraction #16450
Conversation
mertalev
force-pushed
the
refactor/server-metadata-extraction-group-async-calls
branch
3 times, most recently
from
a88e6bd to
ecdb914
Compare
|
I tested #14277 and refined the different parts of it in each split PR. I haven't tested the individual PRs extensively, but did confirm they work locally with one or two assets. |
There was a problem hiding this comment.
As far as I can tell this looks good. However I do think there are a lot of changes here, so it'd be good to get some testing done with a range of assets before we merge this and check everything is still handled correctly. Specifically around geo and also tag handling for a number tag or a tag with embedded pipes and slashes
| @@ -491,7 +492,7 @@ describe(MetadataService.name, () => { | |||
| }); | |||
| expect(mocks.user.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512); | |||
| expect(mocks.storage.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); | |||
| expect(mocks.asset.update).toHaveBeenNthCalledWith(1, { | |||
There was a problem hiding this comment.
What is the reason for changing all of these tests to remove the check that they were only called once?
There was a problem hiding this comment.
They don't check that they were called once, but that the e.g. 1st call looked like this. Since the calls are grouped, there's no clear order anymore. I can make it check that the total call count matches though.
| private async mergeExifTags(asset: AssetEntity): Promise<ImmichTags> { | ||
| const [mediaTags, sidecarTags, videoTags] = await Promise.all([ | ||
| this.metadataRepository.readTags(asset.originalPath), | ||
| asset.sidecarPath ? this.metadataRepository.readTags(asset.sidecarPath) : null, |
There was a problem hiding this comment.
Why not have this return {} still, now we need extra handling lower down to deal with something we realistically don't care about (the null case).
There was a problem hiding this comment.
Good call, making them null is a holdover from an earlier version of the PR.
There was a problem hiding this comment.
Why not have this return {} still, now we need extra handling lower down to deal with something we realistically don't care about (the null case).
It gives a type error that Property 'Duration' does not exist on type '{}'. down below. I have to do ({} as ImmichTags) to make it happy. Would you still prefer that?
|
|
||
| const { width, height } = this.getImageDimensions(exifTags); | ||
| let geo: ReverseGeocodeResult, latitude: number | null, longitude: number | null; |
There was a problem hiding this comment.
I'd probably go with a { geo, latitude, longitude } | null object here, since it makes the else case a bit less ugly. Probably personal preference though
There was a problem hiding this comment.
It makes the actual usage of it uglier because of ? and ?? null
There was a problem hiding this comment.
Am I confused or can't you just myObject?.latitude?
There was a problem hiding this comment.
No, because then it's undefined and wouldn't update the DB
| tags = exifTags.HierarchicalSubject.map((tag) => | ||
| // convert | to / | ||
| typeof tag === 'number' | ||
| ? String(tag) | ||
| : tag | ||
| .split('|') | ||
| .map((tag) => tag.replaceAll('/', '|')) | ||
| .join('/'), | ||
| ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh, that's pretty neat
use debugFn no need to change mock
mertalev
force-pushed
the
refactor/server-metadata-extraction-group-async-calls
branch
from
ecdb914 to
3cbd12e
Compare
Description
This PR refactors metadata extraction to increase concurrent asynchronous operations as well as making smaller code improvements, improving overall performance. It is based on #16390, which should be merged first.
Split off from #14277