[{"id":35292,"web_url":"https://patchwork.libcamera.org/comment/35292/","msgid":"<175450214098.50296.8243716677499100461@ping.linuxembedded.co.uk>","date":"2025-08-06T17:42:20","subject":"Re: [PATCH v2 1/2] libcamera: geometry: Add Rectangle::croppedBy","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-07-24 16:29:35)\n> Let's add a new Rectangle method that returns the coordinates of the\n> central area of the original rectangle cropped by given\n> numerator/denominator fraction. This is useful to specify a limited\n> area of the image to operate on, for example computing statistics only\n> from a reduced area for speedup.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> include/libcamera/geometry.h | 2 ++\n> src/libcamera/geometry.cpp | 19 +++++++++++++++++++\n> test/geometry.cpp | 9 +++++++++\n> 3 files changed, 30 insertions(+)\n> \n> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> index f322e3d5b..603c9a117 100644\n> --- a/include/libcamera/geometry.h\n> +++ b/include/libcamera/geometry.h\n> @@ -297,6 +297,8 @@ public:\n> [[nodiscard]] Rectangle scaledBy(const Size &numerator,\n> const Size &denominator) const;\n> [[nodiscard]] Rectangle translatedBy(const Point &point) const;\n> + [[nodiscard]] Rectangle croppedBy(const unsigned int numerator,\n> + const unsigned int denominator) const;\n> \n> Rectangle transformedBetween(const Rectangle &source,\n> const Rectangle &target) const;\n> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> index 81cc8cd53..8ceb75698 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -837,6 +837,25 @@ Rectangle Rectangle::translatedBy(const Point &point) const\n> return { x + point.x, y + point.y, width, height };\n> }\n> \n> +/**\n> + * \\brief Return a central area crop of the rectangle\n> + * \\param[in] numerator The numerator of the crop factor\n> + * \\param[in] denominator The denominator of the crop factor\n> + *\n> + * The rectangle of the central part of the original rectangle is computed, of\n> + * numerator/denominator times the original width and height.\n> + *\n> + * \\return The cropped Rectangle coordinates\n> + */\n> +Rectangle Rectangle::croppedBy(const unsigned int numerator,\n> + const unsigned int denominator) const\n> +{\n> + unsigned int w = static_cast(width) * numerator / denominator;\n> + unsigned int h = static_cast(height) * numerator / denominator;\n> +\n> + return Size(w, h).centeredTo(center());\n\nI think that's a cleaner implementation. My only worries here are still\non the naming. I can't convince myself if I'm sure if 'croppedBy'\nimplicitly means centered or not ...\n\nI ... think it seems reasonable to be implicit ... any other thoughts /\ncomments from others ?\n\n\n> +}\n> +\n> /**\n> * \\brief Transform a Rectangle from one reference rectangle to another\n> * \\param[in] source The \\a source reference rectangle\n> diff --git a/test/geometry.cpp b/test/geometry.cpp\n> index 11df043b7..9864774f4 100644\n> --- a/test/geometry.cpp\n> +++ b/test/geometry.cpp\n> @@ -466,6 +466,15 @@ protected:\n> return TestFail;\n> }\n> \n> + /* Rectangle::croppedBy() tests */\n> + if (Rectangle(Size(300, 100)).croppedBy(2, 3) !=\n> + Rectangle(50, 17, 200, 66) ||\n> + Rectangle(-100, 100, 300, 700).croppedBy(3, 7) !=\n> + Rectangle(-14, 300, 128, 300)) {\n> + cout << \"Rectangle::croppedBy() test failed \" << endl;\n> + return TestFail;\n> + }\n\nthanks for adding the test. Simple - but enforces/documents/reinforces\nthe behaviour expectations.\n\n\n\n> +\n> /* Rectangle::translateBy() tests */\n> r = Rectangle(10, -20, 300, 400);\n> r.translateBy(Point(-30, 40));\n> -- \n> 2.50.1\n>","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 34DACBDCC1\n\tfor ;\n\tWed, 6 Aug 2025 17:42:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B7C96921A;\n\tWed, 6 Aug 2025 19:42:24 +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 5DFD2691F5\n\tfor ;\n\tWed, 6 Aug 2025 19:42:23 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AD50EEBA;\n\tWed, 6 Aug 2025 19:41:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QmXVzbFG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754502094;\n\tbh=KI4AGNu+AJGEytstU1doOlpShY6gKoyMdesHe6Mkic8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=QmXVzbFG1k5KvUGkW0yP3C5aa3nX5r3Tw0zCLUIbq4T2ldbf7o332LIiOfdlzpVID\n\tGxZMj4ybp3SsCcy5qy+xeA1P6FbXCLlL2QVsVLm19GKXetohFVTcoThii7gBi/kquC\n\t2GmdWOFNcmUeBeI20T17/mK/nY94RjDI2I2RVlUA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250724152936.99830-2-mzamazal@redhat.com>","References":"<20250724152936.99830-1-mzamazal@redhat.com>\n\t<20250724152936.99830-2-mzamazal@redhat.com>","Subject":"Re: [PATCH v2 1/2] libcamera: geometry: Add Rectangle::croppedBy","From":"Kieran Bingham ","Cc":"Milan Zamazal ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Wed, 06 Aug 2025 18:42:20 +0100","Message-ID":"<175450214098.50296.8243716677499100461@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35294,"web_url":"https://patchwork.libcamera.org/comment/35294/","msgid":"","date":"2025-08-06T18:35:22","subject":"Re: [PATCH v2 1/2] libcamera: geometry: Add Rectangle::croppedBy","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 08. 06. 19:42 keltezéssel, Kieran Bingham írta:\n> Quoting Milan Zamazal (2025-07-24 16:29:35)\n>> Let's add a new Rectangle method that returns the coordinates of the\n>> central area of the original rectangle cropped by given\n>> numerator/denominator fraction. This is useful to specify a limited\n>> area of the image to operate on, for example computing statistics only\n>> from a reduced area for speedup.\n>>\n>> Signed-off-by: Milan Zamazal \n>> ---\n>> include/libcamera/geometry.h | 2 ++\n>> src/libcamera/geometry.cpp | 19 +++++++++++++++++++\n>> test/geometry.cpp | 9 +++++++++\n>> 3 files changed, 30 insertions(+)\n>>\n>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n>> index f322e3d5b..603c9a117 100644\n>> --- a/include/libcamera/geometry.h\n>> +++ b/include/libcamera/geometry.h\n>> @@ -297,6 +297,8 @@ public:\n>> [[nodiscard]] Rectangle scaledBy(const Size &numerator,\n>> const Size &denominator) const;\n>> [[nodiscard]] Rectangle translatedBy(const Point &point) const;\n>> + [[nodiscard]] Rectangle croppedBy(const unsigned int numerator,\n>> + const unsigned int denominator) const;\n>> \n>> Rectangle transformedBetween(const Rectangle &source,\n>> const Rectangle &target) const;\n>> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n>> index 81cc8cd53..8ceb75698 100644\n>> --- a/src/libcamera/geometry.cpp\n>> +++ b/src/libcamera/geometry.cpp\n>> @@ -837,6 +837,25 @@ Rectangle Rectangle::translatedBy(const Point &point) const\n>> return { x + point.x, y + point.y, width, height };\n>> }\n>> \n>> +/**\n>> + * \\brief Return a central area crop of the rectangle\n>> + * \\param[in] numerator The numerator of the crop factor\n>> + * \\param[in] denominator The denominator of the crop factor\n>> + *\n>> + * The rectangle of the central part of the original rectangle is computed, of\n>> + * numerator/denominator times the original width and height.\n>> + *\n>> + * \\return The cropped Rectangle coordinates\n>> + */\n>> +Rectangle Rectangle::croppedBy(const unsigned int numerator,\n>> + const unsigned int denominator) const\n>> +{\n>> + unsigned int w = static_cast(width) * numerator / denominator;\n>> + unsigned int h = static_cast(height) * numerator / denominator;\n>> +\n>> + return Size(w, h).centeredTo(center());\n> \n> I think that's a cleaner implementation. My only worries here are still\n> on the naming. I can't convince myself if I'm sure if 'croppedBy'\n> implicitly means centered or not ...\n\nThe name is not clear to me at all. I feel like it should be `scale[d]By()`.\nBut of course that exists. Maybe we should extend `scaleBy()` with an \"origin\"\nparameter that determines the origin of the scaling (which is 0,0 for the current scaleBy()\nbut would be the center for this version). Something like this:\n\n scaleBy(numer, denom, origin) const {\n x = (int64_t(this->x) - int64_t(origin.x)) * numer.x / denom.x + origin.x;\n y = (int64_t(this->y) - int64_t(origin.x)) * numer.y / denom.y + origin.y;\n w = uint64_t(this->w) * numer.x / denom.x;\n h = uint64_t(this->h) * numer.y / denom.y;\n return Rectangle(x, y, w, h);\n }\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> I ... think it seems reasonable to be implicit ... any other thoughts /\n> comments from others ?\n> \n> \n>> +}\n>> +\n>> /**\n>> * \\brief Transform a Rectangle from one reference rectangle to another\n>> * \\param[in] source The \\a source reference rectangle\n>> diff --git a/test/geometry.cpp b/test/geometry.cpp\n>> index 11df043b7..9864774f4 100644\n>> --- a/test/geometry.cpp\n>> +++ b/test/geometry.cpp\n>> @@ -466,6 +466,15 @@ protected:\n>> return TestFail;\n>> }\n>> \n>> + /* Rectangle::croppedBy() tests */\n>> + if (Rectangle(Size(300, 100)).croppedBy(2, 3) !=\n>> + Rectangle(50, 17, 200, 66) ||\n>> + Rectangle(-100, 100, 300, 700).croppedBy(3, 7) !=\n>> + Rectangle(-14, 300, 128, 300)) {\n>> + cout << \"Rectangle::croppedBy() test failed \" << endl;\n>> + return TestFail;\n>> + }\n> \n> thanks for adding the test. Simple - but enforces/documents/reinforces\n> the behaviour expectations.\n> \n> \n> \n>> +\n>> /* Rectangle::translateBy() tests */\n>> r = Rectangle(10, -20, 300, 400);\n>> r.translateBy(Point(-30, 40));\n>> -- \n>> 2.50.1\n>>","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 0D4A8BE086\n\tfor ;\n\tWed, 6 Aug 2025 18:35:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 41EC36921A;\n\tWed, 6 Aug 2025 20:35:48 +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 2342F691F5\n\tfor ;\n\tWed, 6 Aug 2025 20:35:46 +0200 (CEST)","from [192.168.33.11] (185.182.215.213.nat.pool.zt.hu\n\t[185.182.215.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2A671C75;\n\tWed, 6 Aug 2025 20:34:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QIzM/j4k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754505297;\n\tbh=tBG7eZWeU8VvMkYJI9vWorTyc9p9U2dx4aIurdBRl5s=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=QIzM/j4kdNJW4hg+nCBtGMfH931Ri+B7/wE+qdkLNXV1Wa0px72BF/cIWMwl+fh7n\n\tcxwyoFyyvR/6hnSe0+fNaAvs7pGeDVRSLbjzYfqrCrKT83ESIrHVd5+iKy+mqpsMh+\n\tplmt5tLolh93cNNvJGDOygH9R+g3IT9WgS/UkCWk=","Message-ID":"","Date":"Wed, 6 Aug 2025 20:35:22 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 1/2] libcamera: geometry: Add Rectangle::croppedBy","To":"Kieran Bingham ,\n\tMilan Zamazal , libcamera-devel@lists.libcamera.org","References":"<20250724152936.99830-1-mzamazal@redhat.com>\n\t<20250724152936.99830-2-mzamazal@redhat.com>\n\t<175450214098.50296.8243716677499100461@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175450214098.50296.8243716677499100461@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35296,"web_url":"https://patchwork.libcamera.org/comment/35296/","msgid":"<20250807130611.GA13647@pendragon.ideasonboard.com>","date":"2025-08-07T13:06:11","subject":"Re: [PATCH v2 1/2] libcamera: geometry: Add Rectangle::croppedBy","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Aug 06, 2025 at 08:35:22PM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 06. 19:42 keltezéssel, Kieran Bingham írta:\n> > Quoting Milan Zamazal (2025-07-24 16:29:35)\n> >> Let's add a new Rectangle method that returns the coordinates of the\n> >> central area of the original rectangle cropped by given\n> >> numerator/denominator fraction. This is useful to specify a limited\n> >> area of the image to operate on, for example computing statistics only\n> >> from a reduced area for speedup.\n> >>\n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> include/libcamera/geometry.h | 2 ++\n> >> src/libcamera/geometry.cpp | 19 +++++++++++++++++++\n> >> test/geometry.cpp | 9 +++++++++\n> >> 3 files changed, 30 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> >> index f322e3d5b..603c9a117 100644\n> >> --- a/include/libcamera/geometry.h\n> >> +++ b/include/libcamera/geometry.h\n> >> @@ -297,6 +297,8 @@ public:\n> >> [[nodiscard]] Rectangle scaledBy(const Size &numerator,\n> >> const Size &denominator) const;\n> >> [[nodiscard]] Rectangle translatedBy(const Point &point) const;\n> >> + [[nodiscard]] Rectangle croppedBy(const unsigned int numerator,\n> >> + const unsigned int denominator) const;\n> >> \n> >> Rectangle transformedBetween(const Rectangle &source,\n> >> const Rectangle &target) const;\n> >> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> >> index 81cc8cd53..8ceb75698 100644\n> >> --- a/src/libcamera/geometry.cpp\n> >> +++ b/src/libcamera/geometry.cpp\n> >> @@ -837,6 +837,25 @@ Rectangle Rectangle::translatedBy(const Point &point) const\n> >> return { x + point.x, y + point.y, width, height };\n> >> }\n> >> \n> >> +/**\n> >> + * \\brief Return a central area crop of the rectangle\n> >> + * \\param[in] numerator The numerator of the crop factor\n> >> + * \\param[in] denominator The denominator of the crop factor\n> >> + *\n> >> + * The rectangle of the central part of the original rectangle is computed, of\n> >> + * numerator/denominator times the original width and height.\n> >> + *\n> >> + * \\return The cropped Rectangle coordinates\n> >> + */\n> >> +Rectangle Rectangle::croppedBy(const unsigned int numerator,\n> >> + const unsigned int denominator) const\n> >> +{\n> >> + unsigned int w = static_cast(width) * numerator / denominator;\n> >> + unsigned int h = static_cast(height) * numerator / denominator;\n> >> +\n> >> + return Size(w, h).centeredTo(center());\n> > \n> > I think that's a cleaner implementation. My only worries here are still\n> > on the naming. I can't convince myself if I'm sure if 'croppedBy'\n> > implicitly means centered or not ...\n> \n> The name is not clear to me at all. I feel like it should be `scale[d]By()`.\n> But of course that exists. Maybe we should extend `scaleBy()` with an \"origin\"\n> parameter that determines the origin of the scaling (which is 0,0 for the current scaleBy()\n> but would be the center for this version). Something like this:\n> \n> scaleBy(numer, denom, origin) const {\n> x = (int64_t(this->x) - int64_t(origin.x)) * numer.x / denom.x + origin.x;\n> y = (int64_t(this->y) - int64_t(origin.x)) * numer.y / denom.y + origin.y;\n> w = uint64_t(this->w) * numer.x / denom.x;\n> h = uint64_t(this->h) * numer.y / denom.y;\n> return Rectangle(x, y, w, h);\n> }\n\nI think this is becoming too specialized. How about composing multiple\nsimpler geometry functions to obtain the same result ? We could add a\nscaledBy() function to Size, which would allow writing\n\n\tr = rect.size().scaledBy(numerator, denominator).centeredTo(rect.center());\n\nor rather with a float ratio instead of numerator and denominator if the\nx and y ratios are identical:\n\n\tr = rect.size().scaledBy(ratio).centeredTo(rect.center());\n\n> > I ... think it seems reasonable to be implicit ... any other thoughts /\n> > comments from others ?\n> > \n> >> +}\n> >> +\n> >> /**\n> >> * \\brief Transform a Rectangle from one reference rectangle to another\n> >> * \\param[in] source The \\a source reference rectangle\n> >> diff --git a/test/geometry.cpp b/test/geometry.cpp\n> >> index 11df043b7..9864774f4 100644\n> >> --- a/test/geometry.cpp\n> >> +++ b/test/geometry.cpp\n> >> @@ -466,6 +466,15 @@ protected:\n> >> return TestFail;\n> >> }\n> >> \n> >> + /* Rectangle::croppedBy() tests */\n> >> + if (Rectangle(Size(300, 100)).croppedBy(2, 3) !=\n> >> + Rectangle(50, 17, 200, 66) ||\n> >> + Rectangle(-100, 100, 300, 700).croppedBy(3, 7) !=\n> >> + Rectangle(-14, 300, 128, 300)) {\n> >> + cout << \"Rectangle::croppedBy() test failed \" << endl;\n> >> + return TestFail;\n> >> + }\n> > \n> > thanks for adding the test. Simple - but enforces/documents/reinforces\n> > the behaviour expectations.\n> > \n> >> +\n> >> /* Rectangle::translateBy() tests */\n> >> r = Rectangle(10, -20, 300, 400);\n> >> r.translateBy(Point(-30, 40));","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 B55ADBE086\n\tfor ;\n\tThu, 7 Aug 2025 13:06:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C855E6921A;\n\tThu, 7 Aug 2025 15:06:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8F72D69052\n\tfor ;\n\tThu, 7 Aug 2025 15:06:26 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 09689502;\n\tThu, 7 Aug 2025 15:05:36 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MF9WzCR0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754571937;\n\tbh=eUKf1UFv8jEB71I64AjZmm/OI3AMDZgBv6Ea+mEWLgo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MF9WzCR0M6SGEnb3QUq7W654U7/iW5pXEMnWBdYvhpvgQTLPjoVZUsvh1iFeK2aAJ\n\tlq8PL7MCu6OtfVN3tWkYASoyAtmzbBeF7g2PYabhm4CKkMvPU+MIyepLpgsX5JzkSv\n\txxvX26TIXewou2Fgj2JzeNpb2jnYtWVbp/xeDX+g=","Date":"Thu, 7 Aug 2025 16:06:11 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tMilan Zamazal , libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 1/2] libcamera: geometry: Add Rectangle::croppedBy","Message-ID":"<20250807130611.GA13647@pendragon.ideasonboard.com>","References":"<20250724152936.99830-1-mzamazal@redhat.com>\n\t<20250724152936.99830-2-mzamazal@redhat.com>\n\t<175450214098.50296.8243716677499100461@ping.linuxembedded.co.uk>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35306,"web_url":"https://patchwork.libcamera.org/comment/35306/","msgid":"<87f7ca4f-9a57-40d6-a549-20a3db6d1c9b@ideasonboard.com>","date":"2025-08-08T07:22:57","subject":"Re: [PATCH v2 1/2] libcamera: geometry: Add Rectangle::croppedBy","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 08. 07. 15:06 keltezéssel, Laurent Pinchart írta:\n> On Wed, Aug 06, 2025 at 08:35:22PM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 06. 19:42 keltezéssel, Kieran Bingham írta:\n>>> Quoting Milan Zamazal (2025-07-24 16:29:35)\n>>>> Let's add a new Rectangle method that returns the coordinates of the\n>>>> central area of the original rectangle cropped by given\n>>>> numerator/denominator fraction. This is useful to specify a limited\n>>>> area of the image to operate on, for example computing statistics only\n>>>> from a reduced area for speedup.\n>>>>\n>>>> Signed-off-by: Milan Zamazal \n>>>> ---\n>>>> include/libcamera/geometry.h | 2 ++\n>>>> src/libcamera/geometry.cpp | 19 +++++++++++++++++++\n>>>> test/geometry.cpp | 9 +++++++++\n>>>> 3 files changed, 30 insertions(+)\n>>>>\n>>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n>>>> index f322e3d5b..603c9a117 100644\n>>>> --- a/include/libcamera/geometry.h\n>>>> +++ b/include/libcamera/geometry.h\n>>>> @@ -297,6 +297,8 @@ public:\n>>>> [[nodiscard]] Rectangle scaledBy(const Size &numerator,\n>>>> const Size &denominator) const;\n>>>> [[nodiscard]] Rectangle translatedBy(const Point &point) const;\n>>>> + [[nodiscard]] Rectangle croppedBy(const unsigned int numerator,\n>>>> + const unsigned int denominator) const;\n>>>> \n>>>> Rectangle transformedBetween(const Rectangle &source,\n>>>> const Rectangle &target) const;\n>>>> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n>>>> index 81cc8cd53..8ceb75698 100644\n>>>> --- a/src/libcamera/geometry.cpp\n>>>> +++ b/src/libcamera/geometry.cpp\n>>>> @@ -837,6 +837,25 @@ Rectangle Rectangle::translatedBy(const Point &point) const\n>>>> return { x + point.x, y + point.y, width, height };\n>>>> }\n>>>> \n>>>> +/**\n>>>> + * \\brief Return a central area crop of the rectangle\n>>>> + * \\param[in] numerator The numerator of the crop factor\n>>>> + * \\param[in] denominator The denominator of the crop factor\n>>>> + *\n>>>> + * The rectangle of the central part of the original rectangle is computed, of\n>>>> + * numerator/denominator times the original width and height.\n>>>> + *\n>>>> + * \\return The cropped Rectangle coordinates\n>>>> + */\n>>>> +Rectangle Rectangle::croppedBy(const unsigned int numerator,\n>>>> + const unsigned int denominator) const\n>>>> +{\n>>>> + unsigned int w = static_cast(width) * numerator / denominator;\n>>>> + unsigned int h = static_cast(height) * numerator / denominator;\n>>>> +\n>>>> + return Size(w, h).centeredTo(center());\n>>>\n>>> I think that's a cleaner implementation. My only worries here are still\n>>> on the naming. I can't convince myself if I'm sure if 'croppedBy'\n>>> implicitly means centered or not ...\n>>\n>> The name is not clear to me at all. I feel like it should be `scale[d]By()`.\n>> But of course that exists. Maybe we should extend `scaleBy()` with an \"origin\"\n>> parameter that determines the origin of the scaling (which is 0,0 for the current scaleBy()\n>> but would be the center for this version). Something like this:\n>>\n>> scaleBy(numer, denom, origin) const {\n>> x = (int64_t(this->x) - int64_t(origin.x)) * numer.x / denom.x + origin.x;\n>> y = (int64_t(this->y) - int64_t(origin.x)) * numer.y / denom.y + origin.y;\n>> w = uint64_t(this->w) * numer.x / denom.x;\n>> h = uint64_t(this->h) * numer.y / denom.y;\n>> return Rectangle(x, y, w, h);\n>> }\n> \n> I think this is becoming too specialized. How about composing multiple\n\nInteresting, I feel this is an entirely natural extension, in a sense \"revealing\"\na parameter of the transformation that has been hard-coded so far.\n\n\n> simpler geometry functions to obtain the same result ? We could add a\n> scaledBy() function to Size, which would allow writing\n> \n> \tr = rect.size().scaledBy(numerator, denominator).centeredTo(rect.center());\n> \n> or rather with a float ratio instead of numerator and denominator if the\n> x and y ratios are identical:\n> \n> \tr = rect.size().scaledBy(ratio).centeredTo(rect.center());\n> \n>>> I ... think it seems reasonable to be implicit ... any other thoughts /\n>>> comments from others ?\n\nIn my opinion this looks good as well. All that seems to be needed is `Size::scaledBy()`.\nBut there is operator*(), so maybe this is already possible:\n\n (rect.size() * ratio).centeredTo(rect.center())\n\n\nRegards,\nBarnabás Pőcze\n\n>>>\n>>>> +}\n>>>> +\n>>>> /**\n>>>> * \\brief Transform a Rectangle from one reference rectangle to another\n>>>> * \\param[in] source The \\a source reference rectangle\n>>>> diff --git a/test/geometry.cpp b/test/geometry.cpp\n>>>> index 11df043b7..9864774f4 100644\n>>>> --- a/test/geometry.cpp\n>>>> +++ b/test/geometry.cpp\n>>>> @@ -466,6 +466,15 @@ protected:\n>>>> return TestFail;\n>>>> }\n>>>> \n>>>> + /* Rectangle::croppedBy() tests */\n>>>> + if (Rectangle(Size(300, 100)).croppedBy(2, 3) !=\n>>>> + Rectangle(50, 17, 200, 66) ||\n>>>> + Rectangle(-100, 100, 300, 700).croppedBy(3, 7) !=\n>>>> + Rectangle(-14, 300, 128, 300)) {\n>>>> + cout << \"Rectangle::croppedBy() test failed \" << endl;\n>>>> + return TestFail;\n>>>> + }\n>>>\n>>> thanks for adding the test. Simple - but enforces/documents/reinforces\n>>> the behaviour expectations.\n>>>\n>>>> +\n>>>> /* Rectangle::translateBy() tests */\n>>>> r = Rectangle(10, -20, 300, 400);\n>>>> r.translateBy(Point(-30, 40));\n>","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 7DE7FBDCC1\n\tfor ;\n\tFri, 8 Aug 2025 07:23:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A91C6921A;\n\tFri, 8 Aug 2025 09:23:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14EC169215\n\tfor ;\n\tFri, 8 Aug 2025 09:23:02 +0200 (CEST)","from [192.168.33.11] (185.182.215.213.nat.pool.zt.hu\n\t[185.182.215.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 00BF814B0;\n\tFri, 8 Aug 2025 09:22:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Hhb1mHVV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754637732;\n\tbh=HoJzwR9q8RNY3iqK2Okek2l0LKVILWm+azbh6hJSW/0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Hhb1mHVVlkOoJMCBokOoM15ADNGXlTSap+yD22+Iaj5oBl+GjHm84KzAiy0htg4vS\n\tesro02ih1e/Cs8qMY6MBz227yy9AGBMA3XQGxvWAa5TYa/g0dFu1xcK/AXqahf+0ja\n\tzM5fmkpGzIMelBrHgxImWf4meLV9lBZNBauh7rao=","Message-ID":"<87f7ca4f-9a57-40d6-a549-20a3db6d1c9b@ideasonboard.com>","Date":"Fri, 8 Aug 2025 09:22:57 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 1/2] libcamera: geometry: Add Rectangle::croppedBy","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tMilan Zamazal , libcamera-devel@lists.libcamera.org","References":"<20250724152936.99830-1-mzamazal@redhat.com>\n\t<20250724152936.99830-2-mzamazal@redhat.com>\n\t<175450214098.50296.8243716677499100461@ping.linuxembedded.co.uk>\n\t\n\t<20250807130611.GA13647@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250807130611.GA13647@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]