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

[Jacopo Mondi]

Hi Naush,

On Mon, May 24, 2021 at 01:30:20PM +0100, Naushir Patuck wrote:
> Hi Jacopo,
>
> Thank you for your review feedback.
>
> On Mon, 24 May 2021 at 12:58, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> > Hi Naush,
> >
> > On Mon, May 24, 2021 at 09:48:19AM +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 at raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++
> > >  src/libcamera/utils.cpp            | 41 +++++++++++++++++++++++++++++
> > >  2 files changed, 83 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/utils.h
> > b/include/libcamera/internal/utils.h
> > > index 83dada7cc16c..f536f2267d70 100644
> > > --- a/include/libcamera/internal/utils.h
> > > +++ b/include/libcamera/internal/utils.h
> > > @@ -316,8 +316,50 @@ auto enumerate(T (&iterable)[N]) ->
> > details::enumerate_adapter<T *>
> > >  }
> > >  #endif
> > >
> > > +class Duration : public std::chrono::duration<double, std::nano>
> > > +{
> > > +     using BaseDuration = std::chrono::duration<double, std::nano>;
> > > +
> > > +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();
> > > +     }
> > > +};
> > > +
> > >  } /* namespace utils */
> > >
> > > +#ifndef __DOXYGEN__
> > > +template<class CharT, class Traits>
> > > +static inline std::basic_ostream<CharT, Traits>
> > &operator<<(std::basic_ostream<CharT, Traits> &os,
> > > +                                                         const
> > utils::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 libcamera */
> > >
> > >  #endif /* __LIBCAMERA_INTERNAL_UTILS_H__ */
> > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > > index 826144d3c837..480284c6c2c9 100644
> > > --- a/src/libcamera/utils.cpp
> > > +++ b/src/libcamera/utils.cpp
> > > @@ -506,6 +506,47 @@ std::string libcameraSourcePath()
> > >   * loop, iterates over an indexed view of the \a iterable
> > >   */
> > >
> > > +/**
> > > + * \class Duration
> > > + * \brief Helper class for std::chrono::duration types
> >
> > Helper class derived from std::chrono::duration that represents a time
> > duration in nanoseconds with double precision
> >
>
> Ack.
>
>
> >
> > > + */
> > > +
> > > +/**
> > > + * \typedef Duration::BaseDuration
> > > + * \brief Base struct for the \a Duration helper class
> > > + */
> >
> > Isn't this private ? does it need documentation ? It's just a
> > syntactic sugar used in the class definition, isn't it ?
> >
>
> It is private/syntactic sugar - I was not sure what the rule was for
> private members with regards to documentation.  I can remove
> it.

As long as Doxygen does not complain, I think you can leave it out.
However, I see you reference BaseDuration in other parts of the
documentation

>
>
> >
> >
> > > +
> > > +/**
> > > + * \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,
> > > + * internally converting the representation to \a
> > Duration::BaseDuration type

here

If this creates issue I think you could drop it here (or keep the
documentation for BaseDuration, up to you :)

Thanks
  j

> > > + */
> > > +
> > > +/**
> > > + * \fn Duration::get<Period>()
> > > + * \brief Retrieve the tick count, converted 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 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.
> > > + */
> >
> > All minor, which if not other comments, can be changed with your ack
> > when applying
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
>
> Thank's for the tag.  I will submit a v5 revision with the updates once you
> and
> others have a chance to comment on  all the other patches in the series.
>
> Regards,
> Naush
>
>
> >
> > Thanks
> >   j
> >
> > > +
> > >  } /* namespace utils */
> > >
> > >  } /* namespace libcamera */
> > > --
> > > 2.25.1
> > >
> >