[{"id":1728,"web_url":"https://patchwork.libcamera.org/comment/1728/","msgid":"<b40438e1-51a7-272c-8240-edf13bf94504@ideasonboard.com>","date":"2019-05-29T22:22:51","subject":"Re: [libcamera-devel] [PATCH 11/17] libcamera: stream: Add\n\tStreamFormats","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 27/05/2019 01:15, Niklas Söderlund wrote:\n> Add a StreamFormats which describes all the formats a stream can\n> support. The object does not collect any formation itself but can\n> simplify users interaction with formats as it's able to translate a\n> stream format range into discrete list and a discrete list to a range.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  include/libcamera/stream.h |  16 +++\n>  src/libcamera/stream.cpp   | 216 +++++++++++++++++++++++++++++++++++++\n>  2 files changed, 232 insertions(+)\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index e38c0e7e827d5888..48daf5ac23f55d85 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -7,6 +7,7 @@\n>  #ifndef __LIBCAMERA_STREAM_H__\n>  #define __LIBCAMERA_STREAM_H__\n>  \n> +#include <map>\n>  #include <string>\n>  #include <vector>\n>  \n> @@ -18,6 +19,21 @@ namespace libcamera {\n>  class Camera;\n>  class Stream;\n>  \n> +class StreamFormats\n> +{\n> +public:\n> +\tStreamFormats();\n> +\tStreamFormats(const std::map<unsigned int, std::vector<SizeRange>> &formats);\n> +\n> +\tstd::vector<unsigned int> pixelformats() const;\n> +\tstd::vector<Size> sizes(unsigned int pixelformat) const;\n> +\n> +\tSizeRange range(unsigned int pixelformat) const;\n> +\n> +private:\n> +\tstd::map<unsigned int, std::vector<SizeRange>> formats_;\n> +};\n> +\n>  struct StreamConfiguration {\n>  \tStreamConfiguration()\n>  \t\t: stream_(nullptr)\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index eecd37160150d55c..a2931902fda2baa5 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -7,6 +7,7 @@\n>  \n>  #include <libcamera/stream.h>\n>  \n> +#include <algorithm>\n>  #include <iomanip>\n>  #include <sstream>\n>  \n> @@ -36,6 +37,221 @@ namespace libcamera {\n>  \n>  LOG_DEFINE_CATEGORY(Stream)\n>  \n> +/**\n> + * \\class StreamFormats\n> + * \\brief Hold information about supported stream formats\n> + *\n> + * The StreamFormats class holds information about pixel formats and frame\n\n/about pixel formats/about the pixel formats/\n\n> + * sizes a stream supports. The class groups size information by the pixel\n> + * format which can produce it. There are two ways to examine the size\n> + * information, as a range or as a list of discrete sizes.\n> + *\n> + * When sizes are viewed as a range it describes the minimum and maximum width\n> + * and height values. There is a possibility to supplement the range description\n> + * with a horizontal och vertical stepping size. The stepping size describes the\n\n/och/and/ ?\n\n\n> + * step size in pixel from the minimum with/height.\n\n/in pixel/in pixels/\n\n> + *\n> + * When sizes is viewed as a list of discrete sizes it describes exact dimensions\n\nWhen sizes are viewed...  ... they describe exact ...\n\n> + * which can be selected and used.\n> + *\n> + * Pipeline handlers can create StreamFormats describing each pixel format using\n> + * either a range or a list of  discrete sizes. The StreamFormats class attempts\n\n/of  discrete/of discrete/ <remove extra space>\n\n> + * to translates between the two different ways to view them. The translations\n\n/translates/translate/\n\n> + * are performed as:\n> + *\n> + *  - If a pixel format is described as a list of discrete sizes a range is\n\n... of discrete sizes then a range ....\n\n> + *    created by taking the minim and maximum width/height in the list.\n\n/minim/minimum/\n\n> + *    The stepping information is not recreated and set to 1.\n\n... and is set to 1.\n\n\n> + *\n> + *  - If a pixel format is described as a range a list of discrete sizes which\n\n... as a range then a list of ...\n\n> + *    fits inside that range are selected from a list of common sizes. The\n> + *    stepping information is taken into consideration when generating the\n> + *    sizes.\n> + *\n> + * Applications examining sizes as a range with stepping values of 1 should be\n> + * aware that the range could be generated form a list of discrete sizes and\n\n/form/from/\n\n\n> + * there could be big gaps in the range to what the stream can support.\n\n... there could be a large quantity of possible Size combinations which\nmay not be supported by the Stream.\n\n\nCould we set the stepping size to '-1' or '0' or something here to\nidentify that the range is really only specifying the maximum, and\nminimum width/height ?\n\n> + *\n> + * All sizes retrieved from StreamFormats should be treated as advisory and no\n> + * size should be considered to be supported until its been verified using\n> + * CameraConfiguration::validate().\n> + */\n> +\n> +/**\n> + * \\brief Construct a empty StreamFormats object\n> + */\n> +StreamFormats::StreamFormats()\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a StreamFormats object\n> + * \\param[in] formats A map of pixel formats to a sizes description\n> + */\n> +StreamFormats::StreamFormats(const std::map<unsigned int, std::vector<SizeRange>> &formats)\n> +\t: formats_(formats)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of pixel formats supported\n> + * \\returns List of pixel formats\n> + */\n> +std::vector<unsigned int> StreamFormats::pixelformats() const\n> +{\n> +\tstd::vector<unsigned int> formats;\n> +\n> +\tfor (auto const &it : formats_)\n> +\t\tformats.push_back(it.first);\n> +\n> +\treturn formats;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of frame sizes\n> + * \\param[in] pixelformat Pixel format to retrieve sizes for\n> + * \\returns List description of frame sizes\n> + */\n> +std::vector<Size> StreamFormats::sizes(unsigned int pixelformat) const\n> +{\n> +\t/*\n> +\t * Sizes to try and extract from ranges.\n> +\t * \\todo Verify list of resolutions are good, current list compiled\n> +\t * from v4l2 documentation and source code as well as lists of\n> +\t * common frame sizes.\n> +\t */\n> +\tstatic const std::vector<Size> rangeDiscreteSizes = {\n> +\t\tSize(160, 120),\n> +\t\tSize(240, 160),\n> +\t\tSize(320, 240),\n> +\t\tSize(400, 240),\n> +\t\tSize(480, 320),\n> +\t\tSize(640, 360),\n> +\t\tSize(640, 480),\n> +\t\tSize(720, 480),\n> +\t\tSize(720, 576),\n> +\t\tSize(768, 480),\n> +\t\tSize(800, 600),\n> +\t\tSize(854, 480),\n> +\t\tSize(960, 540),\n> +\t\tSize(960, 640),\n> +\t\tSize(1024, 576),\n> +\t\tSize(1024, 600),\n> +\t\tSize(1024, 768),\n> +\t\tSize(1152, 864),\n> +\t\tSize(1280, 1024),\n> +\t\tSize(1280, 1080),\n> +\t\tSize(1280, 720),\n> +\t\tSize(1280, 800),\n> +\t\tSize(1360, 768),\n> +\t\tSize(1366, 768),\n> +\t\tSize(1400, 1050),\n> +\t\tSize(1440, 900),\n> +\t\tSize(1536, 864),\n> +\t\tSize(1600, 1200),\n> +\t\tSize(1600, 900),\n> +\t\tSize(1680, 1050),\n> +\t\tSize(1920, 1080),\n> +\t\tSize(1920, 1200),\n> +\t\tSize(2048, 1080),\n> +\t\tSize(2048, 1152),\n> +\t\tSize(2048, 1536),\n> +\t\tSize(2160, 1080),\n> +\t\tSize(2560, 1080),\n> +\t\tSize(2560, 1440),\n> +\t\tSize(2560, 1600),\n> +\t\tSize(2560, 2048),\n> +\t\tSize(2960, 1440),\n> +\t\tSize(3200, 1800),\n> +\t\tSize(3200, 2048),\n> +\t\tSize(3200, 2400),\n> +\t\tSize(3440, 1440),\n> +\t\tSize(3840, 1080),\n> +\t\tSize(3840, 1600),\n> +\t\tSize(3840, 2160),\n> +\t\tSize(3840, 2400),\n> +\t\tSize(4096, 2160),\n> +\t\tSize(5120, 2160),\n> +\t\tSize(5120, 2880),\n> +\t\tSize(7680, 4320),\n\nI still wonder if we could generate this data table in a better form.\n\nFor a start, perhaps it might be worth grouping common Sizes to a single\nline to form a table based on their ratios? That would show what ratios\nare included maybe?\n\n(perhaps we could generate each ratio from a single width or height\ninstead?)\n\n/* 4:3                     16:9\t\t\t\t16:10 */\n\nSize(1600, 1200),\tSize(1600, 900),\tSize(1680, 1050),\n\t\t\tSize(1920, 1080),\tSize(1920, 1200),\n\n/* 2:1\t\t\t 2:1 ish again?\t\t\t4:3 */\nSize(2048, 1080),\tSize(2048, 1152),\tSize(2048, 1536),\n\n\nIf you did look at doing this, I'd move the table outside of the\nfunction so that it's indent is reduced.\n\n> +\t};\n> +\tstd::vector<Size> sizes;\n> +\n> +\t/* Make sure pixel format exists. */\n> +\tauto const &it = formats_.find(pixelformat);\n> +\tif (it == formats_.end())\n> +\t\treturn {};\n> +\n> +\t/* Try create list of discrete sizes. */\n> +\tconst std::vector<SizeRange> &ranges = it->second;\n> +\tbool discrete = true;\n> +\tfor (const SizeRange &range : ranges) {\n> +\t\tif (range.min != range.max) {\n> +\t\t\tdiscrete = false;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tsizes.emplace_back(range.min.width, range.min.height);\n> +\t}\n> +\n> +\t/* If discrete not possible generate from range. */\n> +\tif (!discrete) {\n> +\t\tif (ranges.size() != 1) {\n> +\t\t\tLOG(Stream, Error) << \"Range format is ambiguous\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tconst SizeRange &limit = ranges.front();\n> +\t\tsizes.clear();\n> +\n> +\t\tfor (const Size &size : rangeDiscreteSizes) {\n> +\t\t\tif (size.width < limit.min.width ||\n> +\t\t\t    size.width > limit.max.width ||\n> +\t\t\t    size.height < limit.min.height ||\n> +\t\t\t    size.height > limit.max.height ||\n> +\t\t\t    (size.width - limit.min.width) % limit.vStep ||\n> +\t\t\t    (size.height - limit.min.height) % limit.hStep)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tsizes.push_back(size);\n> +\t\t}\n> +\t}\n> +\n> +\tstd::sort(sizes.begin(), sizes.end());\n> +\n> +\treturn sizes;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve a frame size range\n> + * \\param[in] pixelformat Pixel format to retrieve range for\n> + * \\returns Range description of frame size\n> + */\n> +SizeRange StreamFormats::range(unsigned int pixelformat) const\n> +{\n> +\tauto const it = formats_.find(pixelformat);\n> +\tif (it == formats_.end())\n> +\t\treturn {};\n> +\n> +\tconst SizeRange &first = it->second.front();\n> +\tif (it->second.size() == 1)\n> +\t\treturn first;\n\nThere's a lot of mixing of 'first' and 'second' here which is a bit\nawkward for comprehension....\n\n> +\n> +\tLOG(Stream, Debug) << \"Building range from discret\";\n\n/discret/discrete/\n\n> +\n> +\tSizeRange range(first.min.width, first.min.height,\n> +\t\t\tfirst.max.width, first.max.height);\n> +\n> +\tfor (const SizeRange &limit : it->second) {\n> +\t\tif (limit.min < range.min)\n> +\t\t\trange.min = limit.min;\n> +\n> +\t\tif (limit.max > range.max)\n> +\t\t\trange.max = limit.max;\n> +\t}\n> +\n> +\treturn range;\n> +}\n> +\n>  /**\n>   * \\struct StreamConfiguration\n>   * \\brief Configuration parameters for a stream\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","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 F18B760E46\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 30 May 2019 00:22:58 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38F85524;\n\tThu, 30 May 2019 00:22:58 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559168578;\n\tbh=OCUvWAaSywMtziCGIWtutGf6njHHs1RHHzIURQzcEFQ=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=fm821N7QAghJZjN6QnQxWvSvLAHZDu6L8nKzIIe6bgNqHHPEKUzj30b5mshVQ+MAd\n\tULMmMtGOoBQdI5tweydW1zyTiAkh5roaiZELoxVLz6zLCsXoGu5b25WDrKbtmknKSE\n\tHvr2cZKBn4Nm4YhafAnvexIkGkFNHbbOgF19MJSU=","Reply-To":"kieran.bingham@ideasonboard.com","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20190527001543.13593-1-niklas.soderlund@ragnatech.se>\n\t<20190527001543.13593-12-niklas.soderlund@ragnatech.se>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<b40438e1-51a7-272c-8240-edf13bf94504@ideasonboard.com>","Date":"Wed, 29 May 2019 23:22:51 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.6.1","MIME-Version":"1.0","In-Reply-To":"<20190527001543.13593-12-niklas.soderlund@ragnatech.se>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 11/17] libcamera: stream: Add\n\tStreamFormats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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>","X-List-Received-Date":"Wed, 29 May 2019 22:22:59 -0000"}},{"id":1807,"web_url":"https://patchwork.libcamera.org/comment/1807/","msgid":"<20190610065006.GA4806@pendragon.ideasonboard.com>","date":"2019-06-10T06:50:06","subject":"Re: [libcamera-devel] [PATCH 11/17] libcamera: stream: Add\n\tStreamFormats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Wed, May 29, 2019 at 11:22:51PM +0100, Kieran Bingham wrote:\n> On 27/05/2019 01:15, Niklas Söderlund wrote:\n> > Add a StreamFormats which describes all the formats a stream can\n\ns/StreamFormats/StreamFormats class/\n\n> > support. The object does not collect any formation itself but can\n> > simplify users interaction with formats as it's able to translate a\n\ns/users interaction/user interactions/\n\n> > stream format range into discrete list and a discrete list to a range.\n> > \n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  include/libcamera/stream.h |  16 +++\n> >  src/libcamera/stream.cpp   | 216 +++++++++++++++++++++++++++++++++++++\n> >  2 files changed, 232 insertions(+)\n> > \n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index e38c0e7e827d5888..48daf5ac23f55d85 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -7,6 +7,7 @@\n> >  #ifndef __LIBCAMERA_STREAM_H__\n> >  #define __LIBCAMERA_STREAM_H__\n> >  \n> > +#include <map>\n> >  #include <string>\n> >  #include <vector>\n> >  \n> > @@ -18,6 +19,21 @@ namespace libcamera {\n> >  class Camera;\n> >  class Stream;\n> >  \n> > +class StreamFormats\n> > +{\n> > +public:\n> > +\tStreamFormats();\n> > +\tStreamFormats(const std::map<unsigned int, std::vector<SizeRange>> &formats);\n> > +\n> > +\tstd::vector<unsigned int> pixelformats() const;\n\nFor efficiency reasons, do you think it would make sense to cache the\npixel formats vector the first time it is computed and return a const\nreference to the cached copy ? Or do you expect applications to\ntypically retrieve the pixel formats once only ?\n\n> > +\tstd::vector<Size> sizes(unsigned int pixelformat) const;\n\nSame question for the sizes and ranges.\n\n> > +\n> > +\tSizeRange range(unsigned int pixelformat) const;\n> > +\n> > +private:\n> > +\tstd::map<unsigned int, std::vector<SizeRange>> formats_;\n> > +};\n> > +\n> >  struct StreamConfiguration {\n> >  \tStreamConfiguration()\n> >  \t\t: stream_(nullptr)\n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > index eecd37160150d55c..a2931902fda2baa5 100644\n> > --- a/src/libcamera/stream.cpp\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -7,6 +7,7 @@\n> >  \n> >  #include <libcamera/stream.h>\n> >  \n> > +#include <algorithm>\n> >  #include <iomanip>\n> >  #include <sstream>\n> >  \n> > @@ -36,6 +37,221 @@ namespace libcamera {\n> >  \n> >  LOG_DEFINE_CATEGORY(Stream)\n> >  \n> > +/**\n> > + * \\class StreamFormats\n> > + * \\brief Hold information about supported stream formats\n> > + *\n> > + * The StreamFormats class holds information about pixel formats and frame\n> \n> /about pixel formats/about the pixel formats/\n> \n> > + * sizes a stream supports. The class groups size information by the pixel\n> > + * format which can produce it. There are two ways to examine the size\n> > + * information, as a range or as a list of discrete sizes.\n> > + *\n\nI would start a new paragraph before the lest sentence, and remove this\nblank line, as the last sentence and the text beflow are related.\n\n> > + * When sizes are viewed as a range it describes the minimum and maximum width\n> > + * and height values. There is a possibility to supplement the range description\n> > + * with a horizontal och vertical stepping size. The stepping size describes the\n> \n> /och/and/ ?\n> \n> \n> > + * step size in pixel from the minimum with/height.\n> \n> /in pixel/in pixels/\n> \n> > + *\n> > + * When sizes is viewed as a list of discrete sizes it describes exact dimensions\n> \n> When sizes are viewed...  ... they describe exact ...\n> \n> > + * which can be selected and used.\n> > + *\n> > + * Pipeline handlers can create StreamFormats describing each pixel format using\n> > + * either a range or a list of  discrete sizes. The StreamFormats class attempts\n> \n> /of  discrete/of discrete/ <remove extra space>\n> \n> > + * to translates between the two different ways to view them. The translations\n> \n> /translates/translate/\n> \n> > + * are performed as:\n> > + *\n> > + *  - If a pixel format is described as a list of discrete sizes a range is\n> \n> ... of discrete sizes then a range ....\n> \n> > + *    created by taking the minim and maximum width/height in the list.\n> \n> /minim/minimum/\n> \n> > + *    The stepping information is not recreated and set to 1.\n> \n> ... and is set to 1.\n> \n> \n> > + *\n> > + *  - If a pixel format is described as a range a list of discrete sizes which\n> \n> ... as a range then a list of ...\n> \n> > + *    fits inside that range are selected from a list of common sizes. The\n> > + *    stepping information is taken into consideration when generating the\n> > + *    sizes.\n> > + *\n> > + * Applications examining sizes as a range with stepping values of 1 should be\n> > + * aware that the range could be generated form a list of discrete sizes and\n> \n> /form/from/\n> \n> > + * there could be big gaps in the range to what the stream can support.\n> \n> ... there could be a large quantity of possible Size combinations which\n> may not be supported by the Stream.\n> \n> \n> Could we set the stepping size to '-1' or '0' or something here to\n> identify that the range is really only specifying the maximum, and\n> minimum width/height ?\n\nI think it would be indeed be useful for an application to determine if\nthe range is continuous, or just exposes the minimum and maximum with\nthe stream only supported discrete resolutions. Setting the step to 0\nwould be one way of doing so. It should then be clearly documented. We\nshould also evaluate the impact on applications, and find a better API\nif it gets too awkward or error-prone to use.\n\n> > + *\n> > + * All sizes retrieved from StreamFormats should be treated as advisory and no\n> > + * size should be considered to be supported until its been verified using\n> > + * CameraConfiguration::validate().\n\ns/should/shall/g\n\nWe should also take care of not returning sizes that are not supported.\nI think the current implementation is good enough for now, if a pipeline\nhandler has requirements more complex than a continuous range with a\nstep, it should likely provide a list of discrete sizes explicitly.\nHowever, we may want to revisit this later, possibly to add a helper for\npipeline handlers that can handle more complex constraints. When we will\ndo so it may make sense to move the logic that constructs the sizes\nbased on the standard discrete sizes to a separate function, outside of\nthe StreamFormats class.\n\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a empty StreamFormats object\n> > + */\n> > +StreamFormats::StreamFormats()\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Construct a StreamFormats object\n> > + * \\param[in] formats A map of pixel formats to a sizes description\n> > + */\n> > +StreamFormats::StreamFormats(const std::map<unsigned int, std::vector<SizeRange>> &formats)\n> > +\t: formats_(formats)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the list of pixel formats supported\n\ns/pixel formats supported/supported pixel formats/\n\n> > + * \\returns List of pixel formats\n\nThe list of supported pixel formats\n\n> > + */\n> > +std::vector<unsigned int> StreamFormats::pixelformats() const\n> > +{\n> > +\tstd::vector<unsigned int> formats;\n> > +\n> > +\tfor (auto const &it : formats_)\n> > +\t\tformats.push_back(it.first);\n> > +\n> > +\treturn formats;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the list of frame sizes\n> > + * \\param[in] pixelformat Pixel format to retrieve sizes for\n> > + * \\returns List description of frame sizes\n> > + */\n\nIt would be useful to extend the documentation to explain that sizes are\ncomputed (and how) if the device supports a continuous range.\n\n> > +std::vector<Size> StreamFormats::sizes(unsigned int pixelformat) const\n> > +{\n> > +\t/*\n> > +\t * Sizes to try and extract from ranges.\n> > +\t * \\todo Verify list of resolutions are good, current list compiled\n> > +\t * from v4l2 documentation and source code as well as lists of\n> > +\t * common frame sizes.\n> > +\t */\n> > +\tstatic const std::vector<Size> rangeDiscreteSizes = {\n> > +\t\tSize(160, 120),\n> > +\t\tSize(240, 160),\n> > +\t\tSize(320, 240),\n> > +\t\tSize(400, 240),\n> > +\t\tSize(480, 320),\n> > +\t\tSize(640, 360),\n> > +\t\tSize(640, 480),\n> > +\t\tSize(720, 480),\n> > +\t\tSize(720, 576),\n> > +\t\tSize(768, 480),\n> > +\t\tSize(800, 600),\n> > +\t\tSize(854, 480),\n> > +\t\tSize(960, 540),\n> > +\t\tSize(960, 640),\n> > +\t\tSize(1024, 576),\n> > +\t\tSize(1024, 600),\n> > +\t\tSize(1024, 768),\n> > +\t\tSize(1152, 864),\n> > +\t\tSize(1280, 1024),\n> > +\t\tSize(1280, 1080),\n> > +\t\tSize(1280, 720),\n> > +\t\tSize(1280, 800),\n> > +\t\tSize(1360, 768),\n> > +\t\tSize(1366, 768),\n> > +\t\tSize(1400, 1050),\n> > +\t\tSize(1440, 900),\n> > +\t\tSize(1536, 864),\n> > +\t\tSize(1600, 1200),\n> > +\t\tSize(1600, 900),\n> > +\t\tSize(1680, 1050),\n> > +\t\tSize(1920, 1080),\n> > +\t\tSize(1920, 1200),\n> > +\t\tSize(2048, 1080),\n> > +\t\tSize(2048, 1152),\n> > +\t\tSize(2048, 1536),\n> > +\t\tSize(2160, 1080),\n> > +\t\tSize(2560, 1080),\n> > +\t\tSize(2560, 1440),\n> > +\t\tSize(2560, 1600),\n> > +\t\tSize(2560, 2048),\n> > +\t\tSize(2960, 1440),\n> > +\t\tSize(3200, 1800),\n> > +\t\tSize(3200, 2048),\n> > +\t\tSize(3200, 2400),\n> > +\t\tSize(3440, 1440),\n> > +\t\tSize(3840, 1080),\n> > +\t\tSize(3840, 1600),\n> > +\t\tSize(3840, 2160),\n> > +\t\tSize(3840, 2400),\n> > +\t\tSize(4096, 2160),\n> > +\t\tSize(5120, 2160),\n> > +\t\tSize(5120, 2880),\n> > +\t\tSize(7680, 4320),\n> \n> I still wonder if we could generate this data table in a better form.\n> \n> For a start, perhaps it might be worth grouping common Sizes to a single\n> line to form a table based on their ratios? That would show what ratios\n> are included maybe?\n> \n> (perhaps we could generate each ratio from a single width or height\n> instead?)\n> \n> /* 4:3                     16:9\t\t\t\t16:10 */\n> \n> Size(1600, 1200),\tSize(1600, 900),\tSize(1680, 1050),\n> \t\t\tSize(1920, 1080),\tSize(1920, 1200),\n> \n> /* 2:1\t\t\t 2:1 ish again?\t\t\t4:3 */\n> Size(2048, 1080),\tSize(2048, 1152),\tSize(2048, 1536),\n> \n> \n> If you did look at doing this, I'd move the table outside of the\n> function so that it's indent is reduced.\n> \n> > +\t};\n> > +\tstd::vector<Size> sizes;\n> > +\n> > +\t/* Make sure pixel format exists. */\n> > +\tauto const &it = formats_.find(pixelformat);\n> > +\tif (it == formats_.end())\n> > +\t\treturn {};\n> > +\n> > +\t/* Try create list of discrete sizes. */\n\ns/create/creating a/\n\n> > +\tconst std::vector<SizeRange> &ranges = it->second;\n> > +\tbool discrete = true;\n> > +\tfor (const SizeRange &range : ranges) {\n> > +\t\tif (range.min != range.max) {\n> > +\t\t\tdiscrete = false;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\tsizes.emplace_back(range.min.width, range.min.height);\n\n\t\tsizes.emplace_back(range.min);\n\n> > +\t}\n> > +\n> > +\t/* If discrete not possible generate from range. */\n> > +\tif (!discrete) {\n> > +\t\tif (ranges.size() != 1) {\n> > +\t\t\tLOG(Stream, Error) << \"Range format is ambiguous\";\n> > +\t\t\treturn {};\n> > +\t\t}\n> > +\n> > +\t\tconst SizeRange &limit = ranges.front();\n> > +\t\tsizes.clear();\n> > +\n> > +\t\tfor (const Size &size : rangeDiscreteSizes) {\n> > +\t\t\tif (size.width < limit.min.width ||\n> > +\t\t\t    size.width > limit.max.width ||\n> > +\t\t\t    size.height < limit.min.height ||\n> > +\t\t\t    size.height > limit.max.height ||\n\nWould it make sense to add a bool SizeRange::contains(const Size &size)\nmethod ?\n\n> > +\t\t\t    (size.width - limit.min.width) % limit.vStep ||\n> > +\t\t\t    (size.height - limit.min.height) % limit.hStep)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tsizes.push_back(size);\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tstd::sort(sizes.begin(), sizes.end());\n> > +\n> > +\treturn sizes;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve a frame size range\n> > + * \\param[in] pixelformat Pixel format to retrieve range for\n> > + * \\returns Range description of frame size\n\nIt would be useful to extend the documentation to explain what the range\nrepresents and how it can be used (in particular that not all sizes\nwithin the range are necessarily supported by the stream).\n\n> > + */\n> > +SizeRange StreamFormats::range(unsigned int pixelformat) const\n> > +{\n> > +\tauto const it = formats_.find(pixelformat);\n> > +\tif (it == formats_.end())\n> > +\t\treturn {};\n> > +\n> > +\tconst SizeRange &first = it->second.front();\n> > +\tif (it->second.size() == 1)\n> > +\t\treturn first;\n> \n> There's a lot of mixing of 'first' and 'second' here which is a bit\n> awkward for comprehension....\n\nYou could write it as\n\n\tconst std::vector<SizeRange> &ranges = it->second;\n\tif (ranges.size() == 1)\n\t\treturn ranges[0];\n\n> > +\n> > +\tLOG(Stream, Debug) << \"Building range from discret\";\n> \n> /discret/discrete/\n> \n> > +\n> > +\tSizeRange range(first.min.width, first.min.height,\n> > +\t\t\tfirst.max.width, first.max.height);\n\nFeel free to add a SizeRange(const Size &min, const Size &max)\nconstructor if you think it would be useful.\n\nYou can also initialise range to (UINT_MAX, UINT_MAX, 0, 0).\n\n> > +\n> > +\tfor (const SizeRange &limit : it->second) {\n> > +\t\tif (limit.min < range.min)\n> > +\t\t\trange.min = limit.min;\n> > +\n> > +\t\tif (limit.max > range.max)\n> > +\t\t\trange.max = limit.max;\n> > +\t}\n> > +\n> > +\treturn range;\n> > +}\n> > +\n> >  /**\n> >   * \\struct StreamConfiguration\n> >   * \\brief Configuration parameters for a stream","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4750630D9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2019 08:50:26 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-134-17-nat.elisa-mobile.fi\n\t[85.76.134.17])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3EF4B569;\n\tMon, 10 Jun 2019 08:50:23 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560149426;\n\tbh=L+XFRNkXnJRfSO6mEWI7NGk1HyO0Z9pxI+GhlRpQBFU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C9cfGHJ0Y45+L93VnBtYOQhWbcPIO/ImZW7F3QPK/lezXmS++m1cK+c2s9AFeBnFS\n\t3v7keOqzD2hbaZ4MQvm7vaJ/gTnaoEMbjUeX2X+MP0MOOGVr5oH8pDJssx6Q7/tTK5\n\tWKO3pjiHBilOhyO2DEpuGvO4KOzEQN4UN4VM1tsQ=","Date":"Mon, 10 Jun 2019 09:50:06 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190610065006.GA4806@pendragon.ideasonboard.com>","References":"<20190527001543.13593-1-niklas.soderlund@ragnatech.se>\n\t<20190527001543.13593-12-niklas.soderlund@ragnatech.se>\n\t<b40438e1-51a7-272c-8240-edf13bf94504@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<b40438e1-51a7-272c-8240-edf13bf94504@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 11/17] libcamera: stream: Add\n\tStreamFormats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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>","X-List-Received-Date":"Mon, 10 Jun 2019 06:50:27 -0000"}}]