[{"id":17401,"web_url":"https://patchwork.libcamera.org/comment/17401/","msgid":"<477251c9-1402-80be-9ed4-405d6cf3a529@ideasonboard.com>","date":"2021-06-04T09:27:15","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: ipu3: Calculate line\n\tduration from IPACameraSensorInfo","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 02/06/2021 11:23, Umang Jain wrote:\n> Squash \\todo by calculating line duration from IPACameraSensorInfo,\n> now passed in, to IPU3Agc::initialise().\n> \n> Since line duration is now calculated from real values, store it as a\n> private member in IPU3Agc class. As a further step, replace the\n> associated global constant, kMaxExposureTime, with a private IPU3Agc\n> class member as well, and assign its value correspondingly in\n> IPU3Agc::initialise(), similar to previous precedence.\n> \n> Signed-off-by: Umang Jain \n\nThis looks reasonable to me. I guess we could have moved the sensorInfo\n [3/4] first but that doesn't make much difference, and this way moves\nit only where there is a user of it.\n\nReviewed-by: Kieran Bingham ---\n> src/ipa/ipu3/ipu3.cpp | 2 +-\n> src/ipa/ipu3/ipu3_agc.cpp | 22 +++++++++++-----------\n> src/ipa/ipu3/ipu3_agc.h | 7 ++++++-\n> 3 files changed, 18 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 700a5660..2496b0a0 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -174,7 +174,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n> \n> \tagcAlgo_ = std::make_unique();\n> -\tagcAlgo_->initialise(bdsGrid_);\n> +\tagcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo);\n> }\n> \n> void IPAIPU3::mapBuffers(const std::vector &buffers)\n> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n> index 8bae423f..ccd01371 100644\n> --- a/src/ipa/ipu3/ipu3_agc.cpp\n> +++ b/src/ipa/ipu3/ipu3_agc.cpp\n> @@ -39,11 +39,6 @@ static constexpr uint32_t kMaxGain = kMaxISO / 100;\n> static constexpr uint32_t kMinExposure = 1;\n> static constexpr uint32_t kMaxExposure = 1976;\n> \n> -/* \\todo those should be got from IPACameraSensorInfo ! */\n> -/* line duration in microseconds */\n> -static constexpr double kLineDuration = 16.8;\n> -static constexpr double kMaxExposureTime = kMaxExposure * kLineDuration;\n> -\n> /* Histogram constants */\n> static constexpr uint32_t knumHistogramBins = 256;\n> static constexpr double kEvGainTarget = 0.5;\n> @@ -54,14 +49,19 @@ static constexpr uint8_t kCellSize = 8;\n> IPU3Agc::IPU3Agc()\n> \t: frameCount_(0), lastFrame_(0), converged_(false),\n> \t updateControls_(false), iqMean_(0.0), gamma_(1.0),\n> +\t lineDuration_(0.0), maxExposureTime_(0.0),\n> \t prevExposure_(0.0), prevExposureNoDg_(0.0),\n> \t currentExposure_(0.0), currentExposureNoDg_(0.0)\n> {\n> }\n> \n> -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid)\n> +void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo)\n> {\n> \taeGrid_ = bdsGrid;\n> +\n> +\t/* line duration in microseconds */\n> +\tlineDuration_ = sensorInfo.lineLength / static_cast(sensorInfo.pixelRate / 1000000);\n> +\tmaxExposureTime_ = kMaxExposure * lineDuration_;\n> }\n> \n> void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)\n> @@ -160,13 +160,13 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, uint32_t &gain)\n> \t\tdouble newGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> \t\t/* extracted from Rpi::Agc::computeTargetExposure */\n> -\t\tdouble currentShutter = exposure * kLineDuration;\n> +\t\tdouble currentShutter = exposure * lineDuration_;\n> \t\tcurrentExposureNoDg_ = currentShutter * gain;\n> \t\tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> \t\t\t\t << \" Shutter speed \" << currentShutter\n> \t\t\t\t << \" Gain \" << gain;\n> \t\tcurrentExposure_ = currentExposureNoDg_ * newGain;\n> -\t\tdouble maxTotalExposure = kMaxExposureTime * kMaxGain;\n> +\t\tdouble maxTotalExposure = maxExposureTime_ * kMaxGain;\n> \t\tcurrentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> \t\tLOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_;\n> \n> @@ -174,18 +174,18 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, uint32_t &gain)\n> \t\tfilterExposure();\n> \n> \t\tdouble newExposure = 0.0;\n> -\t\tif (currentShutter < kMaxExposureTime) {\n> +\t\tif (currentShutter < maxExposureTime_) {\n> \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);\n> \t\t\tnewExposure = currentExposure_ / exposure;\n> \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);\n> \t\t\tupdateControls_ = true;\n> -\t\t} else if (currentShutter >= kMaxExposureTime) {\n> +\t\t} else if (currentShutter >= maxExposureTime_) {\n> \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);\n> \t\t\tnewExposure = currentExposure_ / gain;\n> \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);\n> \t\t\tupdateControls_ = true;\n> \t\t}\n> -\t\tLOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * kLineDuration << \" and gain \" << gain;\n> +\t\tLOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * lineDuration_ << \" and gain \" << gain;\n> \t}\n> \tlastFrame_ = frameCount_;\n> }\n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> index bfdf45d1..ae557bdd 100644\n> --- a/src/ipa/ipu3/ipu3_agc.h\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -12,6 +12,8 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \n> \n> #include \"libipa/algorithm.h\"\n> @@ -26,7 +28,7 @@ public:\n> \tIPU3Agc();\n> \t~IPU3Agc() = default;\n> \n> -\tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid);\n> +\tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);\n> \tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain);\n> \tbool converged() { return converged_; }\n> \tbool updateControls() { return updateControls_; }\n> @@ -49,6 +51,9 @@ private:\n> \tdouble iqMean_;\n> \tdouble gamma_;\n> \n> +\tdouble lineDuration_;\n> +\tdouble maxExposureTime_;\n> +\n> \tdouble prevExposure_;\n> \tdouble prevExposureNoDg_;\n> \tdouble currentExposure_;\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 7DE50C3206\n\tfor ;\n\tFri, 4 Jun 2021 09:27:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40B5E6892D;\n\tFri, 4 Jun 2021 11:27:20 +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 5C49268925\n\tfor ;\n\tFri, 4 Jun 2021 11:27:18 +0200 (CEST)","from [192.168.0.20]\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 CF213EF;\n\tFri, 4 Jun 2021 11:27:17 +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=\"whxNvSKp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622798837;\n\tbh=cPtk2Aan2+6jUDsWnt7hyVpXWSNzH7KTEpHe0+TyBqs=;\n\th=Reply-To:To:References:From:Subject:Date:In-Reply-To:From;\n\tb=whxNvSKpTleSHBYsR9dcjGQ93zgsjRhpiPFr7G0ZECr9TE6AqVYVRqc3wlK63kQa3\n\tTepYCE1SZ46yTU710+yrAud+V6akgYM3ldKsR9QKV7qxeRQA+9B9BoJQqJBh+PGDVL\n\tgII5jvIeVzzdchCQfSlNOL+XEIUWMWU86m7ureA4=","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210602102326.106549-1-umang.jain@ideasonboard.com>\n\t<20210602102326.106549-3-umang.jain@ideasonboard.com>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<477251c9-1402-80be-9ed4-405d6cf3a529@ideasonboard.com>","Date":"Fri, 4 Jun 2021 10:27:15 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"<20210602102326.106549-3-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: ipu3: Calculate line\n\tduration from IPACameraSensorInfo","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","Reply-To":"kieran.bingham@ideasonboard.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17403,"web_url":"https://patchwork.libcamera.org/comment/17403/","msgid":"<8c47b048-c3b2-e2df-11de-ea7d7ab2e792@ideasonboard.com>","date":"2021-06-04T09:39:54","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: ipu3: Calculate line\n\tduration from IPACameraSensorInfo","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 04/06/2021 10:27, Kieran Bingham wrote:\n> Hi Umang,\n> \n> On 02/06/2021 11:23, Umang Jain wrote:\n>> Squash \\todo by calculating line duration from IPACameraSensorInfo,\n>> now passed in, to IPU3Agc::initialise().\n>>\n>> Since line duration is now calculated from real values, store it as a\n>> private member in IPU3Agc class. As a further step, replace the\n>> associated global constant, kMaxExposureTime, with a private IPU3Agc\n>> class member as well, and assign its value correspondingly in\n>> IPU3Agc::initialise(), similar to previous precedence.\n>>\n>> Signed-off-by: Umang Jain \n> \n> This looks reasonable to me. I guess we could have moved the sensorInfo\n> [3/4] first but that doesn't make much difference, and this way moves\n> it only where there is a user of it.\n> \n> Reviewed-by: Kieran Bingham \n>> ---\n>> src/ipa/ipu3/ipu3.cpp | 2 +-\n>> src/ipa/ipu3/ipu3_agc.cpp | 22 +++++++++++-----------\n>> src/ipa/ipu3/ipu3_agc.h | 7 ++++++-\n>> 3 files changed, 18 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 700a5660..2496b0a0 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -174,7 +174,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>> \tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n>> \n>> \tagcAlgo_ = std::make_unique();\n>> -\tagcAlgo_->initialise(bdsGrid_);\n>> +\tagcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo);\n>> }\n>> \n>> void IPAIPU3::mapBuffers(const std::vector &buffers)\n>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n>> index 8bae423f..ccd01371 100644\n>> --- a/src/ipa/ipu3/ipu3_agc.cpp\n>> +++ b/src/ipa/ipu3/ipu3_agc.cpp\n>> @@ -39,11 +39,6 @@ static constexpr uint32_t kMaxGain = kMaxISO / 100;\n>> static constexpr uint32_t kMinExposure = 1;\n>> static constexpr uint32_t kMaxExposure = 1976;\n>> \n>> -/* \\todo those should be got from IPACameraSensorInfo ! */\n>> -/* line duration in microseconds */\n>> -static constexpr double kLineDuration = 16.8;\n>> -static constexpr double kMaxExposureTime = kMaxExposure * kLineDuration;\n>> -\n>> /* Histogram constants */\n>> static constexpr uint32_t knumHistogramBins = 256;\n>> static constexpr double kEvGainTarget = 0.5;\n>> @@ -54,14 +49,19 @@ static constexpr uint8_t kCellSize = 8;\n>> IPU3Agc::IPU3Agc()\n>> \t: frameCount_(0), lastFrame_(0), converged_(false),\n>> \t updateControls_(false), iqMean_(0.0), gamma_(1.0),\n>> +\t lineDuration_(0.0), maxExposureTime_(0.0),\n>> \t prevExposure_(0.0), prevExposureNoDg_(0.0),\n>> \t currentExposure_(0.0), currentExposureNoDg_(0.0)\n>> {\n>> }\n>> \n>> -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid)\n>> +void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo)\n>> {\n>> \taeGrid_ = bdsGrid;\n>> +\n>> +\t/* line duration in microseconds */\n>> +\tlineDuration_ = sensorInfo.lineLength / static_cast(sensorInfo.pixelRate / 1000000);\n>> +\tmaxExposureTime_ = kMaxExposure * lineDuration_;\n>> }\n>> \n>> void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)\n>> @@ -160,13 +160,13 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, uint32_t &gain)\n>> \t\tdouble newGain = kEvGainTarget * knumHistogramBins / iqMean_;\n>> \n>> \t\t/* extracted from Rpi::Agc::computeTargetExposure */\n>> -\t\tdouble currentShutter = exposure * kLineDuration;\n>> +\t\tdouble currentShutter = exposure * lineDuration_;\n>> \t\tcurrentExposureNoDg_ = currentShutter * gain;\n>> \t\tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n>> \t\t\t\t << \" Shutter speed \" << currentShutter\n>> \t\t\t\t << \" Gain \" << gain;\n>> \t\tcurrentExposure_ = currentExposureNoDg_ * newGain;\n>> -\t\tdouble maxTotalExposure = kMaxExposureTime * kMaxGain;\n>> +\t\tdouble maxTotalExposure = maxExposureTime_ * kMaxGain;\n>> \t\tcurrentExposure_ = std::min(currentExposure_, maxTotalExposure);\n>> \t\tLOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_;\n>> \n>> @@ -174,18 +174,18 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, uint32_t &gain)\n>> \t\tfilterExposure();\n>> \n>> \t\tdouble newExposure = 0.0;\n>> -\t\tif (currentShutter < kMaxExposureTime) {\n>> +\t\tif (currentShutter < maxExposureTime_) {\n>> \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);\n>> \t\t\tnewExposure = currentExposure_ / exposure;\n>> \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);\n>> \t\t\tupdateControls_ = true;\n>> -\t\t} else if (currentShutter >= kMaxExposureTime) {\n>> +\t\t} else if (currentShutter >= maxExposureTime_) {\n>> \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);\n>> \t\t\tnewExposure = currentExposure_ / gain;\n>> \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);\n>> \t\t\tupdateControls_ = true;\n>> \t\t}\n>> -\t\tLOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * kLineDuration << \" and gain \" << gain;\n>> +\t\tLOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * lineDuration_ << \" and gain \" << gain;\n>> \t}\n>> \tlastFrame_ = frameCount_;\n>> }\n>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n>> index bfdf45d1..ae557bdd 100644\n>> --- a/src/ipa/ipu3/ipu3_agc.h\n>> +++ b/src/ipa/ipu3/ipu3_agc.h\n>> @@ -12,6 +12,8 @@\n>> \n>> #include \n>> \n>> +#include \n\nIn fact, the IPACameraSensorInfo comes from\n\nlibcamera/ipa/core_ipa_interface.h\n\nIs there anything from ipu3_ipa_interface.h that will be needed in this\nipu3_agc component?\n\n--\nKieran\n\n>> +\n>> #include \n>> \n>> #include \"libipa/algorithm.h\"\n>> @@ -26,7 +28,7 @@ public:\n>> \tIPU3Agc();\n>> \t~IPU3Agc() = default;\n>> \n>> -\tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid);\n>> +\tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);\n>> \tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain);\n>> \tbool converged() { return converged_; }\n>> \tbool updateControls() { return updateControls_; }\n>> @@ -49,6 +51,9 @@ private:\n>> \tdouble iqMean_;\n>> \tdouble gamma_;\n>> \n>> +\tdouble lineDuration_;\n>> +\tdouble maxExposureTime_;\n>> +\n>> \tdouble prevExposure_;\n>> \tdouble prevExposureNoDg_;\n>> \tdouble currentExposure_;\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 6A731C3208\n\tfor ;\n\tFri, 4 Jun 2021 09:39:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE3C968926;\n\tFri, 4 Jun 2021 11:39:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3292968922\n\tfor ;\n\tFri, 4 Jun 2021 11:39:57 +0200 (CEST)","from [192.168.0.20]\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 AB99BEF;\n\tFri, 4 Jun 2021 11:39:56 +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=\"Y/MTRMhp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622799596;\n\tbh=qJlZ57XlPAiBWkP9Ui25pV5ghhUy1kKm8odesoNBi7w=;\n\th=Reply-To:Subject:From:To:References:Date:In-Reply-To:From;\n\tb=Y/MTRMhpwTUqc/7y5bHUlkBZ5NNuYn9IZTy8tq7EugSIkHSUlF4pN5/65VJemYFH0\n\tS5LlL0erFgRlMY14OrJM8kmvWUFou8+P8MlZTcs7ngH/cWDg7oldJa2sSDLJBNhpBx\n\tN1tqWCoWKkSwHgvf5sWzQpUwK9ezNkhjchCXvbzM=","From":"Kieran Bingham ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210602102326.106549-1-umang.jain@ideasonboard.com>\n\t<20210602102326.106549-3-umang.jain@ideasonboard.com>\n\t<477251c9-1402-80be-9ed4-405d6cf3a529@ideasonboard.com>","Organization":"Ideas on Board","Message-ID":"<8c47b048-c3b2-e2df-11de-ea7d7ab2e792@ideasonboard.com>","Date":"Fri, 4 Jun 2021 10:39:54 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"<477251c9-1402-80be-9ed4-405d6cf3a529@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: ipu3: Calculate line\n\tduration from IPACameraSensorInfo","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","Reply-To":"kieran.bingham@ideasonboard.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17455,"web_url":"https://patchwork.libcamera.org/comment/17455/","msgid":"","date":"2021-06-07T22:46:38","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: ipu3: Calculate line\n\tduration from IPACameraSensorInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Wed, Jun 02, 2021 at 03:53:24PM +0530, Umang Jain wrote:\n> Squash \\todo by calculating line duration from IPACameraSensorInfo,\n> now passed in, to IPU3Agc::initialise().\n> \n> Since line duration is now calculated from real values, store it as a\n> private member in IPU3Agc class. As a further step, replace the\n> associated global constant, kMaxExposureTime, with a private IPU3Agc\n> class member as well, and assign its value correspondingly in\n> IPU3Agc::initialise(), similar to previous precedence.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/ipa/ipu3/ipu3.cpp | 2 +-\n> src/ipa/ipu3/ipu3_agc.cpp | 22 +++++++++++-----------\n> src/ipa/ipu3/ipu3_agc.h | 7 ++++++-\n> 3 files changed, 18 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 700a5660..2496b0a0 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -174,7 +174,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n> \n> \tagcAlgo_ = std::make_unique();\n> -\tagcAlgo_->initialise(bdsGrid_);\n> +\tagcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo);\n> }\n> \n> void IPAIPU3::mapBuffers(const std::vector &buffers)\n> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n> index 8bae423f..ccd01371 100644\n> --- a/src/ipa/ipu3/ipu3_agc.cpp\n> +++ b/src/ipa/ipu3/ipu3_agc.cpp\n> @@ -39,11 +39,6 @@ static constexpr uint32_t kMaxGain = kMaxISO / 100;\n> static constexpr uint32_t kMinExposure = 1;\n> static constexpr uint32_t kMaxExposure = 1976;\n> \n> -/* \\todo those should be got from IPACameraSensorInfo ! */\n> -/* line duration in microseconds */\n> -static constexpr double kLineDuration = 16.8;\n> -static constexpr double kMaxExposureTime = kMaxExposure * kLineDuration;\n> -\n> /* Histogram constants */\n> static constexpr uint32_t knumHistogramBins = 256;\n> static constexpr double kEvGainTarget = 0.5;\n> @@ -54,14 +49,19 @@ static constexpr uint8_t kCellSize = 8;\n> IPU3Agc::IPU3Agc()\n> \t: frameCount_(0), lastFrame_(0), converged_(false),\n> \t updateControls_(false), iqMean_(0.0), gamma_(1.0),\n> +\t lineDuration_(0.0), maxExposureTime_(0.0),\n> \t prevExposure_(0.0), prevExposureNoDg_(0.0),\n> \t currentExposure_(0.0), currentExposureNoDg_(0.0)\n> {\n> }\n> \n> -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid)\n> +void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo)\n> {\n> \taeGrid_ = bdsGrid;\n> +\n> +\t/* line duration in microseconds */\n> +\tlineDuration_ = sensorInfo.lineLength / static_cast(sensorInfo.pixelRate / 1000000);\n\nLet's try to avoid losing precision.\n\n\tlineDuration_ = sensorInfo.lineLength * 1000000ULL\n\t\t / static_cast(sensorInfo.pixelRate);\n\nOn a side note, we should really move to std::chrono types at some\npoint.\n\n> +\tmaxExposureTime_ = kMaxExposure * lineDuration_;\n> }\n> \n> void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)\n> @@ -160,13 +160,13 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, uint32_t &gain)\n> \t\tdouble newGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> \t\t/* extracted from Rpi::Agc::computeTargetExposure */\n> -\t\tdouble currentShutter = exposure * kLineDuration;\n> +\t\tdouble currentShutter = exposure * lineDuration_;\n> \t\tcurrentExposureNoDg_ = currentShutter * gain;\n> \t\tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> \t\t\t\t << \" Shutter speed \" << currentShutter\n> \t\t\t\t << \" Gain \" << gain;\n> \t\tcurrentExposure_ = currentExposureNoDg_ * newGain;\n> -\t\tdouble maxTotalExposure = kMaxExposureTime * kMaxGain;\n> +\t\tdouble maxTotalExposure = maxExposureTime_ * kMaxGain;\n> \t\tcurrentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> \t\tLOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_;\n> \n> @@ -174,18 +174,18 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, uint32_t &gain)\n> \t\tfilterExposure();\n> \n> \t\tdouble newExposure = 0.0;\n> -\t\tif (currentShutter < kMaxExposureTime) {\n> +\t\tif (currentShutter < maxExposureTime_) {\n> \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);\n> \t\t\tnewExposure = currentExposure_ / exposure;\n> \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);\n> \t\t\tupdateControls_ = true;\n> -\t\t} else if (currentShutter >= kMaxExposureTime) {\n> +\t\t} else if (currentShutter >= maxExposureTime_) {\n> \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);\n> \t\t\tnewExposure = currentExposure_ / gain;\n> \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);\n> \t\t\tupdateControls_ = true;\n> \t\t}\n> -\t\tLOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * kLineDuration << \" and gain \" << gain;\n> +\t\tLOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * lineDuration_ << \" and gain \" << gain;\n> \t}\n> \tlastFrame_ = frameCount_;\n> }\n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> index bfdf45d1..ae557bdd 100644\n> --- a/src/ipa/ipu3/ipu3_agc.h\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -12,6 +12,8 @@\n> \n> #include \n> \n> +#include \n> +\n\nNo need for a blank line, and you can swap the two headers to order them\nalphabetically. As Kieran mentioned, you can include\ncore_ipa_interface.h instead, or, possibly better, forward-declare the\nIPACameraSensorInfo struct and include core_ipa_interface.h in\nipu3_agc.cpp.\n\nReviewed-by: Laurent Pinchart \n\n> #include \n> \n> #include \"libipa/algorithm.h\"\n> @@ -26,7 +28,7 @@ public:\n> \tIPU3Agc();\n> \t~IPU3Agc() = default;\n> \n> -\tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid);\n> +\tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);\n> \tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain);\n> \tbool converged() { return converged_; }\n> \tbool updateControls() { return updateControls_; }\n> @@ -49,6 +51,9 @@ private:\n> \tdouble iqMean_;\n> \tdouble gamma_;\n> \n> +\tdouble lineDuration_;\n> +\tdouble maxExposureTime_;\n> +\n> \tdouble prevExposure_;\n> \tdouble prevExposureNoDg_;\n> \tdouble currentExposure_;","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 EF330BD22E\n\tfor ;\n\tMon, 7 Jun 2021 22:46:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4AB416892B;\n\tTue, 8 Jun 2021 00:46:54 +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 0C2766891C\n\tfor ;\n\tTue, 8 Jun 2021 00:46:53 +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 6B047292;\n\tTue, 8 Jun 2021 00:46:52 +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=\"Dsjg3MlR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623106012;\n\tbh=d+AjJpgTQDXY1Jk+HaRqNKSYILmYoSX5wI0J+NXepas=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Dsjg3MlRTbLvI8u+HYF4oMOkscB/7v9D591r3OQbFOAI4TY8X4texZtROjZjX8eF6\n\t405ZeNA6Rt+o7CTG1yS0twRyX2RYnak5Xx3SMI29uv+oH7jadKqmVh0S79X3Jvm8pM\n\txaSuJhjcRa2MbfX+A4VuvXB9jYg1LIQ1R7/g4Qy4=","Date":"Tue, 8 Jun 2021 01:46:38 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210602102326.106549-1-umang.jain@ideasonboard.com>\n\t<20210602102326.106549-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210602102326.106549-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: ipu3: Calculate line\n\tduration from IPACameraSensorInfo","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\" "}}]