[{"id":23852,"web_url":"https://patchwork.libcamera.org/comment/23852/","msgid":"","date":"2022-07-13T01:26:51","subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: rkisp1: Add support of\n\tColorProcessing control","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, Jul 04, 2022 at 05:23:18PM +0200, Florian Sylvestre via libcamera-devel wrote:\n> Add ColorProcessing algorithm that is in charge to manage brightness, contrast\n> and saturation controls.\n> These controls are currently based on user controls.\n> \n> Signed-off-by: Florian Sylvestre \n> ---\n> .../rkisp1/algorithms/color_processing.cpp | 101 ++++++++++++++++++\n> src/ipa/rkisp1/algorithms/color_processing.h | 38 +++++++\n> src/ipa/rkisp1/algorithms/meson.build | 1 +\n> src/ipa/rkisp1/data/ov5640.yaml | 1 +\n> src/ipa/rkisp1/rkisp1.cpp | 1 +\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++\n> 6 files changed, 154 insertions(+)\n> create mode 100644 src/ipa/rkisp1/algorithms/color_processing.cpp\n> create mode 100644 src/ipa/rkisp1/algorithms/color_processing.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/color_processing.cpp b/src/ipa/rkisp1/algorithms/color_processing.cpp\n> new file mode 100644\n> index 00000000..a7f2a470\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/color_processing.cpp\n> @@ -0,0 +1,101 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * lsc.cpp - RkISP1 Defect Pixel Cluster Correction control\n\nlsc.cpp ? :-)\n\n> + */\n> +\n> +#include \"color_processing.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +#include \"linux/rkisp1-config.h\"\n\nI think you can drop this header.\n\n> +\n> +/**\n> + * \\file color-processing.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class ColorProcessing\n> + * \\brief RkISP1 Color Processing control\n> + *\n> + * The ColorProcessing algorithm is responsible to apply brightness, contrast\n> + * and saturation corrections.\n> + * The coefficient applied for brightness, contrast and saturation are directly\n> + * provided by user controls.\n\n * The ColorProcessing algorithm is responsible for applying brightness, contrast\n * and saturation corrections. The values are directly provided through requests\n * by the corresponding controls.\n\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1ColorProcessing)\n> +\n> +ColorProcessing::ColorProcessing()\n> +\t: userBrightness_(0), userContrast_(0),\n> +\t userSaturation_(0), updateUserControls_(true)\n> +{\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void ColorProcessing::queueRequest([[maybe_unused]] IPAContext &context,\n> +\t\t\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t\t\t const ControlList &controls)\n> +{\n> +\tfor (auto const &ctrl : controls) {\n> +\t\tif (ctrl.first == controls::BRIGHTNESS) {\n> +\t\t\tuserBrightness_ = ctrl.second.get();\n> +\t\t\tupdateUserControls_ = true;\n> +\n> +\t\t\tLOG(RkISP1ColorProcessing, Debug)\n> +\t\t\t\t<< \"Set brightness to: \"\n\ns/to: /to /\n\n(same below)\n\n> +\t\t\t\t<< int(userBrightness_);\n> +\t\t} else if (ctrl.first == controls::CONTRAST) {\n> +\t\t\tuserContrast_ = ctrl.second.get();\n\nYou need to scale the contrast and saturation values, the controls are\nin the range [0.0, 1.992] and the register values are expressed as 8-bit\nintegers.\n\n> +\t\t\tupdateUserControls_ = true;\n> +\n> +\t\t\tLOG(RkISP1ColorProcessing, Debug)\n> +\t\t\t\t<< \"Set contrast to: \"\n> +\t\t\t\t<< int(userContrast_);\n> +\t\t} else if (ctrl.first == controls::SATURATION) {\n> +\t\t\tuserSaturation_ = ctrl.second.get();\n> +\t\t\tupdateUserControls_ = true;\n> +\n> +\t\t\tLOG(RkISP1ColorProcessing, Debug)\n> +\t\t\t\t<< \"Set saturation to: \"\n> +\t\t\t\t<< int(userSaturation_);\n> +\t\t}\n> +\t}\n\nNo need to loop over the control list. With the usage of std::optional\nfor ControlList that we're about to merge, this can be written\n\n\tconst auto &brightness = controls.get(controls::Brightness);\n\tif (brightness) {\n\t\tuserBrightness_ = *brightness;\n\t\tupdateUserControls_ = true;\n\n\t\tLOG(RkISP1ColorProcessing, Debug)\n\t\t\t<< \"Set brightness to: \"\n\t\t\t<< int(userBrightness_);\n\t}\n\nand similarly for the contract and saturation controls.\n\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,\n> +\t\t\t rkisp1_params_cfg *params)\n> +{\n> +\t/* Check if the algorithm configuration has been updated. */\n> +\tif (!updateUserControls_)\n> +\t\treturn;\n> +\n> +\tupdateUserControls_ = false;\n> +\n> +\tparams->others.cproc_config.brightness = userBrightness_;\n> +\tparams->others.cproc_config.contrast = userContrast_;\n> +\tparams->others.cproc_config.sat = userSaturation_;\n> +\n> +\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_CPROC;\n> +\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_CPROC;\n> +\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CPROC;\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(ColorProcessing, \"ColorProcessing\")\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/color_processing.h b/src/ipa/rkisp1/algorithms/color_processing.h\n> new file mode 100644\n> index 00000000..bdb40bd1\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/color_processing.h\n> @@ -0,0 +1,38 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * dpcc.h - RkISP1 Color Processing control\n\ndpcc.h ? :-)\n\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n\nYou can drop this.\n\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +struct IPACameraSensorInfo;\n\nYou can drop this.\n\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class ColorProcessing : public Algorithm\n> +{\n> +public:\n> +\tColorProcessing();\n> +\t~ColorProcessing() = default;\n> +\n> +\tvoid queueRequest(IPAContext &context, const uint32_t frame,\n> +\t\t\t const ControlList &controls) override;\n> +\tvoid prepare(IPAContext &context, rkisp1_params_cfg *params) override;\n\nMissing blank line.\n\n> +private:\n> +\tuint32_t userBrightness_;\n> +\tuint32_t userContrast_;\n> +\tuint32_t userSaturation_;\n> +\tbool updateUserControls_;\n> +};\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> index 7e078b0d..7f9c3899 100644\n> --- a/src/ipa/rkisp1/algorithms/meson.build\n> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> 'agc.cpp',\n> 'awb.cpp',\n> 'blc.cpp',\n> + 'color_processing.cpp',\n\nWe could call this cproc.cpp to match the name used by the rkisp1.\n\n> 'demosaicing.cpp',\n> 'dpcc.cpp',\n> 'gsl.cpp',\n> diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> index 4ae0ffc0..331f9d2d 100644\n> --- a/src/ipa/rkisp1/data/ov5640.yaml\n> +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> @@ -10,6 +10,7 @@ algorithms:\n> Gr: 256\n> Gb: 256\n> B: 256\n> + - ColorProcessing:\n> - GammaSensorLinearization:\n> x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]\n> y:\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 85a75070..d380c59b 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -31,6 +31,7 @@\n> #include \"algorithms/algorithm.h\"\n> #include \"algorithms/awb.h\"\n> #include \"algorithms/blc.h\"\n> +#include \"algorithms/color_processing.h\"\n> #include \"algorithms/demosaicing.h\"\n> #include \"algorithms/dpcc.h\"\n> #include \"algorithms/gsl.h\"\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 3d0075ee..4e82ab39 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -957,6 +957,18 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \t\t std::forward_as_tuple(&controls::Sharpness),\n> \t\t std::forward_as_tuple(0.0f, 10.0f, 1.0f));\n> \n> +\tctrls.emplace(std::piecewise_construct,\n> +\t\t std::forward_as_tuple(&controls::Brightness),\n> +\t\t std::forward_as_tuple(-128.0f, 127.0f));\n\nLet's make this [-1.0, 1.0] and scale the value in the IPA.\n\n> +\n> +\tctrls.emplace(std::piecewise_construct,\n> +\t\t std::forward_as_tuple(&controls::Contrast),\n> +\t\t std::forward_as_tuple(0.0f, 1.992f));\n> +\n> +\tctrls.emplace(std::piecewise_construct,\n> +\t\t std::forward_as_tuple(&controls::Saturation),\n> +\t\t std::forward_as_tuple(0.0f, 1.992f));\n> +\n> \tctrls.emplace(std::piecewise_construct,\n> \t\t std::forward_as_tuple(&controls::draft::NoiseReductionMode),\n> \t\t std::forward_as_tuple(controls::draft::NoiseReductionModeValues));","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 D3740BD1F1\n\tfor ;\n\tWed, 13 Jul 2022 01:27:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 226C96330F;\n\tWed, 13 Jul 2022 03:27:22 +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 A37196048B\n\tfor ;\n\tWed, 13 Jul 2022 03:27:20 +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 144A6305;\n\tWed, 13 Jul 2022 03:27:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657675642;\n\tbh=/AOTRFwuxhhF1H85y7x4HkKhxJmoXGiSePPBvmDtvgc=;\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=EjPDKPG3AAI8nCDTJPwuFAYDSBdvqUrK48M7GzzXjew5IZGBlgNPNnBAlKs39QsyD\n\tG2UyI2xicaKW9Wkct03BNdQv/56Dp+ZmyjKyxOXbDxDZVWIwRvNizko88uU1UG5Syp\n\tTs6x9UtCoYBnbsEbLt9p0ccOtPYi3ZeYZ6ZK92ninRJS3Ps4Xix6Cx0Zs+9LejAmWt\n\t6EGTiedEnx3GtZTSq3vOI4fx8Axm9/HbXc4CCoKtQ1xRU8lAsUh2K4+qz3np+mUw+B\n\t811PQFQk1nvBFBsjBZ8OYTaoBUgXTsLO/0XiKxZtUs/QI4l5nIBYgTipu3CS6TPFcn\n\tcrmXD1QGyN4Kg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657675640;\n\tbh=/AOTRFwuxhhF1H85y7x4HkKhxJmoXGiSePPBvmDtvgc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NxGPN5UtltCC0rMtEKjrWbtlcb+LdplHT5Vu4BsHcLmCelpprsQvsiM91JI9WHxFa\n\t1yhkpNN5VfGqIsfqjVtds7G+hivTQMRAfcZNugJLFkrRcpP6gqaiOAWFzgB9NF29y4\n\tgfNcmeJNzCV8KHieThLKPUpMVd8nnCUlOEoLsPV0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NxGPN5Ut\"; dkim-atps=neutral","Date":"Wed, 13 Jul 2022 04:26:51 +0300","To":"Florian Sylvestre ","Message-ID":"","References":"<20220704152318.221213-1-fsylvestre@baylibre.com>\n\t<20220704152318.221213-5-fsylvestre@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220704152318.221213-5-fsylvestre@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: rkisp1: Add support of\n\tColorProcessing control","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\" "}}]