-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[camera_android_x] Refactor ImageProxyUtils.planesToNV21 to prevent buffer issues #10163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a960b6a
8041ec7
a5acd21
3452f67
cf488e6
4d711a3
cc365f5
8bbb4a3
e3175fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,55 +18,80 @@ public class ImageProxyUtils { | |
*/ | ||
@NonNull | ||
public static ByteBuffer planesToNV21(@NonNull List<PlaneProxy> planes, int width, int height) { | ||
if (!areUVPlanesNV21(planes, width, height)) { | ||
if (planes.size() < 3) { | ||
throw new IllegalArgumentException( | ||
"Provided UV planes are not in NV21 layout and thus cannot be converted."); | ||
"The plane list must contain at least 3 planes (Y, U, V)."); | ||
} | ||
|
||
int imageSize = width * height; | ||
int nv21Size = imageSize + 2 * (imageSize / 4); | ||
byte[] nv21Bytes = new byte[nv21Size]; | ||
PlaneProxy yPlane = planes.get(0); | ||
PlaneProxy uPlane = planes.get(1); | ||
PlaneProxy vPlane = planes.get(2); | ||
|
||
// Copy Y plane. | ||
ByteBuffer yBuffer = planes.get(0).getBuffer(); | ||
yBuffer.rewind(); | ||
yBuffer.get(nv21Bytes, 0, imageSize); | ||
|
||
// Copy interleaved VU plane (NV21 layout). | ||
ByteBuffer vBuffer = planes.get(2).getBuffer(); | ||
ByteBuffer uBuffer = planes.get(1).getBuffer(); | ||
ByteBuffer yBuffer = yPlane.getBuffer(); | ||
ByteBuffer uBuffer = uPlane.getBuffer(); | ||
ByteBuffer vBuffer = vPlane.getBuffer(); | ||
|
||
vBuffer.rewind(); | ||
// Rewind buffers to start to ensure full read. | ||
yBuffer.rewind(); | ||
uBuffer.rewind(); | ||
vBuffer.get(nv21Bytes, imageSize, 1); | ||
uBuffer.get(nv21Bytes, imageSize + 1, 2 * imageSize / 4 - 1); | ||
|
||
return ByteBuffer.wrap(nv21Bytes); | ||
} | ||
|
||
public static boolean areUVPlanesNV21(@NonNull List<PlaneProxy> planes, int width, int height) { | ||
int imageSize = width * height; | ||
|
||
ByteBuffer uBuffer = planes.get(1).getBuffer(); | ||
ByteBuffer vBuffer = planes.get(2).getBuffer(); | ||
|
||
// Backup buffer properties. | ||
int vBufferPosition = vBuffer.position(); | ||
int uBufferLimit = uBuffer.limit(); | ||
|
||
// Advance the V buffer by 1 byte, since the U buffer will not contain the first V value. | ||
vBuffer.position(vBufferPosition + 1); | ||
// Chop off the last byte of the U buffer, since the V buffer will not contain the last U value. | ||
uBuffer.limit(uBufferLimit - 1); | ||
vBuffer.rewind(); | ||
|
||
// Check that the buffers are equal and have the expected number of elements. | ||
boolean areNV21 = | ||
(vBuffer.remaining() == (2 * imageSize / 4 - 2)) && (vBuffer.compareTo(uBuffer) == 0); | ||
// Allocate a byte array for the NV21 frame. | ||
// NV21 = Y plane + interleaved VU plane. | ||
// Y = width * height; VU = (width * height) / 2 (4:2:0 subsampling). | ||
// If the Y plane includes padding, ySize may be larger than width*height, | ||
// but only the valid Y bytes are copied, so output size remains correct. | ||
int ySize = yBuffer.remaining(); | ||
byte[] nv21Bytes = new byte[ySize + (width * height / 2)]; | ||
int position = 0; | ||
|
||
int yRowStride = yPlane.getRowStride(); | ||
if (yRowStride == width) { | ||
// If no padding, copy entire Y plane at once. | ||
yBuffer.get(nv21Bytes, 0, ySize); | ||
position = ySize; | ||
} else { | ||
// Copy row by row if padding exists. | ||
byte[] row = new byte[width]; | ||
for (int rowIndex = 0; rowIndex < height; rowIndex++) { | ||
yBuffer.get(row, 0, width); | ||
System.arraycopy(row, 0, nv21Bytes, position, width); | ||
position += width; | ||
// Adjust buffer position to start of next row. | ||
// After reading 'width' bytes, move ahead by (yRowStride - width) | ||
// to skip any padding bytes at the end of the current row. | ||
if (rowIndex < height - 1) { | ||
yBuffer.position(yBuffer.position() - width + yRowStride); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this line is what makes the case with padding different from the case without passing. Can you explain why? Maybe leave a comment too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, exactly. I'll add a brief comment to the code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha thanks! |
||
} | ||
} | ||
} | ||
|
||
// Restore buffers to their initial state. | ||
vBuffer.position(vBufferPosition); | ||
uBuffer.limit(uBufferLimit); | ||
int uRowStride = uPlane.getRowStride(); | ||
int vRowStride = vPlane.getRowStride(); | ||
int uPixelStride = uPlane.getPixelStride(); | ||
int vPixelStride = vPlane.getPixelStride(); | ||
|
||
byte[] uRowBuffer = new byte[uRowStride]; | ||
byte[] vRowBuffer = new byte[vRowStride]; | ||
|
||
// Read full row from U and V planes into temporary buffers. | ||
for (int row = 0; row < height / 2; row++) { | ||
int uRemaining = Math.min(uBuffer.remaining(), uRowStride); | ||
int vRemaining = Math.min(vBuffer.remaining(), vRowStride); | ||
|
||
uBuffer.get(uRowBuffer, 0, uRemaining); | ||
vBuffer.get(vRowBuffer, 0, vRemaining); | ||
|
||
// Interleave V and U chroma data into the NV21 buffer. | ||
// In NV21, chroma bytes follow the Y plane in repeating VU pairs (VUVU...). | ||
for (int col = 0; col < width / 2; col++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: can you add a comment to signal (if I'm understanding correctly) that this is adding the overlapping V and U buffers to the NV21 buffer? |
||
int vIndex = col * vPixelStride; | ||
int uIndex = col * uPixelStride; | ||
nv21Bytes[position++] = vRowBuffer[vIndex]; | ||
nv21Bytes[position++] = uRowBuffer[uIndex]; | ||
} | ||
} | ||
|
||
return areNV21; | ||
return ByteBuffer.wrap(nv21Bytes); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | |||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,9 +5,13 @@ | ||||||||||||||||
package io.flutter.plugins.camerax; | |||||||||||||||||
|
|||||||||||||||||
import static org.junit.Assert.assertArrayEquals; | |||||||||||||||||
import static org.junit.Assert.assertEquals; | |||||||||||||||||
import static org.junit.Assert.assertThrows; | |||||||||||||||||
import static org.mockito.Mockito.mock; | |||||||||||||||||
import static org.mockito.Mockito.when; | |||||||||||||||||
|
|||||||||||||||||
import androidx.camera.core.ImageProxy.PlaneProxy; | |||||||||||||||||
import java.nio.BufferUnderflowException; | |||||||||||||||||
import java.nio.ByteBuffer; | |||||||||||||||||
import java.util.Arrays; | |||||||||||||||||
import java.util.List; | |||||||||||||||||
|
@@ -35,18 +39,19 @@ public void planesToNV21_throwsExceptionForNonNV21Layout() { | ||||||||||||||||
List<PlaneProxy> planes = Arrays.asList(yPlane, uPlane, vPlane); | |||||||||||||||||
|
|||||||||||||||||
assertThrows( | |||||||||||||||||
IllegalArgumentException.class, () -> ImageProxyUtils.planesToNV21(planes, width, height)); | |||||||||||||||||
BufferUnderflowException.class, () -> ImageProxyUtils.planesToNV21(planes, width, height)); | |||||||||||||||||
} | |||||||||||||||||
|
|||||||||||||||||
@Test | |||||||||||||||||
public void planesToNV21_returnsExpectedBufferWhenPlanesAreNV21Compatible() { | |||||||||||||||||
int width = 4; | |||||||||||||||||
int height = 2; | |||||||||||||||||
int imageSize = width * height; // 8 | |||||||||||||||||
|
|||||||||||||||||
// Y plane. | |||||||||||||||||
byte[] y = new byte[] {0, 1, 2, 3, 4, 5, 6, 7}; | |||||||||||||||||
PlaneProxy yPlane = mockPlaneProxyWithData(y); | |||||||||||||||||
when(yPlane.getPixelStride()).thenReturn(1); | |||||||||||||||||
when(yPlane.getRowStride()).thenReturn(width); | |||||||||||||||||
|
|||||||||||||||||
// U and V planes in NV21 format. Both have 2 bytes that are overlapping (5, 7). | |||||||||||||||||
ByteBuffer vBuffer = ByteBuffer.wrap(new byte[] {9, 5, 7}); | |||||||||||||||||
|
@@ -58,6 +63,12 @@ public void planesToNV21_returnsExpectedBufferWhenPlanesAreNV21Compatible() { | ||||||||||||||||
Mockito.when(uPlane.getBuffer()).thenReturn(uBuffer); | |||||||||||||||||
Mockito.when(vPlane.getBuffer()).thenReturn(vBuffer); | |||||||||||||||||
|
|||||||||||||||||
// Set pixelStride and rowStride for UV planes to trigger NV21 shortcut | |||||||||||||||||
Mockito.when(uPlane.getPixelStride()).thenReturn(2); | |||||||||||||||||
Mockito.when(uPlane.getRowStride()).thenReturn(width); | |||||||||||||||||
Mockito.when(vPlane.getPixelStride()).thenReturn(2); | |||||||||||||||||
Mockito.when(vPlane.getRowStride()).thenReturn(width); | |||||||||||||||||
|
|||||||||||||||||
List<PlaneProxy> planes = Arrays.asList(yPlane, uPlane, vPlane); | |||||||||||||||||
|
|||||||||||||||||
ByteBuffer nv21Buffer = ImageProxyUtils.planesToNV21(planes, width, height); | |||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1280x720 → ~4.6 ms per frame. Further evaluation is needed to assess the impact on the processing pipeline. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume not but are there any optimizations you know of that can be made? I'll think on it more (because frankly I'm not sure how big an impact this would cause to google-mlkit users versus say using their own NV21 conversion code in Dart, for example) but right now I definitely lean on the side of landing this to correct the implementation. Then, we can investigate how to make this faster. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m considering using one or two There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ran some tests on my S23 and LG K12+, both in debug mode. The results are as follows:
I considered using |
|||||||||||||||||
|
@@ -87,19 +98,62 @@ public void planesToNV21_returnsExpectedBufferWhenPlanesAreNV21Compatible() { | ||||||||||||||||
assertArrayEquals(expected, nv21); | |||||||||||||||||
} | |||||||||||||||||
|
|||||||||||||||||
@Test | |||||||||||||||||
public void areUVPlanesNV21_handlesVBufferAtLimitGracefully() { | |||||||||||||||||
int width = 1280; | |||||||||||||||||
int height = 720; | |||||||||||||||||
|
|||||||||||||||||
// --- Mock Y plane --- | |||||||||||||||||
byte[] yData = new byte[width * height]; | |||||||||||||||||
PlaneProxy yPlane = mock(PlaneProxy.class); | |||||||||||||||||
ByteBuffer yBuffer = ByteBuffer.wrap(yData); | |||||||||||||||||
when(yPlane.getBuffer()).thenReturn(yBuffer); | |||||||||||||||||
when(yPlane.getPixelStride()).thenReturn(1); | |||||||||||||||||
when(yPlane.getRowStride()).thenReturn(width); | |||||||||||||||||
|
|||||||||||||||||
// --- Mock U plane --- | |||||||||||||||||
ByteBuffer uBuffer = ByteBuffer.allocate(width * height / 4); | |||||||||||||||||
PlaneProxy uPlane = mock(PlaneProxy.class); | |||||||||||||||||
when(uPlane.getBuffer()).thenReturn(uBuffer); | |||||||||||||||||
when(uPlane.getPixelStride()).thenReturn(1); | |||||||||||||||||
when(uPlane.getRowStride()).thenReturn(width / 2); | |||||||||||||||||
|
|||||||||||||||||
// --- Mock V plane --- | |||||||||||||||||
ByteBuffer vBuffer = ByteBuffer.allocate(width * height / 4); | |||||||||||||||||
vBuffer.position(vBuffer.limit()); // position == limit | |||||||||||||||||
PlaneProxy vPlane = mock(PlaneProxy.class); | |||||||||||||||||
when(vPlane.getBuffer()).thenReturn(vBuffer); | |||||||||||||||||
when(vPlane.getPixelStride()).thenReturn(1); | |||||||||||||||||
when(vPlane.getRowStride()).thenReturn(width / 2); | |||||||||||||||||
|
|||||||||||||||||
List<PlaneProxy> planes = Arrays.asList(yPlane, uPlane, vPlane); | |||||||||||||||||
|
|||||||||||||||||
ByteBuffer nv21Buffer = ImageProxyUtils.planesToNV21(planes, width, height); | |||||||||||||||||
byte[] nv21 = new byte[nv21Buffer.remaining()]; | |||||||||||||||||
nv21Buffer.get(nv21); | |||||||||||||||||
|
|||||||||||||||||
assertEquals(width * height + (width * height / 2), nv21.length); | |||||||||||||||||
} | |||||||||||||||||
|
|||||||||||||||||
// Creates a mock PlaneProxy with a buffer (of zeroes) of the given size. | |||||||||||||||||
private PlaneProxy mockPlaneProxy(int bufferSize) { | |||||||||||||||||
PlaneProxy plane = Mockito.mock(PlaneProxy.class); | |||||||||||||||||
PlaneProxy plane = mock(PlaneProxy.class); | |||||||||||||||||
ByteBuffer buffer = ByteBuffer.allocate(bufferSize); | |||||||||||||||||
Mockito.when(plane.getBuffer()).thenReturn(buffer); | |||||||||||||||||
when(plane.getBuffer()).thenReturn(buffer); | |||||||||||||||||
return plane; | |||||||||||||||||
} | |||||||||||||||||
|
|||||||||||||||||
// Creates a mock PlaneProxy with specific data. | |||||||||||||||||
private PlaneProxy mockPlaneProxyWithData(byte[] data) { | |||||||||||||||||
PlaneProxy plane = Mockito.mock(PlaneProxy.class); | |||||||||||||||||
ByteBuffer buffer = ByteBuffer.wrap(Arrays.copyOf(data, data.length)); | |||||||||||||||||
Mockito.when(plane.getBuffer()).thenReturn(buffer); | |||||||||||||||||
when(plane.getBuffer()).thenReturn(buffer); | |||||||||||||||||
|
|||||||||||||||||
// Set pixelStride and rowStride to safe defaults for tests | |||||||||||||||||
// For Y plane: pixelStride = 1, rowStride = width (approximate) | |||||||||||||||||
when(plane.getPixelStride()).thenReturn(1); | |||||||||||||||||
when(plane.getRowStride()).thenReturn(data.length); // rowStride ≥ width | |||||||||||||||||
|
|||||||||||||||||
return plane; | |||||||||||||||||
} | |||||||||||||||||
} |
Uh oh!
There was an error while loading. Please reload this page.