fix(mobile): Fixes memory image cache for local images #9019
Conversation
Deploying immich with
|
| Latest commit: |
d40d2a2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4d46105e.immich.pages.dev |
| Branch Preview URL: | https://fix-mobile-image-cache-memor.immich.pages.dev |
|
The old version may have been working, honestly:
This is a fine rewrite of the complicated logic from before, but I don't think it's fixed the issue. I will keep this in draft until I find something more. |
martyfuhry
force-pushed
the
fix/mobile-image-cache-memory-overload
branch
from
9d7c0de
to
9a83cda
Compare
|
Thanks for this PR! It's better now, system (Samsung S20FE) still kills other apps, but Immich luckily survives. |
|
|
||
| /// [ImageCache] that uses two caches for small and large images | ||
| /// so that a single large image does not evict all small iamges | ||
| final class CustomImageCache implements ImageCache { | ||
| final _small = ImageCache(); | ||
| final _large = ImageCache(); | ||
| final _large = ImageCache()..maximumSize = 12; // Maximum 12 images |
There was a problem hiding this comment.
I think it's better to set a size limit in bytes instead of the number of images. This allows more small images and guards against huge images taking up extraordinarily much memory.
However, I think the default of the image cache is already 100mb or something like that... So, not sure why 12 images would make a difference (unless there is a bug in the implementation)
There was a problem hiding this comment.
I thought so as well, but I think the issue is that the image cache incorrectly computes the size of our image provider's decoded images. The image provider uses the multistage image loading, which can load in three or four images (thumbnail -> preview -> full size, etc) at times. And I think that the Image Provider, when computing the size of the image, just takes the last one and multiplies it by 4. I'd have to dig through the docs once more to find that information.
This is all conjecture at this point, but this fix seems to at least stop the bleeding. I'd be happy to investigate further in a future follow up.
There was a problem hiding this comment.
And the large images are only for the full-size images, never for thumbnails. We could set up a third one, _local or _original to store the massive ones. Then we could be more liberal with _large and more strict with _original so that we only store, say, 2 or 3 of the massive ones and dozens of the large ones. In any case, it's not exactly expensive to decode these once more, especially with the multi-stage image loading. And with the on-device cache, loading in the image to decode it is fast, as well. So this image cache is not as important as it once was.
There was a problem hiding this comment.
Alright, so it's probably a quirk/bug in the implementation not counting the size correctly.
I'm fine with setting a small total image limit for the large cache. The disk cache makes this a non-issue.
How many decoded images do we actually need?
3? Previous, current, and next image when swiping through the gallery? I'd really set it super low to guard against crashes when someone has super large images.
Maybe 5 if my math is wrong? XD
There was a problem hiding this comment.
I've tested it with 5 images in the cache, performance is not affected, prev/next image preload is sufficient and mainly system kills no other apps 👍
There was a problem hiding this comment.
Awesome! Let's try it with 5. Since we precache, keeping extra images in that cache honestly affords us almost nothing. I can only barely think of a scenario would it would be preferable to have more full size images in that cache than 5.
I think this is expected system behavior; high memory apps in the foreground are allowed to cause the system to close other apps. If this is unexpected, I'd be happy to look through some "best practices" or something from the app developer (Android / iOS) docs and see what types of things they recommend. There is still likely a memory leak, but I think this addresses the 99% case. |
…mage-cache-memory-overload
ImageCacheManagerto a more genericCacheManagerbecause we don't need the additional complexity of image decoding in the cache.