[{"id":20204,"web_url":"https://patchwork.libcamera.org/comment/20204/","msgid":"<163421122544.3829429.17732458561893152788@Monstersaurus>","date":"2021-10-14T11:33:45","subject":"Re: [libcamera-devel] [PATCH 11/13] ipa: ipu3: agc: Remove\n\tcondition on exposure correction","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-10-13 16:41:23)\n> When the exposure is estimated, we verify if it changed enough for\n> exposure and gain to be updated.\n> \n> There is no need for that because we are now filtering the value with\n> the previous one correctly, so if the change is very small is won't\n> change the exposure and analogue gain.\n\nOh excellent ... looks like here comes the refactor I was half\nsuggesting in the previous patch...\n\nIt's hard to see if there are any specific changes in here, and I assume\nit's just the ultimate removal of the\n \"if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\"\n\nand associated re-indentation.\n\nIf so:\n\nReviewed-by: Kieran Bingham \n\nI'd be tempted to move that lastFrame_ = frameCount_ up to just after it\ngets used by the opening checks, as it's not used by any of the code\nfollowing.\n\nUnless perhaps it's better at the end to signify it's only updated after\nthe calculations are completed. Either way, it's fine really.\n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 96 ++++++++++++++++-----------------\n> 1 file changed, 46 insertions(+), 50 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 7efe0907..b922bcdf 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -132,61 +132,57 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))\n> return;\n> \n> - /* Are we correctly exposed ? */\n> - if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n> - LOG(IPU3Agc, Debug) << \"!!! Good exposure with iqMean = \" << iqMean_;\n> - } else {\n> - double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> -\n> - /* extracted from Rpi::Agc::computeTargetExposure */\n> - Duration currentShutter = exposure * lineDuration_;\n> - currentExposureNoDg_ = currentShutter * analogueGain;\n> - LOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> - << \" Shutter speed \" << currentShutter\n> - << \" Gain \" << analogueGain\n> - << \" Needed ev gain \" << evGain;\n> -\n> - currentExposure_ = prevExposureValue * evGain;\n> - Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;\n> - currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> - LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> - << \" maximum is \" << maxTotalExposure;\n> -\n> - /* \\todo: estimate if we need to desaturate */\n> - filterExposure();\n> -\n> - Duration exposureValue = filteredExposure_;\n> - Duration shutterTime = kMinShutterSpeed;\n> - double stepGain = kMinGain;\n> -\n> - if (shutterTime * stepGain < exposureValue) {\n> - Duration maxShutterMinGain = kMaxShutterSpeed * stepGain;\n> - if (maxShutterMinGain >= exposureValue) {\n> - LOG(IPU3Agc, Debug) << \"Setting shutterTime to \" << shutterTime;\n> - shutterTime = exposureValue / stepGain;\n> - } else {\n> - shutterTime = kMaxShutterSpeed;\n> - }\n> + double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> +\n> + /* extracted from Rpi::Agc::computeTargetExposure */\n> + Duration currentShutter = exposure * lineDuration_;\n> + currentExposureNoDg_ = currentShutter * analogueGain;\n> + LOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> + << \" Shutter speed \" << currentShutter\n> + << \" Gain \" << analogueGain\n> + << \" Needed ev gain \" << evGain;\n> +\n> + currentExposure_ = prevExposureValue * evGain;\n> + Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;\n> + currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> + LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> + << \" maximum is \" << maxTotalExposure;\n> +\n> + /* \\todo: estimate if we need to desaturate */\n> + filterExposure();\n> +\n> + Duration exposureValue = filteredExposure_;\n> + Duration shutterTime = kMinShutterSpeed;\n> + double stepGain = kMinGain;\n> +\n> + if (shutterTime * stepGain < exposureValue) {\n> + Duration maxShutterMinGain = kMaxShutterSpeed * stepGain;\n> + if (maxShutterMinGain >= exposureValue) {\n> + LOG(IPU3Agc, Debug) << \"Setting shutterTime to \" << shutterTime;\n> + shutterTime = exposureValue / stepGain;\n> + } else {\n> + shutterTime = kMaxShutterSpeed;\n> + }\n> \n> - Duration maxGainShutter = kMaxGain * shutterTime;\n> - if (maxGainShutter >= exposureValue) {\n> - stepGain = exposureValue / shutterTime;\n> - LOG(IPU3Agc, Debug) << \"Setting analogue gain to \" << stepGain;\n> - } else {\n> - stepGain = kMaxGain;\n> - }\n> + Duration maxGainShutter = kMaxGain * shutterTime;\n> + if (maxGainShutter >= exposureValue) {\n> + stepGain = exposureValue / shutterTime;\n> + LOG(IPU3Agc, Debug) << \"Setting analogue gain to \" << stepGain;\n> + } else {\n> + stepGain = kMaxGain;\n> }\n> + }\n> \n> - LOG(IPU3Agc, Debug) << \"Divided up shutter and gain are \"\n> - << shutterTime << \" and \"\n> - << stepGain;\n> + LOG(IPU3Agc, Debug) << \"Divided up shutter and gain are \"\n> + << shutterTime << \" and \"\n> + << stepGain;\n> \n> - exposure = shutterTime / lineDuration_;\n> - analogueGain = stepGain;\n> + exposure = shutterTime / lineDuration_;\n> + analogueGain = stepGain;\n> +\n> + /* Update the exposure value for the next process call */\n> + prevExposureValue = shutterTime * analogueGain;\n> \n> - /* Update the exposure value for the next process call */\n> - prevExposureValue = shutterTime * analogueGain;\n> - }\n> lastFrame_ = frameCount_;\n> }\n> \n> -- \n> 2.30.2\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 3B651BDC71\n\tfor ;\n\tThu, 14 Oct 2021 11:33:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3F6F68541;\n\tThu, 14 Oct 2021 13:33:49 +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 8C5C768541\n\tfor ;\n\tThu, 14 Oct 2021 13:33:48 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 308F62F3;\n\tThu, 14 Oct 2021 13:33:48 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bhYhRyqo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634211228;\n\tbh=8qCkapr926FOy3GQdRmkRGeuF6gAirjTDBDPWMhclSc=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=bhYhRyqo7v08PZ/GsnJb5mt21QJ+V0d5e7TxzNqk29FfYibf4E/+kVh0WzgECMl7d\n\tzHkZS/U/FAIfJvEZ5t3Podd0J428/f1WXHsFESj4BThcvcMbkD9wfIsWLPSc+COwqd\n\t5ZqsRCAh9iakYxFzJ5VqC/jIL6ySbVzEtUA4COgQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211013154125.133419-12-jeanmichel.hautbois@ideasonboard.com>","References":"<20211013154125.133419-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211013154125.133419-12-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 14 Oct 2021 12:33:45 +0100","Message-ID":"<163421122544.3829429.17732458561893152788@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 11/13] ipa: ipu3: agc: Remove\n\tcondition on exposure correction","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20221,"web_url":"https://patchwork.libcamera.org/comment/20221/","msgid":"","date":"2021-10-15T01:31:49","subject":"Re: [libcamera-devel] [PATCH 11/13] ipa: ipu3: agc: Remove\n\tcondition on exposure correction","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\n(CC'ing David)\n\nOn Wed, Oct 13, 2021 at 05:41:23PM +0200, Jean-Michel Hautbois wrote:\n> When the exposure is estimated, we verify if it changed enough for\n> exposure and gain to be updated.\n> \n> There is no need for that because we are now filtering the value with\n> the previous one correctly, so if the change is very small is won't\n> change the exposure and analogue gain.\n\nI fail to see why a small change won't change the exposure and analogue\ngain. As far as I understand (maybe David can confirm), this removes a\nhysteresis, which I believe will cause oscillations.\n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 96 ++++++++++++++++-----------------\n> 1 file changed, 46 insertions(+), 50 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 7efe0907..b922bcdf 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -132,61 +132,57 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> \tif ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))\n> \t\treturn;\n> \n> -\t/* Are we correctly exposed ? */\n> -\tif (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n> -\t\tLOG(IPU3Agc, Debug) << \"!!! Good exposure with iqMean = \" << iqMean_;\n> -\t} else {\n> -\t\tdouble evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> -\n> -\t\t/* extracted from Rpi::Agc::computeTargetExposure */\n> -\t\tDuration currentShutter = exposure * lineDuration_;\n> -\t\tcurrentExposureNoDg_ = currentShutter * analogueGain;\n> -\t\tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> -\t\t\t\t << \" Shutter speed \" << currentShutter\n> -\t\t\t\t << \" Gain \" << analogueGain\n> -\t\t\t\t << \" Needed ev gain \" << evGain;\n> -\n> -\t\tcurrentExposure_ = prevExposureValue * evGain;\n> -\t\tDuration maxTotalExposure = kMaxShutterSpeed * kMaxGain;\n> -\t\tcurrentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> -\t\tLOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> -\t\t\t\t << \" maximum is \" << maxTotalExposure;\n> -\n> -\t\t/* \\todo: estimate if we need to desaturate */\n> -\t\tfilterExposure();\n> -\n> -\t\tDuration exposureValue = filteredExposure_;\n> -\t\tDuration shutterTime = kMinShutterSpeed;\n> -\t\tdouble stepGain = kMinGain;\n> -\n> -\t\tif (shutterTime * stepGain < exposureValue) {\n> -\t\t\tDuration maxShutterMinGain = kMaxShutterSpeed * stepGain;\n> -\t\t\tif (maxShutterMinGain >= exposureValue) {\n> -\t\t\t\tLOG(IPU3Agc, Debug) << \"Setting shutterTime to \" << shutterTime;\n> -\t\t\t\tshutterTime = exposureValue / stepGain;\n> -\t\t\t} else {\n> -\t\t\t\tshutterTime = kMaxShutterSpeed;\n> -\t\t\t}\n> +\tdouble evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> +\n> +\t/* extracted from Rpi::Agc::computeTargetExposure */\n> +\tDuration currentShutter = exposure * lineDuration_;\n> +\tcurrentExposureNoDg_ = currentShutter * analogueGain;\n> +\tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> +\t\t\t\t<< \" Shutter speed \" << currentShutter\n> +\t\t\t\t<< \" Gain \" << analogueGain\n> +\t\t\t\t<< \" Needed ev gain \" << evGain;\n> +\n> +\tcurrentExposure_ = prevExposureValue * evGain;\n> +\tDuration maxTotalExposure = kMaxShutterSpeed * kMaxGain;\n> +\tcurrentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> +\tLOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> +\t\t\t\t<< \" maximum is \" << maxTotalExposure;\n> +\n> +\t/* \\todo: estimate if we need to desaturate */\n> +\tfilterExposure();\n> +\n> +\tDuration exposureValue = filteredExposure_;\n> +\tDuration shutterTime = kMinShutterSpeed;\n> +\tdouble stepGain = kMinGain;\n> +\n> +\tif (shutterTime * stepGain < exposureValue) {\n> +\t\tDuration maxShutterMinGain = kMaxShutterSpeed * stepGain;\n> +\t\tif (maxShutterMinGain >= exposureValue) {\n> +\t\t\tLOG(IPU3Agc, Debug) << \"Setting shutterTime to \" << shutterTime;\n> +\t\t\tshutterTime = exposureValue / stepGain;\n> +\t\t} else {\n> +\t\t\tshutterTime = kMaxShutterSpeed;\n> +\t\t}\n> \n> -\t\t\tDuration maxGainShutter = kMaxGain * shutterTime;\n> -\t\t\tif (maxGainShutter >= exposureValue) {\n> -\t\t\t\tstepGain = exposureValue / shutterTime;\n> -\t\t\t\tLOG(IPU3Agc, Debug) << \"Setting analogue gain to \" << stepGain;\n> -\t\t\t} else {\n> -\t\t\t\tstepGain = kMaxGain;\n> -\t\t\t}\n> +\t\tDuration maxGainShutter = kMaxGain * shutterTime;\n> +\t\tif (maxGainShutter >= exposureValue) {\n> +\t\t\tstepGain = exposureValue / shutterTime;\n> +\t\t\tLOG(IPU3Agc, Debug) << \"Setting analogue gain to \" << stepGain;\n> +\t\t} else {\n> +\t\t\tstepGain = kMaxGain;\n> \t\t}\n> +\t}\n> \n> -\t\tLOG(IPU3Agc, Debug) << \"Divided up shutter and gain are \"\n> -\t\t\t\t<< shutterTime << \" and \"\n> -\t\t\t\t<< stepGain;\n> +\tLOG(IPU3Agc, Debug) << \"Divided up shutter and gain are \"\n> +\t\t\t<< shutterTime << \" and \"\n> +\t\t\t<< stepGain;\n> \n> -\t\texposure = shutterTime / lineDuration_;\n> -\t\tanalogueGain = stepGain;\n> +\texposure = shutterTime / lineDuration_;\n> +\tanalogueGain = stepGain;\n> +\n> +\t/* Update the exposure value for the next process call */\n> +\tprevExposureValue = shutterTime * analogueGain;\n> \n> -\t\t/* Update the exposure value for the next process call */\n> -\t\tprevExposureValue = shutterTime * analogueGain;\n> -\t}\n> \tlastFrame_ = frameCount_;\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 63C13C324C\n\tfor ;\n\tFri, 15 Oct 2021 01:32:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6B8E68F50;\n\tFri, 15 Oct 2021 03:32:06 +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 DA98568F4D\n\tfor ;\n\tFri, 15 Oct 2021 03:32:05 +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 29FE12E3;\n\tFri, 15 Oct 2021 03:32:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZZkT2WEg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634261525;\n\tbh=5z1ZntxbfvABcnx52VAGQLnnEI03xGYnhJDORc4wFSU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZZkT2WEgq83d1XL2ZDroRDxbn8f3mW7a+wekCLCJlpB6s6Q90Nd4BXMPXYsAch8TQ\n\t4qMUD04ebkxMfdECP8GdyGR+MpdFBKtjxLIZhX6bMVXklH1unmvHISlqGjiOvLYJ/o\n\tfXik3YlmT5hmCic4bYJDSAp+ymkOfBRLrVasFuwQ=","Date":"Fri, 15 Oct 2021 04:31:49 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ,\n\tDavid Plowman ","Message-ID":"","References":"<20211013154125.133419-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211013154125.133419-12-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211013154125.133419-12-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 11/13] ipa: ipu3: agc: Remove\n\tcondition on exposure correction","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]