[{"id":16314,"web_url":"https://patchwork.libcamera.org/comment/16314/","msgid":"","date":"2021-04-16T13:44:06","subject":"Re: [libcamera-devel] [PATCH v2 3/3] 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 Naush\n\nThanks for version 2 of this patch, and especially for re-basing on\ntop of my still-pending patch!\n\nWeirdly this one, which I'd have expected to be the \"3/3\" has ended up\nas a reply to the cover note, so not sure what's going on there. But\nanyway...\n\nOn Fri, 16 Apr 2021 at 11:31, Naushir Patuck wrote:\n>\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.\n\nIs it worth a remark on how we don't rate-limit while dropping frames?\nI don't really mind, though.\n\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..e96b169ca612 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\nStrictly speaking, I guess they're still Prepare() and Process()...\n(until such time as the lower-casing elves strike those files!)\n\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> @@ -920,6 +936,30 @@ 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> + * 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. All other metadata values\n> + * must remain the same as the last frame.\n> + */\n> + DeviceStatus currentDeviceStatus;\n> +\n> + rpiMetadata_.Get(\"device.status\", currentDeviceStatus);\n> + rpiMetadata_ = std::move(lastMetadata);\n> + rpiMetadata_.Set(\"device.status\", currentDeviceStatus);\n> + processPending_ = false;\n> + LOG(IPARPI, Debug) << \"Rate-limiting the controller! inter-frame duration: \"\n> + << frameTimestamp - lastRunTimestamp_\n> + << \", min duration \"\n> + << controllerMinFrameDuration * 1e3;\n> + return;\n> + }\n> +\n> + lastRunTimestamp_ = frameTimestamp;\n> + processPending_ = true;\n> +\n> ControlList ctrls(ispCtrls_);\n\nHmm, yes, I see there's an interesting manoeuvre going on with the\nmetadata. I wonder if there's a way to rearrange this that involves a\nbit less shuffling, maybe:\n\nfirst delete these lines:\n+ lastMetadata = std::move(rpiMetadata_);\n fillDeviceStatus(data.controls);\n\nnext, in place of:\n+ DeviceStatus currentDeviceStatus;\n+\n+ rpiMetadata_.Get(\"device.status\",\ncurrentDeviceStatus);\n+ rpiMetadata_ = std::move(lastMetadata);\n+ rpiMetadata_.Set(\"device.status\", currentDeviceStatus);\n\ndo this:\n fillDeviceStatus(data.controls);\n\nand finally after the if-block for the \"skip the algos\" case put these back:\n rpiMetadata_.Clear();\n fillDeviceStatus(data.controls);\n\nI don't know if this ends up tidier or not, perhaps it's best if you\ndecide what you prefer. But I'm fine either way, so:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\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> 2.25.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 EC2D8BD236\n\tfor ;\n\tFri, 16 Apr 2021 13:44:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC6526880C;\n\tFri, 16 Apr 2021 15:44:20 +0200 (CEST)","from mail-oi1-x22b.google.com (mail-oi1-x22b.google.com\n\t[IPv6:2607:f8b0:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 449E368806\n\tfor ;\n\tFri, 16 Apr 2021 15:44:19 +0200 (CEST)","by mail-oi1-x22b.google.com with SMTP id i18so6350902oii.2\n\tfor ;\n\tFri, 16 Apr 2021 06:44: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=\"kuZecQVK\"; 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=kJIGe4NR4gHWgXLxQ4vtC59Fymnd4kapV08pnhUGkYM=;\n\tb=kuZecQVKpNKmz2oX2Vxfmf525IF2M9+Fsco6AKHS/3Bo/YYSJqRWB4zN2/H05yno4q\n\tWG16rtTuIjfX/gl1D3fKhpiGfET5jGa1UetQNuqi3Jh+TcvJy5N0uchJSASFCQs9J0sV\n\tuYbJbF94eScpZG/UhS2Lim/LhsR9ZFDGHdah1NFuPwpG4blBW2NdK2iR/lGfO0JyN+xo\n\tsnn2sZEakOFPpJ+l83IGqnaVk3eKIvi7n4dt7Elct+BSOGIjMyIlFmnYgM75B6lWVJoM\n\tnGmfN8BYe+UA40BHZ+l25wefzkM/SKtgdGRDUvnRwM+FObR7N9s+ScZpfbOpUBwzvKKH\n\tqTSQ==","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=kJIGe4NR4gHWgXLxQ4vtC59Fymnd4kapV08pnhUGkYM=;\n\tb=X9TsvBI6m/Gb3EwYp0JYsYRQw4zByZT1WLSbahjkiyB6KijRlKdU/KmGjg3qJTSRMA\n\tj/cAo/lPYi+d73ojeHOy2iRmzQ+JGAn1KIHbhQ9n1Z34GD3SaB7YbTyqZttIck8E7APJ\n\ttYM/mSD3wBd4+o5W9TJ9vWn3F0nY4hwtTF+IPbVhuIUhFzCfO0zUSRrZdswBaaVHjDYq\n\tFIqq9Qil+IEOiU0GVFTXEMYigS/Mr5LiuDpB5fGLm8mhNdj4rr4/8oh3vhedbvaLMPbf\n\t95u3QqCdaTRi/vgDCXLmsMJh0a6e25kWJjVG9RcX5+a7KWfUMJkLLxl0v73WVILRAFZ0\n\tjuPA==","X-Gm-Message-State":"AOAM533wUJeAv/sl7lvt5pot4yFVqv9KEazP1wWz1SCTVNoLtdbv5BLQ\n\tue04DOkLasERam9YLJoOjyRBold6m9zhoh6nku0mvg==","X-Google-Smtp-Source":"ABdhPJzjvicAU6uv31eZI7F8ZYGsLXepYNctJfBDKthQqhH6RaHiwe76irEbO+Ao+zR1huX9ZHEKhT4GonQMQPEMlYs=","X-Received":"by 2002:aca:c74a:: with SMTP id x71mr6345712oif.22.1618580657971;\n\tFri, 16 Apr 2021 06:44:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20210416103141.1483745-1-naush@raspberrypi.com>\n\t<20210416103141.1483745-4-naush@raspberrypi.com>","In-Reply-To":"<20210416103141.1483745-4-naush@raspberrypi.com>","From":"David Plowman ","Date":"Fri, 16 Apr 2021 14:44:06 +0100","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] 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":16316,"web_url":"https://patchwork.libcamera.org/comment/16316/","msgid":"","date":"2021-04-16T14:36:33","subject":"Re: [libcamera-devel] [PATCH v2 3/3] 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 David,\n\nThank you for your feedback.\n\nOn Fri, 16 Apr 2021 at 14:44, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for version 2 of this patch, and especially for re-basing on\n> top of my still-pending patch!\n>\n> Weirdly this one, which I'd have expected to be the \"3/3\" has ended up\n> as a reply to the cover note, so not sure what's going on there. But\n> anyway...\n>\n\nI think that is gmail causing issues by bunching 0/3 and 3/3 into a\nconversation\nfor some reason, even though the subjects are (slightly) different. Happens\non my view as well!\n\n\n>\n> On Fri, 16 Apr 2021 at 11:31, Naushir Patuck \n> wrote:\n> >\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.\n>\n> Is it worth a remark on how we don't rate-limit while dropping frames?\n> I don't really mind, though.\n>\n\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..e96b169ca612 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>\n> Strictly speaking, I guess they're still Prepare() and Process()...\n> (until such time as the lower-casing elves strike those files!)\n>\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> > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > + frameTimestamp - lastRunTimestamp_ <\n> controllerMinFrameDuration * 1e3) {\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. All other\n> metadata values\n> > + * must remain the same as the last frame.\n> > + */\n> > + DeviceStatus currentDeviceStatus;\n> > +\n> > + rpiMetadata_.Get(\"device.status\",\n> currentDeviceStatus);\n> > + rpiMetadata_ = std::move(lastMetadata);\n> > + rpiMetadata_.Set(\"device.status\", currentDeviceStatus);\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> > + return;\n> > + }\n> > +\n> > + lastRunTimestamp_ = frameTimestamp;\n> > + processPending_ = true;\n> > +\n> > ControlList ctrls(ispCtrls_);\n>\n> Hmm, yes, I see there's an interesting manoeuvre going on with the\n> metadata. I wonder if there's a way to rearrange this that involves a\n> bit less shuffling, maybe:\n>\n> first delete these lines:\n> + lastMetadata = std::move(rpiMetadata_);\n> fillDeviceStatus(data.controls);\n>\n> next, in place of:\n> + DeviceStatus currentDeviceStatus;\n> +\n> + rpiMetadata_.Get(\"device.status\",\n> currentDeviceStatus);\n> + rpiMetadata_ = std::move(lastMetadata);\n> + rpiMetadata_.Set(\"device.status\", currentDeviceStatus);\n>\n> do this:\n> fillDeviceStatus(data.controls);\n>\n> and finally after the if-block for the \"skip the algos\" case put these\n> back:\n> rpiMetadata_.Clear();\n> fillDeviceStatus(data.controls);\n>\n\nI did try this type of logic initially, but I don't think it would work.\n\nWe need to call rpiMetadata_.Clear() and fillDeviceStatus() before\nhelper_->Prepare() is called at the beginning of the function. I cannot\nmove the latter call further down as it needs to run before my skip algos\ncheck and the returnEmbeddedBuffer() call. So I think the existing\nmanoeuvre\nis needed without more refactoring.\n\nRegards,\nNaush\n\n\n>\n> I don't know if this ends up tidier or not, perhaps it's best if you\n> decide what you prefer. But I'm fine either way, so:\n>\n> Reviewed-by: David Plowman \n>\n> Thanks!\n> David\n>\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\n> IPA 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> > 2.25.1\n> >\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 DBEBBBD236\n\tfor ;\n\tFri, 16 Apr 2021 14:36:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53A1B6880A;\n\tFri, 16 Apr 2021 16:36:52 +0200 (CEST)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F3DD68806\n\tfor ;\n\tFri, 16 Apr 2021 16:36:50 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id a1so31335093ljp.2\n\tfor ;\n\tFri, 16 Apr 2021 07:36:50 -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=\"kcAqI3cU\"; 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=9ucsTnFLZoyF0tIgh2hG8cz4INN7AKxK2klVHpGuYik=;\n\tb=kcAqI3cUOEyYLhKjggheiTtziwwnBJOrIfjqXiVeEWtnLV0OHrM1XWhUG0kQfiyao6\n\tIB0naFzWXgv0LqiPo5hEqwnZnpw7Zk409d5poqh2Lk30Z/iGQ2pJ0oWCSU1HysSYeGzf\n\tUFPJJXSwykskSpQwVjLVygxTHU/H64cnkKcUfR7N2bIBe8czJ82M4wRDCYziG0aIteRE\n\tS4qywyB/Tp2Euy4s7D2JrCMskP/UW60ucHuhC1H3fe8/MgZigYMN9JOSiqQuKgkTyrf4\n\totcoDYCc75aOzFWjmPzbYyuf/c4imZ8VVzihk29AMORLWwT33V/qKG86BwAzKcou37jp\n\tj8Hw==","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=9ucsTnFLZoyF0tIgh2hG8cz4INN7AKxK2klVHpGuYik=;\n\tb=YfF64HyaYWQ6YviyBpvuY3UVuzpEw5UovVBhAopYKoXlkvfGirQHEIRl8eGp6DbRBs\n\t06boYi1bRK/sFgOUogxhisUKWEkBAnt2m7TdGonGmKHJBJIncnjmIVdV+haYsCMEvpjx\n\ts4UUk3KWM7rT0hnb8ecUP0EqNdsYCUGMujGD2K8/vRKCFX9ouud4Rk8dlWxa4M39YoKJ\n\tOBsDxKeZYecJqZGVWtz+E6LLxRrYPc0HVbC4cS6/3ZOl3SDsIwSEXDuitS1gk2F/keNw\n\tlRkheU1QHB+vte7sXoZvM7JucjhKK1/IdQ3UolQ1d7brxrJMEF+QLhO41kfiBN6fUbkZ\n\tpBNA==","X-Gm-Message-State":"AOAM531JRi7A6zaQA/LbLOqqiiCoWSQmJiLYX48i31NY3D01HWCdrwma\n\t0qfY04+KSDd7TnQLDleUhVj5DzCYjlQjIwcY7BA5cQ==","X-Google-Smtp-Source":"ABdhPJxgvSBrwzNAYfKxk5TFJhz6Voq+39JY939ARPHl/npuv29Oi46Vckcc5nGK8W+5OTB2VsD0o8+BV4SVr28fo68=","X-Received":"by 2002:a2e:1641:: with SMTP id 1mr2762516ljw.253.1618583809481; \n\tFri, 16 Apr 2021 07:36:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20210416103141.1483745-1-naush@raspberrypi.com>\n\t<20210416103141.1483745-4-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 16 Apr 2021 15:36:33 +0100","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] 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=\"===============0279394494211126599==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16319,"web_url":"https://patchwork.libcamera.org/comment/16319/","msgid":"","date":"2021-04-16T15:59:48","subject":"Re: [libcamera-devel] [PATCH v2 3/3] 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 Naush\n\nThanks for explaining.\n\nOn Fri, 16 Apr 2021 at 15:36, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thank you for your feedback.\n>\n> On Fri, 16 Apr 2021 at 14:44, David Plowman wrote:\n>>\n>> Hi Naush\n>>\n>> Thanks for version 2 of this patch, and especially for re-basing on\n>> top of my still-pending patch!\n>>\n>> Weirdly this one, which I'd have expected to be the \"3/3\" has ended up\n>> as a reply to the cover note, so not sure what's going on there. But\n>> anyway...\n>\n>\n> I think that is gmail causing issues by bunching 0/3 and 3/3 into a conversation\n> for some reason, even though the subjects are (slightly) different. Happens\n> on my view as well!\n>\n>>\n>>\n>> On Fri, 16 Apr 2021 at 11:31, Naushir Patuck wrote:\n>> >\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.\n>>\n>> Is it worth a remark on how we don't rate-limit while dropping frames?\n>> I don't really mind, though.\n>>\n>>\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..e96b169ca612 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>>\n>> Strictly speaking, I guess they're still Prepare() and Process()...\n>> (until such time as the lower-casing elves strike those files!)\n>>\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>> > @@ -920,6 +936,30 @@ 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>> > + * 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. All other metadata values\n>> > + * must remain the same as the last frame.\n>> > + */\n>> > + DeviceStatus currentDeviceStatus;\n>> > +\n>> > + rpiMetadata_.Get(\"device.status\", currentDeviceStatus);\n>> > + rpiMetadata_ = std::move(lastMetadata);\n>> > + rpiMetadata_.Set(\"device.status\", currentDeviceStatus);\n>> > + processPending_ = false;\n>> > + LOG(IPARPI, Debug) << \"Rate-limiting the controller! inter-frame duration: \"\n>> > + << frameTimestamp - lastRunTimestamp_\n>> > + << \", min duration \"\n>> > + << controllerMinFrameDuration * 1e3;\n>> > + return;\n>> > + }\n>> > +\n>> > + lastRunTimestamp_ = frameTimestamp;\n>> > + processPending_ = true;\n>> > +\n>> > ControlList ctrls(ispCtrls_);\n>>\n>> Hmm, yes, I see there's an interesting manoeuvre going on with the\n>> metadata. I wonder if there's a way to rearrange this that involves a\n>> bit less shuffling, maybe:\n>>\n>> first delete these lines:\n>> + lastMetadata = std::move(rpiMetadata_);\n>> fillDeviceStatus(data.controls);\n>>\n>> next, in place of:\n>> + DeviceStatus currentDeviceStatus;\n>> +\n>> + rpiMetadata_.Get(\"device.status\",\n>> currentDeviceStatus);\n>> + rpiMetadata_ = std::move(lastMetadata);\n>> + rpiMetadata_.Set(\"device.status\", currentDeviceStatus);\n>>\n>> do this:\n>> fillDeviceStatus(data.controls);\n>>\n>> and finally after the if-block for the \"skip the algos\" case put these back:\n>> rpiMetadata_.Clear();\n>> fillDeviceStatus(data.controls);\n>\n>\n> I did try this type of logic initially, but I don't think it would work.\n>\n> We need to call rpiMetadata_.Clear() and fillDeviceStatus() before\n> helper_->Prepare() is called at the beginning of the function. I cannot\n> move the latter call further down as it needs to run before my skip algos\n> check and the returnEmbeddedBuffer() call. So I think the existing manoeuvre\n> is needed without more refactoring.\n\nAh yes, indeed, you're right - that helper_->Prepare() is troublesome.\nBack to Plan A then...\n\nActually one more little annoyance comes to mind. What happens if\nhelper_->Prepare() gives us more metadata than just the DeviceStatus.\nWould we expect that to get copied over the previous metadata too? In\npractice I would guess it won't matter if that metadata can't escape\nto the application, though I think we can already imagine future\nsensors where that might happen. What do you think? Would a \"merge\"\nmethod (which \"moves\" the data items) be the fix for this? Am I\nover-thinking?\n\nBest regards\nDavid\n\n>\n> Regards,\n> Naush\n>\n>>\n>>\n>> I don't know if this ends up tidier or not, perhaps it's best if you\n>> decide what you prefer. But I'm fine either way, so:\n>>\n>> Reviewed-by: David Plowman \n>>\n>> Thanks!\n>> David\n>>\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>> > 2.25.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 1C455BD235\n\tfor ;\n\tFri, 16 Apr 2021 16:00:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CE646880A;\n\tFri, 16 Apr 2021 18:00:02 +0200 (CEST)","from mail-oi1-x234.google.com (mail-oi1-x234.google.com\n\t[IPv6:2607:f8b0:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B56D168806\n\tfor ;\n\tFri, 16 Apr 2021 18:00:00 +0200 (CEST)","by mail-oi1-x234.google.com with SMTP id i81so28313825oif.6\n\tfor ;\n\tFri, 16 Apr 2021 09:00:00 -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=\"EH8yCyvk\"; 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=m2iSOsFq4NuMgwwwFo1JeopCjFKHbZ56YmUXJnOrT40=;\n\tb=EH8yCyvkxpNrMkGyDQEwhCJ/y83lP6FP0uOqB8CfjfQ2uF04RE0ICzMALkoUBPL05Q\n\tjebUpeSut+DCmsofDX0S4zlQpQt+1Y+L10vNd8sQ0PfQZWpdDGutlCnC89jY8DYsDFyn\n\tbmRck7fK9InD00EyJorVyf0tnspS2XzS/xj9IPcnq3nHBaQHBGbsgMnhBmMIBSoLSxkS\n\t9qBTd333JpZKLtlodkG4h1A3+xiVufEKzZh1Gr+IgJq887yrsvQES4CChbgKsbjemNQL\n\tlfilUetsKgY1Mbe50d0dMrYN7KR9h2FB+ZbA77vJHahHm583L5eKmV6FsccAIrsvOJX1\n\tlFpw==","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=m2iSOsFq4NuMgwwwFo1JeopCjFKHbZ56YmUXJnOrT40=;\n\tb=bI4kVZ13TBj2sdrWJ4Gr3vDBmYrYfRcv15bQQtuDZlnI1s4az9xFT8VTnem/YAgva/\n\tAbJR8lRbYI9BbWppwo2+8hDJVAoOPjhOSv9sHk3cwMX+7j+6m5Kmz9qlzVoqR8iiZ2ue\n\t+NjZmwfNp+F/bwSMjNTTd42PcixQHuZTvtvW2rHlBD1LDRpdGd0gYeKrKcu1t8ecfA6A\n\tNs1PNWB8QGyEKgnWjzUmPZKS6PSa8ww6HCZuyv4tPUZGDC2XDklr9nXDrJ9xG17TubbS\n\ta1ARIJGbDgIfQDYhVL6V7K9C3mzIKKlKngSI4lD8CpJ4k7XiDSVX2fwQWLS1Yn/Twzvn\n\tpgRg==","X-Gm-Message-State":"AOAM530Cy6zsUmzS6CgHd9Jk2QciljbNbcmLm3WEfAQR4/rwmLnA1itE\n\tPa0sbgE54PfHlWre6+VoCfk8yPqHkpNooQXTtpxtKA==","X-Google-Smtp-Source":"ABdhPJwPjSssz5boZBRuHus3Ps1vBGrB2Wsc3iftqFq9oSQAVV5s7LHaxEjIAAbECUvt2KEmzE1jSTFa2A4QV3oUK08=","X-Received":"by 2002:aca:4d0e:: with SMTP id\n\ta14mr6872072oib.107.1618588799582; \n\tFri, 16 Apr 2021 08:59:59 -0700 (PDT)","MIME-Version":"1.0","References":"<20210416103141.1483745-1-naush@raspberrypi.com>\n\t<20210416103141.1483745-4-naush@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Fri, 16 Apr 2021 16:59:48 +0100","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] 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":16320,"web_url":"https://patchwork.libcamera.org/comment/16320/","msgid":"","date":"2021-04-16T16:30:19","subject":"Re: [libcamera-devel] [PATCH v2 3/3] 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 David,\n\nOn Fri, 16 Apr 2021, 5:00 pm David Plowman, \nwrote:\n\n> Hi Naush\n>\n> Thanks for explaining.\n>\n> On Fri, 16 Apr 2021 at 15:36, Naushir Patuck \n> wrote:\n> >\n> > Hi David,\n> >\n> > Thank you for your feedback.\n> >\n> > On Fri, 16 Apr 2021 at 14:44, David Plowman <\n> david.plowman@raspberrypi.com> wrote:\n> >>\n> >> Hi Naush\n> >>\n> >> Thanks for version 2 of this patch, and especially for re-basing on\n> >> top of my still-pending patch!\n> >>\n> >> Weirdly this one, which I'd have expected to be the \"3/3\" has ended up\n> >> as a reply to the cover note, so not sure what's going on there. But\n> >> anyway...\n> >\n> >\n> > I think that is gmail causing issues by bunching 0/3 and 3/3 into a\n> conversation\n> > for some reason, even though the subjects are (slightly) different.\n> Happens\n> > on my view as well!\n> >\n> >>\n> >>\n> >> On Fri, 16 Apr 2021 at 11:31, Naushir Patuck \n> wrote:\n> >> >\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\n> 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.\n> >>\n> >> Is it worth a remark on how we don't rate-limit while dropping frames?\n> >> I don't really mind, though.\n> >>\n> >>\n> >> >\n> >> > Signed-off-by: Naushir Patuck \n> >> > ---\n> >> > src/ipa/raspberrypi/raspberrypi.cpp | 48\n> +++++++++++++++++--\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..e96b169ca612 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\n> run the\n> >> > + * controller algorithms. If the pipeline handler provider frames at\n> a rate\n> >> > + * higher than this, we rate-limit the controller prepare() and\n> process() calls\n> >>\n> >> Strictly speaking, I guess they're still Prepare() and Process()...\n> >> (until such time as the lower-casing elves strike those files!)\n> >>\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\n> ipa::RPi::ISPConfig &data)\n> >> > if (data.embeddedBufferPresent)\n> >> > returnEmbeddedBuffer(data.embeddedBufferId);\n> >> >\n> >> > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> >> > + frameTimestamp - lastRunTimestamp_ <\n> controllerMinFrameDuration * 1e3) {\n> >> > + /*\n> >> > + * Ensure we update the controller metadata with the\n> new frame's\n> >> > + * exposure/gain values so that the correct values\n> are returned\n> >> > + * out in libcamera metadata later on. All other\n> metadata values\n> >> > + * must remain the same as the last frame.\n> >> > + */\n> >> > + DeviceStatus currentDeviceStatus;\n> >> > +\n> >> > + rpiMetadata_.Get(\"device.status\",\n> currentDeviceStatus);\n> >> > + rpiMetadata_ = std::move(lastMetadata);\n> >> > + rpiMetadata_.Set(\"device.status\",\n> currentDeviceStatus);\n> >> > + processPending_ = false;\n> >> > + LOG(IPARPI, Debug) << \"Rate-limiting the controller!\n> inter-frame duration: \"\n> >> > + << frameTimestamp -\n> lastRunTimestamp_\n> >> > + << \", min duration \"\n> >> > + << controllerMinFrameDuration *\n> 1e3;\n> >> > + return;\n> >> > + }\n> >> > +\n> >> > + lastRunTimestamp_ = frameTimestamp;\n> >> > + processPending_ = true;\n> >> > +\n> >> > ControlList ctrls(ispCtrls_);\n> >>\n> >> Hmm, yes, I see there's an interesting manoeuvre going on with the\n> >> metadata. I wonder if there's a way to rearrange this that involves a\n> >> bit less shuffling, maybe:\n> >>\n> >> first delete these lines:\n> >> + lastMetadata = std::move(rpiMetadata_);\n> >> fillDeviceStatus(data.controls);\n> >>\n> >> next, in place of:\n> >> + DeviceStatus currentDeviceStatus;\n> >> +\n> >> + rpiMetadata_.Get(\"device.status\",\n> >> currentDeviceStatus);\n> >> + rpiMetadata_ = std::move(lastMetadata);\n> >> + rpiMetadata_.Set(\"device.status\", currentDeviceStatus);\n> >>\n> >> do this:\n> >> fillDeviceStatus(data.controls);\n> >>\n> >> and finally after the if-block for the \"skip the algos\" case put these\n> back:\n> >> rpiMetadata_.Clear();\n> >> fillDeviceStatus(data.controls);\n> >\n> >\n> > I did try this type of logic initially, but I don't think it would work.\n> >\n> > We need to call rpiMetadata_.Clear() and fillDeviceStatus() before\n> > helper_->Prepare() is called at the beginning of the function. I cannot\n> > move the latter call further down as it needs to run before my skip algos\n> > check and the returnEmbeddedBuffer() call. So I think the existing\n> manoeuvre\n> > is needed without more refactoring.\n>\n> Ah yes, indeed, you're right - that helper_->Prepare() is troublesome.\n> Back to Plan A then...\n>\n> Actually one more little annoyance comes to mind. What happens if\n> helper_->Prepare() gives us more metadata than just the DeviceStatus.\n> Would we expect that to get copied over the previous metadata too? In\n> practice I would guess it won't matter if that metadata can't escape\n> to the application, though I think we can already imagine future\n> sensors where that might happen. What do you think? Would a \"merge\"\n> method (which \"moves\" the data items) be the fix for this? Am I\n> over-thinking?\n>\n\nI think this is one to worry about in the future.\nA merge method would be useful in these situations, but given that we only\nreally care about exposure and gain right now, propose we kick the can down\nthe road.\n\nRegards,\nNaush\n\n\n> Best regards\n> David\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >>\n> >>\n> >> I don't know if this ends up tidier or not, perhaps it's best if you\n> >> decide what you prefer. But I'm fine either way, so:\n> >>\n> >> Reviewed-by: David Plowman \n> >>\n> >> Thanks!\n> >> David\n> >>\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\n> IPA 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> >> > 2.25.1\n> >> >\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 9A747BD236\n\tfor ;\n\tFri, 16 Apr 2021 16:30:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F3C5568812;\n\tFri, 16 Apr 2021 18:30:32 +0200 (CEST)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7EAB68806\n\tfor ;\n\tFri, 16 Apr 2021 18:30:31 +0200 (CEST)","by mail-lj1-x230.google.com with SMTP id m7so20908516ljp.10\n\tfor ;\n\tFri, 16 Apr 2021 09:30:31 -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=\"p0heD6qD\"; 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=4qsAVE8r3wG05lf6umMgfPp8ruTRW9IyVCbcV6Oos1A=;\n\tb=p0heD6qDvuDC6nHcS/s/Rq7jSSxU2RxYmkEToKY2isyGbF9yJqfI2jKypR2Q03h3w+\n\tduBL0Z/CgIRk6xixJuQmT+EIbcrvkX/JVJCqLZVhJ53LVPlkAGzWbmJ8NU0UEdfc4P3o\n\tXJTr5qBiARBJsdEh6nY2RDO+TCsExKuZl3OtcOoAXlRpRocwO9nIhl1/kDPLhlzjwzbx\n\tMlLEfQ0E0WEEWYI0dDlePztBpz35bDVScPAziWwJf7EtvE/VLWhanCayStJ9ajuhdj54\n\tGTiXAmuvLldip9WxzLNjY3X9Xi87hFTrr+9IYDIM601cE1dXdiL5/BpkEQ1rVqkJmA6s\n\tdDkA==","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=4qsAVE8r3wG05lf6umMgfPp8ruTRW9IyVCbcV6Oos1A=;\n\tb=BIRzfdgkEpME76LJdvrvIHWZdUS9kgQTLwSOl65E08bpzx0MW5yqj2pguJkDrC9cuj\n\tRDcvUm//A7b/Lhc2lvEncGA5vFpXE/wzijd2anTCkoI5OC+Gja9CjDrPAS+HzkihTRDI\n\tLoGJkgz7spaaovBmIRwdrWMdKN/N/ZXAAXS87uL86WUyTt6FYIWwoHDOC4q52QdZUUa3\n\tl567e2s7Ui4WjVVh66uIHlRkKAD2QWc/QY4zjFkg1OefNHaLx51qb2w4V9qQLzomt8aa\n\tH+lJiu5k0xBgw6N/m+qJvrr3ARp5MpC5HRtGrysLuNFQ60Otolu1CtFDCoH52NaC3i6m\n\tbJ7g==","X-Gm-Message-State":"AOAM531NAPl9fGdiVNVO7KiRUwZPDSPgy40U4p5zUxPIjPrJMLBbag41\n\t2NbW2XXWn2WTGhstMaa5NiASDzzULMzUJf8yMFnGBQ==","X-Google-Smtp-Source":"ABdhPJzoTAdFmE9N6BVn8ZUhu/ouIQOKSPOm1pg4CTwmiXz5YhB00/rIqwXBnN2EAEL57JzsNwTlAeTARqYbkli3JfI=","X-Received":"by 2002:a2e:1641:: with SMTP id 1mr3064761ljw.253.1618590631280; \n\tFri, 16 Apr 2021 09:30:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20210416103141.1483745-1-naush@raspberrypi.com>\n\t<20210416103141.1483745-4-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 16 Apr 2021 17:30:19 +0100","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] 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=\"===============4825559579400374542==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]