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

[Naushir Patuck]

Hi Laurent,


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

> Hi Naush,
>
> 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

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?

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.

Regards,
Naush


>
> > 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/20210519/1736937b/attachment.htm>