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

[Laurent Pinchart]

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.

> >  }
> >
> >  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