| Message ID | 20190527001543.13593-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Mon, May 27, 2019 at 02:15:30AM +0200, 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. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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; > + do we want the below fields private, now that SizeRange is a class? Otherwise: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > 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; > + > + if (min == max) > + ss << "Width: " << min.width > + << " Height: " << min.height > + << " hStep: " << hStep > + << " vStep: " << vStep; > + else > + ss << "Width: " << min.width << "-" << max.width > + << " Height: " << min.height << "-" << max.height > + << " hStep: " << hStep > + << " vStep: " << vStep; > + > + return ss.str(); > +} > + > /** > * \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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, 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. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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; > + > 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. > + > + 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(); } > + > + return ss.str(); > +} > + > /** > * \brief Compare size ranges for equality > * \return True if the two size ranges are equal, false otherwise >
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@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
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; + 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; + + if (min == max) + ss << "Width: " << min.width + << " Height: " << min.height + << " hStep: " << hStep + << " vStep: " << vStep; + else + ss << "Width: " << min.width << "-" << max.width + << " Height: " << min.height << "-" << max.height + << " hStep: " << hStep + << " vStep: " << vStep; + + return ss.str(); +} + /** * \brief Compare size ranges for equality * \return True if the two size ranges are equal, false otherwise
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. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/geometry.h | 6 +++++- src/libcamera/geometry.cpp | 22 ++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-)