[PATCH v4 1/3] libcamera: Add rectangle two-point constructor
Cheng-Hao Yang
chenghaoyang at google.com
Mon Sep 23 11:44:32 CEST 2024
Hi Jacopo and Kieran,
Uploaded v5. Please take another look.
On Mon, Sep 23, 2024 at 5:20 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> 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.
>
nit: bottom-left
>
>
> ^
> |
> |
> | -------------------
> | | |
> | | |
> | 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
>
I think this constructor creates a Rectangle like this:
(0,0)
-------------------------------->
| |
| |
| |
| |
| ------------------(Size.width, Size.height)
|
V
Which is a rectangle surrounded by (0, 0) and
(Size.width, Size.height).
> 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.
>
Updated it to be two diagonal points. I've updated the variable names
to align with the original documentation, which `topLeft` means the one
with the minimum x and y coordinates.
Let me know what's the better variable names instead though, if we
redefine `top-left` as the origin point `(0, 0)`.
(Or just adjust it for me before accepting the patches, if that's the last
issue in question).
Thanks!
>
> Hope I'm not overthinking this.
>
> > --
> > Kieran
>
--
BR,
Harvey Yang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240923/51695ea4/attachment.htm>
More information about the libcamera-devel
mailing list