[{"id":1913,"web_url":"https://patchwork.libcamera.org/comment/1913/","msgid":"<20190618224831.GK23556@pendragon.ideasonboard.com>","date":"2019-06-18T22:48:31","subject":"Re: [libcamera-devel] [PATCH v3 05/16] libcamera: formats: Add\n\tImageFormats","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:51PM +0200, Niklas Söderlund wrote:\n> Add a new class to hold format information for v4l2 devices and\n\ns/v4l2/V4L2/ (and possibly \"V4L2 video devices and subdevices\" depending\non which series gets merged first)\n\n> subdevices. The object describes the relationship between either pixel\n> formats (v4l2 devices) or media bus codes (v4l2 subdevice) and a list of\n\nV4L2 there too.\n\n> image sizes which can be produced with that format.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/formats.cpp | 80 +++++++++++++++++++++++++++++++++\n> src/libcamera/include/formats.h | 14 ++++++\n> 2 files changed, 94 insertions(+)\n> \n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 56f4ddb51ffad4d3..2fd0c5480324ce33 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -24,4 +24,84 @@ namespace libcamera {\n> * resolutions represented by SizeRange items.\n> */\n> \n> +/**\n> + * \\class ImageFormats\n> + * \\brief Describe V4L2Device and V4L2SubDevice image formats\n> + *\n> + * The class describes information about image formats and supported sizes. If\n> + * the ImageFormat describe a V4L2Device the image formats are described with a\n> + * fourcc pixel format code, while if it describes a V4L2SubDevice the formats\n> + * are described with media bus codes, both defined by the V4L2 specification.\n\nI would propose\n\n\"This class stores a list of image formats, each associated with a\ncorresponding set of image sizes. It is used to describe the formats and\nsizes supported by a V4L2Device or V4L2Subdevice.\n\nFormats are stored as an integer. When used for a V4L2Device, the image\nformats are fourcc pixel formats. When used for a V4L2Subdevice they are\nmedia bus codes. Both are defined by the V4L2 specification.\n\nSizes are stored as a list of SizeRange.\"\n\n> + */\n> +\n> +/**\n> + * \\brief Add a format and sizes to the description\n\ns/and sizes/and corresponding sizes/\n\n> + * \\param[in] format Pixel format or media bus code to describe\n> + * \\param[in] sizes List of supported sizes for the format\n\ns/sizes/size ranges/ ?\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EBUSY The format is already described\n\nI wonder if we really need this, or if we should ignore the error\nsilently. If you think it should be kept, I would use -EEXIST.\n\n> + */\n> +int ImageFormats::addFormat(unsigned int format, const std::vector &sizes)\n> +{\n> +\tif (data_.find(format) != data_.end())\n> +\t\treturn -EBUSY;\n> +\n> +\tdata_[format] = sizes;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\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> +{\n> +\treturn data_.empty();\n> +}\n> +\n> +/**\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> +{\n> +\tstd::vector formats;\n> +\tformats.reserve(data_.size());\n> +\n> +\tfor (auto const &it : data_)\n> +\t\tformats.push_back(it.first);\n> +\n> +\treturn formats;\n\nDo you think it would make sense to cache it internally and return a\nconst reference, or is it not worth it given the existing and foreseen\nuse cases ? If you think it could be useful you don't have to fix it\nnow, but a \\todo would be useful.\n\n> +}\n> +\n> +/**\n> + * \\brief Retrieve all sizes for a specific format\n> + * \\param[in] format A pixelformat or mbus code\n\ns/A/The/\n\n> + *\n> + * Retrieve all SizeRanges for a specific format. For V4L2Device \\a format is a\n\ns/SizeRanges/size ranges/ or SizeRange instances (I would go for the\nformat). The return value type in doxygen will offer a clickable link to\nSizeRange, so there's no strict need to mention SizeRange in the text\nhere.\n\n> + * pixel format while for a V4L2Subdevice \\a format is a media bus code.\n> + *\n> + * \\return he list of image sizes produced using \\a format, or an empty list if\n\ns/he/The/\ns/produced using/supported for/ ?\n\n> + * format is not supported\n\ns/format/the format/\n\n> + */\n> +std::vector ImageFormats::sizes(unsigned int format) const\n> +{\n> +\tauto const &it = data_.find(format);\n> +\tif (it == data_.end())\n> +\t\treturn {};\n> +\n> +\treturn it->second;\n\nIn this case I think a copy is quite overkill. You could declare\n\n\tstatic std::vector empty;\n\nreplace return {} with return empty, and change the function type to\nreturn a const reference.\n\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the map that associates formats to image sizes\n> + * \\return Map that associates formats to image sizes\n\ns/Map/The map/\n\nWith these small issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> + */\n> +const std::map> &ImageFormats::data() const\n> +{\n> +\treturn data_;\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h\n> index a73772b1eda068b4..a49f83f3d8d60621 100644\n> --- a/src/libcamera/include/formats.h\n> +++ b/src/libcamera/include/formats.h\n> @@ -17,6 +17,20 @@ namespace libcamera {\n> \n> typedef std::map> FormatEnum;\n> \n> +class ImageFormats\n> +{\n> +public:\n> +\tint addFormat(unsigned int format, const std::vector &sizes);\n> +\n> +\tbool isEmpty() const;\n> +\tstd::vector formats() const;\n> +\tstd::vector sizes(unsigned int format) const;\n> +\tconst std::map> &data() const;\n> +\n> +private:\n> +\tstd::map> data_;\n> +};\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_FORMATS_H__ */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E39AE60BF5\n\tfor ;\n\tWed, 19 Jun 2019 00:48:48 +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 6BC39D5;\n\tWed, 19 Jun 2019 00:48:48 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560898128;\n\tbh=fRY4ly3bxaEwq/1KSGm5fZ+B9Jeva3rXzfNdV4b6Xsg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z1Y3TZKzYRP28kggUgruvn6bqPs8jIuEm5RZEXs/lAkSoZkeNwo1PSahxbbT1wwSl\n\tZFlHioOpcq59IniDQyYJEjPZTf5JUEkE6uQAiByyez6wK3jYCqkL/g+tECQnE2VtDf\n\tjxA8WM/8awk7/OSauvz7dcEvh/6UvIzGzeOJUCko=","Date":"Wed, 19 Jun 2019 01:48:31 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190618224831.GK23556@pendragon.ideasonboard.com>","References":"<20190616133402.21934-1-niklas.soderlund@ragnatech.se>\n\t<20190616133402.21934-6-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-6-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 05/16] libcamera: formats: Add\n\tImageFormats","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 22:48:49 -0000"}}]