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

[Jacopo Mondi]

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 ?

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
>