[libcamera-devel,v4,07/11] libcamera: timeline: Add a basic timeline implementation

2089 diff mbox series

The timeline is a helper for pipeline handlers to ease interacting with
an IPA. The idea is that the pipeline handler runs a timeline which
schedules and executes actions on the hardware. The pipeline listens to
signals from the IPA which it translates into actions which are schedule
on the timeline.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/include/meson.build |   1 +
 src/libcamera/include/timeline.h  |  71 ++++++++
 src/libcamera/meson.build         |   1 +
 src/libcamera/timeline.cpp        | 267 ++++++++++++++++++++++++++++++
 4 files changed, 340 insertions(+)
 create mode 100644 src/libcamera/include/timeline.h
 create mode 100644 src/libcamera/timeline.cpp

Hi Niklas,

Thank you for the patch.

On Thu, Oct 03, 2019 at 07:49:37PM +0200, Niklas Söderlund wrote:
> The timeline is a helper for pipeline handlers to ease interacting with
> an IPA.

It's not just a helper. Given that the IPA API is based around queuing
frame actions, the timeline is a central concept in the design.

> The idea is that the pipeline handler runs a timeline which
> schedules and executes actions on the hardware. The pipeline listens to
> signals from the IPA which it translates into actions which are schedule
> on the timeline.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/include/meson.build |   1 +
>  src/libcamera/include/timeline.h  |  71 ++++++++
>  src/libcamera/meson.build         |   1 +
>  src/libcamera/timeline.cpp        | 267 ++++++++++++++++++++++++++++++
>  4 files changed, 340 insertions(+)
>  create mode 100644 src/libcamera/include/timeline.h
>  create mode 100644 src/libcamera/timeline.cpp
> 
> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build
> index 2c74d29bd925ede2..238ddc2c2cf96a5d 100644
> --- a/src/libcamera/include/meson.build
> +++ b/src/libcamera/include/meson.build
> @@ -18,6 +18,7 @@ libcamera_headers = files([
>      'pipeline_handler.h',
>      'process.h',
>      'thread.h',
> +    'timeline.h',
>      'utils.h',
>      'v4l2_controls.h',
>      'v4l2_device.h',
> diff --git a/src/libcamera/include/timeline.h b/src/libcamera/include/timeline.h
> new file mode 100644
> index 0000000000000000..1ec28a0d9134d35c
> --- /dev/null
> +++ b/src/libcamera/include/timeline.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * timeline.h - Timeline for per-frame controls
> + */
> +#ifndef __LIBCAMERA_TIMELINE_H__
> +#define __LIBCAMERA_TIMELINE_H__
> +
> +#include <list>
> +#include <map>
> +
> +#include <libcamera/timer.h>
> +
> +#include "utils.h"
> +
> +namespace libcamera {
> +
> +class FrameAction
> +{
> +public:
> +	FrameAction(unsigned int type, unsigned int frame)
> +		: type_(type), frame_(frame) {}

Let's flip frame and type, here and in the methods and fields below.
frame is the most generic parameter, and within a frame, type further
qualifies the action.

> +
> +	virtual ~FrameAction() {}
> +
> +	unsigned int type() const { return type_; }
> +	unsigned int frame() const { return frame_; }
> +
> +	virtual void run() = 0;
> +
> +private:
> +	unsigned int type_;
> +	unsigned int frame_;
> +};
> +
> +class Timeline
> +{
> +public:
> +	Timeline();
> +	virtual ~Timeline() {}
> +
> +	virtual void reset();
> +	virtual void scheduleAction(FrameAction *action);
> +	virtual void notifyStartOfExposure(unsigned int frame, utils::time_point time);
> +
> +	utils::duration frameInterval() const { return frameInterval_; }

Is this method needed ?

> +
> +protected:
> +	int frameOffset(unsigned int type) const;
> +	utils::duration timeOffset(unsigned int type) const;
> +
> +	void setRawDelay(unsigned int type, int frame, utils::duration time);
> +
> +private:
> +	static constexpr unsigned int HistoryDepth = 10;

CamelCase is used for class names. You should use SNAKE_CASE for
constants.

> +
> +	void timeout(Timer *timer);
> +	void updateDeadline(const utils::time_point &deadline);

If time_point (and duration) are large enough to be passed as a const
reference, you should do the same above in all functions that take or
return such a value. Otherwise you should pass deadline by value here.

> +
> +	std::list<std::pair<unsigned int, utils::time_point>> history_;
> +	std::multimap<utils::time_point, FrameAction *> actions_;
> +	std::map<unsigned int, std::pair<int, utils::duration>> delays_;
> +	utils::duration frameInterval_;
> +
> +	Timer timer_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_TIMELINE_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 3a3b388a6a344961..195368e77e5e6f01 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -30,6 +30,7 @@ libcamera_sources = files([
>      'signal.cpp',
>      'stream.cpp',
>      'thread.cpp',
> +    'timeline.cpp',
>      'timer.cpp',
>      'utils.cpp',
>      'v4l2_controls.cpp',
> diff --git a/src/libcamera/timeline.cpp b/src/libcamera/timeline.cpp
> new file mode 100644
> index 0000000000000000..042ead41d2ff54d5
> --- /dev/null
> +++ b/src/libcamera/timeline.cpp
> @@ -0,0 +1,267 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * timeline.cpp - Timeline for per-frame controls
> + */
> +
> +#include "timeline.h"
> +
> +#include "log.h"

You're missing \file, which prevents the documentation from being
compiled.

> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Timeline)
> +
> +/**
> + * \class FrameAction
> + * \brief Action which can be schedule within a frame's lifetime
> + *
> + * A frame action is an event which takes place at some point during a frame's
> + * lifetime inside libcamera. Each action has two primal attributes; type and
> + * frame number.
> + *
> + * The type is a numerical ID which identifies the action within the pipeline
> + * handler. The type is pipeline specific and has no meaning outside the
> + * specific implementation. The frame number is the frame the action applies to.
> + */
> +
> +/**
> + * \fn FrameAction::FrameAction

s/$/()/

> + * \brief Create a frame action
> + * \param[in] type Action type
> + * \param[in] frame Frame number
> + */
> +
> +/**
> + * \fn FrameAction::type()
> + * \brief Retrieve the type of action
> + * \return Action type
> + */
> +
> +/**
> + * \fn FrameAction::frame()
> + * \brief Retrieve the frame number
> + * \return Frame number
> + */
> +
> +/**
> + * \fn FrameAction::run()
> + * \brief The action to perform when the action is triggered
> + *
> + * Pipeline handlers shall subclass the FrameAction object and implement run()

s/object/class/

> + * functions which describes the actions they wish to happen when the act is

s/functions/methods/

> + * schedule using the Timeline.
> + *
> + * \sa Timeline
> + */
> +
> +/**
> + * \class Timeline
> + * \brief Scheduler and executor of FrameAction's
> + *
> + * On the timeline the pipeline can schedule FrameAction and expect them to be
> + * executed at the correct time. The correct time to execute them is a sum
> + * of which frame the action should apply to and a list of timing delays for
> + * each action the pipeline handler describes.
> + *
> + * The timing delays can either be set by the pipeline handler or by the IPA.
> + * The exact implementation of how the timing delays are setup is pipeline
> + * specific. It is expected that the IPA will update the timing delays as it
> + * make changes to how the pipeline operates in different situations.
> + *
> + * The pipeline handler is responsible for feeding the timeline as accurate as
> + * possible information of the exposures it observes.
> + */
> +
> +Timeline::Timeline()
> +	: frameInterval_(std::chrono::milliseconds(1))

Why 1 ? I don't think we should have magic values.

> +{
> +	timer_.timeout.connect(this, &Timeline::timeout);
> +}
> +
> +/**
> + * \brief Reset the timeline

I wondered when reading the class definition above what the reset method
would do, am I'm none the wiser now :-S

> + */
> +void Timeline::reset()
> +{
> +	timer_.stop();
> +
> +	actions_.clear();
> +	history_.clear();
> +}
> +
> +/**
> + * \brief Add a action to the timeline
> + * \param[in] action FrameAction to add
> + *
> + * Schedule an action at a specific time taking the action and timing delays
> + * into account. If a action is schedule too late execute it immediately and
> + * try to recover.
> + */
> +void Timeline::scheduleAction(FrameAction *action)

Ownership of action isn't clear. Is it transferred to this function as I
believe it is ? If so you should use a unique pointer to make this
explicit.

> +{
> +	unsigned int lastFrame;
> +	utils::time_point lastTime;
> +
> +	if (history_.empty()) {
> +		/*
> +		 * This only happens for the first number of frames, up to
> +		 * pipeline depth.
> +		 */
> +		lastFrame = 0;
> +		lastTime = std::chrono::steady_clock::now();
> +	} else {
> +		lastFrame = history_.back().first;
> +		lastTime = history_.back().second;
> +	}
> +
> +	/*
> +	 * Calculate action trigger time by first figuring the out the start of
> +	 * exposure (SOE) of the frame the action corresponds to and then adding
> +	 * the frame and timing offsets.
> +	 */
> +	int frame = action->frame() + frameOffset(action->type()) - lastFrame;
> +	utils::time_point deadline = lastTime + frame * frameInterval_
> +		+ timeOffset(action->type());
> +
> +	utils::time_point now = std::chrono::steady_clock::now();
> +	if (deadline < now) {
> +		LOG(Timeline, Warning)
> +			<< "Action scheduled too late "
> +			<< utils::time_point_to_string(deadline)
> +			<< ", run now " << utils::time_point_to_string(now);
> +		action->run();

action is leaked.

> +	} else {
> +		actions_.insert({ deadline, action });
> +		updateDeadline(deadline);
> +	}
> +}
> +
> +/**
> + * \brief Inform timeline of a new exposure
> + * \param[in] frame Frame number of the exposure
> + * \param[in] time The best approximation of when the exposure started
> + */
> +void Timeline::notifyStartOfExposure(unsigned int frame, utils::time_point time)
> +{
> +	history_.push_back(std::make_pair(frame, time));
> +
> +	if (history_.size() <= HistoryDepth / 2)
> +		return;
> +
> +	while (history_.size() > HistoryDepth)
> +		history_.pop_front();
> +
> +	/* Update esitmated time between two start of exposures. */
> +	utils::duration sumExposures = std::chrono::duration_values<std::chrono::nanoseconds>::zero();


	utils::duration sumExposures(0);

compiles, does it do the right thing ?

> +	unsigned int numExposures = 0;
> +
> +	utils::time_point lastTime;
> +	for (auto it = history_.begin(); it != history_.end(); it++) {
> +		if (it != history_.begin()) {
> +			sumExposures += it->second - lastTime;
> +			numExposures++;
> +		}
> +
> +		lastTime = it->second;
> +	}
> +
> +	frameInterval_ = sumExposures;
> +	if (numExposures)
> +		frameInterval_ /= numExposures;

This algorithm is going to produce an awful jitter, and I'm not even
sure it will give a correct average value.

> +}
> +
> +/**
> + * \fn Timeline::frameInterval()
> + * \brief Retrieve the best estimate of the frame interval
> + * \return Frame interval estimate
> + */
> +
> +/**
> + * \brief Retrieve the frame offset for an action type
> + * \param[in] type Action type
> + * \return Frame offset for the action type
> + */
> +int Timeline::frameOffset(unsigned int type) const
> +{
> +	const auto it = delays_.find(type);
> +	if (it == delays_.end()) {
> +		LOG(Timeline, Error)
> +			<< "No frame offset set for action type " << type;
> +		return 0;
> +	}
> +
> +	return it->second.first;
> +}
> +
> +/**
> + * \brief Retrieve the time offset for an action type
> + * \param[in] type Action type
> + * \return Time offset for the action type
> + */
> +utils::duration Timeline::timeOffset(unsigned int type) const
> +{
> +	const auto it = delays_.find(type);
> +	if (it == delays_.end()) {
> +		LOG(Timeline, Error)
> +			<< "No time offset set for action type " << type;
> +		return utils::duration::zero();
> +	}
> +
> +	return it->second.second;
> +}
> +
> +/**
> + * \brief Set the frame and time offset delays for an action type
> + * \param[in] type Action type
> + * \param[in] frame Frame offset
> + * \param[in] time Time offset
> + */
> +void Timeline::setRawDelay(unsigned int type, int frame, utils::duration time)
> +{
> +	delays_[type] = std::make_pair(frame, time);
> +}
> +
> +void Timeline::updateDeadline(const utils::time_point &deadline)
> +{
> +	if (timer_.isRunning() && deadline >= timer_.deadline())
> +		return;
> +
> +	utils::time_point now = std::chrono::steady_clock::now();
> +	utils::duration duration = deadline - now;
> +
> +	/*
> +	 * Translate nanoseconds resolution the timeline to milliseconds using
> +	 * a ceiling approach to not trigger an action before it's scheduled.

	/*
	 * Convert the duration to milliseconds. Round up to avoid
	 * trigerring the action before it should be scheduled.
	 */

> +	 *
> +	 * \todo: Should the Timer operate using nanoseconds?

I think Timer should have an overloaded start() method that takes a time
point, as the first thing start() does is converting the duration to a
time point.

> +	 */
> +	unsigned long nsecs = std::chrono::duration_cast<std::chrono::nanoseconds>(duration).count();
> +	unsigned int msecs = nsecs / 1000000 + (nsecs % 1000000 ? 1 : 0);

Should we add an implementation of
https://en.cppreference.com/w/cpp/chrono/duration/ceil to utils ?

> +
> +	timer_.stop();
> +	timer_.start(msecs);
> +}
> +
> +void Timeline::timeout(Timer *timer)
> +{
> +	for (auto it = actions_.begin(); it != actions_.end();) {
> +		utils::time_point now = std::chrono::steady_clock::now();

Can this be moved outside of the loop ?

> +
> +		utils::time_point sched = it->first;

const reference ?

> +		FrameAction *action = it->second;

This can be moved after the if ().

> +
> +		if (sched >= now) {
> +			updateDeadline(sched);

I would move this after the for loop.

> +			break;
> +		}
> +
> +		action->run();
> +
> +		it = actions_.erase(it);
> +		delete action;

The delete won't be needed anymore if you store unique pointers.

> +	}

	updateDeadline();

with no argument here, and use actions_.front() to retrieve the next
action to schedule in updateDeadline() (after a check for empty() of
course).

> +}
> +
> +} /* namespace libcamera */