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

[Naushir Patuck]

Hi Jacopo,

Thank you for your review feedback.

On Fri, 21 May 2021 at 13:27, Jacopo Mondi <jacopo at 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 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>;
>
> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210521/edfd6bd4/attachment.htm>