[libcamera-devel] [PATCH 2/2] libcamera: Switch to the std::chrono API
Jacopo Mondi
jacopo at jmondi.org
Sat Sep 14 10:26:48 CEST 2019
Hi Laurent,
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).
>
> 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() ?
Thanks
j
> 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190914/5fb682b0/attachment.sig>
More information about the libcamera-devel
mailing list