Re: [PATCH v4 04/15] media: uapi: HEVC: Add missing fields in HEVC controls

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Le 28/02/2022 à 17:57, Sebastian Fricke a écrit :
Hey Benjamin,

On 28.02.2022 15:08, Benjamin Gaignard wrote:
Complete the HEVC controls with missing fields from H.265 specifications.
Even if these fields aren't used by the current mainlined drivers
they will be need for (at least) rkvdec driver.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
---
.../media/v4l/ext-ctrls-codec.rst             | 22 +++++++++++++++++++
include/media/hevc-ctrls.h                    |  6 ++++-
2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 4cd7c541fc30..d096cb75993a 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -2661,6 +2661,16 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
    :stub-columns: 0
    :widths:       1 1 2

+    * - __u8
+      - ``video_parameter_set_id``
+      - Specifies the value of the vps_video_parameter_set_id of the active VPS +        as descibed in section "7.4.3.2.1 General sequence parameter set RBSP semantics"
+        of H.265 specifications.
+    * - __u8
+      - ``seq_parameter_set_id``
+      - Provides an identifier for the SPS for reference by other syntax elements +        as descibed in section "7.4.3.2.1 General sequence parameter set RBSP semantics"
+        of H.265 specifications.
    * - __u16
      - ``pic_width_in_luma_samples``
      -
@@ -2800,6 +2810,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
    :stub-columns: 0
    :widths:       1 1 2

+    * - __u8
+      - ``pic_parameter_set_id``
+      - Identifies the PPS for reference by other syntax elements.
    * - __u8
      - ``num_extra_slice_header_bits``
      -
@@ -3026,6 +3039,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
    * - __u8
      - ``ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
      - The list of L1 reference elements as indices in the DPB.
+    * - __u16
+      - ``short_term_ref_pic_set_size``
+      - Specifies the number of st_ref_pic_set( ) syntax structures included in the SPS. +        The value of num_short_term_ref_pic_sets shall be in the range of 0 to 64, inclusive.
+    * - __u16
+      - ``long_term_ref_pic_set_size``
+      - Specifies the number of candidate long-term reference pictures that are specified +        in the SPS. The value of num_long_term_ref_pics_sps shall be in the range
+        of 0 to 32, inclusive.
    * - __u8

I would like to argue that the names for these fields are not optimal.

The are quite similar to the ones from the specification:
`num_short_term_ref_pic_sets` & `num_long_term_ref_pics_sps`, while
they actually do something different. (Which means that descriptions for
the fields are sadly incorrect as well)

Looking at the code from the H265 parser in GStreamer:
```
      READ_UINT8 (&nr, slice->short_term_ref_pic_set_sps_flag, 1);
      if (!slice->short_term_ref_pic_set_sps_flag) {
        guint pos = nal_reader_get_pos (&nr);
        if (!gst_h265_parser_parse_short_term_ref_pic_sets
            (&slice->short_term_ref_pic_sets, &nr,
                sps->num_short_term_ref_pic_sets, sps))
          goto error;

        slice->short_term_ref_pic_set_size = nal_reader_get_pos (&nr) - pos;
```

We can see that the `short_term_ref_pic_set_size` is calculated by
gettting the difference between the nal_reader position before calling
`gst_h265_parser_parse_short_term_ref_pic_sets` and the position of the
nal reader afterwards.
The variable `num_short_term_ref_pic_sets` is used as part of the short
term reference picture set parsing process, but it is not directly
related to `short_term_ref_pic_set_size` (otherwise a direct
transformation of `num_short_term_ref_pic_sets` ->
`short_term_ref_pic_set_size` would have been way easier)

Further when I look at a patch from Alex Bee for RKVDEC that uses these
fields (actually the only user) (https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-2000-v4l2-wip-rkvdec-hevc.patch#L3007)
I can see that he describes them as bit offsets.

So, to avoid confusion, I would argue that we should rename these
(They are not part of the specification anyway)

s/short_term_ref_pic_set_size/short_term_ref_pic_set_bit_offset/
s/long_term_ref_pic_set_size/long_term_ref_pic_set_bit_offset/

These names describe the purpose and the content a bit better and avoid
confusion with existing values.

Additonally, I noticed that calculating the bit offset for the long term
is a bit tricky. I wasn't able to find a direct reference in
'non-vendor' code.

The process for parsing the short term reference picture set is depicted with a lot of detail in the specification, but I wasn't able to find the something equivalent for the long term
reference picture set.

Having a switft look into mpp, I can see at:
https://github.com/JeffyCN/rockchip_mirrors/blob/mpp/mpp/hal/rkdec/h265d/hal_h265d_com.c#L512

That they do roughly the same short term is simply the read bits by the
BitReader - the read bits before the operation on the short term
reference picture set. (so very similar to what the h265 parser does in
GStreamer)
The bit offset for long term is equal to short term unless the
`long_term_ref_pics_present_flag` is set. In which case, we perform some
operations on the long term reference picture set and add the amount of
used bits to the bit offset.

I think the names are correct, these fields provides the size of short and long term ref picture.

It isn't an offset as you explain your self it is the diff between end and start of reference picture in the bitstream.

The documentation is incorrect, I will fix it in the next version like this:
* @short_term_ref_pic_set_size: specifies the size of short-term reference
*				 pictures included in the SPS
* @long_term_ref_pic_set_size: specifies the size of long-term reference
*				picture include in the SPS

Thanks,
Benjamin


Greetings,
Sebastian

      - ``padding``
      - Applications and drivers must set this to zero.
diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
index 01ccda48d8c5..a329e086a89a 100644
--- a/include/media/hevc-ctrls.h
+++ b/include/media/hevc-ctrls.h
@@ -58,6 +58,8 @@ enum v4l2_mpeg_video_hevc_start_code {
/* The controls are not stable at the moment and will likely be reworked. */
struct v4l2_ctrl_hevc_sps {
    /* ISO/IEC 23008-2, ITU-T Rec. H.265: Sequence parameter set */
+    __u8    video_parameter_set_id;
+    __u8    seq_parameter_set_id;
    __u16    pic_width_in_luma_samples;
    __u16    pic_height_in_luma_samples;
    __u8    bit_depth_luma_minus8;
@@ -108,6 +110,7 @@ struct v4l2_ctrl_hevc_sps {

struct v4l2_ctrl_hevc_pps {
    /* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture parameter set */
+    __u8    pic_parameter_set_id;
    __u8    num_extra_slice_header_bits;
    __u8    num_ref_idx_l0_default_active_minus1;
    __u8    num_ref_idx_l1_default_active_minus1;
@@ -199,7 +202,8 @@ struct v4l2_ctrl_hevc_slice_params {
    __u32    slice_segment_addr;
    __u8    ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
    __u8    ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
-
+    __u16    short_term_ref_pic_set_size;
+    __u16    long_term_ref_pic_set_size;
    __u8    padding;

    /* ISO/IEC 23008-2, ITU-T Rec. H.265: Weighted prediction parameter */
--
2.32.0






[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux