[PATCH 0/7] media: Clean H264 stateless uAPI

[PATCH 0/7] media: Clean H264 stateless uAPI

[Ezequiel Garcia]

The recent patch posted by Jernej (which I'm including for context),
encouraged me to address all the known issues in the uAPI.

I hope we can finally make this uAPI interface
public; and then also address the other codec
interfaces so we can move the codec drivers out of staging.

It should be noted that there is already GStreamer native
support for this interface, which will be part of 1.18,
once it's released [1], as well as support in Chromium [2].

I have pushed a branch porting GStreamer to
support these interface changes:

https://gitlab.freedesktop.org/ezequielgarcia/gst-plugins-bad/-/commits/for_h264_uapi_v2

The main changes are:

* Split prediction weight table to a separate control.
* Increase size of first_mb_in_slice field.
* Cleanup DPB entry interface, to support field coding.
* Increase of DPB entry pic_num field.
* Move slice invariant fields to the per-frame control.

I'm adding here the change from Jernej, and a change from
Philipp Zabel which apparently fell thru the cracks.

Ezequiel Garcia (8):
  media: uapi: h264: Further clarify scaling lists order
  media: uapi: h264: Split prediction weight parameters
  media: uapi: h264: Increase size of 'first_mb_in_slice' field
  media: uapi: h264: Cleanup DPB entry interface
  media: uapi: h264: Increase size of DPB entry pic_num
  media: uapi: h264: Clean slice invariants syntax elements
  media: hantro: Don't require unneeded H264_SLICE_PARAMS
  media: rkvdec: Don't require unneeded H264_SLICE_PARAMS

Jernej Skrabec (1):
  media: uapi: h264: Update reference lists

Philipp Zabel (1):
  media: uapi: h264: Clarify pic_order_cnt_bit_size field

 .../media/v4l/ext-ctrls-codec.rst             | 192 +++++++++++-------
 drivers/media/v4l2-core/v4l2-ctrls.c          |  25 +++
 drivers/media/v4l2-core/v4l2-h264.c           |  12 +-
 drivers/staging/media/hantro/hantro_drv.c     |   5 -
 .../staging/media/hantro/hantro_g1_h264_dec.c |  21 +-
 drivers/staging/media/hantro/hantro_h264.c    |   8 +-
 drivers/staging/media/hantro/hantro_hw.h      |   2 -
 drivers/staging/media/rkvdec/rkvdec-h264.c    |  18 +-
 drivers/staging/media/rkvdec/rkvdec.c         |   5 -
 drivers/staging/media/sunxi/cedrus/cedrus.h   |   1 +
 .../staging/media/sunxi/cedrus/cedrus_dec.c   |   2 +
 .../staging/media/sunxi/cedrus/cedrus_h264.c  |  21 +-
 include/media/h264-ctrls.h                    |  80 +++++---
 include/media/v4l2-h264.h                     |   3 +-
 14 files changed, 234 insertions(+), 161 deletions(-)

-- 
2.27.0

[PATCH 01/10] media: uapi: h264: Update reference lists

[Ezequiel Garcia]

From: Jernej Skrabec <jernej.skrabec@siol.net>

When dealing with with interlaced frames, reference lists must tell if
each particular reference is meant for top or bottom field. This info
is currently not provided at all in the H264 related controls.

Make reference lists hold a structure which will also hold flags along
index into DPB array. Flags will tell if reference is meant for top or
bottom field.

Currently the only user of these lists is Cedrus which is just compile
fixed here. Actual usage of newly introduced flags will come in
following commit.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
[Ezequiel: Use a macro for the reference picture list length]
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
 .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
 include/media/h264-ctrls.h                    | 20 +++++++---
 3 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index d0d506a444b1..c2e17c02f77e 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1843,10 +1843,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u32
       - ``slice_group_change_cycle``
       -
-    * - __u8
+    * - struct :c:type:`v4l2_h264_reference`
       - ``ref_pic_list0[32]``
       - Reference picture list after applying the per-slice modifications
-    * - __u8
+    * - struct :c:type:`v4l2_h264_reference`
       - ``ref_pic_list1[32]``
       - Reference picture list after applying the per-slice modifications
     * - __u32
@@ -1926,6 +1926,42 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - ``chroma_offset[32][2]``
       -
 
+``Picture Reference``
+
+.. c:type:: v4l2_h264_reference
+
+.. cssclass:: longtable
+
+.. flat-table:: struct v4l2_h264_reference
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - __u16
+      - ``flags``
+      - See :ref:`Picture Reference Flags <h264_reference_flags>`
+    * - __u8
+      - ``index``
+      - Index into the :c:type:`v4l2_ctrl_h264_decode_params`.dpb array.
+
+.. _h264_reference_flags:
+
+``Picture Reference Flags``
+
+.. cssclass:: longtable
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - ``V4L2_H264_REFERENCE_FLAG_TOP_FIELD``
+      - 0x00000001
+      -
+    * - ``V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD``
+      - 0x00000002
+      -
+
 ``V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS (struct)``
     Specifies the decode parameters (as extracted from the bitstream)
     for the associated H264 slice data. This includes the necessary
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index 54ee2aa423e2..cce527bbdf86 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -166,8 +166,8 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
 
 static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
 				   struct cedrus_run *run,
-				   const u8 *ref_list, u8 num_ref,
-				   enum cedrus_h264_sram_off sram)
+				   const struct v4l2_h264_reference *ref_list,
+				   u8 num_ref, enum cedrus_h264_sram_off sram)
 {
 	const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params;
 	struct vb2_queue *cap_q;
@@ -188,7 +188,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
 		int buf_idx;
 		u8 dpb_idx;
 
-		dpb_idx = ref_list[i];
+		dpb_idx = ref_list[i].index;
 		dpb = &decode->dpb[dpb_idx];
 
 		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index 080fd1293c42..bca4a9887e7e 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -19,6 +19,8 @@
  */
 #define V4L2_H264_NUM_DPB_ENTRIES 16
 
+#define V4L2_H264_REF_LIST_LEN (2 * V4L2_H264_NUM_DPB_ENTRIES)
+
 /* Our pixel format isn't stable at the moment */
 #define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */
 
@@ -140,6 +142,16 @@ struct v4l2_h264_pred_weight_table {
 #define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x04
 #define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x08
 
+#define V4L2_H264_REFERENCE_FLAG_TOP_FIELD		0x01
+#define V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD		0x02
+
+struct v4l2_h264_reference {
+	__u8 flags;
+
+	/* Index into v4l2_ctrl_h264_decode_params.dpb[] */
+	__u8 index;
+};
+
 struct v4l2_ctrl_h264_slice_params {
 	/* Size in bytes, including header */
 	__u32 size;
@@ -178,12 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
 	__u8 num_ref_idx_l1_active_minus1;
 	__u32 slice_group_change_cycle;
 
-	/*
-	 * Entries on each list are indices into
-	 * v4l2_ctrl_h264_decode_params.dpb[].
-	 */
-	__u8 ref_pic_list0[32];
-	__u8 ref_pic_list1[32];
+	struct v4l2_h264_reference ref_pic_list0[V4L2_H264_REF_LIST_LEN];
+	struct v4l2_h264_reference ref_pic_list1[V4L2_H264_REF_LIST_LEN];
 
 	__u32 flags;
 };
-- 
2.27.0

[PATCH 02/10] media: uapi: h264: Further clarify scaling lists order

[Ezequiel Garcia]

Commit 0b0393d59eb4a ("media: uapi: h264: clarify
expected scaling_list_4x4/8x8 order") improved the
documentation on H264 scaling lists order.

This commit improves the documentation by clarifying
that the lists themselves are expected in matrix order.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index c2e17c02f77e..16bfc98ab2f6 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1725,12 +1725,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - ``scaling_list_4x4[6][16]``
       - Scaling matrix after applying the inverse scanning process.
         Expected list order is Intra Y, Intra Cb, Intra Cr, Inter Y,
-        Inter Cb, Inter Cr.
+        Inter Cb, Inter Cr. The values on each scaling list are
+        expected in matrix order.
     * - __u8
       - ``scaling_list_8x8[6][64]``
       - Scaling matrix after applying the inverse scanning process.
         Expected list order is Intra Y, Inter Y, Intra Cb, Inter Cb,
-        Intra Cr, Inter Cr.
+        Intra Cr, Inter Cr. The values on each scaling list are
+        expected in matrix order.
 
 ``V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS (struct)``
     Specifies the slice parameters (as extracted from the bitstream)
-- 
2.27.0

[PATCH 03/10] media: uapi: h264: Split prediction weight parameters

[Ezequiel Garcia]

The prediction weight parameters are only required under
certain conditions, which depend on slice header parameters.

The slice header syntax specifies that the prediction
weight table is present if:

((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \
(weighted_bipred_idc == 1 && slice_type == B))

Given its size, it makes sense to move this table to its control,
so applications can avoid passing it if the slice doesn't specify it.

Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes.
With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight
is 772 bytes.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 14 +++++++++-----
 drivers/media/v4l2-core/v4l2-ctrls.c               |  6 ++++++
 drivers/staging/media/sunxi/cedrus/cedrus.h        |  1 +
 drivers/staging/media/sunxi/cedrus/cedrus_dec.c    |  2 ++
 drivers/staging/media/sunxi/cedrus/cedrus_h264.c   |  6 ++----
 include/media/h264-ctrls.h                         |  5 +++--
 6 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 16bfc98ab2f6..d1695ae96700 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1879,18 +1879,22 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - 0x00000008
       -
 
-``Prediction Weight Table``
-
-    The bitstream parameters are defined according to :ref:`h264`,
+``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT (struct)``
+    Prediction weight table defined according to :ref:`h264`,
     section 7.4.3.2 "Prediction Weight Table Semantics". For further
     documentation, refer to the above specification, unless there is
     an explicit comment stating otherwise.
 
-.. c:type:: v4l2_h264_pred_weight_table
+    .. note::
+
+       This compound control is not yet part of the public kernel API and
+       it is expected to change.
+
+.. c:type:: v4l2_ctrl_h264_pred_weight
 
 .. cssclass:: longtable
 
-.. flat-table:: struct v4l2_h264_pred_weight_table
+.. flat-table:: struct v4l2_ctrl_h264_pred_weight
     :header-rows:  0
     :stub-columns: 0
     :widths:       1 1 2
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 3f3fbcd60cc6..3a8a23e34c70 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1412,6 +1412,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS:
 		*type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS;
 		break;
+	case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT:
+		*type = V4L2_CTRL_TYPE_H264_PRED_WEIGHT;
+		break;
 	case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
 		*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
 		break;
@@ -2553,6 +2556,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
 		elem_size = sizeof(struct v4l2_ctrl_h264_decode_params);
 		break;
+	case V4L2_CTRL_TYPE_H264_PRED_WEIGHT:
+		elem_size = sizeof(struct v4l2_ctrl_h264_pred_weight);
+		break;
 	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
 		elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
 		break;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 96765555ab8a..80a0ecffa384 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -62,6 +62,7 @@ struct cedrus_h264_run {
 	const struct v4l2_ctrl_h264_scaling_matrix	*scaling_matrix;
 	const struct v4l2_ctrl_h264_slice_params	*slice_params;
 	const struct v4l2_ctrl_h264_sps			*sps;
+	const struct v4l2_ctrl_h264_pred_weight		*pred_weight;
 };
 
 struct cedrus_mpeg2_run {
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index 58c48e4fdfe9..047f773f64f9 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -57,6 +57,8 @@ void cedrus_device_run(void *priv)
 			V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS);
 		run.h264.sps = cedrus_find_control_data(ctx,
 			V4L2_CID_MPEG_VIDEO_H264_SPS);
+		run.h264.pred_weight = cedrus_find_control_data(ctx,
+			V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT);
 		break;
 
 	case V4L2_PIX_FMT_HEVC_SLICE:
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index cce527bbdf86..614b1b496e40 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -256,10 +256,8 @@ static void cedrus_write_scaling_lists(struct cedrus_ctx *ctx,
 static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx,
 					   struct cedrus_run *run)
 {
-	const struct v4l2_ctrl_h264_slice_params *slice =
-		run->h264.slice_params;
-	const struct v4l2_h264_pred_weight_table *pred_weight =
-		&slice->pred_weight_table;
+	const struct v4l2_ctrl_h264_pred_weight *pred_weight =
+		run->h264.pred_weight;
 	struct cedrus_dev *dev = ctx->dev;
 	int i, j, k;
 
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index bca4a9887e7e..da2ffb77b491 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -36,6 +36,7 @@
 #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS	(V4L2_CID_MPEG_BASE+1004)
 #define V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE	(V4L2_CID_MPEG_BASE+1005)
 #define V4L2_CID_MPEG_VIDEO_H264_START_CODE	(V4L2_CID_MPEG_BASE+1006)
+#define V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT	(V4L2_CID_MPEG_BASE+1007)
 
 /* enum v4l2_ctrl_type type values */
 #define V4L2_CTRL_TYPE_H264_SPS			0x0110
@@ -43,6 +44,7 @@
 #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX	0x0112
 #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS	0x0113
 #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS	0x0114
+#define V4L2_CTRL_TYPE_H264_PRED_WEIGHT		0x0115
 
 enum v4l2_mpeg_video_h264_decode_mode {
 	V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED,
@@ -125,7 +127,7 @@ struct v4l2_h264_weight_factors {
 	__s16 chroma_offset[32][2];
 };
 
-struct v4l2_h264_pred_weight_table {
+struct v4l2_ctrl_h264_pred_weight {
 	__u16 luma_log2_weight_denom;
 	__u16 chroma_log2_weight_denom;
 	struct v4l2_h264_weight_factors weight_factors[2];
@@ -174,7 +176,6 @@ struct v4l2_ctrl_h264_slice_params {
 	__s32 delta_pic_order_cnt0;
 	__s32 delta_pic_order_cnt1;
 
-	struct v4l2_h264_pred_weight_table pred_weight_table;
 	/* Size in bits of dec_ref_pic_marking() syntax element. */
 	__u32 dec_ref_pic_marking_bit_size;
 	/* Size in bits of pic order count syntax. */
-- 
2.27.0

[PATCH 04/10] media: uapi: h264: Clarify pic_order_cnt_bit_size field

[Ezequiel Garcia]

From: Philipp Zabel <p.zabel@pengutronix.de>

Since pic_order_cnt_bit_size is not a syntax element itself, explicitly
state that it is the total size in bits of the pic_order_cnt_lsb,
delta_pic_order_cnt_bottom, delta_pic_order_cnt[0], and
delta_pic_order_cnt[1] syntax elements contained in the slice.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
[Ezequiel: rebase]
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index d1695ae96700..86e30b80a6c1 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1815,7 +1815,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - Size in bits of the dec_ref_pic_marking() syntax element.
     * - __u32
       - ``pic_order_cnt_bit_size``
-      -
+      - Combined size in bits of the picture order count related syntax
+        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
+        delta_pic_order_cnt0, and delta_pic_order_cnt1.
     * - __u8
       - ``cabac_init_idc``
       -
-- 
2.27.0

[PATCH 05/10] media: uapi: h264: Increase size of 'first_mb_in_slice' field

[Ezequiel Garcia]

Slice header syntax element 'first_mb_in_slice' can point
to the last macroblock, currently the field can only reference
65536 macroblocks which is insufficient for 8K videos.

Although unlikely, a 8192x4320 video (where macroblocks are 16x16),
would contain 138240 macroblocks on a frame.

As per the H264 specification, 'first_mb_in_slice' can be up to
PicSizeInMbs - 1, so increase the size of the field to 32-bits.

Note that v4l2_ctrl_h264_slice_params struct will be modified
in a follow-up commit, and so we defer its 64-bit padding.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 2 +-
 include/media/h264-ctrls.h                                | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 86e30b80a6c1..dd8e5a2e8986 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1774,7 +1774,7 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u32
       - ``header_bit_size``
       -
-    * - __u16
+    * - __u32
       - ``first_mb_in_slice``
       -
     * - __u8
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index da2ffb77b491..620ee8863d74 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -164,7 +164,8 @@ struct v4l2_ctrl_h264_slice_params {
 	/* Offset in bits to slice_data() from the beginning of this slice. */
 	__u32 header_bit_size;
 
-	__u16 first_mb_in_slice;
+	__u32 first_mb_in_slice;
+
 	__u8 slice_type;
 	__u8 pic_parameter_set_id;
 	__u8 colour_plane_id;
-- 
2.27.0

[PATCH 06/10] media: uapi: h264: Cleanup DPB entry interface

[Ezequiel Garcia]

As discussed recently, the current interface for the
Decoded Picture Buffer is not enough to properly
support field coding.

This commit introduces enough semantics to support
frame and field coding, and to signal how DPB entries
are "used for reference".

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../media/v4l/ext-ctrls-codec.rst             | 46 ++++++++++++-------
 drivers/media/v4l2-core/v4l2-h264.c           |  4 +-
 drivers/staging/media/rkvdec/rkvdec-h264.c    |  8 ++--
 include/media/h264-ctrls.h                    |  8 +++-
 4 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index dd8e5a2e8986..46d4c8c6ad47 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __s32
       - ``bottom_field_order_cnt``
       -
+    * - enum :c:type:`v4l2_h264_dpb_reference`
+      - ``reference``
+      - Specifies how the DPB entry is referenced.
     * - __u32
       - ``flags``
       - See :ref:`DPB Entry Flags <h264_dpb_flags>`
 
+.. c:type:: v4l2_h264_dpb_reference
+
+.. cssclass:: longtable
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - ``V4L2_H264_DPB_TOP_REF``
+      - 0x1
+      - The top field in field pair is used for
+        short-term reference.
+    * - ``V4L2_H264_DPB_BOTTOM_REF``
+      - 0x2
+      - The bottom field in field pair is used for
+        short-term reference.
+    * - ``V4L2_H264_DPB_FRAME_REF``
+      - 0x3
+      - The frame (or the top/bottom fields, if it's a field pair)
+        is used for short-term reference.
+
 .. _h264_dpb_flags:
 
 ``DPB Entries Flags``
@@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
 
     * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID``
       - 0x00000001
-      - The DPB entry is valid and should be considered
+      - The DPB entry is valid (non-empty) and should be considered.
     * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE``
       - 0x00000002
-      - The DPB entry is currently being used as a reference frame
+      - The DPB entry is used for reference.
     * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
       - 0x00000004
-      - The DPB entry is a long term reference frame
+      - The DPB entry is used for long-term reference.
     * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD``
       - 0x00000008
-      - The DPB entry is a field reference, which means only one of the field
-        will be used when decoding the new frame/field. When not set the DPB
-        entry is a frame reference (both fields will be used). Note that this
-        flag does not say anything about the number of fields contained in the
-        reference frame, it just describes the one used to decode the new
-        field/frame
-    * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD``
-      - 0x00000010
-      - The DPB entry is a bottom field reference (only the bottom field of the
-        reference frame is needed to decode the new frame/field). Only valid if
-        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When
-        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but
-        V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the
-        DPB entry is a top field reference
+      - The DPB entry is a single field or a complementary field pair.
 
 ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
     Specifies the decoding mode to use. Currently exposes slice-based and
diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
index edf6225f0522..306a51683606 100644
--- a/drivers/media/v4l2-core/v4l2-h264.c
+++ b/drivers/media/v4l2-core/v4l2-h264.c
@@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 		else
 			b->refs[i].frame_num = dpb[i].frame_num;
 
-		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
+		if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF)
 			pic_order_count = min(dpb[i].top_field_order_cnt,
 					      dpb[i].bottom_field_order_cnt);
-		else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
+		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
 			pic_order_count = dpb[i].bottom_field_order_cnt;
 		else
 			pic_order_count = dpb[i].top_field_order_cnt;
diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 7b66e2743a4f..57539c630422 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx,
 			     RKVDEC_COLMV_USED_FLAG_REF;
 
 		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
-			refer_addr |= RKVDEC_TOPFIELD_USED_REF |
-				      RKVDEC_BOTFIELD_USED_REF;
-		else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
+			refer_addr |= RKVDEC_FIELD_REF;
+
+		if (dpb[i].reference & V4L2_H264_DPB_TOP_REF)
 			refer_addr |= RKVDEC_BOTFIELD_USED_REF;
-		else
+		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
 			refer_addr |= RKVDEC_TOPFIELD_USED_REF;
 
 		writel_relaxed(dpb[i].top_field_order_cnt,
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index 620ee8863d74..52f3976b986c 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -202,7 +202,12 @@ struct v4l2_ctrl_h264_slice_params {
 #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE		0x02
 #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
 #define V4L2_H264_DPB_ENTRY_FLAG_FIELD		0x08
-#define V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD	0x10
+
+enum v4l2_h264_dpb_reference {
+	V4L2_H264_DPB_TOP_REF = 0x1,
+	V4L2_H264_DPB_BOTTOM_REF = 0x2,
+	V4L2_H264_DPB_FRAME_REF = 0x3,
+};
 
 struct v4l2_h264_dpb_entry {
 	__u64 reference_ts;
@@ -211,6 +216,7 @@ struct v4l2_h264_dpb_entry {
 	/* Note that field is indicated by v4l2_buffer.field */
 	__s32 top_field_order_cnt;
 	__s32 bottom_field_order_cnt;
+	enum v4l2_h264_dpb_reference reference;
 	__u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
 };
 
-- 
2.27.0

[PATCH 07/10] media: uapi: h264: Increase size of DPB entry pic_num

[Ezequiel Garcia]

DPB entry PicNum maximum value is 2*MaxFrameNum for interlaced
content (field_pic_flag=1).

As specified, MaxFrameNum is 2^(log2_max_frame_num_minus4 + 4)
and log2_max_frame_num_minus4 is in the range of 0 to 12,
which means pic_num should be a 32-bit field.

The v4l2_h264_dpb_entry struct needs to be padded to avoid a hole,
which might be also useful to allow future uAPI extensions.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../userspace-api/media/v4l/ext-ctrls-codec.rst     |  5 ++++-
 drivers/media/v4l2-core/v4l2-ctrls.c                | 13 +++++++++++++
 include/media/h264-ctrls.h                          |  3 ++-
 include/media/v4l2-h264.h                           |  2 +-
 4 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 46d4c8c6ad47..fc8ca4f8be25 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -2049,7 +2049,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u16
       - ``frame_num``
       -
-    * - __u16
+    * - __u8
+      - ``reserved[2]``
+      - Applications and drivers must set this to zero.
+    * - __u32
       - ``pic_num``
       -
     * - __s32
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 3a8a23e34c70..0c13a7e0e63c 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1724,6 +1724,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
 
 #define zero_padding(s) \
 	memset(&(s).padding, 0, sizeof((s).padding))
+#define zero_reserved(s) \
+	memset(&(s).reserved, 0, sizeof((s).reserved))
 
 /*
  * Compound controls validation requires setting unused fields/flags to zero
@@ -1734,6 +1736,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 {
 	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+	struct v4l2_ctrl_h264_decode_params *p_h264_dec_params;
 	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;
@@ -1794,7 +1797,17 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	case V4L2_CTRL_TYPE_H264_PPS:
 	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
 	case V4L2_CTRL_TYPE_H264_SLICE_PARAMS:
+		break;
+
 	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
+		p_h264_dec_params = p;
+
+		for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
+			struct v4l2_h264_dpb_entry *dpb_entry =
+				&p_h264_dec_params->dpb[i];
+
+			zero_reserved(*dpb_entry);
+		}
 		break;
 
 	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index 52f3976b986c..f90fe96f0a59 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -212,7 +212,8 @@ enum v4l2_h264_dpb_reference {
 struct v4l2_h264_dpb_entry {
 	__u64 reference_ts;
 	__u16 frame_num;
-	__u16 pic_num;
+	__u8 reserved[2];
+	__u32 pic_num;
 	/* Note that field is indicated by v4l2_buffer.field */
 	__s32 top_field_order_cnt;
 	__s32 bottom_field_order_cnt;
diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
index bc9ebb560ccf..1a5f26fc2a9a 100644
--- a/include/media/v4l2-h264.h
+++ b/include/media/v4l2-h264.h
@@ -33,7 +33,7 @@ struct v4l2_h264_reflist_builder {
 	struct {
 		s32 pic_order_count;
 		int frame_num;
-		u16 pic_num;
+		u32 pic_num;
 		u16 longterm : 1;
 	} refs[V4L2_H264_NUM_DPB_ENTRIES];
 	s32 cur_pic_order_count;
-- 
2.27.0

[PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Ezequiel Garcia]

The H.264 specification requires in its "Slice header semantics"
section that the following values shall be the same in all slice headers:

  pic_parameter_set_id
  frame_num
  field_pic_flag
  bottom_field_flag
  idr_pic_id
  pic_order_cnt_lsb
  delta_pic_order_cnt_bottom
  delta_pic_order_cnt[ 0 ]
  delta_pic_order_cnt[ 1 ]
  sp_for_switch_flag
  slice_group_change_cycle

and can therefore be moved to the per-frame decode parameters control.

Field 'pic_parameter_set_id' is simply removed in this case,
because the PPS control must currently contain the active PPS.

Syntax elements dec_ref_pic_marking() and those related
to pic order count, remain invariant as well, and therefore,
the fields dec_ref_pic_marking_bit_size and pic_order_cnt_bit_size
are also common to all slices.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 .../media/v4l/ext-ctrls-codec.rst             | 83 +++++++++----------
 drivers/media/v4l2-core/v4l2-ctrls.c          |  6 ++
 drivers/media/v4l2-core/v4l2-h264.c           |  8 +-
 .../staging/media/hantro/hantro_g1_h264_dec.c | 21 +++--
 drivers/staging/media/hantro/hantro_h264.c    |  3 +-
 drivers/staging/media/rkvdec/rkvdec-h264.c    |  6 +-
 .../staging/media/sunxi/cedrus/cedrus_h264.c  |  9 +-
 include/media/h264-ctrls.h                    | 41 ++++-----
 include/media/v4l2-h264.h                     |  1 -
 9 files changed, 88 insertions(+), 90 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index fc8ca4f8be25..e3c9bfaeff6f 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1780,44 +1780,12 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u8
       - ``slice_type``
       -
-    * - __u8
-      - ``pic_parameter_set_id``
-      -
     * - __u8
       - ``colour_plane_id``
       -
     * - __u8
       - ``redundant_pic_cnt``
       -
-    * - __u16
-      - ``frame_num``
-      -
-    * - __u16
-      - ``idr_pic_id``
-      -
-    * - __u16
-      - ``pic_order_cnt_lsb``
-      -
-    * - __s32
-      - ``delta_pic_order_cnt_bottom``
-      -
-    * - __s32
-      - ``delta_pic_order_cnt0``
-      -
-    * - __s32
-      - ``delta_pic_order_cnt1``
-      -
-    * - struct :c:type:`v4l2_h264_pred_weight_table`
-      - ``pred_weight_table``
-      -
-    * - __u32
-      - ``dec_ref_pic_marking_bit_size``
-      - Size in bits of the dec_ref_pic_marking() syntax element.
-    * - __u32
-      - ``pic_order_cnt_bit_size``
-      - Combined size in bits of the picture order count related syntax
-        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
-        delta_pic_order_cnt0, and delta_pic_order_cnt1.
     * - __u8
       - ``cabac_init_idc``
       -
@@ -1844,9 +1812,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - ``num_ref_idx_l1_active_minus1``
       - If num_ref_idx_active_override_flag is not set, this field must be
         set to the value of num_ref_idx_l1_default_active_minus1.
-    * - __u32
-      - ``slice_group_change_cycle``
-      -
+    * - __u8
+      - ``reserved``
+      - Applications and drivers must set this to zero.
     * - struct :c:type:`v4l2_h264_reference`
       - ``ref_pic_list0[32]``
       - Reference picture list after applying the per-slice modifications
@@ -1868,17 +1836,11 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     :stub-columns: 0
     :widths:       1 1 2
 
-    * - ``V4L2_H264_SLICE_FLAG_FIELD_PIC``
-      - 0x00000001
-      -
-    * - ``V4L2_H264_SLICE_FLAG_BOTTOM_FIELD``
-      - 0x00000002
-      -
     * - ``V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED``
-      - 0x00000004
+      - 0x00000001
       -
     * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH``
-      - 0x00000008
+      - 0x00000002
       -
 
 ``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT (struct)``
@@ -2011,6 +1973,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __s32
       - ``bottom_field_order_cnt``
       - Picture Order Count for the coded bottom field
+    * - __u16
+      - ``frame_num``
+      -
+    * - __u16
+      - ``idr_pic_id``
+      -
+    * - __u16
+      - ``pic_order_cnt_lsb``
+      -
+    * - __s32
+      - ``delta_pic_order_cnt_bottom``
+      -
+    * - __s32
+      - ``delta_pic_order_cnt0``
+      -
+    * - __s32
+      - ``delta_pic_order_cnt1``
+      -
+    * - __u32
+      - ``dec_ref_pic_marking_bit_size``
+      - Size in bits of the dec_ref_pic_marking() syntax element.
+    * - __u32
+      - ``pic_order_cnt_bit_size``
+      - Combined size in bits of the picture order count related syntax
+        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
+        delta_pic_order_cnt0, and delta_pic_order_cnt1.
+    * - __u32
+      - ``slice_group_change_cycle``
+      -
     * - __u32
       - ``flags``
       - See :ref:`Decode Parameters Flags <h264_decode_params_flags>`
@@ -2029,6 +2020,12 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - ``V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC``
       - 0x00000001
       - That picture is an IDR picture
+    * - ``V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC``
+      - 0x00000002
+      -
+    * - ``V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD``
+      - 0x00000004
+      -
 
 .. c:type:: v4l2_h264_dpb_entry
 
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 0c13a7e0e63c..c6d82534f1fa 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1736,6 +1736,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 {
 	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
 	struct v4l2_ctrl_h264_decode_params *p_h264_dec_params;
 	struct v4l2_ctrl_hevc_sps *p_hevc_sps;
 	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
@@ -1796,7 +1797,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	case V4L2_CTRL_TYPE_H264_SPS:
 	case V4L2_CTRL_TYPE_H264_PPS:
 	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
+		break;
+
 	case V4L2_CTRL_TYPE_H264_SLICE_PARAMS:
+		p_h264_slice_params = p;
+
+		zero_reserved(*p_h264_slice_params);
 		break;
 
 	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
index 306a51683606..1a9dcbbba06c 100644
--- a/drivers/media/v4l2-core/v4l2-h264.c
+++ b/drivers/media/v4l2-core/v4l2-h264.c
@@ -18,14 +18,12 @@
  *
  * @b: the builder context to initialize
  * @dec_params: decode parameters control
- * @slice_params: first slice parameters control
  * @sps: SPS control
  * @dpb: DPB to use when creating the reference list
  */
 void
 v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 		const struct v4l2_ctrl_h264_decode_params *dec_params,
-		const struct v4l2_ctrl_h264_slice_params *slice_params,
 		const struct v4l2_ctrl_h264_sps *sps,
 		const struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES])
 {
@@ -33,13 +31,13 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 	unsigned int i;
 
 	max_frame_num = 1 << (sps->log2_max_frame_num_minus4 + 4);
-	cur_frame_num = slice_params->frame_num;
+	cur_frame_num = dec_params->frame_num;
 
 	memset(b, 0, sizeof(*b));
-	if (!(slice_params->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC))
+	if (!(dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC))
 		b->cur_pic_order_count = min(dec_params->bottom_field_order_cnt,
 					     dec_params->top_field_order_cnt);
-	else if (slice_params->flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
+	else if (dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
 		b->cur_pic_order_count = dec_params->bottom_field_order_cnt;
 	else
 		b->cur_pic_order_count = dec_params->top_field_order_cnt;
diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index 424c648ce9fc..f9839e9c6da5 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -23,7 +23,6 @@ static void set_params(struct hantro_ctx *ctx)
 {
 	const struct hantro_h264_dec_ctrls *ctrls = &ctx->h264_dec.ctrls;
 	const struct v4l2_ctrl_h264_decode_params *dec_param = ctrls->decode;
-	const struct v4l2_ctrl_h264_slice_params *slices = ctrls->slices;
 	const struct v4l2_ctrl_h264_sps *sps = ctrls->sps;
 	const struct v4l2_ctrl_h264_pps *pps = ctrls->pps;
 	struct vb2_v4l2_buffer *src_buf = hantro_get_src_buf(ctx);
@@ -42,11 +41,11 @@ static void set_params(struct hantro_ctx *ctx)
 
 	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
 	    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD ||
-	     slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC))
+	     dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC))
 		reg |= G1_REG_DEC_CTRL0_PIC_INTERLACE_E;
-	if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
+	if (dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
 		reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
-	if (!(slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD))
+	if (!(dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD))
 		reg |= G1_REG_DEC_CTRL0_PIC_TOPFIELD_E;
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
 
@@ -75,7 +74,7 @@ static void set_params(struct hantro_ctx *ctx)
 
 	/* Decoder control register 4. */
 	reg = G1_REG_DEC_CTRL4_FRAMENUM_LEN(sps->log2_max_frame_num_minus4 + 4) |
-	      G1_REG_DEC_CTRL4_FRAMENUM(slices[0].frame_num) |
+	      G1_REG_DEC_CTRL4_FRAMENUM(dec_param->frame_num) |
 	      G1_REG_DEC_CTRL4_WEIGHT_BIPR_IDC(pps->weighted_bipred_idc);
 	if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE)
 		reg |= G1_REG_DEC_CTRL4_CABAC_E;
@@ -88,8 +87,8 @@ static void set_params(struct hantro_ctx *ctx)
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL4);
 
 	/* Decoder control register 5. */
-	reg = G1_REG_DEC_CTRL5_REFPIC_MK_LEN(slices[0].dec_ref_pic_marking_bit_size) |
-	      G1_REG_DEC_CTRL5_IDR_PIC_ID(slices[0].idr_pic_id);
+	reg = G1_REG_DEC_CTRL5_REFPIC_MK_LEN(dec_param->dec_ref_pic_marking_bit_size) |
+	      G1_REG_DEC_CTRL5_IDR_PIC_ID(dec_param->idr_pic_id);
 	if (pps->flags & V4L2_H264_PPS_FLAG_CONSTRAINED_INTRA_PRED)
 		reg |= G1_REG_DEC_CTRL5_CONST_INTRA_E;
 	if (pps->flags & V4L2_H264_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT)
@@ -103,10 +102,10 @@ static void set_params(struct hantro_ctx *ctx)
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL5);
 
 	/* Decoder control register 6. */
-	reg = G1_REG_DEC_CTRL6_PPS_ID(slices[0].pic_parameter_set_id) |
+	reg = G1_REG_DEC_CTRL6_PPS_ID(pps->pic_parameter_set_id) |
 	      G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(pps->num_ref_idx_l0_default_active_minus1 + 1) |
 	      G1_REG_DEC_CTRL6_REFIDX1_ACTIVE(pps->num_ref_idx_l1_default_active_minus1 + 1) |
-	      G1_REG_DEC_CTRL6_POC_LENGTH(slices[0].pic_order_cnt_bit_size);
+	      G1_REG_DEC_CTRL6_POC_LENGTH(dec_param->pic_order_cnt_bit_size);
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL6);
 
 	/* Error concealment register. */
@@ -246,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx)
 	/* Destination (decoded frame) buffer. */
 	dst_dma = hantro_get_dec_buf_addr(ctx, &dst_buf->vb2_buf);
 	/* Adjust dma addr to start at second line for bottom field */
-	if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
+	if (ctrls->decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
 		offset = ALIGN(ctx->src_fmt.width, MB_DIM);
 	vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DST);
 
@@ -265,7 +264,7 @@ static void set_buffers(struct hantro_ctx *ctx)
 		 * DMV buffer is split in two for field encoded frames,
 		 * adjust offset for bottom field
 		 */
-		if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
+		if (ctrls->decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
 			offset += 32 * MB_WIDTH(ctx->src_fmt.width) *
 				  MB_HEIGHT(ctx->src_fmt.height);
 		vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DIR_MV);
diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 194d05848077..0cbe514dc79a 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -372,8 +372,7 @@ int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
 
 	/* Build the P/B{0,1} ref lists. */
 	v4l2_h264_init_reflist_builder(&reflist_builder, ctrls->decode,
-				       &ctrls->slices[0], ctrls->sps,
-				       ctx->h264_dec.dpb);
+				       ctrls->sps, ctx->h264_dec.dpb);
 	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
 				    h264_ctx->reflists.b1);
diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 57539c630422..57c084910b3b 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -730,7 +730,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 			    struct rkvdec_h264_run *run)
 {
 	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
-	const struct v4l2_ctrl_h264_slice_params *sl_params = &run->slices_params[0];
 	const struct v4l2_h264_dpb_entry *dpb = dec_params->dpb;
 	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
 	const struct v4l2_ctrl_h264_sps *sps = run->sps;
@@ -754,7 +753,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 			continue;
 
 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
-		    dpb[i].frame_num < sl_params->frame_num) {
+		    dpb[i].frame_num < dec_params->frame_num) {
 			p[i] = dpb[i].frame_num;
 			continue;
 		}
@@ -1093,8 +1092,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 
 	/* Build the P/B{0,1} ref lists. */
 	v4l2_h264_init_reflist_builder(&reflist_builder, run.decode_params,
-				       &run.slices_params[0], run.sps,
-				       run.decode_params->dpb);
+				       run.sps, run.decode_params->dpb);
 	h264_ctx->reflists.num_valid = reflist_builder.num_valid;
 	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index 614b1b496e40..2a00b2175ca1 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -95,7 +95,6 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
 {
 	struct cedrus_h264_sram_ref_pic pic_list[CEDRUS_H264_FRAME_NUM];
 	const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params;
-	const struct v4l2_ctrl_h264_slice_params *slice = run->h264.slice_params;
 	const struct v4l2_ctrl_h264_sps *sps = run->h264.sps;
 	struct vb2_queue *cap_q;
 	struct cedrus_buffer *output_buf;
@@ -144,7 +143,7 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
 	output_buf = vb2_to_cedrus_buffer(&run->dst->vb2_buf);
 	output_buf->codec.h264.position = position;
 
-	if (slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
+	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
 		output_buf->codec.h264.pic_type = CEDRUS_H264_PIC_TYPE_FIELD;
 	else if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
 		output_buf->codec.h264.pic_type = CEDRUS_H264_PIC_TYPE_MBAFF;
@@ -412,7 +411,7 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
 		reg |= VE_H264_SPS_DIRECT_8X8_INFERENCE;
 	cedrus_write(dev, VE_H264_SPS, reg);
 
-	mbaff_pic = !(slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC) &&
+	mbaff_pic = !(decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) &&
 		    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD);
 	pic_width_in_mbs = sps->pic_width_in_mbs_minus1 + 1;
 
@@ -426,9 +425,9 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
 	reg |= slice->cabac_init_idc & 0x3;
 	if (ctx->fh.m2m_ctx->new_frame)
 		reg |= VE_H264_SHS_FIRST_SLICE_IN_PIC;
-	if (slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
+	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
 		reg |= VE_H264_SHS_FIELD_PIC;
-	if (slice->flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
+	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
 		reg |= VE_H264_SHS_BOTTOM_FIELD;
 	if (slice->flags & V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED)
 		reg |= VE_H264_SHS_DIRECT_SPATIAL_MV_PRED;
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index f90fe96f0a59..521ffd8f7b34 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -139,10 +139,8 @@ struct v4l2_ctrl_h264_pred_weight {
 #define V4L2_H264_SLICE_TYPE_SP				3
 #define V4L2_H264_SLICE_TYPE_SI				4
 
-#define V4L2_H264_SLICE_FLAG_FIELD_PIC			0x01
-#define V4L2_H264_SLICE_FLAG_BOTTOM_FIELD		0x02
-#define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x04
-#define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x08
+#define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x01
+#define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x02
 
 #define V4L2_H264_REFERENCE_FLAG_TOP_FIELD		0x01
 #define V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD		0x02
@@ -167,21 +165,8 @@ struct v4l2_ctrl_h264_slice_params {
 	__u32 first_mb_in_slice;
 
 	__u8 slice_type;
-	__u8 pic_parameter_set_id;
 	__u8 colour_plane_id;
 	__u8 redundant_pic_cnt;
-	__u16 frame_num;
-	__u16 idr_pic_id;
-	__u16 pic_order_cnt_lsb;
-	__s32 delta_pic_order_cnt_bottom;
-	__s32 delta_pic_order_cnt0;
-	__s32 delta_pic_order_cnt1;
-
-	/* Size in bits of dec_ref_pic_marking() syntax element. */
-	__u32 dec_ref_pic_marking_bit_size;
-	/* Size in bits of pic order count syntax. */
-	__u32 pic_order_cnt_bit_size;
-
 	__u8 cabac_init_idc;
 	__s8 slice_qp_delta;
 	__s8 slice_qs_delta;
@@ -190,7 +175,8 @@ struct v4l2_ctrl_h264_slice_params {
 	__s8 slice_beta_offset_div2;
 	__u8 num_ref_idx_l0_active_minus1;
 	__u8 num_ref_idx_l1_active_minus1;
-	__u32 slice_group_change_cycle;
+
+	__u8 reserved;
 
 	struct v4l2_h264_reference ref_pic_list0[V4L2_H264_REF_LIST_LEN];
 	struct v4l2_h264_reference ref_pic_list1[V4L2_H264_REF_LIST_LEN];
@@ -221,7 +207,9 @@ struct v4l2_h264_dpb_entry {
 	__u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
 };
 
-#define V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC	0x01
+#define V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC		0x01
+#define V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC		0x02
+#define V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD	0x04
 
 struct v4l2_ctrl_h264_decode_params {
 	struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES];
@@ -229,6 +217,21 @@ struct v4l2_ctrl_h264_decode_params {
 	__u16 nal_ref_idc;
 	__s32 top_field_order_cnt;
 	__s32 bottom_field_order_cnt;
+
+	__u16 frame_num;
+	__u16 idr_pic_id;
+	__u16 reserved;
+
+	__u16 pic_order_cnt_lsb;
+	__s32 delta_pic_order_cnt_bottom;
+	__s32 delta_pic_order_cnt0;
+	__s32 delta_pic_order_cnt1;
+	/* Size in bits of dec_ref_pic_marking() syntax element. */
+	__u32 dec_ref_pic_marking_bit_size;
+	/* Size in bits of pic order count syntax. */
+	__u32 pic_order_cnt_bit_size;
+	__u32 slice_group_change_cycle;
+
 	__u32 flags; /* V4L2_H264_DECODE_PARAM_FLAG_* */
 };
 
diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
index 1a5f26fc2a9a..f08ba181263d 100644
--- a/include/media/v4l2-h264.h
+++ b/include/media/v4l2-h264.h
@@ -44,7 +44,6 @@ struct v4l2_h264_reflist_builder {
 void
 v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 		const struct v4l2_ctrl_h264_decode_params *dec_params,
-		const struct v4l2_ctrl_h264_slice_params *slice_params,
 		const struct v4l2_ctrl_h264_sps *sps,
 		const struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES]);
 
-- 
2.27.0

[PATCH 09/10] media: hantro: Don't require unneeded H264_SLICE_PARAMS

[Ezequiel Garcia]

Now that slice invariant parameters have been moved,
the driver no longer needs this control, so drop it.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/hantro/hantro_drv.c  | 5 -----
 drivers/staging/media/hantro/hantro_h264.c | 5 -----
 drivers/staging/media/hantro/hantro_hw.h   | 2 --
 3 files changed, 12 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 34797507f214..3cd00cc0a364 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -306,11 +306,6 @@ static const struct hantro_ctrl controls[] = {
 		.cfg = {
 			.id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
 		},
-	}, {
-		.codec = HANTRO_H264_DECODER,
-		.cfg = {
-			.id = V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
-		},
 	}, {
 		.codec = HANTRO_H264_DECODER,
 		.cfg = {
diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 0cbe514dc79a..6887318ed4d8 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -349,11 +349,6 @@ int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
 	if (WARN_ON(!ctrls->decode))
 		return -EINVAL;
 
-	ctrls->slices =
-		hantro_get_ctrl(ctx, V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS);
-	if (WARN_ON(!ctrls->slices))
-		return -EINVAL;
-
 	ctrls->sps =
 		hantro_get_ctrl(ctx, V4L2_CID_MPEG_VIDEO_H264_SPS);
 	if (WARN_ON(!ctrls->sps))
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index f066de6b592d..219283a06f52 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -56,14 +56,12 @@ struct hantro_jpeg_enc_hw_ctx {
  * struct hantro_h264_dec_ctrls
  * @decode:	Decode params
  * @scaling:	Scaling info
- * @slice:	Slice params
  * @sps:	SPS info
  * @pps:	PPS info
  */
 struct hantro_h264_dec_ctrls {
 	const struct v4l2_ctrl_h264_decode_params *decode;
 	const struct v4l2_ctrl_h264_scaling_matrix *scaling;
-	const struct v4l2_ctrl_h264_slice_params *slices;
 	const struct v4l2_ctrl_h264_sps *sps;
 	const struct v4l2_ctrl_h264_pps *pps;
 };
-- 
2.27.0

[PATCH 10/10] media: rkvdec: Don't require unneeded H264_SLICE_PARAMS

[Ezequiel Garcia]

Now that slice invariant parameters have been moved,
the driver no longer needs this control, so drop it.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 4 ----
 drivers/staging/media/rkvdec/rkvdec.c      | 5 -----
 2 files changed, 9 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 57c084910b3b..f6e1fa19d625 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -109,7 +109,6 @@ struct rkvdec_h264_reflists {
 struct rkvdec_h264_run {
 	struct rkvdec_run base;
 	const struct v4l2_ctrl_h264_decode_params *decode_params;
-	const struct v4l2_ctrl_h264_slice_params *slices_params;
 	const struct v4l2_ctrl_h264_sps *sps;
 	const struct v4l2_ctrl_h264_pps *pps;
 	const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
@@ -1067,9 +1066,6 @@ static void rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
 	run->decode_params = ctrl ? ctrl->p_cur.p : NULL;
 	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
 			      V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS);
-	run->slices_params = ctrl ? ctrl->p_cur.p : NULL;
-	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
-			      V4L2_CID_MPEG_VIDEO_H264_SPS);
 	run->sps = ctrl ? ctrl->p_cur.p : NULL;
 	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
 			      V4L2_CID_MPEG_VIDEO_H264_PPS);
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index accb4a902fdd..8ebc9dfc83be 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -59,11 +59,6 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
 		.mandatory = true,
 		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
 	},
-	{
-		.per_request = true,
-		.mandatory = true,
-		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
-	},
 	{
 		.per_request = true,
 		.mandatory = true,
-- 
2.27.0

Re: [PATCH 02/10] media: uapi: h264: Further clarify scaling lists order

[Hans Verkuil]

On 15/07/2020 22:22, Ezequiel Garcia wrote:
> Commit 0b0393d59eb4a ("media: uapi: h264: clarify
> expected scaling_list_4x4/8x8 order") improved the
> documentation on H264 scaling lists order.
> 
> This commit improves the documentation by clarifying
> that the lists themselves are expected in matrix order.

"matrix order" is not a well defined term. Especially since different
programming languages lay out matrices differently (e.g. fortran uses
column-major order). Perhaps something like this is more unambiguous:

"The values on each scaling list are in row-major order."

BTW, why not be explicit and use:

__u8 scaling_list_4x4[6][4][4];
__u8 scaling_list_8x8[6][8][8];

That makes it explicit and the order is just that of what the C language
uses.

Regards,

	Hans

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index c2e17c02f77e..16bfc98ab2f6 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1725,12 +1725,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        - ``scaling_list_4x4[6][16]``
>        - Scaling matrix after applying the inverse scanning process.
>          Expected list order is Intra Y, Intra Cb, Intra Cr, Inter Y,
> -        Inter Cb, Inter Cr.
> +        Inter Cb, Inter Cr. The values on each scaling list are
> +        expected in matrix order.
>      * - __u8
>        - ``scaling_list_8x8[6][64]``
>        - Scaling matrix after applying the inverse scanning process.
>          Expected list order is Intra Y, Inter Y, Intra Cb, Inter Cb,
> -        Intra Cr, Inter Cr.
> +        Intra Cr, Inter Cr. The values on each scaling list are
> +        expected in matrix order.
>  
>  ``V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS (struct)``
>      Specifies the slice parameters (as extracted from the bitstream)
> 

Re: [PATCH 03/10] media: uapi: h264: Split prediction weight parameters

[Hans Verkuil]

On 15/07/2020 22:22, Ezequiel Garcia wrote:
> The prediction weight parameters are only required under
> certain conditions, which depend on slice header parameters.
> 
> The slice header syntax specifies that the prediction
> weight table is present if:
> 
> ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \
> (weighted_bipred_idc == 1 && slice_type == B))
> 
> Given its size, it makes sense to move this table to its control,
> so applications can avoid passing it if the slice doesn't specify it.
> 
> Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes.
> With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight
> is 772 bytes.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 14 +++++++++-----
>  drivers/media/v4l2-core/v4l2-ctrls.c               |  6 ++++++
>  drivers/staging/media/sunxi/cedrus/cedrus.h        |  1 +
>  drivers/staging/media/sunxi/cedrus/cedrus_dec.c    |  2 ++
>  drivers/staging/media/sunxi/cedrus/cedrus_h264.c   |  6 ++----
>  include/media/h264-ctrls.h                         |  5 +++--
>  6 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 16bfc98ab2f6..d1695ae96700 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1879,18 +1879,22 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        - 0x00000008
>        -
>  
> -``Prediction Weight Table``
> -
> -    The bitstream parameters are defined according to :ref:`h264`,
> +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT (struct)``
> +    Prediction weight table defined according to :ref:`h264`,
>      section 7.4.3.2 "Prediction Weight Table Semantics". For further
>      documentation, refer to the above specification, unless there is
>      an explicit comment stating otherwise.
>  
> -.. c:type:: v4l2_h264_pred_weight_table
> +    .. note::
> +
> +       This compound control is not yet part of the public kernel API and
> +       it is expected to change.
> +
> +.. c:type:: v4l2_ctrl_h264_pred_weight
>  
>  .. cssclass:: longtable
>  
> -.. flat-table:: struct v4l2_h264_pred_weight_table
> +.. flat-table:: struct v4l2_ctrl_h264_pred_weight
>      :header-rows:  0
>      :stub-columns: 0
>      :widths:       1 1 2
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 3f3fbcd60cc6..3a8a23e34c70 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1412,6 +1412,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS:
>  		*type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS;
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT:
> +		*type = V4L2_CTRL_TYPE_H264_PRED_WEIGHT;
> +		break;
>  	case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
>  		*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
>  		break;
> @@ -2553,6 +2556,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
>  		elem_size = sizeof(struct v4l2_ctrl_h264_decode_params);
>  		break;
> +	case V4L2_CTRL_TYPE_H264_PRED_WEIGHT:
> +		elem_size = sizeof(struct v4l2_ctrl_h264_pred_weight);
> +		break;
>  	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
>  		elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
>  		break;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 96765555ab8a..80a0ecffa384 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -62,6 +62,7 @@ struct cedrus_h264_run {
>  	const struct v4l2_ctrl_h264_scaling_matrix	*scaling_matrix;
>  	const struct v4l2_ctrl_h264_slice_params	*slice_params;
>  	const struct v4l2_ctrl_h264_sps			*sps;
> +	const struct v4l2_ctrl_h264_pred_weight		*pred_weight;
>  };
>  
>  struct cedrus_mpeg2_run {
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> index 58c48e4fdfe9..047f773f64f9 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> @@ -57,6 +57,8 @@ void cedrus_device_run(void *priv)
>  			V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS);
>  		run.h264.sps = cedrus_find_control_data(ctx,
>  			V4L2_CID_MPEG_VIDEO_H264_SPS);
> +		run.h264.pred_weight = cedrus_find_control_data(ctx,
> +			V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT);
>  		break;
>  
>  	case V4L2_PIX_FMT_HEVC_SLICE:
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index cce527bbdf86..614b1b496e40 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -256,10 +256,8 @@ static void cedrus_write_scaling_lists(struct cedrus_ctx *ctx,
>  static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx,
>  					   struct cedrus_run *run)
>  {
> -	const struct v4l2_ctrl_h264_slice_params *slice =
> -		run->h264.slice_params;
> -	const struct v4l2_h264_pred_weight_table *pred_weight =
> -		&slice->pred_weight_table;
> +	const struct v4l2_ctrl_h264_pred_weight *pred_weight =
> +		run->h264.pred_weight;
>  	struct cedrus_dev *dev = ctx->dev;
>  	int i, j, k;
>  
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index bca4a9887e7e..da2ffb77b491 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -36,6 +36,7 @@
>  #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS	(V4L2_CID_MPEG_BASE+1004)
>  #define V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE	(V4L2_CID_MPEG_BASE+1005)
>  #define V4L2_CID_MPEG_VIDEO_H264_START_CODE	(V4L2_CID_MPEG_BASE+1006)
> +#define V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT	(V4L2_CID_MPEG_BASE+1007)
>  
>  /* enum v4l2_ctrl_type type values */
>  #define V4L2_CTRL_TYPE_H264_SPS			0x0110
> @@ -43,6 +44,7 @@
>  #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX	0x0112
>  #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS	0x0113
>  #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS	0x0114
> +#define V4L2_CTRL_TYPE_H264_PRED_WEIGHT		0x0115
>  
>  enum v4l2_mpeg_video_h264_decode_mode {
>  	V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED,
> @@ -125,7 +127,7 @@ struct v4l2_h264_weight_factors {
>  	__s16 chroma_offset[32][2];
>  };
>  
> -struct v4l2_h264_pred_weight_table {
> +struct v4l2_ctrl_h264_pred_weight {
>  	__u16 luma_log2_weight_denom;
>  	__u16 chroma_log2_weight_denom;
>  	struct v4l2_h264_weight_factors weight_factors[2];
> @@ -174,7 +176,6 @@ struct v4l2_ctrl_h264_slice_params {
>  	__s32 delta_pic_order_cnt0;
>  	__s32 delta_pic_order_cnt1;
>  
> -	struct v4l2_h264_pred_weight_table pred_weight_table;
>  	/* Size in bits of dec_ref_pic_marking() syntax element. */
>  	__u32 dec_ref_pic_marking_bit_size;
>  	/* Size in bits of pic order count syntax. */
> 

Shouldn't this be added to union v4l2_ctrl_ptr as well?

Regards,

	Hans

Re: [PATCH 03/10] media: uapi: h264: Split prediction weight parameters

[Ezequiel Garcia]

On Thu, 2020-07-16 at 09:26 +0200, Hans Verkuil wrote:
> On 15/07/2020 22:22, Ezequiel Garcia wrote:
> > The prediction weight parameters are only required under
> > certain conditions, which depend on slice header parameters.
> > 
> > The slice header syntax specifies that the prediction
> > weight table is present if:
> > 
> > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \
> > (weighted_bipred_idc == 1 && slice_type == B))
> > 
> > Given its size, it makes sense to move this table to its control,
> > so applications can avoid passing it if the slice doesn't specify it.
> > 
> > Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes.
> > With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight
> > is 772 bytes.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 14 +++++++++-----
> >  drivers/media/v4l2-core/v4l2-ctrls.c               |  6 ++++++
> >  drivers/staging/media/sunxi/cedrus/cedrus.h        |  1 +
> >  drivers/staging/media/sunxi/cedrus/cedrus_dec.c    |  2 ++
> >  drivers/staging/media/sunxi/cedrus/cedrus_h264.c   |  6 ++----
> >  include/media/h264-ctrls.h                         |  5 +++--
> >  6 files changed, 23 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 16bfc98ab2f6..d1695ae96700 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -1879,18 +1879,22 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >        - 0x00000008
> >        -
> >  
> > -``Prediction Weight Table``
> > -
> > -    The bitstream parameters are defined according to :ref:`h264`,
> > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT (struct)``
> > +    Prediction weight table defined according to :ref:`h264`,
> >      section 7.4.3.2 "Prediction Weight Table Semantics". For further
> >      documentation, refer to the above specification, unless there is
> >      an explicit comment stating otherwise.
> >  
> > -.. c:type:: v4l2_h264_pred_weight_table
> > +    .. note::
> > +
> > +       This compound control is not yet part of the public kernel API and
> > +       it is expected to change.
> > +
> > +.. c:type:: v4l2_ctrl_h264_pred_weight
> >  
> >  .. cssclass:: longtable
> >  
> > -.. flat-table:: struct v4l2_h264_pred_weight_table
> > +.. flat-table:: struct v4l2_ctrl_h264_pred_weight
> >      :header-rows:  0
> >      :stub-columns: 0
> >      :widths:       1 1 2
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index 3f3fbcd60cc6..3a8a23e34c70 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -1412,6 +1412,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >  	case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS:
> >  		*type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS;
> >  		break;
> > +	case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT:
> > +		*type = V4L2_CTRL_TYPE_H264_PRED_WEIGHT;
> > +		break;
> >  	case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
> >  		*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
> >  		break;
> > @@ -2553,6 +2556,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >  	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
> >  		elem_size = sizeof(struct v4l2_ctrl_h264_decode_params);
> >  		break;
> > +	case V4L2_CTRL_TYPE_H264_PRED_WEIGHT:
> > +		elem_size = sizeof(struct v4l2_ctrl_h264_pred_weight);
> > +		break;
> >  	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
> >  		elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
> >  		break;
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > index 96765555ab8a..80a0ecffa384 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > @@ -62,6 +62,7 @@ struct cedrus_h264_run {
> >  	const struct v4l2_ctrl_h264_scaling_matrix	*scaling_matrix;
> >  	const struct v4l2_ctrl_h264_slice_params	*slice_params;
> >  	const struct v4l2_ctrl_h264_sps			*sps;
> > +	const struct v4l2_ctrl_h264_pred_weight		*pred_weight;
> >  };
> >  
> >  struct cedrus_mpeg2_run {
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > index 58c48e4fdfe9..047f773f64f9 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > @@ -57,6 +57,8 @@ void cedrus_device_run(void *priv)
> >  			V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS);
> >  		run.h264.sps = cedrus_find_control_data(ctx,
> >  			V4L2_CID_MPEG_VIDEO_H264_SPS);
> > +		run.h264.pred_weight = cedrus_find_control_data(ctx,
> > +			V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT);
> >  		break;
> >  
> >  	case V4L2_PIX_FMT_HEVC_SLICE:
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > index cce527bbdf86..614b1b496e40 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > @@ -256,10 +256,8 @@ static void cedrus_write_scaling_lists(struct cedrus_ctx *ctx,
> >  static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx,
> >  					   struct cedrus_run *run)
> >  {
> > -	const struct v4l2_ctrl_h264_slice_params *slice =
> > -		run->h264.slice_params;
> > -	const struct v4l2_h264_pred_weight_table *pred_weight =
> > -		&slice->pred_weight_table;
> > +	const struct v4l2_ctrl_h264_pred_weight *pred_weight =
> > +		run->h264.pred_weight;
> >  	struct cedrus_dev *dev = ctx->dev;
> >  	int i, j, k;
> >  
> > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> > index bca4a9887e7e..da2ffb77b491 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/media/h264-ctrls.h
> > @@ -36,6 +36,7 @@
> >  #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS	(V4L2_CID_MPEG_BASE+1004)
> >  #define V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE	(V4L2_CID_MPEG_BASE+1005)
> >  #define V4L2_CID_MPEG_VIDEO_H264_START_CODE	(V4L2_CID_MPEG_BASE+1006)
> > +#define V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT	(V4L2_CID_MPEG_BASE+1007)
> >  
> >  /* enum v4l2_ctrl_type type values */
> >  #define V4L2_CTRL_TYPE_H264_SPS			0x0110
> > @@ -43,6 +44,7 @@
> >  #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX	0x0112
> >  #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS	0x0113
> >  #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS	0x0114
> > +#define V4L2_CTRL_TYPE_H264_PRED_WEIGHT		0x0115
> >  
> >  enum v4l2_mpeg_video_h264_decode_mode {
> >  	V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED,
> > @@ -125,7 +127,7 @@ struct v4l2_h264_weight_factors {
> >  	__s16 chroma_offset[32][2];
> >  };
> >  
> > -struct v4l2_h264_pred_weight_table {
> > +struct v4l2_ctrl_h264_pred_weight {
> >  	__u16 luma_log2_weight_denom;
> >  	__u16 chroma_log2_weight_denom;
> >  	struct v4l2_h264_weight_factors weight_factors[2];
> > @@ -174,7 +176,6 @@ struct v4l2_ctrl_h264_slice_params {
> >  	__s32 delta_pic_order_cnt0;
> >  	__s32 delta_pic_order_cnt1;
> >  
> > -	struct v4l2_h264_pred_weight_table pred_weight_table;
> >  	/* Size in bits of dec_ref_pic_marking() syntax element. */
> >  	__u32 dec_ref_pic_marking_bit_size;
> >  	/* Size in bits of pic order count syntax. */
> > 
> 
> Shouldn't this be added to union v4l2_ctrl_ptr as well?
> 

Yup, that's correct.

Thanks,
Ezequiel

Re: [PATCH 02/10] media: uapi: h264: Further clarify scaling lists order

[Ezequiel Garcia]

On Thu, 2020-07-16 at 09:23 +0200, Hans Verkuil wrote:
> On 15/07/2020 22:22, Ezequiel Garcia wrote:
> > Commit 0b0393d59eb4a ("media: uapi: h264: clarify
> > expected scaling_list_4x4/8x8 order") improved the
> > documentation on H264 scaling lists order.
> > 
> > This commit improves the documentation by clarifying
> > that the lists themselves are expected in matrix order.
> 
> "matrix order" is not a well defined term. Especially since different
> programming languages lay out matrices differently (e.g. fortran uses
> column-major order). Perhaps something like this is more unambiguous:
> 

Agreed, "matrix order" is perhaps not a proper choice of words.

> "The values on each scaling list are in row-major order."
> 
> BTW, why not be explicit and use:
> 
> __u8 scaling_list_4x4[6][4][4];
> __u8 scaling_list_8x8[6][8][8];
> 
> That makes it explicit and the order is just that of what the C language
> uses.
> 

I am not sure if that'll go in clearer direction.

I'm thinking we just need to clarify the coefficients
are in raster scan order, as opposed to a zig-zag scan
order, which is a part of the decoding process.

How about we simply say "raster scan order" and keep the patch as is?

Thanks,
Ezequiel


> Regards,
> 
> 	Hans
> 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index c2e17c02f77e..16bfc98ab2f6 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -1725,12 +1725,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >        - ``scaling_list_4x4[6][16]``
> >        - Scaling matrix after applying the inverse scanning process.
> >          Expected list order is Intra Y, Intra Cb, Intra Cr, Inter Y,
> > -        Inter Cb, Inter Cr.
> > +        Inter Cb, Inter Cr. The values on each scaling list are
> > +        expected in matrix order.
> >      * - __u8
> >        - ``scaling_list_8x8[6][64]``
> >        - Scaling matrix after applying the inverse scanning process.
> >          Expected list order is Intra Y, Inter Y, Intra Cb, Inter Cb,
> > -        Intra Cr, Inter Cr.
> > +        Intra Cr, Inter Cr. The values on each scaling list are
> > +        expected in matrix order.
> >  
> >  ``V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS (struct)``
> >      Specifies the slice parameters (as extracted from the bitstream)
> > 

Re: [PATCH 02/10] media: uapi: h264: Further clarify scaling lists order

[Hans Verkuil]

On 16/07/2020 13:43, Ezequiel Garcia wrote:
> On Thu, 2020-07-16 at 09:23 +0200, Hans Verkuil wrote:
>> On 15/07/2020 22:22, Ezequiel Garcia wrote:
>>> Commit 0b0393d59eb4a ("media: uapi: h264: clarify
>>> expected scaling_list_4x4/8x8 order") improved the
>>> documentation on H264 scaling lists order.
>>>
>>> This commit improves the documentation by clarifying
>>> that the lists themselves are expected in matrix order.
>>
>> "matrix order" is not a well defined term. Especially since different
>> programming languages lay out matrices differently (e.g. fortran uses
>> column-major order). Perhaps something like this is more unambiguous:
>>
> 
> Agreed, "matrix order" is perhaps not a proper choice of words.
> 
>> "The values on each scaling list are in row-major order."
>>
>> BTW, why not be explicit and use:
>>
>> __u8 scaling_list_4x4[6][4][4];
>> __u8 scaling_list_8x8[6][8][8];
>>
>> That makes it explicit and the order is just that of what the C language
>> uses.
>>
> 
> I am not sure if that'll go in clearer direction.
> 
> I'm thinking we just need to clarify the coefficients
> are in raster scan order, as opposed to a zig-zag scan
> order, which is a part of the decoding process.
> 
> How about we simply say "raster scan order" and keep the patch as is?

That works for me.

Regards,

	Hans

> 
> Thanks,
> Ezequiel
> 
> 
>> Regards,
>>
>> 	Hans
>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> index c2e17c02f77e..16bfc98ab2f6 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -1725,12 +1725,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>        - ``scaling_list_4x4[6][16]``
>>>        - Scaling matrix after applying the inverse scanning process.
>>>          Expected list order is Intra Y, Intra Cb, Intra Cr, Inter Y,
>>> -        Inter Cb, Inter Cr.
>>> +        Inter Cb, Inter Cr. The values on each scaling list are
>>> +        expected in matrix order.
>>>      * - __u8
>>>        - ``scaling_list_8x8[6][64]``
>>>        - Scaling matrix after applying the inverse scanning process.
>>>          Expected list order is Intra Y, Inter Y, Intra Cb, Inter Cb,
>>> -        Intra Cr, Inter Cr.
>>> +        Intra Cr, Inter Cr. The values on each scaling list are
>>> +        expected in matrix order.
>>>  
>>>  ``V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS (struct)``
>>>      Specifies the slice parameters (as extracted from the bitstream)
>>>
> 
> 

Re: [PATCH 03/10] media: uapi: h264: Split prediction weight parameters

[Jernej Škrabec]

Hi!

Dne sreda, 15. julij 2020 ob 22:22:26 CEST je Ezequiel Garcia napisal(a):
> The prediction weight parameters are only required under
> certain conditions, which depend on slice header parameters.
> 
> The slice header syntax specifies that the prediction
> weight table is present if:
> 
> ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \
> (weighted_bipred_idc == 1 && slice_type == B))
> 
> Given its size, it makes sense to move this table to its control,
> so applications can avoid passing it if the slice doesn't specify it.
> 
> Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes.
> With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight
> is 772 bytes.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 14 +++++++++-----
>  drivers/media/v4l2-core/v4l2-ctrls.c               |  6 ++++++
>  drivers/staging/media/sunxi/cedrus/cedrus.h        |  1 +
>  drivers/staging/media/sunxi/cedrus/cedrus_dec.c    |  2 ++
>  drivers/staging/media/sunxi/cedrus/cedrus_h264.c   |  6 ++----
>  include/media/h264-ctrls.h                         |  5 +++--
>  6 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index
> 16bfc98ab2f6..d1695ae96700 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1879,18 +1879,22 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> - 0x00000008
>        -
> 
> -``Prediction Weight Table``
> -
> -    The bitstream parameters are defined according to :ref:`h264`,
> +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT (struct)``
> +    Prediction weight table defined according to :ref:`h264`,
>      section 7.4.3.2 "Prediction Weight Table Semantics". For further
>      documentation, refer to the above specification, unless there is
>      an explicit comment stating otherwise.
> 
> -.. c:type:: v4l2_h264_pred_weight_table
> +    .. note::
> +
> +       This compound control is not yet part of the public kernel API and
> +       it is expected to change.
> +
> +.. c:type:: v4l2_ctrl_h264_pred_weight
> 
>  .. cssclass:: longtable
> 
> -.. flat-table:: struct v4l2_h264_pred_weight_table
> +.. flat-table:: struct v4l2_ctrl_h264_pred_weight
> 
>      :header-rows:  0
>      :stub-columns: 0
>      :widths:       1 1 2
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c index 3f3fbcd60cc6..3a8a23e34c70
> 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1412,6 +1412,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type, case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS:
>  		*type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS;
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT:
> +		*type = V4L2_CTRL_TYPE_H264_PRED_WEIGHT;
> +		break;
>  	case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
>  		*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
>  		break;
> @@ -2553,6 +2556,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
> v4l2_ctrl_handler *hdl, case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
>  		elem_size = sizeof(struct 
v4l2_ctrl_h264_decode_params);
>  		break;
> +	case V4L2_CTRL_TYPE_H264_PRED_WEIGHT:
> +		elem_size = sizeof(struct v4l2_ctrl_h264_pred_weight);
> +		break;
>  	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
>  		elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
>  		break;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> 96765555ab8a..80a0ecffa384 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -62,6 +62,7 @@ struct cedrus_h264_run {
>  	const struct v4l2_ctrl_h264_scaling_matrix	
*scaling_matrix;
>  	const struct v4l2_ctrl_h264_slice_params	*slice_params;
>  	const struct v4l2_ctrl_h264_sps			*sps;
> +	const struct v4l2_ctrl_h264_pred_weight		
*pred_weight;
>  };
> 
>  struct cedrus_mpeg2_run {
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index
> 58c48e4fdfe9..047f773f64f9 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> @@ -57,6 +57,8 @@ void cedrus_device_run(void *priv)
>  			V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS);
>  		run.h264.sps = cedrus_find_control_data(ctx,
>  			V4L2_CID_MPEG_VIDEO_H264_SPS);
> +		run.h264.pred_weight = cedrus_find_control_data(ctx,
> +			V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT);
>  		break;
> 
>  	case V4L2_PIX_FMT_HEVC_SLICE:
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> cce527bbdf86..614b1b496e40 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -256,10 +256,8 @@ static void cedrus_write_scaling_lists(struct
> cedrus_ctx *ctx, static void cedrus_write_pred_weight_table(struct
> cedrus_ctx *ctx, struct cedrus_run *run)
>  {
> -	const struct v4l2_ctrl_h264_slice_params *slice =
> -		run->h264.slice_params;
> -	const struct v4l2_h264_pred_weight_table *pred_weight =
> -		&slice->pred_weight_table;
> +	const struct v4l2_ctrl_h264_pred_weight *pred_weight =
> +		run->h264.pred_weight;
>  	struct cedrus_dev *dev = ctx->dev;
>  	int i, j, k;
> 

You forgot to add new control to list of all supported controls in cedrus.c at 
the top of the file.

Best regards,
Jernej


> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index bca4a9887e7e..da2ffb77b491 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -36,6 +36,7 @@
>  #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS	(V4L2_CID_MPEG_BASE+1004)
>  #define V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE	(V4L2_CID_MPEG_BASE+1005)
>  #define V4L2_CID_MPEG_VIDEO_H264_START_CODE	
(V4L2_CID_MPEG_BASE+1006)
> +#define V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHT	(V4L2_CID_MPEG_BASE+1007)
> 
>  /* enum v4l2_ctrl_type type values */
>  #define V4L2_CTRL_TYPE_H264_SPS			0x0110
> @@ -43,6 +44,7 @@
>  #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX	0x0112
>  #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS	0x0113
>  #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS	0x0114
> +#define V4L2_CTRL_TYPE_H264_PRED_WEIGHT		0x0115
> 
>  enum v4l2_mpeg_video_h264_decode_mode {
>  	V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED,
> @@ -125,7 +127,7 @@ struct v4l2_h264_weight_factors {
>  	__s16 chroma_offset[32][2];
>  };
> 
> -struct v4l2_h264_pred_weight_table {
> +struct v4l2_ctrl_h264_pred_weight {
>  	__u16 luma_log2_weight_denom;
>  	__u16 chroma_log2_weight_denom;
>  	struct v4l2_h264_weight_factors weight_factors[2];
> @@ -174,7 +176,6 @@ struct v4l2_ctrl_h264_slice_params {
>  	__s32 delta_pic_order_cnt0;
>  	__s32 delta_pic_order_cnt1;
> 
> -	struct v4l2_h264_pred_weight_table pred_weight_table;
>  	/* Size in bits of dec_ref_pic_marking() syntax element. */
>  	__u32 dec_ref_pic_marking_bit_size;
>  	/* Size in bits of pic order count syntax. */

Re: [PATCH 06/10] media: uapi: h264: Cleanup DPB entry interface

[Jernej Škrabec]

Hi!

Dne sreda, 15. julij 2020 ob 22:22:29 CEST je Ezequiel Garcia napisal(a):
> As discussed recently, the current interface for the
> Decoded Picture Buffer is not enough to properly
> support field coding.
> 
> This commit introduces enough semantics to support
> frame and field coding, and to signal how DPB entries
> are "used for reference".
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 46 ++++++++++++-------
>  drivers/media/v4l2-core/v4l2-h264.c           |  4 +-
>  drivers/staging/media/rkvdec/rkvdec-h264.c    |  8 ++--
>  include/media/h264-ctrls.h                    |  8 +++-
>  4 files changed, 42 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index
> dd8e5a2e8986..46d4c8c6ad47 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> * - __s32
>        - ``bottom_field_order_cnt``
>        -
> +    * - enum :c:type:`v4l2_h264_dpb_reference`
> +      - ``reference``
> +      - Specifies how the DPB entry is referenced.
>      * - __u32
>        - ``flags``
>        - See :ref:`DPB Entry Flags <h264_dpb_flags>`
> 
> +.. c:type:: v4l2_h264_dpb_reference
> +
> +.. cssclass:: longtable
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - ``V4L2_H264_DPB_TOP_REF``
> +      - 0x1
> +      - The top field in field pair is used for
> +        short-term reference.
> +    * - ``V4L2_H264_DPB_BOTTOM_REF``
> +      - 0x2
> +      - The bottom field in field pair is used for
> +        short-term reference.
> +    * - ``V4L2_H264_DPB_FRAME_REF``
> +      - 0x3
> +      - The frame (or the top/bottom fields, if it's a field pair)
> +        is used for short-term reference.
> +
>  .. _h264_dpb_flags:
> 
>  ``DPB Entries Flags``
> @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> 
>      * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID``
>        - 0x00000001
> -      - The DPB entry is valid and should be considered
> +      - The DPB entry is valid (non-empty) and should be considered.
>      * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE``

I'm still not sure that we actually need both flags. Technically, if entry is 
not used for reference then doesn't need to be present. Am I missing 
something?

Best regards,
Jernej

>        - 0x00000002
> -      - The DPB entry is currently being used as a reference frame
> +      - The DPB entry is used for reference.
>      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
>        - 0x00000004
> -      - The DPB entry is a long term reference frame
> +      - The DPB entry is used for long-term reference.
>      * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD``
>        - 0x00000008
> -      - The DPB entry is a field reference, which means only one of the
> field -        will be used when decoding the new frame/field. When not set
> the DPB -        entry is a frame reference (both fields will be used).
> Note that this -        flag does not say anything about the number of
> fields contained in the -        reference frame, it just describes the one
> used to decode the new -        field/frame
> -    * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD``
> -      - 0x00000010
> -      - The DPB entry is a bottom field reference (only the bottom field of
> the -        reference frame is needed to decode the new frame/field). Only
> valid if -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When
> -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but
> -        V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the
> -        DPB entry is a top field reference
> +      - The DPB entry is a single field or a complementary field pair.
> 
>  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
>      Specifies the decoding mode to use. Currently exposes slice-based and
> diff --git a/drivers/media/v4l2-core/v4l2-h264.c
> b/drivers/media/v4l2-core/v4l2-h264.c index edf6225f0522..306a51683606
> 100644
> --- a/drivers/media/v4l2-core/v4l2-h264.c
> +++ b/drivers/media/v4l2-core/v4l2-h264.c
> @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct
> v4l2_h264_reflist_builder *b, else
>  			b->refs[i].frame_num = dpb[i].frame_num;
> 
> -		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
> +		if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF)
>  			pic_order_count = 
min(dpb[i].top_field_order_cnt,
>  					      
dpb[i].bottom_field_order_cnt);
> -		else if (dpb[i].flags & 
V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
> +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
>  			pic_order_count = 
dpb[i].bottom_field_order_cnt;
>  		else
>  			pic_order_count = dpb[i].top_field_order_cnt;
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c
> b/drivers/staging/media/rkvdec/rkvdec-h264.c index
> 7b66e2743a4f..57539c630422 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx,
>  			     RKVDEC_COLMV_USED_FLAG_REF;
> 
>  		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
> -			refer_addr |= RKVDEC_TOPFIELD_USED_REF |
> -				      RKVDEC_BOTFIELD_USED_REF;
> -		else if (dpb[i].flags & 
V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
> +			refer_addr |= RKVDEC_FIELD_REF;
> +
> +		if (dpb[i].reference & V4L2_H264_DPB_TOP_REF)
>  			refer_addr |= RKVDEC_BOTFIELD_USED_REF;
> -		else
> +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
>  			refer_addr |= RKVDEC_TOPFIELD_USED_REF;
> 
>  		writel_relaxed(dpb[i].top_field_order_cnt,
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index 620ee8863d74..52f3976b986c 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -202,7 +202,12 @@ struct v4l2_ctrl_h264_slice_params {
>  #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE		0x02
>  #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
>  #define V4L2_H264_DPB_ENTRY_FLAG_FIELD		0x08
> -#define V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD	0x10
> +
> +enum v4l2_h264_dpb_reference {
> +	V4L2_H264_DPB_TOP_REF = 0x1,
> +	V4L2_H264_DPB_BOTTOM_REF = 0x2,
> +	V4L2_H264_DPB_FRAME_REF = 0x3,
> +};
> 
>  struct v4l2_h264_dpb_entry {
>  	__u64 reference_ts;
> @@ -211,6 +216,7 @@ struct v4l2_h264_dpb_entry {
>  	/* Note that field is indicated by v4l2_buffer.field */
>  	__s32 top_field_order_cnt;
>  	__s32 bottom_field_order_cnt;
> +	enum v4l2_h264_dpb_reference reference;
>  	__u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
>  };

Re: [PATCH 06/10] media: uapi: h264: Cleanup DPB entry interface

[Ezequiel Garcia]

On Wed, 2020-07-22 at 18:09 +0200, Jernej Škrabec wrote:
> Hi!
> 
> Dne sreda, 15. julij 2020 ob 22:22:29 CEST je Ezequiel Garcia napisal(a):
> > As discussed recently, the current interface for the
> > Decoded Picture Buffer is not enough to properly
> > support field coding.
> > 
> > This commit introduces enough semantics to support
> > frame and field coding, and to signal how DPB entries
> > are "used for reference".
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  .../media/v4l/ext-ctrls-codec.rst             | 46 ++++++++++++-------
> >  drivers/media/v4l2-core/v4l2-h264.c           |  4 +-
> >  drivers/staging/media/rkvdec/rkvdec-h264.c    |  8 ++--
> >  include/media/h264-ctrls.h                    |  8 +++-
> >  4 files changed, 42 insertions(+), 24 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index
> > dd8e5a2e8986..46d4c8c6ad47 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > * - __s32
> >        - ``bottom_field_order_cnt``
> >        -
> > +    * - enum :c:type:`v4l2_h264_dpb_reference`
> > +      - ``reference``
> > +      - Specifies how the DPB entry is referenced.
> >      * - __u32
> >        - ``flags``
> >        - See :ref:`DPB Entry Flags <h264_dpb_flags>`
> > 
> > +.. c:type:: v4l2_h264_dpb_reference
> > +
> > +.. cssclass:: longtable
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +    :widths:       1 1 2
> > +
> > +    * - ``V4L2_H264_DPB_TOP_REF``
> > +      - 0x1
> > +      - The top field in field pair is used for
> > +        short-term reference.
> > +    * - ``V4L2_H264_DPB_BOTTOM_REF``
> > +      - 0x2
> > +      - The bottom field in field pair is used for
> > +        short-term reference.
> > +    * - ``V4L2_H264_DPB_FRAME_REF``
> > +      - 0x3
> > +      - The frame (or the top/bottom fields, if it's a field pair)
> > +        is used for short-term reference.
> > +
> >  .. _h264_dpb_flags:
> > 
> >  ``DPB Entries Flags``
> > @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > 
> >      * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID``
> >        - 0x00000001
> > -      - The DPB entry is valid and should be considered
> > +      - The DPB entry is valid (non-empty) and should be considered.
> >      * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE``
> 
> I'm still not sure that we actually need both flags. Technically, if entry is 
> not used for reference then doesn't need to be present. Am I missing 
> something?
> 

Indeed, that's exactly what I raised during the review of these flags.

However, note that the Cedrus driver seem to do something with
VALID but not ACTIVE DPB entries, although maybe that's not really needed.

If you could check the code and drop the VALID usage, and confirm
ACTIVE is the only flag needed, that would be nice.

Now, OTOH, having an extra flag is not hurting us.
It might be more consistent for applications, as it
would mean simpler setup of the DPB, and more consistent mapping
between application kept DPB and kernel DPB.

Thanks,
Ezequiel

> Best regards,
> Jernej
> 
> >        - 0x00000002
> > -      - The DPB entry is currently being used as a reference frame
> > +      - The DPB entry is used for reference.
> >      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
> >        - 0x00000004
> > -      - The DPB entry is a long term reference frame
> > +      - The DPB entry is used for long-term reference.
> >      * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD``
> >        - 0x00000008
> > -      - The DPB entry is a field reference, which means only one of the
> > field -        will be used when decoding the new frame/field. When not set
> > the DPB -        entry is a frame reference (both fields will be used).
> > Note that this -        flag does not say anything about the number of
> > fields contained in the -        reference frame, it just describes the one
> > used to decode the new -        field/frame
> > -    * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD``
> > -      - 0x00000010
> > -      - The DPB entry is a bottom field reference (only the bottom field of
> > the -        reference frame is needed to decode the new frame/field). Only
> > valid if -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When
> > -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but
> > -        V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the
> > -        DPB entry is a top field reference
> > +      - The DPB entry is a single field or a complementary field pair.
> > 
> >  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
> >      Specifies the decoding mode to use. Currently exposes slice-based and
> > diff --git a/drivers/media/v4l2-core/v4l2-h264.c
> > b/drivers/media/v4l2-core/v4l2-h264.c index edf6225f0522..306a51683606
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-h264.c
> > +++ b/drivers/media/v4l2-core/v4l2-h264.c
> > @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct
> > v4l2_h264_reflist_builder *b, else
> >  			b->refs[i].frame_num = dpb[i].frame_num;
> > 
> > -		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
> > +		if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF)
> >  			pic_order_count = 
> min(dpb[i].top_field_order_cnt,
> >  					      
> dpb[i].bottom_field_order_cnt);
> > -		else if (dpb[i].flags & 
> V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
> > +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
> >  			pic_order_count = 
> dpb[i].bottom_field_order_cnt;
> >  		else
> >  			pic_order_count = dpb[i].top_field_order_cnt;
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > b/drivers/staging/media/rkvdec/rkvdec-h264.c index
> > 7b66e2743a4f..57539c630422 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx,
> >  			     RKVDEC_COLMV_USED_FLAG_REF;
> > 
> >  		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
> > -			refer_addr |= RKVDEC_TOPFIELD_USED_REF |
> > -				      RKVDEC_BOTFIELD_USED_REF;
> > -		else if (dpb[i].flags & 
> V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
> > +			refer_addr |= RKVDEC_FIELD_REF;
> > +
> > +		if (dpb[i].reference & V4L2_H264_DPB_TOP_REF)
> >  			refer_addr |= RKVDEC_BOTFIELD_USED_REF;
> > -		else
> > +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
> >  			refer_addr |= RKVDEC_TOPFIELD_USED_REF;
> > 
> >  		writel_relaxed(dpb[i].top_field_order_cnt,
> > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> > index 620ee8863d74..52f3976b986c 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/media/h264-ctrls.h
> > @@ -202,7 +202,12 @@ struct v4l2_ctrl_h264_slice_params {
> >  #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE		0x02
> >  #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
> >  #define V4L2_H264_DPB_ENTRY_FLAG_FIELD		0x08
> > -#define V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD	0x10
> > +
> > +enum v4l2_h264_dpb_reference {
> > +	V4L2_H264_DPB_TOP_REF = 0x1,
> > +	V4L2_H264_DPB_BOTTOM_REF = 0x2,
> > +	V4L2_H264_DPB_FRAME_REF = 0x3,
> > +};
> > 
> >  struct v4l2_h264_dpb_entry {
> >  	__u64 reference_ts;
> > @@ -211,6 +216,7 @@ struct v4l2_h264_dpb_entry {
> >  	/* Note that field is indicated by v4l2_buffer.field */
> >  	__s32 top_field_order_cnt;
> >  	__s32 bottom_field_order_cnt;
> > +	enum v4l2_h264_dpb_reference reference;
> >  	__u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
> >  };
> 
> 
> 

Re: [PATCH 01/10] media: uapi: h264: Update reference lists

[Jonas Karlman]

On 2020-07-15 22:22, Ezequiel Garcia wrote:
> From: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> When dealing with with interlaced frames, reference lists must tell if
> each particular reference is meant for top or bottom field. This info
> is currently not provided at all in the H264 related controls.
> 
> Make reference lists hold a structure which will also hold flags along
> index into DPB array. Flags will tell if reference is meant for top or
> bottom field.
> 
> Currently the only user of these lists is Cedrus which is just compile
> fixed here. Actual usage of newly introduced flags will come in
> following commit.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> [Ezequiel: Use a macro for the reference picture list length]
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
>  include/media/h264-ctrls.h                    | 20 +++++++---
>  3 files changed, 55 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index d0d506a444b1..c2e17c02f77e 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1843,10 +1843,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u32
>        - ``slice_group_change_cycle``
>        -
> -    * - __u8
> +    * - struct :c:type:`v4l2_h264_reference`
>        - ``ref_pic_list0[32]``
>        - Reference picture list after applying the per-slice modifications
> -    * - __u8
> +    * - struct :c:type:`v4l2_h264_reference`
>        - ``ref_pic_list1[32]``
>        - Reference picture list after applying the per-slice modifications
>      * - __u32
> @@ -1926,6 +1926,42 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        - ``chroma_offset[32][2]``
>        -
>  
> +``Picture Reference``
> +
> +.. c:type:: v4l2_h264_reference
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_h264_reference
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - __u16
> +      - ``flags``
> +      - See :ref:`Picture Reference Flags <h264_reference_flags>`
> +    * - __u8
> +      - ``index``
> +      - Index into the :c:type:`v4l2_ctrl_h264_decode_params`.dpb array.
> +
> +.. _h264_reference_flags:
> +
> +``Picture Reference Flags``
> +
> +.. cssclass:: longtable
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - ``V4L2_H264_REFERENCE_FLAG_TOP_FIELD``
> +      - 0x00000001
> +      -
> +    * - ``V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD``
> +      - 0x00000002
> +      -
> +
>  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS (struct)``
>      Specifies the decode parameters (as extracted from the bitstream)
>      for the associated H264 slice data. This includes the necessary
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index 54ee2aa423e2..cce527bbdf86 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -166,8 +166,8 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
>  
>  static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  				   struct cedrus_run *run,
> -				   const u8 *ref_list, u8 num_ref,
> -				   enum cedrus_h264_sram_off sram)
> +				   const struct v4l2_h264_reference *ref_list,
> +				   u8 num_ref, enum cedrus_h264_sram_off sram)
>  {
>  	const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params;
>  	struct vb2_queue *cap_q;
> @@ -188,7 +188,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  		int buf_idx;
>  		u8 dpb_idx;
>  
> -		dpb_idx = ref_list[i];
> +		dpb_idx = ref_list[i].index;
>  		dpb = &decode->dpb[dpb_idx];
>  
>  		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index 080fd1293c42..bca4a9887e7e 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -19,6 +19,8 @@
>   */
>  #define V4L2_H264_NUM_DPB_ENTRIES 16
>  
> +#define V4L2_H264_REF_LIST_LEN (2 * V4L2_H264_NUM_DPB_ENTRIES)
> +
>  /* Our pixel format isn't stable at the moment */
>  #define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */
>  
> @@ -140,6 +142,16 @@ struct v4l2_h264_pred_weight_table {
>  #define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x04
>  #define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x08
>  
> +#define V4L2_H264_REFERENCE_FLAG_TOP_FIELD		0x01
> +#define V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD		0x02

Later in this series enum v4l2_h264_dpb_reference is introduced that
represent same information as these two flags.
Maybe consolidate and use a shared enum for both slice ref list and dpb?

Regards,
Jonas

> +
> +struct v4l2_h264_reference {
> +	__u8 flags;
> +
> +	/* Index into v4l2_ctrl_h264_decode_params.dpb[] */
> +	__u8 index;
> +};
> +
>  struct v4l2_ctrl_h264_slice_params {
>  	/* Size in bytes, including header */
>  	__u32 size;
> @@ -178,12 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
>  	__u8 num_ref_idx_l1_active_minus1;
>  	__u32 slice_group_change_cycle;
>  
> -	/*
> -	 * Entries on each list are indices into
> -	 * v4l2_ctrl_h264_decode_params.dpb[].
> -	 */
> -	__u8 ref_pic_list0[32];
> -	__u8 ref_pic_list1[32];
> +	struct v4l2_h264_reference ref_pic_list0[V4L2_H264_REF_LIST_LEN];
> +	struct v4l2_h264_reference ref_pic_list1[V4L2_H264_REF_LIST_LEN];
>  
>  	__u32 flags;
>  };
> 

Re: [PATCH 06/10] media: uapi: h264: Cleanup DPB entry interface

[Jonas Karlman]

On 2020-07-15 22:22, Ezequiel Garcia wrote:
> As discussed recently, the current interface for the
> Decoded Picture Buffer is not enough to properly
> support field coding.
> 
> This commit introduces enough semantics to support
> frame and field coding, and to signal how DPB entries
> are "used for reference".
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 46 ++++++++++++-------
>  drivers/media/v4l2-core/v4l2-h264.c           |  4 +-
>  drivers/staging/media/rkvdec/rkvdec-h264.c    |  8 ++--
>  include/media/h264-ctrls.h                    |  8 +++-
>  4 files changed, 42 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index dd8e5a2e8986..46d4c8c6ad47 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __s32
>        - ``bottom_field_order_cnt``
>        -
> +    * - enum :c:type:`v4l2_h264_dpb_reference`
> +      - ``reference``
> +      - Specifies how the DPB entry is referenced.
>      * - __u32
>        - ``flags``
>        - See :ref:`DPB Entry Flags <h264_dpb_flags>`
>  
> +.. c:type:: v4l2_h264_dpb_reference
> +
> +.. cssclass:: longtable
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - ``V4L2_H264_DPB_TOP_REF``
> +      - 0x1
> +      - The top field in field pair is used for
> +        short-term reference.
> +    * - ``V4L2_H264_DPB_BOTTOM_REF``
> +      - 0x2
> +      - The bottom field in field pair is used for
> +        short-term reference.
> +    * - ``V4L2_H264_DPB_FRAME_REF``
> +      - 0x3
> +      - The frame (or the top/bottom fields, if it's a field pair)
> +        is used for short-term reference.
> +
>  .. _h264_dpb_flags:
>  
>  ``DPB Entries Flags``
> @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>  
>      * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID``
>        - 0x00000001
> -      - The DPB entry is valid and should be considered
> +      - The DPB entry is valid (non-empty) and should be considered.
>      * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE``
>        - 0x00000002
> -      - The DPB entry is currently being used as a reference frame
> +      - The DPB entry is used for reference.
>      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
>        - 0x00000004
> -      - The DPB entry is a long term reference frame
> +      - The DPB entry is used for long-term reference.
>      * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD``
>        - 0x00000008
> -      - The DPB entry is a field reference, which means only one of the field
> -        will be used when decoding the new frame/field. When not set the DPB
> -        entry is a frame reference (both fields will be used). Note that this
> -        flag does not say anything about the number of fields contained in the
> -        reference frame, it just describes the one used to decode the new
> -        field/frame
> -    * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD``
> -      - 0x00000010
> -      - The DPB entry is a bottom field reference (only the bottom field of the
> -        reference frame is needed to decode the new frame/field). Only valid if
> -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When
> -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but
> -        V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the
> -        DPB entry is a top field reference
> +      - The DPB entry is a single field or a complementary field pair.
>  
>  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
>      Specifies the decoding mode to use. Currently exposes slice-based and
> diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
> index edf6225f0522..306a51683606 100644
> --- a/drivers/media/v4l2-core/v4l2-h264.c
> +++ b/drivers/media/v4l2-core/v4l2-h264.c
> @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
>  		else
>  			b->refs[i].frame_num = dpb[i].frame_num;
>  
> -		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
> +		if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF)

This looks wrong, should probably use ==,

dpb[i].reference == V4L2_H264_DPB_FRAME_REF

else this would match any reference value.

>  			pic_order_count = min(dpb[i].top_field_order_cnt,
>  					      dpb[i].bottom_field_order_cnt);
> -		else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
> +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
>  			pic_order_count = dpb[i].bottom_field_order_cnt;
>  		else
>  			pic_order_count = dpb[i].top_field_order_cnt;
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 7b66e2743a4f..57539c630422 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx,
>  			     RKVDEC_COLMV_USED_FLAG_REF;
>  
>  		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
> -			refer_addr |= RKVDEC_TOPFIELD_USED_REF |
> -				      RKVDEC_BOTFIELD_USED_REF;
> -		else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
> +			refer_addr |= RKVDEC_FIELD_REF;
> +
> +		if (dpb[i].reference & V4L2_H264_DPB_TOP_REF)
>  			refer_addr |= RKVDEC_BOTFIELD_USED_REF;
> -		else
> +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)

This should probably be if and not else if, and BOTFIELD/TOPFIELD_USED_REF
seems to be mixed up.

I have only taken a quick look so far, I will update ffmpeg and runtime test
later this weekend, will get back with result and full review on Sunday evening.

Regards,
Jonas

>  			refer_addr |= RKVDEC_TOPFIELD_USED_REF;
>  
>  		writel_relaxed(dpb[i].top_field_order_cnt,
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index 620ee8863d74..52f3976b986c 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -202,7 +202,12 @@ struct v4l2_ctrl_h264_slice_params {
>  #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE		0x02
>  #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
>  #define V4L2_H264_DPB_ENTRY_FLAG_FIELD		0x08
> -#define V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD	0x10
> +
> +enum v4l2_h264_dpb_reference {
> +	V4L2_H264_DPB_TOP_REF = 0x1,
> +	V4L2_H264_DPB_BOTTOM_REF = 0x2,
> +	V4L2_H264_DPB_FRAME_REF = 0x3,
> +};
>  
>  struct v4l2_h264_dpb_entry {
>  	__u64 reference_ts;
> @@ -211,6 +216,7 @@ struct v4l2_h264_dpb_entry {
>  	/* Note that field is indicated by v4l2_buffer.field */
>  	__s32 top_field_order_cnt;
>  	__s32 bottom_field_order_cnt;
> +	enum v4l2_h264_dpb_reference reference;
>  	__u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
>  };
>  
> 

Re: [PATCH 06/10] media: uapi: h264: Cleanup DPB entry interface

[Ezequiel Garcia]

Hello Jonas,

On Wed, 2020-07-22 at 21:52 +0000, Jonas Karlman wrote:
> On 2020-07-15 22:22, Ezequiel Garcia wrote:
> > As discussed recently, the current interface for the
> > Decoded Picture Buffer is not enough to properly
> > support field coding.
> > 
> > This commit introduces enough semantics to support
> > frame and field coding, and to signal how DPB entries
> > are "used for reference".
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  .../media/v4l/ext-ctrls-codec.rst             | 46 ++++++++++++-------
> >  drivers/media/v4l2-core/v4l2-h264.c           |  4 +-
> >  drivers/staging/media/rkvdec/rkvdec-h264.c    |  8 ++--
> >  include/media/h264-ctrls.h                    |  8 +++-
> >  4 files changed, 42 insertions(+), 24 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index dd8e5a2e8986..46d4c8c6ad47 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __s32
> >        - ``bottom_field_order_cnt``
> >        -
> > +    * - enum :c:type:`v4l2_h264_dpb_reference`
> > +      - ``reference``
> > +      - Specifies how the DPB entry is referenced.
> >      * - __u32
> >        - ``flags``
> >        - See :ref:`DPB Entry Flags <h264_dpb_flags>`
> >  
> > +.. c:type:: v4l2_h264_dpb_reference
> > +
> > +.. cssclass:: longtable
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +    :widths:       1 1 2
> > +
> > +    * - ``V4L2_H264_DPB_TOP_REF``
> > +      - 0x1
> > +      - The top field in field pair is used for
> > +        short-term reference.
> > +    * - ``V4L2_H264_DPB_BOTTOM_REF``
> > +      - 0x2
> > +      - The bottom field in field pair is used for
> > +        short-term reference.
> > +    * - ``V4L2_H264_DPB_FRAME_REF``
> > +      - 0x3
> > +      - The frame (or the top/bottom fields, if it's a field pair)
> > +        is used for short-term reference.
> > +
> >  .. _h264_dpb_flags:
> >  
> >  ``DPB Entries Flags``
> > @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >  
> >      * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID``
> >        - 0x00000001
> > -      - The DPB entry is valid and should be considered
> > +      - The DPB entry is valid (non-empty) and should be considered.
> >      * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE``
> >        - 0x00000002
> > -      - The DPB entry is currently being used as a reference frame
> > +      - The DPB entry is used for reference.
> >      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
> >        - 0x00000004
> > -      - The DPB entry is a long term reference frame
> > +      - The DPB entry is used for long-term reference.
> >      * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD``
> >        - 0x00000008
> > -      - The DPB entry is a field reference, which means only one of the field
> > -        will be used when decoding the new frame/field. When not set the DPB
> > -        entry is a frame reference (both fields will be used). Note that this
> > -        flag does not say anything about the number of fields contained in the
> > -        reference frame, it just describes the one used to decode the new
> > -        field/frame
> > -    * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD``
> > -      - 0x00000010
> > -      - The DPB entry is a bottom field reference (only the bottom field of the
> > -        reference frame is needed to decode the new frame/field). Only valid if
> > -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When
> > -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but
> > -        V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the
> > -        DPB entry is a top field reference
> > +      - The DPB entry is a single field or a complementary field pair.
> >  
> >  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
> >      Specifies the decoding mode to use. Currently exposes slice-based and
> > diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
> > index edf6225f0522..306a51683606 100644
> > --- a/drivers/media/v4l2-core/v4l2-h264.c
> > +++ b/drivers/media/v4l2-core/v4l2-h264.c
> > @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> >  		else
> >  			b->refs[i].frame_num = dpb[i].frame_num;
> >  
> > -		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
> > +		if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF)
> 
> This looks wrong, should probably use ==,
> 
> dpb[i].reference == V4L2_H264_DPB_FRAME_REF
> 
> else this would match any reference value.
> 
> >  			pic_order_count = min(dpb[i].top_field_order_cnt,
> >  					      dpb[i].bottom_field_order_cnt);
> > -		else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
> > +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
> >  			pic_order_count = dpb[i].bottom_field_order_cnt;
> >  		else
> >  			pic_order_count = dpb[i].top_field_order_cnt;
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index 7b66e2743a4f..57539c630422 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx,
> >  			     RKVDEC_COLMV_USED_FLAG_REF;
> >  
> >  		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
> > -			refer_addr |= RKVDEC_TOPFIELD_USED_REF |
> > -				      RKVDEC_BOTFIELD_USED_REF;
> > -		else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
> > +			refer_addr |= RKVDEC_FIELD_REF;
> > +
> > +		if (dpb[i].reference & V4L2_H264_DPB_TOP_REF)
> >  			refer_addr |= RKVDEC_BOTFIELD_USED_REF;
> > -		else
> > +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
> 
> This should probably be if and not else if, and BOTFIELD/TOPFIELD_USED_REF
> seems to be mixed up.
> 
> I have only taken a quick look so far, I will update ffmpeg and runtime test
> later this weekend, will get back with result and full review on Sunday evening.
> 

Thanks that would be useful.

However, keep in mind this series is specifically concerned
with the uAPI review.

This is not supposed to fix the field coded support, or anything
else in any driver.

IMO, at this stage, fixing drivers is somewhat lower priority
than discussing and stabilizing the uAPI.

Thanks,
Ezequiel

Re: [PATCH 03/10] media: uapi: h264: Split prediction weight parameters

[Alexandre Courbot]

On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> The prediction weight parameters are only required under
> certain conditions, which depend on slice header parameters.
>
> The slice header syntax specifies that the prediction
> weight table is present if:
>
> ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \
> (weighted_bipred_idc == 1 && slice_type == B))

This is a pretty important bit - how about mentioning in the documentation when
this new control is expected to be present, so both drivers and
userspace submit it
or omit it in a consistent manner?

Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Alexandre Courbot]

On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> The H.264 specification requires in its "Slice header semantics"
> section that the following values shall be the same in all slice headers:
>
>   pic_parameter_set_id
>   frame_num
>   field_pic_flag
>   bottom_field_flag
>   idr_pic_id
>   pic_order_cnt_lsb
>   delta_pic_order_cnt_bottom
>   delta_pic_order_cnt[ 0 ]
>   delta_pic_order_cnt[ 1 ]
>   sp_for_switch_flag
>   slice_group_change_cycle
>
> and can therefore be moved to the per-frame decode parameters control.

I am really not a H.264 expert, so this question may not be relevant,
but are these values specified for every slice header in the
bitstream, or are they specified only once per frame?

I am asking this because it would certainly make user-space code
simpler if we could remain as close to the bitstream as possible. If
these values are specified once per slice, then factorizing them would
leave user-space with the burden of deciding what to do if they change
across slices.

Note that this is a double-edged sword, because it is not necessarily
better to leave the firmware in charge of deciding what to do in such
a case. :) So hopefully these are only specified once per frame in the
bitstream, in which case your proposal makes complete sense.

Re: [PATCH 09/10] media: hantro: Don't require unneeded H264_SLICE_PARAMS

[Alexandre Courbot]

On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> Now that slice invariant parameters have been moved,
> the driver no longer needs this control, so drop it.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/staging/media/hantro/hantro_drv.c  | 5 -----
>  drivers/staging/media/hantro/hantro_h264.c | 5 -----
>  drivers/staging/media/hantro/hantro_hw.h   | 2 --
>  3 files changed, 12 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 34797507f214..3cd00cc0a364 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -306,11 +306,6 @@ static const struct hantro_ctrl controls[] = {
>                 .cfg = {
>                         .id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
>                 },
> -       }, {
> -               .codec = HANTRO_H264_DECODER,
> -               .cfg = {
> -                       .id = V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
> -               },

Isn't this going to make the driver reject (as opposed to just ignore)
this control altogether? Also, even though the control is not required
anymore, don't we want to check that it is provided in order to ensure
user-space follows the spec (granted, this would be better done in a
common framework shared by all drivers).

I'd also suggest this patch (and the following one) to be merged into
the previous one as they are just removing fields that have become
unneeded because of it.

Re: [PATCH 09/10] media: hantro: Don't require unneeded H264_SLICE_PARAMS

[Alexandre Courbot]

On Sat, Jul 25, 2020 at 11:45 PM Alexandre Courbot
<acourbot@chromium.org> wrote:
>
> On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> >
> > Now that slice invariant parameters have been moved,
> > the driver no longer needs this control, so drop it.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/staging/media/hantro/hantro_drv.c  | 5 -----
> >  drivers/staging/media/hantro/hantro_h264.c | 5 -----
> >  drivers/staging/media/hantro/hantro_hw.h   | 2 --
> >  3 files changed, 12 deletions(-)
> >
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index 34797507f214..3cd00cc0a364 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -306,11 +306,6 @@ static const struct hantro_ctrl controls[] = {
> >                 .cfg = {
> >                         .id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
> >                 },
> > -       }, {
> > -               .codec = HANTRO_H264_DECODER,
> > -               .cfg = {
> > -                       .id = V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
> > -               },
>
> Isn't this going to make the driver reject (as opposed to just ignore)
> this control altogether? Also, even though the control is not required
> anymore, don't we want to check that it is provided in order to ensure
> user-space follows the spec (granted, this would be better done in a
> common framework shared by all drivers).

I kind of forgot about the previous discussion about frame-based vs
slice-based decoders, and since Hantro is frame-based this makes my
point above moot. Please ignore.

>
> I'd also suggest this patch (and the following one) to be merged into
> the previous one as they are just removing fields that have become
> unneeded because of it.

Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Ezequiel Garcia]

Hi Alexandre,

Thanks a lot for the review.

On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote:
> On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > The H.264 specification requires in its "Slice header semantics"
> > section that the following values shall be the same in all slice headers:
> > 
> >   pic_parameter_set_id
> >   frame_num
> >   field_pic_flag
> >   bottom_field_flag
> >   idr_pic_id
> >   pic_order_cnt_lsb
> >   delta_pic_order_cnt_bottom
> >   delta_pic_order_cnt[ 0 ]
> >   delta_pic_order_cnt[ 1 ]
> >   sp_for_switch_flag
> >   slice_group_change_cycle
> > 
> > and can therefore be moved to the per-frame decode parameters control.
> 
> I am really not a H.264 expert, so this question may not be relevant,

All questions are welcome. I'm more than happy to discuss this patchset.

> but are these values specified for every slice header in the
> bitstream, or are they specified only once per frame?
> 
> I am asking this because it would certainly make user-space code
> simpler if we could remain as close to the bitstream as possible. If
> these values are specified once per slice, then factorizing them would
> leave user-space with the burden of deciding what to do if they change
> across slices.
> 
> Note that this is a double-edged sword, because it is not necessarily
> better to leave the firmware in charge of deciding what to do in such
> a case. :) So hopefully these are only specified once per frame in the
> bitstream, in which case your proposal makes complete sense.

Frame-based hardwares accelerators such as Hantro and Rockchip VDEC
are doing the slice header parsing themselves. Therefore, the
driver is not really parsing these fields on each slice header.

Currently, we are already using only the first slice in a frame,
as you can see from:

        if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
                reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;

Even if these fields are transported in the slice header,
I think it makes sense for us to split them into the decode params
(per-frame) control.

They are really specified to be the same across all slices,
so even I'd say if a bitstream violates this, it's likely
either a corrupted bitstream or an encoder bug.

OTOH, one thing this makes me realize is that the slice params control
is wrongly specified as an array. Namely, this text
should be removed:

       This structure is expected to be passed as an array, with one
       entry for each slice included in the bitstream buffer.

As the API is really not defined that way.

I'll remove that on next iteration.

Thanks for raising this point,
Ezequiel

Re: [PATCH 09/10] media: hantro: Don't require unneeded H264_SLICE_PARAMS

[Ezequiel Garcia]

Hi Alexandre,

Despite you've asked to ignore this review,
let me comment on it.

On Sat, 2020-07-25 at 23:45 +0900, Alexandre Courbot wrote:
> On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > Now that slice invariant parameters have been moved,
> > the driver no longer needs this control, so drop it.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/staging/media/hantro/hantro_drv.c  | 5 -----
> >  drivers/staging/media/hantro/hantro_h264.c | 5 -----
> >  drivers/staging/media/hantro/hantro_hw.h   | 2 --
> >  3 files changed, 12 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index 34797507f214..3cd00cc0a364 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -306,11 +306,6 @@ static const struct hantro_ctrl controls[] = {
> >                 .cfg = {
> >                         .id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
> >                 },
> > -       }, {
> > -               .codec = HANTRO_H264_DECODER,
> > -               .cfg = {
> > -                       .id = V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
> > -               },
> 
> Isn't this going to make the driver reject (as opposed to just ignore)
> this control altogether? Also, even though the control is not required
> anymore, don't we want to check that it is provided in order to ensure
> user-space follows the spec (granted, this would be better done in a
> common framework shared by all drivers).
> 

As you mentioned on your next reply, indeed frame-based drivers
can't really parse the slice headers.

I believe the above comment would make sense, if we want to avoid
breaking compatibility.

In our case, we are already breaking this (it's broken from the minute
you change any control in the API, as V4L2 reject mismatch sizes
for the controls).

So, I'd say it makes sense to drop it now while we can.

Also, Nicolas has suggested that this makes applications simpler. 

> I'd also suggest this patch (and the following one) to be merged into
> the previous one as they are just removing fields that have become
> unneeded because of it.

I'd like to keep the patches touching the uAPI separate from the
ones touching the driver, when possible (i.e. when the build
is not broken by API changes).

Thanks,
Ezequiel

Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Tomasz Figa]

On Mon, Jul 27, 2020 at 4:39 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> Hi Alexandre,
>
> Thanks a lot for the review.
>
> On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote:
> > On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > The H.264 specification requires in its "Slice header semantics"
> > > section that the following values shall be the same in all slice headers:
> > >
> > >   pic_parameter_set_id
> > >   frame_num
> > >   field_pic_flag
> > >   bottom_field_flag
> > >   idr_pic_id
> > >   pic_order_cnt_lsb
> > >   delta_pic_order_cnt_bottom
> > >   delta_pic_order_cnt[ 0 ]
> > >   delta_pic_order_cnt[ 1 ]
> > >   sp_for_switch_flag
> > >   slice_group_change_cycle
> > >
> > > and can therefore be moved to the per-frame decode parameters control.
> >
> > I am really not a H.264 expert, so this question may not be relevant,
>
> All questions are welcome. I'm more than happy to discuss this patchset.
>
> > but are these values specified for every slice header in the
> > bitstream, or are they specified only once per frame?
> >
> > I am asking this because it would certainly make user-space code
> > simpler if we could remain as close to the bitstream as possible. If
> > these values are specified once per slice, then factorizing them would
> > leave user-space with the burden of deciding what to do if they change
> > across slices.
> >
> > Note that this is a double-edged sword, because it is not necessarily
> > better to leave the firmware in charge of deciding what to do in such
> > a case. :) So hopefully these are only specified once per frame in the
> > bitstream, in which case your proposal makes complete sense.
>
> Frame-based hardwares accelerators such as Hantro and Rockchip VDEC
> are doing the slice header parsing themselves. Therefore, the
> driver is not really parsing these fields on each slice header.
>
> Currently, we are already using only the first slice in a frame,
> as you can see from:
>
>         if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
>                 reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
>
> Even if these fields are transported in the slice header,
> I think it makes sense for us to split them into the decode params
> (per-frame) control.
>
> They are really specified to be the same across all slices,
> so even I'd say if a bitstream violates this, it's likely
> either a corrupted bitstream or an encoder bug.
>
> OTOH, one thing this makes me realize is that the slice params control
> is wrongly specified as an array.

It is _not_.

> Namely, this text
> should be removed:
>
>        This structure is expected to be passed as an array, with one
>        entry for each slice included in the bitstream buffer.
>
> As the API is really not defined that way.
>
> I'll remove that on next iteration.

The v4l2_ctrl_h264_slice_params struct has more data than those that
are deemed to be the same across all the slices. A remarkable example
are the size and start_byte_offset fields.

Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Ezequiel Garcia]

On Mon, 2020-07-27 at 16:52 +0200, Tomasz Figa wrote:
> On Mon, Jul 27, 2020 at 4:39 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > Hi Alexandre,
> > 
> > Thanks a lot for the review.
> > 
> > On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote:
> > > On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > The H.264 specification requires in its "Slice header semantics"
> > > > section that the following values shall be the same in all slice headers:
> > > > 
> > > >   pic_parameter_set_id
> > > >   frame_num
> > > >   field_pic_flag
> > > >   bottom_field_flag
> > > >   idr_pic_id
> > > >   pic_order_cnt_lsb
> > > >   delta_pic_order_cnt_bottom
> > > >   delta_pic_order_cnt[ 0 ]
> > > >   delta_pic_order_cnt[ 1 ]
> > > >   sp_for_switch_flag
> > > >   slice_group_change_cycle
> > > > 
> > > > and can therefore be moved to the per-frame decode parameters control.
> > > 
> > > I am really not a H.264 expert, so this question may not be relevant,
> > 
> > All questions are welcome. I'm more than happy to discuss this patchset.
> > 
> > > but are these values specified for every slice header in the
> > > bitstream, or are they specified only once per frame?
> > > 
> > > I am asking this because it would certainly make user-space code
> > > simpler if we could remain as close to the bitstream as possible. If
> > > these values are specified once per slice, then factorizing them would
> > > leave user-space with the burden of deciding what to do if they change
> > > across slices.
> > > 
> > > Note that this is a double-edged sword, because it is not necessarily
> > > better to leave the firmware in charge of deciding what to do in such
> > > a case. :) So hopefully these are only specified once per frame in the
> > > bitstream, in which case your proposal makes complete sense.
> > 
> > Frame-based hardwares accelerators such as Hantro and Rockchip VDEC
> > are doing the slice header parsing themselves. Therefore, the
> > driver is not really parsing these fields on each slice header.
> > 
> > Currently, we are already using only the first slice in a frame,
> > as you can see from:
> > 
> >         if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> >                 reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
> > 
> > Even if these fields are transported in the slice header,
> > I think it makes sense for us to split them into the decode params
> > (per-frame) control.
> > 
> > They are really specified to be the same across all slices,
> > so even I'd say if a bitstream violates this, it's likely
> > either a corrupted bitstream or an encoder bug.
> > 
> > OTOH, one thing this makes me realize is that the slice params control
> > is wrongly specified as an array.
> 
> It is _not_.
> 

We introduced the hold capture buffer specifically to support
this without having a slice array.

I don't think we have a plan to support this control properly
as an array.

If we decide to support the slice control as an array,
we would have to implement a mechanism to specify the array size,
which we currently don't have AFAIK.

> > Namely, this text
> > should be removed:
> > 
> >        This structure is expected to be passed as an array, with one
> >        entry for each slice included in the bitstream buffer.
> > 
> > As the API is really not defined that way.
> > 
> > I'll remove that on next iteration.
> 
> The v4l2_ctrl_h264_slice_params struct has more data than those that
> are deemed to be the same across all the slices. A remarkable example
> are the size and start_byte_offset fields.

Not sure how this applies to this discussion.

Thanks!
Ezequiel

Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Tomasz Figa]

On Mon, Jul 27, 2020 at 6:18 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> On Mon, 2020-07-27 at 16:52 +0200, Tomasz Figa wrote:
> > On Mon, Jul 27, 2020 at 4:39 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > Hi Alexandre,
> > >
> > > Thanks a lot for the review.
> > >
> > > On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote:
> > > > On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > > The H.264 specification requires in its "Slice header semantics"
> > > > > section that the following values shall be the same in all slice headers:
> > > > >
> > > > >   pic_parameter_set_id
> > > > >   frame_num
> > > > >   field_pic_flag
> > > > >   bottom_field_flag
> > > > >   idr_pic_id
> > > > >   pic_order_cnt_lsb
> > > > >   delta_pic_order_cnt_bottom
> > > > >   delta_pic_order_cnt[ 0 ]
> > > > >   delta_pic_order_cnt[ 1 ]
> > > > >   sp_for_switch_flag
> > > > >   slice_group_change_cycle
> > > > >
> > > > > and can therefore be moved to the per-frame decode parameters control.
> > > >
> > > > I am really not a H.264 expert, so this question may not be relevant,
> > >
> > > All questions are welcome. I'm more than happy to discuss this patchset.
> > >
> > > > but are these values specified for every slice header in the
> > > > bitstream, or are they specified only once per frame?
> > > >
> > > > I am asking this because it would certainly make user-space code
> > > > simpler if we could remain as close to the bitstream as possible. If
> > > > these values are specified once per slice, then factorizing them would
> > > > leave user-space with the burden of deciding what to do if they change
> > > > across slices.
> > > >
> > > > Note that this is a double-edged sword, because it is not necessarily
> > > > better to leave the firmware in charge of deciding what to do in such
> > > > a case. :) So hopefully these are only specified once per frame in the
> > > > bitstream, in which case your proposal makes complete sense.
> > >
> > > Frame-based hardwares accelerators such as Hantro and Rockchip VDEC
> > > are doing the slice header parsing themselves. Therefore, the
> > > driver is not really parsing these fields on each slice header.
> > >
> > > Currently, we are already using only the first slice in a frame,
> > > as you can see from:
> > >
> > >         if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> > >                 reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
> > >
> > > Even if these fields are transported in the slice header,
> > > I think it makes sense for us to split them into the decode params
> > > (per-frame) control.
> > >
> > > They are really specified to be the same across all slices,
> > > so even I'd say if a bitstream violates this, it's likely
> > > either a corrupted bitstream or an encoder bug.
> > >
> > > OTOH, one thing this makes me realize is that the slice params control
> > > is wrongly specified as an array.
> >
> > It is _not_.
> >
>
> We introduced the hold capture buffer specifically to support
> this without having a slice array.
>
> I don't think we have a plan to support this control properly
> as an array.
>
> If we decide to support the slice control as an array,
> we would have to implement a mechanism to specify the array size,
> which we currently don't have AFAIK.
>

That wasn't the conclusion when we discussed this last time on IRC.
+Nicolas Dufresne

Currently the 1-slice per buffer model is quite impractical:
1) the maximum number of buffers is 32, which for some streams can be
less than needed to queue a single frame,
2) even more system call overhead due to the need to repeat various
operations (e.g. qbuf/dqbuf) per-slice rather than per-frame,
3) no way to do hardware batching for hardware which supports queuing
multiple slices at a time,
4) waste of memory - one needs to allocate all the OUTPUT buffers
pessimistically to accommodate the biggest possible slice, while with
all-slices-per-frame 1 buffer could be just heuristically allocated to
be enough for the whole frame.

These need to be carefully evaluated, with some proper testing done to
confirm whether they are really a problem or not.

> > > Namely, this text
> > > should be removed:
> > >
> > >        This structure is expected to be passed as an array, with one
> > >        entry for each slice included in the bitstream buffer.
> > >
> > > As the API is really not defined that way.
> > >
> > > I'll remove that on next iteration.
> >
> > The v4l2_ctrl_h264_slice_params struct has more data than those that
> > are deemed to be the same across all the slices. A remarkable example
> > are the size and start_byte_offset fields.
>
> Not sure how this applies to this discussion.

These fields need to be specified for each slice in the buffer to make
it possible to handle multiple slices per buffer.

Best regards,
Tomasz

Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 7652 bytes --]

Le lundi 27 juillet 2020 à 20:10 +0200, Tomasz Figa a écrit :
> On Mon, Jul 27, 2020 at 6:18 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > On Mon, 2020-07-27 at 16:52 +0200, Tomasz Figa wrote:
> > > On Mon, Jul 27, 2020 at 4:39 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > Hi Alexandre,
> > > > 
> > > > Thanks a lot for the review.
> > > > 
> > > > On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote:
> > > > > On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > > > The H.264 specification requires in its "Slice header semantics"
> > > > > > section that the following values shall be the same in all slice headers:
> > > > > > 
> > > > > >   pic_parameter_set_id
> > > > > >   frame_num
> > > > > >   field_pic_flag
> > > > > >   bottom_field_flag
> > > > > >   idr_pic_id
> > > > > >   pic_order_cnt_lsb
> > > > > >   delta_pic_order_cnt_bottom
> > > > > >   delta_pic_order_cnt[ 0 ]
> > > > > >   delta_pic_order_cnt[ 1 ]
> > > > > >   sp_for_switch_flag
> > > > > >   slice_group_change_cycle
> > > > > > 
> > > > > > and can therefore be moved to the per-frame decode parameters control.
> > > > > 
> > > > > I am really not a H.264 expert, so this question may not be relevant,
> > > > 
> > > > All questions are welcome. I'm more than happy to discuss this patchset.
> > > > 
> > > > > but are these values specified for every slice header in the
> > > > > bitstream, or are they specified only once per frame?
> > > > > 
> > > > > I am asking this because it would certainly make user-space code
> > > > > simpler if we could remain as close to the bitstream as possible. If
> > > > > these values are specified once per slice, then factorizing them would
> > > > > leave user-space with the burden of deciding what to do if they change
> > > > > across slices.
> > > > > 
> > > > > Note that this is a double-edged sword, because it is not necessarily
> > > > > better to leave the firmware in charge of deciding what to do in such
> > > > > a case. :) So hopefully these are only specified once per frame in the
> > > > > bitstream, in which case your proposal makes complete sense.
> > > > 
> > > > Frame-based hardwares accelerators such as Hantro and Rockchip VDEC
> > > > are doing the slice header parsing themselves. Therefore, the
> > > > driver is not really parsing these fields on each slice header.
> > > > 
> > > > Currently, we are already using only the first slice in a frame,
> > > > as you can see from:
> > > > 
> > > >         if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> > > >                 reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
> > > > 
> > > > Even if these fields are transported in the slice header,
> > > > I think it makes sense for us to split them into the decode params
> > > > (per-frame) control.
> > > > 
> > > > They are really specified to be the same across all slices,
> > > > so even I'd say if a bitstream violates this, it's likely
> > > > either a corrupted bitstream or an encoder bug.
> > > > 
> > > > OTOH, one thing this makes me realize is that the slice params control
> > > > is wrongly specified as an array.
> > > 
> > > It is _not_.
> > > 
> > 
> > We introduced the hold capture buffer specifically to support
> > this without having a slice array.
> > 
> > I don't think we have a plan to support this control properly
> > as an array.
> > 
> > If we decide to support the slice control as an array,
> > we would have to implement a mechanism to specify the array size,
> > which we currently don't have AFAIK.
> > 
> 
> That wasn't the conclusion when we discussed this last time on IRC.
> +Nicolas Dufresne
> 
> Currently the 1-slice per buffer model is quite impractical:
> 1) the maximum number of buffers is 32, which for some streams can be
> less than needed to queue a single frame,

To give more context, it seems the discussion was about being able to
use slice decoder with a 1 poll() per frame model. Of course this will
never be as efficient as when using a frame base decoder, but as
current design, you can keep a list of pending request (each request is
1 slice/buffer), and simply use memory pressure to poll a mid point and
dequeue the remaining. An example, yo have 8 pending request, and reach
your memory limit:
  
  [R1, R2, R3, R4, R5, R6, R7, R8]

As requests are in order and behaves like memory fences, you can pick
R6, and poll() that one. When R6 is ready, you can then dequeue R1 to
R6 without blocking. In this context, a limit of 16 or 32 buffers seems
fair, the optimization we can do in userspace is likely sufficient. So
I'd like to drop problem 1) from our list.

> 2) even more system call overhead due to the need to repeat various
> operations (e.g. qbuf/dqbuf) per-slice rather than per-frame,
> 3) no way to do hardware batching for hardware which supports queuing
> multiple slices at a time,
> 4) waste of memory - one needs to allocate all the OUTPUT buffers
> pessimistically to accommodate the biggest possible slice, while with
> all-slices-per-frame 1 buffer could be just heuristically allocated to
> be enough for the whole frame.
> 
> These need to be carefully evaluated, with some proper testing done to
> confirm whether they are really a problem or not.

2, 3 and 4 seems to match what the currently unimplemented API propose.
You can mitigate 2) but having multiple slices per buffers. That came
with a byte offset to we can program the HW as if it was separate slice
buffers. But was limited to 16 buffers, likely a fair compromise.

3) is about batching, in the only use case we know, the batching
acceleration consist of programming the next operation on the
completion IRQ. I already looked with the Cedrus developers if and how
that was feasible, but we don't have a PoC yet. The optimization is
about removing context switches between operations, which could prevent
fully using the HW.

4) is also well covered with being able to multiplex 1 buffer with
multiple slices.

To be fair, I understand why we'd like to drop this API, as none of the
active developers here of slice decoder (cedrus) have time to engage in
supporting this untested "optimization". It's not only about kernel
support, but also requires userspace work. I also agree that it could
be added later, as an extension. It could be done with 3 new controls,
an array of slice_params and an array of slice start offset and the
number of slices, or just one, introduce a new structure that have a
slice_params structure embedded, num_slices and an array of
slice_start_offset. I don't have preference myself, but I'm just
illustrating that yes, we could drop the slice batching to avoid
pushing untested APIs without scarifying our ability to decode a valid
stream.

> 
> > > > Namely, this text
> > > > should be removed:
> > > > 
> > > >        This structure is expected to be passed as an array, with one
> > > >        entry for each slice included in the bitstream buffer.
> > > > 
> > > > As the API is really not defined that way.
> > > > 
> > > > I'll remove that on next iteration.
> > > 
> > > The v4l2_ctrl_h264_slice_params struct has more data than those that
> > > are deemed to be the same across all the slices. A remarkable example
> > > are the size and start_byte_offset fields.
> > 
> > Not sure how this applies to this discussion.
> 
> These fields need to be specified for each slice in the buffer to make
> it possible to handle multiple slices per buffer.
> 
> Best regards,
> Tomasz

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 10/10] media: rkvdec: Don't require unneeded H264_SLICE_PARAMS

[Jonas Karlman]

Hi,

On 2020-07-15 22:22, Ezequiel Garcia wrote:
> Now that slice invariant parameters have been moved,
> the driver no longer needs this control, so drop it.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 4 ----
>  drivers/staging/media/rkvdec/rkvdec.c      | 5 -----
>  2 files changed, 9 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 57c084910b3b..f6e1fa19d625 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -109,7 +109,6 @@ struct rkvdec_h264_reflists {
>  struct rkvdec_h264_run {
>  	struct rkvdec_run base;
>  	const struct v4l2_ctrl_h264_decode_params *decode_params;
> -	const struct v4l2_ctrl_h264_slice_params *slices_params;
>  	const struct v4l2_ctrl_h264_sps *sps;
>  	const struct v4l2_ctrl_h264_pps *pps;
>  	const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
> @@ -1067,9 +1066,6 @@ static void rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
>  	run->decode_params = ctrl ? ctrl->p_cur.p : NULL;
>  	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
>  			      V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS);
> -	run->slices_params = ctrl ? ctrl->p_cur.p : NULL;
> -	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
> -			      V4L2_CID_MPEG_VIDEO_H264_SPS);

V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS line should be removed not SPS :-)

With that fixed,

Reviewed-by: Jonas Karlman <jonas@kwiboo.se>

Best regards,
Jonas

>  	run->sps = ctrl ? ctrl->p_cur.p : NULL;
>  	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
>  			      V4L2_CID_MPEG_VIDEO_H264_PPS);
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index accb4a902fdd..8ebc9dfc83be 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -59,11 +59,6 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>  		.mandatory = true,
>  		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
>  	},
> -	{
> -		.per_request = true,
> -		.mandatory = true,
> -		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
> -	},
>  	{
>  		.per_request = true,
>  		.mandatory = true,
> 

Re: [PATCH 06/10] media: uapi: h264: Cleanup DPB entry interface

[Jonas Karlman]

Hi,

On 2020-07-24 21:08, Ezequiel Garcia wrote:
> Hello Jonas,
> 
> On Wed, 2020-07-22 at 21:52 +0000, Jonas Karlman wrote:
>> On 2020-07-15 22:22, Ezequiel Garcia wrote:
>>> As discussed recently, the current interface for the
>>> Decoded Picture Buffer is not enough to properly
>>> support field coding.
>>>
>>> This commit introduces enough semantics to support
>>> frame and field coding, and to signal how DPB entries
>>> are "used for reference".
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>>  .../media/v4l/ext-ctrls-codec.rst             | 46 ++++++++++++-------
>>>  drivers/media/v4l2-core/v4l2-h264.c           |  4 +-
>>>  drivers/staging/media/rkvdec/rkvdec-h264.c    |  8 ++--
>>>  include/media/h264-ctrls.h                    |  8 +++-
>>>  4 files changed, 42 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> index dd8e5a2e8986..46d4c8c6ad47 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>      * - __s32
>>>        - ``bottom_field_order_cnt``
>>>        -
>>> +    * - enum :c:type:`v4l2_h264_dpb_reference`
>>> +      - ``reference``
>>> +      - Specifies how the DPB entry is referenced.
>>>      * - __u32
>>>        - ``flags``
>>>        - See :ref:`DPB Entry Flags <h264_dpb_flags>`
>>>  
>>> +.. c:type:: v4l2_h264_dpb_reference
>>> +
>>> +.. cssclass:: longtable
>>> +
>>> +.. flat-table::
>>> +    :header-rows:  0
>>> +    :stub-columns: 0
>>> +    :widths:       1 1 2
>>> +
>>> +    * - ``V4L2_H264_DPB_TOP_REF``
>>> +      - 0x1
>>> +      - The top field in field pair is used for
>>> +        short-term reference.
>>> +    * - ``V4L2_H264_DPB_BOTTOM_REF``
>>> +      - 0x2
>>> +      - The bottom field in field pair is used for
>>> +        short-term reference.
>>> +    * - ``V4L2_H264_DPB_FRAME_REF``
>>> +      - 0x3
>>> +      - The frame (or the top/bottom fields, if it's a field pair)
>>> +        is used for short-term reference.
>>> +
>>>  .. _h264_dpb_flags:
>>>  
>>>  ``DPB Entries Flags``
>>> @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>  
>>>      * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID``
>>>        - 0x00000001
>>> -      - The DPB entry is valid and should be considered
>>> +      - The DPB entry is valid (non-empty) and should be considered.
>>>      * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE``
>>>        - 0x00000002
>>> -      - The DPB entry is currently being used as a reference frame
>>> +      - The DPB entry is used for reference.
>>>      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
>>>        - 0x00000004
>>> -      - The DPB entry is a long term reference frame
>>> +      - The DPB entry is used for long-term reference.
>>>      * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD``
>>>        - 0x00000008
>>> -      - The DPB entry is a field reference, which means only one of the field
>>> -        will be used when decoding the new frame/field. When not set the DPB
>>> -        entry is a frame reference (both fields will be used). Note that this
>>> -        flag does not say anything about the number of fields contained in the
>>> -        reference frame, it just describes the one used to decode the new
>>> -        field/frame
>>> -    * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD``
>>> -      - 0x00000010
>>> -      - The DPB entry is a bottom field reference (only the bottom field of the
>>> -        reference frame is needed to decode the new frame/field). Only valid if
>>> -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When
>>> -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but
>>> -        V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the
>>> -        DPB entry is a top field reference
>>> +      - The DPB entry is a single field or a complementary field pair.
>>>  
>>>  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
>>>      Specifies the decoding mode to use. Currently exposes slice-based and
>>> diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
>>> index edf6225f0522..306a51683606 100644
>>> --- a/drivers/media/v4l2-core/v4l2-h264.c
>>> +++ b/drivers/media/v4l2-core/v4l2-h264.c
>>> @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
>>>  		else
>>>  			b->refs[i].frame_num = dpb[i].frame_num;
>>>  
>>> -		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
>>> +		if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF)
>>
>> This looks wrong, should probably use ==,
>>
>> dpb[i].reference == V4L2_H264_DPB_FRAME_REF
>>
>> else this would match any reference value.
>>
>>>  			pic_order_count = min(dpb[i].top_field_order_cnt,
>>>  					      dpb[i].bottom_field_order_cnt);
>>> -		else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
>>> +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
>>>  			pic_order_count = dpb[i].bottom_field_order_cnt;
>>>  		else
>>>  			pic_order_count = dpb[i].top_field_order_cnt;
>>> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>>> index 7b66e2743a4f..57539c630422 100644
>>> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>>> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>>> @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx,
>>>  			     RKVDEC_COLMV_USED_FLAG_REF;
>>>  
>>>  		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
>>> -			refer_addr |= RKVDEC_TOPFIELD_USED_REF |
>>> -				      RKVDEC_BOTFIELD_USED_REF;
>>> -		else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
>>> +			refer_addr |= RKVDEC_FIELD_REF;
>>> +
>>> +		if (dpb[i].reference & V4L2_H264_DPB_TOP_REF)
>>>  			refer_addr |= RKVDEC_BOTFIELD_USED_REF;
>>> -		else
>>> +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
>>
>> This should probably be if and not else if, and BOTFIELD/TOPFIELD_USED_REF
>> seems to be mixed up.
>>
>> I have only taken a quick look so far, I will update ffmpeg and runtime test
>> later this weekend, will get back with result and full review on Sunday evening.
>>
> 
> Thanks that would be useful.
> 
> However, keep in mind this series is specifically concerned
> with the uAPI review.
> 
> This is not supposed to fix the field coded support, or anything
> else in any driver.
> 
> IMO, at this stage, fixing drivers is somewhat lower priority
> than discussing and stabilizing the uAPI.

I have now tested rkvdec on a RK3328 device and needed to do 3 fixups, see [1].
Initial ffmpeg update using update h264 uapi is located at [2], the ffmpeg
update still needs to be tested with cedrus and hantro.

So far I have not seen any issue with the uapi changes.

Q: ffmpeg will not try to set SLICE_PARAMS or PRED_WEIGHT ctrls for
DECODE_MODE_SLICE_BASED, should userspace check if ctrl exists or is using
DECODE_MODE value okay?

I have also pushed an updated WIP branch at [3] containing high 10,
field encoded and hevc work.

[1] https://github.com/Kwiboo/linux-rockchip/compare/b6b91f27c0cb33520e954e7bb2550e0e07ed4d85...b82b6e93feb9ca44d2c677f25416cf6345f0114d
[2] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.3.1
[3] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-work-in-progress

Best regards,
Jonas

> 
> Thanks,
> Ezequiel
> 
> 

Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Maxime Ripard]

Hi,

On Mon, Jul 27, 2020 at 11:39:12AM -0300, Ezequiel Garcia wrote:
> On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote:
> > On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > The H.264 specification requires in its "Slice header semantics"
> > > section that the following values shall be the same in all slice headers:
> > > 
> > >   pic_parameter_set_id
> > >   frame_num
> > >   field_pic_flag
> > >   bottom_field_flag
> > >   idr_pic_id
> > >   pic_order_cnt_lsb
> > >   delta_pic_order_cnt_bottom
> > >   delta_pic_order_cnt[ 0 ]
> > >   delta_pic_order_cnt[ 1 ]
> > >   sp_for_switch_flag
> > >   slice_group_change_cycle
> > > 
> > > and can therefore be moved to the per-frame decode parameters control.
> > 
> > I am really not a H.264 expert, so this question may not be relevant,
> 
> All questions are welcome. I'm more than happy to discuss this patchset.
> 
> > but are these values specified for every slice header in the
> > bitstream, or are they specified only once per frame?
> > 
> > I am asking this because it would certainly make user-space code
> > simpler if we could remain as close to the bitstream as possible. If
> > these values are specified once per slice, then factorizing them would
> > leave user-space with the burden of deciding what to do if they change
> > across slices.
> > 
> > Note that this is a double-edged sword, because it is not necessarily
> > better to leave the firmware in charge of deciding what to do in such
> > a case. :) So hopefully these are only specified once per frame in the
> > bitstream, in which case your proposal makes complete sense.
> 
> Frame-based hardwares accelerators such as Hantro and Rockchip VDEC
> are doing the slice header parsing themselves. Therefore, the
> driver is not really parsing these fields on each slice header.
> 
> Currently, we are already using only the first slice in a frame,
> as you can see from:
> 
>         if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
>                 reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
> 
> Even if these fields are transported in the slice header,
> I think it makes sense for us to split them into the decode params
> (per-frame) control.

I don't really get it though. The initial plan that was asked was to
mimic as much as possible the bitstream and that's what we did.

But that requirement seems to have changed now?

Even if it did change though, if this is defined as a slice parameter in
the spec, why would we want to move it to some other control entirely if
we are to keep the slice parameters control?

Especially if the reason is to make the life easier on a couple of
drivers, that's really not the point of a userspace API, and we can
always add an helper if it really shows up as a pattern.

> They are really specified to be the same across all slices,
> so even I'd say if a bitstream violates this, it's likely
> either a corrupted bitstream or an encoder bug.

That doesn't look like something we should worry about though. Or at
least, this is true for pretty much any other field in the bitstream and
we won't change those.

Maxime

Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 6125 bytes --]

Le mardi 28 juillet 2020 à 14:44 +0200, Maxime Ripard a écrit :
> Hi,
> 
> On Mon, Jul 27, 2020 at 11:39:12AM -0300, Ezequiel Garcia wrote:
> > On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote:
> > > On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > The H.264 specification requires in its "Slice header semantics"
> > > > section that the following values shall be the same in all slice headers:
> > > > 
> > > >   pic_parameter_set_id
> > > >   frame_num
> > > >   field_pic_flag
> > > >   bottom_field_flag
> > > >   idr_pic_id
> > > >   pic_order_cnt_lsb
> > > >   delta_pic_order_cnt_bottom
> > > >   delta_pic_order_cnt[ 0 ]
> > > >   delta_pic_order_cnt[ 1 ]
> > > >   sp_for_switch_flag
> > > >   slice_group_change_cycle
> > > > 
> > > > and can therefore be moved to the per-frame decode parameters control.
> > > 
> > > I am really not a H.264 expert, so this question may not be relevant,
> > 
> > All questions are welcome. I'm more than happy to discuss this patchset.
> > 
> > > but are these values specified for every slice header in the
> > > bitstream, or are they specified only once per frame?
> > > 
> > > I am asking this because it would certainly make user-space code
> > > simpler if we could remain as close to the bitstream as possible. If
> > > these values are specified once per slice, then factorizing them would
> > > leave user-space with the burden of deciding what to do if they change
> > > across slices.
> > > 
> > > Note that this is a double-edged sword, because it is not necessarily
> > > better to leave the firmware in charge of deciding what to do in such
> > > a case. :) So hopefully these are only specified once per frame in the
> > > bitstream, in which case your proposal makes complete sense.
> > 
> > Frame-based hardwares accelerators such as Hantro and Rockchip VDEC
> > are doing the slice header parsing themselves. Therefore, the
> > driver is not really parsing these fields on each slice header.
> > 
> > Currently, we are already using only the first slice in a frame,
> > as you can see from:
> > 
> >         if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> >                 reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
> > 
> > Even if these fields are transported in the slice header,
> > I think it makes sense for us to split them into the decode params
> > (per-frame) control.
> 
> I don't really get it though. The initial plan that was asked was to
> mimic as much as possible the bitstream and that's what we did.
> 
> But that requirement seems to have changed now?

Indeed, that has changed and has been discussed multiple times already.
But in general what you need to realize is that the bitstream is made
of redundancy in order to be robust over data lost. Most importantly,
it is by design that you can loose a slice, but still the decode the
other slices.

Carrying this redundancy over the kernel API though didn't make much
sense. It only made the amount of data we have to pass (and copy) for
each frames bigger. The goal of this change is to reduce the amount of
data you actually need to pass, both for frame and slice decoding.

For frame decoding, all the invariants from the slice header (notice
the spec name here) have been moved from slice_params (not a spec term)
into the decode_params (also not a spec term). This way, the
slice_params are no longer needed to be passed at all to driver.

As for the slice decoding, assuming you care making use of the VB2
control caching, you can pass the decode_params on the first slice
only, and only pass the slice_params, which is now slimmer, for the
following slices.

Remember that Paul initially believed that V4L2 stateless decoder was a
stateless API, while in fact the HW is stateless, but V4L2 API are
statefull by nature.

p.s. it was also request to use raster scan scaling matrix, which isn't
the order found in the bitstream. That was also discussed, and the
reason is that other existing interfaces use raster order, so not using
that is error prone for both kernel and userspace coders. Cedrus has
been in raster scan order since the start, ignoring that rule already.

> 
> Even if it did change though, if this is defined as a slice parameter in
> the spec, why would we want to move it to some other control entirely if
> we are to keep the slice parameters control?

You are confusing a term we invented, slice_params, with a specified
term slice header. The decode_params are as document information per
frame/picture, while slice_params is information needed per slice,
nothing more.

> 
> Especially if the reason is to make the life easier on a couple of
> drivers, that's really not the point of a userspace API, and we can
> always add an helper if it really shows up as a pattern.

We have made userspace implementation of this, GStreamer for now, I can
only disagree with you. This does not make userspace much more complex
and on top of that, it allow reducing some overhead, as prior to that
we were computing reference lists for frame base decoder that already
compute this in HW. I also find that it make debugging easier, as when
we trace we don't endup looking at identical values over and over.

> 
> > They are really specified to be the same across all slices,
> > so even I'd say if a bitstream violates this, it's likely
> > either a corrupted bitstream or an encoder bug.
> 
> That doesn't look like something we should worry about though. Or at
> least, this is true for pretty much any other field in the bitstream and
> we won't change those.

Sorry, I'm not clear what you refer to, not worry about the fact they
are invariant or that the user may pass invalid data ? For the first,
we believe it matters, that was also motivated by some of Stanimir
research showing that controls are not as free as we'd like to think.
Again, all this have been discussed for quite some time now and the
participants have seen to reach an agreement on this direction.

> 
> Maxime

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 03/10] media: uapi: h264: Split prediction weight parameters

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

Le samedi 25 juillet 2020 à 22:30 +0900, Alexandre Courbot a écrit :
> On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > The prediction weight parameters are only required under
> > certain conditions, which depend on slice header parameters.
> > 
> > The slice header syntax specifies that the prediction
> > weight table is present if:
> > 
> > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \
> > (weighted_bipred_idc == 1 && slice_type == B))
> 
> This is a pretty important bit - how about mentioning in the documentation when
> this new control is expected to be present, so both drivers and
> userspace submit it
> or omit it in a consistent manner?

This is copy paste from the spec. We can add a reference to the syntax
chapter in the spec that express exactly this if condition (syntax is
express in pseudo code). The bitstream works exactly the same. Note
that it's not a fault to provide the control even if not needed,
drivers will kindly ignore it.

Reference: 7.3.3 Slice header syntax

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 06/10] media: uapi: h264: Cleanup DPB entry interface

[Ezequiel Garcia]

Hi Jonas,

On Mon, 2020-07-27 at 23:39 +0000, Jonas Karlman wrote:
> Hi,
> 
> On 2020-07-24 21:08, Ezequiel Garcia wrote:
> > Hello Jonas,
> > 
> > On Wed, 2020-07-22 at 21:52 +0000, Jonas Karlman wrote:
> > > On 2020-07-15 22:22, Ezequiel Garcia wrote:
> > > > As discussed recently, the current interface for the
> > > > Decoded Picture Buffer is not enough to properly
> > > > support field coding.
> > > > 
> > > > This commit introduces enough semantics to support
> > > > frame and field coding, and to signal how DPB entries
> > > > are "used for reference".
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > ---
> > > >  .../media/v4l/ext-ctrls-codec.rst             | 46 ++++++++++++-------
> > > >  drivers/media/v4l2-core/v4l2-h264.c           |  4 +-
> > > >  drivers/staging/media/rkvdec/rkvdec-h264.c    |  8 ++--
> > > >  include/media/h264-ctrls.h                    |  8 +++-
> > > >  4 files changed, 42 insertions(+), 24 deletions(-)
> > > > 
> > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > index dd8e5a2e8986..46d4c8c6ad47 100644
> > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > >      * - __s32
> > > >        - ``bottom_field_order_cnt``
> > > >        -
> > > > +    * - enum :c:type:`v4l2_h264_dpb_reference`
> > > > +      - ``reference``
> > > > +      - Specifies how the DPB entry is referenced.
> > > >      * - __u32
> > > >        - ``flags``
> > > >        - See :ref:`DPB Entry Flags <h264_dpb_flags>`
> > > >  
> > > > +.. c:type:: v4l2_h264_dpb_reference
> > > > +
> > > > +.. cssclass:: longtable
> > > > +
> > > > +.. flat-table::
> > > > +    :header-rows:  0
> > > > +    :stub-columns: 0
> > > > +    :widths:       1 1 2
> > > > +
> > > > +    * - ``V4L2_H264_DPB_TOP_REF``
> > > > +      - 0x1
> > > > +      - The top field in field pair is used for
> > > > +        short-term reference.
> > > > +    * - ``V4L2_H264_DPB_BOTTOM_REF``
> > > > +      - 0x2
> > > > +      - The bottom field in field pair is used for
> > > > +        short-term reference.
> > > > +    * - ``V4L2_H264_DPB_FRAME_REF``
> > > > +      - 0x3
> > > > +      - The frame (or the top/bottom fields, if it's a field pair)
> > > > +        is used for short-term reference.
> > > > +
> > > >  .. _h264_dpb_flags:
> > > >  
> > > >  ``DPB Entries Flags``
> > > > @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > >  
> > > >      * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID``
> > > >        - 0x00000001
> > > > -      - The DPB entry is valid and should be considered
> > > > +      - The DPB entry is valid (non-empty) and should be considered.
> > > >      * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE``
> > > >        - 0x00000002
> > > > -      - The DPB entry is currently being used as a reference frame
> > > > +      - The DPB entry is used for reference.
> > > >      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
> > > >        - 0x00000004
> > > > -      - The DPB entry is a long term reference frame
> > > > +      - The DPB entry is used for long-term reference.
> > > >      * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD``
> > > >        - 0x00000008
> > > > -      - The DPB entry is a field reference, which means only one of the field
> > > > -        will be used when decoding the new frame/field. When not set the DPB
> > > > -        entry is a frame reference (both fields will be used). Note that this
> > > > -        flag does not say anything about the number of fields contained in the
> > > > -        reference frame, it just describes the one used to decode the new
> > > > -        field/frame
> > > > -    * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD``
> > > > -      - 0x00000010
> > > > -      - The DPB entry is a bottom field reference (only the bottom field of the
> > > > -        reference frame is needed to decode the new frame/field). Only valid if
> > > > -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When
> > > > -        V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but
> > > > -        V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the
> > > > -        DPB entry is a top field reference
> > > > +      - The DPB entry is a single field or a complementary field pair.
> > > >  
> > > >  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
> > > >      Specifies the decoding mode to use. Currently exposes slice-based and
> > > > diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
> > > > index edf6225f0522..306a51683606 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-h264.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-h264.c
> > > > @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> > > >  		else
> > > >  			b->refs[i].frame_num = dpb[i].frame_num;
> > > >  
> > > > -		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
> > > > +		if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF)
> > > 
> > > This looks wrong, should probably use ==,
> > > 
> > > dpb[i].reference == V4L2_H264_DPB_FRAME_REF
> > > 
> > > else this would match any reference value.
> > > 
> > > >  			pic_order_count = min(dpb[i].top_field_order_cnt,
> > > >  					      dpb[i].bottom_field_order_cnt);
> > > > -		else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
> > > > +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
> > > >  			pic_order_count = dpb[i].bottom_field_order_cnt;
> > > >  		else
> > > >  			pic_order_count = dpb[i].top_field_order_cnt;
> > > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > > index 7b66e2743a4f..57539c630422 100644
> > > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > > @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx,
> > > >  			     RKVDEC_COLMV_USED_FLAG_REF;
> > > >  
> > > >  		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD))
> > > > -			refer_addr |= RKVDEC_TOPFIELD_USED_REF |
> > > > -				      RKVDEC_BOTFIELD_USED_REF;
> > > > -		else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD)
> > > > +			refer_addr |= RKVDEC_FIELD_REF;
> > > > +
> > > > +		if (dpb[i].reference & V4L2_H264_DPB_TOP_REF)
> > > >  			refer_addr |= RKVDEC_BOTFIELD_USED_REF;
> > > > -		else
> > > > +		else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF)
> > > 
> > > This should probably be if and not else if, and BOTFIELD/TOPFIELD_USED_REF
> > > seems to be mixed up.
> > > 
> > > I have only taken a quick look so far, I will update ffmpeg and runtime test
> > > later this weekend, will get back with result and full review on Sunday evening.
> > > 
> > 
> > Thanks that would be useful.
> > 
> > However, keep in mind this series is specifically concerned
> > with the uAPI review.
> > 
> > This is not supposed to fix the field coded support, or anything
> > else in any driver.
> > 
> > IMO, at this stage, fixing drivers is somewhat lower priority
> > than discussing and stabilizing the uAPI.
> 
> I have now tested rkvdec on a RK3328 device and needed to do 3 fixups, see [1].
> Initial ffmpeg update using update h264 uapi is located at [2], the ffmpeg
> update still needs to be tested with cedrus and hantro.
> 
> So far I have not seen any issue with the uapi changes.
> 

Great, thanks for the test.

> Q: ffmpeg will not try to set SLICE_PARAMS or PRED_WEIGHT ctrls for
> DECODE_MODE_SLICE_BASED,

You mean it will not try to set those controls for
DECODE_MODE_FRAME_BASED?

I think that's correct, as we've discussed multiple times,
frame-based drivers shouldn't need to use those controls,
by definition.

>  should userspace check if ctrl exists or is using
> DECODE_MODE value okay?
> 

If a driver supporting DECODE_MODE_SLICE_BASED doesn't 
support SLICE_PARAMS, that can probably be considered
a failure from the application side.

If it doesn't support PRED_WEIGHTS controls, then
strictly speaking it won't be able to decode slices
that have a prediction weight table, i.e.:

((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \
(weighted_bipred_idc == 1 && slice_type == B))

I doubt we'll ever counter such case, so probably
if the controls aren't supported, applications
can just safely fail (and e.g. fallback to software).

> I have also pushed an updated WIP branch at [3] containing high 10,
> field encoded and hevc work.
> 
> [1] https://github.com/Kwiboo/linux-rockchip/compare/b6b91f27c0cb33520e954e7bb2550e0e07ed4d85...b82b6e93feb9ca44d2c677f25416cf6345f0114d
> [2] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.3.1
> [3] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-work-in-progress
> 
> Best regards,
> Jonas
> 
> > Thanks,
> > Ezequiel
> > 
> > 

Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Tomasz Figa]

On Mon, Jul 27, 2020 at 9:43 PM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> Le lundi 27 juillet 2020 à 20:10 +0200, Tomasz Figa a écrit :
> > On Mon, Jul 27, 2020 at 6:18 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > On Mon, 2020-07-27 at 16:52 +0200, Tomasz Figa wrote:
> > > > On Mon, Jul 27, 2020 at 4:39 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > > Hi Alexandre,
> > > > >
> > > > > Thanks a lot for the review.
> > > > >
> > > > > On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote:
> > > > > > On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > > > > The H.264 specification requires in its "Slice header semantics"
> > > > > > > section that the following values shall be the same in all slice headers:
> > > > > > >
> > > > > > >   pic_parameter_set_id
> > > > > > >   frame_num
> > > > > > >   field_pic_flag
> > > > > > >   bottom_field_flag
> > > > > > >   idr_pic_id
> > > > > > >   pic_order_cnt_lsb
> > > > > > >   delta_pic_order_cnt_bottom
> > > > > > >   delta_pic_order_cnt[ 0 ]
> > > > > > >   delta_pic_order_cnt[ 1 ]
> > > > > > >   sp_for_switch_flag
> > > > > > >   slice_group_change_cycle
> > > > > > >
> > > > > > > and can therefore be moved to the per-frame decode parameters control.
> > > > > >
> > > > > > I am really not a H.264 expert, so this question may not be relevant,
> > > > >
> > > > > All questions are welcome. I'm more than happy to discuss this patchset.
> > > > >
> > > > > > but are these values specified for every slice header in the
> > > > > > bitstream, or are they specified only once per frame?
> > > > > >
> > > > > > I am asking this because it would certainly make user-space code
> > > > > > simpler if we could remain as close to the bitstream as possible. If
> > > > > > these values are specified once per slice, then factorizing them would
> > > > > > leave user-space with the burden of deciding what to do if they change
> > > > > > across slices.
> > > > > >
> > > > > > Note that this is a double-edged sword, because it is not necessarily
> > > > > > better to leave the firmware in charge of deciding what to do in such
> > > > > > a case. :) So hopefully these are only specified once per frame in the
> > > > > > bitstream, in which case your proposal makes complete sense.
> > > > >
> > > > > Frame-based hardwares accelerators such as Hantro and Rockchip VDEC
> > > > > are doing the slice header parsing themselves. Therefore, the
> > > > > driver is not really parsing these fields on each slice header.
> > > > >
> > > > > Currently, we are already using only the first slice in a frame,
> > > > > as you can see from:
> > > > >
> > > > >         if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> > > > >                 reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
> > > > >
> > > > > Even if these fields are transported in the slice header,
> > > > > I think it makes sense for us to split them into the decode params
> > > > > (per-frame) control.
> > > > >
> > > > > They are really specified to be the same across all slices,
> > > > > so even I'd say if a bitstream violates this, it's likely
> > > > > either a corrupted bitstream or an encoder bug.
> > > > >
> > > > > OTOH, one thing this makes me realize is that the slice params control
> > > > > is wrongly specified as an array.
> > > >
> > > > It is _not_.
> > > >
> > >
> > > We introduced the hold capture buffer specifically to support
> > > this without having a slice array.
> > >
> > > I don't think we have a plan to support this control properly
> > > as an array.
> > >
> > > If we decide to support the slice control as an array,
> > > we would have to implement a mechanism to specify the array size,
> > > which we currently don't have AFAIK.
> > >
> >
> > That wasn't the conclusion when we discussed this last time on IRC.
> > +Nicolas Dufresne
> >
> > Currently the 1-slice per buffer model is quite impractical:
> > 1) the maximum number of buffers is 32, which for some streams can be
> > less than needed to queue a single frame,
>
> To give more context, it seems the discussion was about being able to
> use slice decoder with a 1 poll() per frame model. Of course this will
> never be as efficient as when using a frame base decoder, but as
> current design, you can keep a list of pending request (each request is
> 1 slice/buffer), and simply use memory pressure to poll a mid point and
> dequeue the remaining. An example, yo have 8 pending request, and reach
> your memory limit:
>
>   [R1, R2, R3, R4, R5, R6, R7, R8]
>
> As requests are in order and behaves like memory fences, you can pick
> R6, and poll() that one. When R6 is ready, you can then dequeue R1 to
> R6 without blocking. In this context, a limit of 16 or 32 buffers seems
> fair, the optimization we can do in userspace is likely sufficient. So
> I'd like to drop problem 1) from our list.
>

Okay, I forgot about the ability to poll the requests. I guess this
solves a part of the problem indeed.

> > 2) even more system call overhead due to the need to repeat various
> > operations (e.g. qbuf/dqbuf) per-slice rather than per-frame,
> > 3) no way to do hardware batching for hardware which supports queuing
> > multiple slices at a time,
> > 4) waste of memory - one needs to allocate all the OUTPUT buffers
> > pessimistically to accommodate the biggest possible slice, while with
> > all-slices-per-frame 1 buffer could be just heuristically allocated to
> > be enough for the whole frame.
> >
> > These need to be carefully evaluated, with some proper testing done to
> > confirm whether they are really a problem or not.
>
> 2, 3 and 4 seems to match what the currently unimplemented API propose.
> You can mitigate 2) but having multiple slices per buffers. That came
> with a byte offset to we can program the HW as if it was separate slice
> buffers. But was limited to 16 buffers, likely a fair compromise.
>

Do we have some ideas on how these problems could be addressed in the
future? It would be unfortunate to freeze the current API just to
realize that it can't be made efficient anymore and yet another API
would have to be invented to redo things in an efficient way.

With the request polling method, I guess we could solve 2) by making
it possible to dequeue and enqueue multiple buffers at a time. It
could be achieved by introducing DQBUF/QBUF variants which operate on
an array of buffer indexes.

> 3) is about batching, in the only use case we know, the batching
> acceleration consist of programming the next operation on the
> completion IRQ. I already looked with the Cedrus developers if and how
> that was feasible, but we don't have a PoC yet. The optimization is
> about removing context switches between operations, which could prevent
> fully using the HW.

Right, but still, we have to check whether the API we're going to
stabilize wouldn't prevent implementing it in the future.

One idea is to solve it opportunistically. If there are already some
slices queued and not being processed by the hardware yet, queuing
more would just join them to the existing (staging) batch. When the
hardware finishes its current batch, the staging batch would be closed
and handed to the hardware for decoding.

>
> 4) is also well covered with being able to multiplex 1 buffer with
> multiple slices.

Note that with MMAP memory type it's not possible, because 1 buffer
can be only queued once. However, I guess that with DMABUF, one can
just allocate one large buffer and queue it at different V4L2 buffer
indexes with different .data_offset (or whatever we introduce for
proper, well-defined offset handling).

>
> To be fair, I understand why we'd like to drop this API, as none of the
> active developers here of slice decoder (cedrus) have time to engage in
> supporting this untested "optimization". It's not only about kernel
> support, but also requires userspace work. I also agree that it could
> be added later, as an extension. It could be done with 3 new controls,
> an array of slice_params and an array of slice start offset and the
> number of slices, or just one, introduce a new structure that have a
> slice_params structure embedded, num_slices and an array of
> slice_start_offset. I don't have preference myself, but I'm just
> illustrating that yes, we could drop the slice batching to avoid
> pushing untested APIs without scarifying our ability to decode a valid
> stream.

Sure, that makes sense, but as I mentioned above, there are problems
with the existing API and if we don't want to solve them right now, we
at least have to make sure that the problems can be solved later after
stabilizing it.

Best regards,
Tomasz

Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Ezequiel Garcia]

Hi Tomasz,

On Tue, 2020-08-04 at 15:35 +0200, Tomasz Figa wrote:
> On Mon, Jul 27, 2020 at 9:43 PM Nicolas Dufresne
> <nicolas.dufresne@collabora.com> wrote:
> > Le lundi 27 juillet 2020 à 20:10 +0200, Tomasz Figa a écrit :
> > > On Mon, Jul 27, 2020 at 6:18 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > On Mon, 2020-07-27 at 16:52 +0200, Tomasz Figa wrote:
> > > > > On Mon, Jul 27, 2020 at 4:39 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > > > Hi Alexandre,
> > > > > > 
> > > > > > Thanks a lot for the review.
> > > > > > 
> > > > > > On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote:
> > > > > > > On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > > > > > The H.264 specification requires in its "Slice header semantics"
> > > > > > > > section that the following values shall be the same in all slice headers:
> > > > > > > > 
> > > > > > > >   pic_parameter_set_id
> > > > > > > >   frame_num
> > > > > > > >   field_pic_flag
> > > > > > > >   bottom_field_flag
> > > > > > > >   idr_pic_id
> > > > > > > >   pic_order_cnt_lsb
> > > > > > > >   delta_pic_order_cnt_bottom
> > > > > > > >   delta_pic_order_cnt[ 0 ]
> > > > > > > >   delta_pic_order_cnt[ 1 ]
> > > > > > > >   sp_for_switch_flag
> > > > > > > >   slice_group_change_cycle
> > > > > > > > 
> > > > > > > > and can therefore be moved to the per-frame decode parameters control.
> > > > > > > 
> > > > > > > I am really not a H.264 expert, so this question may not be relevant,
> > > > > > 
> > > > > > All questions are welcome. I'm more than happy to discuss this patchset.
> > > > > > 
> > > > > > > but are these values specified for every slice header in the
> > > > > > > bitstream, or are they specified only once per frame?
> > > > > > > 
> > > > > > > I am asking this because it would certainly make user-space code
> > > > > > > simpler if we could remain as close to the bitstream as possible. If
> > > > > > > these values are specified once per slice, then factorizing them would
> > > > > > > leave user-space with the burden of deciding what to do if they change
> > > > > > > across slices.
> > > > > > > 
> > > > > > > Note that this is a double-edged sword, because it is not necessarily
> > > > > > > better to leave the firmware in charge of deciding what to do in such
> > > > > > > a case. :) So hopefully these are only specified once per frame in the
> > > > > > > bitstream, in which case your proposal makes complete sense.
> > > > > > 
> > > > > > Frame-based hardwares accelerators such as Hantro and Rockchip VDEC
> > > > > > are doing the slice header parsing themselves. Therefore, the
> > > > > > driver is not really parsing these fields on each slice header.
> > > > > > 
> > > > > > Currently, we are already using only the first slice in a frame,
> > > > > > as you can see from:
> > > > > > 
> > > > > >         if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> > > > > >                 reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
> > > > > > 
> > > > > > Even if these fields are transported in the slice header,
> > > > > > I think it makes sense for us to split them into the decode params
> > > > > > (per-frame) control.
> > > > > > 
> > > > > > They are really specified to be the same across all slices,
> > > > > > so even I'd say if a bitstream violates this, it's likely
> > > > > > either a corrupted bitstream or an encoder bug.
> > > > > > 
> > > > > > OTOH, one thing this makes me realize is that the slice params control
> > > > > > is wrongly specified as an array.
> > > > > 
> > > > > It is _not_.
> > > > > 
> > > > 
> > > > We introduced the hold capture buffer specifically to support
> > > > this without having a slice array.
> > > > 
> > > > I don't think we have a plan to support this control properly
> > > > as an array.
> > > > 
> > > > If we decide to support the slice control as an array,
> > > > we would have to implement a mechanism to specify the array size,
> > > > which we currently don't have AFAIK.
> > > > 
> > > 
> > > That wasn't the conclusion when we discussed this last time on IRC.
> > > +Nicolas Dufresne
> > > 
> > > Currently the 1-slice per buffer model is quite impractical:
> > > 1) the maximum number of buffers is 32, which for some streams can be
> > > less than needed to queue a single frame,
> > 
> > To give more context, it seems the discussion was about being able to
> > use slice decoder with a 1 poll() per frame model. Of course this will
> > never be as efficient as when using a frame base decoder, but as
> > current design, you can keep a list of pending request (each request is
> > 1 slice/buffer), and simply use memory pressure to poll a mid point and
> > dequeue the remaining. An example, yo have 8 pending request, and reach
> > your memory limit:
> > 
> >   [R1, R2, R3, R4, R5, R6, R7, R8]
> > 
> > As requests are in order and behaves like memory fences, you can pick
> > R6, and poll() that one. When R6 is ready, you can then dequeue R1 to
> > R6 without blocking. In this context, a limit of 16 or 32 buffers seems
> > fair, the optimization we can do in userspace is likely sufficient. So
> > I'd like to drop problem 1) from our list.
> > 
> 
> Okay, I forgot about the ability to poll the requests. I guess this
> solves a part of the problem indeed.
> 
> > > 2) even more system call overhead due to the need to repeat various
> > > operations (e.g. qbuf/dqbuf) per-slice rather than per-frame,
> > > 3) no way to do hardware batching for hardware which supports queuing
> > > multiple slices at a time,
> > > 4) waste of memory - one needs to allocate all the OUTPUT buffers
> > > pessimistically to accommodate the biggest possible slice, while with
> > > all-slices-per-frame 1 buffer could be just heuristically allocated to
> > > be enough for the whole frame.
> > > 
> > > These need to be carefully evaluated, with some proper testing done to
> > > confirm whether they are really a problem or not.
> > 
> > 2, 3 and 4 seems to match what the currently unimplemented API propose.
> > You can mitigate 2) but having multiple slices per buffers. That came
> > with a byte offset to we can program the HW as if it was separate slice
> > buffers. But was limited to 16 buffers, likely a fair compromise.
> > 
> 
> Do we have some ideas on how these problems could be addressed in the
> future? It would be unfortunate to freeze the current API just to
> realize that it can't be made efficient anymore and yet another API
> would have to be invented to redo things in an efficient way.
> 
> With the request polling method, I guess we could solve 2) by making
> it possible to dequeue and enqueue multiple buffers at a time. It
> could be achieved by introducing DQBUF/QBUF variants which operate on
> an array of buffer indexes.
> 
> > 3) is about batching, in the only use case we know, the batching
> > acceleration consist of programming the next operation on the
> > completion IRQ. I already looked with the Cedrus developers if and how
> > that was feasible, but we don't have a PoC yet. The optimization is
> > about removing context switches between operations, which could prevent
> > fully using the HW.
> 
> Right, but still, we have to check whether the API we're going to
> stabilize wouldn't prevent implementing it in the future.
> 
> One idea is to solve it opportunistically. If there are already some
> slices queued and not being processed by the hardware yet, queuing
> more would just join them to the existing (staging) batch. When the
> hardware finishes its current batch, the staging batch would be closed
> and handed to the hardware for decoding.
> 
> > 4) is also well covered with being able to multiplex 1 buffer with
> > multiple slices.
> 
> Note that with MMAP memory type it's not possible, because 1 buffer
> can be only queued once. However, I guess that with DMABUF, one can
> just allocate one large buffer and queue it at different V4L2 buffer
> indexes with different .data_offset (or whatever we introduce for
> proper, well-defined offset handling).
> 
> > To be fair, I understand why we'd like to drop this API, as none of the
> > active developers here of slice decoder (cedrus) have time to engage in
> > supporting this untested "optimization". It's not only about kernel
> > support, but also requires userspace work. I also agree that it could
> > be added later, as an extension. It could be done with 3 new controls,
> > an array of slice_params and an array of slice start offset and the
> > number of slices, or just one, introduce a new structure that have a
> > slice_params structure embedded, num_slices and an array of
> > slice_start_offset. I don't have preference myself, but I'm just
> > illustrating that yes, we could drop the slice batching to avoid
> > pushing untested APIs without scarifying our ability to decode a valid
> > stream.
> 
> Sure, that makes sense, but as I mentioned above, there are problems
> with the existing API and if we don't want to solve them right now, we
> at least have to make sure that the problems can be solved later after
> stabilizing it.
> 

I think the plan is to support 1-slice per request using the current
SLICE_PARAMS control.

So, next iteration of this series should clarify that the SLICE_PARAMS
is not intended as an array, and clean the fields that were added
when the control was first envisioned as part of an array.

As you have pointed out, for slice-based hardware, pushing
1-slice per request can be suboptimal.

However, this is the current mode that is supported by Cedrus,
which is good enough for Cedrus users. I believe there's enough
reasons to stabilize this SLICE_PARAMS and clarify the use-case
is for 1-slice per request.

Nothing prevents to introduce a new control to support another mode,
though. So, going forward, if we see slice-based hardware that can
handle multiple slices per request, or, if we want to pass an entire
frame to Cedrus to save the syscall overhead, we will have to introduce
a new control (array-like).

I believe a decent plan is along Nicolas' suggestions.
I'd like to quote that because it totally matches what I had in mind:

"""
It could be done with 3 new controls,
an array of slice_params and an array of slice start offset and the
number of slices, or just one, introduce a new structure that have a
slice_params structure embedded, num_slices and an array of
slice_start_offset.
"""

Currently, the length of control arrays are fixed and the
kernel rejects applications from passing a size that
differs from the size of the array.
 
Before proposing N-slices per request, we would have to introduce
new V4L2 control semantics, to be able to pass a dynamic array
of controls. This has been in our roadmap, so it'll happen
sooner or later.

Thanks!
Ezequiel