Skip to content

POTD widget fails with huge images #2820

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
domdomegg opened this issue Mar 30, 2019 · 10 comments · Fixed by #2906
Closed

POTD widget fails with huge images #2820

domdomegg opened this issue Mar 30, 2019 · 10 comments · Fixed by #2906
Assignees
Labels

Comments

@domdomegg
Copy link
Member

Summary:

Today's POTD is massive, which causes max parcel memory limit to be exceeded.

https://upload.wikimedia.org/wikipedia/commons/c/cb/Attacus_taprobanis-Kadavoor-2018-07-13-001.jpg

It would be good to get a smaller version of the file to avoid this.

2019-03-30 14:10:13.462 20695-20821/fr.free.nrw.commons E/unknown:: unhandled exception
    java.lang.IllegalArgumentException: RemoteViews for widget update exceeds maximum bitmap memory usage (used: 6000000, max: 5529600)
        at android.os.Parcel.createException(Parcel.java:1946)
        at android.os.Parcel.readException(Parcel.java:1910)
        at android.os.Parcel.readException(Parcel.java:1860)
        at com.android.internal.appwidget.IAppWidgetService$Stub$Proxy.updateAppWidgetIds(IAppWidgetService.java:697)
        at android.appwidget.AppWidgetManager.updateAppWidget(AppWidgetManager.java:515)
        at android.appwidget.AppWidgetManager.updateAppWidget(AppWidgetManager.java:588)
        at fr.free.nrw.commons.widget.PicOfDayAppWidget$1.onNewResultImpl(PicOfDayAppWidget.java:111)
        at com.facebook.imagepipeline.datasource.BaseBitmapDataSubscriber.onNewResultImpl(BaseBitmapDataSubscriber.java:60)
        at com.facebook.datasource.BaseDataSubscriber.onNewResult(BaseDataSubscriber.java:46)
        at com.facebook.datasource.AbstractDataSource$1.run(AbstractDataSource.java:176)
        at com.facebook.common.executors.CallerThreadExecutor.execute(CallerThreadExecutor.java:47)
        at com.facebook.datasource.AbstractDataSource.notifyDataSubscriber(AbstractDataSource.java:167)
        at com.facebook.datasource.AbstractDataSource.notifyDataSubscribers(AbstractDataSource.java:158)
        at com.facebook.datasource.AbstractDataSource.setResult(AbstractDataSource.java:210)
        at com.facebook.imagepipeline.datasource.AbstractProducerToDataSourceAdapter.onNewResultImpl(AbstractProducerToDataSourceAdapter.java:93)
        at com.facebook.imagepipeline.datasource.CloseableProducerToDataSourceAdapter.onNewResultImpl(CloseableProducerToDataSourceAdapter.java:63)
        at com.facebook.imagepipeline.datasource.CloseableProducerToDataSourceAdapter.onNewResultImpl(CloseableProducerToDataSourceAdapter.java:24)
        at com.facebook.imagepipeline.datasource.AbstractProducerToDataSourceAdapter$1.onNewResultImpl(AbstractProducerToDataSourceAdapter.java:71)
        at com.facebook.imagepipeline.producers.BaseConsumer.onNewResult(BaseConsumer.java:97)
        at com.facebook.imagepipeline.producers.MultiplexProducer$Multiplexer.onNextResult(MultiplexProducer.java:452)
        at com.facebook.imagepipeline.producers.MultiplexProducer$Multiplexer$ForwardingConsumer.onNewResultImpl(MultiplexProducer.java:513)
        at com.facebook.imagepipeline.producers.MultiplexProducer$Multiplexer$ForwardingConsumer.onNewResultImpl(MultiplexProducer.java:506)
        at com.facebook.imagepipeline.producers.BaseConsumer.onNewResult(BaseConsumer.java:97)
        at com.facebook.imagepipeline.producers.BitmapMemoryCacheProducer$1.onNewResultImpl(BitmapMemoryCacheProducer.java:167)
        at com.facebook.imagepipeline.producers.BitmapMemoryCacheProducer$1.onNewResultImpl(BitmapMemoryCacheProducer.java:118)
        at com.facebook.imagepipeline.producers.BaseConsumer.onNewResult(BaseConsumer.java:97)
        at com.facebook.imagepipeline.producers.DecodeProducer$ProgressiveDecoder.handleResult(DecodeProducer.java:397)
        at com.facebook.imagepipeline.producers.DecodeProducer$ProgressiveDecoder.doDecode(DecodeProducer.java:319)
        at com.facebook.imagepipeline.producers.DecodeProducer$ProgressiveDecoder.access$200(DecodeProducer.java:124)
        at com.facebook.imagepipeline.producers.DecodeProducer$ProgressiveDecoder$1.run(DecodeProducer.java:166)
        at com.facebook.imagepipeline.producers.JobScheduler.doJob(JobScheduler.java:202)
        at com.facebook.imagepipeline.producers.JobScheduler.access$000(JobScheduler.java:22)
        at com.facebook.imagepipeline.producers.JobScheduler$1.run(JobScheduler.java:73)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at com.facebook.imagepipeline.core.PriorityThreadFactory$1.run(PriorityThreadFactory.java:51)
        at java.lang.Thread.run(Thread.java:764)
@domdomegg domdomegg added the bug label Mar 30, 2019
@maskaravivek
Copy link
Member

The API that we use lets us specify the height and width properties.

iiurlwidth, iiurlheight

API doc: https://www.mediawiki.org/wiki/API:Imageinfo

We can either choose to send these params for all our media calls, which would mean adding these params in appendMediaProperties method. Otherwise, if we want to send these params only for POTD, then getPictureOfTheDay needs to be edited.

Should we hardcode the height and width or should be it determined on the phone's hardware capabilities? @domdomegg @misaochan

@domdomegg
Copy link
Member Author

I don't mind either way - I think it would be ideal if we could do it by the phones hardware but only if it doesn't over complicate the code. Otherwise I think something like fixing a max width and height would be good.

@misaochan
Copy link
Member

misaochan commented Apr 1, 2019

I think it's OK to set a (reasonable) max height and width, for most phone screens that should be fine IMO. My main concern would be tablet users who would have a much bigger screen and be more likely to notice. I wonder if setting the max height and width based on screen size would be a good idea? @nicolas-raoul @neslihanturan

@neslihanturan
Copy link
Collaborator

Anything based on px can be noticed based on different devices. But we can not get a dp based API response either. So I think we can calculate required values based on the screen if this won't be too complicated.

@nicolas-raoul
Copy link
Member

Commons is the highest-consuming app when I look at my 10GB mobile data plan usage, even though I don't use the app that much and don't upload more than 100MB per month. I guess many people in other countries have smaller data plans and no WiFi. So, in all activities, we should really try our best to avoid using too much bandwidth, while still showing picture in a quality that satisfies pro photographers. The balance is difficult, but I hope we can find good heuristics using screen size.

How about setting thumbnail width to the width of the screen? Note that width changes when rotating the screen. Let's not worry about niche cases like split screen.

@misaochan
Copy link
Member

How about setting thumbnail width to the width of the screen? Note that width changes when rotating the screen.

I agree with this. It may not be the best quality, but IMO people who are wanting to view pictures in ultra high res are probably not going to be doing it on their phone anyway. :)

@madhurgupta10
Copy link
Collaborator

@misaochan Yes, I recently added a click event for the POTD where a user who is interested in the photo can click on it and know more about it.

@nicolas-raoul
Copy link
Member

The full image should only be downloaded when the user clicks to view the image. Not when just displaying a thumbnail of it in a widget or other activity such as Explore.

@maskaravivek
Copy link
Member

I just found out that MediaWikiImageView fetches the thumbnail for the image by calling a separate API for each image. So basically the current flow is:

  • Call API/use local DB to fetch Media list
  • For each item in Media list, call findThumbnailByFilename API to fetch thumbnail for the image.

findThumbnailByFilename method specifies the thumbnail width as 640. So we can probably use the same width in the first API call itself.

@maskaravivek maskaravivek self-assigned this Apr 20, 2019
@maskaravivek
Copy link
Member

The full image should only be downloaded when the user clicks to view the image.

As per the current code base, even on clicking the image, the media detail view shows the 640 width image as MediaDetailFragment also uses MediaWikiImageView. Will be fixing this too.

Going forward,

  • list views will use the thumbnail, and fallback to the original image if thumbnail is not available
  • the media detail view will use the original URL instead of using the thumbnail.

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

Successfully merging a pull request may close this issue.

6 participants