[{"id":30196,"web_url":"https://patchwork.libcamera.org/comment/30196/","msgid":"","date":"2024-07-02T08:00:44","subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","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:26PM GMT, Stefan Klug wrote:\n> The black levels from the camera sensor helper are then used to do the\n> black level correction. Black levels can still be overwritten by the\n> tuning file.\n>\n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----\n> 1 file changed, 24 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> index d2e743541c99..0c39c3b47da5 100644\n> --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()\n> int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> \t\t\t const YamlObject &tuningData)\n> {\n> -\tblackLevelRed_ = tuningData[\"R\"].get(256);\n> -\tblackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> -\tblackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> -\tblackLevelBlue_ = tuningData[\"B\"].get(256);\n> +\tauto blackLevels = context.camHelper->blackLevels();\n> +\tif (blackLevels) {\n> +\t\tSpan levels = *blackLevels;\n> +\t\tblackLevelRed_ = levels[0];\n> +\t\tblackLevelGreenR_ = levels[1];\n> +\t\tblackLevelGreenB_ = levels[2];\n> +\t\tblackLevelBlue_ = levels[3];\n> +\t} else\n> +\t\tLOG(RkISP1Blc, Warning)\n> +\t\t\t<< \"No black levels provided by camera sensor helper\";\n> +\n> +\tif (!blackLevels || (tuningData.contains(\"R\") &&\n\nIs there any need to get context.camHelper->blackLevels() if the\nvalues are in the config file ?\n\n\n> +\t\t\t tuningData.contains(\"Gr\") &&\n> +\t\t\t tuningData.contains(\"Gb\") &&\n> +\t\t\t tuningData.contains(\"B\"))) {\n> +\t\tblackLevelRed_ = tuningData[\"R\"].get(256);\n> +\t\tblackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> +\t\tblackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> +\t\tblackLevelBlue_ = tuningData[\"B\"].get(256);\n> +\n> +\t\tif (blackLevels)\n\nIs this a Warning ? Same question above. I would Warn only if no BLC in\nsensor helpers AND in config file.\n\n> +\t\t\tLOG(RkISP1Blc, Warning)\n> +\t\t\t\t<< \"Black levels overwritten by tuning file\";\n> +\t}\n>\n> \ttuningParameters_ = true;\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 57E31BEFBE\n\tfor ;\n\tTue, 2 Jul 2024 08:00:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E65B762E24;\n\tTue, 2 Jul 2024 10:00:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49EA5619C8\n\tfor ;\n\tTue, 2 Jul 2024 10:00:47 +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 BFA0B836;\n\tTue, 2 Jul 2024 10:00:19 +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=\"jIzBnMEP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719907219;\n\tbh=MgiEYvlZWOizMmL+34r00qmi6xAmjOvIQEPUedPfe7s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jIzBnMEPGiqwZHAxFQsH8oF143W8/1qMRjPDPe+z7ti2oxFv6dxV8Ycce2fQHkP9f\n\t66xHuQ5qpWjYIwouDyKaC9+ly8qLYVISZtmYbphXTMPsgkGONOI7pTvaRhGem6c01L\n\tKdZilPfleI47DM7f0+XbMFbASIwUL4HGYzhgxsN0=","Date":"Tue, 2 Jul 2024 10:00:44 +0200","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","Message-ID":"","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-4-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240701144122.3418955-4-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":30205,"web_url":"https://patchwork.libcamera.org/comment/30205/","msgid":"<171991746343.3353312.18367841913910179492@ping.linuxembedded.co.uk>","date":"2024-07-02T10:51:03","subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2024-07-02 09:00:44)\n> Hi Stefan\n> \n> On Mon, Jul 01, 2024 at 04:38:26PM GMT, Stefan Klug wrote:\n> > The black levels from the camera sensor helper are then used to do the\n> > black level correction. Black levels can still be overwritten by the\n> > tuning file.\n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----\n> > 1 file changed, 24 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > index d2e743541c99..0c39c3b47da5 100644\n> > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > const YamlObject &tuningData)\n> > {\n> > - blackLevelRed_ = tuningData[\"R\"].get(256);\n> > - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > + auto blackLevels = context.camHelper->blackLevels();\n> > + if (blackLevels) {\n> > + Span levels = *blackLevels;\n> > + blackLevelRed_ = levels[0];\n> > + blackLevelGreenR_ = levels[1];\n> > + blackLevelGreenB_ = levels[2];\n> > + blackLevelBlue_ = levels[3];\n> > + } else\n> > + LOG(RkISP1Blc, Warning)\n> > + << \"No black levels provided by camera sensor helper\";\n> > +\n> > + if (!blackLevels || (tuningData.contains(\"R\") &&\n> \n> Is there any need to get context.camHelper->blackLevels() if the\n> values are in the config file ?\n> \n> \n> > + tuningData.contains(\"Gr\") &&\n> > + tuningData.contains(\"Gb\") &&\n> > + tuningData.contains(\"B\"))) {\n> > + blackLevelRed_ = tuningData[\"R\"].get(256);\n> > + blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > + blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > + blackLevelBlue_ = tuningData[\"B\"].get(256);\n\nWhy 256 here? Does a value have to be specified?\n\nMaybe we should move a parse black level helper out to some common yaml\nhelpers (Do we have anywhere for that yet, that can read defined\nlibcamera types from yaml?) ... and it should only return all values if\nall of them are present and parsed correctly - otherwise R=3200, Gr=256,\nGb=256, B=256 probably wouldn't make sense?\n\n\n\n> > +\n> > + if (blackLevels)\n> \n> Is this a Warning ? Same question above. I would Warn only if no BLC in\n> sensor helpers AND in config file.\n> \n> > + LOG(RkISP1Blc, Warning)\n> > + << \"Black levels overwritten by tuning file\";\n\nYes, I think the tuning file should take precedence, so I don't think\nit's a warning. Perhaps a debug print to say which values get loaded and\nwhere from ?\n\n\n> > + }\n> >\n> > tuningParameters_ = true;\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 80DBBBEFBE\n\tfor ;\n\tTue, 2 Jul 2024 10:51:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A52B62C96;\n\tTue, 2 Jul 2024 12:51:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D3FD619CC\n\tfor ;\n\tTue, 2 Jul 2024 12:51:06 +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 0AFB65A4;\n\tTue, 2 Jul 2024 12:50: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=\"VtxC9h3B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719917439;\n\tbh=7eohL/KStnPCM+BHTVvFuQn54I76e1eLSmfMYK6/Eng=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VtxC9h3BA00Y4gNPl3oEgb8QBffwt0oVQlvio4AbQ0CXWuNMuP2OkMt9nHyQ5Q3kd\n\tP6VSXNj4IPASGMPu2PKk0bFv4njw3lVfFj9nhjLvt/8YATKRYGIeOw175uydjj3gwf\n\tS5QCJnqpSlF6Nl34bP2qU2gb6olfiZSVmnb0jHpo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-4-stefan.klug@ideasonboard.com>\n\t","Subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Jacopo Mondi ,\n\tStefan Klug ","Date":"Tue, 02 Jul 2024 11:51:03 +0100","Message-ID":"<171991746343.3353312.18367841913910179492@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":30219,"web_url":"https://patchwork.libcamera.org/comment/30219/","msgid":"","date":"2024-07-02T13:18:08","subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","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 10:00:44AM +0200, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Mon, Jul 01, 2024 at 04:38:26PM GMT, Stefan Klug wrote:\n> > The black levels from the camera sensor helper are then used to do the\n> > black level correction. Black levels can still be overwritten by the\n> > tuning file.\n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----\n> > 1 file changed, 24 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > index d2e743541c99..0c39c3b47da5 100644\n> > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > \t\t\t const YamlObject &tuningData)\n> > {\n> > -\tblackLevelRed_ = tuningData[\"R\"].get(256);\n> > -\tblackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > -\tblackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > -\tblackLevelBlue_ = tuningData[\"B\"].get(256);\n> > +\tauto blackLevels = context.camHelper->blackLevels();\n> > +\tif (blackLevels) {\n> > +\t\tSpan levels = *blackLevels;\n> > +\t\tblackLevelRed_ = levels[0];\n> > +\t\tblackLevelGreenR_ = levels[1];\n> > +\t\tblackLevelGreenB_ = levels[2];\n> > +\t\tblackLevelBlue_ = levels[3];\n> > +\t} else\n> > +\t\tLOG(RkISP1Blc, Warning)\n> > +\t\t\t<< \"No black levels provided by camera sensor helper\";\n> > +\n> > +\tif (!blackLevels || (tuningData.contains(\"R\") &&\n> \n> Is there any need to get context.camHelper->blackLevels() if the\n> values are in the config file ?\n\nThe strategy now is to move away from the having the back levels in the\nconfig. After discussion with Laurent, libcamera should at least know\nthe pedestal values. When we add additional measurements they could be\nadded to the tuning file as offsets. So camHelper->blackLevels() is the\ndefault, the tuning file is kept for backwards compatibility and to be\nable to specify the values when libcamera doesn't have that data.\n\n> \n> \n> > +\t\t\t tuningData.contains(\"Gr\") &&\n> > +\t\t\t tuningData.contains(\"Gb\") &&\n> > +\t\t\t tuningData.contains(\"B\"))) {\n> > +\t\tblackLevelRed_ = tuningData[\"R\"].get(256);\n> > +\t\tblackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > +\t\tblackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > +\t\tblackLevelBlue_ = tuningData[\"B\"].get(256);\n> > +\n> > +\t\tif (blackLevels)\n> \n> Is this a Warning ? Same question above. I would Warn only if no BLC in\n> sensor helpers AND in config file.\n\nYes, I would say so. Basically we are depricating the black levels from\nthe tuning file and moving them into sensor helpers. The first one\nshould be issued as it points to an incomplete implementation in\nlibcamera. The second one only shows up, if the fisrt one didn't show\nup.\n\nBest regards,\nStefan\n\n> \n> > +\t\t\tLOG(RkISP1Blc, Warning)\n> > +\t\t\t\t<< \"Black levels overwritten by tuning file\";\n> > +\t}\n> >\n> > \ttuningParameters_ = true;\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 28A1BBD87C\n\tfor ;\n\tTue, 2 Jul 2024 13:18:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B53DA62E25;\n\tTue, 2 Jul 2024 15:18:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6E33C62E01\n\tfor ;\n\tTue, 2 Jul 2024 15:18:11 +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 CFD73664;\n\tTue, 2 Jul 2024 15:17:43 +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=\"E7n6VKk6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719926263;\n\tbh=ApSntZM4GYEEzPO1dke9fADRGmx31VV/qL+5VxDyjLA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E7n6VKk6KCF/uog/itKrakbbe5zTOnKLeIvFIepWFXbBNKZnOivLCNittaFuCGeQf\n\tJAnZvdku0MunJ9Zjyg8m/pWiPAjYzojv5v++2Zfvuis679n1khHOvJzXzbVJVyHlm8\n\tpr8apSz8+bbVCwYcE9l5oHgKe6n+mzOCTkXcaIAA=","Date":"Tue, 2 Jul 2024 15:18:08 +0200","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","Message-ID":"","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-4-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":30222,"web_url":"https://patchwork.libcamera.org/comment/30222/","msgid":"<4lfrzdtbovsv6pkdlcr622bvlx2qkozsmeyuoury5fgu5e4g4g@icq2j64xe4gz>","date":"2024-07-02T13:27:56","subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Tue, Jul 02, 2024 at 11:51:03AM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2024-07-02 09:00:44)\n> > Hi Stefan\n> > \n> > On Mon, Jul 01, 2024 at 04:38:26PM GMT, Stefan Klug wrote:\n> > > The black levels from the camera sensor helper are then used to do the\n> > > black level correction. Black levels can still be overwritten by the\n> > > tuning file.\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----\n> > > 1 file changed, 24 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > index d2e743541c99..0c39c3b47da5 100644\n> > > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > > const YamlObject &tuningData)\n> > > {\n> > > - blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > + auto blackLevels = context.camHelper->blackLevels();\n> > > + if (blackLevels) {\n> > > + Span levels = *blackLevels;\n> > > + blackLevelRed_ = levels[0];\n> > > + blackLevelGreenR_ = levels[1];\n> > > + blackLevelGreenB_ = levels[2];\n> > > + blackLevelBlue_ = levels[3];\n> > > + } else\n> > > + LOG(RkISP1Blc, Warning)\n> > > + << \"No black levels provided by camera sensor helper\";\n> > > +\n> > > + if (!blackLevels || (tuningData.contains(\"R\") &&\n> > \n> > Is there any need to get context.camHelper->blackLevels() if the\n> > values are in the config file ?\n> > \n> > \n> > > + tuningData.contains(\"Gr\") &&\n> > > + tuningData.contains(\"Gb\") &&\n> > > + tuningData.contains(\"B\"))) {\n> > > + blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > + blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > + blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > + blackLevelBlue_ = tuningData[\"B\"].get(256);\n> \n> Why 256 here? Does a value have to be specified?\n\nDigging into the git history... these were taken from the imx219 tuning\nfile (and scaled to 12bits) as sensible defaults. We could either keep\nthem or error out. My strategy was to stay compatible with existing\ntuning files. If we go the \"Either correct data or fail\" route, we\nshould remove the defaults.\n\n> \n> Maybe we should move a parse black level helper out to some common yaml\n> helpers (Do we have anywhere for that yet, that can read defined\n> libcamera types from yaml?) ... and it should only return all values if\n> all of them are present and parsed correctly - otherwise R=3200, Gr=256,\n> Gb=256, B=256 probably wouldn't make sense?\n\nNo that wouldn't make much sense, but is the old behaviour :-). I can\nhave a look there.\n\n> \n> \n> \n> > > +\n> > > + if (blackLevels)\n> > \n> > Is this a Warning ? Same question above. I would Warn only if no BLC in\n> > sensor helpers AND in config file.\n> > \n> > > + LOG(RkISP1Blc, Warning)\n> > > + << \"Black levels overwritten by tuning file\";\n> \n> Yes, I think the tuning file should take precedence, so I don't think\n> it's a warning. Perhaps a debug print to say which values get loaded and\n> where from ?\n\nAs said in my answer to Jacopo, the warning was on purpose, but I'm fine\nwith making it a debug output (or info?)\n\nCheers,\nStefan\n\n> \n> \n> > > + }\n> > >\n> > > tuningParameters_ = true;\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 8DD0DBD87C\n\tfor ;\n\tTue, 2 Jul 2024 13:28:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A7D862E26;\n\tTue, 2 Jul 2024 15:28:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19919619CC\n\tfor ;\n\tTue, 2 Jul 2024 15:27:59 +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 72B5C664;\n\tTue, 2 Jul 2024 15:27:31 +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=\"sdCZWk7P\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719926851;\n\tbh=P17zsyYSNNu8aaFMpKaymXiLaxQH/jnYn9zB5iTRSRg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sdCZWk7PpoaB1SEVnMj9qyhqbOeILuI4Xy5UqkUJB2Yyc2uMlrkG1x9Nhp6q0oupU\n\tQB4u501GN89MiAMp6lwNXN2U4cyuJU3o5DW28PXttEP/12MvoKTUifAFyhaWkulInt\n\t1WVUrLLOhwc3Az9EGvztUHGI9yhh3QDAKbocf454=","Date":"Tue, 2 Jul 2024 15:27:56 +0200","From":"Stefan Klug ","To":"Kieran Bingham ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","Message-ID":"<4lfrzdtbovsv6pkdlcr622bvlx2qkozsmeyuoury5fgu5e4g4g@icq2j64xe4gz>","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-4-stefan.klug@ideasonboard.com>\n\t\n\t<171991746343.3353312.18367841913910179492@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171991746343.3353312.18367841913910179492@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":30230,"web_url":"https://patchwork.libcamera.org/comment/30230/","msgid":"<20240702145025.GD26760@pendragon.ideasonboard.com>","date":"2024-07-02T14:50:25","subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","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 03:27:56PM +0200, Stefan Klug wrote:\n> On Tue, Jul 02, 2024 at 11:51:03AM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2024-07-02 09:00:44)\n> > > On Mon, Jul 01, 2024 at 04:38:26PM GMT, Stefan Klug wrote:\n> > > > The black levels from the camera sensor helper are then used to do the\n> > > > black level correction. Black levels can still be overwritten by the\n> > > > tuning file.\n\nThe commit message should be readable independently from the subject\nline. The first sentence here sounds weird when you do so. I feel\nobliged to point to the obligatory https://cbea.ms/git-commit/ and to\nemphasize the \"why\" of rule 7 :-)\n\n> > > >\n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----\n> > > > 1 file changed, 24 insertions(+), 4 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > index d2e743541c99..0c39c3b47da5 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > > > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > > > const YamlObject &tuningData)\n> > > > {\n> > > > - blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > > + auto blackLevels = context.camHelper->blackLevels();\n> > > > + if (blackLevels) {\n> > > > + Span levels = *blackLevels;\n> > > > + blackLevelRed_ = levels[0];\n> > > > + blackLevelGreenR_ = levels[1];\n> > > > + blackLevelGreenB_ = levels[2];\n> > > > + blackLevelBlue_ = levels[3];\n> > > > + } else\n> > > > + LOG(RkISP1Blc, Warning)\n> > > > + << \"No black levels provided by camera sensor helper\";\n\n\t\t\t<< \"No black levels provided by camera sensor helper, please fix\";\n\nAnother form of \\todo. Maybe we ned a Fixme log level :-)\n\n> > > > +\n> > > > + if (!blackLevels || (tuningData.contains(\"R\") &&\n> > > \n> > > Is there any need to get context.camHelper->blackLevels() if the\n> > > values are in the config file ?\n> > > \n> > > > + tuningData.contains(\"Gr\") &&\n> > > > + tuningData.contains(\"Gb\") &&\n> > > > + tuningData.contains(\"B\"))) {\n> > > > + blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > + blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > + blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > + blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > \n> > Why 256 here? Does a value have to be specified?\n> \n> Digging into the git history... these were taken from the imx219 tuning\n> file (and scaled to 12bits) as sensible defaults. We could either keep\n> them or error out. My strategy was to stay compatible with existing\n> tuning files. If we go the \"Either correct data or fail\" route, we\n> should remove the defaults.\n\nWe use defaults here in two cases:\n\n- The camera sensor helper doesn't provide black levels, so we fall back\n to the tuning file for backward compatibility. We should keep the\n current behaviour in that case.\n\n- The keys are provided in the tuning file but the data is incorrect.\n That's an error. We can error out, or pick a default, I don't mind\n much either way. Given that the first case points to keeping the\n current behaviour, using the same defaults without printing any\n message seems the simplest option.\n\n> > Maybe we should move a parse black level helper out to some common yaml\n> > helpers (Do we have anywhere for that yet, that can read defined\n> > libcamera types from yaml?) ... and it should only return all values if\n> > all of them are present and parsed correctly - otherwise R=3200, Gr=256,\n> > Gb=256, B=256 probably wouldn't make sense?\n> \n> No that wouldn't make much sense, but is the old behaviour :-). I can\n> have a look there.\n> \n> > > > +\n> > > > + if (blackLevels)\n> > > \n> > > Is this a Warning ? Same question above. I would Warn only if no BLC in\n> > > sensor helpers AND in config file.\n> > > \n> > > > + LOG(RkISP1Blc, Warning)\n> > > > + << \"Black levels overwritten by tuning file\";\n> > \n> > Yes, I think the tuning file should take precedence, so I don't think\n> > it's a warning. Perhaps a debug print to say which values get loaded and\n> > where from ?\n> \n> As said in my answer to Jacopo, the warning was on purpose, but I'm fine\n> with making it a debug output (or info?)\n\nMaybe we can make it clearer:\n\n\t\tLOG(RkISP1Blc, Warning)\n\t\t\t<< \"Deprecated: black levels overwritten by tuning file\";\n\nDo we need a Deprecated log level ? :-)\n\nAnd now that I wrote that, I wonder if we need to preserve this\nbehaviour. The goal, when I discussed it with Stefan, was to avoid\nbreaking existing setups using out-of-tree tuning data that contains\nblack levels different from the data pedestal provided by the camera\nsensor helpers. One reason for doing is is to take dark currents into\naccount, and I'm not sure anyone would have done this kind of tuning\ngiven that we didn't provide any tooling to measure the black level.\nAnother reason would be to tweak the values \"just because\". Given how\nwe're evolving the black level compensation implementation (not just in\nthe algorithm but also in the tuning tools), I'm tempted to say we\nshouldn't preserve backward compatibility here.\n\nAlso, would the following express the logic more clearly ?\n\n\tstd::optional levelRed = tuningData.contains(\"R\").get;\n\tstd::optional levelGreenR = tuningData.contains(\"Gr\").get;\n\tstd::optional levelGreenB = tuningData.contains(\"Gb\").get;\n\tstd::optional levelBlue = tuningData.contains(\"B\").get;\n\n\tbool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;\n\n\tauto blackLevels = context.camHelper->blackLevels();\n\n\tif (!blackLevels) {\n\t\t/*\n\t\t * Not all camera sensor helpers have been updated with black\n\t\t * levels. Print a warning and fall back to the levels from the\n\t\t * tuning data to preserve backward compatibility. This should\n\t\t * be removed once allhelpers provide the data.\n\t\t */\n\t\tLOG(RkISP1Blc, Warning)\n\t\t\t<< \"No black levels provided by camera sensor helper, please fix\";\n\n\t\tblackLevelRed_ = levelRed.value_or(256);\n\t\tblackLevelGreenR_ = levelGreenR.value_or(256);\n\t\tblackLevelGreenB_ = levelGreenB.value_or(256);\n\t\tblackLevelBlue_ = levelBlue.value_or(256);\n\t} else if (tuningHasLevels) {\n\t\t/*\n\t\t * If black levels are provided in the tuning file, use them to\n\t\t * avoid breaking existing camera tuning. This is deprecated and\n\t\t * will be removed.\n\t\t */\n\t\tLOG(RkISP1Blc, Warning)\n\t\t\t<< \"Deprecated: black levels overwritten by tuning file\";\n\n\t\tblackLevelRed_ = *levelRed;\n\t\tblackLevelGreenR_ = *levelGreenR;\n\t\tblackLevelGreenB_ = *levelGreenB;\n\t\tblackLevelBlue_ = *levelBlue;\n\t} else {\n\t\tSpan levels = *blackLevels;\n\t\tblackLevelRed_ = levels[0];\n\t\tblackLevelGreenR_ = levels[1];\n\t\tblackLevelGreenB_ = levels[2];\n\t\tblackLevelBlue_ = levels[3];\n\t}\n\nIf we decide to drop the second case then it could be simplified to\n\n\tauto blackLevels = context.camHelper->blackLevels();\n\n\tif (blackLevels) {\n\t\tSpan levels = *blackLevels;\n\t\tblackLevelRed_ = levels[0];\n\t\tblackLevelGreenR_ = levels[1];\n\t\tblackLevelGreenB_ = levels[2];\n\t\tblackLevelBlue_ = levels[3];\n\t} else {\n\t\t/*\n\t\t * Not all camera sensor helpers have been updated with black\n\t\t * levels. Print a warning and fall back to the levels from the\n\t\t * tuning data to preserve backward compatibility. This should\n\t\t * be removed once allhelpers provide the data.\n\t\t */\n\t\tLOG(RkISP1Blc, Warning)\n\t\t\t<< \"No black levels provided by camera sensor helper, please fix\";\n\n\t\tblackLevelRed_ = tuningData[\"R\"].get(256);\n\t\tblackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n\t\tblackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n\t\tblackLevelBlue_ = tuningData[\"B\"].get(256);\n\t}\n\n> > > > + }\n> > > >\n> > > > tuningParameters_ = true;\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 E9453BD87C\n\tfor ;\n\tTue, 2 Jul 2024 14:50:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9BDE62E24;\n\tTue, 2 Jul 2024 16:50:47 +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 86703619CC\n\tfor ;\n\tTue, 2 Jul 2024 16:50:46 +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 F13E2836;\n\tTue, 2 Jul 2024 16:50:18 +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=\"Z7eSTiMk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719931819;\n\tbh=tFFWeaLN9O0cbiwb4LkzvzrHBhUi9SI1c9EgXvTF4kk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z7eSTiMkDvhcennTN5d9XbUO1qQBGIEzR/9GnWTHXWUhPOiKAb0d5WXEMqlccaokt\n\tT7DYUEG6PNBWtPR3YeStGzgOfDlr7vWb2HWB/ygZ+YiW15G9HTCX/8MRi/5QF0ICFP\n\tQfK3jJEbC9aj296KGu84gj5PRj6jm8gjeKSnSzAg=","Date":"Tue, 2 Jul 2024 17:50:25 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"Kieran Bingham ,\n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","Message-ID":"<20240702145025.GD26760@pendragon.ideasonboard.com>","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-4-stefan.klug@ideasonboard.com>\n\t\n\t<171991746343.3353312.18367841913910179492@ping.linuxembedded.co.uk>\n\t<4lfrzdtbovsv6pkdlcr622bvlx2qkozsmeyuoury5fgu5e4g4g@icq2j64xe4gz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<4lfrzdtbovsv6pkdlcr622bvlx2qkozsmeyuoury5fgu5e4g4g@icq2j64xe4gz>","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":30235,"web_url":"https://patchwork.libcamera.org/comment/30235/","msgid":"","date":"2024-07-03T07:30:19","subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Tue, Jul 02, 2024 at 05:50:25PM +0300, Laurent Pinchart wrote:\n> On Tue, Jul 02, 2024 at 03:27:56PM +0200, Stefan Klug wrote:\n> > On Tue, Jul 02, 2024 at 11:51:03AM +0100, Kieran Bingham wrote:\n> > > Quoting Jacopo Mondi (2024-07-02 09:00:44)\n> > > > On Mon, Jul 01, 2024 at 04:38:26PM GMT, Stefan Klug wrote:\n> > > > > The black levels from the camera sensor helper are then used to do the\n> > > > > black level correction. Black levels can still be overwritten by the\n> > > > > tuning file.\n> \n> The commit message should be readable independently from the subject\n> line. The first sentence here sounds weird when you do so. I feel\n> obliged to point to the obligatory https://cbea.ms/git-commit/ and to\n> emphasize the \"why\" of rule 7 :-)\n> \n> > > > >\n> > > > > Signed-off-by: Stefan Klug \n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----\n> > > > > 1 file changed, 24 insertions(+), 4 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > index d2e743541c99..0c39c3b47da5 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > > > > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > > > > const YamlObject &tuningData)\n> > > > > {\n> > > > > - blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > > - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > > - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > > - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > > > + auto blackLevels = context.camHelper->blackLevels();\n> > > > > + if (blackLevels) {\n> > > > > + Span levels = *blackLevels;\n> > > > > + blackLevelRed_ = levels[0];\n> > > > > + blackLevelGreenR_ = levels[1];\n> > > > > + blackLevelGreenB_ = levels[2];\n> > > > > + blackLevelBlue_ = levels[3];\n> > > > > + } else\n> > > > > + LOG(RkISP1Blc, Warning)\n> > > > > + << \"No black levels provided by camera sensor helper\";\n> \n> \t\t\t<< \"No black levels provided by camera sensor helper, please fix\";\n> \n> Another form of \\todo. Maybe we ned a Fixme log level :-)\n> \n> > > > > +\n> > > > > + if (!blackLevels || (tuningData.contains(\"R\") &&\n> > > > \n> > > > Is there any need to get context.camHelper->blackLevels() if the\n> > > > values are in the config file ?\n> > > > \n> > > > > + tuningData.contains(\"Gr\") &&\n> > > > > + tuningData.contains(\"Gb\") &&\n> > > > > + tuningData.contains(\"B\"))) {\n> > > > > + blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > > + blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > > + blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > > + blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > \n> > > Why 256 here? Does a value have to be specified?\n> > \n> > Digging into the git history... these were taken from the imx219 tuning\n> > file (and scaled to 12bits) as sensible defaults. We could either keep\n> > them or error out. My strategy was to stay compatible with existing\n> > tuning files. If we go the \"Either correct data or fail\" route, we\n> > should remove the defaults.\n> \n> We use defaults here in two cases:\n> \n> - The camera sensor helper doesn't provide black levels, so we fall back\n> to the tuning file for backward compatibility. We should keep the\n> current behaviour in that case.\n> \n> - The keys are provided in the tuning file but the data is incorrect.\n> That's an error. We can error out, or pick a default, I don't mind\n> much either way. Given that the first case points to keeping the\n> current behaviour, using the same defaults without printing any\n> message seems the simplest option.\n> \n> > > Maybe we should move a parse black level helper out to some common yaml\n> > > helpers (Do we have anywhere for that yet, that can read defined\n> > > libcamera types from yaml?) ... and it should only return all values if\n> > > all of them are present and parsed correctly - otherwise R=3200, Gr=256,\n> > > Gb=256, B=256 probably wouldn't make sense?\n> > \n> > No that wouldn't make much sense, but is the old behaviour :-). I can\n> > have a look there.\n> > \n> > > > > +\n> > > > > + if (blackLevels)\n> > > > \n> > > > Is this a Warning ? Same question above. I would Warn only if no BLC in\n> > > > sensor helpers AND in config file.\n> > > > \n> > > > > + LOG(RkISP1Blc, Warning)\n> > > > > + << \"Black levels overwritten by tuning file\";\n> > > \n> > > Yes, I think the tuning file should take precedence, so I don't think\n> > > it's a warning. Perhaps a debug print to say which values get loaded and\n> > > where from ?\n> > \n> > As said in my answer to Jacopo, the warning was on purpose, but I'm fine\n> > with making it a debug output (or info?)\n> \n> Maybe we can make it clearer:\n> \n> \t\tLOG(RkISP1Blc, Warning)\n> \t\t\t<< \"Deprecated: black levels overwritten by tuning file\";\n> \n> Do we need a Deprecated log level ? :-)\n> \n> And now that I wrote that, I wonder if we need to preserve this\n> behaviour. The goal, when I discussed it with Stefan, was to avoid\n> breaking existing setups using out-of-tree tuning data that contains\n> black levels different from the data pedestal provided by the camera\n> sensor helpers. One reason for doing is is to take dark currents into\n> account, and I'm not sure anyone would have done this kind of tuning\n> given that we didn't provide any tooling to measure the black level.\n> Another reason would be to tweak the values \"just because\". Given how\n> we're evolving the black level compensation implementation (not just in\n> the algorithm but also in the tuning tools), I'm tempted to say we\n> shouldn't preserve backward compatibility here.\n> \n> Also, would the following express the logic more clearly ?\n> \n> \tstd::optional levelRed = tuningData.contains(\"R\").get;\n> \tstd::optional levelGreenR = tuningData.contains(\"Gr\").get;\n> \tstd::optional levelGreenB = tuningData.contains(\"Gb\").get;\n> \tstd::optional levelBlue = tuningData.contains(\"B\").get;\n> \n> \tbool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;\n> \n> \tauto blackLevels = context.camHelper->blackLevels();\n> \n> \tif (!blackLevels) {\n> \t\t/*\n> \t\t * Not all camera sensor helpers have been updated with black\n> \t\t * levels. Print a warning and fall back to the levels from the\n> \t\t * tuning data to preserve backward compatibility. This should\n> \t\t * be removed once allhelpers provide the data.\n> \t\t */\n> \t\tLOG(RkISP1Blc, Warning)\n> \t\t\t<< \"No black levels provided by camera sensor helper, please fix\";\n> \n> \t\tblackLevelRed_ = levelRed.value_or(256);\n> \t\tblackLevelGreenR_ = levelGreenR.value_or(256);\n> \t\tblackLevelGreenB_ = levelGreenB.value_or(256);\n> \t\tblackLevelBlue_ = levelBlue.value_or(256);\n> \t} else if (tuningHasLevels) {\n> \t\t/*\n> \t\t * If black levels are provided in the tuning file, use them to\n> \t\t * avoid breaking existing camera tuning. This is deprecated and\n> \t\t * will be removed.\n> \t\t */\n> \t\tLOG(RkISP1Blc, Warning)\n> \t\t\t<< \"Deprecated: black levels overwritten by tuning file\";\n> \n> \t\tblackLevelRed_ = *levelRed;\n> \t\tblackLevelGreenR_ = *levelGreenR;\n> \t\tblackLevelGreenB_ = *levelGreenB;\n> \t\tblackLevelBlue_ = *levelBlue;\n> \t} else {\n> \t\tSpan levels = *blackLevels;\n> \t\tblackLevelRed_ = levels[0];\n> \t\tblackLevelGreenR_ = levels[1];\n> \t\tblackLevelGreenB_ = levels[2];\n> \t\tblackLevelBlue_ = levels[3];\n> \t}\n\nI like that proposal. It is definitely easier to follow. While\nimplementing that, I realized that we are breaking backwards\ncompatibility anyways as we should interpret the values differently\n(based on 16bit width instead of 12bit). To stay really backwards\ncompatible we could add a \"referenceBitDepth\" field to the tuning data\nand fall back to 12bit if that is not set (I implemented that in the\ncompanding branch). But honestly I would rather switch to 16bit and\nbreak the old files instead of carrying around that additional flag.\n\nWhen we break the old files, we could directly go to the solution you\nproposed below.\n\nOpinions?\n\nBest regards,\nStefan\n\n> \n> If we decide to drop the second case then it could be simplified to\n> \n> \tauto blackLevels = context.camHelper->blackLevels();\n> \n> \tif (blackLevels) {\n> \t\tSpan levels = *blackLevels;\n> \t\tblackLevelRed_ = levels[0];\n> \t\tblackLevelGreenR_ = levels[1];\n> \t\tblackLevelGreenB_ = levels[2];\n> \t\tblackLevelBlue_ = levels[3];\n> \t} else {\n> \t\t/*\n> \t\t * Not all camera sensor helpers have been updated with black\n> \t\t * levels. Print a warning and fall back to the levels from the\n> \t\t * tuning data to preserve backward compatibility. This should\n> \t\t * be removed once allhelpers provide the data.\n> \t\t */\n> \t\tLOG(RkISP1Blc, Warning)\n> \t\t\t<< \"No black levels provided by camera sensor helper, please fix\";\n> \n> \t\tblackLevelRed_ = tuningData[\"R\"].get(256);\n> \t\tblackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> \t\tblackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> \t\tblackLevelBlue_ = tuningData[\"B\"].get(256);\n> \t}\n> \n> > > > > + }\n> > > > >\n> > > > > tuningParameters_ = true;\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 D33AFBEFBE\n\tfor ;\n\tWed, 3 Jul 2024 07:30:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A08F4619C7;\n\tWed, 3 Jul 2024 09:30:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA369619C7\n\tfor ;\n\tWed, 3 Jul 2024 09:30:22 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:9263:c199:9587:576])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8817F5A4;\n\tWed, 3 Jul 2024 09:29:54 +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=\"RBqBnF9c\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719991794;\n\tbh=81XjUmVpfLqFQge1n/B5nQzvseyvWAY5JYBFRcSWpUQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RBqBnF9c0KZkEX/5BqRIU+FKjIkylYAvS5H8SIzh5QhzPMOUauuE1pTj4b8kh7hKS\n\tBf+BnCVqtYvD657vHUNknDLg8e+LGuK9mgZEC0EWc4pAVQnILU9Txu6wrKmHPeORBJ\n\tw3uGqEHhX3Nco7Qhbons6LJ5864c6QBbO306XG04=","Date":"Wed, 3 Jul 2024 09:30:19 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , \n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","Message-ID":"","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-4-stefan.klug@ideasonboard.com>\n\t\n\t<171991746343.3353312.18367841913910179492@ping.linuxembedded.co.uk>\n\t<4lfrzdtbovsv6pkdlcr622bvlx2qkozsmeyuoury5fgu5e4g4g@icq2j64xe4gz>\n\t<20240702145025.GD26760@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240702145025.GD26760@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":30242,"web_url":"https://patchwork.libcamera.org/comment/30242/","msgid":"<20240703080206.GB30367@pendragon.ideasonboard.com>","date":"2024-07-03T08:02:06","subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jul 03, 2024 at 09:30:19AM +0200, Stefan Klug wrote:\n> Hi Laurent,\n> \n> On Tue, Jul 02, 2024 at 05:50:25PM +0300, Laurent Pinchart wrote:\n> > On Tue, Jul 02, 2024 at 03:27:56PM +0200, Stefan Klug wrote:\n> > > On Tue, Jul 02, 2024 at 11:51:03AM +0100, Kieran Bingham wrote:\n> > > > Quoting Jacopo Mondi (2024-07-02 09:00:44)\n> > > > > On Mon, Jul 01, 2024 at 04:38:26PM GMT, Stefan Klug wrote:\n> > > > > > The black levels from the camera sensor helper are then used to do the\n> > > > > > black level correction. Black levels can still be overwritten by the\n> > > > > > tuning file.\n> > \n> > The commit message should be readable independently from the subject\n> > line. The first sentence here sounds weird when you do so. I feel\n> > obliged to point to the obligatory https://cbea.ms/git-commit/ and to\n> > emphasize the \"why\" of rule 7 :-)\n> > \n> > > > > >\n> > > > > > Signed-off-by: Stefan Klug \n> > > > > > ---\n> > > > > > src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----\n> > > > > > 1 file changed, 24 insertions(+), 4 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > > index d2e743541c99..0c39c3b47da5 100644\n> > > > > > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > > @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > > > > > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > > > > > const YamlObject &tuningData)\n> > > > > > {\n> > > > > > - blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > > > - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > > > - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > > > - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > > > > + auto blackLevels = context.camHelper->blackLevels();\n> > > > > > + if (blackLevels) {\n> > > > > > + Span levels = *blackLevels;\n> > > > > > + blackLevelRed_ = levels[0];\n> > > > > > + blackLevelGreenR_ = levels[1];\n> > > > > > + blackLevelGreenB_ = levels[2];\n> > > > > > + blackLevelBlue_ = levels[3];\n> > > > > > + } else\n> > > > > > + LOG(RkISP1Blc, Warning)\n> > > > > > + << \"No black levels provided by camera sensor helper\";\n> > \n> > \t\t\t<< \"No black levels provided by camera sensor helper, please fix\";\n> > \n> > Another form of \\todo. Maybe we ned a Fixme log level :-)\n> > \n> > > > > > +\n> > > > > > + if (!blackLevels || (tuningData.contains(\"R\") &&\n> > > > > \n> > > > > Is there any need to get context.camHelper->blackLevels() if the\n> > > > > values are in the config file ?\n> > > > > \n> > > > > > + tuningData.contains(\"Gr\") &&\n> > > > > > + tuningData.contains(\"Gb\") &&\n> > > > > > + tuningData.contains(\"B\"))) {\n> > > > > > + blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > > > + blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > > > + blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > > > + blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > > \n> > > > Why 256 here? Does a value have to be specified?\n> > > \n> > > Digging into the git history... these were taken from the imx219 tuning\n> > > file (and scaled to 12bits) as sensible defaults. We could either keep\n> > > them or error out. My strategy was to stay compatible with existing\n> > > tuning files. If we go the \"Either correct data or fail\" route, we\n> > > should remove the defaults.\n> > \n> > We use defaults here in two cases:\n> > \n> > - The camera sensor helper doesn't provide black levels, so we fall back\n> > to the tuning file for backward compatibility. We should keep the\n> > current behaviour in that case.\n> > \n> > - The keys are provided in the tuning file but the data is incorrect.\n> > That's an error. We can error out, or pick a default, I don't mind\n> > much either way. Given that the first case points to keeping the\n> > current behaviour, using the same defaults without printing any\n> > message seems the simplest option.\n> > \n> > > > Maybe we should move a parse black level helper out to some common yaml\n> > > > helpers (Do we have anywhere for that yet, that can read defined\n> > > > libcamera types from yaml?) ... and it should only return all values if\n> > > > all of them are present and parsed correctly - otherwise R=3200, Gr=256,\n> > > > Gb=256, B=256 probably wouldn't make sense?\n> > > \n> > > No that wouldn't make much sense, but is the old behaviour :-). I can\n> > > have a look there.\n> > > \n> > > > > > +\n> > > > > > + if (blackLevels)\n> > > > > \n> > > > > Is this a Warning ? Same question above. I would Warn only if no BLC in\n> > > > > sensor helpers AND in config file.\n> > > > > \n> > > > > > + LOG(RkISP1Blc, Warning)\n> > > > > > + << \"Black levels overwritten by tuning file\";\n> > > > \n> > > > Yes, I think the tuning file should take precedence, so I don't think\n> > > > it's a warning. Perhaps a debug print to say which values get loaded and\n> > > > where from ?\n> > > \n> > > As said in my answer to Jacopo, the warning was on purpose, but I'm fine\n> > > with making it a debug output (or info?)\n> > \n> > Maybe we can make it clearer:\n> > \n> > \t\tLOG(RkISP1Blc, Warning)\n> > \t\t\t<< \"Deprecated: black levels overwritten by tuning file\";\n> > \n> > Do we need a Deprecated log level ? :-)\n> > \n> > And now that I wrote that, I wonder if we need to preserve this\n> > behaviour. The goal, when I discussed it with Stefan, was to avoid\n> > breaking existing setups using out-of-tree tuning data that contains\n> > black levels different from the data pedestal provided by the camera\n> > sensor helpers. One reason for doing is is to take dark currents into\n> > account, and I'm not sure anyone would have done this kind of tuning\n> > given that we didn't provide any tooling to measure the black level.\n> > Another reason would be to tweak the values \"just because\". Given how\n> > we're evolving the black level compensation implementation (not just in\n> > the algorithm but also in the tuning tools), I'm tempted to say we\n> > shouldn't preserve backward compatibility here.\n> > \n> > Also, would the following express the logic more clearly ?\n> > \n> > \tstd::optional levelRed = tuningData.contains(\"R\").get;\n> > \tstd::optional levelGreenR = tuningData.contains(\"Gr\").get;\n> > \tstd::optional levelGreenB = tuningData.contains(\"Gb\").get;\n> > \tstd::optional levelBlue = tuningData.contains(\"B\").get;\n> > \n> > \tbool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;\n> > \n> > \tauto blackLevels = context.camHelper->blackLevels();\n> > \n> > \tif (!blackLevels) {\n> > \t\t/*\n> > \t\t * Not all camera sensor helpers have been updated with black\n> > \t\t * levels. Print a warning and fall back to the levels from the\n> > \t\t * tuning data to preserve backward compatibility. This should\n> > \t\t * be removed once allhelpers provide the data.\n> > \t\t */\n> > \t\tLOG(RkISP1Blc, Warning)\n> > \t\t\t<< \"No black levels provided by camera sensor helper, please fix\";\n> > \n> > \t\tblackLevelRed_ = levelRed.value_or(256);\n> > \t\tblackLevelGreenR_ = levelGreenR.value_or(256);\n> > \t\tblackLevelGreenB_ = levelGreenB.value_or(256);\n> > \t\tblackLevelBlue_ = levelBlue.value_or(256);\n> > \t} else if (tuningHasLevels) {\n> > \t\t/*\n> > \t\t * If black levels are provided in the tuning file, use them to\n> > \t\t * avoid breaking existing camera tuning. This is deprecated and\n> > \t\t * will be removed.\n> > \t\t */\n> > \t\tLOG(RkISP1Blc, Warning)\n> > \t\t\t<< \"Deprecated: black levels overwritten by tuning file\";\n> > \n> > \t\tblackLevelRed_ = *levelRed;\n> > \t\tblackLevelGreenR_ = *levelGreenR;\n> > \t\tblackLevelGreenB_ = *levelGreenB;\n> > \t\tblackLevelBlue_ = *levelBlue;\n> > \t} else {\n> > \t\tSpan levels = *blackLevels;\n> > \t\tblackLevelRed_ = levels[0];\n> > \t\tblackLevelGreenR_ = levels[1];\n> > \t\tblackLevelGreenB_ = levels[2];\n> > \t\tblackLevelBlue_ = levels[3];\n> > \t}\n> \n> I like that proposal. It is definitely easier to follow. While\n> implementing that, I realized that we are breaking backwards\n> compatibility anyways as we should interpret the values differently\n> (based on 16bit width instead of 12bit). To stay really backwards\n> compatible we could add a \"referenceBitDepth\" field to the tuning data\n> and fall back to 12bit if that is not set (I implemented that in the\n> companding branch). But honestly I would rather switch to 16bit and\n> break the old files instead of carrying around that additional flag.\n> \n> When we break the old files, we could directly go to the solution you\n> proposed below.\n> \n> Opinions?\n\nI'm fine with that, but I think we should then add black levels to the\ncamera sensor helpers for all sensors that currently have the levels set\nin tuning files. That's OV4689, OV5640 and IMX219.\n\n> > If we decide to drop the second case then it could be simplified to\n> > \n> > \tauto blackLevels = context.camHelper->blackLevels();\n> > \n> > \tif (blackLevels) {\n> > \t\tSpan levels = *blackLevels;\n> > \t\tblackLevelRed_ = levels[0];\n> > \t\tblackLevelGreenR_ = levels[1];\n> > \t\tblackLevelGreenB_ = levels[2];\n> > \t\tblackLevelBlue_ = levels[3];\n> > \t} else {\n> > \t\t/*\n> > \t\t * Not all camera sensor helpers have been updated with black\n> > \t\t * levels. Print a warning and fall back to the levels from the\n> > \t\t * tuning data to preserve backward compatibility. This should\n> > \t\t * be removed once allhelpers provide the data.\n> > \t\t */\n> > \t\tLOG(RkISP1Blc, Warning)\n> > \t\t\t<< \"No black levels provided by camera sensor helper, please fix\";\n> > \n> > \t\tblackLevelRed_ = tuningData[\"R\"].get(256);\n> > \t\tblackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > \t\tblackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > \t\tblackLevelBlue_ = tuningData[\"B\"].get(256);\n> > \t}\n> > \n> > > > > > + }\n> > > > > >\n> > > > > > tuningParameters_ = true;\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 0660ABEFBE\n\tfor ;\n\tWed, 3 Jul 2024 08:02:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D0D7619C7;\n\tWed, 3 Jul 2024 10:02:29 +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 C96E7619C7\n\tfor ;\n\tWed, 3 Jul 2024 10:02:27 +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 B0B2F6D6;\n\tWed, 3 Jul 2024 10:01:59 +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=\"F0DEehb7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719993719;\n\tbh=yOM7+Ksrxvg7cwuQVTocpmX2KAQGlIUiZbhGOEboDrc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F0DEehb7uxxK9sMcQcxlDZbNQbQ3zE0TUElChJINnxXUjMz61avtX5YSfCyDWhBZ+\n\tqRj1StbY71yivs+EbS46oEGNFSjUy1QmLcjLxLc3cH32EnobcBvM+c7/9tH0jCUU5F\n\t2HOlzlcYaZlh+r72gcg9OmSEQ8h+paRrnc0sXgNU=","Date":"Wed, 3 Jul 2024 11:02:06 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"Kieran Bingham ,\n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","Message-ID":"<20240703080206.GB30367@pendragon.ideasonboard.com>","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-4-stefan.klug@ideasonboard.com>\n\t\n\t<171991746343.3353312.18367841913910179492@ping.linuxembedded.co.uk>\n\t<4lfrzdtbovsv6pkdlcr622bvlx2qkozsmeyuoury5fgu5e4g4g@icq2j64xe4gz>\n\t<20240702145025.GD26760@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":30245,"web_url":"https://patchwork.libcamera.org/comment/30245/","msgid":"<171999528177.1723605.18368842510898383848@ping.linuxembedded.co.uk>","date":"2024-07-03T08:28:01","subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-07-03 08:30:19)\n> Hi Laurent,\n> \n> On Tue, Jul 02, 2024 at 05:50:25PM +0300, Laurent Pinchart wrote:\n> > On Tue, Jul 02, 2024 at 03:27:56PM +0200, Stefan Klug wrote:\n> > > On Tue, Jul 02, 2024 at 11:51:03AM +0100, Kieran Bingham wrote:\n> > > > Quoting Jacopo Mondi (2024-07-02 09:00:44)\n> > > > > On Mon, Jul 01, 2024 at 04:38:26PM GMT, Stefan Klug wrote:\n> > > > > > The black levels from the camera sensor helper are then used to do the\n> > > > > > black level correction. Black levels can still be overwritten by the\n> > > > > > tuning file.\n> > \n> > The commit message should be readable independently from the subject\n> > line. The first sentence here sounds weird when you do so. I feel\n> > obliged to point to the obligatory https://cbea.ms/git-commit/ and to\n> > emphasize the \"why\" of rule 7 :-)\n> > \n> > > > > >\n> > > > > > Signed-off-by: Stefan Klug \n> > > > > > ---\n> > > > > > src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----\n> > > > > > 1 file changed, 24 insertions(+), 4 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > > index d2e743541c99..0c39c3b47da5 100644\n> > > > > > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > > @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > > > > > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > > > > > const YamlObject &tuningData)\n> > > > > > {\n> > > > > > - blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > > > - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > > > - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > > > - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > > > > + auto blackLevels = context.camHelper->blackLevels();\n> > > > > > + if (blackLevels) {\n> > > > > > + Span levels = *blackLevels;\n> > > > > > + blackLevelRed_ = levels[0];\n> > > > > > + blackLevelGreenR_ = levels[1];\n> > > > > > + blackLevelGreenB_ = levels[2];\n> > > > > > + blackLevelBlue_ = levels[3];\n> > > > > > + } else\n> > > > > > + LOG(RkISP1Blc, Warning)\n> > > > > > + << \"No black levels provided by camera sensor helper\";\n> > \n> > << \"No black levels provided by camera sensor helper, please fix\";\n> > \n> > Another form of \\todo. Maybe we ned a Fixme log level :-)\n> > \n> > > > > > +\n> > > > > > + if (!blackLevels || (tuningData.contains(\"R\") &&\n> > > > > \n> > > > > Is there any need to get context.camHelper->blackLevels() if the\n> > > > > values are in the config file ?\n> > > > > \n> > > > > > + tuningData.contains(\"Gr\") &&\n> > > > > > + tuningData.contains(\"Gb\") &&\n> > > > > > + tuningData.contains(\"B\"))) {\n> > > > > > + blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > > > + blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > > > + blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > > > + blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > > \n> > > > Why 256 here? Does a value have to be specified?\n> > > \n> > > Digging into the git history... these were taken from the imx219 tuning\n> > > file (and scaled to 12bits) as sensible defaults. We could either keep\n> > > them or error out. My strategy was to stay compatible with existing\n> > > tuning files. If we go the \"Either correct data or fail\" route, we\n> > > should remove the defaults.\n> > \n> > We use defaults here in two cases:\n> > \n> > - The camera sensor helper doesn't provide black levels, so we fall back\n> > to the tuning file for backward compatibility. We should keep the\n> > current behaviour in that case.\n> > \n> > - The keys are provided in the tuning file but the data is incorrect.\n> > That's an error. We can error out, or pick a default, I don't mind\n> > much either way. Given that the first case points to keeping the\n> > current behaviour, using the same defaults without printing any\n> > message seems the simplest option.\n> > \n> > > > Maybe we should move a parse black level helper out to some common yaml\n> > > > helpers (Do we have anywhere for that yet, that can read defined\n> > > > libcamera types from yaml?) ... and it should only return all values if\n> > > > all of them are present and parsed correctly - otherwise R=3200, Gr=256,\n> > > > Gb=256, B=256 probably wouldn't make sense?\n> > > \n> > > No that wouldn't make much sense, but is the old behaviour :-). I can\n> > > have a look there.\n> > > \n> > > > > > +\n> > > > > > + if (blackLevels)\n> > > > > \n> > > > > Is this a Warning ? Same question above. I would Warn only if no BLC in\n> > > > > sensor helpers AND in config file.\n> > > > > \n> > > > > > + LOG(RkISP1Blc, Warning)\n> > > > > > + << \"Black levels overwritten by tuning file\";\n> > > > \n> > > > Yes, I think the tuning file should take precedence, so I don't think\n> > > > it's a warning. Perhaps a debug print to say which values get loaded and\n> > > > where from ?\n> > > \n> > > As said in my answer to Jacopo, the warning was on purpose, but I'm fine\n> > > with making it a debug output (or info?)\n> > \n> > Maybe we can make it clearer:\n> > \n> > LOG(RkISP1Blc, Warning)\n> > << \"Deprecated: black levels overwritten by tuning file\";\n> > \n> > Do we need a Deprecated log level ? :-)\n> > \n> > And now that I wrote that, I wonder if we need to preserve this\n> > behaviour. The goal, when I discussed it with Stefan, was to avoid\n> > breaking existing setups using out-of-tree tuning data that contains\n> > black levels different from the data pedestal provided by the camera\n> > sensor helpers. One reason for doing is is to take dark currents into\n> > account, and I'm not sure anyone would have done this kind of tuning\n> > given that we didn't provide any tooling to measure the black level.\n> > Another reason would be to tweak the values \"just because\". Given how\n> > we're evolving the black level compensation implementation (not just in\n> > the algorithm but also in the tuning tools), I'm tempted to say we\n> > shouldn't preserve backward compatibility here.\n> > \n> > Also, would the following express the logic more clearly ?\n> > \n> > std::optional levelRed = tuningData.contains(\"R\").get;\n> > std::optional levelGreenR = tuningData.contains(\"Gr\").get;\n> > std::optional levelGreenB = tuningData.contains(\"Gb\").get;\n> > std::optional levelBlue = tuningData.contains(\"B\").get;\n> > \n> > bool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;\n> > \n> > auto blackLevels = context.camHelper->blackLevels();\n> > \n> > if (!blackLevels) {\n> > /*\n> > * Not all camera sensor helpers have been updated with black\n> > * levels. Print a warning and fall back to the levels from the\n> > * tuning data to preserve backward compatibility. This should\n> > * be removed once allhelpers provide the data.\n> > */\n> > LOG(RkISP1Blc, Warning)\n> > << \"No black levels provided by camera sensor helper, please fix\";\n> > \n> > blackLevelRed_ = levelRed.value_or(256);\n> > blackLevelGreenR_ = levelGreenR.value_or(256);\n> > blackLevelGreenB_ = levelGreenB.value_or(256);\n> > blackLevelBlue_ = levelBlue.value_or(256);\n> > } else if (tuningHasLevels) {\n> > /*\n> > * If black levels are provided in the tuning file, use them to\n> > * avoid breaking existing camera tuning. This is deprecated and\n> > * will be removed.\n> > */\n> > LOG(RkISP1Blc, Warning)\n> > << \"Deprecated: black levels overwritten by tuning file\";\n> > \n> > blackLevelRed_ = *levelRed;\n> > blackLevelGreenR_ = *levelGreenR;\n> > blackLevelGreenB_ = *levelGreenB;\n> > blackLevelBlue_ = *levelBlue;\n> > } else {\n> > Span levels = *blackLevels;\n> > blackLevelRed_ = levels[0];\n> > blackLevelGreenR_ = levels[1];\n> > blackLevelGreenB_ = levels[2];\n> > blackLevelBlue_ = levels[3];\n> > }\n> \n> I like that proposal. It is definitely easier to follow. While\n> implementing that, I realized that we are breaking backwards\n> compatibility anyways as we should interpret the values differently\n> (based on 16bit width instead of 12bit). To stay really backwards\n> compatible we could add a \"referenceBitDepth\" field to the tuning data\n> and fall back to 12bit if that is not set (I implemented that in the\n> companding branch). But honestly I would rather switch to 16bit and\n> break the old files instead of carrying around that additional flag.\n> \n> When we break the old files, we could directly go to the solution you\n> proposed below.\n\nI didn't think we needed to maintain any backwards compatibilty here, as\nI thought we didn't have BLC fully implemented - but now I realise it\nwas/is working on rk3399 ... so that's more awkward.\n\nIt's 'unlikely' that anyone else is specifically using external tuning\nfiles, but in theory we don't know - and others could be.\n\nThen if we were going to do an ABI jump on libcamera, I would say it's\neven easier to justify breaking the previous values too ... but I was\nactually anticipating the next libcamera release in ~2 weeks to be the\nfirst opportunity to make a non-ABI breaking release!\n\nI'm so conflicted ;D\n\nWe could also keep the backwards compatiblity for 'one' release if that\nhelps with a warning, and clean the code up after the next libcamera\ntag?\n\n--\nKieran\n\n\n\n> \n> Opinions?\n> \n> Best regards,\n> Stefan\n> \n> > \n> > If we decide to drop the second case then it could be simplified to\n> > \n> > auto blackLevels = context.camHelper->blackLevels();\n> > \n> > if (blackLevels) {\n> > Span levels = *blackLevels;\n> > blackLevelRed_ = levels[0];\n> > blackLevelGreenR_ = levels[1];\n> > blackLevelGreenB_ = levels[2];\n> > blackLevelBlue_ = levels[3];\n> > } else {\n> > /*\n> > * Not all camera sensor helpers have been updated with black\n> > * levels. Print a warning and fall back to the levels from the\n> > * tuning data to preserve backward compatibility. This should\n> > * be removed once allhelpers provide the data.\n> > */\n> > LOG(RkISP1Blc, Warning)\n> > << \"No black levels provided by camera sensor helper, please fix\";\n> > \n> > blackLevelRed_ = tuningData[\"R\"].get(256);\n> > blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > }\n> > \n> > > > > > + }\n> > > > > >\n> > > > > > tuningParameters_ = true;\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 7A29DBD87C\n\tfor ;\n\tWed, 3 Jul 2024 08:28:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B00D62E23;\n\tWed, 3 Jul 2024 10:28:07 +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 2505B619C7\n\tfor ;\n\tWed, 3 Jul 2024 10:28:05 +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 E85E66D6;\n\tWed, 3 Jul 2024 10:27: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=\"jwACHT5Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719995257;\n\tbh=YkgTKWCM4M6twLCWddewzLy6ysun8zwEMxjXtYgAWPg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=jwACHT5ZBYfmefZu9IkicMnv90gFf9Tc3BIbubVPPcFJVZdx6s/RFd320nb08Myoh\n\taH7z+SzrKh84HZEV+7K3MiURwUni9dFSoWLF/zuvXP2B89XWWxTFD2Cs1tTDgfu5Zo\n\tN3dlMEAyQMx6HjnCiQu1CzXqBkSSU8JXpUjPMMkI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-4-stefan.klug@ideasonboard.com>\n\t\n\t<171991746343.3353312.18367841913910179492@ping.linuxembedded.co.uk>\n\t<4lfrzdtbovsv6pkdlcr622bvlx2qkozsmeyuoury5fgu5e4g4g@icq2j64xe4gz>\n\t<20240702145025.GD26760@pendragon.ideasonboard.com>\n\t","Subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","From":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tStefan Klug ","Date":"Wed, 03 Jul 2024 09:28:01 +0100","Message-ID":"<171999528177.1723605.18368842510898383848@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":30246,"web_url":"https://patchwork.libcamera.org/comment/30246/","msgid":"<171999541392.1723605.2375935289169616753@ping.linuxembedded.co.uk>","date":"2024-07-03T08:30:13","subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-07-03 09:02:06)\n> On Wed, Jul 03, 2024 at 09:30:19AM +0200, Stefan Klug wrote:\n> > Hi Laurent,\n> > \n> > On Tue, Jul 02, 2024 at 05:50:25PM +0300, Laurent Pinchart wrote:\n> > > On Tue, Jul 02, 2024 at 03:27:56PM +0200, Stefan Klug wrote:\n> > > > On Tue, Jul 02, 2024 at 11:51:03AM +0100, Kieran Bingham wrote:\n> > > > > Quoting Jacopo Mondi (2024-07-02 09:00:44)\n> > > > > > On Mon, Jul 01, 2024 at 04:38:26PM GMT, Stefan Klug wrote:\n> > > > > > > The black levels from the camera sensor helper are then used to do the\n> > > > > > > black level correction. Black levels can still be overwritten by the\n> > > > > > > tuning file.\n> > > \n> > > The commit message should be readable independently from the subject\n> > > line. The first sentence here sounds weird when you do so. I feel\n> > > obliged to point to the obligatory https://cbea.ms/git-commit/ and to\n> > > emphasize the \"why\" of rule 7 :-)\n> > > \n> > > > > > >\n> > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > ---\n> > > > > > > src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----\n> > > > > > > 1 file changed, 24 insertions(+), 4 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > > > index d2e743541c99..0c39c3b47da5 100644\n> > > > > > > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > > > > @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > > > > > > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > > > > > > const YamlObject &tuningData)\n> > > > > > > {\n> > > > > > > - blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > > > > - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > > > > - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > > > > - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > > > > > + auto blackLevels = context.camHelper->blackLevels();\n> > > > > > > + if (blackLevels) {\n> > > > > > > + Span levels = *blackLevels;\n> > > > > > > + blackLevelRed_ = levels[0];\n> > > > > > > + blackLevelGreenR_ = levels[1];\n> > > > > > > + blackLevelGreenB_ = levels[2];\n> > > > > > > + blackLevelBlue_ = levels[3];\n> > > > > > > + } else\n> > > > > > > + LOG(RkISP1Blc, Warning)\n> > > > > > > + << \"No black levels provided by camera sensor helper\";\n> > > \n> > > << \"No black levels provided by camera sensor helper, please fix\";\n> > > \n> > > Another form of \\todo. Maybe we ned a Fixme log level :-)\n> > > \n> > > > > > > +\n> > > > > > > + if (!blackLevels || (tuningData.contains(\"R\") &&\n> > > > > > \n> > > > > > Is there any need to get context.camHelper->blackLevels() if the\n> > > > > > values are in the config file ?\n> > > > > > \n> > > > > > > + tuningData.contains(\"Gr\") &&\n> > > > > > > + tuningData.contains(\"Gb\") &&\n> > > > > > > + tuningData.contains(\"B\"))) {\n> > > > > > > + blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > > > > + blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > > > > + blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > > > > + blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > > > \n> > > > > Why 256 here? Does a value have to be specified?\n> > > > \n> > > > Digging into the git history... these were taken from the imx219 tuning\n> > > > file (and scaled to 12bits) as sensible defaults. We could either keep\n> > > > them or error out. My strategy was to stay compatible with existing\n> > > > tuning files. If we go the \"Either correct data or fail\" route, we\n> > > > should remove the defaults.\n> > > \n> > > We use defaults here in two cases:\n> > > \n> > > - The camera sensor helper doesn't provide black levels, so we fall back\n> > > to the tuning file for backward compatibility. We should keep the\n> > > current behaviour in that case.\n> > > \n> > > - The keys are provided in the tuning file but the data is incorrect.\n> > > That's an error. We can error out, or pick a default, I don't mind\n> > > much either way. Given that the first case points to keeping the\n> > > current behaviour, using the same defaults without printing any\n> > > message seems the simplest option.\n> > > \n> > > > > Maybe we should move a parse black level helper out to some common yaml\n> > > > > helpers (Do we have anywhere for that yet, that can read defined\n> > > > > libcamera types from yaml?) ... and it should only return all values if\n> > > > > all of them are present and parsed correctly - otherwise R=3200, Gr=256,\n> > > > > Gb=256, B=256 probably wouldn't make sense?\n> > > > \n> > > > No that wouldn't make much sense, but is the old behaviour :-). I can\n> > > > have a look there.\n> > > > \n> > > > > > > +\n> > > > > > > + if (blackLevels)\n> > > > > > \n> > > > > > Is this a Warning ? Same question above. I would Warn only if no BLC in\n> > > > > > sensor helpers AND in config file.\n> > > > > > \n> > > > > > > + LOG(RkISP1Blc, Warning)\n> > > > > > > + << \"Black levels overwritten by tuning file\";\n> > > > > \n> > > > > Yes, I think the tuning file should take precedence, so I don't think\n> > > > > it's a warning. Perhaps a debug print to say which values get loaded and\n> > > > > where from ?\n> > > > \n> > > > As said in my answer to Jacopo, the warning was on purpose, but I'm fine\n> > > > with making it a debug output (or info?)\n> > > \n> > > Maybe we can make it clearer:\n> > > \n> > > LOG(RkISP1Blc, Warning)\n> > > << \"Deprecated: black levels overwritten by tuning file\";\n> > > \n> > > Do we need a Deprecated log level ? :-)\n> > > \n> > > And now that I wrote that, I wonder if we need to preserve this\n> > > behaviour. The goal, when I discussed it with Stefan, was to avoid\n> > > breaking existing setups using out-of-tree tuning data that contains\n> > > black levels different from the data pedestal provided by the camera\n> > > sensor helpers. One reason for doing is is to take dark currents into\n> > > account, and I'm not sure anyone would have done this kind of tuning\n> > > given that we didn't provide any tooling to measure the black level.\n> > > Another reason would be to tweak the values \"just because\". Given how\n> > > we're evolving the black level compensation implementation (not just in\n> > > the algorithm but also in the tuning tools), I'm tempted to say we\n> > > shouldn't preserve backward compatibility here.\n> > > \n> > > Also, would the following express the logic more clearly ?\n> > > \n> > > std::optional levelRed = tuningData.contains(\"R\").get;\n> > > std::optional levelGreenR = tuningData.contains(\"Gr\").get;\n> > > std::optional levelGreenB = tuningData.contains(\"Gb\").get;\n> > > std::optional levelBlue = tuningData.contains(\"B\").get;\n> > > \n> > > bool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;\n> > > \n> > > auto blackLevels = context.camHelper->blackLevels();\n> > > \n> > > if (!blackLevels) {\n> > > /*\n> > > * Not all camera sensor helpers have been updated with black\n> > > * levels. Print a warning and fall back to the levels from the\n> > > * tuning data to preserve backward compatibility. This should\n> > > * be removed once allhelpers provide the data.\n> > > */\n> > > LOG(RkISP1Blc, Warning)\n> > > << \"No black levels provided by camera sensor helper, please fix\";\n> > > \n> > > blackLevelRed_ = levelRed.value_or(256);\n> > > blackLevelGreenR_ = levelGreenR.value_or(256);\n> > > blackLevelGreenB_ = levelGreenB.value_or(256);\n> > > blackLevelBlue_ = levelBlue.value_or(256);\n> > > } else if (tuningHasLevels) {\n> > > /*\n> > > * If black levels are provided in the tuning file, use them to\n> > > * avoid breaking existing camera tuning. This is deprecated and\n> > > * will be removed.\n> > > */\n> > > LOG(RkISP1Blc, Warning)\n> > > << \"Deprecated: black levels overwritten by tuning file\";\n> > > \n> > > blackLevelRed_ = *levelRed;\n> > > blackLevelGreenR_ = *levelGreenR;\n> > > blackLevelGreenB_ = *levelGreenB;\n> > > blackLevelBlue_ = *levelBlue;\n> > > } else {\n> > > Span levels = *blackLevels;\n> > > blackLevelRed_ = levels[0];\n> > > blackLevelGreenR_ = levels[1];\n> > > blackLevelGreenB_ = levels[2];\n> > > blackLevelBlue_ = levels[3];\n> > > }\n> > \n> > I like that proposal. It is definitely easier to follow. While\n> > implementing that, I realized that we are breaking backwards\n> > compatibility anyways as we should interpret the values differently\n> > (based on 16bit width instead of 12bit). To stay really backwards\n> > compatible we could add a \"referenceBitDepth\" field to the tuning data\n> > and fall back to 12bit if that is not set (I implemented that in the\n> > companding branch). But honestly I would rather switch to 16bit and\n> > break the old files instead of carrying around that additional flag.\n> > \n> > When we break the old files, we could directly go to the solution you\n> > proposed below.\n> > \n> > Opinions?\n> \n> I'm fine with that, but I think we should then add black levels to the\n> camera sensor helpers for all sensors that currently have the levels set\n> in tuning files. That's OV4689, OV5640 and IMX219.\n> \n\nTo be clear, following my reply on this too - I don't object to going\nstraight to the 'right' implementation here.\n\nStoring Black levels as 16 bit consistently makes more sense to me than\nvaried bit depths.\n\n--\nKieran\n\n> > > If we decide to drop the second case then it could be simplified to\n> > > \n> > > auto blackLevels = context.camHelper->blackLevels();\n> > > \n> > > if (blackLevels) {\n> > > Span levels = *blackLevels;\n> > > blackLevelRed_ = levels[0];\n> > > blackLevelGreenR_ = levels[1];\n> > > blackLevelGreenB_ = levels[2];\n> > > blackLevelBlue_ = levels[3];\n> > > } else {\n> > > /*\n> > > * Not all camera sensor helpers have been updated with black\n> > > * levels. Print a warning and fall back to the levels from the\n> > > * tuning data to preserve backward compatibility. This should\n> > > * be removed once allhelpers provide the data.\n> > > */\n> > > LOG(RkISP1Blc, Warning)\n> > > << \"No black levels provided by camera sensor helper, please fix\";\n> > > \n> > > blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > }\n> > > \n> > > > > > > + }\n> > > > > > >\n> > > > > > > tuningParameters_ = true;\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 CA764BD87C\n\tfor ;\n\tWed, 3 Jul 2024 08:30:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D33CD62E01;\n\tWed, 3 Jul 2024 10:30:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21A22619C7\n\tfor ;\n\tWed, 3 Jul 2024 10:30:17 +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 123066D6;\n\tWed, 3 Jul 2024 10:29:49 +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=\"SrDHocKF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719995389;\n\tbh=0aI4sL+L7zBU+rjepYF/4eSW+NXj84tpBqOZl6SLsLo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SrDHocKFCZVjHuIh35iL7ICNeLoHxatQlsnKP+XHA6K15sMWKbPzlq7HxLF1NmqIc\n\t7CYGhW2k/A3qOqpKIbGFPLCKFGHBqvn7YSUUKqjX0TEH0MCGD7U7ZPJRbfKkWmF/uc\n\tRpW3+bQi6rdoTFulbPPR6X0WwwmKvo9EFfkILkPs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240703080206.GB30367@pendragon.ideasonboard.com>","References":"<20240701144122.3418955-1-stefan.klug@ideasonboard.com>\n\t<20240701144122.3418955-4-stefan.klug@ideasonboard.com>\n\t\n\t<171991746343.3353312.18367841913910179492@ping.linuxembedded.co.uk>\n\t<4lfrzdtbovsv6pkdlcr622bvlx2qkozsmeyuoury5fgu5e4g4g@icq2j64xe4gz>\n\t<20240702145025.GD26760@pendragon.ideasonboard.com>\n\t\n\t<20240703080206.GB30367@pendragon.ideasonboard.com>","Subject":"Re: [PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","From":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tStefan Klug ","Date":"Wed, 03 Jul 2024 09:30:13 +0100","Message-ID":"<171999541392.1723605.2375935289169616753@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\" "}}]