[{"id":1910,"web_url":"https://patchwork.libcamera.org/comment/1910/","msgid":"<20190618220931.GH23556@pendragon.ideasonboard.com>","date":"2019-06-18T22:09:31","subject":"Re: [libcamera-devel] [PATCH v3 02/16] libcamera: geometry:\n\tSizeRange: Extend with step information","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:48PM +0200, Niklas Söderlund wrote:\n> The size range described might be subject to certain step\n> limitations. Make it possible to record this information.\n> \n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> ---\n> include/libcamera/geometry.h | 13 +++++++++--\n> src/libcamera/geometry.cpp | 44 +++++++++++++++++++++++++++++++++---\n> 2 files changed, 52 insertions(+), 5 deletions(-)\n> \n> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> index ec5ed2bee196c82d..b2a8e5b0e005e4db 100644\n> --- a/include/libcamera/geometry.h\n> +++ b/include/libcamera/geometry.h\n> @@ -73,18 +73,27 @@ struct SizeRange {\n> \t}\n> \n> \tSizeRange(unsigned int width, unsigned int height)\n> -\t\t: min(width, height), max(width, height)\n> +\t\t: min(width, height), max(width, height), hStep(1), vStep(1)\n> \t{\n> \t}\n> \n> \tSizeRange(unsigned int minW, unsigned int minH,\n> \t\t unsigned int maxW, unsigned int maxH)\n> -\t\t: min(minW, minH), max(maxW, maxH)\n> +\t\t: min(minW, minH), max(maxW, maxH), hStep(1), vStep(1)\n> +\t{\n> +\t}\n> +\n> +\tSizeRange(unsigned int minW, unsigned int minH,\n> +\t\t unsigned int maxW, unsigned int maxH,\n> +\t\t unsigned int hstep, unsigned int vstep)\n> +\t\t: min(minW, minH), max(maxW, maxH), hStep(hstep), vStep(vstep)\n> \t{\n> \t}\n> \n> \tSize min;\n> \tSize max;\n> +\tunsigned int hStep;\n> +\tunsigned int vStep;\n> };\n> \n> bool operator==(const SizeRange &lhs, const SizeRange &rhs);\n> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> index 26a05b9e7d800077..dfc264b2700f7f61 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -186,9 +186,24 @@ bool operator<(const Size &lhs, const Size &rhs)\n> * \\struct SizeRange\n> * \\brief Describe a range of sizes\n> *\n> - * SizeRange describes a range of sizes included in the [min, max]\n> - * interval for both the width and the height. If the minimum and\n> - * maximum sizes are identical it represents a single size.\n> + * SizeRange describes a range of sizes included in the [min, max] interval\n\nWhile at it, I would write\n\n\"A SizeRange describes\", \"A SizeRange instance describes\", \"SizeRange\ninstances describe\" or \"The SizeRange class describes\".\n\n> + * for both the width and the height. If the minimum and maximum sizes are\n> + * identical it represents a single size.\n> + *\n> + * The SizeRange may also describe a vertical and horizontal step. The step\n> + * describes the value in pixels the width/height size gets incremented by,\n> + * starting from the minimum size.\n\nHow about\n\n\"Size ranges may further limit the valid sizes through steps in the\nhorizontal and vertical direction. The step values represent the\nincrease in pixels between two valid width or height values, starting\nfrom the minimum. Valid sizes within the range are thus expressed as\"\n\n> + *\n> + *\twidth = min.width + hStep * x\n> + *\theight = min.height + vStep * y\n> + *\n> + *\tWhere:\n\ns/Where:/where/\n\n> + *\n> + *\twidth <= max.width\n> + *\theight < max.height\n> + *\n> + * For SizeRange instances describing a single size the incremental step values\n> + * are fixed to 1.\n\n\"Note that the step values are not equivalent to alignments, as the\nminimum width or height may not be a multiple of the corresponding\nstep.\n\nThe step values are guaranteed to be larger than zero. In particular,\nSizeRange instances that describe a single size have both step values\nset to 1.\"\n\n> */\n> \n> /**\n> @@ -213,6 +228,19 @@ bool operator<(const Size &lhs, const Size &rhs)\n> * \\param[in] maxH The maximum height\n> */\n> \n> +/**\n> + * \\fn SizeRange::SizeRange(unsigned int minW, unsigned int minH,\n> + *\t\t\t unsigned int maxW, unsigned int maxH,\n> + *\t\t\t unsigned int hstep, unsigned int vstep)\n> + * \\brief Construct an initialized size range\n\nKieran would say s/initialized/initialised/ ;-)\n\nWith these issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> + * \\param[in] minW The minimum width\n> + * \\param[in] minH The minimum height\n> + * \\param[in] maxW The maximum width\n> + * \\param[in] maxH The maximum height\n> + * \\param[in] hstep The horizontal step\n> + * \\param[in] vstep The vertical step\n> + */\n> +\n> /**\n> * \\var SizeRange::min\n> * \\brief The minimum size\n> @@ -223,6 +251,16 @@ bool operator<(const Size &lhs, const Size &rhs)\n> * \\brief The maximum size\n> */\n> \n> +/**\n> + * \\var SizeRange::hStep\n> + * \\brief The horizontal step\n> + */\n> +\n> +/**\n> + * \\var SizeRange::vStep\n> + * \\brief The vertical step\n> + */\n> +\n> /**\n> * \\brief Compare size ranges for equality\n> * \\return True if the two size ranges are equal, false otherwise","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 DC5D960BE4\n\tfor ;\n\tWed, 19 Jun 2019 00:09:49 +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 36405D5;\n\tWed, 19 Jun 2019 00:09:49 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560895789;\n\tbh=L68iVm2XVvZ87tCQ3RUw5hCe9X9NtZJrS7OGvQgyAv0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wUU9Q/J4xZgiL4GFNLLqo5B9W9LHordygNdBssHI5tuB680b1FP5T2dJ0sFJTw/Dt\n\t5EA6kqZLOWIlY/T696cHnjVY7FtBeeW/s4PolKMjTg8R9Ii/bnsiBb4Oo22LL2+ArS\n\tk9ttCNZ/6jNto/EBd6Fu1YNbLuulFJJYckZ7VHIA=","Date":"Wed, 19 Jun 2019 01:09:31 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190618220931.GH23556@pendragon.ideasonboard.com>","References":"<20190616133402.21934-1-niklas.soderlund@ragnatech.se>\n\t<20190616133402.21934-3-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-3-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 02/16] libcamera: geometry:\n\tSizeRange: Extend with step information","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:09:50 -0000"}},{"id":1919,"web_url":"https://patchwork.libcamera.org/comment/1919/","msgid":"<20190618233749.GQ23556@pendragon.ideasonboard.com>","date":"2019-06-18T23:37:49","subject":"Re: [libcamera-devel] [PATCH v3 02/16] libcamera: geometry:\n\tSizeRange: Extend with step information","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nA few more comments after reviewing patch 10/16.\n\nOn Wed, Jun 19, 2019 at 01:09:31AM +0300, Laurent Pinchart wrote:\n> On Sun, Jun 16, 2019 at 03:33:48PM +0200, Niklas Söderlund wrote:\n> > The size range described might be subject to certain step\n> > limitations. Make it possible to record this information.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > include/libcamera/geometry.h | 13 +++++++++--\n> > src/libcamera/geometry.cpp | 44 +++++++++++++++++++++++++++++++++---\n> > 2 files changed, 52 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > index ec5ed2bee196c82d..b2a8e5b0e005e4db 100644\n> > --- a/include/libcamera/geometry.h\n> > +++ b/include/libcamera/geometry.h\n> > @@ -73,18 +73,27 @@ struct SizeRange {\n> > \t}\n> > \n> > \tSizeRange(unsigned int width, unsigned int height)\n> > -\t\t: min(width, height), max(width, height)\n> > +\t\t: min(width, height), max(width, height), hStep(1), vStep(1)\n> > \t{\n> > \t}\n> > \n> > \tSizeRange(unsigned int minW, unsigned int minH,\n> > \t\t unsigned int maxW, unsigned int maxH)\n> > -\t\t: min(minW, minH), max(maxW, maxH)\n> > +\t\t: min(minW, minH), max(maxW, maxH), hStep(1), vStep(1)\n> > +\t{\n> > +\t}\n> > +\n> > +\tSizeRange(unsigned int minW, unsigned int minH,\n> > +\t\t unsigned int maxW, unsigned int maxH,\n> > +\t\t unsigned int hstep, unsigned int vstep)\n> > +\t\t: min(minW, minH), max(maxW, maxH), hStep(hstep), vStep(vstep)\n> > \t{\n> > \t}\n> > \n> > \tSize min;\n> > \tSize max;\n> > +\tunsigned int hStep;\n> > +\tunsigned int vStep;\n> > };\n> > \n> > bool operator==(const SizeRange &lhs, const SizeRange &rhs);\n> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > index 26a05b9e7d800077..dfc264b2700f7f61 100644\n> > --- a/src/libcamera/geometry.cpp\n> > +++ b/src/libcamera/geometry.cpp\n> > @@ -186,9 +186,24 @@ bool operator<(const Size &lhs, const Size &rhs)\n> > * \\struct SizeRange\n> > * \\brief Describe a range of sizes\n> > *\n> > - * SizeRange describes a range of sizes included in the [min, max]\n> > - * interval for both the width and the height. If the minimum and\n> > - * maximum sizes are identical it represents a single size.\n> > + * SizeRange describes a range of sizes included in the [min, max] interval\n> \n> While at it, I would write\n> \n> \"A SizeRange describes\", \"A SizeRange instance describes\", \"SizeRange\n> instances describe\" or \"The SizeRange class describes\".\n> \n> > + * for both the width and the height. If the minimum and maximum sizes are\n> > + * identical it represents a single size.\n> > + *\n> > + * The SizeRange may also describe a vertical and horizontal step. The step\n> > + * describes the value in pixels the width/height size gets incremented by,\n> > + * starting from the minimum size.\n> \n> How about\n> \n> \"Size ranges may further limit the valid sizes through steps in the\n> horizontal and vertical direction. The step values represent the\n> increase in pixels between two valid width or height values, starting\n> from the minimum. Valid sizes within the range are thus expressed as\"\n> \n> > + *\n> > + *\twidth = min.width + hStep * x\n> > + *\theight = min.height + vStep * y\n> > + *\n> > + *\tWhere:\n> \n> s/Where:/where/\n> \n> > + *\n> > + *\twidth <= max.width\n> > + *\theight < max.height\n> > + *\n> > + * For SizeRange instances describing a single size the incremental step values\n> > + * are fixed to 1.\n> \n> \"Note that the step values are not equivalent to alignments, as the\n> minimum width or height may not be a multiple of the corresponding\n> step.\n> \n> The step values are guaranteed to be larger than zero. In particular,\n> SizeRange instances that describe a single size have both step values\n> set to 1.\"\n\nThe step values may be zero when the range describes only minimum and\nmaximum sizes without implying that all, or any, intermediate size is\nvalid. SizeRange instances the describe a single size have both set\nvalues set to 1.\n\n> \n> > */\n> > \n> > /**\n> > @@ -213,6 +228,19 @@ bool operator<(const Size &lhs, const Size &rhs)\n> > * \\param[in] maxH The maximum height\n> > */\n> > \n> > +/**\n> > + * \\fn SizeRange::SizeRange(unsigned int minW, unsigned int minH,\n> > + *\t\t\t unsigned int maxW, unsigned int maxH,\n> > + *\t\t\t unsigned int hstep, unsigned int vstep)\n> > + * \\brief Construct an initialized size range\n> \n> Kieran would say s/initialized/initialised/ ;-)\n> \n> With these issues addressed,\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > + * \\param[in] minW The minimum width\n> > + * \\param[in] minH The minimum height\n> > + * \\param[in] maxW The maximum width\n> > + * \\param[in] maxH The maximum height\n> > + * \\param[in] hstep The horizontal step\n> > + * \\param[in] vstep The vertical step\n> > + */\n> > +\n> > /**\n> > * \\var SizeRange::min\n> > * \\brief The minimum size\n> > @@ -223,6 +251,16 @@ bool operator<(const Size &lhs, const Size &rhs)\n> > * \\brief The maximum size\n> > */\n> > \n> > +/**\n> > + * \\var SizeRange::hStep\n> > + * \\brief The horizontal step\n> > + */\n> > +\n> > +/**\n> > + * \\var SizeRange::vStep\n> > + * \\brief The vertical step\n> > + */\n> > +\n> > /**\n> > * \\brief Compare size ranges for equality\n> > * \\return True if the two size ranges are equal, false otherwise","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 B0AF661A27\n\tfor ;\n\tWed, 19 Jun 2019 01:38:06 +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 33DD6D5;\n\tWed, 19 Jun 2019 01:38:06 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560901086;\n\tbh=751Hslfdey5dnI4qNK+1HMYQ003tdnz1AWntp5M0vmY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pxAeEBn+9nOGdkdafRo36TxKg3A0CRO3YUjOwHClC9LbgGxd4tXvmuZlXSLfpqLsj\n\tbgm1Xj7d2J1eCQk1IqE4HvxPm2cfVyl8sekbXuT0RNEBEYPTKpSmJIe+YhpYM31uuE\n\tpFkRk+362B91ECuBTSd2brSWbcVdqLmDsz+cnbtc=","Date":"Wed, 19 Jun 2019 02:37:49 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190618233749.GQ23556@pendragon.ideasonboard.com>","References":"<20190616133402.21934-1-niklas.soderlund@ragnatech.se>\n\t<20190616133402.21934-3-niklas.soderlund@ragnatech.se>\n\t<20190618220931.GH23556@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190618220931.GH23556@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 02/16] libcamera: geometry:\n\tSizeRange: Extend with step information","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:38:06 -0000"}}]