From 44f850e6f5ad47cf8dd72469dc475b6f4ef1b364 Mon Sep 17 00:00:00 2001 From: Alex Andres Date: Fri, 23 Oct 2026 00:17:24 +0200 Subject: [PATCH] fix: size YUY2, UYVY and NV12 frames of odd width in whole pixel pairs YUY2 and UYVY rows, and NV12 and NV21 chroma rows, end with a whole pair, so a frame of odd width was written past the buffer it was checked against, and written with other strides than it is read with. Sizes are now computed in 64 bits, and empty or oversized frames and unknown formats are rejected. --- docs/tools/video/video-buffer-converter.md | 12 +- .../main/cpp/src/JNI_VideoBufferConverter.cpp | 125 ++++++--- .../media/video/VideoBufferConverter.java | 8 + .../media/video/VideoBufferConverterTest.java | 255 +++++++++++++++++- 4 files changed, 351 insertions(+), 49 deletions(-) diff --git a/docs/tools/video/video-buffer-converter.md b/docs/tools/video/video-buffer-converter.md index 51d4837c..def1dcbc 100644 --- a/docs/tools/video/video-buffer-converter.md +++ b/docs/tools/video/video-buffer-converter.md @@ -31,17 +31,19 @@ The target/source pixel layout is selected with `dev.onvoid.webrtc.media.FourCC` - `FourCC.RGBA` – 4 bytes per pixel, RGBA order (commonly used with BufferedImage TYPE_4BYTE_ABGR interop, see example below) - `FourCC.ARGB`, `FourCC.ABGR`, `FourCC.BGRA` – other 32-bit packed variants - `FourCC.NV12`, `FourCC.NV21` – 4:2:0 semi-planar YUV formats +- `FourCC.YUY2`, `FourCC.UYVY` – 4:2:2 packed YUV formats, 4 bytes per pair of pixels Consult the FourCC enum in your version for the complete list. ## Buffer sizing -You must allocate destination buffers large enough for the chosen format: -- For 32-bit RGBA-like formats: `width * height * 4` bytes -- For NV12/NV21: `width * height * 3 / 2` bytes -- For other layouts, compute according to their specification +Frames are stored without row padding, except where a format groups pixels in pairs: a YUY2 or UYVY row, and an NV12 or NV21 chroma row, always ends with a whole pair, so an odd width is rounded up to the next pair. A buffer must hold at least: +- `ARGB`, `BGRA`, `ABGR`, `RGBA`: `width * height * 4` bytes +- `BG24`: `width * height * 3` bytes +- `I420`, `NV12`, `NV21`: `width * height + 2 * ((width + 1) / 2) * ((height + 1) / 2)` bytes +- `YUY2`, `UYVY`: `4 * ((width + 1) / 2) * height` bytes, `4 * ((width + 1) / 2)` per row -Attempting to convert into undersized buffers will result in an error. +Both directions use the same layout, so a frame written by `convertFromI420` reads back with `convertToI420`. Converting into or from a smaller buffer throws, and touches no byte of it. ## Example: Convert VideoFrame to BufferedImage diff --git a/webrtc-jni/src/main/cpp/src/JNI_VideoBufferConverter.cpp b/webrtc-jni/src/main/cpp/src/JNI_VideoBufferConverter.cpp index 9ee67037..3eb28064 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_VideoBufferConverter.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_VideoBufferConverter.cpp @@ -23,10 +23,25 @@ #include "libyuv/convert.h" #include "libyuv/video_common.h" +#include +#include #include -size_t CalcBufferSize(int width, int height, int fourCC) { - size_t bufferSize = 0; +// The size of a frame in the layout libyuv::ConvertToI420 reads, which +// ConvertFromI420 below writes. YUY2 and UYVY rows, and NV12 and NV21 chroma +// rows, hold whole pixel pairs, so an odd width is rounded up to the next pair. +// Returns false for an empty frame, an unsupported format, or a width beyond +// the INT_MAX / 4 that ConvertToI420 accepts. With that width, no row of 4 +// bytes per pixel overflows an int, and no frame size overflows 64 bits. +static bool CalcBufferSize(jint width, jint height, jint fourCC, uint64_t & size) +{ + if (width <= 0 || height <= 0 || width > INT_MAX / 4) { + return false; + } + + const uint64_t pixels = static_cast(width) * height; + const uint64_t pairs = (static_cast(width) + 1) / 2; + const uint64_t chromaRows = (static_cast(height) + 1) / 2; switch (fourCC) { case libyuv::FOURCC_I420: @@ -34,33 +49,85 @@ size_t CalcBufferSize(int width, int height, int fourCC) { case libyuv::FOURCC_NV21: case libyuv::FOURCC_IYUV: case libyuv::FOURCC_YV12: - bufferSize = width * height + ((width + 1) >> 1) * ((height + 1) >> 1) * 2; - break; + size = pixels + pairs * chromaRows * 2; + return true; case libyuv::FOURCC_R444: case libyuv::FOURCC_RGBP: case libyuv::FOURCC_RGBO: + size = pixels * 2; + return true; + case libyuv::FOURCC_YUY2: case libyuv::FOURCC_UYVY: - bufferSize = width * height * 2; - break; + size = pairs * 4 * height; + return true; case libyuv::FOURCC_24BG: - bufferSize = width * height * 3; - break; + size = pixels * 3; + return true; case libyuv::FOURCC_ARGB: case libyuv::FOURCC_ABGR: case libyuv::FOURCC_BGRA: case libyuv::FOURCC_RGBA: - bufferSize = width * height * 4; - break; + size = pixels * 4; + return true; default: - break; + return false; + } +} + +// Throws and returns false unless a frame of the given size and format fits into length bytes. +static bool CheckBufferSize(JNIEnv * env, jint width, jint height, jint fourCC, uint64_t length) +{ + uint64_t requiredSize; + + if (!CalcBufferSize(width, height, fourCC, requiredSize)) { + env->Throw(jni::JavaRuntimeException(env, "Invalid frame [width %d, height %d, format 0x%08x]", + width, height, static_cast(fourCC))); + return false; + } + if (length < requiredSize) { + env->Throw(jni::JavaRuntimeException(env, "Insufficient buffer size [has %llu, need %llu]", + static_cast(length), static_cast(requiredSize))); + return false; } - return bufferSize; + return true; +} + +// libyuv::ConvertFromI420 with the strides of CalcBufferSize. On its own it +// writes YUY2 and UYVY rows width * 2 bytes apart, and NV12 and NV21 chroma +// rows width bytes apart, though each of these rows ends with a whole pair. +static int ConvertFromI420(const uint8_t * srcY, int srcStrideY, const uint8_t * srcU, int srcStrideU, + const uint8_t * srcV, int srcStrideV, uint8_t * dst, int width, int height, uint32_t fourCC) +{ + const int alignedWidth = (width + 1) & ~1; + const ptrdiff_t lumaSize = static_cast(width) * height; + + switch (fourCC) { + case libyuv::FOURCC_YUY2: + return libyuv::I420ToYUY2(srcY, srcStrideY, srcU, srcStrideU, srcV, srcStrideV, + dst, alignedWidth * 2, width, height); + + case libyuv::FOURCC_UYVY: + return libyuv::I420ToUYVY(srcY, srcStrideY, srcU, srcStrideU, srcV, srcStrideV, + dst, alignedWidth * 2, width, height); + + case libyuv::FOURCC_NV12: + return libyuv::I420ToNV12(srcY, srcStrideY, srcU, srcStrideU, srcV, srcStrideV, + dst, width, dst + lumaSize, alignedWidth, width, height); + + case libyuv::FOURCC_NV21: + return libyuv::I420ToNV21(srcY, srcStrideY, srcU, srcStrideU, srcV, srcStrideV, + dst, width, dst + lumaSize, alignedWidth, width, height); + + default: + return libyuv::ConvertFromI420(srcY, srcStrideY, srcU, srcStrideU, srcV, srcStrideV, + dst, 0, width, height, fourCC); + } } // Throws and returns false unless [offset, offset + length) lies within the array. @@ -87,11 +154,7 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_video_VideoBufferConverter_I return; } - size_t requiredSize = CalcBufferSize(width, height, fourCC); - - if (static_cast(dstLength) < requiredSize) { - env->Throw(jni::JavaRuntimeException(env, "Insufficient buffer size [has %d, need %zd]", - dstLength, requiredSize)); + if (!CheckBufferSize(env, width, height, fourCC, static_cast(dstLength))) { return; } @@ -109,8 +172,8 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_video_VideoBufferConverter_I } uint8_t * dstPtr = reinterpret_cast(arrayPtr.get()) + dstOffset; - const int conversionResult = libyuv::ConvertFromI420(srcY, srcStrideY, srcU, srcStrideU, - srcV, srcStrideV, dstPtr, 0, width, height, static_cast(fourCC)); + const int conversionResult = ConvertFromI420(srcY, srcStrideY, srcU, srcStrideU, + srcV, srcStrideV, dstPtr, width, height, static_cast(fourCC)); if (conversionResult < 0) { env->Throw(jni::JavaRuntimeException(env, "Failed to convert buffer to I420: %d", @@ -133,17 +196,14 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_video_VideoBufferConverter_I uint8_t * address = static_cast(env->GetDirectBufferAddress(dst)); if (address != NULL) { - size_t bufferLength = env->GetDirectBufferCapacity(dst); - size_t requiredSize = CalcBufferSize(width, height, fourCC); + jlong bufferLength = env->GetDirectBufferCapacity(dst); - if (bufferLength < requiredSize) { - env->Throw(jni::JavaRuntimeException(env, "Insufficient buffer size [has %zd, need %zd]", - bufferLength, requiredSize)); + if (!CheckBufferSize(env, width, height, fourCC, static_cast(bufferLength))) { return; } - const int conversionResult = libyuv::ConvertFromI420(srcY, srcStrideY, srcU, srcStrideU, - srcV, srcStrideV, address, 0, width, height, static_cast(fourCC)); + const int conversionResult = ConvertFromI420(srcY, srcStrideY, srcU, srcStrideU, + srcV, srcStrideV, address, width, height, static_cast(fourCC)); if (conversionResult < 0) { env->Throw(jni::JavaRuntimeException(env, "Failed to convert buffer to I420: %d", @@ -165,11 +225,7 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_video_VideoBufferConverter_b return; } - size_t requiredSize = CalcBufferSize(width, height, fourCC); - - if (static_cast(srcLength) < requiredSize) { - env->Throw(jni::JavaRuntimeException(env, "Insufficient buffer size [has %d, need %zd]", - srcLength, requiredSize)); + if (!CheckBufferSize(env, width, height, fourCC, static_cast(srcLength))) { return; } @@ -215,12 +271,9 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_video_VideoBufferConverter_d const uint8_t * address = static_cast(env->GetDirectBufferAddress(src)); if (address != NULL) { - size_t bufferLength = env->GetDirectBufferCapacity(src); - size_t requiredSize = CalcBufferSize(width, height, fourCC); + jlong bufferLength = env->GetDirectBufferCapacity(src); - if (bufferLength < requiredSize) { - env->Throw(jni::JavaRuntimeException(env, "Insufficient buffer size [has %zd, need %zd]", - bufferLength, requiredSize)); + if (!CheckBufferSize(env, width, height, fourCC, static_cast(bufferLength))) { return; } diff --git a/webrtc/src/main/java/dev/onvoid/webrtc/media/video/VideoBufferConverter.java b/webrtc/src/main/java/dev/onvoid/webrtc/media/video/VideoBufferConverter.java index 0369463a..05c1a939 100644 --- a/webrtc/src/main/java/dev/onvoid/webrtc/media/video/VideoBufferConverter.java +++ b/webrtc/src/main/java/dev/onvoid/webrtc/media/video/VideoBufferConverter.java @@ -25,6 +25,14 @@ * Utility methods to convert between I420 video frame buffers and other pixel * formats represented by a FourCC. Delegates actual conversion work to native * methods for performance. + *

+ * A frame is stored without row padding, except where a format groups pixels + * in pairs: a {@link FourCC#YUY2} or {@link FourCC#UYVY} row, and an + * {@link FourCC#NV12} or {@link FourCC#NV21} chroma row, always ends with a + * whole pair. With an odd width, a YUY2 or UYVY row takes + * {@code 4 * ((width + 1) / 2)} bytes, and an NV12 or NV21 chroma row + * {@code 2 * ((width + 1) / 2)}. Conversions in both directions use this + * layout, and fail when the buffer is smaller than the frame. * * @author Alex Andres */ diff --git a/webrtc/src/test/java/dev/onvoid/webrtc/media/video/VideoBufferConverterTest.java b/webrtc/src/test/java/dev/onvoid/webrtc/media/video/VideoBufferConverterTest.java index 285db9ac..d183c81e 100644 --- a/webrtc/src/test/java/dev/onvoid/webrtc/media/video/VideoBufferConverterTest.java +++ b/webrtc/src/test/java/dev/onvoid/webrtc/media/video/VideoBufferConverterTest.java @@ -22,11 +22,15 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.List; import dev.onvoid.webrtc.TestBase; import dev.onvoid.webrtc.media.FourCC; +import org.junit.jupiter.api.DynamicTest; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestFactory; class VideoBufferConverterTest extends TestBase { @@ -34,6 +38,11 @@ class VideoBufferConverterTest extends TestBase { private static final int HEIGHT = 8; private static final int RGBA_SIZE = 4 * WIDTH * HEIGHT; + /** Formats whose rows, or chroma rows, are made of whole pixel pairs. */ + private static final FourCC[] PAIRED_FORMATS = { + FourCC.YUY2, FourCC.UYVY, FourCC.NV12, FourCC.NV21 + }; + @Test void convertFromI420ToByteArray() throws Exception { NativeI420Buffer i420 = NativeI420Buffer.allocate(WIDTH, HEIGHT); @@ -254,6 +263,100 @@ void convertFromSmallNonDirectWindowToI420() throws Exception { convertFromSmallWindowToI420(new Window(false, RGBA_SIZE - 1)); } + @TestFactory + List convertOddWidthFromI420() { + return pairedFrames((fourCC, width, height, direct) -> { + NativeI420Buffer i420 = NativeI420Buffer.allocate(width, height); + NativeI420Buffer back = NativeI420Buffer.allocate(width, height); + initializeI420Buffer(i420); + Window window = new Window(direct, pairedFrameSize(fourCC, width, height)); + + try { + VideoBufferConverter.convertFromI420(i420, window.buffer, fourCC); + + window.verifyUnmoved(); + window.verifyGuards(); + + // Written and read with the same row and chroma strides. + VideoBufferConverter.convertToI420(window.buffer, back, fourCC); + verifyI420Equals(i420, back); + } + finally { + i420.release(); + back.release(); + } + }); + } + + @TestFactory + List convertOddWidthFromI420ToSmallWindow() { + return pairedFrames((fourCC, width, height, direct) -> { + NativeI420Buffer i420 = NativeI420Buffer.allocate(width, height); + initializeI420Buffer(i420); + Window window = new Window(direct, pairedFrameSize(fourCC, width, height) - 1); + + try { + assertThrows(RuntimeException.class, () -> { + VideoBufferConverter.convertFromI420(i420, window.buffer, fourCC); + }); + + window.verifyUnmoved(); + window.verifyGuards(); + window.verifyUntouched(); + } + finally { + i420.release(); + } + }); + } + + @TestFactory + List convertOddWidthFromSmallWindowToI420() { + return pairedFrames((fourCC, width, height, direct) -> { + NativeI420Buffer i420 = NativeI420Buffer.allocate(width, height); + Window window = new Window(direct, pairedFrameSize(fourCC, width, height) - 1); + + try { + assertThrows(RuntimeException.class, () -> { + VideoBufferConverter.convertToI420(window.buffer, i420, fourCC); + }); + + window.verifyUnmoved(); + } + finally { + i420.release(); + } + }); + } + + @Test + void convertInvalidFrameSize() { + // 2^15 x 2^17 RGBA pixels take 2^34 bytes, 0 in 32-bit arithmetic. + int[][] sizes = { + { 1 << 15, 1 << 17 }, { Integer.MAX_VALUE, 1 }, { 1, Integer.MAX_VALUE }, + { 0, 1 }, { 1, 0 }, { 1, -1 }, { -1, 1 } + }; + + for (int[] size : sizes) { + I420Buffer frame = new UnbackedI420Buffer(size[0], size[1]); + byte[] array = new byte[16]; + ByteBuffer direct = ByteBuffer.allocateDirect(16); + + for (FourCC fourCC : FourCC.values()) { + String name = fourCC + " " + size[0] + "x" + size[1]; + + assertThrows(RuntimeException.class, + () -> VideoBufferConverter.convertFromI420(frame, array, fourCC), name); + assertThrows(RuntimeException.class, + () -> VideoBufferConverter.convertFromI420(frame, direct, fourCC), name); + assertThrows(RuntimeException.class, + () -> VideoBufferConverter.convertToI420(array, frame, fourCC), name); + assertThrows(RuntimeException.class, + () -> VideoBufferConverter.convertToI420(direct, frame, fourCC), name); + } + } + } + private void convertFromI420ToWindow(Window window) throws Exception { NativeI420Buffer i420 = NativeI420Buffer.allocate(WIDTH, HEIGHT); initializeI420Buffer(i420); @@ -319,14 +422,17 @@ private void initializeI420Buffer(I420Buffer i420) { ByteBuffer dataV = i420.getDataV(); int strideV = i420.getStrideV(); + int width = i420.getWidth(); + int height = i420.getHeight(); + byte value = 0; - for (int y = 0; y < HEIGHT; y++) { - for (int x = 0; x < WIDTH; x++) { + for (int y = 0; y < height; y++) { + for (int x = 0; x < width; x++) { dataY.put(y * strideY + x, value++); } } - for (int y = 0; y < HEIGHT / 2; y++) { - for (int x = 0; x < WIDTH / 2; x++) { + for (int y = 0; y < (height + 1) / 2; y++) { + for (int x = 0; x < (width + 1) / 2; x++) { dataU.put(y * strideU + x, value++); dataV.put(y * strideV + x, value++); } @@ -458,14 +564,17 @@ private void verifyRGBABuffer(ByteBuffer rgba, boolean expectingChanges) { } private void verifyI420Equals(I420Buffer expected, I420Buffer actual) { - for (int y = 0; y < HEIGHT; y++) { - for (int x = 0; x < WIDTH; x++) { + int width = expected.getWidth(); + int height = expected.getHeight(); + + for (int y = 0; y < height; y++) { + for (int x = 0; x < width; x++) { assertEquals(expected.getDataY().get(y * expected.getStrideY() + x), actual.getDataY().get(y * actual.getStrideY() + x)); } } - for (int y = 0; y < HEIGHT / 2; y++) { - for (int x = 0; x < WIDTH / 2; x++) { + for (int y = 0; y < (height + 1) / 2; y++) { + for (int x = 0; x < (width + 1) / 2; x++) { assertEquals(expected.getDataU().get(y * expected.getStrideU() + x), actual.getDataU().get(y * actual.getStrideU() + x)); assertEquals(expected.getDataV().get(y * expected.getStrideV() + x), @@ -474,6 +583,136 @@ private void verifyI420Equals(I420Buffer expected, I420Buffer actual) { } } + /** + * One test per paired format, odd width, height and kind of buffer. + */ + private static List pairedFrames(PairedFrameTest test) { + List tests = new ArrayList<>(); + + for (FourCC fourCC : PAIRED_FORMATS) { + for (int width : new int[] { 1, 3, 5 }) { + for (int height : new int[] { 1, 2, 3 }) { + for (boolean direct : new boolean[] { true, false }) { + String name = fourCC + " " + width + "x" + height + + (direct ? " direct" : " heap"); + + tests.add(DynamicTest.dynamicTest(name, + () -> test.run(fourCC, width, height, direct))); + } + } + } + } + + return tests; + } + + /** + * The size of a frame in a paired format, the last pair of a row complete. + */ + private static int pairedFrameSize(FourCC fourCC, int width, int height) { + int pairs = (width + 1) / 2; + + switch (fourCC) { + case YUY2: + case UYVY: + return pairs * 4 * height; + case NV12: + case NV21: + return width * height + pairs * 2 * ((height + 1) / 2); + default: + throw new IllegalArgumentException(fourCC.name()); + } + } + + + + @FunctionalInterface + private interface PairedFrameTest { + + void run(FourCC fourCC, int width, int height, boolean direct) throws Exception; + + } + + + + /** + * An I420 buffer that claims a size its planes do not have, so that a + * conversion must fail before it reads or writes them. + */ + private static class UnbackedI420Buffer implements I420Buffer { + + private final ByteBuffer plane = ByteBuffer.allocateDirect(16); + + private final int width; + + private final int height; + + + UnbackedI420Buffer(int width, int height) { + this.width = width; + this.height = height; + } + + @Override + public ByteBuffer getDataY() { + return plane; + } + + @Override + public ByteBuffer getDataU() { + return plane; + } + + @Override + public ByteBuffer getDataV() { + return plane; + } + + @Override + public int getStrideY() { + return 1; + } + + @Override + public int getStrideU() { + return 1; + } + + @Override + public int getStrideV() { + return 1; + } + + @Override + public int getWidth() { + return width; + } + + @Override + public int getHeight() { + return height; + } + + @Override + public I420Buffer toI420() { + return this; + } + + @Override + public VideoFrameBuffer cropAndScale(int cropX, int cropY, int cropWidth, + int cropHeight, int scaleWidth, int scaleHeight) { + throw new UnsupportedOperationException(); + } + + @Override + public void retain() { + } + + @Override + public void release() { + } + } + /**