<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 21 May 2021 at 12:14, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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, David,<br>
<br>
On 21/05/2021 11:53, Naushir Patuck wrote:<br>
> Hi David,<br>
> <br>
> Thank you for your feedback.<br>
> <br>
> On Fri, 21 May 2021 at 11:20, David Plowman<br>
> <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a> <mailto:<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>>><br>
> wrote:<br>
> <br>
> 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><br>
> <mailto:<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<br>
> timebases.<br>
> > This helper class may be used by IPAs to represent variables such<br>
> 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>
> <mailto:<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>>><br>
> > ---<br>
> > include/libcamera/internal/utils.h | 42<br>
> ++++++++++++++++++++++++++++++<br>
> > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++<br>
> > 2 files changed, 78 insertions(+)<br>
> ><br>
> > diff --git a/include/libcamera/internal/utils.h<br>
> 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]) -><br>
> 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,<br>
> Period> &d)<br>
> > + : BaseDuration(d)<br>
> > + {<br>
> > + }<br>
> > +<br>
> > + template<typename Period><br>
> > + double get() const<br>
> > + {<br>
> > + auto const c =<br>
> std::chrono::duration_cast<std::chrono::duration<double,<br>
> 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><br>
> &operator<<(std::basic_ostream<CharT, Traits> &os,<br>
> > + const<br>
> 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>
> <br>
> <br>
> Did you mean that we should do something like:<br>
> <br>
> LOG(IPA, Debug) << "Exposure time is " << exposure.toString();<br>
> <br>
> over<br>
> <br>
> LOG(IPA, Debug) << "Exposure time is " << exposure;<br>
> <br>
> I chose the later way with the overload <<() operator as that is more<br>
> similar<br>
> to what C++20 will eventually allow with std::chrono::duration formatting.<br>
> What do others think?<br>
<br>
I think the main reason we've used toString() is so that we don't have<br>
to override operator<< outside of the libcamera namespace, so we've been<br>
more explicit with the toString().<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
I don't mind the override or the explicit, but I think that's the reasoning.<br>
<br>
Would it be cleaner to implement a toString() which doesn't have to<br>
track the outputstream state, using a local stringstream perhaps and as<br>
that returns a string, the override operator<< can simply return that<br>
pre-prepared string?<br></blockquote><div><br></div><div>I don't think it makes much of a difference in the implementation, as this is exactly</div><div>what the operator<< overload does right now. There is an awkwardness in having</div><div>to remember to do a "using libcamera::utils::operator<<" in every source file</div><div>that wants to use the overload. Te toString() usage could eliminate that, but then</div><div>it means typing more characters in your logging statement :-)</div><div><br></div><div>I do like the overload usage (apart from the using.. directive) in that it sort of mirrors</div><div>what C++20 will allow.</div><div><br></div><div>Naush</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>
<br>
> <br>
> Regards,<br>
> Naush<br>
> <br>
> <br>
> <br>
> <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<br>
> BaseDuration<br>
> > + */<br>
> > +<br>
> > +/**<br>
> > + * \fn Duration::Duration(const std::chrono::duration<Rep,<br>
> 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<br>
> 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<br>
> 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>
> <mailto:<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<br>
> non-zero time value.<br>
> > + *<br>
> > + * \return True if \a Duration is a non-zero time value, False<br>
> otherwise.<br>
> > + */<br>
> > +<br>
> > } /* namespace utils */<br>
> ><br>
> > } /* namespace libcamera */<br>
> > --<br>
> > 2.25.1<br>
> ><br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>