[{"id":26372,"web_url":"https://patchwork.libcamera.org/comment/26372/","msgid":"<167508546933.42371.6739065703922714320@Monstersaurus>","date":"2023-01-30T13:31:09","subject":"Re: [libcamera-devel] [PATCH v3 5/5] ipa: raspberrypi: Normalise\n\tregion sums to 16-bits","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:36)\n> The VC4 ISP uses a pipeline bit-depth of 13-bits. The AGC algorithm needs to\n> know this bit-depth when computing the Y value for the image.\n> \n> Instead of hardcoding the VC4 bit-depth in the AGC source code, normalise all\n> region sums to 16-bits when filling the Statistics structure. AWB and ALSC are\n> agnostic about pipeline depth, so do not need changing.\n\npipeline bit-depth to not confuse pipeline depth with the number of\nbuffers as this term has been used for previously?\n\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++------\n> src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++------\n> 2 files changed, 14 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 868c30f03d66..7ec8292d32ff 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n> \n> #define NAME \"rpi.agc\"\n> \n> -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */\n> -\n> int AgcMeteringMode::read(const libcamera::YamlObject ¶ms)\n> {\n> const YamlObject &yamlWeights = params[\"weights\"];\n> @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;\n> for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) {\n> auto ®ion = stats->agcRegions.get(i);\n> - double rAcc = std::min(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> - double gAcc = std::min(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> - double bAcc = std::min(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> + double rAcc = std::min(region.val.rSum * gain, ((1 << 16) - 1) * region.counted);\n> + double gAcc = std::min(region.val.gSum * gain, ((1 << 16) - 1) * region.counted);\n> + double bAcc = std::min(region.val.bSum * gain, ((1 << 16) - 1) * region.counted);\n\n1 << 16 - 1 now looks a bit 'magic', and it may not be clear to a\nreader what it's refering to?\n\nBefore it referenced PipelineBits so it was clearer. I expect this\n ((1 << 16) - 1)\n\ncould be a constexpr somewhere at the top of the file, or a helper to\nwrap the scaling of region.counted?\n\n\nHrm ... it 'was' already a constexpr at the top...\nIs there a reason you chose to hardcode the value here instead of update\n(/and or rename) the existing PipelineBits ?\n\nAside from that:\n\n\nReviewed-by: Kieran Bingham \n\n\n> rSum += rAcc * weights[i];\n> gSum += gAcc * weights[i];\n> bSum += bAcc * weights[i];\n> @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> double ySum = rSum * awb.gainR * .299 +\n> gSum * awb.gainG * .587 +\n> bSum * awb.gainB * .114;\n> - return ySum / pixelSum / (1 << PipelineBits);\n> + return ySum / pixelSum / (1 << 16);\n> }\n> \n> /*\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index cff079bbafb3..50fdeb4f0478 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> /* RGB histograms are not used, so do not populate them. */\n> statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));\n> \n> + /*\n> + * All region sums are based on a 13-bit pipeline bit-depth. Normalise\n> + * this to 16-bits for the AGC/AWB/ALSC algorithms.\n> + */\n> statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });\n> for (i = 0; i < statistics->awbRegions.numRegions(); i++)\n> - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum,\n> - stats->awb_stats[i].g_sum,\n> - stats->awb_stats[i].b_sum },\n> + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3,\n> + stats->awb_stats[i].g_sum << 3,\n> + stats->awb_stats[i].b_sum << 3 },\n> stats->awb_stats[i].counted,\n> stats->awb_stats[i].notcounted });\n> \n> @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> */\n> statistics->agcRegions.init(15);\n> for (i = 0; i < statistics->agcRegions.numRegions(); i++)\n> - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum,\n> - stats->agc_stats[i].g_sum,\n> - stats->agc_stats[i].b_sum },\n> + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3,\n> + stats->agc_stats[i].g_sum << 3,\n> + stats->agc_stats[i].b_sum << 3 },\n> stats->agc_stats[i].counted,\n> stats->awb_stats[i].notcounted });\n> \n> -- \n> 2.25.1\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 B686ABEFBE\n\tfor ;\n\tMon, 30 Jan 2023 13:31:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F8D8625E4;\n\tMon, 30 Jan 2023 14:31:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C444960482\n\tfor ;\n\tMon, 30 Jan 2023 14:31:11 +0100 (CET)","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 500478B8;\n\tMon, 30 Jan 2023 14:31:11 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675085473;\n\tbh=BxSunTVlRaoAbPwTyZH0k37gdft5oVDPTIlDXwzXUIg=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=HhXvYIiFqqSnHgvrZ6nXkW3LbFEhxAgw1i3LPYyvYekgLElSxc8jdK33aBMNcD5R7\n\tu8jfXpMtujlXuXISfnuxQ/ncNvNjI30hXwdUnJ1pC5c85znVY175gDEB/2fIf29xYI\n\tGCuqC2Eelzn/aFlyqz7JXuPjgjR/Eb7RDc0lLQmdus+10NFe8WBrJS2wr9M8AxspjI\n\toODw0NZPkupuwoayB6xd5V4HUy2n9R/YuD8QXrbL0qEx/Iubn1/tp6oi+QXO4aaK9n\n\tC89jRU4PgSLqs/xGc5Wl0yvM/lwetPKhUGdTw642Boo69/zPtibKsitPFexoV+HZLR\n\t3yxFoE1qMfxwA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675085471;\n\tbh=BxSunTVlRaoAbPwTyZH0k37gdft5oVDPTIlDXwzXUIg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=NC5covxiBIr+Cl5W+h2FmjSOK2oNBm3w/Tio1Z1RK49OKfruADPT5qtmQpJ4tVFcW\n\tAuGaGRpvt2t8+mtd102CO7T+mXjJvW88i0yh8ts6Dw3N82uiiCHRDuQ4C2tHsrbUZ6\n\tPEHkwaklqvkHuvKwJwehlLEwKWYh/HZvTjKosYN0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NC5covxi\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221213114836.15473-6-naush@raspberrypi.com>","References":"<20221213114836.15473-1-naush@raspberrypi.com>\n\t<20221213114836.15473-6-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 30 Jan 2023 13:31:09 +0000","Message-ID":"<167508546933.42371.6739065703922714320@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] ipa: raspberrypi: Normalise\n\tregion sums to 16-bits","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26388,"web_url":"https://patchwork.libcamera.org/comment/26388/","msgid":"","date":"2023-01-31T12:29:07","subject":"Re: [libcamera-devel] [PATCH v3 5/5] ipa: raspberrypi: Normalise\n\tregion sums to 16-bits","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Mon, 30 Jan 2023 at 13:31, Kieran Bingham\n wrote:\n>\n> Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:36)\n> > The VC4 ISP uses a pipeline bit-depth of 13-bits. The AGC algorithm needs to\n> > know this bit-depth when computing the Y value for the image.\n> >\n> > Instead of hardcoding the VC4 bit-depth in the AGC source code, normalise all\n> > region sums to 16-bits when filling the Statistics structure. AWB and ALSC are\n> > agnostic about pipeline depth, so do not need changing.\n>\n> pipeline bit-depth to not confuse pipeline depth with the number of\n> buffers as this term has been used for previously?\n>\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++------\n> > src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++------\n> > 2 files changed, 14 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index 868c30f03d66..7ec8292d32ff 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n> >\n> > #define NAME \"rpi.agc\"\n> >\n> > -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */\n> > -\n> > int AgcMeteringMode::read(const libcamera::YamlObject ¶ms)\n> > {\n> > const YamlObject &yamlWeights = params[\"weights\"];\n> > @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;\n> > for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) {\n> > auto ®ion = stats->agcRegions.get(i);\n> > - double rAcc = std::min(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > - double gAcc = std::min(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > - double bAcc = std::min(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > + double rAcc = std::min(region.val.rSum * gain, ((1 << 16) - 1) * region.counted);\n> > + double gAcc = std::min(region.val.gSum * gain, ((1 << 16) - 1) * region.counted);\n> > + double bAcc = std::min(region.val.bSum * gain, ((1 << 16) - 1) * region.counted);\n>\n> 1 << 16 - 1 now looks a bit 'magic', and it may not be clear to a\n> reader what it's refering to?\n>\n> Before it referenced PipelineBits so it was clearer. I expect this\n> ((1 << 16) - 1)\n>\n> could be a constexpr somewhere at the top of the file, or a helper to\n> wrap the scaling of region.counted?\n>\n>\n> Hrm ... it 'was' already a constexpr at the top...\n> Is there a reason you chose to hardcode the value here instead of update\n> (/and or rename) the existing PipelineBits ?\n\nSince the generalised statistics are now normalised to 16-bits,\nperhaps what is needed is to have a constexpr for (1 << 16) - 1 to be\n(possibly) defined in the statistics structure to indicate the\nnormalisation used?\n\nNaush\n\n>\n> Aside from that:\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > rSum += rAcc * weights[i];\n> > gSum += gAcc * weights[i];\n> > bSum += bAcc * weights[i];\n> > @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> > double ySum = rSum * awb.gainR * .299 +\n> > gSum * awb.gainG * .587 +\n> > bSum * awb.gainB * .114;\n> > - return ySum / pixelSum / (1 << PipelineBits);\n> > + return ySum / pixelSum / (1 << 16);\n> > }\n> >\n> > /*\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index cff079bbafb3..50fdeb4f0478 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > /* RGB histograms are not used, so do not populate them. */\n> > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));\n> >\n> > + /*\n> > + * All region sums are based on a 13-bit pipeline bit-depth. Normalise\n> > + * this to 16-bits for the AGC/AWB/ALSC algorithms.\n> > + */\n> > statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });\n> > for (i = 0; i < statistics->awbRegions.numRegions(); i++)\n> > - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum,\n> > - stats->awb_stats[i].g_sum,\n> > - stats->awb_stats[i].b_sum },\n> > + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3,\n> > + stats->awb_stats[i].g_sum << 3,\n> > + stats->awb_stats[i].b_sum << 3 },\n> > stats->awb_stats[i].counted,\n> > stats->awb_stats[i].notcounted });\n> >\n> > @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > */\n> > statistics->agcRegions.init(15);\n> > for (i = 0; i < statistics->agcRegions.numRegions(); i++)\n> > - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum,\n> > - stats->agc_stats[i].g_sum,\n> > - stats->agc_stats[i].b_sum },\n> > + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3,\n> > + stats->agc_stats[i].g_sum << 3,\n> > + stats->agc_stats[i].b_sum << 3 },\n> > stats->agc_stats[i].counted,\n> > stats->awb_stats[i].notcounted });\n> >\n> > --\n> > 2.25.1\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 7784CBEFBE\n\tfor ;\n\tTue, 31 Jan 2023 12:29:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E79EE625D8;\n\tTue, 31 Jan 2023 13:29:23 +0100 (CET)","from mail-yb1-xb34.google.com (mail-yb1-xb34.google.com\n\t[IPv6:2607:f8b0:4864:20::b34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B19C625D0\n\tfor ;\n\tTue, 31 Jan 2023 13:29:22 +0100 (CET)","by mail-yb1-xb34.google.com with SMTP id p141so17799857ybg.12\n\tfor ;\n\tTue, 31 Jan 2023 04:29:22 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675168163;\n\tbh=2vVVMJXy1dLZkeHoeeDpi9g2PsEQRqSugA6tGmKXmK0=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=OwCOe/NHACXYhb4NW2b5UzFmwIcTS2/sqZIXVHj2av3OpKTNk2RRs6/wlW5KUReMX\n\tWrkQoiBY92Wqn1rOUQQcRim1eXrZSwTVZDtYfgldmttX/D6Vd8P08Evzal/Pr6LT6I\n\tx37qMtgG1oJD0/z6DNvxdpQRONL3IndOqcbu5X+p4PmTpzszNGla7HJ+G508Df5lWJ\n\tETS/eSPqtgK8SJKVDvQPGy5B83QQ6PZW1fqG6A3Gxn1AF6egF/lMbdhARoFFZ/OCcE\n\tOaVtPDnWUWorTvh1FKuoZDxPNP774efVS472dKbn+/0YMajI4Mgf5kgVy6JvOiH1DU\n\tOIh/8Z9cJJp0Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=OUHRAWIZNoXrMUBf2xMT362jL4MV7DU7L/YxJsCKEMw=;\n\tb=tE3WYdg8UG/shVkA0hjV/FQ0XnnO9sOV2Uxt0aCCu/92FmGInk6u5eTrRkKZpdpKWA\n\tn/yjdsn9p3jXtsSD5ZghhJO/7MQix2dk5xhWNI39I2+aEyPRRK47ftNVXNeYz0gDj+ih\n\tbUqIHpcvn+09+fzu0j4JosjPfowblm757nm767x7wlHF4bbfRHFj5mIlBHE7T39FP/3s\n\tEICiVv9XDyxgIQgWTywg3HTk/NRVfCgiShWlL/w8UFTuThWu19xOoATlFz3tRerasROQ\n\tAebOMfEDLf+GsLkN2CV/1nvPsfQoc5l+9835oL1RN+OSZCQ6pmlXxNlAXJhlSKVVB8ik\n\tyQvA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"tE3WYdg8\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=OUHRAWIZNoXrMUBf2xMT362jL4MV7DU7L/YxJsCKEMw=;\n\tb=PI93Dn7Rxlw2NkTcGOdl3mlSAP8XhU2XBB/LBVHCLAsuy3tLpzLvvhNqzAP/2i9XYb\n\tECXg39dh+Fzg67tiBRJePM3h/+AFOWTT8BfLboF9iKUalZxzGIYI0UubAMNIyD1LWRvh\n\td1j111VUo1KNfLOJUUtPDmpHpmzol8Pl76LXz30PguYXkwLQFd4s3ZwtM8a/hq9B9lnL\n\tyiU+XVe+ynv0kHG0qbVWTeeRR0FVVodBLZY5MPYWLFYIEIVX2tme5IO3rf5f16MtrB8E\n\tOBlKC/SrXIjFsww21mlDKGtb1q9/SnQmXsxVHYCD6MUZ3nepliVNo4hD7ZlIBd7cY6qy\n\tfeZw==","X-Gm-Message-State":"AFqh2koDdWXrBQfuw4XsVHlPAJC+tmBNQBEAtDxVy0x2t5c7UhYSchPH\n\tYhnutUVsYfZTpTcuKsB1kJ2FjxAgOOY01Nq6JrRtEGEdDjw3RVZVX5k=","X-Google-Smtp-Source":"AMrXdXvThUU/c8BwNffwKF4WRVUA2GQWRHYU15uyiyZHqAHv/wB2e09Xy9VwPfPs3gjzOdtttzRoaGJDpPpBw1QbPpI=","X-Received":"by 2002:a25:7583:0:b0:783:bc5e:3d67 with SMTP id\n\tq125-20020a257583000000b00783bc5e3d67mr4114694ybc.524.1675168160658;\n\tTue, 31 Jan 2023 04:29:20 -0800 (PST)","MIME-Version":"1.0","References":"<20221213114836.15473-1-naush@raspberrypi.com>\n\t<20221213114836.15473-6-naush@raspberrypi.com>\n\t<167508546933.42371.6739065703922714320@Monstersaurus>","In-Reply-To":"<167508546933.42371.6739065703922714320@Monstersaurus>","Date":"Tue, 31 Jan 2023 12:29:07 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] ipa: raspberrypi: Normalise\n\tregion sums to 16-bits","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26389,"web_url":"https://patchwork.libcamera.org/comment/26389/","msgid":"<167517316015.42371.5805420762355394260@Monstersaurus>","date":"2023-01-31T13:52:40","subject":"Re: [libcamera-devel] [PATCH v3 5/5] ipa: raspberrypi: Normalise\n\tregion sums to 16-bits","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2023-01-31 12:29:07)\n> Hi Kieran,\n> \n> On Mon, 30 Jan 2023 at 13:31, Kieran Bingham\n> wrote:\n> >\n> > Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:36)\n> > > The VC4 ISP uses a pipeline bit-depth of 13-bits. The AGC algorithm needs to\n> > > know this bit-depth when computing the Y value for the image.\n> > >\n> > > Instead of hardcoding the VC4 bit-depth in the AGC source code, normalise all\n> > > region sums to 16-bits when filling the Statistics structure. AWB and ALSC are\n> > > agnostic about pipeline depth, so do not need changing.\n> >\n> > pipeline bit-depth to not confuse pipeline depth with the number of\n> > buffers as this term has been used for previously?\n> >\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++------\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++------\n> > > 2 files changed, 14 insertions(+), 12 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > index 868c30f03d66..7ec8292d32ff 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n> > >\n> > > #define NAME \"rpi.agc\"\n> > >\n> > > -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */\n> > > -\n> > > int AgcMeteringMode::read(const libcamera::YamlObject ¶ms)\n> > > {\n> > > const YamlObject &yamlWeights = params[\"weights\"];\n> > > @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> > > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;\n> > > for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) {\n> > > auto ®ion = stats->agcRegions.get(i);\n> > > - double rAcc = std::min(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > > - double gAcc = std::min(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > > - double bAcc = std::min(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > > + double rAcc = std::min(region.val.rSum * gain, ((1 << 16) - 1) * region.counted);\n> > > + double gAcc = std::min(region.val.gSum * gain, ((1 << 16) - 1) * region.counted);\n> > > + double bAcc = std::min(region.val.bSum * gain, ((1 << 16) - 1) * region.counted);\n> >\n> > 1 << 16 - 1 now looks a bit 'magic', and it may not be clear to a\n> > reader what it's refering to?\n> >\n> > Before it referenced PipelineBits so it was clearer. I expect this\n> > ((1 << 16) - 1)\n> >\n> > could be a constexpr somewhere at the top of the file, or a helper to\n> > wrap the scaling of region.counted?\n> >\n> >\n> > Hrm ... it 'was' already a constexpr at the top...\n> > Is there a reason you chose to hardcode the value here instead of update\n> > (/and or rename) the existing PipelineBits ?\n> \n> Since the generalised statistics are now normalised to 16-bits,\n> perhaps what is needed is to have a constexpr for (1 << 16) - 1 to be\n> (possibly) defined in the statistics structure to indicate the\n> normalisation used?\n\nPerhaps, but we're in src/ipa/raspberrypi/ so it's all up to your\npreference.\n\nI'll merge this series, and if you think it deserves a cleanup on top,\nthat should be easy to treat independently.\n\n--\nKieran\n\n\n> \n> Naush\n> \n> >\n> > Aside from that:\n> >\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\n> > > rSum += rAcc * weights[i];\n> > > gSum += gAcc * weights[i];\n> > > bSum += bAcc * weights[i];\n> > > @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> > > double ySum = rSum * awb.gainR * .299 +\n> > > gSum * awb.gainG * .587 +\n> > > bSum * awb.gainB * .114;\n> > > - return ySum / pixelSum / (1 << PipelineBits);\n> > > + return ySum / pixelSum / (1 << 16);\n> > > }\n> > >\n> > > /*\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index cff079bbafb3..50fdeb4f0478 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > > /* RGB histograms are not used, so do not populate them. */\n> > > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));\n> > >\n> > > + /*\n> > > + * All region sums are based on a 13-bit pipeline bit-depth. Normalise\n> > > + * this to 16-bits for the AGC/AWB/ALSC algorithms.\n> > > + */\n> > > statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });\n> > > for (i = 0; i < statistics->awbRegions.numRegions(); i++)\n> > > - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum,\n> > > - stats->awb_stats[i].g_sum,\n> > > - stats->awb_stats[i].b_sum },\n> > > + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3,\n> > > + stats->awb_stats[i].g_sum << 3,\n> > > + stats->awb_stats[i].b_sum << 3 },\n> > > stats->awb_stats[i].counted,\n> > > stats->awb_stats[i].notcounted });\n> > >\n> > > @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > > */\n> > > statistics->agcRegions.init(15);\n> > > for (i = 0; i < statistics->agcRegions.numRegions(); i++)\n> > > - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum,\n> > > - stats->agc_stats[i].g_sum,\n> > > - stats->agc_stats[i].b_sum },\n> > > + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3,\n> > > + stats->agc_stats[i].g_sum << 3,\n> > > + stats->agc_stats[i].b_sum << 3 },\n> > > stats->agc_stats[i].counted,\n> > > stats->awb_stats[i].notcounted });\n> > >\n> > > --\n> > > 2.25.1\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 BE930BEFBE\n\tfor ;\n\tTue, 31 Jan 2023 13:52:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0EAAE625E4;\n\tTue, 31 Jan 2023 14:52:45 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F743625D0\n\tfor ;\n\tTue, 31 Jan 2023 14:52:43 +0100 (CET)","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 EF78BD6;\n\tTue, 31 Jan 2023 14:52:42 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675173165;\n\tbh=bu2uUVY8/T7Tg5zE14loQiCLRYm2/GOqSr2iilVJm80=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=gIHHqQYjFBoYy8E92eOy+erV6xqkB38WMRgb33edKz19hsumi6rBqrvyeDiPmCrEk\n\tj7/TZt7PfoUSbcdVT/HhLc0Si5Cc01n5PDX+EH5oy+KWJVFoKCq8BAXT7sl6AAc7sG\n\tSEycNwpXWSlk8qM0kbZmFN4icBK/bCHoQiyiyHnqRyF+uALZk44xMWT8Cbk/+JeXxn\n\t5n3q2xlh4D+j/nwingpu7ncMjz4kA3alnNIzRERHs6EtlW6jQcN57c/W7wZzLHQbrt\n\twjYX+0IKb6cQ8A5rfUi2IQNbIb1qZNgh44hvPV8woNs2KbqcB1sBguvXv2moaXwT8b\n\tEhaKPNLH+ng3Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675173163;\n\tbh=bu2uUVY8/T7Tg5zE14loQiCLRYm2/GOqSr2iilVJm80=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=dzGbiAG8eozOvD3Twvo+w9mr5ve6ob26/NK5ZmHlq6MruIFSsDOiftMOvq3+9Hidj\n\t+EoY3LqLOLG+aCBIayAO7ByidEk3KUNuS5eanX3fod03DuhUG+5eE60t6se5eK1K6p\n\td1gV9A1wwUtdsaAKq4lLpFIVuQCLEdvEKaw4l/yg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dzGbiAG8\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221213114836.15473-1-naush@raspberrypi.com>\n\t<20221213114836.15473-6-naush@raspberrypi.com>\n\t<167508546933.42371.6739065703922714320@Monstersaurus>\n\t","To":"Naushir Patuck ","Date":"Tue, 31 Jan 2023 13:52:40 +0000","Message-ID":"<167517316015.42371.5805420762355394260@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] ipa: raspberrypi: Normalise\n\tregion sums to 16-bits","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26390,"web_url":"https://patchwork.libcamera.org/comment/26390/","msgid":"<167517321825.42371.10557841175053050314@Monstersaurus>","date":"2023-01-31T13:53:38","subject":"Re: [libcamera-devel] [PATCH v3 5/5] ipa: raspberrypi: Normalise\n\tregion sums to 16-bits","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2023-01-31 13:52:40)\n> Quoting Naushir Patuck (2023-01-31 12:29:07)\n> > Hi Kieran,\n> > \n> > On Mon, 30 Jan 2023 at 13:31, Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:36)\n> > > > The VC4 ISP uses a pipeline bit-depth of 13-bits. The AGC algorithm needs to\n> > > > know this bit-depth when computing the Y value for the image.\n> > > >\n> > > > Instead of hardcoding the VC4 bit-depth in the AGC source code, normalise all\n> > > > region sums to 16-bits when filling the Statistics structure. AWB and ALSC are\n> > > > agnostic about pipeline depth, so do not need changing.\n> > >\n> > > pipeline bit-depth to not confuse pipeline depth with the number of\n> > > buffers as this term has been used for previously?\n> > >\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++------\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++------\n> > > > 2 files changed, 14 insertions(+), 12 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > index 868c30f03d66..7ec8292d32ff 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n> > > >\n> > > > #define NAME \"rpi.agc\"\n> > > >\n> > > > -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */\n> > > > -\n> > > > int AgcMeteringMode::read(const libcamera::YamlObject ¶ms)\n> > > > {\n> > > > const YamlObject &yamlWeights = params[\"weights\"];\n> > > > @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> > > > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;\n> > > > for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) {\n> > > > auto ®ion = stats->agcRegions.get(i);\n> > > > - double rAcc = std::min(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > > > - double gAcc = std::min(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > > > - double bAcc = std::min(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > > > + double rAcc = std::min(region.val.rSum * gain, ((1 << 16) - 1) * region.counted);\n> > > > + double gAcc = std::min(region.val.gSum * gain, ((1 << 16) - 1) * region.counted);\n> > > > + double bAcc = std::min(region.val.bSum * gain, ((1 << 16) - 1) * region.counted);\n> > >\n> > > 1 << 16 - 1 now looks a bit 'magic', and it may not be clear to a\n> > > reader what it's refering to?\n> > >\n> > > Before it referenced PipelineBits so it was clearer. I expect this\n> > > ((1 << 16) - 1)\n> > >\n> > > could be a constexpr somewhere at the top of the file, or a helper to\n> > > wrap the scaling of region.counted?\n> > >\n> > >\n> > > Hrm ... it 'was' already a constexpr at the top...\n> > > Is there a reason you chose to hardcode the value here instead of update\n> > > (/and or rename) the existing PipelineBits ?\n> > \n> > Since the generalised statistics are now normalised to 16-bits,\n> > perhaps what is needed is to have a constexpr for (1 << 16) - 1 to be\n> > (possibly) defined in the statistics structure to indicate the\n> > normalisation used?\n> \n> Perhaps, but we're in src/ipa/raspberrypi/ so it's all up to your\n> preference.\n> \n> I'll merge this series, and if you think it deserves a cleanup on top,\n> that should be easy to treat independently.\n> \n\naha - we crossed paths. I'll make sure your new patch still works, and\nuse that instead of this one. ;-)\n\n--\nKieran\n\n\n> --\n> Kieran\n> \n> \n> > \n> > Naush\n> > \n> > >\n> > > Aside from that:\n> > >\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > >\n> > > > rSum += rAcc * weights[i];\n> > > > gSum += gAcc * weights[i];\n> > > > bSum += bAcc * weights[i];\n> > > > @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> > > > double ySum = rSum * awb.gainR * .299 +\n> > > > gSum * awb.gainG * .587 +\n> > > > bSum * awb.gainB * .114;\n> > > > - return ySum / pixelSum / (1 << PipelineBits);\n> > > > + return ySum / pixelSum / (1 << 16);\n> > > > }\n> > > >\n> > > > /*\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index cff079bbafb3..50fdeb4f0478 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > > > /* RGB histograms are not used, so do not populate them. */\n> > > > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));\n> > > >\n> > > > + /*\n> > > > + * All region sums are based on a 13-bit pipeline bit-depth. Normalise\n> > > > + * this to 16-bits for the AGC/AWB/ALSC algorithms.\n> > > > + */\n> > > > statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });\n> > > > for (i = 0; i < statistics->awbRegions.numRegions(); i++)\n> > > > - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum,\n> > > > - stats->awb_stats[i].g_sum,\n> > > > - stats->awb_stats[i].b_sum },\n> > > > + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3,\n> > > > + stats->awb_stats[i].g_sum << 3,\n> > > > + stats->awb_stats[i].b_sum << 3 },\n> > > > stats->awb_stats[i].counted,\n> > > > stats->awb_stats[i].notcounted });\n> > > >\n> > > > @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > > > */\n> > > > statistics->agcRegions.init(15);\n> > > > for (i = 0; i < statistics->agcRegions.numRegions(); i++)\n> > > > - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum,\n> > > > - stats->agc_stats[i].g_sum,\n> > > > - stats->agc_stats[i].b_sum },\n> > > > + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3,\n> > > > + stats->agc_stats[i].g_sum << 3,\n> > > > + stats->agc_stats[i].b_sum << 3 },\n> > > > stats->agc_stats[i].counted,\n> > > > stats->awb_stats[i].notcounted });\n> > > >\n> > > > --\n> > > > 2.25.1\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 076EDBEFBE\n\tfor ;\n\tTue, 31 Jan 2023 13:53:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4694625D8;\n\tTue, 31 Jan 2023 14:53:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B84C8625D0\n\tfor ;\n\tTue, 31 Jan 2023 14:53:40 +0100 (CET)","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 650C3D6;\n\tTue, 31 Jan 2023 14:53:40 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675173221;\n\tbh=r+DJN2m8EZHaXvsRI1TXz2Fz40kVCk95+YyolbNQ42A=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Ykk0KijQ3CsW7B0WfVAemyRsTe9pZj/Ki1iwuMdX+SEiM8jw2xsOIAXAecb49huwd\n\t7qjO1aI2J1Oj4IRtYgIp2rP4y8gtgr74YQTYFJfEUz8QSNVxiNX6upWSqT0SPCNsRZ\n\tmeZ9Ref0Oul6GJ6sUx69olIvMPQGjx+mh/OROJH8iTWgIenf4fk5+5v57wCat3rU1t\n\tYFP87sKzEZyXIF8WBX0tL1/W/UFgJyHjlOtvkXpBqJeMqpV9P7aA3xdosQhUlLIEXp\n\taV4V9jI8oF1vR7dFDjswhGU9JLWvB4dtjLfGbbWGVdBnLTHZJ6pQTl3HmfwENs3PeM\n\tTVsfvnFRZR3CA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675173220;\n\tbh=r+DJN2m8EZHaXvsRI1TXz2Fz40kVCk95+YyolbNQ42A=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=CXNmcfO5NXl6iEHgZ3dMDiwrTSqZ03bxdfhFkw104EzPXl8LC+MFa+fxW3IKTaXjL\n\tRLBNZdTbQ05IWSGolWNFcazoXrfBraSPDchulKlVkf+ITia2obM/ysCSmn/vENwvoI\n\t1RhO8pVM2h7rpWwMtikxzvDqwtGT2XurhtH00aAA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"CXNmcfO5\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<167517316015.42371.5805420762355394260@Monstersaurus>","References":"<20221213114836.15473-1-naush@raspberrypi.com>\n\t<20221213114836.15473-6-naush@raspberrypi.com>\n\t<167508546933.42371.6739065703922714320@Monstersaurus>\n\t\n\t<167517316015.42371.5805420762355394260@Monstersaurus>","To":"Naushir Patuck ","Date":"Tue, 31 Jan 2023 13:53:38 +0000","Message-ID":"<167517321825.42371.10557841175053050314@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] ipa: raspberrypi: Normalise\n\tregion sums to 16-bits","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26391,"web_url":"https://patchwork.libcamera.org/comment/26391/","msgid":"","date":"2023-01-31T13:56:34","subject":"Re: [libcamera-devel] [PATCH v3 5/5] ipa: raspberrypi: Normalise\n\tregion sums to 16-bits","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Tue, 31 Jan 2023 at 13:53, Kieran Bingham\n wrote:\n>\n> Quoting Kieran Bingham (2023-01-31 13:52:40)\n> > Quoting Naushir Patuck (2023-01-31 12:29:07)\n> > > Hi Kieran,\n> > >\n> > > On Mon, 30 Jan 2023 at 13:31, Kieran Bingham\n> > > wrote:\n> > > >\n> > > > Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:36)\n> > > > > The VC4 ISP uses a pipeline bit-depth of 13-bits. The AGC algorithm needs to\n> > > > > know this bit-depth when computing the Y value for the image.\n> > > > >\n> > > > > Instead of hardcoding the VC4 bit-depth in the AGC source code, normalise all\n> > > > > region sums to 16-bits when filling the Statistics structure. AWB and ALSC are\n> > > > > agnostic about pipeline depth, so do not need changing.\n> > > >\n> > > > pipeline bit-depth to not confuse pipeline depth with the number of\n> > > > buffers as this term has been used for previously?\n> > > >\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > Reviewed-by: David Plowman \n> > > > > ---\n> > > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++------\n> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++------\n> > > > > 2 files changed, 14 insertions(+), 12 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > > index 868c30f03d66..7ec8292d32ff 100644\n> > > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > > @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n> > > > >\n> > > > > #define NAME \"rpi.agc\"\n> > > > >\n> > > > > -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */\n> > > > > -\n> > > > > int AgcMeteringMode::read(const libcamera::YamlObject ¶ms)\n> > > > > {\n> > > > > const YamlObject &yamlWeights = params[\"weights\"];\n> > > > > @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> > > > > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;\n> > > > > for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) {\n> > > > > auto ®ion = stats->agcRegions.get(i);\n> > > > > - double rAcc = std::min(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > > > > - double gAcc = std::min(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > > > > - double bAcc = std::min(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> > > > > + double rAcc = std::min(region.val.rSum * gain, ((1 << 16) - 1) * region.counted);\n> > > > > + double gAcc = std::min(region.val.gSum * gain, ((1 << 16) - 1) * region.counted);\n> > > > > + double bAcc = std::min(region.val.bSum * gain, ((1 << 16) - 1) * region.counted);\n> > > >\n> > > > 1 << 16 - 1 now looks a bit 'magic', and it may not be clear to a\n> > > > reader what it's refering to?\n> > > >\n> > > > Before it referenced PipelineBits so it was clearer. I expect this\n> > > > ((1 << 16) - 1)\n> > > >\n> > > > could be a constexpr somewhere at the top of the file, or a helper to\n> > > > wrap the scaling of region.counted?\n> > > >\n> > > >\n> > > > Hrm ... it 'was' already a constexpr at the top...\n> > > > Is there a reason you chose to hardcode the value here instead of update\n> > > > (/and or rename) the existing PipelineBits ?\n> > >\n> > > Since the generalised statistics are now normalised to 16-bits,\n> > > perhaps what is needed is to have a constexpr for (1 << 16) - 1 to be\n> > > (possibly) defined in the statistics structure to indicate the\n> > > normalisation used?\n> >\n> > Perhaps, but we're in src/ipa/raspberrypi/ so it's all up to your\n> > preference.\n> >\n> > I'll merge this series, and if you think it deserves a cleanup on top,\n> > that should be easy to treat independently.\n> >\n>\n> aha - we crossed paths. I'll make sure your new patch still works, and\n> use that instead of this one. ;-)\n\nPerfect, thanks! :-)\n\n>\n> --\n> Kieran\n>\n>\n> > --\n> > Kieran\n> >\n> >\n> > >\n> > > Naush\n> > >\n> > > >\n> > > > Aside from that:\n> > > >\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > > >\n> > > >\n> > > > > rSum += rAcc * weights[i];\n> > > > > gSum += gAcc * weights[i];\n> > > > > bSum += bAcc * weights[i];\n> > > > > @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> > > > > double ySum = rSum * awb.gainR * .299 +\n> > > > > gSum * awb.gainG * .587 +\n> > > > > bSum * awb.gainB * .114;\n> > > > > - return ySum / pixelSum / (1 << PipelineBits);\n> > > > > + return ySum / pixelSum / (1 << 16);\n> > > > > }\n> > > > >\n> > > > > /*\n> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > index cff079bbafb3..50fdeb4f0478 100644\n> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > > > > /* RGB histograms are not used, so do not populate them. */\n> > > > > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));\n> > > > >\n> > > > > + /*\n> > > > > + * All region sums are based on a 13-bit pipeline bit-depth. Normalise\n> > > > > + * this to 16-bits for the AGC/AWB/ALSC algorithms.\n> > > > > + */\n> > > > > statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });\n> > > > > for (i = 0; i < statistics->awbRegions.numRegions(); i++)\n> > > > > - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum,\n> > > > > - stats->awb_stats[i].g_sum,\n> > > > > - stats->awb_stats[i].b_sum },\n> > > > > + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3,\n> > > > > + stats->awb_stats[i].g_sum << 3,\n> > > > > + stats->awb_stats[i].b_sum << 3 },\n> > > > > stats->awb_stats[i].counted,\n> > > > > stats->awb_stats[i].notcounted });\n> > > > >\n> > > > > @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > > > > */\n> > > > > statistics->agcRegions.init(15);\n> > > > > for (i = 0; i < statistics->agcRegions.numRegions(); i++)\n> > > > > - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum,\n> > > > > - stats->agc_stats[i].g_sum,\n> > > > > - stats->agc_stats[i].b_sum },\n> > > > > + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3,\n> > > > > + stats->agc_stats[i].g_sum << 3,\n> > > > > + stats->agc_stats[i].b_sum << 3 },\n> > > > > stats->agc_stats[i].counted,\n> > > > > stats->awb_stats[i].notcounted });\n> > > > >\n> > > > > --\n> > > > > 2.25.1\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 E3EAAC323E\n\tfor ;\n\tTue, 31 Jan 2023 13:56:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68FCE625D8;\n\tTue, 31 Jan 2023 14:56:50 +0100 (CET)","from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com\n\t[IPv6:2607:f8b0:4864:20::b36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDFE9625D0\n\tfor ;\n\tTue, 31 Jan 2023 14:56:48 +0100 (CET)","by mail-yb1-xb36.google.com with SMTP id 129so18202481ybb.0\n\tfor ;\n\tTue, 31 Jan 2023 05:56:48 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675173410;\n\tbh=XPCWJ9iGDNTyTgi8MYxV/CHonU1KMx4Eff1b0d0KUok=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=PI0Z+jFwC606/ZuXXEmlMykhpz2A3fLH1JGGTZ+nL+TrXWIfhMfLGyKP21G0h61Ig\n\tzl09MZ8xbLiPOhAi8shLmqUhIlJHaU/6FUkJFr5glPUdBvSVpca4YLLGvginoJo+jM\n\t3Eb0JQXLxcJ35aA51fyGWpiLPC4fNgTCfkxSNwSak+RClEDQ1rs9n0FMoW7H4MVrfx\n\tq1qG9hjJAQ8kW6sJihfNIRovZrvQzQ272dOEoew2K7Q6yOIJeVkXZO7StMTG7AboEw\n\tl57Rsdk8/tD8gvV0gYL2EA+VACLKP8FFiHNuBtW2ru42gwnnMiLE7K3Cp+eXsZKc9n\n\teco/JI//QPyHw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=X6o8UDxO6PuNcLbnWjc8sJeAGFBdGDpTrWhS/Z//+AE=;\n\tb=kkTOEqmh1Vca4++xWKqRcJ8jLkXJTcISDkbB6keQF315yqjkMVHny0mhTXF7uwKWqw\n\tQt6RS3Pf25ictpaQGk9gUwj96mvvja/jO6X0kNTA9QXuL3s42IiyfaarMagmuEVWwy0K\n\tANVa5ybWxiXxmnlxK87IILKum3CR4o+gsy0YYRgOZdr7Ji5/ij9JFri0+z+WkmAmfl5n\n\tzcS8As9beXMwk87LjGV+wermFdcqb0vKugLno8RkjyN0cm+Ku+V8Lua6YzXzoLsE8Vp9\n\tC4t8PzSAijWbEkRj8zEEy566Ee0dgu2N1h1RgfNW/aLHneiXJiQhpZejiIaJiEhid4xE\n\tLIgw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"kkTOEqmh\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=X6o8UDxO6PuNcLbnWjc8sJeAGFBdGDpTrWhS/Z//+AE=;\n\tb=nw1UXj6k7M+sCXZrxUQ0Ef2l02RvYYRDaCfCBHOWGMDCKshULMJWZDRtSf50vWd4Qw\n\tFIdpA9NYTmkdNF1xHkVZQojKMo7XeMFxKSFA7kFcSzPPFoYhcROJx1cuBGHwxSiFeFsl\n\trLhbSs2ORxgX8kWtxA2oQkChJlhVyV8JGl3n6Bim6/+vT+kS65j1pIyzIbI02AUDvfKe\n\tbfm48nnQRE5ma7omyuQKMK7LXBUZgR0FWf3bx8E3veVEmP6EN7xUZRE5NDF3kTqrnf7w\n\ttns3V5ROgdNmoymriVey3LiKcx3z9M0VIngQO+oMKKhYmp0yF5OoQ6Xzp5J6i3OKtcyk\n\tDcFw==","X-Gm-Message-State":"AO0yUKVvxzunim574ic8tvJYA1cNxGvSOVPjDnRlWYgefdxu2ZuIRk+f\n\tMcS7rpQD3vMyL7XPkAmAgQevgmjGzKpAlVruFWTwVwZuh+GkXs32ufY=","X-Google-Smtp-Source":"AK7set/HzlGCzyjpwX+NxUA5wr1JdTNf9IqO9E9AGYTI5aLZZa1Qsq8bQTllWqNwLb2kPupLtyEb8TTi2TT3kYyPauI=","X-Received":"by 2002:a25:260c:0:b0:80b:663d:6a98 with SMTP id\n\tm12-20020a25260c000000b0080b663d6a98mr2979786ybm.598.1675173407420;\n\tTue, 31 Jan 2023 05:56:47 -0800 (PST)","MIME-Version":"1.0","References":"<20221213114836.15473-1-naush@raspberrypi.com>\n\t<20221213114836.15473-6-naush@raspberrypi.com>\n\t<167508546933.42371.6739065703922714320@Monstersaurus>\n\t\n\t<167517316015.42371.5805420762355394260@Monstersaurus>\n\t<167517321825.42371.10557841175053050314@Monstersaurus>","In-Reply-To":"<167517321825.42371.10557841175053050314@Monstersaurus>","Date":"Tue, 31 Jan 2023 13:56:34 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] ipa: raspberrypi: Normalise\n\tregion sums to 16-bits","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]