perf(mobile): remove small thumbnail and cache generated thumbnails #17792
Conversation
|
Tested and unfortunately, the issue is still there. I don't find the custom cache implementation needed because they are already cached in this provider I believe the |
alextran1502
changed the title
|
Hey, I just tested again with the checksum in the cache key construction The issue with loading the wrong content is fixed. However, a problem remains on the Backup Page > Album selection > Clicking on the image icon to view the assets still renders the whole page with a single asset. It was fixed in #17799, however, with the cache implementation in this PR, the issue pops back up. |
|
Is there any case where checksum can be null? Maybe it should be asset id + checksum? Hmmm, I see you already have both. I'd be interested to know what the actual key is when the backup page is open. Clearly there's a collision, but can both checksum and id be null or an empty string? No, right? |
|
Oh I'm sorry... That's my mistake. I guess we should use the actual user id instead of the remote owner, something like |
|
We also need to ensure that editing an asset that could change the thumbnail should be updated within the app as well. i.e, Let's say a local asset is flipped on device, it should be reflected in the app. This is currently an issue that needs to be addressed in the remote provider as well. Eg, Go to web, upload an asset, let it sync to the mobile app, flip the asset and use the "Replace with Upload" feature in the web to replace it with the flipped version. The app would still be displaying the older thumbnail. We could look into using the modified time of the asset in the cache key which would fix this issue as well. |
|
There are apparently quite a few factors that can affect the image. Is the checksum calculated for local images as well or only for uploaded images? If also for local images the best key would probably be: |
EinToni
force-pushed
the
localThumbnailCaching
branch
from
8343df9 to
0ee8a1a
Compare
|
I also just saw that the asset contains an id and a localId, I assume we should rather use the localId, but what exactly is the difference? |
* 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. * 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. * Use the key provided in the loadImage method instead of the asset of the constructor.
EinToni
force-pushed
the
localThumbnailCaching
branch
from
c88ce0a to
1ac6a5e
Compare
…17792) * 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. * 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. * Use the key provided in the loadImage method instead of the asset of the constructor. * Use userId instead of ownerId * Remove import * Add checksum to thumbnail cache key
…mmich-app#17792) * 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. * 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. * Use the key provided in the loadImage method instead of the asset of the constructor. * Use userId instead of ownerId * Remove import * Add checksum to thumbnail cache key
Description
Redo of #17682 after it got reverted in #17786
It contains the same implementation with two adaptions, the key now uses the ownerId of the asset (is there maybe a better way to get the userId or is this the preferred way) and secondly I replaced the usage of the asset in the
_codecmethod with the key provided as parameter, as suggested by @shenlong-tanwen in #17787Instead of the user id, the checksum of the file would probably also work.
The changes:
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/)