[{"id":29280,"web_url":"https://patchwork.libcamera.org/comment/29280/","msgid":"<171355946360.3257362.6093408626960336117@ping.linuxembedded.co.uk>","date":"2024-04-19T20:44:23","subject":"Re: [PATCH v2] ipa: libipa: histogram: Add transform parameter to\n\tconstructor","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-04-19 06:53:36)\n> Add a parameter to the histogram constructor that takes a transformation\n> function to apply to all the bins upon construction.\n> \n> This is necessary notably for the rkisp1, as the values reported from\n> the hardware are 20 bits where the upper 16-bits are meaningful integer\n> values and the lower 4 bits are fractional and meant to be discarded. As\n> adding a right-shift parameter is probably too specialized, a generic\n> function is added as a parameter instead.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> This used to be \"ipa: libipa: histogram: Add rshift parameter to\n> constructor\"\n> \n> Changes in v2:\n> - change rshift parameter to a function parameter\n> ---\n> src/ipa/libipa/histogram.cpp | 6 ++++--\n> src/ipa/libipa/histogram.h | 6 ++++--\n> 2 files changed, 8 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp\n> index c1aac59b..2a4095f4 100644\n> --- a/src/ipa/libipa/histogram.cpp\n> +++ b/src/ipa/libipa/histogram.cpp\n> @@ -40,13 +40,15 @@ namespace ipa {\n> /**\n> * \\brief Create a cumulative histogram\n> * \\param[in] data A pre-sorted histogram to be passed\n> + * \\param[in] transform The transformation function to apply to every bin\n\nI would have likely said \"A transformation function to apply to every\nbin\" but ... it doesn't really impact here.\n\n> */\n> -Histogram::Histogram(Span data)\n> +Histogram::Histogram(Span data,\n> + std::function transform)\n> {\n> cumulative_.reserve(data.size());\n> cumulative_.push_back(0);\n> for (const uint32_t &value : data)\n> - cumulative_.push_back(cumulative_.back() + value);\n> + cumulative_.push_back(cumulative_.back() + transform(value));\n> }\n> \n> /**\n> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h\n> index 54bb2a19..89a6b550 100644\n> --- a/src/ipa/libipa/histogram.h\n> +++ b/src/ipa/libipa/histogram.h\n> @@ -8,9 +8,9 @@\n> #pragma once\n> \n> #include \n> +#include \n> #include \n> #include \n> -\n> #include \n> \n> #include \n> @@ -23,7 +23,9 @@ class Histogram\n> {\n> public:\n> Histogram() { cumulative_.push_back(0); }\n> - Histogram(Span data);\n> + Histogram(Span data,\n> + std::function transform =\n> + [](uint32_t x) { return x; });\n\nWell, that default transform is likely quite easy for a compiler to\noptimise inline so I think this is as generic as we get here!\n\nThanks for reworking.\n\nReviewed-by: Kieran Bingham \n\n> size_t bins() const { return cumulative_.size() - 1; }\n> uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }\n> uint64_t cumulativeFrequency(double bin) const;\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 9D974BE08B\n\tfor ;\n\tFri, 19 Apr 2024 20:44:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0F6C633F4;\n\tFri, 19 Apr 2024 22:44:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C27C661C15\n\tfor ;\n\tFri, 19 Apr 2024 22:44:26 +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 0C15B802;\n\tFri, 19 Apr 2024 22:43:38 +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=\"WQgcOSG5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713559418;\n\tbh=kZguDPXzFlgAy9xLUidELkYhsD3kclNsAUfJZTm0hP8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=WQgcOSG53iMAIuW9WyAzJnP9Z+Hq4lExPLpP36v9OiLm0Ax2qh8sv+gr4GRrzGeod\n\tgucUnL1OM0bt0hPJpsZGyObHBzr2a5dQ655kdOE+ivJgRF9Qlhfw0pTVDx4/dkdYRP\n\tG/mwB6KypmCfBC7TqIPyWLlR8TgqDPbR/M7U8XYs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240419055336.1070164-1-paul.elder@ideasonboard.com>","References":"<20240419055336.1070164-1-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v2] ipa: libipa: histogram: Add transform parameter to\n\tconstructor","From":"Kieran Bingham ","Cc":"Paul Elder ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 19 Apr 2024 21:44:23 +0100","Message-ID":"<171355946360.3257362.6093408626960336117@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":29304,"web_url":"https://patchwork.libcamera.org/comment/29304/","msgid":"<7e18cad4-8d23-4e80-bddd-14500453c853@ideasonboard.com>","date":"2024-04-22T22:38:14","subject":"Re: [PATCH v2] ipa: libipa: histogram: Add transform parameter to\n\tconstructor","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"On 19/04/2024 21:44, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-04-19 06:53:36)\n>> Add a parameter to the histogram constructor that takes a transformation\n>> function to apply to all the bins upon construction.\n>>\n>> This is necessary notably for the rkisp1, as the values reported from\n>> the hardware are 20 bits where the upper 16-bits are meaningful integer\n>> values and the lower 4 bits are fractional and meant to be discarded. As\n>> adding a right-shift parameter is probably too specialized, a generic\n>> function is added as a parameter instead.\n>>\n>> Signed-off-by: Paul Elder \n>>\n>> ---\n>> This used to be \"ipa: libipa: histogram: Add rshift parameter to\n>> constructor\"\n>>\n>> Changes in v2:\n>> - change rshift parameter to a function parameter\n>> ---\n>> src/ipa/libipa/histogram.cpp | 6 ++++--\n>> src/ipa/libipa/histogram.h | 6 ++++--\n>> 2 files changed, 8 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp\n>> index c1aac59b..2a4095f4 100644\n>> --- a/src/ipa/libipa/histogram.cpp\n>> +++ b/src/ipa/libipa/histogram.cpp\n>> @@ -40,13 +40,15 @@ namespace ipa {\n>> /**\n>> * \\brief Create a cumulative histogram\n>> * \\param[in] data A pre-sorted histogram to be passed\n>> + * \\param[in] transform The transformation function to apply to every bin\n> I would have likely said \"A transformation function to apply to every\n> bin\" but ... it doesn't really impact here.\n\n\nI had the same thought...on the grounds that \"The\" implies it's mandatory whereas \"A\" does not. But \nI  think I'm just being picky:\n\n\nReviewed-by: Daniel Scally \n\n>\n>> */\n>> -Histogram::Histogram(Span data)\n>> +Histogram::Histogram(Span data,\n>> + std::function transform)\n>> {\n>> cumulative_.reserve(data.size());\n>> cumulative_.push_back(0);\n>> for (const uint32_t &value : data)\n>> - cumulative_.push_back(cumulative_.back() + value);\n>> + cumulative_.push_back(cumulative_.back() + transform(value));\n>> }\n>> \n>> /**\n>> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h\n>> index 54bb2a19..89a6b550 100644\n>> --- a/src/ipa/libipa/histogram.h\n>> +++ b/src/ipa/libipa/histogram.h\n>> @@ -8,9 +8,9 @@\n>> #pragma once\n>> \n>> #include \n>> +#include \n>> #include \n>> #include \n>> -\n>> #include \n>> \n>> #include \n>> @@ -23,7 +23,9 @@ class Histogram\n>> {\n>> public:\n>> Histogram() { cumulative_.push_back(0); }\n>> - Histogram(Span data);\n>> + Histogram(Span data,\n>> + std::function transform =\n>> + [](uint32_t x) { return x; });\n> Well, that default transform is likely quite easy for a compiler to\n> optimise inline so I think this is as generic as we get here!\n>\n> Thanks for reworking.\n>\n> Reviewed-by: Kieran Bingham \n>\n>> size_t bins() const { return cumulative_.size() - 1; }\n>> uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }\n>> uint64_t cumulativeFrequency(double bin) const;\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 8549BC3200\n\tfor ;\n\tMon, 22 Apr 2024 22:38:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C10D563416;\n\tTue, 23 Apr 2024 00:38:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DACA633FA\n\tfor ;\n\tTue, 23 Apr 2024 00:38:17 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4AFAAE4\n\tfor ;\n\tTue, 23 Apr 2024 00:37:26 +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=\"iQjEW390\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713825446;\n\tbh=EaAN5SVN1HAaQHsHvTVMg83p4bynLwTcAIHMNI4XjHw=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=iQjEW390gDXBIbon2fOciO/MyxDq5qs8LpWPmwb+ns+VPhC5JgRkLxVKTusU72gyU\n\tiqsVoB7x9l2RU2jPd72LtXTXwG4BrjXJP0fzPAfi53B8XeFw/stGCAl/1h8h2BFVyw\n\tRzefd19mONr1JElNc04JUsnglJwcIBQ/rUdYKmrg=","Message-ID":"<7e18cad4-8d23-4e80-bddd-14500453c853@ideasonboard.com>","Date":"Mon, 22 Apr 2024 23:38:14 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2] ipa: libipa: histogram: Add transform parameter to\n\tconstructor","To":"libcamera-devel@lists.libcamera.org","References":"<20240419055336.1070164-1-paul.elder@ideasonboard.com>\n\t<171355946360.3257362.6093408626960336117@ping.linuxembedded.co.uk>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<171355946360.3257362.6093408626960336117@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":29321,"web_url":"https://patchwork.libcamera.org/comment/29321/","msgid":"","date":"2024-04-23T14:21:20","subject":"Re: [PATCH v2] ipa: libipa: histogram: Add transform parameter to\n\tconstructor","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. április 19., péntek 22:44 keltezéssel, Kieran Bingham írta:\n\n> Quoting Paul Elder (2024-04-19 06:53:36)\n> > Add a parameter to the histogram constructor that takes a transformation\n> > function to apply to all the bins upon construction.\n> >\n> > This is necessary notably for the rkisp1, as the values reported from\n> > the hardware are 20 bits where the upper 16-bits are meaningful integer\n> > values and the lower 4 bits are fractional and meant to be discarded. As\n> > adding a right-shift parameter is probably too specialized, a generic\n> > function is added as a parameter instead.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > This used to be \"ipa: libipa: histogram: Add rshift parameter to\n> > constructor\"\n> >\n> > Changes in v2:\n> > - change rshift parameter to a function parameter\n> > ---\n> > src/ipa/libipa/histogram.cpp | 6 ++++--\n> > src/ipa/libipa/histogram.h | 6 ++++--\n> > 2 files changed, 8 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp\n> > index c1aac59b..2a4095f4 100644\n> > --- a/src/ipa/libipa/histogram.cpp\n> > +++ b/src/ipa/libipa/histogram.cpp\n> > @@ -40,13 +40,15 @@ namespace ipa {\n> > /**\n> > * \\brief Create a cumulative histogram\n> > * \\param[in] data A pre-sorted histogram to be passed\n> > + * \\param[in] transform The transformation function to apply to every bin\n> \n> I would have likely said \"A transformation function to apply to every\n> bin\" but ... it doesn't really impact here.\n> \n> > */\n> > -Histogram::Histogram(Span data)\n> > +Histogram::Histogram(Span data,\n> > + std::function transform)\n> > {\n> > cumulative_.reserve(data.size());\n> > cumulative_.push_back(0);\n> > for (const uint32_t &value : data)\n> > - cumulative_.push_back(cumulative_.back() + value);\n> > + cumulative_.push_back(cumulative_.back() + transform(value));\n> > }\n> >\n> > /**\n> > diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h\n> > index 54bb2a19..89a6b550 100644\n> > --- a/src/ipa/libipa/histogram.h\n> > +++ b/src/ipa/libipa/histogram.h\n> > @@ -8,9 +8,9 @@\n> > #pragma once\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > -\n> > #include \n> >\n> > #include \n> > @@ -23,7 +23,9 @@ class Histogram\n> > {\n> > public:\n> > Histogram() { cumulative_.push_back(0); }\n> > - Histogram(Span data);\n> > + Histogram(Span data,\n> > + std::function transform =\n> > + [](uint32_t x) { return x; });\n> \n> Well, that default transform is likely quite easy for a compiler to\n> optimise inline so I think this is as generic as we get here!\n\nUnfortunately, there will be no inlining without LTO here. And std::function has\nnot insignificant overhead even if inlined. For inlining to happen reliably and\nwithout much overhead, the constructor would need to be templated and inline.\n\n\n> \n> Thanks for reworking.\n> \n> Reviewed-by: Kieran Bingham \n> \n> > size_t bins() const { return cumulative_.size() - 1; }\n> > uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }\n> > uint64_t cumulativeFrequency(double bin) const;\n> > --\n> > 2.39.2\n> >\n> \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 E23F5BE08B\n\tfor ;\n\tTue, 23 Apr 2024 14:21:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED61A61B28;\n\tTue, 23 Apr 2024 16:21:29 +0200 (CEST)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8199061AC9\n\tfor ;\n\tTue, 23 Apr 2024 16:21:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"DyglsiXo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1713882087; x=1714141287;\n\tbh=4CwpfJTLBwVCraQreDHn0t3d8OAr6icoI7e3ObJ4vI8=;\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=DyglsiXo+Wr//7zwxyFhNIpogLDSAsZXH5PNWDrDorAMaIqq66h4HSfHQiYx71KsO\n\tf24lNwK86geeWDmODgxZrv+nLgUZJ3H/IeA8EO4dgnmMZzT70ZrIOeZY+D/eHLv/kA\n\tv5NvxH8uT4uTUpJchG3irWKVvp23dln0vqoYclvNz4c42GKUFR+/NemEAgE2dvUYmf\n\tHVVjN3QoM8/8PFeGAewb6BoNjU1OAx18TgeBGlb0Zj5v6A1Nlk8G0z3uePiIJQe/Bo\n\tdw51+jXUK2N/sRzI3WC5y3XNHG6/PCaF9panZG3AohAcaRb52XRX0YUvWjaP6VRVpG\n\tmFL/aAvS3CVSw==","Date":"Tue, 23 Apr 2024 14:21:20 +0000","To":"Kieran Bingham ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2] ipa: libipa: histogram: Add transform parameter to\n\tconstructor","Message-ID":"","In-Reply-To":"<171355946360.3257362.6093408626960336117@ping.linuxembedded.co.uk>","References":"<20240419055336.1070164-1-paul.elder@ideasonboard.com>\n\t<171355946360.3257362.6093408626960336117@ping.linuxembedded.co.uk>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"4830612af3929e024c666a44789546b01cc884e0","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":29402,"web_url":"https://patchwork.libcamera.org/comment/29402/","msgid":"<20240503012057.GK4959@pendragon.ideasonboard.com>","date":"2024-05-03T01:20:57","subject":"Re: [PATCH v2] ipa: libipa: histogram: Add transform parameter to\n\tconstructor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Apr 23, 2024 at 02:21:20PM +0000, Barnabás Pőcze wrote:\n> Hi\n> \n> \n> 2024. április 19., péntek 22:44 keltezéssel, Kieran Bingham írta:\n> \n> > Quoting Paul Elder (2024-04-19 06:53:36)\n> > > Add a parameter to the histogram constructor that takes a transformation\n> > > function to apply to all the bins upon construction.\n> > >\n> > > This is necessary notably for the rkisp1, as the values reported from\n> > > the hardware are 20 bits where the upper 16-bits are meaningful integer\n> > > values and the lower 4 bits are fractional and meant to be discarded. As\n> > > adding a right-shift parameter is probably too specialized, a generic\n> > > function is added as a parameter instead.\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > This used to be \"ipa: libipa: histogram: Add rshift parameter to\n> > > constructor\"\n> > >\n> > > Changes in v2:\n> > > - change rshift parameter to a function parameter\n> > > ---\n> > > src/ipa/libipa/histogram.cpp | 6 ++++--\n> > > src/ipa/libipa/histogram.h | 6 ++++--\n> > > 2 files changed, 8 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp\n> > > index c1aac59b..2a4095f4 100644\n> > > --- a/src/ipa/libipa/histogram.cpp\n> > > +++ b/src/ipa/libipa/histogram.cpp\n> > > @@ -40,13 +40,15 @@ namespace ipa {\n> > > /**\n> > > * \\brief Create a cumulative histogram\n> > > * \\param[in] data A pre-sorted histogram to be passed\n> > > + * \\param[in] transform The transformation function to apply to every bin\n> > \n> > I would have likely said \"A transformation function to apply to every\n> > bin\" but ... it doesn't really impact here.\n> > \n> > > */\n> > > -Histogram::Histogram(Span data)\n> > > +Histogram::Histogram(Span data,\n> > > + std::function transform)\n> > > {\n> > > cumulative_.reserve(data.size());\n> > > cumulative_.push_back(0);\n> > > for (const uint32_t &value : data)\n> > > - cumulative_.push_back(cumulative_.back() + value);\n> > > + cumulative_.push_back(cumulative_.back() + transform(value));\n> > > }\n> > >\n> > > /**\n> > > diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h\n> > > index 54bb2a19..89a6b550 100644\n> > > --- a/src/ipa/libipa/histogram.h\n> > > +++ b/src/ipa/libipa/histogram.h\n> > > @@ -8,9 +8,9 @@\n> > > #pragma once\n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > -\n> > > #include \n> > >\n> > > #include \n> > > @@ -23,7 +23,9 @@ class Histogram\n> > > {\n> > > public:\n> > > Histogram() { cumulative_.push_back(0); }\n> > > - Histogram(Span data);\n> > > + Histogram(Span data,\n> > > + std::function transform =\n> > > + [](uint32_t x) { return x; });\n> > \n> > Well, that default transform is likely quite easy for a compiler to\n> > optimise inline so I think this is as generic as we get here!\n> \n> Unfortunately, there will be no inlining without LTO here. And std::function has\n> not insignificant overhead even if inlined. For inlining to happen reliably and\n> without much overhead, the constructor would need to be templated and inline.\n\nDo you mean passing the transform as a template parameter ? That could\nbe a good idea, I don't expect a big difference in code size.\n\nPaul, could you give it a try, and check how big the constructor gets ?\n\n> > Thanks for reworking.\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > size_t bins() const { return cumulative_.size() - 1; }\n> > > uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }\n> > > uint64_t cumulativeFrequency(double bin) const;","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 54411C3220\n\tfor ;\n\tFri, 3 May 2024 01:21:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F7F563418;\n\tFri, 3 May 2024 03:21:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87A2E62C9F\n\tfor ;\n\tFri, 3 May 2024 03:21:05 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 47ED73A3;\n\tFri, 3 May 2024 03:20:07 +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=\"rUKojX11\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1714699207;\n\tbh=/cznh3cBJasMyxvc71B1uB33eN21xEJdFflRn0LnvwM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rUKojX115iZS+yBYmnnk4nApa9ERnAf7tHt7RhIMnYkVjWsNOaY5eRGsFt6QpJffZ\n\txN6qmC8H4IvF6JOcFumDnboOnUJU7htFe/y9S1ecbkwkBQlN00+vTjhG++3spqVxQy\n\tnqTRGn7LQp2wGgFa/l26XYGs3aAhhQStjWUfI1I0=","Date":"Fri, 3 May 2024 04:20:57 +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 v2] ipa: libipa: histogram: Add transform parameter to\n\tconstructor","Message-ID":"<20240503012057.GK4959@pendragon.ideasonboard.com>","References":"<20240419055336.1070164-1-paul.elder@ideasonboard.com>\n\t<171355946360.3257362.6093408626960336117@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\" "}},{"id":29403,"web_url":"https://patchwork.libcamera.org/comment/29403/","msgid":"","date":"2024-05-03T01:43:22","subject":"Re: [PATCH v2] ipa: libipa: histogram: Add transform parameter to\n\tconstructor","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ájus 3., péntek 3:20 keltezéssel, Laurent Pinchart írta:\n\n> On Tue, Apr 23, 2024 at 02:21:20PM +0000, Barnabás Pőcze wrote:\n> > Hi\n> >\n> >\n> > 2024. április 19., péntek 22:44 keltezéssel, Kieran Bingham írta:\n> >\n> > > Quoting Paul Elder (2024-04-19 06:53:36)\n> > > > Add a parameter to the histogram constructor that takes a transformation\n> > > > function to apply to all the bins upon construction.\n> > > >\n> > > > This is necessary notably for the rkisp1, as the values reported from\n> > > > the hardware are 20 bits where the upper 16-bits are meaningful integer\n> > > > values and the lower 4 bits are fractional and meant to be discarded. As\n> > > > adding a right-shift parameter is probably too specialized, a generic\n> > > > function is added as a parameter instead.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > This used to be \"ipa: libipa: histogram: Add rshift parameter to\n> > > > constructor\"\n> > > >\n> > > > Changes in v2:\n> > > > - change rshift parameter to a function parameter\n> > > > ---\n> > > > src/ipa/libipa/histogram.cpp | 6 ++++--\n> > > > src/ipa/libipa/histogram.h | 6 ++++--\n> > > > 2 files changed, 8 insertions(+), 4 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp\n> > > > index c1aac59b..2a4095f4 100644\n> > > > --- a/src/ipa/libipa/histogram.cpp\n> > > > +++ b/src/ipa/libipa/histogram.cpp\n> > > > @@ -40,13 +40,15 @@ namespace ipa {\n> > > > [...]\n> > > > -Histogram::Histogram(Span data)\n> > > > +Histogram::Histogram(Span data,\n> > > > + std::function transform)\n> > > > {\n> > > > cumulative_.reserve(data.size());\n> > > > cumulative_.push_back(0);\n> > > > for (const uint32_t &value : data)\n> > > > - cumulative_.push_back(cumulative_.back() + value);\n> > > > + cumulative_.push_back(cumulative_.back() + transform(value));\n> > > > }\n> > > >\n> > > > /**\n> > > > diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h\n> > > > index 54bb2a19..89a6b550 100644\n> > > > --- a/src/ipa/libipa/histogram.h\n> > > > +++ b/src/ipa/libipa/histogram.h\n> > > > [...]\n> > > > @@ -23,7 +23,9 @@ class Histogram\n> > > > {\n> > > > public:\n> > > > Histogram() { cumulative_.push_back(0); }\n> > > > - Histogram(Span data);\n> > > > + Histogram(Span data,\n> > > > + std::function transform =\n> > > > + [](uint32_t x) { return x; });\n> > >\n> > > Well, that default transform is likely quite easy for a compiler to\n> > > optimise inline so I think this is as generic as we get here!\n> >\n> > Unfortunately, there will be no inlining without LTO here. And std::function has\n> > not insignificant overhead even if inlined. For inlining to happen reliably and\n> > without much overhead, the constructor would need to be templated and inline.\n> \n> Do you mean passing the transform as a template parameter ? That could\n> be a good idea, I don't expect a big difference in code size.\n\nYes.\n\n\n> \n> Paul, could you give it a try, and check how big the constructor gets ?\n\nAvoiding the push_back()s would also curb the code size somewhat.\nE.g.\n\n template\n Histogram(Span data, Transform transform)\n {\n cumulative_.resize(data.size() + 1);\n cumulative_[0] = 0;\n for (const auto &[i, val] : enumerate(data))\n cumulative[i + 1] = cumulative_[i] + transform(val);\n }\n\n> [...]\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 C42D5BDE6B\n\tfor ;\n\tFri, 3 May 2024 01:43:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B76886340B;\n\tFri, 3 May 2024 03:43:28 +0200 (CEST)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47F5262C9F\n\tfor ;\n\tFri, 3 May 2024 03:43:27 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"SCdTKlp7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1714700606; x=1714959806;\n\tbh=ofO+cp9AYkOOyvT1uVQ6IUqaheux0zsQAj9o1MLkzhA=;\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=SCdTKlp7q//OlqFS/UofX27ZW3LwSl6wJmgK93KJq46kPva/WjO+MtXYC5GdQbn/4\n\tbJbheLfmaZEgOVYl7fzFRBOzWKcG6jKkSUF3cRlSPV0Mej8rsdIdYdnGl6WSEMwy1z\n\tARRiVRCdRGiRT+nFF/LTQT+UENbQhn7yN14V1NLQ2zPWVe3QvBJhWJKXWVd8fuhsqq\n\takLEEZdeYtAmBuvdBdx7kWiPIom3QtdEYdv1dR8Mq2UPazrEW07Bom1iA6St8ZkDww\n\tKBTopZLbGQu8KozWpCSHsteuLJRcKQWeRZW/WkCpJIBpGwNKcoWgriLlRKMeFplPRY\n\tdLL2p6Wtw+kMg==","Date":"Fri, 03 May 2024 01:43:22 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2] ipa: libipa: histogram: Add transform parameter to\n\tconstructor","Message-ID":"","In-Reply-To":"<20240503012057.GK4959@pendragon.ideasonboard.com>","References":"<20240419055336.1070164-1-paul.elder@ideasonboard.com>\n\t<171355946360.3257362.6093408626960336117@ping.linuxembedded.co.uk>\n\t\n\t<20240503012057.GK4959@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"2160d15f631aa3adee1b232b71c972733d02d57e","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\" "}}]