-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(server): generate all thumbnails for an asset in one job #13012
base: main
Are you sure you want to change the base?
Conversation
a233104
to
ad1b572
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
if (options.crop) { | ||
pipeline.extract(options.crop); | ||
} | ||
async decodeImage(input: string, options: DecodeToBufferOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary async, right?
'should generate a %s %s for an image when specified', | ||
async ([format, type]) => { | ||
systemMock.get.mockResolvedValue({ image: { [type]: { format: format as ImageFormat } } }); | ||
assetMock.getById.mockResolvedValue(assetStub.image); | ||
const thumbhashBuffer = Buffer.from('a thumbhash', 'utf8'); | ||
mediaMock.generateThumbhash.mockResolvedValue(thumbhashBuffer); | ||
|
||
it('should skip invisible assets', async () => { | ||
assetMock.getByIds.mockResolvedValue([assetStub.livePhotoMotionAsset]); | ||
const previewFormat = type === AssetFileType.PREVIEW ? format : ImageFormat.JPEG; | ||
const thumbnailFormat = type === AssetFileType.THUMBNAIL ? format : ImageFormat.WEBP; | ||
const previewPath = `upload/thumbs/user-id/as/se/asset-id-preview.${previewFormat}`; | ||
const thumbnailPath = `upload/thumbs/user-id/as/se/asset-id-thumbnail.${thumbnailFormat}`; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test has little value imo. Looking at it, it is not clear what it is doing. It takes reading and understanding the test code and checking the different branching going on here. Tests should not have branching and should be very simple. I would prefer 4 individual, trivial tests that are easy to read.
await this.assetRepository.upsertFile({ assetId: asset.id, type: AssetFileType.THUMBNAIL, path: thumbnailPath }); | ||
await this.assetRepository.update({ id: asset.id, updatedAt: new Date() }); | ||
await this.assetRepository.upsertJobStatus({ assetId: asset.id, thumbnailAt: new Date() }); | ||
await this.assetRepository.upsertJobStatus({ assetId: asset.id, previewAt: new Date(), thumbnailAt: new Date() }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future should we combine these into one column?
Description
This PR changes image thumbnail generation to first decode and resize the input image to a preview-sized raw buffer, then reuse that buffer as the input for the preview, thumbnail and thumbhash. This has several advantages:
A concern I had was around RAM usage. Remarkably, this approach uses less RAM than the current one despite the use of an in-memory buffer. Peak memory usage dropped from 2.47GiB to 1.22GiB during testing, and the average is also lower compared to preview generation.
Video thumbnails are not optimized in this PR to keep it smaller and more focused. A later PR will use a similar approach through FFmpeg. In the meantime, I refactored the code such that it will be easy to make this change. I intentionally made it generate video thumbnails sequentially instead of using
Promise.all
to avoid excessive resource usage.Testing
I ran thumbnail generation on all assets and confirmed that jobs were processed successfully. Setting different image settings and enabling the "extract embedded preview" setting all functioned as expected. I additionally ran an image-only comparison on all assets:
In order from left to right:
On main, generating the previews took 43 minutes and completing all jobs took 76 minutes , while this PR completed all thumbnail generation tasks in 44 minutes. For this particular library with default settings, this is a ~73% speedup for images. The gap will widen with more RAW or high megapixel images and when the "extract embedded preview" setting is enabled.