[{"id":13877,"web_url":"https://patchwork.libcamera.org/comment/13877/","msgid":"<5ee9b0fb-c1ce-74ef-e793-e9cb3d6fbb6b@ideasonboard.com>","date":"2020-11-25T11:14:57","subject":"Re: [libcamera-devel] [PATCH v3 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 23/11/2020 22:12, Niklas Söderlund wrote:\n> Some sensor controls take effect with a delay as the sensor needs time\n> to adjust, for example exposure. Add a optional helper DelayedControls\n> to help pipelines deal with such controls.\n> \n> The idea is to provide a queue of controls towards the V4L2 device and\n> apply individual controls with the specified delay with the aim to get\n> predictable and retrievable control values for any given frame. To do\n> this the queue of controls needs to be at least as deep as the control\n> with the largest delay.\n> \n> The DelayedControls needs to be informed of every start of exposure.\n> This can be emulated but the helper is designed to be used with this\n> event being provide by the kernel thru V4L2 events.\n> \n> This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> handler but expands on its API. This helpers aims to replace the\n> Raspberry Pi implementations and mimics it behavior perfectly.\n\ns/behavior/behaviour/\n\nBut that could be a valid americanism (behavior)\n\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since v2\n> - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> - s/fistSequence_/firstSequence_/\n> \n> * Changes since v1\n> - Correct copyright to reflect work is derived from Raspberry Pi\n> pipeline handler. This was always the intention and was wrong in v1.\n> - Rewrite large parts of the documentation.\n> - Join two loops to one in DelayedControls::DelayedControls()\n> ---\n> include/libcamera/internal/delayed_controls.h | 87 ++++++\n> src/libcamera/delayed_controls.cpp | 265 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 353 insertions(+)\n> create mode 100644 include/libcamera/internal/delayed_controls.h\n> create mode 100644 src/libcamera/delayed_controls.cpp\n> \n> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> new file mode 100644\n> index 0000000000000000..e9cb0b90aac99847\n> --- /dev/null\n> +++ b/include/libcamera/internal/delayed_controls.h\n> @@ -0,0 +1,87 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class V4L2Device;\n> +\n> +class DelayedControls\n> +{\n> +public:\n> +\tDelayedControls(V4L2Device *device,\n> +\t\t\tconst std::unordered_map &delays);\n> +\n> +\tvoid reset(ControlList *controls = nullptr);\n> +\n> +\tbool push(const ControlList &controls);\n> +\tControlList get(uint32_t sequence);\n> +\n> +\tvoid frameStart(uint32_t sequence);\n> +\n> +private:\n> +\tclass ControlInfo\n> +\t{\n> +\tpublic:\n> +\t\tControlInfo()\n> +\t\t\t: updated(false)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tControlInfo(const ControlValue &v)\n> +\t\t\t: value(v), updated(true)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tControlValue value;\n> +\t\tbool updated;\n> +\t};\n> +\n> +\tstatic constexpr int listSize = 16;\n> +\tclass ControlArray : public std::array\n> +\t{\n> +\tpublic:\n> +\t\tControlInfo &operator[](unsigned int index)\n> +\t\t{\n> +\t\t\treturn std::array::operator[](index % listSize);\n> +\t\t}\n> +\n> +\t\tconst ControlInfo &operator[](unsigned int index) const\n> +\t\t{\n> +\t\t\treturn std::array::operator[](index % listSize);\n> +\t\t}\n> +\t};\n> +\n> +\tusing ControlsDelays = std::unordered_map;\n> +\tusing ControlsValues = std::unordered_map;\n\nI'd remove the half-way pluralisation here\n\tControlDelays\n\tControlValues\n\n\n\n> +\n> +\tbool queue(const ControlList &controls);\n> +\n> +\tstd::mutex lock_;\n> +\n> +\tV4L2Device *device_;\n> +\tControlsDelays delays_;\n> +\tunsigned int maxDelay_;\n> +\n> +\tbool running_;\n> +\tuint32_t firstSequence_;\n> +\n> +\tuint32_t queueCount_;\n> +\tuint32_t writeCount_;\n> +\tControlsValues ctrls_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> new file mode 100644\n> index 0000000000000000..c810be48a36be515\n> --- /dev/null\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -0,0 +1,265 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> + */\n> +\n> +#include \"libcamera/internal/delayed_controls.h\"\n> +#include \"libcamera/internal/v4l2_device.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file delayed_controls.h\n> + * \\brief Helper to deal with controls that are applied with a delay\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(DelayedControls)\n> +\n> +/**\n> + * \\class DelayedControls\n> + * \\brief Helper to deal with controls that take effect with a delay\n> + *\n> + * Some sensor controls take effect with a delay as the sensor needs time to\n> + * adjust, for example exposure and focus. This is an optional helper class to\n> + * deal with such controls and the intended users are pipeline handlers.\n> + *\n> + * The idea is to extend the concept of the buffer depth of a pipeline the\n> + * application need to maintain to also cover controls. Just as with buffer\n> + * depth if the application keeps the number of requests queued above the\n> + * control depth the controls are guaranteed to take effect for the correct\n> + * request. The control depth is determined by the control with the grates\n> + * delay.\n> + */\n> +\n> +/**\n> + * \\brief Construct a DelayedControls\n> + * \\param[in] device The V4L2 device the controls have to be applied to\n> + * \\param[in] delays Map of the numerical V4L2 control ids to their associated\n> + * delays (in frames)\n> + *\n> + * Only controls specified in \\a delays are handled. If it's desired to mix\n> + * delayed controls and controls that take effect immediately the immediate\n> + * controls must be listed in the \\a delays map with a delay value of 0.\n> + */\n> +DelayedControls::DelayedControls(V4L2Device *device,\n> +\t\t\t\t const std::unordered_map &delays)\n> +\t: device_(device), maxDelay_(0)\n> +{\n> +\tconst ControlInfoMap &controls = device_->controls();\n> +\n> +\t/*\n> +\t * Create a map of control ids to delays for controls exposed by the\n> +\t * device.\n> +\t */\n> +\tfor (auto const &delay : delays) {\n> +\t\tauto itControl = controls.find(delay.first);\n> +\t\tif (itControl == controls.end()) {\n> +\t\t\tLOG(DelayedControls, Error)\n> +\t\t\t\t<< \"Delay request for control id \"\n> +\t\t\t\t<< utils::hex(delay.first)\n> +\t\t\t\t<< \" but control is not exposed by device \"\n> +\t\t\t\t<< device_->deviceNode();\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tconst ControlId *id = itControl->first;\n> +\n> +\t\tdelays_[id] = delay.second;\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Set a delay of \" << delays_[id]\n> +\t\t\t<< \" for \" << id->name();\n> +\n> +\t\tmaxDelay_ = std::max(maxDelay_, delays_[id]);\n> +\t}\n> +\n> +\treset();\n> +}\n> +\n> +/**\n> + * \\brief Reset state machine and controls\n> + * \\param[in] controls Optional list of controls to apply to the device\n> + *\n> + * Resets the state machine to a starting position based on control values\n> + * retrieved from the device. Controls may optionally be set before they are\n> + * read back using \\a controls.\n> + */\n> +void DelayedControls::reset(ControlList *controls)\n> +{\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\trunning_ = false;\n> +\tfirstSequence_ = 0;\n> +\tqueueCount_ = 0;\n> +\twriteCount_ = 0;\n> +\n> +\t/* Set the controls on the device if requested. */\n> +\tif (controls)\n> +\t\tdevice_->setControls(controls);\n> +\n> +\t/* Retrieve current control values reported by the device. */\n> +\tstd::vector ids;\n> +\tfor (auto const &delay : delays_)\n> +\t\tids.push_back(delay.first->id());\n> +\n> +\tControlList devCtrls = device_->getControls(ids);\n> +\n> +\t/* Seed the control queue with the controls reported by the device. */\n> +\tctrls_.clear();\n> +\tfor (const auto &ctrl : devCtrls) {\n> +\t\tconst ControlId *id = devCtrls.infoMap()->idmap().at(ctrl.first);\n> +\t\tctrls_[id][queueCount_] = ControlInfo(ctrl.second);\n> +\t}\n> +\n> +\tqueueCount_++;\n> +}\n> +\n> +/**\n> + * \\brief Push a set of controls on the queue\n> + * \\param[in] controls List of controls to add to the device queue\n> + *\n> + * Push a set of controls to the control queue. This increases the control queue\n> + * depth by one.\n> + *\n> + * \\returns true if \\a controls are accepted, or false otherwise\n> + */\n> +bool DelayedControls::push(const ControlList &controls)\n> +{\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\treturn queue(controls);\n> +}\n> +\n> +bool DelayedControls::queue(const ControlList &controls)\n> +{\n> +\t/* Copy state from previous frame. */\n> +\tfor (auto &ctrl : ctrls_) {\n> +\t\tControlInfo &info = ctrl.second[queueCount_];\n> +\t\tinfo.value = ctrls_[ctrl.first][queueCount_ - 1].value;\n> +\t\tinfo.updated = false;\n> +\t}\n> +\n> +\t/* Update with new controls. */\n> +\tconst ControlIdMap &idmap = device_->controls().idmap();\n> +\tfor (const auto &control : controls) {\n> +\t\tconst auto &itCtrl = idmap.find(control.first);\n> +\t\tif (itCtrl == idmap.end())\n> +\t\t\treturn false;\n> +\n> +\t\tconst ControlId *id = itCtrl->second;\n> +\n> +\t\tif (delays_.find(id) == delays_.end())\n> +\t\t\treturn false;\n> +\n> +\t\tControlInfo &info = ctrls_[id][queueCount_];\n> +\n> +\t\tinfo.value = control.second;\n> +\t\tinfo.updated = true;\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Queuing \" << id->name()\n> +\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t<< \" at index \" << queueCount_;\n> +\t}\n> +\n> +\tqueueCount_++;\n> +\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\brief Read back controls in effect at a sequence number\n> + * \\param[in] sequence The sequence number to get controls for\n> + *\n> + * Read back what controls where in effect at a specific sequence number. The\n> + * history is a ring buffer of 16 entries where new and old values coexist. It's\n> + * the callers responsibility to not read to old sequence numbers that have been\n> + * pushed out of the history.\n> + *\n> + * Historic values are evicted by pushing new values onto the queue using\n> + * push(). The max history from the current sequence number that yields valid\n> + * values are thus 16 minus number of controls pushed.\n> + *\n> + * \\return The controls at \\a sequence number\n> + */\n> +ControlList DelayedControls::get(uint32_t sequence)\n> +{\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\tuint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> +\tunsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> +\n> +\tControlList out(device_->controls());\n> +\tfor (const auto &ctrl : ctrls_) {\n> +\t\tconst ControlId *id = ctrl.first;\n> +\t\tconst ControlInfo &info = ctrl.second[index];\n> +\n> +\t\tout.set(id->id(), info.value);\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Reading \" << id->name()\n> +\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t<< \" at index \" << index;\n> +\t}\n> +\n> +\treturn out;\n> +}\n> +\n> +/**\n> + * \\brief Inform DelayedControls of the start of a new frame\n> + * \\param[in] sequence Sequence number of the frame that started\n> + *\n> + * Inform the state machine that a new frame has started and of its sequence\n> + * number. Any user of these helpers is responsible to inform the helper about\n> + * the start of any frame.This can be connected with ease to the start of a\n> + * exposure (SOE) V4L2 event.\n> + */\n> +void DelayedControls::frameStart(uint32_t sequence)\n> +{\n> +\tLOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> +\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\tif (!running_) {\n> +\t\tfirstSequence_ = sequence;\n> +\t\trunning_ = true;\n> +\t}\n> +\n> +\t/*\n> +\t * Create control list peaking ahead in the value queue to ensure\n> +\t * values are set in time to satisfy the sensor delay.\n> +\t */\n> +\tControlList out(device_->controls());\n> +\tfor (const auto &ctrl : ctrls_) {\n> +\t\tconst ControlId *id = ctrl.first;\n> +\t\tunsigned int delayDiff = maxDelay_ - delays_[id];\n> +\t\tunsigned int index = std::max(0, writeCount_ - delayDiff);\n> +\t\tconst ControlInfo &info = ctrl.second[index];\n> +\n> +\t\tif (info.updated) {\n> +\t\t\tout.set(id->id(), info.value);\n> +\t\t\tLOG(DelayedControls, Debug)\n> +\t\t\t\t<< \"Setting \" << id->name()\n> +\t\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t\t<< \" at index \" << index;\n> +\t\t}\n> +\t}\n> +\n> +\twriteCount_++;\n> +\n> +\twhile (writeCount_ >= queueCount_) {\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Queue is empty, auto queue no-op.\";\n> +\t\tqueue({});\n> +\t}\n> +\n> +\tdevice_->setControls(&out);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -12,6 +12,7 @@ libcamera_sources = files([\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> + 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'event_dispatcher.cpp',\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5F06CBE176\n\tfor ;\n\tWed, 25 Nov 2020 11:15:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A8B0A63409;\n\tWed, 25 Nov 2020 12:15:01 +0100 (CET)","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 74E4B633E5\n\tfor ;\n\tWed, 25 Nov 2020 12:15:00 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DB437292;\n\tWed, 25 Nov 2020 12:14:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KgLoh/us\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606302900;\n\tbh=Pb+i8H5obrpyqCBIJi9RIiTCQ0S/yigSyXV77Zz6U74=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=KgLoh/us7QqWQ83CYEfTqCgq3vIcd5L/RkWPrLiLATpz9AOLNhznvMcul8WWgVcxN\n\t+WywPg2rpcb37deZa+u3cxyJre/aNasOuVqA334vF9D+KWssX6EJzx2/5EnxBhSQXz\n\tkC3X0fSLCJuqTjj+dY1dnyTc8uzQbRUyb8Oq7b0E=","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org, naush@raspberrypi.com","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-2-niklas.soderlund@ragnatech.se>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<5ee9b0fb-c1ce-74ef-e793-e9cb3d6fbb6b@ideasonboard.com>","Date":"Wed, 25 Nov 2020 11:14:57 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201123221234.485933-2-niklas.soderlund@ragnatech.se>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13939,"web_url":"https://patchwork.libcamera.org/comment/13939/","msgid":"<20201126173859.3b6ppgodjpqviics@uno.localdomain>","date":"2020-11-26T17:38:59","subject":"Re: [libcamera-devel] [PATCH v3 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Nov 23, 2020 at 11:12:27PM +0100, Niklas Söderlund wrote:\n> Some sensor controls take effect with a delay as the sensor needs time\n> to adjust, for example exposure. Add a optional helper DelayedControls\n> to help pipelines deal with such controls.\n>\n> The idea is to provide a queue of controls towards the V4L2 device and\n> apply individual controls with the specified delay with the aim to get\n> predictable and retrievable control values for any given frame. To do\n> this the queue of controls needs to be at least as deep as the control\n> with the largest delay.\n>\n> The DelayedControls needs to be informed of every start of exposure.\n> This can be emulated but the helper is designed to be used with this\n> event being provide by the kernel thru V4L2 events.\n>\n> This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> handler but expands on its API. This helpers aims to replace the\n> Raspberry Pi implementations and mimics it behavior perfectly.\n>\n\nNow tha the whole mechanism clicked for me (it only took three days\nafter all) I mostly have comments on the 'cosmetic' part\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since v2\n> - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> - s/fistSequence_/firstSequence_/\n>\n> * Changes since v1\n> - Correct copyright to reflect work is derived from Raspberry Pi\n> pipeline handler. This was always the intention and was wrong in v1.\n> - Rewrite large parts of the documentation.\n> - Join two loops to one in DelayedControls::DelayedControls()\n> ---\n> include/libcamera/internal/delayed_controls.h | 87 ++++++\n> src/libcamera/delayed_controls.cpp | 265 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 353 insertions(+)\n> create mode 100644 include/libcamera/internal/delayed_controls.h\n> create mode 100644 src/libcamera/delayed_controls.cpp\n>\n> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> new file mode 100644\n> index 0000000000000000..e9cb0b90aac99847\n> --- /dev/null\n> +++ b/include/libcamera/internal/delayed_controls.h\n> @@ -0,0 +1,87 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class V4L2Device;\n> +\n> +class DelayedControls\n> +{\n> +public:\n> +\tDelayedControls(V4L2Device *device,\n> +\t\t\tconst std::unordered_map &delays);\n> +\n> +\tvoid reset(ControlList *controls = nullptr);\n> +\n> +\tbool push(const ControlList &controls);\n> +\tControlList get(uint32_t sequence);\n\nI liked the set/get pair that we had in StaggeredControls more\n\n> +\n> +\tvoid frameStart(uint32_t sequence);\n\nI also think 'write' was probably nicer, but that's minor.\nI would have called this 'notifySOF()' but that's just my preference,\nso keep whatever you have here\n\n> +\n> +private:\n> +\tclass ControlInfo\n\nAs I've commented on v2, this is already the name of a library wide\ntype.\n\nTo be honest I would drop the 'Control' suffix for all the internal\ntypes\n\n ControlInfo -> Info\n ControlsDelays -> Delays\n ControlsValues -> Values\n\nThey're internal after all (I know this goes both ways, but for sure\nthey're shorter)\n\n> +\t{\n> +\tpublic:\n> +\t\tControlInfo()\n> +\t\t\t: updated(false)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tControlInfo(const ControlValue &v)\n> +\t\t\t: value(v), updated(true)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tControlValue value;\n> +\t\tbool updated;\n> +\t};\n> +\n> +\tstatic constexpr int listSize = 16;\n> +\tclass ControlArray : public std::array\n> +\t{\n> +\tpublic:\n> +\t\tControlInfo &operator[](unsigned int index)\n> +\t\t{\n> +\t\t\treturn std::array::operator[](index % listSize);\n> +\t\t}\n> +\n> +\t\tconst ControlInfo &operator[](unsigned int index) const\n> +\t\t{\n> +\t\t\treturn std::array::operator[](index % listSize);\n> +\t\t}\n> +\t};\n> +\n> +\tusing ControlsDelays = std::unordered_map;\n> +\tusing ControlsValues = std::unordered_map;\n> +\n> +\tbool queue(const ControlList &controls);\n> +\n> +\tstd::mutex lock_;\n> +\n> +\tV4L2Device *device_;\n> +\tControlsDelays delays_;\n> +\tunsigned int maxDelay_;\n> +\n> +\tbool running_;\n> +\tuint32_t firstSequence_;\n> +\n> +\tuint32_t queueCount_;\n> +\tuint32_t writeCount_;\n> +\tControlsValues ctrls_;\n\nI would then call this 'values_'\n\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> new file mode 100644\n> index 0000000000000000..c810be48a36be515\n> --- /dev/null\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -0,0 +1,265 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> + */\n> +\n> +#include \"libcamera/internal/delayed_controls.h\"\n> +#include \"libcamera/internal/v4l2_device.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file delayed_controls.h\n> + * \\brief Helper to deal with controls that are applied with a delay\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(DelayedControls)\n> +\n> +/**\n> + * \\class DelayedControls\n> + * \\brief Helper to deal with controls that take effect with a delay\n> + *\n> + * Some sensor controls take effect with a delay as the sensor needs time to\n> + * adjust, for example exposure and focus. This is an optional helper class to\n> + * deal with such controls and the intended users are pipeline handlers.\n> + *\n> + * The idea is to extend the concept of the buffer depth of a pipeline the\n> + * application need to maintain to also cover controls. Just as with buffer\n> + * depth if the application keeps the number of requests queued above the\n> + * control depth the controls are guaranteed to take effect for the correct\n> + * request. The control depth is determined by the control with the grates\n\n'greatest'\n\nI find the reference to the buffer depth more confusing than helping.\n\n> + * delay.\n> + */\n> +\n> +/**\n> + * \\brief Construct a DelayedControls\n \\brief Construct a DelayedControls instance\n\n> + * \\param[in] device The V4L2 device the controls have to be applied to\n> + * \\param[in] delays Map of the numerical V4L2 control ids to their associated\n> + * delays (in frames)\n> + *\n> + * Only controls specified in \\a delays are handled. If it's desired to mix\n> + * delayed controls and controls that take effect immediately the immediate\n> + * controls must be listed in the \\a delays map with a delay value of 0.\n> + */\n> +DelayedControls::DelayedControls(V4L2Device *device,\n> +\t\t\t\t const std::unordered_map &delays)\n> +\t: device_(device), maxDelay_(0)\n> +{\n> +\tconst ControlInfoMap &controls = device_->controls();\n> +\n> +\t/*\n> +\t * Create a map of control ids to delays for controls exposed by the\n> +\t * device.\n> +\t */\n> +\tfor (auto const &delay : delays) {\n> +\t\tauto itControl = controls.find(delay.first);\n> +\t\tif (itControl == controls.end()) {\n> +\t\t\tLOG(DelayedControls, Error)\n> +\t\t\t\t<< \"Delay request for control id \"\n> +\t\t\t\t<< utils::hex(delay.first)\n> +\t\t\t\t<< \" but control is not exposed by device \"\n> +\t\t\t\t<< device_->deviceNode();\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tconst ControlId *id = itControl->first;\n> +\n> +\t\tdelays_[id] = delay.second;\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Set a delay of \" << delays_[id]\n> +\t\t\t<< \" for \" << id->name();\n> +\n> +\t\tmaxDelay_ = std::max(maxDelay_, delays_[id]);\n> +\t}\n> +\n> +\treset();\n> +}\n> +\n> +/**\n> + * \\brief Reset state machine and controls\n> + * \\param[in] controls Optional list of controls to apply to the device\n> + *\n> + * Resets the state machine to a starting position based on control values\n> + * retrieved from the device. Controls may optionally be set before they are\n> + * read back using \\a controls.\n> + */\n> +void DelayedControls::reset(ControlList *controls)\n> +{\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\trunning_ = false;\n> +\tfirstSequence_ = 0;\n> +\tqueueCount_ = 0;\n> +\twriteCount_ = 0;\n> +\n> +\t/* Set the controls on the device if requested. */\n> +\tif (controls)\n> +\t\tdevice_->setControls(controls);\n\nChecking for the return value would guarantee Controls not supported\nby the device are not supplied and not erronously added to the ctrls_\nmap below ?\n\n> +\n> +\t/* Retrieve current control values reported by the device. */\n> +\tstd::vector ids;\n> +\tfor (auto const &delay : delays_)\n> +\t\tids.push_back(delay.first->id());\n> +\n> +\tControlList devCtrls = device_->getControls(ids);\n> +\n> +\t/* Seed the control queue with the controls reported by the device. */\n> +\tctrls_.clear();\n> +\tfor (const auto &ctrl : devCtrls) {\n> +\t\tconst ControlId *id = devCtrls.infoMap()->idmap().at(ctrl.first);\n\nAs .at() with an unsupported id would fail afaict\n\n> +\t\tctrls_[id][queueCount_] = ControlInfo(ctrl.second);\n> +\t}\n> +\n> +\tqueueCount_++;\n> +}\n> +\n> +/**\n> + * \\brief Push a set of controls on the queue\n> + * \\param[in] controls List of controls to add to the device queue\n> + *\n> + * Push a set of controls to the control queue. This increases the control queue\n> + * depth by one.\n> + *\n> + * \\returns true if \\a controls are accepted, or false otherwise\n> + */\n> +bool DelayedControls::push(const ControlList &controls)\n> +{\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\treturn queue(controls);\n> +}\n> +\n> +bool DelayedControls::queue(const ControlList &controls)\n> +{\n> +\t/* Copy state from previous frame. */\n> +\tfor (auto &ctrl : ctrls_) {\n> +\t\tControlInfo &info = ctrl.second[queueCount_];\n> +\t\tinfo.value = ctrls_[ctrl.first][queueCount_ - 1].value;\n> +\t\tinfo.updated = false;\n> +\t}\n> +\n> +\t/* Update with new controls. */\n> +\tconst ControlIdMap &idmap = device_->controls().idmap();\n> +\tfor (const auto &control : controls) {\n> +\t\tconst auto &itCtrl = idmap.find(control.first);\n> +\t\tif (itCtrl == idmap.end())\n> +\t\t\treturn false;\n> +\n> +\t\tconst ControlId *id = itCtrl->second;\n> +\n> +\t\tif (delays_.find(id) == delays_.end())\n> +\t\t\treturn false;\n> +\n> +\t\tControlInfo &info = ctrls_[id][queueCount_];\n> +\n> +\t\tinfo.value = control.second;\n> +\t\tinfo.updated = true;\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Queuing \" << id->name()\n> +\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t<< \" at index \" << queueCount_;\n> +\t}\n> +\n> +\tqueueCount_++;\n> +\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\brief Read back controls in effect at a sequence number\n> + * \\param[in] sequence The sequence number to get controls for\n> + *\n> + * Read back what controls where in effect at a specific sequence number. The\n> + * history is a ring buffer of 16 entries where new and old values coexist. It's\n> + * the callers responsibility to not read to old sequence numbers that have been\n> + * pushed out of the history.\n> + *\n> + * Historic values are evicted by pushing new values onto the queue using\n> + * push(). The max history from the current sequence number that yields valid\n> + * values are thus 16 minus number of controls pushed.\n> + *\n> + * \\return The controls at \\a sequence number\n> + */\n> +ControlList DelayedControls::get(uint32_t sequence)\n> +{\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\tuint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> +\tunsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> +\n> +\tControlList out(device_->controls());\n> +\tfor (const auto &ctrl : ctrls_) {\n> +\t\tconst ControlId *id = ctrl.first;\n> +\t\tconst ControlInfo &info = ctrl.second[index];\n> +\n> +\t\tout.set(id->id(), info.value);\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Reading \" << id->name()\n> +\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t<< \" at index \" << index;\n> +\t}\n> +\n> +\treturn out;\n> +}\n> +\n> +/**\n> + * \\brief Inform DelayedControls of the start of a new frame\n> + * \\param[in] sequence Sequence number of the frame that started\n> + *\n> + * Inform the state machine that a new frame has started and of its sequence\n> + * number. Any user of these helpers is responsible to inform the helper about\n> + * the start of any frame.This can be connected with ease to the start of a\n> + * exposure (SOE) V4L2 event.\n\nThis is all true, this function informs about an SOE events, but\nwhat's relevant is that in response to this event a new set of\ncontrols are written to the device. It is not mentioned anywhere in\ndocumentation.\n\n> + */\n> +void DelayedControls::frameStart(uint32_t sequence)\n> +{\n> +\tLOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> +\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\tif (!running_) {\n> +\t\tfirstSequence_ = sequence;\n> +\t\trunning_ = true;\n> +\t}\n> +\n> +\t/*\n> +\t * Create control list peaking ahead in the value queue to ensure\n> +\t * values are set in time to satisfy the sensor delay.\n> +\t */\n> +\tControlList out(device_->controls());\n> +\tfor (const auto &ctrl : ctrls_) {\n> +\t\tconst ControlId *id = ctrl.first;\n> +\t\tunsigned int delayDiff = maxDelay_ - delays_[id];\n> +\t\tunsigned int index = std::max(0, writeCount_ - delayDiff);\n> +\t\tconst ControlInfo &info = ctrl.second[index];\n> +\n> +\t\tif (info.updated) {\n> +\t\t\tout.set(id->id(), info.value);\n> +\t\t\tLOG(DelayedControls, Debug)\n> +\t\t\t\t<< \"Setting \" << id->name()\n> +\t\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t\t<< \" at index \" << index;\n> +\t\t}\n> +\t}\n> +\n> +\twriteCount_++;\n> +\n> +\twhile (writeCount_ >= queueCount_) {\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Queue is empty, auto queue no-op.\";\n> +\t\tqueue({});\n\nI would check at the beginning of the function if writeCount_ >\nqueueCount_ and in the case exit immediately to avoid pushing a set of\nempty controls just to create a new slot. If I'm not mistaken it has\nthe same effect ?\n\nThis would allow you merge queue() and push() in a single function (I\nstill don't like queue({}) and still seems a bit of an hack :)\n\nThanks\n j\n\n> +\t}\n> +\n> +\tdevice_->setControls(&out);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -12,6 +12,7 @@ libcamera_sources = files([\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> + 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'event_dispatcher.cpp',\n> --\n> 2.29.2\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D71EDBE08A\n\tfor ;\n\tThu, 26 Nov 2020 17:38:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37B5C63472;\n\tThu, 26 Nov 2020 18:38:56 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F2E026346A\n\tfor ;\n\tThu, 26 Nov 2020 18:38:54 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 30AF724000F;\n\tThu, 26 Nov 2020 17:38:53 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 26 Nov 2020 18:38:59 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201126173859.3b6ppgodjpqviics@uno.localdomain>","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201123221234.485933-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14066,"web_url":"https://patchwork.libcamera.org/comment/14066/","msgid":"<20201205012324.GO4109@pendragon.ideasonboard.com>","date":"2020-12-05T01:23:24","subject":"Re: [libcamera-devel] [PATCH v3 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","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, Nov 26, 2020 at 06:38:59PM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 23, 2020 at 11:12:27PM +0100, Niklas Söderlund wrote:\n> > Some sensor controls take effect with a delay as the sensor needs time\n> > to adjust, for example exposure. Add a optional helper DelayedControls\n\ns/a optional/an optional/\n\n> > to help pipelines deal with such controls.\n> >\n> > The idea is to provide a queue of controls towards the V4L2 device and\n> > apply individual controls with the specified delay with the aim to get\n> > predictable and retrievable control values for any given frame. To do\n> > this the queue of controls needs to be at least as deep as the control\n> > with the largest delay.\n> >\n> > The DelayedControls needs to be informed of every start of exposure.\n> > This can be emulated but the helper is designed to be used with this\n> > event being provide by the kernel thru V4L2 events.\n\nI think that's a good design. If we ever have to emulate this using\ntimers, that can always be implement this as an extension (possibly in a\nseparate class whose purpose would be to generate the emulate events).\n\n> > This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> > handler but expands on its API. This helpers aims to replace the\n> > Raspberry Pi implementations and mimics it behavior perfectly.\n> \n> Now tha the whole mechanism clicked for me (it only took three days\n> after all) I mostly have comments on the 'cosmetic' part\n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > * Changes since v2\n> > - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> > - s/fistSequence_/firstSequence_/\n> >\n> > * Changes since v1\n> > - Correct copyright to reflect work is derived from Raspberry Pi\n> > pipeline handler. This was always the intention and was wrong in v1.\n> > - Rewrite large parts of the documentation.\n> > - Join two loops to one in DelayedControls::DelayedControls()\n> > ---\n> > include/libcamera/internal/delayed_controls.h | 87 ++++++\n> > src/libcamera/delayed_controls.cpp | 265 ++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 3 files changed, 353 insertions(+)\n> > create mode 100644 include/libcamera/internal/delayed_controls.h\n> > create mode 100644 src/libcamera/delayed_controls.cpp\n> >\n> > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> > new file mode 100644\n> > index 0000000000000000..e9cb0b90aac99847\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/delayed_controls.h\n> > @@ -0,0 +1,87 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> > + */\n> > +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class V4L2Device;\n> > +\n> > +class DelayedControls\n> > +{\n> > +public:\n> > +\tDelayedControls(V4L2Device *device,\n> > +\t\t\tconst std::unordered_map &delays);\n\nI could imagine this being useful for non-V4L2 devices too. We could\ninstead pass a ControlInfoMap to the constructor, and move the\nsetControls() call to the user of this class. There's no need to do so\nnow (but of course if you think it's a good idea, I'm not asking to\ndelay such rework :-)), but could you capture this in a \\todo comment\n(if you agree with the idea) ?\n\n> > +\n> > +\tvoid reset(ControlList *controls = nullptr);\n> > +\n> > +\tbool push(const ControlList &controls);\n> > +\tControlList get(uint32_t sequence);\n> \n> I liked the set/get pair that we had in StaggeredControls more\n> \n> > +\n> > +\tvoid frameStart(uint32_t sequence);\n> \n> I also think 'write' was probably nicer, but that's minor.\n> I would have called this 'notifySOF()' but that's just my preference,\n> so keep whatever you have here\n\napplyControls() could also be an option. Or, if we decide to move\nsetControls() out of this class, we would need to make this function\nreturn a ControlList.\n\n> > +\n> > +private:\n> > +\tclass ControlInfo\n> \n> As I've commented on v2, this is already the name of a library wide\n> type.\n> \n> To be honest I would drop the 'Control' suffix for all the internal\n> types\n> \n> ControlInfo -> Info\n> ControlsDelays -> Delays\n> ControlsValues -> Values\n> \n> They're internal after all (I know this goes both ways, but for sure\n> they're shorter)\n\nThat could indeed avoid confusion.\n\n> > +\t{\n> > +\tpublic:\n> > +\t\tControlInfo()\n> > +\t\t\t: updated(false)\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\tControlInfo(const ControlValue &v)\n> > +\t\t\t: value(v), updated(true)\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\tControlValue value;\n> > +\t\tbool updated;\n> > +\t};\n\nYou could also make this class inherit from ControlValue, to avoid the\nexplicit .value below.\n\n> > +\n> > +\tstatic constexpr int listSize = 16;\n> > +\tclass ControlArray : public std::array\n\nHow about a name that conveys this is a ring buffer, such as\nControlCircularArray, ControlRingBuffer, CircularArray, ... ? \n\n> > +\t{\n> > +\tpublic:\n> > +\t\tControlInfo &operator[](unsigned int index)\n> > +\t\t{\n> > +\t\t\treturn std::array::operator[](index % listSize);\n> > +\t\t}\n> > +\n> > +\t\tconst ControlInfo &operator[](unsigned int index) const\n> > +\t\t{\n> > +\t\t\treturn std::array::operator[](index % listSize);\n> > +\t\t}\n> > +\t};\n> > +\n> > +\tusing ControlsDelays = std::unordered_map;\n> > +\tusing ControlsValues = std::unordered_map;\n\nAs these two types are only used once, below in the class declaration,\nhow about dropping the aliases ? Aliases are useful to avoid typing\ntypes out explicitly in every location, but they have the drawback of\npossibly obfuscating the code. I would then rename the above ControlInfo\nto Value, as it stores a value.\n\nI wonder if we should bundle the delay and array in a single class, to\nhave a single map (now or on top).\n\nI also wonder if we shouldn't use the integer control ID as the key,\nthis would simplify DelayedControls::queue() that wouldn't have to look\nup the ControlId pointer from the idmap.\n\n> > +\n> > +\tbool queue(const ControlList &controls);\n> > +\n> > +\tstd::mutex lock_;\n\nDo we need a lock, isn't this class used in a single thread ? If we want\nto make it thread-safe, we should document this explicitly for each\nfunction.\n\n> > +\n> > +\tV4L2Device *device_;\n> > +\tControlsDelays delays_;\n> > +\tunsigned int maxDelay_;\n> > +\n> > +\tbool running_;\n> > +\tuint32_t firstSequence_;\n> > +\n> > +\tuint32_t queueCount_;\n> > +\tuint32_t writeCount_;\n> > +\tControlsValues ctrls_;\n> \n> I would then call this 'values_'\n> \n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > new file mode 100644\n> > index 0000000000000000..c810be48a36be515\n> > --- /dev/null\n> > +++ b/src/libcamera/delayed_controls.cpp\n> > @@ -0,0 +1,265 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> > + */\n> > +\n> > +#include \"libcamera/internal/delayed_controls.h\"\n> > +#include \"libcamera/internal/v4l2_device.h\"\n\nThis header...\n\n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/log.h\"\n\n... should go here.\n\n> > +\n> > +/**\n> > + * \\file delayed_controls.h\n> > + * \\brief Helper to deal with controls that are applied with a delay\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(DelayedControls)\n> > +\n> > +/**\n> > + * \\class DelayedControls\n> > + * \\brief Helper to deal with controls that take effect with a delay\n> > + *\n> > + * Some sensor controls take effect with a delay as the sensor needs time to\n> > + * adjust, for example exposure and focus. This is an optional helper class to\n\ns/an optional helper/a helper/ ?\n\n> > + * deal with such controls and the intended users are pipeline handlers.\n> > + *\n> > + * The idea is to extend the concept of the buffer depth of a pipeline the\n> > + * application need to maintain to also cover controls. Just as with buffer\n\ns/need/needs/\n\n> > + * depth if the application keeps the number of requests queued above the\n> > + * control depth the controls are guaranteed to take effect for the correct\n> > + * request. The control depth is determined by the control with the grates\n> \n> 'greatest'\n> \n> I find the reference to the buffer depth more confusing than helping.\n> \n> > + * delay.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a DelayedControls\n>\n> \\brief Construct a DelayedControls instance\n> \n> > + * \\param[in] device The V4L2 device the controls have to be applied to\n> > + * \\param[in] delays Map of the numerical V4L2 control ids to their associated\n> > + * delays (in frames)\n> > + *\n> > + * Only controls specified in \\a delays are handled. If it's desired to mix\n> > + * delayed controls and controls that take effect immediately the immediate\n> > + * controls must be listed in the \\a delays map with a delay value of 0.\n> > + */\n> > +DelayedControls::DelayedControls(V4L2Device *device,\n> > +\t\t\t\t const std::unordered_map &delays)\n> > +\t: device_(device), maxDelay_(0)\n> > +{\n> > +\tconst ControlInfoMap &controls = device_->controls();\n> > +\n> > +\t/*\n> > +\t * Create a map of control ids to delays for controls exposed by the\n> > +\t * device.\n> > +\t */\n> > +\tfor (auto const &delay : delays) {\n> > +\t\tauto itControl = controls.find(delay.first);\n\nAs there's a single iterator in this function we usually use \"it\" or\n\"iter\". Up to you.\n\n> > +\t\tif (itControl == controls.end()) {\n> > +\t\t\tLOG(DelayedControls, Error)\n> > +\t\t\t\t<< \"Delay request for control id \"\n> > +\t\t\t\t<< utils::hex(delay.first)\n> > +\t\t\t\t<< \" but control is not exposed by device \"\n> > +\t\t\t\t<< device_->deviceNode();\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tconst ControlId *id = itControl->first;\n> > +\n> > +\t\tdelays_[id] = delay.second;\n> > +\n> > +\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t<< \"Set a delay of \" << delays_[id]\n> > +\t\t\t<< \" for \" << id->name();\n> > +\n> > +\t\tmaxDelay_ = std::max(maxDelay_, delays_[id]);\n> > +\t}\n> > +\n> > +\treset();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Reset state machine and controls\n> > + * \\param[in] controls Optional list of controls to apply to the device\n> > + *\n> > + * Resets the state machine to a starting position based on control values\n> > + * retrieved from the device. Controls may optionally be set before they are\n> > + * read back using \\a controls.\n> > + */\n> > +void DelayedControls::reset(ControlList *controls)\n> > +{\n> > +\tstd::lock_guard lock(lock_);\n> > +\n> > +\trunning_ = false;\n> > +\tfirstSequence_ = 0;\n> > +\tqueueCount_ = 0;\n> > +\twriteCount_ = 0;\n> > +\n> > +\t/* Set the controls on the device if requested. */\n> > +\tif (controls)\n> > +\t\tdevice_->setControls(controls);\n> \n> Checking for the return value would guarantee Controls not supported\n> by the device are not supplied and not erronously added to the ctrls_\n> map below ?\n\nShould this be moved to the caller ? Looking at how this is used, only\nthe Raspberry Pi pipeline handler calls this function with a non-null\ncontrols parameter, to set initial control values at configure time. It\ndoesn't seem to really fit in the DelayedControls helper.\n\n> > +\n> > +\t/* Retrieve current control values reported by the device. */\n> > +\tstd::vector ids;\n> > +\tfor (auto const &delay : delays_)\n> > +\t\tids.push_back(delay.first->id());\n> > +\n> > +\tControlList devCtrls = device_->getControls(ids);\n\n(This variable could then be called controls)\n\n> > +\n> > +\t/* Seed the control queue with the controls reported by the device. */\n> > +\tctrls_.clear();\n> > +\tfor (const auto &ctrl : devCtrls) {\n> > +\t\tconst ControlId *id = devCtrls.infoMap()->idmap().at(ctrl.first);\n\nYou use devCtrls.infoMap() here, and device_->controls() below. They\nshould be identical (or there's something I don't get :-)), so I think\nthe code would be easier to understand if we used one of the two\nconsistently.\n\n> As .at() with an unsupported id would fail afaict\n> \n> > +\t\tctrls_[id][queueCount_] = ControlInfo(ctrl.second);\n\nMaybe s/queueCount/0/ to make this more explicit ?\n\n> > +\t}\n> > +\n> > +\tqueueCount_++;\n\nYou could then fold this above, making it queueCount_ = 1.\n\n> > +}\n> > +\n> > +/**\n> > + * \\brief Push a set of controls on the queue\n> > + * \\param[in] controls List of controls to add to the device queue\n> > + *\n> > + * Push a set of controls to the control queue. This increases the control queue\n> > + * depth by one.\n> > + *\n> > + * \\returns true if \\a controls are accepted, or false otherwise\n> > + */\n> > +bool DelayedControls::push(const ControlList &controls)\n> > +{\n> > +\tstd::lock_guard lock(lock_);\n> > +\n> > +\treturn queue(controls);\n> > +}\n> > +\n> > +bool DelayedControls::queue(const ControlList &controls)\n> > +{\n> > +\t/* Copy state from previous frame. */\n> > +\tfor (auto &ctrl : ctrls_) {\n> > +\t\tControlInfo &info = ctrl.second[queueCount_];\n> > +\t\tinfo.value = ctrls_[ctrl.first][queueCount_ - 1].value;\n> > +\t\tinfo.updated = false;\n> > +\t}\n> > +\n> > +\t/* Update with new controls. */\n> > +\tconst ControlIdMap &idmap = device_->controls().idmap();\n> > +\tfor (const auto &control : controls) {\n> > +\t\tconst auto &itCtrl = idmap.find(control.first);\n\nSame comment about the iterator name.\n\n> > +\t\tif (itCtrl == idmap.end())\n\nShould a warning message be printed here ?\n\n> > +\t\t\treturn false;\n> > +\n> > +\t\tconst ControlId *id = itCtrl->second;\n> > +\n> > +\t\tif (delays_.find(id) == delays_.end())\n> > +\t\t\treturn false;\n> > +\n> > +\t\tControlInfo &info = ctrls_[id][queueCount_];\n> > +\n> > +\t\tinfo.value = control.second;\n> > +\t\tinfo.updated = true;\n> > +\n> > +\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t<< \"Queuing \" << id->name()\n> > +\t\t\t<< \" to \" << info.value.toString()\n> > +\t\t\t<< \" at index \" << queueCount_;\n> > +\t}\n> > +\n> > +\tqueueCount_++;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Read back controls in effect at a sequence number\n> > + * \\param[in] sequence The sequence number to get controls for\n> > + *\n> > + * Read back what controls where in effect at a specific sequence number. The\n> > + * history is a ring buffer of 16 entries where new and old values coexist. It's\n> > + * the callers responsibility to not read to old sequence numbers that have been\n\ns/to/too/\n\n> > + * pushed out of the history.\n\nWhat history size do we need in practice, do we need to go higher than\nthe maximum delay ? I wonder if we should make this dynamic (or just\nlower, 16 seems fairly large). Making it dynamic could be done on top (a\n\\todo comment would then be nice).\n\n> > + *\n> > + * Historic values are evicted by pushing new values onto the queue using\n> > + * push(). The max history from the current sequence number that yields valid\n> > + * values are thus 16 minus number of controls pushed.\n> > + *\n> > + * \\return The controls at \\a sequence number\n> > + */\n> > +ControlList DelayedControls::get(uint32_t sequence)\n> > +{\n> > +\tstd::lock_guard lock(lock_);\n> > +\n> > +\tuint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> > +\tunsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> > +\n> > +\tControlList out(device_->controls());\n> > +\tfor (const auto &ctrl : ctrls_) {\n> > +\t\tconst ControlId *id = ctrl.first;\n> > +\t\tconst ControlInfo &info = ctrl.second[index];\n> > +\n> > +\t\tout.set(id->id(), info.value);\n> > +\n> > +\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t<< \"Reading \" << id->name()\n> > +\t\t\t<< \" to \" << info.value.toString()\n> > +\t\t\t<< \" at index \" << index;\n> > +\t}\n> > +\n> > +\treturn out;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Inform DelayedControls of the start of a new frame\n> > + * \\param[in] sequence Sequence number of the frame that started\n> > + *\n> > + * Inform the state machine that a new frame has started and of its sequence\n> > + * number. Any user of these helpers is responsible to inform the helper about\n> > + * the start of any frame.This can be connected with ease to the start of a\n> > + * exposure (SOE) V4L2 event.\n> \n> This is all true, this function informs about an SOE events, but\n> what's relevant is that in response to this event a new set of\n> controls are written to the device. It is not mentioned anywhere in\n> documentation.\n\nOnce we agree on the review comments (and a new version is posted to\naddress them if needed), I can help with the documentation if that would\nbe useful.\n\n> > + */\n> > +void DelayedControls::frameStart(uint32_t sequence)\n> > +{\n> > +\tLOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> > +\n> > +\tstd::lock_guard lock(lock_);\n> > +\n> > +\tif (!running_) {\n> > +\t\tfirstSequence_ = sequence;\n> > +\t\trunning_ = true;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Create control list peaking ahead in the value queue to ensure\n> > +\t * values are set in time to satisfy the sensor delay.\n> > +\t */\n> > +\tControlList out(device_->controls());\n> > +\tfor (const auto &ctrl : ctrls_) {\n> > +\t\tconst ControlId *id = ctrl.first;\n> > +\t\tunsigned int delayDiff = maxDelay_ - delays_[id];\n> > +\t\tunsigned int index = std::max(0, writeCount_ - delayDiff);\n\nLet's assume a sensor that only a single control, with a delay of 1\nframe. It has been pre-programmed before stream start with an initial\nvalue X with reset(). Then, two requests are queued before the first\nSOF, with values A and B. push() is called twice, with A and B.\nqueueCount is thus 3 (initial value set to 1 in reset(), and increased\ntwice by push), and the queue contains X, A and B in positions 0, 1 and\n2.\n\nThe first frame start arrives, frameStart(0) is called. writeCount_ is\n0, maxDelay_ is 1, delays_[id] is 1. index will thus be 0, which causes\nX to be written to the sensor.\n\nIsn't that an incorrect behaviour ? Shouldn't B be written instead,\ngiven that the value will take effect in frame 1, which will be captured\nin request B ?\n\n> > +\t\tconst ControlInfo &info = ctrl.second[index];\n> > +\n> > +\t\tif (info.updated) {\n> > +\t\t\tout.set(id->id(), info.value);\n> > +\t\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t\t<< \"Setting \" << id->name()\n> > +\t\t\t\t<< \" to \" << info.value.toString()\n> > +\t\t\t\t<< \" at index \" << index;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\twriteCount_++;\n> > +\n> > +\twhile (writeCount_ >= queueCount_) {\n> > +\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t<< \"Queue is empty, auto queue no-op.\";\n> > +\t\tqueue({});\n> \n> I would check at the beginning of the function if writeCount_ >\n> queueCount_ and in the case exit immediately to avoid pushing a set of\n> empty controls just to create a new slot. If I'm not mistaken it has\n> the same effect ?\n> \n> This would allow you merge queue() and push() in a single function (I\n> still don't like queue({}) and still seems a bit of an hack :)\n> \n> > +\t}\n> > +\n> > +\tdevice_->setControls(&out);\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -12,6 +12,7 @@ libcamera_sources = files([\n> > 'controls.cpp',\n> > 'control_serializer.cpp',\n> > 'control_validator.cpp',\n> > + 'delayed_controls.cpp',\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > 'event_dispatcher.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1B99CBE177\n\tfor ;\n\tSat, 5 Dec 2020 01:23:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0467635F1;\n\tSat, 5 Dec 2020 02:23:27 +0100 (CET)","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 1776D60325\n\tfor ;\n\tSat, 5 Dec 2020 02:23:26 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6C59A2A4;\n\tSat, 5 Dec 2020 02:23:25 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qSTEYJRA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607131405;\n\tbh=v2KpxJWPi1oj4gq+WEX0c6stReX86MArZ3YH+1Ev3UE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qSTEYJRA6rj0A8VYJJOLA3GLXLCncwe2gox4ORFpe2Aqz2v1EwyqJVpPOubDt1njS\n\ts3880QRJqgObWX3IsHxnK6yARAJCwGxVTQJJ/Y6ggQ79RM5NHqoz96XnpODi6RBhws\n\tcotDDmg7UKr0Wn7llohHCYwLm+X34tP+8qDrXm20=","Date":"Sat, 5 Dec 2020 03:23:24 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201205012324.GO4109@pendragon.ideasonboard.com>","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-2-niklas.soderlund@ragnatech.se>\n\t<20201126173859.3b6ppgodjpqviics@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201126173859.3b6ppgodjpqviics@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]