[libcamera-devel] [PATCH v6 02/12] libcamera: geometry: Add Size structure

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Apr 02, 2019 at 06:21:24PM +0200, Jacopo Mondi wrote:
> Add a simple Size structure that contains an image width and height.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/geometry.cpp       | 29 +++++++++++++++++++++++++++++
>  src/libcamera/include/geometry.h | 15 +++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index f76001d94562..30531d0c678a 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -105,4 +105,33 @@ const std::string Rectangle::toString() const
>   * \brief The maximum image height
>   */
>  
> +/**
> + * \struct Size
> + * \brief Describe an image size
> + *
> + * Size describes the vertical and horizontal sizes of an image.

How about "The Size structure defines a two-dimensional size with integer precision" ?

> + */
> +
> +/**
> + * \fn Size::Size()
> + * \brief Construct a size initialized to 0

Construct a Size with width and height set to 0

> + */
> +
> +/**
> + * \fn Size::Size(unsigned int w, unsigned int h)

I'd name the arguments width and height, at least in the documentation.

> + * \brief Construct an initialized size

Construct a Size with the given \a width and \a height

> + * \param w The image width
> + * \param h The image height

This could represent the size of something else than an image, I
wouldn't mention image. Same for the two doxygen comments below.

> + */
> +
> +/**
> + * \var Size::width
> + * \brief The image width
> + */
> +
> +/**
> + * \var Size::height
> + * \brief The image height
> + */
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/geometry.h b/src/libcamera/include/geometry.h
> index 567a54299b24..24be5e2f6f78 100644
> --- a/src/libcamera/include/geometry.h
> +++ b/src/libcamera/include/geometry.h
> @@ -40,6 +40,21 @@ struct SizeRange {
>  	unsigned int maxHeight;
>  };
>  
> +struct Size {
> +	Size(void)

No need for void.

> +		: Size(0, 0)
> +	{
> +	}
> +
> +	Size(unsigned int w, unsigned int h)
> +		: width(w), height(h)
> +	{
> +	}
> +
> +	unsigned int width;
> +	unsigned int height;

Do we want to support negative dimensions that would denote an invalid
or uninitialized size ? The default constructor could set width and
height to -1, and we could provide an isValid() function to test for
that. Maybe something for later, or is it simple enough that we should
do it now ?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +};
> +
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_GEOMETRY_H__ */

-- 
Regards,

Laurent Pinchart