[{"id":29641,"web_url":"https://patchwork.libcamera.org/comment/29641/","msgid":"<20240529000016.GA19014@pendragon.ideasonboard.com>","date":"2024-05-29T00:00:16","subject":"Re: [PATCH v4 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, May 28, 2024 at 06:11:23PM +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> Exposure computation already subtracts black level, no changes are\n> needed there.\n> \n> Signed-off-by: Milan Zamazal \n> Reviewed-by: Andrei Konovalov \n> ---\n> src/ipa/simple/soft_simple.cpp | 36 ++++++++++++++++++++++------------\n> 1 file changed, 23 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index a5bb2bbf..722aac83 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -5,6 +5,8 @@\n> * Simple Software Image Processing Algorithm module\n> */\n> \n> +#include \n> +#include \n> #include \n> \n> #include \n> @@ -240,28 +242,36 @@ 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\nThese two lines should be moved to [*] below.\n\n> +\t * Black level must be subtracted to get the correct AWB ratios,\n> +\t * from the sensor range.\n\nThanks for adding a comment, but could you explain *why* this is needed\n?\n\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> -\n> -\tif (stats_->sumB_ <= stats_->sumG_ / 4)\n> -\t\tparams_->gainB = 1024;\n> -\telse\n> -\t\tparams_->gainB = 256 * stats_->sumG_ / stats_->sumB_;\n> +\tconst uint64_t nPixels = std::accumulate(\n> +\t\thistogram.begin(), histogram.end(), 0);\n> +\tauto subtractBlackLevel = [blackLevel, nPixels](\n> +\t\t\t\t\t uint64_t sum, uint64_t div)\n\nI'd avoid the line break between the previous two lines, it makes the\ncode harder to read.\n\n> +\t\t-> uint64_t {\n> +\t\treturn sum - blackLevel * nPixels / div;\n> +\t};\n> +\tconst uint64_t sumR = subtractBlackLevel(stats_->sumR_, 4);\n> +\tconst uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n> +\tconst uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n\nWouldn't the following be both simpler and more readable ?\n\n\tconst uint64_t offset = blackLevel * nPixels;\n\tconst uint64_t sumR = stats_->sumR_ - offset / 4;\n\tconst uint64_t sumG = stats_->sumG_ - offset / 2;\n\tconst uint64_t sumB = stats_->sumB_ - offset / 4;\n\n[*]\n\nWith these small issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\tparams_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> +\tparams_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\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 56042BDE6B\n\tfor ;\n\tWed, 29 May 2024 00:00:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D77E634B0;\n\tWed, 29 May 2024 02:00:30 +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 0FBEE6349B\n\tfor ;\n\tWed, 29 May 2024 02:00:29 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4BD0D397;\n\tWed, 29 May 2024 02:00:25 +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=\"UuZQLnjK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716940825;\n\tbh=SMpqgkNHvPtK2low+MsGaEy1a2hU4nApptO1ejC1NCI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UuZQLnjKuhc4hrWKEGt0CJjK1YR8aBe734kN0O4x+2JQtk7X9jK30Lr3/QfXIdWbQ\n\txr3SW4czLQK8D9gPmTz8+DxXW2WMRiNO4W9jPLaK1Kk8lR6Zy4tPKRj+mD9RaLuFG4\n\tmdvwrnfpgBz+ZUMF+rQpIi1WBglBCCYCOIzLpoJQ=","Date":"Wed, 29 May 2024 03:00:16 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tAndrei Konovalov ","Subject":"Re: [PATCH v4 2/5] libcamera: software_isp: Honor black level in AWB","Message-ID":"<20240529000016.GA19014@pendragon.ideasonboard.com>","References":"<20240528161126.35119-1-mzamazal@redhat.com>\n\t<20240528161126.35119-3-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240528161126.35119-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":29676,"web_url":"https://patchwork.libcamera.org/comment/29676/","msgid":"<874jafqbx4.fsf@redhat.com>","date":"2024-05-30T20:31:35","subject":"Re: [PATCH v4 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 review.\n\n> On Tue, May 28, 2024 at 06:11:23PM +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>> Exposure computation already subtracts black level, no changes are\n>> needed there.\n>> \n>> Signed-off-by: Milan Zamazal \n>> Reviewed-by: Andrei Konovalov \n>> ---\n>> src/ipa/simple/soft_simple.cpp | 36 ++++++++++++++++++++++------------\n>> 1 file changed, 23 insertions(+), 13 deletions(-)\n>> \n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index a5bb2bbf..722aac83 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -5,6 +5,8 @@\n>> * Simple Software Image Processing Algorithm module\n>> */\n>> \n>> +#include \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -240,28 +242,36 @@ 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>\n> These two lines should be moved to [*] below.\n\nOK.\n\n>> +\t * Black level must be subtracted to get the correct AWB ratios,\n>> +\t * from the sensor range.\n>\n> Thanks for adding a comment, but could you explain *why* this is needed\n> ?\n\nOK.\n\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>> -\n>> -\tif (stats_->sumB_ <= stats_->sumG_ / 4)\n>> -\t\tparams_->gainB = 1024;\n>> -\telse\n>> -\t\tparams_->gainB = 256 * stats_->sumG_ / stats_->sumB_;\n>> +\tconst uint64_t nPixels = std::accumulate(\n>> +\t\thistogram.begin(), histogram.end(), 0);\n>> +\tauto subtractBlackLevel = [blackLevel, nPixels](\n>> +\t\t\t\t\t uint64_t sum, uint64_t div)\n>\n> I'd avoid the line break between the previous two lines, it makes the\n> code harder to read.\n>\n>> +\t\t-> uint64_t {\n>> +\t\treturn sum - blackLevel * nPixels / div;\n>> +\t};\n>> +\tconst uint64_t sumR = subtractBlackLevel(stats_->sumR_, 4);\n>> +\tconst uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n>> +\tconst uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n>\n> Wouldn't the following be both simpler and more readable ?\n\nYes, will change it.\n\n> \tconst uint64_t offset = blackLevel * nPixels;\n> \tconst uint64_t sumR = stats_->sumR_ - offset / 4;\n> \tconst uint64_t sumG = stats_->sumG_ - offset / 2;\n> \tconst uint64_t sumB = stats_->sumB_ - offset / 4;\n>\n> [*]\n>\n> With these small issues addressed,\n>\n> Reviewed-by: Laurent Pinchart \n>\n>> +\n>> +\tparams_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n>> +\tparams_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\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 3ED59BDE6B\n\tfor ;\n\tThu, 30 May 2024 20:31:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56F78634B6;\n\tThu, 30 May 2024 22:31:43 +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 892F661A43\n\tfor ;\n\tThu, 30 May 2024 22:31:41 +0200 (CEST)","from mail-lf1-f71.google.com (mail-lf1-f71.google.com\n\t[209.85.167.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-327-wV80F4mKO9SR1Xo7ciHR7A-1; Thu, 30 May 2024 16:31:38 -0400","by mail-lf1-f71.google.com with SMTP id\n\t2adb3069b0e04-52b7e876973so805084e87.1\n\tfor ;\n\tThu, 30 May 2024 13:31:38 -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\ta640c23a62f3a-a67e73f8192sm12452166b.51.2024.05.30.13.31.35\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 30 May 2024 13:31:36 -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=\"XtPGqIBb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1717101100;\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=HVSdac3SDXc0WTNHKqa3ib9JSQLao95Yr8FrMEEzDQA=;\n\tb=XtPGqIBbowfRzkV9NrSAZOumRoslcsB3cd/+9LfpojUacE+ayLYDGMQQdBUCjH37Eqnszo\n\tD8FiOB0matXhnitz2w2UfC/xZPvQ3vGuKLBFcXgKI/V+0dyKl1z8aUzj5oOAygamoXGJeh\n\ty/MgNPC9jkwRgP5VXBtZN2ThsECio/I=","X-MC-Unique":"wV80F4mKO9SR1Xo7ciHR7A-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1717101097; x=1717705897;\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=HVSdac3SDXc0WTNHKqa3ib9JSQLao95Yr8FrMEEzDQA=;\n\tb=qnpsaXat/a0TidFolZopNTQJnBMmV1H/uAvZdwtjLRtCNat1vfLCB8CzTqWBLptPbz\n\tSfTp7v66r47iJo50VAvoPjVf0GNC84SuspnrwXe2pG/vyEFxhl8njatJRIp3m2po8Sgr\n\tljDxqHa0peLY9brxk2mQQifT22DUC7fIaCHrpe9fKQMLnoR2EDmcx5DtU42KCd1XOK09\n\t0RJ8Xq3p8jRwQI6i+cDLeGMCn/dk+gFfd755Oc4TT2a2GyWEDh3Cl3RvIQDHW7D4n/V+\n\twkiVpxXfobUMNBnoeexNB7WOePSetcCcxlyaTt4/zOuIv6cbMazX37a04gOw29NRZlgA\n\to/kw==","X-Gm-Message-State":"AOJu0YzJblWSXhygQB1MsMfqb8cKTrETJ0XvGkXoGETIetEFoF5l/sbW\n\trlkxKDY/in74XwFColKxS2+TdvOVxLO+ozJ2HnEB/Q7oCR/f8b3yAGpcKyFg4QOYIOuRgzxjfXc\n\tifByw0Y4EkVyi6KNrvwgP5y9Qm4HZpEz+f60kXWnXKGnv2sw/XBV+LE9/2zpKpT0OkaE908s=","X-Received":["by 2002:ac2:5338:0:b0:525:32aa:443e with SMTP id\n\t2adb3069b0e04-52b7d4282demr2573602e87.17.1717101097387; \n\tThu, 30 May 2024 13:31:37 -0700 (PDT)","by 2002:ac2:5338:0:b0:525:32aa:443e with SMTP id\n\t2adb3069b0e04-52b7d4282demr2573585e87.17.1717101096889; \n\tThu, 30 May 2024 13:31:36 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGJLzrcBMhe2CJPRIRzxwX9MvV4WXt3SAjn7ogNFY5tUaVklEauf5FYVxUpoMlFFmpryvWl9w==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Andrei Konovalov\n\t","Subject":"Re: [PATCH v4 2/5] libcamera: software_isp: Honor black level in AWB","In-Reply-To":"<20240529000016.GA19014@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 29 May 2024 03:00:16 +0300\")","References":"<20240528161126.35119-1-mzamazal@redhat.com>\n\t<20240528161126.35119-3-mzamazal@redhat.com>\n\t<20240529000016.GA19014@pendragon.ideasonboard.com>","Date":"Thu, 30 May 2024 22:31:35 +0200","Message-ID":"<874jafqbx4.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\" "}}]