Skip to content

Limit number of uploads displayed in main activity #510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
misaochan opened this issue Apr 16, 2017 · 16 comments
Closed

Limit number of uploads displayed in main activity #510

misaochan opened this issue Apr 16, 2017 · 16 comments
Assignees

Comments

@misaochan
Copy link
Member

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
Copy link
Member

nicolas-raoul commented Apr 16, 2017 via email

@misaochan
Copy link
Member Author

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
Copy link
Collaborator

dbrant commented Apr 19, 2017

@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
Copy link
Member

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

@Poyekhali
Copy link

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
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link

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

@dbrant
Copy link
Collaborator

dbrant commented May 16, 2017

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):

  • When you load a piece of Media into your MediaWikiImageView, you try to "guess" the thumbnail URL by constructing the expected path to the thumbnail.
  • If the above URL fails to load, you fall back to loading the full-resolution image. <<< This, on its own, can potentially cause an out-of-memory event.

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 misaochan reopened this May 18, 2017
@misaochan
Copy link
Member Author

@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
Copy link

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
Copy link
Member Author

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
Copy link

Yes, I am subscribed to beta.

@Poyekhali
Copy link

Poyekhali commented May 20, 2017

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
Copy link
Member Author

Great, thanks everyone! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants