[v4,1/3] libcamera: Add rectangle two-point constructor
diff mbox series

Message ID 20240903104018.3289112-2-chenghaoyang@chromium.org
State Superseded
Headers show
Series
  • Add Face Detection Controls
Related show

Commit Message

Harvey Yang Sept. 3, 2024, 10:39 a.m. UTC 
From: Yudhistira Erlandinata <yerlandinata@chromium.org>

Add a Rectangle constructor that accepts two points:
topLeft and bottomRight.

Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/geometry.h |  2 ++
 src/libcamera/geometry.cpp   | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

Comments

Barnabás Pőcze Sept. 4, 2024, 11:26 a.m. UTC | #1 
Hi


2024. szeptember 3., kedd 13:39 keltezéssel, Harvey Yang <chenghaoyang@chromium.org> írta:

> From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> 
> Add a Rectangle constructor that accepts two points:
> topLeft and bottomRight.
> 
> Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@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.


Regards,
Barnabás Pőcze


> +
>  	int x;
>  	int y;
>  	unsigned int width;
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index 000151364..029b8dad2 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -629,6 +629,20 @@ std::ostream &operator<<(std::ostream &out, const SizeRange &sr)
>   * \param[in] size The desired Rectangle size
>   */
> 
> +/**
> + * \fn Rectangle::Rectangle(const Point &topLeft, const Point &bottomRight)
> + * \brief Construct a Rectangle with the two given points
> + * \param[in] topLeft The top-left corner
> + * \param[in] bottomRight The bottom-right corner
> + */
> +constexpr Rectangle::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);
> +}
> +
>  /**
>   * \var Rectangle::x
>   * \brief The horizontal coordinate of the rectangle's top-left corner
> --
> 2.46.0.469.g59c65b2a67-goog
> 
>
Harvey Yang Sept. 5, 2024, 4:11 a.m. UTC | #2 
Thanks Barnabás,

Updated on gitlab:
https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1264782
Will be included in v5.

BR,
Harvey

On Wed, Sep 4, 2024 at 7:26 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:

> Hi
>
>
> 2024. szeptember 3., kedd 13:39 keltezéssel, Harvey Yang <
> chenghaoyang@chromium.org> írta:
>
> > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> >
> > Add a Rectangle constructor that accepts two points:
> > topLeft and bottomRight.
> >
> > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@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.
>
>
> Regards,
> Barnabás Pőcze
>
>
> > +
> >       int x;
> >       int y;
> >       unsigned int width;
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index 000151364..029b8dad2 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -629,6 +629,20 @@ std::ostream &operator<<(std::ostream &out, const
> SizeRange &sr)
> >   * \param[in] size The desired Rectangle size
> >   */
> >
> > +/**
> > + * \fn Rectangle::Rectangle(const Point &topLeft, const Point
> &bottomRight)
> > + * \brief Construct a Rectangle with the two given points
> > + * \param[in] topLeft The top-left corner
> > + * \param[in] bottomRight The bottom-right corner
> > + */
> > +constexpr Rectangle::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);
> > +}
> > +
> >  /**
> >   * \var Rectangle::x
> >   * \brief The horizontal coordinate of the rectangle's top-left corner
> > --
> > 2.46.0.469.g59c65b2a67-goog
> >
> >
>
Jacopo Mondi Sept. 5, 2024, 8:35 a.m. UTC | #3 
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@chromium.org> írta:
>
> > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> >
> > Add a Rectangle constructor that accepts two points:
> > topLeft and bottomRight.
> >
> > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@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 ?

However, I can't find it clearly specified in cppreference. Do you
have any pointer ?

Anyway, if inline is the reason, isn't it better to inline the
definition and maintain the constexpr specifier ?

>
>
> Regards,
> Barnabás Pőcze
>
>
> > +
> >  	int x;
> >  	int y;
> >  	unsigned int width;
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index 000151364..029b8dad2 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -629,6 +629,20 @@ std::ostream &operator<<(std::ostream &out, const SizeRange &sr)
> >   * \param[in] size The desired Rectangle size
> >   */
> >
> > +/**
> > + * \fn Rectangle::Rectangle(const Point &topLeft, const Point &bottomRight)
> > + * \brief Construct a Rectangle with the two given points
> > + * \param[in] topLeft The top-left corner
> > + * \param[in] bottomRight The bottom-right corner
> > + */
> > +constexpr Rectangle::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);
> > +}
> > +
> >  /**
> >   * \var Rectangle::x
> >   * \brief The horizontal coordinate of the rectangle's top-left corner
> > --
> > 2.46.0.469.g59c65b2a67-goog
> >
> >
Barnabás Pőcze Sept. 15, 2024, 5:27 p.m. UTC | #4 
Hi


2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <jacopo.mondi@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@chromium.org> írta:
> >
> > > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > >
> > > Add a Rectangle constructor that accepts two points:
> > > topLeft and bottomRight.
> > >
> > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@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

> 
> 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.


Regards,
Barnabás Pőcze
Harvey Yang Sept. 16, 2024, 4:34 a.m. UTC | #5 
Hi Barnabás and Jacopo,

On Mon, Sep 16, 2024 at 1:27 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:

> Hi
>
>
> 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> jacopo.mondi@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@chromium.org> írta:
> > >
> > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > >
> > > > Add a Rectangle constructor that accepts two points:
> > > > topLeft and bottomRight.
> > > >
> > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@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
>
> >
> > 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"
```

BR,
Harvey


>
> Regards,
> Barnabás Pőcze
>
>
Jacopo Mondi Sept. 16, 2024, 7:24 a.m. UTC | #6 
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@protonmail.com> wrote:
>
> > Hi
> >
> >
> > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> > jacopo.mondi@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@chromium.org> írta:
> > > >
> > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > >
> > > > > Add a Rectangle constructor that accepts two points:
> > > > > topLeft and bottomRight.
> > > > >
> > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@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 ?

I'll ask others what they think about this
>
> BR,
> Harvey
>
>
> >
> > Regards,
> > Barnabás Pőcze
> >
> >
Jacopo Mondi Sept. 19, 2024, 11:27 a.m. UTC | #7 
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@protonmail.com> wrote:
> >
> > > Hi
> > >
> > >
> > > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> > > jacopo.mondi@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@chromium.org> írta:
> > > > >
> > > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > > >
> > > > > > Add a Rectangle constructor that accepts two points:
> > > > > > topLeft and bottomRight.
> > > > > >
> > > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@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 ?

> 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))
	{
	}

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;
		}

Thanks
  j

> >
> > BR,
> > Harvey
> >
> >
> > >
> > > Regards,
> > > Barnabás Pőcze
> > >
> > >
Cheng-Hao Yang Sept. 19, 2024, 2:15 p.m. UTC | #8 
Hi Jacopo,

On Thu, Sep 19, 2024 at 7:27 PM Jacopo Mondi <jacopo.mondi@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@protonmail.com>
> wrote:
> > >
> > > > Hi
> > > >
> > > >
> > > > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> > > > jacopo.mondi@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@chromium.org> írta:
> > > > > >
> > > > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > > > >
> > > > > > > Add a Rectangle constructor that accepts two points:
> > > > > > > topLeft and bottomRight.
> > > > > > >
> > > > > > > Signed-off-by: Yudhistira Erlandinata <
> yerlandinata@chromium.org>
> > > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@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


>
> > 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


>
> 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;
>                 }

Thanks
>   j
>
> > >
> > > BR,
> > > Harvey
> > >
> > >
> > > >
> > > > Regards,
> > > > Barnabás Pőcze
> > > >
> > > >
>
Jacopo Mondi Sept. 23, 2024, 8:32 a.m. UTC | #9 
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@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@protonmail.com>
> > wrote:
> > > >
> > > > > Hi
> > > > >
> > > > >
> > > > > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> > > > > jacopo.mondi@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@chromium.org> írta:
> > > > > > >
> > > > > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > > > > >
> > > > > > > > Add a Rectangle constructor that accepts two points:
> > > > > > > > topLeft and bottomRight.
> > > > > > > >
> > > > > > > > Signed-off-by: Yudhistira Erlandinata <
> > yerlandinata@chromium.org>
> > > > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@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 ?

>
>
> >
> > > 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
>
>
> >
> > 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;
> >                 }
>
> Thanks
> >   j
> >
> > > >
> > > > BR,
> > > > Harvey
> > > >
> > > >
> > > > >
> > > > > Regards,
> > > > > Barnabás Pőcze
> > > > >
> > > > >
> >
>
>
> --
> BR,
> Harvey Yang
Kieran Bingham Sept. 23, 2024, 8:42 a.m. UTC | #10 
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@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@protonmail.com>
> > wrote:
> > > >
> > > > > Hi
> > > > >
> > > > >
> > > > > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> > > > > jacopo.mondi@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@chromium.org> írta:
> > > > > > >
> > > > > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > > > > >
> > > > > > > > Add a Rectangle constructor that accepts two points:
> > > > > > > > topLeft and bottomRight.
> > > > > > > >
> > > > > > > > Signed-off-by: Yudhistira Erlandinata <
> > yerlandinata@chromium.org>
> > > > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@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
Kieran Bingham Sept. 23, 2024, 8:48 a.m. UTC | #11 
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@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@protonmail.com>
> > > wrote:
> > > > >
> > > > > > Hi
> > > > > >
> > > > > >
> > > > > > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> > > > > > jacopo.mondi@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@chromium.org> írta:
> > > > > > > >
> > > > > > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > > > > > >
> > > > > > > > > Add a Rectangle constructor that accepts two points:
> > > > > > > > > topLeft and bottomRight.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Yudhistira Erlandinata <
> > > yerlandinata@chromium.org>
> > > > > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@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.

--
Kieran
Jacopo Mondi Sept. 23, 2024, 9:20 a.m. UTC | #12 
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@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@protonmail.com>
> > > > wrote:
> > > > > >
> > > > > > > Hi
> > > > > > >
> > > > > > >
> > > > > > > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> > > > > > > jacopo.mondi@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@chromium.org> írta:
> > > > > > > > >
> > > > > > > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > > > > > > >
> > > > > > > > > > Add a Rectangle constructor that accepts two points:
> > > > > > > > > > topLeft and bottomRight.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Yudhistira Erlandinata <
> > > > yerlandinata@chromium.org>
> > > > > > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@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
Cheng-Hao Yang Sept. 23, 2024, 9:44 a.m. UTC | #13 
Hi Jacopo and Kieran,

Uploaded v5. Please take another look.

On Mon, Sep 23, 2024 at 5:20 PM Jacopo Mondi <jacopo.mondi@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@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@protonmail.com>
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi
> > > > > > > >
> > > > > > > >
> > > > > > > > 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo
> Mondi <
> > > > > > > > jacopo.mondi@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@chromium.org> írta:
> > > > > > > > > >
> > > > > > > > > > > From: Yudhistira Erlandinata <
> yerlandinata@chromium.org>
> > > > > > > > > > >
> > > > > > > > > > > Add a Rectangle constructor that accepts two points:
> > > > > > > > > > > topLeft and bottomRight.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Yudhistira Erlandinata <
> > > > > yerlandinata@chromium.org>
> > > > > > > > > > > Co-developed-by: Harvey Yang <
> chenghaoyang@chromium.org>
> > > > > > > > > > > Reviewed-by: Jacopo Mondi <
> jacopo.mondi@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
>

Patch
diff mbox series 

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);
+
 	int x;
 	int y;
 	unsigned int width;
diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
index 000151364..029b8dad2 100644
--- a/src/libcamera/geometry.cpp
+++ b/src/libcamera/geometry.cpp
@@ -629,6 +629,20 @@  std::ostream &operator<<(std::ostream &out, const SizeRange &sr)
  * \param[in] size The desired Rectangle size
  */
 
+/**
+ * \fn Rectangle::Rectangle(const Point &topLeft, const Point &bottomRight)
+ * \brief Construct a Rectangle with the two given points
+ * \param[in] topLeft The top-left corner
+ * \param[in] bottomRight The bottom-right corner
+ */
+constexpr Rectangle::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);
+}
+
 /**
  * \var Rectangle::x
  * \brief The horizontal coordinate of the rectangle's top-left corner