[{"id":12879,"web_url":"https://patchwork.libcamera.org/comment/12879/","msgid":"","date":"2020-09-29T19:40:38","subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: Add geometry helper\n\tfunctions","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 29/09/2020 17:39, David Plowman wrote:\n> These functions are aimed at making it easier to calculate cropping\n> rectangles, particularly in order to implement digital zoom.\n\nPersonally, I love seeing these helpers grow ;-)\n\n> Signed-off-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n\nSome minor spelling errors below, but with those fixed:\n\nReviewed-by: Kieran Bingham \n\n> ---\n> include/libcamera/geometry.h | 20 ++++++\n> src/libcamera/geometry.cpp | 129 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 149 insertions(+)\n> \n> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> index 02fb63c0..0447ee3e 100644\n> --- a/include/libcamera/geometry.h\n> +++ b/include/libcamera/geometry.h\n> @@ -13,6 +13,8 @@\n> \n> namespace libcamera {\n> \n> +class Rectangle;\n> +\n> class Size\n> {\n> public:\n> @@ -93,8 +95,17 @@ public:\n> \t\t\tstd::max(height, expand.height)\n> \t\t};\n> \t}\n> +\n> +\tSize alignedDownToAspectRatio(const Size &ratio) const;\n> +\tSize alignedUpToAspectRatio(const Size &ratio) const;\n> +\n> +\tRectangle centredTo(const Rectangle ®ion,\n> +\t\t\t int offsetX = 0, int offsetY = 0) const;\n> };\n> \n> +Size operator*(const Size &size, float f);\n> +Size operator/(const Size &size, float f);\n> +\n> bool operator==(const Size &lhs, const Size &rhs);\n> bool operator<(const Size &lhs, const Size &rhs);\n> \n> @@ -176,6 +187,11 @@ public:\n> \t{\n> \t}\n> \n> +\tconstexpr explicit Rectangle(const Size &size)\n> +\t\t: x(0), y(0), width(size.width), height(size.height)\n> +\t{\n> +\t}\n> +\n> \tint x;\n> \tint y;\n> \tunsigned int width;\n> @@ -183,6 +199,10 @@ public:\n> \n> \tbool isNull() const { return !width && !height; }\n> \tconst std::string toString() const;\n> +\n> +\tSize size() const;\n> +\n> +\tRectangle boundedTo(const Rectangle &boundary) const;\n> };\n> \n> bool operator==(const Rectangle &lhs, const Rectangle &rhs);\n> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> index b12e1a62..649ee179 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -143,6 +143,88 @@ const std::string Size::toString() const\n> * height of this size and the \\a expand size\n> */\n> \n> +/**\n> + * \\brief Align down to the aspect ratio given by \\a ratio\n> + * \\param[in] ratio The size whose aspect ratio to align down to\n> + * \\return A Size whose width and height are equal to the width and height\n> + * of this Size aligned down to the aspect ratio of \\a ratio\n> + */\n> +Size Size::alignedDownToAspectRatio(const Size &ratio) const\n> +{\n> +\tuint64_t ratio1 = static_cast(width) *\n> +\t\t\t static_cast(ratio.height);\n> +\tuint64_t ratio2 = static_cast(ratio.width) *\n> +\t\t\t static_cast(height);\n> +\n> +\tif (ratio1 > ratio2)\n> +\t\treturn { static_cast(ratio2 / ratio.height), height };\n> +\telse\n> +\t\treturn { width, static_cast(ratio1 / ratio.width) };\n> +}\n> +\n> +/**\n> + * \\brief Align up to the aspect ratio given by \\a ratio\n> + * \\param[in] ratio The size whose aspect ratio to align up to\n> + * \\return A Size whose width and height are equal to the width and height\n> + * of this Size aligned up to the aspect ratio of \\a ratio\n> + */\n> +Size Size::alignedUpToAspectRatio(const Size &ratio) const\n> +{\n> +\tuint64_t ratio1 = static_cast(width) *\n> +\t\t\t static_cast(ratio.height);\n> +\tuint64_t ratio2 = static_cast(ratio.width) *\n> +\t\t\t static_cast(height);\n> +\n> +\tif (ratio1 < ratio2)\n> +\t\treturn { static_cast(ratio2 / ratio.height), height };\n> +\telse\n> +\t\treturn { width, static_cast(ratio1 / ratio.width) };\n> +}\n> +\n> +/**\n> + * \\brief Centre a rectangle of this size within another rectangular region,\n> + * with optional offsets\n> + * \\param[in] region The rectangular region relative to which the returned\n> + * rectangle can be position\n> + * \\param[in] offsetX The X offset of the mid-point of the returned rectangle\n> + * relative to the mid-point of the region\n> + * \\param[in] offsetY The Y offset of the mid-point of the returned rectangle\n> + * relative to the mid-point of the region\n> + *\n> + * A Rectangle of this object's size is positioned within the Rectangle\n> + * given by \\a region. It is positioned so that its mid-point coincides\n> + * with the mid-point of \\a region, and is then further offset by the\n> + * values \\a offsetX and \\a offsetY.\n> + *\n> + * \\return A Rectangle with the horizontal and vertical sizes of\n> + * this Size instance, centred with offsets within a region\n> + */\n> +Rectangle Size::centredTo(const Rectangle ®ion, int offsetX, int offsetY) const\n> +{\n> +\tint x = (region.width - width) / 2 + region.x + offsetX;\n> +\tint y = (region.height - height) / 2 + region.y + offsetY;\n> +\n> +\treturn Rectangle(x, y, width, height);\n> +}\n> +\n> +/**\n> + * \\brief Scale size up by the given factor\n> + * \\return The scaled Size\n> + */\n> +Size operator*(const Size &size, float f)\n> +{\n> +\treturn Size(size.width * f, size.height * f);\n> +}\n> +\n> +/**\n> + * \\brief Scale size down by the given factor\n> + * \\return The scaled Size\n> + */\n> +Size operator/(const Size &size, float f)\n> +{\n> +\treturn Size(size.width / f, size.height / f);\n> +}\n> +\n> /**\n> * \\brief Compare sizes for equality\n> * \\return True if the two sizes are equal, false otherwise\n> @@ -365,6 +447,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)\n> * \\param[in] height The height\n> */\n> \n> +/**\n> + * \\fn Rectangle::Rectangle(const Size &size)\n> + * \\brief Construct a Rectangle with zero offsets and the given \\a size\n> + * \\param[in] size The desired Rectangle size\n> + */\n> +\n> /**\n> * \\var Rectangle::x\n> * \\brief The horizontal coordinate of the rectangle's top-left corner\n> @@ -404,6 +492,47 @@ const std::string Rectangle::toString() const\n> \treturn ss.str();\n> }\n> \n> +/**\n> + * \\brief Retrieve the size of this rectangle\n> + * \\return A Size reporting the Rectangle horizontal and vertical sizes\n> + */\n> +Size Rectangle::size() const\n> +{\n> +\treturn Size(width, height);\n> +}\n> +\n> +/**\n> + * \\brief Bound a Rectangle so as not to exceeed another Rectangle\n\ns/exceeed/exceed/\n\n> + * \\param[in] boundary The limit that the returned Rectangle will not exceed\n> + *\n> + * The Rectangle is bounded so that it does not exceeed the given \\a boundary.\n\ns/exceeed/exceed/\n\n> + * This process involves translating the Rectangle if any of its edges\n> + * lie beyond \\a boundary, so that those edges then lie along the boundary\n> + * instead.\n> + *\n> + * If either width or height are larger than \\a bounary, then the returned\n\ns/bounary/boundary/\n\n> + * Rectangle is clipped to be no larger. But other than this, the\n> + * Rectangle is not clipped or reduced in size, merely translated.\n> + *\n> + * We note that this is not a conventional Rectangle intersection function.\n> + *\n> + * \\return A Rectangle that does not extend beyond a boundary Rectangle\n> + */\n> +Rectangle Rectangle::boundedTo(const Rectangle &boundary) const\n> +{\n> +\tRectangle result(*this);\n> +\n> +\tresult.width = std::min(result.width, boundary.width);\n> +\tresult.x = std::clamp(result.x, boundary.x,\n> +\t\t\t\t boundary.x + boundary.width - result.width);\n> +\n> +\tresult.height = std::min(result.height, boundary.height);\n> +\tresult.y = std::clamp(result.y, boundary.y,\n> +\t\t\t\t boundary.y + boundary.height - result.height);\n> +\n> +\treturn result;\n> +}\n> +\n> /**\n> * \\brief Compare rectangles for equality\n> * \\return True if the two rectangles are equal, false otherwise\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 0BC6AC3B5C\n\tfor ;\n\tTue, 29 Sep 2020 19:40:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F58F621C9;\n\tTue, 29 Sep 2020 21:40:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C25E560394\n\tfor ;\n\tTue, 29 Sep 2020 21:40:41 +0200 (CEST)","from [192.168.0.20]\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 2B74A9CC;\n\tTue, 29 Sep 2020 21:40:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tBeej7cR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601408441;\n\tbh=yydaHiJBsaJucBMMSNLW9VYPsm++2ndS0yaWtHuJIvU=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=tBeej7cRPUh04df3dFMebR021eHQkFxbwUuE+Y7OovsEz0/kPVqCU0kY3qZ6rqQoZ\n\tvtLXSNGNEUctuRY/Dw8PCR6taVQ3fq4auk291cT9tCAl51BC0aQ4GYxwTEDMz+YK5B\n\tlkLe+035uOZGYJeyoSSoPbU9h+29l5cpIChKQnjY=","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20200929164000.15429-5-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Tue, 29 Sep 2020 20:40:38 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200929164000.15429-5-david.plowman@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: Add geometry helper\n\tfunctions","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13160,"web_url":"https://patchwork.libcamera.org/comment/13160/","msgid":"<20201011021020.GF3939@pendragon.ideasonboard.com>","date":"2020-10-11T02:10:20","subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: Add geometry helper\n\tfunctions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nThis looks good overall, but I'm afraid I have quite a few small\ncomments, possibly on the nitpicking side, given that I think these\nfunctions need to be very explicit. Once we agree on the details I can\nhelp with the implementation if desired.\n\nPlease also note that many of the comments below are open questions,\nespecially when they refer to naming of functions, so feel free to tell\nme when you think my proposals are not good.\n\nOn Tue, Sep 29, 2020 at 05:39:58PM +0100, David Plowman wrote:\n> These functions are aimed at making it easier to calculate cropping\n> rectangles, particularly in order to implement digital zoom.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n> ---\n> include/libcamera/geometry.h | 20 ++++++\n> src/libcamera/geometry.cpp | 129 +++++++++++++++++++++++++++++++++++\n\nWe need test cases for the new functions, in test/geometry.cpp.\n\n> 2 files changed, 149 insertions(+)\n> \n> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> index 02fb63c0..0447ee3e 100644\n> --- a/include/libcamera/geometry.h\n> +++ b/include/libcamera/geometry.h\n> @@ -13,6 +13,8 @@\n> \n> namespace libcamera {\n> \n> +class Rectangle;\n> +\n> class Size\n> {\n> public:\n> @@ -93,8 +95,17 @@ public:\n> \t\t\tstd::max(height, expand.height)\n> \t\t};\n> \t}\n> +\n> +\tSize alignedDownToAspectRatio(const Size &ratio) const;\n> +\tSize alignedUpToAspectRatio(const Size &ratio) const;\n\nThe names are a bit long but I don't have better alternatives to\npropose.\n\n> +\n> +\tRectangle centredTo(const Rectangle ®ion,\n> +\t\t\t int offsetX = 0, int offsetY = 0) const;\n> };\n> \n> +Size operator*(const Size &size, float f);\n> +Size operator/(const Size &size, float f);\n\nTo be complete, should we also have member operators that modify the\nsize in-place ?\n\n\tSize &operator*=(float f);\n\tSize &operator/=(float f);\n\n> +\n> bool operator==(const Size &lhs, const Size &rhs);\n> bool operator<(const Size &lhs, const Size &rhs);\n> \n> @@ -176,6 +187,11 @@ public:\n> \t{\n> \t}\n> \n> +\tconstexpr explicit Rectangle(const Size &size)\n\nNot saying it's a bad idea, but why do you think this constructor should\nbe explicit ?\n\n> +\t\t: x(0), y(0), width(size.width), height(size.height)\n> +\t{\n> +\t}\n> +\n> \tint x;\n> \tint y;\n> \tunsigned int width;\n> @@ -183,6 +199,10 @@ public:\n> \n> \tbool isNull() const { return !width && !height; }\n> \tconst std::string toString() const;\n> +\n> +\tSize size() const;\n> +\n> +\tRectangle boundedTo(const Rectangle &boundary) const;\n> };\n> \n> bool operator==(const Rectangle &lhs, const Rectangle &rhs);\n> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> index b12e1a62..649ee179 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -143,6 +143,88 @@ const std::string Size::toString() const\n> * height of this size and the \\a expand size\n> */\n> \n> +/**\n> + * \\brief Align down to the aspect ratio given by \\a ratio\n\nAligning down to an aspect ratio sounds a bit strange to me, as it's not\nreally about alignments. Would shrunkToAspectRatio() be a better name ?\nThe documentation could then become\n\n * \\brief Compute the size shrunk to achieve the given aspect \\a ratio\n * \\param[in] ratio The desired aspect ratio\n *\n * This function computes the largest size that fits in this size and\n * that matches the aspect ratio of \\ratio.\n *\n * \\return The size shrunk to the aspect \\ratio\n\nSame for the next function, named expandedToAspectRatio().\n\n> + * \\param[in] ratio The size whose aspect ratio to align down to\n> + * \\return A Size whose width and height are equal to the width and height\n> + * of this Size aligned down to the aspect ratio of \\a ratio\n> + */\n> +Size Size::alignedDownToAspectRatio(const Size &ratio) const\n> +{\n> +\tuint64_t ratio1 = static_cast(width) *\n> +\t\t\t static_cast(ratio.height);\n> +\tuint64_t ratio2 = static_cast(ratio.width) *\n> +\t\t\t static_cast(height);\n> +\n> +\tif (ratio1 > ratio2)\n> +\t\treturn { static_cast(ratio2 / ratio.height), height };\n> +\telse\n> +\t\treturn { width, static_cast(ratio1 / ratio.width) };\n> +}\n> +\n> +/**\n> + * \\brief Align up to the aspect ratio given by \\a ratio\n> + * \\param[in] ratio The size whose aspect ratio to align up to\n> + * \\return A Size whose width and height are equal to the width and height\n> + * of this Size aligned up to the aspect ratio of \\a ratio\n> + */\n> +Size Size::alignedUpToAspectRatio(const Size &ratio) const\n> +{\n> +\tuint64_t ratio1 = static_cast(width) *\n> +\t\t\t static_cast(ratio.height);\n> +\tuint64_t ratio2 = static_cast(ratio.width) *\n> +\t\t\t static_cast(height);\n> +\n> +\tif (ratio1 < ratio2)\n> +\t\treturn { static_cast(ratio2 / ratio.height), height };\n> +\telse\n> +\t\treturn { width, static_cast(ratio1 / ratio.width) };\n> +}\n> +\n> +/**\n> + * \\brief Centre a rectangle of this size within another rectangular region,\n> + * with optional offsets\n> + * \\param[in] region The rectangular region relative to which the returned\n> + * rectangle can be position\n> + * \\param[in] offsetX The X offset of the mid-point of the returned rectangle\n> + * relative to the mid-point of the region\n> + * \\param[in] offsetY The Y offset of the mid-point of the returned rectangle\n> + * relative to the mid-point of the region\n> + *\n> + * A Rectangle of this object's size is positioned within the Rectangle\n> + * given by \\a region. It is positioned so that its mid-point coincides\n> + * with the mid-point of \\a region, and is then further offset by the\n> + * values \\a offsetX and \\a offsetY.\n> + *\n> + * \\return A Rectangle with the horizontal and vertical sizes of\n> + * this Size instance, centred with offsets within a region\n> + */\n\nThis feels very ad-hoc, and the name isn't very explicit. Would it make\nsense to split it in the following basic operations ? I'll leave any\ndocumentation out as an exercise to see if their names are explicit.\n\nBefore:\n\nRectangle Size::centredTo(const Rectangle ®ion, int offsetX, int offsetY) const;\n\n\tSize size{ ... };\n\tRectangle region{ ... };\n\tint dx = ..., dy = ...;\n\n\tRectangle r = size.centredTo(region, dx, dy);\n\nAfter:\n\nclass Point\n{\npublic:\n\tPoint()\n\t\t: x(0), y(0)\n\t{\n\t}\n\n\tint x;\n\tint y;\n};\n\nPoint Rectangle::center() const;\nRectangle &Rectangle::moveCenter(const Point &position);\nRectangle &Rectangle::translate(int dx, int dx);\n\n\tSize size{ ... };\n\tRectangle region{ ... };\n\tint dx = ..., dy = ...;\n\n\tRectangle r{size};\n\tr.moveCenter(region.center()).translate(dx, dy);\n\nor\n\n\tRectangle r = Rectangle{size}.moveCenter(region.center()).translate(dx, dy);\n\nIt's a bit longer, but I think it's move explicit, and the operations\ncan be reused for other purposes.\n\n> +Rectangle Size::centredTo(const Rectangle ®ion, int offsetX, int offsetY) const\n> +{\n> +\tint x = (region.width - width) / 2 + region.x + offsetX;\n> +\tint y = (region.height - height) / 2 + region.y + offsetY;\n> +\n> +\treturn Rectangle(x, y, width, height);\n> +}\n> +\n> +/**\n> + * \\brief Scale size up by the given factor\n\n * \\param[in] size The size to scale\n * \\param[in] f The scaling factor\n\n> + * \\return The scaled Size\n> + */\n> +Size operator*(const Size &size, float f)\n\nI'd name the parameter factor.\n\n> +{\n> +\treturn Size(size.width * f, size.height * f);\n> +}\n> +\n> +/**\n> + * \\brief Scale size down by the given factor\n> + * \\return The scaled Size\n> + */\n> +Size operator/(const Size &size, float f)\n\nSame comments here.\n\n> +{\n> +\treturn Size(size.width / f, size.height / f);\n> +}\n> +\n> /**\n> * \\brief Compare sizes for equality\n> * \\return True if the two sizes are equal, false otherwise\n> @@ -365,6 +447,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)\n> * \\param[in] height The height\n> */\n> \n> +/**\n> + * \\fn Rectangle::Rectangle(const Size &size)\n> + * \\brief Construct a Rectangle with zero offsets and the given \\a size\n\nWe don't use a concept of \"offset\" for the Rectangle class at the\nmoment. I'd write this\n\n * \\brief Construct a Rectangle of \\a size located at (0,0)\n\n> + * \\param[in] size The desired Rectangle size\n> + */\n> +\n> /**\n> * \\var Rectangle::x\n> * \\brief The horizontal coordinate of the rectangle's top-left corner\n> @@ -404,6 +492,47 @@ const std::string Rectangle::toString() const\n> \treturn ss.str();\n> }\n> \n> +/**\n> + * \\brief Retrieve the size of this rectangle\n> + * \\return A Size reporting the Rectangle horizontal and vertical sizes\n\nI think \"The Rectangle size\" should be enough.\n\n> + */\n> +Size Rectangle::size() const\n> +{\n> +\treturn Size(width, height);\n> +}\n> +\n> +/**\n> + * \\brief Bound a Rectangle so as not to exceeed another Rectangle\n\nthis operation is also commonly referred to as computing the\nintersection of two rectangles. We would thus name the function\nintersection(), intersected() or intersectedWith(). The related\noperation of computing the union of two rectangles would then be named\nunion(), united() or unitedWith() I suppose, while with bountedTo() it\nwould be named expandedTo(). I don't have a strong preference.\n\nOn a separate note, if we want to shorten the code, we could also define\noperator& as an alias for this function (and operator| for the union).\n\n> + * \\param[in] boundary The limit that the returned Rectangle will not exceed\n> + *\n> + * The Rectangle is bounded so that it does not exceeed the given \\a boundary.\n> + * This process involves translating the Rectangle if any of its edges\n> + * lie beyond \\a boundary, so that those edges then lie along the boundary\n> + * instead.\n> + *\n> + * If either width or height are larger than \\a bounary, then the returned\n\ns/bounary/boundary/\n\n> + * Rectangle is clipped to be no larger. But other than this, the\n> + * Rectangle is not clipped or reduced in size, merely translated.\n> + *\n> + * We note that this is not a conventional Rectangle intersection function.\n\nSo much for my question above :-)\n\nHmmmm... I think the name is a bit misleading then, or at least not\nself-explicit. I fear this will cause confusion. I wonder if we could\nfind a better name, or a different set of standard operations that would\nresult in the same outcome when composed. I'll try to propose\nalternatives when reviewing the patch that uses this function.\n\n> + *\n> + * \\return A Rectangle that does not extend beyond a boundary Rectangle\n> + */\n> +Rectangle Rectangle::boundedTo(const Rectangle &boundary) const\n> +{\n> +\tRectangle result(*this);\n> +\n> +\tresult.width = std::min(result.width, boundary.width);\n> +\tresult.x = std::clamp(result.x, boundary.x,\n> +\t\t\t\t boundary.x + boundary.width - result.width);\n> +\n> +\tresult.height = std::min(result.height, boundary.height);\n> +\tresult.y = std::clamp(result.y, boundary.y,\n> +\t\t\t\t boundary.y + boundary.height - result.height);\n> +\n> +\treturn result;\n> +}\n> +\n> /**\n> * \\brief Compare rectangles for equality\n> * \\return True if the two rectangles are equal, false otherwise","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 E39D5BEEE0\n\tfor ;\n\tSun, 11 Oct 2020 02:11:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BCF7605BE;\n\tSun, 11 Oct 2020 04:11:06 +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 E6B9B60358\n\tfor ;\n\tSun, 11 Oct 2020 04:11:04 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 60876528;\n\tSun, 11 Oct 2020 04:11:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cTEbJ288\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602382264;\n\tbh=hentBfb0h19E+elFZFWL7JF+tHqw7hDv5RWlmpt1KBw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cTEbJ2880321WqBc57/dM9DwkNHS49lT7TdeTwyIS3eQ4ll5oPTI5AwXhH9XpDY4d\n\tnW73dw5ynrhggGgnhVl7qjRkZ2gK2mARiHx+qMmoSSljK/L/NF9Q9zPJhZsF++XDps\n\tHkrGdJZcvF0WMIoUdVExbIfUS+/RqwIzFGvHH7jo=","Date":"Sun, 11 Oct 2020 05:10:20 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201011021020.GF3939@pendragon.ideasonboard.com>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20200929164000.15429-5-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200929164000.15429-5-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: Add geometry helper\n\tfunctions","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13175,"web_url":"https://patchwork.libcamera.org/comment/13175/","msgid":"","date":"2020-10-12T10:19:09","subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: Add geometry helper\n\tfunctions","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"On Sun, 11 Oct 2020 at 03:11, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> This looks good overall, but I'm afraid I have quite a few small\n> comments, possibly on the nitpicking side, given that I think these\n> functions need to be very explicit. Once we agree on the details I can\n> help with the implementation if desired.\n>\n> Please also note that many of the comments below are open questions,\n> especially when they refer to naming of functions, so feel free to tell\n> me when you think my proposals are not good.\n>\n> On Tue, Sep 29, 2020 at 05:39:58PM +0100, David Plowman wrote:\n> > These functions are aimed at making it easier to calculate cropping\n> > rectangles, particularly in order to implement digital zoom.\n> >\n> > Signed-off-by: David Plowman \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > include/libcamera/geometry.h | 20 ++++++\n> > src/libcamera/geometry.cpp | 129 +++++++++++++++++++++++++++++++++++\n>\n> We need test cases for the new functions, in test/geometry.cpp.\n>\n> > 2 files changed, 149 insertions(+)\n> >\n> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > index 02fb63c0..0447ee3e 100644\n> > --- a/include/libcamera/geometry.h\n> > +++ b/include/libcamera/geometry.h\n> > @@ -13,6 +13,8 @@\n> >\n> > namespace libcamera {\n> >\n> > +class Rectangle;\n> > +\n> > class Size\n> > {\n> > public:\n> > @@ -93,8 +95,17 @@ public:\n> > std::max(height, expand.height)\n> > };\n> > }\n> > +\n> > + Size alignedDownToAspectRatio(const Size &ratio) const;\n> > + Size alignedUpToAspectRatio(const Size &ratio) const;\n>\n> The names are a bit long but I don't have better alternatives to\n> propose.\n\nMaybe \"Ar\" instead of \"AspectRatio\"? e.g. alignedDownToAr\nBut I'm not convinced. Nor do I think it matters greatly...\n\nIn line with your comment just below (\"To be complete\"), do you think\nwe should include in-place versions of all these functions,\nalginUpToAspectRatio, centreTo etc.?\n\n>\n> > +\n> > + Rectangle centredTo(const Rectangle ®ion,\n> > + int offsetX = 0, int offsetY = 0) const;\n> > };\n> >\n> > +Size operator*(const Size &size, float f);\n> > +Size operator/(const Size &size, float f);\n>\n> To be complete, should we also have member operators that modify the\n> size in-place ?\n>\n> Size &operator*=(float f);\n> Size &operator/=(float f);\n\nAgreed - why not!\n\n>\n> > +\n> > bool operator==(const Size &lhs, const Size &rhs);\n> > bool operator<(const Size &lhs, const Size &rhs);\n> >\n> > @@ -176,6 +187,11 @@ public:\n> > {\n> > }\n> >\n> > + constexpr explicit Rectangle(const Size &size)\n>\n> Not saying it's a bad idea, but why do you think this constructor should\n> be explicit ?\n\nI think I'm a bit paranoid. If you did a calculation with various\nRectangles and Sizes flying around, and then you want (for example,\nand borrowing the translate function from below...)\n\nanswer = rect.translate(dx, dy)\n\nbut wrote\n\nanswer = size.translate(dx, dy)\n\nIt's a question of whether you want the compiler to catch those and\nforce you to say exactly what you mean. Maybe you end up with\nsomething a little clearer to understand too. Again, I don't really\nmind...\n\n>\n> > + : x(0), y(0), width(size.width), height(size.height)\n> > + {\n> > + }\n> > +\n> > int x;\n> > int y;\n> > unsigned int width;\n> > @@ -183,6 +199,10 @@ public:\n> >\n> > bool isNull() const { return !width && !height; }\n> > const std::string toString() const;\n> > +\n> > + Size size() const;\n> > +\n> > + Rectangle boundedTo(const Rectangle &boundary) const;\n> > };\n> >\n> > bool operator==(const Rectangle &lhs, const Rectangle &rhs);\n> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > index b12e1a62..649ee179 100644\n> > --- a/src/libcamera/geometry.cpp\n> > +++ b/src/libcamera/geometry.cpp\n> > @@ -143,6 +143,88 @@ const std::string Size::toString() const\n> > * height of this size and the \\a expand size\n> > */\n> >\n> > +/**\n> > + * \\brief Align down to the aspect ratio given by \\a ratio\n>\n> Aligning down to an aspect ratio sounds a bit strange to me, as it's not\n> really about alignments. Would shrunkToAspectRatio() be a better name ?\n> The documentation could then become\n>\n> * \\brief Compute the size shrunk to achieve the given aspect \\a ratio\n> * \\param[in] ratio The desired aspect ratio\n> *\n> * This function computes the largest size that fits in this size and\n> * that matches the aspect ratio of \\ratio.\n> *\n> * \\return The size shrunk to the aspect \\ratio\n>\n> Same for the next function, named expandedToAspectRatio().\n\nYes, shrunk/expanded sounds good to me. An alternative might be\nboundedToAspectRatio (instead of shrunk), as we tend to use bounded\nelsewhere.\n\n>\n> > + * \\param[in] ratio The size whose aspect ratio to align down to\n> > + * \\return A Size whose width and height are equal to the width and height\n> > + * of this Size aligned down to the aspect ratio of \\a ratio\n> > + */\n> > +Size Size::alignedDownToAspectRatio(const Size &ratio) const\n> > +{\n> > + uint64_t ratio1 = static_cast(width) *\n> > + static_cast(ratio.height);\n> > + uint64_t ratio2 = static_cast(ratio.width) *\n> > + static_cast(height);\n> > +\n> > + if (ratio1 > ratio2)\n> > + return { static_cast(ratio2 / ratio.height), height };\n> > + else\n> > + return { width, static_cast(ratio1 / ratio.width) };\n> > +}\n> > +\n> > +/**\n> > + * \\brief Align up to the aspect ratio given by \\a ratio\n> > + * \\param[in] ratio The size whose aspect ratio to align up to\n> > + * \\return A Size whose width and height are equal to the width and height\n> > + * of this Size aligned up to the aspect ratio of \\a ratio\n> > + */\n> > +Size Size::alignedUpToAspectRatio(const Size &ratio) const\n> > +{\n> > + uint64_t ratio1 = static_cast(width) *\n> > + static_cast(ratio.height);\n> > + uint64_t ratio2 = static_cast(ratio.width) *\n> > + static_cast(height);\n> > +\n> > + if (ratio1 < ratio2)\n> > + return { static_cast(ratio2 / ratio.height), height };\n> > + else\n> > + return { width, static_cast(ratio1 / ratio.width) };\n> > +}\n> > +\n> > +/**\n> > + * \\brief Centre a rectangle of this size within another rectangular region,\n> > + * with optional offsets\n> > + * \\param[in] region The rectangular region relative to which the returned\n> > + * rectangle can be position\n> > + * \\param[in] offsetX The X offset of the mid-point of the returned rectangle\n> > + * relative to the mid-point of the region\n> > + * \\param[in] offsetY The Y offset of the mid-point of the returned rectangle\n> > + * relative to the mid-point of the region\n> > + *\n> > + * A Rectangle of this object's size is positioned within the Rectangle\n> > + * given by \\a region. It is positioned so that its mid-point coincides\n> > + * with the mid-point of \\a region, and is then further offset by the\n> > + * values \\a offsetX and \\a offsetY.\n> > + *\n> > + * \\return A Rectangle with the horizontal and vertical sizes of\n> > + * this Size instance, centred with offsets within a region\n> > + */\n>\n> This feels very ad-hoc, and the name isn't very explicit. Would it make\n> sense to split it in the following basic operations ? I'll leave any\n> documentation out as an exercise to see if their names are explicit.\n>\n> Before:\n>\n> Rectangle Size::centredTo(const Rectangle ®ion, int offsetX, int offsetY) const;\n>\n> Size size{ ... };\n> Rectangle region{ ... };\n> int dx = ..., dy = ...;\n>\n> Rectangle r = size.centredTo(region, dx, dy);\n>\n> After:\n>\n> class Point\n> {\n> public:\n> Point()\n> : x(0), y(0)\n> {\n> }\n>\n> int x;\n> int y;\n> };\n>\n> Point Rectangle::center() const;\n> Rectangle &Rectangle::moveCenter(const Point &position);\n> Rectangle &Rectangle::translate(int dx, int dx);\n>\n> Size size{ ... };\n> Rectangle region{ ... };\n> int dx = ..., dy = ...;\n>\n> Rectangle r{size};\n> r.moveCenter(region.center()).translate(dx, dy);\n>\n> or\n>\n> Rectangle r = Rectangle{size}.moveCenter(region.center()).translate(dx, dy);\n>\n> It's a bit longer, but I think it's move explicit, and the operations\n> can be reused for other purposes.\n\nI think that's a fair cop, as we say!\n\nBTW, do we have a policy on UK vs. American spelling? (centre/center)\n\nI think I still prefer something like \"centredTo\" instead of\n\"moveCenter\", it feels more like the other functions. An in-place\nversion could be \"centreTo\". How about \"translateBy\" and\n\"translatedBy\" instead of \"translate\"...?\n\n>\n> > +Rectangle Size::centredTo(const Rectangle ®ion, int offsetX, int offsetY) const\n> > +{\n> > + int x = (region.width - width) / 2 + region.x + offsetX;\n> > + int y = (region.height - height) / 2 + region.y + offsetY;\n> > +\n> > + return Rectangle(x, y, width, height);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Scale size up by the given factor\n>\n> * \\param[in] size The size to scale\n> * \\param[in] f The scaling factor\n>\n> > + * \\return The scaled Size\n> > + */\n> > +Size operator*(const Size &size, float f)\n>\n> I'd name the parameter factor.\n>\n> > +{\n> > + return Size(size.width * f, size.height * f);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Scale size down by the given factor\n> > + * \\return The scaled Size\n> > + */\n> > +Size operator/(const Size &size, float f)\n>\n> Same comments here.\n>\n> > +{\n> > + return Size(size.width / f, size.height / f);\n> > +}\n> > +\n> > /**\n> > * \\brief Compare sizes for equality\n> > * \\return True if the two sizes are equal, false otherwise\n> > @@ -365,6 +447,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)\n> > * \\param[in] height The height\n> > */\n> >\n> > +/**\n> > + * \\fn Rectangle::Rectangle(const Size &size)\n> > + * \\brief Construct a Rectangle with zero offsets and the given \\a size\n>\n> We don't use a concept of \"offset\" for the Rectangle class at the\n> moment. I'd write this\n>\n> * \\brief Construct a Rectangle of \\a size located at (0,0)\n\nSure. I might like \"top left corner located at...\" given that there's\nincreasing amounts of \"centering\" going-on.\n\n>\n> > + * \\param[in] size The desired Rectangle size\n> > + */\n> > +\n> > /**\n> > * \\var Rectangle::x\n> > * \\brief The horizontal coordinate of the rectangle's top-left corner\n> > @@ -404,6 +492,47 @@ const std::string Rectangle::toString() const\n> > return ss.str();\n> > }\n> >\n> > +/**\n> > + * \\brief Retrieve the size of this rectangle\n> > + * \\return A Size reporting the Rectangle horizontal and vertical sizes\n>\n> I think \"The Rectangle size\" should be enough.\n>\n> > + */\n> > +Size Rectangle::size() const\n> > +{\n> > + return Size(width, height);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Bound a Rectangle so as not to exceeed another Rectangle\n>\n> this operation is also commonly referred to as computing the\n> intersection of two rectangles. We would thus name the function\n> intersection(), intersected() or intersectedWith(). The related\n> operation of computing the union of two rectangles would then be named\n> union(), united() or unitedWith() I suppose, while with bountedTo() it\n> would be named expandedTo(). I don't have a strong preference.\n>\n> On a separate note, if we want to shorten the code, we could also define\n> operator& as an alias for this function (and operator| for the union).\n>\n> > + * \\param[in] boundary The limit that the returned Rectangle will not exceed\n> > + *\n> > + * The Rectangle is bounded so that it does not exceeed the given \\a boundary.\n> > + * This process involves translating the Rectangle if any of its edges\n> > + * lie beyond \\a boundary, so that those edges then lie along the boundary\n> > + * instead.\n> > + *\n> > + * If either width or height are larger than \\a bounary, then the returned\n>\n> s/bounary/boundary/\n>\n> > + * Rectangle is clipped to be no larger. But other than this, the\n> > + * Rectangle is not clipped or reduced in size, merely translated.\n> > + *\n> > + * We note that this is not a conventional Rectangle intersection function.\n>\n> So much for my question above :-)\n>\n> Hmmmm... I think the name is a bit misleading then, or at least not\n> self-explicit. I fear this will cause confusion. I wonder if we could\n> find a better name, or a different set of standard operations that would\n> result in the same outcome when composed. I'll try to propose\n> alternatives when reviewing the patch that uses this function.\n\nI guess there are two operations going on:\n\n* Limiting the size of the rectangle (to be no larger than the boundary).\n\n* Translating it so that no part of it exceeds the boundary.\n\nFor the first one... well, it could stay as \"boundedTo\". Might take a\nSize rather than a Rectangle as the parameter? However it's not very\nclear what happens to the top left corner, though in this case it\ndoesn't matter.\n\nFor the second one... \"restrictedTo\"? or even \"pannedTo\" (in the sense\nthat the source Rectangle is being \"panned\" to lie within the\nboundary)? perhaps \"shiftedTo\" is less weird.\n\nTricky!\n\nThanks and best regards\nDavid\n\n>\n> > + *\n> > + * \\return A Rectangle that does not extend beyond a boundary Rectangle\n> > + */\n> > +Rectangle Rectangle::boundedTo(const Rectangle &boundary) const\n> > +{\n> > + Rectangle result(*this);\n> > +\n> > + result.width = std::min(result.width, boundary.width);\n> > + result.x = std::clamp(result.x, boundary.x,\n> > + boundary.x + boundary.width - result.width);\n> > +\n> > + result.height = std::min(result.height, boundary.height);\n> > + result.y = std::clamp(result.y, boundary.y,\n> > + boundary.y + boundary.height - result.height);\n> > +\n> > + return result;\n> > +}\n> > +\n> > /**\n> > * \\brief Compare rectangles for equality\n> > * \\return True if the two rectangles are equal, false otherwise\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 53283BEEE0\n\tfor ;\n\tMon, 12 Oct 2020 10:19:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5B6160D55;\n\tMon, 12 Oct 2020 12:19:23 +0200 (CEST)","from mail-oo1-xc35.google.com (mail-oo1-xc35.google.com\n\t[IPv6:2607:f8b0:4864:20::c35])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F4E3600F2\n\tfor ;\n\tMon, 12 Oct 2020 12:19:22 +0200 (CEST)","by mail-oo1-xc35.google.com with SMTP id c10so572112oon.6\n\tfor ;\n\tMon, 12 Oct 2020 03:19:22 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Qd+QpVaJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=fYbJAn29YeRRVN7lDHwW+d+rkkqk9GkqF9AMcMOtuXw=;\n\tb=Qd+QpVaJMTsfXJYzuz43H9OPcfm7WpD1K5/CRbdtJOO1uAtLO+0RloeTxonVUfYsar\n\twkBC4EvBycYmRqgWA7gf6EP8SrMlm022JuEkjkviXIyvaYuyEZeB4436b0k5zUa/9VrE\n\tFNqsKArqh/x/gqb/tfPQu0+41lCWU6VqBV26G8lMshEFaTTNowna9jLceXnytC5a3E9Y\n\tmZqt75mPfioHX0EVkaVqz2g79KkgBovlr3kFjiYqpJFuPbablRVMepOiR2Sp46akjalv\n\t0G05iVUEoqaeuJwRFO4SqO91OUyTdFK5LuRRJS1m0cEKde0sRu9Ilgead95KK5cnpwNa\n\tOZ+A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=fYbJAn29YeRRVN7lDHwW+d+rkkqk9GkqF9AMcMOtuXw=;\n\tb=POc+GC7kejrQSqrQ6qYhzQFhXW+13qL3OxKWnhuyvEvcWaHptNjazg9ua+BtlKXaS4\n\tmOb4J/M2hovfYP46hkuNr0GLj1iMPEPROFYWS72V2p4dkQz1jZrHEBL2ApKZfowDhPbH\n\tDMtloi3pr1IMjf6JDFcX0nBqDUuOYO97t/2TApllnQXq9uRIQybih6dkj0dm2LvLMw4d\n\toxQo9+y0eJi20qiT0y07rYtvdgLTGzvOruTwUZOCXzBF8NayzRtp3xwFreuf8hJN9aTF\n\tWU0aifjbzSqhhBSYGm9fj2HirN/eLUQBZUsbyjISOrIkornSEnGbioJs9OgsMXI9V98z\n\tO9+Q==","X-Gm-Message-State":"AOAM5323HFQ2ILCFVhnVf4EckhLguHGhyG4u6sD6srZXBljWpmQafZ/i\n\t67TYLqUdo1HsU1qMunK3DIc0K8YhnrwfSDHeHudEqw==","X-Google-Smtp-Source":"ABdhPJxDBSVBeE2c9R/VfOxDS9eA8mbXak6Ov58Tpe1aAdWunaStFWHulMn+/B4sgmFztAUGsIrZ08Q9SC0RJu02G7U=","X-Received":"by 2002:a4a:e592:: with SMTP id\n\to18mr17593700oov.28.1602497960727; \n\tMon, 12 Oct 2020 03:19:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20200929164000.15429-5-david.plowman@raspberrypi.com>\n\t<20201011021020.GF3939@pendragon.ideasonboard.com>","In-Reply-To":"<20201011021020.GF3939@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Mon, 12 Oct 2020 11:19:09 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: Add geometry helper\n\tfunctions","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13181,"web_url":"https://patchwork.libcamera.org/comment/13181/","msgid":"<20201013012234.GC3942@pendragon.ideasonboard.com>","date":"2020-10-13T01:22:34","subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: Add geometry helper\n\tfunctions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Mon, Oct 12, 2020 at 11:19:09AM +0100, David Plowman wrote:\n> On Sun, 11 Oct 2020 at 03:11, Laurent Pinchart wrote:\n> >\n> > Hi David,\n> >\n> > Thank you for the patch.\n> >\n> > This looks good overall, but I'm afraid I have quite a few small\n> > comments, possibly on the nitpicking side, given that I think these\n> > functions need to be very explicit. Once we agree on the details I can\n> > help with the implementation if desired.\n> >\n> > Please also note that many of the comments below are open questions,\n> > especially when they refer to naming of functions, so feel free to tell\n> > me when you think my proposals are not good.\n> >\n> > On Tue, Sep 29, 2020 at 05:39:58PM +0100, David Plowman wrote:\n> > > These functions are aimed at making it easier to calculate cropping\n> > > rectangles, particularly in order to implement digital zoom.\n> > >\n> > > Signed-off-by: David Plowman \n> > > Reviewed-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/geometry.h | 20 ++++++\n> > > src/libcamera/geometry.cpp | 129 +++++++++++++++++++++++++++++++++++\n> >\n> > We need test cases for the new functions, in test/geometry.cpp.\n> >\n> > > 2 files changed, 149 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > index 02fb63c0..0447ee3e 100644\n> > > --- a/include/libcamera/geometry.h\n> > > +++ b/include/libcamera/geometry.h\n> > > @@ -13,6 +13,8 @@\n> > >\n> > > namespace libcamera {\n> > >\n> > > +class Rectangle;\n> > > +\n> > > class Size\n> > > {\n> > > public:\n> > > @@ -93,8 +95,17 @@ public:\n> > > std::max(height, expand.height)\n> > > };\n> > > }\n> > > +\n> > > + Size alignedDownToAspectRatio(const Size &ratio) const;\n> > > + Size alignedUpToAspectRatio(const Size &ratio) const;\n> >\n> > The names are a bit long but I don't have better alternatives to\n> > propose.\n> \n> Maybe \"Ar\" instead of \"AspectRatio\"? e.g. alignedDownToAr\n> But I'm not convinced. Nor do I think it matters greatly...\n\nI'm not convinced either, Ar would be a bit cryptic I think.\n\n> In line with your comment just below (\"To be complete\"), do you think\n> we should include in-place versions of all these functions,\n> alginUpToAspectRatio, centreTo etc.?\n\nThey could be useful. I'm fine leaving them out for now and adding them\nlater, or biting the bullet already.\n\n> > > +\n> > > + Rectangle centredTo(const Rectangle ®ion,\n> > > + int offsetX = 0, int offsetY = 0) const;\n> > > };\n> > >\n> > > +Size operator*(const Size &size, float f);\n> > > +Size operator/(const Size &size, float f);\n> >\n> > To be complete, should we also have member operators that modify the\n> > size in-place ?\n> >\n> > Size &operator*=(float f);\n> > Size &operator/=(float f);\n> \n> Agreed - why not!\n> \n> > > +\n> > > bool operator==(const Size &lhs, const Size &rhs);\n> > > bool operator<(const Size &lhs, const Size &rhs);\n> > >\n> > > @@ -176,6 +187,11 @@ public:\n> > > {\n> > > }\n> > >\n> > > + constexpr explicit Rectangle(const Size &size)\n> >\n> > Not saying it's a bad idea, but why do you think this constructor should\n> > be explicit ?\n> \n> I think I'm a bit paranoid. If you did a calculation with various\n> Rectangles and Sizes flying around, and then you want (for example,\n> and borrowing the translate function from below...)\n> \n> answer = rect.translate(dx, dy)\n> \n> but wrote\n> \n> answer = size.translate(dx, dy)\n\nThis one wouldn't compile as Size::translate() doesn't exist. I do see\nyour point though.\n\n> It's a question of whether you want the compiler to catch those and\n> force you to say exactly what you mean. Maybe you end up with\n> something a little clearer to understand too. Again, I don't really\n> mind...\n\nI don't really mind either, I was just curious.\n\n> > > + : x(0), y(0), width(size.width), height(size.height)\n> > > + {\n> > > + }\n> > > +\n> > > int x;\n> > > int y;\n> > > unsigned int width;\n> > > @@ -183,6 +199,10 @@ public:\n> > >\n> > > bool isNull() const { return !width && !height; }\n> > > const std::string toString() const;\n> > > +\n> > > + Size size() const;\n> > > +\n> > > + Rectangle boundedTo(const Rectangle &boundary) const;\n> > > };\n> > >\n> > > bool operator==(const Rectangle &lhs, const Rectangle &rhs);\n> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > index b12e1a62..649ee179 100644\n> > > --- a/src/libcamera/geometry.cpp\n> > > +++ b/src/libcamera/geometry.cpp\n> > > @@ -143,6 +143,88 @@ const std::string Size::toString() const\n> > > * height of this size and the \\a expand size\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Align down to the aspect ratio given by \\a ratio\n> >\n> > Aligning down to an aspect ratio sounds a bit strange to me, as it's not\n> > really about alignments. Would shrunkToAspectRatio() be a better name ?\n> > The documentation could then become\n> >\n> > * \\brief Compute the size shrunk to achieve the given aspect \\a ratio\n> > * \\param[in] ratio The desired aspect ratio\n> > *\n> > * This function computes the largest size that fits in this size and\n> > * that matches the aspect ratio of \\ratio.\n> > *\n> > * \\return The size shrunk to the aspect \\ratio\n> >\n> > Same for the next function, named expandedToAspectRatio().\n> \n> Yes, shrunk/expanded sounds good to me. An alternative might be\n> boundedToAspectRatio (instead of shrunk), as we tend to use bounded\n> elsewhere.\n\nYes that sounds good.\n\nI stumbled upon https://doc.qt.io/qt-5/qsize.html#scale-1 today (while\nresearching something else, but I often get inspiration from Qt when it\ncomes to API design). Do you think it would be a good option ? Maybe we\ncan borrow the flag idea, and have\n\nclass Size\n{\npublic:\n\tenum AspectRatioMode {\n\t\tIgnoreAspectRatio,\n\t\tKeepAspectRatio,\n\t};\n\n\tconstexpr Size boundedTo(const Size &bound,\n\t\t\t\t AspectRatioMode mode = IgnoreAspectRatio);\n\tconstexpr Size expandedTo(const Size &expand,\n\t\t\t\t AspectRatioMode mode = IgnoreAspectRatio);\n\t...\n};\n\nBut now that I've written this, I think we would have two very different\nsemantics for the same function, the function would compute min/max when\nmode is IgnoreAspectRatio, and scale otherwise. I think that would be\nconfusing. I'd rather go for scale()/scaled() as Qt (not sure about\nIgnoreAspectRatio though, that seems fairly useless in this case, we\nshould probably have two modes only, and possibly rename them), or keep\nboundedToAspectRatio()/expandedToAspectRatio().\n\n> > > + * \\param[in] ratio The size whose aspect ratio to align down to\n> > > + * \\return A Size whose width and height are equal to the width and height\n> > > + * of this Size aligned down to the aspect ratio of \\a ratio\n> > > + */\n> > > +Size Size::alignedDownToAspectRatio(const Size &ratio) const\n> > > +{\n> > > + uint64_t ratio1 = static_cast(width) *\n> > > + static_cast(ratio.height);\n> > > + uint64_t ratio2 = static_cast(ratio.width) *\n> > > + static_cast(height);\n> > > +\n> > > + if (ratio1 > ratio2)\n> > > + return { static_cast(ratio2 / ratio.height), height };\n> > > + else\n> > > + return { width, static_cast(ratio1 / ratio.width) };\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Align up to the aspect ratio given by \\a ratio\n> > > + * \\param[in] ratio The size whose aspect ratio to align up to\n> > > + * \\return A Size whose width and height are equal to the width and height\n> > > + * of this Size aligned up to the aspect ratio of \\a ratio\n> > > + */\n> > > +Size Size::alignedUpToAspectRatio(const Size &ratio) const\n> > > +{\n> > > + uint64_t ratio1 = static_cast(width) *\n> > > + static_cast(ratio.height);\n> > > + uint64_t ratio2 = static_cast(ratio.width) *\n> > > + static_cast(height);\n> > > +\n> > > + if (ratio1 < ratio2)\n> > > + return { static_cast(ratio2 / ratio.height), height };\n> > > + else\n> > > + return { width, static_cast(ratio1 / ratio.width) };\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Centre a rectangle of this size within another rectangular region,\n> > > + * with optional offsets\n> > > + * \\param[in] region The rectangular region relative to which the returned\n> > > + * rectangle can be position\n> > > + * \\param[in] offsetX The X offset of the mid-point of the returned rectangle\n> > > + * relative to the mid-point of the region\n> > > + * \\param[in] offsetY The Y offset of the mid-point of the returned rectangle\n> > > + * relative to the mid-point of the region\n> > > + *\n> > > + * A Rectangle of this object's size is positioned within the Rectangle\n> > > + * given by \\a region. It is positioned so that its mid-point coincides\n> > > + * with the mid-point of \\a region, and is then further offset by the\n> > > + * values \\a offsetX and \\a offsetY.\n> > > + *\n> > > + * \\return A Rectangle with the horizontal and vertical sizes of\n> > > + * this Size instance, centred with offsets within a region\n> > > + */\n> >\n> > This feels very ad-hoc, and the name isn't very explicit. Would it make\n> > sense to split it in the following basic operations ? I'll leave any\n> > documentation out as an exercise to see if their names are explicit.\n> >\n> > Before:\n> >\n> > Rectangle Size::centredTo(const Rectangle ®ion, int offsetX, int offsetY) const;\n> >\n> > Size size{ ... };\n> > Rectangle region{ ... };\n> > int dx = ..., dy = ...;\n> >\n> > Rectangle r = size.centredTo(region, dx, dy);\n> >\n> > After:\n> >\n> > class Point\n> > {\n> > public:\n> > Point()\n> > : x(0), y(0)\n> > {\n> > }\n> >\n> > int x;\n> > int y;\n> > };\n> >\n> > Point Rectangle::center() const;\n> > Rectangle &Rectangle::moveCenter(const Point &position);\n> > Rectangle &Rectangle::translate(int dx, int dx);\n> >\n> > Size size{ ... };\n> > Rectangle region{ ... };\n> > int dx = ..., dy = ...;\n> >\n> > Rectangle r{size};\n> > r.moveCenter(region.center()).translate(dx, dy);\n> >\n> > or\n> >\n> > Rectangle r = Rectangle{size}.moveCenter(region.center()).translate(dx, dy);\n> >\n> > It's a bit longer, but I think it's move explicit, and the operations\n> > can be reused for other purposes.\n> \n> I think that's a fair cop, as we say!\n> \n> BTW, do we have a policy on UK vs. American spelling? (centre/center)\n\nWe've discussed that internally in the past, and why we would all prefer\nto go for a British spelling (the documentation currently uses Oxford\nEnglish), I think it will be a lost battle when it comes to API\nelements. Developers expect analog, color and center :-( I think we'll\nneed to go through the code base and rename the functions at some point.\nWhile I'd like to keep the Oxford English spelling for the\ndocumentation, I'm not sure the majority will agree, given that there\nwill then be a mismatch between the code and the documentation.\n\nIf only American spoke English :-)\n\n> I think I still prefer something like \"centredTo\" instead of\n> \"moveCenter\", it feels more like the other functions. An in-place\n> version could be \"centreTo\".\n\nNote that moveCenter() is the in-place version. But otherwise it sounds\ngood to me, moveCenter() doesn't give us a good name for the\nnon-in-place version.\n\n> How about \"translateBy\" and \"translatedBy\" instead of \"translate\"...?\n\nWhile I usually try to keep names short and fairly fluid to read, I\nthink standardizing the geometry functions with a suffix, or without\none, would make sense.\n\n> > > +Rectangle Size::centredTo(const Rectangle ®ion, int offsetX, int offsetY) const\n> > > +{\n> > > + int x = (region.width - width) / 2 + region.x + offsetX;\n> > > + int y = (region.height - height) / 2 + region.y + offsetY;\n> > > +\n> > > + return Rectangle(x, y, width, height);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Scale size up by the given factor\n> >\n> > * \\param[in] size The size to scale\n> > * \\param[in] f The scaling factor\n> >\n> > > + * \\return The scaled Size\n> > > + */\n> > > +Size operator*(const Size &size, float f)\n> >\n> > I'd name the parameter factor.\n> >\n> > > +{\n> > > + return Size(size.width * f, size.height * f);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Scale size down by the given factor\n> > > + * \\return The scaled Size\n> > > + */\n> > > +Size operator/(const Size &size, float f)\n> >\n> > Same comments here.\n> >\n> > > +{\n> > > + return Size(size.width / f, size.height / f);\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Compare sizes for equality\n> > > * \\return True if the two sizes are equal, false otherwise\n> > > @@ -365,6 +447,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)\n> > > * \\param[in] height The height\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn Rectangle::Rectangle(const Size &size)\n> > > + * \\brief Construct a Rectangle with zero offsets and the given \\a size\n> >\n> > We don't use a concept of \"offset\" for the Rectangle class at the\n> > moment. I'd write this\n> >\n> > * \\brief Construct a Rectangle of \\a size located at (0,0)\n> \n> Sure. I might like \"top left corner located at...\" given that there's\n> increasing amounts of \"centering\" going-on.\n\nSounds good to me.\n\n> > > + * \\param[in] size The desired Rectangle size\n> > > + */\n> > > +\n> > > /**\n> > > * \\var Rectangle::x\n> > > * \\brief The horizontal coordinate of the rectangle's top-left corner\n> > > @@ -404,6 +492,47 @@ const std::string Rectangle::toString() const\n> > > return ss.str();\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Retrieve the size of this rectangle\n> > > + * \\return A Size reporting the Rectangle horizontal and vertical sizes\n> >\n> > I think \"The Rectangle size\" should be enough.\n> >\n> > > + */\n> > > +Size Rectangle::size() const\n> > > +{\n> > > + return Size(width, height);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Bound a Rectangle so as not to exceeed another Rectangle\n> >\n> > this operation is also commonly referred to as computing the\n> > intersection of two rectangles. We would thus name the function\n> > intersection(), intersected() or intersectedWith(). The related\n> > operation of computing the union of two rectangles would then be named\n> > union(), united() or unitedWith() I suppose, while with bountedTo() it\n> > would be named expandedTo(). I don't have a strong preference.\n> >\n> > On a separate note, if we want to shorten the code, we could also define\n> > operator& as an alias for this function (and operator| for the union).\n> >\n> > > + * \\param[in] boundary The limit that the returned Rectangle will not exceed\n> > > + *\n> > > + * The Rectangle is bounded so that it does not exceeed the given \\a boundary.\n> > > + * This process involves translating the Rectangle if any of its edges\n> > > + * lie beyond \\a boundary, so that those edges then lie along the boundary\n> > > + * instead.\n> > > + *\n> > > + * If either width or height are larger than \\a bounary, then the returned\n> >\n> > s/bounary/boundary/\n> >\n> > > + * Rectangle is clipped to be no larger. But other than this, the\n> > > + * Rectangle is not clipped or reduced in size, merely translated.\n> > > + *\n> > > + * We note that this is not a conventional Rectangle intersection function.\n> >\n> > So much for my question above :-)\n> >\n> > Hmmmm... I think the name is a bit misleading then, or at least not\n> > self-explicit. I fear this will cause confusion. I wonder if we could\n> > find a better name, or a different set of standard operations that would\n> > result in the same outcome when composed. I'll try to propose\n> > alternatives when reviewing the patch that uses this function.\n> \n> I guess there are two operations going on:\n> \n> * Limiting the size of the rectangle (to be no larger than the boundary).\n> \n> * Translating it so that no part of it exceeds the boundary.\n> \n> For the first one... well, it could stay as \"boundedTo\". Might take a\n> Size rather than a Rectangle as the parameter? However it's not very\n> clear what happens to the top left corner, though in this case it\n> doesn't matter.\n> \n> For the second one... \"restrictedTo\"? or even \"pannedTo\" (in the sense\n> that the source Rectangle is being \"panned\" to lie within the\n> boundary)? perhaps \"shiftedTo\" is less weird.\n> \n> Tricky!\n\nA bit :-) I need to check the code that uses this first.\n\n> > > + *\n> > > + * \\return A Rectangle that does not extend beyond a boundary Rectangle\n> > > + */\n> > > +Rectangle Rectangle::boundedTo(const Rectangle &boundary) const\n> > > +{\n> > > + Rectangle result(*this);\n> > > +\n> > > + result.width = std::min(result.width, boundary.width);\n> > > + result.x = std::clamp(result.x, boundary.x,\n> > > + boundary.x + boundary.width - result.width);\n> > > +\n> > > + result.height = std::min(result.height, boundary.height);\n> > > + result.y = std::clamp(result.y, boundary.y,\n> > > + boundary.y + boundary.height - result.height);\n> > > +\n> > > + return result;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Compare rectangles for equality\n> > > * \\return True if the two rectangles are equal, false otherwise","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 5524BBEEDF\n\tfor ;\n\tTue, 13 Oct 2020 01:23:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C438F60D4B;\n\tTue, 13 Oct 2020 03:23:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8712C605BE\n\tfor ;\n\tTue, 13 Oct 2020 03:23:20 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D9C9E528;\n\tTue, 13 Oct 2020 03:23:19 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BPJgq76d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602552200;\n\tbh=SfnAWffTHIgeE/qgaCcw3U31jWrFciH7xf9Ca1GHpck=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BPJgq76dEaNYnseEP8QsSCNavyoXRprU0xCR+fpPXVhAoSp4BTKo73GRLWYtCvoJW\n\tle3011FRAk0HrmZCffqblYHF5DV2zOdpIAUJj/+VK0fRyoZa11EtsvkkWXoMEpGxqI\n\tIJlKEMfSVlnLgaSB43JUQnROzw+j8tfwXuOkdVC8=","Date":"Tue, 13 Oct 2020 04:22:34 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201013012234.GC3942@pendragon.ideasonboard.com>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20200929164000.15429-5-david.plowman@raspberrypi.com>\n\t<20201011021020.GF3939@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: Add geometry helper\n\tfunctions","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]