Skip to content

perf(mobile): remove load of thumbnails in the image provider #17773

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

Merged
merged 4 commits into from
Apr 23, 2025

Conversation

EinToni
Copy link
Contributor

@EinToni EinToni commented Apr 22, 2025

Description

There is no need to load the thumbnail in the immich_*_image_provider before the image is loaded.
In the gallery_viewer, the loadingBuilder of the PopScope is an ImmichThumbnail and therefore already loads the (cached) thumbnail. After that is loaded, the actual imageProvider ImmichImage is invoked which then loads and displays the actual image. Loading the thumbnail again in the imageProvider ImmichImage is redundant and just slows things down without the user actually seeing any difference as it loads the same thing.

Therefore I did the following changes:

  • Removed the load of the thumbnail from the local and remote image provider as they shall provide the image, not the thumbnail. The thumbnail gets provided by the thumbnail provider.
  • The thumbnail provider is used as the loadingBuilder and the image provider as the imageProvider. Therefore loading the thumbnail in the image provider loads it a second time which is completely redundant, uses precious time and yields no results.

This is a follow up of #17682

How Has This Been Tested?

It can be tested by normal usage to feel a slightly faster load.

To actually verify that these changes are valid and the extra load of the thumbnail in the image provider is unnecessary I suggest the following:

  • In the immich_remote_image_provider and immich_local_image_provider input a sleep at the top of the _codec method: await Future.delayed(const Duration(seconds: 2))
  • Use the app and open some images.
  • => When selecting an image to view, the thumbnail will be loaded as always and then two seconds afterwards the actual image.
  • (If you are curious put the thumbnail generation back in and add another sleep after that. You'll see that nothing changes after the first delay because the equal thumbnail is already there and only after the second sleep the actual image is loaded.)

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if applicable
  • I have no unrelated changes in the PR.
  • I have confirmed that any new dependencies are strictly necessary.
  • I have written tests for new code (if applicable)
  • I have followed naming conventions/patterns in the surrounding code
  • All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
  • All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

* Removed the load of the thumbnail from the local and remote image provider as they shall provide the image, not the thumbnail. The thumbnail gets provided by the thumbnail provider.
* The thumbnail provider is used as the loadingBuilder and the image provider as the imageProvider. Therefore loading the thumbnail in the image provider loads it a second time which is completely redundant, uses precious time and yields no results.
@bo0tzz bo0tzz changed the title Remove load of thumbnails in the image provider perf(mobile): remove load of thumbnails in the image provider Apr 22, 2025
@bo0tzz bo0tzz requested review from shenlong-tanwen and alextran1502 and removed request for shenlong-tanwen April 22, 2025 12:34
Copy link
Contributor

@alextran1502 alextran1502 left a comment

Choose a reason for hiding this comment

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

Great improvement! Thank you

While you are at it? What device do you use for testing? Android? iPhone?

There is a strange issue on iPhone 15 and below and iPad that sometimes it takes 2 swipes to navigate to the next/prev assets. On iPad I can reliably reproduce this when the iPad is in vertical orientation, and the photo has vertical display, it will always takes 2 swipes

Do you have any thoughts?

@alextran1502 alextran1502 enabled auto-merge (squash) April 23, 2025 03:40
@EinToni
Copy link
Contributor Author

EinToni commented Apr 23, 2025

That sounds weird. Is this behavior only appearing with these changes?
I tested it on android, although I have an iPad I don't know how to deploy the test app onto it.

@shenlong-tanwen
Copy link
Member

That sounds weird. Is this behavior only appearing with these changes? I tested it on android, although I have an iPad I don't know how to deploy the test app onto it.

AFAIK, The double swipe issue is more prominent on iOS. I couldn't reproduce it as well on my iPhone 15P. If you're on Discord, send me a message there. I can help you with deploying the app onto your iPad.

@alextran1502
Copy link
Contributor

Is this behavior only appearing with these changes?

No, this has been a long-standing issue

@alextran1502 alextran1502 merged commit 1de2eae into immich-app:main Apr 23, 2025
40 checks passed
@EinToni
Copy link
Contributor Author

EinToni commented Apr 23, 2025

Regarding the double swipe I found an interesting behavior while testing something else on an emulated android. After adding two logs I took a quick video, is that behavior equal to the one you two are describing?

DoubleSwipe_compressed.mp4

Edit: The underlying issue seems to be a missing PointerDownEvent at the beginning of the movement. When that is missing the gesture wont be correctly validated. But it is only missing on this specific image.

@alextran1502
Copy link
Contributor

@EinToni Yeah that looks very much like it. Not sure if it is more prominent on some phone models

@EinToni
Copy link
Contributor Author

EinToni commented Apr 23, 2025

I moved it into #6249 to discuss it further there.

shenlong-tanwen pushed a commit that referenced this pull request Jun 8, 2025
Remove loading of thumbnail in the image provider

* Removed the load of the thumbnail from the local and remote image provider as they shall provide the image, not the thumbnail. The thumbnail gets provided by the thumbnail provider.
* The thumbnail provider is used as the loadingBuilder and the image provider as the imageProvider. Therefore loading the thumbnail in the image provider loads it a second time which is completely redundant, uses precious time and yields no results.

Co-authored-by: Alex <[email protected]>
savely-krasovsky pushed a commit to savely-krasovsky/immich that referenced this pull request Jun 8, 2025
…-app#17773)

Remove loading of thumbnail in the image provider

* Removed the load of the thumbnail from the local and remote image provider as they shall provide the image, not the thumbnail. The thumbnail gets provided by the thumbnail provider.
* The thumbnail provider is used as the loadingBuilder and the image provider as the imageProvider. Therefore loading the thumbnail in the image provider loads it a second time which is completely redundant, uses precious time and yields no results.

Co-authored-by: Alex <[email protected]>
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.

4 participants