Re: [RFC PATCH 3/5] media: v4l2: Add m2m codec helpers

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 5 Aug 2019 13:53:20 -0300
Hans Verkuil <hverkuil@xs4all.nl> 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...