Fixes Issue 6262: [Bug]: Error occurred while loading images #6291
Conversation
Before this commit, the updateThumbURL() method would attempt to derive a larger resolution thumbnail URL from the current thumbnail's width and height. This derived thumbnail URL would sometimes not match an actual URL. Even creating this thumbnail URL would sometimes fail when doing string manipulation. Both errors can cause issues, with the second one crashing the thread and preventing images from being loaded. This commit simply removes the call to updateThumbURL(). Images with their thumbnails now load correctly, especially with panoramic images.
…properly Before this change, the API calls to MediaWiki would request thumbnails with atleast a specific width, rather than height. These thumbnails would often be extremely low resolution on very wide images, such as panoramas. This change instead requests thumbnails with atleast a specific height. Panorama thumbnails are now at a higher resolution than before. Additionally, the height parameter is now represented as an integer which can be changed more easily.
| "&prop=imageinfo|coordinates&iiprop=url|extmetadata|user&&iiurlheight=" + | ||
| THUMB_HEIGHT_PX + | ||
| "&iiextmetadatafilter=DateTime|Categories|GPSLatitude|GPSLongitude|" + | ||
| "ImageDescription|DateTimeOriginal|Artist|LicenseShortName|LicenseUrl" |
There was a problem hiding this comment.
If I understand correctly, the reasoning is that there are more very wide images (such as panorama landscapes) than very high images?
There was a problem hiding this comment.
That is correct.
The original API call would try and retrieve a thumbnail of a fixed width instead. In ImageInfo.updateThumbUrl(), if it was too short, modify the thumbnail URL to try and get a tall enough thumbnail.
Modifying the thumbnail URL would lead to errors that stop results from displaying when searching or in Explore.
This new MediaWiki call achieves the original intention of updateThumbUrl() but instead puts the work onto the MediaWiki server to respond with a thumbnail that is tall enough.
|
Vertical images like these recent ones in "Featured" seem to have low resolution. If there is anything that can be done towards this, please do not hesitate to file an issue. 🙂
|
|
I am still testing this pull request. Sometimes I am getting this popup a lot, and exploring feels very slow. Other times it is fast. 😅
|
Interesting. I just did a lot of scrolling through "Uploaded via mobile" with the Android Studio emulator and did not get that popup. I suspect that the popup may be appearing because the MediaWiki server is taking a long time to respond. I think the MediaWiki server will scale the image to the specified thumbnail dimension if it is not cached. For large images this may take a long time, especially because the queries are in batches of 10 at a time. |
|
Do not slow server responses usually result in different symptoms, not blocking the UI? Could the new strategy somehow be taking more memory or something? 🙂 |
|
You are probably right. I'll try to reproduce this new error on my physical device and also run the profiler for CPU and memory. I'll let you know what I find. |
|
I'm back to working on this after doing some traveling. I've tried reproducing the ANR message on both the Android Studio emulator and my physical device (Nokia 6.2) with no success. Sometimes the scrolling is slow, but not enough to cause the ANR message. Could you provide me with more details on how to reproduce the ANR? Any information would be appreciated. Thank you! |
|
I'm still unable to exactly reproduce the ANR, but I did do both CPU and memory profiling. Memory profiling shows no more than 300 mb being used when loading images. CPU profiling showed a large spike of activity when switching from the the "Featured" tab to "Uploaded via Mobile" tab:
About half of this CPU activity is from the ExploreMapFragment becoming active and moving the map center, which then causes onScroll to be called many times in the main thread. I believe this is what is causing #6276 and the ANR in the screenshot. Ultimately, I don't think the ANR is caused by this PR. I can work on #6276 soon since I know what is causing it. |
|
Very interesting! Do we really need to move the map when switching from the the "Featured" tab to "Uploaded via Mobile" tab? |
|
As far as I can tell, the only advantage to doing the map's onResume (which includes moving the map) before it is visible is so that the Fragment can probably load before the user actually selects the tab. I also discovered that one of the methods causing this performance issue, Also, do you want me to do these new fixes in this PR or in a new PR? |
Thanks a lot for checking! Among users who open "Explore", I believe very few then go to the "Map" tab. So, pre-loading the map does not sound like a good use of memory. Feel free to perform these fixes in the same pull request. |
Before this commit, ViewPagerAdapter only had one constructor which used the default behavior where more than the current fragment would be in the Resume state. This commit adds a constructor where the behavior parameter can be specified.
… constructor Before this change, onCreateView would use the old ViewPagerAdapter constructor, which had a default behavior where fragments not currently visible would be in the Resume state. After this change, the new ViewPagerAdapter constructor is used with a non-default behavior where only the visible fragment would be in the Resume state. This has performance benefits for most users since the Fragments will only be active when they want to view them.
Before this commit, performMapReadyActions() was called in both onViewCreated and onResume. In effect, the method is called twice before the user can even see the map, leading to performance issues. After this commit, the call in onViewCreated is removed. The method is now called only once when the user switches to the ExploreMapFragment.
Before this commit, there was a call loop that included onScroll and animateTo on the map. These two method calls caused performance issues in ExploreMapFragment. This commit breaks the call loop by removing moveCameraToPosition from getMapCenter. Also, the removed code did not fit the purpose of getMapCenter.
…'s last known location Before this commit, fixing a previous bug caused performMapReadyActions to center the map on a default location rather than the available last known location. This commit adds code which will attempt to get the last known user location. If it cannot be retrieved, the default location is used. The map now centers properly.
|
The code I've just pushed should fix the performance issues. Only the visible Fragment out of the three tabs will be in the The largest CPU spike has gone from 5 seconds to 1 second in length in the profiler. Let me know if this still has any ANR errors. Thank you! |
|
Fantastic! 🎉 I am on a backpacking trip until next week, anyone else able to review and test? Thanks 🙂 |
There was a problem hiding this comment.
Tested these changes. I am not getting any ANRs. Images are getting loaded as expected. Thanks @Jason-Whitmore for exploring this in such depth and fixing it!
I thought this was not related to this PR. I tested two branches, and the resolution in this one seems much lower. Sharing the screenshots (observe the text):
I think there are tradeoffs. We could either have a cropped image or a complete image with lower resolution. The change from 640 (width based) to 220 px (height based) might be causing this. |
|
One potential solution I was thinking about would be to:
This potential solution would have a few issues, however:
In any case, this kind of problem and solution would be in a separate issue and PR. Although the |
Agreed. Let's limit the scope of this PR and discuss this by creating another issue. Could you please modify THUMB_HEIGHT_PX to something that resembles the current state? |
|
How about: a single request, but after receiving the image we check its dimensions, and request again if the resulting resolution is not satisfying? |
Before this commit, the thumbnail height parameter was too small, causing low resolution thumbnails to render. This commit more than doubles the parameter, increasing the thumbnail resolution.
|
I've adjusted |
|
✅ Generated APK variants! |
|
I tested this branch again. Resolution seems much better now (except for tall images, as mentioned above). Thanks @Jason-Whitmore! |
…-app#6291) * ImageInfo.kt: remove call to updateThumbURL() Before this commit, the updateThumbURL() method would attempt to derive a larger resolution thumbnail URL from the current thumbnail's width and height. This derived thumbnail URL would sometimes not match an actual URL. Even creating this thumbnail URL would sometimes fail when doing string manipulation. Both errors can cause issues, with the second one crashing the thread and preventing images from being loaded. This commit simply removes the call to updateThumbURL(). Images with their thumbnails now load correctly, especially with panoramic images. * MediaInterface.kt: fix MediaWiki API call to retrieve thumbnail URLs properly Before this change, the API calls to MediaWiki would request thumbnails with atleast a specific width, rather than height. These thumbnails would often be extremely low resolution on very wide images, such as panoramas. This change instead requests thumbnails with atleast a specific height. Panorama thumbnails are now at a higher resolution than before. Additionally, the height parameter is now represented as an integer which can be changed more easily. * ViewPagerAdapter.java: create new constructor with behavior parameter Before this commit, ViewPagerAdapter only had one constructor which used the default behavior where more than the current fragment would be in the Resume state. This commit adds a constructor where the behavior parameter can be specified. * ExploreFragment.java: modify onCreateView to use new ViewPagerAdapter constructor Before this change, onCreateView would use the old ViewPagerAdapter constructor, which had a default behavior where fragments not currently visible would be in the Resume state. After this change, the new ViewPagerAdapter constructor is used with a non-default behavior where only the visible fragment would be in the Resume state. This has performance benefits for most users since the Fragments will only be active when they want to view them. * ExploreMapFragment.java: remove redundant method call Before this commit, performMapReadyActions() was called in both onViewCreated and onResume. In effect, the method is called twice before the user can even see the map, leading to performance issues. After this commit, the call in onViewCreated is removed. The method is now called only once when the user switches to the ExploreMapFragment. * ExploreMapFragment.java: remove method call that causes recursive loop Before this commit, there was a call loop that included onScroll and animateTo on the map. These two method calls caused performance issues in ExploreMapFragment. This commit breaks the call loop by removing moveCameraToPosition from getMapCenter. Also, the removed code did not fit the purpose of getMapCenter. * ExploreMapFragment.java: fix performMapReadyActions to center to user's last known location Before this commit, fixing a previous bug caused performMapReadyActions to center the map on a default location rather than the available last known location. This commit adds code which will attempt to get the last known user location. If it cannot be retrieved, the default location is used. The map now centers properly. * MediaInterface.kt: adjust thumbnail height parameter Before this commit, the thumbnail height parameter was too small, causing low resolution thumbnails to render. This commit more than doubles the parameter, increasing the thumbnail resolution. --------- Co-authored-by: Ritika Pahwa <[email protected]> Co-authored-by: Nicolas Raoul <[email protected]>
Description (required)
Fixes #6262
What changes did you make and why?
Several small changes were made.
Removing the call to
ImageInfo.updateThumbUrl()where the attempt to get a higher resolution thumbnail via changing the thumbnail URL would sometimes crash the thread.Fixing the API call to MediaWiki to retrieve thumbnails with a minimum height, rather than width. This replaces the work intended by
ImageInfo.updateThumbUrl().Code quality improvements: Created a
THUMB_HEIGHT_PXinteger inMediaInterface.ktto allow for easy modification of the minimum thumbnail height. Also, the API call string was spread across several lines to comply with line length requirements.Tests performed (required)
To easily test this PR, go to the Explore tab, then search for "panorama". On this PR, results will load, but will have the "Error occurred while loading images" message appear on main. Also, as stated in #6262, searching "lunar" one letter at a time should show results on this PR, but fail on main.
Tested ProdDebug on Android Studio Emulator with API level 34.