| Message ID | 20200806151310.98624-4-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean H264 stateless uAPI | expand |
On 2020-08-06 17:12, Ezequiel Garcia wrote: > The prediction weight parameters are only required under > certain conditions, which depend on slice header parameters. > > As specified in section 7.3.3 Slice header syntax, the prediction > weight table is present if: > > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ > (weighted_bipred_idc == 1 && slice_type == B)) Maybe a macro can be added to help check this contition? Something like this maybe: #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ ((pps)->weighted_bipred_idc == 1 && \ (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) > > Given its size, it makes sense to move this table to its control, > so applications can avoid passing it if the slice doesn't specify it. > > Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. > With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight > is 772 bytes. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > v2: Fix missing Cedrus changes and mssing control declaration, > as noted by Hans and Jernej. > --- > .../media/v4l/ext-ctrls-codec.rst | 19 ++++++++++++------- > drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ > drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- > include/media/h264-ctrls.h | 5 +++-- > include/media/v4l2-ctrls.h | 2 ++ > 8 files changed, 37 insertions(+), 13 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > index d1438b1e259f..c36ce5a95fc5 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > @@ -1879,18 +1879,23 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > - 0x00000008 > - > > -``Prediction Weight Table`` > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` > + Prediction weight table defined according to :ref:`h264`, > + section 7.4.3.2 "Prediction Weight Table Semantics". > + The prediction weight table must be passed by applications > + under the conditions explained in section 7.3.3 "Slice header > + syntax". > > - The bitstream parameters are defined according to :ref:`h264`, > - section 7.4.3.2 "Prediction Weight Table Semantics". For further > - documentation, refer to the above specification, unless there is > - an explicit comment stating otherwise. > + .. note:: > + > + This compound control is not yet part of the public kernel API and > + it is expected to change. > > -.. c:type:: v4l2_h264_pred_weight_table > +.. c:type:: v4l2_ctrl_h264_pred_weights > > .. cssclass:: longtable > > -.. flat-table:: struct v4l2_h264_pred_weight_table > +.. flat-table:: struct v4l2_ctrl_h264_pred_weights > :header-rows: 0 > :stub-columns: 0 > :widths: 1 1 2 > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 3f3fbcd60cc6..76c8dc8fb31c 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return "H264 Decode Parameters"; > case V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return "H264 Decode Mode"; > case V4L2_CID_MPEG_VIDEO_H264_START_CODE: return "H264 Start Code"; > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return "H264 Prediction Weight Table"; > 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"; > @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; > break; > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; > + break; > case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: > *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; > break; > @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, > case V4L2_CTRL_TYPE_H264_SPS: > case V4L2_CTRL_TYPE_H264_PPS: > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > break; > @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > elem_size = sizeof(struct v4l2_ctrl_h264_decode_params); > break; > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > + elem_size = sizeof(struct v4l2_ctrl_h264_pred_weights); > + break; > case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: > elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); > break; > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c > index bc27f9430eeb..027cdd1be5a0 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > @@ -78,6 +78,13 @@ static const struct cedrus_control cedrus_controls[] = { > .codec = CEDRUS_CODEC_H264, > .required = true, > }, > + { > + .cfg = { > + .id = V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > + }, > + .codec = CEDRUS_CODEC_H264, > + .required = true, This should probably be false if this control is to be optional as implied by the commit message. Best regards, Jonas > + }, > { > .cfg = { > .id = V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE, > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h > index 96765555ab8a..93c843ae14bb 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h > @@ -62,6 +62,7 @@ struct cedrus_h264_run { > const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix; > const struct v4l2_ctrl_h264_slice_params *slice_params; > const struct v4l2_ctrl_h264_sps *sps; > + const struct v4l2_ctrl_h264_pred_weights *pred_weights; > }; > > struct cedrus_mpeg2_run { > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > index 58c48e4fdfe9..6385026d1b6b 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > @@ -57,6 +57,8 @@ void cedrus_device_run(void *priv) > V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS); > run.h264.sps = cedrus_find_control_data(ctx, > V4L2_CID_MPEG_VIDEO_H264_SPS); > + run.h264.pred_weights = cedrus_find_control_data(ctx, > + V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS); > break; > > case V4L2_PIX_FMT_HEVC_SLICE: > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > index cce527bbdf86..a9ba78b15907 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > @@ -256,10 +256,8 @@ static void cedrus_write_scaling_lists(struct cedrus_ctx *ctx, > static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx, > struct cedrus_run *run) > { > - const struct v4l2_ctrl_h264_slice_params *slice = > - run->h264.slice_params; > - const struct v4l2_h264_pred_weight_table *pred_weight = > - &slice->pred_weight_table; > + const struct v4l2_ctrl_h264_pred_weights *pred_weight = > + run->h264.pred_weights; > struct cedrus_dev *dev = ctx->dev; > int i, j, k; > > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h > index 4c0bb7f5fb05..54cd9bec0b23 100644 > --- a/include/media/h264-ctrls.h > +++ b/include/media/h264-ctrls.h > @@ -36,6 +36,7 @@ > #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS (V4L2_CID_MPEG_BASE+1004) > #define V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (V4L2_CID_MPEG_BASE+1005) > #define V4L2_CID_MPEG_VIDEO_H264_START_CODE (V4L2_CID_MPEG_BASE+1006) > +#define V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (V4L2_CID_MPEG_BASE+1007) > > /* enum v4l2_ctrl_type type values */ > #define V4L2_CTRL_TYPE_H264_SPS 0x0110 > @@ -43,6 +44,7 @@ > #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX 0x0112 > #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS 0x0113 > #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS 0x0114 > +#define V4L2_CTRL_TYPE_H264_PRED_WEIGHTS 0x0115 > > enum v4l2_mpeg_video_h264_decode_mode { > V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED, > @@ -125,7 +127,7 @@ struct v4l2_h264_weight_factors { > __s16 chroma_offset[32][2]; > }; > > -struct v4l2_h264_pred_weight_table { > +struct v4l2_ctrl_h264_pred_weights { > __u16 luma_log2_weight_denom; > __u16 chroma_log2_weight_denom; > struct v4l2_h264_weight_factors weight_factors[2]; > @@ -177,7 +179,6 @@ struct v4l2_ctrl_h264_slice_params { > __s32 delta_pic_order_cnt0; > __s32 delta_pic_order_cnt1; > > - struct v4l2_h264_pred_weight_table pred_weight_table; > /* Size in bits of dec_ref_pic_marking() syntax element. */ > __u32 dec_ref_pic_marking_bit_size; > /* Size in bits of pic order count syntax. */ > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index f40e2cbb21d3..cb25f345e9ad 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -51,6 +51,7 @@ struct video_device; > * @p_h264_scaling_matrix: Pointer to a struct v4l2_ctrl_h264_scaling_matrix. > * @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params. > * @p_h264_decode_params: Pointer to a struct v4l2_ctrl_h264_decode_params. > + * @p_h264_pred_weights: Pointer to a struct v4l2_ctrl_h264_pred_weights. > * @p_vp8_frame_header: Pointer to a VP8 frame header structure. > * @p_hevc_sps: Pointer to an HEVC sequence parameter set structure. > * @p_hevc_pps: Pointer to an HEVC picture parameter set structure. > @@ -74,6 +75,7 @@ union v4l2_ctrl_ptr { > struct v4l2_ctrl_h264_scaling_matrix *p_h264_scaling_matrix; > struct v4l2_ctrl_h264_slice_params *p_h264_slice_params; > struct v4l2_ctrl_h264_decode_params *p_h264_decode_params; > + struct v4l2_ctrl_h264_pred_weights *p_h264_pred_weights; > struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header; > struct v4l2_ctrl_hevc_sps *p_hevc_sps; > struct v4l2_ctrl_hevc_pps *p_hevc_pps; >
On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: > > On 2020-08-06 17:12, Ezequiel Garcia wrote: > > The prediction weight parameters are only required under > > certain conditions, which depend on slice header parameters. > > > > As specified in section 7.3.3 Slice header syntax, the prediction > > weight table is present if: > > > > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ > > (weighted_bipred_idc == 1 && slice_type == B)) > > Maybe a macro can be added to help check this contition? > > Something like this maybe: > > #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ > ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ > ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ > (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ > ((pps)->weighted_bipred_idc == 1 && \ > (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) > Yeah, that could make sense. Note that the biggest value in having the prediction weight table separated is to allow applications to skip setting this largish control, reducing the amount of data that needs to be passed from userspace -- especially when not needed :-) > > > > Given its size, it makes sense to move this table to its control, > > so applications can avoid passing it if the slice doesn't specify it. > > > > Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. > > With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight > > is 772 bytes. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > v2: Fix missing Cedrus changes and mssing control declaration, > > as noted by Hans and Jernej. > > --- > > .../media/v4l/ext-ctrls-codec.rst | 19 ++++++++++++------- > > drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ > > drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- > > include/media/h264-ctrls.h | 5 +++-- > > include/media/v4l2-ctrls.h | 2 ++ > > 8 files changed, 37 insertions(+), 13 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > index d1438b1e259f..c36ce5a95fc5 100644 > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > @@ -1879,18 +1879,23 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > - 0x00000008 > > - > > > > -``Prediction Weight Table`` > > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` > > + Prediction weight table defined according to :ref:`h264`, > > + section 7.4.3.2 "Prediction Weight Table Semantics". > > + The prediction weight table must be passed by applications > > + under the conditions explained in section 7.3.3 "Slice header > > + syntax". > > > > - The bitstream parameters are defined according to :ref:`h264`, > > - section 7.4.3.2 "Prediction Weight Table Semantics". For further > > - documentation, refer to the above specification, unless there is > > - an explicit comment stating otherwise. > > + .. note:: > > + > > + This compound control is not yet part of the public kernel API and > > + it is expected to change. > > > > -.. c:type:: v4l2_h264_pred_weight_table > > +.. c:type:: v4l2_ctrl_h264_pred_weights > > > > .. cssclass:: longtable > > > > -.. flat-table:: struct v4l2_h264_pred_weight_table > > +.. flat-table:: struct v4l2_ctrl_h264_pred_weights > > :header-rows: 0 > > :stub-columns: 0 > > :widths: 1 1 2 > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > > index 3f3fbcd60cc6..76c8dc8fb31c 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return "H264 Decode Parameters"; > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return "H264 Decode Mode"; > > case V4L2_CID_MPEG_VIDEO_H264_START_CODE: return "H264 Start Code"; > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return "H264 Prediction Weight Table"; > > 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"; > > @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; > > break; > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; > > + break; > > case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: > > *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; > > break; > > @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, > > case V4L2_CTRL_TYPE_H264_SPS: > > case V4L2_CTRL_TYPE_H264_PPS: > > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > break; > > @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > elem_size = sizeof(struct v4l2_ctrl_h264_decode_params); > > break; > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > + elem_size = sizeof(struct v4l2_ctrl_h264_pred_weights); > > + break; > > case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: > > elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); > > break; > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c > > index bc27f9430eeb..027cdd1be5a0 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > @@ -78,6 +78,13 @@ static const struct cedrus_control cedrus_controls[] = { > > .codec = CEDRUS_CODEC_H264, > > .required = true, > > }, > > + { > > + .cfg = { > > + .id = V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > > + }, > > + .codec = CEDRUS_CODEC_H264, > > + .required = true, > > This should probably be false if this control is to be optional as implied > by the commit message. > Well, the control is optional if the driver implements it as optional, which Cedrus isn't currently doing :-) I have been reluctant to change Cedrus much, since I am not testing there. Perhaps we can address this with a follow-up patch, adding the macro you suggest and changing the control to optional? Speaking of optional controls, note that v4l2 controls are cached per-fd (per context). This means for instance, that the active PPS and SPS need only be passed to drivers when activated, and not on each frame. Similarly, another optimization to reduce the amount of data passed is to have default scaling matrices in the kernel, and allow applications to not pass them, and have the drivers fallback to default (default or previously set?) in that case. I have tested these things, but haven't posted patches because I'm not entirely sure how well-defined and specified this behavior currently is. It is something to keep in mind though, as a future optimization. Thanks! Ezequiel > Best regards, > Jonas > > > + }, > > { > > .cfg = { > > .id = V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE, > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h > > index 96765555ab8a..93c843ae14bb 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h > > @@ -62,6 +62,7 @@ struct cedrus_h264_run { > > const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix; > > const struct v4l2_ctrl_h264_slice_params *slice_params; > > const struct v4l2_ctrl_h264_sps *sps; > > + const struct v4l2_ctrl_h264_pred_weights *pred_weights; > > }; > > > > struct cedrus_mpeg2_run { > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > index 58c48e4fdfe9..6385026d1b6b 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > @@ -57,6 +57,8 @@ void cedrus_device_run(void *priv) > > V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS); > > run.h264.sps = cedrus_find_control_data(ctx, > > V4L2_CID_MPEG_VIDEO_H264_SPS); > > + run.h264.pred_weights = cedrus_find_control_data(ctx, > > + V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS); > > break; > > > > case V4L2_PIX_FMT_HEVC_SLICE: > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > index cce527bbdf86..a9ba78b15907 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > @@ -256,10 +256,8 @@ static void cedrus_write_scaling_lists(struct cedrus_ctx *ctx, > > static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx, > > struct cedrus_run *run) > > { > > - const struct v4l2_ctrl_h264_slice_params *slice = > > - run->h264.slice_params; > > - const struct v4l2_h264_pred_weight_table *pred_weight = > > - &slice->pred_weight_table; > > + const struct v4l2_ctrl_h264_pred_weights *pred_weight = > > + run->h264.pred_weights; > > struct cedrus_dev *dev = ctx->dev; > > int i, j, k; > > > > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h > > index 4c0bb7f5fb05..54cd9bec0b23 100644 > > --- a/include/media/h264-ctrls.h > > +++ b/include/media/h264-ctrls.h > > @@ -36,6 +36,7 @@ > > #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS (V4L2_CID_MPEG_BASE+1004) > > #define V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (V4L2_CID_MPEG_BASE+1005) > > #define V4L2_CID_MPEG_VIDEO_H264_START_CODE (V4L2_CID_MPEG_BASE+1006) > > +#define V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (V4L2_CID_MPEG_BASE+1007) > > > > /* enum v4l2_ctrl_type type values */ > > #define V4L2_CTRL_TYPE_H264_SPS 0x0110 > > @@ -43,6 +44,7 @@ > > #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX 0x0112 > > #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS 0x0113 > > #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS 0x0114 > > +#define V4L2_CTRL_TYPE_H264_PRED_WEIGHTS 0x0115 > > > > enum v4l2_mpeg_video_h264_decode_mode { > > V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED, > > @@ -125,7 +127,7 @@ struct v4l2_h264_weight_factors { > > __s16 chroma_offset[32][2]; > > }; > > > > -struct v4l2_h264_pred_weight_table { > > +struct v4l2_ctrl_h264_pred_weights { > > __u16 luma_log2_weight_denom; > > __u16 chroma_log2_weight_denom; > > struct v4l2_h264_weight_factors weight_factors[2]; > > @@ -177,7 +179,6 @@ struct v4l2_ctrl_h264_slice_params { > > __s32 delta_pic_order_cnt0; > > __s32 delta_pic_order_cnt1; > > > > - struct v4l2_h264_pred_weight_table pred_weight_table; > > /* Size in bits of dec_ref_pic_marking() syntax element. */ > > __u32 dec_ref_pic_marking_bit_size; > > /* Size in bits of pic order count syntax. */ > > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > > index f40e2cbb21d3..cb25f345e9ad 100644 > > --- a/include/media/v4l2-ctrls.h > > +++ b/include/media/v4l2-ctrls.h > > @@ -51,6 +51,7 @@ struct video_device; > > * @p_h264_scaling_matrix: Pointer to a struct v4l2_ctrl_h264_scaling_matrix. > > * @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params. > > * @p_h264_decode_params: Pointer to a struct v4l2_ctrl_h264_decode_params. > > + * @p_h264_pred_weights: Pointer to a struct v4l2_ctrl_h264_pred_weights. > > * @p_vp8_frame_header: Pointer to a VP8 frame header structure. > > * @p_hevc_sps: Pointer to an HEVC sequence parameter set structure. > > * @p_hevc_pps: Pointer to an HEVC picture parameter set structure. > > @@ -74,6 +75,7 @@ union v4l2_ctrl_ptr { > > struct v4l2_ctrl_h264_scaling_matrix *p_h264_scaling_matrix; > > struct v4l2_ctrl_h264_slice_params *p_h264_slice_params; > > struct v4l2_ctrl_h264_decode_params *p_h264_decode_params; > > + struct v4l2_ctrl_h264_pred_weights *p_h264_pred_weights; > > struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header; > > struct v4l2_ctrl_hevc_sps *p_hevc_sps; > > struct v4l2_ctrl_hevc_pps *p_hevc_pps; > >
Dne nedelja, 09. avgust 2020 ob 15:55:50 CEST je Ezequiel Garcia napisal(a): > On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: > > On 2020-08-06 17:12, Ezequiel Garcia wrote: > > > The prediction weight parameters are only required under > > > certain conditions, which depend on slice header parameters. > > > > > > As specified in section 7.3.3 Slice header syntax, the prediction > > > weight table is present if: > > > > > > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ > > > (weighted_bipred_idc == 1 && slice_type == B)) > > > > Maybe a macro can be added to help check this contition? > > > > Something like this maybe: > > > > #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ > > > > ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ > > > > ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ > > > > ((pps)->weighted_bipred_idc == 1 && \ > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) > > Yeah, that could make sense. > > Note that the biggest value in having the prediction weight table > separated is to allow applications to skip setting this largish control, > reducing the amount of data that needs to be passed from userspace > -- especially when not needed :-) > > > > Given its size, it makes sense to move this table to its control, > > > so applications can avoid passing it if the slice doesn't specify it. > > > > > > Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. > > > With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight > > > is 772 bytes. > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > --- > > > v2: Fix missing Cedrus changes and mssing control declaration, > > > > > > as noted by Hans and Jernej. > > > > > > --- > > > > > > .../media/v4l/ext-ctrls-codec.rst | 19 ++++++++++++------- > > > drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- > > > include/media/h264-ctrls.h | 5 +++-- > > > include/media/v4l2-ctrls.h | 2 ++ > > > 8 files changed, 37 insertions(+), 13 deletions(-) > > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index > > > d1438b1e259f..c36ce5a95fc5 100644 > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > @@ -1879,18 +1879,23 @@ enum > > > v4l2_mpeg_video_h264_hierarchical_coding_type -> > > > > - 0x00000008 > > > - > > > > > > -``Prediction Weight Table`` > > > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` > > > + Prediction weight table defined according to :ref:`h264`, > > > + section 7.4.3.2 "Prediction Weight Table Semantics". > > > + The prediction weight table must be passed by applications > > > + under the conditions explained in section 7.3.3 "Slice header > > > + syntax". > > > > > > - The bitstream parameters are defined according to :ref:`h264`, > > > - section 7.4.3.2 "Prediction Weight Table Semantics". For further > > > - documentation, refer to the above specification, unless there is > > > - an explicit comment stating otherwise. > > > + .. note:: > > > + > > > + This compound control is not yet part of the public kernel API > > > and > > > + it is expected to change. > > > > > > -.. c:type:: v4l2_h264_pred_weight_table > > > +.. c:type:: v4l2_ctrl_h264_pred_weights > > > > > > .. cssclass:: longtable > > > > > > -.. flat-table:: struct v4l2_h264_pred_weight_table > > > +.. flat-table:: struct v4l2_ctrl_h264_pred_weights > > > > > > :header-rows: 0 > > > :stub-columns: 0 > > > :widths: 1 1 2 > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c > > > b/drivers/media/v4l2-core/v4l2-ctrls.c index 3f3fbcd60cc6..76c8dc8fb31c > > > 100644 > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return > > > "H264 Decode Parameters"; case > > > V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return "H264 > > > Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: > > > return "H264 Start Code";> > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return > > > "H264 Prediction Weight Table";> > > > > 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";> > > > > @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, > > > enum v4l2_ctrl_type *type,> > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > > *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; > > > break; > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > > + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; > > > + break; > > > > > > case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: > > > *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; > > > break; > > > > > > @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct > > > v4l2_ctrl *ctrl, u32 idx,> > > > > case V4L2_CTRL_TYPE_H264_SPS: > > > case V4L2_CTRL_TYPE_H264_PPS: > > > > > > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > break; > > > > > > @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct > > > v4l2_ctrl_handler *hdl,> > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > elem_size = sizeof(struct v4l2_ctrl_h264_decode_params); > > > break; > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > + elem_size = sizeof(struct v4l2_ctrl_h264_pred_weights); > > > + break; > > > > > > case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: > > > elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); > > > break; > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > > bc27f9430eeb..027cdd1be5a0 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > > @@ -78,6 +78,13 @@ static const struct cedrus_control cedrus_controls[] > > > = { > > > > > > .codec = CEDRUS_CODEC_H264, > > > .required = true, > > > > > > }, > > > > > > + { > > > + .cfg = { > > > + .id = V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > > > + }, > > > + .codec = CEDRUS_CODEC_H264, > > > + .required = true, > > > > This should probably be false if this control is to be optional as implied > > by the commit message. > > Well, the control is optional if the driver implements it as optional, > which Cedrus isn't currently doing :-) Why do you think so? Prediction weights are filled only when they are needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ sunxi/cedrus/cedrus_h264.c#L370 Best regards, Jernej > > I have been reluctant to change Cedrus much, since I am not > testing there. Perhaps we can address this with a follow-up patch, > adding the macro you suggest and changing the control to optional? > > Speaking of optional controls, note that v4l2 controls are cached > per-fd (per context). This means for instance, that the active PPS > and SPS need only be passed to drivers when activated, > and not on each frame. > > Similarly, another optimization to reduce the amount of > data passed is to have default scaling matrices in the kernel, > and allow applications to not pass them, and have the drivers > fallback to default (default or previously set?) in that case. > > I have tested these things, but haven't posted patches because > I'm not entirely sure how well-defined and specified this behavior > currently is. It is something to keep in mind though, as a future > optimization. > > Thanks! > Ezequiel > > > Best regards, > > Jonas > > > > > + }, > > > > > > { > > > > > > .cfg = { > > > > > > .id = V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE, > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h > > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index > > > 96765555ab8a..93c843ae14bb 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h > > > @@ -62,6 +62,7 @@ struct cedrus_h264_run { > > > > > > const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix; > > > const struct v4l2_ctrl_h264_slice_params *slice_params; > > > const struct v4l2_ctrl_h264_sps *sps; > > > > > > + const struct v4l2_ctrl_h264_pred_weights *pred_weights; > > > > > > }; > > > > > > struct cedrus_mpeg2_run { > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index > > > 58c48e4fdfe9..6385026d1b6b 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > > @@ -57,6 +57,8 @@ void cedrus_device_run(void *priv) > > > > > > V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS); > > > > > > run.h264.sps = cedrus_find_control_data(ctx, > > > > > > V4L2_CID_MPEG_VIDEO_H264_SPS); > > > > > > + run.h264.pred_weights = cedrus_find_control_data(ctx, > > > + V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS); > > > > > > break; > > > > > > case V4L2_PIX_FMT_HEVC_SLICE: > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > > cce527bbdf86..a9ba78b15907 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > @@ -256,10 +256,8 @@ static void cedrus_write_scaling_lists(struct > > > cedrus_ctx *ctx,> > > > > static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx, > > > > > > struct cedrus_run *run) > > > > > > { > > > > > > - const struct v4l2_ctrl_h264_slice_params *slice = > > > - run->h264.slice_params; > > > - const struct v4l2_h264_pred_weight_table *pred_weight = > > > - &slice->pred_weight_table; > > > + const struct v4l2_ctrl_h264_pred_weights *pred_weight = > > > + run->h264.pred_weights; > > > > > > struct cedrus_dev *dev = ctx->dev; > > > int i, j, k; > > > > > > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h > > > index 4c0bb7f5fb05..54cd9bec0b23 100644 > > > --- a/include/media/h264-ctrls.h > > > +++ b/include/media/h264-ctrls.h > > > @@ -36,6 +36,7 @@ > > > > > > #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS > > > (V4L2_CID_MPEG_BASE+1004) #define V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE > > > (V4L2_CID_MPEG_BASE+1005) #define V4L2_CID_MPEG_VIDEO_H264_START_CODE > > > (V4L2_CID_MPEG_BASE+1006)> > > > > +#define V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS > > > (V4L2_CID_MPEG_BASE+1007)> > > > > /* enum v4l2_ctrl_type type values */ > > > #define V4L2_CTRL_TYPE_H264_SPS 0x0110 > > > > > > @@ -43,6 +44,7 @@ > > > > > > #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX 0x0112 > > > #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS 0x0113 > > > #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS 0x0114 > > > > > > +#define V4L2_CTRL_TYPE_H264_PRED_WEIGHTS 0x0115 > > > > > > enum v4l2_mpeg_video_h264_decode_mode { > > > > > > V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED, > > > > > > @@ -125,7 +127,7 @@ struct v4l2_h264_weight_factors { > > > > > > __s16 chroma_offset[32][2]; > > > > > > }; > > > > > > -struct v4l2_h264_pred_weight_table { > > > +struct v4l2_ctrl_h264_pred_weights { > > > > > > __u16 luma_log2_weight_denom; > > > __u16 chroma_log2_weight_denom; > > > struct v4l2_h264_weight_factors weight_factors[2]; > > > > > > @@ -177,7 +179,6 @@ struct v4l2_ctrl_h264_slice_params { > > > > > > __s32 delta_pic_order_cnt0; > > > __s32 delta_pic_order_cnt1; > > > > > > - struct v4l2_h264_pred_weight_table pred_weight_table; > > > > > > /* Size in bits of dec_ref_pic_marking() syntax element. */ > > > __u32 dec_ref_pic_marking_bit_size; > > > /* Size in bits of pic order count syntax. */ > > > > > > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > > > index f40e2cbb21d3..cb25f345e9ad 100644 > > > --- a/include/media/v4l2-ctrls.h > > > +++ b/include/media/v4l2-ctrls.h > > > @@ -51,6 +51,7 @@ struct video_device; > > > > > > * @p_h264_scaling_matrix: Pointer to a struct > > > v4l2_ctrl_h264_scaling_matrix. * @p_h264_slice_params: Pointer to > > > a struct v4l2_ctrl_h264_slice_params. * @p_h264_decode_params: > > > Pointer to a struct v4l2_ctrl_h264_decode_params.> > > > > + * @p_h264_pred_weights: Pointer to a struct > > > v4l2_ctrl_h264_pred_weights.> > > > > * @p_vp8_frame_header: Pointer to a VP8 frame header > > > structure. * @p_hevc_sps: Pointer to an HEVC > > > sequence parameter set structure. * @p_hevc_pps: > > > Pointer to an HEVC picture parameter set structure.> > > > > @@ -74,6 +75,7 @@ union v4l2_ctrl_ptr { > > > > > > struct v4l2_ctrl_h264_scaling_matrix *p_h264_scaling_matrix; > > > struct v4l2_ctrl_h264_slice_params *p_h264_slice_params; > > > struct v4l2_ctrl_h264_decode_params *p_h264_decode_params; > > > > > > + struct v4l2_ctrl_h264_pred_weights *p_h264_pred_weights; > > > > > > struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header; > > > struct v4l2_ctrl_hevc_sps *p_hevc_sps; > > > struct v4l2_ctrl_hevc_pps *p_hevc_pps;
On Sun, 2020-08-09 at 23:11 +0200, Jernej Škrabec wrote: > Dne nedelja, 09. avgust 2020 ob 15:55:50 CEST je Ezequiel Garcia napisal(a): > > On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: > > > On 2020-08-06 17:12, Ezequiel Garcia wrote: > > > > The prediction weight parameters are only required under > > > > certain conditions, which depend on slice header parameters. > > > > > > > > As specified in section 7.3.3 Slice header syntax, the prediction > > > > weight table is present if: > > > > > > > > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ > > > > (weighted_bipred_idc == 1 && slice_type == B)) > > > > > > Maybe a macro can be added to help check this contition? > > > > > > Something like this maybe: > > > > > > #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ > > > > > > ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ > > > > > > ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ > > > > > > ((pps)->weighted_bipred_idc == 1 && \ > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) > > > > Yeah, that could make sense. > > > > Note that the biggest value in having the prediction weight table > > separated is to allow applications to skip setting this largish control, > > reducing the amount of data that needs to be passed from userspace > > -- especially when not needed :-) > > > > > > Given its size, it makes sense to move this table to its control, > > > > so applications can avoid passing it if the slice doesn't specify it. > > > > > > > > Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. > > > > With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight > > > > is 772 bytes. > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > --- > > > > v2: Fix missing Cedrus changes and mssing control declaration, > > > > > > > > as noted by Hans and Jernej. > > > > > > > > --- > > > > > > > > .../media/v4l/ext-ctrls-codec.rst | 19 ++++++++++++------- > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ > > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ > > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- > > > > include/media/h264-ctrls.h | 5 +++-- > > > > include/media/v4l2-ctrls.h | 2 ++ > > > > 8 files changed, 37 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index > > > > d1438b1e259f..c36ce5a95fc5 100644 > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > @@ -1879,18 +1879,23 @@ enum > > > > v4l2_mpeg_video_h264_hierarchical_coding_type -> > > > > > - 0x00000008 > > > > - > > > > > > > > -``Prediction Weight Table`` > > > > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` > > > > + Prediction weight table defined according to :ref:`h264`, > > > > + section 7.4.3.2 "Prediction Weight Table Semantics". > > > > + The prediction weight table must be passed by applications > > > > + under the conditions explained in section 7.3.3 "Slice header > > > > + syntax". > > > > > > > > - The bitstream parameters are defined according to :ref:`h264`, > > > > - section 7.4.3.2 "Prediction Weight Table Semantics". For further > > > > - documentation, refer to the above specification, unless there is > > > > - an explicit comment stating otherwise. > > > > + .. note:: > > > > + > > > > + This compound control is not yet part of the public kernel API > > > > and > > > > + it is expected to change. > > > > > > > > -.. c:type:: v4l2_h264_pred_weight_table > > > > +.. c:type:: v4l2_ctrl_h264_pred_weights > > > > > > > > .. cssclass:: longtable > > > > > > > > -.. flat-table:: struct v4l2_h264_pred_weight_table > > > > +.. flat-table:: struct v4l2_ctrl_h264_pred_weights > > > > > > > > :header-rows: 0 > > > > :stub-columns: 0 > > > > :widths: 1 1 2 > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > b/drivers/media/v4l2-core/v4l2-ctrls.c index 3f3fbcd60cc6..76c8dc8fb31c > > > > 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > > @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return > > > > "H264 Decode Parameters"; case > > > > V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return "H264 > > > > Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: > > > > return "H264 Start Code";> > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return > > > > "H264 Prediction Weight Table";> > > > > > 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";> > > > > > @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, > > > > enum v4l2_ctrl_type *type,> > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > > > *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; > > > > break; > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > > > + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; > > > > + break; > > > > > > > > case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: > > > > *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; > > > > break; > > > > > > > > @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct > > > > v4l2_ctrl *ctrl, u32 idx,> > > > > > case V4L2_CTRL_TYPE_H264_SPS: > > > > case V4L2_CTRL_TYPE_H264_PPS: > > > > > > > > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > break; > > > > > > > > @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct > > > > v4l2_ctrl_handler *hdl,> > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > elem_size = sizeof(struct v4l2_ctrl_h264_decode_params); > > > > break; > > > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > + elem_size = sizeof(struct v4l2_ctrl_h264_pred_weights); > > > > + break; > > > > > > > > case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: > > > > elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); > > > > break; > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > > > bc27f9430eeb..027cdd1be5a0 100644 > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > @@ -78,6 +78,13 @@ static const struct cedrus_control cedrus_controls[] > > > > = { > > > > > > > > .codec = CEDRUS_CODEC_H264, > > > > .required = true, > > > > > > > > }, > > > > > > > > + { > > > > + .cfg = { > > > > + .id = V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > > > > + }, > > > > + .codec = CEDRUS_CODEC_H264, > > > > + .required = true, > > > > > > This should probably be false if this control is to be optional as implied > > > by the commit message. > > > > Well, the control is optional if the driver implements it as optional, > > which Cedrus isn't currently doing :-) > > Why do you think so? Prediction weights are filled only when they are > needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ > sunxi/cedrus/cedrus_h264.c#L370 > Right, but that should be changed to be really optional. How does the driver reject/fail the request if the table is NULL? In any case, I don't think it's necessarily something we need to tackle now. Thanks, Ezequiel
On 2020-08-10 14:57, Ezequiel Garcia wrote: > On Sun, 2020-08-09 at 23:11 +0200, Jernej Škrabec wrote: >> Dne nedelja, 09. avgust 2020 ob 15:55:50 CEST je Ezequiel Garcia napisal(a): >>> On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: >>>> On 2020-08-06 17:12, Ezequiel Garcia wrote: >>>>> The prediction weight parameters are only required under >>>>> certain conditions, which depend on slice header parameters. >>>>> >>>>> As specified in section 7.3.3 Slice header syntax, the prediction >>>>> weight table is present if: >>>>> >>>>> ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ >>>>> (weighted_bipred_idc == 1 && slice_type == B)) >>>> >>>> Maybe a macro can be added to help check this contition? >>>> >>>> Something like this maybe: >>>> >>>> #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ >>>> >>>> ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ >>>> >>>> ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ >>>> >>>> (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ >>>> >>>> ((pps)->weighted_bipred_idc == 1 && \ >>>> >>>> (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) >>> >>> Yeah, that could make sense. >>> >>> Note that the biggest value in having the prediction weight table >>> separated is to allow applications to skip setting this largish control, >>> reducing the amount of data that needs to be passed from userspace >>> -- especially when not needed :-) >>> >>>>> Given its size, it makes sense to move this table to its control, >>>>> so applications can avoid passing it if the slice doesn't specify it. >>>>> >>>>> Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. >>>>> With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight >>>>> is 772 bytes. >>>>> >>>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>>>> --- >>>>> v2: Fix missing Cedrus changes and mssing control declaration, >>>>> >>>>> as noted by Hans and Jernej. >>>>> >>>>> --- >>>>> >>>>> .../media/v4l/ext-ctrls-codec.rst | 19 ++++++++++++------- >>>>> drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ >>>>> drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ >>>>> drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + >>>>> .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ >>>>> .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- >>>>> include/media/h264-ctrls.h | 5 +++-- >>>>> include/media/v4l2-ctrls.h | 2 ++ >>>>> 8 files changed, 37 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >>>>> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index >>>>> d1438b1e259f..c36ce5a95fc5 100644 >>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >>>>> @@ -1879,18 +1879,23 @@ enum >>>>> v4l2_mpeg_video_h264_hierarchical_coding_type -> > >>>>> - 0x00000008 >>>>> - >>>>> >>>>> -``Prediction Weight Table`` >>>>> +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` >>>>> + Prediction weight table defined according to :ref:`h264`, >>>>> + section 7.4.3.2 "Prediction Weight Table Semantics". >>>>> + The prediction weight table must be passed by applications >>>>> + under the conditions explained in section 7.3.3 "Slice header >>>>> + syntax". >>>>> >>>>> - The bitstream parameters are defined according to :ref:`h264`, >>>>> - section 7.4.3.2 "Prediction Weight Table Semantics". For further >>>>> - documentation, refer to the above specification, unless there is >>>>> - an explicit comment stating otherwise. >>>>> + .. note:: >>>>> + >>>>> + This compound control is not yet part of the public kernel API >>>>> and >>>>> + it is expected to change. >>>>> >>>>> -.. c:type:: v4l2_h264_pred_weight_table >>>>> +.. c:type:: v4l2_ctrl_h264_pred_weights >>>>> >>>>> .. cssclass:: longtable >>>>> >>>>> -.. flat-table:: struct v4l2_h264_pred_weight_table >>>>> +.. flat-table:: struct v4l2_ctrl_h264_pred_weights >>>>> >>>>> :header-rows: 0 >>>>> :stub-columns: 0 >>>>> :widths: 1 1 2 >>>>> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c >>>>> b/drivers/media/v4l2-core/v4l2-ctrls.c index 3f3fbcd60cc6..76c8dc8fb31c >>>>> 100644 >>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >>>>> @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) >>>>> >>>>> case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return >>>>> "H264 Decode Parameters"; case >>>>> V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return "H264 >>>>> Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: >>>>> return "H264 Start Code";> > >>>>> + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return >>>>> "H264 Prediction Weight Table";> > >>>>> 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";> > >>>>> @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, >>>>> enum v4l2_ctrl_type *type,> > >>>>> case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: >>>>> *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; >>>>> break; >>>>> >>>>> + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: >>>>> + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; >>>>> + break; >>>>> >>>>> case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: >>>>> *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; >>>>> break; >>>>> >>>>> @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct >>>>> v4l2_ctrl *ctrl, u32 idx,> > >>>>> case V4L2_CTRL_TYPE_H264_SPS: >>>>> case V4L2_CTRL_TYPE_H264_PPS: >>>>> >>>>> case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: >>>>> + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: >>>>> case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: >>>>> >>>>> case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: >>>>> break; >>>>> >>>>> @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct >>>>> v4l2_ctrl_handler *hdl,> > >>>>> case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: >>>>> elem_size = sizeof(struct v4l2_ctrl_h264_decode_params); >>>>> break; >>>>> >>>>> + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: >>>>> + elem_size = sizeof(struct v4l2_ctrl_h264_pred_weights); >>>>> + break; >>>>> >>>>> case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: >>>>> elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); >>>>> break; >>>>> >>>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c >>>>> b/drivers/staging/media/sunxi/cedrus/cedrus.c index >>>>> bc27f9430eeb..027cdd1be5a0 100644 >>>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c >>>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c >>>>> @@ -78,6 +78,13 @@ static const struct cedrus_control cedrus_controls[] >>>>> = { >>>>> >>>>> .codec = CEDRUS_CODEC_H264, >>>>> .required = true, >>>>> >>>>> }, >>>>> >>>>> + { >>>>> + .cfg = { >>>>> + .id = V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, >>>>> + }, >>>>> + .codec = CEDRUS_CODEC_H264, >>>>> + .required = true, >>>> >>>> This should probably be false if this control is to be optional as implied >>>> by the commit message. >>> >>> Well, the control is optional if the driver implements it as optional, >>> which Cedrus isn't currently doing :-) >> >> Why do you think so? Prediction weights are filled only when they are >> needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ >> sunxi/cedrus/cedrus_h264.c#L370 >> > > Right, but that should be changed to be really optional. > How does the driver reject/fail the request if the table is NULL? > > In any case, I don't think it's necessarily something we need > to tackle now. I do not fully follow, the commit message state following: Note that the biggest value in having the prediction weight table separated is to allow applications to skip setting this largish control Yet the driver still require this new control to be included in the request thanks to the .required = true statement. (if i understand the code correctly) So applications still need to set this largish control? Best regards, Jonas > > Thanks, > Ezequiel >
On Mon, 2020-08-10 at 14:55 +0000, Jonas Karlman wrote: > On 2020-08-10 14:57, Ezequiel Garcia wrote: > > On Sun, 2020-08-09 at 23:11 +0200, Jernej Škrabec wrote: > > > Dne nedelja, 09. avgust 2020 ob 15:55:50 CEST je Ezequiel Garcia napisal(a): > > > > On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: > > > > > On 2020-08-06 17:12, Ezequiel Garcia wrote: > > > > > > The prediction weight parameters are only required under > > > > > > certain conditions, which depend on slice header parameters. > > > > > > > > > > > > As specified in section 7.3.3 Slice header syntax, the prediction > > > > > > weight table is present if: > > > > > > > > > > > > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ > > > > > > (weighted_bipred_idc == 1 && slice_type == B)) > > > > > > > > > > Maybe a macro can be added to help check this contition? > > > > > > > > > > Something like this maybe: > > > > > > > > > > #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ > > > > > > > > > > ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ > > > > > > > > > > ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ > > > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ > > > > > > > > > > ((pps)->weighted_bipred_idc == 1 && \ > > > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) > > > > > > > > Yeah, that could make sense. > > > > > > > > Note that the biggest value in having the prediction weight table > > > > separated is to allow applications to skip setting this largish control, > > > > reducing the amount of data that needs to be passed from userspace > > > > -- especially when not needed :-) > > > > > > > > > > Given its size, it makes sense to move this table to its control, > > > > > > so applications can avoid passing it if the slice doesn't specify it. > > > > > > > > > > > > Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. > > > > > > With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight > > > > > > is 772 bytes. > > > > > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > > > --- > > > > > > v2: Fix missing Cedrus changes and mssing control declaration, > > > > > > > > > > > > as noted by Hans and Jernej. > > > > > > > > > > > > --- > > > > > > > > > > > > .../media/v4l/ext-ctrls-codec.rst | 19 ++++++++++++------- > > > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > > > > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > > > > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- > > > > > > include/media/h264-ctrls.h | 5 +++-- > > > > > > include/media/v4l2-ctrls.h | 2 ++ > > > > > > 8 files changed, 37 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index > > > > > > d1438b1e259f..c36ce5a95fc5 100644 > > > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > @@ -1879,18 +1879,23 @@ enum > > > > > > v4l2_mpeg_video_h264_hierarchical_coding_type -> > > > > > > > - 0x00000008 > > > > > > - > > > > > > > > > > > > -``Prediction Weight Table`` > > > > > > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` > > > > > > + Prediction weight table defined according to :ref:`h264`, > > > > > > + section 7.4.3.2 "Prediction Weight Table Semantics". > > > > > > + The prediction weight table must be passed by applications > > > > > > + under the conditions explained in section 7.3.3 "Slice header > > > > > > + syntax". > > > > > > > > > > > > - The bitstream parameters are defined according to :ref:`h264`, > > > > > > - section 7.4.3.2 "Prediction Weight Table Semantics". For further > > > > > > - documentation, refer to the above specification, unless there is > > > > > > - an explicit comment stating otherwise. > > > > > > + .. note:: > > > > > > + > > > > > > + This compound control is not yet part of the public kernel API > > > > > > and > > > > > > + it is expected to change. > > > > > > > > > > > > -.. c:type:: v4l2_h264_pred_weight_table > > > > > > +.. c:type:: v4l2_ctrl_h264_pred_weights > > > > > > > > > > > > .. cssclass:: longtable > > > > > > > > > > > > -.. flat-table:: struct v4l2_h264_pred_weight_table > > > > > > +.. flat-table:: struct v4l2_ctrl_h264_pred_weights > > > > > > > > > > > > :header-rows: 0 > > > > > > :stub-columns: 0 > > > > > > :widths: 1 1 2 > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > b/drivers/media/v4l2-core/v4l2-ctrls.c index 3f3fbcd60cc6..76c8dc8fb31c > > > > > > 100644 > > > > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return > > > > > > "H264 Decode Parameters"; case > > > > > > V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return "H264 > > > > > > Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: > > > > > > return "H264 Start Code";> > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return > > > > > > "H264 Prediction Weight Table";> > > > > > > > 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";> > > > > > > > @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, > > > > > > enum v4l2_ctrl_type *type,> > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > > > > > *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; > > > > > > break; > > > > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > > > > > + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; > > > > > > + break; > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: > > > > > > *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; > > > > > > break; > > > > > > > > > > > > @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct > > > > > > v4l2_ctrl *ctrl, u32 idx,> > > > > > > > case V4L2_CTRL_TYPE_H264_SPS: > > > > > > case V4L2_CTRL_TYPE_H264_PPS: > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > > break; > > > > > > > > > > > > @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct > > > > > > v4l2_ctrl_handler *hdl,> > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > > elem_size = sizeof(struct v4l2_ctrl_h264_decode_params); > > > > > > break; > > > > > > > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > > + elem_size = sizeof(struct v4l2_ctrl_h264_pred_weights); > > > > > > + break; > > > > > > > > > > > > case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: > > > > > > elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); > > > > > > break; > > > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > > > > > bc27f9430eeb..027cdd1be5a0 100644 > > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > @@ -78,6 +78,13 @@ static const struct cedrus_control cedrus_controls[] > > > > > > = { > > > > > > > > > > > > .codec = CEDRUS_CODEC_H264, > > > > > > .required = true, > > > > > > > > > > > > }, > > > > > > > > > > > > + { > > > > > > + .cfg = { > > > > > > + .id = V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > > > > > > + }, > > > > > > + .codec = CEDRUS_CODEC_H264, > > > > > > + .required = true, > > > > > > > > > > This should probably be false if this control is to be optional as implied > > > > > by the commit message. > > > > > > > > Well, the control is optional if the driver implements it as optional, > > > > which Cedrus isn't currently doing :-) > > > > > > Why do you think so? Prediction weights are filled only when they are > > > needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ > > > sunxi/cedrus/cedrus_h264.c#L370 > > > > > > > Right, but that should be changed to be really optional. > > How does the driver reject/fail the request if the table is NULL? > > > > In any case, I don't think it's necessarily something we need > > to tackle now. > > I do not fully follow, the commit message state following: > > Note that the biggest value in having the prediction weight table > separated is to allow applications to skip setting this largish control > This is not exactly what the commit message says, but yeah that's the idea. > Yet the driver still require this new control to be included in the request > thanks to the .required = true statement. (if i understand the code correctly) > > So applications still need to set this largish control? > This is a uAPI change that paves the way for Cedrus to make the control optional. The series doesn't take care of it, but it prepares the road for it. Since we are not stabilizing the uAPI (yet), I think we still have some room to make this change in steps: first we merge the new control and then we add the needed changes to Cedrus? Does that make sense? > Best regards, > Jonas > > > Thanks, > > Ezequiel > >
On 2020-08-10 17:28, Ezequiel Garcia wrote: > On Mon, 2020-08-10 at 14:55 +0000, Jonas Karlman wrote: >> On 2020-08-10 14:57, Ezequiel Garcia wrote: >>> On Sun, 2020-08-09 at 23:11 +0200, Jernej Škrabec wrote: >>>> Dne nedelja, 09. avgust 2020 ob 15:55:50 CEST je Ezequiel Garcia napisal(a): >>>>> On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: >>>>>> On 2020-08-06 17:12, Ezequiel Garcia wrote: >>>>>>> The prediction weight parameters are only required under >>>>>>> certain conditions, which depend on slice header parameters. >>>>>>> >>>>>>> As specified in section 7.3.3 Slice header syntax, the prediction >>>>>>> weight table is present if: >>>>>>> >>>>>>> ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ >>>>>>> (weighted_bipred_idc == 1 && slice_type == B)) >>>>>> >>>>>> Maybe a macro can be added to help check this contition? >>>>>> >>>>>> Something like this maybe: >>>>>> >>>>>> #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ >>>>>> >>>>>> ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ >>>>>> >>>>>> ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ >>>>>> >>>>>> (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ >>>>>> >>>>>> ((pps)->weighted_bipred_idc == 1 && \ >>>>>> >>>>>> (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) >>>>> >>>>> Yeah, that could make sense. >>>>> >>>>> Note that the biggest value in having the prediction weight table >>>>> separated is to allow applications to skip setting this largish control, >>>>> reducing the amount of data that needs to be passed from userspace >>>>> -- especially when not needed :-) >>>>> >>>>>>> Given its size, it makes sense to move this table to its control, >>>>>>> so applications can avoid passing it if the slice doesn't specify it. >>>>>>> >>>>>>> Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. >>>>>>> With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight >>>>>>> is 772 bytes. >>>>>>> >>>>>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>>>>>> --- >>>>>>> v2: Fix missing Cedrus changes and mssing control declaration, >>>>>>> >>>>>>> as noted by Hans and Jernej. >>>>>>> >>>>>>> --- >>>>>>> >>>>>>> .../media/v4l/ext-ctrls-codec.rst | 19 ++++++++++++------- >>>>>>> drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ >>>>>>> drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ >>>>>>> drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + >>>>>>> .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ >>>>>>> .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- >>>>>>> include/media/h264-ctrls.h | 5 +++-- >>>>>>> include/media/v4l2-ctrls.h | 2 ++ >>>>>>> 8 files changed, 37 insertions(+), 13 deletions(-) >>>>>>> >>>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >>>>>>> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index >>>>>>> d1438b1e259f..c36ce5a95fc5 100644 >>>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >>>>>>> @@ -1879,18 +1879,23 @@ enum >>>>>>> v4l2_mpeg_video_h264_hierarchical_coding_type -> > >>>>>>> - 0x00000008 >>>>>>> - >>>>>>> >>>>>>> -``Prediction Weight Table`` >>>>>>> +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` >>>>>>> + Prediction weight table defined according to :ref:`h264`, >>>>>>> + section 7.4.3.2 "Prediction Weight Table Semantics". >>>>>>> + The prediction weight table must be passed by applications >>>>>>> + under the conditions explained in section 7.3.3 "Slice header >>>>>>> + syntax". >>>>>>> >>>>>>> - The bitstream parameters are defined according to :ref:`h264`, >>>>>>> - section 7.4.3.2 "Prediction Weight Table Semantics". For further >>>>>>> - documentation, refer to the above specification, unless there is >>>>>>> - an explicit comment stating otherwise. >>>>>>> + .. note:: >>>>>>> + >>>>>>> + This compound control is not yet part of the public kernel API >>>>>>> and >>>>>>> + it is expected to change. >>>>>>> >>>>>>> -.. c:type:: v4l2_h264_pred_weight_table >>>>>>> +.. c:type:: v4l2_ctrl_h264_pred_weights >>>>>>> >>>>>>> .. cssclass:: longtable >>>>>>> >>>>>>> -.. flat-table:: struct v4l2_h264_pred_weight_table >>>>>>> +.. flat-table:: struct v4l2_ctrl_h264_pred_weights >>>>>>> >>>>>>> :header-rows: 0 >>>>>>> :stub-columns: 0 >>>>>>> :widths: 1 1 2 >>>>>>> >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c >>>>>>> b/drivers/media/v4l2-core/v4l2-ctrls.c index 3f3fbcd60cc6..76c8dc8fb31c >>>>>>> 100644 >>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >>>>>>> @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) >>>>>>> >>>>>>> case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return >>>>>>> "H264 Decode Parameters"; case >>>>>>> V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return "H264 >>>>>>> Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: >>>>>>> return "H264 Start Code";> > >>>>>>> + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return >>>>>>> "H264 Prediction Weight Table";> > >>>>>>> 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";> > >>>>>>> @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, >>>>>>> enum v4l2_ctrl_type *type,> > >>>>>>> case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: >>>>>>> *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; >>>>>>> break; >>>>>>> >>>>>>> + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: >>>>>>> + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; >>>>>>> + break; >>>>>>> >>>>>>> case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: >>>>>>> *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; >>>>>>> break; >>>>>>> >>>>>>> @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct >>>>>>> v4l2_ctrl *ctrl, u32 idx,> > >>>>>>> case V4L2_CTRL_TYPE_H264_SPS: >>>>>>> case V4L2_CTRL_TYPE_H264_PPS: >>>>>>> >>>>>>> case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: >>>>>>> + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: >>>>>>> case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: >>>>>>> >>>>>>> case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: >>>>>>> break; >>>>>>> >>>>>>> @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct >>>>>>> v4l2_ctrl_handler *hdl,> > >>>>>>> case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: >>>>>>> elem_size = sizeof(struct v4l2_ctrl_h264_decode_params); >>>>>>> break; >>>>>>> >>>>>>> + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: >>>>>>> + elem_size = sizeof(struct v4l2_ctrl_h264_pred_weights); >>>>>>> + break; >>>>>>> >>>>>>> case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: >>>>>>> elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); >>>>>>> break; >>>>>>> >>>>>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c >>>>>>> b/drivers/staging/media/sunxi/cedrus/cedrus.c index >>>>>>> bc27f9430eeb..027cdd1be5a0 100644 >>>>>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c >>>>>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c >>>>>>> @@ -78,6 +78,13 @@ static const struct cedrus_control cedrus_controls[] >>>>>>> = { >>>>>>> >>>>>>> .codec = CEDRUS_CODEC_H264, >>>>>>> .required = true, >>>>>>> >>>>>>> }, >>>>>>> >>>>>>> + { >>>>>>> + .cfg = { >>>>>>> + .id = V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, >>>>>>> + }, >>>>>>> + .codec = CEDRUS_CODEC_H264, >>>>>>> + .required = true, >>>>>> >>>>>> This should probably be false if this control is to be optional as implied >>>>>> by the commit message. >>>>> >>>>> Well, the control is optional if the driver implements it as optional, >>>>> which Cedrus isn't currently doing :-) >>>> >>>> Why do you think so? Prediction weights are filled only when they are >>>> needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ >>>> sunxi/cedrus/cedrus_h264.c#L370 >>>> >>> >>> Right, but that should be changed to be really optional. >>> How does the driver reject/fail the request if the table is NULL? >>> >>> In any case, I don't think it's necessarily something we need >>> to tackle now. >> >> I do not fully follow, the commit message state following: >> >> Note that the biggest value in having the prediction weight table >> separated is to allow applications to skip setting this largish control >> > > This is not exactly what the commit message says, but yeah > that's the idea. Hehe, I copied your reply instead of commit message / doc changes :-) > >> Yet the driver still require this new control to be included in the request >> thanks to the .required = true statement. (if i understand the code correctly) >> >> So applications still need to set this largish control? >> > > This is a uAPI change that paves the way for Cedrus to make the control > optional. The series doesn't take care of it, but it prepares the road > for it. > > Since we are not stabilizing the uAPI (yet), I think we still have > some room to make this change in steps: first we merge the new control > and then we add the needed changes to Cedrus? > > Does that make sense? Sure, make sense, I will rephrase it as questions instead :-) What is not fully clear to me is if this new ctrl should be considered optional or required from userspace point of view. Will there be a way for userspace to know if a ctrl is optional or not? If I implement uapi changes as suggested in this patch in ffmpeg, i.e. only set weight table ctrl when data if it is present in slice header, then decoding stops working for cedrus when weight table is not present. Should I just play it safe and continue to always set the new ctrl for slice based decoding and treat it as required? Or are we saying that it should be optional and cedrus is just not following the uapi after this series? Best regards, Jonas > >> Best regards, >> Jonas >> >>> Thanks, >>> Ezequiel >>> > >
Dne ponedeljek, 10. avgust 2020 ob 14:57:17 CEST je Ezequiel Garcia napisal(a): > On Sun, 2020-08-09 at 23:11 +0200, Jernej Škrabec wrote: > > Dne nedelja, 09. avgust 2020 ob 15:55:50 CEST je Ezequiel Garcia napisal(a): > > > On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: > > > > On 2020-08-06 17:12, Ezequiel Garcia wrote: > > > > > The prediction weight parameters are only required under > > > > > certain conditions, which depend on slice header parameters. > > > > > > > > > > As specified in section 7.3.3 Slice header syntax, the prediction > > > > > weight table is present if: > > > > > > > > > > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ > > > > > (weighted_bipred_idc == 1 && slice_type == B)) > > > > > > > > Maybe a macro can be added to help check this contition? > > > > > > > > Something like this maybe: > > > > > > > > #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ > > > > > > > > ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ > > > > > > > > ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ > > > > > > > > ((pps)->weighted_bipred_idc == 1 && \ > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) > > > > > > Yeah, that could make sense. > > > > > > Note that the biggest value in having the prediction weight table > > > separated is to allow applications to skip setting this largish > > > control, > > > reducing the amount of data that needs to be passed from userspace > > > -- especially when not needed :-) > > > > > > > > Given its size, it makes sense to move this table to its control, > > > > > so applications can avoid passing it if the slice doesn't specify > > > > > it. > > > > > > > > > > Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. > > > > > With this change, it's 188 bytes and struct > > > > > v4l2_ctrl_h264_pred_weight > > > > > is 772 bytes. > > > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > > --- > > > > > v2: Fix missing Cedrus changes and mssing control declaration, > > > > > > > > > > as noted by Hans and Jernej. > > > > > > > > > > --- > > > > > > > > > > .../media/v4l/ext-ctrls-codec.rst | 19 > > > > > ++++++++++++------- > > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ > > > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ > > > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > > > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > > > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- > > > > > include/media/h264-ctrls.h | 5 +++-- > > > > > include/media/v4l2-ctrls.h | 2 ++ > > > > > 8 files changed, 37 insertions(+), 13 deletions(-) > > > > > > > > > > diff --git > > > > > a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index > > > > > d1438b1e259f..c36ce5a95fc5 100644 > > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > @@ -1879,18 +1879,23 @@ enum > > > > > v4l2_mpeg_video_h264_hierarchical_coding_type -> > > > > > > > > > > > - 0x00000008 > > > > > - > > > > > > > > > > -``Prediction Weight Table`` > > > > > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` > > > > > + Prediction weight table defined according to :ref:`h264`, > > > > > + section 7.4.3.2 "Prediction Weight Table Semantics". > > > > > + The prediction weight table must be passed by applications > > > > > + under the conditions explained in section 7.3.3 "Slice header > > > > > + syntax". > > > > > > > > > > - The bitstream parameters are defined according to :ref:`h264`, > > > > > - section 7.4.3.2 "Prediction Weight Table Semantics". For > > > > > further > > > > > - documentation, refer to the above specification, unless there > > > > > is > > > > > - an explicit comment stating otherwise. > > > > > + .. note:: > > > > > + > > > > > + This compound control is not yet part of the public kernel > > > > > API > > > > > and > > > > > + it is expected to change. > > > > > > > > > > -.. c:type:: v4l2_h264_pred_weight_table > > > > > +.. c:type:: v4l2_ctrl_h264_pred_weights > > > > > > > > > > .. cssclass:: longtable > > > > > > > > > > -.. flat-table:: struct v4l2_h264_pred_weight_table > > > > > +.. flat-table:: struct v4l2_ctrl_h264_pred_weights > > > > > > > > > > :header-rows: 0 > > > > > :stub-columns: 0 > > > > > :widths: 1 1 2 > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > b/drivers/media/v4l2-core/v4l2-ctrls.c index > > > > > 3f3fbcd60cc6..76c8dc8fb31c > > > > > 100644 > > > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return > > > > > "H264 Decode Parameters"; case > > > > > V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return > > > > > "H264 > > > > > > > > > > Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: > > > > > return "H264 Start Code";> > > > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return > > > > > "H264 Prediction Weight Table";> > > > > > > > > > > > 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";> > > > > > > > > > > > @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, > > > > > enum v4l2_ctrl_type *type,> > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > > > > *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; > > > > > break; > > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > > > > + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; > > > > > + break; > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: > > > > > *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; > > > > > break; > > > > > > > > > > @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct > > > > > v4l2_ctrl *ctrl, u32 idx,> > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_SPS: > > > > > case V4L2_CTRL_TYPE_H264_PPS: > > > > > > > > > > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: > > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > break; > > > > > > > > > > @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct > > > > > v4l2_ctrl_handler *hdl,> > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > elem_size = sizeof(struct > > > > > v4l2_ctrl_h264_decode_params); > > > > > break; > > > > > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > + elem_size = sizeof(struct > > > > > v4l2_ctrl_h264_pred_weights); > > > > > + break; > > > > > > > > > > case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: > > > > > elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); > > > > > break; > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > > > > bc27f9430eeb..027cdd1be5a0 100644 > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > @@ -78,6 +78,13 @@ static const struct cedrus_control > > > > > cedrus_controls[] > > > > > = { > > > > > > > > > > .codec = CEDRUS_CODEC_H264, > > > > > .required = true, > > > > > > > > > > }, > > > > > > > > > > + { > > > > > + .cfg = { > > > > > + .id = > > > > > V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > > > > > + }, > > > > > + .codec = CEDRUS_CODEC_H264, > > > > > + .required = true, > > > > > > > > This should probably be false if this control is to be optional as > > > > implied > > > > by the commit message. > > > > > > Well, the control is optional if the driver implements it as optional, > > > which Cedrus isn't currently doing :-) > > > > Why do you think so? Prediction weights are filled only when they are > > needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/medi > > a/ sunxi/cedrus/cedrus_h264.c#L370 > > Right, but that should be changed to be really optional. > How does the driver reject/fail the request if the table is NULL? It's my understanding that pointer to this table can't be NULL. NULL would mean that there is no control with that ID registered in the driver. Best regards, Jernej > > In any case, I don't think it's necessarily something we need > to tackle now. > > Thanks, > Ezequiel
On Mon, 2020-08-10 at 19:05 +0200, Jernej Škrabec wrote: > Dne ponedeljek, 10. avgust 2020 ob 14:57:17 CEST je Ezequiel Garcia > napisal(a): > > On Sun, 2020-08-09 at 23:11 +0200, Jernej Škrabec wrote: > > > Dne nedelja, 09. avgust 2020 ob 15:55:50 CEST je Ezequiel Garcia > napisal(a): > > > > On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: > > > > > On 2020-08-06 17:12, Ezequiel Garcia wrote: > > > > > > The prediction weight parameters are only required under > > > > > > certain conditions, which depend on slice header parameters. > > > > > > > > > > > > As specified in section 7.3.3 Slice header syntax, the prediction > > > > > > weight table is present if: > > > > > > > > > > > > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ > > > > > > (weighted_bipred_idc == 1 && slice_type == B)) > > > > > > > > > > Maybe a macro can be added to help check this contition? > > > > > > > > > > Something like this maybe: > > > > > > > > > > #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ > > > > > > > > > > ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ > > > > > > > > > > ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ > > > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ > > > > > > > > > > ((pps)->weighted_bipred_idc == 1 && \ > > > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) > > > > > > > > Yeah, that could make sense. > > > > > > > > Note that the biggest value in having the prediction weight table > > > > separated is to allow applications to skip setting this largish > > > > control, > > > > reducing the amount of data that needs to be passed from userspace > > > > -- especially when not needed :-) > > > > > > > > > > Given its size, it makes sense to move this table to its control, > > > > > > so applications can avoid passing it if the slice doesn't specify > > > > > > it. > > > > > > > > > > > > Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. > > > > > > With this change, it's 188 bytes and struct > > > > > > v4l2_ctrl_h264_pred_weight > > > > > > is 772 bytes. > > > > > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > > > --- > > > > > > v2: Fix missing Cedrus changes and mssing control declaration, > > > > > > > > > > > > as noted by Hans and Jernej. > > > > > > > > > > > > --- > > > > > > > > > > > > .../media/v4l/ext-ctrls-codec.rst | 19 > > > > > > ++++++++++++------- > > > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > > > > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > > > > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- > > > > > > include/media/h264-ctrls.h | 5 +++-- > > > > > > include/media/v4l2-ctrls.h | 2 ++ > > > > > > 8 files changed, 37 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git > > > > > > a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index > > > > > > d1438b1e259f..c36ce5a95fc5 100644 > > > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > @@ -1879,18 +1879,23 @@ enum > > > > > > v4l2_mpeg_video_h264_hierarchical_coding_type -> > > > > > > > > > > > > > - 0x00000008 > > > > > > - > > > > > > > > > > > > -``Prediction Weight Table`` > > > > > > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` > > > > > > + Prediction weight table defined according to :ref:`h264`, > > > > > > + section 7.4.3.2 "Prediction Weight Table Semantics". > > > > > > + The prediction weight table must be passed by applications > > > > > > + under the conditions explained in section 7.3.3 "Slice header > > > > > > + syntax". > > > > > > > > > > > > - The bitstream parameters are defined according to :ref:`h264`, > > > > > > - section 7.4.3.2 "Prediction Weight Table Semantics". For > > > > > > further > > > > > > - documentation, refer to the above specification, unless there > > > > > > is > > > > > > - an explicit comment stating otherwise. > > > > > > + .. note:: > > > > > > + > > > > > > + This compound control is not yet part of the public kernel > > > > > > API > > > > > > and > > > > > > + it is expected to change. > > > > > > > > > > > > -.. c:type:: v4l2_h264_pred_weight_table > > > > > > +.. c:type:: v4l2_ctrl_h264_pred_weights > > > > > > > > > > > > .. cssclass:: longtable > > > > > > > > > > > > -.. flat-table:: struct v4l2_h264_pred_weight_table > > > > > > +.. flat-table:: struct v4l2_ctrl_h264_pred_weights > > > > > > > > > > > > :header-rows: 0 > > > > > > :stub-columns: 0 > > > > > > :widths: 1 1 2 > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > b/drivers/media/v4l2-core/v4l2-ctrls.c index > > > > > > 3f3fbcd60cc6..76c8dc8fb31c > > > > > > 100644 > > > > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return > > > > > > "H264 Decode Parameters"; case > > > > > > V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return > > > > > > "H264 > > > > > > > > > > > > Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: > > > > > > return "H264 Start Code";> > > > > > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return > > > > > > "H264 Prediction Weight Table";> > > > > > > > > > > > > > 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";> > > > > > > > > > > > > > @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, > > > > > > enum v4l2_ctrl_type *type,> > > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > > > > > *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; > > > > > > break; > > > > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > > > > > + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; > > > > > > + break; > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: > > > > > > *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; > > > > > > break; > > > > > > > > > > > > @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct > > > > > > v4l2_ctrl *ctrl, u32 idx,> > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_SPS: > > > > > > case V4L2_CTRL_TYPE_H264_PPS: > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > > break; > > > > > > > > > > > > @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct > > > > > > v4l2_ctrl_handler *hdl,> > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > > elem_size = sizeof(struct > > > > > > v4l2_ctrl_h264_decode_params); > > > > > > break; > > > > > > > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > > + elem_size = sizeof(struct > > > > > > v4l2_ctrl_h264_pred_weights); > > > > > > + break; > > > > > > > > > > > > case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: > > > > > > elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); > > > > > > break; > > > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > > > > > bc27f9430eeb..027cdd1be5a0 100644 > > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > @@ -78,6 +78,13 @@ static const struct cedrus_control > > > > > > cedrus_controls[] > > > > > > = { > > > > > > > > > > > > .codec = CEDRUS_CODEC_H264, > > > > > > .required = true, > > > > > > > > > > > > }, > > > > > > > > > > > > + { > > > > > > + .cfg = { > > > > > > + .id = > > > > > > V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > > > > > > + }, > > > > > > + .codec = CEDRUS_CODEC_H264, > > > > > > + .required = true, > > > > > > > > > > This should probably be false if this control is to be optional as > > > > > implied > > > > > by the commit message. > > > > > > > > Well, the control is optional if the driver implements it as optional, > > > > which Cedrus isn't currently doing :-) > > > > > > Why do you think so? Prediction weights are filled only when they are > > > needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/medi > > > a/ sunxi/cedrus/cedrus_h264.c#L370 > > > > Right, but that should be changed to be really optional. > > How does the driver reject/fail the request if the table is NULL? > > It's my understanding that pointer to this table can't be NULL. NULL would > mean that there is no control with that ID registered in the driver. > Hm, I'm starting to think you are right. So, does this mean the default quantization matrix here is bogus? if (quantization && quantization->load_intra_quantiser_matrix) matrix = quantization->intra_quantiser_matrix; else matrix = intra_quantization_matrix_default; Thanks, Ezequiel > Best regards, > Jernej > > > In any case, I don't think it's necessarily something we need > > to tackle now. > > > > Thanks, > > Ezequiel > > >
Dne ponedeljek, 10. avgust 2020 ob 21:30:59 CEST je Ezequiel Garcia napisal(a): > On Mon, 2020-08-10 at 19:05 +0200, Jernej Škrabec wrote: > > Dne ponedeljek, 10. avgust 2020 ob 14:57:17 CEST je Ezequiel Garcia > > > > napisal(a): > > > On Sun, 2020-08-09 at 23:11 +0200, Jernej Škrabec wrote: > > > > Dne nedelja, 09. avgust 2020 ob 15:55:50 CEST je Ezequiel Garcia > > > > napisal(a): > > > > > On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: > > > > > > On 2020-08-06 17:12, Ezequiel Garcia wrote: > > > > > > > The prediction weight parameters are only required under > > > > > > > certain conditions, which depend on slice header parameters. > > > > > > > > > > > > > > As specified in section 7.3.3 Slice header syntax, the > > > > > > > prediction > > > > > > > weight table is present if: > > > > > > > > > > > > > > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) > > > > > > > || \ > > > > > > > (weighted_bipred_idc == 1 && slice_type == B)) > > > > > > > > > > > > Maybe a macro can be added to help check this contition? > > > > > > > > > > > > Something like this maybe: > > > > > > > > > > > > #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ > > > > > > > > > > > > ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ > > > > > > > > > > > > ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ > > > > > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ > > > > > > > > > > > > ((pps)->weighted_bipred_idc == 1 && \ > > > > > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) > > > > > > > > > > Yeah, that could make sense. > > > > > > > > > > Note that the biggest value in having the prediction weight table > > > > > separated is to allow applications to skip setting this largish > > > > > control, > > > > > reducing the amount of data that needs to be passed from userspace > > > > > -- especially when not needed :-) > > > > > > > > > > > > Given its size, it makes sense to move this table to its > > > > > > > control, > > > > > > > so applications can avoid passing it if the slice doesn't > > > > > > > specify > > > > > > > it. > > > > > > > > > > > > > > Before this change struct v4l2_ctrl_h264_slice_params was 960 > > > > > > > bytes. > > > > > > > With this change, it's 188 bytes and struct > > > > > > > v4l2_ctrl_h264_pred_weight > > > > > > > is 772 bytes. > > > > > > > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > > > > --- > > > > > > > v2: Fix missing Cedrus changes and mssing control declaration, > > > > > > > > > > > > > > as noted by Hans and Jernej. > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > .../media/v4l/ext-ctrls-codec.rst | 19 > > > > > > > ++++++++++++------- > > > > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > > > > > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > > > > > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- > > > > > > > include/media/h264-ctrls.h | 5 +++-- > > > > > > > include/media/v4l2-ctrls.h | 2 ++ > > > > > > > 8 files changed, 37 insertions(+), 13 deletions(-) > > > > > > > > > > > > > > diff --git > > > > > > > a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > index > > > > > > > d1438b1e259f..c36ce5a95fc5 100644 > > > > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > @@ -1879,18 +1879,23 @@ enum > > > > > > > v4l2_mpeg_video_h264_hierarchical_coding_type -> > > > > > > > > > > > > > > > - 0x00000008 > > > > > > > - > > > > > > > > > > > > > > -``Prediction Weight Table`` > > > > > > > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` > > > > > > > + Prediction weight table defined according to :ref:`h264`, > > > > > > > + section 7.4.3.2 "Prediction Weight Table Semantics". > > > > > > > + The prediction weight table must be passed by applications > > > > > > > + under the conditions explained in section 7.3.3 "Slice > > > > > > > header > > > > > > > + syntax". > > > > > > > > > > > > > > - The bitstream parameters are defined according to > > > > > > > :ref:`h264`, > > > > > > > - section 7.4.3.2 "Prediction Weight Table Semantics". For > > > > > > > further > > > > > > > - documentation, refer to the above specification, unless > > > > > > > there > > > > > > > is > > > > > > > - an explicit comment stating otherwise. > > > > > > > + .. note:: > > > > > > > + > > > > > > > + This compound control is not yet part of the public > > > > > > > kernel > > > > > > > API > > > > > > > and > > > > > > > + it is expected to change. > > > > > > > > > > > > > > -.. c:type:: v4l2_h264_pred_weight_table > > > > > > > +.. c:type:: v4l2_ctrl_h264_pred_weights > > > > > > > > > > > > > > .. cssclass:: longtable > > > > > > > > > > > > > > -.. flat-table:: struct v4l2_h264_pred_weight_table > > > > > > > +.. flat-table:: struct v4l2_ctrl_h264_pred_weights > > > > > > > > > > > > > > :header-rows: 0 > > > > > > > :stub-columns: 0 > > > > > > > :widths: 1 1 2 > > > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > > b/drivers/media/v4l2-core/v4l2-ctrls.c index > > > > > > > 3f3fbcd60cc6..76c8dc8fb31c > > > > > > > 100644 > > > > > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > > @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > > > > > > return > > > > > > > "H264 Decode Parameters"; case > > > > > > > V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return > > > > > > > "H264 > > > > > > > > > > > > > > Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: > > > > > > > return "H264 Start Code";> > > > > > > > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > > > > > > return > > > > > > > "H264 Prediction Weight Table";> > > > > > > > > > > > > > > > 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";> > > > > > > > > > > > > > > > @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char > > > > > > > **name, > > > > > > > enum v4l2_ctrl_type *type,> > > > > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > > > > > > *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; > > > > > > > break; > > > > > > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > > > > > > + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; > > > > > > > + break; > > > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: > > > > > > > *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; > > > > > > > break; > > > > > > > > > > > > > > @@ -1790,6 +1794,7 @@ static int std_validate_compound(const > > > > > > > struct > > > > > > > v4l2_ctrl *ctrl, u32 idx,> > > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_SPS: > > > > > > > case V4L2_CTRL_TYPE_H264_PPS: > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: > > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > > > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > > > break; > > > > > > > > > > > > > > @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl > > > > > > > *v4l2_ctrl_new(struct > > > > > > > v4l2_ctrl_handler *hdl,> > > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > > > elem_size = sizeof(struct > > > > > > > v4l2_ctrl_h264_decode_params); > > > > > > > break; > > > > > > > > > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > > > + elem_size = sizeof(struct > > > > > > > v4l2_ctrl_h264_pred_weights); > > > > > > > + break; > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: > > > > > > > elem_size = sizeof(struct > > > > > > > v4l2_ctrl_vp8_frame_header); > > > > > > > break; > > > > > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > > > > > > bc27f9430eeb..027cdd1be5a0 100644 > > > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > > @@ -78,6 +78,13 @@ static const struct cedrus_control > > > > > > > cedrus_controls[] > > > > > > > = { > > > > > > > > > > > > > > .codec = CEDRUS_CODEC_H264, > > > > > > > .required = true, > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > + { > > > > > > > + .cfg = { > > > > > > > + .id = > > > > > > > V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > > > > > > > + }, > > > > > > > + .codec = CEDRUS_CODEC_H264, > > > > > > > + .required = true, > > > > > > > > > > > > This should probably be false if this control is to be optional as > > > > > > implied > > > > > > by the commit message. > > > > > > > > > > Well, the control is optional if the driver implements it as > > > > > optional, > > > > > which Cedrus isn't currently doing :-) > > > > > > > > Why do you think so? Prediction weights are filled only when they are > > > > needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/ > > > > medi > > > > a/ sunxi/cedrus/cedrus_h264.c#L370 > > > > > > Right, but that should be changed to be really optional. > > > How does the driver reject/fail the request if the table is NULL? > > > > It's my understanding that pointer to this table can't be NULL. NULL would > > mean that there is no control with that ID registered in the driver. > > Hm, I'm starting to think you are right. So, does this mean > the default quantization matrix here is bogus? > > if (quantization && quantization->load_intra_quantiser_matrix) > matrix = quantization->intra_quantiser_matrix; > else > matrix = intra_quantization_matrix_default; No, not really. Userspace can set load_intra_quantiser_matrix flag to false. Best regards, Jernej > > Thanks, > Ezequiel > > > Best regards, > > Jernej > > > > > In any case, I don't think it's necessarily something we need > > > to tackle now. > > > > > > Thanks, > > > Ezequiel
On Mon, 2020-08-10 at 16:57 +0000, Jonas Karlman wrote: > On 2020-08-10 17:28, Ezequiel Garcia wrote: > > On Mon, 2020-08-10 at 14:55 +0000, Jonas Karlman wrote: > > > On 2020-08-10 14:57, Ezequiel Garcia wrote: > > > > On Sun, 2020-08-09 at 23:11 +0200, Jernej Škrabec wrote: > > > > > Dne nedelja, 09. avgust 2020 ob 15:55:50 CEST je Ezequiel Garcia napisal(a): > > > > > > On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: > > > > > > > On 2020-08-06 17:12, Ezequiel Garcia wrote: > > > > > > > > The prediction weight parameters are only required under > > > > > > > > certain conditions, which depend on slice header parameters. > > > > > > > > > > > > > > > > As specified in section 7.3.3 Slice header syntax, the prediction > > > > > > > > weight table is present if: > > > > > > > > > > > > > > > > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ > > > > > > > > (weighted_bipred_idc == 1 && slice_type == B)) > > > > > > > > > > > > > > Maybe a macro can be added to help check this contition? > > > > > > > > > > > > > > Something like this maybe: > > > > > > > > > > > > > > #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ > > > > > > > > > > > > > > ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ > > > > > > > > > > > > > > ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ > > > > > > > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ > > > > > > > > > > > > > > ((pps)->weighted_bipred_idc == 1 && \ > > > > > > > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) > > > > > > > > > > > > Yeah, that could make sense. > > > > > > > > > > > > Note that the biggest value in having the prediction weight table > > > > > > separated is to allow applications to skip setting this largish control, > > > > > > reducing the amount of data that needs to be passed from userspace > > > > > > -- especially when not needed :-) > > > > > > > > > > > > > > Given its size, it makes sense to move this table to its control, > > > > > > > > so applications can avoid passing it if the slice doesn't specify it. > > > > > > > > > > > > > > > > Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. > > > > > > > > With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight > > > > > > > > is 772 bytes. > > > > > > > > > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > > > > > --- > > > > > > > > v2: Fix missing Cedrus changes and mssing control declaration, > > > > > > > > > > > > > > > > as noted by Hans and Jernej. > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > .../media/v4l/ext-ctrls-codec.rst | 19 ++++++++++++------- > > > > > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > > > > > > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > > > > > > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- > > > > > > > > include/media/h264-ctrls.h | 5 +++-- > > > > > > > > include/media/v4l2-ctrls.h | 2 ++ > > > > > > > > 8 files changed, 37 insertions(+), 13 deletions(-) > > > > > > > > > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index > > > > > > > > d1438b1e259f..c36ce5a95fc5 100644 > > > > > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > > @@ -1879,18 +1879,23 @@ enum > > > > > > > > v4l2_mpeg_video_h264_hierarchical_coding_type -> > > > > > > > > > - 0x00000008 > > > > > > > > - > > > > > > > > > > > > > > > > -``Prediction Weight Table`` > > > > > > > > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` > > > > > > > > + Prediction weight table defined according to :ref:`h264`, > > > > > > > > + section 7.4.3.2 "Prediction Weight Table Semantics". > > > > > > > > + The prediction weight table must be passed by applications > > > > > > > > + under the conditions explained in section 7.3.3 "Slice header > > > > > > > > + syntax". > > > > > > > > > > > > > > > > - The bitstream parameters are defined according to :ref:`h264`, > > > > > > > > - section 7.4.3.2 "Prediction Weight Table Semantics". For further > > > > > > > > - documentation, refer to the above specification, unless there is > > > > > > > > - an explicit comment stating otherwise. > > > > > > > > + .. note:: > > > > > > > > + > > > > > > > > + This compound control is not yet part of the public kernel API > > > > > > > > and > > > > > > > > + it is expected to change. > > > > > > > > > > > > > > > > -.. c:type:: v4l2_h264_pred_weight_table > > > > > > > > +.. c:type:: v4l2_ctrl_h264_pred_weights > > > > > > > > > > > > > > > > .. cssclass:: longtable > > > > > > > > > > > > > > > > -.. flat-table:: struct v4l2_h264_pred_weight_table > > > > > > > > +.. flat-table:: struct v4l2_ctrl_h264_pred_weights > > > > > > > > > > > > > > > > :header-rows: 0 > > > > > > > > :stub-columns: 0 > > > > > > > > :widths: 1 1 2 > > > > > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > > > b/drivers/media/v4l2-core/v4l2-ctrls.c index 3f3fbcd60cc6..76c8dc8fb31c > > > > > > > > 100644 > > > > > > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > > > @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > > > > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return > > > > > > > > "H264 Decode Parameters"; case > > > > > > > > V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return "H264 > > > > > > > > Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: > > > > > > > > return "H264 Start Code";> > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return > > > > > > > > "H264 Prediction Weight Table";> > > > > > > > > > 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";> > > > > > > > > > @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, > > > > > > > > enum v4l2_ctrl_type *type,> > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > > > > > > > *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; > > > > > > > > break; > > > > > > > > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > > > > > > > + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; > > > > > > > > + break; > > > > > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: > > > > > > > > *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; > > > > > > > > break; > > > > > > > > > > > > > > > > @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct > > > > > > > > v4l2_ctrl *ctrl, u32 idx,> > > > > > > > > > case V4L2_CTRL_TYPE_H264_SPS: > > > > > > > > case V4L2_CTRL_TYPE_H264_PPS: > > > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: > > > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > > > > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: > > > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > > > > break; > > > > > > > > > > > > > > > > @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct > > > > > > > > v4l2_ctrl_handler *hdl,> > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > > > > elem_size = sizeof(struct v4l2_ctrl_h264_decode_params); > > > > > > > > break; > > > > > > > > > > > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > > > > + elem_size = sizeof(struct v4l2_ctrl_h264_pred_weights); > > > > > > > > + break; > > > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: > > > > > > > > elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); > > > > > > > > break; > > > > > > > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > > > > > > > bc27f9430eeb..027cdd1be5a0 100644 > > > > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > > > @@ -78,6 +78,13 @@ static const struct cedrus_control cedrus_controls[] > > > > > > > > = { > > > > > > > > > > > > > > > > .codec = CEDRUS_CODEC_H264, > > > > > > > > .required = true, > > > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > > > + { > > > > > > > > + .cfg = { > > > > > > > > + .id = V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > > > > > > > > + }, > > > > > > > > + .codec = CEDRUS_CODEC_H264, > > > > > > > > + .required = true, > > > > > > > > > > > > > > This should probably be false if this control is to be optional as implied > > > > > > > by the commit message. > > > > > > > > > > > > Well, the control is optional if the driver implements it as optional, > > > > > > which Cedrus isn't currently doing :-) > > > > > > > > > > Why do you think so? Prediction weights are filled only when they are > > > > > needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ > > > > > sunxi/cedrus/cedrus_h264.c#L370 > > > > > > > > > > > > > Right, but that should be changed to be really optional. > > > > How does the driver reject/fail the request if the table is NULL? > > > > > > > > In any case, I don't think it's necessarily something we need > > > > to tackle now. > > > > > > I do not fully follow, the commit message state following: > > > > > > Note that the biggest value in having the prediction weight table > > > separated is to allow applications to skip setting this largish control > > > > > > > This is not exactly what the commit message says, but yeah > > that's the idea. > > Hehe, I copied your reply instead of commit message / doc changes :-) > > > > Yet the driver still require this new control to be included in the request > > > thanks to the .required = true statement. (if i understand the code correctly) > > > > > > So applications still need to set this largish control? > > > > > > > This is a uAPI change that paves the way for Cedrus to make the control > > optional. The series doesn't take care of it, but it prepares the road > > for it. > > > > Since we are not stabilizing the uAPI (yet), I think we still have > > some room to make this change in steps: first we merge the new control > > and then we add the needed changes to Cedrus? > > > > Does that make sense? > > Sure, make sense, I will rephrase it as questions instead :-) > > What is not fully clear to me is if this new ctrl should be considered > optional or required from userspace point of view. > > Will there be a way for userspace to know if a ctrl is optional or not? > We don't currently have a way for applications to query if a control is mandatory for a given request. However... > If I implement uapi changes as suggested in this patch in ffmpeg, > i.e. only set weight table ctrl when data if it is present in slice header, > then decoding stops working for cedrus when weight table is not present. > > Should I just play it safe and continue to always set the new ctrl for slice > based decoding and treat it as required? Or are we saying that it should be > optional and cedrus is just not following the uapi after this series? > ... as Jernej pointed-out, if the control is initialized then it'll never be NULL. It's initialized in std_init_compound. So we can just go ahead and add this patch to the series? It will be the application responsability to fill the control when needed, and applications are now able to not pass the control when not needed, just as expected. diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c index 027cdd1be5a0..826324faad7e 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c @@ -83,7 +83,7 @@ static const struct cedrus_control cedrus_controls[] = { .id = V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, }, .codec = CEDRUS_CODEC_H264, - .required = true, + .required = false, }, { .cfg = { diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index 7b2169d185b8..57bad8733aed 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c @@ -364,11 +364,7 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, cedrus_skip_bits(dev, slice->header_bit_size); - if (((pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && - (slice->slice_type == V4L2_H264_SLICE_TYPE_P || - slice->slice_type == V4L2_H264_SLICE_TYPE_SP)) || - (pps->weighted_bipred_idc == 1 && - slice->slice_type == V4L2_H264_SLICE_TYPE_B)) + if (V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice)) cedrus_write_pred_weight_table(ctx, run); if ((slice->slice_type == V4L2_H264_SLICE_TYPE_P) || diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h index fa5663876e73..7d1a6f97eb12 100644 --- a/include/media/h264-ctrls.h +++ b/include/media/h264-ctrls.h @@ -127,6 +127,13 @@ struct v4l2_h264_weight_factors { __s16 chroma_offset[32][2]; }; +#define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ + ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ + ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ + (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ + ((pps)->weighted_bipred_idc == 1 && \ + (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) + struct v4l2_ctrl_h264_pred_weights { __u16 luma_log2_weight_denom; __u16 chroma_log2_weight_denom;
On Mon, 2020-08-10 at 21:34 +0200, Jernej Škrabec wrote: > Dne ponedeljek, 10. avgust 2020 ob 21:30:59 CEST je Ezequiel Garcia > napisal(a): > > On Mon, 2020-08-10 at 19:05 +0200, Jernej Škrabec wrote: > > > Dne ponedeljek, 10. avgust 2020 ob 14:57:17 CEST je Ezequiel Garcia > > > > > > napisal(a): > > > > On Sun, 2020-08-09 at 23:11 +0200, Jernej Škrabec wrote: > > > > > Dne nedelja, 09. avgust 2020 ob 15:55:50 CEST je Ezequiel Garcia > > > > > > napisal(a): > > > > > > On Sat, 8 Aug 2020 at 18:01, Jonas Karlman <jonas@kwiboo.se> wrote: > > > > > > > On 2020-08-06 17:12, Ezequiel Garcia wrote: > > > > > > > > The prediction weight parameters are only required under > > > > > > > > certain conditions, which depend on slice header parameters. > > > > > > > > > > > > > > > > As specified in section 7.3.3 Slice header syntax, the > > > > > > > > prediction > > > > > > > > weight table is present if: > > > > > > > > > > > > > > > > ((weighted_pred_flag && (slice_type == P || slice_type == SP)) > > > > > > > > > > \ > > > > > > > > (weighted_bipred_idc == 1 && slice_type == B)) > > > > > > > > > > > > > > Maybe a macro can be added to help check this contition? > > > > > > > > > > > > > > Something like this maybe: > > > > > > > > > > > > > > #define V4L2_H264_CTRL_PRED_WEIGHTS_REQUIRED(pps, slice) \ > > > > > > > > > > > > > > ((((pps)->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && \ > > > > > > > > > > > > > > ((slice)->slice_type == V4L2_H264_SLICE_TYPE_P || \ > > > > > > > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_SP)) || \ > > > > > > > > > > > > > > ((pps)->weighted_bipred_idc == 1 && \ > > > > > > > > > > > > > > (slice)->slice_type == V4L2_H264_SLICE_TYPE_B)) > > > > > > > > > > > > Yeah, that could make sense. > > > > > > > > > > > > Note that the biggest value in having the prediction weight table > > > > > > separated is to allow applications to skip setting this largish > > > > > > control, > > > > > > reducing the amount of data that needs to be passed from userspace > > > > > > -- especially when not needed :-) > > > > > > > > > > > > > > Given its size, it makes sense to move this table to its > > > > > > > > control, > > > > > > > > so applications can avoid passing it if the slice doesn't > > > > > > > > specify > > > > > > > > it. > > > > > > > > > > > > > > > > Before this change struct v4l2_ctrl_h264_slice_params was 960 > > > > > > > > bytes. > > > > > > > > With this change, it's 188 bytes and struct > > > > > > > > v4l2_ctrl_h264_pred_weight > > > > > > > > is 772 bytes. > > > > > > > > > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > > > > > --- > > > > > > > > v2: Fix missing Cedrus changes and mssing control declaration, > > > > > > > > > > > > > > > > as noted by Hans and Jernej. > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > .../media/v4l/ext-ctrls-codec.rst | 19 > > > > > > > > ++++++++++++------- > > > > > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > > > > > > > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > > > > > > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- > > > > > > > > include/media/h264-ctrls.h | 5 +++-- > > > > > > > > include/media/v4l2-ctrls.h | 2 ++ > > > > > > > > 8 files changed, 37 insertions(+), 13 deletions(-) > > > > > > > > > > > > > > > > diff --git > > > > > > > > a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > > index > > > > > > > > d1438b1e259f..c36ce5a95fc5 100644 > > > > > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > > > > > @@ -1879,18 +1879,23 @@ enum > > > > > > > > v4l2_mpeg_video_h264_hierarchical_coding_type -> > > > > > > > > > > > > > > > > > - 0x00000008 > > > > > > > > - > > > > > > > > > > > > > > > > -``Prediction Weight Table`` > > > > > > > > +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` > > > > > > > > + Prediction weight table defined according to :ref:`h264`, > > > > > > > > + section 7.4.3.2 "Prediction Weight Table Semantics". > > > > > > > > + The prediction weight table must be passed by applications > > > > > > > > + under the conditions explained in section 7.3.3 "Slice > > > > > > > > header > > > > > > > > + syntax". > > > > > > > > > > > > > > > > - The bitstream parameters are defined according to > > > > > > > > :ref:`h264`, > > > > > > > > - section 7.4.3.2 "Prediction Weight Table Semantics". For > > > > > > > > further > > > > > > > > - documentation, refer to the above specification, unless > > > > > > > > there > > > > > > > > is > > > > > > > > - an explicit comment stating otherwise. > > > > > > > > + .. note:: > > > > > > > > + > > > > > > > > + This compound control is not yet part of the public > > > > > > > > kernel > > > > > > > > API > > > > > > > > and > > > > > > > > + it is expected to change. > > > > > > > > > > > > > > > > -.. c:type:: v4l2_h264_pred_weight_table > > > > > > > > +.. c:type:: v4l2_ctrl_h264_pred_weights > > > > > > > > > > > > > > > > .. cssclass:: longtable > > > > > > > > > > > > > > > > -.. flat-table:: struct v4l2_h264_pred_weight_table > > > > > > > > +.. flat-table:: struct v4l2_ctrl_h264_pred_weights > > > > > > > > > > > > > > > > :header-rows: 0 > > > > > > > > :stub-columns: 0 > > > > > > > > :widths: 1 1 2 > > > > > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > > > b/drivers/media/v4l2-core/v4l2-ctrls.c index > > > > > > > > 3f3fbcd60cc6..76c8dc8fb31c > > > > > > > > 100644 > > > > > > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > > > > > > @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > > > > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > > > > > > > return > > > > > > > > "H264 Decode Parameters"; case > > > > > > > > V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return > > > > > > > > "H264 > > > > > > > > > > > > > > > > Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: > > > > > > > > return "H264 Start Code";> > > > > > > > > > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > > > > > > > return > > > > > > > > "H264 Prediction Weight Table";> > > > > > > > > > > > > > > > > > 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";> > > > > > > > > > > > > > > > > > @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char > > > > > > > > **name, > > > > > > > > enum v4l2_ctrl_type *type,> > > > > > > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: > > > > > > > > *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; > > > > > > > > break; > > > > > > > > > > > > > > > > + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: > > > > > > > > + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; > > > > > > > > + break; > > > > > > > > > > > > > > > > case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: > > > > > > > > *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; > > > > > > > > break; > > > > > > > > > > > > > > > > @@ -1790,6 +1794,7 @@ static int std_validate_compound(const > > > > > > > > struct > > > > > > > > v4l2_ctrl *ctrl, u32 idx,> > > > > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_SPS: > > > > > > > > case V4L2_CTRL_TYPE_H264_PPS: > > > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: > > > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > > > > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: > > > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > > > > break; > > > > > > > > > > > > > > > > @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl > > > > > > > > *v4l2_ctrl_new(struct > > > > > > > > v4l2_ctrl_handler *hdl,> > > > > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > > > > > > > > elem_size = sizeof(struct > > > > > > > > v4l2_ctrl_h264_decode_params); > > > > > > > > break; > > > > > > > > > > > > > > > > + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: > > > > > > > > + elem_size = sizeof(struct > > > > > > > > v4l2_ctrl_h264_pred_weights); > > > > > > > > + break; > > > > > > > > > > > > > > > > case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: > > > > > > > > elem_size = sizeof(struct > > > > > > > > v4l2_ctrl_vp8_frame_header); > > > > > > > > break; > > > > > > > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > > > > > > > bc27f9430eeb..027cdd1be5a0 100644 > > > > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > > > > > > > @@ -78,6 +78,13 @@ static const struct cedrus_control > > > > > > > > cedrus_controls[] > > > > > > > > = { > > > > > > > > > > > > > > > > .codec = CEDRUS_CODEC_H264, > > > > > > > > .required = true, > > > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > > > + { > > > > > > > > + .cfg = { > > > > > > > > + .id = > > > > > > > > V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > > > > > > > > + }, > > > > > > > > + .codec = CEDRUS_CODEC_H264, > > > > > > > > + .required = true, > > > > > > > > > > > > > > This should probably be false if this control is to be optional as > > > > > > > implied > > > > > > > by the commit message. > > > > > > > > > > > > Well, the control is optional if the driver implements it as > > > > > > optional, > > > > > > which Cedrus isn't currently doing :-) > > > > > > > > > > Why do you think so? Prediction weights are filled only when they are > > > > > needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/ > > > > > medi > > > > > a/ sunxi/cedrus/cedrus_h264.c#L370 > > > > > > > > Right, but that should be changed to be really optional. > > > > How does the driver reject/fail the request if the table is NULL? > > > > > > It's my understanding that pointer to this table can't be NULL. NULL would > > > mean that there is no control with that ID registered in the driver. > > > > Hm, I'm starting to think you are right. So, does this mean > > the default quantization matrix here is bogus? > > > > if (quantization && quantization->load_intra_quantiser_matrix) > > matrix = quantization->intra_quantiser_matrix; > > else > > matrix = intra_quantization_matrix_default; > > No, not really. Userspace can set load_intra_quantiser_matrix flag to false. > Right. But quantization can never be NULL. I think this if (quantization ...) has been confusing me into thinking a control can be NULL. :-) Thanks for the pointers, this is useful. Ezequiel > Best regards, > Jernej > > > Thanks, > > Ezequiel > > > > > Best regards, > > > Jernej > > > > > > > In any case, I don't think it's necessarily something we need > > > > to tackle now. > > > > > > > > Thanks, > > > > Ezequiel > > >
Hi Jonas, Jernej and everyone :) > > > > > > > > + { > > > > > > > > + .cfg = { > > > > > > > > + .id = > > > > > > > > V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, > > > > > > > > + }, > > > > > > > > + .codec = CEDRUS_CODEC_H264, > > > > > > > > + .required = true, > > > > > > > > > > > > > > This should probably be false if this control is to be optional as > > > > > > > implied > > > > > > > by the commit message. > > > > > > > > > > > > Well, the control is optional if the driver implements it as > > > > > > optional, > > > > > > which Cedrus isn't currently doing :-) > > > > > > > > > > Why do you think so? Prediction weights are filled only when they are > > > > > needed:https://elixir.bootlin.com/linux/latest/source/drivers/staging/ > > > > > medi > > > > > a/ sunxi/cedrus/cedrus_h264.c#L370 > > > > > > > > Right, but that should be changed to be really optional. > > > > How does the driver reject/fail the request if the table is NULL? > > > > > > It's my understanding that pointer to this table can't be NULL. NULL would > > > mean that there is no control with that ID registered in the driver. > > > > Hm, I'm starting to think you are right. So, does this mean > > the default quantization matrix here is bogus? > > > > if (quantization && quantization->load_intra_quantiser_matrix) > > matrix = quantization->intra_quantiser_matrix; > > else > > matrix = intra_quantization_matrix_default; > > No, not really. Userspace can set load_intra_quantiser_matrix flag to false. > The above made me revisit the current H264 semantics for the picture scaling matrix. As you can see, we currently have V4L2_H264_PPS_FLAG_PIC_SCALING_MATRIX_PRESENT, which we were expecting to match 1:1 the H264 PPS syntax element "pic_scaling_matrix_present_flag". However, after a bit of reflection and discussion with Nicolas, I believe it's not appropriate to have this flag as a 1:1 match with the PPS syntax element. A H264 scaling matrix can be first specified by the SPS and then modified by the PPS. We can expect the modification process to be solved by userspace. All we need in the uAPI is a flag that indicates if V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX should be used or not. (As Jernej already pointed out, a initialized control shall never be NULL, so we want to flag if the control should be used or not) [1]. Applications are expected to fill V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX if a scaling matrix needs to be passed, which is the case is: sps->scaling_matrix_present_flag || pps->pic_scaling_matrix_present_flag So that is the meaning of the flag we want. [2] Moreover, Baseline, Main and Extended profiles are specified to have neither SPS scaling_matrix_present_flag nor PPS pic_scaling_matrix_present_flag syntax elements, so it makes sense to allow applications _not_ setting V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX in a request. On the uAPI side, the only change needed is: -#define V4L2_H264_PPS_FLAG_PIC_SCALING_MATRIX_PRESENT 0x0080 +#define V4L2_H264_PPS_FLAG_SCALING_MATRIX_PRESENT 0x0080 (just to avoid confusing the flag with the syntax element) Together with proper documentation to clarify what the flag is. Drivers can use this flag as (rkvdec as an example): - /* always use the matrix sent from userspace */ - WRITE_PPS(1, SCALING_LIST_ENABLE_FLAG); - + WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_SCALING_MATRIX_PRESENT), + SCALING_LIST_ENABLE_FLAG); Which also means the scaling matrix control is optional and won't be programmed to the hardware when the not present. [3] Thanks! Ezequiel [1] We may also check if a control is part of a request or not, but that seems more complex and more obscure than just checking a flag. [2] In theory, the uAPI could also have semantics to flag seq_scaling_list_present_flag[i] and pic_scaling_list_present_flag[i], for each scaling list. I think this makes things overly complicated. [3] We could add Flat_4x4_16 and Flat_8x8_16 in the kernel, but all the drivers we support, have a flag for scaling-matrix-not-present, so there's no need.
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index d1438b1e259f..c36ce5a95fc5 100644 --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst @@ -1879,18 +1879,23 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - - 0x00000008 - -``Prediction Weight Table`` +``V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (struct)`` + Prediction weight table defined according to :ref:`h264`, + section 7.4.3.2 "Prediction Weight Table Semantics". + The prediction weight table must be passed by applications + under the conditions explained in section 7.3.3 "Slice header + syntax". - The bitstream parameters are defined according to :ref:`h264`, - section 7.4.3.2 "Prediction Weight Table Semantics". For further - documentation, refer to the above specification, unless there is - an explicit comment stating otherwise. + .. note:: + + This compound control is not yet part of the public kernel API and + it is expected to change. -.. c:type:: v4l2_h264_pred_weight_table +.. c:type:: v4l2_ctrl_h264_pred_weights .. cssclass:: longtable -.. flat-table:: struct v4l2_h264_pred_weight_table +.. flat-table:: struct v4l2_ctrl_h264_pred_weights :header-rows: 0 :stub-columns: 0 :widths: 1 1 2 diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 3f3fbcd60cc6..76c8dc8fb31c 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -897,6 +897,7 @@ const char *v4l2_ctrl_get_name(u32 id) case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: return "H264 Decode Parameters"; case V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE: return "H264 Decode Mode"; case V4L2_CID_MPEG_VIDEO_H264_START_CODE: return "H264 Start Code"; + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: return "H264 Prediction Weight Table"; 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"; @@ -1412,6 +1413,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS: *type = V4L2_CTRL_TYPE_H264_DECODE_PARAMS; break; + case V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS: + *type = V4L2_CTRL_TYPE_H264_PRED_WEIGHTS; + break; case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER: *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER; break; @@ -1790,6 +1794,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, case V4L2_CTRL_TYPE_H264_SPS: case V4L2_CTRL_TYPE_H264_PPS: case V4L2_CTRL_TYPE_H264_SCALING_MATRIX: + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: case V4L2_CTRL_TYPE_H264_SLICE_PARAMS: case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: break; @@ -2553,6 +2558,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: elem_size = sizeof(struct v4l2_ctrl_h264_decode_params); break; + case V4L2_CTRL_TYPE_H264_PRED_WEIGHTS: + elem_size = sizeof(struct v4l2_ctrl_h264_pred_weights); + break; case V4L2_CTRL_TYPE_VP8_FRAME_HEADER: elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header); break; diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c index bc27f9430eeb..027cdd1be5a0 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c @@ -78,6 +78,13 @@ static const struct cedrus_control cedrus_controls[] = { .codec = CEDRUS_CODEC_H264, .required = true, }, + { + .cfg = { + .id = V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS, + }, + .codec = CEDRUS_CODEC_H264, + .required = true, + }, { .cfg = { .id = V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE, diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h index 96765555ab8a..93c843ae14bb 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus.h +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h @@ -62,6 +62,7 @@ struct cedrus_h264_run { const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix; const struct v4l2_ctrl_h264_slice_params *slice_params; const struct v4l2_ctrl_h264_sps *sps; + const struct v4l2_ctrl_h264_pred_weights *pred_weights; }; struct cedrus_mpeg2_run { diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index 58c48e4fdfe9..6385026d1b6b 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c @@ -57,6 +57,8 @@ void cedrus_device_run(void *priv) V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS); run.h264.sps = cedrus_find_control_data(ctx, V4L2_CID_MPEG_VIDEO_H264_SPS); + run.h264.pred_weights = cedrus_find_control_data(ctx, + V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS); break; case V4L2_PIX_FMT_HEVC_SLICE: diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index cce527bbdf86..a9ba78b15907 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c @@ -256,10 +256,8 @@ static void cedrus_write_scaling_lists(struct cedrus_ctx *ctx, static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx, struct cedrus_run *run) { - const struct v4l2_ctrl_h264_slice_params *slice = - run->h264.slice_params; - const struct v4l2_h264_pred_weight_table *pred_weight = - &slice->pred_weight_table; + const struct v4l2_ctrl_h264_pred_weights *pred_weight = + run->h264.pred_weights; struct cedrus_dev *dev = ctx->dev; int i, j, k; diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h index 4c0bb7f5fb05..54cd9bec0b23 100644 --- a/include/media/h264-ctrls.h +++ b/include/media/h264-ctrls.h @@ -36,6 +36,7 @@ #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS (V4L2_CID_MPEG_BASE+1004) #define V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (V4L2_CID_MPEG_BASE+1005) #define V4L2_CID_MPEG_VIDEO_H264_START_CODE (V4L2_CID_MPEG_BASE+1006) +#define V4L2_CID_MPEG_VIDEO_H264_PRED_WEIGHTS (V4L2_CID_MPEG_BASE+1007) /* enum v4l2_ctrl_type type values */ #define V4L2_CTRL_TYPE_H264_SPS 0x0110 @@ -43,6 +44,7 @@ #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX 0x0112 #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS 0x0113 #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS 0x0114 +#define V4L2_CTRL_TYPE_H264_PRED_WEIGHTS 0x0115 enum v4l2_mpeg_video_h264_decode_mode { V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED, @@ -125,7 +127,7 @@ struct v4l2_h264_weight_factors { __s16 chroma_offset[32][2]; }; -struct v4l2_h264_pred_weight_table { +struct v4l2_ctrl_h264_pred_weights { __u16 luma_log2_weight_denom; __u16 chroma_log2_weight_denom; struct v4l2_h264_weight_factors weight_factors[2]; @@ -177,7 +179,6 @@ struct v4l2_ctrl_h264_slice_params { __s32 delta_pic_order_cnt0; __s32 delta_pic_order_cnt1; - struct v4l2_h264_pred_weight_table pred_weight_table; /* Size in bits of dec_ref_pic_marking() syntax element. */ __u32 dec_ref_pic_marking_bit_size; /* Size in bits of pic order count syntax. */ diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h index f40e2cbb21d3..cb25f345e9ad 100644 --- a/include/media/v4l2-ctrls.h +++ b/include/media/v4l2-ctrls.h @@ -51,6 +51,7 @@ struct video_device; * @p_h264_scaling_matrix: Pointer to a struct v4l2_ctrl_h264_scaling_matrix. * @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params. * @p_h264_decode_params: Pointer to a struct v4l2_ctrl_h264_decode_params. + * @p_h264_pred_weights: Pointer to a struct v4l2_ctrl_h264_pred_weights. * @p_vp8_frame_header: Pointer to a VP8 frame header structure. * @p_hevc_sps: Pointer to an HEVC sequence parameter set structure. * @p_hevc_pps: Pointer to an HEVC picture parameter set structure. @@ -74,6 +75,7 @@ union v4l2_ctrl_ptr { struct v4l2_ctrl_h264_scaling_matrix *p_h264_scaling_matrix; struct v4l2_ctrl_h264_slice_params *p_h264_slice_params; struct v4l2_ctrl_h264_decode_params *p_h264_decode_params; + struct v4l2_ctrl_h264_pred_weights *p_h264_pred_weights; struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header; struct v4l2_ctrl_hevc_sps *p_hevc_sps; struct v4l2_ctrl_hevc_pps *p_hevc_pps;
The prediction weight parameters are only required under certain conditions, which depend on slice header parameters. As specified in section 7.3.3 Slice header syntax, the prediction weight table is present if: ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ (weighted_bipred_idc == 1 && slice_type == B)) Given its size, it makes sense to move this table to its control, so applications can avoid passing it if the slice doesn't specify it. Before this change struct v4l2_ctrl_h264_slice_params was 960 bytes. With this change, it's 188 bytes and struct v4l2_ctrl_h264_pred_weight is 772 bytes. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- v2: Fix missing Cedrus changes and mssing control declaration, as noted by Hans and Jernej. --- .../media/v4l/ext-ctrls-codec.rst | 19 ++++++++++++------- drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++++++ drivers/staging/media/sunxi/cedrus/cedrus.c | 7 +++++++ drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ .../staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++---- include/media/h264-ctrls.h | 5 +++-- include/media/v4l2-ctrls.h | 2 ++ 8 files changed, 37 insertions(+), 13 deletions(-)