Minor improvements#968
Conversation
colinrtwhite
left a comment
There was a problem hiding this comment.
Appreciate the work, though I'm not sure that we can merge all the changes. I added a few comments.
|
|
||
| val context = options.context | ||
| val resources = context.packageManager.getResourcesForApplication(packageName) | ||
| val resources = if (packageName == context.packageName) { |
There was a problem hiding this comment.
Why is it better to use context.resources in this case?
There was a problem hiding this comment.
In general, it's because the Configuration is different between:
applicationContext.getResources().getConfiguration()
AND
activityContext.getResources().getConfiguration()
The Application context is first constructed with a default Configuration of the system, then Activity inherits and extends that configuration with its state, for example, Application context may have Configuration#orientation = PORTRAIT, but Activity context has Configuration#orientation = LANDSCAPE therefore the resource retrieved will be different if it's varied by orientation.
PackageManager#getResourcesForApplication will returns a Resource object with default system Configuration, same as applicationContext.resources.
So we better use the context from ImageRequest object.
There was a problem hiding this comment.
Ah great point! So we're actually using the application resource object here? Sounds like this could potentially fix: #954
| val decoderDispatcher: CoroutineDispatcher = Dispatchers.IO, | ||
| val transformationDispatcher: CoroutineDispatcher = Dispatchers.IO, | ||
| val decoderDispatcher: CoroutineDispatcher = Dispatchers.Default, | ||
| val transformationDispatcher: CoroutineDispatcher = Dispatchers.Default, |
There was a problem hiding this comment.
I think we actually want to keep this as Dispatchers.IO as it's unlikely, but possible some IO will be done while decoding. Also changing this to Dispatchers.Default doesn't fix the issue with the video frames in the sample app - that's due to MediaMetadataRetriever being slow and not supporting cancellation. Unfortunately, there's not much we can do to optimize that.
There was a problem hiding this comment.
Yeah, you're right. I observed lagging in demo when switch from MP4 screen to JPG, I thought 100% CPU is stolen, but I ran CPU profiler and saw the CPU isn't stolen that much, it just because Dispatchers.IO is out of room (MediaMetadataRetriever hanging), and the ImageListAdapter's differ is also scheduled in Dispatchers.IO.
coil/coil-sample/src/main/java/coil/sample/Utils.kt
Lines 53 to 57 in 986e91d
There was a problem hiding this comment.
MediaMetadataRetriever being slow and not supporting cancellation
Did you try runInterruptible or just call MediaMetadataRetriever#realease() early to support cancellation?
There was a problem hiding this comment.
@Khang-NT Sorry forgot to respond to this. I tried out runInterruptible and calling release if the coroutine is cancelled and unfortunately neither free up the thread that's decoding. In fact, I was able to get MediaMetadataRetriever to completely lock up and never return by using release while the decode was in progress.
It will load drawable in correct (context) configuration. Closes coil-kt#752 Closes coil-kt#954
d870ef3 to
25e25c1
Compare
Dispatchers.Defaultas default. Proof: in the example app, go to the "MP4" screen and the app will be freezzing.ViewTargetRequestDelegate: unregister lifecycle observer as soon as the request is complete, so itself will be eligible for GC.