From 0d22d02df5c3ae5f5e371185f6a64425475538ac Mon Sep 17 00:00:00 2001 From: olly Date: Mon, 11 May 2020 18:52:28 +0100 Subject: [PATCH] FFmpeg extension: Correctly pad input buffers FFmpeg requires input buffers to be sized larger than the size of the data they contain. This is to allow optimized decoder implementations that read data in fixed size chunks, without the risk of such decoders reading beyond the end of the buffer. Issue: #2159 PiperOrigin-RevId: 310946866 --- .../exoplayer2/ext/av1/Gav1Decoder.java | 3 +- .../ext/ffmpeg/FfmpegAudioDecoder.java | 6 +- .../ext/ffmpeg/FfmpegAudioRenderer.java | 2 +- .../exoplayer2/ext/ffmpeg/FfmpegLibrary.java | 31 ++++++- extensions/ffmpeg/src/main/jni/ffmpeg_jni.cc | 4 + .../exoplayer2/ext/vp9/VpxDecoder.java | 3 +- .../java/com/google/android/exoplayer2/C.java | 2 +- .../decoder/DecoderInputBuffer.java | 45 +++++++--- .../decoder/DecoderInputBufferTest.java | 87 +++++++++++++++++++ .../video/VideoDecoderInputBuffer.java | 27 +++++- .../video/DecoderVideoRendererTest.java | 4 +- 11 files changed, 191 insertions(+), 23 deletions(-) create mode 100644 library/common/src/test/java/com/google/android/exoplayer2/decoder/DecoderInputBufferTest.java diff --git a/extensions/av1/src/main/java/com/google/android/exoplayer2/ext/av1/Gav1Decoder.java b/extensions/av1/src/main/java/com/google/android/exoplayer2/ext/av1/Gav1Decoder.java index bf90c6b1a3..76d7ddd380 100644 --- a/extensions/av1/src/main/java/com/google/android/exoplayer2/ext/av1/Gav1Decoder.java +++ b/extensions/av1/src/main/java/com/google/android/exoplayer2/ext/av1/Gav1Decoder.java @@ -20,6 +20,7 @@ import static java.lang.Runtime.getRuntime; import android.view.Surface; import androidx.annotation.Nullable; import com.google.android.exoplayer2.C; +import com.google.android.exoplayer2.decoder.DecoderInputBuffer; import com.google.android.exoplayer2.decoder.SimpleDecoder; import com.google.android.exoplayer2.util.Util; import com.google.android.exoplayer2.video.VideoDecoderInputBuffer; @@ -94,7 +95,7 @@ import java.nio.ByteBuffer; @Override protected VideoDecoderInputBuffer createInputBuffer() { - return new VideoDecoderInputBuffer(); + return new VideoDecoderInputBuffer(DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_DIRECT); } @Override diff --git a/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegAudioDecoder.java b/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegAudioDecoder.java index c5072a3398..d6980f2801 100644 --- a/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegAudioDecoder.java +++ b/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegAudioDecoder.java @@ -52,10 +52,10 @@ import java.util.List; private volatile int sampleRate; public FfmpegAudioDecoder( + Format format, int numInputBuffers, int numOutputBuffers, int initialInputBufferSize, - Format format, boolean outputFloat) throws FfmpegDecoderException { super(new DecoderInputBuffer[numInputBuffers], new SimpleOutputBuffer[numOutputBuffers]); @@ -82,7 +82,9 @@ import java.util.List; @Override protected DecoderInputBuffer createInputBuffer() { - return new DecoderInputBuffer(DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_DIRECT); + return new DecoderInputBuffer( + DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_DIRECT, + FfmpegLibrary.getInputBufferPaddingSize()); } @Override diff --git a/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegAudioRenderer.java b/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegAudioRenderer.java index efa1d3965f..f5e5281886 100644 --- a/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegAudioRenderer.java +++ b/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegAudioRenderer.java @@ -125,7 +125,7 @@ public final class FfmpegAudioRenderer extends DecoderAudioRenderer { format.maxInputSize != Format.NO_VALUE ? format.maxInputSize : DEFAULT_INPUT_BUFFER_SIZE; decoder = new FfmpegAudioDecoder( - NUM_BUFFERS, NUM_BUFFERS, initialInputBufferSize, format, shouldUseFloatOutput(format)); + format, NUM_BUFFERS, NUM_BUFFERS, initialInputBufferSize, shouldUseFloatOutput(format)); TraceUtil.endSection(); return decoder; } diff --git a/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegLibrary.java b/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegLibrary.java index cc2a78ae86..110de5ddad 100644 --- a/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegLibrary.java +++ b/extensions/ffmpeg/src/main/java/com/google/android/exoplayer2/ext/ffmpeg/FfmpegLibrary.java @@ -16,10 +16,12 @@ package com.google.android.exoplayer2.ext.ffmpeg; import androidx.annotation.Nullable; +import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.ExoPlayerLibraryInfo; import com.google.android.exoplayer2.util.LibraryLoader; import com.google.android.exoplayer2.util.Log; import com.google.android.exoplayer2.util.MimeTypes; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; /** * Configures and queries the underlying native library. @@ -35,6 +37,9 @@ public final class FfmpegLibrary { private static final LibraryLoader LOADER = new LibraryLoader("avutil", "swresample", "avcodec", "ffmpeg"); + private static @MonotonicNonNull String version; + private static int inputBufferPaddingSize = C.LENGTH_UNSET; + private FfmpegLibrary() {} /** @@ -58,7 +63,27 @@ public final class FfmpegLibrary { /** Returns the version of the underlying library if available, or null otherwise. */ @Nullable public static String getVersion() { - return isAvailable() ? ffmpegGetVersion() : null; + if (!isAvailable()) { + return null; + } + if (version == null) { + version = ffmpegGetVersion(); + } + return version; + } + + /** + * Returns the required amount of padding for input buffers in bytes, or {@link C#LENGTH_UNSET} if + * the underlying library is not available. + */ + public static int getInputBufferPaddingSize() { + if (!isAvailable()) { + return C.LENGTH_UNSET; + } + if (inputBufferPaddingSize == C.LENGTH_UNSET) { + inputBufferPaddingSize = ffmpegGetInputBufferPaddingSize(); + } + return inputBufferPaddingSize; } /** @@ -130,6 +155,8 @@ public final class FfmpegLibrary { } private static native String ffmpegGetVersion(); - private static native boolean ffmpegHasDecoder(String codecName); + private static native int ffmpegGetInputBufferPaddingSize(); + + private static native boolean ffmpegHasDecoder(String codecName); } diff --git a/extensions/ffmpeg/src/main/jni/ffmpeg_jni.cc b/extensions/ffmpeg/src/main/jni/ffmpeg_jni.cc index adbf515f9b..7738e5c2d5 100644 --- a/extensions/ffmpeg/src/main/jni/ffmpeg_jni.cc +++ b/extensions/ffmpeg/src/main/jni/ffmpeg_jni.cc @@ -113,6 +113,10 @@ LIBRARY_FUNC(jstring, ffmpegGetVersion) { return env->NewStringUTF(LIBAVCODEC_IDENT); } +LIBRARY_FUNC(jint, ffmpegGetInputBufferPaddingSize) { + return (jint)AV_INPUT_BUFFER_PADDING_SIZE; +} + LIBRARY_FUNC(jboolean, ffmpegHasDecoder, jstring codecName) { return getCodecByName(env, codecName) != NULL; } diff --git a/extensions/vp9/src/main/java/com/google/android/exoplayer2/ext/vp9/VpxDecoder.java b/extensions/vp9/src/main/java/com/google/android/exoplayer2/ext/vp9/VpxDecoder.java index b7b98e836a..f55e3e6f15 100644 --- a/extensions/vp9/src/main/java/com/google/android/exoplayer2/ext/vp9/VpxDecoder.java +++ b/extensions/vp9/src/main/java/com/google/android/exoplayer2/ext/vp9/VpxDecoder.java @@ -19,6 +19,7 @@ import android.view.Surface; import androidx.annotation.Nullable; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.decoder.CryptoInfo; +import com.google.android.exoplayer2.decoder.DecoderInputBuffer; import com.google.android.exoplayer2.decoder.SimpleDecoder; import com.google.android.exoplayer2.drm.DecryptionException; import com.google.android.exoplayer2.drm.ExoMediaCrypto; @@ -97,7 +98,7 @@ import java.nio.ByteBuffer; @Override protected VideoDecoderInputBuffer createInputBuffer() { - return new VideoDecoderInputBuffer(); + return new VideoDecoderInputBuffer(DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_DIRECT); } @Override diff --git a/library/common/src/main/java/com/google/android/exoplayer2/C.java b/library/common/src/main/java/com/google/android/exoplayer2/C.java index 9f4e8beb1c..7558844b87 100644 --- a/library/common/src/main/java/com/google/android/exoplayer2/C.java +++ b/library/common/src/main/java/com/google/android/exoplayer2/C.java @@ -963,7 +963,7 @@ public final class C { /** * Mode specifying whether the player should hold a WakeLock and a WifiLock. One of {@link - * #WAKE_MODE_NONE}, {@link #WAKE_MODE_LOCAL} and {@link #WAKE_MODE_NETWORK}. + * #WAKE_MODE_NONE}, {@link #WAKE_MODE_LOCAL} or {@link #WAKE_MODE_NETWORK}. */ @Documented @Retention(RetentionPolicy.SOURCE) diff --git a/library/common/src/main/java/com/google/android/exoplayer2/decoder/DecoderInputBuffer.java b/library/common/src/main/java/com/google/android/exoplayer2/decoder/DecoderInputBuffer.java index bd5df4c8b1..0ae8ce31f9 100644 --- a/library/common/src/main/java/com/google/android/exoplayer2/decoder/DecoderInputBuffer.java +++ b/library/common/src/main/java/com/google/android/exoplayer2/decoder/DecoderInputBuffer.java @@ -30,7 +30,8 @@ import org.checkerframework.checker.nullness.qual.EnsuresNonNull; public class DecoderInputBuffer extends Buffer { /** - * The buffer replacement mode, which may disable replacement. One of {@link + * The buffer replacement mode. This controls how {@link #ensureSpaceForWrite} generates + * replacement buffers when the capacity of the existing buffer is insufficient. One of {@link * #BUFFER_REPLACEMENT_MODE_DISABLED}, {@link #BUFFER_REPLACEMENT_MODE_NORMAL} or {@link * #BUFFER_REPLACEMENT_MODE_DIRECT}. */ @@ -83,6 +84,7 @@ public class DecoderInputBuffer extends Buffer { @Nullable public ByteBuffer supplementalData; @BufferReplacementMode private final int bufferReplacementMode; + private final int paddingSize; /** * Creates a new instance for which {@link #isFlagsOnly()} will return true. @@ -94,13 +96,28 @@ public class DecoderInputBuffer extends Buffer { } /** - * @param bufferReplacementMode Determines the behavior of {@link #ensureSpaceForWrite(int)}. One - * of {@link #BUFFER_REPLACEMENT_MODE_DISABLED}, {@link #BUFFER_REPLACEMENT_MODE_NORMAL} and - * {@link #BUFFER_REPLACEMENT_MODE_DIRECT}. + * Creates a new instance. + * + * @param bufferReplacementMode The {@link BufferReplacementMode} replacement mode. */ public DecoderInputBuffer(@BufferReplacementMode int bufferReplacementMode) { + this(bufferReplacementMode, /* paddingSize= */ 0); + } + + /** + * Creates a new instance. + * + * @param bufferReplacementMode The {@link BufferReplacementMode} replacement mode. + * @param paddingSize If non-zero, {@link #ensureSpaceForWrite(int)} will ensure that the buffer + * is this number of bytes larger than the requested length. This can be useful for decoders + * that consume data in fixed size blocks, for efficiency. Setting the padding size to the + * decoder's fixed read size is necessary to prevent such a decoder from trying to read beyond + * the end of the buffer. + */ + public DecoderInputBuffer(@BufferReplacementMode int bufferReplacementMode, int paddingSize) { this.cryptoInfo = new CryptoInfo(); this.bufferReplacementMode = bufferReplacementMode; + this.paddingSize = paddingSize; } /** @@ -132,24 +149,27 @@ public class DecoderInputBuffer extends Buffer { */ @EnsuresNonNull("data") public void ensureSpaceForWrite(int length) { - if (data == null) { + length += paddingSize; + @Nullable ByteBuffer currentData = data; + if (currentData == null) { data = createReplacementByteBuffer(length); return; } // Check whether the current buffer is sufficient. - int capacity = data.capacity(); - int position = data.position(); + int capacity = currentData.capacity(); + int position = currentData.position(); int requiredCapacity = position + length; if (capacity >= requiredCapacity) { + data = currentData; return; } // Instantiate a new buffer if possible. ByteBuffer newData = createReplacementByteBuffer(requiredCapacity); - newData.order(data.order()); + newData.order(currentData.order()); // Copy data up to the current position from the old buffer to the new one. if (position > 0) { - data.flip(); - newData.put(data); + currentData.flip(); + newData.put(currentData); } // Set the new buffer. data = newData; @@ -176,7 +196,9 @@ public class DecoderInputBuffer extends Buffer { * @see java.nio.Buffer#flip() */ public final void flip() { - data.flip(); + if (data != null) { + data.flip(); + } if (supplementalData != null) { supplementalData.flip(); } @@ -205,5 +227,4 @@ public class DecoderInputBuffer extends Buffer { + requiredCapacity + ")"); } } - } diff --git a/library/common/src/test/java/com/google/android/exoplayer2/decoder/DecoderInputBufferTest.java b/library/common/src/test/java/com/google/android/exoplayer2/decoder/DecoderInputBufferTest.java new file mode 100644 index 0000000000..58e2db93dc --- /dev/null +++ b/library/common/src/test/java/com/google/android/exoplayer2/decoder/DecoderInputBufferTest.java @@ -0,0 +1,87 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * 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 com.google.android.exoplayer2.decoder; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import androidx.test.ext.junit.runners.AndroidJUnit4; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Arrays; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** Unit tests for {@link DecoderInputBuffer} */ +@RunWith(AndroidJUnit4.class) +public class DecoderInputBufferTest { + + @Test + public void ensureSpaceForWrite_replacementModeDisabled_doesNothingIfResizeNotNeeded() { + DecoderInputBuffer buffer = + new DecoderInputBuffer(DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_DISABLED); + ByteBuffer data = ByteBuffer.allocate(32); + buffer.data = data; + buffer.ensureSpaceForWrite(32); + assertThat(buffer.data).isSameInstanceAs(data); + } + + @Test + public void ensureSpaceForWrite_replacementModeDisabled_failsIfResizeNeeded() { + DecoderInputBuffer buffer = + new DecoderInputBuffer(DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_DISABLED); + buffer.data = ByteBuffer.allocate(16); + assertThrows(IllegalStateException.class, () -> buffer.ensureSpaceForWrite(32)); + } + + @Test + public void ensureSpaceForWrite_usesPaddingSize() { + DecoderInputBuffer buffer = + new DecoderInputBuffer( + DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_NORMAL, /* paddingSize= */ 16); + buffer.data = ByteBuffer.allocate(32); + buffer.ensureSpaceForWrite(32); + assertThat(buffer.data.capacity()).isEqualTo(32 + 16); + } + + @Test + public void ensureSpaceForWrite_usesPosition() { + DecoderInputBuffer buffer = + new DecoderInputBuffer(DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_NORMAL); + buffer.data = ByteBuffer.wrap(new byte[] {0, 1, 2, 3, 4, 5, 6, 7}); + buffer.data.position(4); + buffer.ensureSpaceForWrite(12); + // The new capacity should be the current position (4) + the required space (12). + assertThat(buffer.data.capacity()).isEqualTo(4 + 12); + // The current position should have been retained. + assertThat(buffer.data.position()).isEqualTo(4); + // Data should have been copied up to the current position. + byte[] expectedData = Arrays.copyOf(new byte[] {0, 1, 2, 3}, 16); + assertThat(buffer.data.array()).isEqualTo(expectedData); + } + + @Test + public void ensureSpaceForWrite_copiesByteOrder() { + DecoderInputBuffer buffer = + new DecoderInputBuffer(DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_NORMAL); + buffer.data = ByteBuffer.allocate(8).order(ByteOrder.LITTLE_ENDIAN); + buffer.ensureSpaceForWrite(16); + assertThat(buffer.data.order()).isEqualTo(ByteOrder.LITTLE_ENDIAN); + buffer.data = ByteBuffer.allocate(8).order(ByteOrder.BIG_ENDIAN); + buffer.ensureSpaceForWrite(16); + assertThat(buffer.data.order()).isEqualTo(ByteOrder.BIG_ENDIAN); + } +} diff --git a/library/core/src/main/java/com/google/android/exoplayer2/video/VideoDecoderInputBuffer.java b/library/core/src/main/java/com/google/android/exoplayer2/video/VideoDecoderInputBuffer.java index 0acb255a18..c496dbabde 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/video/VideoDecoderInputBuffer.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/video/VideoDecoderInputBuffer.java @@ -24,8 +24,31 @@ public class VideoDecoderInputBuffer extends DecoderInputBuffer { @Nullable public Format format; - public VideoDecoderInputBuffer() { - super(DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_DIRECT); + /** + * Creates a new instance. + * + * @param bufferReplacementMode Determines the behavior of {@link #ensureSpaceForWrite(int)}. One + * of {@link #BUFFER_REPLACEMENT_MODE_DISABLED}, {@link #BUFFER_REPLACEMENT_MODE_NORMAL} and + * {@link #BUFFER_REPLACEMENT_MODE_DIRECT}. + */ + public VideoDecoderInputBuffer(@BufferReplacementMode int bufferReplacementMode) { + super(bufferReplacementMode); } + /** + * Creates a new instance. + * + * @param bufferReplacementMode Determines the behavior of {@link #ensureSpaceForWrite(int)}. One + * of {@link #BUFFER_REPLACEMENT_MODE_DISABLED}, {@link #BUFFER_REPLACEMENT_MODE_NORMAL} and + * {@link #BUFFER_REPLACEMENT_MODE_DIRECT}. + * @param paddingSize If non-zero, {@link #ensureSpaceForWrite(int)} will ensure that the buffer + * is this number of bytes larger than the requested length. This can be useful for decoders + * that consume data in fixed size blocks, for efficiency. Setting the padding size to the + * decoder's fixed read size is necessary to prevent such a decoder from trying to read beyond + * the end of the buffer. + */ + public VideoDecoderInputBuffer( + @BufferReplacementMode int bufferReplacementMode, int paddingSize) { + super(bufferReplacementMode, paddingSize); + } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/video/DecoderVideoRendererTest.java b/library/core/src/test/java/com/google/android/exoplayer2/video/DecoderVideoRendererTest.java index f4aee42f25..17ba57698f 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/video/DecoderVideoRendererTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/video/DecoderVideoRendererTest.java @@ -32,6 +32,7 @@ import com.google.android.exoplayer2.Format; import com.google.android.exoplayer2.RendererCapabilities; import com.google.android.exoplayer2.RendererConfiguration; import com.google.android.exoplayer2.decoder.DecoderException; +import com.google.android.exoplayer2.decoder.DecoderInputBuffer; import com.google.android.exoplayer2.decoder.SimpleDecoder; import com.google.android.exoplayer2.drm.ExoMediaCrypto; import com.google.android.exoplayer2.testutil.FakeSampleStream; @@ -141,7 +142,8 @@ public final class DecoderVideoRendererTest { new VideoDecoderInputBuffer[10], new VideoDecoderOutputBuffer[10]) { @Override protected VideoDecoderInputBuffer createInputBuffer() { - return new VideoDecoderInputBuffer(); + return new VideoDecoderInputBuffer( + DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_DIRECT); } @Override