[{"id":24176,"web_url":"https://patchwork.libcamera.org/comment/24176/","msgid":"","date":"2022-07-27T04:06:47","subject":"Re: [libcamera-devel] [PATCH v3 2/2] 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 Tue, Jul 26, 2022 at 04:32:11PM +0200, Florian Sylvestre 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> Reviewed-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/algorithms/cproc.cpp | 97 ++++++++++++++++++++++++\n> src/ipa/rkisp1/algorithms/cproc.h | 30 ++++++++\n> src/ipa/rkisp1/algorithms/meson.build | 1 +\n> src/ipa/rkisp1/data/ov5640.yaml | 1 +\n> src/ipa/rkisp1/ipa_context.cpp | 17 +++++\n> src/ipa/rkisp1/ipa_context.h | 7 ++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++\n> 7 files changed, 165 insertions(+)\n> create mode 100644 src/ipa/rkisp1/algorithms/cproc.cpp\n> create mode 100644 src/ipa/rkisp1/algorithms/cproc.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> new file mode 100644\n> index 00000000..fa989497\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> @@ -0,0 +1,97 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * cproc.cpp - RkISP1 Color Processing control\n> + */\n> +\n> +#include \"cproc.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file cproc.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 for applying brightness,\n> + * contrast and saturation corrections. The values are directly provided\n> + * through requests by the corresponding controls.\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1CProc)\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void ColorProcessing::queueRequest([[maybe_unused]] IPAContext &context,\n\ncontext is used.\n\n> +\t\t\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t\t\t const ControlList &controls)\n> +{\n> +\tauto &cproc = context.frameContext.cproc;\n> +\n> +\tconst auto &brightness = controls.get(controls::Brightness);\n> +\tif (brightness) {\n> +\t\tcproc.brightness = std::round(std::clamp(*brightness * 128.0f, -128.0f, 127.0f));\n\nWould it be a problem to round before clamping, in order to clamp on\nintegers, which would be slightly more efficient ? I think you could use\n\n\t\tcproc.brightness = std::clamp(std::lround(*brightness * 128.0f), -128, 127);\n\nSame for the contrast and saturation.\n\n> +\t\tcproc.updateParams = true;\n> +\n> +\t\tLOG(RkISP1CProc, Debug) << \"Set brightness to \" << *brightness;\n> +\t}\n> +\n> +\tconst auto &contrast = controls.get(controls::Contrast);\n> +\tif (contrast) {\n> +\t\tcproc.contrast = std::round(std::clamp(*contrast * 128.0f, 0.0f, 255.0f));\n> +\t\tcproc.updateParams = true;\n> +\n> +\t\tLOG(RkISP1CProc, Debug) << \"Set contrast to \" << *contrast;\n> +\t}\n> +\n> +\tconst auto saturation = controls.get(controls::Saturation);\n> +\tif (saturation) {\n> +\t\tcproc.saturation = std::round(std::clamp(*saturation * 128.0f, 0.0f, 255.0f));\n> +\t\tcproc.updateParams = true;\n> +\n> +\t\tLOG(RkISP1CProc, Debug) << \"Set saturation to \" << *saturation;\n> +\t}\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,\n\ncontext is used.\n\n> +\t\t\t rkisp1_params_cfg *params)\n> +{\n> +\tauto &cproc = context.frameContext.cproc;\n> +\n> +\t/* Check if the algorithm configuration has been updated. */\n> +\tif (!cproc.updateParams)\n> +\t\treturn;\n> +\n> +\tcproc.updateParams = false;\n> +\n> +\tparams->others.cproc_config.brightness = cproc.brightness;\n> +\tparams->others.cproc_config.contrast = cproc.contrast;\n> +\tparams->others.cproc_config.sat = cproc.saturation;\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/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h\n> new file mode 100644\n> index 00000000..4b7e4064\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/cproc.h\n> @@ -0,0 +1,30 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * cproc.h - RkISP1 Color Processing control\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class ColorProcessing : public Algorithm\n> +{\n> +public:\n> +\tColorProcessing() = default;\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> +};\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 dcd24fe0..e48974b4 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> + 'cproc.cpp',\n> 'dpcc.cpp',\n> 'filter.cpp',\n> 'gsl.cpp',\n> diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> index 99529481..93d7d1e7 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/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 4b117186..ef8bb8e9 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -136,6 +136,23 @@ namespace libcamera::ipa::rkisp1 {\n> * \\brief Estimated color temperature\n> */\n> \n> +/**\n> + * \\var IPAFrameContext::cproc\n> + * \\brief Context for the Color Processing algorithm\n> + *\n> + * \\struct IPAFrameContext::cproc.brightness\n> + * \\brief Brightness level\n> + *\n> + * \\var IPAFrameContext::cproc.contrast\n> + * \\brief Contrast level\n> + *\n> + * \\var IPAFrameContext::cproc.saturation\n> + * \\brief Saturation level\n> + *\n> + * \\var IPAFrameContext::cproc.updateParams\n> + * \\brief Indicates if ISP parameters need to be updated\n> + */\n> +\n> /**\n> * \\var IPAFrameContext::filter\n> * \\brief Context for the Filter algorithm\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 3b2f6af1..2bdb6a81 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -57,6 +57,13 @@ struct IPAFrameContext {\n> \t\tdouble temperatureK;\n> \t} awb;\n> \n> +\tstruct {\n> +\t\tint8_t brightness;\n> +\t\tuint8_t contrast;\n> +\t\tuint8_t saturation;\n> +\t\tbool updateParams;\n> +\t} cproc;\n> +\n> \tstruct {\n> \t\tuint8_t denoise;\n> \t\tuint8_t sharpness;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 4e000d31..de687f4d 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -972,6 +972,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(-1.0f, 0.993f));\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.993f));\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.993f));\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 0F470C3275\n\tfor ;\n\tWed, 27 Jul 2022 04:06:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A98763312;\n\tWed, 27 Jul 2022 06:06:50 +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 B5A6B603E8\n\tfor ;\n\tWed, 27 Jul 2022 06:06:48 +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 2814456D;\n\tWed, 27 Jul 2022 06:06:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658894810;\n\tbh=CAAItVu0e8CGUl6MAu1TPMKwKgsqsReZ0kW17+oFjxc=;\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=QJnm4UZKwjvJkmz2Hzl/VdRi2x6DRcIpHV9gBL5cJjdYyUJ9Qdij7kq0PQN+wTLHJ\n\tcUoszRAWZBYIxgghByXtzmmd6tsYYH6yvwMlyzFxYqyw39rVzBwZv5z1uvJWh4uNoO\n\t+SxDL3QkdFFCLp+ebzBJpZeQHEPXMMeGUgXfzZaIHFw+i/oWntUnJoH5JH8w+mUR+l\n\tocoCJmav60JxftXa7Pm63RzNPD4WQUbCGDEsErmlTQus91YGXvUdGNBdTIhQkE/n67\n\tOluT+SbgsAARtunuAeItYgvrZOxLoFEPoxR6yXOTgpqKOXMNozIRrhk7k4Z1A1m7AN\n\t6B9NRNZ3gtVFQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658894808;\n\tbh=CAAItVu0e8CGUl6MAu1TPMKwKgsqsReZ0kW17+oFjxc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jeO4mG+kEMXPbL254x7v7jJ1FKCCLf43nr7m5j2bMIIZUa1Nz6Yyv+Cdj2SeABUCi\n\t0AH85Diew/+7wTVGPR6xbivMWhnFyfIPm4BS2XuZ3VqTGJqSPzHoOlnHir+NIW5oQk\n\tZ4yeA73FGtGjBtDukiDCB8hqmGpdaqzg1MhoQBec="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jeO4mG+k\"; dkim-atps=neutral","Date":"Wed, 27 Jul 2022 07:06:47 +0300","To":"Florian Sylvestre ","Message-ID":"","References":"<20220726143211.517231-1-fsylvestre@baylibre.com>\n\t<20220726143211.517231-3-fsylvestre@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220726143211.517231-3-fsylvestre@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] 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\" "}}]