[libcamera-devel] [PATCH v3 1/4] libcamera: utils: Add helper class for std::chrono::duration
Naushir Patuck
naush at raspberrypi.com
Fri May 21 13:44:26 CEST 2021
Hi Kieran,
On Fri, 21 May 2021 at 12:14, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> Hi Naush, David,
>
> On 21/05/2021 11:53, Naushir Patuck wrote:
> > Hi David,
> >
> > Thank you for your feedback.
> >
> > On Fri, 21 May 2021 at 11:20, David Plowman
> > <david.plowman at raspberrypi.com <mailto:david.plowman at raspberrypi.com>>
> > wrote:
> >
> > Hi Naush
> >
> > Thanks for the updated patch!
> >
> > On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush at raspberrypi.com
> > <mailto:naush at raspberrypi.com>> wrote:
> > >
> > > A new utils::Duration class is defined to represent a
> > > std::chrono::duration type with double precision nanosecond
> timebase.
> > > Using a double minimises the loss of precision when converting
> > timebases.
> > > This helper class may be used by IPAs to represent variables such
> > as frame
> > > durations and exposure times.
> > >
> > > An operator << overload is define to help with displaying
> > > utils::Duration value in stream objects. Currently, this will
> display
> > > the duration value in microseconds.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com
> > <mailto:naush at raspberrypi.com>>
> > > ---
> > > include/libcamera/internal/utils.h | 42
> > ++++++++++++++++++++++++++++++
> > > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++
> > > 2 files changed, 78 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/utils.h
> > b/include/libcamera/internal/utils.h
> > > index 83dada7cc16c..a377d4ea07ab 100644
> > > --- a/include/libcamera/internal/utils.h
> > > +++ b/include/libcamera/internal/utils.h
> > > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) ->
> > details::enumerate_adapter<T *>
> > > }
> > > #endif
> > >
> > > +using BaseDuration = std::chrono::duration<double, std::nano>;
> > > +
> > > +class Duration : public BaseDuration
> > > +{
> > > +public:
> > > + Duration() = default;
> > > +
> > > + template<typename Rep, typename Period>
> > > + constexpr Duration(const std::chrono::duration<Rep,
> > Period> &d)
> > > + : BaseDuration(d)
> > > + {
> > > + }
> > > +
> > > + template<typename Period>
> > > + double get() const
> > > + {
> > > + auto const c =
> > std::chrono::duration_cast<std::chrono::duration<double,
> > Period>>(*this);
> > > + return c.count();
> > > + }
> > > +
> > > + explicit constexpr operator bool() const
> > > + {
> > > + return *this != BaseDuration::zero();
> > > + }
> > > +};
> > > +
> > > +#ifndef __DOXYGEN__
> > > +template<class CharT, class Traits>
> > > +static inline std::basic_ostream<CharT, Traits>
> > &operator<<(std::basic_ostream<CharT, Traits> &os,
> > > + const
> > Duration &d)
> > > +{
> > > + std::basic_ostringstream<CharT, Traits> s;
> > > +
> > > + s.flags(os.flags());
> > > + s.imbue(os.getloc());
> > > + s.setf(std::ios_base::fixed, std::ios_base::floatfield);
> > > + s.precision(2);
> > > + s << d.get<std::micro>() << "us";
> > > + return os << s.str();
> > > +}
> > > +#endif
> >
> > One small thing I wonder about is that many classes have a toString
> > method. Do we want one here? It could even take an optional argument
> > to allow you to format the output differently... but I think that's
> > overkill and best left for now!
> >
> >
> > Did you mean that we should do something like:
> >
> > LOG(IPA, Debug) << "Exposure time is " << exposure.toString();
> >
> > over
> >
> > LOG(IPA, Debug) << "Exposure time is " << exposure;
> >
> > I chose the later way with the overload <<() operator as that is more
> > similar
> > to what C++20 will eventually allow with std::chrono::duration
> formatting.
> > What do others think?
>
> I think the main reason we've used toString() is so that we don't have
> to override operator<< outside of the libcamera namespace, so we've been
> more explicit with the toString().
>
> I don't mind the override or the explicit, but I think that's the
> reasoning.
>
> Would it be cleaner to implement a toString() which doesn't have to
> track the outputstream state, using a local stringstream perhaps and as
> that returns a string, the override operator<< can simply return that
> pre-prepared string?
>
I don't think it makes much of a difference in the implementation, as this
is exactly
what the operator<< overload does right now. There is an awkwardness in
having
to remember to do a "using libcamera::utils::operator<<" in every source
file
that wants to use the overload. Te toString() usage could eliminate that,
but then
it means typing more characters in your logging statement :-)
I do like the overload usage (apart from the using.. directive) in that it
sort of mirrors
what C++20 will allow.
Naush
>
>
> >
> > Regards,
> > Naush
> >
> >
> >
> >
> > > +
> > > } /* namespace utils */
> > >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > > index 826144d3c837..3131aa2d6666 100644
> > > --- a/src/libcamera/utils.cpp
> > > +++ b/src/libcamera/utils.cpp
> > > @@ -506,6 +506,42 @@ std::string libcameraSourcePath()
> > > * loop, iterates over an indexed view of the \a iterable
> > > */
> > >
> > > +/**
> > > + * \class Duration
> > > + * \brief Helper for std::chrono::duration types. Derived from \a
> > BaseDuration
> > > + */
> > > +
> > > +/**
> > > + * \fn Duration::Duration(const std::chrono::duration<Rep,
> > Period> &d)
> > > + * \brief Copy constructor from a \a std::chrono::duration type.
> > > + * \param[in] d The std::chrono::duration object to convert from.
> > > + *
> > > + * Constructs a \a Duration object from a \a
> > std::chrono::duration object,
> > > + * converting the representation to \a BaseDuration type.
> > > + */
> > > +
> > > +/**
> > > + * \fn Duration::get<Period>()
> > > + * \brief Retrieve the tick count, coverted to the timebase
> > provided by the
> >
> > s/coverted/converted/
> >
> > With this tiny fix:
> >
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com
> > <mailto:david.plowman at raspberrypi.com>>
> >
> > Thanks!
> > David
> >
> > > + * template argument Period of type \a std::ratio.
> > > + *
> > > + * A typical usage example is given below:
> > > + *
> > > + * \code{.cpp}
> > > + * utils::Duration d = 5s;
> > > + * double d_in_ms = d.get<std::milli>();
> > > + * \endcode
> > > + *
> > > + * \return Tick count
> > > + */
> > > +
> > > +/**
> > > + * \fn Duration::operator bool()
> > > + * \brief Boolean operator to test if a \a Duration holds a
> > non-zero time value.
> > > + *
> > > + * \return True if \a Duration is a non-zero time value, False
> > otherwise.
> > > + */
> > > +
> > > } /* namespace utils */
> > >
> > > } /* namespace libcamera */
> > > --
> > > 2.25.1
> > >
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210521/27177ed9/attachment-0001.htm>
More information about the libcamera-devel
mailing list