From 245c9fec5a0ec42598dcc0113c325ae916bd323b Mon Sep 17 00:00:00 2001 From: Steve Mayhew Date: Fri, 15 Nov 2019 13:35:48 -0800 Subject: [PATCH 1/3] Cleanup tunneled onFrameRenderListener Cleanup the handler to prevent stale messages and potential handler leaks --- .../video/MediaCodecVideoRenderer.java | 48 ++++++++++++++++++- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java b/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java index 14f864b08a..68ebb3893d 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java @@ -563,6 +563,9 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { clearReportedVideoSize(); clearRenderedFirstFrame(); frameReleaseTimeHelper.disable(); + if (tunnelingOnFrameRenderedListener != null && getCodec() != null) { + tunnelingOnFrameRenderedListener.destroyHandler(getCodec()); + } tunnelingOnFrameRenderedListener = null; try { super.onDisabled(); @@ -687,6 +690,9 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { } codec.configure(mediaFormat, surface, crypto, 0); if (Util.SDK_INT >= 23 && tunneling) { + if (tunnelingOnFrameRenderedListener != null) { + tunnelingOnFrameRenderedListener.destroyHandler(codec); + } tunnelingOnFrameRenderedListener = new OnFrameRenderedListenerV23(codec); } } @@ -1228,6 +1234,9 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { MediaCodec codec = getCodec(); // If codec is null then the listener will be instantiated in configureCodec. if (codec != null) { + if (tunnelingOnFrameRenderedListener != null) { + tunnelingOnFrameRenderedListener.destroyHandler(codec); + } tunnelingOnFrameRenderedListener = new OnFrameRenderedListenerV23(codec); } } @@ -1808,8 +1817,23 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { @TargetApi(23) private final class OnFrameRenderedListenerV23 implements MediaCodec.OnFrameRenderedListener { + private final Handler handler; + private OnFrameRenderedListenerV23(MediaCodec codec) { - codec.setOnFrameRenderedListener(/* listener= */ this, Util.createHandler()); + handler = new Handler(); + codec.setOnFrameRenderedListener(/* listener= */ this, handler); + } + + /** + * Cleanup any messages pending for this listener before removing references to it + * + * @param codec optional, if the codec is still around it should be passed here + */ + void destroyHandler(@Nullable MediaCodec codec) { + if (codec != null) { + codec.setOnFrameRenderedListener(null, null); + } + handler.removeCallbacksAndMessages(null); } @Override @@ -1818,12 +1842,32 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { // Stale event. return; } + + // Work around bug in MediaCodec that causes deadlocks if you call back a + // MediaCodec API that requires MediaCodec to use it's Looper, as it calls this + // listener method holding locks. This was fixed in: + // https://android-review.googlesource.com/1156807 + // + // The work around simply queues the processing to a subsequent message, where + // the lock will not be held. + // + if (Util.SDK_INT < 30) { + handler.post(() -> { + if (this == tunnelingOnFrameRenderedListener) { // event not stale + handleFrameRendered(presentationTimeUs); + } + }); + } else { + handleFrameRendered(presentationTimeUs); + } + } + + private void handleFrameRendered(long presentationTimeUs) { if (presentationTimeUs == TUNNELING_EOS_PRESENTATION_TIME_US) { onProcessedTunneledEndOfStream(); } else { onProcessedTunneledBuffer(presentationTimeUs); } } - } } From 224236b4b60ed5e5696530050738f6990e41a944 Mon Sep 17 00:00:00 2001 From: Steve Mayhew Date: Tue, 19 Nov 2019 16:51:26 -0800 Subject: [PATCH 2/3] Cleanup onFrameRenderListener handler... Even if the codec is null (presume it's handler was run down first, this happens in `MediaCodec.stop()`) make sure to run down our own Handler. --- .../android/exoplayer2/video/MediaCodecVideoRenderer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java b/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java index 68ebb3893d..872cd4dcc8 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java @@ -563,7 +563,7 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { clearReportedVideoSize(); clearRenderedFirstFrame(); frameReleaseTimeHelper.disable(); - if (tunnelingOnFrameRenderedListener != null && getCodec() != null) { + if (tunnelingOnFrameRenderedListener != null) { tunnelingOnFrameRenderedListener.destroyHandler(getCodec()); } tunnelingOnFrameRenderedListener = null; From 621d7a493b2b71f624be32b8353c7f1523a002d2 Mon Sep 17 00:00:00 2001 From: Steve Mayhew Date: Fri, 6 Dec 2019 12:45:07 -0800 Subject: [PATCH 3/3] Fix not to call codec.setOnFrameRenderedListener(null, null) unless stoping codec Refator to lift a method to create (and destroy) the OnFrameRenderedListenerV23() and split destroying the Handler from removing the MediaCodec onFrameRenderedListener to avoid double call to codec.setOnFrameRenderedListener() --- .../video/MediaCodecVideoRenderer.java | 59 +++++++++++++------ 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java b/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java index 872cd4dcc8..b4aa3f48cc 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/video/MediaCodecVideoRenderer.java @@ -158,7 +158,7 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { private boolean tunneling; private int tunnelingAudioSessionId; - /* package */ OnFrameRenderedListenerV23 tunnelingOnFrameRenderedListener; + /* package */ @Nullable OnFrameRenderedListenerV23 tunnelingOnFrameRenderedListener; private long lastInputTimeUs; private long outputStreamOffsetUs; @@ -563,10 +563,7 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { clearReportedVideoSize(); clearRenderedFirstFrame(); frameReleaseTimeHelper.disable(); - if (tunnelingOnFrameRenderedListener != null) { - tunnelingOnFrameRenderedListener.destroyHandler(getCodec()); - } - tunnelingOnFrameRenderedListener = null; + destroyOnFrameRenderListener(); try { super.onDisabled(); } finally { @@ -690,10 +687,7 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { } codec.configure(mediaFormat, surface, crypto, 0); if (Util.SDK_INT >= 23 && tunneling) { - if (tunnelingOnFrameRenderedListener != null) { - tunnelingOnFrameRenderedListener.destroyHandler(codec); - } - tunnelingOnFrameRenderedListener = new OnFrameRenderedListenerV23(codec); + createOnFrameRenderedListener(codec); } } @@ -1234,10 +1228,7 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { MediaCodec codec = getCodec(); // If codec is null then the listener will be instantiated in configureCodec. if (codec != null) { - if (tunnelingOnFrameRenderedListener != null) { - tunnelingOnFrameRenderedListener.destroyHandler(codec); - } - tunnelingOnFrameRenderedListener = new OnFrameRenderedListenerV23(codec); + createOnFrameRenderedListener(codec); } } } @@ -1796,6 +1787,28 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { return deviceNeedsSetOutputSurfaceWorkaround; } + private void createOnFrameRenderedListener(MediaCodec codec) { + OnFrameRenderedListenerV23 oldListener = tunnelingOnFrameRenderedListener; + tunnelingOnFrameRenderedListener = new OnFrameRenderedListenerV23(codec); + if (oldListener != null) { + oldListener.destroyHandler(); + } + } + + private void destroyOnFrameRenderListener() { + if (tunnelingOnFrameRenderedListener != null) { + tunnelingOnFrameRenderedListener.destroyHandler(); + + MediaCodec codec = getCodec(); + if (codec != null) { + tunnelingOnFrameRenderedListener.disable(codec); + } + } + tunnelingOnFrameRenderedListener = null; + + + } + protected Surface getSurface() { return surface; } @@ -1826,16 +1839,24 @@ public class MediaCodecVideoRenderer extends MediaCodecRenderer { /** * Cleanup any messages pending for this listener before removing references to it - * - * @param codec optional, if the codec is still around it should be passed here */ - void destroyHandler(@Nullable MediaCodec codec) { - if (codec != null) { - codec.setOnFrameRenderedListener(null, null); - } + void destroyHandler() { handler.removeCallbacksAndMessages(null); } + /** + * Not required if we are switching one frame render listener for another and may cause + * overhead (MediaCodec calls native_enableOnFrameRenderedListener() twice then). The + * stale event logic should prevent issues for a listener switch. + * + * This still might be a good idea when stopping the codec. + * + * @param codec codec that is being run down. + */ + private void disable(MediaCodec codec) { + codec.setOnFrameRenderedListener(null, null); + } + @Override public void onFrameRendered(MediaCodec codec, long presentationTimeUs, long nanoTime) { if (this != tunnelingOnFrameRenderedListener) {