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

refactor(mobile): Use ImmichThumbnail and local thumbnail image provider #7279

Merged
merged 10 commits into from Feb 27, 2024

Conversation

martyfuhry
Copy link
Contributor

@martyfuhry martyfuhry commented Feb 21, 2024

  • Makes a new ImmichThumbnail with image providers
  • New ImmichLocalThumbnailProvider for showing local thumbnails
  • Loads low resolution thumbnail in multipart loading (need to test the usefulness of this)
  • Adds blur effect to thumbnail previews in gallery mode
  • Changes custom image provider type to be their own class
  • Adds blurhash (Closes feat(mobile): Adds blurhash #7055 )

Blurhash

Thumbnail preview

Without blur / With blur

Also might add in cache for remote images in this one.

@martyfuhry martyfuhry self-assigned this Feb 21, 2024
@martyfuhry martyfuhry marked this pull request as draft February 21, 2024 03:13
Copy link

cloudflare-pages bot commented Feb 21, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: ef2f605
Status: ✅  Deploy successful!
Preview URL: https://479bdae2.immich.pages.dev
Branch Preview URL: https://refactor-immich-thumbnail.immich.pages.dev

View logs

linter errors

linter
@martyfuhry
Copy link
Contributor Author

That blur is a bit aggressive, maybe it could be taken down a bit? It's difficult to find the "right value" which reduces the sharpness of the pixelated version without making it look blurry.

@martyfuhry martyfuhry marked this pull request as ready for review February 22, 2024 00:04
children: [
placeholder,
FadeInImage(
fadeInDuration: const Duration(milliseconds: 100),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fadeInDuration: const Duration(milliseconds: 100),
fadeInDuration: duration,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I missed this!

base64Decode(asset!.thumbhash!),
);

return useState(thumbhash.rgbaToBmp(rbga));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this to be a state? Can we use useRef instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know the difference! Thanks for the suggestion. I've made the change, and I think it'll be very helpful.

Copy link
Member

@shenlong-tanwen shenlong-tanwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Love the ImageProvider related PRs <3

@alextran1502
Copy link
Contributor

Hi @martyfuhry, I just tested this PR, and here are a few thoughts.

  • When dragging across the timeline, I found the gradient placeholder is more prominent than the blur hash image. I wonder if the gradient placeholder should be replaced entirely by the blurhash.
  • If the point above cannot be solved, I found the transition from the gradient -> blurhash -> actual image is very fast. I don't see the need to add blurhash and increase the complexity.

I think my point is to try to keep the complexity down if we can when the trade-off is minimal. Let me know what you think

@fyfrey
Copy link
Contributor

fyfrey commented Feb 27, 2024

Hi @alextran1502

we can't show the blurhash while drag scrolling because we don't load the asset information. We only fetch this information from the database once drag scrolling stops.

The transition is indeed very fast for cached thumbnails. For non-cached images it makes a lot of sense, though. We can probably improve this in the future to only use the blurhash if the image is not cashed

@martyfuhry
Copy link
Contributor Author

Google photos doesn't show a blurhash at all on my device. I agree it is very very fast and may appear too busy. We could replace the placeholders entirely with blurhashes if we can query the assets while scrubbing quickly enough to read their blurhash.

@alextran1502 alextran1502 merged commit d76baee into main Feb 27, 2024
25 checks passed
@alextran1502 alextran1502 deleted the refactor/immich-thumbnail branch February 27, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants