From 159e3a80d5dbd7496954a0fa790fbeda587c007c Mon Sep 17 00:00:00 2001 From: christosts Date: Mon, 11 May 2020 12:15:48 +0100 Subject: [PATCH] Keep pending output format on MediaCodec.flush() When operating the MediaCodec in asynchronous mode, we are seeing NPEs raised from the DefaultAudioSink because output buffers are pushed to the DefaultAudioSink with the DefaultAudioSink not configured yet. This is possible to happen if an output buffer is passed with MediaCodecAudioRenderer.processOutputBuffer() before an output format change. One possible code path to trigger this is when an output format change is pending and we flush MediaCodec (e.g. for a seek): the callback is waiting in the looper's queue but we ignore all currently queued callbacks after a flush(). This commit checks for a pending output format change during a flush(): if one exists, and the next MediaCodec output callback right after the flush() is an output buffer (and not a new output format), then the pending output format is propagated first. The only adapter that needs to change is the AsynchronousMediaCodecAdapter which previsouly deleted all pending callbacks immediately on flush(). The AsynchronousMediaCodecAdapter now needs to also handle every enqueued callback in order to identify is there's a pending output format change. Testing: added unit tests for the new code. I have verified that if we pass an output buffer to the audio renderer before the output format, it will result on the specific NPE, but I cannot reproduce the base scenario (an output format change is pending when flushing) on a real device because we can't know if an output format is indeed pending. PiperOrigin-RevId: 310885283 --- .../mediacodec/MediaCodecAsyncCallback.java | 23 ++++-- .../AsynchronousMediaCodecAdapterTest.java | 77 +++++++++++++++++-- ...catedThreadAsyncMediaCodecAdapterTest.java | 43 +++++++++++ .../MediaCodecAsyncCallbackTest.java | 41 ++++++++++ 4 files changed, 170 insertions(+), 14 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/mediacodec/MediaCodecAsyncCallback.java b/library/core/src/main/java/com/google/android/exoplayer2/mediacodec/MediaCodecAsyncCallback.java index 5be6031356..abb5d87343 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/mediacodec/MediaCodecAsyncCallback.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/mediacodec/MediaCodecAsyncCallback.java @@ -31,6 +31,7 @@ import java.util.ArrayDeque; private final ArrayDeque bufferInfos; private final ArrayDeque formats; @Nullable private MediaFormat currentFormat; + @Nullable private MediaFormat pendingOutputFormat; @Nullable private IllegalStateException mediaCodecException; /** Creates a new MediaCodecAsyncCallback. */ @@ -111,6 +112,7 @@ import java.util.ArrayDeque; * and any error that was previously set. */ public void flush() { + pendingOutputFormat = formats.isEmpty() ? null : formats.getLast(); availableInputBuffers.clear(); availableOutputBuffers.clear(); bufferInfos.clear(); @@ -119,14 +121,18 @@ import java.util.ArrayDeque; } @Override - public void onInputBufferAvailable(MediaCodec mediaCodec, int i) { - availableInputBuffers.add(i); + public void onInputBufferAvailable(MediaCodec mediaCodec, int index) { + availableInputBuffers.add(index); } @Override public void onOutputBufferAvailable( - MediaCodec mediaCodec, int i, MediaCodec.BufferInfo bufferInfo) { - availableOutputBuffers.add(i); + MediaCodec mediaCodec, int index, MediaCodec.BufferInfo bufferInfo) { + if (pendingOutputFormat != null) { + addOutputFormat(pendingOutputFormat); + pendingOutputFormat = null; + } + availableOutputBuffers.add(index); bufferInfos.add(bufferInfo); } @@ -137,12 +143,17 @@ import java.util.ArrayDeque; @Override public void onOutputFormatChanged(MediaCodec mediaCodec, MediaFormat mediaFormat) { - availableOutputBuffers.add(MediaCodec.INFO_OUTPUT_FORMAT_CHANGED); - formats.add(mediaFormat); + addOutputFormat(mediaFormat); + pendingOutputFormat = null; } @VisibleForTesting() void onMediaCodecError(IllegalStateException e) { mediaCodecException = e; } + + private void addOutputFormat(MediaFormat mediaFormat) { + availableOutputBuffers.add(MediaCodec.INFO_OUTPUT_FORMAT_CHANGED); + formats.add(mediaFormat); + } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/AsynchronousMediaCodecAdapterTest.java b/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/AsynchronousMediaCodecAdapterTest.java index f816d1d11b..c36bf74c9c 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/AsynchronousMediaCodecAdapterTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/AsynchronousMediaCodecAdapterTest.java @@ -19,6 +19,7 @@ package com.google.android.exoplayer2.mediacodec; import static com.google.android.exoplayer2.testutil.TestUtil.assertBufferInfosEqual; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import static org.robolectric.Shadows.shadowOf; import static org.robolectric.annotation.LooperMode.Mode.LEGACY; import android.media.MediaCodec; @@ -33,7 +34,6 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.robolectric.Shadows; import org.robolectric.annotation.LooperMode; /** Unit tests for {@link AsynchronousMediaCodecAdapter}. */ @@ -81,9 +81,12 @@ public class AsynchronousMediaCodecAdapterTest { @Test public void dequeueInputBufferIndex_whileFlushing_returnsTryAgainLater() { adapter.start(); + adapter.getMediaCodecCallback().onInputBufferAvailable(codec, /* index=*/ 0); + // A callback that is pending. + new Handler(looper) + .post(() -> adapter.getMediaCodecCallback().onInputBufferAvailable(codec, /* index=*/ 1)); adapter.flush(); - adapter.getMediaCodecCallback().onInputBufferAvailable(codec, /* index=*/ 1); assertThat(adapter.dequeueInputBufferIndex()).isEqualTo(MediaCodec.INFO_TRY_AGAIN_LATER); } @@ -99,7 +102,7 @@ public class AsynchronousMediaCodecAdapterTest { () -> adapter.getMediaCodecCallback().onInputBufferAvailable(codec, /* index=*/ 1)); // Wait until all tasks have been handled. - Shadows.shadowOf(looper).idle(); + shadowOf(looper).idle(); assertThat(adapter.dequeueInputBufferIndex()).isEqualTo(1); } @@ -116,7 +119,7 @@ public class AsynchronousMediaCodecAdapterTest { adapter.flush(); // Wait until all tasks have been handled. - Shadows.shadowOf(looper).idle(); + shadowOf(looper).idle(); assertThrows( IllegalStateException.class, () -> { @@ -168,7 +171,7 @@ public class AsynchronousMediaCodecAdapterTest { () -> adapter.getMediaCodecCallback().onOutputBufferAvailable(codec, /* index=*/ 1, info1)); // Wait until all tasks have been handled. - Shadows.shadowOf(looper).idle(); + shadowOf(looper).idle(); assertThat(adapter.dequeueOutputBufferIndex(bufferInfo)).isEqualTo(1); assertBufferInfosEqual(info1, bufferInfo); } @@ -186,10 +189,68 @@ public class AsynchronousMediaCodecAdapterTest { adapter.flush(); // Wait until all tasks have been handled. - Shadows.shadowOf(looper).idle(); + shadowOf(looper).idle(); assertThrows(IllegalStateException.class, () -> adapter.dequeueOutputBufferIndex(bufferInfo)); } + @Test + public void dequeueOutputBufferIndex_withPendingOutputFormat_returnsPendingOutputFormat() { + MediaFormat pendingOutputFormat = new MediaFormat(); + MediaCodec.BufferInfo outBufferInfo = new MediaCodec.BufferInfo(); + MediaCodec.Callback mediaCodecCallback = adapter.getMediaCodecCallback(); + Handler handler = new Handler(looper); + adapter.start(); + + // Enqueue callbacks + handler.post(() -> mediaCodecCallback.onOutputFormatChanged(codec, new MediaFormat())); + handler.post( + () -> + mediaCodecCallback.onOutputBufferAvailable( + codec, /* index= */ 0, new MediaCodec.BufferInfo())); + handler.post(() -> mediaCodecCallback.onOutputFormatChanged(codec, pendingOutputFormat)); + handler.post( + () -> + mediaCodecCallback.onOutputBufferAvailable( + codec, /* index= */ 1, new MediaCodec.BufferInfo())); + adapter.flush(); + // After flush is complete, MediaCodec sends on output buffer. + handler.post( + () -> + mediaCodecCallback.onOutputBufferAvailable( + codec, /* index= */ 2, new MediaCodec.BufferInfo())); + shadowOf(looper).idle(); + + assertThat(adapter.dequeueOutputBufferIndex(outBufferInfo)) + .isEqualTo(MediaCodec.INFO_OUTPUT_FORMAT_CHANGED); + assertThat(adapter.getOutputFormat()).isEqualTo(pendingOutputFormat); + assertThat(adapter.dequeueOutputBufferIndex(outBufferInfo)).isEqualTo(2); + } + + @Test + public void dequeueOutputBufferIndex_withPendingAndNewOutputFormat_returnsNewOutputFormat() { + MediaCodec.BufferInfo outBufferInfo = new MediaCodec.BufferInfo(); + MediaCodec.Callback mediaCodecCallback = adapter.getMediaCodecCallback(); + Handler handler = new Handler(looper); + adapter.start(); + + // Enqueue callbacks + handler.post(() -> mediaCodecCallback.onOutputFormatChanged(codec, new MediaFormat())); + handler.post( + () -> + mediaCodecCallback.onOutputBufferAvailable( + codec, /* index= */ 0, new MediaCodec.BufferInfo())); + adapter.flush(); + // After flush is complete, MediaCodec sends an output format change, it should overwrite + // the pending format. + MediaFormat newMediaFormat = new MediaFormat(); + handler.post(() -> mediaCodecCallback.onOutputFormatChanged(codec, newMediaFormat)); + shadowOf(looper).idle(); + + assertThat(adapter.dequeueOutputBufferIndex(outBufferInfo)) + .isEqualTo(MediaCodec.INFO_OUTPUT_FORMAT_CHANGED); + assertThat(adapter.getOutputFormat()).isEqualTo(newMediaFormat); + } + @Test public void getOutputFormat_withMultipleFormats_returnsFormatsInCorrectOrder() { adapter.start(); @@ -223,7 +284,7 @@ public class AsynchronousMediaCodecAdapterTest { adapter.flush(); // Wait until all tasks have been handled. - Shadows.shadowOf(looper).idle(); + shadowOf(looper).idle(); assertThat(adapter.getOutputFormat()).isEqualTo(format); } @@ -236,7 +297,7 @@ public class AsynchronousMediaCodecAdapterTest { adapter.shutdown(); // Wait until all tasks have been handled. - Shadows.shadowOf(looper).idle(); + shadowOf(looper).idle(); assertThat(onCodecStartCalled.get()).isEqualTo(1); } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/DedicatedThreadAsyncMediaCodecAdapterTest.java b/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/DedicatedThreadAsyncMediaCodecAdapterTest.java index 7ea55b1d82..7f61d7d67e 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/DedicatedThreadAsyncMediaCodecAdapterTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/DedicatedThreadAsyncMediaCodecAdapterTest.java @@ -221,6 +221,49 @@ public class DedicatedThreadAsyncMediaCodecAdapterTest { assertThrows(IllegalStateException.class, () -> adapter.dequeueOutputBufferIndex(bufferInfo)); } + @Test + public void dequeueOutputBufferIndex_withPendingOutputFormat_returnsPendingOutputFormat() { + adapter.start(); + MediaCodec.BufferInfo outputBufferInfo = new MediaCodec.BufferInfo(); + MediaFormat pendingMediaFormat = new MediaFormat(); + + adapter.onOutputFormatChanged(codec, new MediaFormat()); + adapter.onOutputBufferAvailable(codec, /* index= */ 0, new MediaCodec.BufferInfo()); + adapter.onOutputFormatChanged(codec, pendingMediaFormat); + adapter.onOutputBufferAvailable(codec, /* index= */ 1, new MediaCodec.BufferInfo()); + // Flush should clear the output queue except from the last pending output format received. + adapter.flush(); + shadowOf(handlerThread.getLooper()).idle(); + adapter.onOutputBufferAvailable(codec, /* index= */ 2, new MediaCodec.BufferInfo()); + + assertThat(adapter.dequeueOutputBufferIndex(outputBufferInfo)) + .isEqualTo(MediaCodec.INFO_OUTPUT_FORMAT_CHANGED); + assertThat(adapter.getOutputFormat()).isEqualTo(pendingMediaFormat); + assertThat(adapter.dequeueOutputBufferIndex(outputBufferInfo)).isEqualTo(2); + } + + @Test + public void dequeueOutputBufferIndex_withPendingAndNewOutputFormat_returnsNewOutputFormat() { + adapter.start(); + MediaCodec.BufferInfo outputBufferInfo = new MediaCodec.BufferInfo(); + MediaFormat pendingMediaFormat = new MediaFormat(); + MediaFormat latestOutputFormat = new MediaFormat(); + + adapter.onOutputFormatChanged(codec, new MediaFormat()); + adapter.onOutputBufferAvailable(codec, /* index= */ 0, new MediaCodec.BufferInfo()); + adapter.onOutputFormatChanged(codec, pendingMediaFormat); + adapter.onOutputBufferAvailable(codec, /* index= */ 1, new MediaCodec.BufferInfo()); + // Flush should clear the output queue except from the last pending output format received. + adapter.flush(); + shadowOf(handlerThread.getLooper()).idle(); + // New output format should overwrite the pending format. + adapter.onOutputFormatChanged(codec, latestOutputFormat); + + assertThat(adapter.dequeueOutputBufferIndex(outputBufferInfo)) + .isEqualTo(MediaCodec.INFO_OUTPUT_FORMAT_CHANGED); + assertThat(adapter.getOutputFormat()).isEqualTo(latestOutputFormat); + } + @Test public void getOutputFormat_withoutFormatReceived_throwsException() { adapter.start(); diff --git a/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/MediaCodecAsyncCallbackTest.java b/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/MediaCodecAsyncCallbackTest.java index 0161b541f1..7cf3f32391 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/MediaCodecAsyncCallbackTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/mediacodec/MediaCodecAsyncCallbackTest.java @@ -135,6 +135,47 @@ public class MediaCodecAsyncCallbackTest { assertThat(mediaCodecAsyncCallback.dequeueOutputBufferIndex(outBufferInfo)).isEqualTo(2); } + @Test + public void dequeOutputBufferIndex_withPendingOutputFormat_returnsPendingOutputFormat() { + MediaCodec.BufferInfo outBufferInfo = new MediaCodec.BufferInfo(); + + mediaCodecAsyncCallback.onOutputFormatChanged(codec, new MediaFormat()); + MediaCodec.BufferInfo bufferInfo = new MediaCodec.BufferInfo(); + mediaCodecAsyncCallback.onOutputBufferAvailable(codec, /* index= */ 0, bufferInfo); + MediaFormat pendingMediaFormat = new MediaFormat(); + mediaCodecAsyncCallback.onOutputFormatChanged(codec, pendingMediaFormat); + // Flush should not discard the last format. + mediaCodecAsyncCallback.flush(); + // First callback after flush is an output buffer, pending output format should be pushed first. + mediaCodecAsyncCallback.onOutputBufferAvailable(codec, /* index= */ 1, bufferInfo); + + assertThat(mediaCodecAsyncCallback.dequeueOutputBufferIndex(outBufferInfo)) + .isEqualTo(MediaCodec.INFO_OUTPUT_FORMAT_CHANGED); + assertThat(mediaCodecAsyncCallback.getOutputFormat()).isEqualTo(pendingMediaFormat); + assertThat(mediaCodecAsyncCallback.dequeueOutputBufferIndex(outBufferInfo)).isEqualTo(1); + } + + @Test + public void dequeOutputBufferIndex_withPendingOutputFormatAndNewFormat_returnsNewFormat() { + mediaCodecAsyncCallback.onOutputFormatChanged(codec, new MediaFormat()); + MediaCodec.BufferInfo bufferInfo = new MediaCodec.BufferInfo(); + mediaCodecAsyncCallback.onOutputBufferAvailable(codec, /* index= */ 0, bufferInfo); + MediaFormat pendingMediaFormat = new MediaFormat(); + mediaCodecAsyncCallback.onOutputFormatChanged(codec, pendingMediaFormat); + // Flush should not discard the last format + mediaCodecAsyncCallback.flush(); + // The first callback after flush is a new MediaFormat, it should overwrite the pending format. + MediaFormat newFormat = new MediaFormat(); + mediaCodecAsyncCallback.onOutputFormatChanged(codec, newFormat); + mediaCodecAsyncCallback.onOutputBufferAvailable(codec, /* index= */ 1, bufferInfo); + MediaCodec.BufferInfo outBufferInfo = new MediaCodec.BufferInfo(); + + assertThat(mediaCodecAsyncCallback.dequeueOutputBufferIndex(outBufferInfo)) + .isEqualTo(MediaCodec.INFO_OUTPUT_FORMAT_CHANGED); + assertThat(mediaCodecAsyncCallback.getOutputFormat()).isEqualTo(newFormat); + assertThat(mediaCodecAsyncCallback.dequeueOutputBufferIndex(outBufferInfo)).isEqualTo(1); + } + @Test public void getOutputFormat_onNewInstance_raisesException() { try {