[{"id":22564,"web_url":"https://patchwork.libcamera.org/comment/22564/","msgid":"<76b13f61-cef8-3bef-da3b-7776eb62ce17@ideasonboard.com>","date":"2022-04-05T13:41:15","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn 3/31/22 17:35, Kieran Bingham via libcamera-devel wrote:\n> Provide two new operations to support clamping a Size to a given\n> minimum or maximum Size, or returning a const variant of the same\n> restriction.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n>\n> I was expecting to use this for clamping the block width and height\n> for the AF hardware restrictions on the IPU3 ... but it turns out to be\n> not quite appropriate to use a Size there, as the clamped values are\n> stored in an IPU3 struct directly.\n>\n> However, having made this - I think it is likely to be useful elsewhere\n> so posting so it doesn't get lost.\n>\n> Tests added, so it could be merged already even if there is no current\n> user yet. I expect it's more likely to get used if it exists, than if it\n> doesn't ;-)\n\n\n+1\n\n>\n>\n> include/libcamera/geometry.h | 16 ++++++++++++++++\n> src/libcamera/geometry.cpp | 21 +++++++++++++++++++++\n> test/geometry.cpp | 24 ++++++++++++++++++++++--\n> 3 files changed, 59 insertions(+), 2 deletions(-)\n>\n> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> index 7838b6793617..a447beb55965 100644\n> --- a/include/libcamera/geometry.h\n> +++ b/include/libcamera/geometry.h\n> @@ -93,6 +93,13 @@ public:\n> \t\treturn *this;\n> \t}\n> \n> +\tSize &clamp(const Size &minimum, const Size &maximum)\n> +\t{\n> +\t\twidth = std::clamp(width, minimum.width, maximum.width);\n> +\t\theight = std::clamp(height, minimum.height, maximum.height);\n> +\t\treturn *this;\n> +\t}\n> +\n> \tSize &growBy(const Size &margins)\n> \t{\n> \t\twidth += margins.width;\n> @@ -141,6 +148,15 @@ public:\n> \t\t};\n> \t}\n> \n> +\t__nodiscard constexpr Size clampedTo(const Size &minimum,\n> +\t\t\t\t\t const Size &maximum) const\n> +\t{\n> +\t\treturn {\n> +\t\t\tstd::clamp(width, minimum.width, maximum.width),\n> +\t\t\tstd::clamp(height, minimum.height, maximum.height)\n> +\t\t};\n> +\t}\n> +\n> \t__nodiscard constexpr Size grownBy(const Size &margins) const\n> \t{\n> \t\treturn {\n> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> index cb3c2de18d5e..7ac053a536c1 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -173,6 +173,18 @@ const std::string Size::toString() const\n> * \\return A reference to this object\n> */\n> \n> +/**\n> + * \\fn Size::clamp(const Size &minimum, const Size &maximum)\n> + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> + * \\param[in] minimum The minimum size\n> + * \\param[in] maximum The maximum size\n> + *\n> + * This function restricts the width and height to the constraints of the \\a\n> + * minimum and the \\a maximum sizes given.\n> + *\n> + * \\return A reference to this object\n> + */\n> +\n> /**\n> * \\fn Size::growBy(const Size &margins)\n> * \\brief Grow the size by \\a margins in place\n> @@ -231,6 +243,15 @@ const std::string Size::toString() const\n> * height of this size and the \\a expand size\n> */\n> \n> +/**\n> + * \\fn Size::clampedTo(const Size &minimum, const Size &maximum)\n> + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> + * \\param[in] minimum The minimum size\n> + * \\param[in] maximum The maximum size\n> + * \\return A size whose width and height match this size within the constraints\n> + * of the \\a minimum and \\a maximum sizes given.\n> + */\n> +\n> /**\n> * \\fn Size::grownBy(const Size &margins)\n> * \\brief Grow the size by \\a margins\n> diff --git a/test/geometry.cpp b/test/geometry.cpp\n> index 5125692496b3..1d3d3cad7174 100644\n> --- a/test/geometry.cpp\n> +++ b/test/geometry.cpp\n> @@ -106,7 +106,7 @@ protected:\n> \n> \t\t/*\n> \t\t * Test alignDownTo(), alignUpTo(), boundTo(), expandTo(),\n> -\t\t * growBy() and shrinkBy()\n> +\t\t * clamp() growBy() and shrinkBy()\n> \t\t */\n> \t\tSize s(50, 50);\n> \n> @@ -134,6 +134,18 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> +\t\ts.clamp({ 80, 120 }, { 160, 240 });\n\n\nis it okay to ignore the reference returned by .clamp() ? I think so, \nsince it's doesn't have __nodiscard anyways,\n\nReviewed-by: Umang Jain \n\n> +\t\tif (s != Size(80, 120)) {\n> +\t\t\tcout << \"Size::clamp (minium) test failed\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\ts.clamp({ 20, 30 }, { 50, 50 });\n> +\t\tif (s != Size(50, 50)) {\n> +\t\t\tcout << \"Size::clamp (maximum) test failed\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> \t\ts.growBy({ 10, 20 });\n> \t\tif (s != Size(60, 70)) {\n> \t\t\tcout << \"Size::growBy() test failed\" << endl;\n> @@ -162,7 +174,7 @@ protected:\n> \n> \t\t/*\n> \t\t * Test alignedDownTo(), alignedUpTo(), boundedTo(),\n> -\t\t * expandedTo(), grownBy() and shrunkBy()\n> +\t\t * expandedTo(), clampedTo(), grownBy() and shrunkBy()\n> \t\t */\n> \t\tif (Size(0, 0).alignedDownTo(16, 8) != Size(0, 0) ||\n> \t\t Size(1, 1).alignedDownTo(16, 8) != Size(0, 0) ||\n> @@ -192,6 +204,14 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> +\t\tif (Size(0, 0).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 40) ||\n> +\t\t Size(100, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 90) ||\n> +\t\t Size(30, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 90) ||\n> +\t\t Size(100, 30).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 40)) {\n> +\t\t\tcout << \"Size::clampedTo() test failed\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> \t\tif (Size(0, 0).grownBy({ 10, 20 }) != Size(10, 20) ||\n> \t\t Size(200, 50).grownBy({ 10, 20 }) != Size(210, 70)) {\n> \t\t\tcout << \"Size::grownBy() test failed\" << endl;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A5F55C3256\n\tfor ;\n\tTue, 5 Apr 2022 13:41:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B8436563F;\n\tTue, 5 Apr 2022 15:41:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F1CA2604BB\n\tfor ;\n\tTue, 5 Apr 2022 15:41:21 +0200 (CEST)","from [192.168.1.110] (unknown [27.57.186.178])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 597B75D;\n\tTue, 5 Apr 2022 15:41:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649166084;\n\tbh=r4C/aKEvEn/6gK4oksrVgqvUN46t/2Cir2C1lCBBCM0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=a+eE/LUD+TqOYIl3YYgjDqqvUftszougujHaJdzxEJ3bVM8FLU7serOCDBXWGVV69\n\tY9yqVWj62SGJDsGsmME0imlWaa4oRkvjyiDs0UOSxpxw36QVvVc9Q9P0SS1GABvfT9\n\tLa4jBXzH+3AOB1Vf6xINoq6V+I8s93jHvvwAKF47IBloktdt6OlIuMqJYEFzUJ2FzW\n\tZvf3xgnpwupcqO90+3fRM+P275rJB5WMHlMs/Bt4BppFgPDoZisikcECypKBi2fZG1\n\tWDOVSpy49+kJV0jEJyK3gBJ5u8+leUSp+cZzBlu6PrhYCo1PPT47C0TmtxE+ekxGQX\n\tqpCu4aaYQMrhg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649166081;\n\tbh=r4C/aKEvEn/6gK4oksrVgqvUN46t/2Cir2C1lCBBCM0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=hPSKfW56qrYhV98XiZgmgwwdc35eew8I7vMtLpxA2RxGqoHJsek2r40BjkDHzwTO5\n\tif9lePMweZ1isGB8MWOED+djq4gMYGLL8K7i7V4zRomA21/tlSQOuoPmCUHOosbCXK\n\tWXVvItObtBACZY78l4cG9381i73mRC6whXxfhorU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"hPSKfW56\"; dkim-atps=neutral","Message-ID":"<76b13f61-cef8-3bef-da3b-7776eb62ce17@ideasonboard.com>","Date":"Tue, 5 Apr 2022 19:11:15 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20220331120556.2147745-1-kieran.bingham@ideasonboard.com>","In-Reply-To":"<20220331120556.2147745-1-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22567,"web_url":"https://patchwork.libcamera.org/comment/22567/","msgid":"","date":"2022-04-05T14:18:07","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Apr 05, 2022 at 07:11:15PM +0530, Umang Jain via libcamera-devel wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On 3/31/22 17:35, Kieran Bingham via libcamera-devel wrote:\n> > Provide two new operations to support clamping a Size to a given\n> > minimum or maximum Size, or returning a const variant of the same\n> > restriction.\n\nDid you really mean \"restriction\" here ?\n\n> >\n> > Signed-off-by: Kieran Bingham \n> > ---\n> >\n> > I was expecting to use this for clamping the block width and height\n> > for the AF hardware restrictions on the IPU3 ... but it turns out to be\n> > not quite appropriate to use a Size there, as the clamped values are\n> > stored in an IPU3 struct directly.\n> >\n> > However, having made this - I think it is likely to be useful elsewhere\n> > so posting so it doesn't get lost.\n\n.clamp(min, max) could be implemented as .expandTo(min).shrinkTo(max),\nbut Size::clamp() is likely more explicit and better.\n\n> > Tests added, so it could be merged already even if there is no current\n> > user yet. I expect it's more likely to get used if it exists, than if it\n> > doesn't ;-)\n> \n> +1\n> \n> > include/libcamera/geometry.h | 16 ++++++++++++++++\n> > src/libcamera/geometry.cpp | 21 +++++++++++++++++++++\n> > test/geometry.cpp | 24 ++++++++++++++++++++++--\n> > 3 files changed, 59 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > index 7838b6793617..a447beb55965 100644\n> > --- a/include/libcamera/geometry.h\n> > +++ b/include/libcamera/geometry.h\n> > @@ -93,6 +93,13 @@ public:\n> > \t\treturn *this;\n> > \t}\n> > \n> > +\tSize &clamp(const Size &minimum, const Size &maximum)\n> > +\t{\n> > +\t\twidth = std::clamp(width, minimum.width, maximum.width);\n> > +\t\theight = std::clamp(height, minimum.height, maximum.height);\n> > +\t\treturn *this;\n> > +\t}\n> > +\n> > \tSize &growBy(const Size &margins)\n> > \t{\n> > \t\twidth += margins.width;\n> > @@ -141,6 +148,15 @@ public:\n> > \t\t};\n> > \t}\n> > \n> > +\t__nodiscard constexpr Size clampedTo(const Size &minimum,\n> > +\t\t\t\t\t const Size &maximum) const\n> > +\t{\n> > +\t\treturn {\n> > +\t\t\tstd::clamp(width, minimum.width, maximum.width),\n> > +\t\t\tstd::clamp(height, minimum.height, maximum.height)\n> > +\t\t};\n> > +\t}\n> > +\n> > \t__nodiscard constexpr Size grownBy(const Size &margins) const\n> > \t{\n> > \t\treturn {\n> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > index cb3c2de18d5e..7ac053a536c1 100644\n> > --- a/src/libcamera/geometry.cpp\n> > +++ b/src/libcamera/geometry.cpp\n> > @@ -173,6 +173,18 @@ const std::string Size::toString() const\n> > * \\return A reference to this object\n> > */\n> > \n> > +/**\n> > + * \\fn Size::clamp(const Size &minimum, const Size &maximum)\n> > + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n\n\"Restrict the size to be constrainted\" sounds quite weird to me. Maybe\nusing the word \"clamp\" would be better ?\n\n> > + * \\param[in] minimum The minimum size\n> > + * \\param[in] maximum The maximum size\n> > + *\n> > + * This function restricts the width and height to the constraints of the \\a\n> > + * minimum and the \\a maximum sizes given.\n\nAnd here too, the help text doesn't make it clear what the function\ndoes. I get more information from the function name than from the\ndocumentation :-)\n\nSame for clampedTo().\n\n> > + *\n> > + * \\return A reference to this object\n> > + */\n> > +\n> > /**\n> > * \\fn Size::growBy(const Size &margins)\n> > * \\brief Grow the size by \\a margins in place\n> > @@ -231,6 +243,15 @@ const std::string Size::toString() const\n> > * height of this size and the \\a expand size\n> > */\n> > \n> > +/**\n> > + * \\fn Size::clampedTo(const Size &minimum, const Size &maximum)\n> > + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> > + * \\param[in] minimum The minimum size\n> > + * \\param[in] maximum The maximum size\n> > + * \\return A size whose width and height match this size within the constraints\n> > + * of the \\a minimum and \\a maximum sizes given.\n> > + */\n> > +\n> > /**\n> > * \\fn Size::grownBy(const Size &margins)\n> > * \\brief Grow the size by \\a margins\n> > diff --git a/test/geometry.cpp b/test/geometry.cpp\n> > index 5125692496b3..1d3d3cad7174 100644\n> > --- a/test/geometry.cpp\n> > +++ b/test/geometry.cpp\n> > @@ -106,7 +106,7 @@ protected:\n> > \n> > \t\t/*\n> > \t\t * Test alignDownTo(), alignUpTo(), boundTo(), expandTo(),\n> > -\t\t * growBy() and shrinkBy()\n> > +\t\t * clamp() growBy() and shrinkBy()\n\ns/ growBy/, growBy/\n\n> > \t\t */\n> > \t\tSize s(50, 50);\n> > \n> > @@ -134,6 +134,18 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > \n> > +\t\ts.clamp({ 80, 120 }, { 160, 240 });\n> \n> \n> is it okay to ignore the reference returned by .clamp() ? I think so, \n> since it's doesn't have __nodiscard anyways,\n\nThe __nodiscard attribute was added to the \"-ed\" versions of the\nfunctions, to make sure that someone will not accidentally write\n\n\ts.clampedTo({ 80, 120 }, { 160, 240 });\n\nwhen they meant\n\n\ts.clamp({ 80, 120 }, { 160, 240 });\n\nclamp() is meant to be called with its return value potentially ignored,\notherwise it would be hard to clamp a Size() in-place.\n\n> Reviewed-by: Umang Jain \n> \n> > +\t\tif (s != Size(80, 120)) {\n> > +\t\t\tcout << \"Size::clamp (minium) test failed\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\ts.clamp({ 20, 30 }, { 50, 50 });\n> > +\t\tif (s != Size(50, 50)) {\n> > +\t\t\tcout << \"Size::clamp (maximum) test failed\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > \t\ts.growBy({ 10, 20 });\n> > \t\tif (s != Size(60, 70)) {\n> > \t\t\tcout << \"Size::growBy() test failed\" << endl;\n> > @@ -162,7 +174,7 @@ protected:\n> > \n> > \t\t/*\n> > \t\t * Test alignedDownTo(), alignedUpTo(), boundedTo(),\n> > -\t\t * expandedTo(), grownBy() and shrunkBy()\n> > +\t\t * expandedTo(), clampedTo(), grownBy() and shrunkBy()\n> > \t\t */\n> > \t\tif (Size(0, 0).alignedDownTo(16, 8) != Size(0, 0) ||\n> > \t\t Size(1, 1).alignedDownTo(16, 8) != Size(0, 0) ||\n> > @@ -192,6 +204,14 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > \n> > +\t\tif (Size(0, 0).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 40) ||\n> > +\t\t Size(100, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 90) ||\n> > +\t\t Size(30, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 90) ||\n> > +\t\t Size(100, 30).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 40)) {\n> > +\t\t\tcout << \"Size::clampedTo() test failed\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > \t\tif (Size(0, 0).grownBy({ 10, 20 }) != Size(10, 20) ||\n> > \t\t Size(200, 50).grownBy({ 10, 20 }) != Size(210, 70)) {\n> > \t\t\tcout << \"Size::grownBy() test failed\" << endl;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A8CA2C0F1B\n\tfor ;\n\tTue, 5 Apr 2022 14:18:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E592865642;\n\tTue, 5 Apr 2022 16:18:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FA3F604BB\n\tfor ;\n\tTue, 5 Apr 2022 16:18:11 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D0C4C499;\n\tTue, 5 Apr 2022 16:18:10 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649168292;\n\tbh=cLmloy6BRF5lCnO+QDqw6+qCCHiuJNpO/0WB/b8KLLA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=2Hys35l89KvWKMXMH/E+ayA6ClbFJYIFbpdWQfvBLVnYhGX1cUqwmADhZ4VChTNDa\n\tVBCsrjP18lN1oRBtW9X2O5pSp1ek7jhn6EuwCRKXTz2zjJn+ZbbCA3RaJHILHZS8Ov\n\tvhoVluj1y3C+frQzd7LUfTqftBrHslURzlj3MgWnXmrTwCu6ava8HnvRd8NNEa7ynM\n\tMkjF1lz58uQwk0EA7yrhGWBqWKWJ4p5ndILjvSyg3DpOvmb1lBYhsnGI4dAlewt6z1\n\ttLx4ZoQq+c1Wuc/F203Khq+7TGsqwvtxmNU7KHOiKn9D1OM1ymtC0P+fGW6eSf7vmQ\n\tkt5qP6DPXWIGA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649168290;\n\tbh=cLmloy6BRF5lCnO+QDqw6+qCCHiuJNpO/0WB/b8KLLA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=epcXOvVY+yJtbMfbVUgQaAOCVBHvylOw/VkktsBpP1cJpLA0Mf+kPqKHXlLdIFyMq\n\t+ZThYTaFytA0Bw4f/reTDxmrt/7oNzqlM3TT1liVGBCiPPSWaq7Yh9iPft3t2EHevC\n\tqzb40M34qDHQRdjU2WnuL7X8+eVGouUh73YBAsSo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"epcXOvVY\"; dkim-atps=neutral","Date":"Tue, 5 Apr 2022 17:18:07 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220331120556.2147745-1-kieran.bingham@ideasonboard.com>\n\t<76b13f61-cef8-3bef-da3b-7776eb62ce17@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<76b13f61-cef8-3bef-da3b-7776eb62ce17@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22570,"web_url":"https://patchwork.libcamera.org/comment/22570/","msgid":"<54374d2d-cc8d-d77a-3ea0-0bd7af8d5192@ideasonboard.com>","date":"2022-04-05T14:24:35","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nOn 4/5/22 19:48, Laurent Pinchart wrote:\n> On Tue, Apr 05, 2022 at 07:11:15PM +0530, Umang Jain via libcamera-devel wrote:\n>> Hi Kieran,\n>>\n>> Thank you for the patch.\n>>\n>> On 3/31/22 17:35, Kieran Bingham via libcamera-devel wrote:\n>>> Provide two new operations to support clamping a Size to a given\n>>> minimum or maximum Size, or returning a const variant of the same\n>>> restriction.\n> Did you really mean \"restriction\" here ?\n>\n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>>\n>>> I was expecting to use this for clamping the block width and height\n>>> for the AF hardware restrictions on the IPU3 ... but it turns out to be\n>>> not quite appropriate to use a Size there, as the clamped values are\n>>> stored in an IPU3 struct directly.\n>>>\n>>> However, having made this - I think it is likely to be useful elsewhere\n>>> so posting so it doesn't get lost.\n> .clamp(min, max) could be implemented as .expandTo(min).shrinkTo(max),\n> but Size::clamp() is likely more explicit and better.\n>\n>>> Tests added, so it could be merged already even if there is no current\n>>> user yet. I expect it's more likely to get used if it exists, than if it\n>>> doesn't ;-)\n>> +1\n>>\n>>> include/libcamera/geometry.h | 16 ++++++++++++++++\n>>> src/libcamera/geometry.cpp | 21 +++++++++++++++++++++\n>>> test/geometry.cpp | 24 ++++++++++++++++++++++--\n>>> 3 files changed, 59 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n>>> index 7838b6793617..a447beb55965 100644\n>>> --- a/include/libcamera/geometry.h\n>>> +++ b/include/libcamera/geometry.h\n>>> @@ -93,6 +93,13 @@ public:\n>>> \t\treturn *this;\n>>> \t}\n>>> \n>>> +\tSize &clamp(const Size &minimum, const Size &maximum)\n>>> +\t{\n>>> +\t\twidth = std::clamp(width, minimum.width, maximum.width);\n>>> +\t\theight = std::clamp(height, minimum.height, maximum.height);\n>>> +\t\treturn *this;\n>>> +\t}\n>>> +\n>>> \tSize &growBy(const Size &margins)\n>>> \t{\n>>> \t\twidth += margins.width;\n>>> @@ -141,6 +148,15 @@ public:\n>>> \t\t};\n>>> \t}\n>>> \n>>> +\t__nodiscard constexpr Size clampedTo(const Size &minimum,\n>>> +\t\t\t\t\t const Size &maximum) const\n>>> +\t{\n>>> +\t\treturn {\n>>> +\t\t\tstd::clamp(width, minimum.width, maximum.width),\n>>> +\t\t\tstd::clamp(height, minimum.height, maximum.height)\n>>> +\t\t};\n>>> +\t}\n>>> +\n>>> \t__nodiscard constexpr Size grownBy(const Size &margins) const\n>>> \t{\n>>> \t\treturn {\n>>> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n>>> index cb3c2de18d5e..7ac053a536c1 100644\n>>> --- a/src/libcamera/geometry.cpp\n>>> +++ b/src/libcamera/geometry.cpp\n>>> @@ -173,6 +173,18 @@ const std::string Size::toString() const\n>>> * \\return A reference to this object\n>>> */\n>>> \n>>> +/**\n>>> + * \\fn Size::clamp(const Size &minimum, const Size &maximum)\n>>> + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> \"Restrict the size to be constrainted\" sounds quite weird to me. Maybe\n> using the word \"clamp\" would be better ?\n\n\nNow that I am reading it, I agree with Laurent :S\n\n>\n>>> + * \\param[in] minimum The minimum size\n>>> + * \\param[in] maximum The maximum size\n>>> + *\n>>> + * This function restricts the width and height to the constraints of the \\a\n>>> + * minimum and the \\a maximum sizes given.\n> And here too, the help text doesn't make it clear what the function\n> does. I get more information from the function name than from the\n> documentation :-)\n>\n> Same for clampedTo().\n>\n>>> + *\n>>> + * \\return A reference to this object\n>>> + */\n>>> +\n>>> /**\n>>> * \\fn Size::growBy(const Size &margins)\n>>> * \\brief Grow the size by \\a margins in place\n>>> @@ -231,6 +243,15 @@ const std::string Size::toString() const\n>>> * height of this size and the \\a expand size\n>>> */\n>>> \n>>> +/**\n>>> + * \\fn Size::clampedTo(const Size &minimum, const Size &maximum)\n>>> + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n>>> + * \\param[in] minimum The minimum size\n>>> + * \\param[in] maximum The maximum size\n>>> + * \\return A size whose width and height match this size within the constraints\n>>> + * of the \\a minimum and \\a maximum sizes given.\n>>> + */\n>>> +\n>>> /**\n>>> * \\fn Size::grownBy(const Size &margins)\n>>> * \\brief Grow the size by \\a margins\n>>> diff --git a/test/geometry.cpp b/test/geometry.cpp\n>>> index 5125692496b3..1d3d3cad7174 100644\n>>> --- a/test/geometry.cpp\n>>> +++ b/test/geometry.cpp\n>>> @@ -106,7 +106,7 @@ protected:\n>>> \n>>> \t\t/*\n>>> \t\t * Test alignDownTo(), alignUpTo(), boundTo(), expandTo(),\n>>> -\t\t * growBy() and shrinkBy()\n>>> +\t\t * clamp() growBy() and shrinkBy()\n> s/ growBy/, growBy/\n>\n>>> \t\t */\n>>> \t\tSize s(50, 50);\n>>> \n>>> @@ -134,6 +134,18 @@ protected:\n>>> \t\t\treturn TestFail;\n>>> \t\t}\n>>> \n>>> +\t\ts.clamp({ 80, 120 }, { 160, 240 });\n>>\n>> is it okay to ignore the reference returned by .clamp() ? I think so,\n>> since it's doesn't have __nodiscard anyways,\n> The __nodiscard attribute was added to the \"-ed\" versions of the\n> functions, to make sure that someone will not accidentally write\n>\n> \ts.clampedTo({ 80, 120 }, { 160, 240 });\n>\n> when they meant\n>\n> \ts.clamp({ 80, 120 }, { 160, 240 });\n\n\nMake sense.\n\n>\n> clamp() is meant to be called with its return value potentially ignored,\n> otherwise it would be hard to clamp a Size() in-place.\n\n\nYeah, I wasn't sure if the complier warns out(or not) when we are \nignoring the returned references. Yes, the in-place op. makes and I have \nwritten them a numerous times (with ignoring the reference), but \nsomething I didn't give a thought about, until now.\n\n>\n>> Reviewed-by: Umang Jain \n>>\n>>> +\t\tif (s != Size(80, 120)) {\n>>> +\t\t\tcout << \"Size::clamp (minium) test failed\" << endl;\n>>> +\t\t\treturn TestFail;\n>>> +\t\t}\n>>> +\n>>> +\t\ts.clamp({ 20, 30 }, { 50, 50 });\n>>> +\t\tif (s != Size(50, 50)) {\n>>> +\t\t\tcout << \"Size::clamp (maximum) test failed\" << endl;\n>>> +\t\t\treturn TestFail;\n>>> +\t\t}\n>>> +\n>>> \t\ts.growBy({ 10, 20 });\n>>> \t\tif (s != Size(60, 70)) {\n>>> \t\t\tcout << \"Size::growBy() test failed\" << endl;\n>>> @@ -162,7 +174,7 @@ protected:\n>>> \n>>> \t\t/*\n>>> \t\t * Test alignedDownTo(), alignedUpTo(), boundedTo(),\n>>> -\t\t * expandedTo(), grownBy() and shrunkBy()\n>>> +\t\t * expandedTo(), clampedTo(), grownBy() and shrunkBy()\n>>> \t\t */\n>>> \t\tif (Size(0, 0).alignedDownTo(16, 8) != Size(0, 0) ||\n>>> \t\t Size(1, 1).alignedDownTo(16, 8) != Size(0, 0) ||\n>>> @@ -192,6 +204,14 @@ protected:\n>>> \t\t\treturn TestFail;\n>>> \t\t}\n>>> \n>>> +\t\tif (Size(0, 0).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 40) ||\n>>> +\t\t Size(100, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 90) ||\n>>> +\t\t Size(30, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 90) ||\n>>> +\t\t Size(100, 30).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 40)) {\n>>> +\t\t\tcout << \"Size::clampedTo() test failed\" << endl;\n>>> +\t\t\treturn TestFail;\n>>> +\t\t}\n>>> +\n>>> \t\tif (Size(0, 0).grownBy({ 10, 20 }) != Size(10, 20) ||\n>>> \t\t Size(200, 50).grownBy({ 10, 20 }) != Size(210, 70)) {\n>>> \t\t\tcout << \"Size::grownBy() test failed\" << endl;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B04F8C3256\n\tfor ;\n\tTue, 5 Apr 2022 14:24:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E70B65642;\n\tTue, 5 Apr 2022 16:24:42 +0200 (CEST)","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 7E245604BB\n\tfor ;\n\tTue, 5 Apr 2022 16:24:40 +0200 (CEST)","from [192.168.1.110] (unknown [27.57.186.178])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4702F5D;\n\tTue, 5 Apr 2022 16:24:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649168682;\n\tbh=tXxSJAV+/XdOP2SMkYasT5u7oxDBNNPkZzJD3lowfe0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=4at4NlmVnn93U881+RR9NfC8P6tVM09xNnx52pSQaeFd3AlgFj1S4J4KOtbYhK/31\n\tSL221kf3/k2TBqdzEr2vk2PFohtj6AHt8KE7vYlpGe9xjTX3ox0E0HdPhrJ9WxwORw\n\tEnKCfSrTtKut7+afr+/BygQ6L76FUPA75G5shFk+XWCfAxz+LVacpWXUvGdNXUo/X9\n\tL743SdS9cXq4luPc0bNMkiYHfGswgybedKocCWdcV0yg3iBu60dtyUrhWg14Xuoli2\n\tu06tAu0zvXNIFCItMMYrjBFkdU5TV8yadu0c2MIcLRSZ68BRYzdMRBAoMWv56SDBYD\n\tIjNC21xwHR+MQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649168680;\n\tbh=tXxSJAV+/XdOP2SMkYasT5u7oxDBNNPkZzJD3lowfe0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ar7sR5vBUNm2JO92jgzhfVm7vD0sC8hL9/D62oPV9VnhqIcifcfFBNMQRZNap0vBx\n\tBRHwPz0KOshCPkAge49GJmyZYOHr2szgZNLegIBHko14s6pD9PHAS/7bKUcgQGyi7s\n\tmDsWH7e6bjc7tRXKt7aN9RJX32dAlHJMY8MLa8uA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ar7sR5vB\"; dkim-atps=neutral","Message-ID":"<54374d2d-cc8d-d77a-3ea0-0bd7af8d5192@ideasonboard.com>","Date":"Tue, 5 Apr 2022 19:54:35 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220331120556.2147745-1-kieran.bingham@ideasonboard.com>\n\t<76b13f61-cef8-3bef-da3b-7776eb62ce17@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22647,"web_url":"https://patchwork.libcamera.org/comment/22647/","msgid":"<164934233618.3968198.9096792656692370437@Monstersaurus>","date":"2022-04-07T14:38:56","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-04-05 15:18:07)\n> On Tue, Apr 05, 2022 at 07:11:15PM +0530, Umang Jain via libcamera-devel wrote:\n> > Hi Kieran,\n> > \n> > Thank you for the patch.\n> > \n> > On 3/31/22 17:35, Kieran Bingham via libcamera-devel wrote:\n> > > Provide two new operations to support clamping a Size to a given\n> > > minimum or maximum Size, or returning a const variant of the same\n> > > restriction.\n> \n> Did you really mean \"restriction\" here ?\n\nI did yes. I mean the same restriction of the minimum and maximum Size\nbut as a const.\n\n\n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > >\n> > > I was expecting to use this for clamping the block width and height\n> > > for the AF hardware restrictions on the IPU3 ... but it turns out to be\n> > > not quite appropriate to use a Size there, as the clamped values are\n> > > stored in an IPU3 struct directly.\n> > >\n> > > However, having made this - I think it is likely to be useful elsewhere\n> > > so posting so it doesn't get lost.\n> \n> .clamp(min, max) could be implemented as .expandTo(min).shrinkTo(max),\n> but Size::clamp() is likely more explicit and better.\n\nYes, a caller could call .expandTo.shrinkTo instead already but I don't\nthink that should be the implementation detail here.\n\n> \n> > > Tests added, so it could be merged already even if there is no current\n> > > user yet. I expect it's more likely to get used if it exists, than if it\n> > > doesn't ;-)\n> > \n> > +1\n> > \n> > > include/libcamera/geometry.h | 16 ++++++++++++++++\n> > > src/libcamera/geometry.cpp | 21 +++++++++++++++++++++\n> > > test/geometry.cpp | 24 ++++++++++++++++++++++--\n> > > 3 files changed, 59 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > index 7838b6793617..a447beb55965 100644\n> > > --- a/include/libcamera/geometry.h\n> > > +++ b/include/libcamera/geometry.h\n> > > @@ -93,6 +93,13 @@ public:\n> > > return *this;\n> > > }\n> > > \n> > > + Size &clamp(const Size &minimum, const Size &maximum)\n> > > + {\n> > > + width = std::clamp(width, minimum.width, maximum.width);\n> > > + height = std::clamp(height, minimum.height, maximum.height);\n> > > + return *this;\n> > > + }\n> > > +\n> > > Size &growBy(const Size &margins)\n> > > {\n> > > width += margins.width;\n> > > @@ -141,6 +148,15 @@ public:\n> > > };\n> > > }\n> > > \n> > > + __nodiscard constexpr Size clampedTo(const Size &minimum,\n> > > + const Size &maximum) const\n> > > + {\n> > > + return {\n> > > + std::clamp(width, minimum.width, maximum.width),\n> > > + std::clamp(height, minimum.height, maximum.height)\n> > > + };\n> > > + }\n> > > +\n> > > __nodiscard constexpr Size grownBy(const Size &margins) const\n> > > {\n> > > return {\n> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > index cb3c2de18d5e..7ac053a536c1 100644\n> > > --- a/src/libcamera/geometry.cpp\n> > > +++ b/src/libcamera/geometry.cpp\n> > > @@ -173,6 +173,18 @@ const std::string Size::toString() const\n> > > * \\return A reference to this object\n> > > */\n> > > \n> > > +/**\n> > > + * \\fn Size::clamp(const Size &minimum, const Size &maximum)\n> > > + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> \n> \"Restrict the size to be constrainted\" sounds quite weird to me. Maybe\n> using the word \"clamp\" would be better ?\n\nI was trying to avoid using the term I'm describing to describe itself\nin the description.\n\nRe-reading now it still sounds fine to me, but I can change it.\n\nDo you mean you prefer:\n\nClamp the size to be constrained within the minimum and maximum\n\nor\n\nRestrict the size to be clamped within the minimum and maximum\n\n\n> \n> > > + * \\param[in] minimum The minimum size\n> > > + * \\param[in] maximum The maximum size\n> > > + *\n> > > + * This function restricts the width and height to the constraints of the \\a\n> > > + * minimum and the \\a maximum sizes given.\n> \n> And here too, the help text doesn't make it clear what the function\n> does. I get more information from the function name than from the\n> documentation :-)\n\nIs it because of the word 'constraints' as you mentioned above? or\nsomething else?\n\nDocumenting a function called 'clamp' as 'it will clamp the values'\ndoesn't add any value either.\n\n\n> \n> Same for clampedTo().\n> \n> > > + *\n> > > + * \\return A reference to this object\n> > > + */\n> > > +\n> > > /**\n> > > * \\fn Size::growBy(const Size &margins)\n> > > * \\brief Grow the size by \\a margins in place\n> > > @@ -231,6 +243,15 @@ const std::string Size::toString() const\n> > > * height of this size and the \\a expand size\n> > > */\n> > > \n> > > +/**\n> > > + * \\fn Size::clampedTo(const Size &minimum, const Size &maximum)\n> > > + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> > > + * \\param[in] minimum The minimum size\n> > > + * \\param[in] maximum The maximum size\n> > > + * \\return A size whose width and height match this size within the constraints\n> > > + * of the \\a minimum and \\a maximum sizes given.\n> > > + */\n> > > +\n> > > /**\n> > > * \\fn Size::grownBy(const Size &margins)\n> > > * \\brief Grow the size by \\a margins\n> > > diff --git a/test/geometry.cpp b/test/geometry.cpp\n> > > index 5125692496b3..1d3d3cad7174 100644\n> > > --- a/test/geometry.cpp\n> > > +++ b/test/geometry.cpp\n> > > @@ -106,7 +106,7 @@ protected:\n> > > \n> > > /*\n> > > * Test alignDownTo(), alignUpTo(), boundTo(), expandTo(),\n> > > - * growBy() and shrinkBy()\n> > > + * clamp() growBy() and shrinkBy()\n> \n> s/ growBy/, growBy/\n\nAck.\n\n\n> \n> > > */\n> > > Size s(50, 50);\n> > > \n> > > @@ -134,6 +134,18 @@ protected:\n> > > return TestFail;\n> > > }\n> > > \n> > > + s.clamp({ 80, 120 }, { 160, 240 });\n> > \n> > \n> > is it okay to ignore the reference returned by .clamp() ? I think so, \n> > since it's doesn't have __nodiscard anyways,\n> \n> The __nodiscard attribute was added to the \"-ed\" versions of the\n> functions, to make sure that someone will not accidentally write\n> \n> s.clampedTo({ 80, 120 }, { 160, 240 });\n> \n> when they meant\n> \n> s.clamp({ 80, 120 }, { 160, 240 });\n> \n> clamp() is meant to be called with its return value potentially ignored,\n> otherwise it would be hard to clamp a Size() in-place.\n> \n> > Reviewed-by: Umang Jain \n> > \n> > > + if (s != Size(80, 120)) {\n> > > + cout << \"Size::clamp (minium) test failed\" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + s.clamp({ 20, 30 }, { 50, 50 });\n> > > + if (s != Size(50, 50)) {\n> > > + cout << \"Size::clamp (maximum) test failed\" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > s.growBy({ 10, 20 });\n> > > if (s != Size(60, 70)) {\n> > > cout << \"Size::growBy() test failed\" << endl;\n> > > @@ -162,7 +174,7 @@ protected:\n> > > \n> > > /*\n> > > * Test alignedDownTo(), alignedUpTo(), boundedTo(),\n> > > - * expandedTo(), grownBy() and shrunkBy()\n> > > + * expandedTo(), clampedTo(), grownBy() and shrunkBy()\n> > > */\n> > > if (Size(0, 0).alignedDownTo(16, 8) != Size(0, 0) ||\n> > > Size(1, 1).alignedDownTo(16, 8) != Size(0, 0) ||\n> > > @@ -192,6 +204,14 @@ protected:\n> > > return TestFail;\n> > > }\n> > > \n> > > + if (Size(0, 0).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 40) ||\n> > > + Size(100, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 90) ||\n> > > + Size(30, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 90) ||\n> > > + Size(100, 30).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 40)) {\n> > > + cout << \"Size::clampedTo() test failed\" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > if (Size(0, 0).grownBy({ 10, 20 }) != Size(10, 20) ||\n> > > Size(200, 50).grownBy({ 10, 20 }) != Size(210, 70)) {\n> > > cout << \"Size::grownBy() test failed\" << endl;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 10483C0F1B\n\tfor ;\n\tThu, 7 Apr 2022 14:39:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42DFC65644;\n\tThu, 7 Apr 2022 16:39:00 +0200 (CEST)","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 CD8026563F\n\tfor ;\n\tThu, 7 Apr 2022 16:38:58 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3B1A2499;\n\tThu, 7 Apr 2022 16:38:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649342340;\n\tbh=45oJ/0A2qwvDFkC503tj0Mt1mDiHP0mtle2ty8Izrhg=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=z0BSkEen9BG5Du4MFxao4vKqU+Rk0Ghm6UsbetkJGJoG2Et3kt2Ynm56ZyC3lFGf7\n\tmVSYUWbrVTjLmCQ2sqn9Uv/Sol2w08bd5UiXb/A6bf/AgeW2vcf0P1BkuhR0OlnRFy\n\tmTKz1oIy1Ms0ZApgVSOzQgz5vUpC7pyR2JbmRfCTx/NAfT3irwMCzZGZGvWFqPGi2Q\n\tz8cEwRUZfnJGvHw0e7rybIsFeS0y/k2oGUkk4do01G5fkUREMu9fMH+Gb9DR26uAy9\n\tyxlxZ9B+GKAsnapHMj39pFlcT23a6LeMLS9upLYPR4oz1lnTyYpwo04E5FVQ0QLwBX\n\tpaWrTBx3hboOA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649342338;\n\tbh=45oJ/0A2qwvDFkC503tj0Mt1mDiHP0mtle2ty8Izrhg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=DsmmIpFYtOP7Fn592hP9ji2dcaVBnIkaV6xJroBc4pO9zeHXSynbr02RyXuA+xTxP\n\ti7Ab8cdbBXXCTWXNpAWm0gJeNTjr3OPX1sVq4YytNMqojwTCPQzvlUxGH+gJPzy2pE\n\tCPo0bvx47ZWa+CY+ROOWa/pYMq5XZ3Z4KgR3dpg0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"DsmmIpFY\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220331120556.2147745-1-kieran.bingham@ideasonboard.com>\n\t<76b13f61-cef8-3bef-da3b-7776eb62ce17@ideasonboard.com>\n\t","To":"Laurent Pinchart ,\n\tUmang Jain ","Date":"Thu, 07 Apr 2022 15:38:56 +0100","Message-ID":"<164934233618.3968198.9096792656692370437@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22648,"web_url":"https://patchwork.libcamera.org/comment/22648/","msgid":"","date":"2022-04-07T14:58:09","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Apr 07, 2022 at 03:38:56PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2022-04-05 15:18:07)\n> > On Tue, Apr 05, 2022 at 07:11:15PM +0530, Umang Jain via libcamera-devel wrote:\n> > > Hi Kieran,\n> > > \n> > > Thank you for the patch.\n> > > \n> > > On 3/31/22 17:35, Kieran Bingham via libcamera-devel wrote:\n> > > > Provide two new operations to support clamping a Size to a given\n> > > > minimum or maximum Size, or returning a const variant of the same\n> > > > restriction.\n> > \n> > Did you really mean \"restriction\" here ?\n> \n> I did yes. I mean the same restriction of the minimum and maximum Size\n> but as a const.\n> \n> > > > Signed-off-by: Kieran Bingham \n> > > > ---\n> > > >\n> > > > I was expecting to use this for clamping the block width and height\n> > > > for the AF hardware restrictions on the IPU3 ... but it turns out to be\n> > > > not quite appropriate to use a Size there, as the clamped values are\n> > > > stored in an IPU3 struct directly.\n> > > >\n> > > > However, having made this - I think it is likely to be useful elsewhere\n> > > > so posting so it doesn't get lost.\n> > \n> > .clamp(min, max) could be implemented as .expandTo(min).shrinkTo(max),\n> > but Size::clamp() is likely more explicit and better.\n> \n> Yes, a caller could call .expandTo.shrinkTo instead already but I don't\n> think that should be the implementation detail here.\n> \n> > > > Tests added, so it could be merged already even if there is no current\n> > > > user yet. I expect it's more likely to get used if it exists, than if it\n> > > > doesn't ;-)\n> > > \n> > > +1\n> > > \n> > > > include/libcamera/geometry.h | 16 ++++++++++++++++\n> > > > src/libcamera/geometry.cpp | 21 +++++++++++++++++++++\n> > > > test/geometry.cpp | 24 ++++++++++++++++++++++--\n> > > > 3 files changed, 59 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > > index 7838b6793617..a447beb55965 100644\n> > > > --- a/include/libcamera/geometry.h\n> > > > +++ b/include/libcamera/geometry.h\n> > > > @@ -93,6 +93,13 @@ public:\n> > > > return *this;\n> > > > }\n> > > > \n> > > > + Size &clamp(const Size &minimum, const Size &maximum)\n> > > > + {\n> > > > + width = std::clamp(width, minimum.width, maximum.width);\n> > > > + height = std::clamp(height, minimum.height, maximum.height);\n> > > > + return *this;\n> > > > + }\n> > > > +\n> > > > Size &growBy(const Size &margins)\n> > > > {\n> > > > width += margins.width;\n> > > > @@ -141,6 +148,15 @@ public:\n> > > > };\n> > > > }\n> > > > \n> > > > + __nodiscard constexpr Size clampedTo(const Size &minimum,\n> > > > + const Size &maximum) const\n> > > > + {\n> > > > + return {\n> > > > + std::clamp(width, minimum.width, maximum.width),\n> > > > + std::clamp(height, minimum.height, maximum.height)\n> > > > + };\n> > > > + }\n> > > > +\n> > > > __nodiscard constexpr Size grownBy(const Size &margins) const\n> > > > {\n> > > > return {\n> > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > > index cb3c2de18d5e..7ac053a536c1 100644\n> > > > --- a/src/libcamera/geometry.cpp\n> > > > +++ b/src/libcamera/geometry.cpp\n> > > > @@ -173,6 +173,18 @@ const std::string Size::toString() const\n> > > > * \\return A reference to this object\n> > > > */\n> > > > \n> > > > +/**\n> > > > + * \\fn Size::clamp(const Size &minimum, const Size &maximum)\n> > > > + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> > \n> > \"Restrict the size to be constrainted\" sounds quite weird to me. Maybe\n> > using the word \"clamp\" would be better ?\n> \n> I was trying to avoid using the term I'm describing to describe itself\n> in the description.\n\nThe fact that the function is named from the term that best describes\nits purpose means we picked the right name :-)\n\n> Re-reading now it still sounds fine to me, but I can change it.\n> \n> Do you mean you prefer:\n> \n> Clamp the size to be constrained within the minimum and maximum\n\nI meant this, yes. Or maybe\n\nClamp the size to the \\a minimum and \\a maximum values\n\n(with s/to/between/, and/or adding the word \"range\" somewhere, if\ndesired)\n\n> or\n> \n> Restrict the size to be clamped within the minimum and maximum\n> \n> > > > + * \\param[in] minimum The minimum size\n> > > > + * \\param[in] maximum The maximum size\n> > > > + *\n> > > > + * This function restricts the width and height to the constraints of the \\a\n> > > > + * minimum and the \\a maximum sizes given.\n> > \n> > And here too, the help text doesn't make it clear what the function\n> > does. I get more information from the function name than from the\n> > documentation :-)\n> \n> Is it because of the word 'constraints' as you mentioned above? or\n> something else?\n\nYes, it's \"restricts to the constraints\" that sounds convoluted to me.\n\n> Documenting a function called 'clamp' as 'it will clamp the values'\n> doesn't add any value either.\n> \n> > Same for clampedTo().\n> > \n> > > > + *\n> > > > + * \\return A reference to this object\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\fn Size::growBy(const Size &margins)\n> > > > * \\brief Grow the size by \\a margins in place\n> > > > @@ -231,6 +243,15 @@ const std::string Size::toString() const\n> > > > * height of this size and the \\a expand size\n> > > > */\n> > > > \n> > > > +/**\n> > > > + * \\fn Size::clampedTo(const Size &minimum, const Size &maximum)\n> > > > + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> > > > + * \\param[in] minimum The minimum size\n> > > > + * \\param[in] maximum The maximum size\n> > > > + * \\return A size whose width and height match this size within the constraints\n> > > > + * of the \\a minimum and \\a maximum sizes given.\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\fn Size::grownBy(const Size &margins)\n> > > > * \\brief Grow the size by \\a margins\n> > > > diff --git a/test/geometry.cpp b/test/geometry.cpp\n> > > > index 5125692496b3..1d3d3cad7174 100644\n> > > > --- a/test/geometry.cpp\n> > > > +++ b/test/geometry.cpp\n> > > > @@ -106,7 +106,7 @@ protected:\n> > > > \n> > > > /*\n> > > > * Test alignDownTo(), alignUpTo(), boundTo(), expandTo(),\n> > > > - * growBy() and shrinkBy()\n> > > > + * clamp() growBy() and shrinkBy()\n> > \n> > s/ growBy/, growBy/\n> \n> Ack.\n> \n> > > > */\n> > > > Size s(50, 50);\n> > > > \n> > > > @@ -134,6 +134,18 @@ protected:\n> > > > return TestFail;\n> > > > }\n> > > > \n> > > > + s.clamp({ 80, 120 }, { 160, 240 });\n> > > \n> > > \n> > > is it okay to ignore the reference returned by .clamp() ? I think so, \n> > > since it's doesn't have __nodiscard anyways,\n> > \n> > The __nodiscard attribute was added to the \"-ed\" versions of the\n> > functions, to make sure that someone will not accidentally write\n> > \n> > s.clampedTo({ 80, 120 }, { 160, 240 });\n> > \n> > when they meant\n> > \n> > s.clamp({ 80, 120 }, { 160, 240 });\n> > \n> > clamp() is meant to be called with its return value potentially ignored,\n> > otherwise it would be hard to clamp a Size() in-place.\n> > \n> > > Reviewed-by: Umang Jain \n> > > \n> > > > + if (s != Size(80, 120)) {\n> > > > + cout << \"Size::clamp (minium) test failed\" << endl;\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + s.clamp({ 20, 30 }, { 50, 50 });\n> > > > + if (s != Size(50, 50)) {\n> > > > + cout << \"Size::clamp (maximum) test failed\" << endl;\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > s.growBy({ 10, 20 });\n> > > > if (s != Size(60, 70)) {\n> > > > cout << \"Size::growBy() test failed\" << endl;\n> > > > @@ -162,7 +174,7 @@ protected:\n> > > > \n> > > > /*\n> > > > * Test alignedDownTo(), alignedUpTo(), boundedTo(),\n> > > > - * expandedTo(), grownBy() and shrunkBy()\n> > > > + * expandedTo(), clampedTo(), grownBy() and shrunkBy()\n> > > > */\n> > > > if (Size(0, 0).alignedDownTo(16, 8) != Size(0, 0) ||\n> > > > Size(1, 1).alignedDownTo(16, 8) != Size(0, 0) ||\n> > > > @@ -192,6 +204,14 @@ protected:\n> > > > return TestFail;\n> > > > }\n> > > > \n> > > > + if (Size(0, 0).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 40) ||\n> > > > + Size(100, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 90) ||\n> > > > + Size(30, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 90) ||\n> > > > + Size(100, 30).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 40)) {\n> > > > + cout << \"Size::clampedTo() test failed\" << endl;\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > if (Size(0, 0).grownBy({ 10, 20 }) != Size(10, 20) ||\n> > > > Size(200, 50).grownBy({ 10, 20 }) != Size(210, 70)) {\n> > > > cout << \"Size::grownBy() test failed\" << endl;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5D720C3256\n\tfor ;\n\tThu, 7 Apr 2022 14:58:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B54D565642;\n\tThu, 7 Apr 2022 16:58:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B05736563F\n\tfor ;\n\tThu, 7 Apr 2022 16:58:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 28DCF499;\n\tThu, 7 Apr 2022 16:58:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649343494;\n\tbh=K0gL3aOX8ARd2rHArMdpuZC9XBC2PCRsJ8YmenkfL5w=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Xs6NQbbTHVtmmwtH35502XQJJIkGW84SuzCZl6xGlHpL3t48zLylZDCOJBlOAMBRy\n\tRqrg7+TlqPA+VGM5XQh5bS1Ah/n0Db6JUdGWPOsMAHB9//A4TBqMEYrbvPm6KFrk01\n\tIrbjlVnQXR5nMUViIEouvE0L7tVBPnPSvjHOcYnPnOmmFBYxo1GpwMOeAR2GnPq0q1\n\tvRMLBl4pDNWP+DkO1Ibkj39V1ifudLb4fXw/P9baSn0hsgD2lcjbeYlkpWp5jT9Ejx\n\tfk44HHzZzFPbWNUHPRTttP50toAC9Gg1WViMQ/H4NdjX9wirxgJijk6FeK3RanhC/I\n\ti3O8W7H/B8cEA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649343493;\n\tbh=K0gL3aOX8ARd2rHArMdpuZC9XBC2PCRsJ8YmenkfL5w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YXsBJYy4S6Iy2ln8pX7aG3eZKNImqsjIh100yg0SXyVvlQTr5J3Eb19K6SC4KRkn1\n\tMQrfkJn6Cn9GasODTvv875TSw6ctw+ANoBYnBnGktJMciA622escvwZPwV0VPVAS1z\n\tNdehkwvIJuB0hW1Iq8Md3BvZK7zBoq8UBVCIaHu4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YXsBJYy4\"; dkim-atps=neutral","Date":"Thu, 7 Apr 2022 17:58:09 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220331120556.2147745-1-kieran.bingham@ideasonboard.com>\n\t<76b13f61-cef8-3bef-da3b-7776eb62ce17@ideasonboard.com>\n\t\n\t<164934233618.3968198.9096792656692370437@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164934233618.3968198.9096792656692370437@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22649,"web_url":"https://patchwork.libcamera.org/comment/22649/","msgid":"<164936806035.117583.2740008354778763383@Monstersaurus>","date":"2022-04-07T21:47:40","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-04-07 15:58:09)\n> Hi Kieran,\n> \n> On Thu, Apr 07, 2022 at 03:38:56PM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2022-04-05 15:18:07)\n> > > On Tue, Apr 05, 2022 at 07:11:15PM +0530, Umang Jain via libcamera-devel wrote:\n> > > > Hi Kieran,\n> > > > \n> > > > Thank you for the patch.\n> > > > \n> > > > On 3/31/22 17:35, Kieran Bingham via libcamera-devel wrote:\n> > > > > Provide two new operations to support clamping a Size to a given\n> > > > > minimum or maximum Size, or returning a const variant of the same\n> > > > > restriction.\n> > > \n> > > Did you really mean \"restriction\" here ?\n> > \n> > I did yes. I mean the same restriction of the minimum and maximum Size\n> > but as a const.\n> > \n> > > > > Signed-off-by: Kieran Bingham \n> > > > > ---\n> > > > >\n> > > > > I was expecting to use this for clamping the block width and height\n> > > > > for the AF hardware restrictions on the IPU3 ... but it turns out to be\n> > > > > not quite appropriate to use a Size there, as the clamped values are\n> > > > > stored in an IPU3 struct directly.\n> > > > >\n> > > > > However, having made this - I think it is likely to be useful elsewhere\n> > > > > so posting so it doesn't get lost.\n> > > \n> > > .clamp(min, max) could be implemented as .expandTo(min).shrinkTo(max),\n> > > but Size::clamp() is likely more explicit and better.\n> > \n> > Yes, a caller could call .expandTo.shrinkTo instead already but I don't\n> > think that should be the implementation detail here.\n> > \n> > > > > Tests added, so it could be merged already even if there is no current\n> > > > > user yet. I expect it's more likely to get used if it exists, than if it\n> > > > > doesn't ;-)\n> > > > \n> > > > +1\n> > > > \n> > > > > include/libcamera/geometry.h | 16 ++++++++++++++++\n> > > > > src/libcamera/geometry.cpp | 21 +++++++++++++++++++++\n> > > > > test/geometry.cpp | 24 ++++++++++++++++++++++--\n> > > > > 3 files changed, 59 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > > > index 7838b6793617..a447beb55965 100644\n> > > > > --- a/include/libcamera/geometry.h\n> > > > > +++ b/include/libcamera/geometry.h\n> > > > > @@ -93,6 +93,13 @@ public:\n> > > > > return *this;\n> > > > > }\n> > > > > \n> > > > > + Size &clamp(const Size &minimum, const Size &maximum)\n> > > > > + {\n> > > > > + width = std::clamp(width, minimum.width, maximum.width);\n> > > > > + height = std::clamp(height, minimum.height, maximum.height);\n> > > > > + return *this;\n> > > > > + }\n> > > > > +\n> > > > > Size &growBy(const Size &margins)\n> > > > > {\n> > > > > width += margins.width;\n> > > > > @@ -141,6 +148,15 @@ public:\n> > > > > };\n> > > > > }\n> > > > > \n> > > > > + __nodiscard constexpr Size clampedTo(const Size &minimum,\n> > > > > + const Size &maximum) const\n> > > > > + {\n> > > > > + return {\n> > > > > + std::clamp(width, minimum.width, maximum.width),\n> > > > > + std::clamp(height, minimum.height, maximum.height)\n> > > > > + };\n> > > > > + }\n> > > > > +\n> > > > > __nodiscard constexpr Size grownBy(const Size &margins) const\n> > > > > {\n> > > > > return {\n> > > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > > > index cb3c2de18d5e..7ac053a536c1 100644\n> > > > > --- a/src/libcamera/geometry.cpp\n> > > > > +++ b/src/libcamera/geometry.cpp\n> > > > > @@ -173,6 +173,18 @@ const std::string Size::toString() const\n> > > > > * \\return A reference to this object\n> > > > > */\n> > > > > \n> > > > > +/**\n> > > > > + * \\fn Size::clamp(const Size &minimum, const Size &maximum)\n> > > > > + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> > > \n> > > \"Restrict the size to be constrainted\" sounds quite weird to me. Maybe\n> > > using the word \"clamp\" would be better ?\n> > \n> > I was trying to avoid using the term I'm describing to describe itself\n> > in the description.\n> \n> The fact that the function is named from the term that best describes\n> its purpose means we picked the right name :-)\n\nPerhaps you only know the term 'clamp' in regards to the coding\nimplementation.\n\nDescribing the function 'clamp()' with the word 'clamp' conveys the\nwrong meaning to me. (And maybe other english speakers who have used\nclamps?)\n\nBut to me - a clamp is a mechanical device used to temporarily fix two\n(or more) objects such that they remain in the same position, usually\nwhile you then perform some other operation like screwing the two items\ntogether, or marking them. You 'could' use a clamp to squash something\n... but I don't think that quite fits the real definition of\nstd::clamp() (At least I don't think it can constrain to a non-zero\nminimum).\n\ngoogle-translate tells me that a clamp might be known as 'serrer' in\nFrench or 'Morsetto' in Italian, so perhaps 'clamp' is only known as\nit's use in code ... but I haven't validated those usages in a sentence\n;-) \n\nAnyway, I presume someone who wants to use this - knows what the coding\nterm is - but my point is - I don't think the function name is\nautomatically the best way to describe what it does.\n\nIf it was using both the term restrict and contrain in the same sentence\nI could offer:\n\n\n\"Constrain the Size to be within the dimensions of both the minimum and\nmaximum values.\"\n\nWhich also works with 's/Constrain/Restrict/'\n\n> > Re-reading now it still sounds fine to me, but I can change it.\n> > \n> > Do you mean you prefer:\n> > \n> > Clamp the size to be constrained within the minimum and maximum\n> \n> I meant this, yes. Or maybe\n> \n> Clamp the size to the \\a minimum and \\a maximum values\n> \n> (with s/to/between/, and/or adding the word \"range\" somewhere, if\n> desired)\n> \n> > or\n> > \n> > Restrict the size to be clamped within the minimum and maximum\n> > \n> > > > > + * \\param[in] minimum The minimum size\n> > > > > + * \\param[in] maximum The maximum size\n> > > > > + *\n> > > > > + * This function restricts the width and height to the constraints of the \\a\n> > > > > + * minimum and the \\a maximum sizes given.\n> > > \n> > > And here too, the help text doesn't make it clear what the function\n> > > does. I get more information from the function name than from the\n> > > documentation :-)\n> > \n> > Is it because of the word 'constraints' as you mentioned above? or\n> > something else?\n> \n> Yes, it's \"restricts to the constraints\" that sounds convoluted to me.\n> \n> > Documenting a function called 'clamp' as 'it will clamp the values'\n> > doesn't add any value either.\n> > \n> > > Same for clampedTo().\n> > > \n> > > > > + *\n> > > > > + * \\return A reference to this object\n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\fn Size::growBy(const Size &margins)\n> > > > > * \\brief Grow the size by \\a margins in place\n> > > > > @@ -231,6 +243,15 @@ const std::string Size::toString() const\n> > > > > * height of this size and the \\a expand size\n> > > > > */\n> > > > > \n> > > > > +/**\n> > > > > + * \\fn Size::clampedTo(const Size &minimum, const Size &maximum)\n> > > > > + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> > > > > + * \\param[in] minimum The minimum size\n> > > > > + * \\param[in] maximum The maximum size\n> > > > > + * \\return A size whose width and height match this size within the constraints\n> > > > > + * of the \\a minimum and \\a maximum sizes given.\n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\fn Size::grownBy(const Size &margins)\n> > > > > * \\brief Grow the size by \\a margins\n> > > > > diff --git a/test/geometry.cpp b/test/geometry.cpp\n> > > > > index 5125692496b3..1d3d3cad7174 100644\n> > > > > --- a/test/geometry.cpp\n> > > > > +++ b/test/geometry.cpp\n> > > > > @@ -106,7 +106,7 @@ protected:\n> > > > > \n> > > > > /*\n> > > > > * Test alignDownTo(), alignUpTo(), boundTo(), expandTo(),\n> > > > > - * growBy() and shrinkBy()\n> > > > > + * clamp() growBy() and shrinkBy()\n> > > \n> > > s/ growBy/, growBy/\n> > \n> > Ack.\n> > \n> > > > > */\n> > > > > Size s(50, 50);\n> > > > > \n> > > > > @@ -134,6 +134,18 @@ protected:\n> > > > > return TestFail;\n> > > > > }\n> > > > > \n> > > > > + s.clamp({ 80, 120 }, { 160, 240 });\n> > > > \n> > > > \n> > > > is it okay to ignore the reference returned by .clamp() ? I think so, \n> > > > since it's doesn't have __nodiscard anyways,\n> > > \n> > > The __nodiscard attribute was added to the \"-ed\" versions of the\n> > > functions, to make sure that someone will not accidentally write\n> > > \n> > > s.clampedTo({ 80, 120 }, { 160, 240 });\n> > > \n> > > when they meant\n> > > \n> > > s.clamp({ 80, 120 }, { 160, 240 });\n> > > \n> > > clamp() is meant to be called with its return value potentially ignored,\n> > > otherwise it would be hard to clamp a Size() in-place.\n> > > \n> > > > Reviewed-by: Umang Jain \n> > > > \n> > > > > + if (s != Size(80, 120)) {\n> > > > > + cout << \"Size::clamp (minium) test failed\" << endl;\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > +\n> > > > > + s.clamp({ 20, 30 }, { 50, 50 });\n> > > > > + if (s != Size(50, 50)) {\n> > > > > + cout << \"Size::clamp (maximum) test failed\" << endl;\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > +\n> > > > > s.growBy({ 10, 20 });\n> > > > > if (s != Size(60, 70)) {\n> > > > > cout << \"Size::growBy() test failed\" << endl;\n> > > > > @@ -162,7 +174,7 @@ protected:\n> > > > > \n> > > > > /*\n> > > > > * Test alignedDownTo(), alignedUpTo(), boundedTo(),\n> > > > > - * expandedTo(), grownBy() and shrunkBy()\n> > > > > + * expandedTo(), clampedTo(), grownBy() and shrunkBy()\n> > > > > */\n> > > > > if (Size(0, 0).alignedDownTo(16, 8) != Size(0, 0) ||\n> > > > > Size(1, 1).alignedDownTo(16, 8) != Size(0, 0) ||\n> > > > > @@ -192,6 +204,14 @@ protected:\n> > > > > return TestFail;\n> > > > > }\n> > > > > \n> > > > > + if (Size(0, 0).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 40) ||\n> > > > > + Size(100, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 90) ||\n> > > > > + Size(30, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 90) ||\n> > > > > + Size(100, 30).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 40)) {\n> > > > > + cout << \"Size::clampedTo() test failed\" << endl;\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > +\n> > > > > if (Size(0, 0).grownBy({ 10, 20 }) != Size(10, 20) ||\n> > > > > Size(200, 50).grownBy({ 10, 20 }) != Size(210, 70)) {\n> > > > > cout << \"Size::grownBy() test failed\" << endl;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 16975C0F1B\n\tfor ;\n\tThu, 7 Apr 2022 21:47:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18E1665642;\n\tThu, 7 Apr 2022 23:47:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B42DD6563F\n\tfor ;\n\tThu, 7 Apr 2022 23:47:43 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 34F97499;\n\tThu, 7 Apr 2022 23:47:43 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649368066;\n\tbh=YajAg2KfWzUYR9C9ulBiSc8YKnOTodXqWlvJueH3jEw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=fThRI4CMMd01QSAUgXr7GYiXxXpe373MzuKKjH/BwkKXvLu5aIxCw3uEw/haOQ7IV\n\tR+qb5NLeyfX2t1qrXRODAXN3ffkHALYcFl8QprtuYV+LtL+8yyeiXSpAaZtyyO2R1x\n\t+fKoFcMTqEi3itFzLNur54q7ppAo2N+6TDwYY4IPj5SJBpIvH4zvYpK16j3PIqv6RN\n\tJe8scU6fU+PpaxJQxKCKJIwVc4U2X5SVXW/LUiR9ebrW5l2zSkiqkkTZUHcvTz66BB\n\tRX91fXVUzf6FS3n2Ks7CX+/j3OP7PoA0VCE3eDFVXpgskUPCeFs6Xxe0z+QoyU2mMC\n\tpN0DxXdeyc89Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649368063;\n\tbh=YajAg2KfWzUYR9C9ulBiSc8YKnOTodXqWlvJueH3jEw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=B81P0QsdletnKpqEZSVUPCBJro3C4XUlzJhjfd0xC3J3T5gR0mPIy47k2B+n0/Z0a\n\twZ64rE+kTt9vclgQkVk9v8QKAf59+8/PX1SSsChDdwA3hgup6pnFHuT/NPf9rWLiUp\n\tdwWd+kUw/z1LkUTYgibAFlAGQpPwqw/C7MFjGItA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"B81P0Qsd\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220331120556.2147745-1-kieran.bingham@ideasonboard.com>\n\t<76b13f61-cef8-3bef-da3b-7776eb62ce17@ideasonboard.com>\n\t\n\t<164934233618.3968198.9096792656692370437@Monstersaurus>\n\t","To":"Laurent Pinchart ","Date":"Thu, 07 Apr 2022 22:47:40 +0100","Message-ID":"<164936806035.117583.2740008354778763383@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22668,"web_url":"https://patchwork.libcamera.org/comment/22668/","msgid":"","date":"2022-04-08T07:34:51","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Apr 07, 2022 at 10:47:40PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2022-04-07 15:58:09)\n> > On Thu, Apr 07, 2022 at 03:38:56PM +0100, Kieran Bingham wrote:\n> > > Quoting Laurent Pinchart (2022-04-05 15:18:07)\n> > > > On Tue, Apr 05, 2022 at 07:11:15PM +0530, Umang Jain via libcamera-devel wrote:\n> > > > > Hi Kieran,\n> > > > > \n> > > > > Thank you for the patch.\n> > > > > \n> > > > > On 3/31/22 17:35, Kieran Bingham via libcamera-devel wrote:\n> > > > > > Provide two new operations to support clamping a Size to a given\n> > > > > > minimum or maximum Size, or returning a const variant of the same\n> > > > > > restriction.\n> > > > \n> > > > Did you really mean \"restriction\" here ?\n> > > \n> > > I did yes. I mean the same restriction of the minimum and maximum Size\n> > > but as a const.\n> > > \n> > > > > > Signed-off-by: Kieran Bingham \n> > > > > > ---\n> > > > > >\n> > > > > > I was expecting to use this for clamping the block width and height\n> > > > > > for the AF hardware restrictions on the IPU3 ... but it turns out to be\n> > > > > > not quite appropriate to use a Size there, as the clamped values are\n> > > > > > stored in an IPU3 struct directly.\n> > > > > >\n> > > > > > However, having made this - I think it is likely to be useful elsewhere\n> > > > > > so posting so it doesn't get lost.\n> > > > \n> > > > .clamp(min, max) could be implemented as .expandTo(min).shrinkTo(max),\n> > > > but Size::clamp() is likely more explicit and better.\n> > > \n> > > Yes, a caller could call .expandTo.shrinkTo instead already but I don't\n> > > think that should be the implementation detail here.\n> > > \n> > > > > > Tests added, so it could be merged already even if there is no current\n> > > > > > user yet. I expect it's more likely to get used if it exists, than if it\n> > > > > > doesn't ;-)\n> > > > > \n> > > > > +1\n> > > > > \n> > > > > > include/libcamera/geometry.h | 16 ++++++++++++++++\n> > > > > > src/libcamera/geometry.cpp | 21 +++++++++++++++++++++\n> > > > > > test/geometry.cpp | 24 ++++++++++++++++++++++--\n> > > > > > 3 files changed, 59 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > > > > index 7838b6793617..a447beb55965 100644\n> > > > > > --- a/include/libcamera/geometry.h\n> > > > > > +++ b/include/libcamera/geometry.h\n> > > > > > @@ -93,6 +93,13 @@ public:\n> > > > > > return *this;\n> > > > > > }\n> > > > > > \n> > > > > > + Size &clamp(const Size &minimum, const Size &maximum)\n> > > > > > + {\n> > > > > > + width = std::clamp(width, minimum.width, maximum.width);\n> > > > > > + height = std::clamp(height, minimum.height, maximum.height);\n> > > > > > + return *this;\n> > > > > > + }\n> > > > > > +\n> > > > > > Size &growBy(const Size &margins)\n> > > > > > {\n> > > > > > width += margins.width;\n> > > > > > @@ -141,6 +148,15 @@ public:\n> > > > > > };\n> > > > > > }\n> > > > > > \n> > > > > > + __nodiscard constexpr Size clampedTo(const Size &minimum,\n> > > > > > + const Size &maximum) const\n> > > > > > + {\n> > > > > > + return {\n> > > > > > + std::clamp(width, minimum.width, maximum.width),\n> > > > > > + std::clamp(height, minimum.height, maximum.height)\n> > > > > > + };\n> > > > > > + }\n> > > > > > +\n> > > > > > __nodiscard constexpr Size grownBy(const Size &margins) const\n> > > > > > {\n> > > > > > return {\n> > > > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > > > > index cb3c2de18d5e..7ac053a536c1 100644\n> > > > > > --- a/src/libcamera/geometry.cpp\n> > > > > > +++ b/src/libcamera/geometry.cpp\n> > > > > > @@ -173,6 +173,18 @@ const std::string Size::toString() const\n> > > > > > * \\return A reference to this object\n> > > > > > */\n> > > > > > \n> > > > > > +/**\n> > > > > > + * \\fn Size::clamp(const Size &minimum, const Size &maximum)\n> > > > > > + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> > > > \n> > > > \"Restrict the size to be constrainted\" sounds quite weird to me. Maybe\n> > > > using the word \"clamp\" would be better ?\n> > > \n> > > I was trying to avoid using the term I'm describing to describe itself\n> > > in the description.\n> > \n> > The fact that the function is named from the term that best describes\n> > its purpose means we picked the right name :-)\n> \n> Perhaps you only know the term 'clamp' in regards to the coding\n> implementation.\n> \n> Describing the function 'clamp()' with the word 'clamp' conveys the\n> wrong meaning to me. (And maybe other english speakers who have used\n> clamps?)\n> \n> But to me - a clamp is a mechanical device used to temporarily fix two\n> (or more) objects such that they remain in the same position, usually\n> while you then perform some other operation like screwing the two items\n> together, or marking them. You 'could' use a clamp to squash something\n> ... but I don't think that quite fits the real definition of\n> std::clamp() (At least I don't think it can constrain to a non-zero\n> minimum).\n> \n> google-translate tells me that a clamp might be known as 'serrer' in\n> French or 'Morsetto' in Italian, so perhaps 'clamp' is only known as\n> it's use in code ... but I haven't validated those usages in a sentence\n> ;-) \n\nWe could call the function puristaa() but we will then restrict our\naudience.\n\nJokes aside, the word has multiple meanings, with \"to modify a numeric\nvalue so it lies within a specific range\" being one of them only.\nPerhaps I am more familiar with that meaning than the average developer\nwithout being aware of that (it's widely used in the kernel after all).\n\n> Anyway, I presume someone who wants to use this - knows what the coding\n> term is - but my point is - I don't think the function name is\n> automatically the best way to describe what it does.\n> \n> If it was using both the term restrict and contrain in the same sentence\n> I could offer:\n> \n> \n> \"Constrain the Size to be within the dimensions of both the minimum and\n> maximum values.\"\n> \n> Which also works with 's/Constrain/Restrict/'\n\nI think we've spent enough time bikeshedding, I'm fine with this, or any\nvariation that would be preferred by the team.\n\n> > > Re-reading now it still sounds fine to me, but I can change it.\n> > > \n> > > Do you mean you prefer:\n> > > \n> > > Clamp the size to be constrained within the minimum and maximum\n> > \n> > I meant this, yes. Or maybe\n> > \n> > Clamp the size to the \\a minimum and \\a maximum values\n> > \n> > (with s/to/between/, and/or adding the word \"range\" somewhere, if\n> > desired)\n> > \n> > > or\n> > > \n> > > Restrict the size to be clamped within the minimum and maximum\n> > > \n> > > > > > + * \\param[in] minimum The minimum size\n> > > > > > + * \\param[in] maximum The maximum size\n> > > > > > + *\n> > > > > > + * This function restricts the width and height to the constraints of the \\a\n> > > > > > + * minimum and the \\a maximum sizes given.\n> > > > \n> > > > And here too, the help text doesn't make it clear what the function\n> > > > does. I get more information from the function name than from the\n> > > > documentation :-)\n> > > \n> > > Is it because of the word 'constraints' as you mentioned above? or\n> > > something else?\n> > \n> > Yes, it's \"restricts to the constraints\" that sounds convoluted to me.\n> > \n> > > Documenting a function called 'clamp' as 'it will clamp the values'\n> > > doesn't add any value either.\n> > > \n> > > > Same for clampedTo().\n> > > > \n> > > > > > + *\n> > > > > > + * \\return A reference to this object\n> > > > > > + */\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\fn Size::growBy(const Size &margins)\n> > > > > > * \\brief Grow the size by \\a margins in place\n> > > > > > @@ -231,6 +243,15 @@ const std::string Size::toString() const\n> > > > > > * height of this size and the \\a expand size\n> > > > > > */\n> > > > > > \n> > > > > > +/**\n> > > > > > + * \\fn Size::clampedTo(const Size &minimum, const Size &maximum)\n> > > > > > + * \\brief Restrict the size to be constrained within the \\a minimum and \\a maximum\n> > > > > > + * \\param[in] minimum The minimum size\n> > > > > > + * \\param[in] maximum The maximum size\n> > > > > > + * \\return A size whose width and height match this size within the constraints\n> > > > > > + * of the \\a minimum and \\a maximum sizes given.\n> > > > > > + */\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\fn Size::grownBy(const Size &margins)\n> > > > > > * \\brief Grow the size by \\a margins\n> > > > > > diff --git a/test/geometry.cpp b/test/geometry.cpp\n> > > > > > index 5125692496b3..1d3d3cad7174 100644\n> > > > > > --- a/test/geometry.cpp\n> > > > > > +++ b/test/geometry.cpp\n> > > > > > @@ -106,7 +106,7 @@ protected:\n> > > > > > \n> > > > > > /*\n> > > > > > * Test alignDownTo(), alignUpTo(), boundTo(), expandTo(),\n> > > > > > - * growBy() and shrinkBy()\n> > > > > > + * clamp() growBy() and shrinkBy()\n> > > > \n> > > > s/ growBy/, growBy/\n> > > \n> > > Ack.\n> > > \n> > > > > > */\n> > > > > > Size s(50, 50);\n> > > > > > \n> > > > > > @@ -134,6 +134,18 @@ protected:\n> > > > > > return TestFail;\n> > > > > > }\n> > > > > > \n> > > > > > + s.clamp({ 80, 120 }, { 160, 240 });\n> > > > > \n> > > > > \n> > > > > is it okay to ignore the reference returned by .clamp() ? I think so, \n> > > > > since it's doesn't have __nodiscard anyways,\n> > > > \n> > > > The __nodiscard attribute was added to the \"-ed\" versions of the\n> > > > functions, to make sure that someone will not accidentally write\n> > > > \n> > > > s.clampedTo({ 80, 120 }, { 160, 240 });\n> > > > \n> > > > when they meant\n> > > > \n> > > > s.clamp({ 80, 120 }, { 160, 240 });\n> > > > \n> > > > clamp() is meant to be called with its return value potentially ignored,\n> > > > otherwise it would be hard to clamp a Size() in-place.\n> > > > \n> > > > > Reviewed-by: Umang Jain \n> > > > > \n> > > > > > + if (s != Size(80, 120)) {\n> > > > > > + cout << \"Size::clamp (minium) test failed\" << endl;\n> > > > > > + return TestFail;\n> > > > > > + }\n> > > > > > +\n> > > > > > + s.clamp({ 20, 30 }, { 50, 50 });\n> > > > > > + if (s != Size(50, 50)) {\n> > > > > > + cout << \"Size::clamp (maximum) test failed\" << endl;\n> > > > > > + return TestFail;\n> > > > > > + }\n> > > > > > +\n> > > > > > s.growBy({ 10, 20 });\n> > > > > > if (s != Size(60, 70)) {\n> > > > > > cout << \"Size::growBy() test failed\" << endl;\n> > > > > > @@ -162,7 +174,7 @@ protected:\n> > > > > > \n> > > > > > /*\n> > > > > > * Test alignedDownTo(), alignedUpTo(), boundedTo(),\n> > > > > > - * expandedTo(), grownBy() and shrunkBy()\n> > > > > > + * expandedTo(), clampedTo(), grownBy() and shrunkBy()\n> > > > > > */\n> > > > > > if (Size(0, 0).alignedDownTo(16, 8) != Size(0, 0) ||\n> > > > > > Size(1, 1).alignedDownTo(16, 8) != Size(0, 0) ||\n> > > > > > @@ -192,6 +204,14 @@ protected:\n> > > > > > return TestFail;\n> > > > > > }\n> > > > > > \n> > > > > > + if (Size(0, 0).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 40) ||\n> > > > > > + Size(100, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 90) ||\n> > > > > > + Size(30, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 90) ||\n> > > > > > + Size(100, 30).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 40)) {\n> > > > > > + cout << \"Size::clampedTo() test failed\" << endl;\n> > > > > > + return TestFail;\n> > > > > > + }\n> > > > > > +\n> > > > > > if (Size(0, 0).grownBy({ 10, 20 }) != Size(10, 20) ||\n> > > > > > Size(200, 50).grownBy({ 10, 20 }) != Size(210, 70)) {\n> > > > > > cout << \"Size::grownBy() test failed\" << endl;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 176C2C0F1B\n\tfor ;\n\tFri, 8 Apr 2022 07:34:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C27365642;\n\tFri, 8 Apr 2022 09:34:58 +0200 (CEST)","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 9DB6A61FBA\n\tfor ;\n\tFri, 8 Apr 2022 09:34:56 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09BC6499;\n\tFri, 8 Apr 2022 09:34:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649403298;\n\tbh=j0nbreE3CsxewtNoR9WPC9w/xxATYBm/CG+6J2R80N8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=NI9LZXTGkT5LGAGV8v/0aR54oWh44ueNrcyAX/opAVDVy1VUzYzls7BQPA6k5UaTB\n\tVmbGnieyXNntWuudLeTwu3Ad+Yjd6NdGd0DDmfn1NmWwZNFO1IJbE4/dfn1wvdiSHr\n\tpzq3kyu7k07fCZr9owybOwAhWC5BfGnCSTwqPJcBphKfNgL19yeuNQmP7otkA4ZT/t\n\t2lCVhaEtaU5shKnnB9064eU5lB7MjdTYLlUgFdwbucX7V1NwvHLnuimI0a7nu2k8g6\n\t4X38XxsMHBTfM42qIhIi1c2abMUAU3lxPpCI4FgwAE1AGAFkJZVNnQRxrpIF31jZ1j\n\t9yw/rzQtDhwxg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649403296;\n\tbh=j0nbreE3CsxewtNoR9WPC9w/xxATYBm/CG+6J2R80N8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ODWMP/QxJUirGv2VktG2RP0G7lNb1+7o7XEU0GwtcOfwQ+/sMwcaLm+skI5vpPy5L\n\tI8GFOGj1DgpMhtHG1cJJrbZQgkMcqPPCphZ3weiEgV9rUBF50KcdaILlcn8ZLSZQkQ\n\tlEvZJhqsvLcG4yZDmXrJ1Ac+elZukgXWp40fQKUM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ODWMP/Qx\"; dkim-atps=neutral","Date":"Fri, 8 Apr 2022 10:34:51 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220331120556.2147745-1-kieran.bingham@ideasonboard.com>\n\t<76b13f61-cef8-3bef-da3b-7776eb62ce17@ideasonboard.com>\n\t\n\t<164934233618.3968198.9096792656692370437@Monstersaurus>\n\t\n\t<164936806035.117583.2740008354778763383@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164936806035.117583.2740008354778763383@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Add Size members\n\tto clamp to a min/max","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]