The H.265 ITU specification (section 7.4) define the general
slice segment header semantics.
Modified/added fields are:
- video_parameter_set_id: (7.4.3.1) identifies the VPS for
reference by other syntax elements.
- seq_parameter_set_id: (7.4.3.2.1) specifies the value of
the vps_video_parameter_set_id of the active VPS.
- chroma_format_idc: (7.4.3.2.1) specifies the chroma sampling
relative to the luma sampling
- pic_parameter_set_id: (7.4.3.3.1) identifies the PPS for
reference by other syntax elements
- num_ref_idx_l0_default_active_minus1: (7.4.3.3.1) specifies
the inferred value of num_ref_idx_l0_active_minus1
- num_ref_idx_l1_default_active_minus1: (7.4.3.3.1) specifies
the inferred value of num_ref_idx_l1_active_minus1
- slice_segment_addr: (7.4.7.1) specifies the address of
the first coding tree block in the slice segment
- num_entry_point_offsets: (7.4.7.1) specifies the number of
entry_point_offset_minus1[ i ] syntax elements in the slice header
Add HEVC decode params contains the information used in section
"8.3 Slice decoding process" of the specification to let the hardware
perform decoding of a slices.
Adapt Cedrus driver according to these changes.
Signed-off-by: Benjamin Gaignard <[email protected]>
---
version 3:
- Add documentation about the new structuers and fields.
version 2:
- remove all change related to scaling
- squash commits to a coherent split
- be more verbose about the added fields
.../media/v4l/ext-ctrls-codec.rst | 126 +++++++++++++++---
.../media/v4l/vidioc-queryctrl.rst | 6 +
drivers/media/v4l2-core/v4l2-ctrls.c | 26 +++-
drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +
drivers/staging/media/sunxi/cedrus/cedrus.h | 1 +
.../staging/media/sunxi/cedrus/cedrus_dec.c | 2 +
.../staging/media/sunxi/cedrus/cedrus_h265.c | 6 +-
include/media/hevc-ctrls.h | 45 +++++--
8 files changed, 186 insertions(+), 32 deletions(-)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 00944e97d638..5e6d77e858c0 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -3109,6 +3109,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
:stub-columns: 0
:widths: 1 1 2
+ * - __u8
+ - ``video_parameter_set_id``
+ - Identifies the VPS for reference by other syntax elements
+ * - __u8
+ - ``seq_parameter_set_id̀``
+ - Specifies the value of the vps_video_parameter_set_id of the active VPS
+ * - __u8
+ - ``chroma_format_idc``
+ - Specifies the chroma sampling relative to the luma sampling
* - __u16
- ``pic_width_in_luma_samples``
-
@@ -3172,6 +3181,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
* - __u8
- ``chroma_format_idc``
-
+ * - __u8
+ - ``num_slices``
+ -
* - __u64
- ``flags``
- See :ref:`Sequence Parameter Set Flags <hevc_sps_flags>`
@@ -3231,9 +3243,18 @@ 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``
-
+ * - __u8
+ - ``num_ref_idx_l0_default_active_minus1``
+ - Specifies the inferred value of num_ref_idx_l0_active_minus1
+ * - __u8
+ - ``num_ref_idx_l1_default_active_minus1``
+ - Specifies the inferred value of num_ref_idx_l1_active_minus1
* - __s8
- ``init_qp_minus26``
-
@@ -3342,6 +3363,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
* - ``V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT``
- 0x00040000
-
+ * - ``V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT``
+ - 0x00080000
+ -
+ * - ``V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING``
+ - 0x00100000
+ -
``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (struct)``
Specifies various slice-specific parameters, especially from the NAL unit
@@ -3366,6 +3393,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
* - __u32
- ``data_bit_offset``
- Offset (in bits) to the video data in the current slice data.
+ * - __u32
+ - ``slice_segment_addr``
+ - Specifies the address of the first coding tree block in the slice segment
+ * - __u32
+ - ``num_entry_point_offsets``
+ - Specifies the number of entry_point_offset_minus1[ i ] syntax elements in the slice header
* - __u8
- ``nal_unit_type``
-
@@ -3422,28 +3455,20 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
* - __u8
- ``pic_struct``
-
- * - __u8
- - ``num_active_dpb_entries``
- - The number of entries in ``dpb``.
* - __u8
- ``ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
- The list of L0 reference elements as indices in the DPB.
* - __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``
+ -
+ * - __u16
+ - ``long_term_ref_pic_set_size``
+ -
* - __u8
- - ``num_rps_poc_st_curr_before``
- - The number of reference pictures in the short-term set that come before
- the current frame.
- * - __u8
- - ``num_rps_poc_st_curr_after``
- - The number of reference pictures in the short-term set that come after
- the current frame.
- * - __u8
- - ``num_rps_poc_lt_curr``
- - The number of reference pictures in the long-term set.
- * - __u8
- - ``padding[7]``
+ - ``padding``
- Applications and drivers must set this to zero.
* - struct :c:type:`v4l2_hevc_dpb_entry`
- ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
@@ -3646,3 +3671,74 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
so this has to come from client.
This is applicable to H264 and valid Range is from 0 to 63.
Source Rec. ITU-T H.264 (06/2019); G.7.4.1.1, G.8.8.1.
+
+``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (struct)``
+ Specifies various decode parameters, especially the references picture order
+ count (POC) for all the lists (short, long, before, current, after) and the
+ number of entries for each of them.
+ These parameters are defined according to :ref:`hevc`.
+ They are described in section 8.3 "Slice decoding process" of the
+ specification.
+
+.. c:type:: v4l2_ctrl_hevc_decode_params
+
+.. cssclass:: longtable
+
+.. flat-table:: struct v4l2_ctrl_hevc_decode_params
+ :header-rows: 0
+ :stub-columns: 0
+ :widths: 1 1 2
+
+ * - __s32
+ - ``pic_order_cnt_val``
+ -
+ * - __u8
+ - ``num_active_dpb_entries``
+ - The number of entries in ``dpb``.
+ * - struct :c:type:`v4l2_hevc_dpb_entry`
+ - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
+ - The decoded picture buffer, for meta-data about reference frames.
+ * - __u8
+ - ``num_rps_poc_st_curr_before``
+ - The number of reference pictures in the short-term set that come before
+ the current frame.
+ * - __u8
+ - ``num_rps_poc_st_curr_after``
+ - The number of reference pictures in the short-term set that come after
+ the current frame.
+ * - __u8
+ - ``num_rps_poc_lt_curr``
+ - The number of reference pictures in the long-term set.
+ * - __u8
+ - ``rps_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
+ -
+ * - __u8
+ - ``rps_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
+ -
+ * - __u8
+ - ``rps_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
+ -
+ * - __u64
+ - ``flags``
+ - See :ref:`Decode Parameters Flags <hevc_decode_params_flags>`
+
+.. _hevc_decode_params_flags:
+
+``Decode Parameters Flags``
+
+.. cssclass:: longtable
+
+.. flat-table::
+ :header-rows: 0
+ :stub-columns: 0
+ :widths: 1 1 2
+
+ * - ``V4L2_HEVC_DECODE_PARAM_FLAG_IRAP_PIC``
+ - 0x00000001
+ -
+ * - ``V4L2_HEVC_DECODE_PARAM_FLAG_IDR_PIC``
+ - 0x00000002
+ -
+ * - ``V4L2_HEVC_DECODE_PARAM_FLAG_NO_OUTPUT_OF_PRIOR``
+ - 0x00000004
+ -
diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
index 82f61f1e2fb8..d84ae255bc79 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
@@ -486,6 +486,12 @@ See also the examples in :ref:`control`.
- n/a
- A struct :c:type:`v4l2_ctrl_hevc_slice_params`, containing HEVC
slice parameters for stateless video decoders.
+ * - ``V4L2_CTRL_TYPE_HEVC_DECODE_PARAMS``
+ - n/a
+ - n/a
+ - n/a
+ - A struct :c:type:`v4l2_ctrl_hevc_decode_params`, containing HEVC
+ decoding parameters for stateless video decoders.
.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 016cf6204cbb..4060b5bcc3c0 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1028,6 +1028,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_MPEG_VIDEO_HEVC_SPS: return "HEVC Sequence Parameter Set";
case V4L2_CID_MPEG_VIDEO_HEVC_PPS: return "HEVC Picture Parameter Set";
case V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS: return "HEVC Slice Parameters";
+ case V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS: return "HEVC Decode Parameters";
case V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE: return "HEVC Decode Mode";
case V4L2_CID_MPEG_VIDEO_HEVC_START_CODE: return "HEVC Start Code";
@@ -1482,6 +1483,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
case V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS:
*type = V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS;
break;
+ case V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS:
+ *type = V4L2_CTRL_TYPE_HEVC_DECODE_PARAMS;
+ break;
case V4L2_CID_UNIT_CELL_SIZE:
*type = V4L2_CTRL_TYPE_AREA;
*flags |= V4L2_CTRL_FLAG_READ_ONLY;
@@ -1833,6 +1837,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
struct v4l2_ctrl_hevc_sps *p_hevc_sps;
struct v4l2_ctrl_hevc_pps *p_hevc_pps;
struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
+ struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params;
struct v4l2_area *area;
void *p = ptr.p + idx * ctrl->elem_size;
unsigned int i;
@@ -2108,23 +2113,27 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
zero_padding(*p_hevc_pps);
break;
- case V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS:
- p_hevc_slice_params = p;
+ case V4L2_CTRL_TYPE_HEVC_DECODE_PARAMS:
+ p_hevc_decode_params = p;
- if (p_hevc_slice_params->num_active_dpb_entries >
+ if (p_hevc_decode_params->num_active_dpb_entries >
V4L2_HEVC_DPB_ENTRIES_NUM_MAX)
return -EINVAL;
- zero_padding(p_hevc_slice_params->pred_weight_table);
-
- for (i = 0; i < p_hevc_slice_params->num_active_dpb_entries;
+ for (i = 0; i < p_hevc_decode_params->num_active_dpb_entries;
i++) {
struct v4l2_hevc_dpb_entry *dpb_entry =
- &p_hevc_slice_params->dpb[i];
+ &p_hevc_decode_params->dpb[i];
zero_padding(*dpb_entry);
}
+ break;
+
+ case V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS:
+ p_hevc_slice_params = p;
+
+ zero_padding(p_hevc_slice_params->pred_weight_table);
zero_padding(*p_hevc_slice_params);
break;
@@ -2821,6 +2830,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
case V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS:
elem_size = sizeof(struct v4l2_ctrl_hevc_slice_params);
break;
+ case V4L2_CTRL_TYPE_HEVC_DECODE_PARAMS:
+ elem_size = sizeof(struct v4l2_ctrl_hevc_decode_params);
+ break;
case V4L2_CTRL_TYPE_AREA:
elem_size = sizeof(struct v4l2_area);
break;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 7bd9291c8d5f..4cd3cab1a257 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -151,6 +151,12 @@ static const struct cedrus_control cedrus_controls[] = {
},
.codec = CEDRUS_CODEC_VP8,
},
+ {
+ .cfg = {
+ .id = V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS,
+ },
+ .codec = CEDRUS_CODEC_H265,
+ },
};
#define CEDRUS_CONTROLS_COUNT ARRAY_SIZE(cedrus_controls)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 251a6a660351..2ca33ac38b9a 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -76,6 +76,7 @@ struct cedrus_h265_run {
const struct v4l2_ctrl_hevc_sps *sps;
const struct v4l2_ctrl_hevc_pps *pps;
const struct v4l2_ctrl_hevc_slice_params *slice_params;
+ const struct v4l2_ctrl_hevc_decode_params *decode_params;
};
struct cedrus_vp8_run {
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index a9090daf626a..cd821f417a14 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -68,6 +68,8 @@ void cedrus_device_run(void *priv)
V4L2_CID_MPEG_VIDEO_HEVC_PPS);
run.h265.slice_params = cedrus_find_control_data(ctx,
V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS);
+ run.h265.decode_params = cedrus_find_control_data(ctx,
+ V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS);
break;
case V4L2_PIX_FMT_VP8_FRAME:
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
index ce497d0197df..dce5db6be13a 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
@@ -245,6 +245,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
const struct v4l2_ctrl_hevc_sps *sps;
const struct v4l2_ctrl_hevc_pps *pps;
const struct v4l2_ctrl_hevc_slice_params *slice_params;
+ const struct v4l2_ctrl_hevc_decode_params *decode_params;
const struct v4l2_hevc_pred_weight_table *pred_weight_table;
dma_addr_t src_buf_addr;
dma_addr_t src_buf_end_addr;
@@ -256,6 +257,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
sps = run->h265.sps;
pps = run->h265.pps;
slice_params = run->h265.slice_params;
+ decode_params = run->h265.decode_params;
pred_weight_table = &slice_params->pred_weight_table;
/* MV column buffer size and allocation. */
@@ -487,7 +489,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
reg = VE_DEC_H265_DEC_SLICE_HDR_INFO1_SLICE_TC_OFFSET_DIV2(slice_params->slice_tc_offset_div2) |
VE_DEC_H265_DEC_SLICE_HDR_INFO1_SLICE_BETA_OFFSET_DIV2(slice_params->slice_beta_offset_div2) |
- VE_DEC_H265_DEC_SLICE_HDR_INFO1_SLICE_POC_BIGEST_IN_RPS_ST(slice_params->num_rps_poc_st_curr_after == 0) |
+ VE_DEC_H265_DEC_SLICE_HDR_INFO1_SLICE_POC_BIGEST_IN_RPS_ST(decode_params->num_rps_poc_st_curr_after == 0) |
VE_DEC_H265_DEC_SLICE_HDR_INFO1_SLICE_CR_QP_OFFSET(slice_params->slice_cr_qp_offset) |
VE_DEC_H265_DEC_SLICE_HDR_INFO1_SLICE_CB_QP_OFFSET(slice_params->slice_cb_qp_offset) |
VE_DEC_H265_DEC_SLICE_HDR_INFO1_SLICE_QP_DELTA(slice_params->slice_qp_delta);
@@ -528,7 +530,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
/* Write decoded picture buffer in pic list. */
cedrus_h265_frame_info_write_dpb(ctx, slice_params->dpb,
- slice_params->num_active_dpb_entries);
+ decode_params->num_active_dpb_entries);
/* Output frame. */
diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
index b4cb2ef02f17..7fe704a08f77 100644
--- a/include/media/hevc-ctrls.h
+++ b/include/media/hevc-ctrls.h
@@ -19,6 +19,7 @@
#define V4L2_CID_MPEG_VIDEO_HEVC_SPS (V4L2_CID_CODEC_BASE + 1008)
#define V4L2_CID_MPEG_VIDEO_HEVC_PPS (V4L2_CID_CODEC_BASE + 1009)
#define V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (V4L2_CID_CODEC_BASE + 1010)
+#define V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (V4L2_CID_CODEC_BASE + 1012)
#define V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE (V4L2_CID_CODEC_BASE + 1015)
#define V4L2_CID_MPEG_VIDEO_HEVC_START_CODE (V4L2_CID_CODEC_BASE + 1016)
@@ -26,6 +27,7 @@
#define V4L2_CTRL_TYPE_HEVC_SPS 0x0120
#define V4L2_CTRL_TYPE_HEVC_PPS 0x0121
#define V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS 0x0122
+#define V4L2_CTRL_TYPE_HEVC_DECODE_PARAMS 0x0124
enum v4l2_mpeg_video_hevc_decode_mode {
V4L2_MPEG_VIDEO_HEVC_DECODE_MODE_SLICE_BASED,
@@ -54,6 +56,9 @@ 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;
+ __u8 chroma_format_idc;
__u16 pic_width_in_luma_samples;
__u16 pic_height_in_luma_samples;
__u8 bit_depth_luma_minus8;
@@ -74,9 +79,9 @@ struct v4l2_ctrl_hevc_sps {
__u8 log2_diff_max_min_pcm_luma_coding_block_size;
__u8 num_short_term_ref_pic_sets;
__u8 num_long_term_ref_pics_sps;
- __u8 chroma_format_idc;
- __u8 padding;
+ __u8 num_slices;
+ __u8 padding[6];
__u64 flags;
};
@@ -100,10 +105,15 @@ struct v4l2_ctrl_hevc_sps {
#define V4L2_HEVC_PPS_FLAG_PPS_DISABLE_DEBLOCKING_FILTER (1ULL << 16)
#define V4L2_HEVC_PPS_FLAG_LISTS_MODIFICATION_PRESENT (1ULL << 17)
#define V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT (1ULL << 18)
+#define V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT (1ULL << 19)
+#define V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING (1ULL << 20)
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;
__s8 init_qp_minus26;
__u8 diff_cu_qp_delta_depth;
__s8 pps_cb_qp_offset;
@@ -116,7 +126,7 @@ struct v4l2_ctrl_hevc_pps {
__s8 pps_tc_offset_div2;
__u8 log2_parallel_merge_level_minus2;
- __u8 padding[4];
+ __u8 padding;
__u64 flags;
};
@@ -165,6 +175,10 @@ struct v4l2_ctrl_hevc_slice_params {
__u32 bit_size;
__u32 data_bit_offset;
+ /* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header */
+ __u32 slice_segment_addr;
+ __u32 num_entry_point_offsets;
+
/* ISO/IEC 23008-2, ITU-T Rec. H.265: NAL unit header */
__u8 nal_unit_type;
__u8 nuh_temporal_id_plus1;
@@ -190,15 +204,13 @@ struct v4l2_ctrl_hevc_slice_params {
__u8 pic_struct;
/* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header */
- __u8 num_active_dpb_entries;
__u8 ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
__u8 ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
- __u8 num_rps_poc_st_curr_before;
- __u8 num_rps_poc_st_curr_after;
- __u8 num_rps_poc_lt_curr;
+ __u16 short_term_ref_pic_set_size;
+ __u16 long_term_ref_pic_set_size;
- __u8 padding;
+ __u8 padding[5];
/* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header */
struct v4l2_hevc_dpb_entry dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
@@ -209,4 +221,21 @@ struct v4l2_ctrl_hevc_slice_params {
__u64 flags;
};
+#define V4L2_HEVC_DECODE_PARAM_FLAG_IRAP_PIC 0x1
+#define V4L2_HEVC_DECODE_PARAM_FLAG_IDR_PIC 0x2
+#define V4L2_HEVC_DECODE_PARAM_FLAG_NO_OUTPUT_OF_PRIOR 0x4
+
+struct v4l2_ctrl_hevc_decode_params {
+ __s32 pic_order_cnt_val;
+ __u8 num_active_dpb_entries;
+ struct v4l2_hevc_dpb_entry dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
+ __u8 num_rps_poc_st_curr_before;
+ __u8 num_rps_poc_st_curr_after;
+ __u8 num_rps_poc_lt_curr;
+ __u8 rps_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
+ __u8 rps_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
+ __u8 rps_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
+ __u64 flags;
+};
+
#endif
--
2.25.1
Hi Benjamin,
Thanks for the good work.
On Mon, 2021-02-22 at 13:23 +0100, Benjamin Gaignard wrote:
> The H.265 ITU specification (section 7.4) define the general
> slice segment header semantics.
> Modified/added fields are:
> - video_parameter_set_id: (7.4.3.1) identifies the VPS for
> reference by other syntax elements.
> - seq_parameter_set_id: (7.4.3.2.1) specifies the value of
> the vps_video_parameter_set_id of the active VPS.
> - chroma_format_idc: (7.4.3.2.1) specifies the chroma sampling
> relative to the luma sampling
> - pic_parameter_set_id: (7.4.3.3.1) identifies the PPS for
> reference by other syntax elements
> - num_ref_idx_l0_default_active_minus1: (7.4.3.3.1) specifies
> the inferred value of num_ref_idx_l0_active_minus1
> - num_ref_idx_l1_default_active_minus1: (7.4.3.3.1) specifies
> the inferred value of num_ref_idx_l1_active_minus1
> - slice_segment_addr: (7.4.7.1) specifies the address of
> the first coding tree block in the slice segment
> - num_entry_point_offsets: (7.4.7.1) specifies the number of
> entry_point_offset_minus1[ i ] syntax elements in the slice header
>
> Add HEVC decode params contains the information used in section
> "8.3 Slice decoding process" of the specification to let the hardware
> perform decoding of a slices.
>
> Adapt Cedrus driver according to these changes.
>
> Signed-off-by: Benjamin Gaignard <[email protected]>
> ---
> version 3:
> - Add documentation about the new structuers and fields.
>
> version 2:
> - remove all change related to scaling
> - squash commits to a coherent split
> - be more verbose about the added fields
>
> .../media/v4l/ext-ctrls-codec.rst | 126 +++++++++++++++---
> .../media/v4l/vidioc-queryctrl.rst | 6 +
> drivers/media/v4l2-core/v4l2-ctrls.c | 26 +++-
> drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +
> drivers/staging/media/sunxi/cedrus/cedrus.h | 1 +
> .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 +
> .../staging/media/sunxi/cedrus/cedrus_h265.c | 6 +-
> include/media/hevc-ctrls.h | 45 +++++--
> 8 files changed, 186 insertions(+), 32 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 00944e97d638..5e6d77e858c0 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -3109,6 +3109,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> :stub-columns: 0
> :widths: 1 1 2
>
> + * - __u8
> + - ``video_parameter_set_id``
> + - Identifies the VPS for reference by other syntax elements
> + * - __u8
> + - ``seq_parameter_set_id̀``
> + - Specifies the value of the vps_video_parameter_set_id of the active VPS
> + * - __u8
> + - ``chroma_format_idc``
> + - Specifies the chroma sampling relative to the luma sampling
None of these fields seem needed for the Hantro G2 driver,
so I suggest you drop them for now.
> * - __u16
> - ``pic_width_in_luma_samples``
> -
> @@ -3172,6 +3181,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> * - __u8
> - ``chroma_format_idc``
> -
> + * - __u8
> + - ``num_slices``
> +
Not used, but also doesn't seem part of the SPS syntax. If we have to
pass the number of slices, we'll need another mechanism.
> -
> * - __u64
> - ``flags``
> - See :ref:`Sequence Parameter Set Flags <hevc_sps_flags>`
> @@ -3231,9 +3243,18 @@ 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
Not used.
> * - __u8
> - ``num_extra_slice_header_bits``
> -
> + * - __u8
> + - ``num_ref_idx_l0_default_active_minus1``
> + - Specifies the inferred value of num_ref_idx_l0_active_minus1
> + * - __u8
> + - ``num_ref_idx_l1_default_active_minus1``
> + - Specifies the inferred value of num_ref_idx_l1_active_minus1
> * - __s8
> - ``init_qp_minus26``
> -
> @@ -3342,6 +3363,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> * - ``V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT``
> - 0x00040000
> -
> + * - ``V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT``
> + - 0x00080000
> + -
> + * - ``V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING``
> + - 0x00100000
> + -
>
I suggest to do all the PPS control changes in a separate patch,
feels easier to review and cleaner as you can explain the
changes with more detail in the commit description.
Looking at the PPS syntax for tiles, I'm wondering if these
deserve their own control, which would be used if tiles are enabled,
i.e. V4L2_HEVC_PPS_FLAG_TILES_ENABLED is set.
__u8 num_tile_columns_minus1;
__u8 num_tile_rows_minus1;
__u8 column_width_minus1[20];
__u8 row_height_minus1[22];
Not something we necessarily have to tackle now.
> ``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (struct)``
> Specifies various slice-specific parameters, especially from the NAL unit
> @@ -3366,6 +3393,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> * - __u32
> - ``data_bit_offset``
> - Offset (in bits) to the video data in the current slice data.
> + * - __u32
> + - ``slice_segment_addr``
> + - Specifies the address of the first coding tree block in the slice segment
Not used.
> + * - __u32
> + - ``num_entry_point_offsets``
> + - Specifies the number of entry_point_offset_minus1[ i ] syntax elements in the slice header
Not used.
> * - __u8
> - ``nal_unit_type``
> -
> @@ -3422,28 +3455,20 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> * - __u8
> - ``pic_struct``
> -
> - * - __u8
> - - ``num_active_dpb_entries``
> - - The number of entries in ``dpb``.
Need to explain in the commit description why this field is moved.
> * - __u8
> - ``ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> - The list of L0 reference elements as indices in the DPB.
> * - __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``
> +
Not used.
> -
> + * - __u16
> + - ``long_term_ref_pic_set_size``
> + -
Not used.
> * - __u8
> - - ``num_rps_poc_st_curr_before``
> - - The number of reference pictures in the short-term set that come before
> - the current frame.
If this matches NumPocStCurrBefore from section 8.3.2 "Decoding process for reference picture set"
then I would document that. And perhaps rename it to num_poc_st_curr_before.
> - * - __u8
> - - ``num_rps_poc_st_curr_after``
> - - The number of reference pictures in the short-term set that come after
> - the current frame.
Ditto.
> - * - __u8
> - - ``num_rps_poc_lt_curr``
> - - The number of reference pictures in the long-term set.
Ditto.
Also, I'd like the changes that move fields from V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS
to the new V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS control, to be in their
patch.
That will allow us to put in the commit description a proper
explanation of why are fields being moved. Nothing fancy, simply
explaining that these variables come from section 8.3.2
"Decoding process for reference picture set", which describes
a process invoked once per picture, so they are not per-slice.
> - * - __u8
> - - ``padding[7]``
> + - ``padding``
> - Applications and drivers must set this to zero.
> * - struct :c:type:`v4l2_hevc_dpb_entry`
> - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> @@ -3646,3 +3671,74 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> so this has to come from client.
> This is applicable to H264 and valid Range is from 0 to 63.
> Source Rec. ITU-T H.264 (06/2019); G.7.4.1.1, G.8.8.1.
> +
> +``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (struct)``
> + Specifies various decode parameters, especially the references picture order
> + count (POC) for all the lists (short, long, before, current, after) and the
> + number of entries for each of them.
> + These parameters are defined according to :ref:`hevc`.
> + They are described in section 8.3 "Slice decoding process" of the
> + specification.
> +
> +.. c:type:: v4l2_ctrl_hevc_decode_params
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_ctrl_hevc_decode_params
> + :header-rows: 0
> + :stub-columns: 0
> + :widths: 1 1 2
> +
> + * - __s32
> + - ``pic_order_cnt_val``
> + -
Can be documented as:
"""
PicOrderCntVal as described in section 8.3.1 "Decoding process
for picture order count" of the specification.
"""
Note that snake case is used to match the kernel style,
but other than that we try to keep the HEVC spec variable
names.
> + * - __u8
> + - ``num_active_dpb_entries``
> + - The number of entries in ``dpb``.
> + * - struct :c:type:`v4l2_hevc_dpb_entry`
> + - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> + - The decoded picture buffer, for meta-data about reference frames.
The DPB is here, but it seems it's also in the slice control?
> + * - __u8
> + - ``num_rps_poc_st_curr_before``
> + - The number of reference pictures in the short-term set that come before
> + the current frame.
> + * - __u8
> + - ``num_rps_poc_st_curr_after``
> + - The number of reference pictures in the short-term set that come after
> + the current frame.
> + * - __u8
> + - ``num_rps_poc_lt_curr``
> + - The number of reference pictures in the long-term set.
> + * - __u8
> + - ``rps_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> + -
> + * - __u8
> + - ``rps_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> + -
> + * - __u8
> + - ``rps_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> + -
Could you document these as well?
Thanks a lot,
Ezequiel
Hi Ezequiel,
Dne četrtek, 25. februar 2021 ob 14:09:52 CET je Ezequiel Garcia napisal(a):
> Hi Benjamin,
>
> Thanks for the good work.
>
> On Mon, 2021-02-22 at 13:23 +0100, Benjamin Gaignard wrote:
> > The H.265 ITU specification (section 7.4) define the general
> > slice segment header semantics.
> > Modified/added fields are:
> > - video_parameter_set_id: (7.4.3.1) identifies the VPS for
> > reference by other syntax elements.
> > - seq_parameter_set_id: (7.4.3.2.1) specifies the value of
> > the vps_video_parameter_set_id of the active VPS.
> > - chroma_format_idc: (7.4.3.2.1) specifies the chroma sampling
> > relative to the luma sampling
> > - pic_parameter_set_id: (7.4.3.3.1) identifies the PPS for
> > reference by other syntax elements
> > - num_ref_idx_l0_default_active_minus1: (7.4.3.3.1) specifies
> > the inferred value of num_ref_idx_l0_active_minus1
> > - num_ref_idx_l1_default_active_minus1: (7.4.3.3.1) specifies
> > the inferred value of num_ref_idx_l1_active_minus1
> > - slice_segment_addr: (7.4.7.1) specifies the address of
> > the first coding tree block in the slice segment
> > - num_entry_point_offsets: (7.4.7.1) specifies the number of
> > entry_point_offset_minus1[ i ] syntax elements in the slice header
> >
> > Add HEVC decode params contains the information used in section
> > "8.3 Slice decoding process" of the specification to let the hardware
> > perform decoding of a slices.
> >
> > Adapt Cedrus driver according to these changes.
> >
> > Signed-off-by: Benjamin Gaignard <[email protected]>
> > ---
> > version 3:
> > - Add documentation about the new structuers and fields.
> >
> > version 2:
> > - remove all change related to scaling
> > - squash commits to a coherent split
> > - be more verbose about the added fields
> >
> > .../media/v4l/ext-ctrls-codec.rst | 126 +++++++++++++++---
> > .../media/v4l/vidioc-queryctrl.rst | 6 +
> > drivers/media/v4l2-core/v4l2-ctrls.c | 26 +++-
> > drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +
> > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 +
> > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 +
> > .../staging/media/sunxi/cedrus/cedrus_h265.c | 6 +-
> > include/media/hevc-ctrls.h | 45 +++++--
> > 8 files changed, 186 insertions(+), 32 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/
Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 00944e97d638..5e6d77e858c0 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -3109,6 +3109,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > :stub-columns: 0
> > :widths: 1 1 2
> >
> > + * - __u8
> > + - ``video_parameter_set_id``
> > + - Identifies the VPS for reference by other syntax elements
> > + * - __u8
> > + - ``seq_parameter_set_id̀``
> > + - Specifies the value of the vps_video_parameter_set_id of the
active VPS
> > + * - __u8
> > + - ``chroma_format_idc``
> > + - Specifies the chroma sampling relative to the luma sampling
>
> None of these fields seem needed for the Hantro G2 driver,
> so I suggest you drop them for now.
>
> > * - __u16
> > - ``pic_width_in_luma_samples``
> > -
> > @@ -3172,6 +3181,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > * - __u8
> > - ``chroma_format_idc``
> > -
> > + * - __u8
> > + - ``num_slices``
> > +
>
> Not used, but also doesn't seem part of the SPS syntax. If we have to
> pass the number of slices, we'll need another mechanism.
>
> > -
> > * - __u64
> > - ``flags``
> > - See :ref:`Sequence Parameter Set Flags <hevc_sps_flags>`
> > @@ -3231,9 +3243,18 @@ 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
>
> Not used.
>
> > * - __u8
> > - ``num_extra_slice_header_bits``
> > -
> > + * - __u8
> > + - ``num_ref_idx_l0_default_active_minus1``
> > + - Specifies the inferred value of num_ref_idx_l0_active_minus1
> > + * - __u8
> > + - ``num_ref_idx_l1_default_active_minus1``
> > + - Specifies the inferred value of num_ref_idx_l1_active_minus1
> > * - __s8
> > - ``init_qp_minus26``
> > -
> > @@ -3342,6 +3363,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > * - ``V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT``
> > - 0x00040000
> > -
> > + * - ``V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT``
> > + - 0x00080000
> > + -
> > + * - ``V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING``
> > + - 0x00100000
> > + -
> >
>
> I suggest to do all the PPS control changes in a separate patch,
> feels easier to review and cleaner as you can explain the
> changes with more detail in the commit description.
>
> Looking at the PPS syntax for tiles, I'm wondering if these
> deserve their own control, which would be used if tiles are enabled,
> i.e. V4L2_HEVC_PPS_FLAG_TILES_ENABLED is set.
>
> __u8 num_tile_columns_minus1;
> __u8 num_tile_rows_minus1;
> __u8 column_width_minus1[20];
> __u8 row_height_minus1[22];
>
> Not something we necessarily have to tackle now.
>
> > ``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (struct)``
> > Specifies various slice-specific parameters, especially from the NAL
unit
> > @@ -3366,6 +3393,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > * - __u32
> > - ``data_bit_offset``
> > - Offset (in bits) to the video data in the current slice data.
> > + * - __u32
> > + - ``slice_segment_addr``
> > + - Specifies the address of the first coding tree block in the slice
segment
>
> Not used.
>
> > + * - __u32
> > + - ``num_entry_point_offsets``
> > + - Specifies the number of entry_point_offset_minus1[ i ] syntax
elements in the slice header
>
> Not used.
While above two fields may not be used in Hantro, they are for sure useful for
Cedrus and RPi4. I would like to keep them, otherwise with such approach HEVC
will stay in staging for a long time. I'm still baffled why scaling matrix
control was dropped. It would fit well in Cedrus and RPi4 driver and after a
quick look, it seems that it was used in driver in later patch.
Best regards,
Jernej
>
> > * - __u8
> > - ``nal_unit_type``
> > -
> > @@ -3422,28 +3455,20 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > * - __u8
> > - ``pic_struct``
> > -
> > - * - __u8
> > - - ``num_active_dpb_entries``
> > - - The number of entries in ``dpb``.
>
> Need to explain in the commit description why this field is moved.
>
> > * - __u8
> > - ``ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > - The list of L0 reference elements as indices in the DPB.
> > * - __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``
> > +
>
> Not used.
>
> > -
> > + * - __u16
> > + - ``long_term_ref_pic_set_size``
> > + -
>
> Not used.
>
> > * - __u8
> > - - ``num_rps_poc_st_curr_before``
> > - - The number of reference pictures in the short-term set that come
before
> > - the current frame.
>
> If this matches NumPocStCurrBefore from section 8.3.2 "Decoding process for
reference picture set"
> then I would document that. And perhaps rename it to num_poc_st_curr_before.
>
> > - * - __u8
> > - - ``num_rps_poc_st_curr_after``
> > - - The number of reference pictures in the short-term set that come
after
> > - the current frame.
>
> Ditto.
>
> > - * - __u8
> > - - ``num_rps_poc_lt_curr``
> > - - The number of reference pictures in the long-term set.
>
> Ditto.
>
> Also, I'd like the changes that move fields from
V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS
> to the new V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS control, to be in their
> patch.
>
> That will allow us to put in the commit description a proper
> explanation of why are fields being moved. Nothing fancy, simply
> explaining that these variables come from section 8.3.2
> "Decoding process for reference picture set", which describes
> a process invoked once per picture, so they are not per-slice.
>
> > - * - __u8
> > - - ``padding[7]``
> > + - ``padding``
> > - Applications and drivers must set this to zero.
> > * - struct :c:type:`v4l2_hevc_dpb_entry`
> > - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > @@ -3646,3 +3671,74 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > so this has to come from client.
> > This is applicable to H264 and valid Range is from 0 to 63.
> > Source Rec. ITU-T H.264 (06/2019); G.7.4.1.1, G.8.8.1.
> > +
> > +``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (struct)``
> > + Specifies various decode parameters, especially the references picture
order
> > + count (POC) for all the lists (short, long, before, current, after)
and the
> > + number of entries for each of them.
> > + These parameters are defined according to :ref:`hevc`.
> > + They are described in section 8.3 "Slice decoding process" of the
> > + specification.
> > +
> > +.. c:type:: v4l2_ctrl_hevc_decode_params
> > +
> > +.. cssclass:: longtable
> > +
> > +.. flat-table:: struct v4l2_ctrl_hevc_decode_params
> > + :header-rows: 0
> > + :stub-columns: 0
> > + :widths: 1 1 2
> > +
> > + * - __s32
> > + - ``pic_order_cnt_val``
> > + -
>
> Can be documented as:
>
> """
> PicOrderCntVal as described in section 8.3.1 "Decoding process
> for picture order count" of the specification.
> """
>
> Note that snake case is used to match the kernel style,
> but other than that we try to keep the HEVC spec variable
> names.
>
> > + * - __u8
> > + - ``num_active_dpb_entries``
> > + - The number of entries in ``dpb``.
> > + * - struct :c:type:`v4l2_hevc_dpb_entry`
> > + - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > + - The decoded picture buffer, for meta-data about reference frames.
>
> The DPB is here, but it seems it's also in the slice control?
>
> > + * - __u8
> > + - ``num_rps_poc_st_curr_before``
> > + - The number of reference pictures in the short-term set that come
before
> > + the current frame.
> > + * - __u8
> > + - ``num_rps_poc_st_curr_after``
> > + - The number of reference pictures in the short-term set that come
after
> > + the current frame.
> > + * - __u8
> > + - ``num_rps_poc_lt_curr``
> > + - The number of reference pictures in the long-term set.
> > + * - __u8
> > + - ``rps_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > + -
> > + * - __u8
> > + - ``rps_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > + -
> > + * - __u8
> > + - ``rps_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > + -
>
> Could you document these as well?
>
> Thanks a lot,
> Ezequiel
>
>
Hey Jernej,
On Thu, 2021-02-25 at 18:01 +0100, Jernej Škrabec wrote:
> Hi Ezequiel,
>
> Dne četrtek, 25. februar 2021 ob 14:09:52 CET je Ezequiel Garcia napisal(a):
> > Hi Benjamin,
> >
> > Thanks for the good work.
> >
> > On Mon, 2021-02-22 at 13:23 +0100, Benjamin Gaignard wrote:
> > > The H.265 ITU specification (section 7.4) define the general
> > > slice segment header semantics.
> > > Modified/added fields are:
> > > - video_parameter_set_id: (7.4.3.1) identifies the VPS for
> > > reference by other syntax elements.
> > > - seq_parameter_set_id: (7.4.3.2.1) specifies the value of
> > > the vps_video_parameter_set_id of the active VPS.
> > > - chroma_format_idc: (7.4.3.2.1) specifies the chroma sampling
> > > relative to the luma sampling
> > > - pic_parameter_set_id: (7.4.3.3.1) identifies the PPS for
> > > reference by other syntax elements
> > > - num_ref_idx_l0_default_active_minus1: (7.4.3.3.1) specifies
> > > the inferred value of num_ref_idx_l0_active_minus1
> > > - num_ref_idx_l1_default_active_minus1: (7.4.3.3.1) specifies
> > > the inferred value of num_ref_idx_l1_active_minus1
> > > - slice_segment_addr: (7.4.7.1) specifies the address of
> > > the first coding tree block in the slice segment
> > > - num_entry_point_offsets: (7.4.7.1) specifies the number of
> > > entry_point_offset_minus1[ i ] syntax elements in the slice header
> > >
> > > Add HEVC decode params contains the information used in section
> > > "8.3 Slice decoding process" of the specification to let the hardware
> > > perform decoding of a slices.
> > >
> > > Adapt Cedrus driver according to these changes.
> > >
> > > Signed-off-by: Benjamin Gaignard <[email protected]>
> > > ---
> > > version 3:
> > > - Add documentation about the new structuers and fields.
> > >
> > > version 2:
> > > - remove all change related to scaling
> > > - squash commits to a coherent split
> > > - be more verbose about the added fields
> > >
> > > .../media/v4l/ext-ctrls-codec.rst | 126 +++++++++++++++---
> > > .../media/v4l/vidioc-queryctrl.rst | 6 +
> > > drivers/media/v4l2-core/v4l2-ctrls.c | 26 +++-
> > > drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +
> > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 +
> > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 +
> > > .../staging/media/sunxi/cedrus/cedrus_h265.c | 6 +-
> > > include/media/hevc-ctrls.h | 45 +++++--
> > > 8 files changed, 186 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/
> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > index 00944e97d638..5e6d77e858c0 100644
> > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > @@ -3109,6 +3109,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > :stub-columns: 0
> > > :widths: 1 1 2
> > >
> > > + * - __u8
> > > + - ``video_parameter_set_id``
> > > + - Identifies the VPS for reference by other syntax elements
> > > + * - __u8
> > > + - ``seq_parameter_set_id̀``
> > > + - Specifies the value of the vps_video_parameter_set_id of the
> active VPS
> > > + * - __u8
> > > + - ``chroma_format_idc``
> > > + - Specifies the chroma sampling relative to the luma sampling
> >
> > None of these fields seem needed for the Hantro G2 driver,
> > so I suggest you drop them for now.
> >
> > > * - __u16
> > > - ``pic_width_in_luma_samples``
> > > -
> > > @@ -3172,6 +3181,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > * - __u8
> > > - ``chroma_format_idc``
> > > -
> > > + * - __u8
> > > + - ``num_slices``
> > > +
> >
> > Not used, but also doesn't seem part of the SPS syntax. If we have to
> > pass the number of slices, we'll need another mechanism.
> >
> > > -
> > > * - __u64
> > > - ``flags``
> > > - See :ref:`Sequence Parameter Set Flags <hevc_sps_flags>`
> > > @@ -3231,9 +3243,18 @@ 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
> >
> > Not used.
> >
> > > * - __u8
> > > - ``num_extra_slice_header_bits``
> > > -
> > > + * - __u8
> > > + - ``num_ref_idx_l0_default_active_minus1``
> > > + - Specifies the inferred value of num_ref_idx_l0_active_minus1
> > > + * - __u8
> > > + - ``num_ref_idx_l1_default_active_minus1``
> > > + - Specifies the inferred value of num_ref_idx_l1_active_minus1
> > > * - __s8
> > > - ``init_qp_minus26``
> > > -
> > > @@ -3342,6 +3363,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > * - ``V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT``
> > > - 0x00040000
> > > -
> > > + * - ``V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT``
> > > + - 0x00080000
> > > + -
> > > + * - ``V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING``
> > > + - 0x00100000
> > > + -
> > >
> >
> > I suggest to do all the PPS control changes in a separate patch,
> > feels easier to review and cleaner as you can explain the
> > changes with more detail in the commit description.
> >
> > Looking at the PPS syntax for tiles, I'm wondering if these
> > deserve their own control, which would be used if tiles are enabled,
> > i.e. V4L2_HEVC_PPS_FLAG_TILES_ENABLED is set.
> >
> > __u8 num_tile_columns_minus1;
> > __u8 num_tile_rows_minus1;
> > __u8 column_width_minus1[20];
> > __u8 row_height_minus1[22];
> >
> > Not something we necessarily have to tackle now.
> >
> > > ``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (struct)``
> > > Specifies various slice-specific parameters, especially from the NAL
> unit
> > > @@ -3366,6 +3393,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > * - __u32
> > > - ``data_bit_offset``
> > > - Offset (in bits) to the video data in the current slice data.
> > > + * - __u32
> > > + - ``slice_segment_addr``
> > > + - Specifies the address of the first coding tree block in the slice
> segment
> >
> > Not used.
> >
> > > + * - __u32
> > > + - ``num_entry_point_offsets``
> > > + - Specifies the number of entry_point_offset_minus1[ i ] syntax
> elements in the slice header
> >
> > Not used.
>
> While above two fields may not be used in Hantro, they are for sure useful for
> Cedrus and RPi4. I would like to keep them, otherwise with such approach HEVC
> will stay in staging for a long time. I'm still baffled why scaling matrix
> control was dropped. It would fit well in Cedrus and RPi4 driver and after a
> quick look, it seems that it was used in driver in later patch.
>
I'd like to make sure each modification we are making to the uAPI
goes in the right direction, that is in the direction of moving
the API out of staging.
Since reviewing each field is quite hard, and opens some discussions,
I wanted to keep this patchset specific to what's needed for Hantro G2.
The Scaling matrix control is certainly a good one, as well as the ones
needed for Cedrus and RPi4. However, I feel it's better to discuss
them in their own "uAPI review" series so we can review all the changes
with an API hat.
This way we decouple the Hantro G2 discussion and work from the API work.
Also please feel free to submit RFC patches fo Cedrus and RPi4
(API and driver changes). We can certainly start the discussion around that,
with driver changes in context.
Hope I'm making sense here :)
Thanks,
Ezequiel
> Best regards,
> Jernej
>
> >
> > > * - __u8
> > > - ``nal_unit_type``
> > > -
> > > @@ -3422,28 +3455,20 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > * - __u8
> > > - ``pic_struct``
> > > -
> > > - * - __u8
> > > - - ``num_active_dpb_entries``
> > > - - The number of entries in ``dpb``.
> >
> > Need to explain in the commit description why this field is moved.
> >
> > > * - __u8
> > > - ``ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > - The list of L0 reference elements as indices in the DPB.
> > > * - __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``
> > > +
> >
> > Not used.
> >
> > > -
> > > + * - __u16
> > > + - ``long_term_ref_pic_set_size``
> > > + -
> >
> > Not used.
> >
> > > * - __u8
> > > - - ``num_rps_poc_st_curr_before``
> > > - - The number of reference pictures in the short-term set that come
> before
> > > - the current frame.
> >
> > If this matches NumPocStCurrBefore from section 8.3.2 "Decoding process for
> reference picture set"
> > then I would document that. And perhaps rename it to num_poc_st_curr_before.
> >
> > > - * - __u8
> > > - - ``num_rps_poc_st_curr_after``
> > > - - The number of reference pictures in the short-term set that come
> after
> > > - the current frame.
> >
> > Ditto.
> >
> > > - * - __u8
> > > - - ``num_rps_poc_lt_curr``
> > > - - The number of reference pictures in the long-term set.
> >
> > Ditto.
> >
> > Also, I'd like the changes that move fields from
> V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS
> > to the new V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS control, to be in their
> > patch.
> >
> > That will allow us to put in the commit description a proper
> > explanation of why are fields being moved. Nothing fancy, simply
> > explaining that these variables come from section 8.3.2
> > "Decoding process for reference picture set", which describes
> > a process invoked once per picture, so they are not per-slice.
> >
> > > - * - __u8
> > > - - ``padding[7]``
> > > + - ``padding``
> > > - Applications and drivers must set this to zero.
> > > * - struct :c:type:`v4l2_hevc_dpb_entry`
> > > - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > @@ -3646,3 +3671,74 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > so this has to come from client.
> > > This is applicable to H264 and valid Range is from 0 to 63.
> > > Source Rec. ITU-T H.264 (06/2019); G.7.4.1.1, G.8.8.1.
> > > +
> > > +``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (struct)``
> > > + Specifies various decode parameters, especially the references picture
> order
> > > + count (POC) for all the lists (short, long, before, current, after)
> and the
> > > + number of entries for each of them.
> > > + These parameters are defined according to :ref:`hevc`.
> > > + They are described in section 8.3 "Slice decoding process" of the
> > > + specification.
> > > +
> > > +.. c:type:: v4l2_ctrl_hevc_decode_params
> > > +
> > > +.. cssclass:: longtable
> > > +
> > > +.. flat-table:: struct v4l2_ctrl_hevc_decode_params
> > > + :header-rows: 0
> > > + :stub-columns: 0
> > > + :widths: 1 1 2
> > > +
> > > + * - __s32
> > > + - ``pic_order_cnt_val``
> > > + -
> >
> > Can be documented as:
> >
> > """
> > PicOrderCntVal as described in section 8.3.1 "Decoding process
> > for picture order count" of the specification.
> > """
> >
> > Note that snake case is used to match the kernel style,
> > but other than that we try to keep the HEVC spec variable
> > names.
> >
> > > + * - __u8
> > > + - ``num_active_dpb_entries``
> > > + - The number of entries in ``dpb``.
> > > + * - struct :c:type:`v4l2_hevc_dpb_entry`
> > > + - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > + - The decoded picture buffer, for meta-data about reference frames.
> >
> > The DPB is here, but it seems it's also in the slice control?
> >
> > > + * - __u8
> > > + - ``num_rps_poc_st_curr_before``
> > > + - The number of reference pictures in the short-term set that come
> before
> > > + the current frame.
> > > + * - __u8
> > > + - ``num_rps_poc_st_curr_after``
> > > + - The number of reference pictures in the short-term set that come
> after
> > > + the current frame.
> > > + * - __u8
> > > + - ``num_rps_poc_lt_curr``
> > > + - The number of reference pictures in the long-term set.
> > > + * - __u8
> > > + - ``rps_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > + -
> > > + * - __u8
> > > + - ``rps_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > + -
> > > + * - __u8
> > > + - ``rps_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > + -
> >
> > Could you document these as well?
> >
> > Thanks a lot,
> > Ezequiel
> >
> >
>
>
Dne četrtek, 25. februar 2021 ob 18:34:48 CET je Ezequiel Garcia napisal(a):
> Hey Jernej,
>
> On Thu, 2021-02-25 at 18:01 +0100, Jernej Škrabec wrote:
> > Hi Ezequiel,
> >
> > Dne četrtek, 25. februar 2021 ob 14:09:52 CET je Ezequiel Garcia
napisal(a):
> > > Hi Benjamin,
> > >
> > > Thanks for the good work.
> > >
> > > On Mon, 2021-02-22 at 13:23 +0100, Benjamin Gaignard wrote:
> > > > The H.265 ITU specification (section 7.4) define the general
> > > > slice segment header semantics.
> > > > Modified/added fields are:
> > > > - video_parameter_set_id: (7.4.3.1) identifies the VPS for
> > > > reference by other syntax elements.
> > > > - seq_parameter_set_id: (7.4.3.2.1) specifies the value of
> > > > the vps_video_parameter_set_id of the active VPS.
> > > > - chroma_format_idc: (7.4.3.2.1) specifies the chroma sampling
> > > > relative to the luma sampling
> > > > - pic_parameter_set_id: (7.4.3.3.1) identifies the PPS for
> > > > reference by other syntax elements
> > > > - num_ref_idx_l0_default_active_minus1: (7.4.3.3.1) specifies
> > > > the inferred value of num_ref_idx_l0_active_minus1
> > > > - num_ref_idx_l1_default_active_minus1: (7.4.3.3.1) specifies
> > > > the inferred value of num_ref_idx_l1_active_minus1
> > > > - slice_segment_addr: (7.4.7.1) specifies the address of
> > > > the first coding tree block in the slice segment
> > > > - num_entry_point_offsets: (7.4.7.1) specifies the number of
> > > > entry_point_offset_minus1[ i ] syntax elements in the slice header
> > > >
> > > > Add HEVC decode params contains the information used in section
> > > > "8.3 Slice decoding process" of the specification to let the hardware
> > > > perform decoding of a slices.
> > > >
> > > > Adapt Cedrus driver according to these changes.
> > > >
> > > > Signed-off-by: Benjamin Gaignard <[email protected]>
> > > > ---
> > > > version 3:
> > > > - Add documentation about the new structuers and fields.
> > > >
> > > > version 2:
> > > > - remove all change related to scaling
> > > > - squash commits to a coherent split
> > > > - be more verbose about the added fields
> > > >
> > > > .../media/v4l/ext-ctrls-codec.rst | 126 ++++++++++++++
+---
> > > > .../media/v4l/vidioc-queryctrl.rst | 6 +
> > > > drivers/media/v4l2-core/v4l2-ctrls.c | 26 +++-
> > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +
> > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 +
> > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 +
> > > > .../staging/media/sunxi/cedrus/cedrus_h265.c | 6 +-
> > > > include/media/hevc-ctrls.h | 45 +++++--
> > > > 8 files changed, 186 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
b/
> > Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > index 00944e97d638..5e6d77e858c0 100644
> > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > @@ -3109,6 +3109,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > > :stub-columns: 0
> > > > :widths: 1 1 2
> > > >
> > > > + * - __u8
> > > > + - ``video_parameter_set_id``
> > > > + - Identifies the VPS for reference by other syntax elements
> > > > + * - __u8
> > > > + - ``seq_parameter_set_id̀``
> > > > + - Specifies the value of the vps_video_parameter_set_id of the
> > active VPS
> > > > + * - __u8
> > > > + - ``chroma_format_idc``
> > > > + - Specifies the chroma sampling relative to the luma sampling
> > >
> > > None of these fields seem needed for the Hantro G2 driver,
> > > so I suggest you drop them for now.
> > >
> > > > * - __u16
> > > > - ``pic_width_in_luma_samples``
> > > > -
> > > > @@ -3172,6 +3181,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > > * - __u8
> > > > - ``chroma_format_idc``
> > > > -
> > > > + * - __u8
> > > > + - ``num_slices``
> > > > +
> > >
> > > Not used, but also doesn't seem part of the SPS syntax. If we have to
> > > pass the number of slices, we'll need another mechanism.
> > >
> > > > -
> > > > * - __u64
> > > > - ``flags``
> > > > - See :ref:`Sequence Parameter Set Flags <hevc_sps_flags>`
> > > > @@ -3231,9 +3243,18 @@ 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
> > >
> > > Not used.
> > >
> > > > * - __u8
> > > > - ``num_extra_slice_header_bits``
> > > > -
> > > > + * - __u8
> > > > + - ``num_ref_idx_l0_default_active_minus1``
> > > > + - Specifies the inferred value of num_ref_idx_l0_active_minus1
> > > > + * - __u8
> > > > + - ``num_ref_idx_l1_default_active_minus1``
> > > > + - Specifies the inferred value of num_ref_idx_l1_active_minus1
> > > > * - __s8
> > > > - ``init_qp_minus26``
> > > > -
> > > > @@ -3342,6 +3363,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > > * - ``V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT``
> > > > - 0x00040000
> > > > -
> > > > + * - ``V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT``
> > > > + - 0x00080000
> > > > + -
> > > > + * - ``V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING``
> > > > + - 0x00100000
> > > > + -
> > > >
> > >
> > > I suggest to do all the PPS control changes in a separate patch,
> > > feels easier to review and cleaner as you can explain the
> > > changes with more detail in the commit description.
> > >
> > > Looking at the PPS syntax for tiles, I'm wondering if these
> > > deserve their own control, which would be used if tiles are enabled,
> > > i.e. V4L2_HEVC_PPS_FLAG_TILES_ENABLED is set.
> > >
> > > __u8
num_tile_columns_minus1;
> > > __u8
num_tile_rows_minus1;
> > > __u8
column_width_minus1[20];
> > > __u8 row_height_minus1[22];
> > >
> > > Not something we necessarily have to tackle now.
> > >
> > > > ``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (struct)``
> > > > Specifies various slice-specific parameters, especially from the
NAL
> > unit
> > > > @@ -3366,6 +3393,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > > * - __u32
> > > > - ``data_bit_offset``
> > > > - Offset (in bits) to the video data in the current slice data.
> > > > + * - __u32
> > > > + - ``slice_segment_addr``
> > > > + - Specifies the address of the first coding tree block in the
slice
> > segment
> > >
> > > Not used.
> > >
> > > > + * - __u32
> > > > + - ``num_entry_point_offsets``
> > > > + - Specifies the number of entry_point_offset_minus1[ i ] syntax
> > elements in the slice header
> > >
> > > Not used.
> >
> > While above two fields may not be used in Hantro, they are for sure useful
for
> > Cedrus and RPi4. I would like to keep them, otherwise with such approach
HEVC
> > will stay in staging for a long time. I'm still baffled why scaling matrix
> > control was dropped. It would fit well in Cedrus and RPi4 driver and after
a
> > quick look, it seems that it was used in driver in later patch.
> >
>
> I'd like to make sure each modification we are making to the uAPI
> goes in the right direction, that is in the direction of moving
> the API out of staging.
>
> Since reviewing each field is quite hard, and opens some discussions,
> I wanted to keep this patchset specific to what's needed for Hantro G2.
>
> The Scaling matrix control is certainly a good one, as well as the ones
> needed for Cedrus and RPi4. However, I feel it's better to discuss
> them in their own "uAPI review" series so we can review all the changes
> with an API hat.
>
> This way we decouple the Hantro G2 discussion and work from the API work.
>
> Also please feel free to submit RFC patches fo Cedrus and RPi4
> (API and driver changes). We can certainly start the discussion around that,
> with driver changes in context.
I don't know much about RPi4 driver, only few implementation details, so
you'll have to ping developer who wrote it. Regarding HEVC on Cedrus - it has
one pain point - it needs entry point table which in turn needs support for
variable arrays in order to be feasable AFAIK. I don't plan to develop that.
Patches for scaling matrix and segment address were sent a bit more than a
year ago but were turned down because they change control structures (among
other things). Sorry to say, but I work on other things now, so Cedrus will
have to wait. Alternatively, someone can take my patches from LibreELEC,
update and submit them. They are in use for a long time.
Best regards,
Jernej
>
> Hope I'm making sense here :)
>
> Thanks,
> Ezequiel
>
> > Best regards,
> > Jernej
> >
> > >
> > > > * - __u8
> > > > - ``nal_unit_type``
> > > > -
> > > > @@ -3422,28 +3455,20 @@ enum v4l2_mpeg_video_hevc_size_of_length_field
-
> > > > * - __u8
> > > > - ``pic_struct``
> > > > -
> > > > - * - __u8
> > > > - - ``num_active_dpb_entries``
> > > > - - The number of entries in ``dpb``.
> > >
> > > Need to explain in the commit description why this field is moved.
> > >
> > > > * - __u8
> > > > - ``ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > - The list of L0 reference elements as indices in the DPB.
> > > > * - __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``
> > > > +
> > >
> > > Not used.
> > >
> > > > -
> > > > + * - __u16
> > > > + - ``long_term_ref_pic_set_size``
> > > > + -
> > >
> > > Not used.
> > >
> > > > * - __u8
> > > > - - ``num_rps_poc_st_curr_before``
> > > > - - The number of reference pictures in the short-term set that
come
> > before
> > > > - the current frame.
> > >
> > > If this matches NumPocStCurrBefore from section 8.3.2 "Decoding process
for
> > reference picture set"
> > > then I would document that. And perhaps rename it to
num_poc_st_curr_before.
> > >
> > > > - * - __u8
> > > > - - ``num_rps_poc_st_curr_after``
> > > > - - The number of reference pictures in the short-term set that
come
> > after
> > > > - the current frame.
> > >
> > > Ditto.
> > >
> > > > - * - __u8
> > > > - - ``num_rps_poc_lt_curr``
> > > > - - The number of reference pictures in the long-term set.
> > >
> > > Ditto.
> > >
> > > Also, I'd like the changes that move fields from
> > V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS
> > > to the new V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS control, to be in
their
> > > patch.
> > >
> > > That will allow us to put in the commit description a proper
> > > explanation of why are fields being moved. Nothing fancy, simply
> > > explaining that these variables come from section 8.3.2
> > > "Decoding process for reference picture set", which describes
> > > a process invoked once per picture, so they are not per-slice.
> > >
> > > > - * - __u8
> > > > - - ``padding[7]``
> > > > + - ``padding``
> > > > - Applications and drivers must set this to zero.
> > > > * - struct :c:type:`v4l2_hevc_dpb_entry`
> > > > - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > @@ -3646,3 +3671,74 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > > > so this has to come from client.
> > > > This is applicable to H264 and valid Range is from 0 to 63.
> > > > Source Rec. ITU-T H.264 (06/2019); G.7.4.1.1, G.8.8.1.
> > > > +
> > > > +``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (struct)``
> > > > + Specifies various decode parameters, especially the references
picture
> > order
> > > > + count (POC) for all the lists (short, long, before, current,
after)
> > and the
> > > > + number of entries for each of them.
> > > > + These parameters are defined according to :ref:`hevc`.
> > > > + They are described in section 8.3 "Slice decoding process" of the
> > > > + specification.
> > > > +
> > > > +.. c:type:: v4l2_ctrl_hevc_decode_params
> > > > +
> > > > +.. cssclass:: longtable
> > > > +
> > > > +.. flat-table:: struct v4l2_ctrl_hevc_decode_params
> > > > + :header-rows: 0
> > > > + :stub-columns: 0
> > > > + :widths: 1 1 2
> > > > +
> > > > + * - __s32
> > > > + - ``pic_order_cnt_val``
> > > > + -
> > >
> > > Can be documented as:
> > >
> > > """
> > > PicOrderCntVal as described in section 8.3.1 "Decoding process
> > > for picture order count" of the specification.
> > > """
> > >
> > > Note that snake case is used to match the kernel style,
> > > but other than that we try to keep the HEVC spec variable
> > > names.
> > >
> > > > + * - __u8
> > > > + - ``num_active_dpb_entries``
> > > > + - The number of entries in ``dpb``.
> > > > + * - struct :c:type:`v4l2_hevc_dpb_entry`
> > > > + - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > + - The decoded picture buffer, for meta-data about reference
frames.
> > >
> > > The DPB is here, but it seems it's also in the slice control?
> > >
> > > > + * - __u8
> > > > + - ``num_rps_poc_st_curr_before``
> > > > + - The number of reference pictures in the short-term set that
come
> > before
> > > > + the current frame.
> > > > + * - __u8
> > > > + - ``num_rps_poc_st_curr_after``
> > > > + - The number of reference pictures in the short-term set that
come
> > after
> > > > + the current frame.
> > > > + * - __u8
> > > > + - ``num_rps_poc_lt_curr``
> > > > + - The number of reference pictures in the long-term set.
> > > > + * - __u8
> > > > + - ``rps_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > + -
> > > > + * - __u8
> > > > + - ``rps_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > + -
> > > > + * - __u8
> > > > + - ``rps_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > + -
> > >
> > > Could you document these as well?
> > >
> > > Thanks a lot,
> > > Ezequiel
> > >
> > >
> >
> >
>
>
>
On Thu, 25 Feb 2021 19:05:55 +0100, you wrote:
>Dne ?etrtek, 25. februar 2021 ob 18:34:48 CET je Ezequiel Garcia napisal(a):
>> Hey Jernej,
>>
>> On Thu, 2021-02-25 at 18:01 +0100, Jernej Škrabec wrote:
>> > Hi Ezequiel,
>> >
>> > Dne ?etrtek, 25. februar 2021 ob 14:09:52 CET je Ezequiel Garcia
>napisal(a):
>> > > Hi Benjamin,
>> > >
>> > > Thanks for the good work.
>> > >
>> > > On Mon, 2021-02-22 at 13:23 +0100, Benjamin Gaignard wrote:
>> > > > The H.265 ITU specification (section 7.4) define the general
>> > > > slice segment header semantics.
>> > > > Modified/added fields are:
>> > > > - video_parameter_set_id: (7.4.3.1) identifies the VPS for
>> > > > reference by other syntax elements.
>> > > > - seq_parameter_set_id: (7.4.3.2.1) specifies the value of
>> > > > the vps_video_parameter_set_id of the active VPS.
>> > > > - chroma_format_idc: (7.4.3.2.1) specifies the chroma sampling
>> > > > relative to the luma sampling
>> > > > - pic_parameter_set_id: (7.4.3.3.1) identifies the PPS for
>> > > > reference by other syntax elements
>> > > > - num_ref_idx_l0_default_active_minus1: (7.4.3.3.1) specifies
>> > > > the inferred value of num_ref_idx_l0_active_minus1
>> > > > - num_ref_idx_l1_default_active_minus1: (7.4.3.3.1) specifies
>> > > > the inferred value of num_ref_idx_l1_active_minus1
>> > > > - slice_segment_addr: (7.4.7.1) specifies the address of
>> > > > the first coding tree block in the slice segment
>> > > > - num_entry_point_offsets: (7.4.7.1) specifies the number of
>> > > > entry_point_offset_minus1[ i ] syntax elements in the slice header
>> > > >
>> > > > Add HEVC decode params contains the information used in section
>> > > > "8.3 Slice decoding process" of the specification to let the hardware
>> > > > perform decoding of a slices.
>> > > >
>> > > > Adapt Cedrus driver according to these changes.
>> > > >
>> > > > Signed-off-by: Benjamin Gaignard <[email protected]>
>> > > > ---
>> > > > version 3:
>> > > > - Add documentation about the new structuers and fields.
>> > > >
>> > > > version 2:
>> > > > - remove all change related to scaling
>> > > > - squash commits to a coherent split
>> > > > - be more verbose about the added fields
>> > > >
>> > > > .../media/v4l/ext-ctrls-codec.rst | 126 ++++++++++++++
>+---
>> > > > .../media/v4l/vidioc-queryctrl.rst | 6 +
>> > > > drivers/media/v4l2-core/v4l2-ctrls.c | 26 +++-
>> > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +
>> > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 +
>> > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 +
>> > > > .../staging/media/sunxi/cedrus/cedrus_h265.c | 6 +-
>> > > > include/media/hevc-ctrls.h | 45 +++++--
>> > > > 8 files changed, 186 insertions(+), 32 deletions(-)
>> > > >
>> > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>b/
>> > Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> > > > index 00944e97d638..5e6d77e858c0 100644
>> > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> > > > @@ -3109,6 +3109,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>> > > > :stub-columns: 0
>> > > > :widths: 1 1 2
>> > > >
>> > > > + * - __u8
>> > > > + - ``video_parameter_set_id``
>> > > > + - Identifies the VPS for reference by other syntax elements
>> > > > + * - __u8
>> > > > + - ``seq_parameter_set_id?``
>> > > > + - Specifies the value of the vps_video_parameter_set_id of the
>> > active VPS
>> > > > + * - __u8
>> > > > + - ``chroma_format_idc``
>> > > > + - Specifies the chroma sampling relative to the luma sampling
>> > >
>> > > None of these fields seem needed for the Hantro G2 driver,
>> > > so I suggest you drop them for now.
>> > >
>> > > > * - __u16
>> > > > - ``pic_width_in_luma_samples``
>> > > > -
>> > > > @@ -3172,6 +3181,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>> > > > * - __u8
>> > > > - ``chroma_format_idc``
>> > > > -
>> > > > + * - __u8
>> > > > + - ``num_slices``
>> > > > +
>> > >
>> > > Not used, but also doesn't seem part of the SPS syntax. If we have to
>> > > pass the number of slices, we'll need another mechanism.
>> > >
>> > > > -
>> > > > * - __u64
>> > > > - ``flags``
>> > > > - See :ref:`Sequence Parameter Set Flags <hevc_sps_flags>`
>> > > > @@ -3231,9 +3243,18 @@ 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
>> > >
>> > > Not used.
>> > >
>> > > > * - __u8
>> > > > - ``num_extra_slice_header_bits``
>> > > > -
>> > > > + * - __u8
>> > > > + - ``num_ref_idx_l0_default_active_minus1``
>> > > > + - Specifies the inferred value of num_ref_idx_l0_active_minus1
>> > > > + * - __u8
>> > > > + - ``num_ref_idx_l1_default_active_minus1``
>> > > > + - Specifies the inferred value of num_ref_idx_l1_active_minus1
>> > > > * - __s8
>> > > > - ``init_qp_minus26``
>> > > > -
>> > > > @@ -3342,6 +3363,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>> > > > * - ``V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT``
>> > > > - 0x00040000
>> > > > -
>> > > > + * - ``V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT``
>> > > > + - 0x00080000
>> > > > + -
>> > > > + * - ``V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING``
>> > > > + - 0x00100000
>> > > > + -
>> > > >
>> > >
>> > > I suggest to do all the PPS control changes in a separate patch,
>> > > feels easier to review and cleaner as you can explain the
>> > > changes with more detail in the commit description.
>> > >
>> > > Looking at the PPS syntax for tiles, I'm wondering if these
>> > > deserve their own control, which would be used if tiles are enabled,
>> > > i.e. V4L2_HEVC_PPS_FLAG_TILES_ENABLED is set.
>> > >
>> > > __u8
>num_tile_columns_minus1;
>> > > __u8
>num_tile_rows_minus1;
>> > > __u8
>column_width_minus1[20];
>> > > __u8 row_height_minus1[22];
>> > >
>> > > Not something we necessarily have to tackle now.
>> > >
>> > > > ``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (struct)``
>> > > > Specifies various slice-specific parameters, especially from the
>NAL
>> > unit
>> > > > @@ -3366,6 +3393,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>> > > > * - __u32
>> > > > - ``data_bit_offset``
>> > > > - Offset (in bits) to the video data in the current slice data.
>> > > > + * - __u32
>> > > > + - ``slice_segment_addr``
>> > > > + - Specifies the address of the first coding tree block in the
>slice
>> > segment
>> > >
>> > > Not used.
>> > >
>> > > > + * - __u32
>> > > > + - ``num_entry_point_offsets``
>> > > > + - Specifies the number of entry_point_offset_minus1[ i ] syntax
>> > elements in the slice header
>> > >
>> > > Not used.
>> >
>> > While above two fields may not be used in Hantro, they are for sure useful
>for
>> > Cedrus and RPi4. I would like to keep them, otherwise with such approach
>HEVC
>> > will stay in staging for a long time. I'm still baffled why scaling matrix
>> > control was dropped. It would fit well in Cedrus and RPi4 driver and after
>a
>> > quick look, it seems that it was used in driver in later patch.
>> >
>>
>> I'd like to make sure each modification we are making to the uAPI
>> goes in the right direction, that is in the direction of moving
>> the API out of staging.
>>
>> Since reviewing each field is quite hard, and opens some discussions,
>> I wanted to keep this patchset specific to what's needed for Hantro G2.
>>
>> The Scaling matrix control is certainly a good one, as well as the ones
>> needed for Cedrus and RPi4. However, I feel it's better to discuss
>> them in their own "uAPI review" series so we can review all the changes
>> with an API hat.
>>
>> This way we decouple the Hantro G2 discussion and work from the API work.
>>
>> Also please feel free to submit RFC patches fo Cedrus and RPi4
>> (API and driver changes). We can certainly start the discussion around that,
>> with driver changes in context.
>
>I don't know much about RPi4 driver, only few implementation details, so
>you'll have to ping developer who wrote it. Regarding HEVC on Cedrus - it has
>one pain point - it needs entry point table which in turn needs support for
>variable arrays in order to be feasable AFAIK. I don't plan to develop that.
>Patches for scaling matrix and segment address were sent a bit more than a
>year ago but were turned down because they change control structures (among
>other things). Sorry to say, but I work on other things now, so Cedrus will
>have to wait. Alternatively, someone can take my patches from LibreELEC,
>update and submit them. They are in use for a long time.
It seems to me that H.265 should be the source for what fields are
needed in the uapi - not whatever happens to be supported by current
h/w. The standard defines the list of fields that can occur in the
parameter sets and headers, and everything that is needed to decode
any slice_seqgment_data should be in the upai. Eventually all those
fields are going to be needed (they aren't in the standard just to look
pretty) and given the reluctance I've observed to change the uapi once
released they should be there from the start.
Now some hardware requires more fields that aren't in the standard -
those can (and should) be added in h/w specific extensions.
Regards
JC
>Best regards,
>Jernej
>
>>
>> Hope I'm making sense here :)
>>
>> Thanks,
>> Ezequiel
>>
>> > Best regards,
>> > Jernej
>> >
>> > >
>> > > > * - __u8
>> > > > - ``nal_unit_type``
>> > > > -
>> > > > @@ -3422,28 +3455,20 @@ enum v4l2_mpeg_video_hevc_size_of_length_field
>-
>> > > > * - __u8
>> > > > - ``pic_struct``
>> > > > -
>> > > > - * - __u8
>> > > > - - ``num_active_dpb_entries``
>> > > > - - The number of entries in ``dpb``.
>> > >
>> > > Need to explain in the commit description why this field is moved.
>> > >
>> > > > * - __u8
>> > > > - ``ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>> > > > - The list of L0 reference elements as indices in the DPB.
>> > > > * - __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``
>> > > > +
>> > >
>> > > Not used.
>> > >
>> > > > -
>> > > > + * - __u16
>> > > > + - ``long_term_ref_pic_set_size``
>> > > > + -
>> > >
>> > > Not used.
>> > >
>> > > > * - __u8
>> > > > - - ``num_rps_poc_st_curr_before``
>> > > > - - The number of reference pictures in the short-term set that
>come
>> > before
>> > > > - the current frame.
>> > >
>> > > If this matches NumPocStCurrBefore from section 8.3.2 "Decoding process
>for
>> > reference picture set"
>> > > then I would document that. And perhaps rename it to
>num_poc_st_curr_before.
>> > >
>> > > > - * - __u8
>> > > > - - ``num_rps_poc_st_curr_after``
>> > > > - - The number of reference pictures in the short-term set that
>come
>> > after
>> > > > - the current frame.
>> > >
>> > > Ditto.
>> > >
>> > > > - * - __u8
>> > > > - - ``num_rps_poc_lt_curr``
>> > > > - - The number of reference pictures in the long-term set.
>> > >
>> > > Ditto.
>> > >
>> > > Also, I'd like the changes that move fields from
>> > V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS
>> > > to the new V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS control, to be in
>their
>> > > patch.
>> > >
>> > > That will allow us to put in the commit description a proper
>> > > explanation of why are fields being moved. Nothing fancy, simply
>> > > explaining that these variables come from section 8.3.2
>> > > "Decoding process for reference picture set", which describes
>> > > a process invoked once per picture, so they are not per-slice.
>> > >
>> > > > - * - __u8
>> > > > - - ``padding[7]``
>> > > > + - ``padding``
>> > > > - Applications and drivers must set this to zero.
>> > > > * - struct :c:type:`v4l2_hevc_dpb_entry`
>> > > > - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>> > > > @@ -3646,3 +3671,74 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>> > > > so this has to come from client.
>> > > > This is applicable to H264 and valid Range is from 0 to 63.
>> > > > Source Rec. ITU-T H.264 (06/2019); G.7.4.1.1, G.8.8.1.
>> > > > +
>> > > > +``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (struct)``
>> > > > + Specifies various decode parameters, especially the references
>picture
>> > order
>> > > > + count (POC) for all the lists (short, long, before, current,
>after)
>> > and the
>> > > > + number of entries for each of them.
>> > > > + These parameters are defined according to :ref:`hevc`.
>> > > > + They are described in section 8.3 "Slice decoding process" of the
>> > > > + specification.
>> > > > +
>> > > > +.. c:type:: v4l2_ctrl_hevc_decode_params
>> > > > +
>> > > > +.. cssclass:: longtable
>> > > > +
>> > > > +.. flat-table:: struct v4l2_ctrl_hevc_decode_params
>> > > > + :header-rows: 0
>> > > > + :stub-columns: 0
>> > > > + :widths: 1 1 2
>> > > > +
>> > > > + * - __s32
>> > > > + - ``pic_order_cnt_val``
>> > > > + -
>> > >
>> > > Can be documented as:
>> > >
>> > > """
>> > > PicOrderCntVal as described in section 8.3.1 "Decoding process
>> > > for picture order count" of the specification.
>> > > """
>> > >
>> > > Note that snake case is used to match the kernel style,
>> > > but other than that we try to keep the HEVC spec variable
>> > > names.
>> > >
>> > > > + * - __u8
>> > > > + - ``num_active_dpb_entries``
>> > > > + - The number of entries in ``dpb``.
>> > > > + * - struct :c:type:`v4l2_hevc_dpb_entry`
>> > > > + - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>> > > > + - The decoded picture buffer, for meta-data about reference
>frames.
>> > >
>> > > The DPB is here, but it seems it's also in the slice control?
>> > >
>> > > > + * - __u8
>> > > > + - ``num_rps_poc_st_curr_before``
>> > > > + - The number of reference pictures in the short-term set that
>come
>> > before
>> > > > + the current frame.
>> > > > + * - __u8
>> > > > + - ``num_rps_poc_st_curr_after``
>> > > > + - The number of reference pictures in the short-term set that
>come
>> > after
>> > > > + the current frame.
>> > > > + * - __u8
>> > > > + - ``num_rps_poc_lt_curr``
>> > > > + - The number of reference pictures in the long-term set.
>> > > > + * - __u8
>> > > > + - ``rps_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>> > > > + -
>> > > > + * - __u8
>> > > > + - ``rps_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>> > > > + -
>> > > > + * - __u8
>> > > > + - ``rps_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>> > > > + -
>> > >
>> > > Could you document these as well?
>> > >
>> > > Thanks a lot,
>> > > Ezequiel
>> > >
>> > >
>> >
>> >
>>
>>
>>
>
Le jeudi 25 février 2021 à 10:09 -0300, Ezequiel Garcia a écrit :
> Hi Benjamin,
>
> Thanks for the good work.
>
> On Mon, 2021-02-22 at 13:23 +0100, Benjamin Gaignard wrote:
> > The H.265 ITU specification (section 7.4) define the general
> > slice segment header semantics.
> > Modified/added fields are:
> > - video_parameter_set_id: (7.4.3.1) identifies the VPS for
> > reference by other syntax elements.
> > - seq_parameter_set_id: (7.4.3.2.1) specifies the value of
> > the vps_video_parameter_set_id of the active VPS.
> > - chroma_format_idc: (7.4.3.2.1) specifies the chroma sampling
> > relative to the luma sampling
> > - pic_parameter_set_id: (7.4.3.3.1) identifies the PPS for
> > reference by other syntax elements
> > - num_ref_idx_l0_default_active_minus1: (7.4.3.3.1) specifies
> > the inferred value of num_ref_idx_l0_active_minus1
> > - num_ref_idx_l1_default_active_minus1: (7.4.3.3.1) specifies
> > the inferred value of num_ref_idx_l1_active_minus1
> > - slice_segment_addr: (7.4.7.1) specifies the address of
> > the first coding tree block in the slice segment
> > - num_entry_point_offsets: (7.4.7.1) specifies the number of
> > entry_point_offset_minus1[ i ] syntax elements in the slice header
> >
> > Add HEVC decode params contains the information used in section
> > "8.3 Slice decoding process" of the specification to let the hardware
> > perform decoding of a slices.
> >
> > Adapt Cedrus driver according to these changes.
> >
> > Signed-off-by: Benjamin Gaignard <[email protected]>
> > ---
> > version 3:
> > - Add documentation about the new structuers and fields.
> >
> > version 2:
> > - remove all change related to scaling
> > - squash commits to a coherent split
> > - be more verbose about the added fields
> >
> > .../media/v4l/ext-ctrls-codec.rst | 126 +++++++++++++++---
> > .../media/v4l/vidioc-queryctrl.rst | 6 +
> > drivers/media/v4l2-core/v4l2-ctrls.c | 26 +++-
> > drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +
> > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 +
> > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 +
> > .../staging/media/sunxi/cedrus/cedrus_h265.c | 6 +-
> > include/media/hevc-ctrls.h | 45 +++++--
> > 8 files changed, 186 insertions(+), 32 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 00944e97d638..5e6d77e858c0 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -3109,6 +3109,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > :stub-columns: 0
> > :widths: 1 1 2
> >
> > + * - __u8
> > + - ``video_parameter_set_id``
> > + - Identifies the VPS for reference by other syntax elements
> > + * - __u8
> > + - ``seq_parameter_set_id̀``
> > + - Specifies the value of the vps_video_parameter_set_id of the active
> > VPS
> > + * - __u8
> > + - ``chroma_format_idc``
> > + - Specifies the chroma sampling relative to the luma sampling
>
> None of these fields seem needed for the Hantro G2 driver,
> so I suggest you drop them for now.
You should be using chroma_format_idc in your validation. I think Hantro G2 does
not do YUV 4:4:4, which is what chroma_format_idc tells the driver. Our
implementation might also be missing 4:0:0 (black and white).
As this value will aftect the buffer size, please keep them, and ideally
validate them. Note that it feels rather problematic / unsafe situation as we
endup having to trust userspace, hopefully the HW validates it's buffer size ?
(do we tell the HW the buffer sizes ?)
>
> > * - __u16
> > - ``pic_width_in_luma_samples``
> > -
> > @@ -3172,6 +3181,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > * - __u8
> > - ``chroma_format_idc``
> > -
> > + * - __u8
> > + - ``num_slices``
> > +
>
> Not used, but also doesn't seem part of the SPS syntax. If we have to
> pass the number of slices, we'll need another mechanism.
>
> > -
> > * - __u64
> > - ``flags``
> > - See :ref:`Sequence Parameter Set Flags <hevc_sps_flags>`
> > @@ -3231,9 +3243,18 @@ 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
>
> Not used.
>
> > * - __u8
> > - ``num_extra_slice_header_bits``
> > -
> > + * - __u8
> > + - ``num_ref_idx_l0_default_active_minus1``
> > + - Specifies the inferred value of num_ref_idx_l0_active_minus1
> > + * - __u8
> > + - ``num_ref_idx_l1_default_active_minus1``
> > + - Specifies the inferred value of num_ref_idx_l1_active_minus1
> > * - __s8
> > - ``init_qp_minus26``
> > -
> > @@ -3342,6 +3363,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > * - ``V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT``
> > - 0x00040000
> > -
> > + * - ``V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT``
> > + - 0x00080000
> > + -
> > + * - ``V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING``
> > + - 0x00100000
> > + -
> >
>
> I suggest to do all the PPS control changes in a separate patch,
> feels easier to review and cleaner as you can explain the
> changes with more detail in the commit description.
>
> Looking at the PPS syntax for tiles, I'm wondering if these
> deserve their own control, which would be used if tiles are enabled,
> i.e. V4L2_HEVC_PPS_FLAG_TILES_ENABLED is set.
>
> __u8
> num_tile_columns_minus1;
> __u8
> num_tile_rows_minus1;
> __u8
> column_width_minus1[20];
> __u8 row_height_minus1[22];
>
> Not something we necessarily have to tackle now.
>
> > ``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (struct)``
> > Specifies various slice-specific parameters, especially from the NAL
> > unit
> > @@ -3366,6 +3393,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > * - __u32
> > - ``data_bit_offset``
> > - Offset (in bits) to the video data in the current slice data.
> > + * - __u32
> > + - ``slice_segment_addr``
> > + - Specifies the address of the first coding tree block in the slice
> > segment
>
> Not used.
>
> > + * - __u32
> > + - ``num_entry_point_offsets``
> > + - Specifies the number of entry_point_offset_minus1[ i ] syntax
> > elements in the slice header
>
> Not used.
>
> > * - __u8
> > - ``nal_unit_type``
> > -
> > @@ -3422,28 +3455,20 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > * - __u8
> > - ``pic_struct``
> > -
> > - * - __u8
> > - - ``num_active_dpb_entries``
> > - - The number of entries in ``dpb``.
>
> Need to explain in the commit description why this field is moved.
>
> > * - __u8
> > - ``ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > - The list of L0 reference elements as indices in the DPB.
> > * - __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``
> > +
>
> Not used.
>
> > -
> > + * - __u16
> > + - ``long_term_ref_pic_set_size``
> > + -
>
> Not used.
>
> > * - __u8
> > - - ``num_rps_poc_st_curr_before``
> > - - The number of reference pictures in the short-term set that come
> > before
> > - the current frame.
>
> If this matches NumPocStCurrBefore from section 8.3.2 "Decoding process for
> reference picture set"
> then I would document that. And perhaps rename it to num_poc_st_curr_before.
>
> > - * - __u8
> > - - ``num_rps_poc_st_curr_after``
> > - - The number of reference pictures in the short-term set that come
> > after
> > - the current frame.
>
> Ditto.
>
> > - * - __u8
> > - - ``num_rps_poc_lt_curr``
> > - - The number of reference pictures in the long-term set.
>
> Ditto.
>
> Also, I'd like the changes that move fields from
> V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS
> to the new V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS control, to be in their
> patch.
>
> That will allow us to put in the commit description a proper
> explanation of why are fields being moved. Nothing fancy, simply
> explaining that these variables come from section 8.3.2
> "Decoding process for reference picture set", which describes
> a process invoked once per picture, so they are not per-slice.
>
> > - * - __u8
> > - - ``padding[7]``
> > + - ``padding``
> > - Applications and drivers must set this to zero.
> > * - struct :c:type:`v4l2_hevc_dpb_entry`
> > - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > @@ -3646,3 +3671,74 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > so this has to come from client.
> > This is applicable to H264 and valid Range is from 0 to 63.
> > Source Rec. ITU-T H.264 (06/2019); G.7.4.1.1, G.8.8.1.
> > +
> > +``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (struct)``
> > + Specifies various decode parameters, especially the references picture
> > order
> > + count (POC) for all the lists (short, long, before, current, after) and
> > the
> > + number of entries for each of them.
> > + These parameters are defined according to :ref:`hevc`.
> > + They are described in section 8.3 "Slice decoding process" of the
> > + specification.
> > +
> > +.. c:type:: v4l2_ctrl_hevc_decode_params
> > +
> > +.. cssclass:: longtable
> > +
> > +.. flat-table:: struct v4l2_ctrl_hevc_decode_params
> > + :header-rows: 0
> > + :stub-columns: 0
> > + :widths: 1 1 2
> > +
> > + * - __s32
> > + - ``pic_order_cnt_val``
> > + -
>
> Can be documented as:
>
> """
> PicOrderCntVal as described in section 8.3.1 "Decoding process
> for picture order count" of the specification.
> """
>
> Note that snake case is used to match the kernel style,
> but other than that we try to keep the HEVC spec variable
> names.
>
> > + * - __u8
> > + - ``num_active_dpb_entries``
> > + - The number of entries in ``dpb``.
> > + * - struct :c:type:`v4l2_hevc_dpb_entry`
> > + - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > + - The decoded picture buffer, for meta-data about reference frames.
>
> The DPB is here, but it seems it's also in the slice control?
>
> > + * - __u8
> > + - ``num_rps_poc_st_curr_before``
> > + - The number of reference pictures in the short-term set that come
> > before
> > + the current frame.
> > + * - __u8
> > + - ``num_rps_poc_st_curr_after``
> > + - The number of reference pictures in the short-term set that come
> > after
> > + the current frame.
> > + * - __u8
> > + - ``num_rps_poc_lt_curr``
> > + - The number of reference pictures in the long-term set.
> > + * - __u8
> > + - ``rps_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > + -
> > + * - __u8
> > + - ``rps_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > + -
> > + * - __u8
> > + - ``rps_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > + -
>
> Could you document these as well?
>
> Thanks a lot,
> Ezequiel
>
>
Le jeudi 25 février 2021 à 18:34 +0000, John Cox a écrit :
> On Thu, 25 Feb 2021 19:05:55 +0100, you wrote:
>
> > Dne ?etrtek, 25. februar 2021 ob 18:34:48 CET je Ezequiel Garcia napisal(a):
> > > Hey Jernej,
> > >
> > > On Thu, 2021-02-25 at 18:01 +0100, Jernej Škrabec wrote:
> > > > Hi Ezequiel,
> > > >
> > > > Dne ?etrtek, 25. februar 2021 ob 14:09:52 CET je Ezequiel Garcia
> > napisal(a):
> > > > > Hi Benjamin,
> > > > >
> > > > > Thanks for the good work.
> > > > >
> > > > > On Mon, 2021-02-22 at 13:23 +0100, Benjamin Gaignard wrote:
> > > > > > The H.265 ITU specification (section 7.4) define the general
> > > > > > slice segment header semantics.
> > > > > > Modified/added fields are:
> > > > > > - video_parameter_set_id: (7.4.3.1) identifies the VPS for
> > > > > > reference by other syntax elements.
> > > > > > - seq_parameter_set_id: (7.4.3.2.1) specifies the value of
> > > > > > the vps_video_parameter_set_id of the active VPS.
> > > > > > - chroma_format_idc: (7.4.3.2.1) specifies the chroma sampling
> > > > > > relative to the luma sampling
> > > > > > - pic_parameter_set_id: (7.4.3.3.1) identifies the PPS for
> > > > > > reference by other syntax elements
> > > > > > - num_ref_idx_l0_default_active_minus1: (7.4.3.3.1) specifies
> > > > > > the inferred value of num_ref_idx_l0_active_minus1
> > > > > > - num_ref_idx_l1_default_active_minus1: (7.4.3.3.1) specifies
> > > > > > the inferred value of num_ref_idx_l1_active_minus1
> > > > > > - slice_segment_addr: (7.4.7.1) specifies the address of
> > > > > > the first coding tree block in the slice segment
> > > > > > - num_entry_point_offsets: (7.4.7.1) specifies the number of
> > > > > > entry_point_offset_minus1[ i ] syntax elements in the slice header
> > > > > >
> > > > > > Add HEVC decode params contains the information used in section
> > > > > > "8.3 Slice decoding process" of the specification to let the
> > > > > > hardware
> > > > > > perform decoding of a slices.
> > > > > >
> > > > > > Adapt Cedrus driver according to these changes.
> > > > > >
> > > > > > Signed-off-by: Benjamin Gaignard <[email protected]>
> > > > > > ---
> > > > > > version 3:
> > > > > > - Add documentation about the new structuers and fields.
> > > > > >
> > > > > > version 2:
> > > > > > - remove all change related to scaling
> > > > > > - squash commits to a coherent split
> > > > > > - be more verbose about the added fields
> > > > > >
> > > > > > .../media/v4l/ext-ctrls-codec.rst | 126 ++++++++++++++
> > +---
> > > > > > .../media/v4l/vidioc-queryctrl.rst | 6 +
> > > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 26 +++-
> > > > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +
> > > > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 +
> > > > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 +
> > > > > > .../staging/media/sunxi/cedrus/cedrus_h265.c | 6 +-
> > > > > > include/media/hevc-ctrls.h | 45 +++++--
> > > > > > 8 files changed, 186 insertions(+), 32 deletions(-)
> > > > > >
> > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-
> > > > > > codec.rst
> > b/
> > > > Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > > > index 00944e97d638..5e6d77e858c0 100644
> > > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > > > @@ -3109,6 +3109,15 @@ enum
> > > > > > v4l2_mpeg_video_hevc_size_of_length_field -
> > > > > > :stub-columns: 0
> > > > > > :widths: 1 1 2
> > > > > >
> > > > > > + * - __u8
> > > > > > + - ``video_parameter_set_id``
> > > > > > + - Identifies the VPS for reference by other syntax elements
> > > > > > + * - __u8
> > > > > > + - ``seq_parameter_set_id?``
> > > > > > + - Specifies the value of the vps_video_parameter_set_id of
> > > > > > the
> > > > active VPS
> > > > > > + * - __u8
> > > > > > + - ``chroma_format_idc``
> > > > > > + - Specifies the chroma sampling relative to the luma sampling
> > > > >
> > > > > None of these fields seem needed for the Hantro G2 driver,
> > > > > so I suggest you drop them for now.
> > > > >
> > > > > > * - __u16
> > > > > > - ``pic_width_in_luma_samples``
> > > > > > -
> > > > > > @@ -3172,6 +3181,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field
> > > > > > -
> > > > > > * - __u8
> > > > > > - ``chroma_format_idc``
> > > > > > -
> > > > > > + * - __u8
> > > > > > + - ``num_slices``
> > > > > > +
> > > > >
> > > > > Not used, but also doesn't seem part of the SPS syntax. If we have to
> > > > > pass the number of slices, we'll need another mechanism.
> > > > >
> > > > > > -
> > > > > > * - __u64
> > > > > > - ``flags``
> > > > > > - See :ref:`Sequence Parameter Set Flags <hevc_sps_flags>`
> > > > > > @@ -3231,9 +3243,18 @@ 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
> > > > >
> > > > > Not used.
> > > > >
> > > > > > * - __u8
> > > > > > - ``num_extra_slice_header_bits``
> > > > > > -
> > > > > > + * - __u8
> > > > > > + - ``num_ref_idx_l0_default_active_minus1``
> > > > > > + - Specifies the inferred value of
> > > > > > num_ref_idx_l0_active_minus1
> > > > > > + * - __u8
> > > > > > + - ``num_ref_idx_l1_default_active_minus1``
> > > > > > + - Specifies the inferred value of
> > > > > > num_ref_idx_l1_active_minus1
> > > > > > * - __s8
> > > > > > - ``init_qp_minus26``
> > > > > > -
> > > > > > @@ -3342,6 +3363,12 @@ enum
> > > > > > v4l2_mpeg_video_hevc_size_of_length_field -
> > > > > > * -
> > > > > > ``V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT``
> > > > > > - 0x00040000
> > > > > > -
> > > > > > + * - ``V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT``
> > > > > > + - 0x00080000
> > > > > > + -
> > > > > > + * - ``V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING``
> > > > > > + - 0x00100000
> > > > > > + -
> > > > > >
> > > > >
> > > > > I suggest to do all the PPS control changes in a separate patch,
> > > > > feels easier to review and cleaner as you can explain the
> > > > > changes with more detail in the commit description.
> > > > >
> > > > > Looking at the PPS syntax for tiles, I'm wondering if these
> > > > > deserve their own control, which would be used if tiles are enabled,
> > > > > i.e. V4L2_HEVC_PPS_FLAG_TILES_ENABLED is set.
> > > > >
> > > > > __u8
> > num_tile_columns_minus1;
> > > > > __u8
> > num_tile_rows_minus1;
> > > > > __u8
> > column_width_minus1[20];
> > > > > __u8 row_height_minus1[22];
> > > > >
> > > > > Not something we necessarily have to tackle now.
> > > > >
> > > > > > ``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (struct)``
> > > > > > Specifies various slice-specific parameters, especially from
> > > > > > the
> > NAL
> > > > unit
> > > > > > @@ -3366,6 +3393,12 @@ enum
> > > > > > v4l2_mpeg_video_hevc_size_of_length_field -
> > > > > > * - __u32
> > > > > > - ``data_bit_offset``
> > > > > > - Offset (in bits) to the video data in the current slice
> > > > > > data.
> > > > > > + * - __u32
> > > > > > + - ``slice_segment_addr``
> > > > > > + - Specifies the address of the first coding tree block in the
> > slice
> > > > segment
> > > > >
> > > > > Not used.
> > > > >
> > > > > > + * - __u32
> > > > > > + - ``num_entry_point_offsets``
> > > > > > + - Specifies the number of entry_point_offset_minus1[ i ]
> > > > > > syntax
> > > > elements in the slice header
> > > > >
> > > > > Not used.
> > > >
> > > > While above two fields may not be used in Hantro, they are for sure
> > > > useful
> > for
> > > > Cedrus and RPi4. I would like to keep them, otherwise with such approach
> > HEVC
> > > > will stay in staging for a long time. I'm still baffled why scaling
> > > > matrix
> > > > control was dropped. It would fit well in Cedrus and RPi4 driver and
> > > > after
> > a
> > > > quick look, it seems that it was used in driver in later patch.
> > > >
> > >
> > > I'd like to make sure each modification we are making to the uAPI
> > > goes in the right direction, that is in the direction of moving
> > > the API out of staging.
> > >
> > > Since reviewing each field is quite hard, and opens some discussions,
> > > I wanted to keep this patchset specific to what's needed for Hantro G2.
> > >
> > > The Scaling matrix control is certainly a good one, as well as the ones
> > > needed for Cedrus and RPi4. However, I feel it's better to discuss
> > > them in their own "uAPI review" series so we can review all the changes
> > > with an API hat.
> > >
> > > This way we decouple the Hantro G2 discussion and work from the API work.
> > >
> > > Also please feel free to submit RFC patches fo Cedrus and RPi4
> > > (API and driver changes). We can certainly start the discussion around
> > > that,
> > > with driver changes in context.
> >
> > I don't know much about RPi4 driver, only few implementation details, so
> > you'll have to ping developer who wrote it. Regarding HEVC on Cedrus - it
> > has
> > one pain point - it needs entry point table which in turn needs support for
> > variable arrays in order to be feasable AFAIK. I don't plan to develop that.
> > Patches for scaling matrix and segment address were sent a bit more than a
> > year ago but were turned down because they change control structures (among
> > other things). Sorry to say, but I work on other things now, so Cedrus will
> > have to wait. Alternatively, someone can take my patches from LibreELEC,
> > update and submit them. They are in use for a long time.
>
> It seems to me that H.265 should be the source for what fields are
> needed in the uapi - not whatever happens to be supported by current
> h/w. The standard defines the list of fields that can occur in the
> parameter sets and headers, and everything that is needed to decode
> any slice_seqgment_data should be in the upai. Eventually all those
> fields are going to be needed (they aren't in the standard just to look
> pretty) and given the reluctance I've observed to change the uapi once
> released they should be there from the start.
>
> Now some hardware requires more fields that aren't in the standard -
> those can (and should) be added in h/w specific extensions.
While this is not wrong, there is obvious bitstream parameters that are not of
the HW accelerator domain. We did the work of deciding what to omit in the H264
uAPI, and we'll do that for HEVC for sure. But now is not the time yet.
Now the point that you seem to be missing is that Benjamin is trying to stage a
driver, not a uAPI. With this third driver, we can have more folks contributing
and testing toward our goal of unstaging the uAPI.
After this, we'll be able to iteratively add features (like scaling list) to
this driver along with the required uAPI and discuss each of these needed API
separatly. Note that previous iteration, the driver implementation for the
scaling list wasn't correct, and for this reason, the uAPI wasn't being
validated.
When this is getting stable enough, there will be a final stage were someone
will have to pick the uAPI and fill in the blank with remaining field that has a
strong probability to be used by later HW WITHIN the supported HEVC profiles et
have defined.
I emphasis on that because there is a lot of HEVC (and H264) extensions we
haven't covered or mentionned, and these will be added with dedicated controls
when we have a use case for it of course. Remember that adding controls is not a
problem, we need these basic control of the basic decoding, and for sure, over
the decade to come we will have to introduce new controls for other stuff we
haven't supported yet.
All this may look like taking forever, but as Jernej states, not everyone can
afford contributing upstream. So unless someone step in, this pace is about what
we can offer, and we strongly believe that it will converge. I do personnally
expect HEVC to take more time as the codec rationales are mostly kept secret,
and our answers burried into a mountain of reference code. It's also, if you
defocus from the film industry, just one CODEC. Notably, for the web, VP9 is
more important, and this will likely puts some shadow on this work.
Meawhile, you are strongly encouraged if you can afford it to open the
discussion around the RPi4 multi-stage HEVC decoder. While doing this, you could
send a patched for staging the RPi4 driver. The benefit would be that we get to
update your drivers when we update the uAPI, saving you times, and also giving
everyone more data on what is strictly needed.
>
> Regards
>
> JC
>
> > Best regards,
> > Jernej
> >
> > >
> > > Hope I'm making sense here :)
> > >
> > > Thanks,
> > > Ezequiel
> > >
> > > > Best regards,
> > > > Jernej
> > > >
> > > > >
> > > > > > * - __u8
> > > > > > - ``nal_unit_type``
> > > > > > -
> > > > > > @@ -3422,28 +3455,20 @@ enum
> > > > > > v4l2_mpeg_video_hevc_size_of_length_field
> > -
> > > > > > * - __u8
> > > > > > - ``pic_struct``
> > > > > > -
> > > > > > - * - __u8
> > > > > > - - ``num_active_dpb_entries``
> > > > > > - - The number of entries in ``dpb``.
> > > > >
> > > > > Need to explain in the commit description why this field is moved.
> > > > >
> > > > > > * - __u8
> > > > > > - ``ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > > > - The list of L0 reference elements as indices in the DPB.
> > > > > > * - __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``
> > > > > > +
> > > > >
> > > > > Not used.
> > > > >
> > > > > > -
> > > > > > + * - __u16
> > > > > > + - ``long_term_ref_pic_set_size``
> > > > > > + -
> > > > >
> > > > > Not used.
> > > > >
> > > > > > * - __u8
> > > > > > - - ``num_rps_poc_st_curr_before``
> > > > > > - - The number of reference pictures in the short-term set that
> > come
> > > > before
> > > > > > - the current frame.
> > > > >
> > > > > If this matches NumPocStCurrBefore from section 8.3.2 "Decoding
> > > > > process
> > for
> > > > reference picture set"
> > > > > then I would document that. And perhaps rename it to
> > num_poc_st_curr_before.
> > > > >
> > > > > > - * - __u8
> > > > > > - - ``num_rps_poc_st_curr_after``
> > > > > > - - The number of reference pictures in the short-term set that
> > come
> > > > after
> > > > > > - the current frame.
> > > > >
> > > > > Ditto.
> > > > >
> > > > > > - * - __u8
> > > > > > - - ``num_rps_poc_lt_curr``
> > > > > > - - The number of reference pictures in the long-term set.
> > > > >
> > > > > Ditto.
> > > > >
> > > > > Also, I'd like the changes that move fields from
> > > > V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS
> > > > > to the new V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS control, to be in
> > their
> > > > > patch.
> > > > >
> > > > > That will allow us to put in the commit description a proper
> > > > > explanation of why are fields being moved. Nothing fancy, simply
> > > > > explaining that these variables come from section 8.3.2
> > > > > "Decoding process for reference picture set", which describes
> > > > > a process invoked once per picture, so they are not per-slice.
> > > > >
> > > > > > - * - __u8
> > > > > > - - ``padding[7]``
> > > > > > + - ``padding``
> > > > > > - Applications and drivers must set this to zero.
> > > > > > * - struct :c:type:`v4l2_hevc_dpb_entry`
> > > > > > - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > > > @@ -3646,3 +3671,74 @@ enum
> > > > > > v4l2_mpeg_video_hevc_size_of_length_field -
> > > > > > so this has to come from client.
> > > > > > This is applicable to H264 and valid Range is from 0 to 63.
> > > > > > Source Rec. ITU-T H.264 (06/2019); G.7.4.1.1, G.8.8.1.
> > > > > > +
> > > > > > +``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (struct)``
> > > > > > + Specifies various decode parameters, especially the references
> > picture
> > > > order
> > > > > > + count (POC) for all the lists (short, long, before, current,
> > after)
> > > > and the
> > > > > > + number of entries for each of them.
> > > > > > + These parameters are defined according to :ref:`hevc`.
> > > > > > + They are described in section 8.3 "Slice decoding process" of
> > > > > > the
> > > > > > + specification.
> > > > > > +
> > > > > > +.. c:type:: v4l2_ctrl_hevc_decode_params
> > > > > > +
> > > > > > +.. cssclass:: longtable
> > > > > > +
> > > > > > +.. flat-table:: struct v4l2_ctrl_hevc_decode_params
> > > > > > + :header-rows: 0
> > > > > > + :stub-columns: 0
> > > > > > + :widths: 1 1 2
> > > > > > +
> > > > > > + * - __s32
> > > > > > + - ``pic_order_cnt_val``
> > > > > > + -
> > > > >
> > > > > Can be documented as:
> > > > >
> > > > > """
> > > > > PicOrderCntVal as described in section 8.3.1 "Decoding process
> > > > > for picture order count" of the specification.
> > > > > """
> > > > >
> > > > > Note that snake case is used to match the kernel style,
> > > > > but other than that we try to keep the HEVC spec variable
> > > > > names.
> > > > >
> > > > > > + * - __u8
> > > > > > + - ``num_active_dpb_entries``
> > > > > > + - The number of entries in ``dpb``.
> > > > > > + * - struct :c:type:`v4l2_hevc_dpb_entry`
> > > > > > + - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > > > + - The decoded picture buffer, for meta-data about reference
> > frames.
> > > > >
> > > > > The DPB is here, but it seems it's also in the slice control?
> > > > >
> > > > > > + * - __u8
> > > > > > + - ``num_rps_poc_st_curr_before``
> > > > > > + - The number of reference pictures in the short-term set that
> > come
> > > > before
> > > > > > + the current frame.
> > > > > > + * - __u8
> > > > > > + - ``num_rps_poc_st_curr_after``
> > > > > > + - The number of reference pictures in the short-term set that
> > come
> > > > after
> > > > > > + the current frame.
> > > > > > + * - __u8
> > > > > > + - ``num_rps_poc_lt_curr``
> > > > > > + - The number of reference pictures in the long-term set.
> > > > > > + * - __u8
> > > > > > + - ``rps_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > > > + -
> > > > > > + * - __u8
> > > > > > + - ``rps_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > > > + -
> > > > > > + * - __u8
> > > > > > + - ``rps_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > > > > > + -
> > > > >
> > > > > Could you document these as well?
> > > > >
> > > > > Thanks a lot,
> > > > > Ezequiel
> > > > >
> > > > >
> > > >
> > > >
> > >
> > >
> > >
> >
>