Fix potential null pointer access in HardwareVideoEncoder
There was no check for null in the code that prepends config buffer to key frame buffer. It is not a requirement for codec to deliver config buffer. Some codecs probably do not do that.
Bug: none
Change-Id: Id9c57efc5d1de5f569fa773313da4db3cd8b074c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299900
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Sergey Silkin <ssilkin@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39807}
diff --git a/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java b/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java
index bd01b7d..0a6e344 100644
--- a/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java
+++ b/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java
@@ -579,75 +579,81 @@
return;
}
- ByteBuffer codecOutputBuffer = codec.getOutputBuffer(index);
- codecOutputBuffer.position(info.offset);
- codecOutputBuffer.limit(info.offset + info.size);
+ ByteBuffer outputBuffer = codec.getOutputBuffer(index);
+ outputBuffer.position(info.offset);
+ outputBuffer.limit(info.offset + info.size);
if ((info.flags & MediaCodec.BUFFER_FLAG_CODEC_CONFIG) != 0) {
Logging.d(TAG, "Config frame generated. Offset: " + info.offset + ". Size: " + info.size);
- configBuffer = ByteBuffer.allocateDirect(info.size);
- configBuffer.put(codecOutputBuffer);
- } else {
- bitrateAdjuster.reportEncodedFrame(info.size);
- if (adjustedBitrate != bitrateAdjuster.getAdjustedBitrateBps()) {
- updateBitrate();
+ if (info.size > 0
+ && (codecType == VideoCodecMimeType.H264 || codecType == VideoCodecMimeType.H265)) {
+ // In case of H264 and H265 config buffer contains SPS and PPS headers. Presence of these
+ // headers makes IDR frame a truly keyframe. Some encoders issue IDR frames without SPS
+ // and PPS. We save config buffer here to prepend it to all IDR frames encoder delivers.
+ configBuffer = ByteBuffer.allocateDirect(info.size);
+ configBuffer.put(outputBuffer);
}
-
- final boolean isKeyFrame = (info.flags & MediaCodec.BUFFER_FLAG_SYNC_FRAME) != 0;
- if (isKeyFrame) {
- Logging.d(TAG, "Sync frame generated");
- }
-
- final ByteBuffer frameBuffer;
- if (isKeyFrame && codecType == VideoCodecMimeType.H264) {
- Logging.d(TAG,
- "Prepending config frame of size " + configBuffer.capacity()
- + " to output buffer with offset " + info.offset + ", size " + info.size);
- // For H.264 key frame prepend SPS and PPS NALs at the start.
- frameBuffer = ByteBuffer.allocateDirect(info.size + configBuffer.capacity());
- configBuffer.rewind();
- frameBuffer.put(configBuffer);
- frameBuffer.put(codecOutputBuffer);
- frameBuffer.rewind();
- } else {
- frameBuffer = codecOutputBuffer.slice();
- }
-
- final EncodedImage.FrameType frameType = isKeyFrame
- ? EncodedImage.FrameType.VideoFrameKey
- : EncodedImage.FrameType.VideoFrameDelta;
-
- outputBuffersBusyCount.increment();
- EncodedImage.Builder builder = outputBuilders.poll();
- builder
- .setBuffer(frameBuffer,
- () -> {
- // This callback should not throw any exceptions since
- // it may be called on an arbitrary thread.
- // Check bug webrtc:11230 for more details.
- try {
- codec.releaseOutputBuffer(index, false);
- } catch (Exception e) {
- Logging.e(TAG, "releaseOutputBuffer failed", e);
- }
- outputBuffersBusyCount.decrement();
- })
- .setFrameType(frameType);
-
- if (isEncodingStatisticsEnabled) {
- MediaFormat format = codec.getOutputFormat(index);
- if (format != null && format.containsKey(MediaFormat.KEY_VIDEO_QP_AVERAGE)) {
- int qp = format.getInteger(MediaFormat.KEY_VIDEO_QP_AVERAGE);
- builder.setQp(qp);
- }
- }
-
- EncodedImage encodedImage = builder.createEncodedImage();
- // TODO(mellem): Set codec-specific info.
- callback.onEncodedFrame(encodedImage, new CodecSpecificInfo());
- // Note that the callback may have retained the image.
- encodedImage.release();
+ return;
}
+
+ bitrateAdjuster.reportEncodedFrame(info.size);
+ if (adjustedBitrate != bitrateAdjuster.getAdjustedBitrateBps()) {
+ updateBitrate();
+ }
+
+ final boolean isKeyFrame = (info.flags & MediaCodec.BUFFER_FLAG_SYNC_FRAME) != 0;
+ if (isKeyFrame) {
+ Logging.d(TAG, "Sync frame generated");
+ }
+
+ final ByteBuffer frameBuffer;
+ final Runnable releaseCallback;
+ if (isKeyFrame && configBuffer != null) {
+ Logging.d(TAG,
+ "Prepending config buffer of size " + configBuffer.capacity()
+ + " to output buffer with offset " + info.offset + ", size " + info.size);
+ frameBuffer = ByteBuffer.allocateDirect(info.size + configBuffer.capacity());
+ configBuffer.rewind();
+ frameBuffer.put(configBuffer);
+ frameBuffer.put(outputBuffer);
+ frameBuffer.rewind();
+ codec.releaseOutputBuffer(index, /* render= */ false);
+ releaseCallback = null;
+ } else {
+ frameBuffer = outputBuffer.slice();
+ outputBuffersBusyCount.increment();
+ releaseCallback = () -> {
+ // This callback should not throw any exceptions since
+ // it may be called on an arbitrary thread.
+ // Check bug webrtc:11230 for more details.
+ try {
+ codec.releaseOutputBuffer(index, /* render= */ false);
+ } catch (Exception e) {
+ Logging.e(TAG, "releaseOutputBuffer failed", e);
+ }
+ outputBuffersBusyCount.decrement();
+ };
+ }
+
+ final EncodedImage.FrameType frameType = isKeyFrame ? EncodedImage.FrameType.VideoFrameKey
+ : EncodedImage.FrameType.VideoFrameDelta;
+
+ EncodedImage.Builder builder = outputBuilders.poll();
+ builder.setBuffer(frameBuffer, releaseCallback).setFrameType(frameType);
+
+ if (isEncodingStatisticsEnabled) {
+ MediaFormat format = codec.getOutputFormat(index);
+ if (format != null && format.containsKey(MediaFormat.KEY_VIDEO_QP_AVERAGE)) {
+ int qp = format.getInteger(MediaFormat.KEY_VIDEO_QP_AVERAGE);
+ builder.setQp(qp);
+ }
+ }
+
+ EncodedImage encodedImage = builder.createEncodedImage();
+ // TODO(mellem): Set codec-specific info.
+ callback.onEncodedFrame(encodedImage, new CodecSpecificInfo());
+ // Note that the callback may have retained the image.
+ encodedImage.release();
} catch (IllegalStateException e) {
Logging.e(TAG, "deliverOutput failed", e);
}
diff --git a/sdk/android/tests/src/org/webrtc/HardwareVideoEncoderTest.java b/sdk/android/tests/src/org/webrtc/HardwareVideoEncoderTest.java
index 36bfb20..919975d 100644
--- a/sdk/android/tests/src/org/webrtc/HardwareVideoEncoderTest.java
+++ b/sdk/android/tests/src/org/webrtc/HardwareVideoEncoderTest.java
@@ -10,15 +10,24 @@
package org.webrtc;
+import static android.media.MediaCodec.BUFFER_FLAG_CODEC_CONFIG;
+import static android.media.MediaCodec.BUFFER_FLAG_SYNC_FRAME;
import static com.google.common.truth.Truth.assertThat;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.ArgumentMatchers.nullable;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
+import static org.webrtc.VideoCodecMimeType.AV1;
+import static org.webrtc.VideoCodecMimeType.H264;
+import static org.webrtc.VideoCodecMimeType.H265;
+import static org.webrtc.VideoCodecMimeType.VP8;
+import static org.webrtc.VideoCodecMimeType.VP9;
import android.media.MediaCodec;
import android.media.MediaCodecInfo;
@@ -27,6 +36,7 @@
import android.os.Bundle;
import androidx.test.runner.AndroidJUnit4;
import java.nio.ByteBuffer;
+import java.nio.charset.Charset;
import java.util.HashMap;
import java.util.Map;
import org.junit.Before;
@@ -131,7 +141,7 @@
}
private class TestEncoderBuilder {
- private VideoCodecMimeType codecType = VideoCodecMimeType.VP8;
+ private VideoCodecMimeType codecType = VP8;
private BitrateAdjuster bitrateAdjuster = new BaseBitrateAdjuster();
private boolean isEncodingStatisticsSupported;
@@ -187,8 +197,7 @@
@Test
public void testInit() {
// Set-up.
- HardwareVideoEncoder encoder =
- new TestEncoderBuilder().setCodecType(VideoCodecMimeType.VP8).build();
+ HardwareVideoEncoder encoder = new TestEncoderBuilder().setCodecType(VP8).build();
// Test.
assertThat(encoder.initEncode(TEST_ENCODER_SETTINGS, mockEncoderCallback))
@@ -203,8 +212,7 @@
.isEqualTo(TEST_ENCODER_SETTINGS.width);
assertThat(mediaFormat.getInteger(MediaFormat.KEY_HEIGHT))
.isEqualTo(TEST_ENCODER_SETTINGS.height);
- assertThat(mediaFormat.getString(MediaFormat.KEY_MIME))
- .isEqualTo(VideoCodecMimeType.VP8.mimeType());
+ assertThat(mediaFormat.getString(MediaFormat.KEY_MIME)).isEqualTo(VP8.mimeType());
assertThat(fakeMediaCodecWrapper.getConfiguredFlags())
.isEqualTo(MediaCodec.CONFIGURE_FLAG_ENCODE);
@@ -231,7 +239,7 @@
fakeMediaCodecWrapper.addOutputData(CodecTestHelper.generateRandomData(100),
/* presentationTimestampUs= */ 0,
- /* flags= */ MediaCodec.BUFFER_FLAG_SYNC_FRAME);
+ /* flags= */ BUFFER_FLAG_SYNC_FRAME);
encoder.waitDeliverEncodedImage();
@@ -267,7 +275,7 @@
fakeMediaCodecWrapper.addOutputData(CodecTestHelper.generateRandomData(100),
/* presentationTimestampUs= */ 0,
- /* flags= */ MediaCodec.BUFFER_FLAG_SYNC_FRAME);
+ /* flags= */ BUFFER_FLAG_SYNC_FRAME);
encoder.waitDeliverEncodedImage();
@@ -454,4 +462,83 @@
assertThat(timestampCaptor.getAllValues())
.containsExactly(0L, frameDurationMs, 2 * frameDurationMs);
}
+
+ private void encodeWithConfigBuffer(VideoCodecMimeType codecType, boolean keyFrame,
+ boolean emptyConfig, String expected) throws InterruptedException {
+ String configData = emptyConfig ? "" : "config";
+ byte[] configBytes = configData.getBytes(Charset.defaultCharset());
+ byte[] frameBytes = "frame".getBytes(Charset.defaultCharset());
+ byte[] expectedBytes = expected.getBytes(Charset.defaultCharset());
+
+ TestEncoder encoder = new TestEncoderBuilder().setCodecType(codecType).build();
+ encoder.initEncode(TEST_ENCODER_SETTINGS, mockEncoderCallback);
+
+ encoder.encode(createTestVideoFrame(/* timestampNs= */ 0), ENCODE_INFO_DELTA_FRAME);
+
+ fakeMediaCodecWrapper.addOutputData(
+ configBytes, /* presentationTimestampUs= */ 0, /* flags= */ BUFFER_FLAG_CODEC_CONFIG);
+ encoder.waitDeliverEncodedImage();
+
+ fakeMediaCodecWrapper.addOutputData(frameBytes, /* presentationTimestampUs= */ 0,
+ /* flags= */ keyFrame ? BUFFER_FLAG_SYNC_FRAME : 0);
+ encoder.waitDeliverEncodedImage();
+
+ verify(mockEncoderCallback)
+ .onEncodedFrame(
+ argThat(
+ (EncodedImage encoded) -> encoded.buffer.equals(ByteBuffer.wrap(expectedBytes))),
+ nullable(CodecSpecificInfo.class));
+
+ assertThat(encoder.release()).isEqualTo(VideoCodecStatus.OK);
+ }
+
+ @Test
+ public void encode_vp8KeyFrame_nonEmptyConfig_configNotPrepended() throws InterruptedException {
+ encodeWithConfigBuffer(VP8, /*keyFrame=*/true, /* emptyConfig= */ false, "frame");
+ }
+
+ @Test
+ public void encode_vp9KeyFrame_nonEmptyConfig_configNotPrepended() throws InterruptedException {
+ encodeWithConfigBuffer(VP9, /*keyFrame=*/true, /* emptyConfig= */ false, "frame");
+ }
+
+ @Test
+ public void encode_av1KeyFrame_nonEmptyConfig_configNotPrepended() throws InterruptedException {
+ encodeWithConfigBuffer(AV1, /*keyFrame=*/true, /* emptyConfig= */ false, "frame");
+ }
+
+ @Test
+ public void encode_h264KeyFrame_nonEmptyConfig_configPrepended() throws InterruptedException {
+ encodeWithConfigBuffer(H264, /*keyFrame=*/true, /* emptyConfig= */ false, "configframe");
+ }
+
+ @Test
+ public void encode_h265KeyFrame_nonEmptyConfig_configPrepended() throws InterruptedException {
+ encodeWithConfigBuffer(H265, /*keyFrame=*/true, /* emptyConfig= */ false, "configframe");
+ }
+
+ @Test
+ public void encode_vp8DeltaFrame_nonEmptyConfig_configNotPrepended() throws InterruptedException {
+ encodeWithConfigBuffer(VP8, /*keyFrame=*/false, /* emptyConfig= */ false, "frame");
+ }
+
+ @Test
+ public void encode_vp9DeltaFrame_nonEmptyConfig_configNotPrepended() throws InterruptedException {
+ encodeWithConfigBuffer(VP9, /*keyFrame=*/false, /* emptyConfig= */ false, "frame");
+ }
+
+ @Test
+ public void encode_av1DeltaFrame_nonEmptyConfig_configNotPrepended() throws InterruptedException {
+ encodeWithConfigBuffer(AV1, /*keyFrame=*/false, /* emptyConfig= */ false, "frame");
+ }
+
+ @Test
+ public void encode_h264KeyFrame_emptyConfig_configNotPrepended() throws InterruptedException {
+ encodeWithConfigBuffer(H264, /*keyFrame=*/true, /* emptyConfig= */ true, "frame");
+ }
+
+ @Test
+ public void encode_h265KeyFrame_emptyConfig_configNotPrepended() throws InterruptedException {
+ encodeWithConfigBuffer(H265, /*keyFrame=*/true, /* emptyConfig= */ true, "frame");
+ }
}
