[PATCH v4 1/3] libcamera: Add rectangle two-point constructor
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Sep 23 10:42:11 CEST 2024
Quoting Cheng-Hao Yang (2024-09-19 15:15:14)
> 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 ?
Indeed, we that error means we really can't use that feature in a place
that's in public headers.
> > >
> >
> > 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
>
Top left being 0,0 is how I understand coordinates on images too. Of
course for maths it's usually bottom left ...
Perhaps we need to do better for documenting our expectations on origin point coordinates ....
>
> >
> > > I'll ask others what they think about this
> >
> > As they're smarter than me, they made me notice that you should rather
> > define a constructor with Point1 and Point2 and construct a Rectangle
> > that spans between these two points defined using the existing
> > Rectangle constructor as:
> >
> > constexpr Rectangle(const Point &point1, const Point &point2)
> > : Rectangle(std::min(point1.x, point2.x),
> > std::max(point1.y, point2.y),
> > std::max(point1.x, point2.x) -
> > std::min(point1.x, point2.x),
> > std::max(point1.y, point2.y) -
> > std::min(point1.y, point2.y))
> > {
> > }
> >
>
> We originally were trying to avoid adding this powerful c'tor that allows
> every
> combination, as libcamera's Rectangle API seems to prefer indicating the
> `topLeft` point in c'tors [3].
>
> Are you sure that the span is what we prefer?
>
> [3]:
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/geometry.cpp#n609
>
>
If only a single point is given, then indeed it has to be defined at
which location it is (hence [3]). But if a rectangle is produced from
two (opposing corner) points, then the dimensions can be clearly
obtained so I do think constructing from two points with the min/max
helpers is a suitable way to go here without needing any assertsions.
> >
> > Please make sure to add proper documentation for this new constructor.
> >
> > You can also add the following snippet to test/geometry.cpp
> >
> > Point topLeft(3, 30);
> > Point bottomRight(30, 3);
> > Point topRight(30, 30);
> > Point bottomLeft(3, 3);
> > Rectangle rect1(topLeft, bottomRight);
> > Rectangle rect2(topRight, bottomLeft);
> > Rectangle rect3(bottomRight, topLeft);
> > Rectangle rect4(bottomLeft, topRight);
> >
> > if (rect1 != rect2 || rect1 != rect3 || rect1 != rect4) {
> > cout << "Point-from-point construction failed" <<
> > endl;
> > return TestFail;
> > }
And looks like a good addition to codify it all.
--
Kieran
>
> Thanks
> > j
> >
> > > >
> > > > BR,
> > > > Harvey
> > > >
> > > >
> > > > >
> > > > > Regards,
> > > > > Barnabás Pőcze
> > > > >
> > > > >
> >
>
>
> --
> BR,
> Harvey Yang
More information about the libcamera-devel
mailing list