[{"id":10870,"web_url":"https://patchwork.libcamera.org/comment/10870/","msgid":"<20200626012541.GR5865@pendragon.ideasonboard.com>","date":"2020-06-26T01:25:41","subject":"Re: [libcamera-devel] [PATCH v2 1/6] libcamera: formats: Make\n\tImageFormats a templated class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Jun 09, 2020 at 01:28:39AM +0200, Jacopo Mondi wrote:\n> The ImageFormats class was originally designed to be used by both\n> V4L2VideoDevices and V4L2Subdevices. As video devices enumerates their\n> image formats using V4L2PixelFormat instances, the ImageFormats class\n> cannot be used there anymore and it has been replaced by raw maps.\n> \n> Prepare to re-introduce usage of ImageFormats in the V4L2VideoDevice\n> class and its users by making ImageFormats a templated class that\n> indexes the image sizes using on keys of variadic type.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/camera_sensor.h | 2 +-\n> include/libcamera/internal/formats.h | 11 ++++-----\n> include/libcamera/internal/v4l2_subdevice.h | 2 +-\n> src/libcamera/formats.cpp | 25 +++++++++++++--------\n> src/libcamera/v4l2_subdevice.cpp | 6 ++---\n> test/v4l2_subdevice/list_formats.cpp | 2 +-\n> 6 files changed, 28 insertions(+), 20 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 7f07413f9560..d5814a26a121 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -75,7 +75,7 @@ private:\n> \n> \tstd::string model_;\n> \n> -\tImageFormats formats_;\n> +\tImageFormats formats_;\n> \tSize resolution_;\n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> index 4b172efc6588..5668f3744c5d 100644\n> --- a/include/libcamera/internal/formats.h\n> +++ b/include/libcamera/internal/formats.h\n> @@ -18,18 +18,19 @@\n> \n> namespace libcamera {\n> \n> +template\n> class ImageFormats\n> {\n> public:\n> -\tint addFormat(unsigned int format, const std::vector &sizes);\n> +\tint addFormat(T format, const std::vector &sizes);\n> \n> \tbool isEmpty() const;\n> -\tstd::vector formats() const;\n> -\tconst std::vector &sizes(unsigned int format) const;\n> -\tconst std::map> &data() const;\n> +\tstd::vector formats() const;\n> +\tconst std::vector &sizes(T format) const;\n> +\tconst std::map> &data() const;\n> \n> private:\n> -\tstd::map> data_;\n> +\tstd::map> data_;\n> };\n> \n> class PixelFormatInfo\n> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> index a3ecf123f640..f811d316dada 100644\n> --- a/include/libcamera/internal/v4l2_subdevice.h\n> +++ b/include/libcamera/internal/v4l2_subdevice.h\n> @@ -51,7 +51,7 @@ public:\n> \tint setSelection(unsigned int pad, unsigned int target,\n> \t\t\t Rectangle *rect);\n> \n> -\tImageFormats formats(unsigned int pad);\n> +\tImageFormats formats(unsigned int pad);\n> \n> \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> \t\t Whence whence = ActiveFormat);\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 2ac3b412ecdb..62fd46686d7d 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -28,9 +28,8 @@ LOG_DEFINE_CATEGORY(Formats)\n> * corresponding set of image sizes. It is used to describe the formats and\n> * sizes supported by a V4L2Device or V4L2Subdevice.\n> *\n> - * Formats are stored as an integer. When used for a V4L2Device, the image\n> - * formats are fourcc pixel formats. When used for a V4L2Subdevice they are\n> - * media bus codes. Both are defined by the V4L2 specification.\n> + * When used for a V4L2Device, the image formats are V4L2PixelFormat instances.\n> + * When used for a V4L2Subdevice formats are uint32_t media bus codes.\n> *\n> * Sizes are stored as a list of SizeRange.\n> */\n> @@ -43,7 +42,8 @@ LOG_DEFINE_CATEGORY(Formats)\n> * \\return 0 on success or a negative error code otherwise\n> * \\retval -EEXIST The format is already described\n> */\n> -int ImageFormats::addFormat(unsigned int format, const std::vector &sizes)\n> +template\n> +int ImageFormats::addFormat(T format, const std::vector &sizes)\n> {\n> \tif (data_.find(format) != data_.end())\n> \t\treturn -EEXIST;\n> @@ -57,7 +57,8 @@ int ImageFormats::addFormat(unsigned int format, const std::vector &s\n> * \\brief Check if the list of devices supported formats is empty\n> * \\return True if the list of supported formats is empty\n> */\n> -bool ImageFormats::isEmpty() const\n> +template\n> +bool ImageFormats::isEmpty() const\n> {\n> \treturn data_.empty();\n> }\n> @@ -66,9 +67,10 @@ bool ImageFormats::isEmpty() const\n> * \\brief Retrieve a list of all supported image formats\n> * \\return List of pixel formats or media bus codes\n> */\n> -std::vector ImageFormats::formats() const\n> +template\n> +std::vector ImageFormats::formats() const\n> {\n> -\tstd::vector formats;\n> +\tstd::vector formats;\n> \tformats.reserve(data_.size());\n> \n> \t/* \\todo: Should this be cached instead of computed each time? */\n> @@ -88,7 +90,8 @@ std::vector ImageFormats::formats() const\n> * \\return The list of image sizes supported for \\a format, or an empty list if\n> * the format is not supported\n> */\n> -const std::vector &ImageFormats::sizes(unsigned int format) const\n> +template\n> +const std::vector &ImageFormats::sizes(T format) const\n> {\n> \tstatic const std::vector empty;\n> \n> @@ -103,11 +106,15 @@ const std::vector &ImageFormats::sizes(unsigned int format) const\n> * \\brief Retrieve the map that associates formats to image sizes\n> * \\return The map that associates formats to image sizes\n> */\n> -const std::map> &ImageFormats::data() const\n> +template\n> +const std::map> &ImageFormats::data() const\n> {\n> \treturn data_;\n> }\n> \n> +template class ImageFormats;\n> +template class ImageFormats;\n\nShould we include libcamera/internal/v4l2_pixelformat.h at the top ?\nIt's pulled in by formats.h, but that's for PixelFormatInfo, and it\ncould change in the future.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> /**\n> * \\class PixelFormatInfo\n> * \\brief Information about pixel formats\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 7aefc1be032d..9fa20e84a904 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -320,16 +320,16 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> *\n> * \\return A list of the supported device formats\n> */\n> -ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> +ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> {\n> -\tImageFormats formats;\n> +\tImageFormats formats;\n> \n> \tif (pad >= entity_->pads().size()) {\n> \t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> \t\treturn {};\n> \t}\n> \n> -\tfor (unsigned int code : enumPadCodes(pad)) {\n> +\tfor (uint32_t code : enumPadCodes(pad)) {\n> \t\tstd::vector sizes = enumPadSizes(pad, code);\n> \t\tif (sizes.empty())\n> \t\t\treturn {};\n> diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp\n> index a55af1100d9a..3e8d4cdba045 100644\n> --- a/test/v4l2_subdevice/list_formats.cpp\n> +++ b/test/v4l2_subdevice/list_formats.cpp\n> @@ -47,7 +47,7 @@ void ListFormatsTest::printFormats(unsigned int pad,\n> int ListFormatsTest::run()\n> {\n> \t/* List all formats available on existing \"Scaler\" pads. */\n> -\tImageFormats formats;\n> +\tImageFormats formats;\n> \n> \tformats = scaler_->formats(0);\n> \tif (formats.isEmpty()) {","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 8DAFFC2E65\n\tfor ;\n\tFri, 26 Jun 2020 01:25:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CA22609C5;\n\tFri, 26 Jun 2020 03:25:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EEF77603BA\n\tfor ;\n\tFri, 26 Jun 2020 03:25:44 +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 11AF3595;\n\tFri, 26 Jun 2020 03:25:43 +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=\"BcoOw0tu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593134744;\n\tbh=L8CsVO5Bz/aonjvuqORghdZHo+SUXaIQGaXRAUV9AA4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BcoOw0tu5Uy1OQawnZ7tCxtWPGUcnGiz2UYS2yNaR9Toj/uiiUv2E3M3kn6AoZ0jr\n\trdobVuqs3ulYHbb07EiyVud/WIXFkB+u9tnKjpMQwl7C+MzPSIe/6lXymuDBbdWLyb\n\tvT+eXPtAjmVbcK+9kj3C3fRlmmXUs0UjwytVDRck=","Date":"Fri, 26 Jun 2020 04:25:41 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200626012541.GR5865@pendragon.ideasonboard.com>","References":"<20200608232844.10150-1-jacopo@jmondi.org>\n\t<20200608232844.10150-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200608232844.10150-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/6] libcamera: formats: Make\n\tImageFormats a templated class","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","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\" "}}]