[PATCH v2 0/3] media: uapi: h264: First batch of adjusments

[PATCH v2 0/3] media: uapi: h264: First batch of adjusments

[Boris Brezillon]

Hello,

This is a first batch of adjustments to the stateless H264 decoder
uAPI. The first one is about adding support for per-frame decoding,
which is the only mode supported on some codecs (the hantro G1 block
supports per-slice decoding but not in an way that would allow
efficient multiplexing of several decoding contexts).

The second modification drops the P0/B0/B1 ref lists from the
decode_params control. These lists are no longer needed now that we know
we can build them kernel side based on the DPB.

There are few more changes in the pipe, but I'd like to sync with Paul,
Jonas, Jernej and Nicolas before modifying:
* Enforce order of the scaling list (looks like the rockchip and cedrus
  have different expectations)
* Pass top/bottom field info as flags in the DPB entry: the field
  attached to the capture buffer is not accurate as capture bufs might
  contain both top/bottom (meaning they are actually interlaced), but a
  coded frame might contain only one of these fields. Note
  that there's also a problem with the output -> capture field flag
  propagation we have in copy_metadata() because a coded slice might
  contain only data for top or bottom, but the capture frame might
  contain both. Doing capture->field = output->field means we're lying
  about what's inside the capture buffer (not sure we have
  implementation checking that)
* s/dpb/refs/: looks like we're abusing the term DPB which is supposed
  to be implementation specific. What's provided by the bitstream is a
  list of references that will be used to decode a frame
* ... (add your own)

Feel free to comment on these changes and/or propose alternatives.

Regards,

Boris

Changes in v2:
* Allow decoding multiple slices in per-slice decoding mode
* Minor doc improvements/fixes

Boris Brezillon (3):
  media: uapi: h264: Clarify our expectations regarding NAL header
    format
  media: uapi: h264: Add the concept of decoding mode
  media: uapi: h264: Get rid of the p0/b0/b1 ref-lists

 .../media/uapi/v4l/ext-ctrls-codec.rst        | 56 +++++++++++++++----
 drivers/media/v4l2-core/v4l2-ctrls.c          |  9 +++
 include/media/h264-ctrls.h                    | 13 +++++
 3 files changed, 68 insertions(+), 10 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] media: uapi: h264: Clarify our expectations regarding NAL header format

[Boris Brezillon]

Looks like some stateless decoders expect slices to be prefixed with
ANNEX B start codes (they most likely do some kind of bitstream parsing
and/or need that to delimit slices when doing per frame decoding).
Since skipping those start codes for dummy stateless decoders (those
expecting all params to be passed through controls) should be pretty
easy, let's mandate that all slices be prepended with ANNEX B start
codes.

If we ever need to support AVC headers, we can add a new menu control
to select the type of NAL header to use.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v2:
* None

Note: we might want to add a nal_header_size field to allow drivers to
quickly find where the actual payload start. That'd be particularly
useful here since ANNEX B start codes can be of arbitrary size
(2+(0x00 bytes) + 1(0x01 byte)). The other option would be to enforce
the number of leading 0x00 bytes (a minimum of 2 is required).
---
 Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
index d6ea2ffd65c5..82547d5de250 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
@@ -1726,6 +1726,7 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     :ref:`h264`, section 7.4.3 "Slice Header Semantics". For further
     documentation, refer to the above specification, unless there is
     an explicit comment stating otherwise.
+    All slices should be prepended with an ANNEX B start code.
 
     .. note::
 
-- 
2.20.1

[PATCH v2 2/3] media: uapi: h264: Add the concept of decoding mode

[Boris Brezillon]

Some stateless decoders don't support per-slice decoding (or at least
not in a way that would make them efficient or easy to use).
Let's expose a menu to control and expose the supported decoding modes.
Drivers are allowed to support only one decoding but they can support
both too.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v2:
* Allow decoding multiple slices in per-slice decoding mode
* Minor doc improvement/fixes
---
 .../media/uapi/v4l/ext-ctrls-codec.rst        | 46 ++++++++++++++++++-
 drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
 include/media/h264-ctrls.h                    | 13 ++++++
 3 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
index 82547d5de250..e3b9ab73a588 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
@@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u32
       - ``size``
       -
+    * - __u32
+      - ``start_byte_offset``
+      - Where the slice payload starts in the output buffer. Useful when
+        operating in per frame decoding mode and decoding multi-slice content.
+        In this case, the output buffer will contain more than one slice and
+        some codecs need to know where each slice starts. Note that this
+        offsets points to the beginning of the slice which is supposed to
+        contain an ANNEX B start code.
     * - __u32
       - ``header_bit_size``
       -
@@ -1931,7 +1939,13 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       -
     * - __u16
       - ``num_slices``
-      - Number of slices needed to decode the current frame
+      - Number of slices attached to decode the operation. When operating in
+        per-frame decoding mode (see
+        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field should
+        be set to the number of slices needed to fully decode the frame. When
+        operating in per-slice decoding mode this field can be set to anything
+        between 1 and the remaining number of slices needed to fully decode the
+        frame.
     * - __u16
       - ``nal_ref_idc``
       - NAL reference ID value coming from the NAL Unit header
@@ -2022,6 +2036,36 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - 0x00000004
       - The DPB entry is a long term reference frame
 
+``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
+    Specifies the decoding mode to use. Currently exposes per slice and per
+    frame decoding but new modes might be added later on.
+
+    .. note::
+
+       This menu control is not yet part of the public kernel API and
+       it is expected to change.
+
+.. c:type:: v4l2_mpeg_video_h264_decoding_mode
+
+.. cssclass:: longtable
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
+      - 0
+      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
+        can be set to anything between 1 and the remaining number of slices
+        needed to fully decode a frame.
+    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME``
+      - 1
+      - The decoding is done per frame. v4l2_ctrl_h264_decode_params->num_slices
+        can be > 1. When that happens, the output buffer should contain all
+        slices needed to decode a frame/field, each slice being prefixed by an
+        Annex B NAL header/start-code.
+
 .. _v4l2-mpeg-mpeg2:
 
 ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS (struct)``
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1870cecad9ae..957eb9e2ab0f 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -406,6 +406,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"Explicit",
 		NULL,
 	};
+	static const char * const h264_decoding_mode[] = {
+		"Per Slice",
+		"Per Frame",
+		NULL,
+	};
 	static const char * const mpeg_mpeg2_level[] = {
 		"Low",
 		"Main",
@@ -637,6 +642,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return h264_fp_arrangement_type;
 	case V4L2_CID_MPEG_VIDEO_H264_FMO_MAP_TYPE:
 		return h264_fmo_map_type;
+	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:
+		return h264_decoding_mode;
 	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:
 		return mpeg_mpeg2_level;
 	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:
@@ -856,6 +863,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX:		return "H264 Scaling Matrix";
 	case V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS:		return "H264 Slice Parameters";
 	case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS:		return "H264 Decode Parameters";
+	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:		return "H264 Decoding Mode";
 	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:			return "MPEG2 Level";
 	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:			return "MPEG2 Profile";
 	case V4L2_CID_MPEG_VIDEO_MPEG4_I_FRAME_QP:		return "MPEG4 I-Frame QP Value";
@@ -1223,6 +1231,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_VIDEO_H264_VUI_SAR_IDC:
 	case V4L2_CID_MPEG_VIDEO_H264_SEI_FP_ARRANGEMENT_TYPE:
 	case V4L2_CID_MPEG_VIDEO_H264_FMO_MAP_TYPE:
+	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:
 	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:
 	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:
 	case V4L2_CID_MPEG_VIDEO_MPEG4_LEVEL:
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index e1404d78d6ff..4572936b7c48 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -26,6 +26,7 @@
 #define V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX	(V4L2_CID_MPEG_BASE+1002)
 #define V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS	(V4L2_CID_MPEG_BASE+1003)
 #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS	(V4L2_CID_MPEG_BASE+1004)
+#define V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE	(V4L2_CID_MPEG_BASE+1005)
 
 /* enum v4l2_ctrl_type type values */
 #define V4L2_CTRL_TYPE_H264_SPS			0x0110
@@ -33,6 +34,12 @@
 #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_DECODING_MODE	0x0115
+
+enum v4l2_mpeg_video_h264_decoding_mode {
+	V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE,
+	V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME,
+};
 
 #define V4L2_H264_SPS_CONSTRAINT_SET0_FLAG			0x01
 #define V4L2_H264_SPS_CONSTRAINT_SET1_FLAG			0x02
@@ -111,6 +118,8 @@ struct v4l2_h264_pred_weight_table {
 	struct v4l2_h264_weight_factors weight_factors[2];
 };
 
+#define V4L2_H264_MAX_SLICES_PER_FRAME			16
+
 #define V4L2_H264_SLICE_TYPE_P				0
 #define V4L2_H264_SLICE_TYPE_B				1
 #define V4L2_H264_SLICE_TYPE_I				2
@@ -125,6 +134,10 @@ struct v4l2_h264_pred_weight_table {
 struct v4l2_ctrl_h264_slice_params {
 	/* Size in bytes, including header */
 	__u32 size;
+
+	/* Offset in bytes relative to the beginning of the output buffer. */
+	__u32 start_byte_offset;
+
 	/* Offset in bits to slice_data() from the beginning of this slice. */
 	__u32 header_bit_size;
 
-- 
2.20.1

[PATCH v2 3/3] media: uapi: h264: Get rid of the p0/b0/b1 ref-lists

[Boris Brezillon]

Those lists can be extracted from the dpb, let's simplify userspace
life and build that list kernel-side (generic helpers will be provided
for drivers that need this list).

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v2:
* None
---
 Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
index e3b9ab73a588..8be0ca5c96ab 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
@@ -1949,15 +1949,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u16
       - ``nal_ref_idc``
       - NAL reference ID value coming from the NAL Unit header
-    * - __u8
-      - ``ref_pic_list_p0[32]``
-      - Backward reference list used by P-frames in the original bitstream order
-    * - __u8
-      - ``ref_pic_list_b0[32]``
-      - Backward reference list used by B-frames in the original bitstream order
-    * - __u8
-      - ``ref_pic_list_b1[32]``
-      - Forward reference list used by B-frames in the original bitstream order
     * - __s32
       - ``top_field_order_cnt``
       - Picture Order Count for the coded top field
-- 
2.20.1

Re: [PATCH v2 0/3] media: uapi: h264: First batch of adjusments

[Boris Brezillon]

On Mon, 10 Jun 2019 10:52:47 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Hello,
> 
> This is a first batch of adjustments to the stateless H264 decoder
> uAPI. The first one is about adding support for per-frame decoding,
> which is the only mode supported on some codecs (the hantro G1 block
> supports per-slice decoding but not in an way that would allow
> efficient multiplexing of several decoding contexts).
> 
> The second modification drops the P0/B0/B1 ref lists from the
> decode_params control. These lists are no longer needed now that we know
> we can build them kernel side based on the DPB.
> 
> There are few more changes in the pipe, but I'd like to sync with Paul,
> Jonas, Jernej and Nicolas before modifying:
> * Enforce order of the scaling list (looks like the rockchip and cedrus
>   have different expectations)
> * Pass top/bottom field info as flags in the DPB entry: the field
>   attached to the capture buffer is not accurate as capture bufs might
>   contain both top/bottom (meaning they are actually interlaced), but a
>   coded frame might contain only one of these fields. Note
>   that there's also a problem with the output -> capture field flag
>   propagation we have in copy_metadata() because a coded slice might
>   contain only data for top or bottom, but the capture frame might
>   contain both. Doing capture->field = output->field means we're lying
>   about what's inside the capture buffer (not sure we have
>   implementation checking that)
> * s/dpb/refs/: looks like we're abusing the term DPB which is supposed
>   to be implementation specific. What's provided by the bitstream is a
>   list of references that will be used to decode a frame
> * ... (add your own)
> 
> Feel free to comment on these changes and/or propose alternatives.
> 
> Regards,
> 
> Boris
> 
> Changes in v2:
> * Allow decoding multiple slices in per-slice decoding mode
> * Minor doc improvements/fixes

Forgot:

* Drop sunxi changes until we settle on the uAPI changes (supporting
  multi-slice decoding in per-slice mode requires a bit more work and I
  can't test it)

> 
> Boris Brezillon (3):
>   media: uapi: h264: Clarify our expectations regarding NAL header
>     format
>   media: uapi: h264: Add the concept of decoding mode
>   media: uapi: h264: Get rid of the p0/b0/b1 ref-lists
> 
>  .../media/uapi/v4l/ext-ctrls-codec.rst        | 56 +++++++++++++++----
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 +++
>  include/media/h264-ctrls.h                    | 13 +++++
>  3 files changed, 68 insertions(+), 10 deletions(-)
> 

Re: [PATCH v2 1/3] media: uapi: h264: Clarify our expectations regarding NAL header format

[Paul Kocialkowski]

Hi,

On Mon, 2019-06-10 at 10:52 +0200, Boris Brezillon wrote:
> Looks like some stateless decoders expect slices to be prefixed with
> ANNEX B start codes (they most likely do some kind of bitstream parsing
> and/or need that to delimit slices when doing per frame decoding).
> Since skipping those start codes for dummy stateless decoders (those
> expecting all params to be passed through controls) should be pretty
> easy, let's mandate that all slices be prepended with ANNEX B start
> codes.
> 
> If we ever need to support AVC headers, we can add a new menu control
> to select the type of NAL header to use.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Looks good to me:
Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> ---
> Changes in v2:
> * None
> 
> Note: we might want to add a nal_header_size field to allow drivers to
> quickly find where the actual payload start. That'd be particularly
> useful here since ANNEX B start codes can be of arbitrary size
> (2+(0x00 bytes) + 1(0x01 byte)). The other option would be to enforce
> the number of leading 0x00 bytes (a minimum of 2 is required).
> ---
>  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> index d6ea2ffd65c5..82547d5de250 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> @@ -1726,6 +1726,7 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      :ref:`h264`, section 7.4.3 "Slice Header Semantics". For further
>      documentation, refer to the above specification, unless there is
>      an explicit comment stating otherwise.
> +    All slices should be prepended with an ANNEX B start code.
>  
>      .. note::
>  
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v2 2/3] media: uapi: h264: Add the concept of decoding mode

[Paul Kocialkowski]

Hi,

On Mon, 2019-06-10 at 10:52 +0200, Boris Brezillon wrote:
> Some stateless decoders don't support per-slice decoding (or at least
> not in a way that would make them efficient or easy to use).
> Let's expose a menu to control and expose the supported decoding modes.
> Drivers are allowed to support only one decoding but they can support
> both too.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Changes in v2:
> * Allow decoding multiple slices in per-slice decoding mode
> * Minor doc improvement/fixes
> ---
>  .../media/uapi/v4l/ext-ctrls-codec.rst        | 46 ++++++++++++++++++-
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
>  include/media/h264-ctrls.h                    | 13 ++++++
>  3 files changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> index 82547d5de250..e3b9ab73a588 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u32
>        - ``size``
>        -
> +    * - __u32
> +      - ``start_byte_offset``
> +      - Where the slice payload starts in the output buffer. Useful when
> +        operating in per frame decoding mode and decoding multi-slice content.
> +        In this case, the output buffer will contain more than one slice and
> +        some codecs need to know where each slice starts. Note that this
> +        offsets points to the beginning of the slice which is supposed to
> +        contain an ANNEX B start code.
>      * - __u32
>        - ``header_bit_size``
>        -
> @@ -1931,7 +1939,13 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        -
>      * - __u16
>        - ``num_slices``
> -      - Number of slices needed to decode the current frame
> +      - Number of slices attached to decode the operation. When operating in
> +        per-frame decoding mode (see
> +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field should
> +        be set to the number of slices needed to fully decode the frame. When
> +        operating in per-slice decoding mode this field can be set to anything
> +        between 1 and the remaining number of slices needed to fully decode the
> +        frame.
>      * - __u16
>        - ``nal_ref_idc``
>        - NAL reference ID value coming from the NAL Unit header
> @@ -2022,6 +2036,36 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        - 0x00000004
>        - The DPB entry is a long term reference frame
>  
> +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> +    Specifies the decoding mode to use. Currently exposes per slice and per
> +    frame decoding but new modes might be added later on.

I would maybe formulate this as slice-based and frame-based decoding
since I feel like per-slice implies that slices have to be passed one-
by-one. This wouldn't be the case with the latest proposal for slice-
based decoding, where more than one slice can be passed at a time.

About that, I'm wondering how we could handle that in our drivers.
It will probably be something like:

device_run -+-> decode slice i -> IRQ -+-> job_finish
            `----------- i++ ----------'

And I'm wondering if there could be common helpers to help implement
this, if that's needed at all.

What do you think?

Anyway if you agree with the rewording, this is:

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> +
> +    .. note::
> +
> +       This menu control is not yet part of the public kernel API and
> +       it is expected to change.
> +
> +.. c:type:: v4l2_mpeg_video_h264_decoding_mode
> +
> +.. cssclass:: longtable
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
> +      - 0
> +      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
> +        can be set to anything between 1 and the remaining number of slices
> +        needed to fully decode a frame.
> +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME``
> +      - 1
> +      - The decoding is done per frame. v4l2_ctrl_h264_decode_params->num_slices
> +        can be > 1. When that happens, the output buffer should contain all
> +        slices needed to decode a frame/field, each slice being prefixed by an
> +        Annex B NAL header/start-code.
> +
>  .. _v4l2-mpeg-mpeg2:
>  
>  ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS (struct)``
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 1870cecad9ae..957eb9e2ab0f 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -406,6 +406,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"Explicit",
>  		NULL,
>  	};
> +	static const char * const h264_decoding_mode[] = {
> +		"Per Slice",
> +		"Per Frame",
> +		NULL,
> +	};
>  	static const char * const mpeg_mpeg2_level[] = {
>  		"Low",
>  		"Main",
> @@ -637,6 +642,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return h264_fp_arrangement_type;
>  	case V4L2_CID_MPEG_VIDEO_H264_FMO_MAP_TYPE:
>  		return h264_fmo_map_type;
> +	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:
> +		return h264_decoding_mode;
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:
>  		return mpeg_mpeg2_level;
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:
> @@ -856,6 +863,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX:		return "H264 Scaling Matrix";
>  	case V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS:		return "H264 Slice Parameters";
>  	case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS:		return "H264 Decode Parameters";
> +	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:		return "H264 Decoding Mode";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:			return "MPEG2 Level";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:			return "MPEG2 Profile";
>  	case V4L2_CID_MPEG_VIDEO_MPEG4_I_FRAME_QP:		return "MPEG4 I-Frame QP Value";
> @@ -1223,6 +1231,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MPEG_VIDEO_H264_VUI_SAR_IDC:
>  	case V4L2_CID_MPEG_VIDEO_H264_SEI_FP_ARRANGEMENT_TYPE:
>  	case V4L2_CID_MPEG_VIDEO_H264_FMO_MAP_TYPE:
> +	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:
>  	case V4L2_CID_MPEG_VIDEO_MPEG4_LEVEL:
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index e1404d78d6ff..4572936b7c48 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -26,6 +26,7 @@
>  #define V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX	(V4L2_CID_MPEG_BASE+1002)
>  #define V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS	(V4L2_CID_MPEG_BASE+1003)
>  #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS	(V4L2_CID_MPEG_BASE+1004)
> +#define V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE	(V4L2_CID_MPEG_BASE+1005)
>  
>  /* enum v4l2_ctrl_type type values */
>  #define V4L2_CTRL_TYPE_H264_SPS			0x0110
> @@ -33,6 +34,12 @@
>  #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_DECODING_MODE	0x0115
> +
> +enum v4l2_mpeg_video_h264_decoding_mode {
> +	V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE,
> +	V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME,
> +};
>  
>  #define V4L2_H264_SPS_CONSTRAINT_SET0_FLAG			0x01
>  #define V4L2_H264_SPS_CONSTRAINT_SET1_FLAG			0x02
> @@ -111,6 +118,8 @@ struct v4l2_h264_pred_weight_table {
>  	struct v4l2_h264_weight_factors weight_factors[2];
>  };
>  
> +#define V4L2_H264_MAX_SLICES_PER_FRAME			16
> +
>  #define V4L2_H264_SLICE_TYPE_P				0
>  #define V4L2_H264_SLICE_TYPE_B				1
>  #define V4L2_H264_SLICE_TYPE_I				2
> @@ -125,6 +134,10 @@ struct v4l2_h264_pred_weight_table {
>  struct v4l2_ctrl_h264_slice_params {
>  	/* Size in bytes, including header */
>  	__u32 size;
> +
> +	/* Offset in bytes relative to the beginning of the output buffer. */
> +	__u32 start_byte_offset;
> +
>  	/* Offset in bits to slice_data() from the beginning of this slice. */
>  	__u32 header_bit_size;
>  
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v2 3/3] media: uapi: h264: Get rid of the p0/b0/b1 ref-lists

[Paul Kocialkowski]

Hi,

On Mon, 2019-06-10 at 10:52 +0200, Boris Brezillon wrote:
> Those lists can be extracted from the dpb, let's simplify userspace
> life and build that list kernel-side (generic helpers will be provided
> for drivers that need this list).

I don't really have any clear idea about that, but there was a
discussion about DPB vs reference picture lists some weeks ago.

Is there some mail thread with a rationale about it, some IRC logs I
could look at or could the people involved in the discussion provide
some additional background at this point?

IIRC we also talked about removing the DPB or at least renaming it, but
I don't have a clear idea of the outcome as well.

Cheers,

Paul

> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Changes in v2:
> * None
> ---
>  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> index e3b9ab73a588..8be0ca5c96ab 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> @@ -1949,15 +1949,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u16
>        - ``nal_ref_idc``
>        - NAL reference ID value coming from the NAL Unit header
> -    * - __u8
> -      - ``ref_pic_list_p0[32]``
> -      - Backward reference list used by P-frames in the original bitstream order
> -    * - __u8
> -      - ``ref_pic_list_b0[32]``
> -      - Backward reference list used by B-frames in the original bitstream order
> -    * - __u8
> -      - ``ref_pic_list_b1[32]``
> -      - Forward reference list used by B-frames in the original bitstream order
>      * - __s32
>        - ``top_field_order_cnt``
>        - Picture Order Count for the coded top field
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v2 3/3] media: uapi: h264: Get rid of the p0/b0/b1 ref-lists

[Boris Brezillon]

On Wed, 26 Jun 2019 13:33:41 +0200
Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:

> Hi,
> 
> On Mon, 2019-06-10 at 10:52 +0200, Boris Brezillon wrote:
> > Those lists can be extracted from the dpb, let's simplify userspace
> > life and build that list kernel-side (generic helpers will be provided
> > for drivers that need this list).  
> 
> I don't really have any clear idea about that, but there was a
> discussion about DPB vs reference picture lists some weeks ago.

What we call DPB right now is actually a list of reference pictures
(each entry being flagged long or short term). When reading the spec,
you said DPB was referring to something that's more implementation
specific, and I think that's what motivated your initial suggestion to
rename this field into something more appropriate (ref_pics?). TBH, I'm
just guessing here, since you were the one initially proposing this
change, and I must say that having to explain what you had in mind at
that time is a bit weird :P.

> 
> Is there some mail thread with a rationale about it, some IRC logs I
> could look at or could the people involved in the discussion provide
> some additional background at this point?

Well, you were part of the discussion, and I think most of it happened
in the "Proposed updates and guidelines for MPEG-2, H.264 and H.265
stateless support" thread you started.

> 
> IIRC we also talked about removing the DPB or at least renaming it, but
> I don't have a clear idea of the outcome as well.

The list of long/short refs has to be passed, and that's actually what
we currently call "DPB", so we're just talking about a rename here,
nothing more. The ordered P/B0/B1 reflists can easily be built from the
un-ordered list of ref pics, so I'm just proposing to get rid of these
fields and have a generic implementation kernel-side so that drivers
that need it don't have to re-implement it.

Re: [PATCH v2 2/3] media: uapi: h264: Add the concept of decoding mode

[Boris Brezillon]

On Wed, 26 Jun 2019 13:30:38 +0200
Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:

> Hi,
> 
> On Mon, 2019-06-10 at 10:52 +0200, Boris Brezillon wrote:
> > Some stateless decoders don't support per-slice decoding (or at least
> > not in a way that would make them efficient or easy to use).
> > Let's expose a menu to control and expose the supported decoding modes.
> > Drivers are allowed to support only one decoding but they can support
> > both too.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> > Changes in v2:
> > * Allow decoding multiple slices in per-slice decoding mode
> > * Minor doc improvement/fixes
> > ---
> >  .../media/uapi/v4l/ext-ctrls-codec.rst        | 46 ++++++++++++++++++-
> >  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
> >  include/media/h264-ctrls.h                    | 13 ++++++
> >  3 files changed, 67 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > index 82547d5de250..e3b9ab73a588 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __u32
> >        - ``size``
> >        -
> > +    * - __u32
> > +      - ``start_byte_offset``
> > +      - Where the slice payload starts in the output buffer. Useful when
> > +        operating in per frame decoding mode and decoding multi-slice content.
> > +        In this case, the output buffer will contain more than one slice and
> > +        some codecs need to know where each slice starts. Note that this
> > +        offsets points to the beginning of the slice which is supposed to
> > +        contain an ANNEX B start code.
> >      * - __u32
> >        - ``header_bit_size``
> >        -
> > @@ -1931,7 +1939,13 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >        -
> >      * - __u16
> >        - ``num_slices``
> > -      - Number of slices needed to decode the current frame
> > +      - Number of slices attached to decode the operation. When operating in
> > +        per-frame decoding mode (see
> > +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field should
> > +        be set to the number of slices needed to fully decode the frame. When
> > +        operating in per-slice decoding mode this field can be set to anything
> > +        between 1 and the remaining number of slices needed to fully decode the
> > +        frame.
> >      * - __u16
> >        - ``nal_ref_idc``
> >        - NAL reference ID value coming from the NAL Unit header
> > @@ -2022,6 +2036,36 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >        - 0x00000004
> >        - The DPB entry is a long term reference frame
> >  
> > +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> > +    Specifies the decoding mode to use. Currently exposes per slice and per
> > +    frame decoding but new modes might be added later on.  
> 
> I would maybe formulate this as slice-based and frame-based decoding
> since I feel like per-slice implies that slices have to be passed one-
> by-one. This wouldn't be the case with the latest proposal for slice-
> based decoding, where more than one slice can be passed at a time.
> 
> About that, I'm wondering how we could handle that in our drivers.
> It will probably be something like:
> 
> device_run -+-> decode slice i -> IRQ -+-> job_finish
>             `----------- i++ ----------'
> 
> And I'm wondering if there could be common helpers to help implement
> this, if that's needed at all.

Yep, we could probably have that kind of helper. I haven't had time to
work on the generic m2m stateless codec layer since last time we talked
about it on IRC, but I plan to resume working on this task soon. I'll
try to think about this generic "decode all slices" helper once the
basic building blocks are ready.

> 
> What do you think?
> 
> Anyway if you agree with the rewording, this is:

I'm perfectly fine with the suggested rewording.

> 
> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Thanks for the review.

Boris

Re: [PATCH v2 2/3] media: uapi: h264: Add the concept of decoding mode

[Tomasz Figa]

On Thu, Jun 27, 2019 at 12:26 AM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Wed, 26 Jun 2019 13:30:38 +0200
> Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:
>
> > Hi,
> >
> > On Mon, 2019-06-10 at 10:52 +0200, Boris Brezillon wrote:
> > > Some stateless decoders don't support per-slice decoding (or at least
> > > not in a way that would make them efficient or easy to use).
> > > Let's expose a menu to control and expose the supported decoding modes.
> > > Drivers are allowed to support only one decoding but they can support
> > > both too.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > > Changes in v2:
> > > * Allow decoding multiple slices in per-slice decoding mode
> > > * Minor doc improvement/fixes
> > > ---
> > >  .../media/uapi/v4l/ext-ctrls-codec.rst        | 46 ++++++++++++++++++-
> > >  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
> > >  include/media/h264-ctrls.h                    | 13 ++++++
> > >  3 files changed, 67 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > index 82547d5de250..e3b9ab73a588 100644
> > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > >      * - __u32
> > >        - ``size``
> > >        -
> > > +    * - __u32
> > > +      - ``start_byte_offset``
> > > +      - Where the slice payload starts in the output buffer. Useful when
> > > +        operating in per frame decoding mode and decoding multi-slice content.
> > > +        In this case, the output buffer will contain more than one slice and
> > > +        some codecs need to know where each slice starts. Note that this
> > > +        offsets points to the beginning of the slice which is supposed to
> > > +        contain an ANNEX B start code.
> > >      * - __u32
> > >        - ``header_bit_size``
> > >        -
> > > @@ -1931,7 +1939,13 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > >        -
> > >      * - __u16
> > >        - ``num_slices``
> > > -      - Number of slices needed to decode the current frame
> > > +      - Number of slices attached to decode the operation. When operating in
> > > +        per-frame decoding mode (see
> > > +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field should
> > > +        be set to the number of slices needed to fully decode the frame. When
> > > +        operating in per-slice decoding mode this field can be set to anything
> > > +        between 1 and the remaining number of slices needed to fully decode the
> > > +        frame.
> > >      * - __u16
> > >        - ``nal_ref_idc``
> > >        - NAL reference ID value coming from the NAL Unit header
> > > @@ -2022,6 +2036,36 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > >        - 0x00000004
> > >        - The DPB entry is a long term reference frame
> > >
> > > +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> > > +    Specifies the decoding mode to use. Currently exposes per slice and per
> > > +    frame decoding but new modes might be added later on.
> >
> > I would maybe formulate this as slice-based and frame-based decoding
> > since I feel like per-slice implies that slices have to be passed one-
> > by-one. This wouldn't be the case with the latest proposal for slice-
> > based decoding, where more than one slice can be passed at a time.
> >
> > About that, I'm wondering how we could handle that in our drivers.
> > It will probably be something like:
> >
> > device_run -+-> decode slice i -> IRQ -+-> job_finish
> >             `----------- i++ ----------'
> >
> > And I'm wondering if there could be common helpers to help implement
> > this, if that's needed at all.
>
> Yep, we could probably have that kind of helper. I haven't had time to
> work on the generic m2m stateless codec layer since last time we talked
> about it on IRC, but I plan to resume working on this task soon. I'll
> try to think about this generic "decode all slices" helper once the
> basic building blocks are ready.

Most of the time it's a good idea to just open code it in one driver
first and then another driver needs to do the same (or very similar)
thing, generalizing the code from the first driver into a helper.

Best regards,
Tomasz