Skip to content
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): optimize person thumbnail generation #7513

Merged
merged 11 commits into from
May 8, 2024

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Feb 29, 2024

Description

Person thumbnails are currently generated by cropping to a buffer and passing this buffer to sharp to resize. This is inefficient:

  1. It ends up generating two thumbnails instead of one (the buffer is implicitly a new JPEG file)
  2. libvips is optimized for longer image processing pipelines, so separating the workload here is worse for performance
  3. It lowers quality as person thumbnails are compressed at least three times: once due to the use of the compressed preview image, a second time with the buffer, and a third time with the resize

This PR combines cropping and resizing to the same pipeline for better performance and less compressed thumbnails. Additionally, it uses the original image to generate the thumbnail to further reduce compression artifacts, scaling the bounding box dimensions as needed.

How Has This Been Tested?

I ran facial recognition on all assets and confirmed the person thumbnails are accurately cropped and look normal.

Before:

person-thumbnail-before

After:

person-thumbnail-after

Copy link

cloudflare-pages bot commented Feb 29, 2024

Deploying immich with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0309ad1
Status: ✅  Deploy successful!
Preview URL: https://e0654264.immich.pages.dev
Branch Preview URL: https://feat-optimize-thumbnail-gene.immich.pages.dev

View logs

@alextran1502
Copy link
Contributor

I tested with a new instance, and the facial thumbnail crop is off
image

I also got some errors due to bad dimensions.

image

@mertalev
Copy link
Contributor Author

Hmm that's weird, it was working well when I tested it. I'll do more testing tomorrow

@mertalev mertalev force-pushed the feat/optimize-thumbnail-generation branch from 7763ba3 to 4410e31 Compare March 6, 2024 04:36
@mertalev
Copy link
Contributor Author

mertalev commented Mar 6, 2024

@alextran1502 Can you confirm if you still have the issue as of the latest commit? I'm wondering if you maybe tried it with an older commit

@alextran1502 alextran1502 self-assigned this Mar 6, 2024
@alextran1502
Copy link
Contributor

@alextran1502 Can you confirm if you still have the issue as of the latest commit? I'm wondering if you maybe tried it with an older commit

I will take a look again and report back

@mertalev mertalev force-pushed the feat/optimize-thumbnail-generation branch from 80c9f29 to c45e525 Compare April 30, 2024 02:16
server/src/interfaces/media.interface.ts Outdated Show resolved Hide resolved
@mertalev
Copy link
Contributor Author

mertalev commented May 1, 2024

I tried on a slice of my library and reproduced the issue. I think it's because of #3569.

@mertalev
Copy link
Contributor Author

mertalev commented May 1, 2024

Can confirm that handling orientation fixed all the bad crops.

@mertalev
Copy link
Contributor Author

mertalev commented May 6, 2024

@alextran1502 Could you test this PR when you get a chance? I want to be sure the cropping issues you noticed are fixed now.

@alextran1502
Copy link
Contributor

Great work! Thank you for the discovery and the fix!

@alextran1502 alextran1502 merged commit 1167f0f into main May 8, 2024
23 checks passed
@alextran1502 alextran1502 deleted the feat/optimize-thumbnail-generation branch May 8, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants