[libcamera-devel] [PATCH 04/17] libcamera: geometry: SizeRange: Add toString()

[Laurent Pinchart]

Hi Niklas,
 
On Thu, May 30, 2019 at 12:00:13AM +0100, Kieran Bingham wrote:
> On 27/05/2019 01:15, Niklas Söderlund wrote:
> > It's useful to be able to print a string representation of a SizeRange
> > to the log or console, add a toString() method. While at it turn the
> > structure into a class as it contains functions as well as data.

Note that a class can also contain methods. The only difference is that
members of a struct are public by default, while members of a class are
private. This being said, I'm not opposed to the change.

> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  include/libcamera/geometry.h |  6 +++++-
> >  src/libcamera/geometry.cpp   | 22 ++++++++++++++++++++++
> >  2 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > index 1c9267b14274cb5d..ca920a1e16817860 100644
> > --- a/include/libcamera/geometry.h
> > +++ b/include/libcamera/geometry.h
> > @@ -67,7 +67,9 @@ static inline bool operator>=(const Size &lhs, const Size &rhs)
> >  	return !(lhs < rhs);
> >  }
> >  
> > -struct SizeRange {
> > +class SizeRange
> > +{
> > +public:
> >  	SizeRange()
> >  	{
> >  	}
> > @@ -90,6 +92,8 @@ struct SizeRange {
> >  	{
> >  	}
> >  
> > +	const std::string toString() const;

No need for the first const, you're returning a value.

> > +
> >  	Size min;
> >  	Size max;
> >  	unsigned int hStep;
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index e7f1bafd40e944f5..337f2efa828ba996 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -259,6 +259,28 @@ bool operator<(const Size &lhs, const Size &rhs)
> >   * \brief The vertical step
> >   */
> >  
> > +/**
> > + * \brief Assemble and return a string describing the size range
> > + * \return A string describing the SizeRange
> > + */
> > +const std::string SizeRange::toString() const
> > +{
> > +	std::stringstream ss;
> 
> Seeing the output of this in the cam --info option:
> 
>  * Pixelformat: 0x47504a4d Width: 176-1920 Height: 144-1080 hStep: 1
> vStep: 1
> 
> Seems a bit convoluted to read.

I agree.

> > +
> > +	if (min == max)
> > +		ss << "Width: " << min.width
> > +		   << " Height: " << min.height
> 
> Can't we just do the following ?:
> 		ss << min.toString
> 
> 
> > +		   << " hStep: " << hStep
> > +		   << " vStep: " << vStep;> +	else
> > +		ss << "Width: " << min.width << "-" << max.width
> > +		   << " Height: " << min.height << "-" << max.height
> > +		   << " hStep: " << hStep
> > +		   << " vStep: " << vStep;
> 
> 
> 
> How about:
> 
> const std::string SizeRange::toString() const
> {
> 	std::stringstream ss;
> 
> 	ss << min.toString;
> 
> 	if (min != max)
> 		ss << " -> " << max.toString();
> 
> 	ss   << " hStep: " << hStep  << " vStep: " << vStep;
> 
> 	return ss.str();
> }

That's better but still a bit long. How about

	ss << "(" << min.toString() << ")-(" << max.toString() << ")/(+"
	   << hStep << ",+" << vStep << ")";

which should print

	(640x480)-(1024x768)/(+4,+4)

Feel free to propose a different representation, but I think we should
keep it compact and avoid spaces.

> > +
> > +	return ss.str();
> > +}
> > +
> >  /**
> >   * \brief Compare size ranges for equality
> >   * \return True if the two size ranges are equal, false otherwise

-- 
Regards,

Laurent Pinchart