[libcamera-devel] [PATCH v2 02/16] libcamera: geometry: SizeRange: Extend with step information

[Jacopo Mondi]

Hi Niklas,
   two minor nits


On Wed, Jun 12, 2019 at 02:43:45AM +0200, Niklas Söderlund wrote:
> The size range described might be subject to certain step
> limitations. Make it possible to record this information.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/geometry.h | 13 +++++++++--
>  src/libcamera/geometry.cpp   | 44 +++++++++++++++++++++++++++++++++---
>  2 files changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> index ec5ed2bee196c82d..b2a8e5b0e005e4db 100644
> --- a/include/libcamera/geometry.h
> +++ b/include/libcamera/geometry.h
> @@ -73,18 +73,27 @@ struct SizeRange {
>  	}
>
>  	SizeRange(unsigned int width, unsigned int height)
> -		: min(width, height), max(width, height)
> +		: min(width, height), max(width, height), hStep(1), vStep(1)
>  	{
>  	}
>
>  	SizeRange(unsigned int minW, unsigned int minH,
>  		  unsigned int maxW, unsigned int maxH)
> -		: min(minW, minH), max(maxW, maxH)
> +		: min(minW, minH), max(maxW, maxH), hStep(1), vStep(1)
> +	{
> +	}
> +
> +	SizeRange(unsigned int minW, unsigned int minH,
> +		  unsigned int maxW, unsigned int maxH,
> +		  unsigned int hstep, unsigned int vstep)
> +		: min(minW, minH), max(maxW, maxH), hStep(hstep), vStep(vstep)
>  	{
>  	}
>
>  	Size min;
>  	Size max;
> +	unsigned int hStep;
> +	unsigned int vStep;
>  };
>
>  bool operator==(const SizeRange &lhs, const SizeRange &rhs);
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index 26a05b9e7d800077..c56c4e399ef307f5 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -186,9 +186,24 @@ bool operator<(const Size &lhs, const Size &rhs)
>   * \struct SizeRange
>   * \brief Describe a range of sizes
>   *
> - * SizeRange describes a range of sizes included in the [min, max]
> - * interval for both the width and the height. If the minimum and
> - * maximum sizes are identical it represents a single size.
> + * SizeRange describes a range of sizes included in the [min, max] interval
> + * for both the width and the height. If the minimum and maximum sizes are
> + * identical it represents a single size.
> + *
> + * The SizeRange may also describe a vertical and horizontal step. The step
> + * describes the value in pixels the width/height size can be incremented with,

s/size can be incremented with/size gets incremented by

"can" sounds like a conditional statement to me. If that's intended,
it's fine.

> + * starting from the minimum size.
> + *
> + *	width = min.width + hStep * x
> + *	height = min.height + vStep * y
> + *
> + *	Where:
> + *
> + *	width <= max.width
> + *	height < max.height
> + *
> + * For SizeRanges describing a single size the incremental step values are fixed

I think we don't make plurals with class names, so I would rather
s/SizeRanges/SizeRange instances/ or start the phrase with "When a
SizeRange desribes ... "

Nits apart
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

> + * to 1.
>   */
>
>  /**
> @@ -213,6 +228,19 @@ bool operator<(const Size &lhs, const Size &rhs)
>   * \param[in] maxH The maximum height
>   */
>
> +/**
> + * \fn SizeRange::SizeRange(unsigned int minW, unsigned int minH,
> + *			    unsigned int maxW, unsigned int maxH,
> + *			    unsigned int hstep, unsigned int vstep)
> + * \brief Construct an initialized size range
> + * \param[in] minW The minimum width
> + * \param[in] minH The minimum height
> + * \param[in] maxW The maximum width
> + * \param[in] maxH The maximum height
> + * \param[in] hstep The horizontal step
> + * \param[in] vstep The vertical step
> + */
> +
>  /**
>   * \var SizeRange::min
>   * \brief The minimum size
> @@ -223,6 +251,16 @@ bool operator<(const Size &lhs, const Size &rhs)
>   * \brief The maximum size
>   */
>
> +/**
> + * \var SizeRange::hStep
> + * \brief The horizontal step
> + */
> +
> +/**
> + * \var SizeRange::vStep
> + * \brief The vertical step
> + */
> +
>  /**
>   * \brief Compare size ranges for equality
>   * \return True if the two size ranges are equal, false otherwise
> --
> 2.21.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190613/b7fab43b/attachment-0001.sig>