[libcamera-devel,3/5] libcamera: geometry: Give constructors to Rectangle
diff mbox series

Message ID 20200714234009.16596-4-laurent.pinchart@ideasonboard.com
State Accepted
Commit 4f509caa8ed0166ef2e99846f12b7997a37c3a7e
Headers show
Series
  • libcamera: Various improvements to geometry classes
Related show

Commit Message

Laurent Pinchart July 14, 2020, 11:40 p.m. UTC 
Rectangle, unlike Size, has no constructor, requiring the users to
explicitly initialize the instances. This is error-prone, add
constructors.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/geometry.h                  | 15 +++++++++++++
 src/libcamera/camera_sensor.cpp               |  2 +-
 src/libcamera/geometry.cpp                    | 22 +++++++++++++++++++
 src/libcamera/pipeline/ipu3/imgu.cpp          |  7 +-----
 .../pipeline/raspberrypi/raspberrypi.cpp      |  7 +-----
 src/libcamera/pipeline/vimc/vimc.cpp          |  7 +-----
 6 files changed, 41 insertions(+), 19 deletions(-)

Comments

Niklas Söderlund July 15, 2020, 6:33 a.m. UTC | #1 
Hi Laurent,

Thanks for your work.

On 2020-07-15 02:40:07 +0300, Laurent Pinchart wrote:
> Rectangle, unlike Size, has no constructor, requiring the users to
> explicitly initialize the instances. This is error-prone, add
> constructors.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/geometry.h                  | 15 +++++++++++++
>  src/libcamera/camera_sensor.cpp               |  2 +-
>  src/libcamera/geometry.cpp                    | 22 +++++++++++++++++++
>  src/libcamera/pipeline/ipu3/imgu.cpp          |  7 +-----
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  7 +-----
>  src/libcamera/pipeline/vimc/vimc.cpp          |  7 +-----
>  6 files changed, 41 insertions(+), 19 deletions(-)
> 
> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> index f3088d33fa2c..380248ac9a50 100644
> --- a/include/libcamera/geometry.h
> +++ b/include/libcamera/geometry.h
> @@ -127,6 +127,21 @@ static inline bool operator!=(const SizeRange &lhs, const SizeRange &rhs)
>  }
>  
>  struct Rectangle {

I wonder if we should not turn Size and Rectangle into classes instead 
of structs when we start to add methods to them. Or is there an 
advantage to keep them as structs?

> +	Rectangle()
> +		: Rectangle(0, 0, 0, 0)
> +	{
> +	}
> +
> +	Rectangle(int xpos, int ypos, const Size &size)
> +		: x(xpos), y(ypos), width(size.width), height(size.height)
> +	{
> +	}
> +
> +	Rectangle(int xpos, int ypos, unsigned int w, unsigned int h)
> +		: x(xpos), y(ypos), width(w), height(h)
> +	{
> +	}
> +
>  	int x;
>  	int y;
>  	unsigned int width;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index b14b4051dca6..6e93cc51155b 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -487,7 +487,7 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  	info->model = model();
>  
>  	/* Get the active area size. */
> -	Rectangle rect = {};
> +	Rectangle rect;
>  	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
>  	if (ret) {
>  		LOG(CameraSensor, Error)
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index 5a0bcf7c1e12..3a3784df39e6 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -297,6 +297,28 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
>   * refers to, are defined by the context were rectangle is used.
>   */
>  
> +/**
> + * \fn Rectangle::Rectangle()
> + * \brief Construct a Rectangle with all coordinates set to 0
> + */
> +
> +/**
> + * \fn Rectangle::Rectangle(int x, int y, const Size &size)
> + * \brief Construct a Rectangle with the given position and size
> + * \param[in] x The horizontal coordinate of the top-left corner
> + * \param[in] y The vertical coordinate of the top-left corner
> + * \param[in] size The size
> + */
> +
> +/**
> + * \fn Rectangle::Rectangle(int x, int y, unsigned int width, unsigned int height)
> + * \brief Construct a Rectangle with the given position and size
> + * \param[in] x The horizontal coordinate of the top-left corner
> + * \param[in] y The vertical coordinate of the top-left corner
> + * \param[in] width The width
> + * \param[in] height The height
> + */
> +
>  /**
>   * \var Rectangle::x
>   * \brief The horizontal coordinate of the rectangle's top-left corner
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index d7f4173d3607..4bec4b2f1a76 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -100,12 +100,7 @@ int ImgUDevice::configureInput(const Size &size,
>  	 * to configure the crop/compose rectangles, contradicting the
>  	 * V4L2 specification.
>  	 */
> -	Rectangle rect = {
> -		.x = 0,
> -		.y = 0,
> -		.width = inputFormat->size.width,
> -		.height = inputFormat->size.height,
> -	};
> +	Rectangle rect{ 0, 0, inputFormat->size };

Out out curiosity why do you use list initialization ?

Questions for my education apart,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

>  	ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect);
>  	if (ret)
>  		return ret;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 009e502b288c..2d848ac3c735 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -752,12 +752,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	}
>  
>  	/* Adjust aspect ratio by providing crops on the input image. */
> -	Rectangle crop = {
> -		.x = 0,
> -		.y = 0,
> -		.width = sensorFormat.size.width,
> -		.height = sensorFormat.size.height
> -	};
> +	Rectangle crop{ 0, 0, sensorFormat.size };
>  
>  	int ar = maxSize.height * sensorFormat.size.width - maxSize.width * sensorFormat.size.height;
>  	if (ar > 0)
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index a6f457c71fb1..4f461b928514 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -256,12 +256,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
>  		return ret;
>  
>  	if (data->media_->version() >= KERNEL_VERSION(5, 6, 0)) {
> -		Rectangle crop = {
> -			.x = 0,
> -			.y = 0,
> -			.width = subformat.size.width,
> -			.height = subformat.size.height,
> -		};
> +		Rectangle crop{ 0, 0, subformat.size };
>  		ret = data->scaler_->setSelection(0, V4L2_SEL_TGT_CROP, &crop);
>  		if (ret)
>  			return ret;
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart July 15, 2020, 9:31 a.m. UTC | #2 
Hi Niklas,

On Wed, Jul 15, 2020 at 08:33:45AM +0200, Niklas Söderlund wrote:
> On 2020-07-15 02:40:07 +0300, Laurent Pinchart wrote:
> > Rectangle, unlike Size, has no constructor, requiring the users to
> > explicitly initialize the instances. This is error-prone, add
> > constructors.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/geometry.h                  | 15 +++++++++++++
> >  src/libcamera/camera_sensor.cpp               |  2 +-
> >  src/libcamera/geometry.cpp                    | 22 +++++++++++++++++++
> >  src/libcamera/pipeline/ipu3/imgu.cpp          |  7 +-----
> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  7 +-----
> >  src/libcamera/pipeline/vimc/vimc.cpp          |  7 +-----
> >  6 files changed, 41 insertions(+), 19 deletions(-)
> > 
> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > index f3088d33fa2c..380248ac9a50 100644
> > --- a/include/libcamera/geometry.h
> > +++ b/include/libcamera/geometry.h
> > @@ -127,6 +127,21 @@ static inline bool operator!=(const SizeRange &lhs, const SizeRange &rhs)
> >  }
> >  
> >  struct Rectangle {
> 
> I wonder if we should not turn Size and Rectangle into classes instead 
> of structs when we start to add methods to them. Or is there an 
> advantage to keep them as structs?

See the rest of the series :-)

> > +	Rectangle()
> > +		: Rectangle(0, 0, 0, 0)
> > +	{
> > +	}
> > +
> > +	Rectangle(int xpos, int ypos, const Size &size)
> > +		: x(xpos), y(ypos), width(size.width), height(size.height)
> > +	{
> > +	}
> > +
> > +	Rectangle(int xpos, int ypos, unsigned int w, unsigned int h)
> > +		: x(xpos), y(ypos), width(w), height(h)
> > +	{
> > +	}
> > +
> >  	int x;
> >  	int y;
> >  	unsigned int width;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index b14b4051dca6..6e93cc51155b 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -487,7 +487,7 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> >  	info->model = model();
> >  
> >  	/* Get the active area size. */
> > -	Rectangle rect = {};
> > +	Rectangle rect;
> >  	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
> >  	if (ret) {
> >  		LOG(CameraSensor, Error)
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index 5a0bcf7c1e12..3a3784df39e6 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -297,6 +297,28 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
> >   * refers to, are defined by the context were rectangle is used.
> >   */
> >  
> > +/**
> > + * \fn Rectangle::Rectangle()
> > + * \brief Construct a Rectangle with all coordinates set to 0
> > + */
> > +
> > +/**
> > + * \fn Rectangle::Rectangle(int x, int y, const Size &size)
> > + * \brief Construct a Rectangle with the given position and size
> > + * \param[in] x The horizontal coordinate of the top-left corner
> > + * \param[in] y The vertical coordinate of the top-left corner
> > + * \param[in] size The size
> > + */
> > +
> > +/**
> > + * \fn Rectangle::Rectangle(int x, int y, unsigned int width, unsigned int height)
> > + * \brief Construct a Rectangle with the given position and size
> > + * \param[in] x The horizontal coordinate of the top-left corner
> > + * \param[in] y The vertical coordinate of the top-left corner
> > + * \param[in] width The width
> > + * \param[in] height The height
> > + */
> > +
> >  /**
> >   * \var Rectangle::x
> >   * \brief The horizontal coordinate of the rectangle's top-left corner
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index d7f4173d3607..4bec4b2f1a76 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -100,12 +100,7 @@ int ImgUDevice::configureInput(const Size &size,
> >  	 * to configure the crop/compose rectangles, contradicting the
> >  	 * V4L2 specification.
> >  	 */
> > -	Rectangle rect = {
> > -		.x = 0,
> > -		.y = 0,
> > -		.width = inputFormat->size.width,
> > -		.height = inputFormat->size.height,
> > -	};
> > +	Rectangle rect{ 0, 0, inputFormat->size };
> 
> Out out curiosity why do you use list initialization ?
> 
> Questions for my education apart,

Mostly because the code was already using curly braces :-)

There are a few important differences between 

	Rectangle rect{ 0, 0, inputFormat->size };

and

	Rectangle rect(0, 0, inputFormat->size);

One is that the former will not narrow arguments through implicit cast.
See https://en.cppreference.com/w/cpp/language/list_initialization:

"[...] all constructors of T participate in overload resolution against
the set of arguments that consists of the elements of the
braced-init-list, with the restriction that only non-narrowing
conversions are allowed."

It's commonly considered as a good practice as the default
initialization mechanism. There are however drawbacks,
https://stackoverflow.com/questions/18222926/why-is-list-initialization-using-curly-braces-better-than-the-alternatives
explains some of them.

> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> >  	ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect);
> >  	if (ret)
> >  		return ret;
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 009e502b288c..2d848ac3c735 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -752,12 +752,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >  	}
> >  
> >  	/* Adjust aspect ratio by providing crops on the input image. */
> > -	Rectangle crop = {
> > -		.x = 0,
> > -		.y = 0,
> > -		.width = sensorFormat.size.width,
> > -		.height = sensorFormat.size.height
> > -	};
> > +	Rectangle crop{ 0, 0, sensorFormat.size };
> >  
> >  	int ar = maxSize.height * sensorFormat.size.width - maxSize.width * sensorFormat.size.height;
> >  	if (ar > 0)
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index a6f457c71fb1..4f461b928514 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -256,12 +256,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> >  		return ret;
> >  
> >  	if (data->media_->version() >= KERNEL_VERSION(5, 6, 0)) {
> > -		Rectangle crop = {
> > -			.x = 0,
> > -			.y = 0,
> > -			.width = subformat.size.width,
> > -			.height = subformat.size.height,
> > -		};
> > +		Rectangle crop{ 0, 0, subformat.size };
> >  		ret = data->scaler_->setSelection(0, V4L2_SEL_TGT_CROP, &crop);
> >  		if (ret)
> >  			return ret;

Patch
diff mbox series 

diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
index f3088d33fa2c..380248ac9a50 100644
--- a/include/libcamera/geometry.h
+++ b/include/libcamera/geometry.h
@@ -127,6 +127,21 @@  static inline bool operator!=(const SizeRange &lhs, const SizeRange &rhs)
 }
 
 struct Rectangle {
+	Rectangle()
+		: Rectangle(0, 0, 0, 0)
+	{
+	}
+
+	Rectangle(int xpos, int ypos, const Size &size)
+		: x(xpos), y(ypos), width(size.width), height(size.height)
+	{
+	}
+
+	Rectangle(int xpos, int ypos, unsigned int w, unsigned int h)
+		: x(xpos), y(ypos), width(w), height(h)
+	{
+	}
+
 	int x;
 	int y;
 	unsigned int width;
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index b14b4051dca6..6e93cc51155b 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -487,7 +487,7 @@  int CameraSensor::sensorInfo(CameraSensorInfo *info) const
 	info->model = model();
 
 	/* Get the active area size. */
-	Rectangle rect = {};
+	Rectangle rect;
 	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
 	if (ret) {
 		LOG(CameraSensor, Error)
diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
index 5a0bcf7c1e12..3a3784df39e6 100644
--- a/src/libcamera/geometry.cpp
+++ b/src/libcamera/geometry.cpp
@@ -297,6 +297,28 @@  bool operator==(const SizeRange &lhs, const SizeRange &rhs)
  * refers to, are defined by the context were rectangle is used.
  */
 
+/**
+ * \fn Rectangle::Rectangle()
+ * \brief Construct a Rectangle with all coordinates set to 0
+ */
+
+/**
+ * \fn Rectangle::Rectangle(int x, int y, const Size &size)
+ * \brief Construct a Rectangle with the given position and size
+ * \param[in] x The horizontal coordinate of the top-left corner
+ * \param[in] y The vertical coordinate of the top-left corner
+ * \param[in] size The size
+ */
+
+/**
+ * \fn Rectangle::Rectangle(int x, int y, unsigned int width, unsigned int height)
+ * \brief Construct a Rectangle with the given position and size
+ * \param[in] x The horizontal coordinate of the top-left corner
+ * \param[in] y The vertical coordinate of the top-left corner
+ * \param[in] width The width
+ * \param[in] height The height
+ */
+
 /**
  * \var Rectangle::x
  * \brief The horizontal coordinate of the rectangle's top-left corner
diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
index d7f4173d3607..4bec4b2f1a76 100644
--- a/src/libcamera/pipeline/ipu3/imgu.cpp
+++ b/src/libcamera/pipeline/ipu3/imgu.cpp
@@ -100,12 +100,7 @@  int ImgUDevice::configureInput(const Size &size,
 	 * to configure the crop/compose rectangles, contradicting the
 	 * V4L2 specification.
 	 */
-	Rectangle rect = {
-		.x = 0,
-		.y = 0,
-		.width = inputFormat->size.width,
-		.height = inputFormat->size.height,
-	};
+	Rectangle rect{ 0, 0, inputFormat->size };
 	ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect);
 	if (ret)
 		return ret;
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 009e502b288c..2d848ac3c735 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -752,12 +752,7 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 	}
 
 	/* Adjust aspect ratio by providing crops on the input image. */
-	Rectangle crop = {
-		.x = 0,
-		.y = 0,
-		.width = sensorFormat.size.width,
-		.height = sensorFormat.size.height
-	};
+	Rectangle crop{ 0, 0, sensorFormat.size };
 
 	int ar = maxSize.height * sensorFormat.size.width - maxSize.width * sensorFormat.size.height;
 	if (ar > 0)
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index a6f457c71fb1..4f461b928514 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -256,12 +256,7 @@  int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
 		return ret;
 
 	if (data->media_->version() >= KERNEL_VERSION(5, 6, 0)) {
-		Rectangle crop = {
-			.x = 0,
-			.y = 0,
-			.width = subformat.size.width,
-			.height = subformat.size.height,
-		};
+		Rectangle crop{ 0, 0, subformat.size };
 		ret = data->scaler_->setSelection(0, V4L2_SEL_TGT_CROP, &crop);
 		if (ret)
 			return ret;