[libcamera-devel] [PATCH v3 1/4] libcamera: utils: Add helper class for std::chrono::duration

[Kieran Bingham]

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 at raspberrypi.com <mailto:david.plowman at raspberrypi.com>>
> wrote:
> 
>     Hi Naush
> 
>     Thanks for the updated patch!
> 
>     On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush at raspberrypi.com
>     <mailto:naush at 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 at raspberrypi.com
>     <mailto:naush at 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 at raspberrypi.com
>     <mailto:david.plowman at 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