ExoPlayer set up with Rtmp extension leaks memory when closing the activity with playerView #4249

AdamGrzybkowski · 2018-05-14T06:41:33Z

Issue description

LeakCanary detects a leak when closing activity with PlayerView.

I'm using the ExoPlayer with Rtmp extensions to display the live stream. When the url is invalid (there is no live stream there) the leak occurs.

This is how the releasePlayer method looks like

private fun releasePlayer() {
        player?.release()
        player = null
        playerView.player = null
        mediaSource = null
        trackSelector = null
}

Version of ExoPlayer being used

2.8.0

The text was updated successfully, but these errors were encountered:

ojw28 · 2018-05-30T14:19:26Z

There are two parts to this issue:

The leak occurs because RtmpClient.open blocks forever when an attempt is made to connect to an invalid address, rather than failing. This is what stops the thread right at the top of the chain from exiting, and is the root cause. This is a bug in LibRtmp rather than ExoPlayer, which I've reported here: RtmpClient.open hangs forever when connecting to invalid address ant-media/LibRtmp-Client-for-Android#67.
We could mitigate the leak by nulling ExtractorMediaPeriod.callback when ExtractorMediaPeriod.release() is called. It's probably a good idea to do this for all other MediaPeriod implementations as well, particularly given that loader threads may not exit immediately even when they're working correctly (e.g. whilst LibRtmp shouldn't block forever, it's not unreasonable that it might end up blocking until some timeout is reached).

As highlighted by the ref'd issue, we can end up with memory leaks if Loadable.load implementations take a long time to return upon cancelation. This change cuts off one of the two problematic reference chains. This doesn't do much about the ref'd issue, since there's a second reference chain that's much harder to deal with: Thread->LoadTask->loadable. But since it's easy just to cut this one off, I figure it makes sense to do so. Issue: #4249 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=198735386

The bug here was that we'd create a VideoFrameReleaseTimeHelper using whatever context DefaultRenderersFactory has, and it would then hold a reference to that context via DisplayManager. A leak could then occur if the player outlived the life of the context used to create it (which would be strange/unusual, but not impossible). Issue: #4249 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=198747599

If a MediaPeriod uses a Loadable, then there are typically reference chains of the form: LoadingThread[GCroot]->Loadable->MediaPeriod->Player Where the player is the MediaPeriod callback. When the player is released, this reference chain prevents the player from being GC'd until Loadable cancellation completes, which may not always be fast. This in turn will typically prevent the application's activity from being GC'd, since it'll normally be registered as a listener on the player (directly or indirectly via something like a view). This change mitigates the issue by removing references that the MediaPeriod holds back to the player. The MediaPeriod will still not be eligible for GC, but the player and application activity will be, which in most cases will be most of the leak (in terms of size). Issue: #4249 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=199143646

ojw28 · 2018-06-05T13:59:17Z

We've made some changes to ExoPlayer that will mitigate the leak. The MediaPeriod will still leak, which is unavoidable until the root cause is fixed in LibRtmp. However we now explicitly null references from that point, to allow GC of ExoPlayerImplInternal, the Activity and so on. This is a good thing to do from a robustness point of view, and may allow faster GC for some other use cases too.

Leaving the issue open to track picking up a newer version of LibRtmp when the root cause is addressed.

AdamGrzybkowski · 2018-06-05T16:51:52Z

Great @ojw28 thanks :)

As highlighted by the ref'd issue, we can end up with memory leaks if Loadable.load implementations take a long time to return upon cancelation. This change cuts off one of the two problematic reference chains. This doesn't do much about the ref'd issue, since there's a second reference chain that's much harder to deal with: Thread->LoadTask->loadable. But since it's easy just to cut this one off, I figure it makes sense to do so. Issue: #4249 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=198735386

The bug here was that we'd create a VideoFrameReleaseTimeHelper using whatever context DefaultRenderersFactory has, and it would then hold a reference to that context via DisplayManager. A leak could then occur if the player outlived the life of the context used to create it (which would be strange/unusual, but not impossible). Issue: #4249 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=198747599

If a MediaPeriod uses a Loadable, then there are typically reference chains of the form: LoadingThread[GCroot]->Loadable->MediaPeriod->Player Where the player is the MediaPeriod callback. When the player is released, this reference chain prevents the player from being GC'd until Loadable cancellation completes, which may not always be fast. This in turn will typically prevent the application's activity from being GC'd, since it'll normally be registered as a listener on the player (directly or indirectly via something like a view). This change mitigates the issue by removing references that the MediaPeriod holds back to the player. The MediaPeriod will still not be eligible for GC, but the player and application activity will be, which in most cases will be most of the leak (in terms of size). Issue: #4249 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=199143646

erdemguven self-assigned this May 21, 2018

erdemguven added the bug label May 21, 2018

ojw28 added the bug: in dependency label May 30, 2018

ojw28 assigned ojw28 and unassigned erdemguven Jun 5, 2018

ojw28 removed the bug label Jun 5, 2018

vavadhani mentioned this issue Jun 21, 2019

Add a connection timeout to the socket ant-media/LibRtmp-Client-for-Android#76

Merged

vavadhani mentioned this issue Aug 16, 2019

Upgrade librtmp-client to 3.1.0 #6303

Merged

ojw28 closed this as completed Sep 1, 2019

google locked and limited conversation to collaborators Nov 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ExoPlayer set up with Rtmp extension leaks memory when closing the activity with playerView #4249

ExoPlayer set up with Rtmp extension leaks memory when closing the activity with playerView #4249

AdamGrzybkowski commented May 14, 2018

ojw28 commented May 30, 2018 •

edited

Loading

ojw28 commented Jun 5, 2018

AdamGrzybkowski commented Jun 5, 2018

ExoPlayer set up with Rtmp extension leaks memory when closing the activity with playerView #4249

ExoPlayer set up with Rtmp extension leaks memory when closing the activity with playerView #4249

Comments

AdamGrzybkowski commented May 14, 2018

Issue description

Version of ExoPlayer being used

ojw28 commented May 30, 2018 • edited Loading

ojw28 commented Jun 5, 2018

AdamGrzybkowski commented Jun 5, 2018

ojw28 commented May 30, 2018 •

edited

Loading