[{"id":26731,"web_url":"https://patchwork.libcamera.org/comment/26731/","msgid":"<20230324142332.ud5roktlajo6arqs@uno.localdomain>","date":"2023-03-24T14:23:32","subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate\n\tSensorLimits","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via libcamera-devel wrote:\n> Populate all the fields of the SensorLimits structure in configure().\n> This allows us to use the cached values instead of re-computing them\n> on every frame.\n>\n> For the gain -> code convertion, ensure we clamp to the analogue gain\n> limits set in the SensorLimits structure.\n\nI wonder if this can't be done by expanding your CameraMode structure.\nCameraMode already combines information from the SensorInfo and could\nvery well include information from sensorControls.\n\nThis would avoid introducing another type to move it around across\ndifferent layers of your IPA.\n\nHave you considered that and decided you prefer introducing\nSensorLimits ?\n\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 67 ++++++++++++++++-------------\n> 1 file changed, 38 insertions(+), 29 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index c3b2c375036f..e8d8023bcfe7 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -224,8 +224,8 @@ private:\n> \tDuration minFrameDuration_;\n> \tDuration maxFrameDuration_;\n>\n> -\t/* Maximum gain code for the sensor. */\n> -\tuint32_t maxSensorGainCode_;\n> +\t/* Mode specific sensor gain/exposure limits. */\n> +\tRPiController::AgcAlgorithm::SensorLimits sensorLimits_;\n>\n> \t/* Track the frame length times over FrameLengthsQueueSize frames. */\n> \tstd::deque frameLengths_;\n> @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> \t\t}\n> \t}\n>\n> -\tmaxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get();\n> -\n> \t/* Setup a metadata ControlList to output metadata. */\n> \tlibcameraMetadata_ = ControlList(controls::controls);\n>\n> @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> \t/* Pass the camera mode to the CamHelper to setup algorithms. */\n> \thelper_->setCameraMode(mode_);\n>\n> +\t/*\n> +\t * Store the sensor gain and shutter limits for the mode.\n> +\t *\n> +\t * The maximum shutter value are calculated from the frame duration limit\n> +\t * as V4L2 will restrict the maximum control value based on the current\n> +\t * VBLANK value.\n> +\t *\n> +\t * These limits get set in the AGC algorithm through applyFrameDurations()\n> +\t * below.\n> +\t */\n> +\tconst ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN);\n> +\tconst ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE);\n> +\n> +\tsensorLimits_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> +\tsensorLimits_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n> +\tsensorLimits_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n> +\tsensorLimits_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> +\tsensorLimits_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> +\tsensorLimits_.maxShutter = Duration::max();\n> +\thelper_->getBlanking(sensorLimits_.maxShutter,\n> +\t\t\t sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n> +\n> \t/*\n> \t * Initialise this ControlList correctly, even if empty, in case the IPA is\n> \t * running is isolation mode (passing the ControlList through the IPC layer).\n> @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> \t * based on the current sensor mode.\n> \t */\n> \tControlInfoMap::Map ctrlMap = ipaControls;\n> -\tconst Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n> -\tconst Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> \tctrlMap[&controls::FrameDurationLimits] =\n> -\t\tControlInfo(static_cast(minSensorFrameDuration.get()),\n> -\t\t\t static_cast(maxSensorFrameDuration.get()));\n> +\t\tControlInfo(static_cast(sensorLimits_.minFrameDuration.get()),\n> +\t\t\t static_cast(sensorLimits_.maxFrameDuration.get()));\n>\n> \tctrlMap[&controls::AnalogueGain] =\n> -\t\tControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)));\n> -\n> -\t/*\n> -\t * Calculate the max exposure limit from the frame duration limit as V4L2\n> -\t * will limit the maximum control value based on the current VBLANK value.\n> -\t */\n> -\tDuration maxShutter = Duration::max();\n> -\thelper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration);\n> -\tconst uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get();\n> +\t\tControlInfo(static_cast(sensorLimits_.minAnalogueGain),\n> +\t\t\t static_cast(sensorLimits_.maxAnalogueGain));\n>\n> \tctrlMap[&controls::ExposureTime] =\n> -\t\tControlInfo(static_cast(helper_->exposure(exposureMin, mode_.minLineLength).get()),\n> -\t\t\t static_cast(maxShutter.get()));\n> +\t\tControlInfo(static_cast(sensorLimits_.minShutter.get()),\n> +\t\t\t static_cast(sensorLimits_.maxShutter.get()));\n>\n> \t/* Declare Autofocus controls, only if we have a controllable lens */\n> \tif (lensPresent_)\n> @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n>\n> void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)\n> {\n> -\tconst Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n> -\tconst Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> -\n> \t/*\n> \t * This will only be applied once AGC recalculations occur.\n> \t * The values may be clamped based on the sensor mode capabilities as well.\n> @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur\n> \tminFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;\n> \tmaxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;\n> \tminFrameDuration_ = std::clamp(minFrameDuration_,\n> -\t\t\t\t minSensorFrameDuration, maxSensorFrameDuration);\n> +\t\t\t\t sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n> \tmaxFrameDuration_ = std::clamp(maxFrameDuration_,\n> -\t\t\t\t minSensorFrameDuration, maxSensorFrameDuration);\n> +\t\t\t\t sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n> \tmaxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);\n>\n> \t/* Return the validated limits via metadata. */\n> @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur\n> \t * getBlanking() will update maxShutter with the largest exposure\n> \t * value possible.\n> \t */\n> -\tRPiController::AgcAlgorithm::SensorLimits limits;\n> -\tlimits.maxShutter = Duration::max();\n> -\thelper_->getBlanking(limits.maxShutter, minFrameDuration_, maxFrameDuration_);\n> +\tsensorLimits_.maxShutter = Duration::max();\n> +\thelper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_, maxFrameDuration_);\n>\n> \tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\tcontroller_.getAlgorithm(\"agc\"));\n> -\tagc->setSensorLimits(limits);\n> +\tagc->setSensorLimits(sensorLimits_);\n> }\n>\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> {\n> +\tconst int32_t minGainCode = helper_->gainCode(sensorLimits_.minAnalogueGain);\n> +\tconst int32_t maxGainCode = helper_->gainCode(sensorLimits_.maxAnalogueGain);\n> \tint32_t gainCode = helper_->gainCode(agcStatus->analogueGain);\n>\n> \t/*\n> @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> \t * DelayedControls. The AGC will correctly handle a lower gain returned\n> \t * by the sensor, provided it knows the actual gain used.\n> \t */\n> -\tgainCode = std::min(gainCode, maxSensorGainCode_);\n> +\tgainCode = std::clamp(gainCode, minGainCode, maxGainCode);\n>\n> \t/* getBlanking might clip exposure time to the fps limits. */\n> \tDuration exposure = agcStatus->shutterTime;\n> --\n> 2.34.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 9A081C0F1B\n\tfor ;\n\tFri, 24 Mar 2023 14:23:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D2EB8626E6;\n\tFri, 24 Mar 2023 15:23:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4066A61ECF\n\tfor ;\n\tFri, 24 Mar 2023 15:23:39 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4EE66A58;\n\tFri, 24 Mar 2023 15:23:38 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679667820;\n\tbh=uDTxDJkfJmGkHj6CkG569qjDj49zNoym+XHq+Sz5rjc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=JefCVLFZufWU5t0eYaQ27n96Fqid37gaC+IUCo7KQ/cWVuRXP4l6eks37cnpndcJc\n\t0RVqb1rxYcUye0obwLwx3SAx7UGaiafRG7X5FcvVU25WOoYM3wPaywI4vw1GekqNBe\n\tK5fkqMfKUAaesaoyzMj0PdeST+c41vGlao8FbL2OXSXmy89bS9ngz6HA7qyqVCXkiB\n\tkmFyUthlJRzjuI2h0UwqWZjFWT7F+j6AHktpNo0f7NHqdmw9yar+OJi/kUIlN+NtXp\n\tjC/pLuY4iTCFLHjh1OwuBdxpEErY1YZqNDdKPA74zfARsL/pU+BphhGAFC38aJ+ehr\n\tcumSjkux4LxTg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679667818;\n\tbh=uDTxDJkfJmGkHj6CkG569qjDj49zNoym+XHq+Sz5rjc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bNNt+SW2d1m3X/Ay4MJBcBldwma75tn9upoRlYOQPtM6G7YzXavGFkwyrKvjNO0qc\n\tiIlGLqaQRs22M+9w7BN072H4eDOkzWgtsaWKIh4Qpc+1L69lP8SNpe3iw9lR+eyjcr\n\tb+uwsq9n6jEzqdQu56vDSWCFVSE2g+bGIi5wud0U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bNNt+SW2\"; dkim-atps=neutral","Date":"Fri, 24 Mar 2023 15:23:32 +0100","To":"Naushir Patuck ","Message-ID":"<20230324142332.ud5roktlajo6arqs@uno.localdomain>","References":"<20230322161317.18055-1-naush@raspberrypi.com>\n\t<20230322161317.18055-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230322161317.18055-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate\n\tSensorLimits","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26733,"web_url":"https://patchwork.libcamera.org/comment/26733/","msgid":"","date":"2023-03-24T14:54:21","subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate\n\tSensorLimits","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nThank you for the feedback.\n\n\nOn Fri, 24 Mar 2023 at 14:23, Jacopo Mondi \nwrote:\n\n> Hi Naush\n>\n> On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > Populate all the fields of the SensorLimits structure in configure().\n> > This allows us to use the cached values instead of re-computing them\n> > on every frame.\n> >\n> > For the gain -> code convertion, ensure we clamp to the analogue gain\n> > limits set in the SensorLimits structure.\n>\n> I wonder if this can't be done by expanding your CameraMode structure.\n> CameraMode already combines information from the SensorInfo and could\n> very well include information from sensorControls.\n>\n> This would avoid introducing another type to move it around across\n> different layers of your IPA.\n>\n> Have you considered that and decided you prefer introducing\n> SensorLimits ?\n>\n\nThat's a great idea! It definitely fits well in the CameraMode structure.\nIf there are no objection from David, I'll rework the series to move these\nfields into CameraMode.\n\nRegards,\nNaush\n\n\n>\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 67 ++++++++++++++++-------------\n> > 1 file changed, 38 insertions(+), 29 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index c3b2c375036f..e8d8023bcfe7 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -224,8 +224,8 @@ private:\n> > Duration minFrameDuration_;\n> > Duration maxFrameDuration_;\n> >\n> > - /* Maximum gain code for the sensor. */\n> > - uint32_t maxSensorGainCode_;\n> > + /* Mode specific sensor gain/exposure limits. */\n> > + RPiController::AgcAlgorithm::SensorLimits sensorLimits_;\n> >\n> > /* Track the frame length times over FrameLengthsQueueSize frames.\n> */\n> > std::deque frameLengths_;\n> > @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo, const IPAConfig &ip\n> > }\n> > }\n> >\n> > - maxSensorGainCode_ =\n> sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get();\n> > -\n> > /* Setup a metadata ControlList to output metadata. */\n> > libcameraMetadata_ = ControlList(controls::controls);\n> >\n> > @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo, const IPAConfig &ip\n> > /* Pass the camera mode to the CamHelper to setup algorithms. */\n> > helper_->setCameraMode(mode_);\n> >\n> > + /*\n> > + * Store the sensor gain and shutter limits for the mode.\n> > + *\n> > + * The maximum shutter value are calculated from the frame\n> duration limit\n> > + * as V4L2 will restrict the maximum control value based on the\n> current\n> > + * VBLANK value.\n> > + *\n> > + * These limits get set in the AGC algorithm through\n> applyFrameDurations()\n> > + * below.\n> > + */\n> > + const ControlInfo &gainCtrl =\n> sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN);\n> > + const ControlInfo &shutterCtrl =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE);\n> > +\n> > + sensorLimits_.minAnalogueGain =\n> helper_->gain(gainCtrl.min().get());\n> > + sensorLimits_.maxAnalogueGain =\n> helper_->gain(gainCtrl.max().get());\n> > + sensorLimits_.minFrameDuration = mode_.minFrameLength *\n> mode_.minLineLength;\n> > + sensorLimits_.maxFrameDuration = mode_.maxFrameLength *\n> mode_.maxLineLength;\n> > + sensorLimits_.minShutter =\n> helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > + sensorLimits_.maxShutter = Duration::max();\n> > + helper_->getBlanking(sensorLimits_.maxShutter,\n> > + sensorLimits_.minFrameDuration,\n> sensorLimits_.maxFrameDuration);\n> > +\n> > /*\n> > * Initialise this ControlList correctly, even if empty, in case\n> the IPA is\n> > * running is isolation mode (passing the ControlList through the\n> IPC layer).\n> > @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo, const IPAConfig &ip\n> > * based on the current sensor mode.\n> > */\n> > ControlInfoMap::Map ctrlMap = ipaControls;\n> > - const Duration minSensorFrameDuration = mode_.minFrameLength *\n> mode_.minLineLength;\n> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength *\n> mode_.maxLineLength;\n> > ctrlMap[&controls::FrameDurationLimits] =\n> > -\n> ControlInfo(static_cast(minSensorFrameDuration.get()),\n> > -\n> static_cast(maxSensorFrameDuration.get()));\n> > +\n> ControlInfo(static_cast(sensorLimits_.minFrameDuration.get()),\n> > +\n> static_cast(sensorLimits_.maxFrameDuration.get()));\n> >\n> > ctrlMap[&controls::AnalogueGain] =\n> > - ControlInfo(1.0f,\n> static_cast(helper_->gain(maxSensorGainCode_)));\n> > -\n> > - /*\n> > - * Calculate the max exposure limit from the frame duration limit\n> as V4L2\n> > - * will limit the maximum control value based on the current\n> VBLANK value.\n> > - */\n> > - Duration maxShutter = Duration::max();\n> > - helper_->getBlanking(maxShutter, minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > - const uint32_t exposureMin =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get();\n> > +\n> ControlInfo(static_cast(sensorLimits_.minAnalogueGain),\n> > +\n> static_cast(sensorLimits_.maxAnalogueGain));\n> >\n> > ctrlMap[&controls::ExposureTime] =\n> > -\n> ControlInfo(static_cast(helper_->exposure(exposureMin,\n> mode_.minLineLength).get()),\n> > -\n> static_cast(maxShutter.get()));\n> > +\n> ControlInfo(static_cast(sensorLimits_.minShutter.get()),\n> > +\n> static_cast(sensorLimits_.maxShutter.get()));\n> >\n> > /* Declare Autofocus controls, only if we have a controllable lens\n> */\n> > if (lensPresent_)\n> > @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration\n> maxFrameDuration)\n> > {\n> > - const Duration minSensorFrameDuration = mode_.minFrameLength *\n> mode_.minLineLength;\n> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength *\n> mode_.maxLineLength;\n> > -\n> > /*\n> > * This will only be applied once AGC recalculations occur.\n> > * The values may be clamped based on the sensor mode capabilities\n> as well.\n> > @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration\n> minFrameDuration, Duration maxFrameDur\n> > minFrameDuration_ = minFrameDuration ? minFrameDuration :\n> defaultMaxFrameDuration;\n> > maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :\n> defaultMinFrameDuration;\n> > minFrameDuration_ = std::clamp(minFrameDuration_,\n> > - minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > + sensorLimits_.minFrameDuration,\n> sensorLimits_.maxFrameDuration);\n> > maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > - minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > + sensorLimits_.minFrameDuration,\n> sensorLimits_.maxFrameDuration);\n> > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);\n> >\n> > /* Return the validated limits via metadata. */\n> > @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration\n> minFrameDuration, Duration maxFrameDur\n> > * getBlanking() will update maxShutter with the largest exposure\n> > * value possible.\n> > */\n> > - RPiController::AgcAlgorithm::SensorLimits limits;\n> > - limits.maxShutter = Duration::max();\n> > - helper_->getBlanking(limits.maxShutter, minFrameDuration_,\n> maxFrameDuration_);\n> > + sensorLimits_.maxShutter = Duration::max();\n> > + helper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_,\n> maxFrameDuration_);\n> >\n> > RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> > controller_.getAlgorithm(\"agc\"));\n> > - agc->setSensorLimits(limits);\n> > + agc->setSensorLimits(sensorLimits_);\n> > }\n> >\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> > {\n> > + const int32_t minGainCode =\n> helper_->gainCode(sensorLimits_.minAnalogueGain);\n> > + const int32_t maxGainCode =\n> helper_->gainCode(sensorLimits_.maxAnalogueGain);\n> > int32_t gainCode = helper_->gainCode(agcStatus->analogueGain);\n> >\n> > /*\n> > @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> > * DelayedControls. The AGC will correctly handle a lower gain\n> returned\n> > * by the sensor, provided it knows the actual gain used.\n> > */\n> > - gainCode = std::min(gainCode, maxSensorGainCode_);\n> > + gainCode = std::clamp(gainCode, minGainCode, maxGainCode);\n> >\n> > /* getBlanking might clip exposure time to the fps limits. */\n> > Duration exposure = agcStatus->shutterTime;\n> > --\n> > 2.34.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 0BC6CC0F1B\n\tfor ;\n\tFri, 24 Mar 2023 14:54:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C93F6270E;\n\tFri, 24 Mar 2023 15:54:37 +0100 (CET)","from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com\n\t[IPv6:2607:f8b0:4864:20::b36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8586D603AA\n\tfor ;\n\tFri, 24 Mar 2023 15:54:35 +0100 (CET)","by mail-yb1-xb36.google.com with SMTP id p203so2434086ybb.13\n\tfor ;\n\tFri, 24 Mar 2023 07:54:35 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679669677;\n\tbh=UahexMwj1m3XXown2EdlOeEqZyvIf4rk6JP6HXJP7As=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ad1H9OquIIEYlmDxqNmWDb7faQ6QGifvJsjuhm4En81ENvwm7PdnimWLZSMJgcZnJ\n\tFzdkiMo95wLe/YT8fg66v1dJawAvzspTz+rzZc9kgzEn6PCtxc5bJKw5ByZFMHKTc6\n\tvZ+O1Wlgqe9degDVymaZIwtCC6KYaJSweSOUW3GzaHlUOjm+CPtTbOLpvfLNcXHrw4\n\tsbyuWpqjN05SfzG9gx12T1j+PfzUolo1rp2h4KgNQnWDoMFQIKGVftFMr992gozWkJ\n\taybjnzqXTUDqZz3euOpaJ5q2WSfRSpcZOdFRhyMqZI/B/rhrK8gs9eVn3MeXnyx0f+\n\t8BCJnAnEZwpkA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1679669674;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=YP+SvPsQwhyY1N6aL8wb9fceZqf8Hb0OqjLJPS4YPDI=;\n\tb=T4B7PpYnCorTjTaq52M77pXm07KZCh2D5qTGxWbAjCodHKowEwKNOUWX19it1eY09v\n\twkOHjKozISyilSwOj0xmPsbGhODP9TyEph881oE5JDn1LEsQEwHKQKqAgxM71fkP5lqn\n\tgoSai/JFQzraSsZDWC6e69dsAFHXhiq8Oglisi33/Fx5TTeDIn5n7bxCr8lMc03hlbn1\n\t0tyorEqihMzOT0P7cA0R/bXC+IafUHd0HcMYxOebbzB/5Qc9RYtMvC+byj0cPN8hEPqb\n\tC2cVgE5xNpEawqH5jk5s+imQ4GPyJWm4HvryDevF571MAWEN9P8xMU8LWa86fGxGwu/T\n\tSx7A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"T4B7PpYn\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679669674;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=YP+SvPsQwhyY1N6aL8wb9fceZqf8Hb0OqjLJPS4YPDI=;\n\tb=R8E989HCbXObXBWifg5BE+EWvC+IMIcSVd10Ycd0zPHghvNiy4oBmUYhXRpGKnLA7N\n\tcW7LfYBNjh4yjvTlBJEVjPx2Bk6ew244/NGPG2KlMNmkpK0aJEvJrCYrQlbIq57546yQ\n\t71cUapKA77+mu8Ruv4OZ23V4ydPLZv8XVHhwadEsfikrtkk6y0cP0MuZjxSay1ps07Zf\n\tvXAMA85OJrlX5z7buP+jF84qUDpjY9dujw8XcBTHOmptlxBp48/nBvbWo5refmVpOBe7\n\tzmuZqjv79mDIUcerDmlZ4VliNsVQg+69aZt0mrHyH3VwAxutzIq8yjoKSsb9seKhpg+I\n\tsrOQ==","X-Gm-Message-State":"AAQBX9fRYjW28SXAUmzE9FbasUUJW9csWuEcImnFuDRCZBwEMoxGJ5RQ\n\tylOXP9nq5Fp2hF29NAj+Jp51U7E2VRRs36IsKWNYpgkC021W4yZm/0EIsQ==","X-Google-Smtp-Source":"AKy350Z5EXsZ0LKPdJ9pOecE/0LT1ah0LxVhfDYDiCtuBTcLrKABd1ckzS2E4LFZ+ooz/ph/UBuTf1S3DlgxXjJdHtM=","X-Received":"by 2002:a25:ce94:0:b0:b77:780f:5888 with SMTP id\n\tx142-20020a25ce94000000b00b77780f5888mr1425188ybe.9.1679669674343;\n\tFri, 24 Mar 2023 07:54:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20230322161317.18055-1-naush@raspberrypi.com>\n\t<20230322161317.18055-3-naush@raspberrypi.com>\n\t<20230324142332.ud5roktlajo6arqs@uno.localdomain>","In-Reply-To":"<20230324142332.ud5roktlajo6arqs@uno.localdomain>","Date":"Fri, 24 Mar 2023 14:54:21 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"00000000000047934805f7a692dd\"","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate\n\tSensorLimits","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26743,"web_url":"https://patchwork.libcamera.org/comment/26743/","msgid":"","date":"2023-03-24T18:11:06","subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate\n\tSensorLimits","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush, Jacopo\n\nJust wanted to check whether there's no issue with these things\nchanging dynamically (by which I mean not a mode-switch time). For\nexample if someone sets a new framerate while things are running, or\nsomething.\n\nThanks!\nDavid\n\nOn Fri, 24 Mar 2023 at 14:54, Naushir Patuck via libcamera-devel\n wrote:\n>\n> Hi Jacopo,\n>\n> Thank you for the feedback.\n>\n>\n> On Fri, 24 Mar 2023 at 14:23, Jacopo Mondi wrote:\n>>\n>> Hi Naush\n>>\n>> On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via libcamera-devel wrote:\n>> > Populate all the fields of the SensorLimits structure in configure().\n>> > This allows us to use the cached values instead of re-computing them\n>> > on every frame.\n>> >\n>> > For the gain -> code convertion, ensure we clamp to the analogue gain\n>> > limits set in the SensorLimits structure.\n>>\n>> I wonder if this can't be done by expanding your CameraMode structure.\n>> CameraMode already combines information from the SensorInfo and could\n>> very well include information from sensorControls.\n>>\n>> This would avoid introducing another type to move it around across\n>> different layers of your IPA.\n>>\n>> Have you considered that and decided you prefer introducing\n>> SensorLimits ?\n>\n>\n> That's a great idea! It definitely fits well in the CameraMode structure.\n> If there are no objection from David, I'll rework the series to move these\n> fields into CameraMode.\n>\n> Regards,\n> Naush\n>\n>>\n>>\n>> >\n>> > Signed-off-by: Naushir Patuck \n>> > Reviewed-by: David Plowman \n>> > ---\n>> > src/ipa/raspberrypi/raspberrypi.cpp | 67 ++++++++++++++++-------------\n>> > 1 file changed, 38 insertions(+), 29 deletions(-)\n>> >\n>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > index c3b2c375036f..e8d8023bcfe7 100644\n>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > @@ -224,8 +224,8 @@ private:\n>> > Duration minFrameDuration_;\n>> > Duration maxFrameDuration_;\n>> >\n>> > - /* Maximum gain code for the sensor. */\n>> > - uint32_t maxSensorGainCode_;\n>> > + /* Mode specific sensor gain/exposure limits. */\n>> > + RPiController::AgcAlgorithm::SensorLimits sensorLimits_;\n>> >\n>> > /* Track the frame length times over FrameLengthsQueueSize frames. */\n>> > std::deque frameLengths_;\n>> > @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n>> > }\n>> > }\n>> >\n>> > - maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get();\n>> > -\n>> > /* Setup a metadata ControlList to output metadata. */\n>> > libcameraMetadata_ = ControlList(controls::controls);\n>> >\n>> > @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n>> > /* Pass the camera mode to the CamHelper to setup algorithms. */\n>> > helper_->setCameraMode(mode_);\n>> >\n>> > + /*\n>> > + * Store the sensor gain and shutter limits for the mode.\n>> > + *\n>> > + * The maximum shutter value are calculated from the frame duration limit\n>> > + * as V4L2 will restrict the maximum control value based on the current\n>> > + * VBLANK value.\n>> > + *\n>> > + * These limits get set in the AGC algorithm through applyFrameDurations()\n>> > + * below.\n>> > + */\n>> > + const ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN);\n>> > + const ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE);\n>> > +\n>> > + sensorLimits_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n>> > + sensorLimits_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n>> > + sensorLimits_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n>> > + sensorLimits_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n>> > + sensorLimits_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n>> > + sensorLimits_.maxShutter = Duration::max();\n>> > + helper_->getBlanking(sensorLimits_.maxShutter,\n>> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n>> > +\n>> > /*\n>> > * Initialise this ControlList correctly, even if empty, in case the IPA is\n>> > * running is isolation mode (passing the ControlList through the IPC layer).\n>> > @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n>> > * based on the current sensor mode.\n>> > */\n>> > ControlInfoMap::Map ctrlMap = ipaControls;\n>> > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n>> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n>> > ctrlMap[&controls::FrameDurationLimits] =\n>> > - ControlInfo(static_cast(minSensorFrameDuration.get()),\n>> > - static_cast(maxSensorFrameDuration.get()));\n>> > + ControlInfo(static_cast(sensorLimits_.minFrameDuration.get()),\n>> > + static_cast(sensorLimits_.maxFrameDuration.get()));\n>> >\n>> > ctrlMap[&controls::AnalogueGain] =\n>> > - ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)));\n>> > -\n>> > - /*\n>> > - * Calculate the max exposure limit from the frame duration limit as V4L2\n>> > - * will limit the maximum control value based on the current VBLANK value.\n>> > - */\n>> > - Duration maxShutter = Duration::max();\n>> > - helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration);\n>> > - const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get();\n>> > + ControlInfo(static_cast(sensorLimits_.minAnalogueGain),\n>> > + static_cast(sensorLimits_.maxAnalogueGain));\n>> >\n>> > ctrlMap[&controls::ExposureTime] =\n>> > - ControlInfo(static_cast(helper_->exposure(exposureMin, mode_.minLineLength).get()),\n>> > - static_cast(maxShutter.get()));\n>> > + ControlInfo(static_cast(sensorLimits_.minShutter.get()),\n>> > + static_cast(sensorLimits_.maxShutter.get()));\n>> >\n>> > /* Declare Autofocus controls, only if we have a controllable lens */\n>> > if (lensPresent_)\n>> > @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n>> >\n>> > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)\n>> > {\n>> > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n>> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n>> > -\n>> > /*\n>> > * This will only be applied once AGC recalculations occur.\n>> > * The values may be clamped based on the sensor mode capabilities as well.\n>> > @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur\n>> > minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;\n>> > maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;\n>> > minFrameDuration_ = std::clamp(minFrameDuration_,\n>> > - minSensorFrameDuration, maxSensorFrameDuration);\n>> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n>> > maxFrameDuration_ = std::clamp(maxFrameDuration_,\n>> > - minSensorFrameDuration, maxSensorFrameDuration);\n>> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n>> > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);\n>> >\n>> > /* Return the validated limits via metadata. */\n>> > @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur\n>> > * getBlanking() will update maxShutter with the largest exposure\n>> > * value possible.\n>> > */\n>> > - RPiController::AgcAlgorithm::SensorLimits limits;\n>> > - limits.maxShutter = Duration::max();\n>> > - helper_->getBlanking(limits.maxShutter, minFrameDuration_, maxFrameDuration_);\n>> > + sensorLimits_.maxShutter = Duration::max();\n>> > + helper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_, maxFrameDuration_);\n>> >\n>> > RPiController::AgcAlgorithm *agc = dynamic_cast(\n>> > controller_.getAlgorithm(\"agc\"));\n>> > - agc->setSensorLimits(limits);\n>> > + agc->setSensorLimits(sensorLimits_);\n>> > }\n>> >\n>> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n>> > {\n>> > + const int32_t minGainCode = helper_->gainCode(sensorLimits_.minAnalogueGain);\n>> > + const int32_t maxGainCode = helper_->gainCode(sensorLimits_.maxAnalogueGain);\n>> > int32_t gainCode = helper_->gainCode(agcStatus->analogueGain);\n>> >\n>> > /*\n>> > @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n>> > * DelayedControls. The AGC will correctly handle a lower gain returned\n>> > * by the sensor, provided it knows the actual gain used.\n>> > */\n>> > - gainCode = std::min(gainCode, maxSensorGainCode_);\n>> > + gainCode = std::clamp(gainCode, minGainCode, maxGainCode);\n>> >\n>> > /* getBlanking might clip exposure time to the fps limits. */\n>> > Duration exposure = agcStatus->shutterTime;\n>> > --\n>> > 2.34.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 C8AD0C0F1B\n\tfor ;\n\tFri, 24 Mar 2023 18:11:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 240A262718;\n\tFri, 24 Mar 2023 19:11:20 +0100 (CET)","from mail-oa1-x33.google.com (mail-oa1-x33.google.com\n\t[IPv6:2001:4860:4864:20::33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB087603AA\n\tfor ;\n\tFri, 24 Mar 2023 19:11:18 +0100 (CET)","by mail-oa1-x33.google.com with SMTP id\n\t586e51a60fabf-17786581fe1so2536145fac.10\n\tfor ;\n\tFri, 24 Mar 2023 11:11:18 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679681480;\n\tbh=L1LH2et0M9K2Hi4Q3JWyjBxVMwczUaBkMA1R4yFYBFI=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=a2lkzr+ZSaa4HxEKS2imeYeF4TNWDog8SEHoWKxMeB5oGWIyguZOvtvhuN1JAsWof\n\tmFP0JZotuIBaAIVbvqbwkq9E9Veqa68jIrKIjTgSj7yrsDJv6A+qonJbXfRmxLvhvZ\n\tc/L1kAw1ioa7MAJIZ4+yF+xIkMZXzfa9mc68XmjqoKk4XAjkabKmu6ZgXyUXDG0GL1\n\tAlbR8kK0mIPnj+ntRt4RP9JqTurjPaOehNbA1IVie7a8m2ad6yA56EVmkep/cZKxZ3\n\tBtwdJOevorqqcg2v75w6qnhbxjAFpe+tDWBqDErximrTehIF+pi9BsPR9KaGRUS/N4\n\t0719pIVk4Y0tA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1679681477;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=VR6KtZQpn8B5Fk+oDNVz2Pxt8YC4NQ8ibdk6f9fyFGg=;\n\tb=An8BCUXuEGjI47LgpdqI90YRRDjs5dYjNG6RfbBSjk0ZhF3nrQZ0GuShaV8M3/PFDp\n\tNXtkUOyKZSytzTUGt+Y1MRy92Gjv3gRtDyZmdmICVgvx79juxM6W3Bex4ZITIkBm2317\n\t9MXDoIHQoCqikjBW/r7L+NpY3DoEKOx3lbvVHVtdnz6iHlIiQPD8he10LqhDLiiJ9VMu\n\th2ct5TbKl0fSnXqCnLFC+klyTxDnNb9opUr7e98BKy5mtqpg3J9sF63AxigDOryFnXZQ\n\t5Dv1W1sVYkiBKMgUGwUYG8Ab89iH1ewX3KNQFwaAeUa9KFZLRnzktAMaFAp6NyFVUkhn\n\tQkrA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"An8BCUXu\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679681477;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=VR6KtZQpn8B5Fk+oDNVz2Pxt8YC4NQ8ibdk6f9fyFGg=;\n\tb=FtSfmiApb1ZbwXrrrLYXjkpjBVdnfZr6gAjw3FtPG3hTaLVUSFscEYJSkYoXmCAeYu\n\tzz/V57WDSNN8a1tnJr1olvC1oa+uFERqTj66zVdlMLy2GDEQ/WL/2IrypDAxdwfvnJka\n\tCDfP63cxCQ0A3Q8QwKFc5d/WHqT+481doqdV5ZKiZDej3o9gZ0ZNY1dX30oODbMKAzhp\n\tpl2HTTFyJzeW/eMCgIO2NpBWjcipskyIWo8dBd9hmrRBeFt9aXD2aWCkukhcE1VelUQq\n\tL6k36HO23WDy9ZInO5HPOB5o+nCzV9FCjPSv9pWGhMIC8d1LRt1RlYDANeeWaqnb5qff\n\tseEQ==","X-Gm-Message-State":"AO0yUKWMMZm6up6ikQTkmqKaS0DOJHPeTCnloyacrxGqskOwKNtmHfw3\n\tOf9XzMnCnmr+OsLxoebebVEVhL0PI8mezW83b+6ezg==","X-Google-Smtp-Source":"AK7set9508ieV0u3U9u2C4qa+N31j91qvyGt9lFYb7RcPytRFIXovK0fF8jkpzpf3HTt9RUNlgLFAL+jmjRYrvnhow4=","X-Received":"by 2002:a05:6870:df8d:b0:17a:a5a2:62a6 with SMTP id\n\tus13-20020a056870df8d00b0017aa5a262a6mr1292249oab.11.1679681477282;\n\tFri, 24 Mar 2023 11:11:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20230322161317.18055-1-naush@raspberrypi.com>\n\t<20230322161317.18055-3-naush@raspberrypi.com>\n\t<20230324142332.ud5roktlajo6arqs@uno.localdomain>\n\t","In-Reply-To":"","Date":"Fri, 24 Mar 2023 18:11:06 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate\n\tSensorLimits","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26746,"web_url":"https://patchwork.libcamera.org/comment/26746/","msgid":"<20230325091940.cep4nov3xjy3eveh@uno.localdomain>","date":"2023-03-25T09:19:40","subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate\n\tSensorLimits","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Fri, Mar 24, 2023 at 06:11:06PM +0000, David Plowman via libcamera-devel wrote:\n> Hi Naush, Jacopo\n>\n> Just wanted to check whether there's no issue with these things\n> changing dynamically (by which I mean not a mode-switch time). For\n> example if someone sets a new framerate while things are running, or\n> something.\n\nDo I parse your questions right as:\n\nThe 'sensor limits' passed to the AGC does not only change at configure()\ntime, but also when a new FrameDurationLimits is set by an application.\nParticularly, setting a new FrameDurationLimits limits the maximum\nshutter time the AGC algorithm can use.\n\nAs I read it, maxShutter is the only parameter that changes\ndynamically, all the rest of the structure remains un-changed between\nconfigurations. That's why it feels a bit of a duplication of\nCameraMode to me. This highlights that the maxShutter limit calculation\ndoesn't probably belong to the main IPA module but is rather part of\nthe AGC. The only other place where you use it is the ControlInfo\nlimits initialization, again at configure() time.\n\nWe handle cases like this in other IPA by having each algorithm implement\nqueueRequest() and handling the controls they're interested in. In\nthis way the state of an algorithm is as self-contained as possible,\nand the main reason I'm not thrilled by this patch is that is\nintroduces yet-another state representation shared between multiple\nlayers. I understand that to do the shutter calculation in the algorithm\nyou would need to have access to the helper there, something you\ncurrently have not. Is this by design ?\n\nBy the way your IPA is kind of \"your territory\" as you do most of the\nmaintainance there, so if this feels right/better to you, I won't\ncertainly oppose\n>\n> Thanks!\n> David\n>\n> On Fri, 24 Mar 2023 at 14:54, Naushir Patuck via libcamera-devel\n> wrote:\n> >\n> > Hi Jacopo,\n> >\n> > Thank you for the feedback.\n> >\n> >\n> > On Fri, 24 Mar 2023 at 14:23, Jacopo Mondi wrote:\n> >>\n> >> Hi Naush\n> >>\n> >> On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via libcamera-devel wrote:\n> >> > Populate all the fields of the SensorLimits structure in configure().\n> >> > This allows us to use the cached values instead of re-computing them\n> >> > on every frame.\n> >> >\n> >> > For the gain -> code convertion, ensure we clamp to the analogue gain\n> >> > limits set in the SensorLimits structure.\n> >>\n> >> I wonder if this can't be done by expanding your CameraMode structure.\n> >> CameraMode already combines information from the SensorInfo and could\n> >> very well include information from sensorControls.\n> >>\n> >> This would avoid introducing another type to move it around across\n> >> different layers of your IPA.\n> >>\n> >> Have you considered that and decided you prefer introducing\n> >> SensorLimits ?\n> >\n> >\n> > That's a great idea! It definitely fits well in the CameraMode structure.\n> > If there are no objection from David, I'll rework the series to move these\n> > fields into CameraMode.\n> >\n> > Regards,\n> > Naush\n> >\n> >>\n> >>\n> >> >\n> >> > Signed-off-by: Naushir Patuck \n> >> > Reviewed-by: David Plowman \n> >> > ---\n> >> > src/ipa/raspberrypi/raspberrypi.cpp | 67 ++++++++++++++++-------------\n> >> > 1 file changed, 38 insertions(+), 29 deletions(-)\n> >> >\n> >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > index c3b2c375036f..e8d8023bcfe7 100644\n> >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > @@ -224,8 +224,8 @@ private:\n> >> > Duration minFrameDuration_;\n> >> > Duration maxFrameDuration_;\n> >> >\n> >> > - /* Maximum gain code for the sensor. */\n> >> > - uint32_t maxSensorGainCode_;\n> >> > + /* Mode specific sensor gain/exposure limits. */\n> >> > + RPiController::AgcAlgorithm::SensorLimits sensorLimits_;\n> >> >\n> >> > /* Track the frame length times over FrameLengthsQueueSize frames. */\n> >> > std::deque frameLengths_;\n> >> > @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> >> > }\n> >> > }\n> >> >\n> >> > - maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get();\n> >> > -\n> >> > /* Setup a metadata ControlList to output metadata. */\n> >> > libcameraMetadata_ = ControlList(controls::controls);\n> >> >\n> >> > @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> >> > /* Pass the camera mode to the CamHelper to setup algorithms. */\n> >> > helper_->setCameraMode(mode_);\n> >> >\n> >> > + /*\n> >> > + * Store the sensor gain and shutter limits for the mode.\n> >> > + *\n> >> > + * The maximum shutter value are calculated from the frame duration limit\n> >> > + * as V4L2 will restrict the maximum control value based on the current\n> >> > + * VBLANK value.\n> >> > + *\n> >> > + * These limits get set in the AGC algorithm through applyFrameDurations()\n> >> > + * below.\n> >> > + */\n> >> > + const ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN);\n> >> > + const ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE);\n> >> > +\n> >> > + sensorLimits_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> >> > + sensorLimits_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n> >> > + sensorLimits_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n> >> > + sensorLimits_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> >> > + sensorLimits_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> >> > + sensorLimits_.maxShutter = Duration::max();\n> >> > + helper_->getBlanking(sensorLimits_.maxShutter,\n> >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n> >> > +\n> >> > /*\n> >> > * Initialise this ControlList correctly, even if empty, in case the IPA is\n> >> > * running is isolation mode (passing the ControlList through the IPC layer).\n> >> > @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> >> > * based on the current sensor mode.\n> >> > */\n> >> > ControlInfoMap::Map ctrlMap = ipaControls;\n> >> > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n> >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> >> > ctrlMap[&controls::FrameDurationLimits] =\n> >> > - ControlInfo(static_cast(minSensorFrameDuration.get()),\n> >> > - static_cast(maxSensorFrameDuration.get()));\n> >> > + ControlInfo(static_cast(sensorLimits_.minFrameDuration.get()),\n> >> > + static_cast(sensorLimits_.maxFrameDuration.get()));\n> >> >\n> >> > ctrlMap[&controls::AnalogueGain] =\n> >> > - ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)));\n> >> > -\n> >> > - /*\n> >> > - * Calculate the max exposure limit from the frame duration limit as V4L2\n> >> > - * will limit the maximum control value based on the current VBLANK value.\n> >> > - */\n> >> > - Duration maxShutter = Duration::max();\n> >> > - helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration);\n> >> > - const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get();\n> >> > + ControlInfo(static_cast(sensorLimits_.minAnalogueGain),\n> >> > + static_cast(sensorLimits_.maxAnalogueGain));\n> >> >\n> >> > ctrlMap[&controls::ExposureTime] =\n> >> > - ControlInfo(static_cast(helper_->exposure(exposureMin, mode_.minLineLength).get()),\n> >> > - static_cast(maxShutter.get()));\n> >> > + ControlInfo(static_cast(sensorLimits_.minShutter.get()),\n> >> > + static_cast(sensorLimits_.maxShutter.get()));\n> >> >\n> >> > /* Declare Autofocus controls, only if we have a controllable lens */\n> >> > if (lensPresent_)\n> >> > @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> >> >\n> >> > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)\n> >> > {\n> >> > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n> >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> >> > -\n> >> > /*\n> >> > * This will only be applied once AGC recalculations occur.\n> >> > * The values may be clamped based on the sensor mode capabilities as well.\n> >> > @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur\n> >> > minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;\n> >> > maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;\n> >> > minFrameDuration_ = std::clamp(minFrameDuration_,\n> >> > - minSensorFrameDuration, maxSensorFrameDuration);\n> >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n> >> > maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> >> > - minSensorFrameDuration, maxSensorFrameDuration);\n> >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n> >> > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);\n> >> >\n> >> > /* Return the validated limits via metadata. */\n> >> > @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur\n> >> > * getBlanking() will update maxShutter with the largest exposure\n> >> > * value possible.\n> >> > */\n> >> > - RPiController::AgcAlgorithm::SensorLimits limits;\n> >> > - limits.maxShutter = Duration::max();\n> >> > - helper_->getBlanking(limits.maxShutter, minFrameDuration_, maxFrameDuration_);\n> >> > + sensorLimits_.maxShutter = Duration::max();\n> >> > + helper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_, maxFrameDuration_);\n> >> >\n> >> > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> >> > controller_.getAlgorithm(\"agc\"));\n> >> > - agc->setSensorLimits(limits);\n> >> > + agc->setSensorLimits(sensorLimits_);\n> >> > }\n> >> >\n> >> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> >> > {\n> >> > + const int32_t minGainCode = helper_->gainCode(sensorLimits_.minAnalogueGain);\n> >> > + const int32_t maxGainCode = helper_->gainCode(sensorLimits_.maxAnalogueGain);\n> >> > int32_t gainCode = helper_->gainCode(agcStatus->analogueGain);\n> >> >\n> >> > /*\n> >> > @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> >> > * DelayedControls. The AGC will correctly handle a lower gain returned\n> >> > * by the sensor, provided it knows the actual gain used.\n> >> > */\n> >> > - gainCode = std::min(gainCode, maxSensorGainCode_);\n> >> > + gainCode = std::clamp(gainCode, minGainCode, maxGainCode);\n> >> >\n> >> > /* getBlanking might clip exposure time to the fps limits. */\n> >> > Duration exposure = agcStatus->shutterTime;\n> >> > --\n> >> > 2.34.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 D873DC0F2A\n\tfor ;\n\tSat, 25 Mar 2023 09:19:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3AB062719;\n\tSat, 25 Mar 2023 10:19:45 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 175CB626DA\n\tfor ;\n\tSat, 25 Mar 2023 10:19:44 +0100 (CET)","from ideasonboard.com (host-213-45-201-116.retail.telecomitalia.it\n\t[213.45.201.116])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2D9469A8;\n\tSat, 25 Mar 2023 10:19:43 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679735986;\n\tbh=8IIN0mkXXaFDzmXNmBLAlVQhSbtoIVBTrrbijQx+RVk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=z+jkCHJdffmAstZXbjWKTFLYN+014fWAbrc07Rauxnv9ZtoVSEFfyAD1sGCFlnrEA\n\tDDQuhHVrbfEue+0KF+JvxR/Glefx5QKeBsewb6MKsSfhWbUWDMAvng7Gtwvm0MTR4M\n\t+/syHQzL0WqJIC1SahqcXnPQLlmqHF/UMlJaNmPBU4CfA75O5nA8WNQ84zuaqyUi99\n\tAWPEKnjJYGnYELP7BwQ09aT3UVDHgY7j3FjRecM8fdgPUoF+K64oRAc8jdsQVTcU50\n\tdqegdlaNZsuul0hmnczYc0M4xzghVmloP+0YEHABmCpojWjI8b2/MmZuqegmmMNP2j\n\t32TKts+A104og==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679735983;\n\tbh=8IIN0mkXXaFDzmXNmBLAlVQhSbtoIVBTrrbijQx+RVk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nRmgOj/nYbzrsR/Tn5mmkCp/xcMGocptSdLmtf4dge6h/unw9gEcIaLe3fo3IkKue\n\tESFtJM/EIQmHIWTESe+ZR63OdaSLjnuQRRGYGcnt5hFothXvCCR9YNfzeIIzv5hK95\n\tSmoof5PYCe7SsFlRjt4iq17yAD9t/dAV3hjXkUuk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nRmgOj/n\"; dkim-atps=neutral","Date":"Sat, 25 Mar 2023 10:19:40 +0100","To":"David Plowman ","Message-ID":"<20230325091940.cep4nov3xjy3eveh@uno.localdomain>","References":"<20230322161317.18055-1-naush@raspberrypi.com>\n\t<20230322161317.18055-3-naush@raspberrypi.com>\n\t<20230324142332.ud5roktlajo6arqs@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate\n\tSensorLimits","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26751,"web_url":"https://patchwork.libcamera.org/comment/26751/","msgid":"","date":"2023-03-27T07:57:27","subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate\n\tSensorLimits","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo and David,\n\nOn Sat, 25 Mar 2023 at 09:19, Jacopo Mondi \nwrote:\n\n> Hi David\n>\n> On Fri, Mar 24, 2023 at 06:11:06PM +0000, David Plowman via\n> libcamera-devel wrote:\n> > Hi Naush, Jacopo\n> >\n> > Just wanted to check whether there's no issue with these things\n> > changing dynamically (by which I mean not a mode-switch time). For\n> > example if someone sets a new framerate while things are running, or\n> > something.\n>\n> Do I parse your questions right as:\n>\n> The 'sensor limits' passed to the AGC does not only change at configure()\n> time, but also when a new FrameDurationLimits is set by an application.\n> Particularly, setting a new FrameDurationLimits limits the maximum\n> shutter time the AGC algorithm can use.\n>\n> As I read it, maxShutter is the only parameter that changes\n> dynamically, all the rest of the structure remains un-changed between\n> configurations. That's why it feels a bit of a duplication of\n> CameraMode to me. This highlights that the maxShutter limit calculation\n> doesn't probably belong to the main IPA module but is rather part of\n> the AGC. The only other place where you use it is the ControlInfo\n> limits initialization, again at configure() time.\n>\n\nThis is correct. At runtime, only the max shutter value will be updated.\nAs\nsuch, I see no reason not to move the SensorLimits fields into the\nCameraMode\nstructure.\n\n\n>\n> We handle cases like this in other IPA by having each algorithm implement\n> queueRequest() and handling the controls they're interested in. In\n> this way the state of an algorithm is as self-contained as possible,\n> and the main reason I'm not thrilled by this patch is that is\n> introduces yet-another state representation shared between multiple\n> layers. I understand that to do the shutter calculation in the algorithm\n> you would need to have access to the helper there, something you\n> currently have not. Is this by design ?\n>\n\nThis is somewhat by design. We try and keep our algorithm code as clean as\npossible, i.e. mostly independent of libcamera dependencies like\nControlLists as\nwell as sensor dependencies like CamHelpers. The IPA code as a bridge\nbetween\nthe algorithms and the rest of the world.\n\n\n>\n> By the way your IPA is kind of \"your territory\" as you do most of the\n> maintainance there, so if this feels right/better to you, I won't\n> certainly oppose\n>\n\nI'll explore the suggestion to move the fields to CameraMode, I think it's\nprobably worth it for convenience.\n\nRegards,\nNaush\n\n\n> >\n> > Thanks!\n> > David\n> >\n> > On Fri, 24 Mar 2023 at 14:54, Naushir Patuck via libcamera-devel\n> > wrote:\n> > >\n> > > Hi Jacopo,\n> > >\n> > > Thank you for the feedback.\n> > >\n> > >\n> > > On Fri, 24 Mar 2023 at 14:23, Jacopo Mondi <\n> jacopo.mondi@ideasonboard.com> wrote:\n> > >>\n> > >> Hi Naush\n> > >>\n> > >> On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > >> > Populate all the fields of the SensorLimits structure in\n> configure().\n> > >> > This allows us to use the cached values instead of re-computing them\n> > >> > on every frame.\n> > >> >\n> > >> > For the gain -> code convertion, ensure we clamp to the analogue\n> gain\n> > >> > limits set in the SensorLimits structure.\n> > >>\n> > >> I wonder if this can't be done by expanding your CameraMode structure.\n> > >> CameraMode already combines information from the SensorInfo and could\n> > >> very well include information from sensorControls.\n> > >>\n> > >> This would avoid introducing another type to move it around across\n> > >> different layers of your IPA.\n> > >>\n> > >> Have you considered that and decided you prefer introducing\n> > >> SensorLimits ?\n> > >\n> > >\n> > > That's a great idea! It definitely fits well in the CameraMode\n> structure.\n> > > If there are no objection from David, I'll rework the series to move\n> these\n> > > fields into CameraMode.\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > >>\n> > >>\n> > >> >\n> > >> > Signed-off-by: Naushir Patuck \n> > >> > Reviewed-by: David Plowman \n> > >> > ---\n> > >> > src/ipa/raspberrypi/raspberrypi.cpp | 67\n> ++++++++++++++++-------------\n> > >> > 1 file changed, 38 insertions(+), 29 deletions(-)\n> > >> >\n> > >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > index c3b2c375036f..e8d8023bcfe7 100644\n> > >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > @@ -224,8 +224,8 @@ private:\n> > >> > Duration minFrameDuration_;\n> > >> > Duration maxFrameDuration_;\n> > >> >\n> > >> > - /* Maximum gain code for the sensor. */\n> > >> > - uint32_t maxSensorGainCode_;\n> > >> > + /* Mode specific sensor gain/exposure limits. */\n> > >> > + RPiController::AgcAlgorithm::SensorLimits sensorLimits_;\n> > >> >\n> > >> > /* Track the frame length times over FrameLengthsQueueSize\n> frames. */\n> > >> > std::deque frameLengths_;\n> > >> > @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo, const IPAConfig &ip\n> > >> > }\n> > >> > }\n> > >> >\n> > >> > - maxSensorGainCode_ =\n> sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get();\n> > >> > -\n> > >> > /* Setup a metadata ControlList to output metadata. */\n> > >> > libcameraMetadata_ = ControlList(controls::controls);\n> > >> >\n> > >> > @@ -473,6 +471,28 @@ int IPARPi::configure(const\n> IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > >> > /* Pass the camera mode to the CamHelper to setup algorithms.\n> */\n> > >> > helper_->setCameraMode(mode_);\n> > >> >\n> > >> > + /*\n> > >> > + * Store the sensor gain and shutter limits for the mode.\n> > >> > + *\n> > >> > + * The maximum shutter value are calculated from the frame\n> duration limit\n> > >> > + * as V4L2 will restrict the maximum control value based on\n> the current\n> > >> > + * VBLANK value.\n> > >> > + *\n> > >> > + * These limits get set in the AGC algorithm through\n> applyFrameDurations()\n> > >> > + * below.\n> > >> > + */\n> > >> > + const ControlInfo &gainCtrl =\n> sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN);\n> > >> > + const ControlInfo &shutterCtrl =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE);\n> > >> > +\n> > >> > + sensorLimits_.minAnalogueGain =\n> helper_->gain(gainCtrl.min().get());\n> > >> > + sensorLimits_.maxAnalogueGain =\n> helper_->gain(gainCtrl.max().get());\n> > >> > + sensorLimits_.minFrameDuration = mode_.minFrameLength *\n> mode_.minLineLength;\n> > >> > + sensorLimits_.maxFrameDuration = mode_.maxFrameLength *\n> mode_.maxLineLength;\n> > >> > + sensorLimits_.minShutter =\n> helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > >> > + sensorLimits_.maxShutter = Duration::max();\n> > >> > + helper_->getBlanking(sensorLimits_.maxShutter,\n> > >> > + sensorLimits_.minFrameDuration,\n> sensorLimits_.maxFrameDuration);\n> > >> > +\n> > >> > /*\n> > >> > * Initialise this ControlList correctly, even if empty, in\n> case the IPA is\n> > >> > * running is isolation mode (passing the ControlList through\n> the IPC layer).\n> > >> > @@ -501,26 +521,17 @@ int IPARPi::configure(const\n> IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > >> > * based on the current sensor mode.\n> > >> > */\n> > >> > ControlInfoMap::Map ctrlMap = ipaControls;\n> > >> > - const Duration minSensorFrameDuration = mode_.minFrameLength\n> * mode_.minLineLength;\n> > >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength\n> * mode_.maxLineLength;\n> > >> > ctrlMap[&controls::FrameDurationLimits] =\n> > >> > -\n> ControlInfo(static_cast(minSensorFrameDuration.get()),\n> > >> > -\n> static_cast(maxSensorFrameDuration.get()));\n> > >> > +\n> ControlInfo(static_cast(sensorLimits_.minFrameDuration.get()),\n> > >> > +\n> static_cast(sensorLimits_.maxFrameDuration.get()));\n> > >> >\n> > >> > ctrlMap[&controls::AnalogueGain] =\n> > >> > - ControlInfo(1.0f,\n> static_cast(helper_->gain(maxSensorGainCode_)));\n> > >> > -\n> > >> > - /*\n> > >> > - * Calculate the max exposure limit from the frame duration\n> limit as V4L2\n> > >> > - * will limit the maximum control value based on the current\n> VBLANK value.\n> > >> > - */\n> > >> > - Duration maxShutter = Duration::max();\n> > >> > - helper_->getBlanking(maxShutter, minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > >> > - const uint32_t exposureMin =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get();\n> > >> > +\n> ControlInfo(static_cast(sensorLimits_.minAnalogueGain),\n> > >> > +\n> static_cast(sensorLimits_.maxAnalogueGain));\n> > >> >\n> > >> > ctrlMap[&controls::ExposureTime] =\n> > >> > -\n> ControlInfo(static_cast(helper_->exposure(exposureMin,\n> mode_.minLineLength).get()),\n> > >> > -\n> static_cast(maxShutter.get()));\n> > >> > +\n> ControlInfo(static_cast(sensorLimits_.minShutter.get()),\n> > >> > +\n> static_cast(sensorLimits_.maxShutter.get()));\n> > >> >\n> > >> > /* Declare Autofocus controls, only if we have a controllable\n> lens */\n> > >> > if (lensPresent_)\n> > >> > @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > >> >\n> > >> > void IPARPi::applyFrameDurations(Duration minFrameDuration,\n> Duration maxFrameDuration)\n> > >> > {\n> > >> > - const Duration minSensorFrameDuration = mode_.minFrameLength\n> * mode_.minLineLength;\n> > >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength\n> * mode_.maxLineLength;\n> > >> > -\n> > >> > /*\n> > >> > * This will only be applied once AGC recalculations occur.\n> > >> > * The values may be clamped based on the sensor mode\n> capabilities as well.\n> > >> > @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration\n> minFrameDuration, Duration maxFrameDur\n> > >> > minFrameDuration_ = minFrameDuration ? minFrameDuration :\n> defaultMaxFrameDuration;\n> > >> > maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :\n> defaultMinFrameDuration;\n> > >> > minFrameDuration_ = std::clamp(minFrameDuration_,\n> > >> > - minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > >> > +\n> sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n> > >> > maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > >> > - minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > >> > +\n> sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);\n> > >> > maxFrameDuration_ = std::max(maxFrameDuration_,\n> minFrameDuration_);\n> > >> >\n> > >> > /* Return the validated limits via metadata. */\n> > >> > @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration\n> minFrameDuration, Duration maxFrameDur\n> > >> > * getBlanking() will update maxShutter with the largest\n> exposure\n> > >> > * value possible.\n> > >> > */\n> > >> > - RPiController::AgcAlgorithm::SensorLimits limits;\n> > >> > - limits.maxShutter = Duration::max();\n> > >> > - helper_->getBlanking(limits.maxShutter, minFrameDuration_,\n> maxFrameDuration_);\n> > >> > + sensorLimits_.maxShutter = Duration::max();\n> > >> > + helper_->getBlanking(sensorLimits_.maxShutter,\n> minFrameDuration_, maxFrameDuration_);\n> > >> >\n> > >> > RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> > >> > controller_.getAlgorithm(\"agc\"));\n> > >> > - agc->setSensorLimits(limits);\n> > >> > + agc->setSensorLimits(sensorLimits_);\n> > >> > }\n> > >> >\n> > >> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus,\n> ControlList &ctrls)\n> > >> > {\n> > >> > + const int32_t minGainCode =\n> helper_->gainCode(sensorLimits_.minAnalogueGain);\n> > >> > + const int32_t maxGainCode =\n> helper_->gainCode(sensorLimits_.maxAnalogueGain);\n> > >> > int32_t gainCode = helper_->gainCode(agcStatus->analogueGain);\n> > >> >\n> > >> > /*\n> > >> > @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> > >> > * DelayedControls. The AGC will correctly handle a lower\n> gain returned\n> > >> > * by the sensor, provided it knows the actual gain used.\n> > >> > */\n> > >> > - gainCode = std::min(gainCode, maxSensorGainCode_);\n> > >> > + gainCode = std::clamp(gainCode, minGainCode,\n> maxGainCode);\n> > >> >\n> > >> > /* getBlanking might clip exposure time to the fps limits. */\n> > >> > Duration exposure = agcStatus->shutterTime;\n> > >> > --\n> > >> > 2.34.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 80234BD160\n\tfor ;\n\tMon, 27 Mar 2023 07:57:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEBA0626DB;\n\tMon, 27 Mar 2023 09:57:32 +0200 (CEST)","from mail-yb1-xb2f.google.com (mail-yb1-xb2f.google.com\n\t[IPv6:2607:f8b0:4864:20::b2f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 91CFE626DB\n\tfor ;\n\tMon, 27 Mar 2023 09:57:29 +0200 (CEST)","by mail-yb1-xb2f.google.com with SMTP id i6so9360464ybu.8\n\tfor ;\n\tMon, 27 Mar 2023 00:57:29 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679903852;\n\tbh=b62Psm6OGStrnTAAIDSvEiCubfA0RV6clidJgqyWNd4=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=aedO1FOT5vhisLRiVYvvanRubyc31Kwm/WJquM5jrrSYjVBGwU54uR32BLx7SnYgC\n\tXO656AssSsShrpPRyRjYl5XdyhfbDZKFcrHDA/osMqYxvZ04KTU5xsMDtmRLIB+zay\n\t6eieQsoUb2bK0afEx3x7sO1+/oVSxJxDT72JwJMZRJksGy9POUhXANSALfCuBVjXeI\n\t4c9SRp/2ci842KnMkawTHIYc2c+/ZRZLZ+1Pho02YHVE/WMsAGJuqgwICoaRCoeUtS\n\tORZcnWb3yTbMfDA1pvFpwm+G1YKQIrxgwFLG1gvQPMEyrVq08y4PSgHxdn6F6fQZts\n\tmIBQYRH8EZehQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1679903848;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=f6Z9IDO30Atjd7EI7YhnVchNhP6NNrxyRUiASUEis2k=;\n\tb=VrtsoiEEMG2fW4KaU34+skV+yAHTlm6YfDl/eVNGm5f/BqgL6Xwbsx92cSE5KgIAC3\n\tTW6r7YKULVEq3Jx1wMe/yC3B4r50rQWtrJQM0Nlb3D3edAeq9H8Y4Rdz17Hpccc9MilT\n\t/ozs0iePBWT1pS2ZMlxDm5Znt8JAXIdNcpt5fbkHB620IEPtsCjTSLgQiiomTsBaOkr6\n\tR+B235rVgrsxmO1LFODEUALGJna8xccOaRIuejK3FoDLWfFPJWo+wbrrZLDwJBuGTGwT\n\tBBxGV/7dzbAH6GOxsMQR7F4Gey+YEsoC35M2fV8xfM906OMg3M6Z0XWgzwFTlX/cFxmV\n\tjDrw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"VrtsoiEE\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679903848;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=f6Z9IDO30Atjd7EI7YhnVchNhP6NNrxyRUiASUEis2k=;\n\tb=tb+eaCIfr0jdpO9uItv1y+GE0tfBgHdvH+HDY60PoiYiFq8Ko7igNKH/uknxXWERSU\n\toFAuEaMSmJ8eNk4RYubJupiMSrA/Ihw6A+xIVKTOnMZnZ86UNUulGOu9Mc+f0kPbSjMd\n\tLMBQfmKmh6CfAx4EDs9DGru7RL7jRN+dAp1H1033Uv2LzcwVW18Wk7TulnOe4BwSlh/D\n\tzV7vtMzvM7y9U7gWGwqwohBntKwQ9TQ7Z3zmSoxP0DTyOhyrSfduBqFMIFfnpqgCDfJd\n\t7oAxNy0SVivoEbkZ8qOicvBiAIkR5YETxqkP6GBeMuXLlKLwrDnlZxSAGXt6Xz5INXo2\n\tkikQ==","X-Gm-Message-State":"AAQBX9fkx6ubNwQSbUHepgjrpH7wu+KEP2fFvRV9R7NdtwYLKW3u01HL\n\td9aCuZntiUno5uUV1BESh9x3yHgYcQTlDelcvShctw==","X-Google-Smtp-Source":"AKy350ZO8S/ZCYLswHoEKLjC37PcJAoyfEFfciJnKhhhZq4Edl+iiquxFaKeFxRijOBLxA7KPIdBIJq0VP9PYvl+yYs=","X-Received":"by 2002:a05:6902:1501:b0:b4c:9333:2a2 with SMTP id\n\tq1-20020a056902150100b00b4c933302a2mr5078015ybu.9.1679903848186;\n\tMon, 27 Mar 2023 00:57:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20230322161317.18055-1-naush@raspberrypi.com>\n\t<20230322161317.18055-3-naush@raspberrypi.com>\n\t<20230324142332.ud5roktlajo6arqs@uno.localdomain>\n\t\n\t\n\t<20230325091940.cep4nov3xjy3eveh@uno.localdomain>","In-Reply-To":"<20230325091940.cep4nov3xjy3eveh@uno.localdomain>","Date":"Mon, 27 Mar 2023 07:57:27 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"00000000000020dde205f7dd18ad\"","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate\n\tSensorLimits","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]