[{"id":2774,"web_url":"https://patchwork.libcamera.org/comment/2774/","msgid":"<20191004065727.GB9925@pendragon.ideasonboard.com>","date":"2019-10-04T06:57:27","subject":"Re: [libcamera-devel] [PATCH v4 07/11] libcamera: timeline: Add a\n\tbasic timeline implementation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Thu, Oct 03, 2019 at 07:49:37PM +0200, Niklas Söderlund wrote:\n> The timeline is a helper for pipeline handlers to ease interacting with\n> an IPA.\n\nIt's not just a helper. Given that the IPA API is based around queuing\nframe actions, the timeline is a central concept in the design.\n\n> The idea is that the pipeline handler runs a timeline which\n> schedules and executes actions on the hardware. The pipeline listens to\n> signals from the IPA which it translates into actions which are schedule\n> on the timeline.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/include/meson.build |   1 +\n>  src/libcamera/include/timeline.h  |  71 ++++++++\n>  src/libcamera/meson.build         |   1 +\n>  src/libcamera/timeline.cpp        | 267 ++++++++++++++++++++++++++++++\n>  4 files changed, 340 insertions(+)\n>  create mode 100644 src/libcamera/include/timeline.h\n>  create mode 100644 src/libcamera/timeline.cpp\n> \n> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> index 2c74d29bd925ede2..238ddc2c2cf96a5d 100644\n> --- a/src/libcamera/include/meson.build\n> +++ b/src/libcamera/include/meson.build\n> @@ -18,6 +18,7 @@ libcamera_headers = files([\n>      'pipeline_handler.h',\n>      'process.h',\n>      'thread.h',\n> +    'timeline.h',\n>      'utils.h',\n>      'v4l2_controls.h',\n>      'v4l2_device.h',\n> diff --git a/src/libcamera/include/timeline.h b/src/libcamera/include/timeline.h\n> new file mode 100644\n> index 0000000000000000..1ec28a0d9134d35c\n> --- /dev/null\n> +++ b/src/libcamera/include/timeline.h\n> @@ -0,0 +1,71 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * timeline.h - Timeline for per-frame controls\n> + */\n> +#ifndef __LIBCAMERA_TIMELINE_H__\n> +#define __LIBCAMERA_TIMELINE_H__\n> +\n> +#include <list>\n> +#include <map>\n> +\n> +#include <libcamera/timer.h>\n> +\n> +#include \"utils.h\"\n> +\n> +namespace libcamera {\n> +\n> +class FrameAction\n> +{\n> +public:\n> +\tFrameAction(unsigned int type, unsigned int frame)\n> +\t\t: type_(type), frame_(frame) {}\n\nLet's flip frame and type, here and in the methods and fields below.\nframe is the most generic parameter, and within a frame, type further\nqualifies the action.\n\n> +\n> +\tvirtual ~FrameAction() {}\n> +\n> +\tunsigned int type() const { return type_; }\n> +\tunsigned int frame() const { return frame_; }\n> +\n> +\tvirtual void run() = 0;\n> +\n> +private:\n> +\tunsigned int type_;\n> +\tunsigned int frame_;\n> +};\n> +\n> +class Timeline\n> +{\n> +public:\n> +\tTimeline();\n> +\tvirtual ~Timeline() {}\n> +\n> +\tvirtual void reset();\n> +\tvirtual void scheduleAction(FrameAction *action);\n> +\tvirtual void notifyStartOfExposure(unsigned int frame, utils::time_point time);\n> +\n> +\tutils::duration frameInterval() const { return frameInterval_; }\n\nIs this method needed ?\n\n> +\n> +protected:\n> +\tint frameOffset(unsigned int type) const;\n> +\tutils::duration timeOffset(unsigned int type) const;\n> +\n> +\tvoid setRawDelay(unsigned int type, int frame, utils::duration time);\n> +\n> +private:\n> +\tstatic constexpr unsigned int HistoryDepth = 10;\n\nCamelCase is used for class names. You should use SNAKE_CASE for\nconstants.\n\n> +\n> +\tvoid timeout(Timer *timer);\n> +\tvoid updateDeadline(const utils::time_point &deadline);\n\nIf time_point (and duration) are large enough to be passed as a const\nreference, you should do the same above in all functions that take or\nreturn such a value. Otherwise you should pass deadline by value here.\n\n> +\n> +\tstd::list<std::pair<unsigned int, utils::time_point>> history_;\n> +\tstd::multimap<utils::time_point, FrameAction *> actions_;\n> +\tstd::map<unsigned int, std::pair<int, utils::duration>> delays_;\n> +\tutils::duration frameInterval_;\n> +\n> +\tTimer timer_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_TIMELINE_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 3a3b388a6a344961..195368e77e5e6f01 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -30,6 +30,7 @@ libcamera_sources = files([\n>      'signal.cpp',\n>      'stream.cpp',\n>      'thread.cpp',\n> +    'timeline.cpp',\n>      'timer.cpp',\n>      'utils.cpp',\n>      'v4l2_controls.cpp',\n> diff --git a/src/libcamera/timeline.cpp b/src/libcamera/timeline.cpp\n> new file mode 100644\n> index 0000000000000000..042ead41d2ff54d5\n> --- /dev/null\n> +++ b/src/libcamera/timeline.cpp\n> @@ -0,0 +1,267 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * timeline.cpp - Timeline for per-frame controls\n> + */\n> +\n> +#include \"timeline.h\"\n> +\n> +#include \"log.h\"\n\nYou're missing \\file, which prevents the documentation from being\ncompiled.\n\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Timeline)\n> +\n> +/**\n> + * \\class FrameAction\n> + * \\brief Action which can be schedule within a frame's lifetime\n> + *\n> + * A frame action is an event which takes place at some point during a frame's\n> + * lifetime inside libcamera. Each action has two primal attributes; type and\n> + * frame number.\n> + *\n> + * The type is a numerical ID which identifies the action within the pipeline\n> + * handler. The type is pipeline specific and has no meaning outside the\n> + * specific implementation. The frame number is the frame the action applies to.\n> + */\n> +\n> +/**\n> + * \\fn FrameAction::FrameAction\n\ns/$/()/\n\n> + * \\brief Create a frame action\n> + * \\param[in] type Action type\n> + * \\param[in] frame Frame number\n> + */\n> +\n> +/**\n> + * \\fn FrameAction::type()\n> + * \\brief Retrieve the type of action\n> + * \\return Action type\n> + */\n> +\n> +/**\n> + * \\fn FrameAction::frame()\n> + * \\brief Retrieve the frame number\n> + * \\return Frame number\n> + */\n> +\n> +/**\n> + * \\fn FrameAction::run()\n> + * \\brief The action to perform when the action is triggered\n> + *\n> + * Pipeline handlers shall subclass the FrameAction object and implement run()\n\ns/object/class/\n\n> + * functions which describes the actions they wish to happen when the act is\n\ns/functions/methods/\n\n> + * schedule using the Timeline.\n> + *\n> + * \\sa Timeline\n> + */\n> +\n> +/**\n> + * \\class Timeline\n> + * \\brief Scheduler and executor of FrameAction's\n> + *\n> + * On the timeline the pipeline can schedule FrameAction and expect them to be\n> + * executed at the correct time. The correct time to execute them is a sum\n> + * of which frame the action should apply to and a list of timing delays for\n> + * each action the pipeline handler describes.\n> + *\n> + * The timing delays can either be set by the pipeline handler or by the IPA.\n> + * The exact implementation of how the timing delays are setup is pipeline\n> + * specific. It is expected that the IPA will update the timing delays as it\n> + * make changes to how the pipeline operates in different situations.\n> + *\n> + * The pipeline handler is responsible for feeding the timeline as accurate as\n> + * possible information of the exposures it observes.\n> + */\n> +\n> +Timeline::Timeline()\n> +\t: frameInterval_(std::chrono::milliseconds(1))\n\nWhy 1 ? I don't think we should have magic values.\n\n> +{\n> +\ttimer_.timeout.connect(this, &Timeline::timeout);\n> +}\n> +\n> +/**\n> + * \\brief Reset the timeline\n\nI wondered when reading the class definition above what the reset method\nwould do, am I'm none the wiser now :-S\n\n> + */\n> +void Timeline::reset()\n> +{\n> +\ttimer_.stop();\n> +\n> +\tactions_.clear();\n> +\thistory_.clear();\n> +}\n> +\n> +/**\n> + * \\brief Add a action to the timeline\n> + * \\param[in] action FrameAction to add\n> + *\n> + * Schedule an action at a specific time taking the action and timing delays\n> + * into account. If a action is schedule too late execute it immediately and\n> + * try to recover.\n> + */\n> +void Timeline::scheduleAction(FrameAction *action)\n\nOwnership of action isn't clear. Is it transferred to this function as I\nbelieve it is ? If so you should use a unique pointer to make this\nexplicit.\n\n> +{\n> +\tunsigned int lastFrame;\n> +\tutils::time_point lastTime;\n> +\n> +\tif (history_.empty()) {\n> +\t\t/*\n> +\t\t * This only happens for the first number of frames, up to\n> +\t\t * pipeline depth.\n> +\t\t */\n> +\t\tlastFrame = 0;\n> +\t\tlastTime = std::chrono::steady_clock::now();\n> +\t} else {\n> +\t\tlastFrame = history_.back().first;\n> +\t\tlastTime = history_.back().second;\n> +\t}\n> +\n> +\t/*\n> +\t * Calculate action trigger time by first figuring the out the start of\n> +\t * exposure (SOE) of the frame the action corresponds to and then adding\n> +\t * the frame and timing offsets.\n> +\t */\n> +\tint frame = action->frame() + frameOffset(action->type()) - lastFrame;\n> +\tutils::time_point deadline = lastTime + frame * frameInterval_\n> +\t\t+ timeOffset(action->type());\n> +\n> +\tutils::time_point now = std::chrono::steady_clock::now();\n> +\tif (deadline < now) {\n> +\t\tLOG(Timeline, Warning)\n> +\t\t\t<< \"Action scheduled too late \"\n> +\t\t\t<< utils::time_point_to_string(deadline)\n> +\t\t\t<< \", run now \" << utils::time_point_to_string(now);\n> +\t\taction->run();\n\naction is leaked.\n\n> +\t} else {\n> +\t\tactions_.insert({ deadline, action });\n> +\t\tupdateDeadline(deadline);\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Inform timeline of a new exposure\n> + * \\param[in] frame Frame number of the exposure\n> + * \\param[in] time The best approximation of when the exposure started\n> + */\n> +void Timeline::notifyStartOfExposure(unsigned int frame, utils::time_point time)\n> +{\n> +\thistory_.push_back(std::make_pair(frame, time));\n> +\n> +\tif (history_.size() <= HistoryDepth / 2)\n> +\t\treturn;\n> +\n> +\twhile (history_.size() > HistoryDepth)\n> +\t\thistory_.pop_front();\n> +\n> +\t/* Update esitmated time between two start of exposures. */\n> +\tutils::duration sumExposures = std::chrono::duration_values<std::chrono::nanoseconds>::zero();\n\n\n\tutils::duration sumExposures(0);\n\ncompiles, does it do the right thing ?\n\n> +\tunsigned int numExposures = 0;\n> +\n> +\tutils::time_point lastTime;\n> +\tfor (auto it = history_.begin(); it != history_.end(); it++) {\n> +\t\tif (it != history_.begin()) {\n> +\t\t\tsumExposures += it->second - lastTime;\n> +\t\t\tnumExposures++;\n> +\t\t}\n> +\n> +\t\tlastTime = it->second;\n> +\t}\n> +\n> +\tframeInterval_ = sumExposures;\n> +\tif (numExposures)\n> +\t\tframeInterval_ /= numExposures;\n\nThis algorithm is going to produce an awful jitter, and I'm not even\nsure it will give a correct average value.\n\n> +}\n> +\n> +/**\n> + * \\fn Timeline::frameInterval()\n> + * \\brief Retrieve the best estimate of the frame interval\n> + * \\return Frame interval estimate\n> + */\n> +\n> +/**\n> + * \\brief Retrieve the frame offset for an action type\n> + * \\param[in] type Action type\n> + * \\return Frame offset for the action type\n> + */\n> +int Timeline::frameOffset(unsigned int type) const\n> +{\n> +\tconst auto it = delays_.find(type);\n> +\tif (it == delays_.end()) {\n> +\t\tLOG(Timeline, Error)\n> +\t\t\t<< \"No frame offset set for action type \" << type;\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn it->second.first;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the time offset for an action type\n> + * \\param[in] type Action type\n> + * \\return Time offset for the action type\n> + */\n> +utils::duration Timeline::timeOffset(unsigned int type) const\n> +{\n> +\tconst auto it = delays_.find(type);\n> +\tif (it == delays_.end()) {\n> +\t\tLOG(Timeline, Error)\n> +\t\t\t<< \"No time offset set for action type \" << type;\n> +\t\treturn utils::duration::zero();\n> +\t}\n> +\n> +\treturn it->second.second;\n> +}\n> +\n> +/**\n> + * \\brief Set the frame and time offset delays for an action type\n> + * \\param[in] type Action type\n> + * \\param[in] frame Frame offset\n> + * \\param[in] time Time offset\n> + */\n> +void Timeline::setRawDelay(unsigned int type, int frame, utils::duration time)\n> +{\n> +\tdelays_[type] = std::make_pair(frame, time);\n> +}\n> +\n> +void Timeline::updateDeadline(const utils::time_point &deadline)\n> +{\n> +\tif (timer_.isRunning() && deadline >= timer_.deadline())\n> +\t\treturn;\n> +\n> +\tutils::time_point now = std::chrono::steady_clock::now();\n> +\tutils::duration duration = deadline - now;\n> +\n> +\t/*\n> +\t * Translate nanoseconds resolution the timeline to milliseconds using\n> +\t * a ceiling approach to not trigger an action before it's scheduled.\n\n\t/*\n\t * Convert the duration to milliseconds. Round up to avoid\n\t * trigerring the action before it should be scheduled.\n\t */\n\n> +\t *\n> +\t * \\todo: Should the Timer operate using nanoseconds?\n\nI think Timer should have an overloaded start() method that takes a time\npoint, as the first thing start() does is converting the duration to a\ntime point.\n\n> +\t */\n> +\tunsigned long nsecs = std::chrono::duration_cast<std::chrono::nanoseconds>(duration).count();\n> +\tunsigned int msecs = nsecs / 1000000 + (nsecs % 1000000 ? 1 : 0);\n\nShould we add an implementation of\nhttps://en.cppreference.com/w/cpp/chrono/duration/ceil to utils ?\n\n> +\n> +\ttimer_.stop();\n> +\ttimer_.start(msecs);\n> +}\n> +\n> +void Timeline::timeout(Timer *timer)\n> +{\n> +\tfor (auto it = actions_.begin(); it != actions_.end();) {\n> +\t\tutils::time_point now = std::chrono::steady_clock::now();\n\nCan this be moved outside of the loop ?\n\n> +\n> +\t\tutils::time_point sched = it->first;\n\nconst reference ?\n\n> +\t\tFrameAction *action = it->second;\n\nThis can be moved after the if ().\n\n> +\n> +\t\tif (sched >= now) {\n> +\t\t\tupdateDeadline(sched);\n\nI would move this after the for loop.\n\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\taction->run();\n> +\n> +\t\tit = actions_.erase(it);\n> +\t\tdelete action;\n\nThe delete won't be needed anymore if you store unique pointers.\n\n> +\t}\n\n\tupdateDeadline();\n\nwith no argument here, and use actions_.front() to retrieve the next\naction to schedule in updateDeadline() (after a check for empty() of\ncourse).\n\n> +}\n> +\n> +} /* namespace libcamera */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 56ED060E1E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  4 Oct 2019 08:57:42 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(modemcable151.96-160-184.mc.videotron.ca [184.160.96.151])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B03FB2E5;\n\tFri,  4 Oct 2019 08:57:41 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570172262;\n\tbh=GrkTeQrtKgP87LoiO/XzLkSjlRWsM3+a9xrF/qphMJI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bIARN2Q6i19bInqsF4h2T44VlLLtYCmI6UR2lXhdu+nlwcotEO22lhGGuw4Pnfunf\n\tcVpw2lBz0xX/DOuz5Mo627wI77smksppkhrBaafp9R1rpnsPV/KfUHEOBvSqGuWgk7\n\tYflDVznvLqTBfTs07Vlih3Jnx49lmMm0jCOBlXDk=","Date":"Fri, 4 Oct 2019 09:57:27 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191004065727.GB9925@pendragon.ideasonboard.com>","References":"<20191003174941.1296988-1-niklas.soderlund@ragnatech.se>\n\t<20191003174941.1296988-8-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191003174941.1296988-8-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 07/11] libcamera: timeline: Add a\n\tbasic timeline implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 04 Oct 2019 06:57:42 -0000"}}]