Message-ID: <21ef619f142d4b0b70351b9b5a2530f9f2424383.camel@collabora.com>
Date:   Thu, 24 Sep 2020 16:21:29 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Hans Verkuil <hverkuil@...all.nl>,
        Ezequiel Garcia <ezequiel@...labora.com>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     kernel@...labora.com
Subject: Re: [PATCH] media: uapi: h264: Add documentation to the interface
 header

Le jeudi 24 septembre 2020 à 10:29 +0200, Hans Verkuil a écrit :
> Hi Ezequiel,
> 
> On 21/09/2020 21:38, Ezequiel Garcia wrote:
> > In preparation for making the interface public,
> > document all the structures. Special care is taken to
> > annotate those fields that depart from the H264 syntax.
> > 
> > This commit only adds documentation and doesn't affect
> > functionality in any way.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@...labora.com>
> > ---
> >  include/media/h264-ctrls.h | 138 ++++++++++++++++++++++++++++++++++---
> >  1 file changed, 128 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> > index ec4799154438..afc8e7c05c18 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/media/h264-ctrls.h
> > @@ -46,11 +46,38 @@
> >  #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS	0x0114
> >  #define V4L2_CTRL_TYPE_H264_PRED_WEIGHTS	0x0115
> >  
> > +/**
> > + * enum v4l2_mpeg_video_h264_decode_mode - Decoding mode
> > + *
> > + * @V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED: indicates that decoding
> > + * is performed one slice at a time. In this mode,
> > + * V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS must contain the parsed slice
> > + * parameters and the OUTPUT buffer must contain a single slice.
> > + * V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF feature is used
> > + * in order to support multislice frames.
> > + * @V4L2_MPEG_VIDEO_H264_DECODE_MODE_FRAME_BASED: indicates that
> > + * decoding is performed per frame. The OUTPUT buffer must contain
> > + * all slices and also both fields. This mode is typically supported
> > + * by device drivers that are able to parse the slice(s) header(s)
> > + * in hardware. When this mode is selected,
> > + * V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS is not used.
> > + */
> >  enum v4l2_mpeg_video_h264_decode_mode {
> >  	V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED,
> >  	V4L2_MPEG_VIDEO_H264_DECODE_MODE_FRAME_BASED,
> >  };
> >  
> > +/**
> > + * enum v4l2_mpeg_video_h264_start_code - Start code
> > + *
> > + * @V4L2_MPEG_VIDEO_H264_START_CODE_NONE: slices are passed
> > + * to the driver without any start code.
> > + * @V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B: slices are passed
> > + * to the driver with an Annex B start code prefix
> > + * (legal start codes can be 3-bytes 0x000001 or 4-bytes 0x00000001).
> > + * This mode is typically supported by device drivers that parse
> > + * the start code in hardware.
> > + */
> >  enum v4l2_mpeg_video_h264_start_code {
> >  	V4L2_MPEG_VIDEO_H264_START_CODE_NONE,
> >  	V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
> > @@ -71,6 +98,12 @@ enum v4l2_mpeg_video_h264_start_code {
> >  #define V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD		0x20
> >  #define V4L2_H264_SPS_FLAG_DIRECT_8X8_INFERENCE			0x40
> >  
> > +/**
> > + * struct v4l2_ctrl_h264_sps - H264 sequence parameter set
> > + *
> > + * All the members on this sequence parameter set structure match the
> > + * sequence parameter set syntax as specified by the H264 specification.
> > + */
> >  struct v4l2_ctrl_h264_sps {
> >  	__u8 profile_idc;
> >  	__u8 constraint_set_flags;
> > @@ -101,6 +134,20 @@ struct v4l2_ctrl_h264_sps {
> >  #define V4L2_H264_PPS_FLAG_TRANSFORM_8X8_MODE				0x0040
> >  #define V4L2_H264_PPS_FLAG_SCALING_MATRIX_PRESENT			0x0080
> >  
> > +/**
> > + * struct v4l2_ctrl_h264_pps - H264 picture parameter set
> > + *
> > + * Except where noted, all the members on this picture parameter set
> > + * structure match the sequence parameter set syntax as specified
> > + * by the H264 specification.
> > + *
> > + * In particular, V4L2_H264_PPS_FLAG_SCALING_MATRIX_PRESENT flag
> > + * has a specific meaning. This flag should be set if a non-flat
> > + * scaling matrix applies to the picture. In this case, applications
> > + * are expected to use V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX.
> > + * This will be the case if SPS scaling_matrix_present_flag or
> > + * PPS pic_scaling_matrix_present_flag syntax elements are set.
> 
> This is a bit confusing. 'This will be the case': what does 'This' refer
> to?
> 
> > + */
> >  struct v4l2_ctrl_h264_pps {
> >  	__u8 pic_parameter_set_id;
> >  	__u8 seq_parameter_set_id;
> > @@ -115,6 +162,18 @@ struct v4l2_ctrl_h264_pps {
> >  	__u16 flags;
> >  };
> >  
> > +/**
> > + * struct v4l2_ctrl_h264_scaling_matrix - H264 scaling matrices
> > + *
> > + * @scaling_list_4x4: scaling matrix after applying the inverse
> > + * scanning process. Expected list order is Intra Y, Intra Cb,
> > + * Intra Cr, Inter Y, Inter Cb, Inter Cr. The values on each
> > + * scaling list are expected in raster scan order.
> > + * @scaling_list_8x8: scaling matrix after applying the inverse
> > + * scanning process. Expected list order is Intra Y, Inter Y,
> > + * Intra Cb, Inter Cb, Intra Cr, Inter Cr. The values on each
> > + * scaling list are expected in raster scan order.
> 
> The list order is different for the 4x4 and 8x8 matrices. Is that
> correct?
> 
> If it is correct, then there should perhaps be a sentence like this
> at the start:
> 
> "Note that the list order is different for the 4x4 and 8x8 matrices
> as per the H264 specification."
> 
> (I assume that the order is based on the H264 spec)

That table I think:

Table 7-2 – Assignment of mnemonic names to scaling list indices and
specification of fall-back rule

> 
> > + */
> >  struct v4l2_ctrl_h264_scaling_matrix {
> >  	__u8 scaling_list_4x4[6][16];
> >  	__u8 scaling_list_8x8[6][64];
> > @@ -134,6 +193,12 @@ struct v4l2_h264_weight_factors {
> >  	 ((pps)->weighted_bipred_idc == 1 && \
> >  	  (slice)->slice_type == V4L2_H264_SLICE_TYPE_B))
> >  
> > +/**
> > + * struct v4l2_ctrl_h264_pred_weights - Prediction weight table
> > + *
> > + * Prediction weight table, which matches the syntax specified
> > + * by the H264 specification.
> > + */
> >  struct v4l2_ctrl_h264_pred_weights {
> >  	__u16 luma_log2_weight_denom;
> >  	__u16 chroma_log2_weight_denom;
> > @@ -153,19 +218,41 @@ struct v4l2_ctrl_h264_pred_weights {
> >  #define V4L2_H264_BOTTOM_FIELD_REF			0x2
> >  #define V4L2_H264_FRAME_REF				0x3
> >  
> > +/**
> > + * struct v4l2_h264_reference - H264 picture reference
> > + *
> > + * @fields: indicates how the picture is referenced.
> > + * Valid values are V4L2_H264_{}_REF.
> > + * @index: index into v4l2_ctrl_h264_decode_params.dpb[].
> > + */
> >  struct v4l2_h264_reference {
> >  	__u8 fields;
> > -
> > -	/* Index into v4l2_ctrl_h264_decode_params.dpb[] */
> >  	__u8 index;
> >  };
> >  
> > +/**
> > + * struct v4l2_ctrl_h264_slice_params - H264 slice parameters
> > + *
> > + * This structure holds the H264 syntax elements that are specified
> > + * as non-invariant for the slices in a given frame.
> > + *
> > + * Slice invariant syntax elements are contained in struct
> > + * v4l2_ctrl_h264_decode_params. This is done to reduce the API surface
> > + * on frame-based decoders, where slice header parsing is done by the
> > + * hardware.
> > + *
> > + * Slice invariant syntax elements are specified in specification section
> > + * "7.4.3 Slice header semantics".
> > + *
> > + * Except where noted, the members on this struct match the slice header syntax.
> > + *
> > + * @header_bit_size: offset in bits to slice_data() from the beginning of this slice.
> > + * @ref_pic_list0: reference picture list 0 after applying the per-slice modifications.
> > + * @ref_pic_list1: reference picture list 1 after applying the per-slice modifications.
> 
> There are a lot more fields here that are not mentioned.
> 
> In order to prevent the doc checker to issue warnings about undocumented field,
> I would suggest adding them all, but just keep the description simple:
> 
> @slice_type: see H264 specification.
> 
> You should also document @reserved since that's obviously not part of the h264 spec.
> 
> > + */
> >  struct v4l2_ctrl_h264_slice_params {
> > -	/* Offset in bits to slice_data() from the beginning of this slice. */
> >  	__u32 header_bit_size;
> > -
> >  	__u32 first_mb_in_slice;
> > -
> >  	__u8 slice_type;
> >  	__u8 colour_plane_id;
> >  	__u8 redundant_pic_cnt;
> > @@ -191,22 +278,55 @@ struct v4l2_ctrl_h264_slice_params {
> >  #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
> >  #define V4L2_H264_DPB_ENTRY_FLAG_FIELD		0x08
> >  
> > +/**
> > + * struct v4l2_h264_dpb_entry - H264 decoded picture buffer entry
> > + *
> > + * @reference_ts: timestamp of the V4L2 capture buffer to use as reference.
> > + * The timestamp refers to the timestamp field in struct v4l2_buffer.
> > + * Use v4l2_timeval_to_ns() to convert the struct timeval to a __u64.
> > + * @pic_num: matches PicNum variable assigned during the reference
> > + * picture lists construction process.
> > + * @frame_num: frame identifier which matches frame_num syntax element.
> > + * @fields: indicates how the DPB entry is referenced. Valid values are
> > + * V4L2_H264_{}_REF.
> > + * @top_field_order_cnt: matches TopFieldOrderCnt picture value.
> > + * @bottom_field_order_cnt: matches BottomFieldOrderCnt picture value.
> > + * Note that picture field is indicated by v4l2_buffer.field.
> 
> @flags and @reserved are missing.
> 
> > + */
> >  struct v4l2_h264_dpb_entry {
> >  	__u64 reference_ts;
> >  	__u32 pic_num;
> >  	__u16 frame_num;
> >  	__u8 fields;
> >  	__u8 reserved[5];
> > -	/* Note that field is indicated by v4l2_buffer.field */
> >  	__s32 top_field_order_cnt;
> >  	__s32 bottom_field_order_cnt;
> > -	__u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
> > +	__u32 flags;
> >  };
> >  
> >  #define V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC		0x01
> >  #define V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC		0x02
> >  #define V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD	0x04
> >  
> > +/**
> > + * struct v4l2_ctrl_h264_decode_params - H264 decoding parameters
> > + *
> > + * @dpb: decoded picture buffer.
> > + * @nal_ref_idc: slice header syntax element.
> > + * @frame_num: slice header syntax element.
> > + * @top_field_order_cnt: matches TopFieldOrderCnt picture value.
> > + * @bottom_field_order_cnt: matches BottomFieldOrderCnt picture value.
> > + * Note that picture field is indicated by v4l2_buffer.field.
> > + * @idr_pic_id: slice header syntax element.
> > + * @pic_order_cnt_lsb: slice header syntax element.
> > + * @delta_pic_order_cnt_bottom: slice header syntax element.
> > + * @delta_pic_order_cnt0: slice header syntax element.
> > + * @delta_pic_order_cnt1: slice header syntax element.
> > + * @dec_ref_pic_marking_bit_size: size in bits of dec_ref_pic_marking()
> > + * syntax element.
> > + * @pic_order_cnt_bit_size: size in bits of pic order count syntax.
> > + * @slice_group_change_cycle: slice header syntax element.
> 
> @reserved and @flags are missing.
> 
> > + */
> >  struct v4l2_ctrl_h264_decode_params {
> >  	struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES];
> >  	__u16 nal_ref_idc;
> > @@ -218,14 +338,12 @@ struct v4l2_ctrl_h264_decode_params {
> >  	__s32 delta_pic_order_cnt_bottom;
> >  	__s32 delta_pic_order_cnt0;
> >  	__s32 delta_pic_order_cnt1;
> > -	/* Size in bits of dec_ref_pic_marking() syntax element. */
> >  	__u32 dec_ref_pic_marking_bit_size;
> > -	/* Size in bits of pic order count syntax. */
> >  	__u32 pic_order_cnt_bit_size;
> >  	__u32 slice_group_change_cycle;
> >  
> >  	__u32 reserved;
> > -	__u32 flags; /* V4L2_H264_DECODE_PARAM_FLAG_* */
> > +	__u32 flags;
> >  };
> >  
> >  #endif
> > 
> 
> Regards,
> 
> 	Hans

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)

Powered by blists - more mailing lists