[PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content

[PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content

[Jernej Skrabec]

Currently H264 interlaced content it's not properly decoded on Cedrus.
There are two reasons for this:
1. slice parameters control doesn't provide enough information
2. bug in frame list construction in Cedrus driver

As described in commit message in patch 1, references stored in
reference lists should tell if reference targets top or bottom field.
However, this information is currently not provided. Patch 1 adds
it in form of flags which are set for each reference. Patch 2 then
uses those flags in Cedrus driver.

Frame list construction is fixed in patch 3.

This solution was extensively tested using Kodi on LibreELEC with A64,
H3, H5 and H6 SoCs in slightly different form (flags were transmitted
in MSB bits in index).

Note: I'm not 100% sure if flags for both, top and bottom fields are
needed. Any input here would be welcome.

Please take a look.

Best regards,
Jernej

Jernej Skrabec (3):
  media: uapi: h264: update reference lists
  media: cedrus: h264: Properly configure reference field
  media: cedrus: h264: Fix frame list construction

 .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
 .../staging/media/sunxi/cedrus/cedrus_h264.c  | 27 +++++++------
 include/media/h264-ctrls.h                    | 12 +++++-
 3 files changed, 62 insertions(+), 17 deletions(-)

-- 
2.27.0

[PATCH 1/3] media: uapi: h264: update reference lists

[Jernej Skrabec]

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>
---
 .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
 .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
 include/media/h264-ctrls.h                    | 12 +++++-
 3 files changed, 51 insertions(+), 7 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..6c36d298db20 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``
+      -
+
+.. _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..9b1cbc9bc38e 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -140,6 +140,14 @@ 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;
+	__u8 index;
+};
+
 struct v4l2_ctrl_h264_slice_params {
 	/* Size in bytes, including header */
 	__u32 size;
@@ -182,8 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
 	 * 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[32];
+	struct v4l2_h264_reference ref_pic_list1[32];
 
 	__u32 flags;
 };
-- 
2.27.0

[PATCH 2/3] media: cedrus: h264: Properly configure reference field

[Jernej Skrabec]

When interlaced H264 content is being decoded, references must indicate
which field is being referenced. Currently this was done by checking
capture buffer flags. However, that is not correct because capture
buffer may hold both fields.

Fix this by checking newly introduced flags in reference lists.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index cce527bbdf86..c87717d17ec5 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -183,7 +183,6 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
 	for (i = 0; i < num_ref; i++) {
 		const struct v4l2_h264_dpb_entry *dpb;
 		const struct cedrus_buffer *cedrus_buf;
-		const struct vb2_v4l2_buffer *ref_buf;
 		unsigned int position;
 		int buf_idx;
 		u8 dpb_idx;
@@ -198,12 +197,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
 		if (buf_idx < 0)
 			continue;
 
-		ref_buf = to_vb2_v4l2_buffer(cap_q->bufs[buf_idx]);
-		cedrus_buf = vb2_v4l2_to_cedrus_buffer(ref_buf);
+		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
 		position = cedrus_buf->codec.h264.position;
 
 		sram_array[i] |= position << 1;
-		if (ref_buf->field == V4L2_FIELD_BOTTOM)
+		if (ref_list[i].flags & V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD)
 			sram_array[i] |= BIT(0);
 	}
 
-- 
2.27.0

[PATCH 3/3] media: cedrus: h264: Fix frame list construction

[Jernej Skrabec]

Current frame list construction algorithm assumes that decoded image
will be output into its own buffer. That is true for progressive content
but not for interlaced where each field is decoded separately into same
buffer.

Fix that by checking if capture buffer is listed in DPB. If it is, reuse
it.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index c87717d17ec5..4f79386315ae 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -102,7 +102,7 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
 	struct cedrus_dev *dev = ctx->dev;
 	unsigned long used_dpbs = 0;
 	unsigned int position;
-	unsigned int output = 0;
+	int output = -1;
 	unsigned int i;
 
 	cap_q = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
@@ -125,6 +125,11 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
 		position = cedrus_buf->codec.h264.position;
 		used_dpbs |= BIT(position);
 
+		if (run->dst->vb2_buf.timestamp == dpb->reference_ts) {
+			output = position;
+			continue;
+		}
+
 		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
 			continue;
 
@@ -132,13 +137,11 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
 				    dpb->top_field_order_cnt,
 				    dpb->bottom_field_order_cnt,
 				    &pic_list[position]);
-
-		output = max(position, output);
 	}
 
-	position = find_next_zero_bit(&used_dpbs, CEDRUS_H264_FRAME_NUM,
-				      output);
-	if (position >= CEDRUS_H264_FRAME_NUM)
+	if (output >= 0)
+		position = output;
+	else
 		position = find_first_zero_bit(&used_dpbs, CEDRUS_H264_FRAME_NUM);
 
 	output_buf = vb2_to_cedrus_buffer(&run->dst->vb2_buf);
-- 
2.27.0

Re: [PATCH 1/3] media: uapi: h264: update reference lists

[Nicolas Dufresne]

Le jeudi 04 juin 2020 à 20:57 +0200, Jernej Skrabec a écrit :
> 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>

This looks like the right approach to me and is extensible if anything
else is needed for MVC and SVC special referencing (at least will be
enough for what H.264 actually supports in this regard).

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
>  include/media/h264-ctrls.h                    | 12 +++++-
>  3 files changed, 51 insertions(+), 7 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..6c36d298db20 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``
> +      -
> +
> +.. _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..9b1cbc9bc38e 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -140,6 +140,14 @@ 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;
> +	__u8 index;
> +};
> +
>  struct v4l2_ctrl_h264_slice_params {
>  	/* Size in bytes, including header */
>  	__u32 size;
> @@ -182,8 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
>  	 * 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[32];
> +	struct v4l2_h264_reference ref_pic_list1[32];
>  
>  	__u32 flags;
>  };

Re: [PATCH 1/3] media: uapi: h264: update reference lists

[Nicolas Dufresne]

Sorry, missed one thing.

Le vendredi 05 juin 2020 à 13:08 -0400, Nicolas Dufresne a écrit :
> Le jeudi 04 juin 2020 à 20:57 +0200, Jernej Skrabec a écrit :
> > 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>
> 
> This looks like the right approach to me and is extensible if anything
> else is needed for MVC and SVC special referencing (at least will be
> enough for what H.264 actually supports in this regard).
> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> > ---
> >  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
> >  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
> >  include/media/h264-ctrls.h                    | 12 +++++-
> >  3 files changed, 51 insertions(+), 7 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..6c36d298db20 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``
> > +      -
> > +
> > +.. _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..9b1cbc9bc38e 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/media/h264-ctrls.h
> > @@ -140,6 +140,14 @@ 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;
> > +	__u8 index;
> > +};
> > +
> >  struct v4l2_ctrl_h264_slice_params {
> >  	/* Size in bytes, including header */
> >  	__u32 size;
> > @@ -182,8 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
> >  	 * Entries on each list are indices into
> >  	 * v4l2_ctrl_h264_decode_params.dpb[].
> >  	 */

This comment needs to be updated or moved inside the structure.

> > -	__u8 ref_pic_list0[32];
> > -	__u8 ref_pic_list1[32];
> > +	struct v4l2_h264_reference ref_pic_list0[32];
> > +	struct v4l2_h264_reference ref_pic_list1[32];
> >  
> >  	__u32 flags;
> >  };

Re: [PATCH 2/3] media: cedrus: h264: Properly configure reference field

[Nicolas Dufresne]

Le jeudi 04 juin 2020 à 20:57 +0200, Jernej Skrabec a écrit :
> When interlaced H264 content is being decoded, references must indicate
> which field is being referenced. Currently this was done by checking
> capture buffer flags. However, that is not correct because capture
> buffer may hold both fields.
> 
> Fix this by checking newly introduced flags in reference lists.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Perhaps an additional patch could cleanup the miss-leading comment in
v4l2_h264_dpb_entry definition.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index cce527bbdf86..c87717d17ec5 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -183,7 +183,6 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  	for (i = 0; i < num_ref; i++) {
>  		const struct v4l2_h264_dpb_entry *dpb;
>  		const struct cedrus_buffer *cedrus_buf;
> -		const struct vb2_v4l2_buffer *ref_buf;
>  		unsigned int position;
>  		int buf_idx;
>  		u8 dpb_idx;
> @@ -198,12 +197,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  		if (buf_idx < 0)
>  			continue;
>  
> -		ref_buf = to_vb2_v4l2_buffer(cap_q->bufs[buf_idx]);
> -		cedrus_buf = vb2_v4l2_to_cedrus_buffer(ref_buf);
> +		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
>  		position = cedrus_buf->codec.h264.position;
>  
>  		sram_array[i] |= position << 1;
> -		if (ref_buf->field == V4L2_FIELD_BOTTOM)
> +		if (ref_list[i].flags & V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD)
>  			sram_array[i] |= BIT(0);
>  	}
>  

Re: [PATCH 2/3] media: cedrus: h264: Properly configure reference field

[Jernej Škrabec]

Dne petek, 05. junij 2020 ob 19:16:35 CEST je Nicolas Dufresne napisal(a):
> Le jeudi 04 juin 2020 à 20:57 +0200, Jernej Skrabec a écrit :
> > When interlaced H264 content is being decoded, references must indicate
> > which field is being referenced. Currently this was done by checking
> > capture buffer flags. However, that is not correct because capture
> > buffer may hold both fields.
> > 
> > Fix this by checking newly introduced flags in reference lists.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> Perhaps an additional patch could cleanup the miss-leading comment in
> v4l2_h264_dpb_entry definition.

I missed that. I think this change actually belongs to patch 1. I'll fix it in 
v2.

Best regards,
Jernej

> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> > ---
> > 
> >  drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > cce527bbdf86..c87717d17ec5 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > @@ -183,7 +183,6 @@ static void _cedrus_write_ref_list(struct cedrus_ctx
> > *ctx,> 
> >  	for (i = 0; i < num_ref; i++) {
> >  	
> >  		const struct v4l2_h264_dpb_entry *dpb;
> >  		const struct cedrus_buffer *cedrus_buf;
> > 
> > -		const struct vb2_v4l2_buffer *ref_buf;
> > 
> >  		unsigned int position;
> >  		int buf_idx;
> >  		u8 dpb_idx;
> > 
> > @@ -198,12 +197,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx
> > *ctx,> 
> >  		if (buf_idx < 0)
> >  		
> >  			continue;
> > 
> > -		ref_buf = to_vb2_v4l2_buffer(cap_q->bufs[buf_idx]);
> > -		cedrus_buf = vb2_v4l2_to_cedrus_buffer(ref_buf);
> > +		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > 
> >  		position = cedrus_buf->codec.h264.position;
> >  		
> >  		sram_array[i] |= position << 1;
> > 
> > -		if (ref_buf->field == V4L2_FIELD_BOTTOM)
> > +		if (ref_list[i].flags & 
V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD)
> > 
> >  			sram_array[i] |= BIT(0);
> >  	
> >  	}

Re: [PATCH 1/3] media: uapi: h264: update reference lists

[Jernej Škrabec]

Dne petek, 05. junij 2020 ob 19:13:24 CEST je Nicolas Dufresne napisal(a):
> Sorry, missed one thing.
> 
> Le vendredi 05 juin 2020 à 13:08 -0400, Nicolas Dufresne a écrit :
> > Le jeudi 04 juin 2020 à 20:57 +0200, Jernej Skrabec a écrit :
> > > 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>
> > 
> > This looks like the right approach to me and is extensible if anything
> > else is needed for MVC and SVC special referencing (at least will be
> > enough for what H.264 actually supports in this regard).
> > 
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > > ---
> > > 
> > >  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
> > >  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
> > >  include/media/h264-ctrls.h                    | 12 +++++-
> > >  3 files changed, 51 insertions(+), 7 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..6c36d298db20 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``
> > > +      -
> > > +
> > > +.. _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..9b1cbc9bc38e 100644
> > > --- a/include/media/h264-ctrls.h
> > > +++ b/include/media/h264-ctrls.h
> > > @@ -140,6 +140,14 @@ 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;
> > > +	__u8 index;
> > > +};
> > > +
> > > 
> > >  struct v4l2_ctrl_h264_slice_params {
> > >  
> > >  	/* Size in bytes, including header */
> > >  	__u32 size;
> > > 
> > > @@ -182,8 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
> > > 
> > >  	 * Entries on each list are indices into
> > >  	 * v4l2_ctrl_h264_decode_params.dpb[].
> > >  	 */
> 
> This comment needs to be updated or moved inside the structure.

I'll move it in v2.

Best regards,
Jernej

> 
> > > -	__u8 ref_pic_list0[32];
> > > -	__u8 ref_pic_list1[32];
> > > +	struct v4l2_h264_reference ref_pic_list0[32];
> > > +	struct v4l2_h264_reference ref_pic_list1[32];
> > > 
> > >  	__u32 flags;
> > >  
> > >  };

Re: [PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content

[Ezequiel Garcia]

Hi Jernej,

On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
>
> Currently H264 interlaced content it's not properly decoded on Cedrus.
> There are two reasons for this:
> 1. slice parameters control doesn't provide enough information
> 2. bug in frame list construction in Cedrus driver
>
> As described in commit message in patch 1, references stored in
> reference lists should tell if reference targets top or bottom field.
> However, this information is currently not provided. Patch 1 adds
> it in form of flags which are set for each reference. Patch 2 then
> uses those flags in Cedrus driver.
>
> Frame list construction is fixed in patch 3.
>
> This solution was extensively tested using Kodi on LibreELEC with A64,
> H3, H5 and H6 SoCs in slightly different form (flags were transmitted
> in MSB bits in index).
>

So, if I understand correctly the field needs to be passed per-reference,
and the current per-DPB entry is not good?

If you could point at the userspace code for this, it would be interesting
to take a look.

> Note: I'm not 100% sure if flags for both, top and bottom fields are
> needed. Any input here would be welcome.
>

Given enum v4l2_field is already part of the uAPI, perhaps it makes
sense to just reuse that for the field type? Maybe it's an overkill,
but it would make sense to reuse the concepts and types that
already exist.

We can still add a reserved field to make this new reference type
extensive.

Thanks,
Ezequiel


> Please take a look.
>
> Best regards,
> Jernej
>
> Jernej Skrabec (3):
>   media: uapi: h264: update reference lists
>   media: cedrus: h264: Properly configure reference field
>   media: cedrus: h264: Fix frame list construction
>
>  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 27 +++++++------
>  include/media/h264-ctrls.h                    | 12 +++++-
>  3 files changed, 62 insertions(+), 17 deletions(-)
>
> --
> 2.27.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content

[Nicolas Dufresne]

Le samedi 06 juin 2020 à 09:46 -0300, Ezequiel Garcia a écrit :
> Hi Jernej,
> 
> On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> > Currently H264 interlaced content it's not properly decoded on Cedrus.
> > There are two reasons for this:
> > 1. slice parameters control doesn't provide enough information
> > 2. bug in frame list construction in Cedrus driver
> > 
> > As described in commit message in patch 1, references stored in
> > reference lists should tell if reference targets top or bottom field.
> > However, this information is currently not provided. Patch 1 adds
> > it in form of flags which are set for each reference. Patch 2 then
> > uses those flags in Cedrus driver.
> > 
> > Frame list construction is fixed in patch 3.
> > 
> > This solution was extensively tested using Kodi on LibreELEC with A64,
> > H3, H5 and H6 SoCs in slightly different form (flags were transmitted
> > in MSB bits in index).
> > 
> 
> So, if I understand correctly the field needs to be passed per-reference,
> and the current per-DPB entry is not good?

For interlaced content we reference fields separately. That's the
reason there is 32 entries in l0/l1. Though, as we decode both fields
in the same buffer (interleaved), the DPB indice is not sufficient to
inform the decoder what we are referencing. Understand that a top field
can be used to decode the next bottom field. This even make sense as
they are closer on the capture timeline. This covers slice based
decoders.

The flags to indicate presence of top/bottom fields in DPB array seems
only useful to frame base decoders. This is so that decoder can avoid
using lost fields when constructing it's own l0/l1 internally.

> 
> If you could point at the userspace code for this, it would be interesting
> to take a look.
> 
> > Note: I'm not 100% sure if flags for both, top and bottom fields are
> > needed. Any input here would be welcome.
> > 
> 
> Given enum v4l2_field is already part of the uAPI, perhaps it makes
> sense to just reuse that for the field type? Maybe it's an overkill,
> but it would make sense to reuse the concepts and types that
> already exist.
> 
> We can still add a reserved field to make this new reference type
> extensive.

I think it's fine to create new flag for this, as your solution would
require extending a reference to 24bit (this patch extend to 16bits).
Though indeed, we could combine frame and TOP and reserve more bits for
future use.

> 
> Thanks,
> Ezequiel
> 
> 
> > Please take a look.
> > 
> > Best regards,
> > Jernej
> > 
> > Jernej Skrabec (3):
> >   media: uapi: h264: update reference lists
> >   media: cedrus: h264: Properly configure reference field
> >   media: cedrus: h264: Fix frame list construction
> > 
> >  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
> >  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 27 +++++++------
> >  include/media/h264-ctrls.h                    | 12 +++++-
> >  3 files changed, 62 insertions(+), 17 deletions(-)
> > 
> > --
> > 2.27.0
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content

[Nicolas Dufresne]

Le dimanche 07 juin 2020 à 16:21 -0400, Nicolas Dufresne a écrit :
> Le samedi 06 juin 2020 à 09:46 -0300, Ezequiel Garcia a écrit :
> > Hi Jernej,
> > 
> > On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> > > Currently H264 interlaced content it's not properly decoded on Cedrus.
> > > There are two reasons for this:
> > > 1. slice parameters control doesn't provide enough information
> > > 2. bug in frame list construction in Cedrus driver
> > > 
> > > As described in commit message in patch 1, references stored in
> > > reference lists should tell if reference targets top or bottom field.
> > > However, this information is currently not provided. Patch 1 adds
> > > it in form of flags which are set for each reference. Patch 2 then
> > > uses those flags in Cedrus driver.
> > > 
> > > Frame list construction is fixed in patch 3.
> > > 
> > > This solution was extensively tested using Kodi on LibreELEC with A64,
> > > H3, H5 and H6 SoCs in slightly different form (flags were transmitted
> > > in MSB bits in index).
> > > 
> > 
> > So, if I understand correctly the field needs to be passed per-reference,
> > and the current per-DPB entry is not good?
> 
> For interlaced content we reference fields separately. That's the
> reason there is 32 entries in l0/l1. Though, as we decode both fields
> in the same buffer (interleaved), the DPB indice is not sufficient to
> inform the decoder what we are referencing. Understand that a top field
> can be used to decode the next bottom field. This even make sense as
> they are closer on the capture timeline. This covers slice based
> decoders.
> 
> The flags to indicate presence of top/bottom fields in DPB array seems
> only useful to frame base decoders. This is so that decoder can avoid
> using lost fields when constructing it's own l0/l1 internally.
> 
> > If you could point at the userspace code for this, it would be interesting
> > to take a look.
> > 
> > > Note: I'm not 100% sure if flags for both, top and bottom fields are
> > > needed. Any input here would be welcome.
> > > 
> > 
> > Given enum v4l2_field is already part of the uAPI, perhaps it makes
> > sense to just reuse that for the field type? Maybe it's an overkill,
> > but it would make sense to reuse the concepts and types that
> > already exist.
> > 
> > We can still add a reserved field to make this new reference type
> > extensive.
> 
> I think it's fine to create new flag for this, as your solution would
> require extending a reference to 24bit (this patch extend to 16bits).
> Though indeed, we could combine frame and TOP and reserve more bits for
> future use.

Sorry for the oversight, the flags is 16bits, so we already use 24bits.
But looking at "enum v4l2_field", which is not a flag, seems like a
miss-fit. It would create such a confusion, as V4L2_FIELD_SEQ_TB/BT can
still be used with this interface, even though we still need to say if
we reference TOP or BOTTOM. Only V4L2_FIELD_ALTERNATE is not supported.
But as you can see, "enum v4l2_fiel" is really meant to describe the
layout of the interleaved frame rather then signalling a field
directly.

> 
> > Thanks,
> > Ezequiel
> > 
> > 
> > > Please take a look.
> > > 
> > > Best regards,
> > > Jernej
> > > 
> > > Jernej Skrabec (3):
> > >   media: uapi: h264: update reference lists
> > >   media: cedrus: h264: Properly configure reference field
> > >   media: cedrus: h264: Fix frame list construction
> > > 
> > >  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
> > >  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 27 +++++++------
> > >  include/media/h264-ctrls.h                    | 12 +++++-
> > >  3 files changed, 62 insertions(+), 17 deletions(-)
> > > 
> > > --
> > > 2.27.0
> > > 
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] media: uapi: h264: update reference lists

[Ezequiel Garcia]

Hello Jernej,

I'd like to post a new H264 uAPI cleanup series soon,
would you mind resending this, or otherwise do you
mind if I include this patch in the series?

See below for a tiny comment.

On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
>
> 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>
> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
>  include/media/h264-ctrls.h                    | 12 +++++-
>  3 files changed, 51 insertions(+), 7 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..6c36d298db20 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``
> +      -
> +
> +.. _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..9b1cbc9bc38e 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -140,6 +140,14 @@ 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;
> +       __u8 index;
> +};
> +
>  struct v4l2_ctrl_h264_slice_params {
>         /* Size in bytes, including header */
>         __u32 size;
> @@ -182,8 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
>          * 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[32];
> +       struct v4l2_h264_reference ref_pic_list1[32];
>

Could we use a macro for "32" here? Something like:

#define V4L2_H264_REF_PIC_LIST_LEN (V4L2_H264_NUM_DPB_ENTRIES * 2).

Does it make sense to add a comment as well?

I was thinking something along these lines:

"""
Pictures in the DPB can be a frame, a complementary field pair or a
single field.
Therefore, reference pictures lists need twice as much entries, so it
can reference
either field of a field pair.
"""

While it doesn't replace proper H264 specification reading,
it would add some clarity.

Thanks,
Ezequiel

Re: [PATCH 1/3] media: uapi: h264: update reference lists

[Jernej Škrabec]

Hi!

Dne sreda, 08. julij 2020 ob 15:28:52 CEST je Ezequiel Garcia napisal(a):
> Hello Jernej,
> 
> I'd like to post a new H264 uAPI cleanup series soon,
> would you mind resending this, or otherwise do you
> mind if I include this patch in the series?

I don't mind at all. Currently my focus was elsewhere...

> 
> See below for a tiny comment.
> 
> On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> > 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>
> > ---
> > 
> >  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
> >  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
> >  include/media/h264-ctrls.h                    | 12 +++++-
> >  3 files changed, 51 insertions(+), 7 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..6c36d298db20 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``
> > +      -
> > +
> > +.. _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..9b1cbc9bc38e 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/media/h264-ctrls.h
> > @@ -140,6 +140,14 @@ 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;
> > +       __u8 index;
> > +};
> > +
> > 
> >  struct v4l2_ctrl_h264_slice_params {
> >  
> >         /* Size in bytes, including header */
> >         __u32 size;
> > 
> > @@ -182,8 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
> > 
> >          * 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[32];
> > +       struct v4l2_h264_reference ref_pic_list1[32];
> 
> Could we use a macro for "32" here? Something like:
> 
> #define V4L2_H264_REF_PIC_LIST_LEN (V4L2_H264_NUM_DPB_ENTRIES * 2).
> 
> Does it make sense to add a comment as well?
> 
> I was thinking something along these lines:
> 
> """
> Pictures in the DPB can be a frame, a complementary field pair or a
> single field.

To be honest, I don't know if user has a free choice to select same or 
different destination (capture) buffer for another field. I never tested it and 
I'm not sure how to test it with ffmpeg. HW deinterlacing cores on Allwinner 
SoCs support only interleaved fields as a input, that's why I never though 
about separate fields.

Best regards,
Jernej

> Therefore, reference pictures lists need twice as much entries, so it
> can reference
> either field of a field pair.
> """
> 
> While it doesn't replace proper H264 specification reading,
> it would add some clarity.
> 
> Thanks,
> Ezequiel

Re: [PATCH 1/3] media: uapi: h264: update reference lists

[Nicolas Dufresne]

Le mercredi 08 juillet 2020 à 17:57 +0200, Jernej Škrabec a écrit :
> Hi!
> 
> Dne sreda, 08. julij 2020 ob 15:28:52 CEST je Ezequiel Garcia napisal(a):
> > Hello Jernej,
> > 
> > I'd like to post a new H264 uAPI cleanup series soon,
> > would you mind resending this, or otherwise do you
> > mind if I include this patch in the series?
> 
> I don't mind at all. Currently my focus was elsewhere...
> 
> > See below for a tiny comment.
> > 
> > On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> > > 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>
> > > ---
> > > 
> > >  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
> > >  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
> > >  include/media/h264-ctrls.h                    | 12 +++++-
> > >  3 files changed, 51 insertions(+), 7 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..6c36d298db20 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``
> > > +      -
> > > +
> > > +.. _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..9b1cbc9bc38e 100644
> > > --- a/include/media/h264-ctrls.h
> > > +++ b/include/media/h264-ctrls.h
> > > @@ -140,6 +140,14 @@ 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;
> > > +       __u8 index;
> > > +};
> > > +
> > > 
> > >  struct v4l2_ctrl_h264_slice_params {
> > >  
> > >         /* Size in bytes, including header */
> > >         __u32 size;
> > > 
> > > @@ -182,8 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
> > > 
> > >          * 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[32];
> > > +       struct v4l2_h264_reference ref_pic_list1[32];
> > 
> > Could we use a macro for "32" here? Something like:
> > 
> > #define V4L2_H264_REF_PIC_LIST_LEN (V4L2_H264_NUM_DPB_ENTRIES * 2).
> > 
> > Does it make sense to add a comment as well?
> > 
> > I was thinking something along these lines:
> > 
> > """
> > Pictures in the DPB can be a frame, a complementary field pair or a
> > single field.
> 
> To be honest, I don't know if user has a free choice to select same or 
> different destination (capture) buffer for another field. I never tested it and 
> I'm not sure how to test it with ffmpeg. HW deinterlacing cores on Allwinner 
> SoCs support only interleaved fields as a input, that's why I never though 
> about separate fields.

When setting the format, one will choose V4L2_FIELD_INTERLACED_TB/BT.
Most driver will support that, but if not, they can update field in the
structure. Interlaced being very common, I think it's fine to only
support that, but the drive must enforce this field value.

But I know the Xilinx ZynqMP will do ALTERNATE instead, which would be
each field get decoded in it's own buffer. That, to be honest, I
haven't though about. The tricky part is for H264, since it means 1
slice may populate two capture buffers and I don't know if we can
support this right now.

For now I'd say we should just ensure that whatever userspace ask, we
enforce V4L2_FIELD_INTERLACED_TB/BT appropriately. That is likely not
complete, just look at how interlacing is signal in the bitstream.

> 
> Best regards,
> Jernej
> 
> > Therefore, reference pictures lists need twice as much entries, so it
> > can reference
> > either field of a field pair.
> > """
> > 
> > While it doesn't replace proper H264 specification reading,
> > it would add some clarity.
> > 
> > Thanks,
> > Ezequiel
> 
> 
>