[libcamera-devel,v3,02/16] libcamera: geometry: SizeRange: Extend with step information
diff mbox series

Message ID 20190616133402.21934-3-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: Add support for format information and validation
Related show

Commit Message

Niklas Söderlund June 16, 2019, 1:33 p.m. UTC 
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@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/geometry.h | 13 +++++++++--
 src/libcamera/geometry.cpp   | 44 +++++++++++++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 5 deletions(-)

Comments

Laurent Pinchart June 18, 2019, 10:09 p.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

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@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@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."

>   */
>  
>  /**
> @@ -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@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
Laurent Pinchart June 18, 2019, 11:37 p.m. UTC | #2 
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@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo@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@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

Patch
diff mbox series 

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
+ * 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.
+ *
+ *	width = min.width + hStep * x
+ *	height = min.height + vStep * y
+ *
+ *	Where:
+ *
+ *	width <= max.width
+ *	height < max.height
+ *
+ * For SizeRange instances describing a single size the incremental step values
+ * are fixed 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