[{"id":18031,"web_url":"https://patchwork.libcamera.org/comment/18031/","msgid":"","date":"2021-07-08T12:58:57","subject":"Re: [libcamera-devel] [PATCH v1 5/7] ipa: ipu3: Improve AWB\n\tbehaviour","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Mon, Jun 28, 2021 at 10:22:53PM +0200, Jean-Michel Hautbois wrote:\n> In order to use a better grid, clamp the values calculated from the BDS\n> resolution to at least 2 times the minimum grid size.\n> The number of zones needed to have sufficiently relevant statistics is\n> now based on the region sizes and not on an arbitrary value.\n> Last, the default green gains where not properly used, it should be 8192\n> and not 4096 to have a multiplier of 1.0 on the R/G/B gains.\n\nSounds like this should be split in two patches.\n\n> The default CCM is adjusted for Surface Go2 only, and should eventually be\n> calculated in the CameraSensorHelper class.\n\nHow do you envision the CameraSensorHelper class calculating this ?\n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3.cpp | 12 ++++++++----\n> src/ipa/ipu3/ipu3_awb.cpp | 23 +++++++++++------------\n> src/ipa/ipu3/ipu3_awb.h | 1 +\n> 3 files changed, 20 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 4466391a..9a2def64 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -123,12 +123,16 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \tbdsGrid_ = {};\n> \n> \tfor (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {\n> -\t\tuint32_t width = std::min(kMaxCellWidthPerSet,\n> -\t\t\t\t\t bdsOutputSize.width >> widthShift);\n> +\t\tuint32_t width = std::clamp(bdsOutputSize.width >> widthShift,\n> +\t\t\t\t\t 2 * kAwbStatsSizeX,\n> +\t\t\t\t\t kMaxCellWidthPerSet);\n> +\n> \t\twidth = width << widthShift;\n> \t\tfor (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {\n> -\t\t\tint32_t height = std::min(kMaxCellHeightPerSet,\n> -\t\t\t\t\t\t bdsOutputSize.height >> heightShift);\n> +\t\t\tuint32_t height = std::clamp(bdsOutputSize.height >> heightShift,\n> +\t\t\t\t\t\t 2 * kAwbStatsSizeY,\n> +\t\t\t\t\t\t kMaxCellHeightPerSet);\n> +\n> \t\t\theight = height << heightShift;\n> \t\t\tuint32_t error = std::abs(static_cast(width - bdsOutputSize.width))\n> \t\t\t\t\t\t\t+ std::abs(static_cast(height - bdsOutputSize.height));\n> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> index a94935c5..a39536b0 100644\n> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> @@ -18,9 +18,6 @@ namespace ipa::ipu3 {\n> \n> LOG_DEFINE_CATEGORY(IPU3Awb)\n> \n> -static constexpr uint32_t kMinZonesCounted = 16;\n> -static constexpr uint32_t kMinGreenLevelInZone = 32;\n> -\n> /**\n> * \\struct IspStatsRegion\n> * \\brief RGB statistics for a given region\n> @@ -92,7 +89,7 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;\n> \n> /* Default settings for Bayer noise reduction replicated from the Kernel */\n> static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {\n> -\t.wb_gains = { 16, 16, 16, 16 },\n> +\t.wb_gains = { 8192, 8192, 8192, 8192 },\n\nDoes the kernel report 8192 as a default value ?\n\n> \t.wb_gains_thr = { 255, 255, 255, 255 },\n> \t.thr_coeffs = { 1700, 0, 31, 31, 0, 16 },\n> \t.thr_ctrl_shd = { 26, 26, 26, 26 },\n> @@ -130,7 +127,7 @@ static const struct ipu3_uapi_awb_config_s imguCssAwbDefaults = {\n> /* Default color correction matrix defined as an identity matrix */\n> static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {\n> \t8191, 0, 0, 0,\n> -\t0, 8191, 0, 0,\n> +\t0, 6000, 0, 0,\n\nthat's not an identify matrix anymore.\n\n> \t0, 0, 8191, 0\n> };\n> \n> @@ -166,6 +163,7 @@ IPU3Awb::IPU3Awb()\n> \tasyncResults_.greenGain = 1.0;\n> \tasyncResults_.redGain = 1.0;\n> \tasyncResults_.temperatureK = 4500;\n> +\tminZonesCounted_ = 0;\n> }\n> \n> IPU3Awb::~IPU3Awb()\n> @@ -241,7 +239,7 @@ void IPU3Awb::generateZones(std::vector &zones)\n> \tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n> \t\tRGB zone;\n> \t\tdouble counted = awbStats_[i].counted;\n> -\t\tif (counted >= kMinZonesCounted) {\n> +\t\tif (counted >= minZonesCounted_) {\n> \t\t\tzone.G = awbStats_[i].gSum / counted;\n> \t\t\tif (zone.G >= kMinGreenLevelInZone) {\n> \t\t\t\tzone.R = awbStats_[i].rSum / counted;\n> @@ -258,6 +256,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> \tuint32_t regionWidth = round(awbGrid_.width / static_cast(kAwbStatsSizeX));\n> \tuint32_t regionHeight = round(awbGrid_.height / static_cast(kAwbStatsSizeY));\n> \n> +\tminZonesCounted_ = ((regionWidth * regionHeight) * 4) / 5;\n> \t/*\n> \t * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is\n> \t * (awbGrid_.width x awbGrid_.height).\n> @@ -269,7 +268,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> \t\t\tuint32_t cellY = ((cellPosition / awbGrid_.width) / regionHeight) % kAwbStatsSizeY;\n> \n> \t\t\tuint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;\n> -\t\t\tcellPosition *= 8;\n> +\t\t\tcellPosition *= sizeof(Ipu3AwbCell);\n> \n> \t\t\t/* Cast the initial IPU3 structure to simplify the reading */\n> \t\t\tIpu3AwbCell *currentCell = reinterpret_cast(const_cast(&stats->awb_raw_buffer.meta_data[cellPosition]));\n> @@ -361,12 +360,12 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma)\n> \t/*\n> \t * Green gains should not be touched and considered 1.\n> \t * Default is 16, so do not change it at all.\n> -\t * 4096 is the value for a gain of 1.0\n> +\t * 8192 is the value for a gain of 1.0\n> \t */\n> -\tparams.acc_param.bnr.wb_gains.gr = 16;\n> -\tparams.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;\n> -\tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n> -\tparams.acc_param.bnr.wb_gains.gb = 16;\n> +\tparams.acc_param.bnr.wb_gains.gr = 8192;\n> +\tparams.acc_param.bnr.wb_gains.r = 8192 * asyncResults_.redGain;\n> +\tparams.acc_param.bnr.wb_gains.b = 8192 * asyncResults_.blueGain;\n> +\tparams.acc_param.bnr.wb_gains.gb = 8192;\n> \n> \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK\n> \t\t\t << \" and gamma calculated: \" << agcGamma;\n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> index 795e32e3..23865c21 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/ipu3_awb.h\n> @@ -49,6 +49,7 @@ private:\n> \tstd::vector zones_;\n> \tIspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n> \tAwbStatus asyncResults_;\n> +\tuint32_t minZonesCounted_;\n> };\n> \n> } /* namespace ipa::ipu3 */","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 2966ABD794\n\tfor ;\n\tThu, 8 Jul 2021 12:59:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9844B68513;\n\tThu, 8 Jul 2021 14:59:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8EB368506\n\tfor ;\n\tThu, 8 Jul 2021 14:59:41 +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 52146E7;\n\tThu, 8 Jul 2021 14:59:41 +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=\"OmNPvmC4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625749181;\n\tbh=NjNKvtnhoYPqXITWEtDpb5LZBYweUYSJZpCp5hbUZ+g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OmNPvmC4UKHLNYD4GIdTxnZUfDtxcN9EmMgicli1rslsrXTx8A7sML205h2EzdFgR\n\thrpP+sqPQDa1tXbTtm4YHN83lZn6f6Ntmwzexx2g+wq+P9vFPmIPAbbGGB2F3TVgq4\n\tqOTi1pbPYlOmq/2M1RebNr5t0QF1R4StgqLsJChw=","Date":"Thu, 8 Jul 2021 15:58:57 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210628202255.138874-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210628202255.138874-6-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210628202255.138874-6-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 5/7] ipa: ipu3: Improve AWB\n\tbehaviour","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\" "}}]