Fix handling non-tightly packed ByteBuffers in HardwareVideoDecoder.
Before this CL, there would be an out-of-bounds write in the ByteBuffer
copying when a decoded frame had height != sliceHeight.
Bug: webrtc:9194
Change-Id: Ibb80e5555e8f00d9e1fd4cb8a73f5e4ccd5a0b81
Tested: 640x360 loopback with eglContext == null in AppRTCMobile on Pixel.
Reviewed-on: https://webrtc-review.googlesource.com/74120
Commit-Queue: Sami Kalliomäki <sakal@webrtc.org>
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23184}diff --git a/sdk/android/BUILD.gn b/sdk/android/BUILD.gn
index d7f5b0d..0bdccd6 100644
--- a/sdk/android/BUILD.gn
+++ b/sdk/android/BUILD.gn
@@ -278,7 +278,6 @@
"api/org/webrtc/VideoEncoderFactory.java",
"api/org/webrtc/VideoEncoderFallback.java",
"api/org/webrtc/VideoFrame.java",
- "api/org/webrtc/VideoFrameDrawer.java",
"api/org/webrtc/VideoRenderer.java",
"api/org/webrtc/VideoSink.java",
"api/org/webrtc/VideoSource.java",
@@ -330,7 +329,6 @@
"src/jni/videoencoderwrapper.h",
"src/jni/videoframe.cc",
"src/jni/videoframe.h",
- "src/jni/videoframedrawer.cc",
"src/jni/videosink.cc",
"src/jni/videosink.h",
"src/jni/videotrack.cc",
diff --git a/sdk/android/api/org/webrtc/VideoFrameDrawer.java b/sdk/android/api/org/webrtc/VideoFrameDrawer.java
index d2c5be2..9909f37 100644
--- a/sdk/android/api/org/webrtc/VideoFrameDrawer.java
+++ b/sdk/android/api/org/webrtc/VideoFrameDrawer.java
@@ -13,8 +13,8 @@
import android.graphics.Matrix;
import android.graphics.Point;
import android.opengl.GLES20;
-import javax.annotation.Nullable;
import java.nio.ByteBuffer;
+import javax.annotation.Nullable;
/**
* Helper class to draw VideoFrames. Calls either drawer.drawOes, drawer.drawRgb, or
@@ -98,8 +98,8 @@
// Input is packed already.
packedByteBuffer = planes[i];
} else {
- nativeCopyPlane(
- planes[i], planeWidths[i], planeHeights[i], strides[i], copyBuffer, planeWidths[i]);
+ YuvHelper.copyPlane(
+ planes[i], strides[i], copyBuffer, planeWidths[i], planeWidths[i], planeHeights[i]);
packedByteBuffer = copyBuffer;
}
GLES20.glTexImage2D(GLES20.GL_TEXTURE_2D, 0, GLES20.GL_LUMINANCE, planeWidths[i],
@@ -229,8 +229,4 @@
yuvUploader.release();
lastI420Frame = null;
}
-
- // Helper native function to do a video frame plane copying.
- static native void nativeCopyPlane(
- ByteBuffer src, int width, int height, int srcStride, ByteBuffer dst, int dstStride);
}
diff --git a/sdk/android/api/org/webrtc/YuvHelper.java b/sdk/android/api/org/webrtc/YuvHelper.java
index 105584c..da94fec 100644
--- a/sdk/android/api/org/webrtc/YuvHelper.java
+++ b/sdk/android/api/org/webrtc/YuvHelper.java
@@ -97,6 +97,12 @@
dstChromaWidth, dstV, dstChromaWidth, srcWidth, srcHeight, rotationMode);
}
+ /** Helper method for copying a single colour plane. */
+ public static void copyPlane(
+ ByteBuffer src, int srcStride, ByteBuffer dst, int dstStride, int width, int height) {
+ nativeCopyPlane(src, srcStride, dst, dstStride, width, height);
+ }
+
public static void I420Copy(ByteBuffer srcY, int srcStrideY, ByteBuffer srcU, int srcStrideU,
ByteBuffer srcV, int srcStrideV, ByteBuffer dstY, int dstStrideY, ByteBuffer dstU,
int dstStrideU, ByteBuffer dstV, int dstStrideV, int width, int height) {
@@ -119,6 +125,8 @@
dstStrideU, dstV, dstStrideV, srcWidth, srcHeight, rotationMode);
}
+ private static native void nativeCopyPlane(
+ ByteBuffer src, int srcStride, ByteBuffer dst, int dstStride, int width, int height);
private static native void nativeI420Copy(ByteBuffer srcY, int srcStrideY, ByteBuffer srcU,
int srcStrideU, ByteBuffer srcV, int srcStrideV, ByteBuffer dstY, int dstStrideY,
ByteBuffer dstU, int dstStrideU, ByteBuffer dstV, int dstStrideV, int width, int height);
diff --git a/sdk/android/instrumentationtests/src/org/webrtc/HardwareVideoDecoderTest.java b/sdk/android/instrumentationtests/src/org/webrtc/HardwareVideoDecoderTest.java
index bcbfa4e..b3b9588 100644
--- a/sdk/android/instrumentationtests/src/org/webrtc/HardwareVideoDecoderTest.java
+++ b/sdk/android/instrumentationtests/src/org/webrtc/HardwareVideoDecoderTest.java
@@ -45,6 +45,12 @@
CLASS_PARAMS.add(new ParameterSet()
.value("VP8" /* codecType */, true /* useEglContext */)
.name("VP8WithEglContext"));
+ CLASS_PARAMS.add(new ParameterSet()
+ .value("H264" /* codecType */, false /* useEglContext */)
+ .name("H264WithoutEglContext"));
+ CLASS_PARAMS.add(new ParameterSet()
+ .value("H264" /* codecType */, true /* useEglContext */)
+ .name("H264WithEglContext"));
}
private final String codecType;
@@ -65,12 +71,12 @@
private static final boolean ENABLE_INTEL_VP8_ENCODER = true;
private static final boolean ENABLE_H264_HIGH_PROFILE = true;
private static final VideoEncoder.Settings ENCODER_SETTINGS =
- new VideoEncoder.Settings(1 /* core */, 640 /* width */, 480 /* height */, 300 /* kbps */,
+ new VideoEncoder.Settings(1 /* core */, TEST_FRAME_WIDTH, TEST_FRAME_HEIGHT, 300 /* kbps */,
30 /* fps */, true /* automaticResizeOn */);
private static final int DECODE_TIMEOUT_MS = 1000;
private static final VideoDecoder.Settings SETTINGS =
- new VideoDecoder.Settings(1 /* core */, 640 /* width */, 480 /* height */);
+ new VideoDecoder.Settings(1 /* core */, TEST_FRAME_WIDTH, TEST_FRAME_HEIGHT);
private static class MockDecodeCallback implements VideoDecoder.Callback {
private BlockingQueue<VideoFrame> frameQueue = new LinkedBlockingQueue<>();
diff --git a/sdk/android/instrumentationtests/src/org/webrtc/YuvHelperTest.java b/sdk/android/instrumentationtests/src/org/webrtc/YuvHelperTest.java
index e4217cc..8ee8921 100644
--- a/sdk/android/instrumentationtests/src/org/webrtc/YuvHelperTest.java
+++ b/sdk/android/instrumentationtests/src/org/webrtc/YuvHelperTest.java
@@ -60,6 +60,17 @@
@SmallTest
@Test
+ public void testCopyPlane() {
+ final int dstStride = TEST_WIDTH;
+ final ByteBuffer dst = ByteBuffer.allocateDirect(TEST_HEIGHT * dstStride);
+
+ YuvHelper.copyPlane(TEST_I420_Y, TEST_I420_STRIDE_Y, dst, dstStride, TEST_WIDTH, TEST_HEIGHT);
+
+ assertByteBufferContentEquals(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9}, dst);
+ }
+
+ @SmallTest
+ @Test
public void testI420Copy() {
final int dstStrideY = TEST_WIDTH;
final int dstStrideU = TEST_CHROMA_WIDTH;
diff --git a/sdk/android/src/java/org/webrtc/HardwareVideoDecoder.java b/sdk/android/src/java/org/webrtc/HardwareVideoDecoder.java
index a24da4d..4c235a9 100644
--- a/sdk/android/src/java/org/webrtc/HardwareVideoDecoder.java
+++ b/sdk/android/src/java/org/webrtc/HardwareVideoDecoder.java
@@ -15,13 +15,13 @@
import android.media.MediaCodecInfo.CodecCapabilities;
import android.media.MediaFormat;
import android.os.SystemClock;
-import javax.annotation.Nullable;
import android.view.Surface;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.concurrent.BlockingDeque;
import java.util.concurrent.LinkedBlockingDeque;
import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
import org.webrtc.ThreadUtils.ThreadChecker;
/** Android hardware video decoder. */
@@ -512,37 +512,57 @@
private VideoFrame.Buffer copyI420Buffer(
ByteBuffer buffer, int stride, int sliceHeight, int width, int height) {
+ if (stride % 2 != 0) {
+ throw new AssertionError("Stride is not divisible by two: " + stride);
+ }
+
+ // Note that the case with odd |sliceHeight| is handled in a special way.
+ // The chroma height contained in the payload is rounded down instead of
+ // up, making it one row less than what we expect in WebRTC. Therefore, we
+ // have to duplicate the last chroma rows for this case. Also, the offset
+ // between the Y plane and the U plane is unintuitive for this case. See
+ // http://bugs.webrtc.org/6651 for more info.
+ final int chromaWidth = (width + 1) / 2;
+ final int chromaHeight = (sliceHeight % 2 == 0) ? (height + 1) / 2 : height / 2;
+
final int uvStride = stride / 2;
final int yPos = 0;
+ final int yEnd = yPos + stride * height;
final int uPos = yPos + stride * sliceHeight;
- final int uEnd = uPos + uvStride * (sliceHeight / 2);
+ final int uEnd = uPos + uvStride * chromaHeight;
final int vPos = uPos + uvStride * sliceHeight / 2;
- final int vEnd = vPos + uvStride * (sliceHeight / 2);
+ final int vEnd = vPos + uvStride * chromaHeight;
VideoFrame.I420Buffer frameBuffer = JavaI420Buffer.allocate(width, height);
- ByteBuffer dataY = frameBuffer.getDataY();
+ buffer.limit(yEnd);
buffer.position(yPos);
- buffer.limit(uPos);
- dataY.put(buffer);
+ YuvHelper.copyPlane(
+ buffer.slice(), stride, frameBuffer.getDataY(), frameBuffer.getStrideY(), width, height);
- ByteBuffer dataU = frameBuffer.getDataU();
- buffer.position(uPos);
buffer.limit(uEnd);
- dataU.put(buffer);
- if (sliceHeight % 2 != 0) {
- buffer.position(uEnd - uvStride); // Repeat the last row.
- dataU.put(buffer);
+ buffer.position(uPos);
+ YuvHelper.copyPlane(buffer.slice(), uvStride, frameBuffer.getDataU(), frameBuffer.getStrideU(),
+ chromaWidth, chromaHeight);
+ if (sliceHeight % 2 == 1) {
+ buffer.position(uPos + uvStride * (chromaHeight - 1)); // Seek to beginning of last full row.
+
+ ByteBuffer dataU = frameBuffer.getDataU();
+ dataU.position(frameBuffer.getStrideU() * chromaHeight); // Seek to beginning of last row.
+ dataU.put(buffer); // Copy the last row.
}
- ByteBuffer dataV = frameBuffer.getDataV();
- buffer.position(vPos);
buffer.limit(vEnd);
- dataV.put(buffer);
- if (sliceHeight % 2 != 0) {
- buffer.position(vEnd - uvStride); // Repeat the last row.
- dataV.put(buffer);
+ buffer.position(vPos);
+ YuvHelper.copyPlane(buffer.slice(), uvStride, frameBuffer.getDataV(), frameBuffer.getStrideV(),
+ chromaWidth, chromaHeight);
+ if (sliceHeight % 2 == 1) {
+ buffer.position(vPos + uvStride * (chromaHeight - 1)); // Seek to beginning of last full row.
+
+ ByteBuffer dataV = frameBuffer.getDataV();
+ dataV.position(frameBuffer.getStrideV() * chromaHeight); // Seek to beginning of last row.
+ dataV.put(buffer); // Copy the last row.
}
return frameBuffer;
diff --git a/sdk/android/src/jni/videoframedrawer.cc b/sdk/android/src/jni/videoframedrawer.cc
deleted file mode 100644
index 46cf0e5..0000000
--- a/sdk/android/src/jni/videoframedrawer.cc
+++ /dev/null
@@ -1,52 +0,0 @@
-/*
- * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
- *
- * Use of this source code is governed by a BSD-style license
- * that can be found in the LICENSE file in the root of the source
- * tree. An additional intellectual property rights grant can be found
- * in the file PATENTS. All contributing project authors may
- * be found in the AUTHORS file in the root of the source tree.
- */
-
-#include <jni.h>
-
-#include "sdk/android/generated_video_jni/jni/VideoFrameDrawer_jni.h"
-#include "sdk/android/native_api/jni/scoped_java_ref.h"
-
-namespace webrtc {
-namespace jni {
-
-static void JNI_VideoFrameDrawer_CopyPlane(
- JNIEnv* jni,
- const JavaParamRef<jclass>&,
- const JavaParamRef<jobject>& j_src_buffer,
- jint width,
- jint height,
- jint src_stride,
- const JavaParamRef<jobject>& j_dst_buffer,
- jint dst_stride) {
- size_t src_size = jni->GetDirectBufferCapacity(j_src_buffer.obj());
- size_t dst_size = jni->GetDirectBufferCapacity(j_dst_buffer.obj());
- RTC_CHECK(src_stride >= width) << "Wrong source stride " << src_stride;
- RTC_CHECK(dst_stride >= width) << "Wrong destination stride " << dst_stride;
- RTC_CHECK(src_size >= src_stride * height)
- << "Insufficient source buffer capacity " << src_size;
- RTC_CHECK(dst_size >= dst_stride * height)
- << "Insufficient destination buffer capacity " << dst_size;
- uint8_t* src = reinterpret_cast<uint8_t*>(
- jni->GetDirectBufferAddress(j_src_buffer.obj()));
- uint8_t* dst = reinterpret_cast<uint8_t*>(
- jni->GetDirectBufferAddress(j_dst_buffer.obj()));
- if (src_stride == dst_stride) {
- memcpy(dst, src, src_stride * height);
- } else {
- for (int i = 0; i < height; i++) {
- memcpy(dst, src, width);
- src += src_stride;
- dst += dst_stride;
- }
- }
-}
-
-} // namespace jni
-} // namespace webrtc
diff --git a/sdk/android/src/jni/yuvhelper.cc b/sdk/android/src/jni/yuvhelper.cc
index 95bbed2..1acd142 100644
--- a/sdk/android/src/jni/yuvhelper.cc
+++ b/sdk/android/src/jni/yuvhelper.cc
@@ -13,10 +13,27 @@
#include "sdk/android/generated_video_jni/jni/YuvHelper_jni.h"
#include "sdk/android/src/jni/jni_helpers.h"
#include "third_party/libyuv/include/libyuv/convert.h"
+#include "third_party/libyuv/include/libyuv/planar_functions.h"
namespace webrtc {
namespace jni {
+void JNI_YuvHelper_CopyPlane(JNIEnv* jni,
+ const JavaParamRef<jclass>&,
+ const JavaParamRef<jobject>& j_src,
+ jint src_stride,
+ const JavaParamRef<jobject>& j_dst,
+ jint dst_stride,
+ jint width,
+ jint height) {
+ const uint8_t* src =
+ static_cast<const uint8_t*>(jni->GetDirectBufferAddress(j_src.obj()));
+ uint8_t* dst =
+ static_cast<uint8_t*>(jni->GetDirectBufferAddress(j_dst.obj()));
+
+ libyuv::CopyPlane(src, src_stride, dst, dst_stride, width, height);
+}
+
void JNI_YuvHelper_I420Copy(JNIEnv* jni,
const JavaParamRef<jclass>&,
const JavaParamRef<jobject>& j_src_y,