-
Notifications
You must be signed in to change notification settings - Fork 646
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
Minor improvements #968
Minor improvements #968
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the work, though I'm not sure that we can merge all the changes. I added a few comments.
@@ -28,7 +28,11 @@ internal class ResourceUriFetcher( | |||
val resId = data.pathSegments.lastOrNull()?.toIntOrNull() ?: throwInvalidUriException(data) | |||
|
|||
val context = options.context | |||
val resources = context.packageManager.getResourcesForApplication(packageName) | |||
val resources = if (packageName == context.packageName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it better to use context.resources
in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah great point! So we're actually using the application resource object here? Sounds like this could potentially fix: #954
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
fun <T> DiffUtil.ItemCallback<T>.asConfig(): AsyncDifferConfig<T> { | |
return AsyncDifferConfig.Builder(this) | |
.setBackgroundThreadExecutor(Dispatchers.IO.asExecutor()) | |
.build() | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Dispatchers.Default
as 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.