[{"id":28998,"web_url":"https://patchwork.libcamera.org/comment/28998/","msgid":"<171085196722.1676185.17818719357426432548@ping.linuxembedded.co.uk>","date":"2024-03-19T12:39:27","subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-03-19 10:54:03)\n> The histogram reported by the rkisp1 hardware is 20 bits, where the\n> upper 16 bits are meaningful integer data and the lower 4 bits are\n> fractional and meant to be discarded. Remove these 4 bits when\n> constructing the histogram.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 47a6f7b2..278903a5 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span expMeans, double gain)\n> */\n> double Agc::measureBrightness(Span hist) const\n> {\n> - Histogram histogram{ hist };\n> + Histogram histogram{ hist, 4 };\n\nOn it's own this doesn't look very clear. So I think we need to add a\ncomment here above to say (in the code) why we're doing the shift.\n\nAlso, and this is only an idea really - the passing of a parameter here\nfeels odd. I wonder if instead we should implement operator>>= and\noperator<<= on the Histogram class which will iterate over each entry and\ndo the shift accordingly. Otherwise, it's hard to convey 'which way' the\nshift of 4 is. And what if someone wants to shift the otherway to make a\nhistogram fit to 20 bits?\n\nSomething like\n\n\tHistogram histogram{ hist };\n\n\t/*\n\t * The RKISP1 hardware histogram is 20 bits, but represent 16\n\t * bits integer and 4 bits are fractional. Shift to discard the\n\t * fractional components.\n\t */\n\thistogram >>= 4;\n\nThis might be seen as less efficient, but I think the intent and\noperations are then clearer.\n\n\n> /* Estimate the quantile mean of the top 2% of the histogram. */\n> return histogram.interQuantileMean(0.98, 1.0);\n> }\n> -- \n> 2.39.2\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 20709BD160\n\tfor ;\n\tTue, 19 Mar 2024 12:39:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FD0962D37;\n\tTue, 19 Mar 2024 13:39:30 +0100 (CET)","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 7AED362CA7\n\tfor ;\n\tTue, 19 Mar 2024 13:39:29 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8ECC8F02;\n\tTue, 19 Mar 2024 13:39:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nrcKY53Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710851942;\n\tbh=mvGE48ZCJvyCVclXYp90qY7kBdpaibs8FiO7vVs8MkQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=nrcKY53Zd1HbWOv288NvGU3HbaKxVo9hTcWX2+NCE5BvVvHv6e7x7djThShNf5J79\n\t3tDctCW/OOxjAOUdEeo75VUX5hGYvboI/6wEB3j/vzXyB8sPFILS9w9nawkWHxCrOl\n\tA5Ak9HNTyQq4msKulx35wJDbaWiRAde4yI5F/ir4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240319105403.4045592-3-paul.elder@ideasonboard.com>","References":"<20240319105403.4045592-1-paul.elder@ideasonboard.com>\n\t<20240319105403.4045592-3-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","From":"Kieran Bingham ","Cc":"Paul Elder ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 19 Mar 2024 12:39:27 +0000","Message-ID":"<171085196722.1676185.17818719357426432548@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":28999,"web_url":"https://patchwork.libcamera.org/comment/28999/","msgid":"<20240319124849.GB29058@pendragon.ideasonboard.com>","date":"2024-03-19T12:48:49","subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Mar 19, 2024 at 12:39:27PM +0000, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-03-19 10:54:03)\n> > The histogram reported by the rkisp1 hardware is 20 bits, where the\n> > upper 16 bits are meaningful integer data and the lower 4 bits are\n> > fractional and meant to be discarded. Remove these 4 bits when\n> > constructing the histogram.\n\nAre they completely unusable, or are they fractional bits that could be\nhelpful ? I wonder if we shouldn't drop them in the kernel if there's no\nway whatsoever to use them.\n\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 47a6f7b2..278903a5 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span expMeans, double gain)\n> > */\n> > double Agc::measureBrightness(Span hist) const\n> > {\n> > - Histogram histogram{ hist };\n> > + Histogram histogram{ hist, 4 };\n> \n> On it's own this doesn't look very clear. So I think we need to add a\n> comment here above to say (in the code) why we're doing the shift.\n> \n> Also, and this is only an idea really - the passing of a parameter here\n> feels odd. I wonder if instead we should implement operator>>= and\n> operator<<= on the Histogram class which will iterate over each entry and\n> do the shift accordingly. Otherwise, it's hard to convey 'which way' the\n> shift of 4 is. And what if someone wants to shift the otherway to make a\n> histogram fit to 20 bits?\n> \n> Something like\n> \n> \tHistogram histogram{ hist };\n> \n> \t/*\n> \t * The RKISP1 hardware histogram is 20 bits, but represent 16\n> \t * bits integer and 4 bits are fractional. Shift to discard the\n> \t * fractional components.\n> \t */\n> \thistogram >>= 4;\n\nWhat if we need other types of conversions in the future ? This doesn't\nseem very generic. Passing a lambda function that transforms the value\nwould seem better, and would also be more readable. You don't want that\nfunction to be called for every value without allowing for compiler\noptimization though, so the constructor would likely need to be inlined.\n\n> This might be seen as less efficient, but I think the intent and\n> operations are then clearer.\n\nEfficiency-wise, that's not great indeed :-(\n\n> > /* Estimate the quantile mean of the top 2% of the histogram. */\n> > return histogram.interQuantileMean(0.98, 1.0);\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 2E5E4C3272\n\tfor ;\n\tTue, 19 Mar 2024 12:48:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0980962CAF;\n\tTue, 19 Mar 2024 13:48:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD5D662CAB\n\tfor ;\n\tTue, 19 Mar 2024 13:48:51 +0100 (CET)","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 E13C6480;\n\tTue, 19 Mar 2024 13:48:24 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"H8sLmaWp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710852505;\n\tbh=YH1NJmcLX1iOeesoJV9PD/m30JjM/ebWgvRKirvXqfI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H8sLmaWpYPWMJTgpUUMslBC6AjyDL7to50F3H+VYXSB8knlLDOmBw7nxBE98iwhsM\n\tMK4LnwDfKGfhPwqlntxRv6yB90+sQfeb3ZYnBqRYTWmlo8IyncaMz1j/ecaC+Q+sqb\n\tdAiGOH9gRXpRCTkWyh9xfw6V1asAfkqdp4pZPQSI=","Date":"Tue, 19 Mar 2024 14:48:49 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","Message-ID":"<20240319124849.GB29058@pendragon.ideasonboard.com>","References":"<20240319105403.4045592-1-paul.elder@ideasonboard.com>\n\t<20240319105403.4045592-3-paul.elder@ideasonboard.com>\n\t<171085196722.1676185.17818719357426432548@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171085196722.1676185.17818719357426432548@ping.linuxembedded.co.uk>","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":29076,"web_url":"https://patchwork.libcamera.org/comment/29076/","msgid":"","date":"2024-03-27T09:35:43","subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Tue, Mar 19, 2024 at 02:48:49PM +0200, Laurent Pinchart wrote:\n> On Tue, Mar 19, 2024 at 12:39:27PM +0000, Kieran Bingham wrote:\n> > Quoting Paul Elder (2024-03-19 10:54:03)\n> > > The histogram reported by the rkisp1 hardware is 20 bits, where the\n> > > upper 16 bits are meaningful integer data and the lower 4 bits are\n> > > fractional and meant to be discarded. Remove these 4 bits when\n> > > constructing the histogram.\n> \n> Are they completely unusable, or are they fractional bits that could be\n> helpful ? I wonder if we shouldn't drop them in the kernel if there's no\n> way whatsoever to use them.\n\nThe programming guide says to not use them. They're generated because\nthe hardware does weighting and it doesn't actually count every single\npixel, apparently.\n\n> > > \n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 47a6f7b2..278903a5 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span expMeans, double gain)\n> > > */\n> > > double Agc::measureBrightness(Span hist) const\n> > > {\n> > > - Histogram histogram{ hist };\n> > > + Histogram histogram{ hist, 4 };\n> > \n> > On it's own this doesn't look very clear. So I think we need to add a\n> > comment here above to say (in the code) why we're doing the shift.\n> > \n> > Also, and this is only an idea really - the passing of a parameter here\n> > feels odd. I wonder if instead we should implement operator>>= and\n> > operator<<= on the Histogram class which will iterate over each entry and\n> > do the shift accordingly. Otherwise, it's hard to convey 'which way' the\n> > shift of 4 is. And what if someone wants to shift the otherway to make a\n> > histogram fit to 20 bits?\n> > \n> > Something like\n> > \n> > \tHistogram histogram{ hist };\n> > \n> > \t/*\n> > \t * The RKISP1 hardware histogram is 20 bits, but represent 16\n> > \t * bits integer and 4 bits are fractional. Shift to discard the\n> > \t * fractional components.\n> > \t */\n> > \thistogram >>= 4;\n> \n> What if we need other types of conversions in the future ? This doesn't\n> seem very generic. Passing a lambda function that transforms the value\n> would seem better, and would also be more readable. You don't want that\n> function to be called for every value without allowing for compiler\n> optimization though, so the constructor would likely need to be inlined.\n\nidk I can only imagine needing arithmetic operations... which we could\nadd as we need them...?\n\n> > This might be seen as less efficient, but I think the intent and\n> > operations are then clearer.\n\nYeah I think you're right.\n\n\nPaul\n\n> Efficiency-wise, that's not great indeed :-(\n> \n> > > /* Estimate the quantile mean of the top 2% of the histogram. */\n> > > return histogram.interQuantileMean(0.98, 1.0);\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 35AA5BD160\n\tfor ;\n\tWed, 27 Mar 2024 09:35:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5562163339;\n\tWed, 27 Mar 2024 10:35:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0780462CA2\n\tfor ;\n\tWed, 27 Mar 2024 10:35:50 +0100 (CET)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3FFEC505;\n\tWed, 27 Mar 2024 10:35:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hZAWwlb2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711532118;\n\tbh=9uBsam2v1J8lD4HCLnGnNupJOPX6Zq8PLkYPzsF7MnQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hZAWwlb2a1/LfLJHrarie+2xQGp/87HLDcWqalp9Q1BysXebEj26Alikq3hgh+wfV\n\tQd/hT/NHpGbiJFgxE+o532TdH1Gn0zGNuFphQ/DBWHIJvkNSQNb8DBk22otvTwtL/u\n\tMyTVT86z3HSyOuudOmrYthMxmQ8Q4+Tl+3ZeTxZc=","Date":"Wed, 27 Mar 2024 18:35:43 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","Message-ID":"","References":"<20240319105403.4045592-1-paul.elder@ideasonboard.com>\n\t<20240319105403.4045592-3-paul.elder@ideasonboard.com>\n\t<171085196722.1676185.17818719357426432548@ping.linuxembedded.co.uk>\n\t<20240319124849.GB29058@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240319124849.GB29058@pendragon.ideasonboard.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":29079,"web_url":"https://patchwork.libcamera.org/comment/29079/","msgid":"<171154001295.3566204.3560842836569847840@ping.linuxembedded.co.uk>","date":"2024-03-27T11:46:52","subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-03-27 09:35:43)\n> On Tue, Mar 19, 2024 at 02:48:49PM +0200, Laurent Pinchart wrote:\n> > On Tue, Mar 19, 2024 at 12:39:27PM +0000, Kieran Bingham wrote:\n> > > Quoting Paul Elder (2024-03-19 10:54:03)\n> > > > The histogram reported by the rkisp1 hardware is 20 bits, where the\n> > > > upper 16 bits are meaningful integer data and the lower 4 bits are\n> > > > fractional and meant to be discarded. Remove these 4 bits when\n> > > > constructing the histogram.\n> > \n> > Are they completely unusable, or are they fractional bits that could be\n> > helpful ? I wonder if we shouldn't drop them in the kernel if there's no\n> > way whatsoever to use them.\n> \n> The programming guide says to not use them. They're generated because\n> the hardware does weighting and it doesn't actually count every single\n> pixel, apparently.\n> \n> > > > \n> > > > Signed-off-by: Paul Elder \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 2 +-\n> > > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > > \n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index 47a6f7b2..278903a5 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span expMeans, double gain)\n> > > > */\n> > > > double Agc::measureBrightness(Span hist) const\n> > > > {\n> > > > - Histogram histogram{ hist };\n> > > > + Histogram histogram{ hist, 4 };\n> > > \n> > > On it's own this doesn't look very clear. So I think we need to add a\n> > > comment here above to say (in the code) why we're doing the shift.\n> > > \n> > > Also, and this is only an idea really - the passing of a parameter here\n> > > feels odd. I wonder if instead we should implement operator>>= and\n> > > operator<<= on the Histogram class which will iterate over each entry and\n> > > do the shift accordingly. Otherwise, it's hard to convey 'which way' the\n> > > shift of 4 is. And what if someone wants to shift the otherway to make a\n> > > histogram fit to 20 bits?\n> > > \n> > > Something like\n> > > \n> > > Histogram histogram{ hist };\n> > > \n> > > /*\n> > > * The RKISP1 hardware histogram is 20 bits, but represent 16\n> > > * bits integer and 4 bits are fractional. Shift to discard the\n> > > * fractional components.\n> > > */\n> > > histogram >>= 4;\n> > \n> > What if we need other types of conversions in the future ? This doesn't\n> > seem very generic. Passing a lambda function that transforms the value\n> > would seem better, and would also be more readable. You don't want that\n> > function to be called for every value without allowing for compiler\n> > optimization though, so the constructor would likely need to be inlined.\n> \n> idk I can only imagine needing arithmetic operations... which we could\n> add as we need them...?\n> \n> > > This might be seen as less efficient, but I think the intent and\n> > > operations are then clearer.\n> \n> Yeah I think you're right.\n> \n> \n> Paul\n> \n> > Efficiency-wise, that's not great indeed :-(\n\nSure, but what's our cost here. Is this a hot path? Or are we talking\nabout iterating a table of 5-10 values twice once to add them and once to\nshift them right? How big is the histogram expected to be?\n\nIf we're under a cacheline or two in size ~64 bytes? then is it better\nto prefer readability against over-optimisations?\n\nBut I'm probably fine with a lambda here too.\n\nHow would that look ? I can't quite figure out how to do this inline\nwithout a copy (which would probably make any performance optimisation redundant\nanyway?)\n\n\nIs this what you had in mind?\n\n double measureBrightness(std::span hist) const {\n std::vector shiftedHist;\n shiftedHist.reserve(hist.size());\n\n /*\n * The RKISP1 hardware histogram is 20 bits, but represents 16\n * bits integer and 4 bits are fractional. Shift to discard the\n * fractional components.\n */\n std::transform(hist.begin(), hist.end(),\n std::back_inserter(shiftedHist),\n [](uint32_t value) { return value >> 4; });\n\n Histogram histogram{ shiftedHist };\n\nFor full context, here's my sketch I tried to do it in:\n https://godbolt.org/z/e9sTbYncb\n\nI'm equally fine with something like the above though. And it puts the\nexpressiveness of what the adjustment is and why it's required in the\nplatform specific components.\n\n--\nKieran","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 F0969BD160\n\tfor ;\n\tWed, 27 Mar 2024 11:46:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E073663331;\n\tWed, 27 Mar 2024 12:46:57 +0100 (CET)","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 1748761C35\n\tfor ;\n\tWed, 27 Mar 2024 12:46:56 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9DD64675;\n\tWed, 27 Mar 2024 12:46:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ttFhblJZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711539983;\n\tbh=IdCllAJKCyuWBvD6AsVVpLfROyvbcs8BeL3ALPGY9uc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ttFhblJZPDFCxb52Nnh8mR8CJGB9Pr2qJ7l/4IZjjFqIJ0If6FaAza4POk1x3h12r\n\tNqsOh/i1eMyVW5rdG/hLZGfwCV88kz/gKSO82Od63L3Q/nh8PVTruVYYx2Y0V6LPtC\n\tNV5UM2UuAFAE8YBqaxZRhYX4XqnJpcc390WbOOSY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240319105403.4045592-1-paul.elder@ideasonboard.com>\n\t<20240319105403.4045592-3-paul.elder@ideasonboard.com>\n\t<171085196722.1676185.17818719357426432548@ping.linuxembedded.co.uk>\n\t<20240319124849.GB29058@pendragon.ideasonboard.com>\n\t","Subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tPaul Elder ","Date":"Wed, 27 Mar 2024 11:46:52 +0000","Message-ID":"<171154001295.3566204.3560842836569847840@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":29082,"web_url":"https://patchwork.libcamera.org/comment/29082/","msgid":"<-DSybNdMjM66n-HzqNmnIMXqOfV2AunF8l8GWlbMrA_izSnM4scU0Ntiz2YjRCLdFPg6o-SwKx1bgDnZmmAtvlFIo9H2F2D1694pkTmMG88=@protonmail.com>","date":"2024-03-27T13:44:23","subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. március 27., szerda 12:46 keltezéssel, Kieran Bingham írta:\n\n> Quoting Paul Elder (2024-03-27 09:35:43)\n> > On Tue, Mar 19, 2024 at 02:48:49PM +0200, Laurent Pinchart wrote:\n> > > On Tue, Mar 19, 2024 at 12:39:27PM +0000, Kieran Bingham wrote:\n> > > > Quoting Paul Elder (2024-03-19 10:54:03)\n> > > > > The histogram reported by the rkisp1 hardware is 20 bits, where the\n> > > > > upper 16 bits are meaningful integer data and the lower 4 bits are\n> > > > > fractional and meant to be discarded. Remove these 4 bits when\n> > > > > constructing the histogram.\n> > >\n> > > Are they completely unusable, or are they fractional bits that could be\n> > > helpful ? I wonder if we shouldn't drop them in the kernel if there's no\n> > > way whatsoever to use them.\n> >\n> > The programming guide says to not use them. They're generated because\n> > the hardware does weighting and it doesn't actually count every single\n> > pixel, apparently.\n> >\n> > > > >\n> > > > > Signed-off-by: Paul Elder \n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/agc.cpp | 2 +-\n> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > index 47a6f7b2..278903a5 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span expMeans, double gain)\n> > > > > */\n> > > > > double Agc::measureBrightness(Span hist) const\n> > > > > {\n> > > > > - Histogram histogram{ hist };\n> > > > > + Histogram histogram{ hist, 4 };\n> > > >\n> > > > On it's own this doesn't look very clear. So I think we need to add a\n> > > > comment here above to say (in the code) why we're doing the shift.\n> > > >\n> > > > Also, and this is only an idea really - the passing of a parameter here\n> > > > feels odd. I wonder if instead we should implement operator>>= and\n> > > > operator<<= on the Histogram class which will iterate over each entry and\n> > > > do the shift accordingly. Otherwise, it's hard to convey 'which way' the\n> > > > shift of 4 is. And what if someone wants to shift the otherway to make a\n> > > > histogram fit to 20 bits?\n> > > >\n> > > > Something like\n> > > >\n> > > > Histogram histogram{ hist };\n> > > >\n> > > > /*\n> > > > * The RKISP1 hardware histogram is 20 bits, but represent 16\n> > > > * bits integer and 4 bits are fractional. Shift to discard the\n> > > > * fractional components.\n> > > > */\n> > > > histogram >>= 4;\n> > >\n> > > What if we need other types of conversions in the future ? This doesn't\n> > > seem very generic. Passing a lambda function that transforms the value\n> > > would seem better, and would also be more readable. You don't want that\n> > > function to be called for every value without allowing for compiler\n> > > optimization though, so the constructor would likely need to be inlined.\n> >\n> > idk I can only imagine needing arithmetic operations... which we could\n> > add as we need them...?\n> >\n> > > > This might be seen as less efficient, but I think the intent and\n> > > > operations are then clearer.\n> >\n> > Yeah I think you're right.\n> >\n> >\n> > Paul\n> >\n> > > Efficiency-wise, that's not great indeed :-(\n> \n> Sure, but what's our cost here. Is this a hot path? Or are we talking\n> about iterating a table of 5-10 values twice once to add them and once to\n> shift them right? How big is the histogram expected to be?\n> \n> If we're under a cacheline or two in size ~64 bytes? then is it better\n> to prefer readability against over-optimisations?\n> \n> But I'm probably fine with a lambda here too.\n> \n> How would that look ? I can't quite figure out how to do this inline\n> without a copy (which would probably make any performance optimisation redundant\n> anyway?)\n> \n> \n> Is this what you had in mind?\n> \n> double measureBrightness(std::span hist) const {\n> std::vector shiftedHist;\n> shiftedHist.reserve(hist.size());\n> \n> /*\n> * The RKISP1 hardware histogram is 20 bits, but represents 16\n> * bits integer and 4 bits are fractional. Shift to discard the\n> * fractional components.\n> */\n> std::transform(hist.begin(), hist.end(),\n> std::back_inserter(shiftedHist),\n> [](uint32_t value) { return value >> 4; });\n> \n> Histogram histogram{ shiftedHist };\n> \n> For full context, here's my sketch I tried to do it in:\n> https://godbolt.org/z/e9sTbYncb\n> \n> I'm equally fine with something like the above though. And it puts the\n> expressiveness of what the adjustment is and why it's required in the\n> platform specific components.\n> [...]\n\nI know libcamera does not utilize C++20, but I just wanted to mention that the\nranges library would provide a nice solution to this, I think.\nE.g. you could do something like\n\n // e.g. std::span values\n histogram { values | std::views::transform([](auto x) { return x >> 4; }) }\n\n( https://gcc.godbolt.org/z/f88TGqGW6 )\n\nAnd the values would be shifted when creating inserting into the vector,\nwithout the need to allocate a separate vector, like in the code above.\n\n\nRegards,\nBarnabás Pőcze","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 4587BBD160\n\tfor ;\n\tWed, 27 Mar 2024 13:44:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2DCDE63331;\n\tWed, 27 Mar 2024 14:44:39 +0100 (CET)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 988A061C35\n\tfor ;\n\tWed, 27 Mar 2024 14:44:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"HyAnF1MO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1711547076; x=1711806276;\n\tbh=yJU9AF4VsnVc8UBd3vbHa8ul9ETMutLIgmA5fOlC6Zc=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=HyAnF1MOCzNoi3G15oZKqZXZPqDIugdeqo2pmkN2l2hmKVmRQAsjf8Lb/4LAN00wb\n\tVW6zThqos0OtGJgir9OaVXGQIjE+HO/RiRRFN05k+TOn9TFtBK610OLQhKknmJfqMD\n\t1g5pjiZti34yddfrpLzH/6TtzaiVmgZJYWgbJDf/3rysssizv07R5e1z1AAfXHq3H6\n\tpR/aSjv7Hel2uoxqU/5Eo0BAG79XsuDIIcCj0rtIbacv7ebQWk0hPz2DpcVGN60hKN\n\t17fHG9sm8h6micNAMnQixtI7lPhrBBKCp0570NUQi1yfv8QZqMX+61H8RgLlTmRcec\n\trTMKaxTUlisPg==","Date":"Wed, 27 Mar 2024 13:44:23 +0000","To":"Kieran Bingham ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Laurent Pinchart ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","Message-ID":"<-DSybNdMjM66n-HzqNmnIMXqOfV2AunF8l8GWlbMrA_izSnM4scU0Ntiz2YjRCLdFPg6o-SwKx1bgDnZmmAtvlFIo9H2F2D1694pkTmMG88=@protonmail.com>","In-Reply-To":"<171154001295.3566204.3560842836569847840@ping.linuxembedded.co.uk>","References":"<20240319105403.4045592-1-paul.elder@ideasonboard.com>\n\t<20240319105403.4045592-3-paul.elder@ideasonboard.com>\n\t<171085196722.1676185.17818719357426432548@ping.linuxembedded.co.uk>\n\t<20240319124849.GB29058@pendragon.ideasonboard.com>\n\t\n\t<171154001295.3566204.3560842836569847840@ping.linuxembedded.co.uk>","Feedback-ID":"20568564:user:proton","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":29086,"web_url":"https://patchwork.libcamera.org/comment/29086/","msgid":"<171155057061.252503.8096447340115444647@ping.linuxembedded.co.uk>","date":"2024-03-27T14:42:50","subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2024-03-27 13:44:23)\n> Hi\n> \n> > How would that look ? I can't quite figure out how to do this inline\n> > without a copy (which would probably make any performance optimisation redundant\n> > anyway?)\n> > \n> \n> > \n> \n> > Is this what you had in mind?\n> > \n> \n> > double measureBrightness(std::span hist) const {\n> > std::vector shiftedHist;\n> > shiftedHist.reserve(hist.size());\n> > \n> \n> > /*\n> > * The RKISP1 hardware histogram is 20 bits, but represents 16\n> > * bits integer and 4 bits are fractional. Shift to discard the\n> > * fractional components.\n> > */\n> > std::transform(hist.begin(), hist.end(),\n> > std::back_inserter(shiftedHist),\n> > [](uint32_t value) { return value >> 4; });\n> > \n> \n> > Histogram histogram{ shiftedHist };\n> > \n> \n> > For full context, here's my sketch I tried to do it in:\n> > https://godbolt.org/z/e9sTbYncb\n> > \n> \n> > I'm equally fine with something like the above though. And it puts the\n> > expressiveness of what the adjustment is and why it's required in the\n> > platform specific components.\n> > [...]\n> \n> I know libcamera does not utilize C++20, but I just wanted to mention that the\n> ranges library would provide a nice solution to this, I think.\n> E.g. you could do something like\n> \n> // e.g. std::span values\n> histogram { values | std::views::transform([](auto x) { return x >> 4; }) }\n> \n> ( https://gcc.godbolt.org/z/f88TGqGW6 )\n> \n> And the values would be shifted when creating inserting into the vector,\n> without the need to allocate a separate vector, like in the code above.\n\nAha, nice - that's what I thought I could try to do with a lambda,\nbut failed without the copy of course - hence the above.\n\nBut indeed, alas, we may be some time away from C++20.\n--\nKieran\n\n\n> \n> \n> Regards,\n> Barnabás Pőcze","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 72868C0DA4\n\tfor ;\n\tWed, 27 Mar 2024 14:42:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9AEF61C35;\n\tWed, 27 Mar 2024 15:42:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C89461C35\n\tfor ;\n\tWed, 27 Mar 2024 15:42:54 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8FA6713AC;\n\tWed, 27 Mar 2024 15:42:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nt99g4/2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711550541;\n\tbh=Pz0hFXSFjAXSrRTMzHSmwbffK2szk8sbslRgnSjyg90=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=nt99g4/2Af4NmCwAtoVhJAcvk8C5Ik/evVRvGIIP7j3ZHrzOXthn09vlBGSrk8iT1\n\tgvoQjnw567JKwho6XwByZJJI78/NUbzgR4Q58MIOqn89ZCnY+o4AHAi8frE/oY8VOX\n\tqyHjoGZSJCJD/BnanoRnMRXN1lUyxopeB36tl+uM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<-DSybNdMjM66n-HzqNmnIMXqOfV2AunF8l8GWlbMrA_izSnM4scU0Ntiz2YjRCLdFPg6o-SwKx1bgDnZmmAtvlFIo9H2F2D1694pkTmMG88=@protonmail.com>","References":"<20240319105403.4045592-1-paul.elder@ideasonboard.com>\n\t<20240319105403.4045592-3-paul.elder@ideasonboard.com>\n\t<171085196722.1676185.17818719357426432548@ping.linuxembedded.co.uk>\n\t<20240319124849.GB29058@pendragon.ideasonboard.com>\n\t\n\t<171154001295.3566204.3560842836569847840@ping.linuxembedded.co.uk>\n\t<-DSybNdMjM66n-HzqNmnIMXqOfV2AunF8l8GWlbMrA_izSnM4scU0Ntiz2YjRCLdFPg6o-SwKx1bgDnZmmAtvlFIo9H2F2D1694pkTmMG88=@protonmail.com>","Subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","From":"Kieran Bingham ","Cc":"Laurent Pinchart ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Date":"Wed, 27 Mar 2024 14:42:50 +0000","Message-ID":"<171155057061.252503.8096447340115444647@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":29091,"web_url":"https://patchwork.libcamera.org/comment/29091/","msgid":"","date":"2024-03-27T15:21:17","subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. március 27., szerda 15:42 keltezéssel, Kieran Bingham írta:\n\n> Quoting Barnabás Pőcze (2024-03-27 13:44:23)\n> > Hi\n> >\n> > > How would that look ? I can't quite figure out how to do this inline\n> > > without a copy (which would probably make any performance optimisation redundant\n> > > anyway?)\n> > >\n> >\n> > >\n> >\n> > > Is this what you had in mind?\n> > >\n> >\n> > > double measureBrightness(std::span hist) const {\n> > > std::vector shiftedHist;\n> > > shiftedHist.reserve(hist.size());\n> > >\n> >\n> > > /*\n> > > * The RKISP1 hardware histogram is 20 bits, but represents 16\n> > > * bits integer and 4 bits are fractional. Shift to discard the\n> > > * fractional components.\n> > > */\n> > > std::transform(hist.begin(), hist.end(),\n> > > std::back_inserter(shiftedHist),\n> > > [](uint32_t value) { return value >> 4; });\n> > >\n> >\n> > > Histogram histogram{ shiftedHist };\n> > >\n> >\n> > > For full context, here's my sketch I tried to do it in:\n> > > https://godbolt.org/z/e9sTbYncb\n> > >\n> >\n> > > I'm equally fine with something like the above though. And it puts the\n> > > expressiveness of what the adjustment is and why it's required in the\n> > > platform specific components.\n> > > [...]\n> >\n> > I know libcamera does not utilize C++20, but I just wanted to mention that the\n> > ranges library would provide a nice solution to this, I think.\n> > E.g. you could do something like\n> >\n> > // e.g. std::span values\n> > histogram { values | std::views::transform([](auto x) { return x >> 4; }) }\n> >\n> > ( https://gcc.godbolt.org/z/f88TGqGW6 )\n> >\n> > And the values would be shifted when creating inserting into the vector,\n> > without the need to allocate a separate vector, like in the code above.\n> \n> Aha, nice - that's what I thought I could try to do with a lambda,\n> but failed without the copy of course - hence the above.\n> [...]\n\nThe constructor could take a lambda like this, which should be equally performant,\neven if a bit more restricted than what is possible with ranges:\n\n template, std::uint32_t>> * = nullptr>\n histogram(std::span values, Func transform)\n {\n for (auto x : values)\n this->values.push_back(transform(x));\n ...\n }\n\n( https://gcc.godbolt.org/z/P35GrYKzn )\n\n> \n> But indeed, alas, we may be some time away from C++20.\n\nAre there any plans by the way? What are the criteria?\n\n\nRegards,\nBarnabás Pőcze","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 AAA35C0DA4\n\tfor ;\n\tWed, 27 Mar 2024 15:21:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B3B4463339;\n\tWed, 27 Mar 2024 16:21:26 +0100 (CET)","from mail-40131.protonmail.ch (mail-40131.protonmail.ch\n\t[185.70.40.131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7DE7D61C41\n\tfor ;\n\tWed, 27 Mar 2024 16:21:24 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"FZje+cPh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1711552883; x=1711812083;\n\tbh=i9KTfWjsdstFTf54H6/HVWnE5CcesL5GiKFgOybNQcM=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=FZje+cPhf+ZH6Yb5OH6mBaeB3rpb53PnuE1jx177iklcwt0q0r2cZLNRGsLd+1Jj8\n\tnGGVqNAsG42arUgIT5JNUEsyVhT/lvvk60r0uAb6mYX35lvqx1bZ5ungYknsPG43n7\n\tqJipLxgG3x7jaEDMRd/r2udsSc3Avfp83O/cQjYc//paLBWbnIJ3FgyVHpXn3Ug8PY\n\tQhpmSga6jRi1sx0fq1SdedcKNZ61JdjpfeBTrHidL94EPASGkB3nNxY9ZYcJdk0hWB\n\totTdHRmJDNZAtQduYXPRVXKhts/j5jWzQgrF7CLuXywZPUnTkOKgbfxRT2tyaUzt3V\n\tw79JHqq2pt8fw==","Date":"Wed, 27 Mar 2024 15:21:17 +0000","To":"Kieran Bingham ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Laurent Pinchart ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","Message-ID":"","In-Reply-To":"<171155057061.252503.8096447340115444647@ping.linuxembedded.co.uk>","References":"<20240319105403.4045592-1-paul.elder@ideasonboard.com>\n\t<20240319105403.4045592-3-paul.elder@ideasonboard.com>\n\t<171085196722.1676185.17818719357426432548@ping.linuxembedded.co.uk>\n\t<20240319124849.GB29058@pendragon.ideasonboard.com>\n\t\n\t<171154001295.3566204.3560842836569847840@ping.linuxembedded.co.uk>\n\t<-DSybNdMjM66n-HzqNmnIMXqOfV2AunF8l8GWlbMrA_izSnM4scU0Ntiz2YjRCLdFPg6o-SwKx1bgDnZmmAtvlFIo9H2F2D1694pkTmMG88=@protonmail.com>\n\t<171155057061.252503.8096447340115444647@ping.linuxembedded.co.uk>","Feedback-ID":"20568564:user:proton","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":29365,"web_url":"https://patchwork.libcamera.org/comment/29365/","msgid":"<20240430090134.GA3714@pendragon.ideasonboard.com>","date":"2024-04-30T09:01:34","subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nOn Wed, Mar 27, 2024 at 03:21:17PM +0000, Barnabás Pőcze wrote:\n> 2024. március 27., szerda 15:42 keltezéssel, Kieran Bingham írta:\n> \n> > Quoting Barnabás Pőcze (2024-03-27 13:44:23)\n> > > Hi\n> > >\n> > > > How would that look ? I can't quite figure out how to do this inline\n> > > > without a copy (which would probably make any performance optimisation redundant\n> > > > anyway?)\n> > > >\n> > > >\n> > > > Is this what you had in mind?\n> > > >\n> > > > double measureBrightness(std::span hist) const {\n> > > > std::vector shiftedHist;\n> > > > shiftedHist.reserve(hist.size());\n> > > >\n> > > > /*\n> > > > * The RKISP1 hardware histogram is 20 bits, but represents 16\n> > > > * bits integer and 4 bits are fractional. Shift to discard the\n> > > > * fractional components.\n> > > > */\n> > > > std::transform(hist.begin(), hist.end(),\n> > > > std::back_inserter(shiftedHist),\n> > > > [](uint32_t value) { return value >> 4; });\n> > > >\n> > > > Histogram histogram{ shiftedHist };\n> > > >\n> > > > For full context, here's my sketch I tried to do it in:\n> > > > https://godbolt.org/z/e9sTbYncb\n> > > >\n> > > > I'm equally fine with something like the above though. And it puts the\n> > > > expressiveness of what the adjustment is and why it's required in the\n> > > > platform specific components.\n> > > > [...]\n> > >\n> > > I know libcamera does not utilize C++20, but I just wanted to mention that the\n> > > ranges library would provide a nice solution to this, I think.\n> > > E.g. you could do something like\n> > >\n> > > // e.g. std::span values\n> > > histogram { values | std::views::transform([](auto x) { return x >> 4; }) }\n> > >\n> > > ( https://gcc.godbolt.org/z/f88TGqGW6 )\n> > >\n> > > And the values would be shifted when creating inserting into the vector,\n> > > without the need to allocate a separate vector, like in the code above.\n> > \n> > Aha, nice - that's what I thought I could try to do with a lambda,\n> > but failed without the copy of course - hence the above.\n> > [...]\n> \n> The constructor could take a lambda like this, which should be equally performant,\n> even if a bit more restricted than what is possible with ranges:\n> \n> template, std::uint32_t>> * = nullptr>\n> histogram(std::span values, Func transform)\n> {\n> for (auto x : values)\n> this->values.push_back(transform(x));\n> ...\n> }\n> \n> ( https://gcc.godbolt.org/z/P35GrYKzn )\n> \n> > \n> > But indeed, alas, we may be some time away from C++20.\n> \n> Are there any plans by the way? What are the criteria?\n\nWe have plans to move to C++20 only to the extent that we think we'll do\nit one day :-) The requirements include\n\n- Having the language well supported by the compilers in LTS releases of\n major distributions.\n\n We're probably there for the latest LTS releases, but maybe not quite\n for all LTS releases that haven't reached their EOL. I haven't\n checked.\n\n- Having ChromeOS and Android HAL layers remain compatible\n\n We currently implement the legacy C-based Android HAL interface. The\n new HAL interface's ABI is based binder protocols. We should thus be\n fine on Android from an ABI point of view. I haven't checked if the\n Android toolchain supports C++20 correctly though.\n\n On Chrome OS the situation is a bit different. The HAL ABI is C-based,\n but we link to Chrome OS C++ libraries. Chrome OS hasn't switched to\n C++20 yet, so we would probably have issues there.\n\n- Minimizing breakage for our users\n\n Existing applications using libcamera may be compatible with C++20.\n They would need to upgrade, and we need to give them enough time to do\n so. This is the criteria that is the most difficult to quantify. I\n don't have any particular worry in mind personally.","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 EB109C3220\n\tfor ;\n\tTue, 30 Apr 2024 09:01:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D97063418;\n\tTue, 30 Apr 2024 11:01:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41F36633EB\n\tfor ;\n\tTue, 30 Apr 2024 11:01:41 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(237.69-130-109.adsl-dyn.isp.belgacom.be [109.130.69.237])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 00CB066F;\n\tTue, 30 Apr 2024 11:00:44 +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=\"ddGCC+fT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1714467645;\n\tbh=hmPvQ7uXNmxvFAWT0r1Hf5hfIoGpQXfMDeXBTHq5Ecg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ddGCC+fTprkFOkVZK0VTRUxMNzc0i2EKrzpqNUT2tlg+h36gkdDC/Qp73ZUOiwjU/\n\taWPxZKUKbCqGNy62eO0aTN45vpauwztdm6vmdG6ugbCc1AmRa24O170UkExDYroOd4\n\tAgzq7cZIilhMauNu2myyy3HXIecC04kgpkFroDac=","Date":"Tue, 30 Apr 2024 12:01:34 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction","Message-ID":"<20240430090134.GA3714@pendragon.ideasonboard.com>","References":"<20240319105403.4045592-1-paul.elder@ideasonboard.com>\n\t<20240319105403.4045592-3-paul.elder@ideasonboard.com>\n\t<171085196722.1676185.17818719357426432548@ping.linuxembedded.co.uk>\n\t<20240319124849.GB29058@pendragon.ideasonboard.com>\n\t\n\t<171154001295.3566204.3560842836569847840@ping.linuxembedded.co.uk>\n\t<-DSybNdMjM66n-HzqNmnIMXqOfV2AunF8l8GWlbMrA_izSnM4scU0Ntiz2YjRCLdFPg6o-SwKx1bgDnZmmAtvlFIo9H2F2D1694pkTmMG88=@protonmail.com>\n\t<171155057061.252503.8096447340115444647@ping.linuxembedded.co.uk>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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\" "}}]