[{"id":13402,"web_url":"https://patchwork.libcamera.org/comment/13402/","msgid":"<20201022070338.GN3942@pendragon.ideasonboard.com>","date":"2020-10-22T07:03:38","subject":"Re: [libcamera-devel] [PATCH v4 4/5] 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\nOn Mon, Oct 19, 2020 at 01:51:55PM +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\nAfter reviewing how they're used, they definitely fulfil their job :-)\n\n> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> ---\n>  include/libcamera/geometry.h |  53 +++++++\n>  src/libcamera/geometry.cpp   | 296 +++++++++++++++++++++++++++++++++++\n>  2 files changed, 349 insertions(+)\n> \n> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> index 02fb63c0..5935cc63 100644\n> --- a/include/libcamera/geometry.h\n> +++ b/include/libcamera/geometry.h\n> @@ -13,6 +13,25 @@\n>  \n>  namespace libcamera {\n>  \n> +class Point\n> +{\n> +public:\n> +\tconstexpr Point()\n> +\t\t: x(0), y(0)\n> +\t{\n> +\t}\n> +\n> +\tconstexpr Point(int xpos, int ypos)\n> +\t\t: x(xpos), y(ypos)\n> +\t{\n> +\t}\n\nIf you want to use this as an offset for the translate operation, you'll\nneed a\n\n\tPoint operator-() const\n\t{\n\t\treturn { -x, -y };\n\t}\n\n> +\n> +\tint x;\n> +\tint y;\n> +};\n> +\n> +class Rectangle;\n\nNitpicking, I'd move this before Point, to have all forward declarations\n(even if there's one only) at the beginning.\n\n> +\n>  class Size\n>  {\n>  public:\n> @@ -32,6 +51,8 @@ public:\n>  \tbool isNull() const { return !width && !height; }\n>  \tconst std::string toString() const;\n>  \n> +\tPoint center() const;\n> +\n>  \tSize &alignDownTo(unsigned int hAlignment, unsigned int vAlignment)\n>  \t{\n>  \t\twidth = width / hAlignment * hAlignment;\n> @@ -93,8 +114,19 @@ public:\n>  \t\t\tstd::max(height, expand.height)\n>  \t\t};\n>  \t}\n> +\n> +\tSize boundedToAspectRatio(const Size &ratio) const;\n> +\tSize expandedToAspectRatio(const Size &ratio) const;\n> +\n> +\tRectangle centeredTo(const Point &center) const;\n>  };\n>  \n> +Size operator*(const Size &size, float factor);\n> +Size operator/(const Size &size, float factor);\n> +\n> +Size &operator*=(Size &size, float factor);\n> +Size &operator/=(Size &size, float factor);\n\nBy the way, the reason why the comparison operators are defined outside\nof the class is to allow implicit construction of either side of the\noperator. As this isn't an issue for the above arithmetic operators, you\ncan also define them as class members if you prefer. Entirely up to you.\n\n> +\n>  bool operator==(const Size &lhs, const Size &rhs);\n>  bool operator<(const Size &lhs, const Size &rhs);\n>  \n> @@ -176,6 +208,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 +220,22 @@ public:\n>  \n>  \tbool isNull() const { return !width && !height; }\n>  \tconst std::string toString() const;\n> +\n> +\tPoint center() const;\n> +\n\nWe try to add blank lines between groups of related functions, but not\nnecessarily between all functions. You could thus drop this one I think.\n\n> +\tSize size() const;\n> +\n> +\tRectangle &translateBy(int dx, int dy);\n> +\n> +\tRectangle &rescaleTo(const Size &from, const Size &to);\n\nMaybe scaleTo instead of rescaleTo to make it slightly shorter ? It's no\nbig deal in this line, but there operations have a tendency to be\nchained, creating fairly long lines. I would also declare this before\ntranslateBy(), as in the absence of any particular constraint on the\nordering, I tend to go for alphabetical.\n\n> +\n> +\tRectangle translatedBy(int dx, int dy) const;\n> +\n> +\tRectangle rescaledTo(const Size &from, const Size &to) const;\n> +\n> +\tRectangle boundedTo(const Rectangle &bound) const;\n> +\n> +\tRectangle clampedTo(const Rectangle &boundary) const;\n\nSame for these functions, I'd order them alphabetically, and remove the\nblank lines in between.\n\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..75741c1e 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -17,6 +17,36 @@\n>  \n>  namespace libcamera {\n>  \n> +/**\n> + * \\struct Point\n\nPoint is a class.\n\n> + * \\brief Describe a point in two-dimensional space\n> + *\n> + * The Point structure defines a point in two-dimensional space with integer\n> + * precision. The coordinates of a Point may be negative as well as positive.\n> + */\n> +\n> +/**\n> + * \\fn Point::Point()\n> + * \\brief Construct a Point with x and y set to 0\n> + */\n> +\n> +/**\n> + * \\fn Point::Point(int x, int y)\n\ns/x/xpos/ and s/y/ypos/\n\n> + * \\brief Construct a Point at given \\a x and \\a y values\n> + * \\param[in] xpos The x-coordinate\n> + * \\param[in] ypos The y-coordinate\n> + */\n> +\n> +/**\n> + * \\var Point::x\n> + * \\brief The x-coordinate of the Point\n> + */\n> +\n> +/**\n> + * \\var Point::y\n> + * \\brief The y-coordinate of the Point\n> + */\n> +\n>  /**\n>   * \\struct Size\n>   * \\brief Describe a two-dimensional size\n> @@ -61,6 +91,15 @@ const std::string Size::toString() const\n>  \treturn std::to_string(width) + \"x\" + std::to_string(height);\n>  }\n>  \n> +/**\n> + * \\brief Retrieve the center point of this size\n> + * \\return The center Point\n> + */\n> +Point Size::center() const\n> +{\n> +\treturn { static_cast<int>(width / 2), static_cast<int>(height / 2) };\n\nNo need to cast, int / int gives you an int.\n\n> +}\n> +\n>  /**\n>   * \\fn Size::alignDownTo(unsigned int hAlignment, unsigned int vAlignment)\n>   * \\brief Align the size down horizontally and vertically in place\n> @@ -143,6 +182,101 @@ const std::string Size::toString() const\n>   * height of this size and the \\a expand size\n>   */\n>  \n> +/**\n> + * \\brief Bound the size down to match the aspect ratio given by \\a ratio\n> + * \\param[in] ratio The size whose aspect ratio must be matched\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::boundedToAspectRatio(const Size &ratio) const\n> +{\n> +\tuint64_t ratio1 = static_cast<uint64_t>(width) *\n> +\t\t\t  static_cast<uint64_t>(ratio.height);\n> +\tuint64_t ratio2 = static_cast<uint64_t>(ratio.width) *\n> +\t\t\t  static_cast<uint64_t>(height);\n> +\n> +\tif (ratio1 > ratio2)\n> +\t\treturn { static_cast<unsigned int>(ratio2 / ratio.height), height };\n> +\telse\n> +\t\treturn { width, static_cast<unsigned int>(ratio1 / ratio.width) };\n> +}\n> +\n> +/**\n> + * \\brief Expand the size to match the aspect ratio given by \\a ratio\n> + * \\param[in] ratio The size whose aspect ratio must be matched\n> + * \\return A Size whose width and height are equal to the width and height\n> + * of this Size expanded up to the aspect ratio of \\a ratio\n> + */\n> +Size Size::expandedToAspectRatio(const Size &ratio) const\n> +{\n> +\tuint64_t ratio1 = static_cast<uint64_t>(width) *\n> +\t\t\t  static_cast<uint64_t>(ratio.height);\n> +\tuint64_t ratio2 = static_cast<uint64_t>(ratio.width) *\n> +\t\t\t  static_cast<uint64_t>(height);\n> +\n> +\tif (ratio1 < ratio2)\n> +\t\treturn { static_cast<unsigned int>(ratio2 / ratio.height), height };\n> +\telse\n> +\t\treturn { width, static_cast<unsigned int>(ratio1 / ratio.width) };\n> +}\n> +\n> +/**\n> + * \\brief Center a rectangle of this size at a given Point\n> + * \\param[in] center The center point the Rectangle is to have\n> + *\n> + * A Rectangle of this object's size is positioned so that its center\n> + * is at the given Point.\n> + *\n> + * \\return A Rectangle of this size, centered at the given Point.\n> + */\n> +Rectangle Size::centeredTo(const Point &center) const\n> +{\n> +\tint x = center.x - width / 2;\n> +\tint y = center.y - height / 2;\n> +\n> +\treturn { x, y, width, height };\n> +}\n> +\n> +/**\n> + * \\brief Scale size up by the given factor\n\n \\param[in] size The size\n \\param[in] factor The factor\n\n> + * \\return The scaled Size\n> + */\n> +Size operator*(const Size &size, float factor)\n> +{\n> +\treturn Size(size.width * factor, size.height * factor);\n> +}\n> +\n> +/**\n> + * \\brief Scale size down by the given factor\n\nSame here and in the functions below.\n\n> + * \\return The scaled Size\n> + */\n> +Size operator/(const Size &size, float factor)\n> +{\n> +\treturn Size(size.width / factor, size.height / factor);\n> +}\n> +\n> +/**\n> + * \\brief Scale size up by the given factor in place\n> + * \\return The scaled Size\n> + */\n> +Size &operator*=(Size &size, float factor)\n> +{\n> +\tsize.width *= factor;\n> +\tsize.height *= factor;\n> +\treturn size;\n> +}\n> +\n> +/**\n> + * \\brief Scale size down by the given factor in place\n> + * \\return The scaled Size\n> + */\n> +Size &operator/=(Size &size, float factor)\n> +{\n> +\tsize.width /= factor;\n> +\tsize.height /= factor;\n> +\treturn size;\n> +}\n> +\n>  /**\n>   * \\brief Compare sizes for equality\n>   * \\return True if the two sizes are equal, false otherwise\n> @@ -365,6 +499,13 @@ 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 of \\a size with its top left corner located\n> + * at (0,0).\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 +545,161 @@ const std::string Rectangle::toString() const\n>  \treturn ss.str();\n>  }\n>  \n> +/**\n> + * \\brief Retrieve the center point of this rectangle\n> + * \\return The center Point\n> + */\n> +Point Rectangle::center() const\n> +{\n> +\treturn { x + static_cast<int>(width / 2), y + static_cast<int>(height / 2) };\n\nNo need for casts either.\n\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the size of this rectangle\n> + * \\return The Rectangle size\n> + */\n> +Size Rectangle::size() const\n> +{\n> +\treturn Size(width, height);\n> +}\n> +\n> +/**\n> + * \\brief Translate this Rectangle by the given amounts in place\n> + * \\param[in] dx The amount to translate the Rectangle in the x direction\n> + * \\param[in] dy The amount to translate the Rectangle in the y direction\n> + *\n> + * The Rectangle is translated by the given amounts \\a dx and \\a dy.\n> + *\n> + * \\return A reference to this object\n> + */\n> +Rectangle &Rectangle::translateBy(int dx, int dy)\n> +{\n> +\tx += dx;\n> +\ty += dy;\n> +\n> +\treturn *this;\n> +}\n> +\n> +/**\n> + * \\brief Rescale this Rectangle in place from one coordinate frame to another\n> + * \\param[in] from The current coordinate frame of the Rectangle\n> + * \\param[in] to The new coordinate frame for the Rectangle\n> + *\n> + * The Rectangle can be regarded as being embedded within a coordinate frame\n> + * given by \\a from. It is rescaled so as to occupy the equivalent part of the\n> + * new coordinate frame given by \\a to.\n> + *\n> + * This is purely a scaling operation, and the Rectangle does not need to lie\n> + * inside the coordinate frames.\n\nI find the coordinate frame explanation a bit unclear. Would it be\nclearer if the from and to parameters were denumerator and numerator\nrespectively ? We could then document the function as applying a\nnon-uniform homothety centered at (0,0) and whose ratio is given by the\nnumerator and denominator.\n\nBonus point if we could convey through the function name this it applies\nan homothety whose centre is not the centre of the rectangle, but I\ncan't think of a good name :-/\n\nSame comments for rescaledTo().\n\n> + *\n> + * \\return A reference to this object\n> + */\n> +Rectangle &Rectangle::rescaleTo(const Size &from, const Size &to)\n> +{\n> +\tx = static_cast<int64_t>(x) * to.width / from.width;\n> +\ty = static_cast<int64_t>(y) * to.height / from.height;\n> +\twidth = static_cast<uint64_t>(width) * to.width / from.width;\n> +\theight = static_cast<uint64_t>(height) * to.height / from.height;\n> +\n> +\treturn *this;\n> +}\n> +\n> +/**\n> + * \\brief Translate a Rectangle by the given amounts\n> + * \\param[in] dx The amount to translate the Rectangle in the x direction\n> + * \\param[in] dy The amount to translate the Rectangle in the y direction\n> + *\n> + * The Rectangle is translated by the given amounts \\a dx and \\a dy.\n> + *\n> + * \\return The translated Rectangle\n> + */\n> +Rectangle Rectangle::translatedBy(int dx, int dy) const\n> +{\n> +\treturn { x + dx, y + dy, width, height };\n> +}\n> +\n> +/**\n> + * \\brief Rescale a Rectangle from one coordinate frame to another\n> + * \\param[in] from The coordinate frame of the given Rectangle\n> + * \\param[in] to The new coordinate frame for the new Rectangle\n> + *\n> + * The Rectangle can be regarded as being embedded within a coordinate frame\n> + * given by \\a from. A new Rectangle, rescaled so as to occupy the equivalent\n> + * part of the new coordinate frame given by \\a to, is created and returned.\n> + *\n> + * This is purely a scaling operation, and the Rectangle does not need to lie\n> + * inside the coordinate frames.\n> + *\n> + * \\return The rescaled Rectangle\n> + */\n> +Rectangle Rectangle::rescaledTo(const Size &from, const Size &to) const\n> +{\n> +\tint scaledX = static_cast<int64_t>(x) * to.width / from.width;\n> +\tint scaledY = static_cast<int64_t>(y) * to.height / from.height;\n> +\tunsigned int scaledWidth = static_cast<uint64_t>(width) * to.width / from.width;\n> +\tunsigned int scaledHeight = static_cast<uint64_t>(height) * to.height / from.height;\n> +\n> +\treturn { scaledX, scaledY, scaledWidth, scaledHeight };\n> +}\n> +\n> +/**\n> + * \\brief Calculate the intersection of this Rectangle with another\n> + * \\param[in] bound The Rectangle that is intersected with this Rectangle\n> + *\n> + * This method calculates the standard intersection of two Rectangles. If the\n> + * Rectangles do not overlap in either the x or y direction, then the size\n> + * of that dimension in the result (its width or height) is set to zero. Even\n> + * when one dimension is set to zero, note that the other dimension may still\n> + * have a positive value if there was some overlap.\n> + *\n> + * \\return A Rectangle that is the intersection of the input Rectangles\n\nWe try not to pluralize class names, so I'd write 'rectangles' instead\nof 'Rectangles'.\n\n> + */\n> +Rectangle Rectangle::boundedTo(const Rectangle &bound) const\n> +{\n> +\tint topLeftX = std::max(x, bound.x);\n> +\tint topLeftY = std::max(y, bound.y);\n> +\tint bottomRightX = std::min<int>(x + width, bound.x + bound.width);\n> +\tint bottomRightY = std::min<int>(y + height, bound.y + bound.height);\n> +\n> +\tint newWidth = bottomRightX - topLeftX;\n> +\tint newHeight = bottomRightY - topLeftY;\n> +\n> +\treturn { topLeftX, topLeftY,\n> +\t\t static_cast<unsigned int>(std::max(newWidth, 0)),\n> +\t\t static_cast<unsigned int>(std::max(newHeight, 0)) };\n\nI think you can drop the cast, in which case this could be written\n\n\tunsigned int newWidth = std::max(bottomRightX - topLeftX, 0);\n\tunsigned int newHeight = std::mx(bottomRightY - topLeftY, 0);\n\n\treturn { topLeftX, topLeftY, newWidth, newHeight };\n\nUp to you.\n\n> +}\n> +\n> +/**\n> + * \\brief Clamp a Rectangle so as not to exceed another Rectangle\n> + * \\param[in] boundary The limit that the returned Rectangle will not exceed\n> + *\n> + * The Rectangle is modified 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 boundary, then the returned\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\nMaybe s/We note/Note/ ?\n\n> + * \\sa Rectangle::boundedTo\n\nAnd \"... intersection function, which is provided by boundedTo().\" ?\n\n> + *\n> + * \\return A Rectangle that does not extend beyond a boundary Rectangle\n> + */\n> +Rectangle Rectangle::clampedTo(const Rectangle &boundary) const\n\nFinding good names is difficult :-S I'm not sure if it's better, but\nenclosedIn() or containtedIn() could also be candidates, although they\nmay sound like they would check if the rectangle is enclosed in the\nboundary, not perform the action of enclosing it (on the other hand, we\nprefix check functions with 'is', so maybe there's no risk of\nconfusion).\n\nI think we're finally converging. I know it's not fun to write these\nhelpers (and even less so to document them), but your work will really\nmake a difference. Thank you for that.\n\nBy the way, I'm not sure I would have dared reminding about test cases\nif you hadn't mentioned them in the cover letter :-) (Or maybe I would\nhave, but I would be more ashamed.)\n\n> +{\n> +\t/* We can't be bigger than the boundary rectangle. */\n> +\tRectangle result = boundedTo(Rectangle{ x, y, boundary.size() });\n> +\n> +\tresult.x = std::clamp<int>(result.x, boundary.x,\n> +\t\t\t\t   boundary.x + boundary.width - result.width);\n> +\tresult.y = std::clamp<int>(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":"<libcamera-devel-bounces@lists.libcamera.org>","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 9D44FBDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 22 Oct 2020 07:04:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 07FF06112D;\n\tThu, 22 Oct 2020 09:04:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D86E260351\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 22 Oct 2020 09:04:24 +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 4C29B53;\n\tThu, 22 Oct 2020 09:04:24 +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=\"O2LsHjfa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603350264;\n\tbh=hlr53Uowv5r6u2NvOtEAQwcloBc5FZOZDCPNwP12AO4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O2LsHjfaGrd/6aR0R/3pKerUsbM1sZ9Umu6SbQRDbPe4gsj3SAeoC0VEjPC8rnP5E\n\tY52fUGhbZbooPTTcXBEvYUvdulRVeBcAhNwqBjNNTZ4KsRYDttkDJGv76mZKnMGZIA\n\t+JshkwuN6DIa07YDrOuMZbND6+sUg9blpxY50Um8=","Date":"Thu, 22 Oct 2020 10:03:38 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<20201022070338.GN3942@pendragon.ideasonboard.com>","References":"<20201019125156.26751-1-david.plowman@raspberrypi.com>\n\t<20201019125156.26751-5-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201019125156.26751-5-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/5] libcamera: Add geometry helper\n\tfunctions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]