coil-kt / coil

Image loading for Android and Compose Multiplatform.
https://coil-kt.github.io/coil/
Apache License 2.0
10.84k stars 665 forks source link

TimeoutException: MediaMetadataRetriever.finalize() #651

Closed ItzNotABug closed 1 year ago

ItzNotABug commented 3 years ago

Describe the bug Issue is not reproducible locally but I've got a few thousand crashes due to this. (Reports via Crashlytics) The use-case is using RecyclerView & showing Images from storage in Grid using GridLayoutManager The crash originates from coil-video module & it seems to be a GC issue.

A similar crash once flodded the WordPress mobile App (maybe unrelated), more info. I followed their PR & managed to lower the impact slightly but couldn't blip the impact completely.

What fixed the issue was wrapping the retriever.release() in a try/catch block in VideoFrameFetcher and moving the retriever variable outside the fetch method & lazy initialising it but I'm not sure if the try/catch is a good idea.

Expected behavior Should not Crash the App.

To Reproduce Not reproducible locally atm.

Logs/Screenshots

Fatal Exception: java.util.concurrent.TimeoutException: android.media.MediaMetadataRetriever.finalize() timed out after 60 seconds
       at android.media.MediaMetadataRetriever.native_finalize(MediaMetadataRetriever.java)
       at android.media.MediaMetadataRetriever.finalize(MediaMetadataRetriever.java:332)
       at java.lang.Daemons$FinalizerDaemon.doFinalize(Daemons.java:224)
       at java.lang.Daemons$FinalizerDaemon.run(Daemons.java:211)
       at java.lang.Thread.run(Thread.java:760)

Extended Logs from Crashlytics: (The stacktrace line numbers don't match properly but gives a fair idea of whats happening )

DefaultDispatcher-worker-2
       at android.media.MediaMetadataRetriever.release(MediaMetadataRetriever.java)
       at coil.fetch.VideoFrameFetcher.fetch$suspendImpl(VideoFrameFetcher.java:153)
       at coil.fetch.VideoFrameFetcher.fetch(VideoFrameFetcher.java:49)
       at coil.fetch.VideoFrameFetcher.fetch(VideoFrameFetcher.java:49)
       at coil.intercept.EngineInterceptor.execute(EngineInterceptor.java:285)
       at coil.intercept.EngineInterceptor$intercept$2.invokeSuspend(EngineInterceptor.java:403)
       at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(BaseContinuationImpl.java:33)
       at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.java:106)
       at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.java:571)
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.java:750)
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.java:678)
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.java:665)

Version What library version are you using? Does this occur on a specific API level or Android device? Library Version: io.coil-kt:coil-video:1.1.1 Device: Xiaomi, LGE, LENOVO, Other (20) Android Version: 80 to 85% on Android 7.

colinrtwhite commented 3 years ago

@ItzNotABug Thanks for the report. When you move the MediaMetadataRetriever out of the fetch do you still call retriever.release()? If you're reusing the retriever, but call release in fetch I'd expect subsequent request to fail since the retriever has already been released.

ItzNotABug commented 3 years ago

Initially, it worked fine. All the requests were loaded as they should, didn't find any issues until sometime later. I use a file-observer & update the grid as new files are generated in the storage, so the issue started there when the new files were added to the existing data set in the RecyclerView.

I agree that the retriever is already released for subsequent requests.

What do you suggest? Keeping the try/catch block and moving the retriever variable inside fetch()? Or something that might be better than this?

colinrtwhite commented 3 years ago

Hmm sounds like we'll need to add some locking to prevent more than one (or maybe a couple) MediaMetadataRetriever instances existing at once.

ItzNotABug commented 3 years ago

Seems like even the try/catch won't help much here. I still see very small number of crash count.

Also, It'd be better as you recommended, to prevent more than one instance of MediaMetadataRetriever.

ItzNotABug commented 3 years ago

@colinrtwhite Any update on this? The current crash velocity is quite high due to this bug.

colinrtwhite commented 3 years ago

@ItzNotABug No updates - I'll add them to this thread if I (or someone else) figures out a solution.

As a work-around you could call ImageView.dispose to cancel the request when the app goes into the background and restart the request when the app returns to the foreground. This what they did for the Word Press app.

lakshman5876 commented 2 years ago

This exception is still there. I am using version 1.4.0 in my pure compose android project. Occurs in very few users all the way from Android 7 to 11.

Stacktrace

Fatal Exception: java.util.concurrent.TimeoutException: android.media.MediaMetadataRetriever.finalize() timed out after 60 seconds
       at android.media.MediaMetadataRetriever.native_finalize(MediaMetadataRetriever.java)
       at android.media.MediaMetadataRetriever.finalize(MediaMetadataRetriever.java:332)
       at java.lang.Daemons$FinalizerDaemon.doFinalize(Daemons.java:224)
       at java.lang.Daemons$FinalizerDaemon.run(Daemons.java:211)
       at java.lang.Thread.run(Thread.java:760)

imageloader initialization

MaterialTheme(
        colors = colors,
        typography = typography,
        shapes = shapes) {
        Surface(color = MaterialTheme.colors.surface) {
            val context = LocalContext.current
            val imageLoader = remember {
                ImageLoader
                    .Builder(context)
                    .componentRegistry {
                        add(VideoFrameFileFetcher(context))
                        add(VideoFrameUriFetcher(context))
                        add(VideoFrameDecoder(context))
                    }
                    .build()
            }
            CompositionLocalProvider(LocalImageLoader provides imageLoader) {
                // Describe the rest of the UI.
                screenContent()
            }
        }
    }

useage

val imagePainter = if (uri != null) {
    rememberImagePainter(data = uri)
} else null
if (imagePainter != null) {
    Image(
        painter = imagePainter, contentDescription = null, modifier = Modifier.size(150.dp)
     )
}
colinrtwhite commented 2 years ago

It looks like Wordpress was able to work around this system issue by cancelling in-flight video thumbnail requests in onStop() and restarting them in onStart().

nesyou01 commented 1 year ago

No fix yet?

colinrtwhite commented 1 year ago

Unfortunately I don't think it's possible for Coil to work-around this Android bug internally, though I'm open to a PR if someone figures out how. If you run into this issue I recommend cancelling the ImageRequest in onStop and restarting it in onStart similar to how Wordpress worked around the issue.