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

[Laurent Pinchart]

Hi Niklas,

A few more comments after reviewing patch 10/16.

On Wed, Jun 19, 2019 at 01:09:31AM +0300, Laurent Pinchart wrote:
> On Sun, Jun 16, 2019 at 03:33:48PM +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>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  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..dfc264b2700f7f61 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
> 
> While at it, I would write
> 
> "A SizeRange describes", "A SizeRange instance describes", "SizeRange
> instances describe" or "The SizeRange class describes".
> 
> > + * 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 gets incremented by,
> > + * starting from the minimum size.
> 
> How about
> 
> "Size ranges may further limit the valid sizes through steps in the
> horizontal and vertical direction. The step values represent the
> increase in pixels between two valid width or height values, starting
> from the minimum. Valid sizes within the range are thus expressed as"
> 
> > + *
> > + *	width = min.width + hStep * x
> > + *	height = min.height + vStep * y
> > + *
> > + *	Where:
> 
> s/Where:/where/
> 
> > + *
> > + *	width <= max.width
> > + *	height < max.height
> > + *
> > + * For SizeRange instances describing a single size the incremental step values
> > + * are fixed to 1.
> 
> "Note that the step values are not equivalent to alignments, as the
> minimum width or height may not be a multiple of the corresponding
> step.
> 
> The step values are guaranteed to be larger than zero. In particular,
> SizeRange instances that describe a single size have both step values
> set to 1."

The step values may be zero when the range describes only minimum and
maximum sizes without implying that all, or any, intermediate size is
valid. SizeRange instances the describe a single size have both set
values set 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
> 
> Kieran would say s/initialized/initialised/ ;-)
> 
> With these issues addressed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > + * \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

-- 
Regards,

Laurent Pinchart