[libcamera-devel] [PATCH 2/2] libcamera: Switch to the std::chrono API
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Sep 14 13:04:17 CEST 2019
Hi Jacopo,
On Sat, Sep 14, 2019 at 10:26:48AM +0200, Jacopo Mondi wrote:
> On Sat, Sep 14, 2019 at 03:54:57AM +0300, Laurent Pinchart wrote:
> > Replace the clock_gettime()-based API with durations expressed as
> > integers with the std::chrono API.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/timer.h | 12 +++++----
> > src/cam/capture.cpp | 15 +++++------
> > src/cam/capture.h | 3 ++-
> > src/libcamera/event_dispatcher_poll.cpp | 22 ++++++-----------
> > src/libcamera/include/log.h | 7 ++++--
> > src/libcamera/log.cpp | 20 +++------------
> > src/libcamera/timer.cpp | 33 +++++++++++++++----------
> > test/event-dispatcher.cpp | 13 +++++-----
> > test/timer.cpp | 17 +++++++------
> > 9 files changed, 67 insertions(+), 75 deletions(-)
> >
> > diff --git a/include/libcamera/timer.h b/include/libcamera/timer.h
> > index f47b6a58404f..476ae45f1e53 100644
> > --- a/include/libcamera/timer.h
> > +++ b/include/libcamera/timer.h
> > @@ -7,6 +7,7 @@
> > #ifndef __LIBCAMERA_TIMER_H__
> > #define __LIBCAMERA_TIMER_H__
> >
> > +#include <chrono>
> > #include <cstdint>
> >
> > #include <libcamera/object.h>
> > @@ -22,12 +23,13 @@ public:
> > Timer(Object *parent = nullptr);
> > ~Timer();
> >
> > - void start(unsigned int msec);
> > + void start(unsigned int msec) { start(std::chrono::milliseconds(msec)); }
> > + void start(std::chrono::milliseconds interval);
> > void stop();
> > bool isRunning() const;
> >
> > - unsigned int interval() const { return interval_; }
> > - uint64_t deadline() const { return deadline_; }
> > + std::chrono::milliseconds interval() const { return interval_; }
> > + std::chrono::steady_clock::time_point deadline() const { return deadline_; }
>
> I wonder if the type definitions you added to utils (timepoint, clock,
> duration) aren't better suited for this header so that applications could
> use them (with proper namespacing indeed).
I've thought about it, but I would prefer avoiding extending the public
API. The only reason why the timer (and event notifier) class is exposed
is to allow applications to implement custom event dispatchers.
> >
> > Signal<Timer *> timeout;
> >
> > @@ -38,8 +40,8 @@ private:
> > void registerTimer();
> > void unregisterTimer();
> >
> > - unsigned int interval_;
> > - uint64_t deadline_;
> > + std::chrono::milliseconds interval_;
> > + std::chrono::steady_clock::time_point deadline_;
> > };
> >
> > } /* namespace libcamera */
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index df9602de4ab8..8a939c622703 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -5,6 +5,7 @@
> > * capture.cpp - Cam capture
> > */
> >
> > +#include <chrono>
> > #include <climits>
> > #include <iomanip>
> > #include <iostream>
> > @@ -16,7 +17,7 @@
> > using namespace libcamera;
> >
> > Capture::Capture(Camera *camera, CameraConfiguration *config)
> > - : camera_(camera), config_(config), writer_(nullptr), last_(0)
> > + : camera_(camera), config_(config), writer_(nullptr)
> > {
> > }
> >
> > @@ -135,17 +136,13 @@ int Capture::capture(EventLoop *loop)
> >
> > void Capture::requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> > {
> > - double fps = 0.0;
> > - uint64_t now;
> > -
> > if (request->status() == Request::RequestCancelled)
> > return;
> >
> > - struct timespec time;
> > - clock_gettime(CLOCK_MONOTONIC, &time);
> > - now = time.tv_sec * 1000 + time.tv_nsec / 1000000;
> > - fps = now - last_;
> > - fps = last_ && fps ? 1000.0 / fps : 0.0;
> > + std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now();
> > + double fps = std::chrono::duration_cast<std::chrono::milliseconds>(now - last_).count();
> > + fps = last_ != std::chrono::steady_clock::time_point() && fps
> > + ? 1000.0 / fps : 0.0;
> > last_ = now;
> >
> > std::stringstream info;
> > diff --git a/src/cam/capture.h b/src/cam/capture.h
> > index 1d4a25a84a51..ee0dc4211111 100644
> > --- a/src/cam/capture.h
> > +++ b/src/cam/capture.h
> > @@ -7,6 +7,7 @@
> > #ifndef __CAM_CAPTURE_H__
> > #define __CAM_CAPTURE_H__
> >
> > +#include <chrono>
> > #include <memory>
> >
> > #include <libcamera/camera.h>
> > @@ -35,7 +36,7 @@ private:
> >
> > std::map<libcamera::Stream *, std::string> streamName_;
> > BufferWriter *writer_;
> > - uint64_t last_;
> > + std::chrono::steady_clock::time_point last_;
> > };
> >
> > #endif /* __CAM_CAPTURE_H__ */
> > diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp
> > index 281f37bdbb16..51ac5adf2f74 100644
> > --- a/src/libcamera/event_dispatcher_poll.cpp
> > +++ b/src/libcamera/event_dispatcher_poll.cpp
> > @@ -8,6 +8,7 @@
> > #include "event_dispatcher_poll.h"
> >
> > #include <algorithm>
> > +#include <chrono>
> > #include <iomanip>
> > #include <poll.h>
> > #include <stdint.h>
> > @@ -20,6 +21,7 @@
> >
> > #include "log.h"
> > #include "thread.h"
> > +#include "utils.h"
> >
> > /**
> > * \file event_dispatcher_poll.h
> > @@ -206,17 +208,12 @@ int EventDispatcherPoll::poll(std::vector<struct pollfd> *pollfds)
> > struct timespec timeout;
> >
> > if (nextTimer) {
> > - clock_gettime(CLOCK_MONOTONIC, &timeout);
> > - uint64_t now = timeout.tv_sec * 1000000000ULL + timeout.tv_nsec;
> > + utils::time_point now = utils::clock::now();
> >
> > - if (nextTimer->deadline() > now) {
> > - uint64_t delta = nextTimer->deadline() - now;
> > - timeout.tv_sec = delta / 1000000000ULL;
> > - timeout.tv_nsec = delta % 1000000000ULL;
> > - } else {
> > - timeout.tv_sec = 0;
> > - timeout.tv_nsec = 0;
> > - }
> > + if (nextTimer->deadline() > now)
> > + timeout = utils::duration_to_timespec(nextTimer->deadline() - now);
> > + else
> > + timeout = { 0, 0 };
> >
> > LOG(Event, Debug)
> > << "timeout " << timeout.tv_sec << "."
> > @@ -295,10 +292,7 @@ void EventDispatcherPoll::processNotifiers(const std::vector<struct pollfd> &pol
> >
> > void EventDispatcherPoll::processTimers()
> > {
> > - struct timespec ts;
> > - uint64_t now;
> > - clock_gettime(CLOCK_MONOTONIC, &ts);
> > - now = ts.tv_sec * 1000000000ULL + ts.tv_nsec;
> > + utils::time_point now = utils::clock::now();
> >
> > while (!timers_.empty()) {
> > Timer *timer = timers_.front();
> > diff --git a/src/libcamera/include/log.h b/src/libcamera/include/log.h
> > index 9b203f97e304..ee0b4069bd32 100644
> > --- a/src/libcamera/include/log.h
> > +++ b/src/libcamera/include/log.h
> > @@ -7,8 +7,11 @@
> > #ifndef __LIBCAMERA_LOG_H__
> > #define __LIBCAMERA_LOG_H__
> >
> > +#include <chrono>
> > #include <sstream>
> >
> > +#include "utils.h"
> > +
> > namespace libcamera {
> >
> > enum LogSeverity {
> > @@ -60,7 +63,7 @@ public:
> >
> > std::ostream &stream() { return msgStream_; }
> >
> > - const struct timespec ×tamp() const { return timestamp_; }
> > + const utils::time_point ×tamp() const { return timestamp_; }
> > LogSeverity severity() const { return severity_; }
> > const LogCategory &category() const { return category_; }
> > const std::string &fileInfo() const { return fileInfo_; }
> > @@ -72,7 +75,7 @@ private:
> > std::ostringstream msgStream_;
> > const LogCategory &category_;
> > LogSeverity severity_;
> > - struct timespec timestamp_;
> > + utils::time_point timestamp_;
> > std::string fileInfo_;
> > };
> >
> > diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> > index 91f7c3ee5157..51f9f86b4c44 100644
> > --- a/src/libcamera/log.cpp
> > +++ b/src/libcamera/log.cpp
> > @@ -11,7 +11,6 @@
> > #include <cstdlib>
> > #include <ctime>
> > #include <fstream>
> > -#include <iomanip>
> > #include <iostream>
> > #include <list>
> > #include <string.h>
> > @@ -78,17 +77,6 @@ static int log_severity_to_syslog(LogSeverity severity)
> > }
> > }
> >
> > -static std::string log_timespec_to_string(const struct timespec ×tamp)
> > -{
> > - std::ostringstream ossTimestamp;
> > - ossTimestamp.fill('0');
> > - ossTimestamp << "[" << timestamp.tv_sec / (60 * 60) << ":"
> > - << std::setw(2) << (timestamp.tv_sec / 60) % 60 << ":"
> > - << std::setw(2) << timestamp.tv_sec % 60 << "."
> > - << std::setw(9) << timestamp.tv_nsec << "]";
> > - return ossTimestamp.str();
> > -}
> > -
> > static const char *log_severity_name(LogSeverity severity)
> > {
> > static const char *const names[] = {
> > @@ -216,10 +204,10 @@ void LogOutput::writeSyslog(const LogMessage &msg)
> >
> > void LogOutput::writeStream(const LogMessage &msg)
> > {
> > - std::string str = std::string(log_timespec_to_string(msg.timestamp()) +
> > - log_severity_name(msg.severity()) + " " +
> > + std::string str = "[" + utils::time_point_to_string(msg.timestamp()) +
> > + "]" + log_severity_name(msg.severity()) + " " +
> > msg.category().name() + " " + msg.fileInfo() + " " +
> > - msg.msg());
> > + msg.msg();
> > stream_->write(str.c_str(), str.size());
> > stream_->flush();
> > }
> > @@ -777,7 +765,7 @@ LogMessage::LogMessage(LogMessage &&other)
> > void LogMessage::init(const char *fileName, unsigned int line)
> > {
> > /* Log the timestamp, severity and file information. */
> > - clock_gettime(CLOCK_MONOTONIC, ×tamp_);
> > + timestamp_ = utils::clock::now();
> >
> > std::ostringstream ossFileInfo;
> > ossFileInfo << utils::basename(fileName) << ":" << line;
> > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp
> > index c61d77e5128f..fe47a348cff9 100644
> > --- a/src/libcamera/timer.cpp
> > +++ b/src/libcamera/timer.cpp
> > @@ -7,7 +7,7 @@
> >
> > #include <libcamera/timer.h>
> >
> > -#include <time.h>
> > +#include <chrono>
> >
> > #include <libcamera/camera_manager.h>
> > #include <libcamera/event_dispatcher.h>
> > @@ -15,6 +15,7 @@
> > #include "log.h"
> > #include "message.h"
> > #include "thread.h"
> > +#include "utils.h"
> >
> > /**
> > * \file timer.h
> > @@ -42,7 +43,7 @@ LOG_DEFINE_CATEGORY(Timer)
> > * \param[in] parent The parent Object
> > */
> > Timer::Timer(Object *parent)
> > - : Object(parent), interval_(0), deadline_(0)
> > + : Object(parent)
> > {
> > }
> >
> > @@ -52,22 +53,28 @@ Timer::~Timer()
> > }
> >
> > /**
> > + * \fn Timer::start(unsigned int msec)
> > * \brief Start or restart the timer with a timeout of \a msec
> > * \param[in] msec The timer duration in milliseconds
> > *
> > * If the timer is already running it will be stopped and restarted.
> > */
> > -void Timer::start(unsigned int msec)
> > -{
> > - struct timespec tp;
> > - clock_gettime(CLOCK_MONOTONIC, &tp);
> >
> > - interval_ = msec;
> > - deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000ULL;
> > +/**
> > + * \brief Start or restart the timer with a timeout of \a interval
> > + * \param[in] interval The timer duration in milliseconds
> > + *
> > + * If the timer is already running it will be stopped and restarted.
> > + */
> > +void Timer::start(std::chrono::milliseconds interval)
> > +{
> > + interval_ = interval;
> > + deadline_ = utils::clock::now() + interval;
> >
> > LOG(Timer, Debug)
> > << "Starting timer " << this << " with interval "
> > - << msec << ": deadline " << deadline_;
> > + << interval.count() << ": deadline "
> > + << utils::time_point_to_string(deadline_);
> >
> > registerTimer();
> > }
> > @@ -84,7 +91,7 @@ void Timer::stop()
> > {
> > unregisterTimer();
> >
> > - deadline_ = 0;
> > + deadline_ = utils::time_point();
> > }
> >
> > void Timer::registerTimer()
> > @@ -103,7 +110,7 @@ void Timer::unregisterTimer()
> > */
> > bool Timer::isRunning() const
> > {
> > - return deadline_ != 0;
> > + return deadline_ != utils::time_point();
> > }
> >
> > /**
> > @@ -115,7 +122,7 @@ bool Timer::isRunning() const
> > /**
> > * \fn Timer::deadline()
> > * \brief Retrieve the timer deadline
> > - * \return The timer deadline in nanoseconds
> > + * \return The timer deadline
> > */
> >
> > /**
> > @@ -128,7 +135,7 @@ bool Timer::isRunning() const
> > void Timer::message(Message *msg)
> > {
> > if (msg->type() == Message::ThreadMoveMessage) {
> > - if (deadline_) {
> > + if (deadline_ != utils::time_point()) {
>
> Nit:
> isRunning() ?
Good point.
> > unregisterTimer();
> > invokeMethod(&Timer::registerTimer);
> > }
> > diff --git a/test/event-dispatcher.cpp b/test/event-dispatcher.cpp
> > index f243ec39bc28..9f9cf17818f2 100644
> > --- a/test/event-dispatcher.cpp
> > +++ b/test/event-dispatcher.cpp
> > @@ -5,6 +5,7 @@
> > * event-dispatcher.cpp - Event dispatcher test
> > */
> >
> > +#include <chrono>
> > #include <iostream>
> > #include <signal.h>
> > #include <sys/time.h>
> > @@ -47,8 +48,7 @@ protected:
> > Timer timer;
> >
> > /* Event processing interruption by signal. */
> > - struct timespec start;
> > - clock_gettime(CLOCK_MONOTONIC, &start);
> > + std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now();
> >
> > timer.start(1000);
> >
> > @@ -59,12 +59,11 @@ protected:
> >
> > dispatcher->processEvents();
> >
> > - struct timespec stop;
> > - clock_gettime(CLOCK_MONOTONIC, &stop);
> > - int duration = (stop.tv_sec - start.tv_sec) * 1000;
> > - duration += (stop.tv_nsec - start.tv_nsec) / 1000000;
> > + std::chrono::steady_clock::time_point stop = std::chrono::steady_clock::now();
> > + std::chrono::steady_clock::duration duration = stop - start;
> > + int msecs = std::chrono::duration_cast<std::chrono::milliseconds>(duration).count();
> >
> > - if (abs(duration - 1000) > 50) {
> > + if (abs(msecs - 1000) > 50) {
> > cout << "Event processing restart test failed" << endl;
> > return TestFail;
> > }
> > diff --git a/test/timer.cpp b/test/timer.cpp
> > index c30709d4109a..af922cb371cd 100644
> > --- a/test/timer.cpp
> > +++ b/test/timer.cpp
> > @@ -5,6 +5,7 @@
> > * timer.cpp - Timer test
> > */
> >
> > +#include <chrono>
> > #include <iostream>
> >
> > #include <libcamera/event_dispatcher.h>
> > @@ -28,28 +29,28 @@ public:
> > void start(int msec)
> > {
> > interval_ = msec;
> > - clock_gettime(CLOCK_MONOTONIC, &start_);
> > - expiration_ = { 0, 0 };
> > + start_ = std::chrono::steady_clock::now();
> > + expiration_ = std::chrono::steady_clock::time_point();
> >
> > Timer::start(msec);
> > }
> >
> > int jitter()
> > {
> > - int duration = (expiration_.tv_sec - start_.tv_sec) * 1000;
> > - duration += (expiration_.tv_nsec - start_.tv_nsec) / 1000000;
> > - return abs(duration - interval_);
> > + std::chrono::steady_clock::duration duration = expiration_ - start_;
> > + int msecs = std::chrono::duration_cast<std::chrono::milliseconds>(duration).count();
> > + return abs(msecs - interval_);
> > }
> >
> > private:
> > void timeoutHandler(Timer *timer)
> > {
> > - clock_gettime(CLOCK_MONOTONIC, &expiration_);
> > + expiration_ = std::chrono::steady_clock::now();
> > }
> >
> > int interval_;
> > - struct timespec start_;
> > - struct timespec expiration_;
> > + std::chrono::steady_clock::time_point start_;
> > + std::chrono::steady_clock::time_point expiration_;
> > };
> >
> > class TimerTest : public Test
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list