From 77126bc52edf972f837b19657dadac71503e55f8 Mon Sep 17 00:00:00 2001 From: Kaido Kert Date: Mon, 24 Jun 2024 12:20:15 -0700 Subject: [PATCH] Always null-check MediaCodec with synchronized Holder (#3597) Wraps mMediaCodec in a synchronized holder class, guaranteeing synchronized access and prevents unhandled null pointer exceptions. This is alternate implementation to #3596 b/331835987 --- .../dev/cobalt/media/MediaCodecBridge.java | 53 +++++++++---------- .../dev/cobalt/util/SynchronizedHolder.java | 41 ++++++++++++++ 2 files changed, 67 insertions(+), 27 deletions(-) create mode 100644 starboard/android/apk/app/src/main/java/dev/cobalt/util/SynchronizedHolder.java diff --git a/starboard/android/apk/app/src/main/java/dev/cobalt/media/MediaCodecBridge.java b/starboard/android/apk/app/src/main/java/dev/cobalt/media/MediaCodecBridge.java index 9de2aa9be64e..6fa4e720b759 100644 --- a/starboard/android/apk/app/src/main/java/dev/cobalt/media/MediaCodecBridge.java +++ b/starboard/android/apk/app/src/main/java/dev/cobalt/media/MediaCodecBridge.java @@ -33,6 +33,7 @@ import android.os.Bundle; import android.view.Surface; import dev.cobalt.util.Log; +import dev.cobalt.util.SynchronizedHolder; import dev.cobalt.util.UsedByNative; import java.nio.ByteBuffer; import java.nio.ByteOrder; @@ -77,7 +78,9 @@ class MediaCodecBridge { private static final int BITRATE_ADJUSTMENT_FPS = 30; private long mNativeMediaCodecBridge; - private MediaCodec mMediaCodec; + private final SynchronizedHolder mMediaCodec = + new SynchronizedHolder<>(() -> new IllegalStateException("MediaCodec was destroyed")); + private MediaCodec.Callback mCallback; private boolean mFlushed; private long mLastPresentationTimeUs; @@ -455,7 +458,7 @@ public MediaCodecBridge( throw new IllegalArgumentException(); } mNativeMediaCodecBridge = nativeMediaCodecBridge; - mMediaCodec = mediaCodec; + mMediaCodec.set(mediaCodec); mMime = mime; // TODO: Delete the unused mMime field mLastPresentationTimeUs = 0; mFlushed = true; @@ -524,7 +527,7 @@ public void onOutputFormatChanged(MediaCodec codec, MediaFormat format) { } } }; - mMediaCodec.setCallback(mCallback); + mMediaCodec.get().setCallback(mCallback); if (isFrameRenderedCallbackEnabled() || tunnelModeAudioSessionId != -1) { mFrameRendererListener = @@ -540,7 +543,7 @@ public void onFrameRendered(MediaCodec codec, long presentationTimeUs, long nano } } }; - mMediaCodec.setOnFrameRenderedListener(mFrameRendererListener, null); + mMediaCodec.get().setOnFrameRenderedListener(mFrameRendererListener, null); } } @@ -763,22 +766,22 @@ public static void createVideoMediaCodecBridge( @UsedByNative public void release() { try { - String codecName = mMediaCodec.getName(); + String codecName = mMediaCodec.get().getName(); Log.w(TAG, "calling MediaCodec.release() on " + codecName); - mMediaCodec.release(); + mMediaCodec.get().release(); } catch (Exception e) { // The MediaCodec is stuck in a wrong state, possibly due to losing // the surface. Log.e(TAG, "Cannot release media codec", e); } - mMediaCodec = null; + mMediaCodec.set(null); } @SuppressWarnings("unused") @UsedByNative public boolean start() { try { - mMediaCodec.start(); + mMediaCodec.get().start(); } catch (IllegalStateException | IllegalArgumentException e) { Log.e(TAG, "Cannot start the media codec", e); return false; @@ -810,15 +813,11 @@ private void updateOperatingRate() { Log.e(TAG, "Failed to set operating rate with invalid fps " + mFps); return; } - if (mMediaCodec == null) { - Log.e(TAG, "Failed to set operating rate when media codec is null."); - return; - } double operatingRate = mPlaybackRate * mFps; Bundle b = new Bundle(); b.putFloat(MediaFormat.KEY_OPERATING_RATE, (float) operatingRate); try { - mMediaCodec.setParameters(b); + mMediaCodec.get().setParameters(b); } catch (IllegalStateException e) { Log.e(TAG, "Failed to set MediaCodec operating rate", e); } @@ -829,7 +828,7 @@ private void updateOperatingRate() { private int flush() { try { mFlushed = true; - mMediaCodec.flush(); + mMediaCodec.get().flush(); } catch (Exception e) { Log.e(TAG, "Failed to flush MediaCodec", e); return MEDIA_CODEC_ERROR; @@ -852,7 +851,7 @@ private void resetNativeMediaCodecBridge() { private void stop() { resetNativeMediaCodecBridge(); try { - mMediaCodec.stop(); + mMediaCodec.get().stop(); } catch (Exception e) { Log.e(TAG, "Failed to stop MediaCodec", e); } @@ -863,7 +862,7 @@ private void stop() { private String getName() { String codecName = "unknown"; try { - codecName = mMediaCodec.getName(); + codecName = mMediaCodec.get().getName(); } catch (IllegalStateException e) { Log.e(TAG, "Cannot get codec name", e); } @@ -876,7 +875,7 @@ private void getOutputFormat(GetOutputFormatResult outGetOutputFormatResult) { MediaFormat format = null; int status = MEDIA_CODEC_OK; try { - format = mMediaCodec.getOutputFormat(); + format = mMediaCodec.get().getOutputFormat(); } catch (IllegalStateException e) { Log.e(TAG, "Failed to get output format", e); status = MEDIA_CODEC_ERROR; @@ -890,7 +889,7 @@ private void getOutputFormat(GetOutputFormatResult outGetOutputFormatResult) { @UsedByNative private ByteBuffer getInputBuffer(int index) { try { - return mMediaCodec.getInputBuffer(index); + return mMediaCodec.get().getInputBuffer(index); } catch (IllegalStateException e) { Log.e(TAG, "Failed to get input buffer", e); return null; @@ -902,7 +901,7 @@ private ByteBuffer getInputBuffer(int index) { @UsedByNative private ByteBuffer getOutputBuffer(int index) { try { - return mMediaCodec.getOutputBuffer(index); + return mMediaCodec.get().getOutputBuffer(index); } catch (IllegalStateException e) { Log.e(TAG, "Failed to get output buffer", e); return null; @@ -915,7 +914,7 @@ private int queueInputBuffer( int index, int offset, int size, long presentationTimeUs, int flags) { resetLastPresentationTimeIfNeeded(presentationTimeUs); try { - mMediaCodec.queueInputBuffer(index, offset, size, presentationTimeUs, flags); + mMediaCodec.get().queueInputBuffer(index, offset, size, presentationTimeUs, flags); } catch (Exception e) { Log.e(TAG, "Failed to queue input buffer", e); return MEDIA_CODEC_ERROR; @@ -934,7 +933,7 @@ private void setVideoBitrate(int bps, int frameRate) { Bundle b = new Bundle(); b.putInt(MediaCodec.PARAMETER_KEY_VIDEO_BITRATE, targetBps); try { - mMediaCodec.setParameters(b); + mMediaCodec.get().setParameters(b); } catch (IllegalStateException e) { Log.e(TAG, "Failed to set MediaCodec parameters", e); } @@ -947,7 +946,7 @@ private void requestKeyFrameSoon() { Bundle b = new Bundle(); b.putInt(MediaCodec.PARAMETER_KEY_REQUEST_SYNC_FRAME, 0); try { - mMediaCodec.setParameters(b); + mMediaCodec.get().setParameters(b); } catch (IllegalStateException e) { Log.e(TAG, "Failed to set MediaCodec parameters", e); } @@ -980,7 +979,7 @@ private int queueSecureInputBuffer( return MEDIA_CODEC_ERROR; } - mMediaCodec.queueSecureInputBuffer(index, offset, cryptoInfo, presentationTimeUs, 0); + mMediaCodec.get().queueSecureInputBuffer(index, offset, cryptoInfo, presentationTimeUs, 0); } catch (MediaCodec.CryptoException e) { int errorCode = e.getErrorCode(); if (errorCode == MediaCodec.CryptoException.ERROR_NO_KEY) { @@ -1012,7 +1011,7 @@ private int queueSecureInputBuffer( @UsedByNative private void releaseOutputBuffer(int index, boolean render) { try { - mMediaCodec.releaseOutputBuffer(index, render); + mMediaCodec.get().releaseOutputBuffer(index, render); } catch (IllegalStateException e) { // TODO: May need to report the error to the caller. crbug.com/356498. Log.e(TAG, "Failed to release output buffer", e); @@ -1023,7 +1022,7 @@ private void releaseOutputBuffer(int index, boolean render) { @UsedByNative private void releaseOutputBuffer(int index, long renderTimestampNs) { try { - mMediaCodec.releaseOutputBuffer(index, renderTimestampNs); + mMediaCodec.get().releaseOutputBuffer(index, renderTimestampNs); } catch (IllegalStateException e) { // TODO: May need to report the error to the caller. crbug.com/356498. Log.e(TAG, "Failed to release output buffer", e); @@ -1057,7 +1056,7 @@ private boolean configureVideo( } maybeSetMaxVideoInputSize(format); - mMediaCodec.configure(format, surface, crypto, flags); + mMediaCodec.get().configure(format, surface, crypto, flags); mFrameRateEstimator = new FrameRateEstimator(); return true; } catch (IllegalArgumentException e) { @@ -1166,7 +1165,7 @@ private void maybeSetMaxVideoInputSize(MediaFormat format) { @UsedByNative public boolean configureAudio(MediaFormat format, MediaCrypto crypto, int flags) { try { - mMediaCodec.configure(format, null, crypto, flags); + mMediaCodec.get().configure(format, null, crypto, flags); return true; } catch (IllegalArgumentException | IllegalStateException e) { Log.e(TAG, "Cannot configure the audio codec", e); diff --git a/starboard/android/apk/app/src/main/java/dev/cobalt/util/SynchronizedHolder.java b/starboard/android/apk/app/src/main/java/dev/cobalt/util/SynchronizedHolder.java new file mode 100644 index 000000000000..11a85f1733f5 --- /dev/null +++ b/starboard/android/apk/app/src/main/java/dev/cobalt/util/SynchronizedHolder.java @@ -0,0 +1,41 @@ +// Copyright 2024 The Cobalt Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package dev.cobalt.util; + +import java.util.function.Supplier; + +/** + * Generic holder class to turn null checks into a known RuntimeException. Access is synchronized, + * so access from multiple threads is safe. + */ +public class SynchronizedHolder { + private T value; + private final Supplier exceptionSupplier; + + public SynchronizedHolder(Supplier exceptionSupplier) { + this.exceptionSupplier = exceptionSupplier; + } + + public synchronized T get() { + if (value == null) { + throw exceptionSupplier.get(); + } + return value; + } + + public synchronized void set(T value) { + this.value = value; + } +}