[libcamera-devel] [PATCH v3 1/4] libcamera: utils: Add helper class for std::chrono::duration
Jacopo Mondi
jacopo at jmondi.org
Fri May 21 15:05:25 CEST 2021
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 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.
>
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
> > >
> >
More information about the libcamera-devel
mailing list