[libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Add helper macros for std::chrono::duration

[Naushir Patuck]

On Wed, 19 May 2021 at 16:38, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

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

My personal preference would be to keep it in microseconds, as I find it
easier to read the
values that relate to exposure, frame durations, etc.  However, I am not
strongly opposed to
changing it if the consensus goes the other way.


>
> > All the existing std::chrono definitions will remain as-is and we can
> > convert to Duration in due
> > course.
> >
> > Let me know if you agree/disagree with the above.
> >
> > > > Sorry David, your review for patch 1/4 may need redoing with :-(
> > > >
> > > > > > +
> > > > > > +} // namespace RPiController
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210520/326b257f/attachment-0001.htm>