[{"id":35441,"web_url":"https://patchwork.libcamera.org/comment/35441/","msgid":"<175525864416.560048.5711198879586476212@ping.linuxembedded.co.uk>","date":"2025-08-15T11:50:44","subject":"Re: [PATCH v3 13/19] ipa: rkisp1: Switch histogram to RGB combined\n\tmode","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-08-15 11:29:33)\n> The Y mode of the histogram gets captured at the ISP output, before the\n> output formatter. This has the side effect that the first and the last\n> bins are empty in case of limited YUV range. Another side effect is\n> that gamma and GWDR processing is included in the histogram which makes\n> algorithm development very difficult. In RGB mode the histogram is taken\n> after xtalk (CCM) and is therefore independent of gamma and WDR. The\n> limited range issue also does not apply. In the ISP reference it is\n> however stated that \"it is not possible to calculate a luminance or\n> grayscale histogram from an RGB histogram since the position information\n> is lost during its generation\".\n> \n> During testing the RGB histogram provided good data and better\n> algorithmic stability at a possible (but not measured) inaccuracy.\n> \n> Another option would be to pass the color space information into the IPA\n> and strip the histogram accordingly. For ease of implementation switch to\n> the RGB mode.\n> \n> Signed-off-by: Stefan Klug \n> Reviewed-by: Paul Elder \n> \n> ---\n> \n> Changes in v3:\n> - Added block comment inside the code\n\nThanks\n\n\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 19 ++++++++++++++++++-\n> 1 file changed, 18 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index d34c041f9fe1..ba617a4e6e0c 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -396,7 +396,24 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> hstConfig.setEnabled(true);\n> \n> hstConfig->meas_window = context.configuration.agc.measureWindow;\n> - hstConfig->mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;\n> + /**\n\nShould this be a doxygen block ** ? Will it get rendered into the\ninternal doxygen docs for Rkisp1 IPA when we build it ?\n\n> + * The Y mode of the histogram gets captured at the ISP output, before\n\nTiny nit: whitespace error at the *\n\nReviewed-by: Kieran Bingham \n\n> + * the output formatter. This has the side effect that the first and\n> + * the last bins are empty in case of limited YUV range. Another side\n> + * effect is that gamma and GWDR processing is included in the histogram\n> + * which makes algorithm development very difficult. In RGB mode the\n> + * histogram is taken after xtalk (CCM) and is therefore independent of\n> + * gamma and WDR. The limited range issue also does not apply. In the\n> + * ISP reference it is however stated that \"it is not possible to\n> + * calculate a luminance or grayscale histogram from an RGB histogram\n> + * since the position information is lost during its generation\".\n> + *\n> + * During testing the RGB histogram provided good data and better\n> + * algorithmic stability at a possible (but not measured) inaccuracy.\n> + *\n> + * \\todo For a proper fix support for HIST64 is needed.\n> + */\n> + hstConfig->mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED;\n> \n> Span weights{\n> hstConfig->hist_weight,\n> -- \n> 2.48.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 777E4BEFBE\n\tfor ;\n\tFri, 15 Aug 2025 11:50:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BAFC6924E;\n\tFri, 15 Aug 2025 13:50:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C41561443\n\tfor ;\n\tFri, 15 Aug 2025 13:50:47 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2448B605;\n\tFri, 15 Aug 2025 13:49:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aTgIpPFv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755258592;\n\tbh=WXw5p6o4KgRDjkoPg3rnO/PVPXqwCUsEEr+Kka5JXM4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=aTgIpPFvAREAmivtudrJXw2WpebOa5DkToegBwb6s3QpfwrmlIvtUve8aV5jCKmBQ\n\t/Xapld2ZXIKGSPj2UUw2QBuzVNJctv9a49/0iHEZ1JfVpiwpNkwkXUaUGVh9MwJmWo\n\tEqdO8LrHCGkiD9xsA3IRkFfj4retgUvLbAfSDwqE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250815102945.1602071-14-stefan.klug@ideasonboard.com>","References":"<20250815102945.1602071-1-stefan.klug@ideasonboard.com>\n\t<20250815102945.1602071-14-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v3 13/19] ipa: rkisp1: Switch histogram to RGB combined\n\tmode","From":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tPaul Elder ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 15 Aug 2025 12:50:44 +0100","Message-ID":"<175525864416.560048.5711198879586476212@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":35463,"web_url":"https://patchwork.libcamera.org/comment/35463/","msgid":"<175550341554.1970129.8752595265596514856@localhost>","date":"2025-08-18T07:50:15","subject":"Re: [PATCH v3 13/19] ipa: rkisp1: Switch histogram to RGB combined\n\tmode","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Quoting Kieran Bingham (2025-08-15 13:50:44)\n> Quoting Stefan Klug (2025-08-15 11:29:33)\n> > The Y mode of the histogram gets captured at the ISP output, before the\n> > output formatter. This has the side effect that the first and the last\n> > bins are empty in case of limited YUV range. Another side effect is\n> > that gamma and GWDR processing is included in the histogram which makes\n> > algorithm development very difficult. In RGB mode the histogram is taken\n> > after xtalk (CCM) and is therefore independent of gamma and WDR. The\n> > limited range issue also does not apply. In the ISP reference it is\n> > however stated that \"it is not possible to calculate a luminance or\n> > grayscale histogram from an RGB histogram since the position information\n> > is lost during its generation\".\n> > \n> > During testing the RGB histogram provided good data and better\n> > algorithmic stability at a possible (but not measured) inaccuracy.\n> > \n> > Another option would be to pass the color space information into the IPA\n> > and strip the histogram accordingly. For ease of implementation switch to\n> > the RGB mode.\n> > \n> > Signed-off-by: Stefan Klug \n> > Reviewed-by: Paul Elder \n> > \n> > ---\n> > \n> > Changes in v3:\n> > - Added block comment inside the code\n> \n> Thanks\n> \n> \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 19 ++++++++++++++++++-\n> > 1 file changed, 18 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index d34c041f9fe1..ba617a4e6e0c 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -396,7 +396,24 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > hstConfig.setEnabled(true);\n> > \n> > hstConfig->meas_window = context.configuration.agc.measureWindow;\n> > - hstConfig->mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;\n> > + /**\n> \n> Should this be a doxygen block ** ? Will it get rendered into the\n> internal doxygen docs for Rkisp1 IPA when we build it ?\n\nNo, that was my muscle memory. I need to check if I also did that\nelsewhere... fixed.\n\n> \n> > + * The Y mode of the histogram gets captured at the ISP output, before\n> \n> Tiny nit: whitespace error at the *\n\nOh I missed that. Thanks.\n\n> \n> Reviewed-by: Kieran Bingham \n> \n> > + * the output formatter. This has the side effect that the first and\n> > + * the last bins are empty in case of limited YUV range. Another side\n> > + * effect is that gamma and GWDR processing is included in the histogram\n> > + * which makes algorithm development very difficult. In RGB mode the\n> > + * histogram is taken after xtalk (CCM) and is therefore independent of\n> > + * gamma and WDR. The limited range issue also does not apply. In the\n> > + * ISP reference it is however stated that \"it is not possible to\n> > + * calculate a luminance or grayscale histogram from an RGB histogram\n> > + * since the position information is lost during its generation\".\n> > + *\n> > + * During testing the RGB histogram provided good data and better\n> > + * algorithmic stability at a possible (but not measured) inaccuracy.\n> > + *\n> > + * \\todo For a proper fix support for HIST64 is needed.\n> > + */\n> > + hstConfig->mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED;\n> > \n> > Span weights{\n> > hstConfig->hist_weight,\n> > -- \n> > 2.48.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 7CC7EBEFBE\n\tfor ;\n\tMon, 18 Aug 2025 07:50:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8976B69259;\n\tMon, 18 Aug 2025 09:50:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6BC76924E\n\tfor ;\n\tMon, 18 Aug 2025 09:50:18 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:766d:a405:f64e:fe3a])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 9AE9C17D1; \n\tMon, 18 Aug 2025 09:49:21 +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=\"hS5ojQ+x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755503361;\n\tbh=jKQS0m8eRWRGXo4oE14LacAxccOkiYkiFdkzl1htqbY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=hS5ojQ+x+nZOYqlFf37MIL/YlV+GpIEAKVqr9+VVvyiZiYehKKJ5CSbmMMcpUdNqs\n\ti4bYaXNBoHVJSE0z/+8yYpSNUW+Jpql3R2pMFT2+4JGWJBtR+kUxuoEMUxwXS4Mmte\n\tkiplyMuqo1qpBR+XkAjOTpdwI2BakjwSS/f4yqs8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175525864416.560048.5711198879586476212@ping.linuxembedded.co.uk>","References":"<20250815102945.1602071-1-stefan.klug@ideasonboard.com>\n\t<20250815102945.1602071-14-stefan.klug@ideasonboard.com>\n\t<175525864416.560048.5711198879586476212@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v3 13/19] ipa: rkisp1: Switch histogram to RGB combined\n\tmode","From":"Stefan Klug ","Cc":"Paul Elder ","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 18 Aug 2025 09:50:15 +0200","Message-ID":"<175550341554.1970129.8752595265596514856@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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\" "}}]