[Bug 795462] video: Add support for the 10bit yuv pixel format from the Rockchip SoC platform

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=795462

--- Comment #1 from Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> ---
Review of attachment 371229:
 --> (https://bugzilla.gnome.org/review?bug=795462&attachment=371229)

Ok, so this is fully packet 10bit (no padding). Please correct me, but for the
Y plane you'll have, 4 pixels per 5bytes:

5bytes:  Y1 0-9, Y2 10-19, Y3 20-29, Y4 20-39
5bytes:  Y5 ...

And for UV plane, you'll have 2 pixels per 5 bytes:
5bytes:  U1V1: 0-19, U2V2: 20-39
5bytes:  U3V3 ...

If that is correct, my suggestion is to create a state machine with 5 states,
and then combine to reduce the algo. That's what I did for the 32bit packed
variants. I didn't unroll the loops though, as it's just for debugging. Then
the state machine can be revered for the pack function.

::: gst-libs/gst/video/video-format.c
@@ +5348,3 @@
       GST_MAKE_FOURCC ('X', 'V', '2', '0'), DPTH10_10_10, PSTR0, PLANE011,
       OFFS001, SUB422, PACK_NV16_10LE32),
+  MAKE_YUV_C_LE_FORMAT (P010_10LEC, "raw video", 0x00000000, DPTH10_10_10,

For the fourcc, as you can see for the Xilinx variant, we have used the fourcc
used by the vendor (X is for Xilinx). May I suggest using something like RV20,
or RK20, or if you prefer it could be RV12, as long as it's not already used. I
like the idea to use two letter RK.., makes the origin pretty clear ;-P

::: gst-libs/gst/video/video-format.h
@@ +114,3 @@
  * @GST_VIDEO_FORMAT_Y444_12BE: planar 4:4:4 YUV, 12 bits per channel (Since:
1.12)
  * @GST_VIDEO_FORMAT_Y444_12LE: planar 4:4:4 YUV, 12 bits per channel (Since:
1.12)
+ * @GST_VIDEO_FORMAT_P010_10LEC: planar 4:2:0 YUV with interleaved UV plane,
10 bits per channel, a compat variant of P010

I'd use the "Fully packed variant of .." when describing this. Though, P010 is
already a variant of NV12, so may I proposed the following name ?

  NV12_10LE40

Which reads NV12 layout (one Y plane and one UV plane), 10bit per component,
packed over 40bits (hence no padding).

::: gst-libs/gst/video/video-info.c
@@ +1030,3 @@
       break;
+    case GST_VIDEO_FORMAT_P010_10LEC:
+      info->stride[0] = GST_ROUND_UP_64 (width * 10 / 8);

Shouldn't this roundup to 5, while rounding up to 64 is hardware specific ?
Extra alignment could be used by default if it more performant for the
pack/unpack functions. Though, these function are likely just for debugging,
the HW will take care of it in normal usage.

@@ +1034,3 @@
+      info->offset[0] = 0;
+      info->offset[1] = info->stride[0] * GST_ROUND_UP_16 (height);
+      cr_h = GST_ROUND_UP_16 (height) / 2;

CbCr plane (or UV plane) only need to be ROUND_UP_2 (so the odd last line of
the Y plane have a line of UV), 16 seems like an H264/H265 specific (16x16
macro blocks).

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.