[PATCH v4 1/3] libcamera: Add rectangle two-point constructor
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Sep 23 11:20:07 CEST 2024
Hi Kieran
On Mon, Sep 23, 2024 at 09:48:07AM GMT, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-09-23 09:32:01)
> > Hi Harvey
> >
> > On Thu, Sep 19, 2024 at 10:15:14PM GMT, Cheng-Hao Yang wrote:
> > > Hi Jacopo,
> > >
> > > On Thu, Sep 19, 2024 at 7:27 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > wrote:
> > >
> > > > Hi Harvey
> > > >
> > > > On Mon, Sep 16, 2024 at 09:24:23AM GMT, Jacopo Mondi wrote:
> > > > > Hello
> > > > >
> > > > > On Mon, Sep 16, 2024 at 12:34:54PM GMT, Cheng-Hao Yang wrote:
> > > > > > Hi Barnabás and Jacopo,
> > > > > >
> > > > > > On Mon, Sep 16, 2024 at 1:27 AM Barnabás Pőcze <pobrn at protonmail.com>
> > > > wrote:
> > > > > >
> > > > > > > Hi
> > > > > > >
> > > > > > >
> > > > > > > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> > > > > > > jacopo.mondi at ideasonboard.com> írta:
> > > > > > >
> > > > > > > > Hi Barnabás
> > > > > > > >
> > > > > > > > On Wed, Sep 04, 2024 at 11:26:37AM GMT, Barnabás Pőcze wrote:
> > > > > > > > > Hi
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > 2024. szeptember 3., kedd 13:39 keltezéssel, Harvey Yang <
> > > > > > > chenghaoyang at chromium.org> írta:
> > > > > > > > >
> > > > > > > > > > From: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > > > > > > > >
> > > > > > > > > > Add a Rectangle constructor that accepts two points:
> > > > > > > > > > topLeft and bottomRight.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Yudhistira Erlandinata <
> > > > yerlandinata at chromium.org>
> > > > > > > > > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > > > > > > > > ---
> > > > > > > > > > include/libcamera/geometry.h | 2 ++
> > > > > > > > > > src/libcamera/geometry.cpp | 14 ++++++++++++++
> > > > > > > > > > 2 files changed, 16 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/include/libcamera/geometry.h
> > > > > > > b/include/libcamera/geometry.h
> > > > > > > > > > index 3e6f0f5d7..dc56f180f 100644
> > > > > > > > > > --- a/include/libcamera/geometry.h
> > > > > > > > > > +++ b/include/libcamera/geometry.h
> > > > > > > > > > @@ -262,6 +262,8 @@ public:
> > > > > > > > > > {
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > + constexpr Rectangle(const Point &topLeft, const Point
> > > > > > > &bottomRight);
> > > > > > > > >
> > > > > > > > > Don't make this `constexpr` because it is not useful since the
> > > > > > > definition is not available.
> > > > > > > >
> > > > > > > > I found references online that constexpr constuctors are implcitly
> > > > > > > > inline, is this the reason of your comment ?
> > > > > > >
> > > > > > > Yes.
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > However, I can't find it clearly specified in cppreference. Do you
> > > > > > > > have any pointer ?
> > > > > > >
> > > > > > > "A constexpr specifier used in a function or static data
> > > > member(since
> > > > > > > C++17) declaration implies inline."
> > > > > > > -- https://en.cppreference.com/w/cpp/language/constexpr
> > > > > > >
> > > > >
> > > > >
> > > > > One day I'll lear to read properly
> > > > >
> > > > > > > >
> > > > > > > > Anyway, if inline is the reason, isn't it better to inline the
> > > > > > > > definition and maintain the constexpr specifier ?
> > > > > > > > [...]
> > > > > > >
> > > > > > > That is an option as well.
> > > > > > >
> > > > > >
> > > > > > IIUC, you're suggesting to put the definition of the new c'tor
> > > > > > back in the header file? As we use `ASSERT` in this c'tor
> > > > > > definition, there will be a compile error if we do that:
> > > > > > ```
> > > > > >
> > > > > > In file included from ../include/libcamera/base/log.h:12,
> > > > > >
> > > > > > from ../include/libcamera/geometry.h:16,
> > > > > >
> > > > > > from ../include/libcamera/controls.h:21,
> > > > > >
> > > > > > from ../include/libcamera/camera.h:22,
> > > > > >
> > > > > > from ../src/apps/common/dng_writer.h:13,
> > > > > >
> > > > > > from ../src/apps/common/dng_writer.cpp:8:
> > > > > >
> > > > > > ../include/libcamera/base/private.h:21:2: error: #error "Private
> > > > headers
> > > > > > must not be included in the libcamera API"
> > > > > >
> > > > > > 21 | #error "Private headers must not be included in the libcamera
> > > > API"
> > > > > > ```
> > > > >
> > > > > I see. log.h is private, and the fact ASSERT is not exposed in the
> > > > > public API makes me think aborting execution on an invalid user input
> > > > > is probably not the best idea ? That's maybe the reason why ASSERT is not
> > > > > exposed to the public API ?
> > > > >
> > > >
> > > > Assuming the top-left corner is always defined as the point with the
> > > > smaller 'x' and the higher 'y' whatever the reference system the
> > > > rectangle is placed in:
> > > >
> > > > In example:
> > > >
> > > > (0,0)
> > > > -------------------------------->
> > > > |
> > > > | -------------------
> > > > | | |
> > > > | | |
> > > > | x------------------
> > > > |
> > > > V
> > > >
> > > > Or:
> > > >
> > > > ^
> > > > |
> > > > |
> > > > | x------------------
> > > > | | |
> > > > | | |
> > > > | -------------------
> > > > |
> > > > |
> > > > -------------------------------->
> > > > (0,0)
> > > >
> > > > The assertion you have introduced in your proposed constructor
> > > >
> > > > constexpr Rectangle(const Point &topLeft, const Point &bottomRight)
> > > > : x(topLeft.x), y(topLeft.y),
> > > > width(bottomRight.x - x),
> > > > height(bottomRight.y - y)
> > > > {
> > > > ASSERT(bottomRight.x >= x && bottomRight.y >= y);
> > > > }
> > > >
> > > > is wrong as bottomRight.y shall always be smaller than topLeft.y
> > > >
> > > > Has this code been ever run ?
> > > >
> > >
> > > Sorry for the confusion, but the `topLeft` point is actually somewhere
> > > like this:
> > >
> > > (0,0)
> > > -------------------------------->
> > > |
> > > | x-----------------
> > > | | |
> > > | | |
> > > | -------------------
> > > |
> > > V
> > >
> > > `top-left` is (0, 0) in Android metadata's description [1], and it seems
> > > that
> > > it's also in libcamera's description to indicate a point with smaller x and
> > > y coordinates [2].
> > >
> > > [1]:
> > > https://android.googlesource.com/platform/system/media/+/refs/heads/main/camera/docs/docs.html#33019
> > > [2]:
> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/geometry.cpp#n639
> >
> > Is this because of
> >
> > /**
> > * \fn Rectangle::Rectangle(const Size &size)
> > * \brief Construct a Rectangle of \a size with its top left corner located
> > * at (0,0)
> >
> > This clearly doesn't match this case
> >
> > ^
> > |
> > |
> > | x------------------
> > | | |
> > | | |
> > | -------------------
> > |
> > |
> > -------------------------------->
> > (0,0)
> >
> > Maybe we have been a bit short-sighted and only assumed the blelow
> > reference system had to be taken into account.
> >
> > (0,0)
> > -------------------------------->
> > |
> > | x-----------------
> > | | |
> > | | |
> > | -------------------
> > |
> > V
> >
> > Personally I would remove this assumption in our doc and define the
> > top-left corner as the point with smaller x and largest y
> >
> > Opinions from libcamera people ?
>
> Aha, I would define top left as 0,0 :-)
>
> I've just opened up GIMP which also uses top left origin.
>
> I expect other image applications would also honour this - so worth
> checking a few ...
>
> For me - even though this is 'geometry' ... it's *image processing*
> geometry.
>
The Rectangle class documentation says already
* 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.
Which seems to suggest that no assumptions on the reference system are
made in the class, but however we also have one constructor that is
documented as
* \brief Construct a Rectangle of \a size with its top left corner located
* at (0,0)
Which, in the below example suggestes "top-left" is actually visually
bottom-right.
^
|
|
| -------------------
| | |
| | |
| X------------------
|
|
-------------------------------->
(0,0)
If it is instead assumed that "top-left" is is the visual top-left
regardless of the coordinate system so we should clarify what
directions do we increment x and y when give a constructor like
Rectangle({x,y}, w, h) as in this case we grow both x and y
(0,0)
-------------------------------->
|
| x------------------
| | |
| | |
| -------------------
|
V
while in this we increase x and decrement y
^
|
|
| x------------------
| | |
| | |
| -------------------
|
|
-------------------------------->
(0,0)
but it would be totally legit to construct the same rectangle as
^ -------------------
| | |
| | |
| x------------------
|
|
|
|
|
-------------------------------->
(0,0)
To remove ambiguities I would rather redefine top-left as "origin"
point defined as the point wiht the lower x and lower y of the
rectangle and clarify that we always increase width and height
from there when constructing Rectangle({x,y}, w, h)
(0,0)
-------------------------------->
|
| x------------------
| | |
| | |
| -------------------
|
V
^
|
|
| -------------------
| | |
| | |
| x------------------
|
|
-------------------------------->
(0,0)
And now define the newly constructor proposed by Harvey as the
two-point constructur I suggested.
Hope I'm not overthinking this.
> --
> Kieran
More information about the libcamera-devel
mailing list