[{"id":23156,"web_url":"https://patchwork.libcamera.org/comment/23156/","msgid":"","date":"2022-05-26T11:04:09","subject":"Re: [libcamera-devel] [libcamera-devel 5/5] ipa: rkisp1: add\n\tsupport of Black Level Correction default values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Florian,\n\nThank you for the patch.\n\nOn Mon, May 23, 2022 at 11:24:35AM +0200, Florian Sylvestre via libcamera-devel wrote:\n> Get default values for Black Level Correction algorithm form Yaml\n> configuration file.\n\ns/form Yaml configuration/from YAML tuning/\n\n> Signed-off-by: Florian Sylvestre \n> ---\n> src/ipa/rkisp1/algorithms/blc.cpp | 46 ++++++++++++++++++++++++++++---\n> src/ipa/rkisp1/algorithms/blc.h | 9 ++++++\n> 2 files changed, 51 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> index 0c5948ff..177bc22e 100644\n> --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> @@ -7,6 +7,10 @@\n> \n> #include \"blc.h\"\n> \n> +#include \n> +\n> +#include \n\nFor internal headers we use \"\" instead of <>.\n\n> +\n> /**\n> * \\file blc.h\n> */\n> @@ -29,6 +33,40 @@ namespace ipa::rkisp1::algorithms {\n> * isn't currently supported.\n> */\n> \n> +LOG_DEFINE_CATEGORY(RkISP1Blc)\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::init\n> + */\n> +int BlackLevelCorrection::init(IPAContext &context,\n> +\t\t\t const YamlObject *params)\n> +{\n> +\tif (context.frameContext.frameCount > 0)\n> +\t\treturn -EBUSY;\n> +\n> +\t/* Parse property \"BlackLevelCorrection\" */\n> +\tif (!params->contains(\"BlackLevelCorrection\"))\n> +\t\treturn -EINVAL;\n> +\n> +\tconst YamlObject &blcObject = (*params)[\"BlackLevelCorrection\"];\n> +\n> +\tif (!blcObject.isDictionary())\n> +\t\treturn -EINVAL;\n> +\n> +\tblackLevelRed_ = blcObject[\"R\"].get(256);\n\nCan the value be negative ? If not, let go to uint32_t. Actually, you'll\nhave uint16_t support when rebasing on top of the YAML parser\nimprovements, so you can use that.\n\n> +\tblackLevelGreenR_ = blcObject[\"Gr\"].get(256);\n> +\tblackLevelGreenB_ = blcObject[\"Gb\"].get(256);\n> +\tblackLevelBlue_ = blcObject[\"B\"].get(256);\n> +\n> +\tLOG(RkISP1Blc, Debug)\n> +\t\t<< \" Read black levels red \" << blackLevelRed_\n> +\t\t<< \" green (red) \" << blackLevelGreenR_\n> +\t\t<< \" green (blue) \" << blackLevelGreenB_\n> +\t\t<< \" blue \" << blackLevelBlue_;\n\n\t\t<< \" Read black levels red \" << blackLevelRed_\n\t\t<< \", green (red) \" << blackLevelGreenR_\n\t\t<< \", green (blue) \" << blackLevelGreenB_\n\t\t<< \", blue \" << blackLevelBlue_;\n\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> @@ -42,10 +80,10 @@ void BlackLevelCorrection::prepare(IPAContext &context,\n> \t * \\todo Use a configuration file for it ?\n> \t */\n> \tparams->others.bls_config.enable_auto = 0;\n> -\tparams->others.bls_config.fixed_val.r = 256;\n> -\tparams->others.bls_config.fixed_val.gr = 256;\n> -\tparams->others.bls_config.fixed_val.gb = 256;\n> -\tparams->others.bls_config.fixed_val.b = 256;\n> +\tparams->others.bls_config.fixed_val.r = blackLevelRed_;\n> +\tparams->others.bls_config.fixed_val.gr = blackLevelGreenR_;\n> +\tparams->others.bls_config.fixed_val.gb = blackLevelGreenB_;\n> +\tparams->others.bls_config.fixed_val.b = blackLevelBlue_;\n> \n> \tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS;\n> \tparams->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;\n> diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h\n> index 69874d8f..2f6e2d82 100644\n> --- a/src/ipa/rkisp1/algorithms/blc.h\n> +++ b/src/ipa/rkisp1/algorithms/blc.h\n> @@ -13,6 +13,8 @@\n> \n> namespace libcamera {\n> \n> +class YamlObject;\n\nYou can drop this as it's alread forward-declared by algorithm.h;\n\n> +\n> struct IPACameraSensorInfo;\n> \n> namespace ipa::rkisp1::algorithms {\n> @@ -23,7 +25,14 @@ public:\n> \tBlackLevelCorrection() = default;\n> \t~BlackLevelCorrection() = default;\n> \n> +\tint init(IPAContext &context, const YamlObject *params) override;\n> \tvoid prepare(IPAContext &context, rkisp1_params_cfg *params) override;\n> +\n> +private:\n> +\tint16_t blackLevelRed_;\n> +\tint16_t blackLevelGreenR_;\n> +\tint16_t blackLevelGreenB_;\n> +\tint16_t blackLevelBlue_;\n\nIf the values can't be negative let's use uint16_t here.\n\n> };\n> \n> } /* namespace ipa::rkisp1::algorithms */","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 BA458BD161\n\tfor ;\n\tThu, 26 May 2022 11:04:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EFBAA65663;\n\tThu, 26 May 2022 13:04:17 +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 2967E60418\n\tfor ;\n\tThu, 26 May 2022 13:04:16 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [37.120.212.14])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7CE7D6D4;\n\tThu, 26 May 2022 13:04:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653563058;\n\tbh=LU8hhziuKFjVUI5jasu+OH8t/H1q0QCrMQm5zM0loCk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=l1tDx11Xx7R6dYCCziOHeQGgIj+bpdAJOTU3PUMHKLK+/3JIlB7ija4DAzykq5Wn0\n\tBsSAbBkPo8ggnec8TO0neh0tS7QgLY0Ei8NXCL8DPp05cxj9NzVtTMLw2Ov3CARnJt\n\t/lSnGBmS1xFolLhYBcWahMe8zHSN+zjt+RkrCeBXNFHVxWw8NymDPYeklISh7NeW9/\n\tDCK4bAzVpNUKPBpjLlcMcguNpin+vyJx3TaBlYCoNQMQOKhFCJlF/p8lZcnDXYaN+r\n\tsmlOTsNYEUDEU/Wur95/byaY3V9tG4/s5DxyIV0yDK339oCfiNtVX/T2Z8X19phftr\n\tGOi5tfAhmTCuA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653563055;\n\tbh=LU8hhziuKFjVUI5jasu+OH8t/H1q0QCrMQm5zM0loCk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ugBcASs0kRTl7q4/YSNuK8vQPeoKL5R08watNF1X72OsiExY1JkhobhgXTirdzW/0\n\tnOoMQ55zsFSopPvxw9uPOSlZR3HCJTdJsKAPBLYYto9EciT3Q+AgWMYUS0l6PFAgmy\n\t3uZ2kLrZ6hIcC7WTesyGZX1Bbd0AOS70pxNPR2j4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ugBcASs0\"; dkim-atps=neutral","Date":"Thu, 26 May 2022 14:04:09 +0300","To":"Florian Sylvestre ","Message-ID":"","References":"<20220523092435.475510-1-fsylvestre@baylibre.com>\n\t<20220523092435.475510-6-fsylvestre@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220523092435.475510-6-fsylvestre@baylibre.com>","Subject":"Re: [libcamera-devel] [libcamera-devel 5/5] ipa: rkisp1: add\n\tsupport of Black Level Correction default values","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23277,"web_url":"https://patchwork.libcamera.org/comment/23277/","msgid":"","date":"2022-06-01T08:19:44","subject":"Re: [libcamera-devel] [libcamera-devel 5/5] ipa: rkisp1: add\n\tsupport of Black Level Correction default values","submitter":{"id":123,"url":"https://patchwork.libcamera.org/api/people/123/","name":"Florian Sylvestre","email":"fsylvestre@baylibre.com"},"content":">> + const YamlObject &blcObject = (*params)[\"BlackLevelCorrection\"];\n>> +\n>> + if (!blcObject.isDictionary())\n>> + return -EINVAL;\n>> +\n>> + blackLevelRed_ = blcObject[\"R\"].get(256);\n>Can the value be negative ? If not, let go to uint32_t. Actually, you'll\n>have uint16_t support when rebasing on top of the YAML parser\n>improvements, so you can use that.\nThe values can be negatives.\nFYI: Extracted from .h comments:\n * struct rkisp1_cif_isp_bls_fixed_val - BLS fixed subtraction values\n *\n * The values will be subtracted from the sensor\n * values. Therefore a negative value means addition instead of subtraction!\n\nI have seen the [RFC] on modifications for Yaml Parser to support int16,\nbut since it's 'only' at [RFC] state, should we not provide this version and\nupdate it when the support of int16 will be available?\n\nRegards,\nFlorian Sylvestre\n\nLe jeu. 26 mai 2022 à 13:04, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> a écrit :\n\n> Hi Florian,\n>\n> Thank you for the patch.\n>\n> On Mon, May 23, 2022 at 11:24:35AM +0200, Florian Sylvestre via\n> libcamera-devel wrote:\n> > Get default values for Black Level Correction algorithm form Yaml\n> > configuration file.\n>\n> s/form Yaml configuration/from YAML tuning/\n>\n> > Signed-off-by: Florian Sylvestre \n> > ---\n> > src/ipa/rkisp1/algorithms/blc.cpp | 46 ++++++++++++++++++++++++++++---\n> > src/ipa/rkisp1/algorithms/blc.h | 9 ++++++\n> > 2 files changed, 51 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp\n> b/src/ipa/rkisp1/algorithms/blc.cpp\n> > index 0c5948ff..177bc22e 100644\n> > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > @@ -7,6 +7,10 @@\n> >\n> > #include \"blc.h\"\n> >\n> > +#include \n> > +\n> > +#include \n>\n> For internal headers we use \"\" instead of <>.\n>\n> > +\n> > /**\n> > * \\file blc.h\n> > */\n> > @@ -29,6 +33,40 @@ namespace ipa::rkisp1::algorithms {\n> > * isn't currently supported.\n> > */\n> >\n> > +LOG_DEFINE_CATEGORY(RkISP1Blc)\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::init\n> > + */\n> > +int BlackLevelCorrection::init(IPAContext &context,\n> > + const YamlObject *params)\n> > +{\n> > + if (context.frameContext.frameCount > 0)\n> > + return -EBUSY;\n> > +\n> > + /* Parse property \"BlackLevelCorrection\" */\n> > + if (!params->contains(\"BlackLevelCorrection\"))\n> > + return -EINVAL;\n> > +\n> > + const YamlObject &blcObject = (*params)[\"BlackLevelCorrection\"];\n> > +\n> > + if (!blcObject.isDictionary())\n> > + return -EINVAL;\n> > +\n> > + blackLevelRed_ = blcObject[\"R\"].get(256);\n>\n> Can the value be negative ? If not, let go to uint32_t. Actually, you'll\n> have uint16_t support when rebasing on top of the YAML parser\n> improvements, so you can use that.\n>\n> > + blackLevelGreenR_ = blcObject[\"Gr\"].get(256);\n> > + blackLevelGreenB_ = blcObject[\"Gb\"].get(256);\n> > + blackLevelBlue_ = blcObject[\"B\"].get(256);\n> > +\n> > + LOG(RkISP1Blc, Debug)\n> > + << \" Read black levels red \" << blackLevelRed_\n> > + << \" green (red) \" << blackLevelGreenR_\n> > + << \" green (blue) \" << blackLevelGreenB_\n> > + << \" blue \" << blackLevelBlue_;\n>\n> << \" Read black levels red \" << blackLevelRed_\n> << \", green (red) \" << blackLevelGreenR_\n> << \", green (blue) \" << blackLevelGreenB_\n> << \", blue \" << blackLevelBlue_;\n>\n> > +\n> > + return 0;\n> > +}\n> > +\n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > */\n> > @@ -42,10 +80,10 @@ void BlackLevelCorrection::prepare(IPAContext\n> &context,\n> > * \\todo Use a configuration file for it ?\n> > */\n> > params->others.bls_config.enable_auto = 0;\n> > - params->others.bls_config.fixed_val.r = 256;\n> > - params->others.bls_config.fixed_val.gr = 256;\n> > - params->others.bls_config.fixed_val.gb = 256;\n> > - params->others.bls_config.fixed_val.b = 256;\n> > + params->others.bls_config.fixed_val.r = blackLevelRed_;\n> > + params->others.bls_config.fixed_val.gr = blackLevelGreenR_;\n> > + params->others.bls_config.fixed_val.gb = blackLevelGreenB_;\n> > + params->others.bls_config.fixed_val.b = blackLevelBlue_;\n> >\n> > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS;\n> > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;\n> > diff --git a/src/ipa/rkisp1/algorithms/blc.h\n> b/src/ipa/rkisp1/algorithms/blc.h\n> > index 69874d8f..2f6e2d82 100644\n> > --- a/src/ipa/rkisp1/algorithms/blc.h\n> > +++ b/src/ipa/rkisp1/algorithms/blc.h\n> > @@ -13,6 +13,8 @@\n> >\n> > namespace libcamera {\n> >\n> > +class YamlObject;\n>\n> You can drop this as it's alread forward-declared by algorithm.h;\n>\n> > +\n> > struct IPACameraSensorInfo;\n> >\n> > namespace ipa::rkisp1::algorithms {\n> > @@ -23,7 +25,14 @@ public:\n> > BlackLevelCorrection() = default;\n> > ~BlackLevelCorrection() = default;\n> >\n> > + int init(IPAContext &context, const YamlObject *params) override;\n> > void prepare(IPAContext &context, rkisp1_params_cfg *params)\n> override;\n> > +\n> > +private:\n> > + int16_t blackLevelRed_;\n> > + int16_t blackLevelGreenR_;\n> > + int16_t blackLevelGreenB_;\n> > + int16_t blackLevelBlue_;\n>\n> If the values can't be negative let's use uint16_t here.\n>\n> > };\n> >\n> > } /* namespace ipa::rkisp1::algorithms */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 512F2BD160\n\tfor ;\n\tWed, 1 Jun 2022 08:20:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A605A65632;\n\tWed, 1 Jun 2022 10:19:59 +0200 (CEST)","from mail-pg1-x52a.google.com (mail-pg1-x52a.google.com\n\t[IPv6:2607:f8b0:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C22360414\n\tfor ;\n\tWed, 1 Jun 2022 10:19:58 +0200 (CEST)","by mail-pg1-x52a.google.com with SMTP id g184so1247240pgc.1\n\tfor ;\n\tWed, 01 Jun 2022 01:19:58 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654071599;\n\tbh=6ajkrIuAoP19SSHR6JV/VQUf1+QDtoJffBsezwFgUIw=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=xLJOepb+shkSsCnV/NtdLI1vojJY8cIni82p8oC3Z7S5n92Ewx5PCmZmIQdhR24SM\n\tJZZf2JXHqeaZn/01hAd3QWkGICPWj2NkwS/EsyOcR2TG3C8P6ybQiBRtEYpH3QGW2W\n\tEw9zmK3YF1z13qnt8HUmPP6SHnDgubRst1hAarGa+3fBcL7/4L/jLYDiWFy8V/itNM\n\tuC0qX+T+BAqb+0Ugid2FEy6qjkXzCmAaFmuDT+TqEkgocehSpUMvpWHlrGO/oVLV7E\n\tWZ+rWgCAy8S6ZKueDdOWEC13+Vm04U3iCRa995hhrNXqoSNJIbellCN1WrsXKBb/4H\n\tak6XSqos5br9g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=baylibre-com.20210112.gappssmtp.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=CZeelQLzTw67vJUhVgXBnPkHYZTCu2ZN5JDOGtq2jb4=;\n\tb=CdgQJ9Z+ypPqc3+e3vzsv4Z5HNhQX0MLx//NoUlxQT0K+4ZTE8mhL2ExAE2PB1xZ0R\n\ty7wNACRO0TcwpEF6cPiMfN0KTdbPQ7azWq1HHzxruuTTKLrwbjU8PXuSrJl3h08obksO\n\tn8JeSMfD45AVj4XmaiP25rztzwrWQe+7jicaggLomt86xNhbaCnQmjvfAsEQsAa062Nb\n\tzDFtSVefq52C2S1JvuqLT75w+2GmYIQwX6XdN6VLaJLuOOWWq4Cadp2E3WG6yOmpaiV8\n\thP4Gvie0lXEo7hsgQRm++V6Bwq0FA1W1mWiWDdYswPzOvDMr2OUFI5yyyWc2Ahgg1l3S\n\tdjhA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=baylibre-com.20210112.gappssmtp.com\n\theader.i=@baylibre-com.20210112.gappssmtp.com header.b=\"CdgQJ9Z+\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=CZeelQLzTw67vJUhVgXBnPkHYZTCu2ZN5JDOGtq2jb4=;\n\tb=gOQZQjrbw62ioZAzQQqsTOSmB9nbD/6nSDAz1KOSRA4SqKB6GzMYJ1+WDiGnfZo2oi\n\tp11iDk4vKCmUtm5wOxXiQSlpB/7J5UFWahMkxlq5NaNVkSaQvmaB4NZtYbwHqtxOSc1F\n\t4BPSiV3eQHSlhKbd7AC7pI1rBgbU6SyYPw2WBNWme2gxJQELZFlWqB03O7urWYDfF5po\n\tXHyjXKth/X/740MGtmbeL0KeH6mXV2BVsc/g9GsF82uFcJZDOQ+2wtALXciVPAVawVoy\n\tUcrYyMOLIFws1WDiITvaZmxM+JWV+8ARpqwEGPXZeOuPXvacq3EJTrY3x3elkdWCQhlY\n\tQc5g==","X-Gm-Message-State":"AOAM5306NL+7R2Eogmd083PY1wbL7yma/OUgo6nvcxfsYswpKX310fxl\n\tkrXvziwa3YihFbHC1TighRszJki0Em4c9DN6CPcsATSD+SG+fQ==","X-Google-Smtp-Source":"ABdhPJzRt0Uz9wpIXYi7GAZrmFZ1fWD17LJZj3k5iC+Lk9HZyEpFYnty3yODCAyJ9pGYl4+I4h50O+PCLCJRq1qAvO4=","X-Received":"by 2002:a63:951b:0:b0:3fc:98c2:4301 with SMTP id\n\tp27-20020a63951b000000b003fc98c24301mr2555332pgd.515.1654071596919;\n\tWed, 01 Jun 2022 01:19:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20220523092435.475510-1-fsylvestre@baylibre.com>\n\t<20220523092435.475510-6-fsylvestre@baylibre.com>\n\t","In-Reply-To":"","Date":"Wed, 1 Jun 2022 10:19:44 +0200","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000f7c57205e05e8de9\"","Subject":"Re: [libcamera-devel] [libcamera-devel 5/5] ipa: rkisp1: add\n\tsupport of Black Level Correction default values","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","From":"Florian Sylvestre via libcamera-devel\n\t","Reply-To":"Florian Sylvestre ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23360,"web_url":"https://patchwork.libcamera.org/comment/23360/","msgid":"","date":"2022-06-08T22:00:39","subject":"Re: [libcamera-devel] [libcamera-devel 5/5] ipa: rkisp1: add\n\tsupport of Black Level Correction default values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Florian,\n\nOn Wed, Jun 01, 2022 at 10:19:44AM +0200, Florian Sylvestre wrote:\n> >> + const YamlObject &blcObject = (*params)[\"BlackLevelCorrection\"];\n> >> +\n> >> + if (!blcObject.isDictionary())\n> >> + return -EINVAL;\n> >> +\n> >> + blackLevelRed_ = blcObject[\"R\"].get(256);\n> > \n> > Can the value be negative ? If not, let go to uint32_t. Actually, you'll\n> > have uint16_t support when rebasing on top of the YAML parser\n> > improvements, so you can use that.\n> \n> The values can be negatives.\n> FYI: Extracted from .h comments:\n> * struct rkisp1_cif_isp_bls_fixed_val - BLS fixed subtraction values\n> *\n> * The values will be subtracted from the sensor\n> * values. Therefore a negative value means addition instead of subtraction!\n\nI don't expect negative values to be very useful, but that's fine.\n\n> I have seen the [RFC] on modifications for Yaml Parser to support int16,\n> but since it's 'only' at [RFC] state, should we not provide this version and\n> update it when the support of int16 will be available?\n\nYou can rebase on top of the RFC version (v2 available from\nhttps://gitlab.com/ideasonboard/nxp/libcamera/-/commits/pinchartl/yaml/)\nas it should get merged very soon.\n\n> Le jeu. 26 mai 2022 à 13:04, Laurent Pinchart a écrit :\n> > On Mon, May 23, 2022 at 11:24:35AM +0200, Florian Sylvestre wrote:\n> > > Get default values for Black Level Correction algorithm form Yaml\n> > > configuration file.\n> >\n> > s/form Yaml configuration/from YAML tuning/\n> >\n> > > Signed-off-by: Florian Sylvestre \n> > > ---\n> > > src/ipa/rkisp1/algorithms/blc.cpp | 46 ++++++++++++++++++++++++++++---\n> > > src/ipa/rkisp1/algorithms/blc.h | 9 ++++++\n> > > 2 files changed, 51 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > index 0c5948ff..177bc22e 100644\n> > > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > @@ -7,6 +7,10 @@\n> > >\n> > > #include \"blc.h\"\n> > >\n> > > +#include \n> > > +\n> > > +#include \n> >\n> > For internal headers we use \"\" instead of <>.\n> >\n> > > +\n> > > /**\n> > > * \\file blc.h\n> > > */\n> > > @@ -29,6 +33,40 @@ namespace ipa::rkisp1::algorithms {\n> > > * isn't currently supported.\n> > > */\n> > >\n> > > +LOG_DEFINE_CATEGORY(RkISP1Blc)\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > + */\n> > > +int BlackLevelCorrection::init(IPAContext &context,\n> > > + const YamlObject *params)\n> > > +{\n> > > + if (context.frameContext.frameCount > 0)\n> > > + return -EBUSY;\n> > > +\n> > > + /* Parse property \"BlackLevelCorrection\" */\n> > > + if (!params->contains(\"BlackLevelCorrection\"))\n> > > + return -EINVAL;\n> > > +\n> > > + const YamlObject &blcObject = (*params)[\"BlackLevelCorrection\"];\n> > > +\n> > > + if (!blcObject.isDictionary())\n> > > + return -EINVAL;\n> > > +\n> > > + blackLevelRed_ = blcObject[\"R\"].get(256);\n> >\n> > Can the value be negative ? If not, let go to uint32_t. Actually, you'll\n> > have uint16_t support when rebasing on top of the YAML parser\n> > improvements, so you can use that.\n> >\n> > > + blackLevelGreenR_ = blcObject[\"Gr\"].get(256);\n> > > + blackLevelGreenB_ = blcObject[\"Gb\"].get(256);\n> > > + blackLevelBlue_ = blcObject[\"B\"].get(256);\n> > > +\n> > > + LOG(RkISP1Blc, Debug)\n> > > + << \" Read black levels red \" << blackLevelRed_\n> > > + << \" green (red) \" << blackLevelGreenR_\n> > > + << \" green (blue) \" << blackLevelGreenB_\n> > > + << \" blue \" << blackLevelBlue_;\n> >\n> > << \" Read black levels red \" << blackLevelRed_\n> > << \", green (red) \" << blackLevelGreenR_\n> > << \", green (blue) \" << blackLevelGreenB_\n> > << \", blue \" << blackLevelBlue_;\n> >\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > /**\n> > > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > */\n> > > @@ -42,10 +80,10 @@ void BlackLevelCorrection::prepare(IPAContext &context,\n> > > * \\todo Use a configuration file for it ?\n> > > */\n> > > params->others.bls_config.enable_auto = 0;\n> > > - params->others.bls_config.fixed_val.r = 256;\n> > > - params->others.bls_config.fixed_val.gr = 256;\n> > > - params->others.bls_config.fixed_val.gb = 256;\n> > > - params->others.bls_config.fixed_val.b = 256;\n> > > + params->others.bls_config.fixed_val.r = blackLevelRed_;\n> > > + params->others.bls_config.fixed_val.gr = blackLevelGreenR_;\n> > > + params->others.bls_config.fixed_val.gb = blackLevelGreenB_;\n> > > + params->others.bls_config.fixed_val.b = blackLevelBlue_;\n> > >\n> > > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS;\n> > > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;\n> > > diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h\n> > > index 69874d8f..2f6e2d82 100644\n> > > --- a/src/ipa/rkisp1/algorithms/blc.h\n> > > +++ b/src/ipa/rkisp1/algorithms/blc.h\n> > > @@ -13,6 +13,8 @@\n> > >\n> > > namespace libcamera {\n> > >\n> > > +class YamlObject;\n> >\n> > You can drop this as it's alread forward-declared by algorithm.h;\n> >\n> > > +\n> > > struct IPACameraSensorInfo;\n> > >\n> > > namespace ipa::rkisp1::algorithms {\n> > > @@ -23,7 +25,14 @@ public:\n> > > BlackLevelCorrection() = default;\n> > > ~BlackLevelCorrection() = default;\n> > >\n> > > + int init(IPAContext &context, const YamlObject *params) override;\n> > > void prepare(IPAContext &context, rkisp1_params_cfg *params) override;\n> > > +\n> > > +private:\n> > > + int16_t blackLevelRed_;\n> > > + int16_t blackLevelGreenR_;\n> > > + int16_t blackLevelGreenB_;\n> > > + int16_t blackLevelBlue_;\n> >\n> > If the values can't be negative let's use uint16_t here.\n> >\n> > > };\n> > >\n> > > } /* namespace ipa::rkisp1::algorithms */","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 B92B2BD160\n\tfor ;\n\tWed, 8 Jun 2022 22:00:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F115465634;\n\tThu, 9 Jun 2022 00:00:46 +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 481BE633A5\n\tfor ;\n\tThu, 9 Jun 2022 00:00:45 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C30E6CF;\n\tThu, 9 Jun 2022 00:00:44 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654725647;\n\tbh=A67gLIpTmNj3UfiQK4QJWCClQ5sWDcu12r5somANRzU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zs7/El6NUg1nNGXExjQH4/imTOdGUzP7mOHHKJgLA4vV3rwudp8tjen47+WToTdBY\n\tjnVYnhlcMY37g5hVg3T5ztakCeotXBUmUBEyQfgmZHQcumH2QpTvURJlr5R9xW+Z37\n\tpQSvoUVsxfRO6AGWp/HaPC938q9MHBHDBY6sWQZSWeZxMAN48M8Obhap84hwKtGP6N\n\tbz6OIN6AoBf4GCnPVqnycBxAi9xJqrySDClYQD4SmScBKZ8T1aklfbe3oZuK5nr/u3\n\tEIjbKjNMwvPFV1LR70Bl5WYgVonFackBciAP9cR6YFW/djtpiLbylA0SBYCqj4LV9u\n\tqrwMC7DpghQNg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654725644;\n\tbh=A67gLIpTmNj3UfiQK4QJWCClQ5sWDcu12r5somANRzU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jXiCiJWAlwVeXuWs64lrLlu6CmzGdTTxfLTB9Z7soCiziJOnf34DvOiMpIjDMo0iO\n\toY8ffc/w14RVQVFdYEEmOy8faf+N+6t6PeCjDK/1/5F97Np2/1uUUjGN/zCVaxYIJN\n\t2cb/udr0LHACNPIllkVBWSVJpLRodJoUFzUpzeR0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jXiCiJWA\"; dkim-atps=neutral","Date":"Thu, 9 Jun 2022 01:00:39 +0300","To":"Florian Sylvestre ","Message-ID":"","References":"<20220523092435.475510-1-fsylvestre@baylibre.com>\n\t<20220523092435.475510-6-fsylvestre@baylibre.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [libcamera-devel 5/5] ipa: rkisp1: add\n\tsupport of Black Level Correction default values","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]