GNOME Bugzilla – Bug 761607
video: add support for P010 video format
Last modified: 2018-11-03 11:44:34 UTC
P010 is a YUV 4:2:0 format with an interleaved U-V plane and 10-bits per pixel element stored in 2-byte fields. A description of the format can be found at: https://msdn.microsoft.com/en-us/library/windows/desktop/bb970578(v=vs.85).aspx#_420formats P010 is the video format used by the vaapi/intel-driver for H265's Main 10 profile: http://cgit.freedesktop.org/vaapi/intel-driver/commit/?id=5ec92f48aeff12c7db0a96b65aca44feb4965d92
Created attachment 320512 [details] [review] video: add P010 format support
(In reply to Scott D Phillips from comment #1) > Created attachment 320512 [details] [review] [review] > video: add P010 format support P010 format has 16 bits per component memory layout where the bits:0-5 set to zero and actual data from bits:6-15 , right? You have used DPTH10_10_10.. I think the values for GstVideoFormatInfo should be like this: @bits = 16 @n_components = 3 @shift = add the shift array @depth= 10, 10, 10, 0 @tim: what do you think?
Review of attachment 320512 [details] [review]: ::: gst-libs/gst/video/video-format.c @@ +3320,3 @@ + + sy += x; + int i; it should be suv += (x & ~1) like nv12 packing, right? @@ +3443,3 @@ + + sy += x; + Y0 |= (Y0 >> 10); similar to unpack_P010_10BE, it should be suv += (x & ~1) like nv12 packing i think..
(In reply to sreerenj from comment #3) > Review of attachment 320512 [details] [review] [review]: > > it should be suv += (x & ~1) like nv12 packing, right? > ... > similar to unpack_P010_10BE, it should be suv += (x & ~1) like > nv12 packing i think.. Yes I believe you are right. I will make a new patch.
(In reply to sreerenj from comment #2) > I think the values for GstVideoFormatInfo should be like this: > @bits = 16 > @n_components = 3 > @shift = add the shift array > @depth= 10, 10, 10, 0 bits=16 wouldn't be consistent with I420_10LE for example which also has 10 bits of data in the low 16 bits of the component. So I guess something like this would be better: #define DPTH10_10_10_HI 10, 3, { 6, 6, 6, 0 }, { 10, 10, 10, 0 } (taking DPTH10_10_10 and putting 6 for the shifts)
Created attachment 321881 [details] [review] video: add P010 format support
(In reply to Scott D Phillips from comment #5) > (In reply to sreerenj from comment #2) > > I think the values for GstVideoFormatInfo should be like this: > > @bits = 16 > > @n_components = 3 > > @shift = add the shift array > > @depth= 10, 10, 10, 0 > > bits=16 wouldn't be consistent with I420_10LE for example which also has 10 > bits of data in the low 16 bits of the component. So I guess something like > this would be better: > > #define DPTH10_10_10_HI 10, 3, { 6, 6, 6, 0 }, { 10, 10, 10, 0 } > > (taking DPTH10_10_10 and putting 6 for the shifts) I still think the @bits field should be 16... Let's wait for opinion from other developers... Adding slomo to CC..
> * @bits: The number of bits used to pack data items. This can be less than 8 > * when multiple pixels are stored in a byte. for values > 8 multiple bytes > * should be read according to the endianness flag before applying the shift > * and mask. > > * @depth: the depth in bits for each component It sounds like bits should indeed be 16 instead of 10, it's about storage if I understand the docs correctly. Changing this now however will potentially break code that assumes it's about the number of valid bits.
bits could be 10 or 16, it doesn't matter, you always round up to get the amount of bytes to read (currently). Maybe make it 16 in the future to be safe. Since the pixel data is already in the low bits after reading the bytes, no shifts are needed. Then you are left with 10 bits of valid pixel data. so the definition is: #define DPTH10_10_10 10, 3, { 0, 0, 0, 0 }, { 10, 10, 10, 0 }
Also, the format is under specified on the microsoft website, is it little or big endian? are the pixels in the low or high bits?
(In reply to Wim Taymans from comment #10) > Also, the format is under specified on the microsoft website, is it little > or big endian? little endian it seems.. Also va-driver using LSB format.. > are the pixels in the low or high bits? I think shifting is needed since pixels are storing in the high bits with lower bits set to zero
(In reply to Wim Taymans from comment #10) > Also, the format is under specified on the microsoft website, is it little > or big endian? are the pixels in the low or high bits? A little above where I linked in the page it says: > The 16-bit representations described here use little-endian > WORD values for each channel. The 10-bit formats also use 16 > bits for each channel, with the lowest 6 bits set to zero I put both big and little endian variants in my patch for consistency with the other multi-byte formats though. I believe specifying a 6-bit shift for each component is correct because the data resides in the high 10 bits. I'll submit a new patch with bits=16.
Created attachment 322312 [details] [review] video: add P010 format support Changes since patch v2: - set bits=16 in DPTH10_10_10_HI
We will get this in after 1.8 release. Have to do some testing with gstreamer-vaapi before pushing...
Few questions: -- What is "HI" in DPTH10_10_10_HI ? -- Since bits are packed in high 10 bits, we have to do the shifting while unpacking to AYUV64..right? With the current code the each component values are residing in the high 10 bits of each unpacked 16 bits component What is the actual memory layout for the I420_10LE ? seems like it is using memory layout of pixel values in lower 10 bits unlike P010_LE -- What will we do with 9 bits per component hevc format, IIRC Scot was testing 9bit streams too,,, ? Adding one more format similar to P010 except shifting fields set to 7???
(In reply to sreerenj from comment #15) > Few questions: > > -- What is "HI" in DPTH10_10_10_HI ? Short for 'high' because the pixel data is in the high bits of the bytes as opposed to DPTH10_10_10 which has pixel data in the low bits. > -- Since bits are packed in high 10 bits, we have to do the shifting while > unpacking to AYUV64..right? With the current code the each component values > are residing in the high 10 bits of each unpacked 16 bits component If you compare with unpack_I420_10LE it seems the conversion to AYUV64 should expand the 10-bit value to a 16-bit value. Because P010 pixel data is in the high bits no shift is required to expand 10 bits to 16 bits. > What is the actual memory layout for the I420_10LE ? seems like it is using > memory layout of pixel values in lower 10 bits unlike P010_LE Right, I420_10LE and P010_10LE store the pixel data in different parts of bytes, low bits and high bits respectively. > -- What will we do with 9 bits per component hevc format, IIRC Scot was > testing 9bit streams too,,, ? Adding one more format similar to P010 except > shifting fields set to 7??? I'm not totally sure what the right solution is for that. One additional complication is that HEVC can have independent bit depths for luma and chroma (bit_depth_luma_minus8 and bit_depth_chroma_minus8 in the sps). I don't think creating formats for each variation is the right approach.
(In reply to Scott D Phillips from comment #16) > (In reply to sreerenj from comment #15) > > > > -- What will we do with 9 bits per component hevc format, IIRC Scot was > > testing 9bit streams too,,, ? Adding one more format similar to P010 except > > shifting fields set to 7??? > > I'm not totally sure what the right solution is for that. One additional > complication is that HEVC can have independent bit depths for luma and > chroma (bit_depth_luma_minus8 and bit_depth_chroma_minus8 in the sps). I > don't think creating formats for each variation is the right approach. Hm, but how could we mention the shift parameter then? Also the actual @bits field in GstVideoFormatInfo... @Wim, @slomo: you guys have any suggestions ??
I am okay to push this patch, anyone have objection ? This is needed for hevc-10bit, vp9-10-bit patches pending in my gstreamer-vaapi tree :)
Review of attachment 322312 [details] [review]: Pushed, thanks for the patch. commit 079ceb894cd73bde243820231f714a5b6a7801f9 Author: Scott D Phillips <scott.d.phillips@intel.com> Date: Tue Mar 29 11:16:42 2016 +0300 video: add P010 format support P010 is a YUV420 format with an interleaved U-V plane and 2-bytes per component with the the color value stored in the 10 most significant bits. https://bugzilla.gnome.org/show_bug.cgi?id=761607 --- Changes since v2: - Set bits=16 in DPTH10_10_10_HI Changes since v1: - Fixed x-offset calculation in uv. - Added 6-bit shifts to FormatInfo.
Created attachment 324933 [details] [review] video-format: Fix bits field for 10bit formats The @bits field used in GstVideoFormatInfo should indicate the actual storage. Exact depth of each component can be extracted from @depth[] field. So the @bits field for 10bit formats will be 16 which is the actual storage allocated. https://bugzilla.gnome.org/show_bug.cgi?id=761607
(In reply to Scott D Phillips from comment #16) > (In reply to sreerenj from comment #15) > > I'm not totally sure what the right solution is for that. One additional > complication is that HEVC can have independent bit depths for luma and > chroma (bit_depth_luma_minus8 and bit_depth_chroma_minus8 in the sps). I > don't think creating formats for each variation is the right approach. Right, do you have samples like that? For eg, luma_depth=10 and chroma_depth=9 ? IMHO, we should add P010_9le and P010_9be at least, because we already have samples like that and fmpeg have similar i420 variants like yuv420p9be/yuv420p9le.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/251.