From 0221e0e159d78dc260399eb8e4348057cd922eba Mon Sep 17 00:00:00 2001 From: christosts Date: Thu, 11 Nov 2021 12:10:50 +0000 Subject: [PATCH] Async buffer queueing: do not throw from flush/shutdown The asynchronous MediaCodec adapter queues input buffers in a background thread. If a codec queueuing operation throws an exception, the buffer enqueuer will store it as a pending exception and re-throw it the next time the adapter will attempt to queue another input buffer. The buffer enqueuer's flush() and shutdown() may throw an exception if the pending error is set. This is subject to a race-condition in which the pending error can be set while the adapter is flushing/shutting down the enqueuer, e.g., if an input buffer is still being queued and the codec throws an exception. As a result, the adapter cannot flush or shutdown gracefully. This change makes the buffer enqueuer to ignore any pending error when flushing/shuttinf down so that the adapter can flush/release gracefully even if a queueing error was detected. PiperOrigin-RevId: 409113054 --- .../AsynchronousMediaCodecBufferEnqueuer.java | 23 +++++++-------- ...nchronousMediaCodecBufferEnqueuerTest.java | 29 +++++++++++++++++++ .../AsynchronousMediaCodecCallbackTest.java | 18 ++++++++++++ 3 files changed, 58 insertions(+), 12 deletions(-) diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecBufferEnqueuer.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecBufferEnqueuer.java index 92c5033904..10f0a24e15 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecBufferEnqueuer.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecBufferEnqueuer.java @@ -16,6 +16,7 @@ package androidx.media3.exoplayer.mediacodec; +import static androidx.annotation.VisibleForTesting.NONE; import static androidx.media3.common.util.Assertions.checkNotNull; import static androidx.media3.common.util.Util.castNonNull; @@ -147,7 +148,7 @@ class AsynchronousMediaCodecBufferEnqueuer { } } - /** Shut down the instance. Make sure to call this method to release its internal resources. */ + /** Shuts down the instance. Make sure to call this method to release its internal resources. */ public void shutdown() { if (started) { flush(); @@ -173,26 +174,23 @@ class AsynchronousMediaCodecBufferEnqueuer { * blocks until the {@link #handlerThread} is idle. */ private void flushHandlerThread() throws InterruptedException { - Handler handler = castNonNull(this.handler); - handler.removeCallbacksAndMessages(null); + checkNotNull(this.handler).removeCallbacksAndMessages(null); blockUntilHandlerThreadIsIdle(); - // Check if any exceptions happened during the last queueing action. - maybeThrowException(); } private void blockUntilHandlerThreadIsIdle() throws InterruptedException { conditionVariable.close(); - castNonNull(handler).obtainMessage(MSG_OPEN_CV).sendToTarget(); + checkNotNull(handler).obtainMessage(MSG_OPEN_CV).sendToTarget(); conditionVariable.block(); } - // Called from the handler thread - - @VisibleForTesting + @VisibleForTesting(otherwise = NONE) /* package */ void setPendingRuntimeException(RuntimeException exception) { pendingRuntimeException.set(exception); } + // Called from the handler thread + private void doHandleMessage(Message msg) { @Nullable MessageParams params = null; switch (msg.what) { @@ -214,7 +212,8 @@ class AsynchronousMediaCodecBufferEnqueuer { conditionVariable.open(); break; default: - setPendingRuntimeException(new IllegalStateException(String.valueOf(msg.what))); + pendingRuntimeException.compareAndSet( + null, new IllegalStateException(String.valueOf(msg.what))); } if (params != null) { recycleMessageParams(params); @@ -226,7 +225,7 @@ class AsynchronousMediaCodecBufferEnqueuer { try { codec.queueInputBuffer(index, offset, size, presentationTimeUs, flag); } catch (RuntimeException e) { - setPendingRuntimeException(e); + pendingRuntimeException.compareAndSet(null, e); } } @@ -240,7 +239,7 @@ class AsynchronousMediaCodecBufferEnqueuer { codec.queueSecureInputBuffer(index, offset, info, presentationTimeUs, flags); } } catch (RuntimeException e) { - setPendingRuntimeException(e); + pendingRuntimeException.compareAndSet(null, e); } } diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecBufferEnqueuerTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecBufferEnqueuerTest.java index e236edc2ed..d4ea15ce5f 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecBufferEnqueuerTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecBufferEnqueuerTest.java @@ -190,6 +190,25 @@ public class AsynchronousMediaCodecBufferEnqueuerTest { enqueuer.flush(); } + @Test + public void flush_withPendingError_doesNotResetError() { + enqueuer.start(); + enqueuer.setPendingRuntimeException( + new MediaCodec.CryptoException(/* errorCode= */ 0, /* detailMessage= */ null)); + + enqueuer.flush(); + + assertThrows( + MediaCodec.CryptoException.class, + () -> + enqueuer.queueInputBuffer( + /* index= */ 0, + /* offset= */ 0, + /* size= */ 0, + /* presentationTimeUs= */ 0, + /* flags= */ 0)); + } + @Test public void shutdown_withoutStart_works() { enqueuer.shutdown(); @@ -219,6 +238,16 @@ public class AsynchronousMediaCodecBufferEnqueuerTest { assertThrows(IllegalStateException.class, () -> enqueuer.shutdown()); } + @Test + public void shutdown_withPendingError_doesNotThrow() { + enqueuer.start(); + enqueuer.setPendingRuntimeException( + new MediaCodec.CryptoException(/* errorCode= */ 0, /* detailMessage= */ null)); + + // Shutting down with a pending error set should not throw . + enqueuer.shutdown(); + } + private static CryptoInfo createCryptoInfo() { CryptoInfo info = new CryptoInfo(); int numSubSamples = 5; diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecCallbackTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecCallbackTest.java index f059e2a24e..fea229347e 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecCallbackTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/mediacodec/AsynchronousMediaCodecCallbackTest.java @@ -437,6 +437,24 @@ public class AsynchronousMediaCodecCallbackTest { assertThat(asynchronousMediaCodecCallback.dequeueOutputBufferIndex(outInfo)).isEqualTo(1); } + @Test + public void flush_withPendingError_resetsError() throws Exception { + asynchronousMediaCodecCallback.onError(codec, createCodecException()); + // Calling flush should clear any pending error. + asynchronousMediaCodecCallback.flush(/* codec= */ null); + + assertThat(asynchronousMediaCodecCallback.dequeueInputBufferIndex()) + .isEqualTo(MediaCodec.INFO_TRY_AGAIN_LATER); + } + + @Test + public void shutdown_withPendingError_doesNotThrow() throws Exception { + asynchronousMediaCodecCallback.onError(codec, createCodecException()); + + // Calling shutdown() should not throw. + asynchronousMediaCodecCallback.shutdown(); + } + /** Reflectively create a {@link MediaCodec.CodecException}. */ private static MediaCodec.CodecException createCodecException() throws Exception { Constructor constructor =