On Mon, 5 Aug 2019 13:53:20 -0300
Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> > +/**
> > + * struct v4l2_m2m_codec_ctx - Codec context
> > + * @fh: file handle
> > + * @coded_fmt: current coded format
> > + * @decoded_fmt: current decoded format
> > + * @coded_fmt_desc: current coded format desc
> > + * @decoded_fmt_desc: current decoded format desc
> > + * @ctrl_hdl: control handler
> > + * @codec: the codec that has created this context
> > + */
> > +struct v4l2_m2m_codec_ctx {
> > + struct v4l2_fh fh;
> > + struct v4l2_format coded_fmt;
> > + struct v4l2_format decoded_fmt;
> > + const struct v4l2_m2m_codec_coded_fmt_desc *coded_fmt_desc;
> > + const struct v4l2_m2m_codec_decoded_fmt_desc *decoded_fmt_desc;
> > + struct v4l2_ctrl_handler ctrl_hdl;
> > + struct v4l2_m2m_codec *codec;
> > +};
>
> ...this struct.
>
> So basically everything in this header :-)
>
> I haven't done an in-depth review, but my main concern is that I
> believe these structs and the helpers depending on them are too
> high-level.
>
> The helpers themselves often look reasonable, except that they could
> be more generic if it wasn't for these high-level structs.
I'll have to double check, but I fear most of those helpers are useless
if we don't have these generic structs.
>
> My feeling is that it would make more sense if you would create structs
> dealing just with formats and structs just for controls and don't try
> to mix in things like struct video_device or struct v4l2_fh.
Except that's where most of the boiler-plate code is (basically all the
ioctl and vb2_queue ops).
>
> I think that will create a better balance between providing helpers
> for codec drivers without hiding too much inside v4l2_m2m_codec_* structs.
I'm fine with that, just not sure this will significantly reduce code
duplication...
