[{"id":16841,"web_url":"https://patchwork.libcamera.org/comment/16841/","msgid":"","date":"2021-05-08T00:30:30","subject":"Re: [libcamera-devel] [PATCH v6 5/5] ipa: raspberrypi: Rate-limit\n\tthe controller algorithms","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Fri, May 07, 2021 at 09:40:42AM +0100, Naushir Patuck wrote:\n> The controller algorithms currently run on every frame provided to the\n> IPA by the pipeline handler. This may be undesirable for very fast fps\n> operating modes where it could significantly increase the computation\n> cycles (per unit time) without providing any significant changes to the\n> IQ parameters. The added latencies could also cause dropped frames.\n> \n> Pass the FrameBuffer timestamp to the IPA through the controls. This\n> timestamp will be used to rate-limit the controller algorithms to run\n> with a minimum inter-frame time given by a compile time constant,\n> currently set to 16.66ms. On startup, we don't rate-limit the algorithms\n> until after the number of frames required for convergence.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 48 +++++++++++++++++--\n> .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++\n> 2 files changed, 49 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f6d1ab16a290..57f5497a6e6b 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -61,6 +61,14 @@ constexpr unsigned int DefaultExposureTime = 20000;\n> constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> \n> +/*\n> + * Determine the minimum allowable inter-frame duration (in us) to run the\n> + * controller algorithms. If the pipeline handler provider frames at a rate\n> + * higher than this, we rate-limit the controller Prepare() and Process() calls\n> + * to lower than or equal to this rate.\n> + */\n> +constexpr double controllerMinFrameDuration = 1e6 / 60.0;\n> +\n> LOG_DEFINE_CATEGORY(IPARPI)\n> \n> class IPARPi : public ipa::RPi::IPARPiInterface\n> @@ -68,7 +76,7 @@ class IPARPi : public ipa::RPi::IPARPiInterface\n> public:\n> \tIPARPi()\n> \t\t: controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0),\n> -\t\t lsTable_(nullptr), firstStart_(true)\n> +\t\t lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true)\n> \t{\n> \t}\n> \n> @@ -146,6 +154,12 @@ private:\n> \t/* Number of frames that need to be dropped on startup. */\n> \tunsigned int dropFrameCount_;\n> \n> +\t/* Frame timestamp for the last run of the controller. */\n> +\tuint64_t lastRunTimestamp_;\n> +\n> +\t/* Do we run a Controller::process() for this frame? */\n> +\tbool processPending_;\n> +\n> \t/* LS table allocation passed in from the pipeline handler. */\n> \tFileDescriptor lsTableHandle_;\n> \tvoid *lsTable_;\n> @@ -262,6 +276,7 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf\n> \tstartConfig->dropFrameCount = dropFrameCount_;\n> \n> \tfirstStart_ = false;\n> +\tlastRunTimestamp_ = 0;\n> }\n> \n> void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> @@ -406,7 +421,7 @@ void IPARPi::signalStatReady(uint32_t bufferId)\n> {\n> \tif (++checkCount_ != frameCount_) /* assert here? */\n> \t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> -\tif (frameCount_ > mistrustCount_)\n> +\tif (processPending_ && frameCount_ > mistrustCount_)\n> \t\tprocessStats(bufferId);\n> \n> \treportMetadata();\n> @@ -894,10 +909,11 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> \n> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> {\n> +\tint64_t frameTimestamp = data.controls.get(controls::draft::SensorTimestamp);\n> +\tRPiController::Metadata lastMetadata;\n> \tSpan embeddedBuffer;\n> \n> -\trpiMetadata_.Clear();\n> -\n> +\tlastMetadata = std::move(rpiMetadata_);\n> \tfillDeviceStatus(data.controls);\n> \n> \tif (data.embeddedBufferPresent) {\n> @@ -920,6 +936,30 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> \tif (data.embeddedBufferPresent)\n> \t\treturnEmbeddedBuffer(data.embeddedBufferId);\n> \n> +\t/* Allow a 10% margin on the comparison below. */\n> +\tconstexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;\n> +\tif (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> +\t frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) {\n\nI would likely have written this\n\n\tif (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n\t frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3 * 0.9) {\n\nbut it doesn't matter much.\n\n> +\t\t/*\n> +\t\t * Ensure we update the controller metadata with the new frame's\n> +\t\t * exposure/gain values so that the correct values are returned\n> +\t\t * out in libcamera metadata later on. Any other bits of metadata\n> +\t\t * that were added in helper_->Prepare() will also be moved across.\n> +\t\t * All other metadata values must remain the same as the last frame.\n> +\t\t *\n> +\t\t * We do this by overwriting the current frame's metadata into the\n> +\t\t * previous frame's metadata object, and then swapping the latter\n\n\"overwriting ... into\" sounds weird. If you can provide an updated text,\nI can fix when applying.\n\n> +\t\t * with the former.\n> +\t\t */\n> +\t\tlastMetadata.Overwrite(rpiMetadata_);\n> +\t\tstd::swap(rpiMetadata_, lastMetadata);\n\nThis could have been written\n\n\t\trpiMetadata_.Merge(lastMetadata);\n\nwithout a swap, with std::map::merge semantics for Metadata::Merge() :-)\nThe double swap seems a bit cumbersome.\n\nIf you're happy with the current implementation though, that's fine with\nme. In either case,\n\nReviewed-by: Laurent Pinchart \n\nIf you think Merge() would be cleaner, I'll merge v7.\n\n> +\t\tprocessPending_ = false;\n> +\t\treturn;\n> +\t}\n> +\n> +\tlastRunTimestamp_ = frameTimestamp;\n> +\tprocessPending_ = true;\n> +\n> \tControlList ctrls(ispCtrls_);\n> \n> \tcontroller_.Prepare(&rpiMetadata_);\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 2a917455500f..9cf9c8c6cebd 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1414,6 +1414,11 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \t\t * DelayedControl and queue them along with the frame buffer.\n> \t\t */\n> \t\tControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);\n> +\t\t/*\n> +\t\t * Add the frame timestamp to the ControlList for the IPA to use\n> +\t\t * as it does not receive the FrameBuffer object.\n> +\t\t */\n> +\t\tctrl.set(controls::draft::SensorTimestamp, buffer->metadata().timestamp);\n> \t\tbayerQueue_.push({ buffer, std::move(ctrl) });\n> \t} else {\n> \t\tembeddedQueue_.push(buffer);","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 63125BF829\n\tfor ;\n\tSat, 8 May 2021 00:30:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C8606890C;\n\tSat, 8 May 2021 02:30:38 +0200 (CEST)","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 6822968901\n\tfor ;\n\tSat, 8 May 2021 02:30:36 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D67FB3D7;\n\tSat, 8 May 2021 02:30:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QsAyEt6H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620433836;\n\tbh=RMg4Dbudsx/ylOQhMiTEjCUPFSN5i4NIG6n4JTydGr8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QsAyEt6HVoJhHZZYGCwW8k3UoyisfxLsTlWh03j6cV03qf9dLcj+ep8ZHFBXofY6i\n\tWatut05fzuHlenYYCvjPYkdx9PNy71R4PA+mdZ+AxxyVbNacZXUy3Mwb4XQD1mdh88\n\t7zSWYBej35pwjere41poEuEi+xJxfLZXEFrY7C4g=","Date":"Sat, 8 May 2021 03:30:30 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210507084042.31879-1-naush@raspberrypi.com>\n\t<20210507084042.31879-6-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210507084042.31879-6-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 5/5] ipa: raspberrypi: Rate-limit\n\tthe controller algorithms","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16843,"web_url":"https://patchwork.libcamera.org/comment/16843/","msgid":"","date":"2021-05-08T00:53:51","subject":"Re: [libcamera-devel] [PATCH v6 5/5] ipa: raspberrypi: Rate-limit\n\tthe controller algorithms","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Sat, May 08, 2021 at 03:30:31AM +0300, Laurent Pinchart wrote:\n> On Fri, May 07, 2021 at 09:40:42AM +0100, Naushir Patuck wrote:\n> > The controller algorithms currently run on every frame provided to the\n> > IPA by the pipeline handler. This may be undesirable for very fast fps\n> > operating modes where it could significantly increase the computation\n> > cycles (per unit time) without providing any significant changes to the\n> > IQ parameters. The added latencies could also cause dropped frames.\n> > \n> > Pass the FrameBuffer timestamp to the IPA through the controls. This\n> > timestamp will be used to rate-limit the controller algorithms to run\n> > with a minimum inter-frame time given by a compile time constant,\n> > currently set to 16.66ms. On startup, we don't rate-limit the algorithms\n> > until after the number of frames required for convergence.\n> > \n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 48 +++++++++++++++++--\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++\n> > 2 files changed, 49 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f6d1ab16a290..57f5497a6e6b 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -61,6 +61,14 @@ constexpr unsigned int DefaultExposureTime = 20000;\n> > constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> > constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> > \n> > +/*\n> > + * Determine the minimum allowable inter-frame duration (in us) to run the\n> > + * controller algorithms. If the pipeline handler provider frames at a rate\n> > + * higher than this, we rate-limit the controller Prepare() and Process() calls\n> > + * to lower than or equal to this rate.\n> > + */\n> > +constexpr double controllerMinFrameDuration = 1e6 / 60.0;\n> > +\n> > LOG_DEFINE_CATEGORY(IPARPI)\n> > \n> > class IPARPi : public ipa::RPi::IPARPiInterface\n> > @@ -68,7 +76,7 @@ class IPARPi : public ipa::RPi::IPARPiInterface\n> > public:\n> > \tIPARPi()\n> > \t\t: controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0),\n> > -\t\t lsTable_(nullptr), firstStart_(true)\n> > +\t\t lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true)\n> > \t{\n> > \t}\n> > \n> > @@ -146,6 +154,12 @@ private:\n> > \t/* Number of frames that need to be dropped on startup. */\n> > \tunsigned int dropFrameCount_;\n> > \n> > +\t/* Frame timestamp for the last run of the controller. */\n> > +\tuint64_t lastRunTimestamp_;\n> > +\n> > +\t/* Do we run a Controller::process() for this frame? */\n> > +\tbool processPending_;\n> > +\n> > \t/* LS table allocation passed in from the pipeline handler. */\n> > \tFileDescriptor lsTableHandle_;\n> > \tvoid *lsTable_;\n> > @@ -262,6 +276,7 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf\n> > \tstartConfig->dropFrameCount = dropFrameCount_;\n> > \n> > \tfirstStart_ = false;\n> > +\tlastRunTimestamp_ = 0;\n> > }\n> > \n> > void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> > @@ -406,7 +421,7 @@ void IPARPi::signalStatReady(uint32_t bufferId)\n> > {\n> > \tif (++checkCount_ != frameCount_) /* assert here? */\n> > \t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > -\tif (frameCount_ > mistrustCount_)\n> > +\tif (processPending_ && frameCount_ > mistrustCount_)\n> > \t\tprocessStats(bufferId);\n> > \n> > \treportMetadata();\n> > @@ -894,10 +909,11 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > \n> > void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> > {\n> > +\tint64_t frameTimestamp = data.controls.get(controls::draft::SensorTimestamp);\n\nActually the series doesn't compile, as controls::draft::SensorTimestamp\nhas become controls::SensorTimestamp. This should be easy to fix, but\nwould you mind sending a v7 ? I'd be more comfortable merging a properly\ntested series than updating this when applying.\n\n> > +\tRPiController::Metadata lastMetadata;\n> > \tSpan embeddedBuffer;\n> > \n> > -\trpiMetadata_.Clear();\n> > -\n> > +\tlastMetadata = std::move(rpiMetadata_);\n> > \tfillDeviceStatus(data.controls);\n> > \n> > \tif (data.embeddedBufferPresent) {\n> > @@ -920,6 +936,30 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> > \tif (data.embeddedBufferPresent)\n> > \t\treturnEmbeddedBuffer(data.embeddedBufferId);\n> > \n> > +\t/* Allow a 10% margin on the comparison below. */\n> > +\tconstexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;\n> > +\tif (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > +\t frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) {\n> \n> I would likely have written this\n> \n> \tif (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> \t frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3 * 0.9) {\n> \n> but it doesn't matter much.\n> \n> > +\t\t/*\n> > +\t\t * Ensure we update the controller metadata with the new frame's\n> > +\t\t * exposure/gain values so that the correct values are returned\n> > +\t\t * out in libcamera metadata later on. Any other bits of metadata\n> > +\t\t * that were added in helper_->Prepare() will also be moved across.\n> > +\t\t * All other metadata values must remain the same as the last frame.\n> > +\t\t *\n> > +\t\t * We do this by overwriting the current frame's metadata into the\n> > +\t\t * previous frame's metadata object, and then swapping the latter\n> \n> \"overwriting ... into\" sounds weird. If you can provide an updated text,\n> I can fix when applying.\n> \n> > +\t\t * with the former.\n> > +\t\t */\n> > +\t\tlastMetadata.Overwrite(rpiMetadata_);\n> > +\t\tstd::swap(rpiMetadata_, lastMetadata);\n> \n> This could have been written\n> \n> \t\trpiMetadata_.Merge(lastMetadata);\n> \n> without a swap, with std::map::merge semantics for Metadata::Merge() :-)\n> The double swap seems a bit cumbersome.\n> \n> If you're happy with the current implementation though, that's fine with\n> me. In either case,\n> \n> Reviewed-by: Laurent Pinchart \n> \n> If you think Merge() would be cleaner, I'll merge v7.\n> \n> > +\t\tprocessPending_ = false;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tlastRunTimestamp_ = frameTimestamp;\n> > +\tprocessPending_ = true;\n> > +\n> > \tControlList ctrls(ispCtrls_);\n> > \n> > \tcontroller_.Prepare(&rpiMetadata_);\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 2a917455500f..9cf9c8c6cebd 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1414,6 +1414,11 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > \t\t * DelayedControl and queue them along with the frame buffer.\n> > \t\t */\n> > \t\tControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);\n> > +\t\t/*\n> > +\t\t * Add the frame timestamp to the ControlList for the IPA to use\n> > +\t\t * as it does not receive the FrameBuffer object.\n> > +\t\t */\n> > +\t\tctrl.set(controls::draft::SensorTimestamp, buffer->metadata().timestamp);\n> > \t\tbayerQueue_.push({ buffer, std::move(ctrl) });\n> > \t} else {\n> > \t\tembeddedQueue_.push(buffer);","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 D4F9ABF829\n\tfor ;\n\tSat, 8 May 2021 00:53:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 57C8F6890C;\n\tSat, 8 May 2021 02:53:59 +0200 (CEST)","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 160C468901\n\tfor ;\n\tSat, 8 May 2021 02:53:57 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 829AA3D7;\n\tSat, 8 May 2021 02:53:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ovTJU4ym\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620435236;\n\tbh=w1+0xAblBubtcJ1zR3gkITbWXgAAk1aiuAvZVJDss0o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ovTJU4ymXhZVRmceIBBOG3mBPS8sBScRyYZwPWtFRX7B7r8fwK+1rRJBrQpdSgidM\n\tDCayYibTY8rBKYZvMpsnZaugXWgBFSbmym2AQB1+xnxpHL9LjLNj5EkFLe4RuFkH31\n\tvz71mkX1586055SNaAK5yT7pNroYRaK+8gENEMkw=","Date":"Sat, 8 May 2021 03:53:51 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210507084042.31879-1-naush@raspberrypi.com>\n\t<20210507084042.31879-6-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 5/5] ipa: raspberrypi: Rate-limit\n\tthe controller algorithms","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16844,"web_url":"https://patchwork.libcamera.org/comment/16844/","msgid":"","date":"2021-05-08T05:19:59","subject":"Re: [libcamera-devel] [PATCH v6 5/5] ipa: raspberrypi: Rate-limit\n\tthe controller algorithms","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Sat, 8 May 2021 at 01:30, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, May 07, 2021 at 09:40:42AM +0100, Naushir Patuck wrote:\n> > The controller algorithms currently run on every frame provided to the\n> > IPA by the pipeline handler. This may be undesirable for very fast fps\n> > operating modes where it could significantly increase the computation\n> > cycles (per unit time) without providing any significant changes to the\n> > IQ parameters. The added latencies could also cause dropped frames.\n> >\n> > Pass the FrameBuffer timestamp to the IPA through the controls. This\n> > timestamp will be used to rate-limit the controller algorithms to run\n> > with a minimum inter-frame time given by a compile time constant,\n> > currently set to 16.66ms. On startup, we don't rate-limit the algorithms\n> > until after the number of frames required for convergence.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 48 +++++++++++++++++--\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++\n> > 2 files changed, 49 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f6d1ab16a290..57f5497a6e6b 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -61,6 +61,14 @@ constexpr unsigned int DefaultExposureTime = 20000;\n> > constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> > constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> >\n> > +/*\n> > + * Determine the minimum allowable inter-frame duration (in us) to run\n> the\n> > + * controller algorithms. If the pipeline handler provider frames at a\n> rate\n> > + * higher than this, we rate-limit the controller Prepare() and\n> Process() calls\n> > + * to lower than or equal to this rate.\n> > + */\n> > +constexpr double controllerMinFrameDuration = 1e6 / 60.0;\n> > +\n> > LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> > class IPARPi : public ipa::RPi::IPARPiInterface\n> > @@ -68,7 +76,7 @@ class IPARPi : public ipa::RPi::IPARPiInterface\n> > public:\n> > IPARPi()\n> > : controller_(), frameCount_(0), checkCount_(0),\n> mistrustCount_(0),\n> > - lsTable_(nullptr), firstStart_(true)\n> > + lastRunTimestamp_(0), lsTable_(nullptr),\n> firstStart_(true)\n> > {\n> > }\n> >\n> > @@ -146,6 +154,12 @@ private:\n> > /* Number of frames that need to be dropped on startup. */\n> > unsigned int dropFrameCount_;\n> >\n> > + /* Frame timestamp for the last run of the controller. */\n> > + uint64_t lastRunTimestamp_;\n> > +\n> > + /* Do we run a Controller::process() for this frame? */\n> > + bool processPending_;\n> > +\n> > /* LS table allocation passed in from the pipeline handler. */\n> > FileDescriptor lsTableHandle_;\n> > void *lsTable_;\n> > @@ -262,6 +276,7 @@ void IPARPi::start(const ControlList &controls,\n> ipa::RPi::StartConfig *startConf\n> > startConfig->dropFrameCount = dropFrameCount_;\n> >\n> > firstStart_ = false;\n> > + lastRunTimestamp_ = 0;\n> > }\n> >\n> > void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> > @@ -406,7 +421,7 @@ void IPARPi::signalStatReady(uint32_t bufferId)\n> > {\n> > if (++checkCount_ != frameCount_) /* assert here? */\n> > LOG(IPARPI, Error) << \"WARNING: Prepare/Process\n> mismatch!!!\";\n> > - if (frameCount_ > mistrustCount_)\n> > + if (processPending_ && frameCount_ > mistrustCount_)\n> > processStats(bufferId);\n> >\n> > reportMetadata();\n> > @@ -894,10 +909,11 @@ void IPARPi::returnEmbeddedBuffer(unsigned int\n> bufferId)\n> >\n> > void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> > {\n> > + int64_t frameTimestamp =\n> data.controls.get(controls::draft::SensorTimestamp);\n> > + RPiController::Metadata lastMetadata;\n> > Span embeddedBuffer;\n> >\n> > - rpiMetadata_.Clear();\n> > -\n> > + lastMetadata = std::move(rpiMetadata_);\n> > fillDeviceStatus(data.controls);\n> >\n> > if (data.embeddedBufferPresent) {\n> > @@ -920,6 +936,30 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig\n> &data)\n> > if (data.embeddedBufferPresent)\n> > returnEmbeddedBuffer(data.embeddedBufferId);\n> >\n> > + /* Allow a 10% margin on the comparison below. */\n> > + constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;\n> > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > + frameTimestamp - lastRunTimestamp_ + eps <\n> controllerMinFrameDuration * 1e3) {\n>\n> I would likely have written this\n>\n> if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> frameTimestamp - lastRunTimestamp_ + eps <\n> controllerMinFrameDuration * 1e3 * 0.9) {\n>\n> but it doesn't matter much.\n>\n> > + /*\n> > + * Ensure we update the controller metadata with the new\n> frame's\n> > + * exposure/gain values so that the correct values are\n> returned\n> > + * out in libcamera metadata later on. Any other bits of\n> metadata\n> > + * that were added in helper_->Prepare() will also be\n> moved across.\n> > + * All other metadata values must remain the same as the\n> last frame.\n> > + *\n> > + * We do this by overwriting the current frame's metadata\n> into the\n> > + * previous frame's metadata object, and then swapping the\n> latter\n>\n> \"overwriting ... into\" sounds weird. If you can provide an updated text,\n> I can fix when applying.\n>\n> > + * with the former.\n> > + */\n> > + lastMetadata.Overwrite(rpiMetadata_);\n> > + std::swap(rpiMetadata_, lastMetadata);\n>\n> This could have been written\n>\n> rpiMetadata_.Merge(lastMetadata);\n>\n> without a swap, with std::map::merge semantics for Metadata::Merge() :-)\n> The double swap seems a bit cumbersome.\n>\n> If you're happy with the current implementation though, that's fine with\n> me. In either case,\n>\n\nYou are right, a Metadata::Merge() would be a less cumbersome way to do\nthis.\nI will rework this (and the previous) patch to add that and resubmit a v7.\n\nThanks,\nNaush\n\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n> If you think Merge() would be cleaner, I'll merge v7.\n>\n> > + processPending_ = false;\n> > + return;\n> > + }\n> > +\n> > + lastRunTimestamp_ = frameTimestamp;\n> > + processPending_ = true;\n> > +\n> > ControlList ctrls(ispCtrls_);\n> >\n> > controller_.Prepare(&rpiMetadata_);\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 2a917455500f..9cf9c8c6cebd 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1414,6 +1414,11 @@ void\n> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > * DelayedControl and queue them along with the frame\n> buffer.\n> > */\n> > ControlList ctrl =\n> delayedCtrls_->get(buffer->metadata().sequence);\n> > + /*\n> > + * Add the frame timestamp to the ControlList for the IPA\n> to use\n> > + * as it does not receive the FrameBuffer object.\n> > + */\n> > + ctrl.set(controls::draft::SensorTimestamp,\n> buffer->metadata().timestamp);\n> > bayerQueue_.push({ buffer, std::move(ctrl) });\n> > } else {\n> > embeddedQueue_.push(buffer);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 D5022BF831\n\tfor ;\n\tSat, 8 May 2021 05:20:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30D6A68919;\n\tSat, 8 May 2021 07:20:17 +0200 (CEST)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DEFE1602B8\n\tfor ;\n\tSat, 8 May 2021 07:20:15 +0200 (CEST)","by mail-lf1-x136.google.com with SMTP id h4so15777407lfv.0\n\tfor ;\n\tFri, 07 May 2021 22:20:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"H9mVI6it\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=TB0QE/pbRPdYgjTAV/ICbTmg5BLm6Kxeroq9fARfnxA=;\n\tb=H9mVI6it71Ko11l0Qa9LfehRuOvixKshL7i2jotw2hGCZcnpl0tvEv0gLgtIva5zYT\n\tFK6GAiFoAhIhGhITAJtUCbsrXcMQ7Wauf1ZU3Q8UCnCCv0jxMpEQUAgolFhDxwG76dI0\n\twhXE6GTqvPTNIyXhBCb3NteN1OA6TtRdBV8WA1ly15bxbmd0fjJeQzKu2shDihm3U9cc\n\t/8gyR+03n7h3vc3fXDNJlSEkZ/P1/bxXAJBUzzDYSjIwWrUtj9LcetAMr+MY19lFEwKt\n\tLaMN3gW/GC3JbcX53wJxZwvpuBUpDRVff5va8VFfd3WXVuzCb0e/dZ05R5VoOqtevWXi\n\t5VXw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=TB0QE/pbRPdYgjTAV/ICbTmg5BLm6Kxeroq9fARfnxA=;\n\tb=qoi548AoGfggIUUBJP3k7/fAqQNfCuhS63t9jI/ixTfZydhg9fB5/G2i7T7qTp4Y1d\n\tzUJcYTvT0gx/7zFrYV270SKWQnInpFk1r54nisSP54KEtsaMOKA139mM4EqqlWs+u79+\n\tm11zis2zNVlzSkRVQA4d+CK3/1a8P/k0fQw56UWledkxmek/ofK24WbouyaHrBZXD5kC\n\t+e6/gCRyN3RFcaWsyK7YgUT+SdljZ2vzwmf1yJ/em7X+EczLPSyWXtqWZH6IHtEVwvio\n\tOHiZGcjMuSa2Hkt/ws/s3ahITXe9YL9F/YrxZzGPVFhKpWpw4TMb9BXGaQWvRHtx7iTy\n\tyCkQ==","X-Gm-Message-State":"AOAM533A0ZMQEsCnMmVcq5cciOPU76LRvEwx1BW1dtt/CnKzyYUk/AQU\n\tgTqAmBwKAl6Kq7ozqa97LrvhgwiYJBfnZtDERuW9dw==","X-Google-Smtp-Source":"ABdhPJyiL+vm3T4+XlA3+YWB+EAr4aPtTCxXUkwjNB5w6XQXyLqfPieTppIxK23yfmIT+2BvfMsAxqi5ymgwXyHiyeM=","X-Received":"by 2002:a05:6512:10d4:: with SMTP id\n\tk20mr8751875lfg.210.1620451215043; \n\tFri, 07 May 2021 22:20:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20210507084042.31879-1-naush@raspberrypi.com>\n\t<20210507084042.31879-6-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Sat, 8 May 2021 06:19:59 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v6 5/5] ipa: raspberrypi: Rate-limit\n\tthe controller algorithms","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============4370623354444709788==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]