amake / orgro

An Org Mode file viewer for iOS and Android
https://orgro.org
GNU General Public License v3.0
466 stars 22 forks source link

Local images cause memory pressure (and eventual crash) #98

Open fw623 opened 1 month ago

fw623 commented 1 month ago

Testing

What I expected to happen

I expect the app to show inline images without crashing (in this case local images from the external storage), even when an org-file contains many such images.

What actually happened

The app shows the spinning progress bars in place of the images for a few minutes and then eventually all or most running services on the phone are killed by the memory manager due to memory pressure (checked in the system log), and eventually Orgro is also closed/killed. From the app logs it seems to try to copy each image from external storage to the app's data storage.

This crash happened for a file containing over 300 images (~3MB each) which are each referenced by relative links ([[file:../assets/image1.jpg]] or similar) and showing all at once, but iirc also happened with about 100 images in other org-files. The org-file and images are both on external storage with the correct permissions for Orgro.

Keeping the sections hidden and opening one after the other, waiting for each image to load before opening the next section, works until about 80 images (~3MB each) are loaded, then the same crash as described above happens.

Workaround

Opening sections one after the other, but this time closing all others before opening the next, works and leads to no crash.

Details

Tested on Orgro 1.37.6, Android 14 (GrapheneOS) on a Pixel 6A.

Cause

Therefore it seems that having too many images displayed at once is the problem. To some extent this is kind of expected because each LocalImage is loaded by reading all its bytes and those seemingly are kept in memory as long as the image is displayed: https://github.com/amake/orgro/blob/3de1d6a820e28d4119e5a6f7685cea1dcca6ff7d/lib/src/components/image.dart#L76

Solution (?)

I have no experience with Flutter, so I don't know if the following ideas would actually work. Maybe using Image and providing the external storage filepath to it, instead of returning just the raw bytes, would already work. Or maybe some kind of downsizing and caching of these downsized images would be necessary (which would also improve performance because it doesn't always have to load the full image when changing section visibility). I have also seen that Flutter has an ImageCache, maybe that already handles such problems.

I don't know for what reason it tries to copy each full image to the app's data storage, maybe that can be avoided as well.

amake commented 1 month ago

Thanks for the report. Ideally this shouldn't happen, but I have not optimized Orgro for this use case.

From the app logs it seems to try to copy each image from external storage to the app's data storage.

This is how file_picker_writable works. On the native layer it copies the file to a location that is guaranteed to be readable from the Flutter layer, then returns the URL to the Flutter layer for the application to handle.

I assume this approach was taken for two reasons:

I think it could be feasible to avoid the copy by adding a "no copy" flag to flutter_picker_writable that returns the data directly over the MethodChannel.

Therefore it seems that having too many images displayed at once is the problem

Ultimately you only have so much RAM. The images need to be in RAM. So in the limit this is not really avoidable.

Maybe using Image and providing the external storage filepath to it, instead of returning just the raw bytes, would already work

I'm not seeing a code path that doesn't load the entire file. However it does look like I could specify cacheWidth and cacheHeight in Image.memory to reduce the cached data.

fw623 commented 1 month ago

Thanks for the reply.

Ultimately you only have so much RAM. The images need to be in RAM. So in the limit this is not really avoidable.

A scalable solution would be to use lazy loading (only loading images within and near the viewport). Ideally also only loading downsized images for the inline preview. The full original image would only need to be loaded when viewing the image in the dedicated image viewer. With this approach, one realistically wouldn't run out of RAM.

If the lazy loading is too much work, I think using downsized images for the inline preview should already give plenty headroom. Maybe this could also be combined with respecting org attributes (e.g. #+attr_org: :width 150px), which would reduce the required file size for the inline preview dramatically.

amake commented 3 weeks ago

Actually as long as I'm using Image.memory I have already loaded the entire image into RAM and have to hold it in the widget's state, so that's not going to fix anything.

Instead I think I need to implement an ImageProvider.

amake commented 3 weeks ago

I have optimized in-app memory usage of images in v1.38.0, available for testing soon:

I have not eliminated the initial copy into app storage because that requires a significant change to how a third-party dependency works, and may introduce new inefficiencies. Note that the copied file is deleted immediately after use.

amake commented 3 weeks ago

v1.38.0 is available for testing.

amake commented 2 weeks ago

v1.38.0 is released for iOS and Android