<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 8 Jun 2021 at 02:09, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.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>
Thank you for the patch.<br>
<br>
On Tue, May 25, 2021 at 12:42:38PM +0100, Naushir Patuck wrote:<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>
Do we really need double precision ? Have you noticed loss of precision<br>
in calculations that would be caused by usage of a 64-bit integer ? I'm<br>
fine keeping the double for now, but moving forward towards more<br>
widespread usage of std::chrono in libcamera, and especially in its<br>
public API, I'd prefer using an integer if possible.<br></blockquote><div><br></div><div>I've used double precision as a catch all more than anything else here.</div><div>I don't expect a precision problem with replacing it with int64 in the</div><div>future.</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>
> An operator << overload is define to help with displaying<br>
<br>
s/define/defined/<br></blockquote><div><br></div><div>Ack</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> ---<br>
> include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++<br>
> src/libcamera/utils.cpp | 37 ++++++++++++++++++++++++++<br>
> 2 files changed, 79 insertions(+)<br>
> <br>
> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h<br>
> index 83dada7cc16c..f536f2267d70 100644<br>
> --- a/include/libcamera/internal/utils.h<br>
> +++ b/include/libcamera/internal/utils.h<br>
> @@ -316,8 +316,50 @@ auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter<T *><br>
> }<br>
> #endif<br>
> <br>
> +class Duration : public std::chrono::duration<double, std::nano><br>
> +{<br>
> + using BaseDuration = std::chrono::duration<double, std::nano>;<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>
> } /* namespace utils */<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 utils::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>
<br>
It would be nice to avoid inlining this. What do you think of the<br>
following ?<br>
<br>
diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h<br>
index f536f2267d70..15beb0f44172 100644<br>
--- a/include/libcamera/internal/utils.h<br>
+++ b/include/libcamera/internal/utils.h<br>
@@ -346,18 +346,8 @@ public:<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 utils::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>
+std::basic_ostream<CharT, Traits> &operator<<(std::basic_ostream<CharT, Traits> &os,<br>
+ const utils::Duration &d);<br>
#endif<br>
<br>
} /* namespace libcamera */<br>
diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp<br>
index 6624ff74cdc7..cdfc6e104b52 100644<br>
--- a/src/libcamera/utils.cpp<br>
+++ b/src/libcamera/utils.cpp<br>
@@ -545,4 +545,26 @@ std::string libcameraSourcePath()<br>
<br>
} /* namespace utils */<br>
<br>
+#ifndef __DOXYGEN__<br>
+template<class CharT, class Traits><br>
+std::basic_ostream<CharT, Traits> &operator<<(std::basic_ostream<CharT, Traits> &os,<br>
+ const utils::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>
+<br>
+template<br>
+std::basic_ostream<char, std::char_traits<char>> &<br>
+operator<< <char, std::char_traits<char>>(<br>
+ std::basic_ostream<char, std::char_traits<char>> &os,<br>
+ const utils::Duration &d);<br>
+#endif /* __DOXYGEN__ */<br>
+<br>
} /* namespace libcamera */<br></blockquote><div><br></div><div>Yes, I can replace the inline function with the above implementation in the next revision.</div><div> <br></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>
On a side node, it took me quite some time to debug linker errors. Turns<br>
out that<br>
<br>
template<><br>
std::basic_ostream<char, std::char_traits<char>> &<br>
operator<< <char, std::char_traits<char>>(<br>
std::basic_ostream<char, std::char_traits<char>> &os,<br>
const utils::Duration &d);<br>
<br>
in utils.cpp doesn't instantiate the template specialization, while<br>
<br>
template<br>
std::basic_ostream<char, std::char_traits<char>> &<br>
operator<< <char, std::char_traits<char>>(<br>
std::basic_ostream<char, std::char_traits<char>> &os,<br>
const utils::Duration &d);<br>
<br>
does.<br></blockquote><div><br></div><div>Interestingly, both the above forms work on my builds with gcc 9.3.0.</div><div>I didn't even know that "template" without arrow brackets was valid!</div><div><br></div><div> > +#endif</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> } /* namespace libcamera */<br>
> <br>
> #endif /* __LIBCAMERA_INTERNAL_UTILS_H__ */<br>
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp<br>
> index 826144d3c837..6624ff74cdc7 100644<br>
> --- a/src/libcamera/utils.cpp<br>
> +++ b/src/libcamera/utils.cpp<br>
> @@ -506,6 +506,43 @@ std::string libcameraSourcePath()<br>
> * loop, iterates over an indexed view of the \a iterable<br>
> */<br>
> <br>
> +/**<br>
> + * \class Duration<br>
> + * \brief Helper class from std::chrono::duration that represents a time<br>
> + * duration in nanoseconds with double precision<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>
<br>
It's not a copy constructor, as d can be of a different type than<br>
Duration. How about<br>
<br>
* \brief Construct a Duration by converting an arbitrary std::chrono::duration<br></blockquote><div><br></div><div>Ack</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>
> + * \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>
> + * internally represented in double precision with nanoseconds ticks<br>
<br>
This should probably updated accordingly, I find the the second part a<br>
bit misleading, it seems to imply that d is of double precision with<br>
nanoseconds ticks.<br></blockquote><div><br></div><div>Ack</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>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> + */<br>
> +<br>
> +/**<br>
> + * \fn Duration::get<Period>()<br>
> + * \brief Retrieve the tick count, converted to the timebase provided by the<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 The tick count of the Duration expressed in \a Period<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>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>