[libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Apr 03, 2020 at 12:43:27PM +0200, Jacopo Mondi wrote:
> The libcamera Rectangle struct has no constructor that allows to
> construct statically initialized instances. Furthermore, its class
> members that represents the rectangle horizontal and vertical sizes are
> named 'w' and 'h' compared to the Size and SizeRange classes which uses
> the 'width' and 'height', which every time results in having to look at
> class definition to know which name to use.

Good point regarding w and h, it makes sense to standardize them. I'm
sure Niklas would ask for two separate patches though :-)

> Add a constructor that takes the rectangle 4 sizes and force generation
> of a default constructor, and while at there rationalize class members
> names to 'width' and 'height'.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/geometry.h         | 10 ++++++++--
>  src/libcamera/geometry.cpp           | 22 ++++++++++++++++++----
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  7 +------
>  src/libcamera/v4l2_subdevice.cpp     |  8 ++++----
>  src/libcamera/v4l2_videodevice.cpp   |  8 ++++----
>  5 files changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> index 7f1b29fe8c19..5eb4e72e365c 100644
> --- a/include/libcamera/geometry.h
> +++ b/include/libcamera/geometry.h
> @@ -13,10 +13,16 @@
>  namespace libcamera {
>  
>  struct Rectangle {
> +	Rectangle() = default;
> +	Rectangle(int x_, int y_, unsigned int width_, unsigned int height_)
> +		: x(x_), y(y_), width(width_), height(height_)
> +	{
> +	}
> +
>  	int x;
>  	int y;
> -	unsigned int w;
> -	unsigned int h;
> +	unsigned int width;
> +	unsigned int height;
>  
>  	const std::string toString() const;
>  };
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index 13f642be526f..97dbdcf301b9 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -29,6 +29,20 @@ namespace libcamera {
>   * refers to, are defined by the context were rectangle is used.
>   */
>  
> +/**
> + * \fn Rectangle::Rectangle
> + * \brief Construct a Rectangle with all sizes initialized to 0
> + */
> +
> +/**
> + * \fn Rectangle::Rectangle(int x_, int y_, unsigned int width_, unsigned int height_)
> + * \brief Construct a Rectangle with its sizes initialized
> + * \param[in] x_: The Rectangle top-left corner horizontal displacement
> + * \param[in] y_: The Rectangle top-left corner vertical displacement
> + * \param[in] width_: The Rectangle horizontal size
> + * \param[in] height_: The Rectangle vertical size
> + */
> +
>  /**
>   * \var Rectangle::x
>   * \brief The horizontal coordinate of the rectangle's top-left corner
> @@ -40,12 +54,12 @@ namespace libcamera {
>   */
>  
>  /**
> - * \var Rectangle::w
> + * \var Rectangle::width
>   * \brief The distance between the left and right sides
>   */
>  
>  /**
> - * \var Rectangle::h
> + * \var Rectangle::height
>   * \brief The distance between the top and bottom sides
>   */
>  
> @@ -57,7 +71,7 @@ const std::string Rectangle::toString() const
>  {
>  	std::stringstream ss;
>  
> -	ss << "(" << x << "x" << y << ")/" << w << "x" << h;
> +	ss << "(" << x << "x" << y << ")/" << width << "x" << height;
>  
>  	return ss.str();
>  }
> @@ -69,7 +83,7 @@ const std::string Rectangle::toString() const
>  bool operator==(const Rectangle &lhs, const Rectangle &rhs)
>  {
>  	return lhs.x == rhs.x && lhs.y == rhs.y &&
> -	       lhs.w == rhs.w && lhs.h == rhs.h;
> +	       lhs.width == rhs.width && lhs.height == rhs.height;
>  }
>  
>  /**
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 1e114ca7ed10..7e15ad28bef2 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1129,12 +1129,7 @@ int ImgUDevice::configureInput(const Size &size,
>  	 * to configure the crop/compose rectangles, contradicting the
>  	 * V4L2 specification.
>  	 */
> -	Rectangle rect = {
> -		.x = 0,
> -		.y = 0,
> -		.w = inputFormat->size.width,
> -		.h = inputFormat->size.height,
> -	};
> +	Rectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);

Do we really need a constructor for this ? We can already do

	Rectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};

and we can also do

	Rectangle rect{};

to initialize everything to 0. What does an explicit constructor bring
us, are there benefits I would be overlooking at this time of the night
?

>  	ret = imgu_->setCrop(PAD_INPUT, &rect);
>  	if (ret)
>  		return ret;
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 8b9da81e8ab3..50fd26d99d9c 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -339,8 +339,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
>  
>  	sel.r.left = rect->x;
>  	sel.r.top = rect->y;
> -	sel.r.width = rect->w;
> -	sel.r.height = rect->h;
> +	sel.r.width = rect->width;
> +	sel.r.height = rect->height;
>  
>  	int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
>  	if (ret < 0) {
> @@ -352,8 +352,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
>  
>  	rect->x = sel.r.left;
>  	rect->y = sel.r.top;
> -	rect->w = sel.r.width;
> -	rect->h = sel.r.height;
> +	rect->width = sel.r.width;
> +	rect->height = sel.r.height;
>  
>  	return 0;
>  }
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index eb33a68e50d6..6e8f230f593d 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1115,8 +1115,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)
>  
>  	sel.r.left = rect->x;
>  	sel.r.top = rect->y;
> -	sel.r.width = rect->w;
> -	sel.r.height = rect->h;
> +	sel.r.width = rect->width;
> +	sel.r.height = rect->height;
>  
>  	int ret = ioctl(VIDIOC_S_SELECTION, &sel);
>  	if (ret < 0) {
> @@ -1127,8 +1127,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)
>  
>  	rect->x = sel.r.left;
>  	rect->y = sel.r.top;
> -	rect->w = sel.r.width;
> -	rect->h = sel.r.height;
> +	rect->width = sel.r.width;
> +	rect->height = sel.r.height;
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart