[{"id":30194,"web_url":"https://patchwork.libcamera.org/comment/30194/","msgid":"","date":"2024-07-02T07:53:21","subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote:\n> For a proper tuning process we need to know the sensor black levels. In\n> most cases these are fixed and not reported by the kernel driver. Store\n> them inside the sensor helpers for later retrieval by the algorithms.\n>\n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++\n> src/ipa/libipa/camera_sensor_helper.h | 6 ++++++\n> 2 files changed, 24 insertions(+)\n>\n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index 782ff9904e81..6d8850eb25a5 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -47,6 +47,21 @@ namespace ipa {\n> * function.\n> */\n>\n> +/**\n> + * \\brief Fetch the black levels of the sensor\n> + *\n> + * This function returns the black levels of the sensor with respect to a 16bit\n> + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is\n\ns/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/\n> + * returned.\n> + *\n> + * \\return The black levels of the sensor\n, or std::nullopt if not initialized\n\n> + */\n> +std::optional\n> +CameraSensorHelper::blackLevels() const\n> +{\n> +\treturn blackLevels_;\n> +}\n> +\n> /**\n> * \\brief Compute gain code from the analogue gain absolute value\n> * \\param[in] gain The real gain to pass\n> @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperImx219()\n> \t{\n> +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> \t\tgainType_ = AnalogueGainLinear;\n> \t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> \t}\n> @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperImx258()\n> \t{\n> +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> \t\tgainType_ = AnalogueGainLinear;\n> \t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> \t}\n> @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperImx335()\n> \t{\n> +\t\tblackLevels_ = { 3200, 3200, 3200, 3200 };\n\nI trust you on these values, which I presume come from the sensor's\ndatasheets ? If you have measured them instead, I would add them in a\nseparate commit with the commit message reporting how these have been\nmeasured\n\n> \t\tgainType_ = AnalogueGainExponential;\n> \t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> \t}\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> index 0d99073bea82..f025727c06ee 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.h\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -9,7 +9,9 @@\n>\n> #include \n>\n> +#include \n> #include \n> +#include \n> #include \n> #include \n>\n> @@ -22,9 +24,12 @@ namespace ipa {\n> class CameraSensorHelper\n> {\n> public:\n> +\ttypedef std::array BlackLevels;\n\nstupid question: are we sure we will always have 4 values only ? We\ntried not to assume a Bayer pattern in the design of the camera sensor\nhelper classes.\n\n> +\n> \tCameraSensorHelper() = default;\n> \tvirtual ~CameraSensorHelper() = default;\n>\n> +\tvirtual std::optional blackLevels() const;\n\nShould this be virtual ? Is there any case when a derived class will\nhave to override this instead of just initializing BlackLevels to the\ndesired values ?\n\nThanks\n j\n\n> \tvirtual uint32_t gainCode(double gain) const;\n> \tvirtual double gain(uint32_t gainCode) const;\n>\n> @@ -51,6 +56,7 @@ protected:\n> \t\tAnalogueGainExpConstants exp;\n> \t};\n>\n> +\tstd::optional blackLevels_;\n> \tAnalogueGainType gainType_;\n> \tAnalogueGainConstants gainConstants_;\n>\n> --\n> 2.43.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 7C609BD87C\n\tfor ;\n\tTue, 2 Jul 2024 07:53:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9CBBE62E01;\n\tTue, 2 Jul 2024 09:53:26 +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 C6BA2619C8\n\tfor ;\n\tTue, 2 Jul 2024 09:53:24 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5AB885A4;\n\tTue, 2 Jul 2024 09:52:57 +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=\"BkgpltM4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719906777;\n\tbh=V5iu4PcIA6IB4g+mUTe1+8+fjeOy1JpT2pihcB2Zut4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BkgpltM4axc3OqHMbVOv4m5v3n5mGNUSzeAT0M5y4oBOUwbOXrDGdfZPR1+qSDagR\n\tIxx/l76ZcmuLqgIZ8V4eeotY3wjKOQh3L6OruWwczNN7AZ/npSgVMZ9VDYkLhiTKZj\n\t5hiLCa+nIwprFzvYTPgpMfmXdoFuN4P5DxDENmNs=","Date":"Tue, 2 Jul 2024 09:53:21 +0200","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","Message-ID":"","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-2-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240701144122.3418955-2-stefan.klug@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":30213,"web_url":"https://patchwork.libcamera.org/comment/30213/","msgid":"<20240702125102.GC26760@pendragon.ideasonboard.com>","date":"2024-07-02T12:51:02","subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan\n\nOn Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote:\n> On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote:\n> > For a proper tuning process we need to know the sensor black levels. In\n> > most cases these are fixed and not reported by the kernel driver. Store\n> > them inside the sensor helpers for later retrieval by the algorithms.\n\nAdd something like\n\n----\nAdd black level values corresponding to the data pedestal for three\ninitial sensors. More should be added, eventually filling the gaps for\nall supported sensors.\n----\n\n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++\n> > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++\n> > 2 files changed, 24 insertions(+)\n> >\n> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > index 782ff9904e81..6d8850eb25a5 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > @@ -47,6 +47,21 @@ namespace ipa {\n> > * function.\n> > */\n> >\n> > +/**\n> > + * \\brief Fetch the black levels of the sensor\n> > + *\n> > + * This function returns the black levels of the sensor with respect to a 16bit\n> > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is\n> \n> s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/\n> \n> > + * returned.\n> > + *\n> > + * \\return The black levels of the sensor\n>\n> , or std::nullopt if not initialized\n\ns/initialized/known/\n\n:-)\n\n> > + */\n> > +std::optional\n> > +CameraSensorHelper::blackLevels() const\n> > +{\n> > +\treturn blackLevels_;\n> > +}\n> > +\n> > /**\n> > * \\brief Compute gain code from the analogue gain absolute value\n> > * \\param[in] gain The real gain to pass\n> > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperImx219()\n> > \t{\n> > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > \t\tgainType_ = AnalogueGainLinear;\n> > \t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> > \t}\n> > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperImx258()\n> > \t{\n> > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > \t\tgainType_ = AnalogueGainLinear;\n> > \t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> > \t}\n> > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperImx335()\n> > \t{\n> > +\t\tblackLevels_ = { 3200, 3200, 3200, 3200 };\n> \n> I trust you on these values, which I presume come from the sensor's\n> datasheets ? If you have measured them instead, I would add them in a\n> separate commit with the commit message reporting how these have been\n> measured\n> \n> > \t\tgainType_ = AnalogueGainExponential;\n> > \t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > \t}\n> > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> > index 0d99073bea82..f025727c06ee 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.h\n> > +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > @@ -9,7 +9,9 @@\n> >\n> > #include \n> >\n> > +#include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -22,9 +24,12 @@ namespace ipa {\n> > class CameraSensorHelper\n> > {\n> > public:\n> > +\ttypedef std::array BlackLevels;\n\n\tusing BlackLevels = std::array;\n\n> \n> stupid question: are we sure we will always have 4 values only ? We\n> tried not to assume a Bayer pattern in the design of the camera sensor\n> helper classes.\n\nI would also have used 4 levels, but that's an interesting point. I\nwould be fine starting with 4, considering that sensors with less than 4\ncolour channels (the main case today is monochrome sensors) would use\nthe first elements, and figure out later what to do for sensors with\nmore than 4 channels. Or, given that we only deal with the pedestal\ntoday, we could replace this with a single black level value. I'm\nslightly tempted to go for the latter, as it's simpler.\n\n> > +\n> > \tCameraSensorHelper() = default;\n> > \tvirtual ~CameraSensorHelper() = default;\n> >\n> > +\tvirtual std::optional blackLevels() const;\n> \n> Should this be virtual ? Is there any case when a derived class will\n> have to override this instead of just initializing BlackLevels to the\n> desired values ?\n\nPossibly, for instance the dark currents could be estimated based on the\nsensor temperature. There's a high chance we'll never get there though,\nso I'd start with a non-virtual function, and I would even make it\ninline.\n\n> > \tvirtual uint32_t gainCode(double gain) const;\n> > \tvirtual double gain(uint32_t gainCode) const;\n> >\n> > @@ -51,6 +56,7 @@ protected:\n> > \t\tAnalogueGainExpConstants exp;\n> > \t};\n> >\n> > +\tstd::optional blackLevels_;\n> > \tAnalogueGainType gainType_;\n> > \tAnalogueGainConstants gainConstants_;\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 1E3ADBD87C\n\tfor ;\n\tTue, 2 Jul 2024 12:51:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB7DA62C96;\n\tTue, 2 Jul 2024 14:51:25 +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 96E95619CC\n\tfor ;\n\tTue, 2 Jul 2024 14:51:23 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 084D85A4;\n\tTue, 2 Jul 2024 14:50:55 +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=\"PC8W+5VW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719924656;\n\tbh=RC3OOU0M6gSjX285TBnv5pHWnsbKU9nhpuhk/V8LNgo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PC8W+5VWtSwJbn1g/967bzoUSD7qFUpumc6uVjkDZdI8ag7gSDbz/ILms+GZJObaB\n\tzIM22fWyWGi2nBjef/TO6YgJMQ5CycdpxcjYLBFpwyl7rc5rlVN/w6SIhSpIN8TObl\n\tJtVKHWM9s7NrW9Z4RU1WpdJdyaG69AUq9v7y6PHQ=","Date":"Tue, 2 Jul 2024 15:51:02 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","Message-ID":"<20240702125102.GC26760@pendragon.ideasonboard.com>","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-2-stefan.klug@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","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":30216,"web_url":"https://patchwork.libcamera.org/comment/30216/","msgid":"","date":"2024-07-02T12:59:32","subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Tue, Jul 02, 2024 at 03:51:02PM GMT, Laurent Pinchart wrote:\n> Hi Stefan\n>\n> On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote:\n> > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote:\n> > > For a proper tuning process we need to know the sensor black levels. In\n> > > most cases these are fixed and not reported by the kernel driver. Store\n> > > them inside the sensor helpers for later retrieval by the algorithms.\n>\n> Add something like\n>\n> ----\n> Add black level values corresponding to the data pedestal for three\n> initial sensors. More should be added, eventually filling the gaps for\n> all supported sensors.\n> ----\n>\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++\n> > > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++\n> > > 2 files changed, 24 insertions(+)\n> > >\n> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > index 782ff9904e81..6d8850eb25a5 100644\n> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > @@ -47,6 +47,21 @@ namespace ipa {\n> > > * function.\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Fetch the black levels of the sensor\n> > > + *\n> > > + * This function returns the black levels of the sensor with respect to a 16bit\n> > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is\n> >\n> > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/\n> >\n> > > + * returned.\n> > > + *\n> > > + * \\return The black levels of the sensor\n> >\n> > , or std::nullopt if not initialized\n>\n> s/initialized/known/\n>\n> :-)\n>\n> > > + */\n> > > +std::optional\n> > > +CameraSensorHelper::blackLevels() const\n> > > +{\n> > > +\treturn blackLevels_;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Compute gain code from the analogue gain absolute value\n> > > * \\param[in] gain The real gain to pass\n> > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> > > public:\n> > > \tCameraSensorHelperImx219()\n> > > \t{\n> > > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > > \t\tgainType_ = AnalogueGainLinear;\n> > > \t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> > > \t}\n> > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper\n> > > public:\n> > > \tCameraSensorHelperImx258()\n> > > \t{\n> > > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > > \t\tgainType_ = AnalogueGainLinear;\n> > > \t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> > > \t}\n> > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper\n> > > public:\n> > > \tCameraSensorHelperImx335()\n> > > \t{\n> > > +\t\tblackLevels_ = { 3200, 3200, 3200, 3200 };\n> >\n> > I trust you on these values, which I presume come from the sensor's\n> > datasheets ? If you have measured them instead, I would add them in a\n> > separate commit with the commit message reporting how these have been\n> > measured\n> >\n> > > \t\tgainType_ = AnalogueGainExponential;\n> > > \t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > > \t}\n> > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> > > index 0d99073bea82..f025727c06ee 100644\n> > > --- a/src/ipa/libipa/camera_sensor_helper.h\n> > > +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > > @@ -9,7 +9,9 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -22,9 +24,12 @@ namespace ipa {\n> > > class CameraSensorHelper\n> > > {\n> > > public:\n> > > +\ttypedef std::array BlackLevels;\n>\n> \tusing BlackLevels = std::array;\n>\n> >\n> > stupid question: are we sure we will always have 4 values only ? We\n> > tried not to assume a Bayer pattern in the design of the camera sensor\n> > helper classes.\n>\n> I would also have used 4 levels, but that's an interesting point. I\n> would be fine starting with 4, considering that sensors with less than 4\n> colour channels (the main case today is monochrome sensors) would use\n> the first elements, and figure out later what to do for sensors with\n> more than 4 channels. Or, given that we only deal with the pedestal\n> today, we could replace this with a single black level value. I'm\n> slightly tempted to go for the latter, as it's simpler.\n>\n\nActually, from a sensor perspective, do we actually have one pedestal\nvalue for each color channel, or is the ISP that requires to express one\nvalue per channel ?\n\n> > > +\n> > > \tCameraSensorHelper() = default;\n> > > \tvirtual ~CameraSensorHelper() = default;\n> > >\n> > > +\tvirtual std::optional blackLevels() const;\n> >\n> > Should this be virtual ? Is there any case when a derived class will\n> > have to override this instead of just initializing BlackLevels to the\n> > desired values ?\n>\n> Possibly, for instance the dark currents could be estimated based on the\n> sensor temperature. There's a high chance we'll never get there though,\n> so I'd start with a non-virtual function, and I would even make it\n> inline.\n>\n> > > \tvirtual uint32_t gainCode(double gain) const;\n> > > \tvirtual double gain(uint32_t gainCode) const;\n> > >\n> > > @@ -51,6 +56,7 @@ protected:\n> > > \t\tAnalogueGainExpConstants exp;\n> > > \t};\n> > >\n> > > +\tstd::optional blackLevels_;\n> > > \tAnalogueGainType gainType_;\n> > > \tAnalogueGainConstants gainConstants_;\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 8A196BD87C\n\tfor ;\n\tTue, 2 Jul 2024 12:59:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A89262E27;\n\tTue, 2 Jul 2024 14:59:36 +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 B678D62C96\n\tfor ;\n\tTue, 2 Jul 2024 14:59:34 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4DFFA5A4;\n\tTue, 2 Jul 2024 14:59: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=\"WkigN44k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719925147;\n\tbh=nCckcWqlmaSwk4vwtk8tPSgQY2TYI5oaSbLa8wc5LBM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WkigN44k6Vm2+0jp4ZzQUOYSjpk7a4vtoLDr1f2anjGWjhPK7Wr3HOUM/vY8aTF6w\n\tep6MgV5ZZuknZN1c+xNpLSwGME9LMv/5q954CZ1GcncTvGGT9huARLa0FI1BLOdZKd\n\tU3bTvA6xY4Kw1Ow7oNF06+4/vl1T6LudRyFtXXP4=","Date":"Tue, 2 Jul 2024 14:59:32 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Stefan Klug , \n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","Message-ID":"","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-2-stefan.klug@ideasonboard.com>\n\t\n\t<20240702125102.GC26760@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240702125102.GC26760@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":30218,"web_url":"https://patchwork.libcamera.org/comment/30218/","msgid":"<20240702131709.GP30900@pendragon.ideasonboard.com>","date":"2024-07-02T13:17:09","subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Jul 02, 2024 at 02:59:32PM +0200, Jacopo Mondi wrote:\n> On Tue, Jul 02, 2024 at 03:51:02PM GMT, Laurent Pinchart wrote:\n> > Hi Stefan\n> >\n> > On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote:\n> > > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote:\n> > > > For a proper tuning process we need to know the sensor black levels. In\n> > > > most cases these are fixed and not reported by the kernel driver. Store\n> > > > them inside the sensor helpers for later retrieval by the algorithms.\n> >\n> > Add something like\n> >\n> > ----\n> > Add black level values corresponding to the data pedestal for three\n> > initial sensors. More should be added, eventually filling the gaps for\n> > all supported sensors.\n> > ----\n> >\n> > > >\n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++\n> > > > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++\n> > > > 2 files changed, 24 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > index 782ff9904e81..6d8850eb25a5 100644\n> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > @@ -47,6 +47,21 @@ namespace ipa {\n> > > > * function.\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\brief Fetch the black levels of the sensor\n> > > > + *\n> > > > + * This function returns the black levels of the sensor with respect to a 16bit\n> > > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is\n> > >\n> > > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/\n> > >\n> > > > + * returned.\n> > > > + *\n> > > > + * \\return The black levels of the sensor\n> > >\n> > > , or std::nullopt if not initialized\n> >\n> > s/initialized/known/\n> >\n> > :-)\n> >\n> > > > + */\n> > > > +std::optional\n> > > > +CameraSensorHelper::blackLevels() const\n> > > > +{\n> > > > +\treturn blackLevels_;\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Compute gain code from the analogue gain absolute value\n> > > > * \\param[in] gain The real gain to pass\n> > > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> > > > public:\n> > > > \tCameraSensorHelperImx219()\n> > > > \t{\n> > > > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > > > \t\tgainType_ = AnalogueGainLinear;\n> > > > \t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> > > > \t}\n> > > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper\n> > > > public:\n> > > > \tCameraSensorHelperImx258()\n> > > > \t{\n> > > > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > > > \t\tgainType_ = AnalogueGainLinear;\n> > > > \t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> > > > \t}\n> > > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper\n> > > > public:\n> > > > \tCameraSensorHelperImx335()\n> > > > \t{\n> > > > +\t\tblackLevels_ = { 3200, 3200, 3200, 3200 };\n> > >\n> > > I trust you on these values, which I presume come from the sensor's\n> > > datasheets ? If you have measured them instead, I would add them in a\n> > > separate commit with the commit message reporting how these have been\n> > > measured\n> > >\n> > > > \t\tgainType_ = AnalogueGainExponential;\n> > > > \t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > > > \t}\n> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> > > > index 0d99073bea82..f025727c06ee 100644\n> > > > --- a/src/ipa/libipa/camera_sensor_helper.h\n> > > > +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > > > @@ -9,7 +9,9 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -22,9 +24,12 @@ namespace ipa {\n> > > > class CameraSensorHelper\n> > > > {\n> > > > public:\n> > > > +\ttypedef std::array BlackLevels;\n> >\n> > \tusing BlackLevels = std::array;\n> >\n> > >\n> > > stupid question: are we sure we will always have 4 values only ? We\n> > > tried not to assume a Bayer pattern in the design of the camera sensor\n> > > helper classes.\n> >\n> > I would also have used 4 levels, but that's an interesting point. I\n> > would be fine starting with 4, considering that sensors with less than 4\n> > colour channels (the main case today is monochrome sensors) would use\n> > the first elements, and figure out later what to do for sensors with\n> > more than 4 channels. Or, given that we only deal with the pedestal\n> > today, we could replace this with a single black level value. I'm\n> > slightly tempted to go for the latter, as it's simpler.\n> \n> Actually, from a sensor perspective, do we actually have one pedestal\n> value for each color channel, or is the ISP that requires to express one\n> value per channel ?\n\nI suppose it's sensor-dependent, but in practice I don't really see a\nreason why one would need per-channel values. There are certainly\nsensors that program the data pedestal using a single register.\n\n> > > > +\n> > > > \tCameraSensorHelper() = default;\n> > > > \tvirtual ~CameraSensorHelper() = default;\n> > > >\n> > > > +\tvirtual std::optional blackLevels() const;\n> > >\n> > > Should this be virtual ? Is there any case when a derived class will\n> > > have to override this instead of just initializing BlackLevels to the\n> > > desired values ?\n> >\n> > Possibly, for instance the dark currents could be estimated based on the\n> > sensor temperature. There's a high chance we'll never get there though,\n> > so I'd start with a non-virtual function, and I would even make it\n> > inline.\n> >\n> > > > \tvirtual uint32_t gainCode(double gain) const;\n> > > > \tvirtual double gain(uint32_t gainCode) const;\n> > > >\n> > > > @@ -51,6 +56,7 @@ protected:\n> > > > \t\tAnalogueGainExpConstants exp;\n> > > > \t};\n> > > >\n> > > > +\tstd::optional blackLevels_;\n> > > > \tAnalogueGainType gainType_;\n> > > > \tAnalogueGainConstants gainConstants_;\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 D1049BD87C\n\tfor ;\n\tTue, 2 Jul 2024 13:17:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64E3462E24;\n\tTue, 2 Jul 2024 15:17:32 +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 D2013619CC\n\tfor ;\n\tTue, 2 Jul 2024 15:17:30 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 29443ABE;\n\tTue, 2 Jul 2024 15:17:03 +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=\"VI2SH7/2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719926223;\n\tbh=7Vpnanzvr3xftwwxa3KkfxsQtU4/wsWPT9AdmKF2JcU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VI2SH7/270Qpw1QqPmA+8/1o41O+pqCAmeEVvRt5xqw0tsTVTEUwOvQlLEj4Fkx6O\n\toKcGt8RNPOjRYVF5afosDJKwGjU+69B7NzhfVJuJZSjY+1AAWpL3+3XgK1byCjLt3K\n\tvEki4Fs0gABtU4jSqd9HdjpxqgHfandD/lZPM+n0=","Date":"Tue, 2 Jul 2024 16:17:09 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","Message-ID":"<20240702131709.GP30900@pendragon.ideasonboard.com>","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-2-stefan.klug@ideasonboard.com>\n\t\n\t<20240702125102.GC26760@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","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":30227,"web_url":"https://patchwork.libcamera.org/comment/30227/","msgid":"","date":"2024-07-02T14:25:23","subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote:\n> > For a proper tuning process we need to know the sensor black levels. In\n> > most cases these are fixed and not reported by the kernel driver. Store\n> > them inside the sensor helpers for later retrieval by the algorithms.\n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++\n> > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++\n> > 2 files changed, 24 insertions(+)\n> >\n> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > index 782ff9904e81..6d8850eb25a5 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > @@ -47,6 +47,21 @@ namespace ipa {\n> > * function.\n> > */\n> >\n> > +/**\n> > + * \\brief Fetch the black levels of the sensor\n> > + *\n> > + * This function returns the black levels of the sensor with respect to a 16bit\n> > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is\n> \n> s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/\n\nDoes that really improve the sentence? Because the values used are\nactually 32 bit, to be the same as the metadata type.\n\n> > + * returned.\n> > + *\n> > + * \\return The black levels of the sensor\n> , or std::nullopt if not initialized\n> \n> > + */\n> > +std::optional\n> > +CameraSensorHelper::blackLevels() const\n> > +{\n> > +\treturn blackLevels_;\n> > +}\n> > +\n> > /**\n> > * \\brief Compute gain code from the analogue gain absolute value\n> > * \\param[in] gain The real gain to pass\n> > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperImx219()\n> > \t{\n> > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > \t\tgainType_ = AnalogueGainLinear;\n> > \t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> > \t}\n> > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperImx258()\n> > \t{\n> > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > \t\tgainType_ = AnalogueGainLinear;\n> > \t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> > \t}\n> > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperImx335()\n> > \t{\n> > +\t\tblackLevels_ = { 3200, 3200, 3200, 3200 };\n> \n> I trust you on these values, which I presume come from the sensor's\n> datasheets ? If you have measured them instead, I would add them in a\n> separate commit with the commit message reporting how these have been\n> measured\n\nYes, these are all from datsheets. I'll add a comment.\n\nAll the other commets were already answered by Lauerent and will be\nfixed where needed.\n\nCheers,\nStefan\n\n> \n> > \t\tgainType_ = AnalogueGainExponential;\n> > \t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > \t}\n> > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> > index 0d99073bea82..f025727c06ee 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.h\n> > +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > @@ -9,7 +9,9 @@\n> >\n> > #include \n> >\n> > +#include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -22,9 +24,12 @@ namespace ipa {\n> > class CameraSensorHelper\n> > {\n> > public:\n> > +\ttypedef std::array BlackLevels;\n> \n> stupid question: are we sure we will always have 4 values only ? We\n> tried not to assume a Bayer pattern in the design of the camera sensor\n> helper classes.\n> \n> > +\n> > \tCameraSensorHelper() = default;\n> > \tvirtual ~CameraSensorHelper() = default;\n> >\n> > +\tvirtual std::optional blackLevels() const;\n> \n> Should this be virtual ? Is there any case when a derived class will\n> have to override this instead of just initializing BlackLevels to the\n> desired values ?\n> \n> Thanks\n> j\n> \n> > \tvirtual uint32_t gainCode(double gain) const;\n> > \tvirtual double gain(uint32_t gainCode) const;\n> >\n> > @@ -51,6 +56,7 @@ protected:\n> > \t\tAnalogueGainExpConstants exp;\n> > \t};\n> >\n> > +\tstd::optional blackLevels_;\n> > \tAnalogueGainType gainType_;\n> > \tAnalogueGainConstants gainConstants_;\n> >\n> > --\n> > 2.43.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 37DD0BD87C\n\tfor ;\n\tTue, 2 Jul 2024 14:25:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2941D62E22;\n\tTue, 2 Jul 2024 16:25:27 +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 E53DC619CC\n\tfor ;\n\tTue, 2 Jul 2024 16:25:25 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:15de:d83a:d962:e44])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5A1C7836;\n\tTue, 2 Jul 2024 16:24:58 +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=\"v+/ANLlm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719930298;\n\tbh=wM7u1ayCfMTwUbTXPu9kXMjgf9bvFZHqXO/j9SPwf1U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v+/ANLlmLp1M+WNMxJMP/Al/U0mZGFfH5S0LhODsoHGE+huE1ZBg/eav/IziSTPLy\n\t9w5FONs3K7Jmac0CPrxdva/ZLJEBeeu4BTnaNeOhuQZ7prSZtohS4/2Z3TS4drlYaa\n\tlUOjLRjXfxyU9FewAvTMHgJPmT0OsL6i7iB/jU9c=","Date":"Tue, 2 Jul 2024 16:25:23 +0200","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","Message-ID":"","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-2-stefan.klug@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","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":30229,"web_url":"https://patchwork.libcamera.org/comment/30229/","msgid":"","date":"2024-07-02T14:42:38","subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Tue, Jul 02, 2024 at 04:25:23PM GMT, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote:\n> > > For a proper tuning process we need to know the sensor black levels. In\n> > > most cases these are fixed and not reported by the kernel driver. Store\n> > > them inside the sensor helpers for later retrieval by the algorithms.\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++\n> > > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++\n> > > 2 files changed, 24 insertions(+)\n> > >\n> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > index 782ff9904e81..6d8850eb25a5 100644\n> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > @@ -47,6 +47,21 @@ namespace ipa {\n> > > * function.\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Fetch the black levels of the sensor\n> > > + *\n> > > + * This function returns the black levels of the sensor with respect to a 16bit\n> > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is\n> >\n> > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/\n>\n> Does that really improve the sentence? Because the values used are\n> actually 32 bit, to be the same as the metadata type.\n\n\"with respect to\" doesn't sound right to me in English, but, not a\nnative speaker, so please wait for someone else opinion\n\n>\n> > > + * returned.\n> > > + *\n> > > + * \\return The black levels of the sensor\n> > , or std::nullopt if not initialized\n> >\n> > > + */\n> > > +std::optional\n> > > +CameraSensorHelper::blackLevels() const\n> > > +{\n> > > +\treturn blackLevels_;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Compute gain code from the analogue gain absolute value\n> > > * \\param[in] gain The real gain to pass\n> > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> > > public:\n> > > \tCameraSensorHelperImx219()\n> > > \t{\n> > > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > > \t\tgainType_ = AnalogueGainLinear;\n> > > \t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> > > \t}\n> > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper\n> > > public:\n> > > \tCameraSensorHelperImx258()\n> > > \t{\n> > > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > > \t\tgainType_ = AnalogueGainLinear;\n> > > \t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> > > \t}\n> > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper\n> > > public:\n> > > \tCameraSensorHelperImx335()\n> > > \t{\n> > > +\t\tblackLevels_ = { 3200, 3200, 3200, 3200 };\n> >\n> > I trust you on these values, which I presume come from the sensor's\n> > datasheets ? If you have measured them instead, I would add them in a\n> > separate commit with the commit message reporting how these have been\n> > measured\n>\n> Yes, these are all from datsheets. I'll add a comment.\n>\n> All the other commets were already answered by Lauerent and will be\n> fixed where needed.\n>\n> Cheers,\n> Stefan\n>\n> >\n> > > \t\tgainType_ = AnalogueGainExponential;\n> > > \t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > > \t}\n> > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> > > index 0d99073bea82..f025727c06ee 100644\n> > > --- a/src/ipa/libipa/camera_sensor_helper.h\n> > > +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > > @@ -9,7 +9,9 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -22,9 +24,12 @@ namespace ipa {\n> > > class CameraSensorHelper\n> > > {\n> > > public:\n> > > +\ttypedef std::array BlackLevels;\n> >\n> > stupid question: are we sure we will always have 4 values only ? We\n> > tried not to assume a Bayer pattern in the design of the camera sensor\n> > helper classes.\n> >\n> > > +\n> > > \tCameraSensorHelper() = default;\n> > > \tvirtual ~CameraSensorHelper() = default;\n> > >\n> > > +\tvirtual std::optional blackLevels() const;\n> >\n> > Should this be virtual ? Is there any case when a derived class will\n> > have to override this instead of just initializing BlackLevels to the\n> > desired values ?\n> >\n> > Thanks\n> > j\n> >\n> > > \tvirtual uint32_t gainCode(double gain) const;\n> > > \tvirtual double gain(uint32_t gainCode) const;\n> > >\n> > > @@ -51,6 +56,7 @@ protected:\n> > > \t\tAnalogueGainExpConstants exp;\n> > > \t};\n> > >\n> > > +\tstd::optional blackLevels_;\n> > > \tAnalogueGainType gainType_;\n> > > \tAnalogueGainConstants gainConstants_;\n> > >\n> > > --\n> > > 2.43.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 44D4DBD87C\n\tfor ;\n\tTue, 2 Jul 2024 14:42:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F8D162E22;\n\tTue, 2 Jul 2024 16:42:43 +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 D6CC5619CC\n\tfor ;\n\tTue, 2 Jul 2024 16:42:41 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 32CFB836;\n\tTue, 2 Jul 2024 16:42:14 +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=\"TWihmyl0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719931334;\n\tbh=pTyjDSrlAEzgPUw8vqy1urWw8xf+J3lI3pF4y/deu50=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TWihmyl0Ha+OOBVAF7JTL8vO7F6Dxyq7wT8LvZruyUH4bk+EzYppXJxWSWkn8mNhO\n\tEGkgsZLJqL6ubOAjfBTTcY+gik3IecRb38e9p1Z/Y1fijhxKHXUK9ilBCumXTfrk2R\n\tYZeO5nVhRog/8EvXUdKuwsKL+guiXDTU++O28Voo=","Date":"Tue, 2 Jul 2024 16:42:38 +0200","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","Message-ID":"","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-2-stefan.klug@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","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":30247,"web_url":"https://patchwork.libcamera.org/comment/30247/","msgid":"","date":"2024-07-03T10:23:59","subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Tue, Jul 02, 2024 at 04:42:38PM +0200, Jacopo Mondi wrote:\n> On Tue, Jul 02, 2024 at 04:25:23PM GMT, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote:\n> > > Hi Stefan\n> > >\n> > > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote:\n> > > > For a proper tuning process we need to know the sensor black levels. In\n> > > > most cases these are fixed and not reported by the kernel driver. Store\n> > > > them inside the sensor helpers for later retrieval by the algorithms.\n> > > >\n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++\n> > > > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++\n> > > > 2 files changed, 24 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > index 782ff9904e81..6d8850eb25a5 100644\n> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > @@ -47,6 +47,21 @@ namespace ipa {\n> > > > * function.\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\brief Fetch the black levels of the sensor\n> > > > + *\n> > > > + * This function returns the black levels of the sensor with respect to a 16bit\n> > > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is\n> > >\n> > > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/\n> >\n> > Does that really improve the sentence? Because the values used are\n> > actually 32 bit, to be the same as the metadata type.\n> \n> \"with respect to\" doesn't sound right to me in English, but, not a\n> native speaker, so please wait for someone else opinion\n\nYeah it's not great in this context.\n\n\nPaul\n\n> \n> >\n> > > > + * returned.\n> > > > + *\n> > > > + * \\return The black levels of the sensor\n> > > , or std::nullopt if not initialized\n> > >\n> > > > + */\n> > > > +std::optional\n> > > > +CameraSensorHelper::blackLevels() const\n> > > > +{\n> > > > +\treturn blackLevels_;\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Compute gain code from the analogue gain absolute value\n> > > > * \\param[in] gain The real gain to pass\n> > > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> > > > public:\n> > > > \tCameraSensorHelperImx219()\n> > > > \t{\n> > > > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > > > \t\tgainType_ = AnalogueGainLinear;\n> > > > \t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> > > > \t}\n> > > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper\n> > > > public:\n> > > > \tCameraSensorHelperImx258()\n> > > > \t{\n> > > > +\t\tblackLevels_ = { 4096, 4096, 4096, 4096 };\n> > > > \t\tgainType_ = AnalogueGainLinear;\n> > > > \t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> > > > \t}\n> > > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper\n> > > > public:\n> > > > \tCameraSensorHelperImx335()\n> > > > \t{\n> > > > +\t\tblackLevels_ = { 3200, 3200, 3200, 3200 };\n> > >\n> > > I trust you on these values, which I presume come from the sensor's\n> > > datasheets ? If you have measured them instead, I would add them in a\n> > > separate commit with the commit message reporting how these have been\n> > > measured\n> >\n> > Yes, these are all from datsheets. I'll add a comment.\n> >\n> > All the other commets were already answered by Lauerent and will be\n> > fixed where needed.\n> >\n> > Cheers,\n> > Stefan\n> >\n> > >\n> > > > \t\tgainType_ = AnalogueGainExponential;\n> > > > \t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > > > \t}\n> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> > > > index 0d99073bea82..f025727c06ee 100644\n> > > > --- a/src/ipa/libipa/camera_sensor_helper.h\n> > > > +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > > > @@ -9,7 +9,9 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -22,9 +24,12 @@ namespace ipa {\n> > > > class CameraSensorHelper\n> > > > {\n> > > > public:\n> > > > +\ttypedef std::array BlackLevels;\n> > >\n> > > stupid question: are we sure we will always have 4 values only ? We\n> > > tried not to assume a Bayer pattern in the design of the camera sensor\n> > > helper classes.\n> > >\n> > > > +\n> > > > \tCameraSensorHelper() = default;\n> > > > \tvirtual ~CameraSensorHelper() = default;\n> > > >\n> > > > +\tvirtual std::optional blackLevels() const;\n> > >\n> > > Should this be virtual ? Is there any case when a derived class will\n> > > have to override this instead of just initializing BlackLevels to the\n> > > desired values ?\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > \tvirtual uint32_t gainCode(double gain) const;\n> > > > \tvirtual double gain(uint32_t gainCode) const;\n> > > >\n> > > > @@ -51,6 +56,7 @@ protected:\n> > > > \t\tAnalogueGainExpConstants exp;\n> > > > \t};\n> > > >\n> > > > +\tstd::optional blackLevels_;\n> > > > \tAnalogueGainType gainType_;\n> > > > \tAnalogueGainConstants gainConstants_;\n> > > >\n> > > > --\n> > > > 2.43.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 02457BEFBE\n\tfor ;\n\tWed, 3 Jul 2024 10:24:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A08562E22;\n\tWed, 3 Jul 2024 12:24: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 14A8B62C95\n\tfor ;\n\tWed, 3 Jul 2024 12:24:06 +0200 (CEST)","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 A17776D6;\n\tWed, 3 Jul 2024 12:23:36 +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=\"F3/iqfwx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720002217;\n\tbh=Js20Xf12nx0olkGiTduG7eBmU+cuEZnjrD5Af33Qosk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F3/iqfwxWTvzzNlp9OT4hQGODsGC29p14EBcNlfrRrT91UQe8lCBQyJaIXAeaLnXG\n\tCAYtzhyRYOWkTr2u5pflrs8n6CcDBB07pthG30yi1vFLc9k88BQstbXAy3TdF1g24A\n\tm8ir5ajA9mkPMitjeAWMBz3SWDvZjp9hgVVI0hoo=","Date":"Wed, 3 Jul 2024 19:23:59 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/5] ipa: libipa: Add black levels to camera sensor\n\thelper","Message-ID":"","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-2-stefan.klug@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","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\" "}}]