[{"id":1916,"web_url":"https://patchwork.libcamera.org/comment/1916/","msgid":"<20190618225942.GN23556@pendragon.ideasonboard.com>","date":"2019-06-18T22:59:42","subject":"Re: [libcamera-devel] [PATCH v3 08/16] libcamera: v4l2_subdevice:\n\tReplace FormatEnum with ImageFormats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sun, Jun 16, 2019 at 03:33:54PM +0200, Niklas Söderlund wrote:\n> Replace all usage of FormatEnum with ImageFormats and completely\n> remove FormatEnum which is no longer needed.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/camera_sensor.cpp | 14 ++++++--------\n> src/libcamera/formats.cpp | 10 ----------\n> src/libcamera/include/formats.h | 2 --\n> src/libcamera/include/v4l2_subdevice.h | 2 +-\n> src/libcamera/v4l2_subdevice.cpp | 26 +++++++++++++-------------\n> test/v4l2_subdevice/list_formats.cpp | 16 ++++++++--------\n> 6 files changed, 28 insertions(+), 42 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index cb6649efac3ff794..a804a68c9d91cda6 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -90,27 +90,25 @@ int CameraSensor::init()\n> \t\treturn ret;\n> \n> \t/* Enumerate and cache media bus codes and sizes. */\n> -\tconst FormatEnum formats = subdev_->formats(0);\n> -\tif (formats.empty()) {\n> +\tconst ImageFormats formats = subdev_->formats(0);\n> +\tif (formats.isEmpty()) {\n> \t\tLOG(CameraSensor, Error) << \"No image format found\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tstd::transform(formats.begin(), formats.end(),\n> -\t\t std::back_inserter(mbusCodes_),\n> -\t\t [](decltype(*formats.begin()) f) { return f.first; });\n> +\tmbusCodes_ = formats.formats();\n> \n> \t/*\n> \t * Extract the supported sizes from the first format as we only support\n> \t * sensors that offer the same frame sizes for all media bus codes.\n> \t * Verify this assumption and reject the sensor if it isn't true.\n> \t */\n> -\tconst std::vector &sizes = formats.begin()->second;\n> +\tconst std::vector &sizes = formats.sizes(mbusCodes_[0]);\n> \tstd::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),\n> \t\t [](const SizeRange &range) { return range.max; });\n> \n> -\tfor (auto it = ++formats.begin(); it != formats.end(); ++it) {\n> -\t\tif (it->second != sizes) {\n> +\tfor (unsigned int code : mbusCodes_) {\n> +\t\tif (formats.sizes(code) != sizes) {\n> \t\t\tLOG(CameraSensor, Error)\n> \t\t\t\t<< \"Frame sizes differ between media bus codes\";\n> \t\t\treturn -EINVAL;\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 2fd0c5480324ce33..0d196ee3a83e9c0d 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -14,16 +14,6 @@\n> \n> namespace libcamera {\n> \n> -/**\n> - * \\typedef FormatEnum\n> - * \\brief Type definition for the map of image formats and sizes\n> - *\n> - * Type definition used to enumerate the supported pixel formats and image\n> - * frame sizes. The type associates in a map a pixel format (for memory\n> - * formats) or a media bus code (for bus formats), to a vector of image\n> - * resolutions represented by SizeRange items.\n> - */\n> -\n> /**\n> * \\class ImageFormats\n> * \\brief Describe V4L2Device and V4L2SubDevice image formats\n> diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h\n> index a49f83f3d8d60621..de63ba557e045585 100644\n> --- a/src/libcamera/include/formats.h\n> +++ b/src/libcamera/include/formats.h\n> @@ -15,8 +15,6 @@\n> \n> namespace libcamera {\n> \n> -typedef std::map> FormatEnum;\n> -\n> class ImageFormats\n> {\n> public:\n> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> index c6fdf417b43c0423..9afd28b6db023ddf 100644\n> --- a/src/libcamera/include/v4l2_subdevice.h\n> +++ b/src/libcamera/include/v4l2_subdevice.h\n> @@ -45,7 +45,7 @@ public:\n> \tint setCrop(unsigned int pad, Rectangle *rect);\n> \tint setCompose(unsigned int pad, Rectangle *rect);\n> \n> -\tFormatEnum formats(unsigned int pad);\n> +\tImageFormats formats(unsigned int pad);\n> \n> \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 35052b4aa45d3e42..e109a00bcd3bdd46 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -187,22 +187,17 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)\n> }\n> \n> /**\n> - * \\brief List the sub-device image resolutions and formats on \\a pad\n> + * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> *\n> - * Retrieve a list of image formats and sizes on the \\a pad of a video\n> - * subdevice. Subdevices can report either a list of discrete sizes they\n> - * support or a list of intervals expressed as a [min-max] sizes range.\n> + * Enumerate all media bus codes and frame sizes supported by the subdevice on\n\ns/ / /\n\n> + * a \\a pad.\n> *\n> - * Each image size list is associated with a media bus pixel code for which\n> - * the reported resolutions are supported.\n> - *\n> - * \\return A map of image formats associated with a list of image sizes, or\n> - * an empty map on error or if the pad does not exist\n> + * \\return A list of the supported device formats\n> */\n> -FormatEnum V4L2Subdevice::formats(unsigned int pad)\n> +ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> {\n> -\tFormatEnum formatMap = {};\n> +\tImageFormats formats;\n> \n> \tif (pad >= entity_->pads().size()) {\n> \t\tLOG(V4L2Subdev, Error) << \"Invalid pad: \" << pad;\n> @@ -215,10 +210,15 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)\n> \t\tif (sizes.empty())\n> \t\t\treturn {};\n> \n> -\t\tformatMap[code] = sizes;\n> +\t\tif (formats.addFormat(code, sizes)) {\n> +\t\t\tLOG(V4L2Subdev, Error)\n> +\t\t\t\t<< \"Could not add sizes for media bus code \"\n> +\t\t\t\t<< code << \"on pad \" << pad;\n\ns/\"on pad \"/\" on pad \"/\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t\treturn {};\n> +\t\t}\n> \t}\n> \n> -\treturn formatMap;\n> +\treturn formats;\n> }\n> \n> /**\n> diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp\n> index 3f0edafcdcd72d6b..067dc5ed30f4edd9 100644\n> --- a/test/v4l2_subdevice/list_formats.cpp\n> +++ b/test/v4l2_subdevice/list_formats.cpp\n> @@ -47,29 +47,29 @@ void ListFormatsTest::printFormats(unsigned int pad,\n> int ListFormatsTest::run()\n> {\n> \t/* List all formats available on existing \"Scaler\" pads. */\n> -\tstd::map> formats;\n> +\tImageFormats formats;\n> \n> \tformats = scaler_->formats(0);\n> -\tif (formats.empty()) {\n> +\tif (formats.isEmpty()) {\n> \t\tcerr << \"Failed to list formats on pad 0 of subdevice \"\n> \t\t << scaler_->entity()->name() << endl;\n> \t\treturn TestFail;\n> \t}\n> -\tfor (auto it = formats.begin(); it != formats.end(); ++it)\n> -\t\tprintFormats(0, it->first, it->second);\n> +\tfor (unsigned int code : formats.formats())\n> +\t\tprintFormats(0, code, formats.sizes(code));\n> \n> \tformats = scaler_->formats(1);\n> -\tif (formats.empty()) {\n> +\tif (formats.isEmpty()) {\n> \t\tcerr << \"Failed to list formats on pad 1 of subdevice \"\n> \t\t << scaler_->entity()->name() << endl;\n> \t\treturn TestFail;\n> \t}\n> -\tfor (auto it = formats.begin(); it != formats.end(); ++it)\n> -\t\tprintFormats(1, it->first, it->second);\n> +\tfor (unsigned int code : formats.formats())\n> +\t\tprintFormats(1, code, formats.sizes(code));\n> \n> \t/* List format on a non-existing pad, format vector shall be empty. */\n> \tformats = scaler_->formats(2);\n> -\tif (!formats.empty()) {\n> +\tif (!formats.isEmpty()) {\n> \t\tcerr << \"Listing formats on non-existing pad 2 of subdevice \"\n> \t\t << scaler_->entity()->name()\n> \t\t << \" should return an empty format list\" << endl;","headers":{"Return-Path":"","Received":["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 D9C7160BF5\n\tfor ;\n\tWed, 19 Jun 2019 00:59:59 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5B71CD5;\n\tWed, 19 Jun 2019 00:59:59 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560898799;\n\tbh=6wB6fWTI5hRQThW+gAK/sMSi7P5UD1yK0wJqc7y0pFE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IVxU2tOXKhAKQiCo6zw8c5PXC5QbmrJ2toSKehaVvdhFyJlyovOmfB7CvOZl6CpMc\n\tARlnKU1GTUsJ3wM5VweBQP8BDMCAIEDhZwV7M99viuC9YrqQzHWxyROonaeMr5shCE\n\ty9WxUr4MmBsjS3U1NcgnmawlQrMyRhC+2GyGfFmQ=","Date":"Wed, 19 Jun 2019 01:59:42 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190618225942.GN23556@pendragon.ideasonboard.com>","References":"<20190616133402.21934-1-niklas.soderlund@ragnatech.se>\n\t<20190616133402.21934-9-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190616133402.21934-9-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 08/16] libcamera: v4l2_subdevice:\n\tReplace FormatEnum with ImageFormats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 18 Jun 2019 23:00:00 -0000"}}]