<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 19 May 2021 at 16:38, 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>
On Wed, May 19, 2021 at 04:21:04PM +0100, Naushir Patuck wrote:<br>
> On Wed, 19 May 2021 at 15:48, Laurent Pinchart wrote:<br>
> > On Wed, May 19, 2021 at 03:34:05PM +0100, Naushir Patuck wrote:<br>
> > > On Wed, 19 May 2021 at 15:24, Laurent Pinchart wrote:<br>
> > > > On Tue, May 18, 2021 at 11:07:03AM +0100, Naushir Patuck wrote:<br>
> > > > > A new RPiController::Duration typedef is defined to represent a<br>
> > > > > std::chrono::duration type with double precision nanosecond timebase<br>
> > > > > that will be used throughout. This minimises the loss of precision when<br>
> > > > > converting timebases. A function for returning the duration count in any<br>
> > > > > timebase is also provided.<br>
> > > > ><br>
> > > > > An operator << overload is define to help with displaying<br>
> > > > > RPiController::Duration value in stream objects.<br>
> > > > ><br>
> > > > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > > > ---<br>
> > > > > src/ipa/raspberrypi/controller/duration.hpp | 33 +++++++++++++++++++++<br>
> > > > > 1 file changed, 33 insertions(+)<br>
> > > > > create mode 100644 src/ipa/raspberrypi/controller/duration.hpp<br>
> > > > ><br>
> > > > > diff --git a/src/ipa/raspberrypi/controller/duration.hpp b/src/ipa/raspberrypi/controller/duration.hpp<br>
> > > > > new file mode 100644<br>
> > > > > index 000000000000..98aa3d78f52f<br>
> > > > > --- /dev/null<br>
> > > > > +++ b/src/ipa/raspberrypi/controller/duration.hpp<br>
> > > > > @@ -0,0 +1,33 @@<br>
> > > > > +/* SPDX-License-Identifier: BSD-2-Clause */<br>
> > > > > +/*<br>
> > > > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited<br>
> > > > > + *<br>
> > > > > + * duration.hpp - Helper macros for std::chrono::duration handling.<br>
> > > > > + */<br>
> > > > > +#pragma once<br>
> > > > > +<br>
> > > > > +#include <chrono><br>
> > > > > +#include <iomanip><br>
> > > > > +#include <iostream><br>
> > > > > +<br>
> > > > > +namespace RPiController {<br>
> > > > > +<br>
> > > > > +using Duration = std::chrono::duration<double, std::nano>;<br>
> > > > > +<br>
> > > > > +// Helper to convert and return the count of any std::chrono::duration type to<br>
> > > > > +// another timebase. Use a double rep type to try and preserve precision.<br>
> > > > > +template <typename P, typename T><br>
> > > > > +static constexpr double DurationValue(T const &d)<br>
> > > > > +{<br>
> > > > > + return std::chrono::duration_cast<std::chrono::duration<double, P>>(d).count();<br>
> > > > > +};<br>
> > > > > +<br>
> > > > > +static inline std::ostream &operator<<(std::ostream &os, const Duration &duration)<br>
> > > > > +{<br>
> > > > > + std::streamsize ss = os.precision();<br>
> > > > > + os << std::fixed << std::setprecision(2) << DurationValue<std::micro>(duration) << " us";<br>
> > > > > + os << std::setprecision(ss) << std::defaultfloat;<br>
> > > ><br>
> > > > This will reset to std::defaultfloat, while the stream may already be in<br>
> > > > std::fixed.<br>
> > ><br>
> > > Good point, I should save and reapply like I do with precision.<br>
> > ><br>
> > > > > + return os;<br>
> > > > > +}<br>
> > > ><br>
> > > > Would it be too much yak-shaving to ask for the implementation to be<br>
> > > > closer to<br>
> > > > <a href="https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt</a>, and<br>
> > > > moved to utils.h ? It could be useful more widely in libcamera than just<br>
> > > > in the RPi IPA.<br>
> > ><br>
> > > Yes, I could look into that. Did you mean move the whole implementation into utils.h, or<br>
> > > only the operator <<() bit?<br>
> ><br>
> > I meant operator<<(). DurationValue would likely need to go there too if<br>
> > it's used internally, but the implementation of operator<<() could also<br>
> > hardcode it as<br>
> ><br>
> > os << std::fixed << std::setprecision(2) << duration.count() / 1000.0 << " us";<br>
> ><br>
> > if needed.<br>
> ><br>
> > > Actually, I would like to make an impactful change in the next<br>
> > > revision of the series, and replace Duration with a class derived from a std::chrono::duration<br>
> > > object. This is a bit neater in terms of encapsulation, and allows me to implement an<br>
> > > operator bool() method that cleans up the code a bit more. Functionally, everything else<br>
> > > remains identical. Did you want me to put that into utils.h?<br>
> ><br>
> > I think it should actually go to a public header if we want to<br>
> > standardize all timestamps and durations to std::chrono, but that's too<br>
> > much yak-shaving :-) We can handle it later.<br>
> ><br>
> > We already have the following types in utils.h:<br>
> ><br>
> > using clock = std::chrono::steady_clock;<br>
> > using duration = std::chrono::steady_clock::duration;<br>
> > using time_point = std::chrono::steady_clock::time_point;<br>
> ><br>
> > There are also a few helpers to perform string conversion and to convert<br>
> > to a timespec. Ideally all of this should be consolidated, but again<br>
> > it's propably a bit too much work. If we can make operator<<() conform<br>
> > to the C++20 definition to be able to later switch to it, I'll be happy<br>
> > already.<br>
> <br>
> Here's what I think I will do:<br>
> <br>
> I will update patch 1/4 with a new Duration class, now defined in utils.h.<br>
> This will include<br>
> operator <<() based on the C++20 signature from:<br>
> <a href="https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt</a><br>
<br>
Sounds good.<br>
<br>
> The only thing I would change from the C++20 functionally is to hardcode it to output in<br>
> microseconds rather than taking the base unit of the Duration class (which is nanoseconds).<br>
> For my current usage, us makes more sense than ns, but I do accept that for timestamps, etc.<br>
> ns may be more appropriate. Perhaps some further specialization is needed?<br>
<br>
Given that you print the value as a floating point number, would it make<br>
sense to print it in seconds ?<br></blockquote><div><br></div><div>My personal preference would be to keep it in microseconds, as I find it easier to read the</div><div>values that relate to exposure, frame durations, etc. However, I am not strongly opposed to</div><div>changing it if the consensus goes the other way.</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 the existing std::chrono definitions will remain as-is and we can<br>
> convert to Duration in due<br>
> course.<br>
> <br>
> Let me know if you agree/disagree with the above.<br>
> <br>
> > > Sorry David, your review for patch 1/4 may need redoing with :-(<br>
> > ><br>
> > > > > +<br>
> > > > > +} // namespace RPiController<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>