commons-app / apps-android-commons

The Wikimedia Commons Android app allows users to upload pictures from their Android phone/tablet to Wikimedia Commons
https://commons-app.github.io/
Apache License 2.0
1k stars 1.18k forks source link

Limit number of uploads displayed in main activity #510

Closed misaochan closed 7 years ago

misaochan commented 7 years ago

Someone suggested that we limit the number of uploads that we show in the main screen, to help with the OutOfMemoryError encountered by those running slower phones. I think he has a point, and there are quite a few others who are encountering the same issue.

IMO we should set the default to something like the last 10 uploads, and maybe have an option in Settings for the user to select to view more if they want to.

USER_COMMENT=I updated to the latest version of the beta app, but it is still crashing when I access my uploads. I already cleared my RAM, but it seems that workaround doesn't work. I suggest that you don't make that uploads page the default screen and limit the number of uploads seen to 5, to avoid crashes due to memory.
ANDROID_VERSION=4.3
APP_VERSION_NAME=2.2
BRAND=samsung
PHONE_MODEL=GT-I9300
CUSTOM_DATA=
STACK_TRACE=java.lang.OutOfMemoryError
at com.android.volley.toolbox.ByteArrayPool.getBuf(ByteArrayPool.java:101)
at com.android.volley.toolbox.PoolingByteArrayOutputStream.<init>(PoolingByteArrayOutputStream.java:53)
at com.android.volley.toolbox.BasicNetwork.entityToBytes(BasicNetwork.java:228)
at com.android.volley.toolbox.BasicNetwork.performRequest(BasicNetwork.java:123)
at com.android.volley.NetworkDispatcher.run(NetworkDispatcher.java:112)
nicolas-raoul commented 7 years ago

How about this: Load 10, then if the user scrolls to the bottom then load another 10, etc.

On Sun, Apr 16, 2017 at 2:52 PM, Josephine Lim notifications@github.com wrote:

Someone suggested that we limit the number of uploads that we show in the main screen, to help with the OutOfMemoryError encountered by those running slower phones. I think he has a point, and there are quite a few others who are encountering the same issue.

IMO we should set the default to something like the last 10 uploads, and maybe have an option in Settings for the user to select to view more if they want to.

USER_COMMENT=I updated to the latest version of the beta app, but it is still crashing when I access my uploads. I already cleared my RAM, but it seems that workaround doesn't work. I suggest that you don't make that uploads page the default screen and limit the number of uploads seen to 5, to avoid crashes due to memory. ANDROID_VERSION=4.3 APP_VERSION_NAME=2.2 BRAND=samsung PHONE_MODEL=GT-I9300 CUSTOM_DATA= STACK_TRACE=java.lang.OutOfMemoryError at com.android.volley.toolbox.ByteArrayPool.getBuf(ByteArrayPool.java:101) at com.android.volley.toolbox.PoolingByteArrayOutputStream.(PoolingByteArrayOutputStream.java:53) at com.android.volley.toolbox.BasicNetwork.entityToBytes(BasicNetwork.java:228) at com.android.volley.toolbox.BasicNetwork.performRequest(BasicNetwork.java:123) at com.android.volley.NetworkDispatcher.run(NetworkDispatcher.java:112)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/commons-app/apps-android-commons/issues/510, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGFBhRaX0y5r1hsq1zO_XG65MB3zL6Bks5rwayogaJpZM4M-jJY .

misaochan commented 7 years ago

Another user facing the same problem. How about we just set a flat limit of 50 or something first so these guys can load the app, and then work on a better solution (like the one proposed by Nicolas) during the hackathon?

dbrant commented 7 years ago

@misaochan Have you done any profiling of your app's memory usage (i.e. using heap analysis)? Or audit the app for possible memory leaks? This could be something I can help with at the hackathon, since it's something we do on a regular basis in the Wikipedia app.

nicolas-raoul commented 7 years ago

@dbrant I believe nobody has done this yet, that would be very cool if you could enlighten us!

Poyekhali commented 7 years ago

Well, the original report where the cellphone model is GT-I9300 (which is a Samsung Galaxy S3) is mine's. I would like to note that I sent more reports than this one, so the "another user" misaochan is saying may be me too. :) I already cleared all of my clearable RAM, but it doesn't work. I recommend that the default limit would be 10 uploads per load, and users may adjust the limit in their choice (from 5 to 50 uploads per load would be best). Thanks

misaochan commented 7 years ago

@dbrant I don't think we have. Thanks, would be great if we could talk about that during the hackathon!

@Pokefan95 Hi! :) The 'other user' is definitely someone else, not the same name as you.

I think we should actually approach this both ways. We should learn how to audit the app for memory leaks and fix them. And we should address the issue of the app loading potentially hundreds of uploads in the default screen. The former will be beneficial for more than just this particular issue, and for the latter, even if memory leaks are fixed there is still no good reason to load hundreds of uploads each time the user runs the app IMO.

sandarumk commented 7 years ago

The current cursor count is 500 (#552) which we can limit it to a number of our preference or a number taken from the settings. But the drawback with that approach is we can only see that number of uploads (similar to now we can only see our top 500 uploads).

misaochan commented 7 years ago

The app now defaults to 100 recent uploads, and the limit can be modified in Settings. Unfortunately the max limit is still 500, same as before (due to API limitation).

Poyekhali commented 7 years ago

It seems the memory leak issue is still there. I set the limit to 5, and I am still running out of memory..

dbrant commented 7 years ago

From a very cursory look at the source code, here's what I've found so far (correct me if I'm getting anything wrong):

Loading full-resolution images is generally a no-no, since the Java VM heap has a very limited amount of memory, and making a sudden allocation of that size can easily put it over the edge.

I routinely upload photos to Commons that have a resolution of 6000x4000. If this is loaded as an RGBa bitmap in the app, it would take 96MB of memory! On older devices, that's already larger than the whole VM size. This is very likely the cause of the behavior you're seeing. (I don't believe you actually have a memory leak in this instance.)

The solution should be to retrieve the proper thumbnail URL from the API[1] (instead of guessing it), and avoid loading the full-resolution image at all costs. As long as you always make sure to load a downscaled version of the image, you shouldn't really need to restrict the number of uploads that you display, since the items that are off-screen don't consume an appreciable amount of memory.

At any rate... let's work on this at the Hackathon!

[1] action=query&prop=imageinfo&iiprop=url&iiurlwidth=640

misaochan commented 7 years ago

@dbrant thanks for your insight! We'd love to work on this with you, will try and catch up with you tomorrow if possible. :)

Poyekhali commented 7 years ago

Curiously, when a video file appears in the recent uploads, does it load the full video, or just a thumbnail of it? I have uploaded some videos, and they appear in my recent uploads. Maybe that's the cause of my problem?

misaochan commented 7 years ago

This should be fixed in the current master build, @Poyekhali please try again when the new version is out. Are you subscribed to beta?

Poyekhali commented 7 years ago

Yes, I am subscribed to beta.

Poyekhali commented 7 years ago

Great, the new update solved the issue, I set the recent upload limit to 50, and it no longer crashes. It also loads thumnails of videos now, unlike before which loads really the full video. Many thanks!

misaochan commented 7 years ago

Great, thanks everyone! :)