Skip to content
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

Merged
merged 1 commit into from
Nov 8, 2021
Merged

Minor improvements #968

merged 1 commit into from
Nov 8, 2021

Conversation

Khang-NT
Copy link
Contributor

@Khang-NT Khang-NT commented Nov 2, 2021

  1. Decoding & transforming image tasks consume heavy CPU resources, it's CPU bound task so should use Dispatchers.Default as default. Proof: in the example app, go to the "MP4" screen and the app will be freezzing.
  2. ViewTargetRequestDelegate: unregister lifecycle observer as soon as the request is complete, so itself will be eligible for GC.

Copy link
Member

@colinrtwhite colinrtwhite left a 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) {
Copy link
Member

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?

Copy link
Contributor Author

@Khang-NT Khang-NT Nov 5, 2021

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.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it fixes both #752 and #954

val decoderDispatcher: CoroutineDispatcher = Dispatchers.IO,
val transformationDispatcher: CoroutineDispatcher = Dispatchers.IO,
val decoderDispatcher: CoroutineDispatcher = Dispatchers.Default,
val transformationDispatcher: CoroutineDispatcher = Dispatchers.Default,
Copy link
Member

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.

Copy link
Contributor Author

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.

fun <T> DiffUtil.ItemCallback<T>.asConfig(): AsyncDifferConfig<T> {
return AsyncDifferConfig.Builder(this)
.setBackgroundThreadExecutor(Dispatchers.IO.asExecutor())
.build()
}

Copy link
Contributor Author

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?

Copy link
Member

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.

coil-base/src/main/java/coil/request/RequestDelegate.kt Outdated Show resolved Hide resolved
It will load drawable in correct (context) configuration.
Closes coil-kt#752
Closes coil-kt#954
Copy link
Member

@colinrtwhite colinrtwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@colinrtwhite colinrtwhite merged commit f6a71dd into coil-kt:main Nov 8, 2021
colinrtwhite pushed a commit that referenced this pull request Oct 5, 2022
It will load drawable in correct (context) configuration.
Closes #752
Closes #954
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants