<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 21 May 2021 at 13:27, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</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>
On Fri, May 21, 2021 at 09:05:27AM +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>
> 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>
I was a bit puzzled by the use of double, but reading the standard I<br>
see constructor 4)<br>
<br>
template< class Rep2, class Period2 ><br>
constexpr duration( const duration<Rep2,Period2>& d );<br>
<br>
which partecipates in overload resolution if<br>
computation of the conversion factor (by<br>
std::ratio_divide<Period2, Period>) does not overflow and:<br>
- std::chrono::treat_as_floating_point<rep>::value == true<br>
or both:<br>
- std::ratio_divide<Period2, period>::den == 1, and<br>
- std::chrono::treat_as_floating_point<Rep2>::value == false.<br>
<br>
(that is, either the duration uses floating-point ticks, or<br>
Period2 is exactly divisible by period)<br>
<br>
So I guess we need double to maximize the number of template<br>
specializations that can be used to construct a Duration ?<br></blockquote><div><br></div><div>That's correct! Also I used double to preserve precision during possible</div><div>base conversions.</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>
Could we use std::chrono:nanoseconds ? Or it gets complicated to<br>
specify the additional <double> template argument ? Just out of<br>
curiosity...<br></blockquote><div><br></div><div>I don't think we can, std::chrono::nanoseconds is a typedef for</div><div>std::chrono::duration</*signed integer*/, std::nano>, so we would</div><div>lose the double.</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>
Also, I would not expose BaseDuration outside. Can this be done as<br>
<br>
class Duration : public std::chrono::duration:...<br>
{<br>
using BaseDuration = ....<br>
<br>
};<br>
<br>
Or do you plan to expose BaseDuration to the rest of the library ?<br></blockquote><div><br></div><div>I don't see BaseDuration being used outside this library, so I will put it</div><div>into the class definition as you suggested.</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>
> +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>
Nit: alignement to (<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>
> +{<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>
<br>
Why micro and not nano ?<br></blockquote><div><br></div><div>Purely personal preference :-)</div><div>Laurent did also suggest using seconds. I don't have strong opinions on this one,</div><div>so I will go with what the majority would like.</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>
> + s << d.get<std::micro>() << "us";<br>
> + return os << s.str();<br>
> +}<br>
> +#endif<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>
Where's BaseDuration documented ?<br></blockquote><div><br></div><div>Sorry, this is my bad. It somehow ended up in patch 4/4, don't ask how :-)</div><div>I'll move it back to this patch 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>
> +<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>
we don't usually end with a . in documentation single sentences<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>
> + *<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>
> + * 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>
<br>
Nice!<br>
<br>
> + * \endcode<br>
> + *<br>
> + * \return Tick count<br>
<br>
The tick count of the Duration expressed in \a Period ?<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>
> +<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>
I'm a bit skeptical of operator overloading like this one, even if<br>
it's understandable that<br>
Duration d = ...<br>
if (!d) {<br>
<br>
}<br>
checks for d being 0 or not.<br>
<br>
Not sure Duration::zero() is not better, nor is Duration::expired()...<br></blockquote><div><br></div><div>I do like that we can use Duration as a scaler equivalent with the overload,</div><div>e.g.</div><div><br></div><div>Duration manualShutter = 5ms;</div><div><br></div><div>If (manualShutter) {</div><div> /* do something */</div><div>}</div><div><br></div><div>But again, I am happy to create a Duration::zero() if you prefer. </div><div><br></div><div>Regards,</div><div>Naush</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>
All minors, the patch looks mostly good, maybe with BaseDuration made<br>
private...<br>
<br>
Thanks<br>
j<br>
<br>
<br>
> +<br>
> } /* namespace utils */<br>
><br>
> } /* namespace libcamera */<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>