Le jeudi 25 juin 2020 à 14:42 -0300, Ezequiel Garcia a écrit : > (Adding Jernej, seems I haven't Cced him!) > > On Thu, 2020-06-25 at 10:53 -0400, Nicolas Dufresne wrote: > > Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit : > > > Align v4l2_h264_reference to 32-bits, giving some room > > > for future extensions. > > > > > > Signed-off-by: Ezequiel Garcia <[email protected]> > > > --- > > > .../userspace-api/media/v4l/ext-ctrls-codec.rst | 3 +++ > > > drivers/media/v4l2-core/v4l2-ctrls.c | 16 ++++++++++++++++ > > > include/media/h264-ctrls.h | 7 +++++-- > > > 3 files changed, 24 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > index 6c36d298db20..7af12447a5b0 100644 > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > @@ -1943,6 +1943,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > > * - __u8 > > > - ``index`` > > > - > > > + * - __u32 > > > + - ``reserved`` > > > + - Applications and drivers must set this to zero. > > > > Is that really appropriate ? There was some effort to keep the controls > > small. Also, as these are fixed size, they could be extended with > > supplementary C arrays of the same size, set in separate control. > > > > That's a very valid concern. > > Currently, each of these v4l2_h264_reference take 64 bytes > (32 x 2 bytes). So it's 128 bytes of references. > > Having the reserved field means 256 bytes instead. > > Without the reserved field v4l2_ctrl_h264_slice_params is 936 bytes, > the majority of the space taken by v4l2_h264_weight_factors. > So, the reserved field accounts for a small increment there. > > Another option would be to split the v4l2_h264_reference lists > to its own control. > > However, given the above, it would have more impact to consider > splitting v4l2_h264_pred_weight_table. This table > being used or not depends on the bitstream, so there might be > some value here. > > What do you think?
This seems like a good idea, making the weight table it's own control,
so when the it's not specified in the syntax, we could skip.
>
> > Also, H264 HW is unlikely to evolve, and that covers what DXVA2 and
> > VAAPI covers already. So it is quite unlikely to ever have to be
> > extended.
> >
>
> That may be true indeed. Perhaps we can then agree that references
> only need an index and a flag.
I also see that MVC was never implemented in FFMPEG, I was quite
surprised. Only left eye playback is there, same in gstreamer-vaapi. It
leaves me thinking that all this complexity wasn't wanted. A lot of
stereo movie just make a larger, or higher image to fit the views.
>
> Thanks!
> Ezequiel
>
> > >
> > > .. _h264_reference_flags:
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > index 3f3fbcd60cc6..6abd023f10c7 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > @@ -1721,6 +1721,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
> > >
> > > #define zero_padding(s) \
> > > memset(&(s).padding, 0, sizeof((s).padding))
> > > +#define zero_reserved(s) \
> > > + memset(&(s).reserved, 0, sizeof((s).reserved))
> > >
> > > /*
> > > * Compound controls validation requires setting unused fields/flags to
> > > zero
> > > @@ -1731,6 +1733,7 @@ static int std_validate_compound(const struct
> > > v4l2_ctrl *ctrl, u32 idx,
> > > {
> > > struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> > > struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> > > + struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
> > > struct v4l2_ctrl_hevc_sps *p_hevc_sps;
> > > struct v4l2_ctrl_hevc_pps *p_hevc_pps;
> > > struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
> > > @@ -1790,7 +1793,20 @@ static int std_validate_compound(const struct
> > > v4l2_ctrl *ctrl, u32 idx,
> > > case V4L2_CTRL_TYPE_H264_SPS:
> > > case V4L2_CTRL_TYPE_H264_PPS:
> > > case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
> > > + break;
> > > case V4L2_CTRL_TYPE_H264_SLICE_PARAMS:
> > > + p_h264_slice_params = p;
> > > +
> > > + for (i = 0; i < V4L2_H264_REF_PIC_LIST_LEN; i++) {
> > > + struct v4l2_h264_reference *ref0 =
> > > + &p_h264_slice_params->ref_pic_list0[i];
> > > + struct v4l2_h264_reference *ref1 =
> > > + &p_h264_slice_params->ref_pic_list1[i];
> > > +
> > > + zero_reserved(*ref0);
> > > + zero_reserved(*ref1);
> > > + }
> > > + break;
> > > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
> > > break;
> > >
> > > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> > > index 9b1cbc9bc38e..c6cbf178c1c9 100644
> > > --- a/include/media/h264-ctrls.h
> > > +++ b/include/media/h264-ctrls.h
> > > @@ -19,6 +19,8 @@
> > > */
> > > #define V4L2_H264_NUM_DPB_ENTRIES 16
> > >
> > > +#define V4L2_H264_REF_PIC_LIST_LEN 32
> > > +
> > > /* Our pixel format isn't stable at the moment */
> > > #define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264
> > > parsed slices */
> > >
> > > @@ -146,6 +148,7 @@ struct v4l2_h264_pred_weight_table {
> > > struct v4l2_h264_reference {
> > > __u8 flags;
> > > __u8 index;
> > > + __u16 reserved;
> > > };
> > >
> > > struct v4l2_ctrl_h264_slice_params {
> > > @@ -190,8 +193,8 @@ struct v4l2_ctrl_h264_slice_params {
> > > * Entries on each list are indices into
> > > * v4l2_ctrl_h264_decode_params.dpb[].
> > > */
> > > - struct v4l2_h264_reference ref_pic_list0[32];
> > > - struct v4l2_h264_reference ref_pic_list1[32];
> > > + struct v4l2_h264_reference ref_pic_list0[V4L2_H264_REF_PIC_LIST_LEN];
> > > + struct v4l2_h264_reference ref_pic_list1[V4L2_H264_REF_PIC_LIST_LEN];
> > >
> > > __u32 flags;
> > > };
>
>
signature.asc
Description: This is a digitally signed message part
