[v2,1/3] libcamera: Add rectangle two-point constructor

21003 diff mbox series

From: Yudhistira Erlandinata <yerlandinata@chromium.org>

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

BUG=b:308714092
TEST=emerge-geralt libcamera-mtkisp7

Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/geometry.h | 10 ++++++++++
 src/libcamera/geometry.cpp   | 11 +++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

Hi

On Fri, Aug 23, 2024 at 02:29:08PM GMT, Harvey Yang wrote:
> From: Yudhistira Erlandinata <yerlandinata@chromium.org>
>
> Add a Rectangle constructor that accepts two points:
> topLeft and bottomRight.
>
> BUG=b:308714092
> TEST=emerge-geralt libcamera-mtkisp7

Please drop them, not related to libcamera

>
> Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  include/libcamera/geometry.h | 10 ++++++++++
>  src/libcamera/geometry.cpp   | 11 +++++++++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> index 3e6f0f5d7..978322a22 100644
> --- a/include/libcamera/geometry.h
> +++ b/include/libcamera/geometry.h
> @@ -8,7 +8,9 @@
>  #pragma once
>
>  #include <algorithm>
> +#include <assert.h>
>  #include <ostream>
> +#include <stdlib.h>

What is this for ?

>  #include <string>
>
>  #include <libcamera/base/compiler.h>
> @@ -262,6 +264,14 @@ public:
>  	{
>  	}
>
> +	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);

assert() is fine, however we have our own ASSERT() defined in log.h
which can be conditionally disabled. The rest of the codebase uses
that.

> +	}
> +
>  	int x;
>  	int y;
>  	unsigned int width;
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index 000151364..86385ad35 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -5,13 +5,13 @@
>   * Geometry-related structures
>   */
>
> -#include <libcamera/geometry.h>
> -
>  #include <sstream>
>  #include <stdint.h>
>
>  #include <libcamera/base/log.h>
>
> +#include <libcamera/geometry.h>
> +

why ?

>  /**
>   * \file geometry.h
>   * \brief Data structures related to geometric objects
> @@ -629,6 +629,13 @@ 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
> + */
> +

the rest looks good

>  /**
>   * \var Rectangle::x
>   * \brief The horizontal coordinate of the rectangle's top-left corner
> --
> 2.46.0.295.g3b9ea8a38a-goog
>

Thanks Jacopo,

On Wed, Aug 28, 2024 at 2:10 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>
wrote:

> Hi
>
> On Fri, Aug 23, 2024 at 02:29:08PM GMT, Harvey Yang wrote:
> > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> >
> > Add a Rectangle constructor that accepts two points:
> > topLeft and bottomRight.
> >
> > BUG=b:308714092
> > TEST=emerge-geralt libcamera-mtkisp7
>
> Please drop them, not related to libcamera
>
Removed.


>
> >
> > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  include/libcamera/geometry.h | 10 ++++++++++
> >  src/libcamera/geometry.cpp   | 11 +++++++++--
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > index 3e6f0f5d7..978322a22 100644
> > --- a/include/libcamera/geometry.h
> > +++ b/include/libcamera/geometry.h
> > @@ -8,7 +8,9 @@
> >  #pragma once
> >
> >  #include <algorithm>
> > +#include <assert.h>
> >  #include <ostream>
> > +#include <stdlib.h>
>
> What is this for ?
>
Removed.

>
> >  #include <string>
> >
> >  #include <libcamera/base/compiler.h>
> > @@ -262,6 +264,14 @@ public:
> >       {
> >       }
> >
> > +     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);
>
> assert() is fine, however we have our own ASSERT() defined in log.h
> which can be conditionally disabled. The rest of the codebase uses
> that.
>
Right, thanks! Done.

>
> > +     }
> > +
> >       int x;
> >       int y;
> >       unsigned int width;
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index 000151364..86385ad35 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -5,13 +5,13 @@
> >   * Geometry-related structures
> >   */
> >
> > -#include <libcamera/geometry.h>
> > -
> >  #include <sstream>
> >  #include <stdint.h>
> >
> >  #include <libcamera/base/log.h>
> >
> > +#include <libcamera/geometry.h>
> > +
>
> why ?
>
My linter went crazy... Reverted.


>
> >  /**
> >   * \file geometry.h
> >   * \brief Data structures related to geometric objects
> > @@ -629,6 +629,13 @@ 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
> > + */
> > +
>
> the rest looks good
>
> >  /**
> >   * \var Rectangle::x
> >   * \brief The horizontal coordinate of the rectangle's top-left corner
> > --
> > 2.46.0.295.g3b9ea8a38a-goog
> >
>