[{"id":11038,"web_url":"https://patchwork.libcamera.org/comment/11038/","msgid":"<20200701111531.GR5963@pendragon.ideasonboard.com>","date":"2020-07-01T11:15:31","subject":"Re: [libcamera-devel] [PATCH v2 6/6] v4l2: v4l2_camera_proxy: Use\n\tlibcamera formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Jun 30, 2020 at 11:58:08PM +0900, Paul Elder wrote:\n> Now that calculation of bytes per line, image size, and conversions to\n\ns/bytes per line/line stride/ :-) (or just stride)\n\n> and from v4l2 and drm formats have been moved into libcamera core,\n> removed them from V4L2CameraProxy. This has a positive side-effect of\n> cleaning up the code.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> \n> ---\n> Changes in v2:\n> - clean up the code a bit more\n> ---\n>  src/v4l2/v4l2_camera_proxy.cpp | 165 +++++++--------------------------\n>  src/v4l2/v4l2_camera_proxy.h   |   7 --\n>  2 files changed, 34 insertions(+), 138 deletions(-)\n> \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index c246570..485e4b6 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -20,6 +20,7 @@\n>  #include <libcamera/formats.h>\n>  #include <libcamera/object.h>\n>  \n> +#include \"libcamera/internal/formats.h\"\n>  #include \"libcamera/internal/log.h\"\n>  #include \"libcamera/internal/utils.h\"\n>  \n> @@ -164,22 +165,20 @@ bool V4L2CameraProxy::validateMemoryType(uint32_t memory)\n>  \n>  void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig)\n>  {\n> -\tcurV4L2Format_.fmt.pix.width = streamConfig.size.width;\n> -\tcurV4L2Format_.fmt.pix.height = streamConfig.size.height;\n> -\tcurV4L2Format_.fmt.pix.pixelformat = drmToV4L2(streamConfig.pixelFormat);\n> -\tcurV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;\n> -\tcurV4L2Format_.fmt.pix.bytesperline =\n> -\t\tbplMultiplier(curV4L2Format_.fmt.pix.pixelformat) *\n> -\t\tcurV4L2Format_.fmt.pix.width;\n> -\tcurV4L2Format_.fmt.pix.sizeimage =\n> -\t\timageSize(curV4L2Format_.fmt.pix.pixelformat,\n> -\t\t\t  curV4L2Format_.fmt.pix.width,\n> -\t\t\t  curV4L2Format_.fmt.pix.height);\n> -\tcurV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\n> -\tcurV4L2Format_.fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;\n> -\tcurV4L2Format_.fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(streamConfig.pixelFormat);\n> +\tSize size = streamConfig.size;\n> +\n> +\tcurV4L2Format_.fmt.pix.width        = size.width;\n> +\tcurV4L2Format_.fmt.pix.height       = size.height;\n> +\tcurV4L2Format_.fmt.pix.pixelformat  = info.v4l2Format;\n> +\tcurV4L2Format_.fmt.pix.field        = V4L2_FIELD_NONE;\n> +\tcurV4L2Format_.fmt.pix.bytesperline = info.bytesPerLine(size.width);\n> +\tcurV4L2Format_.fmt.pix.sizeimage    = info.imageSize(size);\n> +\tcurV4L2Format_.fmt.pix.colorspace   = V4L2_COLORSPACE_SRGB;\n> +\tcurV4L2Format_.fmt.pix.priv         = V4L2_PIX_FMT_PRIV_MAGIC;\n> +\tcurV4L2Format_.fmt.pix.ycbcr_enc    = V4L2_YCBCR_ENC_DEFAULT;\n>  \tcurV4L2Format_.fmt.pix.quantization = V4L2_QUANTIZATION_DEFAULT;\n> -\tcurV4L2Format_.fmt.pix.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> +\tcurV4L2Format_.fmt.pix.xfer_func    = V4L2_XFER_FUNC_DEFAULT;\n>  }\n>  \n>  unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig)\n> @@ -188,9 +187,9 @@ unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConf\n>  \t * \\todo Merge this method with setFmtFromConfig (need imageSize to\n>  \t * support all libcamera formats first, or filter out MJPEG for now).\n>  \t */\n> -\treturn imageSize(drmToV4L2(streamConfig.pixelFormat),\n> -\t\t\t streamConfig.size.width,\n> -\t\t\t streamConfig.size.height);\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(streamConfig.pixelFormat);\n> +\n> +\treturn info.imageSize(streamConfig.size);\n>  }\n>  \n>  void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)\n> @@ -253,12 +252,13 @@ int V4L2CameraProxy::vidioc_enum_framesizes(V4L2CameraFile *file, struct v4l2_fr\n>  {\n>  \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_framesizes fd = \" << file->efd();\n>  \n> -\tPixelFormat argFormat = v4l2ToDrm(arg->pixel_format);\n> +\tV4L2PixelFormat v4l2Format = V4L2PixelFormat(arg->pixel_format);\n> +\tPixelFormat format = PixelFormatInfo::info(v4l2Format).format;\n>  \t/*\n>  \t * \\todo This might need to be expanded as few pipeline handlers\n>  \t * report StreamFormats.\n>  \t */\n> -\tconst std::vector<Size> &frameSizes = streamConfig_.formats().sizes(argFormat);\n> +\tconst std::vector<Size> &frameSizes = streamConfig_.formats().sizes(format);\n>  \n>  \tif (arg->index >= frameSizes.size())\n>  \t\treturn -EINVAL;\n> @@ -279,12 +279,14 @@ int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *file, struct v4l2_fmtdesc *\n>  \t    arg->index >= streamConfig_.formats().pixelformats().size())\n>  \t\treturn -EINVAL;\n>  \n> +\tPixelFormat format = streamConfig_.formats().pixelformats()[arg->index];\n> +\n>  \t/* \\todo Set V4L2_FMT_FLAG_COMPRESSED for compressed formats. */\n\nHow about addressing this one ? :-) I think we can simply check if\nbytesPerGroup is zero for now.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  \targ->flags = 0;\n>  \t/* \\todo Add map from format to description. */\n>  \tutils::strlcpy(reinterpret_cast<char *>(arg->description),\n>  \t\t       \"Video Format Description\", sizeof(arg->description));\n> -\targ->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]);\n> +\targ->pixelformat = PixelFormatInfo::info(format).v4l2Format;\n>  \n>  \tmemset(arg->reserved, 0, sizeof(arg->reserved));\n>  \n> @@ -306,7 +308,8 @@ int V4L2CameraProxy::vidioc_g_fmt(V4L2CameraFile *file, struct v4l2_format *arg)\n>  \n>  void V4L2CameraProxy::tryFormat(struct v4l2_format *arg)\n>  {\n> -\tPixelFormat format = v4l2ToDrm(arg->fmt.pix.pixelformat);\n> +\tV4L2PixelFormat v4l2Format = V4L2PixelFormat(arg->fmt.pix.pixelformat);\n> +\tPixelFormat format = PixelFormatInfo::info(v4l2Format).format;\n>  \tconst std::vector<PixelFormat> &formats =\n>  \t\tstreamConfig_.formats().pixelformats();\n>  \tif (std::find(formats.begin(), formats.end(), format) == formats.end())\n> @@ -317,15 +320,14 @@ void V4L2CameraProxy::tryFormat(struct v4l2_format *arg)\n>  \tif (std::find(sizes.begin(), sizes.end(), size) == sizes.end())\n>  \t\tsize = streamConfig_.formats().sizes(format)[0];\n>  \n> +\tconst PixelFormatInfo &formatInfo = PixelFormatInfo::info(format);\n> +\n>  \targ->fmt.pix.width        = size.width;\n>  \targ->fmt.pix.height       = size.height;\n> -\targ->fmt.pix.pixelformat  = drmToV4L2(format);\n> +\targ->fmt.pix.pixelformat  = formatInfo.v4l2Format;\n>  \targ->fmt.pix.field        = V4L2_FIELD_NONE;\n> -\targ->fmt.pix.bytesperline = bplMultiplier(drmToV4L2(format)) *\n> -\t\t\t\t    arg->fmt.pix.width;\n> -\targ->fmt.pix.sizeimage    = imageSize(drmToV4L2(format),\n> -\t\t\t\t\t      arg->fmt.pix.width,\n> -\t\t\t\t\t      arg->fmt.pix.height);\n> +\targ->fmt.pix.bytesperline = formatInfo.bytesPerLine(size.width);\n> +\targ->fmt.pix.sizeimage    = formatInfo.imageSize(size);\n>  \targ->fmt.pix.colorspace   = V4L2_COLORSPACE_SRGB;\n>  \targ->fmt.pix.priv         = V4L2_PIX_FMT_PRIV_MAGIC;\n>  \targ->fmt.pix.ycbcr_enc    = V4L2_YCBCR_ENC_DEFAULT;\n> @@ -350,8 +352,9 @@ int V4L2CameraProxy::vidioc_s_fmt(V4L2CameraFile *file, struct v4l2_format *arg)\n>  \ttryFormat(arg);\n>  \n>  \tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> +\tV4L2PixelFormat v4l2Format = V4L2PixelFormat(arg->fmt.pix.pixelformat);\n>  \tret = vcam_->configure(&streamConfig_, size,\n> -\t\t\t       v4l2ToDrm(arg->fmt.pix.pixelformat),\n> +\t\t\t       PixelFormatInfo::info(v4l2Format).format,\n>  \t\t\t       bufferCount_);\n>  \tif (ret < 0)\n>  \t\treturn -EINVAL;\n> @@ -495,8 +498,9 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf\n>  \t\tfreeBuffers();\n>  \n>  \tSize size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height);\n> +\tV4L2PixelFormat v4l2Format = V4L2PixelFormat(curV4L2Format_.fmt.pix.pixelformat);\n>  \tint ret = vcam_->configure(&streamConfig_, size,\n> -\t\t\t\t   v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat),\n> +\t\t\t\t   PixelFormatInfo::info(v4l2Format).format,\n>  \t\t\t\t   arg->count);\n>  \tif (ret < 0)\n>  \t\treturn -EINVAL;\n> @@ -835,104 +839,3 @@ void V4L2CameraProxy::release(V4L2CameraFile *file)\n>  \n>  \towner_ = nullptr;\n>  }\n> -\n> -struct PixelFormatPlaneInfo {\n> -\tunsigned int bitsPerPixel;\n> -\tunsigned int hSubSampling;\n> -\tunsigned int vSubSampling;\n> -};\n> -\n> -struct PixelFormatInfo {\n> -\tPixelFormat format;\n> -\tuint32_t v4l2Format;\n> -\tunsigned int numPlanes;\n> -\tstd::array<PixelFormatPlaneInfo, 3> planes;\n> -};\n> -\n> -namespace {\n> -\n> -static const std::array<PixelFormatInfo, 16> pixelFormatInfo = {{\n> -\t/* RGB formats. */\n> -\t{ formats::RGB888,\tV4L2_PIX_FMT_BGR24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ formats::BGR888,\tV4L2_PIX_FMT_RGB24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ formats::BGRA8888,\tV4L2_PIX_FMT_ARGB32,\t1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t/* YUV packed formats. */\n> -\t{ formats::UYVY,\tV4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ formats::VYUY,\tV4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ formats::YUYV,\tV4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ formats::YVYU,\tV4L2_PIX_FMT_YVYU,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t/* YUY planar formats. */\n> -\t{ formats::NV12,\tV4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> -\t{ formats::NV21,\tV4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> -\t{ formats::NV16,\tV4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> -\t{ formats::NV61,\tV4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> -\t{ formats::NV24,\tV4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> -\t{ formats::NV42,\tV4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> -\t{ formats::YUV420,\tV4L2_PIX_FMT_YUV420,\t3, {{ {  8, 1, 1 }, {  8, 2, 2 }, {  8, 2, 2 } }} },\n> -\t{ formats::YUV422,\tV4L2_PIX_FMT_YUV422P,\t3, {{ {  8, 1, 1 }, {  8, 2, 1 }, {  8, 2, 1 } }} },\n> -\t/* Compressed formats. */\n> -\t/*\n> -\t * \\todo Get a better image size estimate for MJPEG, via\n> -\t * StreamConfiguration, instead of using the worst-case\n> -\t * width * height * bpp of uncompressed data.\n> -\t */\n> -\t{ formats::MJPEG,\tV4L2_PIX_FMT_MJPEG,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -}};\n> -\n> -} /* namespace */\n> -\n> -/* \\todo make libcamera export these */\n> -unsigned int V4L2CameraProxy::bplMultiplier(uint32_t format)\n> -{\n> -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> -\t\t\t\t [format](const PixelFormatInfo &info) {\n> -\t\t\t\t\t return info.v4l2Format == format;\n> -\t\t\t\t });\n> -\tif (info == pixelFormatInfo.end())\n> -\t\treturn 0;\n> -\n> -\treturn info->planes[0].bitsPerPixel / 8;\n> -}\n> -\n> -unsigned int V4L2CameraProxy::imageSize(uint32_t format, unsigned int width,\n> -\t\t\t\t\tunsigned int height)\n> -{\n> -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> -\t\t\t\t [format](const PixelFormatInfo &info) {\n> -\t\t\t\t\t return info.v4l2Format == format;\n> -\t\t\t\t });\n> -\tif (info == pixelFormatInfo.end())\n> -\t\treturn 0;\n> -\n> -\tunsigned int multiplier = 0;\n> -\tfor (unsigned int i = 0; i < info->numPlanes; ++i)\n> -\t\tmultiplier += info->planes[i].bitsPerPixel\n> -\t\t\t    / info->planes[i].hSubSampling\n> -\t\t\t    / info->planes[i].vSubSampling;\n> -\n> -\treturn width * height * multiplier / 8;\n> -}\n> -\n> -PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)\n> -{\n> -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> -\t\t\t\t [format](const PixelFormatInfo &info) {\n> -\t\t\t\t\t return info.v4l2Format == format;\n> -\t\t\t\t });\n> -\tif (info == pixelFormatInfo.end())\n> -\t\treturn PixelFormat();\n> -\n> -\treturn info->format;\n> -}\n> -\n> -uint32_t V4L2CameraProxy::drmToV4L2(const PixelFormat &format)\n> -{\n> -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> -\t\t\t\t [format](const PixelFormatInfo &info) {\n> -\t\t\t\t\t return info.format == format;\n> -\t\t\t\t });\n> -\tif (info == pixelFormatInfo.end())\n> -\t\treturn format;\n> -\n> -\treturn info->v4l2Format;\n> -}\n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> index d78a472..49184a1 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -69,13 +69,6 @@ private:\n>  \tint acquire(V4L2CameraFile *file);\n>  \tvoid release(V4L2CameraFile *file);\n>  \n> -\tstatic unsigned int bplMultiplier(uint32_t format);\n> -\tstatic unsigned int imageSize(uint32_t format, unsigned int width,\n> -\t\t\t\t      unsigned int height);\n> -\n> -\tstatic PixelFormat v4l2ToDrm(uint32_t format);\n> -\tstatic uint32_t drmToV4L2(const PixelFormat &format);\n> -\n>  \tstatic const std::set<unsigned long> supportedIoctls_;\n>  \n>  \tunsigned int refcount_;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 137FDBFFE2\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  1 Jul 2020 11:15:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B3F360C56;\n\tWed,  1 Jul 2020 13:15:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC1C8603B4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  1 Jul 2020 13:15:35 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3963F556;\n\tWed,  1 Jul 2020 13:15:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dvE0yA/Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593602135;\n\tbh=1syez5RbmCb6FxUdLbeo7dv9+WYIe7+tgDpGI6iO3/E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dvE0yA/QiN+EtSk1E6PISQGvhSiUt5TnRr0SpI10II0RIA/ILcMdMR5JLrehd4oFk\n\tmVyLM1Fp96VadKYSI4n47L6zF5MBaf4dED8rTvaTGF3FwWoBZQNkj3syoSFfFjYnnL\n\tu5wklI9ir1uAUDoZ4yG6AeufQHqWpfyru4dHQoXw=","Date":"Wed, 1 Jul 2020 14:15:31 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20200701111531.GR5963@pendragon.ideasonboard.com>","References":"<20200630145808.2976956-1-paul.elder@ideasonboard.com>\n\t<20200630145808.2976956-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200630145808.2976956-7-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 6/6] v4l2: v4l2_camera_proxy: Use\n\tlibcamera formats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11041,"web_url":"https://patchwork.libcamera.org/comment/11041/","msgid":"<20200701122949.GA26682@pendragon.ideasonboard.com>","date":"2020-07-01T12:29:49","subject":"Re: [libcamera-devel] [PATCH v2 6/6] v4l2: v4l2_camera_proxy: Use\n\tlibcamera formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"And another thing.\n\nOn Wed, Jul 01, 2020 at 02:15:32PM +0300, Laurent Pinchart wrote:\n> On Tue, Jun 30, 2020 at 11:58:08PM +0900, Paul Elder wrote:\n> > Now that calculation of bytes per line, image size, and conversions to\n> \n> s/bytes per line/line stride/ :-) (or just stride)\n> \n> > and from v4l2 and drm formats have been moved into libcamera core,\n> > removed them from V4L2CameraProxy. This has a positive side-effect of\n> > cleaning up the code.\n> > \n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > \n> > ---\n> > Changes in v2:\n> > - clean up the code a bit more\n> > ---\n> >  src/v4l2/v4l2_camera_proxy.cpp | 165 +++++++--------------------------\n> >  src/v4l2/v4l2_camera_proxy.h   |   7 --\n> >  2 files changed, 34 insertions(+), 138 deletions(-)\n> > \n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index c246570..485e4b6 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -20,6 +20,7 @@\n> >  #include <libcamera/formats.h>\n> >  #include <libcamera/object.h>\n> >  \n> > +#include \"libcamera/internal/formats.h\"\n> >  #include \"libcamera/internal/log.h\"\n> >  #include \"libcamera/internal/utils.h\"\n> >  \n> > @@ -164,22 +165,20 @@ bool V4L2CameraProxy::validateMemoryType(uint32_t memory)\n> >  \n> >  void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig)\n> >  {\n> > -\tcurV4L2Format_.fmt.pix.width = streamConfig.size.width;\n> > -\tcurV4L2Format_.fmt.pix.height = streamConfig.size.height;\n> > -\tcurV4L2Format_.fmt.pix.pixelformat = drmToV4L2(streamConfig.pixelFormat);\n> > -\tcurV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;\n> > -\tcurV4L2Format_.fmt.pix.bytesperline =\n> > -\t\tbplMultiplier(curV4L2Format_.fmt.pix.pixelformat) *\n> > -\t\tcurV4L2Format_.fmt.pix.width;\n> > -\tcurV4L2Format_.fmt.pix.sizeimage =\n> > -\t\timageSize(curV4L2Format_.fmt.pix.pixelformat,\n> > -\t\t\t  curV4L2Format_.fmt.pix.width,\n> > -\t\t\t  curV4L2Format_.fmt.pix.height);\n> > -\tcurV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\n> > -\tcurV4L2Format_.fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;\n> > -\tcurV4L2Format_.fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> > +\tconst PixelFormatInfo &info = PixelFormatInfo::info(streamConfig.pixelFormat);\n> > +\tSize size = streamConfig.size;\n> > +\n> > +\tcurV4L2Format_.fmt.pix.width        = size.width;\n> > +\tcurV4L2Format_.fmt.pix.height       = size.height;\n> > +\tcurV4L2Format_.fmt.pix.pixelformat  = info.v4l2Format;\n> > +\tcurV4L2Format_.fmt.pix.field        = V4L2_FIELD_NONE;\n> > +\tcurV4L2Format_.fmt.pix.bytesperline = info.bytesPerLine(size.width);\n\nStreamConfiguration has a stride field, that's what you should use here\ninstead of calculating it. It allows pipeline handlers to convey\nadditional alignment constraints. The PixelFormatInfo::bytesPerLine()\nfunction is still a good idea, but it should be used by the pipeline\nhandlers (if needed), not the V4L2 compatibility layer.\n\n> > +\tcurV4L2Format_.fmt.pix.sizeimage    = info.imageSize(size);\n> > +\tcurV4L2Format_.fmt.pix.colorspace   = V4L2_COLORSPACE_SRGB;\n> > +\tcurV4L2Format_.fmt.pix.priv         = V4L2_PIX_FMT_PRIV_MAGIC;\n> > +\tcurV4L2Format_.fmt.pix.ycbcr_enc    = V4L2_YCBCR_ENC_DEFAULT;\n> >  \tcurV4L2Format_.fmt.pix.quantization = V4L2_QUANTIZATION_DEFAULT;\n> > -\tcurV4L2Format_.fmt.pix.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> > +\tcurV4L2Format_.fmt.pix.xfer_func    = V4L2_XFER_FUNC_DEFAULT;\n> >  }\n> >  \n> >  unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig)\n> > @@ -188,9 +187,9 @@ unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConf\n> >  \t * \\todo Merge this method with setFmtFromConfig (need imageSize to\n> >  \t * support all libcamera formats first, or filter out MJPEG for now).\n> >  \t */\n> > -\treturn imageSize(drmToV4L2(streamConfig.pixelFormat),\n> > -\t\t\t streamConfig.size.width,\n> > -\t\t\t streamConfig.size.height);\n> > +\tconst PixelFormatInfo &info = PixelFormatInfo::info(streamConfig.pixelFormat);\n> > +\n> > +\treturn info.imageSize(streamConfig.size);\n> >  }\n> >  \n> >  void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)\n> > @@ -253,12 +252,13 @@ int V4L2CameraProxy::vidioc_enum_framesizes(V4L2CameraFile *file, struct v4l2_fr\n> >  {\n> >  \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_framesizes fd = \" << file->efd();\n> >  \n> > -\tPixelFormat argFormat = v4l2ToDrm(arg->pixel_format);\n> > +\tV4L2PixelFormat v4l2Format = V4L2PixelFormat(arg->pixel_format);\n> > +\tPixelFormat format = PixelFormatInfo::info(v4l2Format).format;\n> >  \t/*\n> >  \t * \\todo This might need to be expanded as few pipeline handlers\n> >  \t * report StreamFormats.\n> >  \t */\n> > -\tconst std::vector<Size> &frameSizes = streamConfig_.formats().sizes(argFormat);\n> > +\tconst std::vector<Size> &frameSizes = streamConfig_.formats().sizes(format);\n> >  \n> >  \tif (arg->index >= frameSizes.size())\n> >  \t\treturn -EINVAL;\n> > @@ -279,12 +279,14 @@ int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *file, struct v4l2_fmtdesc *\n> >  \t    arg->index >= streamConfig_.formats().pixelformats().size())\n> >  \t\treturn -EINVAL;\n> >  \n> > +\tPixelFormat format = streamConfig_.formats().pixelformats()[arg->index];\n> > +\n> >  \t/* \\todo Set V4L2_FMT_FLAG_COMPRESSED for compressed formats. */\n> \n> How about addressing this one ? :-) I think we can simply check if\n> bytesPerGroup is zero for now.\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> >  \targ->flags = 0;\n> >  \t/* \\todo Add map from format to description. */\n> >  \tutils::strlcpy(reinterpret_cast<char *>(arg->description),\n> >  \t\t       \"Video Format Description\", sizeof(arg->description));\n> > -\targ->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]);\n> > +\targ->pixelformat = PixelFormatInfo::info(format).v4l2Format;\n> >  \n> >  \tmemset(arg->reserved, 0, sizeof(arg->reserved));\n> >  \n> > @@ -306,7 +308,8 @@ int V4L2CameraProxy::vidioc_g_fmt(V4L2CameraFile *file, struct v4l2_format *arg)\n> >  \n> >  void V4L2CameraProxy::tryFormat(struct v4l2_format *arg)\n> >  {\n> > -\tPixelFormat format = v4l2ToDrm(arg->fmt.pix.pixelformat);\n> > +\tV4L2PixelFormat v4l2Format = V4L2PixelFormat(arg->fmt.pix.pixelformat);\n> > +\tPixelFormat format = PixelFormatInfo::info(v4l2Format).format;\n> >  \tconst std::vector<PixelFormat> &formats =\n> >  \t\tstreamConfig_.formats().pixelformats();\n> >  \tif (std::find(formats.begin(), formats.end(), format) == formats.end())\n> > @@ -317,15 +320,14 @@ void V4L2CameraProxy::tryFormat(struct v4l2_format *arg)\n> >  \tif (std::find(sizes.begin(), sizes.end(), size) == sizes.end())\n> >  \t\tsize = streamConfig_.formats().sizes(format)[0];\n> >  \n> > +\tconst PixelFormatInfo &formatInfo = PixelFormatInfo::info(format);\n> > +\n> >  \targ->fmt.pix.width        = size.width;\n> >  \targ->fmt.pix.height       = size.height;\n> > -\targ->fmt.pix.pixelformat  = drmToV4L2(format);\n> > +\targ->fmt.pix.pixelformat  = formatInfo.v4l2Format;\n> >  \targ->fmt.pix.field        = V4L2_FIELD_NONE;\n> > -\targ->fmt.pix.bytesperline = bplMultiplier(drmToV4L2(format)) *\n> > -\t\t\t\t    arg->fmt.pix.width;\n> > -\targ->fmt.pix.sizeimage    = imageSize(drmToV4L2(format),\n> > -\t\t\t\t\t      arg->fmt.pix.width,\n> > -\t\t\t\t\t      arg->fmt.pix.height);\n> > +\targ->fmt.pix.bytesperline = formatInfo.bytesPerLine(size.width);\n> > +\targ->fmt.pix.sizeimage    = formatInfo.imageSize(size);\n> >  \targ->fmt.pix.colorspace   = V4L2_COLORSPACE_SRGB;\n> >  \targ->fmt.pix.priv         = V4L2_PIX_FMT_PRIV_MAGIC;\n> >  \targ->fmt.pix.ycbcr_enc    = V4L2_YCBCR_ENC_DEFAULT;\n> > @@ -350,8 +352,9 @@ int V4L2CameraProxy::vidioc_s_fmt(V4L2CameraFile *file, struct v4l2_format *arg)\n> >  \ttryFormat(arg);\n> >  \n> >  \tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> > +\tV4L2PixelFormat v4l2Format = V4L2PixelFormat(arg->fmt.pix.pixelformat);\n> >  \tret = vcam_->configure(&streamConfig_, size,\n> > -\t\t\t       v4l2ToDrm(arg->fmt.pix.pixelformat),\n> > +\t\t\t       PixelFormatInfo::info(v4l2Format).format,\n> >  \t\t\t       bufferCount_);\n> >  \tif (ret < 0)\n> >  \t\treturn -EINVAL;\n> > @@ -495,8 +498,9 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf\n> >  \t\tfreeBuffers();\n> >  \n> >  \tSize size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height);\n> > +\tV4L2PixelFormat v4l2Format = V4L2PixelFormat(curV4L2Format_.fmt.pix.pixelformat);\n> >  \tint ret = vcam_->configure(&streamConfig_, size,\n> > -\t\t\t\t   v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat),\n> > +\t\t\t\t   PixelFormatInfo::info(v4l2Format).format,\n> >  \t\t\t\t   arg->count);\n> >  \tif (ret < 0)\n> >  \t\treturn -EINVAL;\n> > @@ -835,104 +839,3 @@ void V4L2CameraProxy::release(V4L2CameraFile *file)\n> >  \n> >  \towner_ = nullptr;\n> >  }\n> > -\n> > -struct PixelFormatPlaneInfo {\n> > -\tunsigned int bitsPerPixel;\n> > -\tunsigned int hSubSampling;\n> > -\tunsigned int vSubSampling;\n> > -};\n> > -\n> > -struct PixelFormatInfo {\n> > -\tPixelFormat format;\n> > -\tuint32_t v4l2Format;\n> > -\tunsigned int numPlanes;\n> > -\tstd::array<PixelFormatPlaneInfo, 3> planes;\n> > -};\n> > -\n> > -namespace {\n> > -\n> > -static const std::array<PixelFormatInfo, 16> pixelFormatInfo = {{\n> > -\t/* RGB formats. */\n> > -\t{ formats::RGB888,\tV4L2_PIX_FMT_BGR24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -\t{ formats::BGR888,\tV4L2_PIX_FMT_RGB24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -\t{ formats::BGRA8888,\tV4L2_PIX_FMT_ARGB32,\t1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -\t/* YUV packed formats. */\n> > -\t{ formats::UYVY,\tV4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -\t{ formats::VYUY,\tV4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -\t{ formats::YUYV,\tV4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -\t{ formats::YVYU,\tV4L2_PIX_FMT_YVYU,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -\t/* YUY planar formats. */\n> > -\t{ formats::NV12,\tV4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > -\t{ formats::NV21,\tV4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > -\t{ formats::NV16,\tV4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > -\t{ formats::NV61,\tV4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > -\t{ formats::NV24,\tV4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> > -\t{ formats::NV42,\tV4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> > -\t{ formats::YUV420,\tV4L2_PIX_FMT_YUV420,\t3, {{ {  8, 1, 1 }, {  8, 2, 2 }, {  8, 2, 2 } }} },\n> > -\t{ formats::YUV422,\tV4L2_PIX_FMT_YUV422P,\t3, {{ {  8, 1, 1 }, {  8, 2, 1 }, {  8, 2, 1 } }} },\n> > -\t/* Compressed formats. */\n> > -\t/*\n> > -\t * \\todo Get a better image size estimate for MJPEG, via\n> > -\t * StreamConfiguration, instead of using the worst-case\n> > -\t * width * height * bpp of uncompressed data.\n> > -\t */\n> > -\t{ formats::MJPEG,\tV4L2_PIX_FMT_MJPEG,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -}};\n> > -\n> > -} /* namespace */\n> > -\n> > -/* \\todo make libcamera export these */\n> > -unsigned int V4L2CameraProxy::bplMultiplier(uint32_t format)\n> > -{\n> > -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> > -\t\t\t\t [format](const PixelFormatInfo &info) {\n> > -\t\t\t\t\t return info.v4l2Format == format;\n> > -\t\t\t\t });\n> > -\tif (info == pixelFormatInfo.end())\n> > -\t\treturn 0;\n> > -\n> > -\treturn info->planes[0].bitsPerPixel / 8;\n> > -}\n> > -\n> > -unsigned int V4L2CameraProxy::imageSize(uint32_t format, unsigned int width,\n> > -\t\t\t\t\tunsigned int height)\n> > -{\n> > -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> > -\t\t\t\t [format](const PixelFormatInfo &info) {\n> > -\t\t\t\t\t return info.v4l2Format == format;\n> > -\t\t\t\t });\n> > -\tif (info == pixelFormatInfo.end())\n> > -\t\treturn 0;\n> > -\n> > -\tunsigned int multiplier = 0;\n> > -\tfor (unsigned int i = 0; i < info->numPlanes; ++i)\n> > -\t\tmultiplier += info->planes[i].bitsPerPixel\n> > -\t\t\t    / info->planes[i].hSubSampling\n> > -\t\t\t    / info->planes[i].vSubSampling;\n> > -\n> > -\treturn width * height * multiplier / 8;\n> > -}\n> > -\n> > -PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)\n> > -{\n> > -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> > -\t\t\t\t [format](const PixelFormatInfo &info) {\n> > -\t\t\t\t\t return info.v4l2Format == format;\n> > -\t\t\t\t });\n> > -\tif (info == pixelFormatInfo.end())\n> > -\t\treturn PixelFormat();\n> > -\n> > -\treturn info->format;\n> > -}\n> > -\n> > -uint32_t V4L2CameraProxy::drmToV4L2(const PixelFormat &format)\n> > -{\n> > -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> > -\t\t\t\t [format](const PixelFormatInfo &info) {\n> > -\t\t\t\t\t return info.format == format;\n> > -\t\t\t\t });\n> > -\tif (info == pixelFormatInfo.end())\n> > -\t\treturn format;\n> > -\n> > -\treturn info->v4l2Format;\n> > -}\n> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> > index d78a472..49184a1 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.h\n> > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > @@ -69,13 +69,6 @@ private:\n> >  \tint acquire(V4L2CameraFile *file);\n> >  \tvoid release(V4L2CameraFile *file);\n> >  \n> > -\tstatic unsigned int bplMultiplier(uint32_t format);\n> > -\tstatic unsigned int imageSize(uint32_t format, unsigned int width,\n> > -\t\t\t\t      unsigned int height);\n> > -\n> > -\tstatic PixelFormat v4l2ToDrm(uint32_t format);\n> > -\tstatic uint32_t drmToV4L2(const PixelFormat &format);\n> > -\n> >  \tstatic const std::set<unsigned long> supportedIoctls_;\n> >  \n> >  \tunsigned int refcount_;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 74F1FBE905\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  1 Jul 2020 12:29:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE64360C59;\n\tWed,  1 Jul 2020 14:29:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 12F61609A9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  1 Jul 2020 14:29:54 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 71E6E556;\n\tWed,  1 Jul 2020 14:29:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NkfT7WC2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593606593;\n\tbh=vZTrQD6+ucHKYmKIA8YThU+B+0ZSwOOzcAv3PKmTW7Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NkfT7WC2bz4wNEpnZ0xDLttWz4ECNsBq3zmLfLFdzq6imT0ykF1RMHjNgXPFy3LRO\n\tR3RtUCO9hQJqVV0owkhXgZ2AkrherOejBlU7YNLl9ErOYSQu0vhItzQXw6O81CWtsT\n\ttbNtvmsnwzxQQ8F390BQY6eITeVaX7YNHS4etpWY=","Date":"Wed, 1 Jul 2020 15:29:49 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20200701122949.GA26682@pendragon.ideasonboard.com>","References":"<20200630145808.2976956-1-paul.elder@ideasonboard.com>\n\t<20200630145808.2976956-7-paul.elder@ideasonboard.com>\n\t<20200701111531.GR5963@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200701111531.GR5963@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 6/6] v4l2: v4l2_camera_proxy: Use\n\tlibcamera formats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]