[{"id":16638,"web_url":"https://patchwork.libcamera.org/comment/16638/","msgid":"","date":"2021-04-27T07:34:33","subject":"Re: [libcamera-devel] [PATCH v5 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 Mon, Apr 19, 2021 at 02:34:51PM +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\nOut of curiosity, is the inter-frame time something you would envision\nbeing made dynamic in the future, maybe coming from the IPA\nconfiguration file ?\n\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 50 +++++++++++++++++--\n> .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++\n> 2 files changed, 51 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f6d1ab16a290..eac1cf8cfe44 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\nThis doesn't apply cleanly. I'm afraid I've lost track, is this series\nbased on another pending series, or have other patches been merged in\nthe meantime that would require a rebase ?\n\n> @@ -920,6 +936,32 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> \tif (data.embeddedBufferPresent)\n> \t\treturnEmbeddedBuffer(data.embeddedBufferId);\n> \n> +\tif (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> +\t frameTimestamp - lastRunTimestamp_ < controllerMinFrameDuration * 1e3) {\n\nThe intent, I assume, is to run the algorithms at max 60fps. Shouldn't\nwe allow for an error margin in the comparison ? Otherwise, with the\njitter, we would skip IPA runs for some frames when running at exactly\n60fps.\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 merging the current frame's metadata into the\n> +\t\t * previous frame's metadata object, and then moving the latter\n> +\t\t * into the former.\n> +\t\t */\n> +\t\tlastMetadata.Merge(rpiMetadata_);\n> +\t\trpiMetadata_ = std::move(lastMetadata);\n> +\t\tprocessPending_ = false;\n> +\t\tLOG(IPARPI, Debug) << \"Rate-limiting the controller! inter-frame duration: \"\n> +\t\t\t\t << frameTimestamp - lastRunTimestamp_\n> +\t\t\t\t << \", min duration: \"\n> +\t\t\t\t << controllerMinFrameDuration * 1e3;\n\nThat would result in loooots of debug messages with a > 60fps frame\nrate. Is it intended ?\n\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 B7905BDE19\n\tfor ;\n\tTue, 27 Apr 2021 07:34:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1751B688BA;\n\tTue, 27 Apr 2021 09:34:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8A9568883\n\tfor ;\n\tTue, 27 Apr 2021 09:34:39 +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 3120645E;\n\tTue, 27 Apr 2021 09:34:39 +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=\"rt3am6Xh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619508879;\n\tbh=r9Fe9Q6X1FOWO6INkz2HlSkQUBR4zCkcJhMuC3V9n10=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rt3am6XhKlQjD0CgNvIntWEO5wsjZQwlupZOlIjUb9qbCi9ZX6vYj03x94GSvI/2C\n\t9yrh9BdEP9znFq4wdOyiMB+Aw6GQcfXTnVSKkx5DCxvrHupFoFQN07qJw2iH75GI3h\n\tlgvtH2WXf1fTgOKEDfATGq04W1tus4EU19TFTU7U=","Date":"Tue, 27 Apr 2021 10:34:33 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210419133451.263733-1-naush@raspberrypi.com>\n\t<20210419133451.263733-6-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210419133451.263733-6-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 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":16651,"web_url":"https://patchwork.libcamera.org/comment/16651/","msgid":"","date":"2021-04-27T09:19:04","subject":"Re: [libcamera-devel] [PATCH v5 5/5] ipa: raspberrypi: Rate-limit\n\tthe controller algorithms","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the review. Answering at least one of your questions for\nNaush (as it relates to one of my patches)...\n\nOn Tue, 27 Apr 2021 at 08:34, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 19, 2021 at 02:34:51PM +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> Out of curiosity, is the inter-frame time something you would envision\n> being made dynamic in the future, maybe coming from the IPA\n> configuration file ?\n>\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 50 +++++++++++++++++--\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++\n> > 2 files changed, 51 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f6d1ab16a290..eac1cf8cfe44 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> > IPARPi()\n> > : controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0),\n> > - lsTable_(nullptr), firstStart_(true)\n> > + lastRunTimestamp_(0), lsTable_(nullptr), 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, 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 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 bufferId)\n> >\n> > void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> > {\n> > + int64_t frameTimestamp = 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>\n> This doesn't apply cleanly. I'm afraid I've lost track, is this series\n> based on another pending series, or have other patches been merged in\n> the meantime that would require a rebase ?\n\nI believe Naush helpfully rebased this patch on top of my \" ipa:\nraspberrypi: Use CamHelpers to generalise sensor metadata parsing\"\npatch. I think that one is still pending...?\n\nThanks\nDavid\n\n>\n> > @@ -920,6 +936,32 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> > if (data.embeddedBufferPresent)\n> > returnEmbeddedBuffer(data.embeddedBufferId);\n> >\n> > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > + frameTimestamp - lastRunTimestamp_ < controllerMinFrameDuration * 1e3) {\n>\n> The intent, I assume, is to run the algorithms at max 60fps. Shouldn't\n> we allow for an error margin in the comparison ? Otherwise, with the\n> jitter, we would skip IPA runs for some frames when running at exactly\n> 60fps.\n>\n> > + /*\n> > + * Ensure we update the controller metadata with the new frame's\n> > + * exposure/gain values so that the correct values are returned\n> > + * out in libcamera metadata later on. Any other bits of metadata\n> > + * that were added in helper_->Prepare() will also be moved across.\n> > + * All other metadata values must remain the same as the last frame.\n> > + *\n> > + * We do this by merging the current frame's metadata into the\n> > + * previous frame's metadata object, and then moving the latter\n> > + * into the former.\n> > + */\n> > + lastMetadata.Merge(rpiMetadata_);\n> > + rpiMetadata_ = std::move(lastMetadata);\n> > + processPending_ = false;\n> > + LOG(IPARPI, Debug) << \"Rate-limiting the controller! inter-frame duration: \"\n> > + << frameTimestamp - lastRunTimestamp_\n> > + << \", min duration: \"\n> > + << controllerMinFrameDuration * 1e3;\n>\n> That would result in loooots of debug messages with a > 60fps frame\n> rate. Is it intended ?\n>\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 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> > * DelayedControl and queue them along with the frame buffer.\n> > */\n> > ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);\n> > + /*\n> > + * Add the frame timestamp to the ControlList for the IPA to use\n> > + * as it does not receive the FrameBuffer object.\n> > + */\n> > + ctrl.set(controls::draft::SensorTimestamp, 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> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A7E18BDCC3\n\tfor ;\n\tTue, 27 Apr 2021 09:19:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8AC4688B5;\n\tTue, 27 Apr 2021 11:19:17 +0200 (CEST)","from mail-wm1-x32c.google.com (mail-wm1-x32c.google.com\n\t[IPv6:2a00:1450:4864:20::32c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A302605BD\n\tfor ;\n\tTue, 27 Apr 2021 11:19:16 +0200 (CEST)","by mail-wm1-x32c.google.com with SMTP id n84so4194699wma.0\n\tfor ;\n\tTue, 27 Apr 2021 02:19:16 -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=\"g+xKeHPx\"; 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=Yuo4a7ojBOtq0N2+745vaVutGjLkkTjuoW1v82QBjO0=;\n\tb=g+xKeHPxQe4OFnU5GTAFUmxt+3bDRKfxBoTh/uf/fD1qS9Hqu29cQwxBCEnVgjaibJ\n\tV9MQSC+xFp6aXnl6YZT5vbtLX3uHDou4+mzatKt469NyUemViJqe0fFYeb6vO0/G2LiA\n\t0XlQSW+HMG/rHKu7wTxPwgrTLsdMdfIovnAR/NbekQvAKT4tR4XGwtkzQbzTwS+UYLk1\n\tavPyEmVTQGil/FUZ9hJfvbL3r8nIoU36vS6m6XXL9jmlV8RMl/E2ha/uFRwdskjBXu8a\n\tJUrhAQkigcnVdAENDC3UN32GjtM+ftVcSWo7606IEze4YQoON5lJBleJdUUnZxQgiw7A\n\tdtxw==","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=Yuo4a7ojBOtq0N2+745vaVutGjLkkTjuoW1v82QBjO0=;\n\tb=etkls3N13Qt5Xj03oOj1U6oefWI+kn3PUVMdp1R2vRTCj+kx2lY+40+9r0sRtVrzK7\n\tvs7QYLB/HO7jj1tqs7x853q/AMciUP7S+/psNSlEjcyXrruSuLX5vFvzVU/1vlwF6E45\n\tVeuNwJtuUvk0eCNwlX/quTHfDkbD1zx28gtk/wq01LelRhK/X7Hu/62e5khDyTgrSShg\n\t4lYSfDXxcw9mH1tU+hWB3YCCbDM+2/oTzT35z+uMl3EFHJuqNuiaDCcARAORpflIm2It\n\tD9kf9tITl3PO97xzlsOw5B1HWJ5sryaV9HPUm+R5VS0vrl8D3QUS9gCeCp8RMTaJr+cx\n\t7lEg==","X-Gm-Message-State":"AOAM533TlsLCgy7kTWZr6os5QCOg+MNLi1VNfA2tjlTsYVdBGKYjW2ym\n\tMlTHvXWuHG4YkPkFXZQYJYHWJcKpNAZeniZ4csWvig==","X-Google-Smtp-Source":"ABdhPJw4k00WlXJTcfqBUkzelpB5f/WgxaemyI/46/yKSYbTsLXyRD6ObS4ehjx5GgZZ2gk/5rr71YS23H6JxuJsm84=","X-Received":"by 2002:a1c:398a:: with SMTP id\n\tg132mr3349307wma.114.1619515156220; \n\tTue, 27 Apr 2021 02:19:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20210419133451.263733-1-naush@raspberrypi.com>\n\t<20210419133451.263733-6-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Tue, 27 Apr 2021 10:19:04 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v5 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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16653,"web_url":"https://patchwork.libcamera.org/comment/16653/","msgid":"","date":"2021-04-27T09:39:03","subject":"Re: [libcamera-devel] [PATCH v5 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 review feedback.\n\nOn Tue, 27 Apr 2021 at 08:34, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 19, 2021 at 02:34:51PM +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> Out of curiosity, is the inter-frame time something you would envision\n> being made dynamic in the future, maybe coming from the IPA\n> configuration file ?\n>\n\nI did question this myself, and to be honest I don't really know. Initially\nI decided to make it a constant, but did wonder if we should make it\na config parameter for devices that are somewhat underpowered (e.g.\nPi Zero). In the end, I went with a constant value to avoid users\nhaving another config parameter to think about. If we do decide the\nother way, it is easy enough to switch.\n\n\n>\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 50 +++++++++++++++++--\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++\n> > 2 files changed, 51 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f6d1ab16a290..eac1cf8cfe44 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>\n> This doesn't apply cleanly. I'm afraid I've lost track, is this series\n> based on another pending series, or have other patches been merged in\n> the meantime that would require a rebase ?\n>\n\nThis commit will need to be applied over\nhttps://patchwork.libcamera.org/patch/11730/\n\n\n>\n> > @@ -920,6 +936,32 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig\n> &data)\n> > if (data.embeddedBufferPresent)\n> > returnEmbeddedBuffer(data.embeddedBufferId);\n> >\n> > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > + frameTimestamp - lastRunTimestamp_ <\n> controllerMinFrameDuration * 1e3) {\n>\n> The intent, I assume, is to run the algorithms at max 60fps. Shouldn't\n> we allow for an error margin in the comparison ? Otherwise, with the\n> jitter, we would skip IPA runs for some frames when running at exactly\n> 60fps.\n>\n\nGood point, I will add a 10% margin on the comparison above.\n\n\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 merging the current frame's metadata into\n> the\n> > + * previous frame's metadata object, and then moving the\n> latter\n> > + * into the former.\n> > + */\n> > + lastMetadata.Merge(rpiMetadata_);\n> > + rpiMetadata_ = std::move(lastMetadata);\n> > + processPending_ = false;\n> > + LOG(IPARPI, Debug) << \"Rate-limiting the controller!\n> inter-frame duration: \"\n> > + << frameTimestamp - lastRunTimestamp_\n> > + << \", min duration: \"\n> > + << controllerMinFrameDuration * 1e3;\n>\n> That would result in loooots of debug messages with a > 60fps frame\n> rate. Is it intended ?\n>\n\nNo, I certainly don't want to be too noisy.\nWhat is the overhead of adding a log point like this, but the logging level\nis set to\nnot display the message? I would like to keep some indication of us\nrate-limiting\nthe IPA run if possible, but not at the cost of excess overhead.\n\nRegards,\nNaush\n\n\n>\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 DD278BDCC3\n\tfor ;\n\tTue, 27 Apr 2021 09:39:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28F6D688C5;\n\tTue, 27 Apr 2021 11:39:21 +0200 (CEST)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F2FBE605BD\n\tfor ;\n\tTue, 27 Apr 2021 11:39:19 +0200 (CEST)","by mail-lj1-x22b.google.com with SMTP id a36so56614782ljq.8\n\tfor ;\n\tTue, 27 Apr 2021 02:39:19 -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=\"gcxZRxm8\"; 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=N8+qB7F7tZHL1/rUePVV1I8kY3UxJls7NhjMmXynmjM=;\n\tb=gcxZRxm8b4rexpH3F28RdnCkW3Ub95fBqS9O0B8+IRdyQLAEsifr+rD3GIzhWpYVy5\n\t2tE30VgBkUU6L/yWDh+UyDwtKDmdDB7dvq5O+cjAR80DC96gAkL3KfzJmkR1oYsB9FSf\n\tgVHE5q1aHEVmocmhw9AvZfN+cj76y62KRtTx8WKbhnSfv1BzUGVFZ5y9ROhoVX2+qbMy\n\tSxCM/NtiuUR7Znw4NAahHmGsnN3llXc0I0p++fJFJ60RsTGjGugGtdtfIxIW1zUzDuG1\n\tV913jkO+5Kp2qTXs8+0wteIhQTvkiWmCLWA75nuCQ3Zsi3jE2AE5znKSF9LnoyCROQeO\n\tzEeQ==","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=N8+qB7F7tZHL1/rUePVV1I8kY3UxJls7NhjMmXynmjM=;\n\tb=c/T7QiJ1f6v3pIsNOwc6c8vptA/A36OFdu3cuf0ViS2M8YJCmxy8N7hjJd4fWSGqhI\n\tU+pqONVyex16xadda7yhA67H/u3Kt97OoBiLMads/Srq0SGYhLHCFEo5ckUD/2X6maXt\n\tbhLTfTpSGBoy9iMqY3O07g7+xTw9eTRZMBpYKAlbC60JQSpIUVuOx1VpU7zkcMbu11nC\n\todCUmzO1sTWWbEp13ivNWz9sU3uiUYPs+muMehwJyYTSPpuke6pMk8IoWHBQOBc8ORYn\n\tamZv6Eg9ocSZKCGljROajOlyZ6wKeXFSmwazuILZJBV3v6pYEFZjg/b+/woPxt9jHQRW\n\t1RJQ==","X-Gm-Message-State":"AOAM530HUbxq5hNrLEQ39DZ0l5yCPZvmpj2q9qsbV04K7UDsiIa8z8Mt\n\tu5vmM1+vDMeJpjRqPtjPkkDOWKkPemOWmoWumncp4Q==","X-Google-Smtp-Source":"ABdhPJw0rZ4bGJr8usaqUnK97KZ7vjuVoqWg9IIwmwQIUgCioe5Cz2LkL8b3IubndM4MuV4C/ZyYv/1N4wKaFHKG7mU=","X-Received":"by 2002:a2e:a177:: with SMTP id\n\tu23mr15912689ljl.329.1619516359264; \n\tTue, 27 Apr 2021 02:39:19 -0700 (PDT)","MIME-Version":"1.0","References":"<20210419133451.263733-1-naush@raspberrypi.com>\n\t<20210419133451.263733-6-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 27 Apr 2021 10:39:03 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v5 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=\"===============4219960431276504231==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]