Fix playback of videos in Gallery. #4313
Conversation
| @@ -46,16 +46,25 @@ class ImageInfo { | |||
| val width = 0 | |||
| val height = 0 | |||
|
|
|||
| val bestDerivative get() = derivatives.lastOrNull() | |||
| fun getBestDerivativeForSize(widthDp: Int): Derivative? { | |||
There was a problem hiding this comment.
Can we add @Suppress("KotlinConstantConditions") for this function?
| derivatives.forEach { | ||
| if (it.width > 0 && it.width < widthDp) { | ||
| if (derivative == null || it.width > derivative!!.width) { | ||
| derivative = it | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it be better to use filter and mayByOrNull?
derivatives.filter { it.width < widthDp && it.width < widthDp }.maxByOrNull { it.width }
I tried a couple of times and it looks like using filter and maxByOrNull can mostly improve the efficiency
forEach 581.093us
filter+mayByOrNull 53.75us
forEach 62.032us
filter+mayByOrNull 67.604us
forEach 51.51us
filter+mayByOrNull 50.781us
forEach 118.489us
filter+mayByOrNull 57.239us
There was a problem hiding this comment.
😄 An interesting place to look for optimizations! This code won't be running thousands of times per second; it will run maybe once or twice per session. But I like the attention to detail.
However, I'm not sure I agree with the performance assessment. Here are some thoughts:
-
Did you run your measurement in Debug mode or Release mode? This kind of performance profiling must be done in Release mode to have any kind of meaning. When I run it in Release mode, my
forEachmethod is much faster thanfilter+maxByOrNull. -
To get a true sense of the performance of a loop like this, it needs to be measured over a billion iterations, not just a couple of times. There are all kinds of events happening in the VM that can throw off a single measurement of a loop.
-
The
filterandmaxByNullfunctions are not O(1). They both have their own performance costs. If you look at the actual source code of thefilterfunction and themaxByOrNullfunction, they are literally usingforloops to do the filtering. So, if we usefilter+maxByOrNull, we would be running two loops instead of one, and also instantiating an unnecessaryArrayList(returned byfilterand passed tomaxByOrNull). This is guaranteed to be slower than a singleforEachloop. So basically, it would be O(2n) + allocation, while my method is just O(n).
There was a problem hiding this comment.
Thanks for the information, that explains the benefit of using the original logic you put in the code! I checked the library and you are correct!
…esign * origin/nearby_design: Bump versionCode. (#4318) Further fix video playback! (#4317) Bump versionCode. (#4316) Fix: make sure getting the coordinates from the page/summary endpoint (#4311) Fix playback of videos in Gallery. (#4313) Fix strings. Localisation updates from https://translatewiki.net. Revert "Fix playback of videos." Fix playback of videos. Fix: scrolling issue in the AddTemplateActivity (#4312) Localisation updates from https://translatewiki.net. (#4310) Update maplibre versions (#4306)
Video playback is currently broken.
This is because our logic for selecting the best possible derivative for playback was not smart enough, and was blindly selecting the "last" derivative.
The list of derivatives can change, and indeed it has changed to include an unplayable derivative as the "last" one.
This improves the logic of selecting the proper derivative.
https://phabricator.wikimedia.org/T352315