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

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

Commit Message

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

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