[libcamera-devel,v3,09/13] libcamera: timeline: Add a basic timeline implementation

2033 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,
   just a few spelling comments before I really grasp the content of
   the series

On Fri, Sep 27, 2019 at 04:44:13AM +0200, Niklas Söderlund wrote:
> 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
>
> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build
> index 933be8543a8d5f02..fc6402b6ffb3d47c 100644
> --- a/src/libcamera/include/meson.build
> +++ b/src/libcamera/include/meson.build
> @@ -16,6 +16,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..519caf5a923f35a7
> --- /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) {}
> +
> +	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);

Is it worth to shorten these ? A scheduleAction() that takes a
FrameAction could be called just 'schedule()' in example.

> +
> +	utils::duration frameInterval() const { return frameInterval_; }
> +
> +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;

Capital H as it's a constexpr ?

> +
> +	void timeout(Timer *timer);
> +	void updateDeadline(const utils::time_point &deadline);
> +
> +	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 755149c55c7b4f84..fef2e8619a42e053 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -28,6 +28,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..b5a713abbf3235eb
> --- /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"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Timeline)
> +
> +/**
> + * \class FrameAction
> + * \brief Action which can be schedule within a frames lifetime

frames/frame

> + *
> + * A frame action is a event which takes place at some point during a frames
an event
frames/frame

> + * lifetime inside libcamera. Each action have two primal attributes; type and

has

> + * frame number.
> + *
> + * The type is a numerical ID which identifies the action within the pipeline
> + * handler. The type is pipeline specific and have no meaning to anyone outside

has

/to anyone//

> + * the specific implementation. The frame number is the frame the action applies
> + * to.
> + */
> +
> +/**
> + * \fn FrameAction::FrameAction
> + * \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()
> + * functions which describes the actions they wish to happen when the act is
> + * schedule using the Timeline.
> + *
> + * \sa Timeline
> + */
> +
> +/**
> + * \class Timeline
> + * \brief Scheduler and executor of FrameAction's
> + *
> + * On the timeline the pipeline can schedule FrameActions and expect them to be

we usually don't pluralize class names: FrameAction instances

> + * executed at the correct time. The correct time to execute them are a sum

them are/them is

> + * 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 the IPA. The

or by the IPA

> + * exact implementation of how the timing delays are setup are pipeline

are pipeline/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.

This is not clear to me. How are timings updated ? Could you mention
the method?

> + *
> + * The pipeline handler is responsible for feeding the timeline as accurate as
> + * possible information of the exposures it observes.

Could you mention how? By notifying start of exposure? Are there other
events?

> + */
> +
> +Timeline::Timeline()
> +	: frameInterval_(std::chrono::milliseconds(1))
> +{
> +	timer_.timeout.connect(this, &Timeline::timeout);
> +}
> +
> +/**
> + * \brief Reset the timeline
> + */
> +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)
> +{
> +	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;

I'm not sure I get what frameOffset is.

Each action has a frame numer it applied to (action->frame())
The timeline has a setRawDealy() which takes a frame offset and a
delay (and is currenly never used by the RkISP IPA if I'm not mistaken,
but that's a different issue). If an action alreay has frame number it
applies to what is the offset for ?

Could you document it? It is reported as just 'frame offset'
everywhere it is used.

Should we make sure we didn't get out of sync and the resulting
'frame' integer is > current frame ?

> +	utils::time_point deadline = lastTime + frame * frameInterval_ + timeOffset(action->type());

Nit: could you break this line ?

> +
> +	utils::time_point now = std::chrono::steady_clock::now();
> +	if (deadline < now) {
> +		LOG(Timeline, Warning)
> +			<< "Action schedule too late "

scheduled

> +			<< utils::time_point_to_string(deadline)
> +			<< ", run now " << utils::time_point_to_string(now);
> +		action->run();
> +	} 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();
> +	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;
> +}
> +
> +/**
> + * \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;
> +}

Those two operations are identical, you just return second or first.
But I agree they're trivial and could stay the same for now.

> +
> +/**
> + * \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

Could you spend a few more words about the actual meaning of those parameters
and the role they play in calculating the delays in the timeline, here
ot in the class description.

> + */
> +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.
> +	 *
> +	 * \todo: Should the Timer operate using nanoseconds?
> +	 */
> +	unsigned long nsecs = std::chrono::duration_cast<std::chrono::nanoseconds>(duration).count();
> +	unsigned int msecs = nsecs / 1000000 + (nsecs % 1000000 ? 1 : 0);
> +
> +	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();
> +
> +		utils::time_point sched = it->first;
> +		FrameAction *action = it->second;
> +
> +		if (sched >= now) {
> +			updateDeadline(sched);

Am I reading it wrong or as soon as you find an action which is
scheduled in the future you program a timeout then exit the loop ? Are
actions_ sorted? Shouldn't you find the closer deadline and program
the sleep interval using that?

> +			break;
> +		}
> +
> +		action->run();

I would also keep the action currenly "in sleep" somewhere, and update
it at updateDeadline() time, so you can program a new sleep after
having erased the action to run, and then run it directly outside of this
for loop.

> +
> +		it = actions_.erase(it);
> +		delete action;
> +	}
> +}
> +
> +} /* namespace libcamera */
> --
> 2.23.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel