NV12 NV21 ImageExt ImageProxyExt wrong

mite-user commented 6 days ago

https://github.com/crow-misia/libyuv-android/blob/main/core/src/main/java/io/github/crow_misia/libyuv/ext/ImageExt.kt#L299-L329

NV12 Image.toNv12Buffer() grabs planeUV from U plane which, at least on my test device, doesn't include the last V byte. As a result, AbstractBuffer.asBuffer() and AbstractBuffer.asByteArray() return the result without that last V byte and with the size less by 1 byte. https://github.com/crow-misia/libyuv-android/blob/main/core/src/main/java/io/github/crow_misia/libyuv/AbstractBuffer.kt#L34-L50 The output of these methods might cause hard-to-debug crashes. Some functions don't care about the last missing byte, some will crash with ArrayIndexOutOfBoundsException.

NV21 Image.toNv21Buffer() also grabs planeVU from U plane, which doesn't include the first V byte. The whole image is wrong, planeVU contains UV, first U grouped with second V, second U with third V, etc.

ImageProxy.toNv12Buffer() ImageProxy.toNv21Buffer() have the same problems.

crow-misia commented 6 days ago

Thanks for the report.

Please give me more details about your situation.

Android device name / OS
Example code
Stack trace causing ArrayIndexOutOfBoundsException

The data stored in Image is hardware dependent and might not be in NV21/12 format. Try I420 as well.

mite-user commented 6 days ago

All functions in this library are unaffected by the missing last byte because nothing uses the output of NV12/21 .asByteArray() and .asBuffer(). What I tried to say, any external library implemented in, for example, pure java would expect the YUV byte[] to be of size width * height * 3 / 2. But the YUV byte[] returned from Nv12Buffer.asByteArray() is (width * height * 3 / 2) - 1 which would obviously cause ArrayIndexOutOfBoundsException or similar problems. It happens only if the Nv12Buffer was obtained with Image.toNv12Buffer(), ImageProxy.toNv12Buffer() or similarly, by using the U plane from the android's Image as planeUV.

I'm testing it on 4 android phones: 2 I420/YV12, 1 NV12, 1 NV21

I'm identifying the YUV type by first checking the pixel stride of the 2nd plane and then comparing the offsets of the 2nd and 3rd planes through JNI. Basically doing the check the same way as it is done in the original libyuv https://android.googlesource.com/platform/external/libyuv/+/4e366538070a3a6c5c163c31b791eab742e1657a/source/rotate.cc#957

The missing byte at the end is a minor issue. What's really important is that Image.toNv21Buffer() is broken. These lines should get the planes[2], the V plane, as a planeVU. https://github.com/crow-misia/libyuv-android/blob/main/core/src/main/java/io/github/crow_misia/libyuv/ext/ImageExt.kt#L324 https://github.com/crow-misia/libyuv-android/blob/main/core/src/main/java/io/github/crow_misia/libyuv/ext/ImageProxyExt.kt#L325

Useful links and info about YUV. https://github.com/android/camera-samples/blob/main/CameraUtils/lib/src/main/java/com/example/android/camera/utils/Yuv.kt

The order of planes in the array returned by Image#getPlanes() is guaranteed such that plane #0 is always Y, plane #1 is always U (Cb), and plane #2 is always V (Cr). https://developer.android.com/reference/android/graphics/ImageFormat#YUV_420_888

crow-misia commented 5 days ago

toNv21Buffer method can only be used if it is in NV21 format.

If the image is in YUV420 format, use toI420Buffer method.

asBuffer/asByteArray only copies the Buffer in Plane, so the size is not calculated from the width or height of the image. I don't have any idea what causes the 1 byte loss.

mite-user commented 5 days ago

Consider the following java code.

Log.e(TAG, "plane 0 size: " + image.getPlanes()[0].getBuffer().limit());
Log.e(TAG, "plane 1 size: " + image.getPlanes()[1].getBuffer().limit());
Log.e(TAG, "plane 2 size: " + image.getPlanes()[2].getBuffer().limit());

on my NV21 and NV12 devices, the output for 176x144 is

plane 0 size: 25344
plane 1 size: 12671
plane 2 size: 12671

The real size of the UV/VU section is width * height / 2, that is 12672

In case of NV12 the U buffer doesn't include the last V byte the V buffer doesn't include the first U byte If the Nv12Buffer was obtained just by wrapping the Image, it just assumes that the U buffer IS the UV buffer. And then the size of that U buffer is assumed to be the size of the whole UV buffer when calculating the output size for asBuffer/asByteArray

crow-misia commented 5 days ago

v0.37.0 is released.

Corrected the VU Plane in NV21 to start with V.

Added pixelStride-based detection, so that if pixelStride is 2, the output size is increased by 1.

please check as I have no devices for NV12 and NV21.

mite-user commented 5 days ago

I think NV21/12 never have pixel stride 1 The libyuv's Android420ToI420Rotate identifies the YUV type as I420 if the pixel stride is 1 https://android.googlesource.com/platform/external/libyuv/+/4e366538070a3a6c5c163c31b791eab742e1657a/source/rotate.cc#956

It makes sense that NV12/21 always have pixel stride 2. In the U buffer each second byte is V, so it's not used as an U value, so we say that the U plane has pixel stride 2. Same for the V buffer.

Otherwise looks good. Going to test it soon.

mite-user commented 5 days ago

To make my own quick workaround, I did a bunch of tests with images before the release of version 0.37.0 What I found:

Missing U byte at the end of NV21 looks like a 4 pixel yellow square at the bottom right.
If you get the U buffer and treat it as a V buffer (like it was in 0.36.0 version with .toNv21Buffer), the colors will be swapped.
(assuming the NV21 wrapper grabs the right plane) If you wrap the NV21 Image as NV12, or NV12 Image as NV21, the image would look mostly normal, except the colors at the rightmost pixels would be obviously influenced by the colors at the leftmost pixels. (because of the first missing byte, the UV/VU are grouped wrongly)

Testing 0.37.0

NV12 java code

    public void toJpegNV12(Image image, int quality, OutputStream outStream) {
        int width = image.getWidth();
        int height = image.getHeight();
        int yuvSize = width * height * 3 / 2;

        Nv12Buffer crowNv12Buf = ImageExt.toNv12Buffer(image);
        byte[] crowNv12ByteArr = crowNv12Buf.asByteArray();

        // Would crash if the last byte is missing
        byte crowLastVByte = crowNv12ByteArr[yuvSize - 1];

        // Manually writing the contents of plane buffers into a separate byte array
        byte[] androidNv12ByteArr = new byte[yuvSize];
        ByteBuffer yBuf = image.getPlanes()[0].getBuffer();
        ByteBuffer uBuf = image.getPlanes()[1].getBuffer();
        ByteBuffer vBuf = image.getPlanes()[2].getBuffer();

        yBuf.get(androidNv12ByteArr, 0, yBuf.limit());
        uBuf.get(androidNv12ByteArr, yBuf.limit(), uBuf.limit());
        vBuf.position(vBuf.limit() - 1);
        vBuf.get(androidNv12ByteArr, yuvSize - 1, 1);

        Log.e(TAG, "crowNv12ByteArr.length: " + crowNv12ByteArr.length);
        Log.e(TAG, "Arrays.equals: " + Arrays.equals(crowNv12ByteArr, androidNv12ByteArr));

        // With NV12, expected to produce swapped colors.
        // It's fine since we are testing for the visual artifacts at the rightmost pixels.
        YuvImage yuvImage = new YuvImage(crowNv12ByteArr, ImageFormat.NV21, width, height, null);
        yuvImage.compressToJpeg(new Rect(0, 0, width, height), 100, outStream);
    }

NV12 phone, version 0.36.0, 176x144 crashes

java.lang.ArrayIndexOutOfBoundsException: length=38015; index=38015
...

NV12 phone, version 0.37.0, 176x144 output jpegs with expected swapped colors, without visual artifacts

crowNv12ByteArr.length: 38016
Arrays.equals: true

intentional NV12 test on the wrong phone NV21 phone, version 0.37.0, 176x144 as expected, swapped colors; as expected, visual artifacts at the rightmost pixels

crowNv12ByteArr.length: 38016
Arrays.equals: false

NV21 java code

    public void toJpegNV21(Image image, int quality, OutputStream outStream) {
        int width = image.getWidth();
        int height = image.getHeight();
        int yuvSize = width * height * 3 / 2;

        Nv21Buffer crowNv21Buf = ImageExt.toNv21Buffer(image);
        byte[] crowNv21ByteArr = crowNv21Buf.asByteArray();

        // Would crash if the last byte is missing
        byte crowLastUByte = crowNv21ByteArr[yuvSize - 1];

        // Manually writing the contents of plane buffers into a separate byte array
        byte[] androidNv21ByteArr = new byte[yuvSize];
        ByteBuffer yBuf = image.getPlanes()[0].getBuffer();
        ByteBuffer uBuf = image.getPlanes()[1].getBuffer();
        ByteBuffer vBuf = image.getPlanes()[2].getBuffer();

        yBuf.get(androidNv21ByteArr, 0, yBuf.limit());
        vBuf.get(androidNv21ByteArr, yBuf.limit(), vBuf.limit());
        uBuf.position(uBuf.limit() - 1);
        uBuf.get(androidNv21ByteArr, yuvSize - 1, 1);

        Log.e(TAG, "crowNv21ByteArr.length: " + crowNv21ByteArr.length);
        Log.e(TAG, "Arrays.equals: " + Arrays.equals(crowNv21ByteArr, androidNv21ByteArr));

        // With NV21, expected to produce normal colors.
        YuvImage yuvImage = new YuvImage(crowNv21ByteArr, ImageFormat.NV21, width, height, null);
        yuvImage.compressToJpeg(new Rect(0, 0, width, height), 100, outStream);
    }

NV21 phone, version 0.36.0, 176x144 crashes

java.lang.ArrayIndexOutOfBoundsException: length=38015; index=38015
...

NV21 phone, version 0.37.0, 176x144 output jpegs with expected normal colors, without visual artifacts

crowNv21ByteArr.length: 38016
Arrays.equals: true

intentional NV21 test on the wrong phone NV12 phone, version 0.37.0, 176x144 normal colors; as expected, visual artifacts at the rightmost pixels

crowNv21ByteArr.length: 38016
Arrays.equals: false

Looks good so far. I can do more tests if you have more ideas how to test it.

mite-user commented 4 days ago

I noticed that the code in my tests doesn't handle unusual row strides. I went through all available YUV_420_888 resolutions on both of my NV21/12 phones trying to find some unusual row stride. Didn't find any. Every single resolution on those phones follows the same formula: width = rowStrideY = rowStrideU = rowStrideV sizeY = width * height sizeU = sizeV = (sizeY / 2) - 1 Of course, it doesn't prove that the NV21/12 phones with unusual row strides do not exist.

What I just realized, we can't be certain that all NV12 devices exclude the last V byte in the U buffer. https://github.com/crow-misia/libyuv-android/blob/5c37232d4d01af3dc39d9e992a084db3a46e60ae/core/src/main/java/io/github/crow_misia/libyuv/ext/ImageExt.kt#L314 Maybe it's better to check if planeU.buffer.capacity() is odd. If even, use planeU.buffer.capacity(), if odd planeU.buffer.capacity() + 1. And obviously do the same for NV21.

mite-user commented 3 days ago

When I was trying to write a java code which uses the rowStride values from Nv21Buffer, I hit another problem.

io.github.crow_misia.libyuv.Plane rowStride and pixelStride use kotlin's inline value class. The names of the getters are mangled. https://kotlinlang.org/docs/inline-classes.html#mangling

Using reflection to get the exact method names

Method[] methods = io.github.crow_misia.libyuv.Plane.class.getMethods();
for (int i = 0; i < methods.length; i++) {
    Log.e(TAG, methods[i].getName());
}

it appears that the getters are

getPixelStride-r_R9t5Y
getRowStride-BElzS_M

Dashes - are not allowed in java identifiers, so the only way to call those functions is by using reflection Method.invoke()

The problem can be solved by using @JvmName https://kotlinlang.org/docs/java-to-kotlin-interop.html#handling-signature-clashes-with-jvmname https://stackoverflow.com/questions/47504279/java-interop-apply-jvmname-to-getters-of-properties-in-interface-or-abstract-c replace these lines https://github.com/crow-misia/libyuv-android/blob/11c6b90b7d1a3dabcbf5bf4b67da8f26136ed3e7/core/src/main/java/io/github/crow_misia/libyuv/Plane.kt#L11-L12 with

    @Suppress("INAPPLICABLE_JVM_NAME")
    @get:JvmName("getRowStride")
    abstract val rowStride: RowStride
    @Suppress("INAPPLICABLE_JVM_NAME")
    @get:JvmName("getPixelStride")
    abstract val pixelStride: PixelStride

Alternatevily, there should be separate functions which return the strides as Int

crow-misia commented 2 days ago

I can't take the time. I will confirm somewhere next week.

crow-misia / libyuv-android

NV12 NV21 ImageExt ImageProxyExt wrong #132