[{"id":1622,"web_url":"https://patchwork.libcamera.org/comment/1622/","msgid":"<20190518181334.GE4995@pendragon.ideasonboard.com>","date":"2019-05-18T18:13:34","subject":"Re: [libcamera-devel] [PATCH/RFC 08/12] 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\nThank you for the patch.\n\nOn Sat, May 18, 2019 at 02:06:17AM +0300, Laurent Pinchart wrote:\n> From: Niklas Söderlund \n> \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 steps.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/stream.h | 30 ++++++\n> src/libcamera/stream.cpp | 191 +++++++++++++++++++++++++++++++++++++\n> 2 files changed, 221 insertions(+)\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 47c007ed52e2..138ed649ba8c 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 \n> #include \n> #include \n> \n> @@ -39,6 +40,35 @@ enum StreamRole {\n> \tViewfinder,\n> };\n> \n> +class StreamFormats\n> +{\n> +public:\n> +\tstruct Range {\n> +\t\tSize min;\n> +\t\tSize max;\n> +\t\tunsigned int stepWidth;\n> +\t\tunsigned int stepHeight;\n> +\t};\n\nThis is very similar to the SizeRange structure defined in geometry.h.\nWould it make sense to extend that structure instead ?\n\nI would also rename stepWidth and stepHeight to hStep and vStep\nrespectively, but that may just be me.\n\n> +\n> +\tStreamFormats() {}\n> +\n> +\tStreamFormats(const std::map &rangeSizes)\n> +\t\t: rangeSizes_(rangeSizes) {}\n> +\n> +\tStreamFormats(const std::map> &discreteSizes)\n> +\t\t: discreteSizes_(discreteSizes) {}\n\nWould it be useful to give an alias to these types, the same way we have\nFormatEnum ? And shouldn't FormatEnum be replaced by this class ? We may\nthen want to rename StreamFormats to something that doesn't use the word\nStream, as it's not just related to streams.\n\n> +\n> +\tstd::vector formats() const;\n> +\tstd::vector sizes(unsigned int pixelformat) const;\n> +\n> +\tbool isRange() const { return !rangeSizes_.empty(); }\n> +\tRange range(unsigned int pixelformat) const;\n\nYou should return a const Range & to optimise the case where the caller\nonly wants to inspect the range and doesn't need a copy.\n\n> +\n> +private:\n> +\tstd::map> discreteSizes_;\n> +\tstd::map rangeSizes_;\n> +};\n> +\n> using StreamRoles = std::vector;\n> \n> class Stream\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index 0c59a31a3a05..e82fa8143b8f 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -7,6 +7,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> #include \n> \n> @@ -122,6 +123,196 @@ std::string StreamConfiguration::toString() const\n> * \\brief A vector of StreamRole\n> */\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> + * sizes a stream supports. The class groups size information by the pixel\n> + * format which can produce it. There are two types of size information which\n> + * can be described in the StreamFormats object discrete and range sizes.\n> + *\n> + * The discrete sizes are a list of fixed sizes of the only resolutions the\n> + * stream can produce. While the range description contains a max and min\n> + * size together with a stepping. The range information can either be consumed\n> + * raw which allows users to calculate a size which the stream could support\n> + * or be accessed thru the sizes() helper which will compute a list of common\n> + * discrete sizes which can be produced within the range.\n> + */\n\nI think we'll have to refine both the API and its documentation, to\nexplain better how this operates and is supposed to be used. Let's\ndiscuss the issues pointed out through this patch first.\n\n> +\n> +/**\n> + * \\class StreamFormats::Range\n> + * \\brief Hold information about stream format range\n> + */\n> +\n> +/**\n> + * \\var StreamFormats::Range::min\n> + * \\brief Range minimum size\n> + */\n> +\n> +/**\n> + * \\var StreamFormats::Range::max\n> + * \\brief Range maximum size\n> + */\n> +\n> +/**\n> + * \\var StreamFormats::Range::stepWidth\n> + * \\brief Range width step length in pixels\n> + */\n> +\n> +/**\n> + * \\var StreamFormats::Range::stepHeight\n> + * \\brief Range height step length in pixels\n> + */\n> +\n> +/**\n> + * \\fn StreamFormats::StreamFormats(const std::map< unsigned int, Range > &rangeSizes)\n> + * \\brief Constrict a ranged based StreamFormats object\n\ns/Constrict/Construct/\n\n> + * \\param[in] rangeSizes A map of pixel format to a ranged description\n> + * \\sa StreamFormats::Range\n> + */\n> +\n> +/**\n> + * \\fn StreamFormats::StreamFormats(const std::map< unsigned int, std::vector< Size >> &discreteSizes)\n> + * \\brief Constrict a discrete based StreamFormats object\n> + * \\param[in] discreteSizes A map of pixel format to a list of frame sizes\n> + */\n> +\n> +/**\n> + * \\brief Retrive a list of pixel formats supported by the stram\n\ns/Retrive/Retrieve/ (in multiple locations)\ns/a list/the list/\ns/stram/stream/\n\n> + * \\returns A list of pixel formats\n> + */\n> +std::vector StreamFormats::formats() const\n> +{\n> +\tstd::vector formats;\n> +\n> +\tif (isRange()) {\n> +\t\tfor (auto const &it : rangeSizes_)\n> +\t\t\tformats.push_back(it.first);\n> +\t} else {\n> +\t\tfor (auto const &it : discreteSizes_)\n> +\t\t\tformats.push_back(it.first);\n> +\t}\n> +\n> +\treturn formats;\n> +}\n> +\n> +/**\n> + * \\brief Retrive a list of frame sizes\n> + * \\param[in] pixelformat Pixel format to retrive sizes for\n\nThis needs to be expanded to explain the mechanism for range-based\nsizes.\n\n> + * \\returns A list of frame sizes\n> + */\n> +std::vector StreamFormats::sizes(unsigned int pixelformat) const\n> +{\n> +\tstd::vector sizes;\n> +\n> +\t/*\n> +\t * Sizes to try and extract from ranges.\n> +\t * \\todo Verify list of resolutions are good\n\nCould you explain where they come from ?\n\n> +\t */\n> +\tstatic const std::vector rangeDescreteSizes = {\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> +\t};\n> +\n> +\tif (!isRange()) {\n> +\t\tauto const &it = discreteSizes_.find(pixelformat);\n> +\t\tif (it == discreteSizes_.end())\n> +\t\t\treturn {};\n> +\t\tsizes = it->second;\n> +\t} else {\n> +\t\tRange limit = range(pixelformat);\n> +\n> +\t\tfor (const Size &size : rangeDescreteSizes) {\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 be useful to add a contains() method to the Range class ?\n\n> +\t\t\t size.width % limit.stepWidth ||\n> +\t\t\t size.height % limit.stepHeight)\n\nIs this correct ? I would have considered the step differently than an\nalignment, as in the range containing all min + i * step values up to\nmax. If min is not aligned to step, your check leads to a different\nresult. In any case this should be clearly documented in the Range\nclass.\n\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> + * \\fn StreamFormats::isRange()\n> + * \\brief Check if the StreamFormat is a range description\n> + * \\returns True if the description is a range, false otherwise\n> + */\n> +\n> +/**\n> + * \\brief Retrive the range description\n> + * \\param[in] pixelformat Pixel format to retrive description for\n\nThis doesn't document that the function is only available for\nrange-based StreamFormats instances, and what happens in the pixel\nformat is not supported. Furthermore, I think the method should also\nsupport the discrete sizes, by returning a range made from the minimum\nand maximum size. Otherwise it gets quite inconvenient for applications\nthat want to quickly check what the minimum and maximum sizes here.\nWe'll run into the issue of what to report as a step in that case, one\noption would be to set the step to 0 to report that there's no single\nstep value, but other options may be possible.\n\nAll in all I think this class suffers from similar issues than\nFormatEnum, we need to think more about how it will be used to offer a\nnice, clean and clear (and clearly documented) API to applications.\n\n> + * \\returns The range description\n> + */\n> +StreamFormats::Range StreamFormats::range(unsigned int pixelformat) const\n> +{\n> +\tauto const it = rangeSizes_.find(pixelformat);\n> +\n> +\tif (it == rangeSizes_.end())\n> +\t\treturn {};\n> +\n> +\treturn it->second;\n> +}\n> +\n> /**\n> * \\class Stream\n> * \\brief Video stream for a camera","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 3470660C1D\n\tfor ;\n\tSat, 18 May 2019 20:13:51 +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 918ECD5\n\tfor ;\n\tSat, 18 May 2019 20:13:50 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558203230;\n\tbh=A9qeHkuytkMC01ftkjeqZqjREk/G8+7PJt9IvFWMMhk=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=v7NOH6O4prQ+y3gM02RWC/ugRISC5GwGlRSSaNxRxenNJhpHbBlD5QhDP37Af9x43\n\t0EKraa89WpX3Z0rRKzYtBjWSiv6HtmDlnlnZgU4Fk/dBq4kuyCATxTXPBosBfDdhVk\n\tGArsTSiu0LutLleuYfVKdwwTQkBUg2T76Ezo2/lM=","Date":"Sat, 18 May 2019 21:13:34 +0300","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190518181334.GE4995@pendragon.ideasonboard.com>","References":"<20190517230621.24668-1-laurent.pinchart@ideasonboard.com>\n\t<20190517230621.24668-9-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190517230621.24668-9-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH/RFC 08/12] libcamera: stream: Add\n\tStreamFormats","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":"Sat, 18 May 2019 18:13:51 -0000"}},{"id":1626,"web_url":"https://patchwork.libcamera.org/comment/1626/","msgid":"","date":"2019-05-19T10:23:38","subject":"Re: [libcamera-devel] [PATCH/RFC 08/12] 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 18/05/2019 19:13, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sat, May 18, 2019 at 02:06:17AM +0300, Laurent Pinchart wrote:\n>> From: Niklas Söderlund \n>>\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 steps.\n>>\n>> Signed-off-by: Niklas Söderlund \n>> ---\n>> include/libcamera/stream.h | 30 ++++++\n>> src/libcamera/stream.cpp | 191 +++++++++++++++++++++++++++++++++++++\n>> 2 files changed, 221 insertions(+)\n>>\n>> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n>> index 47c007ed52e2..138ed649ba8c 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 \n>> #include \n>> #include \n>> \n>> @@ -39,6 +40,35 @@ enum StreamRole {\n>> \tViewfinder,\n>> };\n>> \n>> +class StreamFormats\n>> +{\n>> +public:\n>> +\tstruct Range {\n>> +\t\tSize min;\n>> +\t\tSize max;\n>> +\t\tunsigned int stepWidth;\n>> +\t\tunsigned int stepHeight;\n>> +\t};\n> \n> This is very similar to the SizeRange structure defined in geometry.h.\n> Would it make sense to extend that structure instead ?\n> \n> I would also rename stepWidth and stepHeight to hStep and vStep\n> respectively, but that may just be me.\n> \n>> +\n>> +\tStreamFormats() {}\n>> +\n>> +\tStreamFormats(const std::map &rangeSizes)\n>> +\t\t: rangeSizes_(rangeSizes) {}\n>> +\n>> +\tStreamFormats(const std::map> &discreteSizes)\n>> +\t\t: discreteSizes_(discreteSizes) {}\n> \n> Would it be useful to give an alias to these types, the same way we have\n> FormatEnum ? And shouldn't FormatEnum be replaced by this class ? We may\n> then want to rename StreamFormats to something that doesn't use the word\n> Stream, as it's not just related to streams.\n> \n>> +\n>> +\tstd::vector formats() const;\n>> +\tstd::vector sizes(unsigned int pixelformat) const;\n>> +\n>> +\tbool isRange() const { return !rangeSizes_.empty(); }\n>> +\tRange range(unsigned int pixelformat) const;\n> \n> You should return a const Range & to optimise the case where the caller\n> only wants to inspect the range and doesn't need a copy.\n> \n>> +\n>> +private:\n>> +\tstd::map> discreteSizes_;\n>> +\tstd::map rangeSizes_;\n>> +};\n>> +\n>> using StreamRoles = std::vector;\n>> \n>> class Stream\n>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n>> index 0c59a31a3a05..e82fa8143b8f 100644\n>> --- a/src/libcamera/stream.cpp\n>> +++ b/src/libcamera/stream.cpp\n>> @@ -7,6 +7,7 @@\n>> \n>> #include \n>> \n>> +#include \n>> #include \n>> #include \n>> \n>> @@ -122,6 +123,196 @@ std::string StreamConfiguration::toString() const\n>> * \\brief A vector of StreamRole\n>> */\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>> + * sizes a stream supports. The class groups size information by the pixel\n>> + * format which can produce it. There are two types of size information which\n>> + * can be described in the StreamFormats object discrete and range sizes.\n>> + *\n>> + * The discrete sizes are a list of fixed sizes of the only resolutions the\n>> + * stream can produce. While the range description contains a max and min\n>> + * size together with a stepping. The range information can either be consumed\n>> + * raw which allows users to calculate a size which the stream could support\n>> + * or be accessed thru the sizes() helper which will compute a list of common\n>> + * discrete sizes which can be produced within the range.\n>> + */\n> \n> I think we'll have to refine both the API and its documentation, to\n> explain better how this operates and is supposed to be used. Let's\n> discuss the issues pointed out through this patch first.\n> \n>> +\n>> +/**\n>> + * \\class StreamFormats::Range\n>> + * \\brief Hold information about stream format range\n>> + */\n>> +\n>> +/**\n>> + * \\var StreamFormats::Range::min\n>> + * \\brief Range minimum size\n>> + */\n>> +\n>> +/**\n>> + * \\var StreamFormats::Range::max\n>> + * \\brief Range maximum size\n>> + */\n>> +\n>> +/**\n>> + * \\var StreamFormats::Range::stepWidth\n>> + * \\brief Range width step length in pixels\n>> + */\n>> +\n>> +/**\n>> + * \\var StreamFormats::Range::stepHeight\n>> + * \\brief Range height step length in pixels\n>> + */\n>> +\n>> +/**\n>> + * \\fn StreamFormats::StreamFormats(const std::map< unsigned int, Range > &rangeSizes)\n>> + * \\brief Constrict a ranged based StreamFormats object\n> \n> s/Constrict/Construct/\n> \n>> + * \\param[in] rangeSizes A map of pixel format to a ranged description\n>> + * \\sa StreamFormats::Range\n>> + */\n>> +\n>> +/**\n>> + * \\fn StreamFormats::StreamFormats(const std::map< unsigned int, std::vector< Size >> &discreteSizes)\n>> + * \\brief Constrict a discrete based StreamFormats object\n>> + * \\param[in] discreteSizes A map of pixel format to a list of frame sizes\n>> + */\n>> +\n>> +/**\n>> + * \\brief Retrive a list of pixel formats supported by the stram\n> \n> s/Retrive/Retrieve/ (in multiple locations)\n> s/a list/the list/\n> s/stram/stream/\n> \n>> + * \\returns A list of pixel formats\n>> + */\n>> +std::vector StreamFormats::formats() const\n>> +{\n>> +\tstd::vector formats;\n>> +\n>> +\tif (isRange()) {\n>> +\t\tfor (auto const &it : rangeSizes_)\n>> +\t\t\tformats.push_back(it.first);\n>> +\t} else {\n>> +\t\tfor (auto const &it : discreteSizes_)\n>> +\t\t\tformats.push_back(it.first);\n>> +\t}\n>> +\n>> +\treturn formats;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrive a list of frame sizes\n>> + * \\param[in] pixelformat Pixel format to retrive sizes for\n> \n> This needs to be expanded to explain the mechanism for range-based\n> sizes.\n> \n>> + * \\returns A list of frame sizes\n>> + */\n>> +std::vector StreamFormats::sizes(unsigned int pixelformat) const\n>> +{\n>> +\tstd::vector sizes;\n>> +\n>> +\t/*\n>> +\t * Sizes to try and extract from ranges.\n>> +\t * \\todo Verify list of resolutions are good\n> \n> Could you explain where they come from ?\n> \n>> +\t */\n>> +\tstatic const std::vector rangeDescreteSizes = {\n\n\ns/Descrete/Discrete/\n\n\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>> +\t};\n\nIs that a list that we could generate somehow instead from some conditions?\n\nPerhaps generate based on aspect ratios and known widths and multipliers\nor something... not sure it will be any better - but it might be more\nreadable or easy to maintain?\n\n\n>> +\n>> +\tif (!isRange()) {\n>> +\t\tauto const &it = discreteSizes_.find(pixelformat);\n>> +\t\tif (it == discreteSizes_.end())\n>> +\t\t\treturn {};\n>> +\t\tsizes = it->second;\n>> +\t} else {\n>> +\t\tRange limit = range(pixelformat);\n>> +\n>> +\t\tfor (const Size &size : rangeDescreteSizes) {\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> \n> Would it be useful to add a contains() method to the Range class ?\n> \n>> +\t\t\t size.width % limit.stepWidth ||\n>> +\t\t\t size.height % limit.stepHeight)\n> \n> Is this correct ? I would have considered the step differently than an\n> alignment, as in the range containing all min + i * step values up to\n> max. If min is not aligned to step, your check leads to a different\n> result. In any case this should be clearly documented in the Range\n> class.\n> \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>> + * \\fn StreamFormats::isRange()\n>> + * \\brief Check if the StreamFormat is a range description\n>> + * \\returns True if the description is a range, false otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\brief Retrive the range description\n>> + * \\param[in] pixelformat Pixel format to retrive description for\n> \n> This doesn't document that the function is only available for\n> range-based StreamFormats instances, and what happens in the pixel\n> format is not supported. Furthermore, I think the method should also\n> support the discrete sizes, by returning a range made from the minimum\n> and maximum size. Otherwise it gets quite inconvenient for applications\n> that want to quickly check what the minimum and maximum sizes here.\n> We'll run into the issue of what to report as a step in that case, one\n> option would be to set the step to 0 to report that there's no single\n> step value, but other options may be possible.\n> \n> All in all I think this class suffers from similar issues than\n> FormatEnum, we need to think more about how it will be used to offer a\n> nice, clean and clear (and clearly documented) API to applications.\n> \n>> + * \\returns The range description\n>> + */\n>> +StreamFormats::Range StreamFormats::range(unsigned int pixelformat) const\n>> +{\n>> +\tauto const it = rangeSizes_.find(pixelformat);\n>> +\n>> +\tif (it == rangeSizes_.end())\n>> +\t\treturn {};\n>> +\n>> +\treturn it->second;\n>> +}\n>> +\n>> /**\n>> * \\class Stream\n>> * \\brief Video stream for a camera\n> \n\nWould we later use this class somehow to handle 'format negotiation'\nbetween two entities? I.e. given two sets of formats, return a third set\nwhich is the intersection of both?","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 83AD260103\n\tfor ;\n\tSun, 19 May 2019 12:23:42 +0200 (CEST)","from [192.168.0.26]\n\t(cpc139364-aztw33-2-0-cust1325.18-1.cable.virginm.net\n\t[94.173.229.46])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B19E8443;\n\tSun, 19 May 2019 12:23:41 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558261421;\n\tbh=E+G/stiKgHqAtdbCT+wrgyZQDqjFBgwEsCd+IpC/iRs=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=U5TirUYneNtAtwJjw3zUtNT6mNUCPCEpmU8+UP8m7JKCx8+5XgmlCjg4W+ByPKayY\n\tgyd9+5xnSFeRz49f+9RxrqcwC//YUsvykOq78Qhoe6zFD3TNbK06F00Ik7Mms5U3NM\n\t0YOGpdmNZQ7ygPO1Cj7LLvNnYKEt4GKbqgjT+B1o=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20190517230621.24668-1-laurent.pinchart@ideasonboard.com>\n\t<20190517230621.24668-9-laurent.pinchart@ideasonboard.com>\n\t<20190518181334.GE4995@pendragon.ideasonboard.com>","From":"Kieran Bingham ","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":"","Date":"Sun, 19 May 2019 11:23:38 +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":"<20190518181334.GE4995@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH/RFC 08/12] libcamera: stream: Add\n\tStreamFormats","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":"Sun, 19 May 2019 10:23:42 -0000"}},{"id":1629,"web_url":"https://patchwork.libcamera.org/comment/1629/","msgid":"<20190519145156.GB5213@pendragon.ideasonboard.com>","date":"2019-05-19T14:51:56","subject":"Re: [libcamera-devel] [PATCH/RFC 08/12] 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 Kieran,\n\nOn Sun, May 19, 2019 at 11:23:38AM +0100, Kieran Bingham wrote:\n> On 18/05/2019 19:13, Laurent Pinchart wrote:\n> > On Sat, May 18, 2019 at 02:06:17AM +0300, Laurent Pinchart wrote:\n> >> From: Niklas Söderlund \n> >>\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 steps.\n> >>\n> >> Signed-off-by: Niklas Söderlund \n> >> ---\n> >> include/libcamera/stream.h | 30 ++++++\n> >> src/libcamera/stream.cpp | 191 +++++++++++++++++++++++++++++++++++++\n> >> 2 files changed, 221 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> >> index 47c007ed52e2..138ed649ba8c 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 \n> >> #include \n> >> #include \n> >> \n> >> @@ -39,6 +40,35 @@ enum StreamRole {\n> >> \tViewfinder,\n> >> };\n> >> \n> >> +class StreamFormats\n> >> +{\n> >> +public:\n> >> +\tstruct Range {\n> >> +\t\tSize min;\n> >> +\t\tSize max;\n> >> +\t\tunsigned int stepWidth;\n> >> +\t\tunsigned int stepHeight;\n> >> +\t};\n> > \n> > This is very similar to the SizeRange structure defined in geometry.h.\n> > Would it make sense to extend that structure instead ?\n> > \n> > I would also rename stepWidth and stepHeight to hStep and vStep\n> > respectively, but that may just be me.\n> > \n> >> +\n> >> +\tStreamFormats() {}\n> >> +\n> >> +\tStreamFormats(const std::map &rangeSizes)\n> >> +\t\t: rangeSizes_(rangeSizes) {}\n> >> +\n> >> +\tStreamFormats(const std::map> &discreteSizes)\n> >> +\t\t: discreteSizes_(discreteSizes) {}\n> > \n> > Would it be useful to give an alias to these types, the same way we have\n> > FormatEnum ? And shouldn't FormatEnum be replaced by this class ? We may\n> > then want to rename StreamFormats to something that doesn't use the word\n> > Stream, as it's not just related to streams.\n> > \n> >> +\n> >> +\tstd::vector formats() const;\n> >> +\tstd::vector sizes(unsigned int pixelformat) const;\n> >> +\n> >> +\tbool isRange() const { return !rangeSizes_.empty(); }\n> >> +\tRange range(unsigned int pixelformat) const;\n> > \n> > You should return a const Range & to optimise the case where the caller\n> > only wants to inspect the range and doesn't need a copy.\n> > \n> >> +\n> >> +private:\n> >> +\tstd::map> discreteSizes_;\n> >> +\tstd::map rangeSizes_;\n> >> +};\n> >> +\n> >> using StreamRoles = std::vector;\n> >> \n> >> class Stream\n> >> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> >> index 0c59a31a3a05..e82fa8143b8f 100644\n> >> --- a/src/libcamera/stream.cpp\n> >> +++ b/src/libcamera/stream.cpp\n> >> @@ -7,6 +7,7 @@\n> >> \n> >> #include \n> >> \n> >> +#include \n> >> #include \n> >> #include \n> >> \n> >> @@ -122,6 +123,196 @@ std::string StreamConfiguration::toString() const\n> >> * \\brief A vector of StreamRole\n> >> */\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> >> + * sizes a stream supports. The class groups size information by the pixel\n> >> + * format which can produce it. There are two types of size information which\n> >> + * can be described in the StreamFormats object discrete and range sizes.\n> >> + *\n> >> + * The discrete sizes are a list of fixed sizes of the only resolutions the\n> >> + * stream can produce. While the range description contains a max and min\n> >> + * size together with a stepping. The range information can either be consumed\n> >> + * raw which allows users to calculate a size which the stream could support\n> >> + * or be accessed thru the sizes() helper which will compute a list of common\n> >> + * discrete sizes which can be produced within the range.\n> >> + */\n> > \n> > I think we'll have to refine both the API and its documentation, to\n> > explain better how this operates and is supposed to be used. Let's\n> > discuss the issues pointed out through this patch first.\n> > \n> >> +\n> >> +/**\n> >> + * \\class StreamFormats::Range\n> >> + * \\brief Hold information about stream format range\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var StreamFormats::Range::min\n> >> + * \\brief Range minimum size\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var StreamFormats::Range::max\n> >> + * \\brief Range maximum size\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var StreamFormats::Range::stepWidth\n> >> + * \\brief Range width step length in pixels\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var StreamFormats::Range::stepHeight\n> >> + * \\brief Range height step length in pixels\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn StreamFormats::StreamFormats(const std::map< unsigned int, Range > &rangeSizes)\n> >> + * \\brief Constrict a ranged based StreamFormats object\n> > \n> > s/Constrict/Construct/\n> > \n> >> + * \\param[in] rangeSizes A map of pixel format to a ranged description\n> >> + * \\sa StreamFormats::Range\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn StreamFormats::StreamFormats(const std::map< unsigned int, std::vector< Size >> &discreteSizes)\n> >> + * \\brief Constrict a discrete based StreamFormats object\n> >> + * \\param[in] discreteSizes A map of pixel format to a list of frame sizes\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Retrive a list of pixel formats supported by the stram\n> > \n> > s/Retrive/Retrieve/ (in multiple locations)\n> > s/a list/the list/\n> > s/stram/stream/\n> > \n> >> + * \\returns A list of pixel formats\n> >> + */\n> >> +std::vector StreamFormats::formats() const\n> >> +{\n> >> +\tstd::vector formats;\n> >> +\n> >> +\tif (isRange()) {\n> >> +\t\tfor (auto const &it : rangeSizes_)\n> >> +\t\t\tformats.push_back(it.first);\n> >> +\t} else {\n> >> +\t\tfor (auto const &it : discreteSizes_)\n> >> +\t\t\tformats.push_back(it.first);\n> >> +\t}\n> >> +\n> >> +\treturn formats;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Retrive a list of frame sizes\n> >> + * \\param[in] pixelformat Pixel format to retrive sizes for\n> > \n> > This needs to be expanded to explain the mechanism for range-based\n> > sizes.\n> > \n> >> + * \\returns A list of frame sizes\n> >> + */\n> >> +std::vector StreamFormats::sizes(unsigned int pixelformat) const\n> >> +{\n> >> +\tstd::vector sizes;\n> >> +\n> >> +\t/*\n> >> +\t * Sizes to try and extract from ranges.\n> >> +\t * \\todo Verify list of resolutions are good\n> > \n> > Could you explain where they come from ?\n> > \n> >> +\t */\n> >> +\tstatic const std::vector rangeDescreteSizes = {\n> \n> s/Descrete/Discrete/\n> \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> >> +\t};\n> \n> Is that a list that we could generate somehow instead from some conditions?\n> \n> Perhaps generate based on aspect ratios and known widths and multipliers\n> or something... not sure it will be any better - but it might be more\n> readable or easy to maintain?\n> \n> >> +\n> >> +\tif (!isRange()) {\n> >> +\t\tauto const &it = discreteSizes_.find(pixelformat);\n> >> +\t\tif (it == discreteSizes_.end())\n> >> +\t\t\treturn {};\n> >> +\t\tsizes = it->second;\n> >> +\t} else {\n> >> +\t\tRange limit = range(pixelformat);\n> >> +\n> >> +\t\tfor (const Size &size : rangeDescreteSizes) {\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> > \n> > Would it be useful to add a contains() method to the Range class ?\n> > \n> >> +\t\t\t size.width % limit.stepWidth ||\n> >> +\t\t\t size.height % limit.stepHeight)\n> > \n> > Is this correct ? I would have considered the step differently than an\n> > alignment, as in the range containing all min + i * step values up to\n> > max. If min is not aligned to step, your check leads to a different\n> > result. In any case this should be clearly documented in the Range\n> > class.\n> > \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> >> + * \\fn StreamFormats::isRange()\n> >> + * \\brief Check if the StreamFormat is a range description\n> >> + * \\returns True if the description is a range, false otherwise\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Retrive the range description\n> >> + * \\param[in] pixelformat Pixel format to retrive description for\n> > \n> > This doesn't document that the function is only available for\n> > range-based StreamFormats instances, and what happens in the pixel\n> > format is not supported. Furthermore, I think the method should also\n> > support the discrete sizes, by returning a range made from the minimum\n> > and maximum size. Otherwise it gets quite inconvenient for applications\n> > that want to quickly check what the minimum and maximum sizes here.\n> > We'll run into the issue of what to report as a step in that case, one\n> > option would be to set the step to 0 to report that there's no single\n> > step value, but other options may be possible.\n> > \n> > All in all I think this class suffers from similar issues than\n> > FormatEnum, we need to think more about how it will be used to offer a\n> > nice, clean and clear (and clearly documented) API to applications.\n> > \n> >> + * \\returns The range description\n> >> + */\n> >> +StreamFormats::Range StreamFormats::range(unsigned int pixelformat) const\n> >> +{\n> >> +\tauto const it = rangeSizes_.find(pixelformat);\n> >> +\n> >> +\tif (it == rangeSizes_.end())\n> >> +\t\treturn {};\n> >> +\n> >> +\treturn it->second;\n> >> +}\n> >> +\n> >> /**\n> >> * \\class Stream\n> >> * \\brief Video stream for a camera\n> > \n> \n> Would we later use this class somehow to handle 'format negotiation'\n> between two entities? I.e. given two sets of formats, return a third set\n> which is the intersection of both?\n\nI think we need classes to help with format propagation along the\npipeline. I'm more and more wondering if we need two classes for this, a\nV4L2-oriented class to be used internally, and a friendlier class\nexposed to applications.","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 5060F60E4C\n\tfor ;\n\tSun, 19 May 2019 16:52:13 +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 D6FC9443;\n\tSun, 19 May 2019 16:52:12 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558277533;\n\tbh=UCNklKG1rL48xJOFjMokH9+FIn7m9ji6N5D2EKFbluE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cyXrClGo8lJ9oP1IqR2/M+8Goze/53W1wXomznm9/3pPKMGNWUK86jMY17Z9KMO6m\n\tXg2eEYILS8XOfDGYGEO7mdRGcWZvPh8cfEyEQ5qG+BiHmixrA6iZpbx0X/6s6y3zNR\n\tbkPyk9tsX33Uz9bYoY544gPzrWIywPIh/ATpQXPo=","Date":"Sun, 19 May 2019 17:51:56 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190519145156.GB5213@pendragon.ideasonboard.com>","References":"<20190517230621.24668-1-laurent.pinchart@ideasonboard.com>\n\t<20190517230621.24668-9-laurent.pinchart@ideasonboard.com>\n\t<20190518181334.GE4995@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH/RFC 08/12] libcamera: stream: Add\n\tStreamFormats","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":"Sun, 19 May 2019 14:52:13 -0000"}}]