perf(mobile): remove small thumbnail and cache generated thumbnails #17682
Conversation
* Creating the small thumbnails takes quite some time, which should not be underestimated. * The time needed to generate the small or big thumbnail is not too different from each other. Therefore there is no real benefit of the small thumbnail and it only adds frustration to the end user experience. That is because the image appeared to have loaded (the visual move from blur to something better) but it's still so bad that it is basically a blur. The better solution is therefore to stay at the blur until the actual thumbnail has loaded. * Additionaly to the faster generation of the thumbnail, it now also gets cached similarly to the remote thumbnail which already gets cached. This further speeds up the all over usage of the app and prevents a repeatet thumbnail generation when opening the app.
mobile/lib/providers/image/immich_local_thumbnail_provider.dart
Outdated
Show resolved
Hide resolved
mobile/lib/providers/image/immich_local_thumbnail_provider.dart
Outdated
Show resolved
Hide resolved
* Decreased the quality from the default 95 to 80 to provide similar quality with much reduces thumbnail size. * Use try catch around the read of the cache file.
mobile/lib/providers/image/immich_local_thumbnail_provider.dart
Outdated
Show resolved
Hide resolved
shenlong-tanwen
changed the title
|
@shenlong-tanwen @mertalev Another thing that came to mind is the cache key. Maybe it would be best to use the checksum as the cache key, this way also duplicate images (possible in local folders) could also benefit from the cached image without extra recalculation. I know this would probably be a quite rare case, but could nevertheless be useful. |
mobile/lib/providers/image/immich_local_thumbnail_provider.dart
Outdated
Show resolved
Hide resolved
| yield codec; | ||
| return; | ||
| } catch (error) { | ||
| debugPrint("Loading of thumbnail failed: ${error.toString()}"); |
There was a problem hiding this comment.
debugPrint should be wrapped with kDebugMode
There was a problem hiding this comment.
Ah I see. But failing to load the cached thumbnail is quite severe and if that happens more often (for what ever reason) it could impact the ux. Shouldn't this be therefore logged always as a e.g. severe log?
There was a problem hiding this comment.
Doesn't it just mean the thumbnail is regenerated instead (which is the current behavior always anyway)? I think it can happen if e.g. someone clears the app cache, at which point they'll probably get thousands of these logs.
There was a problem hiding this comment.
No, if they clear the cache, the fileFromCache will be null and it will be directly switched to generating the thumbnail.
If the catch gets invoked, something got loaded from cache (fileFromCache != null) but providing it failed for some reason (maybe fileFromCache.file.path is null)
There was a problem hiding this comment.
Oh, so clearing the app cache doesn't just delete the files, but deletes the cache entries too? I see. In that case it probably warrants a log.severe message since the catch shouldn't be reached in normal circumstances.
There was a problem hiding this comment.
Exactly. I replaced the debugPrint with a log.severe .
|
Awesome, thank you @mertalev |
I'm not sure about it. Checksumming can take a long time in some cases (especially when iCloud is involved), so it could be useful to have a "local-only" page somewhere in the app that doesn't rely on checksums so the app is usable in the meantime. Baking checksums into the thumbnail loader itself would make that impossible. |
|
Okay then it is probably best to leave the cache key like it is. |
|
@mertalev Can I provide anything more for this to get merged soon so the changes make it into the next release? |
|
@EinToni i will run some test today and merge it in |
…17682) * Remove small thumbnail and cache generated thumbnails * Creating the small thumbnails takes quite some time, which should not be underestimated. * The time needed to generate the small or big thumbnail is not too different from each other. Therefore there is no real benefit of the small thumbnail and it only adds frustration to the end user experience. That is because the image appeared to have loaded (the visual move from blur to something better) but it's still so bad that it is basically a blur. The better solution is therefore to stay at the blur until the actual thumbnail has loaded. * Additionaly to the faster generation of the thumbnail, it now also gets cached similarly to the remote thumbnail which already gets cached. This further speeds up the all over usage of the app and prevents a repeatet thumbnail generation when opening the app. * Decrease quality and use try catch * Decreased the quality from the default 95 to 80 to provide similar quality with much reduces thumbnail size. * Use try catch around the read of the cache file. * Replace ImmutableBuffer.fromUint8List with ImmutableBuffer.fromFilePath * Removed unnecessary comment * Replace debugPrint with log.severe for catch of error --------- Co-authored-by: Alex <[email protected]>
…mmich-app#17682) * Remove small thumbnail and cache generated thumbnails * Creating the small thumbnails takes quite some time, which should not be underestimated. * The time needed to generate the small or big thumbnail is not too different from each other. Therefore there is no real benefit of the small thumbnail and it only adds frustration to the end user experience. That is because the image appeared to have loaded (the visual move from blur to something better) but it's still so bad that it is basically a blur. The better solution is therefore to stay at the blur until the actual thumbnail has loaded. * Additionaly to the faster generation of the thumbnail, it now also gets cached similarly to the remote thumbnail which already gets cached. This further speeds up the all over usage of the app and prevents a repeatet thumbnail generation when opening the app. * Decrease quality and use try catch * Decreased the quality from the default 95 to 80 to provide similar quality with much reduces thumbnail size. * Use try catch around the read of the cache file. * Replace ImmutableBuffer.fromUint8List with ImmutableBuffer.fromFilePath * Removed unnecessary comment * Replace debugPrint with log.severe for catch of error --------- Co-authored-by: Alex <[email protected]>
* Revert "fix(mobile): use immutable cache keys for local images (immich-app#17736)" This reverts commit 010b144. * Revert "perf(mobile): remove small thumbnail and cache generated thumbnails (immich-app#17682)" This reverts commit b71039e.
Description
Fixes #13105
How Has This Been Tested?
Videos
Before the change. Including the small thumbnail and a noticeable delay before the actual thumbnail gets loaded:
https://github.com/user-attachments/assets/ddc6f6a2-17f3-4b9d-8014-7ea7f199d097
After the change. No more small thumbnail, the actual thumbnail gets loaded directly, without delay and faster:
https://github.com/user-attachments/assets/3bfa4418-38f0-4677-a77d-8833cf707d3c
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)