[{"id":24821,"web_url":"https://patchwork.libcamera.org/comment/24821/","msgid":"","date":"2022-08-29T22:09:23","subject":"Re: [libcamera-devel] [PATCH v3 3/7] libcamera: v4l2_videodevice:\n\tImprove toColorSpace() readability","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Mon, Aug 29, 2022 at 10:07:38PM +0530, Umang Jain via libcamera-devel wrote:\n> Abstract out usage of V4L2Device::toColorspace() to a separate\n> namespace. Since it is separated out in an unnamed namespace,\n> the V4L2Device::toColorSpace() function needs to made public.\n> This is not an issue since V4L2Device is an internal class.\n> \n> No functional changes intended.\n> \n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/internal/v4l2_device.h | 14 +++++++-------\n> src/libcamera/v4l2_videodevice.cpp | 23 +++++++++++++++--------\n> 2 files changed, 22 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 75304be1..606332e1 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -49,6 +49,13 @@ public:\n> \n> \tvoid updateControlInfo();\n> \n> +\ttemplate\n> +\tstatic std::optional toColorSpace(const T &v4l2Format,\n> +\t\t\t\t\t\t PixelFormatInfo::ColourEncoding colourEncoding);\n> +\n> +\ttemplate\n> +\tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> +\n> protected:\n> \tV4L2Device(const std::string &deviceNode);\n> \t~V4L2Device();\n> @@ -60,13 +67,6 @@ protected:\n> \n> \tint fd() const { return fd_.get(); }\n> \n> -\ttemplate\n> -\tstatic std::optional toColorSpace(const T &v4l2Format,\n> -\t\t\t\t\t\t PixelFormatInfo::ColourEncoding colourEncoding);\n> -\n> -\ttemplate\n> -\tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> -\n> private:\n> \tstatic ControlType v4l2CtrlType(uint32_t ctrlType);\n> \tstatic std::unique_ptr v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 0e3f5436..b443253d 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -914,6 +914,17 @@ int V4L2VideoDevice::trySetFormatMeta(V4L2DeviceFormat *format, bool set)\n> \treturn 0;\n> }\n> \n> +namespace {\n> +\n> +template\n> +std::optional getColorSpace(const T &v4l2Format)\n> +{\n> +\tV4L2PixelFormat fourcc{ v4l2Format.pixelformat };\n> +\treturn V4L2Device::toColorSpace(v4l2Format, PixelFormatInfo::info(fourcc).colourEncoding);\n> +}\n\nHave you considered making this function a (static and private) member\nof the V4L2VideoDevice class ? That way you wouldn't have to make the\nV4L2Device toColorSpace() function public. I'd then also call it\ntoColorSpace() instead of getColorSpace(). I think the resulting patch\nwould be simpler.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +} /* namespace */\n> +\n> int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> {\n> \tstruct v4l2_format v4l2Format = {};\n> @@ -931,8 +942,7 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> \tformat->size.height = pix->height;\n> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n> \tformat->planesCount = pix->num_planes;\n> -\tformat->colorSpace =\n> -\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> +\tformat->colorSpace = getColorSpace(*pix);\n> \n> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> @@ -988,8 +998,7 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> \t\tformat->planes[i].size = pix->plane_fmt[i].sizeimage;\n> \t}\n> -\tformat->colorSpace =\n> -\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> +\tformat->colorSpace = getColorSpace(*pix);\n> \n> \treturn 0;\n> }\n> @@ -1013,8 +1022,7 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> \tformat->planesCount = 1;\n> \tformat->planes[0].bpl = pix->bytesperline;\n> \tformat->planes[0].size = pix->sizeimage;\n> -\tformat->colorSpace =\n> -\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> +\tformat->colorSpace = getColorSpace(*pix);\n> \n> \treturn 0;\n> }\n> @@ -1056,8 +1064,7 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> \tformat->planesCount = 1;\n> \tformat->planes[0].bpl = pix->bytesperline;\n> \tformat->planes[0].size = pix->sizeimage;\n> -\tformat->colorSpace =\n> -\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> +\tformat->colorSpace = getColorSpace(*pix);\n> \n> \treturn 0;\n> }","headers":{"Return-Path":"","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 5BA9AC3272\n\tfor ;\n\tMon, 29 Aug 2022 22:09:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8CFE61FC0;\n\tTue, 30 Aug 2022 00:09:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 486E861FBC\n\tfor ;\n\tTue, 30 Aug 2022 00:09:33 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0135481;\n\tTue, 30 Aug 2022 00:09:32 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661810974;\n\tbh=i+w0fulv87blWJd6Cx+OCk/dPXQ3S+e6JKa9N1YcDgg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=gppfR+HCvVm0OTv2jOQLHFQj0iSm3DPd1MkA/0ZqikyA4hk6byBkohSAODw9Sfocq\n\tssbIogfb1ltbpNrEs7EVW97vDd971WwThY/2VouYyEyTMYHRYDt+vKZNIfQA/7DISz\n\t1ClRQQthHR8iAHcGQz8av7kvFUV65UcGt2TxMQBAwiQ3fayE2GMm/WdWYIXaQpru1A\n\tg+gssSeUqqcDP9e962QOyU3UXqLLii7DsiS0P5Y36+iK2P5+C2u/5miUDQka+5Nvwr\n\tXJtolRyE/VwxPV1nofUC/Y4MdrVtCKZCpvD2mY1iFjpfUuWJVr3woxDsf6VjE1+kO8\n\tbgWI1nIS1zW8A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661810972;\n\tbh=i+w0fulv87blWJd6Cx+OCk/dPXQ3S+e6JKa9N1YcDgg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dHFmv3ZqmjGu5OgNHPL7pyJfFb+yU1ofkbf/A4dy+d9UtuvBtC8Tl3GF8EuZbEMIc\n\tXHxQphUWGhCdXLY9TDhp0kL4j9e4pGpjPZ0Nft+ScqZSJEboFcdH+zDVDyTar1wxCY\n\t/2Nd9MHPP5CmJmncLQsX597Aw1V/UXy3ZRt5//B8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dHFmv3Zq\"; dkim-atps=neutral","Date":"Tue, 30 Aug 2022 01:09:23 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220829163742.1006102-1-umang.jain@ideasonboard.com>\n\t<20220829163742.1006102-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220829163742.1006102-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/7] libcamera: v4l2_videodevice:\n\tImprove toColorSpace() readability","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]