[{"id":2708,"web_url":"https://patchwork.libcamera.org/comment/2708/","msgid":"<20190928100408.gf5m7r755fun7k4e@uno.localdomain>","date":"2019-09-28T10:04:08","subject":"Re: [libcamera-devel] [PATCH v3 09/13] libcamera: timeline: Add a\n\tbasic timeline implementation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n   just a few spelling comments before I really grasp the content of\n   the series\n\nOn Fri, Sep 27, 2019 at 04:44:13AM +0200, Niklas Söderlund wrote:\n> The timeline is a helper for pipeline handlers to ease interacting with\n> an IPA. 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 933be8543a8d5f02..fc6402b6ffb3d47c 100644\n> --- a/src/libcamera/include/meson.build\n> +++ b/src/libcamera/include/meson.build\n> @@ -16,6 +16,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..519caf5a923f35a7\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> +\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\nIs it worth to shorten these ? A scheduleAction() that takes a\nFrameAction could be called just 'schedule()' in example.\n\n> +\n> +\tutils::duration frameInterval() const { return frameInterval_; }\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\nCapital H as it's a constexpr ?\n\n> +\n> +\tvoid timeout(Timer *timer);\n> +\tvoid updateDeadline(const utils::time_point &deadline);\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 755149c55c7b4f84..fef2e8619a42e053 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -28,6 +28,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..b5a713abbf3235eb\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> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Timeline)\n> +\n> +/**\n> + * \\class FrameAction\n> + * \\brief Action which can be schedule within a frames lifetime\n\nframes/frame\n\n> + *\n> + * A frame action is a event which takes place at some point during a frames\nan event\nframes/frame\n\n> + * lifetime inside libcamera. Each action have two primal attributes; type and\n\nhas\n\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 have no meaning to anyone outside\n\nhas\n\n/to anyone//\n\n> + * the specific implementation. The frame number is the frame the action applies\n> + * to.\n> + */\n> +\n> +/**\n> + * \\fn FrameAction::FrameAction\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> + * functions which describes the actions they wish to happen when the act is\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 FrameActions and expect them to be\n\nwe usually don't pluralize class names: FrameAction instances\n\n> + * executed at the correct time. The correct time to execute them are a sum\n\nthem are/them is\n\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 the IPA. The\n\nor by the IPA\n\n> + * exact implementation of how the timing delays are setup are pipeline\n\nare pipeline/is pipeline/\n\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\nThis is not clear to me. How are timings updated ? Could you mention\nthe method?\n\n> + *\n> + * The pipeline handler is responsible for feeding the timeline as accurate as\n> + * possible information of the exposures it observes.\n\nCould you mention how? By notifying start of exposure? Are there other\nevents?\n\n> + */\n> +\n> +Timeline::Timeline()\n> +\t: frameInterval_(std::chrono::milliseconds(1))\n> +{\n> +\ttimer_.timeout.connect(this, &Timeline::timeout);\n> +}\n> +\n> +/**\n> + * \\brief Reset the timeline\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> +{\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\nI'm not sure I get what frameOffset is.\n\nEach action has a frame numer it applied to (action->frame())\nThe timeline has a setRawDealy() which takes a frame offset and a\ndelay (and is currenly never used by the RkISP IPA if I'm not mistaken,\nbut that's a different issue). If an action alreay has frame number it\napplies to what is the offset for ?\n\nCould you document it? It is reported as just 'frame offset'\neverywhere it is used.\n\nShould we make sure we didn't get out of sync and the resulting\n'frame' integer is > current frame ?\n\n> +\tutils::time_point deadline = lastTime + frame * frameInterval_ + timeOffset(action->type());\n\nNit: could you break this line ?\n\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 schedule too late \"\n\nscheduled\n\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> +\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> +\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> +}\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\nThose two operations are identical, you just return second or first.\nBut I agree they're trivial and could stay the same for now.\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\nCould you spend a few more words about the actual meaning of those parameters\nand the role they play in calculating the delays in the timeline, here\not in the class description.\n\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> +\t *\n> +\t * \\todo: Should the Timer operate using nanoseconds?\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> +\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> +\n> +\t\tutils::time_point sched = it->first;\n> +\t\tFrameAction *action = it->second;\n> +\n> +\t\tif (sched >= now) {\n> +\t\t\tupdateDeadline(sched);\n\nAm I reading it wrong or as soon as you find an action which is\nscheduled in the future you program a timeout then exit the loop ? Are\nactions_ sorted? Shouldn't you find the closer deadline and program\nthe sleep interval using that?\n\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\taction->run();\n\nI would also keep the action currenly \"in sleep\" somewhere, and update\nit at updateDeadline() time, so you can program a new sleep after\nhaving erased the action to run, and then run it directly outside of this\nfor loop.\n\n> +\n> +\t\tit = actions_.erase(it);\n> +\t\tdelete action;\n> +\t}\n> +}\n> +\n> +} /* namespace libcamera */\n> --\n> 2.23.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71C0C60BBA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 28 Sep 2019 12:02:31 +0200 (CEST)","from uno.localdomain\n\t(host71-63-dynamic.19-79-r.retail.telecomitalia.it [79.19.63.71])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 215BBE0007;\n\tSat, 28 Sep 2019 10:02:29 +0000 (UTC)"],"X-Originating-IP":"79.19.63.71","Date":"Sat, 28 Sep 2019 12:04:08 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190928100408.gf5m7r755fun7k4e@uno.localdomain>","References":"<20190927024417.725906-1-niklas.soderlund@ragnatech.se>\n\t<20190927024417.725906-10-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"esvvbum2ddw43jks\"","Content-Disposition":"inline","In-Reply-To":"<20190927024417.725906-10-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 09/13] 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":"Sat, 28 Sep 2019 10:02:31 -0000"}}]