[{"id":10995,"web_url":"https://patchwork.libcamera.org/comment/10995/","msgid":"<20200629223013.GB21860@pendragon.ideasonboard.com>","date":"2020-06-29T22:30:13","subject":"Re: [libcamera-devel] [PATCH 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 12:14:11AM +0900, Paul Elder wrote:\n> Now that calculation of bytes per line, image size, and conversions to\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>  src/v4l2/v4l2_camera_proxy.cpp | 167 +++++++--------------------------\n>  src/v4l2/v4l2_camera_proxy.h   |   7 --\n>  2 files changed, 36 insertions(+), 138 deletions(-)\n> \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index c246570..0869797 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,21 @@ 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> +\tPixelFormatInfo formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n\nThis should be a const reference. Same for other locations below. I\nwould delete the copy constructor and assignement operators for\nPixelFormatInfo, but that will mess up initialization of the static map\n:-S\n\n> +\tunsigned int width = streamConfig.size.width;\n> +\tunsigned int height = streamConfig.size.height;\n> +\n> +\tcurV4L2Format_.fmt.pix.width        = width;\n> +\tcurV4L2Format_.fmt.pix.height       = height;\n> +\tcurV4L2Format_.fmt.pix.pixelformat  = formatInfo.v4l2Format;\n> +\tcurV4L2Format_.fmt.pix.field        = V4L2_FIELD_NONE;\n> +\tcurV4L2Format_.fmt.pix.bytesperline = formatInfo.bytesPerLine(width);\n> +\tcurV4L2Format_.fmt.pix.sizeimage    = formatInfo.imageSize(width, 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>  \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 +188,11 @@ 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> +\tPixelFormatInfo formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n> +\tunsigned int width = streamConfig.size.width;\n> +\tunsigned int height = streamConfig.size.height;\n> +\n> +\treturn formatInfo.imageSize(width, height);\n\nIn this case I would skip the local width and height variables. I wonder\nif PixelFormatInfo::imageSize(unsigned int width, unsigned int height)\nshouldn't become PixelFormatInfo::imageSize(const Size &size), so you could write\n\n\treturn formatInfo.imageSize(streamConfig.size);\n\n>  }\n>  \n>  void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)\n> @@ -253,12 +255,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> @@ -284,7 +287,8 @@ int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *file, struct v4l2_fmtdesc *\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 =\n> +\t\tPixelFormatInfo::info(streamConfig_.formats().pixelformats()[arg->index]).v4l2Format;\n\nA local PixelFormat variable could make this more readable.\n\n>  \n>  \tmemset(arg->reserved, 0, sizeof(arg->reserved));\n>  \n> @@ -306,7 +310,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 +322,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> +\tPixelFormatInfo 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.width, size.height);\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 +354,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 +500,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 +841,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 02568BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 29 Jun 2020 22:30:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 60E9E609DB;\n\tTue, 30 Jun 2020 00:30:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D4668603B5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 30 Jun 2020 00:30:17 +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 2AEB2299;\n\tTue, 30 Jun 2020 00:30:16 +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=\"T5Fe1xqq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593469817;\n\tbh=yyUhoOfC1rqLrXkuuEt0+ayqPyUAn4fRyRJekEAPlT8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T5Fe1xqqiN6SdkKostNCkENuKRQGr8FxzAVeBMTqF/RsB0+SODFW8QAfCq8PpzQnB\n\tucQkJv+LkO4fwSBRFmwM7EzooN8Tg5aDxK2ZgCbYL71v5aVFBafPaaPrH4jap+zZ7N\n\t4wj/QlIky5QE+jon3h91l3ql2Hy7wxrVwrmz51bI=","Date":"Tue, 30 Jun 2020 01:30:13 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20200629223013.GB21860@pendragon.ideasonboard.com>","References":"<20200629151411.216477-1-paul.elder@ideasonboard.com>\n\t<20200629151411.216477-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200629151411.216477-7-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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>"}}]