[libcamera-devel] [PATCH 3/5] libcamera: geometry: Give constructors to Rectangle

[Niklas Söderlund]

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 at 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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund