| Message ID | 20210521080530.37591-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the updated patch! On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com> wrote: > > A new utils::Duration class is defined to represent a > std::chrono::duration type with double precision nanosecond timebase. > Using a double minimises the loss of precision when converting timebases. > This helper class may be used by IPAs to represent variables such as frame > durations and exposure times. > > An operator << overload is define to help with displaying > utils::Duration value in stream objects. Currently, this will display > the duration value in microseconds. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++ > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++ > 2 files changed, 78 insertions(+) > > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h > index 83dada7cc16c..a377d4ea07ab 100644 > --- a/include/libcamera/internal/utils.h > +++ b/include/libcamera/internal/utils.h > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *> > } > #endif > > +using BaseDuration = std::chrono::duration<double, std::nano>; > + > +class Duration : public BaseDuration > +{ > +public: > + Duration() = default; > + > + template<typename Rep, typename Period> > + constexpr Duration(const std::chrono::duration<Rep, Period> &d) > + : BaseDuration(d) > + { > + } > + > + template<typename Period> > + double get() const > + { > + auto const c = std::chrono::duration_cast<std::chrono::duration<double, Period>>(*this); > + return c.count(); > + } > + > + explicit constexpr operator bool() const > + { > + return *this != BaseDuration::zero(); > + } > +}; > + > +#ifndef __DOXYGEN__ > +template<class CharT, class Traits> > +static inline std::basic_ostream<CharT, Traits> &operator<<(std::basic_ostream<CharT, Traits> &os, > + const Duration &d) > +{ > + std::basic_ostringstream<CharT, Traits> s; > + > + s.flags(os.flags()); > + s.imbue(os.getloc()); > + s.setf(std::ios_base::fixed, std::ios_base::floatfield); > + s.precision(2); > + s << d.get<std::micro>() << "us"; > + return os << s.str(); > +} > +#endif One small thing I wonder about is that many classes have a toString method. Do we want one here? It could even take an optional argument to allow you to format the output differently... but I think that's overkill and best left for now! > + > } /* namespace utils */ > > } /* namespace libcamera */ > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > index 826144d3c837..3131aa2d6666 100644 > --- a/src/libcamera/utils.cpp > +++ b/src/libcamera/utils.cpp > @@ -506,6 +506,42 @@ std::string libcameraSourcePath() > * loop, iterates over an indexed view of the \a iterable > */ > > +/** > + * \class Duration > + * \brief Helper for std::chrono::duration types. Derived from \a BaseDuration > + */ > + > +/** > + * \fn Duration::Duration(const std::chrono::duration<Rep, Period> &d) > + * \brief Copy constructor from a \a std::chrono::duration type. > + * \param[in] d The std::chrono::duration object to convert from. > + * > + * Constructs a \a Duration object from a \a std::chrono::duration object, > + * converting the representation to \a BaseDuration type. > + */ > + > +/** > + * \fn Duration::get<Period>() > + * \brief Retrieve the tick count, coverted to the timebase provided by the s/coverted/converted/ With this tiny fix: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > + * template argument Period of type \a std::ratio. > + * > + * A typical usage example is given below: > + * > + * \code{.cpp} > + * utils::Duration d = 5s; > + * double d_in_ms = d.get<std::milli>(); > + * \endcode > + * > + * \return Tick count > + */ > + > +/** > + * \fn Duration::operator bool() > + * \brief Boolean operator to test if a \a Duration holds a non-zero time value. > + * > + * \return True if \a Duration is a non-zero time value, False otherwise. > + */ > + > } /* namespace utils */ > > } /* namespace libcamera */ > -- > 2.25.1 >
Hi David, Thank you for your feedback. On Fri, 21 May 2021 at 11:20, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for the updated patch! > > On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > A new utils::Duration class is defined to represent a > > std::chrono::duration type with double precision nanosecond timebase. > > Using a double minimises the loss of precision when converting timebases. > > This helper class may be used by IPAs to represent variables such as > frame > > durations and exposure times. > > > > An operator << overload is define to help with displaying > > utils::Duration value in stream objects. Currently, this will display > > the duration value in microseconds. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++ > > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++ > > 2 files changed, 78 insertions(+) > > > > diff --git a/include/libcamera/internal/utils.h > b/include/libcamera/internal/utils.h > > index 83dada7cc16c..a377d4ea07ab 100644 > > --- a/include/libcamera/internal/utils.h > > +++ b/include/libcamera/internal/utils.h > > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> > details::enumerate_adapter<T *> > > } > > #endif > > > > +using BaseDuration = std::chrono::duration<double, std::nano>; > > + > > +class Duration : public BaseDuration > > +{ > > +public: > > + Duration() = default; > > + > > + template<typename Rep, typename Period> > > + constexpr Duration(const std::chrono::duration<Rep, Period> &d) > > + : BaseDuration(d) > > + { > > + } > > + > > + template<typename Period> > > + double get() const > > + { > > + auto const c = > std::chrono::duration_cast<std::chrono::duration<double, Period>>(*this); > > + return c.count(); > > + } > > + > > + explicit constexpr operator bool() const > > + { > > + return *this != BaseDuration::zero(); > > + } > > +}; > > + > > +#ifndef __DOXYGEN__ > > +template<class CharT, class Traits> > > +static inline std::basic_ostream<CharT, Traits> > &operator<<(std::basic_ostream<CharT, Traits> &os, > > + const > Duration &d) > > +{ > > + std::basic_ostringstream<CharT, Traits> s; > > + > > + s.flags(os.flags()); > > + s.imbue(os.getloc()); > > + s.setf(std::ios_base::fixed, std::ios_base::floatfield); > > + s.precision(2); > > + s << d.get<std::micro>() << "us"; > > + return os << s.str(); > > +} > > +#endif > > One small thing I wonder about is that many classes have a toString > method. Do we want one here? It could even take an optional argument > to allow you to format the output differently... but I think that's > overkill and best left for now! > Did you mean that we should do something like: LOG(IPA, Debug) << "Exposure time is " << exposure.toString(); over LOG(IPA, Debug) << "Exposure time is " << exposure; I chose the later way with the overload <<() operator as that is more similar to what C++20 will eventually allow with std::chrono::duration formatting. What do others think? Regards, Naush > > > + > > } /* namespace utils */ > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > > index 826144d3c837..3131aa2d6666 100644 > > --- a/src/libcamera/utils.cpp > > +++ b/src/libcamera/utils.cpp > > @@ -506,6 +506,42 @@ std::string libcameraSourcePath() > > * loop, iterates over an indexed view of the \a iterable > > */ > > > > +/** > > + * \class Duration > > + * \brief Helper for std::chrono::duration types. Derived from \a > BaseDuration > > + */ > > + > > +/** > > + * \fn Duration::Duration(const std::chrono::duration<Rep, Period> &d) > > + * \brief Copy constructor from a \a std::chrono::duration type. > > + * \param[in] d The std::chrono::duration object to convert from. > > + * > > + * Constructs a \a Duration object from a \a std::chrono::duration > object, > > + * converting the representation to \a BaseDuration type. > > + */ > > + > > +/** > > + * \fn Duration::get<Period>() > > + * \brief Retrieve the tick count, coverted to the timebase provided by > the > > s/coverted/converted/ > > With this tiny fix: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks! > David > > > + * template argument Period of type \a std::ratio. > > + * > > + * A typical usage example is given below: > > + * > > + * \code{.cpp} > > + * utils::Duration d = 5s; > > + * double d_in_ms = d.get<std::milli>(); > > + * \endcode > > + * > > + * \return Tick count > > + */ > > + > > +/** > > + * \fn Duration::operator bool() > > + * \brief Boolean operator to test if a \a Duration holds a non-zero > time value. > > + * > > + * \return True if \a Duration is a non-zero time value, False > otherwise. > > + */ > > + > > } /* namespace utils */ > > > > } /* namespace libcamera */ > > -- > > 2.25.1 > > >
Hi Naush, David, On 21/05/2021 11:53, Naushir Patuck wrote: > Hi David, > > Thank you for your feedback. > > On Fri, 21 May 2021 at 11:20, David Plowman > <david.plowman@raspberrypi.com <mailto:david.plowman@raspberrypi.com>> > wrote: > > Hi Naush > > Thanks for the updated patch! > > On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com > <mailto:naush@raspberrypi.com>> wrote: > > > > A new utils::Duration class is defined to represent a > > std::chrono::duration type with double precision nanosecond timebase. > > Using a double minimises the loss of precision when converting > timebases. > > This helper class may be used by IPAs to represent variables such > as frame > > durations and exposure times. > > > > An operator << overload is define to help with displaying > > utils::Duration value in stream objects. Currently, this will display > > the duration value in microseconds. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com > <mailto:naush@raspberrypi.com>> > > --- > > include/libcamera/internal/utils.h | 42 > ++++++++++++++++++++++++++++++ > > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++ > > 2 files changed, 78 insertions(+) > > > > diff --git a/include/libcamera/internal/utils.h > b/include/libcamera/internal/utils.h > > index 83dada7cc16c..a377d4ea07ab 100644 > > --- a/include/libcamera/internal/utils.h > > +++ b/include/libcamera/internal/utils.h > > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> > details::enumerate_adapter<T *> > > } > > #endif > > > > +using BaseDuration = std::chrono::duration<double, std::nano>; > > + > > +class Duration : public BaseDuration > > +{ > > +public: > > + Duration() = default; > > + > > + template<typename Rep, typename Period> > > + constexpr Duration(const std::chrono::duration<Rep, > Period> &d) > > + : BaseDuration(d) > > + { > > + } > > + > > + template<typename Period> > > + double get() const > > + { > > + auto const c = > std::chrono::duration_cast<std::chrono::duration<double, > Period>>(*this); > > + return c.count(); > > + } > > + > > + explicit constexpr operator bool() const > > + { > > + return *this != BaseDuration::zero(); > > + } > > +}; > > + > > +#ifndef __DOXYGEN__ > > +template<class CharT, class Traits> > > +static inline std::basic_ostream<CharT, Traits> > &operator<<(std::basic_ostream<CharT, Traits> &os, > > + const > Duration &d) > > +{ > > + std::basic_ostringstream<CharT, Traits> s; > > + > > + s.flags(os.flags()); > > + s.imbue(os.getloc()); > > + s.setf(std::ios_base::fixed, std::ios_base::floatfield); > > + s.precision(2); > > + s << d.get<std::micro>() << "us"; > > + return os << s.str(); > > +} > > +#endif > > One small thing I wonder about is that many classes have a toString > method. Do we want one here? It could even take an optional argument > to allow you to format the output differently... but I think that's > overkill and best left for now! > > > Did you mean that we should do something like: > > LOG(IPA, Debug) << "Exposure time is " << exposure.toString(); > > over > > LOG(IPA, Debug) << "Exposure time is " << exposure; > > I chose the later way with the overload <<() operator as that is more > similar > to what C++20 will eventually allow with std::chrono::duration formatting. > What do others think? I think the main reason we've used toString() is so that we don't have to override operator<< outside of the libcamera namespace, so we've been more explicit with the toString(). I don't mind the override or the explicit, but I think that's the reasoning. Would it be cleaner to implement a toString() which doesn't have to track the outputstream state, using a local stringstream perhaps and as that returns a string, the override operator<< can simply return that pre-prepared string? > > Regards, > Naush > > > > > > + > > } /* namespace utils */ > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > > index 826144d3c837..3131aa2d6666 100644 > > --- a/src/libcamera/utils.cpp > > +++ b/src/libcamera/utils.cpp > > @@ -506,6 +506,42 @@ std::string libcameraSourcePath() > > * loop, iterates over an indexed view of the \a iterable > > */ > > > > +/** > > + * \class Duration > > + * \brief Helper for std::chrono::duration types. Derived from \a > BaseDuration > > + */ > > + > > +/** > > + * \fn Duration::Duration(const std::chrono::duration<Rep, > Period> &d) > > + * \brief Copy constructor from a \a std::chrono::duration type. > > + * \param[in] d The std::chrono::duration object to convert from. > > + * > > + * Constructs a \a Duration object from a \a > std::chrono::duration object, > > + * converting the representation to \a BaseDuration type. > > + */ > > + > > +/** > > + * \fn Duration::get<Period>() > > + * \brief Retrieve the tick count, coverted to the timebase > provided by the > > s/coverted/converted/ > > With this tiny fix: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com > <mailto:david.plowman@raspberrypi.com>> > > Thanks! > David > > > + * template argument Period of type \a std::ratio. > > + * > > + * A typical usage example is given below: > > + * > > + * \code{.cpp} > > + * utils::Duration d = 5s; > > + * double d_in_ms = d.get<std::milli>(); > > + * \endcode > > + * > > + * \return Tick count > > + */ > > + > > +/** > > + * \fn Duration::operator bool() > > + * \brief Boolean operator to test if a \a Duration holds a > non-zero time value. > > + * > > + * \return True if \a Duration is a non-zero time value, False > otherwise. > > + */ > > + > > } /* namespace utils */ > > > > } /* namespace libcamera */ > > -- > > 2.25.1 > > >
Hi Kieran, On Fri, 21 May 2021 at 12:14, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Naush, David, > > On 21/05/2021 11:53, Naushir Patuck wrote: > > Hi David, > > > > Thank you for your feedback. > > > > On Fri, 21 May 2021 at 11:20, David Plowman > > <david.plowman@raspberrypi.com <mailto:david.plowman@raspberrypi.com>> > > wrote: > > > > Hi Naush > > > > Thanks for the updated patch! > > > > On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com > > <mailto:naush@raspberrypi.com>> wrote: > > > > > > A new utils::Duration class is defined to represent a > > > std::chrono::duration type with double precision nanosecond > timebase. > > > Using a double minimises the loss of precision when converting > > timebases. > > > This helper class may be used by IPAs to represent variables such > > as frame > > > durations and exposure times. > > > > > > An operator << overload is define to help with displaying > > > utils::Duration value in stream objects. Currently, this will > display > > > the duration value in microseconds. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com > > <mailto:naush@raspberrypi.com>> > > > --- > > > include/libcamera/internal/utils.h | 42 > > ++++++++++++++++++++++++++++++ > > > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++ > > > 2 files changed, 78 insertions(+) > > > > > > diff --git a/include/libcamera/internal/utils.h > > b/include/libcamera/internal/utils.h > > > index 83dada7cc16c..a377d4ea07ab 100644 > > > --- a/include/libcamera/internal/utils.h > > > +++ b/include/libcamera/internal/utils.h > > > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> > > details::enumerate_adapter<T *> > > > } > > > #endif > > > > > > +using BaseDuration = std::chrono::duration<double, std::nano>; > > > + > > > +class Duration : public BaseDuration > > > +{ > > > +public: > > > + Duration() = default; > > > + > > > + template<typename Rep, typename Period> > > > + constexpr Duration(const std::chrono::duration<Rep, > > Period> &d) > > > + : BaseDuration(d) > > > + { > > > + } > > > + > > > + template<typename Period> > > > + double get() const > > > + { > > > + auto const c = > > std::chrono::duration_cast<std::chrono::duration<double, > > Period>>(*this); > > > + return c.count(); > > > + } > > > + > > > + explicit constexpr operator bool() const > > > + { > > > + return *this != BaseDuration::zero(); > > > + } > > > +}; > > > + > > > +#ifndef __DOXYGEN__ > > > +template<class CharT, class Traits> > > > +static inline std::basic_ostream<CharT, Traits> > > &operator<<(std::basic_ostream<CharT, Traits> &os, > > > + const > > Duration &d) > > > +{ > > > + std::basic_ostringstream<CharT, Traits> s; > > > + > > > + s.flags(os.flags()); > > > + s.imbue(os.getloc()); > > > + s.setf(std::ios_base::fixed, std::ios_base::floatfield); > > > + s.precision(2); > > > + s << d.get<std::micro>() << "us"; > > > + return os << s.str(); > > > +} > > > +#endif > > > > One small thing I wonder about is that many classes have a toString > > method. Do we want one here? It could even take an optional argument > > to allow you to format the output differently... but I think that's > > overkill and best left for now! > > > > > > Did you mean that we should do something like: > > > > LOG(IPA, Debug) << "Exposure time is " << exposure.toString(); > > > > over > > > > LOG(IPA, Debug) << "Exposure time is " << exposure; > > > > I chose the later way with the overload <<() operator as that is more > > similar > > to what C++20 will eventually allow with std::chrono::duration > formatting. > > What do others think? > > I think the main reason we've used toString() is so that we don't have > to override operator<< outside of the libcamera namespace, so we've been > more explicit with the toString(). > > I don't mind the override or the explicit, but I think that's the > reasoning. > > Would it be cleaner to implement a toString() which doesn't have to > track the outputstream state, using a local stringstream perhaps and as > that returns a string, the override operator<< can simply return that > pre-prepared string? > I don't think it makes much of a difference in the implementation, as this is exactly what the operator<< overload does right now. There is an awkwardness in having to remember to do a "using libcamera::utils::operator<<" in every source file that wants to use the overload. Te toString() usage could eliminate that, but then it means typing more characters in your logging statement :-) I do like the overload usage (apart from the using.. directive) in that it sort of mirrors what C++20 will allow. Naush > > > > > > Regards, > > Naush > > > > > > > > > > > + > > > } /* namespace utils */ > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > > > index 826144d3c837..3131aa2d6666 100644 > > > --- a/src/libcamera/utils.cpp > > > +++ b/src/libcamera/utils.cpp > > > @@ -506,6 +506,42 @@ std::string libcameraSourcePath() > > > * loop, iterates over an indexed view of the \a iterable > > > */ > > > > > > +/** > > > + * \class Duration > > > + * \brief Helper for std::chrono::duration types. Derived from \a > > BaseDuration > > > + */ > > > + > > > +/** > > > + * \fn Duration::Duration(const std::chrono::duration<Rep, > > Period> &d) > > > + * \brief Copy constructor from a \a std::chrono::duration type. > > > + * \param[in] d The std::chrono::duration object to convert from. > > > + * > > > + * Constructs a \a Duration object from a \a > > std::chrono::duration object, > > > + * converting the representation to \a BaseDuration type. > > > + */ > > > + > > > +/** > > > + * \fn Duration::get<Period>() > > > + * \brief Retrieve the tick count, coverted to the timebase > > provided by the > > > > s/coverted/converted/ > > > > With this tiny fix: > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com > > <mailto:david.plowman@raspberrypi.com>> > > > > Thanks! > > David > > > > > + * template argument Period of type \a std::ratio. > > > + * > > > + * A typical usage example is given below: > > > + * > > > + * \code{.cpp} > > > + * utils::Duration d = 5s; > > > + * double d_in_ms = d.get<std::milli>(); > > > + * \endcode > > > + * > > > + * \return Tick count > > > + */ > > > + > > > +/** > > > + * \fn Duration::operator bool() > > > + * \brief Boolean operator to test if a \a Duration holds a > > non-zero time value. > > > + * > > > + * \return True if \a Duration is a non-zero time value, False > > otherwise. > > > + */ > > > + > > > } /* namespace utils */ > > > > > > } /* namespace libcamera */ > > > -- > > > 2.25.1 > > > > > > > -- > Regards > -- > Kieran >
Hi Naush, On Fri, May 21, 2021 at 09:05:27AM +0100, Naushir Patuck wrote: > A new utils::Duration class is defined to represent a > std::chrono::duration type with double precision nanosecond timebase. > Using a double minimises the loss of precision when converting timebases. > This helper class may be used by IPAs to represent variables such as frame > durations and exposure times. > > An operator << overload is define to help with displaying > utils::Duration value in stream objects. Currently, this will display > the duration value in microseconds. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++ > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++ > 2 files changed, 78 insertions(+) > > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h > index 83dada7cc16c..a377d4ea07ab 100644 > --- a/include/libcamera/internal/utils.h > +++ b/include/libcamera/internal/utils.h > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *> > } > #endif > > +using BaseDuration = std::chrono::duration<double, std::nano>; I was a bit puzzled by the use of double, but reading the standard I see constructor 4) template< class Rep2, class Period2 > constexpr duration( const duration<Rep2,Period2>& d ); which partecipates in overload resolution if computation of the conversion factor (by std::ratio_divide<Period2, Period>) does not overflow and: - std::chrono::treat_as_floating_point<rep>::value == true or both: - std::ratio_divide<Period2, period>::den == 1, and - std::chrono::treat_as_floating_point<Rep2>::value == false. (that is, either the duration uses floating-point ticks, or Period2 is exactly divisible by period) So I guess we need double to maximize the number of template specializations that can be used to construct a Duration ? Could we use std::chrono:nanoseconds ? Or it gets complicated to specify the additional <double> template argument ? Just out of curiosity... Also, I would not expose BaseDuration outside. Can this be done as class Duration : public std::chrono::duration:... { using BaseDuration = .... }; Or do you plan to expose BaseDuration to the rest of the library ? > + > +class Duration : public BaseDuration > +{ > +public: > + Duration() = default; > + > + template<typename Rep, typename Period> > + constexpr Duration(const std::chrono::duration<Rep, Period> &d) > + : BaseDuration(d) > + { > + } > + > + template<typename Period> > + double get() const > + { > + auto const c = std::chrono::duration_cast<std::chrono::duration<double, Period>>(*this); > + return c.count(); > + } > + > + explicit constexpr operator bool() const > + { > + return *this != BaseDuration::zero(); > + } > +}; > + > +#ifndef __DOXYGEN__ > +template<class CharT, class Traits> > +static inline std::basic_ostream<CharT, Traits> &operator<<(std::basic_ostream<CharT, Traits> &os, > + const Duration &d) Nit: alignement to ( > +{ > + std::basic_ostringstream<CharT, Traits> s; > + > + s.flags(os.flags()); > + s.imbue(os.getloc()); > + s.setf(std::ios_base::fixed, std::ios_base::floatfield); > + s.precision(2); Why micro and not nano ? > + s << d.get<std::micro>() << "us"; > + return os << s.str(); > +} > +#endif > + > } /* namespace utils */ > > } /* namespace libcamera */ > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > index 826144d3c837..3131aa2d6666 100644 > --- a/src/libcamera/utils.cpp > +++ b/src/libcamera/utils.cpp > @@ -506,6 +506,42 @@ std::string libcameraSourcePath() > * loop, iterates over an indexed view of the \a iterable > */ > > +/** > + * \class Duration > + * \brief Helper for std::chrono::duration types. Derived from \a BaseDuration Where's BaseDuration documented ? > + */ > + > +/** > + * \fn Duration::Duration(const std::chrono::duration<Rep, Period> &d) > + * \brief Copy constructor from a \a std::chrono::duration type. > + * \param[in] d The std::chrono::duration object to convert from. we don't usually end with a . in documentation single sentences > + * > + * Constructs a \a Duration object from a \a std::chrono::duration object, > + * converting the representation to \a BaseDuration type. > + */ > + > +/** > + * \fn Duration::get<Period>() > + * \brief Retrieve the tick count, coverted to the timebase provided by the > + * template argument Period of type \a std::ratio. > + * > + * A typical usage example is given below: > + * > + * \code{.cpp} > + * utils::Duration d = 5s; > + * double d_in_ms = d.get<std::milli>(); Nice! > + * \endcode > + * > + * \return Tick count The tick count of the Duration expressed in \a Period ? > + */ > + > +/** > + * \fn Duration::operator bool() > + * \brief Boolean operator to test if a \a Duration holds a non-zero time value. > + * > + * \return True if \a Duration is a non-zero time value, False otherwise. > + */ I'm a bit skeptical of operator overloading like this one, even if it's understandable that Duration d = ... if (!d) { } checks for d being 0 or not. Not sure Duration::zero() is not better, nor is Duration::expired()... All minors, the patch looks mostly good, maybe with BaseDuration made private... Thanks j > + > } /* namespace utils */ > > } /* namespace libcamera */ > -- > 2.25.1 >
Hi Jacopo, Thank you for your review feedback. On Fri, 21 May 2021 at 13:27, Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Naush, > > On Fri, May 21, 2021 at 09:05:27AM +0100, Naushir Patuck wrote: > > A new utils::Duration class is defined to represent a > > std::chrono::duration type with double precision nanosecond timebase. > > Using a double minimises the loss of precision when converting timebases. > > This helper class may be used by IPAs to represent variables such as > frame > > durations and exposure times. > > > > An operator << overload is define to help with displaying > > utils::Duration value in stream objects. Currently, this will display > > the duration value in microseconds. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++ > > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++ > > 2 files changed, 78 insertions(+) > > > > diff --git a/include/libcamera/internal/utils.h > b/include/libcamera/internal/utils.h > > index 83dada7cc16c..a377d4ea07ab 100644 > > --- a/include/libcamera/internal/utils.h > > +++ b/include/libcamera/internal/utils.h > > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> > details::enumerate_adapter<T *> > > } > > #endif > > > > +using BaseDuration = std::chrono::duration<double, std::nano>; > > I was a bit puzzled by the use of double, but reading the standard I > see constructor 4) > > template< class Rep2, class Period2 > > constexpr duration( const duration<Rep2,Period2>& d ); > > which partecipates in overload resolution if > computation of the conversion factor (by > std::ratio_divide<Period2, Period>) does not overflow and: > - std::chrono::treat_as_floating_point<rep>::value == true > or both: > - std::ratio_divide<Period2, period>::den == 1, and > - std::chrono::treat_as_floating_point<Rep2>::value == > false. > > (that is, either the duration uses floating-point ticks, or > Period2 is exactly divisible by period) > > So I guess we need double to maximize the number of template > specializations that can be used to construct a Duration ? > That's correct! Also I used double to preserve precision during possible base conversions. > > Could we use std::chrono:nanoseconds ? Or it gets complicated to > specify the additional <double> template argument ? Just out of > curiosity... > I don't think we can, std::chrono::nanoseconds is a typedef for std::chrono::duration</*signed integer*/, std::nano>, so we would lose the double. > > Also, I would not expose BaseDuration outside. Can this be done as > > class Duration : public std::chrono::duration:... > { > using BaseDuration = .... > > }; > > Or do you plan to expose BaseDuration to the rest of the library ? > I don't see BaseDuration being used outside this library, so I will put it into the class definition as you suggested. > > > + > > +class Duration : public BaseDuration > > +{ > > +public: > > + Duration() = default; > > + > > + template<typename Rep, typename Period> > > + constexpr Duration(const std::chrono::duration<Rep, Period> &d) > > + : BaseDuration(d) > > + { > > + } > > + > > + template<typename Period> > > + double get() const > > + { > > + auto const c = > std::chrono::duration_cast<std::chrono::duration<double, Period>>(*this); > > + return c.count(); > > + } > > + > > + explicit constexpr operator bool() const > > + { > > + return *this != BaseDuration::zero(); > > + } > > +}; > > + > > +#ifndef __DOXYGEN__ > > +template<class CharT, class Traits> > > +static inline std::basic_ostream<CharT, Traits> > &operator<<(std::basic_ostream<CharT, Traits> &os, > > + const Duration > &d) > > Nit: alignement to ( > Ack. > > > +{ > > + std::basic_ostringstream<CharT, Traits> s; > > + > > + s.flags(os.flags()); > > + s.imbue(os.getloc()); > > + s.setf(std::ios_base::fixed, std::ios_base::floatfield); > > + s.precision(2); > > Why micro and not nano ? > Purely personal preference :-) Laurent did also suggest using seconds. I don't have strong opinions on this one, so I will go with what the majority would like. > > + s << d.get<std::micro>() << "us"; > > + return os << s.str(); > > +} > > +#endif > > + > > } /* namespace utils */ > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > > index 826144d3c837..3131aa2d6666 100644 > > --- a/src/libcamera/utils.cpp > > +++ b/src/libcamera/utils.cpp > > @@ -506,6 +506,42 @@ std::string libcameraSourcePath() > > * loop, iterates over an indexed view of the \a iterable > > */ > > > > +/** > > + * \class Duration > > + * \brief Helper for std::chrono::duration types. Derived from \a > BaseDuration > > Where's BaseDuration documented ? > Sorry, this is my bad. It somehow ended up in patch 4/4, don't ask how :-) I'll move it back to this patch in the next revision. > > > + */ > > + > > +/** > > + * \fn Duration::Duration(const std::chrono::duration<Rep, Period> &d) > > + * \brief Copy constructor from a \a std::chrono::duration type. > > + * \param[in] d The std::chrono::duration object to convert from. > > we don't usually end with a . in documentation single sentences > Ack. > > > + * > > + * Constructs a \a Duration object from a \a std::chrono::duration > object, > > + * converting the representation to \a BaseDuration type. > > + */ > > + > > +/** > > + * \fn Duration::get<Period>() > > + * \brief Retrieve the tick count, coverted to the timebase provided by > the > > + * template argument Period of type \a std::ratio. > > + * > > + * A typical usage example is given below: > > + * > > + * \code{.cpp} > > + * utils::Duration d = 5s; > > + * double d_in_ms = d.get<std::milli>(); > > Nice! > > > + * \endcode > > + * > > + * \return Tick count > > The tick count of the Duration expressed in \a Period ? > Ack. > > + */ > > + > > +/** > > + * \fn Duration::operator bool() > > + * \brief Boolean operator to test if a \a Duration holds a non-zero > time value. > > + * > > + * \return True if \a Duration is a non-zero time value, False > otherwise. > > + */ > > I'm a bit skeptical of operator overloading like this one, even if > it's understandable that > Duration d = ... > if (!d) { > > } > checks for d being 0 or not. > > Not sure Duration::zero() is not better, nor is Duration::expired()... > I do like that we can use Duration as a scaler equivalent with the overload, e.g. Duration manualShutter = 5ms; If (manualShutter) { /* do something */ } But again, I am happy to create a Duration::zero() if you prefer. Regards, Naush > > All minors, the patch looks mostly good, maybe with BaseDuration made > private... > > Thanks > j > > > > + > > } /* namespace utils */ > > > > } /* namespace libcamera */ > > -- > > 2.25.1 > > >
Hi Naush, On Fri, May 21, 2021 at 01:42:43PM +0100, Naushir Patuck wrote: > Hi Jacopo, > > Thank you for your review feedback. > > On Fri, 21 May 2021 at 13:27, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Hi Naush, > > > > On Fri, May 21, 2021 at 09:05:27AM +0100, Naushir Patuck wrote: > > > A new utils::Duration class is defined to represent a > > > std::chrono::duration type with double precision nanosecond timebase. > > > Using a double minimises the loss of precision when converting timebases. > > > This helper class may be used by IPAs to represent variables such as > > frame > > > durations and exposure times. > > > > > > An operator << overload is define to help with displaying > > > utils::Duration value in stream objects. Currently, this will display > > > the duration value in microseconds. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++ > > > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++ > > > 2 files changed, 78 insertions(+) > > > > > > diff --git a/include/libcamera/internal/utils.h > > b/include/libcamera/internal/utils.h > > > index 83dada7cc16c..a377d4ea07ab 100644 > > > --- a/include/libcamera/internal/utils.h > > > +++ b/include/libcamera/internal/utils.h > > > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> > > details::enumerate_adapter<T *> > > > } > > > #endif > > > > > > +using BaseDuration = std::chrono::duration<double, std::nano>; > > > > I was a bit puzzled by the use of double, but reading the standard I > > see constructor 4) > > > > template< class Rep2, class Period2 > > > constexpr duration( const duration<Rep2,Period2>& d ); > > > > which partecipates in overload resolution if > > computation of the conversion factor (by > > std::ratio_divide<Period2, Period>) does not overflow and: > > - std::chrono::treat_as_floating_point<rep>::value == true > > or both: > > - std::ratio_divide<Period2, period>::den == 1, and > > - std::chrono::treat_as_floating_point<Rep2>::value == > > false. > > > > (that is, either the duration uses floating-point ticks, or > > Period2 is exactly divisible by period) > > > > So I guess we need double to maximize the number of template > > specializations that can be used to construct a Duration ? > > > > That's correct! Also I used double to preserve precision during possible > base conversions. > > > > > > Could we use std::chrono:nanoseconds ? Or it gets complicated to > > specify the additional <double> template argument ? Just out of > > curiosity... > > > > I don't think we can, std::chrono::nanoseconds is a typedef for > std::chrono::duration</*signed integer*/, std::nano>, so we would > lose the double. > > > > > > Also, I would not expose BaseDuration outside. Can this be done as > > > > class Duration : public std::chrono::duration:... > > { > > using BaseDuration = .... > > > > }; > > > > Or do you plan to expose BaseDuration to the rest of the library ? > > > > I don't see BaseDuration being used outside this library, so I will put it > into the class definition as you suggested. > Thanks! > > > > > > + > > > +class Duration : public BaseDuration > > > +{ > > > +public: > > > + Duration() = default; > > > + > > > + template<typename Rep, typename Period> > > > + constexpr Duration(const std::chrono::duration<Rep, Period> &d) > > > + : BaseDuration(d) > > > + { > > > + } > > > + > > > + template<typename Period> > > > + double get() const > > > + { > > > + auto const c = > > std::chrono::duration_cast<std::chrono::duration<double, Period>>(*this); > > > + return c.count(); > > > + } > > > + > > > + explicit constexpr operator bool() const > > > + { > > > + return *this != BaseDuration::zero(); > > > + } > > > +}; > > > + > > > +#ifndef __DOXYGEN__ > > > +template<class CharT, class Traits> > > > +static inline std::basic_ostream<CharT, Traits> > > &operator<<(std::basic_ostream<CharT, Traits> &os, > > > + const Duration > > &d) > > > > Nit: alignement to ( > > > > Ack. > > > > > > > +{ > > > + std::basic_ostringstream<CharT, Traits> s; > > > + > > > + s.flags(os.flags()); > > > + s.imbue(os.getloc()); > > > + s.setf(std::ios_base::fixed, std::ios_base::floatfield); > > > + s.precision(2); > > > > Why micro and not nano ? > > > > Purely personal preference :-) > Laurent did also suggest using seconds. I don't have strong opinions on > this one, > so I will go with what the majority would like. I see. Seconds for the kind of durations we deal with it probably too much ? We have failed to standardize on a time measurement unit unforuntately. We have a mjority of controls that work in microseconds and one nanosecond (SensorTimestamp, my bad). Keep micro might be better to align with most of the time measurement units. Although stabilizing on micro might require some rational values to represent shorter events such as a line duration. Anyway, not strictly related to this one, feel free to keep micro if you like it most. > > > > > + s << d.get<std::micro>() << "us"; > > > + return os << s.str(); > > > +} > > > +#endif > > > + > > > } /* namespace utils */ > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > > > index 826144d3c837..3131aa2d6666 100644 > > > --- a/src/libcamera/utils.cpp > > > +++ b/src/libcamera/utils.cpp > > > @@ -506,6 +506,42 @@ std::string libcameraSourcePath() > > > * loop, iterates over an indexed view of the \a iterable > > > */ > > > > > > +/** > > > + * \class Duration > > > + * \brief Helper for std::chrono::duration types. Derived from \a > > BaseDuration > > > > Where's BaseDuration documented ? > > > > Sorry, this is my bad. It somehow ended up in patch 4/4, don't ask how :-) > I'll move it back to this patch in the next revision. > > > > > > > + */ > > > + > > > +/** > > > + * \fn Duration::Duration(const std::chrono::duration<Rep, Period> &d) > > > + * \brief Copy constructor from a \a std::chrono::duration type. > > > + * \param[in] d The std::chrono::duration object to convert from. > > > > we don't usually end with a . in documentation single sentences > > > > Ack. > > > > > > > + * > > > + * Constructs a \a Duration object from a \a std::chrono::duration > > object, > > > + * converting the representation to \a BaseDuration type. > > > + */ > > > + > > > +/** > > > + * \fn Duration::get<Period>() > > > + * \brief Retrieve the tick count, coverted to the timebase provided by > > the > > > + * template argument Period of type \a std::ratio. > > > + * > > > + * A typical usage example is given below: > > > + * > > > + * \code{.cpp} > > > + * utils::Duration d = 5s; > > > + * double d_in_ms = d.get<std::milli>(); > > > > Nice! > > > > > + * \endcode > > > + * > > > + * \return Tick count > > > > The tick count of the Duration expressed in \a Period ? > > > > Ack. > > > > > + */ > > > + > > > +/** > > > + * \fn Duration::operator bool() > > > + * \brief Boolean operator to test if a \a Duration holds a non-zero > > time value. > > > + * > > > + * \return True if \a Duration is a non-zero time value, False > > otherwise. > > > + */ > > > > I'm a bit skeptical of operator overloading like this one, even if > > it's understandable that > > Duration d = ... > > if (!d) { > > > > } > > checks for d being 0 or not. > > > > Not sure Duration::zero() is not better, nor is Duration::expired()... > > > > I do like that we can use Duration as a scaler equivalent with the overload, > e.g. > > Duration manualShutter = 5ms; > > If (manualShutter) { > /* do something */ > } > > But again, I am happy to create a Duration::zero() if you prefer. I'm not pushing for it, as I think the behaviour of this bool operator is predicatable enough, so feel free to keep it! Thanks j > > Regards, > Naush > > > > > > All minors, the patch looks mostly good, maybe with BaseDuration made > > private... > > > > Thanks > > j > > > > > > > + > > > } /* namespace utils */ > > > > > > } /* namespace libcamera */ > > > -- > > > 2.25.1 > > > > >
diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h index 83dada7cc16c..a377d4ea07ab 100644 --- a/include/libcamera/internal/utils.h +++ b/include/libcamera/internal/utils.h @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *> } #endif +using BaseDuration = std::chrono::duration<double, std::nano>; + +class Duration : public BaseDuration +{ +public: + Duration() = default; + + template<typename Rep, typename Period> + constexpr Duration(const std::chrono::duration<Rep, Period> &d) + : BaseDuration(d) + { + } + + template<typename Period> + double get() const + { + auto const c = std::chrono::duration_cast<std::chrono::duration<double, Period>>(*this); + return c.count(); + } + + explicit constexpr operator bool() const + { + return *this != BaseDuration::zero(); + } +}; + +#ifndef __DOXYGEN__ +template<class CharT, class Traits> +static inline std::basic_ostream<CharT, Traits> &operator<<(std::basic_ostream<CharT, Traits> &os, + const Duration &d) +{ + std::basic_ostringstream<CharT, Traits> s; + + s.flags(os.flags()); + s.imbue(os.getloc()); + s.setf(std::ios_base::fixed, std::ios_base::floatfield); + s.precision(2); + s << d.get<std::micro>() << "us"; + return os << s.str(); +} +#endif + } /* namespace utils */ } /* namespace libcamera */ diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp index 826144d3c837..3131aa2d6666 100644 --- a/src/libcamera/utils.cpp +++ b/src/libcamera/utils.cpp @@ -506,6 +506,42 @@ std::string libcameraSourcePath() * loop, iterates over an indexed view of the \a iterable */ +/** + * \class Duration + * \brief Helper for std::chrono::duration types. Derived from \a BaseDuration + */ + +/** + * \fn Duration::Duration(const std::chrono::duration<Rep, Period> &d) + * \brief Copy constructor from a \a std::chrono::duration type. + * \param[in] d The std::chrono::duration object to convert from. + * + * Constructs a \a Duration object from a \a std::chrono::duration object, + * converting the representation to \a BaseDuration type. + */ + +/** + * \fn Duration::get<Period>() + * \brief Retrieve the tick count, coverted to the timebase provided by the + * template argument Period of type \a std::ratio. + * + * A typical usage example is given below: + * + * \code{.cpp} + * utils::Duration d = 5s; + * double d_in_ms = d.get<std::milli>(); + * \endcode + * + * \return Tick count + */ + +/** + * \fn Duration::operator bool() + * \brief Boolean operator to test if a \a Duration holds a non-zero time value. + * + * \return True if \a Duration is a non-zero time value, False otherwise. + */ + } /* namespace utils */ } /* namespace libcamera */
A new utils::Duration class is defined to represent a std::chrono::duration type with double precision nanosecond timebase. Using a double minimises the loss of precision when converting timebases. This helper class may be used by IPAs to represent variables such as frame durations and exposure times. An operator << overload is define to help with displaying utils::Duration value in stream objects. Currently, this will display the duration value in microseconds. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++ src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++ 2 files changed, 78 insertions(+)