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.

Sign up for GitHub

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

Jump to bottom

refactor(mobile): Immich image provider #7016

Merged
merged 19 commits into from Feb 13, 2024
Merged

refactor(mobile): Immich image provider #7016

merged 19 commits into from Feb 13, 2024

Conversation

martyfuhry
Copy link
Contributor

@martyfuhry martyfuhry commented Feb 10, 2024 •
edited

  • ImmichImage now uses its own image providers for much better image loading
  • Big performance improvements profiling the Gallery Pager
  • Uses a remote image provider with three part image loading
  • Local image provider for original files
  • Keeps using AssetEntityImageProvider for thumbnails for now
  • May fix fix(mobile): Fix iOS swipe not paging #6726 since Alex identified the multipart image provider issue
  • No longer uses multiple widgets for multipart loading, instead it is all done in the image provider
  • Room for improvements and more optimizations later on
  • Still using Fynn's image cache for old original image provider, renamed to ImmichLocalImageProvider

@martyfuhry martyfuhry self-assigned this Feb 10, 2024
@martyfuhry martyfuhry marked this pull request as draft February 10, 2024 16:08
@cloudflare-pages Cloudflare Pages
Copy link

cloudflare-pages bot commented Feb 10, 2024 •
edited

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 21ea5d8
Status: ✅  Deploy successful!
Preview URL: https://8963631b.immich.pages.dev
Branch Preview URL: https://refactor-immich-image-provid.immich.pages.dev

View logs

@martyfuhry martyfuhry marked this pull request as ready for review February 10, 2024 23:31
shenlong-tanwen
shenlong-tanwen requested changes Feb 11, 2024
View reviewed changes
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.

Thanks a lot for working on this. I've added few comments, Can you please look into them?

@shenlong-tanwen shenlong-tanwen mentioned this pull request Feb 11, 2024
Closed
shenlong-tanwen and others added 3 commits February 12, 2024 09:07
@shenlong-tanwen
feat(mobile): thumbhash support (#7028)
e270ae0 
* feat(mobile): thumbhash support

* perf(mobile): store bmp thumbhash bytes in Isar

---------

Co-authored-by: shenlong-tanwen <139912620+shalong-tanwen@users.noreply.github.com>
@martyfuhry
Merge branch 'refactor/immich-image-provider' of github.com:immich-ap…
bf20c36 
…p/immich into refactor/immich-image-provider
@martyfuhry
Uses octoimage for fade in and placeholders
4140a66
shenlong-tanwen
shenlong-tanwen requested changes Feb 12, 2024
View reviewed changes
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.

Can you please check the outstanding comments? Also, we need to update ImmichImage to ImmichImage.thumbnail in the following two places:

  • SharedAlbumThumbnailImage
  • SharingPage::buildAlbumList

shenlong-tanwen
shenlong-tanwen approved these changes Feb 12, 2024
View reviewed changes
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! Fantastic work <3

@martyfuhry martyfuhry mentioned this pull request Feb 12, 2024
Closed
@martyfuhry
Copy link
Contributor Author

I left the thumbhash support out for now and we will reimplement it back after the image provider work gets merged for the sanity of our reviewers.

I've resolved everything once more and this is ready for review once more!

alextran1502 and others added 3 commits February 12, 2024 15:09
@alextran1502
Use big thumbnail for local image on ios
aea7651
@martyfuhry @alextran1502
fix(mobile): Multipart image loading for iOS double swipe (#7064)
ea293df 
* uses local thumb first

* Multipart thumbnail

* Clean up file delete

* await file delete

* Fynn's comments, made thumbnail smaller and doesn't crash on erroring out on thumbnail

* lint

---------

Co-authored-by: Marty Fuhry <marty@fuhry.farm>
Co-authored-by: Alex <alex.tran1502@gmail.com>
@martyfuhry
Moves http client to global private place for reuse
a033d75
@martyfuhry martyfuhry mentioned this pull request Feb 13, 2024
Closed
fyfrey
fyfrey approved these changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@fyfrey fyfrey left a comment

Choose a reason for hiding this comment

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

LGTM! Can't wait to see it in action on my phone :)

@alextran1502 alextran1502 merged commit 9b4a770 into main Feb 13, 2024
24 checks passed
@alextran1502 alextran1502 deleted the refactor/immich-image-provider branch February 13, 2024 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fyfrey fyfrey fyfrey approved these changes

@shenlong-tanwen shenlong-tanwen shenlong-tanwen approved these changes

Assignees

@martyfuhry martyfuhry

Labels
📱mobile
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@martyfuhry @fyfrey @shenlong-tanwen @alextran1502