<div dir="auto"><div>Hi Jacopo,<br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Sep 26, 2024, 4:03 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Harvey<br>
<br>
On Wed, Sep 25, 2024 at 03:09:38PM GMT, Cheng-Hao Yang wrote:<br>
> Hi Jacopo,<br>
><br>
> Thank you for the patch.<br>
><br>
> On Wed, Sep 25, 2024 at 2:36 PM Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank" rel="noreferrer">umang.jain@ideasonboard.com</a>> wrote:<br>
> ><br>
> > Hi Jacopo<br>
> ><br>
> > Thank you for the patch<br>
> ><br>
> > On 24/09/24 6:15 pm, Jacopo Mondi wrote:<br>
> > > The libcamera::Rectangle class should allow defining rectangles<br>
> > > regardless of the reference system where an instance is used in.<br>
> > ><br>
> > > However the class documentation assumes the reference system origin<br>
> > > point to be in the visual top-left side and that rectangles are<br>
> > > constructed by specifying the top-left corner position and sizes.<br>
> > ><br>
> > > However this is ambiguous when using a different reference system as,<br>
> > > in example, constructing a rectangle (x, y, w, h) in the following<br>
> > > scenario, with the origin corner in the top-left position, would require<br>
> > > a negative decrement along the y-axis.<br>
> > ><br>
> > > ^<br>
> > > |<br>
> > > y| o------------------<br>
> > > | | | h<br>
> > > | | |<br>
> > > | -------------------<br>
> > > | w<br>
> > > -------------------------------><br>
> > > (0,0) x<br>
> > ><br>
> > > To remove ambiguities, drop references to the visual top-left corner and<br>
> > > define the rectangle's origin corner as the corner closer to the<br>
> > > reference system origin point, and specify the horizontal and vertical<br>
> > > dimensions always increment along their respective axis, as in the<br>
> > > following example.<br>
> > ><br>
> > > ^<br>
> > > |<br>
> > > | -------------------<br>
> > > | ^ | h<br>
> > > | | |<br>
> > > y| o---->-------------<br>
> > > | w<br>
> > > -------------------------------><br>
> > > (0,0) x<br>
> > ><br>
> > > (0,0) x<br>
> > > ------------------------------><br>
> > > | w<br>
> > > y| o---->-------------<br>
> > > | | | h<br>
> > > | v |<br>
> > > | -------------------<br>
> > > |<br>
> > > V<br>
> > ><br>
> > > Also rename for consistency the Rectangle::topLeft() function to<br>
> > > Rectangle::origin() and update its in-tree users.<br>
> > ><br>
> > > Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank" rel="noreferrer">jacopo.mondi@ideasonboard.com</a>><br>
> > > ---<br>
> > > include/libcamera/geometry.h | 2 +-<br>
> > > src/ipa/ipu3/algorithms/af.cpp | 2 +-<br>
> > > src/libcamera/geometry.cpp | 85 ++++++++++++++-----<br>
> > > .../pipeline/rpi/common/pipeline_base.cpp | 4 +-<br>
> > > src/py/libcamera/py_geometry.cpp | 2 +-<br>
> > > test/geometry.cpp | 8 +-<br>
> > > 6 files changed, 75 insertions(+), 28 deletions(-)<br>
> > ><br>
> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h<br>
> > > index 3e6f0f5d7fab..07cb0a7866fe 100644<br>
> > > --- a/include/libcamera/geometry.h<br>
> > > +++ b/include/libcamera/geometry.h<br>
> > > @@ -277,7 +277,7 @@ public:<br>
> > > return { width, height };<br>
> > > }<br>
> > ><br>
> > > - Point topLeft() const<br>
> > > + Point origin() const<br>
> > > {<br>
> > > return { x, y };<br>
> > > }<br>
> > > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp<br>
> > > index cf68fb59c49b..1b707e9dd9ab 100644<br>
> > > --- a/src/ipa/ipu3/algorithms/af.cpp<br>
> > > +++ b/src/ipa/ipu3/algorithms/af.cpp<br>
> > > @@ -155,7 +155,7 @@ int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)<br>
> > > * - Return the AF ROI as metadata in the Request<br>
> > > */<br>
> > > Rectangle roi = gridSize.centeredTo(bds.center());<br>
> > > - Point start = roi.topLeft();<br>
> > > + Point start = roi.origin();<br>
> > ><br>
> > > /* x_start and y_start should be even */<br>
> > > grid.x_start = utils::alignDown(start.x, 2);<br>
> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp<br>
> > > index 000151364c7f..45ba81a99920 100644<br>
> > > --- a/src/libcamera/geometry.cpp<br>
> > > +++ b/src/libcamera/geometry.cpp<br>
> > > @@ -593,11 +593,39 @@ std::ostream &operator<<(std::ostream &out, const SizeRange &sr)<br>
> > > * \brief Describe a rectangle's position and dimensions<br>
> > > *<br>
> > > * Rectangles are used to identify an area of an image. They are specified by<br>
> > > - * the coordinates of top-left corner and their horizontal and vertical size.<br>
> > > + * the coordinates of the corner closer to the reference system's origin point<br>
> > > + * and by horizontal and vertical increments from there.<br>
> > > *<br>
> > > - * The measure unit of the rectangle coordinates and size, as well as the<br>
> > > - * reference point from which the Rectangle::x and Rectangle::y displacements<br>
> > > - * refers to, are defined by the context were rectangle is used.<br>
> > > + * The corner closer to the reference system's origin point is the rectangle's<br>
> > > + * origin corner.<br>
> > > + *<br>
> > > + * \verbatim<br>
> > > +<br>
> > > + o = origin corner<br>
> > > +<br>
> > > + ^<br>
> > > + |<br>
> > > + | -------------------<br>
> > > + | ^ |<br>
> > > + | | |<br>
> > > + | o---->-------------<br>
> > > + |<br>
> > > + -------------------------------><br>
> > > + (0,0)<br>
> > > +<br>
> > > + (0,0)<br>
> > > + -------------------------------><br>
> > > + |<br>
> > > + | o---->-------------<br>
> > > + | | |<br>
> > > + | v |<br>
> > > + | -------------------<br>
> > > + |<br>
> > > + V<br>
> > > + \endverbatim<br>
> > > + *<br>
> > > + * The measure unit of the rectangle coordinates and size are defined by the<br>
> > > + * context were the rectangle is used.<br>
> > > */<br>
> > ><br>
> > > /**<br>
> > > @@ -608,35 +636,50 @@ std::ostream &operator<<(std::ostream &out, const SizeRange &sr)<br>
> > > /**<br>
> > > * \fn Rectangle::Rectangle(int x, int y, const Size &size)<br>
> > > * \brief Construct a Rectangle with the given position and size<br>
> > > - * \param[in] x The horizontal coordinate of the top-left corner<br>
> > > - * \param[in] y The vertical coordinate of the top-left corner<br>
> > > + * \param[in] x The horizontal coordinate of the origin corner<br>
> > > + * \param[in] y The vertical coordinate of the origin corner<br>
> > > * \param[in] size The size<br>
> > > + *<br>
> > > + * The rectangle's origin corner is the corner closer to the reference system<br>
> > > + * origin point (0, 0).<br>
><br>
> I think we should stick to the previous description:<br>
> `"origin" point defined as the point with the lower x and<br>
> lower y of the rectangle`, as `x` and `y` are signed integers.<br>
><br>
> If we only support coordinates that are positive or 0, which<br>
> I'm not sure if it's the case, how about making the type to be<br>
> `unsigned int`?<br>
><br>
<br>
To be honest I would rather make width and heigh signed to allow<br>
constructing (x, y, -w, -h)<br>
<br>
<br>
<br>
<br>
-------------------<br>
| ^<br>
| -------------><br>
| | |<br>
------<-----|-----o<br>
|<br>
|<br>
V<br>
<br>
Not sure if it's useful or not though<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Hmm... I find it pretty weird. What if all four corners have all negative coordinates?</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> BR,<br>
> Harvey<br>
><br>
><br>
> > > */<br>
> > ><br>
> > > /**<br>
> > > * \fn Rectangle::Rectangle(int x, int y, unsigned int width, unsigned int height)<br>
> > > * \brief Construct a Rectangle with the given position and size<br>
> > > - * \param[in] x The horizontal coordinate of the top-left corner<br>
> > > - * \param[in] y The vertical coordinate of the top-left corner<br>
> > > + * \param[in] x The horizontal coordinate of the origin corner<br>
> > > + * \param[in] y The vertical coordinate of the origin corner<br>
> > > * \param[in] width The width<br>
> > > * \param[in] height The height<br>
> > > + *<br>
> > > + * The rectangle's origin corner is the corner closer to the reference system<br>
> > > + * origin point (0, 0).<br>
> > > */<br>
> > ><br>
> > > /**<br>
> > > * \fn Rectangle::Rectangle(const Size &size)<br>
> > > - * \brief Construct a Rectangle of \a size with its top left corner located<br>
> > > + * \brief Construct a Rectangle of \a size with its origin corner located<br>
> > > * at (0,0)<br>
> > > * \param[in] size The desired Rectangle size<br>
> > > + *<br>
> > > + * The rectangle's origin corner is the corner closer to the reference system<br>
> > > + * origin point (0, 0).<br>
> > > */<br>
> > ><br>
> > > /**<br>
> > > * \var Rectangle::x<br>
> > > - * \brief The horizontal coordinate of the rectangle's top-left corner<br>
> > > + * \brief The horizontal coordinate of the rectangle's origin corner<br>
> > > + *<br>
> > > + * The rectangle's origin corner is the corner closer to the reference system<br>
> > > + * origin point (0, 0).<br>
> > > */<br>
> > ><br>
> > > /**<br>
> > > * \var Rectangle::y<br>
> > > - * \brief The vertical coordinate of the rectangle's top-left corner<br>
> > > + * \brief The vertical coordinate of the rectangle's origin corner<br>
> > > + *<br>
> > > + * The rectangle's origin corner is the corner closer to the reference system<br>
> > > + * origin point (0, 0).<br>
> > > */<br>
> > ><br>
> > > /**<br>
> > > @@ -683,9 +726,13 @@ Point Rectangle::center() const<br>
> > > */<br>
> > ><br>
> > > /**<br>
> > > - * \fn Point Rectangle::topLeft() const<br>
> > > - * \brief Retrieve the coordinates of the top left corner of this Rectangle<br>
> > > - * \return The Rectangle's top left corner<br>
> > > + * \fn Point Rectangle::origin() const<br>
> > > + * \brief Retrieve the coordinates of the origin corner of this Rectangle<br>
> > > + *<br>
> > > + * The rectangle's origin corner is the corner closer to the reference system<br>
> > > + * origin point (0, 0).<br>
> > > + *<br>
> > > + * \return The Rectangle's origin corner<br>
> ><br>
> > I think you can drop "corner" from "origin corner"<br>
> ><br>
> > Probably throughout the patch:<br>
> > s/origin corner/origin/<br>
> ><br>
> > Otherwise, LGTM<br>
> ><br>
> > Reviewed-by: Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank" rel="noreferrer">umang.jain@ideasonboard.com</a>><br>
> > > */<br>
> > ><br>
> > > /**<br>
> > > @@ -740,15 +787,15 @@ Rectangle &Rectangle::translateBy(const Point &point)<br>
> > > */<br>
> > > Rectangle Rectangle::boundedTo(const Rectangle &bound) const<br>
> > > {<br>
> > > - int topLeftX = std::max(x, bound.x);<br>
> > > - int topLeftY = std::max(y, bound.y);<br>
> > > + int originX = std::max(x, bound.x);<br>
> > > + int originY = std::max(y, bound.y);<br>
> > > int bottomRightX = std::min<int>(x + width, bound.x + bound.width);<br>
> > > int bottomRightY = std::min<int>(y + height, bound.y + bound.height);<br>
> > ><br>
> > > - unsigned int newWidth = std::max(bottomRightX - topLeftX, 0);<br>
> > > - unsigned int newHeight = std::max(bottomRightY - topLeftY, 0);<br>
> > > + unsigned int newWidth = std::max(bottomRightX - originX, 0);<br>
> > > + unsigned int newHeight = std::max(bottomRightY - originY, 0);<br>
> > ><br>
> > > - return { topLeftX, topLeftY, newWidth, newHeight };<br>
> > > + return { originX, originY, newWidth, newHeight };<br>
> > > }<br>
> > ><br>
> > > /**<br>
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp<br>
> > > index 3041fd1ed9fd..8d2440026689 100644<br>
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp<br>
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp<br>
> > > @@ -1289,7 +1289,7 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const<br>
> > > */<br>
> > > Rectangle nativeCrop = ispCrop.scaledBy(sensorInfo_.analogCrop.size(),<br>
> > > sensorInfo_.outputSize);<br>
> > > - nativeCrop.translateBy(sensorInfo_.analogCrop.topLeft());<br>
> > > + nativeCrop.translateBy(sensorInfo_.analogCrop.origin());<br>
> > > return nativeCrop;<br>
> > > }<br>
> > ><br>
> > > @@ -1303,7 +1303,7 @@ void CameraData::applyScalerCrop(const ControlList &controls)<br>
> > > nativeCrop = { 0, 0, 1, 1 };<br>
> > ><br>
> > > /* Create a version of the crop scaled to ISP (camera mode) pixels. */<br>
> > > - Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo_.analogCrop.topLeft());<br>
> > > + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo_.analogCrop.origin());<br>
> > > ispCrop.scaleBy(sensorInfo_.outputSize, sensorInfo_.analogCrop.size());<br>
> > ><br>
> > > /*<br>
> > > diff --git a/src/py/libcamera/py_geometry.cpp b/src/py/libcamera/py_geometry.cpp<br>
> > > index c7e303609427..8dee365b25dd 100644<br>
> > > --- a/src/py/libcamera/py_geometry.cpp<br>
> > > +++ b/src/py/libcamera/py_geometry.cpp<br>
> > > @@ -105,7 +105,7 @@ void init_py_geometry(py::module &m)<br>
> > > .def_property_readonly("is_null", &Rectangle::isNull)<br>
> > > .def_property_readonly("center", &Rectangle::center)<br>
> > > .def_property_readonly("size", &Rectangle::size)<br>
> > > - .def_property_readonly("topLeft", &Rectangle::topLeft)<br>
> > > + .def_property_readonly("origin", &Rectangle::origin)<br>
> > > .def("scale_by", &Rectangle::scaleBy)<br>
> > > .def("translate_by", &Rectangle::translateBy)<br>
> > > .def("bounded_to", &Rectangle::boundedTo)<br>
> > > diff --git a/test/geometry.cpp b/test/geometry.cpp<br>
> > > index 64169206ad16..a4ee6f6e167a 100644<br>
> > > --- a/test/geometry.cpp<br>
> > > +++ b/test/geometry.cpp<br>
> > > @@ -363,16 +363,16 @@ protected:<br>
> > > return TestFail;<br>
> > > }<br>
> > ><br>
> > > - /* Rectangle::size(), Rectangle::topLeft() and Rectangle::center() tests */<br>
> > > + /* Rectangle::size(), Rectangle::origin() and Rectangle::center() tests */<br>
> > > if (Rectangle(-1, -2, 3, 4).size() != Size(3, 4) ||<br>
> > > Rectangle(0, 0, 100000, 200000).size() != Size(100000, 200000)) {<br>
> > > cout << "Rectangle::size() test failed" << endl;<br>
> > > return TestFail;<br>
> > > }<br>
> > ><br>
> > > - if (Rectangle(1, 2, 3, 4).topLeft() != Point(1, 2) ||<br>
> > > - Rectangle(-1, -2, 3, 4).topLeft() != Point(-1, -2)) {<br>
> > > - cout << "Rectangle::topLeft() test failed" << endl;<br>
> > > + if (Rectangle(1, 2, 3, 4).origin() != Point(1, 2) ||<br>
> > > + Rectangle(-1, -2, 3, 4).origin() != Point(-1, -2)) {<br>
> > > + cout << "Rectangle::origin() test failed" << endl;<br>
> > > return TestFail;<br>
> > > }<br>
> > ><br>
> > > --<br>
> > > 2.46.1<br>
> > ><br>
> ><br>
><br>
><br>
> --<br>
> BR,<br>
> Harvey Yang<br>
</blockquote></div></div></div>