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

12410 diff mbox series

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@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++
 src/libcamera/utils.cpp            | 37 ++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

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@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo@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@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 */

Hi Laurent,

Thank you for your feedback.

On Tue, 8 Jun 2021 at 02:09, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> 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.
>

I've used double precision as a catch all more than anything else here.
I don't expect a precision problem with replacing it with int64 in the
future.


>
> > An operator << overload is define to help with displaying
>
> s/define/defined/
>

Ack



> > utils::Duration value in stream objects. Currently, this will display
> > the duration value in microseconds.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo@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 */
>

Yes, I can replace the inline function with the above implementation in the
next revision.


>
>
> 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.
>

Interestingly, both the above forms work on my builds  with gcc 9.3.0.
I didn't even know that "template" without arrow brackets was valid!

 > +#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
>

Ack


>
> > + * \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.
>

Ack


>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
>

Hi Naush,

On Tue, Jun 08, 2021 at 10:33:42AM +0100, Naushir Patuck wrote:
> On Tue, 8 Jun 2021 at 02:09, Laurent Pinchart wrote:
> > 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.
> 
> I've used double precision as a catch all more than anything else here.
> I don't expect a precision problem with replacing it with int64 in the
> future.
> 
> > > An operator << overload is define to help with displaying
> >
> > s/define/defined/
> 
> Ack
> 
> > > the duration value in microseconds.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > Reviewed-by: Jacopo Mondi <jacopo@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 */
> >
> 
> Yes, I can replace the inline function with the above implementation in the
> next revision.

Thanks. I'll then merge it right away.

> > 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.
> 
> Interestingly, both the above forms work on my builds  with gcc 9.3.0.
> I didn't even know that "template" without arrow brackets was valid!

Neither did I, and I've refrained from researching this :-) I was
testing with gcc 11 and clang 11.

>  > +#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
> 
> Ack
> 
> > > + * \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.
> 
> Ack
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 */