<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 21 May 2021 at 11:20, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
Thanks for the updated patch!<br>
<br>
On Fri, 21 May 2021 at 09:05, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> A new utils::Duration class is defined to represent a<br>
> std::chrono::duration type with double precision nanosecond timebase.<br>
> Using a double minimises the loss of precision when converting timebases.<br>
> This helper class may be used by IPAs to represent variables such as frame<br>
> durations and exposure times.<br>
><br>
> An operator << overload is define to help with displaying<br>
> utils::Duration value in stream objects. Currently, this will display<br>
> the duration value in microseconds.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++<br>
> src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++<br>
> 2 files changed, 78 insertions(+)<br>
><br>
> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h<br>
> index 83dada7cc16c..a377d4ea07ab 100644<br>
> --- a/include/libcamera/internal/utils.h<br>
> +++ b/include/libcamera/internal/utils.h<br>
> @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *><br>
> }<br>
> #endif<br>
><br>
> +using BaseDuration = std::chrono::duration<double, std::nano>;<br>
> +<br>
> +class Duration : public BaseDuration<br>
> +{<br>
> +public:<br>
> + Duration() = default;<br>
> +<br>
> + template<typename Rep, typename Period><br>
> + constexpr Duration(const std::chrono::duration<Rep, Period> &d)<br>
> + : BaseDuration(d)<br>
> + {<br>
> + }<br>
> +<br>
> + template<typename Period><br>
> + double get() const<br>
> + {<br>
> + auto const c = std::chrono::duration_cast<std::chrono::duration<double, Period>>(*this);<br>
> + return c.count();<br>
> + }<br>
> +<br>
> + explicit constexpr operator bool() const<br>
> + {<br>
> + return *this != BaseDuration::zero();<br>
> + }<br>
> +};<br>
> +<br>
> +#ifndef __DOXYGEN__<br>
> +template<class CharT, class Traits><br>
> +static inline std::basic_ostream<CharT, Traits> &operator<<(std::basic_ostream<CharT, Traits> &os,<br>
> + const Duration &d)<br>
> +{<br>
> + std::basic_ostringstream<CharT, Traits> s;<br>
> +<br>
> + s.flags(os.flags());<br>
> + s.imbue(os.getloc());<br>
> + s.setf(std::ios_base::fixed, std::ios_base::floatfield);<br>
> + s.precision(2);<br>
> + s << d.get<std::micro>() << "us";<br>
> + return os << s.str();<br>
> +}<br>
> +#endif<br>
<br>
One small thing I wonder about is that many classes have a toString<br>
method. Do we want one here? It could even take an optional argument<br>
to allow you to format the output differently... but I think that's<br>
overkill and best left for now!<br></blockquote><div><br></div><div>Did you mean that we should do something like:</div><div><br></div><div>LOG(IPA, Debug) << "Exposure time is " << exposure.toString();</div><div><br></div><div>over</div><div><br></div><div>LOG(IPA, Debug) << "Exposure time is " << exposure;<br></div><div><br></div><div>I chose the later way with the overload <<() operator as that is more similar</div><div>to what C++20 will eventually allow with std::chrono::duration formatting.</div><div>What do others think?</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> } /* namespace utils */<br>
><br>
> } /* namespace libcamera */<br>
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp<br>
> index 826144d3c837..3131aa2d6666 100644<br>
> --- a/src/libcamera/utils.cpp<br>
> +++ b/src/libcamera/utils.cpp<br>
> @@ -506,6 +506,42 @@ std::string libcameraSourcePath()<br>
> * loop, iterates over an indexed view of the \a iterable<br>
> */<br>
><br>
> +/**<br>
> + * \class Duration<br>
> + * \brief Helper for std::chrono::duration types. Derived from \a BaseDuration<br>
> + */<br>
> +<br>
> +/**<br>
> + * \fn Duration::Duration(const std::chrono::duration<Rep, Period> &d)<br>
> + * \brief Copy constructor from a \a std::chrono::duration type.<br>
> + * \param[in] d The std::chrono::duration object to convert from.<br>
> + *<br>
> + * Constructs a \a Duration object from a \a std::chrono::duration object,<br>
> + * converting the representation to \a BaseDuration type.<br>
> + */<br>
> +<br>
> +/**<br>
> + * \fn Duration::get<Period>()<br>
> + * \brief Retrieve the tick count, coverted to the timebase provided by the<br>
<br>
s/coverted/converted/<br>
<br>
With this tiny fix:<br>
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Thanks!<br>
David<br>
<br>
> + * template argument Period of type \a std::ratio.<br>
> + *<br>
> + * A typical usage example is given below:<br>
> + *<br>
> + * \code{.cpp}<br>
> + * utils::Duration d = 5s;<br>
> + * double d_in_ms = d.get<std::milli>();<br>
> + * \endcode<br>
> + *<br>
> + * \return Tick count<br>
> + */<br>
> +<br>
> +/**<br>
> + * \fn Duration::operator bool()<br>
> + * \brief Boolean operator to test if a \a Duration holds a non-zero time value.<br>
> + *<br>
> + * \return True if \a Duration is a non-zero time value, False otherwise.<br>
> + */<br>
> +<br>
> } /* namespace utils */<br>
><br>
> } /* namespace libcamera */<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>