[{"id":16859,"web_url":"https://patchwork.libcamera.org/comment/16859/","msgid":"","date":"2021-05-10T13:35:16","subject":"Re: [libcamera-devel] [PATCH v6 6/6] 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 this patch. For reasons unknown, gmail seems to have tacked\nit on as a reply to the cover note...\n\nOn Mon, 10 May 2021 at 10:59, 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. 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 | 42 +++++++++++++++++--\n> .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++\n> 2 files changed, 43 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 07409621a03a..52d91db282ea 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\nI guess the only thing that caused my brain to pause for a moment was\nthe need to convince myself that processPending_ can't be\nuninitialised. But prepareISP always sets it and must run before this,\nso we're all good...\n\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::SensorTimestamp);\n\nJust wondering if there's any possibility that the first time stamp\ncould be zero, thereby matching the initial value of lastRunTimestamp.\nThe result would be that we start running the control algos a frame or\nso late. If we're happy that this doesn't happen, then I'm OK with it\ntoo.\n\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,24 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &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 < controllerMinFrameDuration * 1e3) {\n\nMight this be easier to read if we defined all these things in\nnanoseconds right from the start? But I really can't get too worked up\nover it!\n\nReviewed-by: David Plowman \n\nBest regards\nDavid\n\n> + /*\n> + * Ensure we merge the previous frame's metadata with the current\n> + * frame. This will not overwrite exposure/gain values for the\n> + * current frame, or any other bits of metadata that were added\n> + * in helper_->Prepare().\n> + */\n> + rpiMetadata_.Merge(lastMetadata);\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 b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index b22564938cdc..d26ae2a9772a 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1426,6 +1426,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::SensorTimestamp, buffer->metadata().timestamp);\n> bayerQueue_.push({ buffer, std::move(ctrl) });\n> } else {\n> embeddedQueue_.push(buffer);\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1F36CBF831\n\tfor ;\n\tMon, 10 May 2021 13:35:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 71C106891A;\n\tMon, 10 May 2021 15:35:28 +0200 (CEST)","from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com\n\t[IPv6:2a00:1450:4864:20::42e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6133602BE\n\tfor ;\n\tMon, 10 May 2021 15:35:27 +0200 (CEST)","by mail-wr1-x42e.google.com with SMTP id d4so16648799wru.7\n\tfor ;\n\tMon, 10 May 2021 06:35:27 -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=\"B+UQj/uu\"; 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=cCOJQRC7XV0Qo2oIQVqsrUkO2sXiOaH4rnDHHMOWAJw=;\n\tb=B+UQj/uujypZ50lQW2nTBFCgYX8oiEl575QrntHsjQi9zTDl4WMw53XzIQFwn5LAFc\n\tfJ990TMF6sVJIEYG7jk+ORXWT34TCNQi1Xfh5+FtTzf3EEiIdFDvSjcSIcJI1diGduix\n\tdeefHmDg49Riwq11N0xcDMzOGvdHvh+3YQdP861b5PIM+kuQI0cBt92IFxm0JmTriAF4\n\tw+cwme0NWKeE17D+fvMYd+nRyMMy2SfLJLyRqIJlkBWd0M/mOd5GrNnFsOU/zISDM+6Y\n\tVSe+faAhFK4iq5vmLfC3JWveMIdzkQFNbq7fJYIq7Obilb1p5kPX3L6QiRzXl+Keq5gj\n\tMI5w==","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=cCOJQRC7XV0Qo2oIQVqsrUkO2sXiOaH4rnDHHMOWAJw=;\n\tb=YfZMu4AywVfZgo8s9s9chBz+mExF4OcHoUxxdSKi4eGwNWtYzcEWxbKG4zV8b1ASYY\n\tAr/x3Z7jdAC6lTRIbiLIJjsEloVxH7lwwOiHiY2IXVzD6ppC1qd7WwRxpaHX9eharyRT\n\teHC8bQdHlib+EUopwgfdICgwzN8PSO7x97ioK9RIv9wgef/gri/MH2pK57ssgTd4UT3V\n\t9Lfse7+5prIsXu+oN7puxxaBCZLrVLLKhnPC/uGbDXJlk5LB9jLMNGo2bG5N6JJJkctC\n\tdECXVQTitwl2YV8kJaKECXchjop/5nTjYYGc8UKHvGFdekwVsOf+gEUpZw9AGRVPNTiR\n\tZ+BA==","X-Gm-Message-State":"AOAM530P9J6sF9Chcmj4ZKYZyucc8nPjMq4SRr3qpe4yIChezlhfoIMF\n\tYHMR0z58ztD2eXDROV2BHbvT1XTTXEccv9obUv7uXw==","X-Google-Smtp-Source":"ABdhPJxev0DWDNFCYrWzm7xZRTLo1o8idtniWB3sW0HzNuamXWDlsGPW7ftjPQaXQQLXR1sCsMk/TOMC2pFunENZN5E=","X-Received":"by 2002:a5d:400f:: with SMTP id\n\tn15mr26814480wrp.274.1620653727308; \n\tMon, 10 May 2021 06:35:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510095814.3732400-1-naush@raspberrypi.com>\n\t<20210510095814.3732400-7-naush@raspberrypi.com>","In-Reply-To":"<20210510095814.3732400-7-naush@raspberrypi.com>","From":"David Plowman ","Date":"Mon, 10 May 2021 14:35:16 +0100","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH v6 6/6] 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":16860,"web_url":"https://patchwork.libcamera.org/comment/16860/","msgid":"","date":"2021-05-10T13:43:42","subject":"Re: [libcamera-devel] [PATCH v6 6/6] 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 Mon, 10 May 2021 at 14:35, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for this patch. For reasons unknown, gmail seems to have tacked\n> it on as a reply to the cover note...\n>\n> On Mon, 10 May 2021 at 10:59, 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. 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 | 42 +++++++++++++++++--\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++\n> > 2 files changed, 43 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 07409621a03a..52d91db282ea 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>\n> I guess the only thing that caused my brain to pause for a moment was\n> the need to convince myself that processPending_ can't be\n> uninitialised. But prepareISP always sets it and must run before this,\n> so we're all good...\n>\n\nYes, this is correct.\n\n\n>\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::SensorTimestamp);\n>\n> Just wondering if there's any possibility that the first time stamp\n> could be zero, thereby matching the initial value of lastRunTimestamp.\n> The result would be that we start running the control algos a frame or\n> so late. If we're happy that this doesn't happen, then I'm OK with it\n> too.\n>\n\nI don't think the first timestamp from the kernel can ever be zero. But I\ndo also test\nthat lastRunTimestamp_ > 0 in the calculation below.\n\n\n>\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,24 @@ 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> Might this be easier to read if we defined all these things in\n> nanoseconds right from the start? But I really can't get too worked up\n> over it!\n>\n\nYes I agree. However, I would prefer to leave this as-is for now, as it\nmatches the other time\nbased constants. One of my next tasks is to switch to using\nstd::chrono::duration for our time base\nvariables, which remove these const factors.\n\nRegards,\nNaush\n\n\n\n>\n> Reviewed-by: David Plowman \n>\n> Best regards\n> David\n>\n> > + /*\n> > + * Ensure we merge the previous frame's metadata with\n> the current\n> > + * frame. This will not overwrite exposure/gain values\n> for the\n> > + * current frame, or any other bits of metadata that\n> were added\n> > + * in helper_->Prepare().\n> > + */\n> > + rpiMetadata_.Merge(lastMetadata);\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 b22564938cdc..d26ae2a9772a 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1426,6 +1426,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::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> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 943A3BF829\n\tfor ;\n\tMon, 10 May 2021 13:44:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F359768911;\n\tMon, 10 May 2021 15:43:59 +0200 (CEST)","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3388A602BE\n\tfor ;\n\tMon, 10 May 2021 15:43:59 +0200 (CEST)","by mail-lf1-x12f.google.com with SMTP id c3so23411276lfs.7\n\tfor ;\n\tMon, 10 May 2021 06:43:59 -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=\"UIW378NU\"; 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=J9qr0/h82ukJYgKWwx9ern6WIKiwgaO3HFPQKxauskc=;\n\tb=UIW378NUUhfpaEUWXpVNCoFbwq3Mc6znAWyCKdkhzmOsuPRyxJ1n4EAZH5COFfsJat\n\tcyEK55UZUf7v+jV2ClC9YgVUvX3H+P72JdBEv0DsjbEi0S0HV3UF08yp6F0J1smcj5LH\n\tpU0whpZmU3a+ptdkBWmEKraEXDdrd0WjnvXlzL3EsG0iZ4JQfiTYcuKG5FmzUO9MjAQ7\n\t6/ZUsItO+ogJ8RhyMmZCKwhJP61dBM9gXtjRVX8NsjUTmkRjhsSuHsMHRNnQ4WflRYV3\n\tltaeVh70ybSwtfVDOOXirbnEQcOUunkoq3CU1B/mc5quPMzqrAkK3011E8NkgE/GjieK\n\tL2gA==","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=J9qr0/h82ukJYgKWwx9ern6WIKiwgaO3HFPQKxauskc=;\n\tb=lRobf2eX0l/k1JVYsaqgdZAXL2dEo2Bhwdel0VN1EtSqCC8EQle2XhgrqDDdnlqXUO\n\tbBfkK2BWG2REQbdvnmxJC/1ISzGA1Y5wDOcMhgo93ado1KP/ZYBpNAa5rgLfScmB7AX3\n\tpFQtWDcAJ9/RnAtx3fMBu/302VUHZsOuY43Po9ZLvwh5hyjdjRjiWKZn502PP0EEnMu2\n\tF1esUps0uXtsTHevpzJEt7/DiID4juO67Yyi+2tOjcODqMcEUB05Ctho5t0VOdaonx/+\n\twJhYNw3jwSPc+fKn/yZZuS6t1LwvgrZLisyeiIbsPIfYl+SAD0we8gXLyGqlCxg2x6cD\n\tyVuQ==","X-Gm-Message-State":"AOAM531/4doR4TV8NmEHZ/SRMzSn/SVEYInEPsGWFkXLPq8Lvsqp+xlI\n\tYxC9BLyXSVo4yiAQa5Y7bF2rOQakYLajxbxW4nyRVQ==","X-Google-Smtp-Source":"ABdhPJzOKl657+2YW8NIMXuQ9DQG81QVqdfGOCTKwlXiC1bp8F/4oToEnWghR3h71J5Gt1t3HQ2c49frVfqHuiynq2M=","X-Received":"by 2002:ac2:544e:: with SMTP id\n\td14mr2750494lfn.567.1620654238290; \n\tMon, 10 May 2021 06:43:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510095814.3732400-1-naush@raspberrypi.com>\n\t<20210510095814.3732400-7-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Mon, 10 May 2021 14:43:42 +0100","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH v6 6/6] 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=\"===============0469307088354442798==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16876,"web_url":"https://patchwork.libcamera.org/comment/16876/","msgid":"","date":"2021-05-10T23:58:08","subject":"Re: [libcamera-devel] [PATCH v6 6/6] 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, May 10, 2021 at 02:43:42PM +0100, Naushir Patuck wrote:\n> On Mon, 10 May 2021 at 14:35, David Plowman wrote:\n> > Hi Naush\n> >\n> > Thanks for this patch. For reasons unknown, gmail seems to have tacked\n> > it on as a reply to the cover note...\n> >\n> > On Mon, 10 May 2021 at 10:59, 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. 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 | 42 +++++++++++++++++--\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++\n> > > 2 files changed, 43 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 07409621a03a..52d91db282ea 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> >\n> > I guess the only thing that caused my brain to pause for a moment was\n> > the need to convince myself that processPending_ can't be\n> > uninitialised. But prepareISP always sets it and must run before this,\n> > so we're all good...\n> \n> Yes, this is correct.\n> \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::SensorTimestamp);\n> >\n> > Just wondering if there's any possibility that the first time stamp\n> > could be zero, thereby matching the initial value of lastRunTimestamp.\n> > The result would be that we start running the control algos a frame or\n> > so late. If we're happy that this doesn't happen, then I'm OK with it\n> > too.\n> \n> I don't think the first timestamp from the kernel can ever be zero. But I\n> do also test\n> that lastRunTimestamp_ > 0 in the calculation below.\n> \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,24 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &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 < controllerMinFrameDuration * 1e3) {\n> >\n> > Might this be easier to read if we defined all these things in\n> > nanoseconds right from the start? But I really can't get too worked up\n> > over it!\n> \n> Yes I agree. However, I would prefer to leave this as-is for now, as it\n> matches the other time\n> based constants. One of my next tasks is to switch to using\n> std::chrono::duration for our time base\n> variables, which remove these const factors.\n\nI'm thinking about doing the same in the libcamera core. And as part of\nthat work, switching everything to nanoseconds may be a code idea, to be\ndone with it once and for all.\n\n> > Reviewed-by: David Plowman \n\nReviewed-by: Laurent Pinchart \n\n> > > + /*\n> > > + * Ensure we merge the previous frame's metadata with the current\n> > > + * frame. This will not overwrite exposure/gain values for the\n> > > + * current frame, or any other bits of metadata that were added\n> > > + * in helper_->Prepare().\n> > > + */\n> > > + rpiMetadata_.Merge(lastMetadata);\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 b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index b22564938cdc..d26ae2a9772a 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1426,6 +1426,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::SensorTimestamp, buffer->metadata().timestamp);\n> > > bayerQueue_.push({ buffer, std::move(ctrl) });\n> > > } else {\n> > > embeddedQueue_.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 110D5BF829\n\tfor ;\n\tMon, 10 May 2021 23:58:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A37868911;\n\tTue, 11 May 2021 01:58:16 +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 F1137602B8\n\tfor ;\n\tTue, 11 May 2021 01:58:14 +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 6678C6EF;\n\tTue, 11 May 2021 01:58:14 +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=\"CDRtaoWW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620691094;\n\tbh=7M19x1+5bQpMYNF+cZYpsvxYFxJc0a86TlssekpIiRI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CDRtaoWWJ0UZzQM2/L7S7SaOkgvnRWJqZe0bMXNKPJ8n3BR2Re9csWu05aE7V5NPY\n\tRaoStx5yQUyq9IA0RiVreuhLqAs2oU1Sm+tTyqI1N/6oHi71SFruqa5/G3K3QCa6IW\n\tg1DIz/jV6KJIlfT/PaFmrk2GNesTLQkL7kAVidb4=","Date":"Tue, 11 May 2021 02:58:08 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210510095814.3732400-1-naush@raspberrypi.com>\n\t<20210510095814.3732400-7-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 6/6] 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\" "}}]