[PATCH] libcamera: geometry: Rename Rectangle's top-left corner
Cheng-Hao Yang
chenghaoyang at google.com
Fri Sep 27 18:15:44 CEST 2024
Hi Jacopo,
On Fri, Sep 27, 2024 at 10:51 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi again
>
> On Fri, Sep 27, 2024 at 10:54:20AM GMT, Jacopo Mondi wrote:
> > Hi Harvey
> >
> > On Fri, Sep 27, 2024 at 04:46:06PM GMT, Cheng-Hao Yang wrote:
> > > Hi Jacopo,
> > >
> > > On Fri, Sep 27, 2024 at 4:05 PM Jacopo Mondi
> > > <jacopo.mondi at ideasonboard.com> wrote:
> > > >
> > > > Hi Harvey
> > > >
> > > > On Thu, Sep 26, 2024 at 11:22:58PM GMT, Cheng-Hao Yang wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > On Thu, Sep 26, 2024 at 10:35 PM Jacopo Mondi
> > > > > <jacopo.mondi at ideasonboard.com> wrote:
> > > > > >
> > > > > > Hi Harvey
> > > > > >
> > > > > > On Thu, Sep 26, 2024 at 10:03:55PM GMT, Cheng-Hao Yang wrote:
> > > > > > > Hi Jacopo,
> > > > > > >
> > > > > > > On Thu, Sep 26, 2024 at 5:36 PM Jacopo Mondi
> > > > > > > <jacopo.mondi at ideasonboard.com> wrote:
> > > > > > > >
> > > > > > > > Hi Harvey
> > > > > > > >
> > > > > > > > On Thu, Sep 26, 2024 at 05:28:13PM GMT, Cheng-Hao Yang wrote:
> > > > > > > > > Hi Jacopo,
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Thu, Sep 26, 2024, 4:38 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi Harvey
> > > > > > > > > >
> > > > > > > > > > On Thu, Sep 26, 2024 at 04:29:50PM GMT, Cheng-Hao Yang wrote:
> > > > > > > > > > > Hi Jacopo,
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Sep 26, 2024, 4:03 PM Jacopo Mondi <
> > > > > > > > > > jacopo.mondi at ideasonboard.com>
> > > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Hi Harvey
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, Sep 25, 2024 at 03:09:38PM GMT, Cheng-Hao Yang wrote:
> > > > > > > > > > > > > Hi Jacopo,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thank you for the patch.
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Wed, Sep 25, 2024 at 2:36 PM Umang Jain <
> > > > > > > > > > umang.jain at ideasonboard.com>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi Jacopo
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Thank you for the patch
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On 24/09/24 6:15 pm, Jacopo Mondi wrote:
> > > > > > > > > > > > > > > The libcamera::Rectangle class should allow defining rectangles
> > > > > > > > > > > > > > > regardless of the reference system where an instance is used in.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > However the class documentation assumes the reference system
> > > > > > > > > > origin
> > > > > > > > > > > > > > > point to be in the visual top-left side and that rectangles are
> > > > > > > > > > > > > > > constructed by specifying the top-left corner position and sizes.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > However this is ambiguous when using a different reference
> > > > > > > > > > system as,
> > > > > > > > > > > > > > > in example, constructing a rectangle (x, y, w, h) in the
> > > > > > > > > > following
> > > > > > > > > > > > > > > scenario, with the origin corner in the top-left position, would
> > > > > > > > > > > > require
> > > > > > > > > > > > > > > a negative decrement along the y-axis.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > ^
> > > > > > > > > > > > > > > |
> > > > > > > > > > > > > > > y| o------------------
> > > > > > > > > > > > > > > | | | h
> > > > > > > > > > > > > > > | | |
> > > > > > > > > > > > > > > | -------------------
> > > > > > > > > > > > > > > | w
> > > > > > > > > > > > > > > ------------------------------->
> > > > > > > > > > > > > > > (0,0) x
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > To remove ambiguities, drop references to the visual top-left
> > > > > > > > > > corner
> > > > > > > > > > > > and
> > > > > > > > > > > > > > > define the rectangle's origin corner as the corner closer to the
> > > > > > > > > > > > > > > reference system origin point, and specify the horizontal and
> > > > > > > > > > > > vertical
> > > > > > > > > > > > > > > dimensions always increment along their respective axis, as in
> > > > > > > > > > the
> > > > > > > > > > > > > > > following example.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > ^
> > > > > > > > > > > > > > > |
> > > > > > > > > > > > > > > | -------------------
> > > > > > > > > > > > > > > | ^ | h
> > > > > > > > > > > > > > > | | |
> > > > > > > > > > > > > > > y| o---->-------------
> > > > > > > > > > > > > > > | w
> > > > > > > > > > > > > > > ------------------------------->
> > > > > > > > > > > > > > > (0,0) x
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > (0,0) x
> > > > > > > > > > > > > > > ------------------------------>
> > > > > > > > > > > > > > > | w
> > > > > > > > > > > > > > > y| o---->-------------
> > > > > > > > > > > > > > > | | | h
> > > > > > > > > > > > > > > | v |
> > > > > > > > > > > > > > > | -------------------
> > > > > > > > > > > > > > > |
> > > > > > > > > > > > > > > V
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Also rename for consistency the Rectangle::topLeft() function to
> > > > > > > > > > > > > > > Rectangle::origin() and update its in-tree users.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > include/libcamera/geometry.h | 2 +-
> > > > > > > > > > > > > > > src/ipa/ipu3/algorithms/af.cpp | 2 +-
> > > > > > > > > > > > > > > src/libcamera/geometry.cpp | 85
> > > > > > > > > > > > ++++++++++++++-----
> > > > > > > > > > > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 4 +-
> > > > > > > > > > > > > > > src/py/libcamera/py_geometry.cpp | 2 +-
> > > > > > > > > > > > > > > test/geometry.cpp | 8 +-
> > > > > > > > > > > > > > > 6 files changed, 75 insertions(+), 28 deletions(-)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > diff --git a/include/libcamera/geometry.h
> > > > > > > > > > > > b/include/libcamera/geometry.h
> > > > > > > > > > > > > > > index 3e6f0f5d7fab..07cb0a7866fe 100644
> > > > > > > > > > > > > > > --- a/include/libcamera/geometry.h
> > > > > > > > > > > > > > > +++ b/include/libcamera/geometry.h
> > > > > > > > > > > > > > > @@ -277,7 +277,7 @@ public:
> > > > > > > > > > > > > > > return { width, height };
> > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > - Point topLeft() const
> > > > > > > > > > > > > > > + Point origin() const
> > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > > return { x, y };
> > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > > diff --git a/src/ipa/ipu3/algorithms/af.cpp
> > > > > > > > > > > > b/src/ipa/ipu3/algorithms/af.cpp
> > > > > > > > > > > > > > > index cf68fb59c49b..1b707e9dd9ab 100644
> > > > > > > > > > > > > > > --- a/src/ipa/ipu3/algorithms/af.cpp
> > > > > > > > > > > > > > > +++ b/src/ipa/ipu3/algorithms/af.cpp
> > > > > > > > > > > > > > > @@ -155,7 +155,7 @@ int Af::configure(IPAContext &context, const
> > > > > > > > > > > > IPAConfigInfo &configInfo)
> > > > > > > > > > > > > > > * - Return the AF ROI as metadata in the Request
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > > Rectangle roi = gridSize.centeredTo(bds.center());
> > > > > > > > > > > > > > > - Point start = roi.topLeft();
> > > > > > > > > > > > > > > + Point start = roi.origin();
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /* x_start and y_start should be even */
> > > > > > > > > > > > > > > grid.x_start = utils::alignDown(start.x, 2);
> > > > > > > > > > > > > > > diff --git a/src/libcamera/geometry.cpp
> > > > > > > > > > b/src/libcamera/geometry.cpp
> > > > > > > > > > > > > > > index 000151364c7f..45ba81a99920 100644
> > > > > > > > > > > > > > > --- a/src/libcamera/geometry.cpp
> > > > > > > > > > > > > > > +++ b/src/libcamera/geometry.cpp
> > > > > > > > > > > > > > > @@ -593,11 +593,39 @@ std::ostream &operator<<(std::ostream &out,
> > > > > > > > > > > > const SizeRange &sr)
> > > > > > > > > > > > > > > * \brief Describe a rectangle's position and dimensions
> > > > > > > > > > > > > > > *
> > > > > > > > > > > > > > > * Rectangles are used to identify an area of an image. They
> > > > > > > > > > are
> > > > > > > > > > > > specified by
> > > > > > > > > > > > > > > - * the coordinates of top-left corner and their horizontal and
> > > > > > > > > > > > vertical size.
> > > > > > > > > > > > > > > + * the coordinates of the corner closer to the reference
> > > > > > > > > > system's
> > > > > > > > > > > > origin point
> > > > > > > > > > > > > > > + * and by horizontal and vertical increments from there.
> > > > > > > > > > > > > > > *
> > > > > > > > > > > > > > > - * The measure unit of the rectangle coordinates and size, as
> > > > > > > > > > well
> > > > > > > > > > > > as the
> > > > > > > > > > > > > > > - * reference point from which the Rectangle::x and Rectangle::y
> > > > > > > > > > > > displacements
> > > > > > > > > > > > > > > - * refers to, are defined by the context were rectangle is used.
> > > > > > > > > > > > > > > + * The corner closer to the reference system's origin point is
> > > > > > > > > > the
> > > > > > > > > > > > rectangle's
> > > > > > > > > > > > > > > + * origin corner.
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * \verbatim
> > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > + o = origin corner
> > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > + ^
> > > > > > > > > > > > > > > + |
> > > > > > > > > > > > > > > + | -------------------
> > > > > > > > > > > > > > > + | ^ |
> > > > > > > > > > > > > > > + | | |
> > > > > > > > > > > > > > > + | o---->-------------
> > > > > > > > > > > > > > > + |
> > > > > > > > > > > > > > > + ------------------------------->
> > > > > > > > > > > > > > > + (0,0)
> > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > + (0,0)
> > > > > > > > > > > > > > > + ------------------------------->
> > > > > > > > > > > > > > > + |
> > > > > > > > > > > > > > > + | o---->-------------
> > > > > > > > > > > > > > > + | | |
> > > > > > > > > > > > > > > + | v |
> > > > > > > > > > > > > > > + | -------------------
> > > > > > > > > > > > > > > + |
> > > > > > > > > > > > > > > + V
> > > > > > > > > > > > > > > + \endverbatim
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * The measure unit of the rectangle coordinates and size are
> > > > > > > > > > > > defined by the
> > > > > > > > > > > > > > > + * context were the rectangle is used.
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > @@ -608,35 +636,50 @@ std::ostream &operator<<(std::ostream &out,
> > > > > > > > > > > > const SizeRange &sr)
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > * \fn Rectangle::Rectangle(int x, int y, const Size &size)
> > > > > > > > > > > > > > > * \brief Construct a Rectangle with the given position and
> > > > > > > > > > size
> > > > > > > > > > > > > > > - * \param[in] x The horizontal coordinate of the top-left corner
> > > > > > > > > > > > > > > - * \param[in] y The vertical coordinate of the top-left corner
> > > > > > > > > > > > > > > + * \param[in] x The horizontal coordinate of the origin corner
> > > > > > > > > > > > > > > + * \param[in] y The vertical coordinate of the origin corner
> > > > > > > > > > > > > > > * \param[in] size The size
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * The rectangle's origin corner is the corner closer to the
> > > > > > > > > > > > reference system
> > > > > > > > > > > > > > > + * origin point (0, 0).
> > > > > > > > > > > > >
> > > > > > > > > > > > > I think we should stick to the previous description:
> > > > > > > > > > > > > `"origin" point defined as the point with the lower x and
> > > > > > > > > > > > > lower y of the rectangle`, as `x` and `y` are signed integers.
> > > > > > > > > > > > >
> > > > > > > > > > > > > If we only support coordinates that are positive or 0, which
> > > > > > > > > > > > > I'm not sure if it's the case, how about making the type to be
> > > > > > > > > > > > > `unsigned int`?
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > To be honest I would rather make width and heigh signed to allow
> > > > > > > > > > > > constructing (x, y, -w, -h)
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > -------------------
> > > > > > > > > > > > | ^
> > > > > > > > > > > > | ------------->
> > > > > > > > > > > > | | |
> > > > > > > > > > > > ------<-----|-----o
> > > > > > > > > > > > |
> > > > > > > > > > > > |
> > > > > > > > > > > > V
> > > > > > > > > > > >
> > > > > > > > > > > > Not sure if it's useful or not though
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Hmm... I find it pretty weird. What if all four corners have all negative
> > > > > > > > > > > coordinates?
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Don't know, maybe if the reference system where the rectangle is used
> > > > > > > > > > into has 4 planes and so an all-negative rectangle makes sense.
> > > > > > > > > >
> > > > > > > > > > My point is that we should allow users of this class to express
> > > > > > > > > > Rectangle in any context they plan to use them. Not sure if I'm going
> > > > > > > > > > overboard here or it makes sense.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yeah I agree, while doesn't that mean the description doesn't fit all
> > > > > > > > > cases, like the corner closer to the reference system origin point of a
> > > > > > > > > rectangle that has all negative coordinates is actually the "bottom-right"
> > > > > > > >
> > > > > > > > This patch aims to remove all references to "top-left" or in general
> > > > > > > > to any visual reference exactly for that purpose. Or have I forgot any
> > > > > > > > ?
> > > > > > >
> > > > > > > In this c'tor `\fn Rectangle::Rectangle(int x, int y, const Size &size)`,
> > > > > > > the description of `x` and `y` are updated to "coordinates of the origin
> > > > > > > corner", where the origin corner is "the corner closer to the reference
> > > > > > > system origin point (0, 0)."
> > > > > > >
> > > > > > > As currently libcamera::Size takes only non-negative width and height,
> > > > > > > take rectangle [(-3, -3), (-3, -1), (-1, -3), (-1, -1)] as the example, you
> > > > > > > can find that the only valid way to construct this rectangle as
> > > > > > > libcamera::Rectangle is `{x = -3, y = -3, size = {2, 2}}`. However, this
> > > > > > > doesn't fit with your updated description, as the corner closer to the
> > > > > > > reference system origin point (0, 0) is actually (-1, -1), instead of
> > > > > > > (-3, -3).
> > > > > > >
> > > > > >
> > > > > > You're right. Let's take a step back though. If we have a 4 planes reference
> > > > > > system, we always have a visual top-left indeed. As the 4 planes have
> > > > > > negative and positive coordinates, we can describe with absolute
> > > > > > positions the rectangle and all points in the system.
> > > > >
> > > > > Sorry, I still don't get your point...
> > > > >
> > > >
> > > > If you have a 4 planes system
> > > >
> > > > ^
> > > > |
> > > > |
> > > > |
> > > > |
> > > > <--------------------------->
> > > > |
> > > > |
> > > > |
> > > > |
> > > > V
> > > >
> > > > Then you always have a top, a left, and bottom and a right, you can
> > > > describe width and height as negative or positive increments and your
> > > > rectangle is always well defined as the reference system cannot be
> > > > ambiguous. The very concept of "reference system orientation" doesn't
> > > > make sense, as a 4-planes system describes the full 2-d space.
> > > >
> > > > The ambiguity I'm trying to solve is about using a 1 plane reference
> > > > system, as depending on how the system is oriented the definition of
> > > > the "top-left" point might change, as well as the definition of the
> > > > horizontal and vertical increments change as well.
> > > >
> > > > In example:
> > > >
> > > > ^
> > > > |
> > > > |
> > > > | o-------
> > > > | | |
> > > > | --------
> > > > |
> > > > ------------------------>
> > > >
> > > > top-left: Smaller x and higher y
> > > > width: positive increment
> > > > height: negative increment
> > > >
> > > >
> > > >
> > > >
> > > > ------------------------>
> > > > | o-------
> > > > | | |
> > > > | --------
> > > > |
> > > > |
> > > > V
> > > >
> > > > top-left: Smaller x and smaller y
> > > > width: positive increment
> > > > height: positive increment
> > > >
> > > >
> > > > I thought our Rectangle definition didn't make assumptions on the
> > > > reference's system orientation and in fact that's what part of the
> > > > documentation says, but indeed the usage of "top-left" is, as shown,
> > > > ambiguous.
> > > >
> > > > So I went and changed that to be the "corner closer to the origin
> > > > point" and specified in documentation that vertical and horizontal
> > > > increments are always positive and I thought I was done with this, but
> > > > then I made the mistake of talking about negative increments and
> > > > 4-plane systems and here we are.
> > > >
> > > >
> > > > > Do you mean that the new description, like
> > > > > `The horizontal coordinate of the rectangle's origin corner`,
> > > > > only fits with the 1-plane reference system? I thought it should fit all
> > > > > 2D reference systems.
> > > >
> > > > I don't think that has never been the case, but indeed we have a
> > > > constructor that takes "signed" x and y.
> > >
> > > IIUC, you mean `you don't think that has ever been the case`, right?
> > >
> >
> >
> > No, I meant "I don't think the Rectangle class has been designed
> > to fit all 2D reference systems".
> >
> > > >
> > > > To be honest I would not be concerned about 4-planes systems, we're
> > > > talking about imaging and what is relevant here is that we should be
> > > > allowed to express rectangles on reference systems that can be
> > > > oriented differently, think of a sensor's pixel array that might be
> > > > represented with the (0, 0) position in the top-left corner compared
> > > > to a plane 2-d image that usually has its origin in the bottom-left
> > > > corner.
> > > >
> > > > I have an hard time finding a use case for expressing negative
> > > > coordinates in this usage context. Furthermore that if we introduce 4
> > > > planes we lose the ability to describe systems with different
> > > > orientation, as this:
> > > >
> > > > ------------------------>
> > > > | o-------
> > > > | | |
> > > > | --------
> > > > |
> > > > |
> > > > V
> > > >
> > > > when expressed in a 4-planes system becomes
> > > >
> > > > ^
> > > > |
> > > > |
> > > > |
> > > > |
> > > > <--------------------------->
> > > > |/////////////
> > > > |/////////////
> > > > |// this /////
> > > > |/////////////
> > > > |/////////////
> > > > V
> > > >
> > > > And would require you to express y and height in negative coordinates.
> > > > Sincerely, I would not go there.
> > > >
> > > > I can specify that the Rectangle class reference system is always a
> > > > 2-d plane with positively increasing x and y axes instead.
> > >
> > > Yeah I think specifying that is a great idea, and maybe a TODO to
> > > set `x` and `y` to be unsigned int.
> >
> > I can try to do this right away and see if any compiler complains
> > about that.
> >
> > > I hope no pipeline handlers is using signed coordinates when we
> > > make the change in the API.
> >
> > However, geometry.h is public API, some applications might be affected
> > by this change.
> >
> > >
> > > Sorry for the chaos.
> > >
> >
> > np, if it helps to make the final result better chaos is welcome.
>
> Seems like I made even more chaos
>
> I tried to standardize the gemotry library to work with a single
> quadrant (what I called 'plane' in the previous email is actually a
> 'quadrant') but it's a mess, we translate and move rectangles and
> defining the behaviour when it happens to get into negative
> coordinates is tricky and I don't like the result.
>
> I also have gone through the issue with Laurent and it's probably
> better to take a step back
>
> 1) let's define top-left as the point with smallest x and smallest y
> (even if it's not actually top-left), I'll send a patch to update the doc
Yes, this is more ideal in my opinion, and it's what I expected :)
> 2) You 2 points constructors could be re-implemented on this by taking
> the min(p1.x, p2.x) and min(p1.y, p2.y), this shouldn't be ambiguous
> anymore if we always take the "smallest" point
> 4) with your ack I'll rebase your series on my patch and will send out
> a new version
Sure, thanks for the effort!
>
> once reviewed I'll merge them
>
> Thanks and sorry for this long thread
No worries. It's nice to discuss and find the best way to go with
together!
>
> >
> > > >
> > > > >
> > > > > >
> > > > > > The problem at hands now is relevant for 1-planes reference systems,
> > > > > > that can be oriented differently, in example with the origin point
> > > > > > being in top-left or bottom-left positions, as per the examples above.
> > > > > > In this case I think we agree that "top-left" is ambiguous. The x and
> > > > > > y coordinates are always positive in the case with a single plane.
> > > > > >
> > > > > > I think we should also be concerned with 1 plane reference systems and
> > > > > > move forward with this patch that removes references to the visual
> > > > > > "top-left" and assumes increments in width and height are always
> > > > > > positive.
> > > > > >
> > > > > > > >
> > > > > > > > > corner, and currently libcamera::Size only allows non-negative width and
> > > > > > > > > height [1].
> > > > > > > >
> > > > > > > > True, Size uses unsigned int. I don't think it's a big deal and I'm
> > > > > > > > not proposing to introduce a new constructor with signed width and
> > > > > > > > height now, but my point is that if the need arises we can now do
> > > > > > > > that.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > [1]:
> > > > > > > > > https://git.libcamera.org/libcamera/libcamera.git/tree/include/libcamera/geometry.h#n65
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > > BR,
> > > > > > > > > > > > > Harvey
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > * \fn Rectangle::Rectangle(int x, int y, unsigned int width,
> > > > > > > > > > > > unsigned int height)
> > > > > > > > > > > > > > > * \brief Construct a Rectangle with the given position and
> > > > > > > > > > size
> > > > > > > > > > > > > > > - * \param[in] x The horizontal coordinate of the top-left corner
> > > > > > > > > > > > > > > - * \param[in] y The vertical coordinate of the top-left corner
> > > > > > > > > > > > > > > + * \param[in] x The horizontal coordinate of the origin corner
> > > > > > > > > > > > > > > + * \param[in] y The vertical coordinate of the origin corner
> > > > > > > > > > > > > > > * \param[in] width The width
> > > > > > > > > > > > > > > * \param[in] height The height
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * The rectangle's origin corner is the corner closer to the
> > > > > > > > > > > > reference system
> > > > > > > > > > > > > > > + * origin point (0, 0).
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > * \fn Rectangle::Rectangle(const Size &size)
> > > > > > > > > > > > > > > - * \brief Construct a Rectangle of \a size with its top left
> > > > > > > > > > corner
> > > > > > > > > > > > located
> > > > > > > > > > > > > > > + * \brief Construct a Rectangle of \a size with its origin
> > > > > > > > > > corner
> > > > > > > > > > > > located
> > > > > > > > > > > > > > > * at (0,0)
> > > > > > > > > > > > > > > * \param[in] size The desired Rectangle size
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * The rectangle's origin corner is the corner closer to the
> > > > > > > > > > > > reference system
> > > > > > > > > > > > > > > + * origin point (0, 0).
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > * \var Rectangle::x
> > > > > > > > > > > > > > > - * \brief The horizontal coordinate of the rectangle's top-left
> > > > > > > > > > > > corner
> > > > > > > > > > > > > > > + * \brief The horizontal coordinate of the rectangle's origin
> > > > > > > > > > corner
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * The rectangle's origin corner is the corner closer to the
> > > > > > > > > > > > reference system
> > > > > > > > > > > > > > > + * origin point (0, 0).
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > * \var Rectangle::y
> > > > > > > > > > > > > > > - * \brief The vertical coordinate of the rectangle's top-left
> > > > > > > > > > corner
> > > > > > > > > > > > > > > + * \brief The vertical coordinate of the rectangle's origin
> > > > > > > > > > corner
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * The rectangle's origin corner is the corner closer to the
> > > > > > > > > > > > reference system
> > > > > > > > > > > > > > > + * origin point (0, 0).
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > @@ -683,9 +726,13 @@ Point Rectangle::center() const
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > - * \fn Point Rectangle::topLeft() const
> > > > > > > > > > > > > > > - * \brief Retrieve the coordinates of the top left corner of
> > > > > > > > > > this
> > > > > > > > > > > > Rectangle
> > > > > > > > > > > > > > > - * \return The Rectangle's top left corner
> > > > > > > > > > > > > > > + * \fn Point Rectangle::origin() const
> > > > > > > > > > > > > > > + * \brief Retrieve the coordinates of the origin corner of this
> > > > > > > > > > > > Rectangle
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * The rectangle's origin corner is the corner closer to the
> > > > > > > > > > > > reference system
> > > > > > > > > > > > > > > + * origin point (0, 0).
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * \return The Rectangle's origin corner
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I think you can drop "corner" from "origin corner"
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Probably throughout the patch:
> > > > > > > > > > > > > > s/origin corner/origin/
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Otherwise, LGTM
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > @@ -740,15 +787,15 @@ Rectangle &Rectangle::translateBy(const
> > > > > > > > > > Point
> > > > > > > > > > > > &point)
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > > Rectangle Rectangle::boundedTo(const Rectangle &bound) const
> > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > > - int topLeftX = std::max(x, bound.x);
> > > > > > > > > > > > > > > - int topLeftY = std::max(y, bound.y);
> > > > > > > > > > > > > > > + int originX = std::max(x, bound.x);
> > > > > > > > > > > > > > > + int originY = std::max(y, bound.y);
> > > > > > > > > > > > > > > int bottomRightX = std::min<int>(x + width, bound.x +
> > > > > > > > > > > > bound.width);
> > > > > > > > > > > > > > > int bottomRightY = std::min<int>(y + height, bound.y +
> > > > > > > > > > > > bound.height);
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > - unsigned int newWidth = std::max(bottomRightX - topLeftX,
> > > > > > > > > > 0);
> > > > > > > > > > > > > > > - unsigned int newHeight = std::max(bottomRightY - topLeftY,
> > > > > > > > > > 0);
> > > > > > > > > > > > > > > + unsigned int newWidth = std::max(bottomRightX - originX,
> > > > > > > > > > 0);
> > > > > > > > > > > > > > > + unsigned int newHeight = std::max(bottomRightY - originY,
> > > > > > > > > > 0);
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > - return { topLeftX, topLeftY, newWidth, newHeight };
> > > > > > > > > > > > > > > + return { originX, originY, newWidth, newHeight };
> > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > > > > > > > > b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > > > > > > > > > > > index 3041fd1ed9fd..8d2440026689 100644
> > > > > > > > > > > > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > > > > > > > > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > > > > > > > > > > > @@ -1289,7 +1289,7 @@ Rectangle CameraData::scaleIspCrop(const
> > > > > > > > > > > > Rectangle &ispCrop) const
> > > > > > > > > > > > > > > */
> > > > > > > > > > > > > > > Rectangle nativeCrop =
> > > > > > > > > > > > ispCrop.scaledBy(sensorInfo_.analogCrop.size(),
> > > > > > > > > > > > > > >
> > > > > > > > > > > > sensorInfo_.outputSize);
> > > > > > > > > > > > > > > - nativeCrop.translateBy(sensorInfo_.analogCrop.topLeft());
> > > > > > > > > > > > > > > + nativeCrop.translateBy(sensorInfo_.analogCrop.origin());
> > > > > > > > > > > > > > > return nativeCrop;
> > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > @@ -1303,7 +1303,7 @@ void CameraData::applyScalerCrop(const
> > > > > > > > > > > > ControlList &controls)
> > > > > > > > > > > > > > > nativeCrop = { 0, 0, 1, 1 };
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /* Create a version of the crop scaled to ISP
> > > > > > > > > > (camera
> > > > > > > > > > > > mode) pixels. */
> > > > > > > > > > > > > > > - Rectangle ispCrop =
> > > > > > > > > > > > nativeCrop.translatedBy(-sensorInfo_.analogCrop.topLeft());
> > > > > > > > > > > > > > > + Rectangle ispCrop =
> > > > > > > > > > > > nativeCrop.translatedBy(-sensorInfo_.analogCrop.origin());
> > > > > > > > > > > > > > > ispCrop.scaleBy(sensorInfo_.outputSize,
> > > > > > > > > > > > sensorInfo_.analogCrop.size());
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > /*
> > > > > > > > > > > > > > > diff --git a/src/py/libcamera/py_geometry.cpp
> > > > > > > > > > > > b/src/py/libcamera/py_geometry.cpp
> > > > > > > > > > > > > > > index c7e303609427..8dee365b25dd 100644
> > > > > > > > > > > > > > > --- a/src/py/libcamera/py_geometry.cpp
> > > > > > > > > > > > > > > +++ b/src/py/libcamera/py_geometry.cpp
> > > > > > > > > > > > > > > @@ -105,7 +105,7 @@ void init_py_geometry(py::module &m)
> > > > > > > > > > > > > > > .def_property_readonly("is_null",
> > > > > > > > > > &Rectangle::isNull)
> > > > > > > > > > > > > > > .def_property_readonly("center",
> > > > > > > > > > &Rectangle::center)
> > > > > > > > > > > > > > > .def_property_readonly("size", &Rectangle::size)
> > > > > > > > > > > > > > > - .def_property_readonly("topLeft",
> > > > > > > > > > &Rectangle::topLeft)
> > > > > > > > > > > > > > > + .def_property_readonly("origin",
> > > > > > > > > > &Rectangle::origin)
> > > > > > > > > > > > > > > .def("scale_by", &Rectangle::scaleBy)
> > > > > > > > > > > > > > > .def("translate_by", &Rectangle::translateBy)
> > > > > > > > > > > > > > > .def("bounded_to", &Rectangle::boundedTo)
> > > > > > > > > > > > > > > diff --git a/test/geometry.cpp b/test/geometry.cpp
> > > > > > > > > > > > > > > index 64169206ad16..a4ee6f6e167a 100644
> > > > > > > > > > > > > > > --- a/test/geometry.cpp
> > > > > > > > > > > > > > > +++ b/test/geometry.cpp
> > > > > > > > > > > > > > > @@ -363,16 +363,16 @@ protected:
> > > > > > > > > > > > > > > return TestFail;
> > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > - /* Rectangle::size(), Rectangle::topLeft() and
> > > > > > > > > > > > Rectangle::center() tests */
> > > > > > > > > > > > > > > + /* Rectangle::size(), Rectangle::origin() and
> > > > > > > > > > > > Rectangle::center() tests */
> > > > > > > > > > > > > > > if (Rectangle(-1, -2, 3, 4).size() != Size(3, 4) ||
> > > > > > > > > > > > > > > Rectangle(0, 0, 100000, 200000).size() !=
> > > > > > > > > > > > Size(100000, 200000)) {
> > > > > > > > > > > > > > > cout << "Rectangle::size() test failed" <<
> > > > > > > > > > > > endl;
> > > > > > > > > > > > > > > return TestFail;
> > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > - if (Rectangle(1, 2, 3, 4).topLeft() != Point(1, 2)
> > > > > > > > > > ||
> > > > > > > > > > > > > > > - Rectangle(-1, -2, 3, 4).topLeft() != Point(-1,
> > > > > > > > > > > > -2)) {
> > > > > > > > > > > > > > > - cout << "Rectangle::topLeft() test failed"
> > > > > > > > > > <<
> > > > > > > > > > > > endl;
> > > > > > > > > > > > > > > + if (Rectangle(1, 2, 3, 4).origin() != Point(1, 2)
> > > > > > > > > > ||
> > > > > > > > > > > > > > > + Rectangle(-1, -2, 3, 4).origin() != Point(-1,
> > > > > > > > > > -2))
> > > > > > > > > > > > {
> > > > > > > > > > > > > > > + cout << "Rectangle::origin() test failed"
> > > > > > > > > > <<
> > > > > > > > > > > > endl;
> > > > > > > > > > > > > > > return TestFail;
> > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > --
> > > > > > > > > > > > > > > 2.46.1
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > --
> > > > > > > > > > > > > BR,
> > > > > > > > > > > > > Harvey Yang
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > BR,
> > > > > > > Harvey Yang
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > BR,
> > > > > Harvey Yang
> > >
> > >
> > >
> > > --
> > > BR,
> > > Harvey Yang
--
BR,
Harvey Yang
More information about the libcamera-devel
mailing list