[libcamera-devel] [PATCH 07/15] v4l2: v4l2_camera_proxy: Fix v4l2-compliance format tests

[Jacopo Mondi]

Hi Laurent,

On Wed, Jun 17, 2020 at 06:49:11PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Jun 17, 2020 at 04:59:31PM +0200, Jacopo Mondi wrote:
> > On Tue, Jun 16, 2020 at 10:12:36PM +0900, Paul Elder wrote:
> > > Fix v4l2-compliance format tests, for enum_fmt, try_fmt, and g/s_fmt.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > >  src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++++--
> > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > index a5fa478..fd2785f 100644
> > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > @@ -179,6 +179,9 @@ void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig)
> > >  			  curV4L2Format_.fmt.pix.width,
> > >  			  curV4L2Format_.fmt.pix.height);
> > >  	curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> > > +	curV4L2Format_.fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
>
> Ah, here's the V4L2_PIX_FMT_PRIV_MAGIC I requested in a previous patch
> :-)
>
> > > +	curV4L2Format_.fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601;
> > > +	curV4L2Format_.fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE;
> >
> > Any specific reason why you chose these encoding and quantization ?
> > Why not simply use the _DEFAULT ones ?
>
> The _DEFAULT values are meant for userspace to ask the kernel to pick a
> default. The kernel should return real values.
>

Are you sure ?

$ git grep QUANTIZATION_DEFAULT drivers/media/i2c/ | wc -l
16
$ git grep V4L2_YCBCR_ENC_DEFAULT drivers/media/i2c/ | wc -l
13

and they're all assignements.

The V4L2 documentation reports:

V4L2_QUANTIZATION_DEFAULT
Use the default quantization encoding as defined by the colorspace.
This is always full range for R’G’B’ (except for the BT.2020
colorspace) and HSV. It is usually limited range for Y’CbCr.

V4L2_YCBCR_ENC_DEFAULT
Use the default Y’CbCr encoding as defined by the colorspace.


> > >  }
> > >
> > >  unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig)
> > > @@ -283,11 +286,15 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg)
> > >  	    arg->index >= streamConfig_.formats().pixelformats().size())
> > >  		return -EINVAL;
> > >
> > > +	/* \todo Set V4L2_FMT_FLAG_COMPRESSED for compressed formats. */
> > > +	arg->flags = 0;
> > >  	/* \todo Add map from format to description. */
> > > -	utils::strlcpy(reinterpret_cast<char *>(arg->description), "Video Format Description",
> > > -		       sizeof(arg->description));
> > > +	utils::strlcpy(reinterpret_cast<char *>(arg->description),
> > > +		       "Video Format Description", sizeof(arg->description));
> > >  	arg->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]);
> > >
> > > +	memset(arg->reserved, 0, sizeof(arg->reserved));
> > > +
> >
> > If I were to be picky, I would have split this to a separate patch.
> > At least please expand the commit message to mention what you're
> > fixing
>
> I'd indeed move the memset to the patch that adds support for
> V4L2_CAP_EXT_PIX_FORMAT. It should be done for all the format-related
> ioctls (of not already there).
>
> > >  	return 0;
> > >  }
> > >
> > > @@ -330,6 +337,9 @@ void V4L2CameraProxy::tryFormat(struct v4l2_format *arg)
> > >  					      arg->fmt.pix.width,
> > >  					      arg->fmt.pix.height);
> > >  	arg->fmt.pix.colorspace   = V4L2_COLORSPACE_SRGB;
> > > +	arg->fmt.pix.priv         = V4L2_PIX_FMT_PRIV_MAGIC;
> > > +	arg->fmt.pix.ycbcr_enc    = V4L2_YCBCR_ENC_601;
> > > +	arg->fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE;
> > >  }
> > >
> > >  int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg)
>
> --
> Regards,
>
> Laurent Pinchart