[libcamera-devel] [PATCH 07/15] v4l2: v4l2_camera_proxy: Fix v4l2-compliance format tests
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jun 17 18:00:55 CEST 2020
Hi Jacopo,
On Wed, Jun 17, 2020 at 06:01:21PM +0200, Jacopo Mondi wrote:
> On Wed, Jun 17, 2020 at 06:49:11PM +0300, Laurent Pinchart wrote:
> > 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.
You may be right. V4L2_COLORSPACE_DEFAULT is not allowed to be reported
by drivers, but for the other fields, it may be fine. Let's ask Hans.
> >>> }
> >>>
> >>> 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
More information about the libcamera-devel
mailing list