[libcamera-devel] [PATCH v5 1/4] libcamera: utils: Add helper class for std::chrono::duration
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 8 03:09:03 CEST 2021
Hi Naush,
Thank you for the patch.
On Tue, May 25, 2021 at 12:42:38PM +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.
Do we really need double precision ? Have you noticed loss of precision
in calculations that would be caused by usage of a 64-bit integer ? I'm
fine keeping the double for now, but moving forward towards more
widespread usage of std::chrono in libcamera, and especially in its
public API, I'd prefer using an integer if possible.
> An operator << overload is define to help with displaying
s/define/defined/
> 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>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++
> src/libcamera/utils.cpp | 37 ++++++++++++++++++++++++++
> 2 files changed, 79 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();
> +}
It would be nice to avoid inlining this. What do you think of the
following ?
diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h
index f536f2267d70..15beb0f44172 100644
--- a/include/libcamera/internal/utils.h
+++ b/include/libcamera/internal/utils.h
@@ -346,18 +346,8 @@ public:
#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();
-}
+std::basic_ostream<CharT, Traits> &operator<<(std::basic_ostream<CharT, Traits> &os,
+ const utils::Duration &d);
#endif
} /* namespace libcamera */
diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
index 6624ff74cdc7..cdfc6e104b52 100644
--- a/src/libcamera/utils.cpp
+++ b/src/libcamera/utils.cpp
@@ -545,4 +545,26 @@ std::string libcameraSourcePath()
} /* namespace utils */
+#ifndef __DOXYGEN__
+template<class CharT, class Traits>
+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();
+}
+
+template
+std::basic_ostream<char, std::char_traits<char>> &
+operator<< <char, std::char_traits<char>>(
+ std::basic_ostream<char, std::char_traits<char>> &os,
+ const utils::Duration &d);
+#endif /* __DOXYGEN__ */
+
} /* namespace libcamera */
On a side node, it took me quite some time to debug linker errors. Turns
out that
template<>
std::basic_ostream<char, std::char_traits<char>> &
operator<< <char, std::char_traits<char>>(
std::basic_ostream<char, std::char_traits<char>> &os,
const utils::Duration &d);
in utils.cpp doesn't instantiate the template specialization, while
template
std::basic_ostream<char, std::char_traits<char>> &
operator<< <char, std::char_traits<char>>(
std::basic_ostream<char, std::char_traits<char>> &os,
const utils::Duration &d);
does.
> +#endif
> +
> } /* namespace libcamera */
>
> #endif /* __LIBCAMERA_INTERNAL_UTILS_H__ */
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 826144d3c837..6624ff74cdc7 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -506,6 +506,43 @@ std::string libcameraSourcePath()
> * loop, iterates over an indexed view of the \a iterable
> */
>
> +/**
> + * \class Duration
> + * \brief Helper class from std::chrono::duration that represents a time
> + * duration in nanoseconds with double precision
> + */
> +
> +/**
> + * \fn Duration::Duration(const std::chrono::duration<Rep, Period> &d)
> + * \brief Copy constructor from a \a std::chrono::duration type
It's not a copy constructor, as d can be of a different type than
Duration. How about
* \brief Construct a Duration by converting an arbitrary std::chrono::duration
> + * \param[in] d The std::chrono::duration object to convert from
> + *
> + * Constructs a \a Duration object from a \a std::chrono::duration object,
> + * internally represented in double precision with nanoseconds ticks
This should probably updated accordingly, I find the the second part a
bit misleading, it seems to imply that d is of double precision with
nanoseconds ticks.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> +
> +/**
> + * \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
> + */
> +
> } /* namespace utils */
>
> } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list