[{"id":28990,"web_url":"https://patchwork.libcamera.org/comment/28990/","msgid":"","date":"2024-03-15T16:15:58","subject":"Re: [PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nNot going to review the logic implementation in detail, it will take\nme some more time as delayed controls are kind of a complex beast, so\nthis will be mostly a stylistic review with some stupid questions here\nand there\n\nOn Wed, Mar 13, 2024 at 01:12:17PM +0100, Stefan Klug wrote:\n> Functional changes:\n> - Requests need to be queued for every frame\n\nDo you mean it is a requirement for the application to queue Request\nfor every frame ?\n\n> - The startup phase is no longer treated as special case\n> - Requests carry a sequence number, so that we can detect when the system\n> gets out of sync\n\nAre you referring to the request sequence number or the frame number ?\n\n> - Controls for a given sequence number can be updated multiple times\n> as long as they are not yet sent out\n> - If it's too late, controls get delayed but they are not lost\n\nWhat do you mean not lost ? Will they be applied anyway ? what about\nthe controls for the next frames ?\n\n> - Requests attached to frame 0 don't get lost\n\nDo you mean the very first request ? Do you mean to apply controls\nbefore streaming is started ?\n\n>\n> Technical notes:\n> A sourceSequence_ replaces the updated flag to be able to track from which\n> frame the update stems. This is needed for the following use case:\n> Assume e.g. On frame 10 an ExposureTime=42 is queued because the user wants\n> manual exposure. Now the agc gets the stats for frame 8 (where auto\n> regulation is still active) and pushes a new ExposureTime for frame 9.\n> Frame 9 was already sent out, so it gets delayed to frame 11 (assuming a\n> delay of 2 on ExposureTime). This would revert the request from frame 10.\n> Taking the sourceSequence into account, the delayed request from frame\n> 9 will be discarded, which is correct.\n\nAlso this is not 100% clear to me. The request for frame 10 that says\n\"manual 42\" should be processed by the IPA/pipeline early enough to\nmake sure the \"ExposureTime=42\" control is emitted early enough for\nbeing applied in time (assuming 2 frames of delay, this mean the\ncontrol should be emitted before frame 8 has started exposing).\n\nIf the AGC gets the stats for frame 8 and computes auto-value for\nframe 11, it means it is late and the IPA is not processing requests\nearly enough ? have you seen this happening ?\n\n>\n> Signed-off-by: Stefan Klug \n> ---\n> include/libcamera/internal/delayed_controls.h | 12 +-\n> src/libcamera/delayed_controls.cpp | 207 ++++++++++++++----\n> 2 files changed, 174 insertions(+), 45 deletions(-)\n>\n> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> index 4f8d2424..2738e8bf 100644\n> --- a/include/libcamera/internal/delayed_controls.h\n> +++ b/include/libcamera/internal/delayed_controls.h\n> @@ -30,25 +30,29 @@ public:\n> \tvoid reset();\n>\n> \tbool push(const ControlList &controls);\n> +\tbool pushForFrame(uint32_t sequence, const ControlList &controls);\n> \tControlList get(uint32_t sequence);\n>\n> \tvoid applyControls(uint32_t sequence);\n>\n> private:\n> +\tbool controlsAreQueuedForFrame(unsigned int frame, const ControlList &controls);\n\nFunctions should be defined after types. Please move this after the\nControlRingBuffer type definition below. Also, this is a very long name :)\n\n> +\n> \tclass Info : public ControlValue\n> \t{\n> \tpublic:\n> \t\tInfo()\n> -\t\t\t: updated(false)\n> +\t\t\t: sourceSequence_(0)\n> \t\t{\n> \t\t}\n>\n> -\t\tInfo(const ControlValue &v, bool updated_ = true)\n> -\t\t\t: ControlValue(v), updated(updated_)\n> +\t\tInfo(const ControlValue &v, std::optional sourceSequence = std::nullopt)\n> +\t\t\t: ControlValue(v), sourceSequence_(sourceSequence)\n> \t\t{\n> \t\t}\n>\n> -\t\tbool updated;\n> +\t\t/* The sequence id, this info stems from*/\n> +\t\tstd::optional sourceSequence_;\n> \t};\n>\n> \t/* \\todo Make the listSize configurable at instance creation time. */\n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> index 86571cd4..59314388 100644\n> --- a/src/libcamera/delayed_controls.cpp\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -115,8 +115,9 @@ DelayedControls::DelayedControls(V4L2Device *device,\n> */\n> void DelayedControls::reset()\n> {\n> -\tqueueIndex_ = 1;\n> -\twriteIndex_ = 0;\n> +\tqueueIndex_ = 0;\n> +\t/* Frames up to maxDelay_ will be based on sensor init values. */\n\nOk, so this is the startup condition!\n\n> +\twriteIndex_ = maxDelay_;\n>\n> \t/* Retrieve control as reported by the device. */\n> \tstd::vector ids;\n> @@ -133,26 +134,129 @@ void DelayedControls::reset()\n> \t\t * Do not mark this control value as updated, it does not need\n> \t\t * to be written to to device on startup.\n\ndoes the comment needs updating too ?\n\n> \t\t */\n> -\t\tvalues_[id][0] = Info(ctrl.second, false);\n> +\t\tvalues_[id][0] = Info(ctrl.second, 0);\n> \t}\n> +\n> +\t/* Copy state from previous frames. */\n\nAre these from the previous frames or simply the initial control values ?\n\n> +\tfor (auto &ctrl : values_) {\n> +\t\tfor (auto i = queueIndex_; i < writeIndex_; i++) {\n\nunsigned int i\n\n> +\t\t\tInfo &info = ctrl.second[i + 1];\n> +\t\t\tinfo = ctrl.second[i];\n> +\t\t\tinfo.sourceSequence_.reset();\n> +\t\t}\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Helper function to check if controls are queued already\n> + * \\param[in] sequence Sequence number to check\n> + * \\param[in] controls List of controls to compare against\n> + *\n> + * This function checks if the controls queued for frame \\a sequence\n> + * are equal to \\a controls. This is helpful in cases where a control algorithm\n> + * unconditionally queues controls for every frame, but always too late.\n> + * In that case this can be used to check if incoming controls are already queued\n> + * or need to be queued for a later frame.\n\nI'm interested to see where and how this is used\n\n> + *\n> + * \\returns true if \\a controls are queued for the given sequence\n, false otherwise\n> + */\n> +bool DelayedControls::controlsAreQueuedForFrame(unsigned int sequence, const ControlList &controls)\n\ncan easily be made < 80 cols.\n\n> +{\n> +\tauto idMap = controls.idMap();\n> +\tif (!idMap) {\n> +\t\tLOG(DelayedControls, Warning) << \" idmap is null\";\n> +\t\treturn false;\n> +\t} else {\n\nNo need for an else branch, you have returned already\n\n> +\t\tfor (const auto &[id, value] : controls) {\n> +\t\t\tif (values_[idMap->at(id)][sequence] != value) {\n> +\t\t\t\treturn false;\n> +\t\t\t}\n\nNo {}\n\n> +\t\t}\n> +\t}\n\nEmpty line\n\n> +\treturn true;\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> + * \\deprecated This function is deprecated in favour of pushForFrame\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> + * \\returns See return value of DelayedControls::pushForFrame\n> */\n> bool DelayedControls::push(const ControlList &controls)\n> {\n> -\t/* Copy state from previous frame. */\n> -\tfor (auto &ctrl : values_) {\n> -\t\tInfo &info = ctrl.second[queueIndex_];\n> -\t\tinfo = values_[ctrl.first][queueIndex_ - 1];\n> -\t\tinfo.updated = false;\n> +\tLOG(DelayedControls, Debug) << \"Using deprecated function push(controls): \" << queueIndex_;\n\nbreak line\n\n> +\treturn pushForFrame(queueIndex_, controls);\n> +}\n> +\n> +/**\n> + * \\brief Push a set of controls for a given frame\n> + * \\param[in] sequence Sequence to push the controls for\n> + * \\param[in] controls List of controls to add to the device queue\n> + *\n> + * Pushes the controls given by \\a controls, to be applied at frame \\a sequence.\n\nApply \\a controls to frame \\a sequence\n\n> + *\n> + * If there are controls already queued for that frame, these get updated.\n\ns/these/they\n\n> + *\n> + * If it's too late for frame \\a sequence (controls are already sent to the sensor),\n> + * the system checks if the controls that where written out for frame \\a sequence\n> + * are the same as the requested ones. In this case, nothing is done.\n> + * If they differ, the controls get queued for the earliest frame possible\n> + * if no other controls with a higher sequence number are queued for that frame already.\n> + *\n> + * \\returns true if \\a controls are accepted, or false otherwise\n> + */\n> +\n\nStray empty line\n\n> +bool DelayedControls::pushForFrame(uint32_t sequence, const ControlList &controls)\n\nCan't you just overload ::push() ?\n\n> +{\n> +\tif (sequence < queueIndex_) {\n> +\t\tLOG(DelayedControls, Debug) << \"Got updated data for frame:\" << sequence;\n> +\t}\n\nNo {}\n\n> +\n> +\tif (sequence > queueIndex_) {\n> +\t\tLOG(DelayedControls, Warning) << \"Hole in queue sequence. This should not happen. Expected: \"\n> +\t\t\t\t\t << queueIndex_ << \" got \" << sequence;\n> +\t}\n\nno {}\nvery long line\n\n> +\n> +\tuint32_t updateIndex = sequence;\n> +\t/* check if its too late for the request */\n> +\tif (sequence < writeIndex_) {\n> +\t\t/* Check if we can safely ignore the request */\n> +\t\tif (controls.empty() || controlsAreQueuedForFrame(sequence, controls)) {\n\nAre we comparing control lists at every push() (so likely for every\nrequest) ?\n\n\n> +\t\t\tif (sequence >= queueIndex_) {\n> +\t\t\t\tqueueIndex_ = sequence + 1;\n> +\t\t\t\treturn true;\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tLOG(DelayedControls, Debug) << \"Controls for frame \" << sequence\n> +\t\t\t\t\t\t << \" are already in flight. Will be queued for frame \" << writeIndex_;\n> +\t\t\tupdateIndex = writeIndex_;\n> +\t\t}\n\nveeeery long lines\n\n> +\t}\n> +\n> +\tif (static_cast(updateIndex) - static_cast(writeIndex_) >= listSize - static_cast(maxDelay_)) {\n\nWhy signed ?\n>\n> +\t\t/* The system is in an undefined state now. This will heal itself, as soon as all controls where rewritten */\n> +\t\tLOG(DelayedControls, Error) << \"Queue length exceeded. The system is out of sync. Index to update:\"\n> +\t\t\t\t\t << updateIndex << \" Next Index to apply: \" << writeIndex_;\n> +\t}\n> +\n> +\t/**\n\n/** is for doxygen\n\n> +\t * Prepare the ringbuffer entries with previous data.\n> +\t * Data up to [writeIndex_] gets prepared in applyControls.\n> +\t */\n> +\tif (updateIndex > writeIndex_ && updateIndex >= queueIndex_) {\n> +\t\tLOG(DelayedControls, Debug) << \"Copy from previous \" << queueIndex_;\n> +\t\tfor (auto i = queueIndex_; i < updateIndex; i++) {\n> +\t\t\t/* Copy state from previous queue. */\n> +\t\t\tfor (auto &ctrl : values_) {\n> +\t\t\t\tauto &ringBuffer = ctrl.second;\n> +\t\t\t\tringBuffer[i] = ringBuffer[i - 1];\n> +\t\t\t\tringBuffer[i].sourceSequence_.reset();\n> +\t\t\t}\n> +\t\t}\n> \t}\n>\n> \t/* Update with new controls. */\n> @@ -167,20 +271,34 @@ bool DelayedControls::push(const ControlList &controls)\n>\n> \t\tconst ControlId *id = it->second;\n>\n> -\t\tif (controlParams_.find(id) == controlParams_.end())\n> -\t\t\treturn false;\n> -\n> -\t\tInfo &info = values_[id][queueIndex_];\n> +\t\tif (controlParams_.find(id) == controlParams_.end()) {\n> +\t\t\tLOG(DelayedControls, Error) << \"Could not find params for control \" << id << \" ignored\";\n\nlong line\n\n> +\t\t\tcontinue;\n> +\t\t}\n\nIs ignoring the control better than erroring out ? Is this worth a\nseparate change ?\n\n>\n> -\t\tinfo = Info(control.second);\n> +\t\tInfo &info = values_[id][updateIndex];\n\nMove after the comment block\n\n> +\t\t/*\n> +\t\t * Update the control only, if the already existing value stems from a request\n\ns/only,/only\n\nlong line\n\n> +\t\t * with a sequence number smaller or equal to the current one\n> +\t\t */\n> +\t\tif (info.sourceSequence_.value_or(0) <= sequence) {\n> +\t\t\tinfo = Info(control.second, sequence);\n>\n> -\t\tLOG(DelayedControls, Debug)\n> -\t\t\t<< \"Queuing \" << id->name()\n> -\t\t\t<< \" to \" << info.toString()\n> -\t\t\t<< \" at index \" << queueIndex_;\n> +\t\t\tLOG(DelayedControls, Debug)\n> +\t\t\t\t<< \"Queuing \" << id->name()\n> +\t\t\t\t<< \" to \" << info.toString()\n> +\t\t\t\t<< \" at index \" << updateIndex;\n> +\t\t} else {\n> +\t\t\tLOG(DelayedControls, Warning)\n> +\t\t\t\t<< \"Skipped update \" << id->name()\n> +\t\t\t\t<< \" at index \" << updateIndex;\n> +\t\t}\n> \t}\n>\n> -\tqueueIndex_++;\n> +\tLOG(DelayedControls, Debug) << \"Queued frame: \" << queueIndex_ << \" it will be active in \" << updateIndex;\n\nvery long line\n\n> +\n> +\tif (sequence >= queueIndex_)\n> +\t\tqueueIndex_ = sequence + 1;\n>\n> \treturn true;\n> }\n> @@ -202,19 +320,17 @@ bool DelayedControls::push(const ControlList &controls)\n> */\n> ControlList DelayedControls::get(uint32_t sequence)\n> {\n> -\tunsigned int index = std::max(0, sequence - maxDelay_);\n> -\n\nThis can now go because the initial frames are now populated, right ?\n\n> \tControlList out(device_->controls());\n> \tfor (const auto &ctrl : values_) {\n> \t\tconst ControlId *id = ctrl.first;\n> -\t\tconst Info &info = ctrl.second[index];\n> +\t\tconst Info &info = ctrl.second[sequence];\n>\n> \t\tout.set(id->id(), info);\n>\n> \t\tLOG(DelayedControls, Debug)\n> \t\t\t<< \"Reading \" << id->name()\n> \t\t\t<< \" to \" << info.toString()\n> -\t\t\t<< \" at index \" << index;\n> +\t\t\t<< \" at index \" << sequence;\n> \t}\n>\n> \treturn out;\n> @@ -222,16 +338,21 @@ ControlList DelayedControls::get(uint32_t sequence)\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> + * \\param[in] sequence Sequence number of the frame that started (0-based)\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> + * Inform the state machine that a new frame has started to arrive at the receiver\n> + * (e.g. the sensor started to clock out pixels) and of its sequence\n> + * number. This is usually the earliest point in time to update registers in the\n\nNot sure this comment change is necessary\n\n> + * sensor for upcoming frames. Any user of these helpers is responsible to inform\n> + * the helper about the start of any frame. This can be connected with ease to\n> + * the start of a exposure (SOE) V4L2 event.\n\nstay in 80-col, please\n\n> */\n> void DelayedControls::applyControls(uint32_t sequence)\n> {\n> -\tLOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> +\tLOG(DelayedControls, Debug) << \"Apply controls \" << sequence;\n> +\tif (sequence != writeIndex_ - maxDelay_) {\n> +\t\tLOG(DelayedControls, Warning) << \"Sequence and writeIndex are out of sync. Expected seq: \" << writeIndex_ - maxDelay_ << \" got \" << sequence;\n\nNot sure I get why this is an error. Is this to guarantee the IPA\nalways stays exactly maxDelay_ frames in advance ? can anything like\nthis be guaranteed ?\n\n> +\t}\n\nno {}\nvery long line\n\n>\n> \t/*\n> \t * Create control list peeking ahead in the value queue to ensure\n> @@ -241,10 +362,10 @@ void DelayedControls::applyControls(uint32_t sequence)\n> \tfor (auto &ctrl : values_) {\n> \t\tconst ControlId *id = ctrl.first;\n> \t\tunsigned int delayDiff = maxDelay_ - controlParams_[id].delay;\n> -\t\tunsigned int index = std::max(0, writeIndex_ - delayDiff);\n> +\t\tunsigned int index = writeIndex_ - delayDiff;\n> \t\tInfo &info = ctrl.second[index];\n>\n> -\t\tif (info.updated) {\n> +\t\tif (info.sourceSequence_.has_value()) {\n> \t\t\tif (controlParams_[id].priorityWrite) {\n> \t\t\t\t/*\n> \t\t\t\t * This control must be written now, it could\n> @@ -262,21 +383,25 @@ void DelayedControls::applyControls(uint32_t sequence)\n> \t\t\t}\n>\n> \t\t\tLOG(DelayedControls, Debug)\n> -\t\t\t\t<< \"Setting \" << id->name()\n> -\t\t\t\t<< \" to \" << info.toString()\n> -\t\t\t\t<< \" at index \" << index;\n> -\n> -\t\t\t/* Done with this update, so mark as completed. */\n> -\t\t\tinfo.updated = false;\n> +\t\t\t\t<< \"Writing \" << id->name()\n> +\t\t\t\t<< \" (\" << info.toString() << \") \"\n> +\t\t\t\t<< \" for frame \" << index;\n> \t\t}\n> \t}\n>\n> -\twriteIndex_ = sequence + 1;\n> +\tauto oldWriteIndex = writeIndex_;\n> +\twriteIndex_ = sequence + maxDelay_ + 1;\n>\n> -\twhile (writeIndex_ > queueIndex_) {\n> +\tif (writeIndex_ >= queueIndex_ && writeIndex_ > oldWriteIndex) {\n> \t\tLOG(DelayedControls, Debug)\n> -\t\t\t<< \"Queue is empty, auto queue no-op.\";\n> -\t\tpush({});\n> +\t\t\t<< \"Index \" << writeIndex_ << \" is not yet queued. Prepare with old state\";\n> +\t\t/* Copy state from previous frames without resetting the sourceSequence */\n> +\t\tfor (auto &ctrl : values_) {\n> +\t\t\tfor (auto i = oldWriteIndex; i < writeIndex_; i++) {\n> +\t\t\t\tInfo &info = ctrl.second[i + 1];\n> +\t\t\t\tinfo = values_[ctrl.first][i];\n> +\t\t\t}\n> +\t\t}\n\nMy main worries is now that, if I understand it right, the system\nalways tries to keep a maxDelay number of frames between the last\ncontrols written to the sensor and the head of the queue, and to do so\nit duplicate controls at every push() and apply(). This happens for\n-every- frame and -every- request if I got it right ? Am I too\nover-concerned this is expensive ?\n\n> \t}\n>\n> \tdevice_->setControls(&out);\n> --\n> 2.40.1\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 16647BD808\n\tfor ;\n\tFri, 15 Mar 2024 16:16:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26D9B6294A;\n\tFri, 15 Mar 2024 17:16:04 +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 2D73F61C65\n\tfor ;\n\tFri, 15 Mar 2024 17:16:02 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 054262E7;\n\tFri, 15 Mar 2024 17:15:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bf6T4Qv2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710519338;\n\tbh=RuiPGmFW9TeG5SHAs6LbJRefRunp6oaZEcFmeMnHhlc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bf6T4Qv2CXOGKKGXJQZVsI42zkwHWFdClvpYEY0qexIzUJcEuivZaJCMF4euAlJn4\n\tAxaPx6Brkff0XTCCF6gyinawkdacGgr08UxMq9HbsOkiGw3hRzqKc7cFR0iFA9Mrjq\n\tYebjBgppIohATi7x3iqLbyqn+TPavIA1S0+J2qOc=","Date":"Fri, 15 Mar 2024 17:15:58 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Subject":"Re: [PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","Message-ID":"","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-7-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240313121223.138150-7-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28997,"web_url":"https://patchwork.libcamera.org/comment/28997/","msgid":"<20240318132236.fptxax3pwsiagmiq@jasper>","date":"2024-03-18T13:22:36","subject":"Re: [PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nthank you for the review. Sorry for all the style issues. I'll fix them\nasap. \n\nOn Fri, Mar 15, 2024 at 05:15:58PM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> Not going to review the logic implementation in detail, it will take\n> me some more time as delayed controls are kind of a complex beast, so\n> this will be mostly a stylistic review with some stupid questions here\n> and there\n> \n> On Wed, Mar 13, 2024 at 01:12:17PM +0100, Stefan Klug wrote:\n> > Functional changes:\n> > - Requests need to be queued for every frame\n> \n> Do you mean it is a requirement for the application to queue Request\n> for every frame ?\n\nYes, that's actually no change. The reason why I wrote that, is that in\nthe old delayed controls tests, there was always a call to\napplyControls() before the first call to push().\n\n> \n> > - The startup phase is no longer treated as special case\n> > - Requests carry a sequence number, so that we can detect when the system\n> > gets out of sync\n> \n> Are you referring to the request sequence number or the frame number ?\n\nAre there actually two different groups of numbers? If yes, I miss something\nconceptually here. The seuence number is meant to be as in \"This is the\nrequest with seq x, which needs to be applied for frame x\"\n\n> \n> > - Controls for a given sequence number can be updated multiple times\n> > as long as they are not yet sent out\n> > - If it's too late, controls get delayed but they are not lost\n> \n> What do you mean not lost ? Will they be applied anyway ? what about\n> the controls for the next frames ?\n\nIf you request a control for frame 8, but the controls for that frame\nare already sent to the sensor, the control will be applied at the\nearliest possible frame (e.g. frame 10). If there are already request\nqueued for frame 9 or 10 the request from frame 8 is discarded.\n\n> \n> > - Requests attached to frame 0 don't get lost\n> \n> Do you mean the very first request ? Do you mean to apply controls\n> before streaming is started ?\n> \n\nYes, controls that are set on the very first request. I added a seperate\ntestcase for that in the next version. Implicitly it was tested in \nsingleControlWithDelayStartUp()\n\n> >\n> > Technical notes:\n> > A sourceSequence_ replaces the updated flag to be able to track from which\n> > frame the update stems. This is needed for the following use case:\n> > Assume e.g. On frame 10 an ExposureTime=42 is queued because the user wants\n> > manual exposure. Now the agc gets the stats for frame 8 (where auto\n> > regulation is still active) and pushes a new ExposureTime for frame 9.\n> > Frame 9 was already sent out, so it gets delayed to frame 11 (assuming a\n> > delay of 2 on ExposureTime). This would revert the request from frame 10.\n> > Taking the sourceSequence into account, the delayed request from frame\n> > 9 will be discarded, which is correct.\n> \n> Also this is not 100% clear to me. The request for frame 10 that says\n> \"manual 42\" should be processed by the IPA/pipeline early enough to\n> make sure the \"ExposureTime=42\" control is emitted early enough for\n> being applied in time (assuming 2 frames of delay, this mean the\n> control should be emitted before frame 8 has started exposing).\n> \n> If the AGC gets the stats for frame 8 and computes auto-value for\n> frame 11, it means it is late and the IPA is not processing requests\n> early enough ? have you seen this happening ?\n\nAs the ISP is acting in a closed loop, it can only react on stats that\nwere calculated for the last frame that entered the system. So it is by\ndefinition always too late. In the example above, the manual 42 can\nactually be sent out early enough because it doesn't require information\nfrom the stats module. This is the reason for patch 11/12 \"rkisp1: Fix\nper-frame-controls\"\n\n> \n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > include/libcamera/internal/delayed_controls.h | 12 +-\n> > src/libcamera/delayed_controls.cpp | 207 ++++++++++++++----\n> > 2 files changed, 174 insertions(+), 45 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> > index 4f8d2424..2738e8bf 100644\n> > --- a/include/libcamera/internal/delayed_controls.h\n> > +++ b/include/libcamera/internal/delayed_controls.h\n> > @@ -30,25 +30,29 @@ public:\n> > \tvoid reset();\n> >\n> > \tbool push(const ControlList &controls);\n> > +\tbool pushForFrame(uint32_t sequence, const ControlList &controls);\n> > \tControlList get(uint32_t sequence);\n> >\n> > \tvoid applyControls(uint32_t sequence);\n> >\n> > private:\n> > +\tbool controlsAreQueuedForFrame(unsigned int frame, const ControlList &controls);\n> \n> Functions should be defined after types. Please move this after the\n> ControlRingBuffer type definition below. Also, this is a very long name :)\n> \n> > +\n> > \tclass Info : public ControlValue\n> > \t{\n> > \tpublic:\n> > \t\tInfo()\n> > -\t\t\t: updated(false)\n> > +\t\t\t: sourceSequence_(0)\n> > \t\t{\n> > \t\t}\n> >\n> > -\t\tInfo(const ControlValue &v, bool updated_ = true)\n> > -\t\t\t: ControlValue(v), updated(updated_)\n> > +\t\tInfo(const ControlValue &v, std::optional sourceSequence = std::nullopt)\n> > +\t\t\t: ControlValue(v), sourceSequence_(sourceSequence)\n> > \t\t{\n> > \t\t}\n> >\n> > -\t\tbool updated;\n> > +\t\t/* The sequence id, this info stems from*/\n> > +\t\tstd::optional sourceSequence_;\n> > \t};\n> >\n> > \t/* \\todo Make the listSize configurable at instance creation time. */\n> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > index 86571cd4..59314388 100644\n> > --- a/src/libcamera/delayed_controls.cpp\n> > +++ b/src/libcamera/delayed_controls.cpp\n> > @@ -115,8 +115,9 @@ DelayedControls::DelayedControls(V4L2Device *device,\n> > */\n> > void DelayedControls::reset()\n> > {\n> > -\tqueueIndex_ = 1;\n> > -\twriteIndex_ = 0;\n> > +\tqueueIndex_ = 0;\n> > +\t/* Frames up to maxDelay_ will be based on sensor init values. */\n> \n> Ok, so this is the startup condition!\n\nI'm not sure if I got you right here. Would it be clearer to say\n\"Frames up to maxDelay_ will be based on the values obtained in reset()?\n\n> \n> > +\twriteIndex_ = maxDelay_;\n> >\n> > \t/* Retrieve control as reported by the device. */\n> > \tstd::vector ids;\n> > @@ -133,26 +134,129 @@ void DelayedControls::reset()\n> > \t\t * Do not mark this control value as updated, it does not need\n> > \t\t * to be written to to device on startup.\n> \n> does the comment needs updating too ?\n> \n\nhm, I removed it altogether. There is nothing special to be said here.\n\n> > \t\t */\n> > -\t\tvalues_[id][0] = Info(ctrl.second, false);\n> > +\t\tvalues_[id][0] = Info(ctrl.second, 0);\n> > \t}\n> > +\n> > +\t/* Copy state from previous frames. */\n> \n> Are these from the previous frames or simply the initial control values ?\n\nYes, initial ones. I updated the comment and factored the loop out.\n\n> \n> > +\tfor (auto &ctrl : values_) {\n> > +\t\tfor (auto i = queueIndex_; i < writeIndex_; i++) {\n> \n> unsigned int i\n> \n> > +\t\t\tInfo &info = ctrl.second[i + 1];\n> > +\t\t\tinfo = ctrl.second[i];\n> > +\t\t\tinfo.sourceSequence_.reset();\n> > +\t\t}\n> > +\t}\n> > +}\n> > +\n> > +/**\n> > + * \\brief Helper function to check if controls are queued already\n> > + * \\param[in] sequence Sequence number to check\n> > + * \\param[in] controls List of controls to compare against\n> > + *\n> > + * This function checks if the controls queued for frame \\a sequence\n> > + * are equal to \\a controls. This is helpful in cases where a control algorithm\n> > + * unconditionally queues controls for every frame, but always too late.\n> > + * In that case this can be used to check if incoming controls are already queued\n> > + * or need to be queued for a later frame.\n> \n> I'm interested to see where and how this is used\n> \n> > + *\n> > + * \\returns true if \\a controls are queued for the given sequence\n> , false otherwise\n> > + */\n> > +bool DelayedControls::controlsAreQueuedForFrame(unsigned int sequence, const ControlList &controls)\n> \n> can easily be made < 80 cols.\n> \n> > +{\n> > +\tauto idMap = controls.idMap();\n> > +\tif (!idMap) {\n> > +\t\tLOG(DelayedControls, Warning) << \" idmap is null\";\n> > +\t\treturn false;\n> > +\t} else {\n> \n> No need for an else branch, you have returned already\n> \n> > +\t\tfor (const auto &[id, value] : controls) {\n> > +\t\t\tif (values_[idMap->at(id)][sequence] != value) {\n> > +\t\t\t\treturn false;\n> > +\t\t\t}\n> \n> No {}\n> \n> > +\t\t}\n> > +\t}\n> \n> Empty line\n> \n> > +\treturn true;\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> > + * \\deprecated This function is deprecated in favour of pushForFrame\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> > + * \\returns See return value of DelayedControls::pushForFrame\n> > */\n> > bool DelayedControls::push(const ControlList &controls)\n> > {\n> > -\t/* Copy state from previous frame. */\n> > -\tfor (auto &ctrl : values_) {\n> > -\t\tInfo &info = ctrl.second[queueIndex_];\n> > -\t\tinfo = values_[ctrl.first][queueIndex_ - 1];\n> > -\t\tinfo.updated = false;\n> > +\tLOG(DelayedControls, Debug) << \"Using deprecated function push(controls): \" << queueIndex_;\n> \n> break line\n> \n> > +\treturn pushForFrame(queueIndex_, controls);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Push a set of controls for a given frame\n> > + * \\param[in] sequence Sequence to push the controls for\n> > + * \\param[in] controls List of controls to add to the device queue\n> > + *\n> > + * Pushes the controls given by \\a controls, to be applied at frame \\a sequence.\n> \n> Apply \\a controls to frame \\a sequence\n\nIsn't that a different meaning? I'm not happy with my sentence either.\nWhat about: Store \\a controls to be applied for frame \\a sequence.\n\n> \n> > + *\n> > + * If there are controls already queued for that frame, these get updated.\n> \n> s/these/they\n> \n> > + *\n> > + * If it's too late for frame \\a sequence (controls are already sent to the sensor),\n> > + * the system checks if the controls that where written out for frame \\a sequence\n> > + * are the same as the requested ones. In this case, nothing is done.\n> > + * If they differ, the controls get queued for the earliest frame possible\n> > + * if no other controls with a higher sequence number are queued for that frame already.\n> > + *\n> > + * \\returns true if \\a controls are accepted, or false otherwise\n> > + */\n> > +\n> \n> Stray empty line\n> \n> > +bool DelayedControls::pushForFrame(uint32_t sequence, const ControlList &controls)\n> \n> Can't you just overload ::push() ?\n\nSure, I can do that. For my brain it is easier to read\nd->pushForFrame(x, ctrls)\nthan\nd->push(x, ctrls)\nBut thats most likely a matter of taste (or I worked too much with\nApple's macOS APIs)\n\n> \n> > +{\n> > +\tif (sequence < queueIndex_) {\n> > +\t\tLOG(DelayedControls, Debug) << \"Got updated data for frame:\" << sequence;\n> > +\t}\n> \n> No {}\n> \n> > +\n> > +\tif (sequence > queueIndex_) {\n> > +\t\tLOG(DelayedControls, Warning) << \"Hole in queue sequence. This should not happen. Expected: \"\n> > +\t\t\t\t\t << queueIndex_ << \" got \" << sequence;\n> > +\t}\n> \n> no {}\n> very long line\n> \n> > +\n> > +\tuint32_t updateIndex = sequence;\n> > +\t/* check if its too late for the request */\n> > +\tif (sequence < writeIndex_) {\n> > +\t\t/* Check if we can safely ignore the request */\n> > +\t\tif (controls.empty() || controlsAreQueuedForFrame(sequence, controls)) {\n> \n> Are we comparing control lists at every push() (so likely for every\n> request) ?\n\nOnly if we are too late. But in a ISP closed loop, that might happen\ncontinuously.\n\n> \n> \n> > +\t\t\tif (sequence >= queueIndex_) {\n> > +\t\t\t\tqueueIndex_ = sequence + 1;\n> > +\t\t\t\treturn true;\n> > +\t\t\t}\n> > +\t\t} else {\n> > +\t\t\tLOG(DelayedControls, Debug) << \"Controls for frame \" << sequence\n> > +\t\t\t\t\t\t << \" are already in flight. Will be queued for frame \" << writeIndex_;\n> > +\t\t\tupdateIndex = writeIndex_;\n> > +\t\t}\n> \n> veeeery long lines\n> \n> > +\t}\n> > +\n> > +\tif (static_cast(updateIndex) - static_cast(writeIndex_) >= listSize - static_cast(maxDelay_)) {\n> \n> Why signed ?\n\nYep, updateIndex is always >= writeIndex. You are right. I changed\nlistSize, to be unsigned also.\n\n> >\n> > +\t\t/* The system is in an undefined state now. This will heal itself, as soon as all controls where rewritten */\n> > +\t\tLOG(DelayedControls, Error) << \"Queue length exceeded. The system is out of sync. Index to update:\"\n> > +\t\t\t\t\t << updateIndex << \" Next Index to apply: \" << writeIndex_;\n> > +\t}\n> > +\n> > +\t/**\n> \n> /** is for doxygen\n> \n> > +\t * Prepare the ringbuffer entries with previous data.\n> > +\t * Data up to [writeIndex_] gets prepared in applyControls.\n> > +\t */\n> > +\tif (updateIndex > writeIndex_ && updateIndex >= queueIndex_) {\n> > +\t\tLOG(DelayedControls, Debug) << \"Copy from previous \" << queueIndex_;\n> > +\t\tfor (auto i = queueIndex_; i < updateIndex; i++) {\n> > +\t\t\t/* Copy state from previous queue. */\n> > +\t\t\tfor (auto &ctrl : values_) {\n> > +\t\t\t\tauto &ringBuffer = ctrl.second;\n> > +\t\t\t\tringBuffer[i] = ringBuffer[i - 1];\n> > +\t\t\t\tringBuffer[i].sourceSequence_.reset();\n> > +\t\t\t}\n> > +\t\t}\n> > \t}\n> >\n> > \t/* Update with new controls. */\n> > @@ -167,20 +271,34 @@ bool DelayedControls::push(const ControlList &controls)\n> >\n> > \t\tconst ControlId *id = it->second;\n> >\n> > -\t\tif (controlParams_.find(id) == controlParams_.end())\n> > -\t\t\treturn false;\n> > -\n> > -\t\tInfo &info = values_[id][queueIndex_];\n> > +\t\tif (controlParams_.find(id) == controlParams_.end()) {\n> > +\t\t\tLOG(DelayedControls, Error) << \"Could not find params for control \" << id << \" ignored\";\n> \n> long line\n> \n> > +\t\t\tcontinue;\n> > +\t\t}\n> \n> Is ignoring the control better than erroring out ? Is this worth a\n> separate change ?\n> \n> >\n> > -\t\tinfo = Info(control.second);\n> > +\t\tInfo &info = values_[id][updateIndex];\n> \n> Move after the comment block\n> \n> > +\t\t/*\n> > +\t\t * Update the control only, if the already existing value stems from a request\n> \n> s/only,/only\n> \n> long line\n> \n> > +\t\t * with a sequence number smaller or equal to the current one\n> > +\t\t */\n> > +\t\tif (info.sourceSequence_.value_or(0) <= sequence) {\n> > +\t\t\tinfo = Info(control.second, sequence);\n> >\n> > -\t\tLOG(DelayedControls, Debug)\n> > -\t\t\t<< \"Queuing \" << id->name()\n> > -\t\t\t<< \" to \" << info.toString()\n> > -\t\t\t<< \" at index \" << queueIndex_;\n> > +\t\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t\t<< \"Queuing \" << id->name()\n> > +\t\t\t\t<< \" to \" << info.toString()\n> > +\t\t\t\t<< \" at index \" << updateIndex;\n> > +\t\t} else {\n> > +\t\t\tLOG(DelayedControls, Warning)\n> > +\t\t\t\t<< \"Skipped update \" << id->name()\n> > +\t\t\t\t<< \" at index \" << updateIndex;\n> > +\t\t}\n> > \t}\n> >\n> > -\tqueueIndex_++;\n> > +\tLOG(DelayedControls, Debug) << \"Queued frame: \" << queueIndex_ << \" it will be active in \" << updateIndex;\n> \n> very long line\n> \n> > +\n> > +\tif (sequence >= queueIndex_)\n> > +\t\tqueueIndex_ = sequence + 1;\n> >\n> > \treturn true;\n> > }\n> > @@ -202,19 +320,17 @@ bool DelayedControls::push(const ControlList &controls)\n> > */\n> > ControlList DelayedControls::get(uint32_t sequence)\n> > {\n> > -\tunsigned int index = std::max(0, sequence - maxDelay_);\n> > -\n> \n> This can now go because the initial frames are now populated, right ?\n> \n> > \tControlList out(device_->controls());\n> > \tfor (const auto &ctrl : values_) {\n> > \t\tconst ControlId *id = ctrl.first;\n> > -\t\tconst Info &info = ctrl.second[index];\n> > +\t\tconst Info &info = ctrl.second[sequence];\n> >\n> > \t\tout.set(id->id(), info);\n> >\n> > \t\tLOG(DelayedControls, Debug)\n> > \t\t\t<< \"Reading \" << id->name()\n> > \t\t\t<< \" to \" << info.toString()\n> > -\t\t\t<< \" at index \" << index;\n> > +\t\t\t<< \" at index \" << sequence;\n> > \t}\n> >\n> > \treturn out;\n> > @@ -222,16 +338,21 @@ ControlList DelayedControls::get(uint32_t sequence)\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> > + * \\param[in] sequence Sequence number of the frame that started (0-based)\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> > + * Inform the state machine that a new frame has started to arrive at the receiver\n> > + * (e.g. the sensor started to clock out pixels) and of its sequence\n> > + * number. This is usually the earliest point in time to update registers in the\n> \n> Not sure this comment change is necessary\n\nFor me it was unclear if \"a new frame has started\" meant:\n\n a) That we are now setting controls to start the exposure of that new frame\n b) That the sensor started exposing that frame\n c) That a new frame started to arrive on the host\n\nNative speakers? Any better ideas?\n\n> \n> > + * sensor for upcoming frames. Any user of these helpers is responsible to inform\n> > + * the helper about the start of any frame. This can be connected with ease to\n> > + * the start of a exposure (SOE) V4L2 event.\n> \n> stay in 80-col, please\n> \n> > */\n> > void DelayedControls::applyControls(uint32_t sequence)\n> > {\n> > -\tLOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> > +\tLOG(DelayedControls, Debug) << \"Apply controls \" << sequence;\n> > +\tif (sequence != writeIndex_ - maxDelay_) {\n> > +\t\tLOG(DelayedControls, Warning) << \"Sequence and writeIndex are out of sync. Expected seq: \" << writeIndex_ - maxDelay_ << \" got \" << sequence;\n> \n> Not sure I get why this is an error. Is this to guarantee the IPA\n> always stays exactly maxDelay_ frames in advance ? can anything like\n> this be guaranteed ?\n> \n\nIn general I would say so. applyControls is normally called as\napplyControls(0)\napplyControls(1)\napplyControls(2)\n\nThis warning is there to inform about missed applyControls. E.g.\napplyControls(1) <-- first warning. Index 0 missed\napplyControls(3) <-- secod warning. Index 2 missed\n\nIf we want that to be acceptable, we would need to accumulate all control\nchanges from the missed frames...\n\n> > +\t}\n> \n> no {}\n> very long line\n> \n> >\n> > \t/*\n> > \t * Create control list peeking ahead in the value queue to ensure\n> > @@ -241,10 +362,10 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > \tfor (auto &ctrl : values_) {\n> > \t\tconst ControlId *id = ctrl.first;\n> > \t\tunsigned int delayDiff = maxDelay_ - controlParams_[id].delay;\n> > -\t\tunsigned int index = std::max(0, writeIndex_ - delayDiff);\n> > +\t\tunsigned int index = writeIndex_ - delayDiff;\n> > \t\tInfo &info = ctrl.second[index];\n> >\n> > -\t\tif (info.updated) {\n> > +\t\tif (info.sourceSequence_.has_value()) {\n> > \t\t\tif (controlParams_[id].priorityWrite) {\n> > \t\t\t\t/*\n> > \t\t\t\t * This control must be written now, it could\n> > @@ -262,21 +383,25 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > \t\t\t}\n> >\n> > \t\t\tLOG(DelayedControls, Debug)\n> > -\t\t\t\t<< \"Setting \" << id->name()\n> > -\t\t\t\t<< \" to \" << info.toString()\n> > -\t\t\t\t<< \" at index \" << index;\n> > -\n> > -\t\t\t/* Done with this update, so mark as completed. */\n> > -\t\t\tinfo.updated = false;\n> > +\t\t\t\t<< \"Writing \" << id->name()\n> > +\t\t\t\t<< \" (\" << info.toString() << \") \"\n> > +\t\t\t\t<< \" for frame \" << index;\n> > \t\t}\n> > \t}\n> >\n> > -\twriteIndex_ = sequence + 1;\n> > +\tauto oldWriteIndex = writeIndex_;\n> > +\twriteIndex_ = sequence + maxDelay_ + 1;\n> >\n> > -\twhile (writeIndex_ > queueIndex_) {\n> > +\tif (writeIndex_ >= queueIndex_ && writeIndex_ > oldWriteIndex) {\n> > \t\tLOG(DelayedControls, Debug)\n> > -\t\t\t<< \"Queue is empty, auto queue no-op.\";\n> > -\t\tpush({});\n> > +\t\t\t<< \"Index \" << writeIndex_ << \" is not yet queued. Prepare with old state\";\n> > +\t\t/* Copy state from previous frames without resetting the sourceSequence */\n> > +\t\tfor (auto &ctrl : values_) {\n> > +\t\t\tfor (auto i = oldWriteIndex; i < writeIndex_; i++) {\n> > +\t\t\t\tInfo &info = ctrl.second[i + 1];\n> > +\t\t\t\tinfo = values_[ctrl.first][i];\n> > +\t\t\t}\n> > +\t\t}\n> \n> My main worries is now that, if I understand it right, the system\n> always tries to keep a maxDelay number of frames between the last\n> controls written to the sensor and the head of the queue, and to do so\n> it duplicate controls at every push() and apply(). This happens for\n> -every- frame and -every- request if I got it right ? Am I too\n> over-concerned this is expensive ?\n\nTo my understanding that is no change to the way it was before. Before,\nthat copying happed due to push({}), thereby messing up the\nsynchronicity of push() and apply().\n\n> \n> > \t}\n> >\n> > \tdevice_->setControls(&out);\n> > --\n> > 2.40.1\n> >\n\nThank you very much for all the feedback. It's time for a v3. And next\ntime without the style issues :-)\n\nCheers,\nStefan","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 A5CC4BD160\n\tfor ;\n\tMon, 18 Mar 2024 13:22:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C899162CA9;\n\tMon, 18 Mar 2024 14:22:41 +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 1873E61C6D\n\tfor ;\n\tMon, 18 Mar 2024 14:22:40 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:29d6:8b80:db5a:3cdc])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D32122A5;\n\tMon, 18 Mar 2024 14:22:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"V7WPLOsh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710768133;\n\tbh=eTvikn9jXs1++BbScWJiEDHcOxw87izcUrdXemp2LWU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V7WPLOshJPmW92HRNVP+PBkIk0aDSLDS+fYZf2za8HkxHkDmVAg2IqPMzrrejF4nK\n\tQPGrvndGpRwMLilMsPwPu4L3dDcIhrEcUgufM/adYFXBbAAiJMS8uNgB0LFcgmrxj5\n\t2YaPUgU9Vyb0b3J/n3fSllO6QgS/wyXPzTzsEW9k=","Date":"Mon, 18 Mar 2024 14:22:36 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","Message-ID":"<20240318132236.fptxax3pwsiagmiq@jasper>","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-7-stefan.klug@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29019,"web_url":"https://patchwork.libcamera.org/comment/29019/","msgid":"<2bcf6izyvs5u27xfvkirit3ua5mdp65gjhyujdidnt4qiihccm@jfbsz4lc5wos>","date":"2024-03-21T09:20:34","subject":"Re: [PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Mon, Mar 18, 2024 at 02:22:36PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> thank you for the review. Sorry for all the style issues. I'll fix them\n> asap.\n>\n> On Fri, Mar 15, 2024 at 05:15:58PM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > Not going to review the logic implementation in detail, it will take\n> > me some more time as delayed controls are kind of a complex beast, so\n> > this will be mostly a stylistic review with some stupid questions here\n> > and there\n> >\n> > On Wed, Mar 13, 2024 at 01:12:17PM +0100, Stefan Klug wrote:\n> > > Functional changes:\n> > > - Requests need to be queued for every frame\n> >\n> > Do you mean it is a requirement for the application to queue Request\n> > for every frame ?\n>\n> Yes, that's actually no change. The reason why I wrote that, is that in\n> the old delayed controls tests, there was always a call to\n> applyControls() before the first call to push().\n>\n\nJust to clarify, is this a requirement for PFC to happen, or in\ngeneral ? (see below)\n\n> >\n> > > - The startup phase is no longer treated as special case\n> > > - Requests carry a sequence number, so that we can detect when the system\n> > > gets out of sync\n> >\n> > Are you referring to the request sequence number or the frame number ?\n>\n> Are there actually two different groups of numbers? If yes, I miss something\n> conceptually here. The seuence number is meant to be as in \"This is the\n> request with seq x, which needs to be applied for frame x\"\n>\n\nI guess this is one of the thing we should clarify first before going\nfurther in the design.\n\nBoth Request and frames have a sequence_number.\n\nRequests are queued to the camera by the application. Their sequence\nnumber increments for each Request created by the Camera. There are no\nrequirments on the timing the application queues requests to the\ncamera, one example is RPi Zero users that due to limited memory only\nseldom queue a Request\n\nFrames are produced by the sensor at a fixed time interval, and their\nsequence number increments at every frame produced by the sensor (or better,\nreceived by the CSI-2 rx, as sometimes frame gets dropped and gaps are\ncreated, anyway..)\n\nIf an application wants full per-frame control, it needs to queue\nenough requests to compensate for the pipeline depth which counts for:\n\n- a full frame time as sensor's settings apply to the 'next' frame, hence\n the one that is getting exposed right now is 'gone'\n- on some ISP some frames of delay to generate statistics (iirc IPU3\n has 1 frame delay between the frame that gets processed and the one\n for which statistics are generated)\n- the sensor's maximum control delay\n\nA reasonable number (to be used as an example here) for the pipeline\ndepth is something between 4 and 6 frames.\n\nNow, if an application wants full per frame control it has to queue\nrequests early enough for the desired controls associated with a\nrequest to be realized, so it has to keep 'pipeline_depth' number of requests\nalways queued to the camera. If you want controls for frame #10 to be\nrealized in time you have to queue Request #10 at the time frame #6\n(or #4 depends on the pipeline_depth) is getting exposed.\n\nIf the application doesn't do that, it creates gaps, and while the\nframe sequence number keeps incrementing (as the sensor is producing\nframes) the request sequence number gets only incremented when a new\nrequest is created, creating a mis-alignment between the request\nsequence and the frame number.\n\nThis is where \"Android PFC\" and \"RPI PFC\" differ. RPI wants to allow\ngaps in the Request by jumping ahead and apply the most recent\ncontrols to the most recent frame. The Android model requires\napplications to keep the requests queue filled in and to queue\nrequests at the same pace as the frames gets produced by the sensor.\nif the application 'underrun' the Request queue, PFC simply can't\nhappen.\n\nNow, you might have noticed we time the IPA with the application's\nrequests. It means that we queue a buffer for statistics and run the\nIPA loop only when a Request is queued. To add confusion, some\nplatforms (RkISP1) count requests using the sensor's frame number,\nsome others (IPU3) use the Request sequence number.\n\nWe have been talking recently about making IPAs free running. They\nneed to receive stats and produce paramteres for every frame produced\nby the sensor, not for every Request, otherwise we depend on the\napplication queue requests in time and at a fast enough pace.\n\nThis however doesn't change the fact that to obtain full PFC\nthere need to be one request for each frame, but it's imho the first\nstep to allow us to implement PFC correctly.\n\n> >\n> > > - Controls for a given sequence number can be updated multiple times\n> > > as long as they are not yet sent out\n> > > - If it's too late, controls get delayed but they are not lost\n> >\n> > What do you mean not lost ? Will they be applied anyway ? what about\n> > the controls for the next frames ?\n>\n> If you request a control for frame 8, but the controls for that frame\n> are already sent to the sensor, the control will be applied at the\n> earliest possible frame (e.g. frame 10). If there are already request\n> queued for frame 9 or 10 the request from frame 8 is discarded.\n>\n\nMy gut feeling is that's not what we want, as if an application is\nlate in queueing a Request, it's just late and those controls are\nlost. However, if as in your example, the lost control switch an\nalgorithm from Auto to Manual, ignoring it might not be the best idea\n\n> >\n> > > - Requests attached to frame 0 don't get lost\n> >\n> > Do you mean the very first request ? Do you mean to apply controls\n> > before streaming is started ?\n> >\n>\n> Yes, controls that are set on the very first request. I added a seperate\n> testcase for that in the next version. Implicitly it was tested in\n> singleControlWithDelayStartUp()\n>\n> > >\n> > > Technical notes:\n> > > A sourceSequence_ replaces the updated flag to be able to track from which\n> > > frame the update stems. This is needed for the following use case:\n> > > Assume e.g. On frame 10 an ExposureTime=42 is queued because the user wants\n> > > manual exposure. Now the agc gets the stats for frame 8 (where auto\n> > > regulation is still active) and pushes a new ExposureTime for frame 9.\n> > > Frame 9 was already sent out, so it gets delayed to frame 11 (assuming a\n> > > delay of 2 on ExposureTime). This would revert the request from frame 10.\n> > > Taking the sourceSequence into account, the delayed request from frame\n> > > 9 will be discarded, which is correct.\n> >\n> > Also this is not 100% clear to me. The request for frame 10 that says\n> > \"manual 42\" should be processed by the IPA/pipeline early enough to\n> > make sure the \"ExposureTime=42\" control is emitted early enough for\n> > being applied in time (assuming 2 frames of delay, this mean the\n> > control should be emitted before frame 8 has started exposing).\n> >\n> > If the AGC gets the stats for frame 8 and computes auto-value for\n> > frame 11, it means it is late and the IPA is not processing requests\n> > early enough ? have you seen this happening ?\n>\n> As the ISP is acting in a closed loop, it can only react on stats that\n> were calculated for the last frame that entered the system. So it is by\n> definition always too late. In the example above, the manual 42 can\n> actually be sent out early enough because it doesn't require information\n> from the stats module. This is the reason for patch 11/12 \"rkisp1: Fix\n> per-frame-controls\"\n>\n\nI'll look at that patch before commenting further\n\n> >\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > include/libcamera/internal/delayed_controls.h | 12 +-\n> > > src/libcamera/delayed_controls.cpp | 207 ++++++++++++++----\n> > > 2 files changed, 174 insertions(+), 45 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> > > index 4f8d2424..2738e8bf 100644\n> > > --- a/include/libcamera/internal/delayed_controls.h\n> > > +++ b/include/libcamera/internal/delayed_controls.h\n> > > @@ -30,25 +30,29 @@ public:\n> > > \tvoid reset();\n> > >\n> > > \tbool push(const ControlList &controls);\n> > > +\tbool pushForFrame(uint32_t sequence, const ControlList &controls);\n> > > \tControlList get(uint32_t sequence);\n> > >\n> > > \tvoid applyControls(uint32_t sequence);\n> > >\n> > > private:\n> > > +\tbool controlsAreQueuedForFrame(unsigned int frame, const ControlList &controls);\n> >\n> > Functions should be defined after types. Please move this after the\n> > ControlRingBuffer type definition below. Also, this is a very long name :)\n> >\n> > > +\n> > > \tclass Info : public ControlValue\n> > > \t{\n> > > \tpublic:\n> > > \t\tInfo()\n> > > -\t\t\t: updated(false)\n> > > +\t\t\t: sourceSequence_(0)\n> > > \t\t{\n> > > \t\t}\n> > >\n> > > -\t\tInfo(const ControlValue &v, bool updated_ = true)\n> > > -\t\t\t: ControlValue(v), updated(updated_)\n> > > +\t\tInfo(const ControlValue &v, std::optional sourceSequence = std::nullopt)\n> > > +\t\t\t: ControlValue(v), sourceSequence_(sourceSequence)\n> > > \t\t{\n> > > \t\t}\n> > >\n> > > -\t\tbool updated;\n> > > +\t\t/* The sequence id, this info stems from*/\n> > > +\t\tstd::optional sourceSequence_;\n> > > \t};\n> > >\n> > > \t/* \\todo Make the listSize configurable at instance creation time. */\n> > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > index 86571cd4..59314388 100644\n> > > --- a/src/libcamera/delayed_controls.cpp\n> > > +++ b/src/libcamera/delayed_controls.cpp\n> > > @@ -115,8 +115,9 @@ DelayedControls::DelayedControls(V4L2Device *device,\n> > > */\n> > > void DelayedControls::reset()\n> > > {\n> > > -\tqueueIndex_ = 1;\n> > > -\twriteIndex_ = 0;\n> > > +\tqueueIndex_ = 0;\n> > > +\t/* Frames up to maxDelay_ will be based on sensor init values. */\n> >\n> > Ok, so this is the startup condition!\n>\n> I'm not sure if I got you right here. Would it be clearer to say\n> \"Frames up to maxDelay_ will be based on the values obtained in reset()?\n>\n> >\n> > > +\twriteIndex_ = maxDelay_;\n> > >\n> > > \t/* Retrieve control as reported by the device. */\n> > > \tstd::vector ids;\n> > > @@ -133,26 +134,129 @@ void DelayedControls::reset()\n> > > \t\t * Do not mark this control value as updated, it does not need\n> > > \t\t * to be written to to device on startup.\n> >\n> > does the comment needs updating too ?\n> >\n>\n> hm, I removed it altogether. There is nothing special to be said here.\n>\n> > > \t\t */\n> > > -\t\tvalues_[id][0] = Info(ctrl.second, false);\n> > > +\t\tvalues_[id][0] = Info(ctrl.second, 0);\n> > > \t}\n> > > +\n> > > +\t/* Copy state from previous frames. */\n> >\n> > Are these from the previous frames or simply the initial control values ?\n>\n> Yes, initial ones. I updated the comment and factored the loop out.\n>\n> >\n> > > +\tfor (auto &ctrl : values_) {\n> > > +\t\tfor (auto i = queueIndex_; i < writeIndex_; i++) {\n> >\n> > unsigned int i\n> >\n> > > +\t\t\tInfo &info = ctrl.second[i + 1];\n> > > +\t\t\tinfo = ctrl.second[i];\n> > > +\t\t\tinfo.sourceSequence_.reset();\n> > > +\t\t}\n> > > +\t}\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Helper function to check if controls are queued already\n> > > + * \\param[in] sequence Sequence number to check\n> > > + * \\param[in] controls List of controls to compare against\n> > > + *\n> > > + * This function checks if the controls queued for frame \\a sequence\n> > > + * are equal to \\a controls. This is helpful in cases where a control algorithm\n> > > + * unconditionally queues controls for every frame, but always too late.\n> > > + * In that case this can be used to check if incoming controls are already queued\n> > > + * or need to be queued for a later frame.\n> >\n> > I'm interested to see where and how this is used\n> >\n> > > + *\n> > > + * \\returns true if \\a controls are queued for the given sequence\n> > , false otherwise\n> > > + */\n> > > +bool DelayedControls::controlsAreQueuedForFrame(unsigned int sequence, const ControlList &controls)\n> >\n> > can easily be made < 80 cols.\n> >\n> > > +{\n> > > +\tauto idMap = controls.idMap();\n> > > +\tif (!idMap) {\n> > > +\t\tLOG(DelayedControls, Warning) << \" idmap is null\";\n> > > +\t\treturn false;\n> > > +\t} else {\n> >\n> > No need for an else branch, you have returned already\n> >\n> > > +\t\tfor (const auto &[id, value] : controls) {\n> > > +\t\t\tif (values_[idMap->at(id)][sequence] != value) {\n> > > +\t\t\t\treturn false;\n> > > +\t\t\t}\n> >\n> > No {}\n> >\n> > > +\t\t}\n> > > +\t}\n> >\n> > Empty line\n> >\n> > > +\treturn true;\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> > > + * \\deprecated This function is deprecated in favour of pushForFrame\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> > > + * \\returns See return value of DelayedControls::pushForFrame\n> > > */\n> > > bool DelayedControls::push(const ControlList &controls)\n> > > {\n> > > -\t/* Copy state from previous frame. */\n> > > -\tfor (auto &ctrl : values_) {\n> > > -\t\tInfo &info = ctrl.second[queueIndex_];\n> > > -\t\tinfo = values_[ctrl.first][queueIndex_ - 1];\n> > > -\t\tinfo.updated = false;\n> > > +\tLOG(DelayedControls, Debug) << \"Using deprecated function push(controls): \" << queueIndex_;\n> >\n> > break line\n> >\n> > > +\treturn pushForFrame(queueIndex_, controls);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Push a set of controls for a given frame\n> > > + * \\param[in] sequence Sequence to push the controls for\n> > > + * \\param[in] controls List of controls to add to the device queue\n> > > + *\n> > > + * Pushes the controls given by \\a controls, to be applied at frame \\a sequence.\n> >\n> > Apply \\a controls to frame \\a sequence\n>\n> Isn't that a different meaning? I'm not happy with my sentence either.\n> What about: Store \\a controls to be applied for frame \\a sequence.\n>\n> >\n> > > + *\n> > > + * If there are controls already queued for that frame, these get updated.\n> >\n> > s/these/they\n> >\n> > > + *\n> > > + * If it's too late for frame \\a sequence (controls are already sent to the sensor),\n> > > + * the system checks if the controls that where written out for frame \\a sequence\n> > > + * are the same as the requested ones. In this case, nothing is done.\n> > > + * If they differ, the controls get queued for the earliest frame possible\n> > > + * if no other controls with a higher sequence number are queued for that frame already.\n> > > + *\n> > > + * \\returns true if \\a controls are accepted, or false otherwise\n> > > + */\n> > > +\n> >\n> > Stray empty line\n> >\n> > > +bool DelayedControls::pushForFrame(uint32_t sequence, const ControlList &controls)\n> >\n> > Can't you just overload ::push() ?\n>\n> Sure, I can do that. For my brain it is easier to read\n> d->pushForFrame(x, ctrls)\n> than\n> d->push(x, ctrls)\n> But thats most likely a matter of taste (or I worked too much with\n> Apple's macOS APIs)\n>\n> >\n> > > +{\n> > > +\tif (sequence < queueIndex_) {\n> > > +\t\tLOG(DelayedControls, Debug) << \"Got updated data for frame:\" << sequence;\n> > > +\t}\n> >\n> > No {}\n> >\n> > > +\n> > > +\tif (sequence > queueIndex_) {\n> > > +\t\tLOG(DelayedControls, Warning) << \"Hole in queue sequence. This should not happen. Expected: \"\n> > > +\t\t\t\t\t << queueIndex_ << \" got \" << sequence;\n> > > +\t}\n> >\n> > no {}\n> > very long line\n> >\n> > > +\n> > > +\tuint32_t updateIndex = sequence;\n> > > +\t/* check if its too late for the request */\n> > > +\tif (sequence < writeIndex_) {\n> > > +\t\t/* Check if we can safely ignore the request */\n> > > +\t\tif (controls.empty() || controlsAreQueuedForFrame(sequence, controls)) {\n> >\n> > Are we comparing control lists at every push() (so likely for every\n> > request) ?\n>\n> Only if we are too late. But in a ISP closed loop, that might happen\n> continuously.\n>\n> >\n> >\n> > > +\t\t\tif (sequence >= queueIndex_) {\n> > > +\t\t\t\tqueueIndex_ = sequence + 1;\n> > > +\t\t\t\treturn true;\n> > > +\t\t\t}\n> > > +\t\t} else {\n> > > +\t\t\tLOG(DelayedControls, Debug) << \"Controls for frame \" << sequence\n> > > +\t\t\t\t\t\t << \" are already in flight. Will be queued for frame \" << writeIndex_;\n> > > +\t\t\tupdateIndex = writeIndex_;\n> > > +\t\t}\n> >\n> > veeeery long lines\n> >\n> > > +\t}\n> > > +\n> > > +\tif (static_cast(updateIndex) - static_cast(writeIndex_) >= listSize - static_cast(maxDelay_)) {\n> >\n> > Why signed ?\n>\n> Yep, updateIndex is always >= writeIndex. You are right. I changed\n> listSize, to be unsigned also.\n>\n> > >\n> > > +\t\t/* The system is in an undefined state now. This will heal itself, as soon as all controls where rewritten */\n> > > +\t\tLOG(DelayedControls, Error) << \"Queue length exceeded. The system is out of sync. Index to update:\"\n> > > +\t\t\t\t\t << updateIndex << \" Next Index to apply: \" << writeIndex_;\n> > > +\t}\n> > > +\n> > > +\t/**\n> >\n> > /** is for doxygen\n> >\n> > > +\t * Prepare the ringbuffer entries with previous data.\n> > > +\t * Data up to [writeIndex_] gets prepared in applyControls.\n> > > +\t */\n> > > +\tif (updateIndex > writeIndex_ && updateIndex >= queueIndex_) {\n> > > +\t\tLOG(DelayedControls, Debug) << \"Copy from previous \" << queueIndex_;\n> > > +\t\tfor (auto i = queueIndex_; i < updateIndex; i++) {\n> > > +\t\t\t/* Copy state from previous queue. */\n> > > +\t\t\tfor (auto &ctrl : values_) {\n> > > +\t\t\t\tauto &ringBuffer = ctrl.second;\n> > > +\t\t\t\tringBuffer[i] = ringBuffer[i - 1];\n> > > +\t\t\t\tringBuffer[i].sourceSequence_.reset();\n> > > +\t\t\t}\n> > > +\t\t}\n> > > \t}\n> > >\n> > > \t/* Update with new controls. */\n> > > @@ -167,20 +271,34 @@ bool DelayedControls::push(const ControlList &controls)\n> > >\n> > > \t\tconst ControlId *id = it->second;\n> > >\n> > > -\t\tif (controlParams_.find(id) == controlParams_.end())\n> > > -\t\t\treturn false;\n> > > -\n> > > -\t\tInfo &info = values_[id][queueIndex_];\n> > > +\t\tif (controlParams_.find(id) == controlParams_.end()) {\n> > > +\t\t\tLOG(DelayedControls, Error) << \"Could not find params for control \" << id << \" ignored\";\n> >\n> > long line\n> >\n> > > +\t\t\tcontinue;\n> > > +\t\t}\n> >\n> > Is ignoring the control better than erroring out ? Is this worth a\n> > separate change ?\n> >\n> > >\n> > > -\t\tinfo = Info(control.second);\n> > > +\t\tInfo &info = values_[id][updateIndex];\n> >\n> > Move after the comment block\n> >\n> > > +\t\t/*\n> > > +\t\t * Update the control only, if the already existing value stems from a request\n> >\n> > s/only,/only\n> >\n> > long line\n> >\n> > > +\t\t * with a sequence number smaller or equal to the current one\n> > > +\t\t */\n> > > +\t\tif (info.sourceSequence_.value_or(0) <= sequence) {\n> > > +\t\t\tinfo = Info(control.second, sequence);\n> > >\n> > > -\t\tLOG(DelayedControls, Debug)\n> > > -\t\t\t<< \"Queuing \" << id->name()\n> > > -\t\t\t<< \" to \" << info.toString()\n> > > -\t\t\t<< \" at index \" << queueIndex_;\n> > > +\t\t\tLOG(DelayedControls, Debug)\n> > > +\t\t\t\t<< \"Queuing \" << id->name()\n> > > +\t\t\t\t<< \" to \" << info.toString()\n> > > +\t\t\t\t<< \" at index \" << updateIndex;\n> > > +\t\t} else {\n> > > +\t\t\tLOG(DelayedControls, Warning)\n> > > +\t\t\t\t<< \"Skipped update \" << id->name()\n> > > +\t\t\t\t<< \" at index \" << updateIndex;\n> > > +\t\t}\n> > > \t}\n> > >\n> > > -\tqueueIndex_++;\n> > > +\tLOG(DelayedControls, Debug) << \"Queued frame: \" << queueIndex_ << \" it will be active in \" << updateIndex;\n> >\n> > very long line\n> >\n> > > +\n> > > +\tif (sequence >= queueIndex_)\n> > > +\t\tqueueIndex_ = sequence + 1;\n> > >\n> > > \treturn true;\n> > > }\n> > > @@ -202,19 +320,17 @@ bool DelayedControls::push(const ControlList &controls)\n> > > */\n> > > ControlList DelayedControls::get(uint32_t sequence)\n> > > {\n> > > -\tunsigned int index = std::max(0, sequence - maxDelay_);\n> > > -\n> >\n> > This can now go because the initial frames are now populated, right ?\n> >\n> > > \tControlList out(device_->controls());\n> > > \tfor (const auto &ctrl : values_) {\n> > > \t\tconst ControlId *id = ctrl.first;\n> > > -\t\tconst Info &info = ctrl.second[index];\n> > > +\t\tconst Info &info = ctrl.second[sequence];\n> > >\n> > > \t\tout.set(id->id(), info);\n> > >\n> > > \t\tLOG(DelayedControls, Debug)\n> > > \t\t\t<< \"Reading \" << id->name()\n> > > \t\t\t<< \" to \" << info.toString()\n> > > -\t\t\t<< \" at index \" << index;\n> > > +\t\t\t<< \" at index \" << sequence;\n> > > \t}\n> > >\n> > > \treturn out;\n> > > @@ -222,16 +338,21 @@ ControlList DelayedControls::get(uint32_t sequence)\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> > > + * \\param[in] sequence Sequence number of the frame that started (0-based)\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> > > + * Inform the state machine that a new frame has started to arrive at the receiver\n> > > + * (e.g. the sensor started to clock out pixels) and of its sequence\n> > > + * number. This is usually the earliest point in time to update registers in the\n> >\n> > Not sure this comment change is necessary\n>\n> For me it was unclear if \"a new frame has started\" meant:\n>\n> a) That we are now setting controls to start the exposure of that new frame\n> b) That the sensor started exposing that frame\n> c) That a new frame started to arrive on the host\n>\n> Native speakers? Any better ideas?\n>\n> >\n> > > + * sensor for upcoming frames. Any user of these helpers is responsible to inform\n> > > + * the helper about the start of any frame. This can be connected with ease to\n> > > + * the start of a exposure (SOE) V4L2 event.\n> >\n> > stay in 80-col, please\n> >\n> > > */\n> > > void DelayedControls::applyControls(uint32_t sequence)\n> > > {\n> > > -\tLOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> > > +\tLOG(DelayedControls, Debug) << \"Apply controls \" << sequence;\n> > > +\tif (sequence != writeIndex_ - maxDelay_) {\n> > > +\t\tLOG(DelayedControls, Warning) << \"Sequence and writeIndex are out of sync. Expected seq: \" << writeIndex_ - maxDelay_ << \" got \" << sequence;\n> >\n> > Not sure I get why this is an error. Is this to guarantee the IPA\n> > always stays exactly maxDelay_ frames in advance ? can anything like\n> > this be guaranteed ?\n> >\n>\n> In general I would say so. applyControls is normally called as\n> applyControls(0)\n> applyControls(1)\n> applyControls(2)\n>\n> This warning is there to inform about missed applyControls. E.g.\n> applyControls(1) <-- first warning. Index 0 missed\n> applyControls(3) <-- secod warning. Index 2 missed\n>\n> If we want that to be acceptable, we would need to accumulate all control\n> changes from the missed frames...\n>\n> > > +\t}\n> >\n> > no {}\n> > very long line\n> >\n> > >\n> > > \t/*\n> > > \t * Create control list peeking ahead in the value queue to ensure\n> > > @@ -241,10 +362,10 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > > \tfor (auto &ctrl : values_) {\n> > > \t\tconst ControlId *id = ctrl.first;\n> > > \t\tunsigned int delayDiff = maxDelay_ - controlParams_[id].delay;\n> > > -\t\tunsigned int index = std::max(0, writeIndex_ - delayDiff);\n> > > +\t\tunsigned int index = writeIndex_ - delayDiff;\n> > > \t\tInfo &info = ctrl.second[index];\n> > >\n> > > -\t\tif (info.updated) {\n> > > +\t\tif (info.sourceSequence_.has_value()) {\n> > > \t\t\tif (controlParams_[id].priorityWrite) {\n> > > \t\t\t\t/*\n> > > \t\t\t\t * This control must be written now, it could\n> > > @@ -262,21 +383,25 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > > \t\t\t}\n> > >\n> > > \t\t\tLOG(DelayedControls, Debug)\n> > > -\t\t\t\t<< \"Setting \" << id->name()\n> > > -\t\t\t\t<< \" to \" << info.toString()\n> > > -\t\t\t\t<< \" at index \" << index;\n> > > -\n> > > -\t\t\t/* Done with this update, so mark as completed. */\n> > > -\t\t\tinfo.updated = false;\n> > > +\t\t\t\t<< \"Writing \" << id->name()\n> > > +\t\t\t\t<< \" (\" << info.toString() << \") \"\n> > > +\t\t\t\t<< \" for frame \" << index;\n> > > \t\t}\n> > > \t}\n> > >\n> > > -\twriteIndex_ = sequence + 1;\n> > > +\tauto oldWriteIndex = writeIndex_;\n> > > +\twriteIndex_ = sequence + maxDelay_ + 1;\n> > >\n> > > -\twhile (writeIndex_ > queueIndex_) {\n> > > +\tif (writeIndex_ >= queueIndex_ && writeIndex_ > oldWriteIndex) {\n> > > \t\tLOG(DelayedControls, Debug)\n> > > -\t\t\t<< \"Queue is empty, auto queue no-op.\";\n> > > -\t\tpush({});\n> > > +\t\t\t<< \"Index \" << writeIndex_ << \" is not yet queued. Prepare with old state\";\n> > > +\t\t/* Copy state from previous frames without resetting the sourceSequence */\n> > > +\t\tfor (auto &ctrl : values_) {\n> > > +\t\t\tfor (auto i = oldWriteIndex; i < writeIndex_; i++) {\n> > > +\t\t\t\tInfo &info = ctrl.second[i + 1];\n> > > +\t\t\t\tinfo = values_[ctrl.first][i];\n> > > +\t\t\t}\n> > > +\t\t}\n> >\n> > My main worries is now that, if I understand it right, the system\n> > always tries to keep a maxDelay number of frames between the last\n> > controls written to the sensor and the head of the queue, and to do so\n> > it duplicate controls at every push() and apply(). This happens for\n> > -every- frame and -every- request if I got it right ? Am I too\n> > over-concerned this is expensive ?\n>\n> To my understanding that is no change to the way it was before. Before,\n> that copying happed due to push({}), thereby messing up the\n> synchronicity of push() and apply().\n>\n> >\n> > > \t}\n> > >\n> > > \tdevice_->setControls(&out);\n> > > --\n> > > 2.40.1\n> > >\n>\n> Thank you very much for all the feedback. It's time for a v3. And next\n> time without the style issues :-)\n>\n> Cheers,\n> Stefan","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 82E94BD160\n\tfor ;\n\tThu, 21 Mar 2024 09:20:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5206E63055;\n\tThu, 21 Mar 2024 10:20:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1081C61C59\n\tfor ;\n\tThu, 21 Mar 2024 10:20:39 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B39BB7E9;\n\tThu, 21 Mar 2024 10:20:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XBrF5A7V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711012810;\n\tbh=EKIap1KRPUbBtafkMiTzVkHVzw7mMqvl72SEMqvdv7w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XBrF5A7V81h0J3ES5FBHo00iug0WZSq//YJbn3D909qgvgWKHtz7VJGvMpxHhD3k8\n\topNi9P3RH0bXRBHsGwxFjSk5vN4yFqUJS7BWttIs97+6nUoV/wltqpWhIQpRIV9L3J\n\tKLeM2xMHuJEOD+gaojfWIrTCTE4WUJCcyzKv+OZo=","Date":"Thu, 21 Mar 2024 10:20:34 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","Message-ID":"<2bcf6izyvs5u27xfvkirit3ua5mdp65gjhyujdidnt4qiihccm@jfbsz4lc5wos>","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-7-stefan.klug@ideasonboard.com>\n\t\n\t<20240318132236.fptxax3pwsiagmiq@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240318132236.fptxax3pwsiagmiq@jasper>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29024,"web_url":"https://patchwork.libcamera.org/comment/29024/","msgid":"<171101834073.3694868.11000332276393792018@ping.linuxembedded.co.uk>","date":"2024-03-21T10:52:20","subject":"Re: [PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2024-03-21 09:20:34)\n> Hi Stefan\n> \n> On Mon, Mar 18, 2024 at 02:22:36PM +0100, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > thank you for the review. Sorry for all the style issues. I'll fix them\n> > asap.\n> >\n> > On Fri, Mar 15, 2024 at 05:15:58PM +0100, Jacopo Mondi wrote:\n> > > Hi Stefan\n> > >\n> > > Not going to review the logic implementation in detail, it will take\n> > > me some more time as delayed controls are kind of a complex beast, so\n> > > this will be mostly a stylistic review with some stupid questions here\n> > > and there\n> > >\n> > > On Wed, Mar 13, 2024 at 01:12:17PM +0100, Stefan Klug wrote:\n> > > > Functional changes:\n> > > > - Requests need to be queued for every frame\n> > >\n> > > Do you mean it is a requirement for the application to queue Request\n> > > for every frame ?\n> >\n> > Yes, that's actually no change. The reason why I wrote that, is that in\n> > the old delayed controls tests, there was always a call to\n> > applyControls() before the first call to push().\n> >\n> \n> Just to clarify, is this a requirement for PFC to happen, or in\n> general ? (see below)\n> \n> > >\n> > > > - The startup phase is no longer treated as special case\n> > > > - Requests carry a sequence number, so that we can detect when the system\n> > > > gets out of sync\n> > >\n> > > Are you referring to the request sequence number or the frame number ?\n> >\n> > Are there actually two different groups of numbers? If yes, I miss something\n> > conceptually here. The seuence number is meant to be as in \"This is the\n> > request with seq x, which needs to be applied for frame x\"\n> >\n> \n> I guess this is one of the thing we should clarify first before going\n> further in the design.\n> \n> Both Request and frames have a sequence_number.\n> \n> Requests are queued to the camera by the application. Their sequence\n> number increments for each Request created by the Camera. There are no\n> requirments on the timing the application queues requests to the\n> camera, one example is RPi Zero users that due to limited memory only\n> seldom queue a Request\n> \n> Frames are produced by the sensor at a fixed time interval, and their\n> sequence number increments at every frame produced by the sensor (or better,\n> received by the CSI-2 rx, as sometimes frame gets dropped and gaps are\n> created, anyway..)\n> \n> If an application wants full per-frame control, it needs to queue\n> enough requests to compensate for the pipeline depth which counts for:\n> \n> - a full frame time as sensor's settings apply to the 'next' frame, hence\n> the one that is getting exposed right now is 'gone'\n> - on some ISP some frames of delay to generate statistics (iirc IPU3\n> has 1 frame delay between the frame that gets processed and the one\n> for which statistics are generated)\n> - the sensor's maximum control delay\n> \n> A reasonable number (to be used as an example here) for the pipeline\n> depth is something between 4 and 6 frames.\n> \n> Now, if an application wants full per frame control it has to queue\n> requests early enough for the desired controls associated with a\n> request to be realized, so it has to keep 'pipeline_depth' number of requests\n> always queued to the camera. If you want controls for frame #10 to be\n> realized in time you have to queue Request #10 at the time frame #6\n> (or #4 depends on the pipeline_depth) is getting exposed.\n> \n> If the application doesn't do that, it creates gaps, and while the\n> frame sequence number keeps incrementing (as the sensor is producing\n> frames) the request sequence number gets only incremented when a new\n> request is created, creating a mis-alignment between the request\n\nI agree with most of the above, but I want to clarify that request\nsequences are generated/incremented only when a request is *queued* to\nthe camera, not when the request is created.\n\n> sequence and the frame number.\n> \n> This is where \"Android PFC\" and \"RPI PFC\" differ. RPI wants to allow\n> gaps in the Request by jumping ahead and apply the most recent\n> controls to the most recent frame. The Android model requires\n> applications to keep the requests queue filled in and to queue\n> requests at the same pace as the frames gets produced by the sensor.\n> if the application 'underrun' the Request queue, PFC simply can't\n> happen.\n> \n> Now, you might have noticed we time the IPA with the application's\n> requests. It means that we queue a buffer for statistics and run the\n> IPA loop only when a Request is queued. To add confusion, some\n> platforms (RkISP1) count requests using the sensor's frame number,\n> some others (IPU3) use the Request sequence number.\n\nThis is definitely an area I hope we can work to clean up soon. And\nindeed we've been talking about it already, but we need to figure this\nout cleanly and apply the same decision to any of the libipa IPAs.\n\n> We have been talking recently about making IPAs free running. They\n> need to receive stats and produce paramteres for every frame produced\n> by the sensor, not for every Request, otherwise we depend on the\n> application queue requests in time and at a fast enough pace.\n\nAnd of course this. Though free-running IPA is essentially a symptom of\nhow we will handle *not* achieving PFC?\n\n> This however doesn't change the fact that to obtain full PFC\n> there need to be one request for each frame, but it's imho the first\n> step to allow us to implement PFC correctly.\n> \n> > > > - Controls for a given sequence number can be updated multiple times\n> > > > as long as they are not yet sent out\n> > > > - If it's too late, controls get delayed but they are not lost\n> > >\n> > > What do you mean not lost ? Will they be applied anyway ? what about\n> > > the controls for the next frames ?\n> >\n> > If you request a control for frame 8, but the controls for that frame\n> > are already sent to the sensor, the control will be applied at the\n> > earliest possible frame (e.g. frame 10). If there are already request\n> > queued for frame 9 or 10 the request from frame 8 is discarded.\n> >\n> \n> My gut feeling is that's not what we want, as if an application is\n> late in queueing a Request, it's just late and those controls are\n> lost. However, if as in your example, the lost control switch an\n> algorithm from Auto to Manual, ignoring it might not be the best idea\n\nI disagree here. (But I think it's due to terminology used rather than\nwhat we are all actually expecting here).\n\nIf a 'per frame control' is missed/late, indeed it is not possible to\napply to the correct frame. But I believe the state should be updated\nand tracked.\n\nLets imagine an application starts streaming with a manual exposure of\n10ms, and after 5 frames wants that to become 30ms exposure, and assume\nno other request changes the manual exposure value.\n\nIf for a non-determined reason request 5 was late, I wouldn't expect to\ncontinue streaming indefinitely with 10ms, but that the streaming should\n(at the earliest opportunity after frame 5) be configured to a 30ms\nexposure.\n\n\nIn fact, re-reading the above, both my text and Jacopo's - what we\nreally need to do here is make the distinction between 'when' the\ncontrol is lost. And I would not call the controls 'lost' but\noverwritten / superceeded by new incoming controls when applicable. And\nif there is no superceeding control/data update - then the control is\nnot lost, just late. But if there is a new update which is on time -\nthat would be able to superceed and apply correctly. At that point - it\ncould be considered that the older control is 'lost' but I would just\nsay that it has been updated rather than lost.\n\nIn 'android model' We should not 'lose' the data. But we can 'update'\n(replace) the data.\n\nI think the RPi model is different here in that instead of replace, it\nwould only push back controls so replacing would not be allowed. But in\nthe same vein - the global context/state would always be updated.\n\n\n> > > > - Requests attached to frame 0 don't get lost\n> > >\n> > > Do you mean the very first request ? Do you mean to apply controls\n> > > before streaming is started ?\n> > >\n> >\n> > Yes, controls that are set on the very first request. I added a seperate\n> > testcase for that in the next version. Implicitly it was tested in\n> > singleControlWithDelayStartUp()\n\nI think we discussed in the past that to establish a configuration at\nframe 0, the controls have to be applied during camera->start(controls).\nAnd that for $MaxDelayedControlDelay at the beginning of a stream\notherwise, we simply can not guarantee PFC.\n\nWe can not apply manual exposure/gains that have a delay of 2 until the\nSoF for the first frame - so while we can set frame 0 - we can *never*\nset frame 1 correctly (in theory if it were supposed to be different).\n\nPlease correct me if I'm wrong above!\n\n> > > > Technical notes:\n> > > > A sourceSequence_ replaces the updated flag to be able to track from which\n> > > > frame the update stems. This is needed for the following use case:\n> > > > Assume e.g. On frame 10 an ExposureTime=42 is queued because the user wants\n> > > > manual exposure. Now the agc gets the stats for frame 8 (where auto\n> > > > regulation is still active) and pushes a new ExposureTime for frame 9.\n> > > > Frame 9 was already sent out, so it gets delayed to frame 11 (assuming a\n> > > > delay of 2 on ExposureTime). This would revert the request from frame 10.\n> > > > Taking the sourceSequence into account, the delayed request from frame\n> > > > 9 will be discarded, which is correct.\n> > >\n> > > Also this is not 100% clear to me. The request for frame 10 that says\n> > > \"manual 42\" should be processed by the IPA/pipeline early enough to\n> > > make sure the \"ExposureTime=42\" control is emitted early enough for\n> > > being applied in time (assuming 2 frames of delay, this mean the\n> > > control should be emitted before frame 8 has started exposing).\n> > >\n> > > If the AGC gets the stats for frame 8 and computes auto-value for\n> > > frame 11, it means it is late and the IPA is not processing requests\n> > > early enough ? have you seen this happening ?\n> >\n> > As the ISP is acting in a closed loop, it can only react on stats that\n> > were calculated for the last frame that entered the system. So it is by\n> > definition always too late. In the example above, the manual 42 can\n> > actually be sent out early enough because it doesn't require information\n> > from the stats module. This is the reason for patch 11/12 \"rkisp1: Fix\n> > per-frame-controls\"\n> >\n> \n> I'll look at that patch before commenting further\n\nThe one thing I remember from when we looked at this last time is that\nwe need a better way to convey all the timing in our text. Which means\nwe need diagrams and pictures IMO.\n\nI really hope we can work on our tracing tools to help us generate such\ntiming diagram/images in some form beacuse I think we need the same\ndiagrams to report what /happens/ too! I have ideas ... but lacking in\ntime currently!\n\n--\nKieran\n\n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > include/libcamera/internal/delayed_controls.h | 12 +-\n> > > > src/libcamera/delayed_controls.cpp | 207 ++++++++++++++----\n> > > > 2 files changed, 174 insertions(+), 45 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> > > > index 4f8d2424..2738e8bf 100644\n> > > > --- a/include/libcamera/internal/delayed_controls.h\n> > > > +++ b/include/libcamera/internal/delayed_controls.h\n> > > > @@ -30,25 +30,29 @@ public:\n> > > > void reset();\n> > > >\n> > > > bool push(const ControlList &controls);\n> > > > + bool pushForFrame(uint32_t sequence, const ControlList &controls);\n> > > > ControlList get(uint32_t sequence);\n> > > >\n> > > > void applyControls(uint32_t sequence);\n> > > >\n> > > > private:\n> > > > + bool controlsAreQueuedForFrame(unsigned int frame, const ControlList &controls);\n> > >\n> > > Functions should be defined after types. Please move this after the\n> > > ControlRingBuffer type definition below. Also, this is a very long name :)\n> > >\n> > > > +\n> > > > class Info : public ControlValue\n> > > > {\n> > > > public:\n> > > > Info()\n> > > > - : updated(false)\n> > > > + : sourceSequence_(0)\n> > > > {\n> > > > }\n> > > >\n> > > > - Info(const ControlValue &v, bool updated_ = true)\n> > > > - : ControlValue(v), updated(updated_)\n> > > > + Info(const ControlValue &v, std::optional sourceSequence = std::nullopt)\n> > > > + : ControlValue(v), sourceSequence_(sourceSequence)\n> > > > {\n> > > > }\n> > > >\n> > > > - bool updated;\n> > > > + /* The sequence id, this info stems from*/\n> > > > + std::optional sourceSequence_;\n> > > > };\n> > > >\n> > > > /* \\todo Make the listSize configurable at instance creation time. */\n> > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > > index 86571cd4..59314388 100644\n> > > > --- a/src/libcamera/delayed_controls.cpp\n> > > > +++ b/src/libcamera/delayed_controls.cpp\n> > > > @@ -115,8 +115,9 @@ DelayedControls::DelayedControls(V4L2Device *device,\n> > > > */\n> > > > void DelayedControls::reset()\n> > > > {\n> > > > - queueIndex_ = 1;\n> > > > - writeIndex_ = 0;\n> > > > + queueIndex_ = 0;\n> > > > + /* Frames up to maxDelay_ will be based on sensor init values. */\n> > >\n> > > Ok, so this is the startup condition!\n> >\n> > I'm not sure if I got you right here. Would it be clearer to say\n> > \"Frames up to maxDelay_ will be based on the values obtained in reset()?\n> >\n> > >\n> > > > + writeIndex_ = maxDelay_;\n> > > >\n> > > > /* Retrieve control as reported by the device. */\n> > > > std::vector ids;\n> > > > @@ -133,26 +134,129 @@ void DelayedControls::reset()\n> > > > * Do not mark this control value as updated, it does not need\n> > > > * to be written to to device on startup.\n> > >\n> > > does the comment needs updating too ?\n> > >\n> >\n> > hm, I removed it altogether. There is nothing special to be said here.\n> >\n> > > > */\n> > > > - values_[id][0] = Info(ctrl.second, false);\n> > > > + values_[id][0] = Info(ctrl.second, 0);\n> > > > }\n> > > > +\n> > > > + /* Copy state from previous frames. */\n> > >\n> > > Are these from the previous frames or simply the initial control values ?\n> >\n> > Yes, initial ones. I updated the comment and factored the loop out.\n> >\n> > >\n> > > > + for (auto &ctrl : values_) {\n> > > > + for (auto i = queueIndex_; i < writeIndex_; i++) {\n> > >\n> > > unsigned int i\n> > >\n> > > > + Info &info = ctrl.second[i + 1];\n> > > > + info = ctrl.second[i];\n> > > > + info.sourceSequence_.reset();\n> > > > + }\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Helper function to check if controls are queued already\n> > > > + * \\param[in] sequence Sequence number to check\n> > > > + * \\param[in] controls List of controls to compare against\n> > > > + *\n> > > > + * This function checks if the controls queued for frame \\a sequence\n> > > > + * are equal to \\a controls. This is helpful in cases where a control algorithm\n> > > > + * unconditionally queues controls for every frame, but always too late.\n> > > > + * In that case this can be used to check if incoming controls are already queued\n> > > > + * or need to be queued for a later frame.\n> > >\n> > > I'm interested to see where and how this is used\n> > >\n> > > > + *\n> > > > + * \\returns true if \\a controls are queued for the given sequence\n> > > , false otherwise\n> > > > + */\n> > > > +bool DelayedControls::controlsAreQueuedForFrame(unsigned int sequence, const ControlList &controls)\n> > >\n> > > can easily be made < 80 cols.\n> > >\n> > > > +{\n> > > > + auto idMap = controls.idMap();\n> > > > + if (!idMap) {\n> > > > + LOG(DelayedControls, Warning) << \" idmap is null\";\n> > > > + return false;\n> > > > + } else {\n> > >\n> > > No need for an else branch, you have returned already\n> > >\n> > > > + for (const auto &[id, value] : controls) {\n> > > > + if (values_[idMap->at(id)][sequence] != value) {\n> > > > + return false;\n> > > > + }\n> > >\n> > > No {}\n> > >\n> > > > + }\n> > > > + }\n> > >\n> > > Empty line\n> > >\n> > > > + return true;\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> > > > + * \\deprecated This function is deprecated in favour of pushForFrame\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> > > > + * \\returns See return value of DelayedControls::pushForFrame\n> > > > */\n> > > > bool DelayedControls::push(const ControlList &controls)\n> > > > {\n> > > > - /* Copy state from previous frame. */\n> > > > - for (auto &ctrl : values_) {\n> > > > - Info &info = ctrl.second[queueIndex_];\n> > > > - info = values_[ctrl.first][queueIndex_ - 1];\n> > > > - info.updated = false;\n> > > > + LOG(DelayedControls, Debug) << \"Using deprecated function push(controls): \" << queueIndex_;\n> > >\n> > > break line\n> > >\n> > > > + return pushForFrame(queueIndex_, controls);\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Push a set of controls for a given frame\n> > > > + * \\param[in] sequence Sequence to push the controls for\n> > > > + * \\param[in] controls List of controls to add to the device queue\n> > > > + *\n> > > > + * Pushes the controls given by \\a controls, to be applied at frame \\a sequence.\n> > >\n> > > Apply \\a controls to frame \\a sequence\n> >\n> > Isn't that a different meaning? I'm not happy with my sentence either.\n> > What about: Store \\a controls to be applied for frame \\a sequence.\n> >\n> > >\n> > > > + *\n> > > > + * If there are controls already queued for that frame, these get updated.\n> > >\n> > > s/these/they\n> > >\n> > > > + *\n> > > > + * If it's too late for frame \\a sequence (controls are already sent to the sensor),\n> > > > + * the system checks if the controls that where written out for frame \\a sequence\n> > > > + * are the same as the requested ones. In this case, nothing is done.\n> > > > + * If they differ, the controls get queued for the earliest frame possible\n> > > > + * if no other controls with a higher sequence number are queued for that frame already.\n> > > > + *\n> > > > + * \\returns true if \\a controls are accepted, or false otherwise\n> > > > + */\n> > > > +\n> > >\n> > > Stray empty line\n> > >\n> > > > +bool DelayedControls::pushForFrame(uint32_t sequence, const ControlList &controls)\n> > >\n> > > Can't you just overload ::push() ?\n> >\n> > Sure, I can do that. For my brain it is easier to read\n> > d->pushForFrame(x, ctrls)\n> > than\n> > d->push(x, ctrls)\n> > But thats most likely a matter of taste (or I worked too much with\n> > Apple's macOS APIs)\n> >\n> > >\n> > > > +{\n> > > > + if (sequence < queueIndex_) {\n> > > > + LOG(DelayedControls, Debug) << \"Got updated data for frame:\" << sequence;\n> > > > + }\n> > >\n> > > No {}\n> > >\n> > > > +\n> > > > + if (sequence > queueIndex_) {\n> > > > + LOG(DelayedControls, Warning) << \"Hole in queue sequence. This should not happen. Expected: \"\n> > > > + << queueIndex_ << \" got \" << sequence;\n> > > > + }\n> > >\n> > > no {}\n> > > very long line\n> > >\n> > > > +\n> > > > + uint32_t updateIndex = sequence;\n> > > > + /* check if its too late for the request */\n> > > > + if (sequence < writeIndex_) {\n> > > > + /* Check if we can safely ignore the request */\n> > > > + if (controls.empty() || controlsAreQueuedForFrame(sequence, controls)) {\n> > >\n> > > Are we comparing control lists at every push() (so likely for every\n> > > request) ?\n> >\n> > Only if we are too late. But in a ISP closed loop, that might happen\n> > continuously.\n> >\n> > >\n> > >\n> > > > + if (sequence >= queueIndex_) {\n> > > > + queueIndex_ = sequence + 1;\n> > > > + return true;\n> > > > + }\n> > > > + } else {\n> > > > + LOG(DelayedControls, Debug) << \"Controls for frame \" << sequence\n> > > > + << \" are already in flight. Will be queued for frame \" << writeIndex_;\n> > > > + updateIndex = writeIndex_;\n> > > > + }\n> > >\n> > > veeeery long lines\n> > >\n> > > > + }\n> > > > +\n> > > > + if (static_cast(updateIndex) - static_cast(writeIndex_) >= listSize - static_cast(maxDelay_)) {\n> > >\n> > > Why signed ?\n> >\n> > Yep, updateIndex is always >= writeIndex. You are right. I changed\n> > listSize, to be unsigned also.\n> >\n> > > >\n> > > > + /* The system is in an undefined state now. This will heal itself, as soon as all controls where rewritten */\n> > > > + LOG(DelayedControls, Error) << \"Queue length exceeded. The system is out of sync. Index to update:\"\n> > > > + << updateIndex << \" Next Index to apply: \" << writeIndex_;\n> > > > + }\n> > > > +\n> > > > + /**\n> > >\n> > > /** is for doxygen\n> > >\n> > > > + * Prepare the ringbuffer entries with previous data.\n> > > > + * Data up to [writeIndex_] gets prepared in applyControls.\n> > > > + */\n> > > > + if (updateIndex > writeIndex_ && updateIndex >= queueIndex_) {\n> > > > + LOG(DelayedControls, Debug) << \"Copy from previous \" << queueIndex_;\n> > > > + for (auto i = queueIndex_; i < updateIndex; i++) {\n> > > > + /* Copy state from previous queue. */\n> > > > + for (auto &ctrl : values_) {\n> > > > + auto &ringBuffer = ctrl.second;\n> > > > + ringBuffer[i] = ringBuffer[i - 1];\n> > > > + ringBuffer[i].sourceSequence_.reset();\n> > > > + }\n> > > > + }\n> > > > }\n> > > >\n> > > > /* Update with new controls. */\n> > > > @@ -167,20 +271,34 @@ bool DelayedControls::push(const ControlList &controls)\n> > > >\n> > > > const ControlId *id = it->second;\n> > > >\n> > > > - if (controlParams_.find(id) == controlParams_.end())\n> > > > - return false;\n> > > > -\n> > > > - Info &info = values_[id][queueIndex_];\n> > > > + if (controlParams_.find(id) == controlParams_.end()) {\n> > > > + LOG(DelayedControls, Error) << \"Could not find params for control \" << id << \" ignored\";\n> > >\n> > > long line\n> > >\n> > > > + continue;\n> > > > + }\n> > >\n> > > Is ignoring the control better than erroring out ? Is this worth a\n> > > separate change ?\n> > >\n> > > >\n> > > > - info = Info(control.second);\n> > > > + Info &info = values_[id][updateIndex];\n> > >\n> > > Move after the comment block\n> > >\n> > > > + /*\n> > > > + * Update the control only, if the already existing value stems from a request\n> > >\n> > > s/only,/only\n> > >\n> > > long line\n> > >\n> > > > + * with a sequence number smaller or equal to the current one\n> > > > + */\n> > > > + if (info.sourceSequence_.value_or(0) <= sequence) {\n> > > > + info = Info(control.second, sequence);\n> > > >\n> > > > - LOG(DelayedControls, Debug)\n> > > > - << \"Queuing \" << id->name()\n> > > > - << \" to \" << info.toString()\n> > > > - << \" at index \" << queueIndex_;\n> > > > + LOG(DelayedControls, Debug)\n> > > > + << \"Queuing \" << id->name()\n> > > > + << \" to \" << info.toString()\n> > > > + << \" at index \" << updateIndex;\n> > > > + } else {\n> > > > + LOG(DelayedControls, Warning)\n> > > > + << \"Skipped update \" << id->name()\n> > > > + << \" at index \" << updateIndex;\n> > > > + }\n> > > > }\n> > > >\n> > > > - queueIndex_++;\n> > > > + LOG(DelayedControls, Debug) << \"Queued frame: \" << queueIndex_ << \" it will be active in \" << updateIndex;\n> > >\n> > > very long line\n> > >\n> > > > +\n> > > > + if (sequence >= queueIndex_)\n> > > > + queueIndex_ = sequence + 1;\n> > > >\n> > > > return true;\n> > > > }\n> > > > @@ -202,19 +320,17 @@ bool DelayedControls::push(const ControlList &controls)\n> > > > */\n> > > > ControlList DelayedControls::get(uint32_t sequence)\n> > > > {\n> > > > - unsigned int index = std::max(0, sequence - maxDelay_);\n> > > > -\n> > >\n> > > This can now go because the initial frames are now populated, right ?\n> > >\n> > > > ControlList out(device_->controls());\n> > > > for (const auto &ctrl : values_) {\n> > > > const ControlId *id = ctrl.first;\n> > > > - const Info &info = ctrl.second[index];\n> > > > + const Info &info = ctrl.second[sequence];\n> > > >\n> > > > out.set(id->id(), info);\n> > > >\n> > > > LOG(DelayedControls, Debug)\n> > > > << \"Reading \" << id->name()\n> > > > << \" to \" << info.toString()\n> > > > - << \" at index \" << index;\n> > > > + << \" at index \" << sequence;\n> > > > }\n> > > >\n> > > > return out;\n> > > > @@ -222,16 +338,21 @@ ControlList DelayedControls::get(uint32_t sequence)\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> > > > + * \\param[in] sequence Sequence number of the frame that started (0-based)\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> > > > + * Inform the state machine that a new frame has started to arrive at the receiver\n> > > > + * (e.g. the sensor started to clock out pixels) and of its sequence\n> > > > + * number. This is usually the earliest point in time to update registers in the\n> > >\n> > > Not sure this comment change is necessary\n> >\n> > For me it was unclear if \"a new frame has started\" meant:\n> >\n> > a) That we are now setting controls to start the exposure of that new frame\n> > b) That the sensor started exposing that frame\n> > c) That a new frame started to arrive on the host\n> >\n> > Native speakers? Any better ideas?\n> >\n> > >\n> > > > + * sensor for upcoming frames. Any user of these helpers is responsible to inform\n> > > > + * the helper about the start of any frame. This can be connected with ease to\n> > > > + * the start of a exposure (SOE) V4L2 event.\n> > >\n> > > stay in 80-col, please\n> > >\n> > > > */\n> > > > void DelayedControls::applyControls(uint32_t sequence)\n> > > > {\n> > > > - LOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> > > > + LOG(DelayedControls, Debug) << \"Apply controls \" << sequence;\n> > > > + if (sequence != writeIndex_ - maxDelay_) {\n> > > > + LOG(DelayedControls, Warning) << \"Sequence and writeIndex are out of sync. Expected seq: \" << writeIndex_ - maxDelay_ << \" got \" << sequence;\n> > >\n> > > Not sure I get why this is an error. Is this to guarantee the IPA\n> > > always stays exactly maxDelay_ frames in advance ? can anything like\n> > > this be guaranteed ?\n> > >\n> >\n> > In general I would say so. applyControls is normally called as\n> > applyControls(0)\n> > applyControls(1)\n> > applyControls(2)\n> >\n> > This warning is there to inform about missed applyControls. E.g.\n> > applyControls(1) <-- first warning. Index 0 missed\n> > applyControls(3) <-- secod warning. Index 2 missed\n> >\n> > If we want that to be acceptable, we would need to accumulate all control\n> > changes from the missed frames...\n> >\n> > > > + }\n> > >\n> > > no {}\n> > > very long line\n> > >\n> > > >\n> > > > /*\n> > > > * Create control list peeking ahead in the value queue to ensure\n> > > > @@ -241,10 +362,10 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > > > for (auto &ctrl : values_) {\n> > > > const ControlId *id = ctrl.first;\n> > > > unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;\n> > > > - unsigned int index = std::max(0, writeIndex_ - delayDiff);\n> > > > + unsigned int index = writeIndex_ - delayDiff;\n> > > > Info &info = ctrl.second[index];\n> > > >\n> > > > - if (info.updated) {\n> > > > + if (info.sourceSequence_.has_value()) {\n> > > > if (controlParams_[id].priorityWrite) {\n> > > > /*\n> > > > * This control must be written now, it could\n> > > > @@ -262,21 +383,25 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > > > }\n> > > >\n> > > > LOG(DelayedControls, Debug)\n> > > > - << \"Setting \" << id->name()\n> > > > - << \" to \" << info.toString()\n> > > > - << \" at index \" << index;\n> > > > -\n> > > > - /* Done with this update, so mark as completed. */\n> > > > - info.updated = false;\n> > > > + << \"Writing \" << id->name()\n> > > > + << \" (\" << info.toString() << \") \"\n> > > > + << \" for frame \" << index;\n> > > > }\n> > > > }\n> > > >\n> > > > - writeIndex_ = sequence + 1;\n> > > > + auto oldWriteIndex = writeIndex_;\n> > > > + writeIndex_ = sequence + maxDelay_ + 1;\n> > > >\n> > > > - while (writeIndex_ > queueIndex_) {\n> > > > + if (writeIndex_ >= queueIndex_ && writeIndex_ > oldWriteIndex) {\n> > > > LOG(DelayedControls, Debug)\n> > > > - << \"Queue is empty, auto queue no-op.\";\n> > > > - push({});\n> > > > + << \"Index \" << writeIndex_ << \" is not yet queued. Prepare with old state\";\n> > > > + /* Copy state from previous frames without resetting the sourceSequence */\n> > > > + for (auto &ctrl : values_) {\n> > > > + for (auto i = oldWriteIndex; i < writeIndex_; i++) {\n> > > > + Info &info = ctrl.second[i + 1];\n> > > > + info = values_[ctrl.first][i];\n> > > > + }\n> > > > + }\n> > >\n> > > My main worries is now that, if I understand it right, the system\n> > > always tries to keep a maxDelay number of frames between the last\n> > > controls written to the sensor and the head of the queue, and to do so\n> > > it duplicate controls at every push() and apply(). This happens for\n> > > -every- frame and -every- request if I got it right ? Am I too\n> > > over-concerned this is expensive ?\n> >\n> > To my understanding that is no change to the way it was before. Before,\n> > that copying happed due to push({}), thereby messing up the\n> > synchronicity of push() and apply().\n> >\n> > >\n> > > > }\n> > > >\n> > > > device_->setControls(&out);\n> > > > --\n> > > > 2.40.1\n> > > >\n> >\n> > Thank you very much for all the feedback. It's time for a v3. And next\n> > time without the style issues :-)\n> >\n> > Cheers,\n> > Stefan","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 16DA8C3272\n\tfor ;\n\tThu, 21 Mar 2024 10:52:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1861663055;\n\tThu, 21 Mar 2024 11:52:26 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD32261C59\n\tfor ;\n\tThu, 21 Mar 2024 11:52:23 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A15882B3;\n\tThu, 21 Mar 2024 11:51:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Dpn9jdiH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711018315;\n\tbh=7KNaJK7vhMjChSAtDMGhs5ZzqciC+LNAcAfnoGxSAkA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Dpn9jdiHIpSuD3Ucar75ECs3cHgdR6ozkw7O45JAoIKTir4IaOve0VWV+nzeGxMRl\n\tUXtoZE35WptslqU38eDPD8vaZlK/FsGeclW2w6jPZZAcXm5IRRoOORrzA0Q7bXUEYg\n\tQqVwz0oXUEoTOyRd4VaWk4WFWnceegnKFiuMGuXo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<2bcf6izyvs5u27xfvkirit3ua5mdp65gjhyujdidnt4qiihccm@jfbsz4lc5wos>","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-7-stefan.klug@ideasonboard.com>\n\t\n\t<20240318132236.fptxax3pwsiagmiq@jasper>\n\t<2bcf6izyvs5u27xfvkirit3ua5mdp65gjhyujdidnt4qiihccm@jfbsz4lc5wos>","Subject":"Re: [PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","From":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","To":"Jacopo Mondi ,\n\tStefan Klug ","Date":"Thu, 21 Mar 2024 10:52:20 +0000","Message-ID":"<171101834073.3694868.11000332276393792018@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29031,"web_url":"https://patchwork.libcamera.org/comment/29031/","msgid":"","date":"2024-03-21T12:07:41","subject":"Re: [PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nGood to see this discussion continuing. Let me add a few observations,\nthough I won't comment exhaustively because I think there's just too\nmuch to unpack in this topic!\n\nOn Thu, 21 Mar 2024 at 10:52, Kieran Bingham\n wrote:\n>\n> Quoting Jacopo Mondi (2024-03-21 09:20:34)\n> > Hi Stefan\n> >\n> > On Mon, Mar 18, 2024 at 02:22:36PM +0100, Stefan Klug wrote:\n> > > Hi Jacopo,\n> > >\n> > > thank you for the review. Sorry for all the style issues. I'll fix them\n> > > asap.\n> > >\n> > > On Fri, Mar 15, 2024 at 05:15:58PM +0100, Jacopo Mondi wrote:\n> > > > Hi Stefan\n> > > >\n> > > > Not going to review the logic implementation in detail, it will take\n> > > > me some more time as delayed controls are kind of a complex beast, so\n> > > > this will be mostly a stylistic review with some stupid questions here\n> > > > and there\n> > > >\n> > > > On Wed, Mar 13, 2024 at 01:12:17PM +0100, Stefan Klug wrote:\n> > > > > Functional changes:\n> > > > > - Requests need to be queued for every frame\n> > > >\n> > > > Do you mean it is a requirement for the application to queue Request\n> > > > for every frame ?\n> > >\n> > > Yes, that's actually no change. The reason why I wrote that, is that in\n> > > the old delayed controls tests, there was always a call to\n> > > applyControls() before the first call to push().\n> > >\n> >\n> > Just to clarify, is this a requirement for PFC to happen, or in\n> > general ? (see below)\n\nGood point. Obviously we wouldn't want anything to be mandated for our\npipeline handler that didn't work for us!\n\n> >\n> > > >\n> > > > > - The startup phase is no longer treated as special case\n> > > > > - Requests carry a sequence number, so that we can detect when the system\n> > > > > gets out of sync\n> > > >\n> > > > Are you referring to the request sequence number or the frame number ?\n> > >\n> > > Are there actually two different groups of numbers? If yes, I miss something\n> > > conceptually here. The seuence number is meant to be as in \"This is the\n> > > request with seq x, which needs to be applied for frame x\"\n> > >\n> >\n> > I guess this is one of the thing we should clarify first before going\n> > further in the design.\n> >\n> > Both Request and frames have a sequence_number.\n\nIn our implementation we have a sequence number for the control lists\nin a request. That is, it goes up by one only when you supply some\ncontrols. Our experience is that it is easier for applications to use,\nunderstand and debug, rather than have numbers that are shooting up\nthe whole time regardless. It's also much easier to debug internally\nwhen implementing PFC within your pipeline handler as you aren't\nalways saying to yourself \"now what request number did I send those\ncontrols on again...??\"\n\n(Partly this is a reflection of the fact that we'd prefer separate\nqueues for control lists, and not associate them with\nrequests/buffers, though if buffers get removed from requests then you\nstart to wonder what a request really is. But that's the start of a\nbigger discussion!)\n\n> >\n> > Requests are queued to the camera by the application. Their sequence\n> > number increments for each Request created by the Camera. There are no\n> > requirments on the timing the application queues requests to the\n> > camera, one example is RPi Zero users that due to limited memory only\n> > seldom queue a Request\n> >\n> > Frames are produced by the sensor at a fixed time interval, and their\n> > sequence number increments at every frame produced by the sensor (or better,\n> > received by the CSI-2 rx, as sometimes frame gets dropped and gaps are\n> > created, anyway..)\n> >\n> > If an application wants full per-frame control, it needs to queue\n> > enough requests to compensate for the pipeline depth which counts for:\n> >\n> > - a full frame time as sensor's settings apply to the 'next' frame, hence\n> > the one that is getting exposed right now is 'gone'\n> > - on some ISP some frames of delay to generate statistics (iirc IPU3\n> > has 1 frame delay between the frame that gets processed and the one\n> > for which statistics are generated)\n> > - the sensor's maximum control delay\n> >\n> > A reasonable number (to be used as an example here) for the pipeline\n> > depth is something between 4 and 6 frames.\n> >\n> > Now, if an application wants full per frame control it has to queue\n> > requests early enough for the desired controls associated with a\n> > request to be realized, so it has to keep 'pipeline_depth' number of requests\n> > always queued to the camera. If you want controls for frame #10 to be\n> > realized in time you have to queue Request #10 at the time frame #6\n> > (or #4 depends on the pipeline_depth) is getting exposed.\n> >\n> > If the application doesn't do that, it creates gaps, and while the\n> > frame sequence number keeps incrementing (as the sensor is producing\n> > frames) the request sequence number gets only incremented when a new\n> > request is created, creating a mis-alignment between the request\n>\n> I agree with most of the above, but I want to clarify that request\n> sequences are generated/incremented only when a request is *queued* to\n> the camera, not when the request is created.\n\nYes, I think I agree with most of that. Just on the use of the term\n\"mis-alignment\" - for us, we regard this as quite \"normal\", it\nhappens. So it's important to us to define a scheme that is usable\nwhen this occurs.\n\n>\n> > sequence and the frame number.\n> >\n> > This is where \"Android PFC\" and \"RPI PFC\" differ. RPI wants to allow\n> > gaps in the Request by jumping ahead and apply the most recent\n> > controls to the most recent frame. The Android model requires\n> > applications to keep the requests queue filled in and to queue\n> > requests at the same pace as the frames gets produced by the sensor.\n> > if the application 'underrun' the Request queue, PFC simply can't\n> > happen.\n\nIn a sense, yes, but it seems like a slightly narrow interpretation.\nIn the Raspberry Pi scheme you still have PFC with just a single\nbuffer. What I mean is that every buffer that comes out tells you (via\na sequence number) exactly which set of controls have been applied. In\nfact, you can have back-to-back sequences of frames coming out, with\nperfect knowledge of how the controls change on each one, with fewer\nrequests than the pipeline depth.\n\n> >\n> > Now, you might have noticed we time the IPA with the application's\n> > requests. It means that we queue a buffer for statistics and run the\n> > IPA loop only when a Request is queued. To add confusion, some\n> > platforms (RkISP1) count requests using the sensor's frame number,\n> > some others (IPU3) use the Request sequence number.\n>\n> This is definitely an area I hope we can work to clean up soon. And\n> indeed we've been talking about it already, but we need to figure this\n> out cleanly and apply the same decision to any of the libipa IPAs.\n>\n> > We have been talking recently about making IPAs free running. They\n> > need to receive stats and produce paramteres for every frame produced\n> > by the sensor, not for every Request, otherwise we depend on the\n> > application queue requests in time and at a fast enough pace.\n>\n> And of course this. Though free-running IPA is essentially a symptom of\n> how we will handle *not* achieving PFC?\n>\n> > This however doesn't change the fact that to obtain full PFC\n> > there need to be one request for each frame, but it's imho the first\n> > step to allow us to implement PFC correctly.\n> >\n> > > > > - Controls for a given sequence number can be updated multiple times\n> > > > > as long as they are not yet sent out\n> > > > > - If it's too late, controls get delayed but they are not lost\n> > > >\n> > > > What do you mean not lost ? Will they be applied anyway ? what about\n> > > > the controls for the next frames ?\n> > >\n> > > If you request a control for frame 8, but the controls for that frame\n> > > are already sent to the sensor, the control will be applied at the\n> > > earliest possible frame (e.g. frame 10). If there are already request\n> > > queued for frame 9 or 10 the request from frame 8 is discarded.\n> > >\n> >\n> > My gut feeling is that's not what we want, as if an application is\n> > late in queueing a Request, it's just late and those controls are\n> > lost. However, if as in your example, the lost control switch an\n> > algorithm from Auto to Manual, ignoring it might not be the best idea\n>\n> I disagree here. (But I think it's due to terminology used rather than\n> what we are all actually expecting here).\n>\n> If a 'per frame control' is missed/late, indeed it is not possible to\n> apply to the correct frame. But I believe the state should be updated\n> and tracked.\n>\n> Lets imagine an application starts streaming with a manual exposure of\n> 10ms, and after 5 frames wants that to become 30ms exposure, and assume\n> no other request changes the manual exposure value.\n>\n> If for a non-determined reason request 5 was late, I wouldn't expect to\n> continue streaming indefinitely with 10ms, but that the streaming should\n> (at the earliest opportunity after frame 5) be configured to a 30ms\n> exposure.\n\nI agree with the \"disagreeing\" here. It's certainly important to\ndefine what happens, and for us, leaving the system in an uncertain\nstate would be bad.\n\nOne of the big runtime risks for us is not hitting the interrupts we\nneed for camera exposure updates. There are two things we could do:\n\n* Wait for the next frame and try again. So you might get the 10ms\nexposure coming out twice, and then changing to 30ms. Personally, I\nthink this is the most useful behaviour.\n\n* Merge the missed controls into the next set of controls, and carry\non. In the above example, you could find that you never got 10ms, as\nthe 10ms would have been applied with the next frame, except that the\nnext frame is already going to get 30ms (which takes precedence). So\nthe 10ms is lost in a sense, though the system always ends in a known\nstate which matches the last controls that you sent. And importantly,\nthe reporting via control list sequence numbers tells you the exact\nstate of every frame that you get, whatever happens.\n\nActually our implementation takes the 2nd approach even though I\nprefer the first. The reason is that the coupling of requests and\ncontrols means you end up with a theoretically unbounded delay between\nthem which is (theoretically) annoying to handle.\n\n>\n>\n> In fact, re-reading the above, both my text and Jacopo's - what we\n> really need to do here is make the distinction between 'when' the\n> control is lost. And I would not call the controls 'lost' but\n> overwritten / superceeded by new incoming controls when applicable. And\n> if there is no superceeding control/data update - then the control is\n> not lost, just late. But if there is a new update which is on time -\n> that would be able to superceed and apply correctly. At that point - it\n> could be considered that the older control is 'lost' but I would just\n> say that it has been updated rather than lost.\n\nYes, I think that agrees with what I wrote just above!\n\n>\n> In 'android model' We should not 'lose' the data. But we can 'update'\n> (replace) the data.\n>\n> I think the RPi model is different here in that instead of replace, it\n> would only push back controls so replacing would not be allowed. But in\n> the same vein - the global context/state would always be updated.\n>\n>\n> > > > > - Requests attached to frame 0 don't get lost\n> > > >\n> > > > Do you mean the very first request ? Do you mean to apply controls\n> > > > before streaming is started ?\n> > > >\n> > >\n> > > Yes, controls that are set on the very first request. I added a seperate\n> > > testcase for that in the next version. Implicitly it was tested in\n> > > singleControlWithDelayStartUp()\n>\n> I think we discussed in the past that to establish a configuration at\n> frame 0, the controls have to be applied during camera->start(controls).\n> And that for $MaxDelayedControlDelay at the beginning of a stream\n> otherwise, we simply can not guarantee PFC.\n>\n> We can not apply manual exposure/gains that have a delay of 2 until the\n> SoF for the first frame - so while we can set frame 0 - we can *never*\n> set frame 1 correctly (in theory if it were supposed to be different).\n>\n> Please correct me if I'm wrong above!\n\nI think I agree with that!\n\nDavid\n\n>\n> > > > > Technical notes:\n> > > > > A sourceSequence_ replaces the updated flag to be able to track from which\n> > > > > frame the update stems. This is needed for the following use case:\n> > > > > Assume e.g. On frame 10 an ExposureTime=42 is queued because the user wants\n> > > > > manual exposure. Now the agc gets the stats for frame 8 (where auto\n> > > > > regulation is still active) and pushes a new ExposureTime for frame 9.\n> > > > > Frame 9 was already sent out, so it gets delayed to frame 11 (assuming a\n> > > > > delay of 2 on ExposureTime). This would revert the request from frame 10.\n> > > > > Taking the sourceSequence into account, the delayed request from frame\n> > > > > 9 will be discarded, which is correct.\n> > > >\n> > > > Also this is not 100% clear to me. The request for frame 10 that says\n> > > > \"manual 42\" should be processed by the IPA/pipeline early enough to\n> > > > make sure the \"ExposureTime=42\" control is emitted early enough for\n> > > > being applied in time (assuming 2 frames of delay, this mean the\n> > > > control should be emitted before frame 8 has started exposing).\n> > > >\n> > > > If the AGC gets the stats for frame 8 and computes auto-value for\n> > > > frame 11, it means it is late and the IPA is not processing requests\n> > > > early enough ? have you seen this happening ?\n> > >\n> > > As the ISP is acting in a closed loop, it can only react on stats that\n> > > were calculated for the last frame that entered the system. So it is by\n> > > definition always too late. In the example above, the manual 42 can\n> > > actually be sent out early enough because it doesn't require information\n> > > from the stats module. This is the reason for patch 11/12 \"rkisp1: Fix\n> > > per-frame-controls\"\n> > >\n> >\n> > I'll look at that patch before commenting further\n>\n> The one thing I remember from when we looked at this last time is that\n> we need a better way to convey all the timing in our text. Which means\n> we need diagrams and pictures IMO.\n>\n> I really hope we can work on our tracing tools to help us generate such\n> timing diagram/images in some form beacuse I think we need the same\n> diagrams to report what /happens/ too! I have ideas ... but lacking in\n> time currently!\n>\n> --\n> Kieran\n>\n> > > > > Signed-off-by: Stefan Klug \n> > > > > ---\n> > > > > include/libcamera/internal/delayed_controls.h | 12 +-\n> > > > > src/libcamera/delayed_controls.cpp | 207 ++++++++++++++----\n> > > > > 2 files changed, 174 insertions(+), 45 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> > > > > index 4f8d2424..2738e8bf 100644\n> > > > > --- a/include/libcamera/internal/delayed_controls.h\n> > > > > +++ b/include/libcamera/internal/delayed_controls.h\n> > > > > @@ -30,25 +30,29 @@ public:\n> > > > > void reset();\n> > > > >\n> > > > > bool push(const ControlList &controls);\n> > > > > + bool pushForFrame(uint32_t sequence, const ControlList &controls);\n> > > > > ControlList get(uint32_t sequence);\n> > > > >\n> > > > > void applyControls(uint32_t sequence);\n> > > > >\n> > > > > private:\n> > > > > + bool controlsAreQueuedForFrame(unsigned int frame, const ControlList &controls);\n> > > >\n> > > > Functions should be defined after types. Please move this after the\n> > > > ControlRingBuffer type definition below. Also, this is a very long name :)\n> > > >\n> > > > > +\n> > > > > class Info : public ControlValue\n> > > > > {\n> > > > > public:\n> > > > > Info()\n> > > > > - : updated(false)\n> > > > > + : sourceSequence_(0)\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > - Info(const ControlValue &v, bool updated_ = true)\n> > > > > - : ControlValue(v), updated(updated_)\n> > > > > + Info(const ControlValue &v, std::optional sourceSequence = std::nullopt)\n> > > > > + : ControlValue(v), sourceSequence_(sourceSequence)\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > - bool updated;\n> > > > > + /* The sequence id, this info stems from*/\n> > > > > + std::optional sourceSequence_;\n> > > > > };\n> > > > >\n> > > > > /* \\todo Make the listSize configurable at instance creation time. */\n> > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > > > index 86571cd4..59314388 100644\n> > > > > --- a/src/libcamera/delayed_controls.cpp\n> > > > > +++ b/src/libcamera/delayed_controls.cpp\n> > > > > @@ -115,8 +115,9 @@ DelayedControls::DelayedControls(V4L2Device *device,\n> > > > > */\n> > > > > void DelayedControls::reset()\n> > > > > {\n> > > > > - queueIndex_ = 1;\n> > > > > - writeIndex_ = 0;\n> > > > > + queueIndex_ = 0;\n> > > > > + /* Frames up to maxDelay_ will be based on sensor init values. */\n> > > >\n> > > > Ok, so this is the startup condition!\n> > >\n> > > I'm not sure if I got you right here. Would it be clearer to say\n> > > \"Frames up to maxDelay_ will be based on the values obtained in reset()?\n> > >\n> > > >\n> > > > > + writeIndex_ = maxDelay_;\n> > > > >\n> > > > > /* Retrieve control as reported by the device. */\n> > > > > std::vector ids;\n> > > > > @@ -133,26 +134,129 @@ void DelayedControls::reset()\n> > > > > * Do not mark this control value as updated, it does not need\n> > > > > * to be written to to device on startup.\n> > > >\n> > > > does the comment needs updating too ?\n> > > >\n> > >\n> > > hm, I removed it altogether. There is nothing special to be said here.\n> > >\n> > > > > */\n> > > > > - values_[id][0] = Info(ctrl.second, false);\n> > > > > + values_[id][0] = Info(ctrl.second, 0);\n> > > > > }\n> > > > > +\n> > > > > + /* Copy state from previous frames. */\n> > > >\n> > > > Are these from the previous frames or simply the initial control values ?\n> > >\n> > > Yes, initial ones. I updated the comment and factored the loop out.\n> > >\n> > > >\n> > > > > + for (auto &ctrl : values_) {\n> > > > > + for (auto i = queueIndex_; i < writeIndex_; i++) {\n> > > >\n> > > > unsigned int i\n> > > >\n> > > > > + Info &info = ctrl.second[i + 1];\n> > > > > + info = ctrl.second[i];\n> > > > > + info.sourceSequence_.reset();\n> > > > > + }\n> > > > > + }\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Helper function to check if controls are queued already\n> > > > > + * \\param[in] sequence Sequence number to check\n> > > > > + * \\param[in] controls List of controls to compare against\n> > > > > + *\n> > > > > + * This function checks if the controls queued for frame \\a sequence\n> > > > > + * are equal to \\a controls. This is helpful in cases where a control algorithm\n> > > > > + * unconditionally queues controls for every frame, but always too late.\n> > > > > + * In that case this can be used to check if incoming controls are already queued\n> > > > > + * or need to be queued for a later frame.\n> > > >\n> > > > I'm interested to see where and how this is used\n> > > >\n> > > > > + *\n> > > > > + * \\returns true if \\a controls are queued for the given sequence\n> > > > , false otherwise\n> > > > > + */\n> > > > > +bool DelayedControls::controlsAreQueuedForFrame(unsigned int sequence, const ControlList &controls)\n> > > >\n> > > > can easily be made < 80 cols.\n> > > >\n> > > > > +{\n> > > > > + auto idMap = controls.idMap();\n> > > > > + if (!idMap) {\n> > > > > + LOG(DelayedControls, Warning) << \" idmap is null\";\n> > > > > + return false;\n> > > > > + } else {\n> > > >\n> > > > No need for an else branch, you have returned already\n> > > >\n> > > > > + for (const auto &[id, value] : controls) {\n> > > > > + if (values_[idMap->at(id)][sequence] != value) {\n> > > > > + return false;\n> > > > > + }\n> > > >\n> > > > No {}\n> > > >\n> > > > > + }\n> > > > > + }\n> > > >\n> > > > Empty line\n> > > >\n> > > > > + return true;\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> > > > > + * \\deprecated This function is deprecated in favour of pushForFrame\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> > > > > + * \\returns See return value of DelayedControls::pushForFrame\n> > > > > */\n> > > > > bool DelayedControls::push(const ControlList &controls)\n> > > > > {\n> > > > > - /* Copy state from previous frame. */\n> > > > > - for (auto &ctrl : values_) {\n> > > > > - Info &info = ctrl.second[queueIndex_];\n> > > > > - info = values_[ctrl.first][queueIndex_ - 1];\n> > > > > - info.updated = false;\n> > > > > + LOG(DelayedControls, Debug) << \"Using deprecated function push(controls): \" << queueIndex_;\n> > > >\n> > > > break line\n> > > >\n> > > > > + return pushForFrame(queueIndex_, controls);\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Push a set of controls for a given frame\n> > > > > + * \\param[in] sequence Sequence to push the controls for\n> > > > > + * \\param[in] controls List of controls to add to the device queue\n> > > > > + *\n> > > > > + * Pushes the controls given by \\a controls, to be applied at frame \\a sequence.\n> > > >\n> > > > Apply \\a controls to frame \\a sequence\n> > >\n> > > Isn't that a different meaning? I'm not happy with my sentence either.\n> > > What about: Store \\a controls to be applied for frame \\a sequence.\n> > >\n> > > >\n> > > > > + *\n> > > > > + * If there are controls already queued for that frame, these get updated.\n> > > >\n> > > > s/these/they\n> > > >\n> > > > > + *\n> > > > > + * If it's too late for frame \\a sequence (controls are already sent to the sensor),\n> > > > > + * the system checks if the controls that where written out for frame \\a sequence\n> > > > > + * are the same as the requested ones. In this case, nothing is done.\n> > > > > + * If they differ, the controls get queued for the earliest frame possible\n> > > > > + * if no other controls with a higher sequence number are queued for that frame already.\n> > > > > + *\n> > > > > + * \\returns true if \\a controls are accepted, or false otherwise\n> > > > > + */\n> > > > > +\n> > > >\n> > > > Stray empty line\n> > > >\n> > > > > +bool DelayedControls::pushForFrame(uint32_t sequence, const ControlList &controls)\n> > > >\n> > > > Can't you just overload ::push() ?\n> > >\n> > > Sure, I can do that. For my brain it is easier to read\n> > > d->pushForFrame(x, ctrls)\n> > > than\n> > > d->push(x, ctrls)\n> > > But thats most likely a matter of taste (or I worked too much with\n> > > Apple's macOS APIs)\n> > >\n> > > >\n> > > > > +{\n> > > > > + if (sequence < queueIndex_) {\n> > > > > + LOG(DelayedControls, Debug) << \"Got updated data for frame:\" << sequence;\n> > > > > + }\n> > > >\n> > > > No {}\n> > > >\n> > > > > +\n> > > > > + if (sequence > queueIndex_) {\n> > > > > + LOG(DelayedControls, Warning) << \"Hole in queue sequence. This should not happen. Expected: \"\n> > > > > + << queueIndex_ << \" got \" << sequence;\n> > > > > + }\n> > > >\n> > > > no {}\n> > > > very long line\n> > > >\n> > > > > +\n> > > > > + uint32_t updateIndex = sequence;\n> > > > > + /* check if its too late for the request */\n> > > > > + if (sequence < writeIndex_) {\n> > > > > + /* Check if we can safely ignore the request */\n> > > > > + if (controls.empty() || controlsAreQueuedForFrame(sequence, controls)) {\n> > > >\n> > > > Are we comparing control lists at every push() (so likely for every\n> > > > request) ?\n> > >\n> > > Only if we are too late. But in a ISP closed loop, that might happen\n> > > continuously.\n> > >\n> > > >\n> > > >\n> > > > > + if (sequence >= queueIndex_) {\n> > > > > + queueIndex_ = sequence + 1;\n> > > > > + return true;\n> > > > > + }\n> > > > > + } else {\n> > > > > + LOG(DelayedControls, Debug) << \"Controls for frame \" << sequence\n> > > > > + << \" are already in flight. Will be queued for frame \" << writeIndex_;\n> > > > > + updateIndex = writeIndex_;\n> > > > > + }\n> > > >\n> > > > veeeery long lines\n> > > >\n> > > > > + }\n> > > > > +\n> > > > > + if (static_cast(updateIndex) - static_cast(writeIndex_) >= listSize - static_cast(maxDelay_)) {\n> > > >\n> > > > Why signed ?\n> > >\n> > > Yep, updateIndex is always >= writeIndex. You are right. I changed\n> > > listSize, to be unsigned also.\n> > >\n> > > > >\n> > > > > + /* The system is in an undefined state now. This will heal itself, as soon as all controls where rewritten */\n> > > > > + LOG(DelayedControls, Error) << \"Queue length exceeded. The system is out of sync. Index to update:\"\n> > > > > + << updateIndex << \" Next Index to apply: \" << writeIndex_;\n> > > > > + }\n> > > > > +\n> > > > > + /**\n> > > >\n> > > > /** is for doxygen\n> > > >\n> > > > > + * Prepare the ringbuffer entries with previous data.\n> > > > > + * Data up to [writeIndex_] gets prepared in applyControls.\n> > > > > + */\n> > > > > + if (updateIndex > writeIndex_ && updateIndex >= queueIndex_) {\n> > > > > + LOG(DelayedControls, Debug) << \"Copy from previous \" << queueIndex_;\n> > > > > + for (auto i = queueIndex_; i < updateIndex; i++) {\n> > > > > + /* Copy state from previous queue. */\n> > > > > + for (auto &ctrl : values_) {\n> > > > > + auto &ringBuffer = ctrl.second;\n> > > > > + ringBuffer[i] = ringBuffer[i - 1];\n> > > > > + ringBuffer[i].sourceSequence_.reset();\n> > > > > + }\n> > > > > + }\n> > > > > }\n> > > > >\n> > > > > /* Update with new controls. */\n> > > > > @@ -167,20 +271,34 @@ bool DelayedControls::push(const ControlList &controls)\n> > > > >\n> > > > > const ControlId *id = it->second;\n> > > > >\n> > > > > - if (controlParams_.find(id) == controlParams_.end())\n> > > > > - return false;\n> > > > > -\n> > > > > - Info &info = values_[id][queueIndex_];\n> > > > > + if (controlParams_.find(id) == controlParams_.end()) {\n> > > > > + LOG(DelayedControls, Error) << \"Could not find params for control \" << id << \" ignored\";\n> > > >\n> > > > long line\n> > > >\n> > > > > + continue;\n> > > > > + }\n> > > >\n> > > > Is ignoring the control better than erroring out ? Is this worth a\n> > > > separate change ?\n> > > >\n> > > > >\n> > > > > - info = Info(control.second);\n> > > > > + Info &info = values_[id][updateIndex];\n> > > >\n> > > > Move after the comment block\n> > > >\n> > > > > + /*\n> > > > > + * Update the control only, if the already existing value stems from a request\n> > > >\n> > > > s/only,/only\n> > > >\n> > > > long line\n> > > >\n> > > > > + * with a sequence number smaller or equal to the current one\n> > > > > + */\n> > > > > + if (info.sourceSequence_.value_or(0) <= sequence) {\n> > > > > + info = Info(control.second, sequence);\n> > > > >\n> > > > > - LOG(DelayedControls, Debug)\n> > > > > - << \"Queuing \" << id->name()\n> > > > > - << \" to \" << info.toString()\n> > > > > - << \" at index \" << queueIndex_;\n> > > > > + LOG(DelayedControls, Debug)\n> > > > > + << \"Queuing \" << id->name()\n> > > > > + << \" to \" << info.toString()\n> > > > > + << \" at index \" << updateIndex;\n> > > > > + } else {\n> > > > > + LOG(DelayedControls, Warning)\n> > > > > + << \"Skipped update \" << id->name()\n> > > > > + << \" at index \" << updateIndex;\n> > > > > + }\n> > > > > }\n> > > > >\n> > > > > - queueIndex_++;\n> > > > > + LOG(DelayedControls, Debug) << \"Queued frame: \" << queueIndex_ << \" it will be active in \" << updateIndex;\n> > > >\n> > > > very long line\n> > > >\n> > > > > +\n> > > > > + if (sequence >= queueIndex_)\n> > > > > + queueIndex_ = sequence + 1;\n> > > > >\n> > > > > return true;\n> > > > > }\n> > > > > @@ -202,19 +320,17 @@ bool DelayedControls::push(const ControlList &controls)\n> > > > > */\n> > > > > ControlList DelayedControls::get(uint32_t sequence)\n> > > > > {\n> > > > > - unsigned int index = std::max(0, sequence - maxDelay_);\n> > > > > -\n> > > >\n> > > > This can now go because the initial frames are now populated, right ?\n> > > >\n> > > > > ControlList out(device_->controls());\n> > > > > for (const auto &ctrl : values_) {\n> > > > > const ControlId *id = ctrl.first;\n> > > > > - const Info &info = ctrl.second[index];\n> > > > > + const Info &info = ctrl.second[sequence];\n> > > > >\n> > > > > out.set(id->id(), info);\n> > > > >\n> > > > > LOG(DelayedControls, Debug)\n> > > > > << \"Reading \" << id->name()\n> > > > > << \" to \" << info.toString()\n> > > > > - << \" at index \" << index;\n> > > > > + << \" at index \" << sequence;\n> > > > > }\n> > > > >\n> > > > > return out;\n> > > > > @@ -222,16 +338,21 @@ ControlList DelayedControls::get(uint32_t sequence)\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> > > > > + * \\param[in] sequence Sequence number of the frame that started (0-based)\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> > > > > + * Inform the state machine that a new frame has started to arrive at the receiver\n> > > > > + * (e.g. the sensor started to clock out pixels) and of its sequence\n> > > > > + * number. This is usually the earliest point in time to update registers in the\n> > > >\n> > > > Not sure this comment change is necessary\n> > >\n> > > For me it was unclear if \"a new frame has started\" meant:\n> > >\n> > > a) That we are now setting controls to start the exposure of that new frame\n> > > b) That the sensor started exposing that frame\n> > > c) That a new frame started to arrive on the host\n> > >\n> > > Native speakers? Any better ideas?\n> > >\n> > > >\n> > > > > + * sensor for upcoming frames. Any user of these helpers is responsible to inform\n> > > > > + * the helper about the start of any frame. This can be connected with ease to\n> > > > > + * the start of a exposure (SOE) V4L2 event.\n> > > >\n> > > > stay in 80-col, please\n> > > >\n> > > > > */\n> > > > > void DelayedControls::applyControls(uint32_t sequence)\n> > > > > {\n> > > > > - LOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> > > > > + LOG(DelayedControls, Debug) << \"Apply controls \" << sequence;\n> > > > > + if (sequence != writeIndex_ - maxDelay_) {\n> > > > > + LOG(DelayedControls, Warning) << \"Sequence and writeIndex are out of sync. Expected seq: \" << writeIndex_ - maxDelay_ << \" got \" << sequence;\n> > > >\n> > > > Not sure I get why this is an error. Is this to guarantee the IPA\n> > > > always stays exactly maxDelay_ frames in advance ? can anything like\n> > > > this be guaranteed ?\n> > > >\n> > >\n> > > In general I would say so. applyControls is normally called as\n> > > applyControls(0)\n> > > applyControls(1)\n> > > applyControls(2)\n> > >\n> > > This warning is there to inform about missed applyControls. E.g.\n> > > applyControls(1) <-- first warning. Index 0 missed\n> > > applyControls(3) <-- secod warning. Index 2 missed\n> > >\n> > > If we want that to be acceptable, we would need to accumulate all control\n> > > changes from the missed frames...\n> > >\n> > > > > + }\n> > > >\n> > > > no {}\n> > > > very long line\n> > > >\n> > > > >\n> > > > > /*\n> > > > > * Create control list peeking ahead in the value queue to ensure\n> > > > > @@ -241,10 +362,10 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > > > > for (auto &ctrl : values_) {\n> > > > > const ControlId *id = ctrl.first;\n> > > > > unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;\n> > > > > - unsigned int index = std::max(0, writeIndex_ - delayDiff);\n> > > > > + unsigned int index = writeIndex_ - delayDiff;\n> > > > > Info &info = ctrl.second[index];\n> > > > >\n> > > > > - if (info.updated) {\n> > > > > + if (info.sourceSequence_.has_value()) {\n> > > > > if (controlParams_[id].priorityWrite) {\n> > > > > /*\n> > > > > * This control must be written now, it could\n> > > > > @@ -262,21 +383,25 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > > > > }\n> > > > >\n> > > > > LOG(DelayedControls, Debug)\n> > > > > - << \"Setting \" << id->name()\n> > > > > - << \" to \" << info.toString()\n> > > > > - << \" at index \" << index;\n> > > > > -\n> > > > > - /* Done with this update, so mark as completed. */\n> > > > > - info.updated = false;\n> > > > > + << \"Writing \" << id->name()\n> > > > > + << \" (\" << info.toString() << \") \"\n> > > > > + << \" for frame \" << index;\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > - writeIndex_ = sequence + 1;\n> > > > > + auto oldWriteIndex = writeIndex_;\n> > > > > + writeIndex_ = sequence + maxDelay_ + 1;\n> > > > >\n> > > > > - while (writeIndex_ > queueIndex_) {\n> > > > > + if (writeIndex_ >= queueIndex_ && writeIndex_ > oldWriteIndex) {\n> > > > > LOG(DelayedControls, Debug)\n> > > > > - << \"Queue is empty, auto queue no-op.\";\n> > > > > - push({});\n> > > > > + << \"Index \" << writeIndex_ << \" is not yet queued. Prepare with old state\";\n> > > > > + /* Copy state from previous frames without resetting the sourceSequence */\n> > > > > + for (auto &ctrl : values_) {\n> > > > > + for (auto i = oldWriteIndex; i < writeIndex_; i++) {\n> > > > > + Info &info = ctrl.second[i + 1];\n> > > > > + info = values_[ctrl.first][i];\n> > > > > + }\n> > > > > + }\n> > > >\n> > > > My main worries is now that, if I understand it right, the system\n> > > > always tries to keep a maxDelay number of frames between the last\n> > > > controls written to the sensor and the head of the queue, and to do so\n> > > > it duplicate controls at every push() and apply(). This happens for\n> > > > -every- frame and -every- request if I got it right ? Am I too\n> > > > over-concerned this is expensive ?\n> > >\n> > > To my understanding that is no change to the way it was before. Before,\n> > > that copying happed due to push({}), thereby messing up the\n> > > synchronicity of push() and apply().\n> > >\n> > > >\n> > > > > }\n> > > > >\n> > > > > device_->setControls(&out);\n> > > > > --\n> > > > > 2.40.1\n> > > > >\n> > >\n> > > Thank you very much for all the feedback. It's time for a v3. And next\n> > > time without the style issues :-)\n> > >\n> > > Cheers,\n> > > Stefan","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 0CEFEBD160\n\tfor ;\n\tThu, 21 Mar 2024 12:07:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E968C62827;\n\tThu, 21 Mar 2024 13:07:56 +0100 (CET)","from mail-qv1-xf2c.google.com (mail-qv1-xf2c.google.com\n\t[IPv6:2607:f8b0:4864:20::f2c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 758D862827\n\tfor ;\n\tThu, 21 Mar 2024 13:07:54 +0100 (CET)","by mail-qv1-xf2c.google.com with SMTP id\n\t6a1803df08f44-690be110d0dso5862806d6.3\n\tfor ;\n\tThu, 21 Mar 2024 05:07:54 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"JLuFHrMS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1711022873; x=1711627673;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=ssBiccLyMaR+48KNffA2V5+wi3wnFz8QQhjq4ksA9P4=;\n\tb=JLuFHrMSAVAJqZvDVEJ9M2W7URWAhhBSt5YkJtaDgc0zAhXX2tE3+LYUuZGocip79y\n\tnGNzwCfNC9wEcEZCXuiBm1oo6Zf7fFVuWqBPq/qKwlCNzxapSSb0cRNFTDnLOLHJh2wb\n\tKFaXZTFPqAdXrVeTY3jq0Wa6xyBWxnd7YSQPbcz2G2qpt4JvrObUSIKqFLsKmjFK5KMg\n\tIdEIby4TU8LGHpAI4W+ttmfmBzPXhnPlGfhCFw845HFwWnjw6e/W6guPs6ujFvPg2UiT\n\thS7py8zmpUV8wffol5/x6sarFGDtPVAV2x6H813RUkNr/XB9uCq2IOfSpQXL4d3wizWT\n\t8rNA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711022873; x=1711627673;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=ssBiccLyMaR+48KNffA2V5+wi3wnFz8QQhjq4ksA9P4=;\n\tb=sJHvFo01amJfR9cky5Hg6bWk1ng23GWkdzgsO7hXTRK6LB6JbXlEAE0vjLZOGmohRc\n\t0soaFnIVZIRhDUbNATeOJlAXY8FFRBULSx7LjZHR2e0O0EFW7kSzzsk4coobki1/1r4L\n\tn4wUhowW47I4fwxov48ZJIAtGYmcV0k8XTveMPKEihLyc7COadGWIyZU+p54Wg7jKmLL\n\tkfyWCv56Id8LyaUa5D5ageLVuJjD+AnNUTjk8cB3xRuWbPn0krkbq4HnovFJinjHyv4X\n\thdHJ6WIGcdiijwsNQxbPT36y0SMMiCkWjVTzBjMNbonT2IS+jayXPYVE/e3a0OXmZ0uo\n\t5OSg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUoQ4IWMYmvukBY8wbhHI9tbay5cGsbpCG1O1wyBqGLXv1Uo2GP10C8IDVMSo+Wpzt+97WGRjaoXHkn54/X9PMc5NclQAaTxXr0v1U48LLfXjRl0w==","X-Gm-Message-State":"AOJu0Yw4xKtnRl8PytwybG25pnSjNHNFC9CHlQmlqfH0NlczYg/HHaC3\n\tztsmyCcYbTYvSKyXGM+XQgPachqeHCrbfDljB7QQj+UpuoWegyjDJxIoIx/KttQwkHtC8lbIhQP\n\tC3rBX/PUleXYs+WsTgHzzSD3w/tSjztRtWxrdYymfBZb0pZku","X-Google-Smtp-Source":"AGHT+IG6sfkYIlOhv3PIu0qBFHqe/x+5rGg03tRDg97FfXFxk5gz9++gJuddi/yQm02CXGVVomjBv13VfNrbQIPZZ+4=","X-Received":"by 2002:a05:6214:d0c:b0:693:b601:b875 with SMTP id\n\t12-20020a0562140d0c00b00693b601b875mr19106972qvh.15.1711022872262;\n\tThu, 21 Mar 2024 05:07:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-7-stefan.klug@ideasonboard.com>\n\t\n\t<20240318132236.fptxax3pwsiagmiq@jasper>\n\t<2bcf6izyvs5u27xfvkirit3ua5mdp65gjhyujdidnt4qiihccm@jfbsz4lc5wos>\n\t<171101834073.3694868.11000332276393792018@ping.linuxembedded.co.uk>","In-Reply-To":"<171101834073.3694868.11000332276393792018@ping.linuxembedded.co.uk>","From":"David Plowman ","Date":"Thu, 21 Mar 2024 12:07:41 +0000","Message-ID":"","Subject":"Re: [PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","To":"Kieran Bingham ","Cc":"Jacopo Mondi , \n\tStefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]