[{"id":29345,"web_url":"https://patchwork.libcamera.org/comment/29345/","msgid":"<20240425174318.GI28454@pendragon.ideasonboard.com>","date":"2024-04-25T17:43:18","subject":"Re: [PATCH 2/5] libcamera: software_isp: Honor black level in AWB","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Tue, Apr 23, 2024 at 08:19:57PM +0200, Milan Zamazal wrote:\n> The white balance computation didn't consider black level. This is\n> wrong because then the computed ratios are off when they are computed\n> from the whole brightness range rather than the sensor range.\n> \n> This patch adjusts white balance computation for the black level. There\n> is no need to change white balance application in debayering as this is\n> already applied after black level correction.\n\nMakes sense.\n\n> Exposure computation already subtracts black level, no changes are\n> needed there.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> src/ipa/simple/soft_simple.cpp | 43 ++++++++++++++++++++++++----------\n> 1 file changed, 31 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index b9fb58b5..30956a19 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -5,6 +5,8 @@\n> * soft_simple.cpp - Simple Software Image Processing Algorithm module\n> */\n> \n> +#include \n> +#include \n> #include \n> \n> #include \n> @@ -240,28 +242,45 @@ void IPASoftSimple::stop()\n> \n> void IPASoftSimple::processStats(const ControlList &sensorControls)\n> {\n> +\tSwIspStats::Histogram histogram = stats_->yHistogram;\n> +\tif (ignoreUpdates_ > 0)\n> +\t\tblackLevel_.update(histogram);\n> +\tconst uint8_t blackLevel = blackLevel_.get();\n> +\tparams_->blackLevel = blackLevel;\n> +\n> \t/*\n> \t * Calculate red and blue gains for AWB.\n> \t * Clamp max gain at 4.0, this also avoids 0 division.\n> \t */\n> -\tif (stats_->sumR_ <= stats_->sumG_ / 4)\n> -\t\tparams_->gainR = 1024;\n> -\telse\n> -\t\tparams_->gainR = 256 * stats_->sumG_ / stats_->sumR_;\n> +\t{\n\nIt's not that I dislike proper indentation, but maybe not just for the\nsake of it ? :-) Why do you enclose that block in curly braces,\nespecially given that you remove extra indentation in the next patch ?\n\nA comment here to explain why you need to subtract the black level would\nbe useful.\n\n> +\t\tconst uint64_t nPixels = std::accumulate(\n> +\t\t\thistogram.begin(), histogram.end(), 0);\n> +\t\tauto subtractBlackLevel = [blackLevel, nPixels](\n> +\t\t\t\t\t\t uint64_t sum, uint64_t div)\n> +\t\t\t-> uint64_t {\n> +\t\t\tuint64_t reducedSum = sum - blackLevel * nPixels / div;\n\nLooks good so far.\n\n> +\t\t\tuint64_t spreadSum = reducedSum * 256 / (256 - blackLevel);\n\nI'm not sure about this. Why do you want to \"spread\" the sum ?\n\n> +\t\t\treturn spreadSum;\n> +\t\t};\n> +\t\tconst uint64_t sumR = subtractBlackLevel(stats_->sumR_, 4);\n> +\t\tconst uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n> +\t\tconst uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n> +\n> +\t\tif (sumR <= sumG / 4)\n> +\t\t\tparams_->gainR = 1024;\n> +\t\telse\n> +\t\t\tparams_->gainR = 256 * sumG / sumR;\n> \n> -\tif (stats_->sumB_ <= stats_->sumG_ / 4)\n> -\t\tparams_->gainB = 1024;\n> -\telse\n> -\t\tparams_->gainB = 256 * stats_->sumG_ / stats_->sumB_;\n> +\t\tif (sumB <= sumG / 4)\n> +\t\t\tparams_->gainB = 1024;\n> +\t\telse\n> +\t\t\tparams_->gainB = 256 * sumG / sumB;\n> +\t}\n> \n> \t/* Green gain and gamma values are fixed */\n> \tparams_->gainG = 256;\n> \tparams_->gamma = 0.5;\n> \n> -\tif (ignoreUpdates_ > 0)\n> -\t\tblackLevel_.update(stats_->yHistogram);\n> -\tparams_->blackLevel = blackLevel_.get();\n> -\n> \tsetIspParams.emit();\n> \n> \t/* \\todo Switch to the libipa/algorithm.h API someday. */","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 B16B7C3220\n\tfor ;\n\tThu, 25 Apr 2024 17:43:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AAEE1633FD;\n\tThu, 25 Apr 2024 19:43:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4188461B43\n\tfor ;\n\tThu, 25 Apr 2024 19:43:26 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4CE7382A;\n\tThu, 25 Apr 2024 19:42:33 +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=\"mon3CeMg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1714066953;\n\tbh=BGfaMM8jlQVCV5He9EgOX+Hj3Ud3eiPQPfCPPrRGiBU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mon3CeMgjfd5glzogaYn8ZZ7DGgRWgaBT3Eqp+wsxNwQkFLmel0HAj6BEBGfRpHb3\n\t+txsED0wq3IACTwuO8XZagCNm8PI7Jm2kSTk9CAN+C5xnwFmve2/V6hbfWX7EgNANG\n\tWs0Q7Gap11vtsymlX9SQVX6+97lJScwVHVxhaXJg=","Date":"Thu, 25 Apr 2024 20:43:18 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/5] libcamera: software_isp: Honor black level in AWB","Message-ID":"<20240425174318.GI28454@pendragon.ideasonboard.com>","References":"<20240423182000.1527425-1-mzamazal@redhat.com>\n\t<20240423182000.1527425-3-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240423182000.1527425-3-mzamazal@redhat.com>","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":29348,"web_url":"https://patchwork.libcamera.org/comment/29348/","msgid":"<87wmokz9xw.fsf@redhat.com>","date":"2024-04-26T10:37:31","subject":"Re: [PATCH 2/5] libcamera: software_isp: Honor black level in AWB","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n\nHi Laurent,\n\nthank you for the reviews.\n\n> On Tue, Apr 23, 2024 at 08:19:57PM +0200, Milan Zamazal wrote:\n>> The white balance computation didn't consider black level. This is\n>> wrong because then the computed ratios are off when they are computed\n>> from the whole brightness range rather than the sensor range.\n>> \n>> This patch adjusts white balance computation for the black level. There\n>> is no need to change white balance application in debayering as this is\n>> already applied after black level correction.\n>\n> Makes sense.\n>\n>> Exposure computation already subtracts black level, no changes are\n>> needed there.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/ipa/simple/soft_simple.cpp | 43 ++++++++++++++++++++++++----------\n>> 1 file changed, 31 insertions(+), 12 deletions(-)\n>> \n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index b9fb58b5..30956a19 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -5,6 +5,8 @@\n>> * soft_simple.cpp - Simple Software Image Processing Algorithm module\n>> */\n>> \n>> +#include \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -240,28 +242,45 @@ void IPASoftSimple::stop()\n>> \n>> void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> {\n>> +\tSwIspStats::Histogram histogram = stats_->yHistogram;\n>> +\tif (ignoreUpdates_ > 0)\n>> +\t\tblackLevel_.update(histogram);\n>> +\tconst uint8_t blackLevel = blackLevel_.get();\n>> +\tparams_->blackLevel = blackLevel;\n>> +\n>> \t/*\n>> \t * Calculate red and blue gains for AWB.\n>> \t * Clamp max gain at 4.0, this also avoids 0 division.\n>> \t */\n>> -\tif (stats_->sumR_ <= stats_->sumG_ / 4)\n>> -\t\tparams_->gainR = 1024;\n>> -\telse\n>> -\t\tparams_->gainR = 256 * stats_->sumG_ / stats_->sumR_;\n>> +\t{\n>\n> It's not that I dislike proper indentation, but maybe not just for the\n> sake of it ? :-) Why do you enclose that block in curly braces,\n> especially given that you remove extra indentation in the next patch ?\n\nThis is to limit the scope of local variables to the relevant region (which is\nextended in the next patch). Functionally, it's just a matter of style so I can\nremove the braces.\n\n> A comment here to explain why you need to subtract the black level would\n> be useful.\n\nOK, will add one.\n\n>> +\t\tconst uint64_t nPixels = std::accumulate(\n>> +\t\t\thistogram.begin(), histogram.end(), 0);\n>> +\t\tauto subtractBlackLevel = [blackLevel, nPixels](\n>> +\t\t\t\t\t\t uint64_t sum, uint64_t div)\n>> +\t\t\t-> uint64_t {\n>> +\t\t\tuint64_t reducedSum = sum - blackLevel * nPixels / div;\n>\n> Looks good so far.\n>\n>> + uint64_t spreadSum = reducedSum * 256 / (256 - blackLevel);\n>\n> I'm not sure about this. Why do you want to \"spread\" the sum ?\n\nRight, the multiplication is meaningless here, I'll remove it.\n\n>> +\t\t\treturn spreadSum;\n>> +\t\t};\n>> +\t\tconst uint64_t sumR = subtractBlackLevel(stats_->sumR_, 4);\n>> +\t\tconst uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n>> +\t\tconst uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n>> +\n>> +\t\tif (sumR <= sumG / 4)\n>> +\t\t\tparams_->gainR = 1024;\n>> +\t\telse\n>> +\t\t\tparams_->gainR = 256 * sumG / sumR;\n>> \n>> -\tif (stats_->sumB_ <= stats_->sumG_ / 4)\n>> -\t\tparams_->gainB = 1024;\n>> -\telse\n>> -\t\tparams_->gainB = 256 * stats_->sumG_ / stats_->sumB_;\n>> +\t\tif (sumB <= sumG / 4)\n>> +\t\t\tparams_->gainB = 1024;\n>> +\t\telse\n>> +\t\t\tparams_->gainB = 256 * sumG / sumB;\n>> +\t}\n>> \n>> \t/* Green gain and gamma values are fixed */\n>> \tparams_->gainG = 256;\n>> \tparams_->gamma = 0.5;\n>> \n>> -\tif (ignoreUpdates_ > 0)\n>> -\t\tblackLevel_.update(stats_->yHistogram);\n>> -\tparams_->blackLevel = blackLevel_.get();\n>> -\n>> \tsetIspParams.emit();\n>> \n>> \t/* \\todo Switch to the libipa/algorithm.h API someday. */","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 01901BE08B\n\tfor ;\n\tFri, 26 Apr 2024 10:37:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0AB4A633FD;\n\tFri, 26 Apr 2024 12:37:41 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B4E6D61A94\n\tfor ;\n\tFri, 26 Apr 2024 12:37:38 +0200 (CEST)","from mail-ej1-f72.google.com (mail-ej1-f72.google.com\n\t[209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-623-nPe7JBxVMGS4y0ACmnPv9A-1; Fri, 26 Apr 2024 06:37:36 -0400","by mail-ej1-f72.google.com with SMTP id\n\ta640c23a62f3a-a55be723396so119575866b.0\n\tfor ;\n\tFri, 26 Apr 2024 03:37:36 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tm24-20020a17090607d800b00a524216fe78sm10504432ejc.64.2024.04.26.03.37.32\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 26 Apr 2024 03:37:32 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Al5jpkD9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1714127857;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=Ld4j39Ps1f6syGotSRX2nMiwpM9O0HVTtMLPsqfUer4=;\n\tb=Al5jpkD9WHL6r4NIPv8IfWSXOikA+xA6Glqut8eyF/u6OCkN0rPkJkh1IVCTAR7AJdprsM\n\tisrnBA1xtbTTjr5HMSExkUcxlDbJQlnzLN2dXO/XjU1R1jlKkwjjDSnrf+X/ijkMHP0Wbr\n\tH3UKO0E8pFXhs4Ja0foPfhoHxw3Njuo=","X-MC-Unique":"nPe7JBxVMGS4y0ACmnPv9A-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1714127854; x=1714732654;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=Ld4j39Ps1f6syGotSRX2nMiwpM9O0HVTtMLPsqfUer4=;\n\tb=qJpsgKKl8ct6FdXcfMq+fmrhgUMcjy1Bp4Hu51QH+pZxalMZUVaaINTyPMrufUbd9I\n\tW6WwCIeDO1nORHWMIfxej31jdijyT0kNLtp3E8HMwsRXl4LD52CKLGkGpnsKjf2oZh6Z\n\t4BFOTQcB+E5hPPWMiVTkAI+9WIy3qbHLREWS/lp7LWw/qruoEI4wzL1GkfROrAVXSV15\n\teaWE2q/slN82JgqiCy+GQX5QafmTY/8trFzDKSpjccVCQVK617O7SaQsOkvUMXtPQQ2J\n\tp5bDyIQa+JZzWLyrJ8SK8PJCLRZhthYkttDwvl/F0fdYqDEyEsOpdCUmEUyfDOkcR0bg\n\tMwvg==","X-Gm-Message-State":"AOJu0YxYAoMG0mi0Rfr9Nwzpj4+9ofhY+krNJPkHtO5YPebhWXyPJMas\n\tUdBFSji2WIOPgaAMSehP5vchXWy0lLxjBh7A+PJ4pRbIGKxCKTkwqPbBsnzOO3jI+eFx5YGx147\n\tfXAZqc/Kaas2kMen3D4gXGh13gPI3VmSQ1TQP+1aND4Puol65VsfOTp8wy+fyQ6NmNCpazJc0sx\n\tXkXr1YWtfDZ0KcDlKjPjrvUl73ukqJwHfrCrV21bVVQ1Q6Yewt94tReP8=","X-Received":["by 2002:a17:906:3d2a:b0:a58:bb3e:9376 with SMTP id\n\tl10-20020a1709063d2a00b00a58bb3e9376mr1604950ejf.61.1714127853952; \n\tFri, 26 Apr 2024 03:37:33 -0700 (PDT)","by 2002:a17:906:3d2a:b0:a58:bb3e:9376 with SMTP id\n\tl10-20020a1709063d2a00b00a58bb3e9376mr1604929ejf.61.1714127853301; \n\tFri, 26 Apr 2024 03:37:33 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHtuXCdIegOcg45lEacqoFf9Dm/J6WYnHbUxY+yabwbzpINnFmXcgfy7eN50bFY1iDBLThONA==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/5] libcamera: software_isp: Honor black level in AWB","In-Reply-To":"<20240425174318.GI28454@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Thu, 25 Apr 2024 20:43:18 +0300\")","References":"<20240423182000.1527425-1-mzamazal@redhat.com>\n\t<20240423182000.1527425-3-mzamazal@redhat.com>\n\t<20240425174318.GI28454@pendragon.ideasonboard.com>","Date":"Fri, 26 Apr 2024 12:37:31 +0200","Message-ID":"<87wmokz9xw.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":29360,"web_url":"https://patchwork.libcamera.org/comment/29360/","msgid":"<20240428231541.GC4950@pendragon.ideasonboard.com>","date":"2024-04-28T23:15:41","subject":"Re: [PATCH 2/5] libcamera: software_isp: Honor black level in AWB","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Apr 26, 2024 at 12:37:31PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> \n> > Hi Milan,\n> >\n> > Thank you for the patch.\n> \n> Hi Laurent,\n> \n> thank you for the reviews.\n> \n> > On Tue, Apr 23, 2024 at 08:19:57PM +0200, Milan Zamazal wrote:\n> >> The white balance computation didn't consider black level. This is\n> >> wrong because then the computed ratios are off when they are computed\n> >> from the whole brightness range rather than the sensor range.\n> >> \n> >> This patch adjusts white balance computation for the black level. There\n> >> is no need to change white balance application in debayering as this is\n> >> already applied after black level correction.\n> >\n> > Makes sense.\n> >\n> >> Exposure computation already subtracts black level, no changes are\n> >> needed there.\n> >> \n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> src/ipa/simple/soft_simple.cpp | 43 ++++++++++++++++++++++++----------\n> >> 1 file changed, 31 insertions(+), 12 deletions(-)\n> >> \n> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> >> index b9fb58b5..30956a19 100644\n> >> --- a/src/ipa/simple/soft_simple.cpp\n> >> +++ b/src/ipa/simple/soft_simple.cpp\n> >> @@ -5,6 +5,8 @@\n> >> * soft_simple.cpp - Simple Software Image Processing Algorithm module\n> >> */\n> >> \n> >> +#include \n> >> +#include \n> >> #include \n> >> \n> >> #include \n> >> @@ -240,28 +242,45 @@ void IPASoftSimple::stop()\n> >> \n> >> void IPASoftSimple::processStats(const ControlList &sensorControls)\n> >> {\n> >> +\tSwIspStats::Histogram histogram = stats_->yHistogram;\n> >> +\tif (ignoreUpdates_ > 0)\n> >> +\t\tblackLevel_.update(histogram);\n> >> +\tconst uint8_t blackLevel = blackLevel_.get();\n> >> +\tparams_->blackLevel = blackLevel;\n> >> +\n> >> \t/*\n> >> \t * Calculate red and blue gains for AWB.\n> >> \t * Clamp max gain at 4.0, this also avoids 0 division.\n> >> \t */\n> >> -\tif (stats_->sumR_ <= stats_->sumG_ / 4)\n> >> -\t\tparams_->gainR = 1024;\n> >> -\telse\n> >> -\t\tparams_->gainR = 256 * stats_->sumG_ / stats_->sumR_;\n> >> +\t{\n> >\n> > It's not that I dislike proper indentation, but maybe not just for the\n> > sake of it ? :-) Why do you enclose that block in curly braces,\n> > especially given that you remove extra indentation in the next patch ?\n> \n> This is to limit the scope of local variables to the relevant region (which is\n> extended in the next patch).\n\nThe next patch removes the nested scope, so I don't really see why it's\nneeded here.\n\nI'm fine restricing scope of variables when there's a technical reason\nto do so, for instance to use pattenrs RAII for locks. That doesn't seem\nto be the case here, and the code compiles fine without the nested\nscope.\n\n> Functionally, it's just a matter of style so I can\n> remove the braces.\n> \n> > A comment here to explain why you need to subtract the black level would\n> > be useful.\n> \n> OK, will add one.\n> \n> >> +\t\tconst uint64_t nPixels = std::accumulate(\n> >> +\t\t\thistogram.begin(), histogram.end(), 0);\n> >> +\t\tauto subtractBlackLevel = [blackLevel, nPixels](\n> >> +\t\t\t\t\t\t uint64_t sum, uint64_t div)\n> >> +\t\t\t-> uint64_t {\n> >> +\t\t\tuint64_t reducedSum = sum - blackLevel * nPixels / div;\n> >\n> > Looks good so far.\n> >\n> >> + uint64_t spreadSum = reducedSum * 256 / (256 - blackLevel);\n> >\n> > I'm not sure about this. Why do you want to \"spread\" the sum ?\n> \n> Right, the multiplication is meaningless here, I'll remove it.\n> \n> >> +\t\t\treturn spreadSum;\n> >> +\t\t};\n> >> +\t\tconst uint64_t sumR = subtractBlackLevel(stats_->sumR_, 4);\n> >> +\t\tconst uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n> >> +\t\tconst uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n> >> +\n> >> +\t\tif (sumR <= sumG / 4)\n> >> +\t\t\tparams_->gainR = 1024;\n> >> +\t\telse\n> >> +\t\t\tparams_->gainR = 256 * sumG / sumR;\n> >> \n> >> -\tif (stats_->sumB_ <= stats_->sumG_ / 4)\n> >> -\t\tparams_->gainB = 1024;\n> >> -\telse\n> >> -\t\tparams_->gainB = 256 * stats_->sumG_ / stats_->sumB_;\n> >> +\t\tif (sumB <= sumG / 4)\n> >> +\t\t\tparams_->gainB = 1024;\n> >> +\t\telse\n> >> +\t\t\tparams_->gainB = 256 * sumG / sumB;\n> >> +\t}\n> >> \n> >> \t/* Green gain and gamma values are fixed */\n> >> \tparams_->gainG = 256;\n> >> \tparams_->gamma = 0.5;\n> >> \n> >> -\tif (ignoreUpdates_ > 0)\n> >> -\t\tblackLevel_.update(stats_->yHistogram);\n> >> -\tparams_->blackLevel = blackLevel_.get();\n> >> -\n> >> \tsetIspParams.emit();\n> >> \n> >> \t/* \\todo Switch to the libipa/algorithm.h API someday. */","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 7E87DBE08B\n\tfor ;\n\tSun, 28 Apr 2024 23:15:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 002D36341B;\n\tMon, 29 Apr 2024 01:15:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F02E63416\n\tfor ;\n\tMon, 29 Apr 2024 01:15:48 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [109.130.69.237])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 370DC720;\n\tMon, 29 Apr 2024 01:14:53 +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=\"VpY97jE+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1714346093;\n\tbh=ieG16kM9EnoTCOOU467Hmz3xezbZ1l/mMaF1gzWf1Bk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VpY97jE+iVRbRYcKZC4cGXCnnuI5jWXCPu//2f+qDN04IajGGU8n3KxP+O6t9al+R\n\tSsDc3l2uLeQV8IJwS3303OVop+FYreKdR0fx5NcLNhUFbb1bgIoz7TeffHSKkpOk2R\n\t2MYiTIJnWpjQUVIbmYza4AW3uiYY+T45v56pRN5U=","Date":"Mon, 29 Apr 2024 02:15:41 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/5] libcamera: software_isp: Honor black level in AWB","Message-ID":"<20240428231541.GC4950@pendragon.ideasonboard.com>","References":"<20240423182000.1527425-1-mzamazal@redhat.com>\n\t<20240423182000.1527425-3-mzamazal@redhat.com>\n\t<20240425174318.GI28454@pendragon.ideasonboard.com>\n\t<87wmokz9xw.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87wmokz9xw.fsf@redhat.com>","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\" "}}]