[{"id":29754,"web_url":"https://patchwork.libcamera.org/comment/29754/","msgid":"<171743024565.205609.5835595704744600028@ping.linuxembedded.co.uk>","date":"2024-06-03T15:57:25","subject":"Re: [PATCH v3 3/3] ipa: rkisp1: Add GammaOutCorrection algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-06-03 15:06:30)\n> Add a gamma algorithm for the rkisp1. It defaults to a gamma of 2.2 which\n> closely resembles sRGB. No seperate sRGB mode was implemented because the pwl\n> that models the gamma curve is so coarse that there is basically no difference\n> between srgb and gamma=2.2. The default can be overridden within the tuning\n> file or set at runtime using the gamma control.\n> \n> The gamma algorithm is not enabled by default. This will be done in future\n> tuning file updates.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> v2 -> v3:\n> - Squashed with patch 1/4 from previous series\n> - Renamed from Gamma to GammaOutCorrection\n> - Read default gamma value from tuning file\n> - Some stylistic fixes from review\n> \n> src/ipa/rkisp1/algorithms/goc.cpp | 161 ++++++++++++++++++++++++++\n> src/ipa/rkisp1/algorithms/goc.h | 48 ++++++++\n> src/ipa/rkisp1/algorithms/meson.build | 1 +\n> src/ipa/rkisp1/ipa_context.h | 4 +\n> 4 files changed, 214 insertions(+)\n> create mode 100644 src/ipa/rkisp1/algorithms/goc.cpp\n> create mode 100644 src/ipa/rkisp1/algorithms/goc.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp\n> new file mode 100644\n> index 00000000..875c82e5\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/goc.cpp\n> @@ -0,0 +1,161 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * RkISP1 Gamma out control\n> + */\n> +#include \"goc.h\"\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"linux/rkisp1-config.h\"\n> +\n> +/**\n> + * \\file goc.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class GammaOutCorrection\n> + * \\brief RkISP1 Gamma out correction\n> + *\n> + * This algorithm implements the gamma out curve for the RkISP1.\n> + * It defaults to a gamma value of 2.2\n> + * As gamma is internally represented as a piecewise linear function with only\n> + * 17 knots, the difference between gamma=2.2 and sRGB gamma is minimal.\n> + * Therefore sRGB gamma was not implemented as special case.\n> + *\n> + * Useful links:\n> + * https://www.cambridgeincolour.com/tutorials/gamma-correction.htm\n> + * https://en.wikipedia.org/wiki/SRGB\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Gamma)\n> +\n> +const double kDefaultGamma = 2.2;\n> +\n> +GammaOutCorrection::GammaOutCorrection()\n> + : gamma_(0)\n> +{\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::init\n> + */\n> +int GammaOutCorrection::init([[maybe_unused]] IPAContext &context,\n> + [[maybe_unused]] const YamlObject &tuningData)\n> +{\n> + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, 2.2f);\n> + defaultGamma_ = tuningData[\"gamma\"].get(kDefaultGamma);\n> +\n> + if (context.hw->numGammaOutSamples !=\n> + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {\n> + LOG(RkISP1Gamma, Error)\n> + << \"Gamma is not implemented for RkISP1 V12\";\n\nIs that because the size is different? Are there other differences that\nprevent us supporting it ?\n\n> + return -EINVAL;\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +/**\n> + * \\brief Configure the Gamma given a configInfo\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] configInfo The IPA configuration data\n> + *\n> + * \\return 0\n> + */\n> +int GammaOutCorrection::configure(IPAContext &context,\n> + [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> +{\n> + context.activeState.gamma = defaultGamma_;\n> + return 0;\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void GammaOutCorrection::queueRequest([[maybe_unused]] IPAContext &context,\n> + [[maybe_unused]] const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + const ControlList &controls)\n> +{\n> + const auto &gamma = controls.get(controls::Gamma);\n> + if (gamma) {\n> + /*\n> + * \\todo This is not correct as it updates the current state with a\n> + * future value. But it does no harm at the moment an allows us to\n> + * track the last active gamma\n> + */\n> + context.activeState.gamma = *gamma;\n> + LOG(RkISP1Gamma, Debug) << \"Set gamma to \" << *gamma;\n> + }\n> +\n> + frameContext.gamma = context.activeState.gamma;\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void GammaOutCorrection::prepare([[maybe_unused]] IPAContext &context,\n> + const uint32_t frame,\n> + [[maybe_unused]] IPAFrameContext &frameContext,\n> + rkisp1_params_cfg *params)\n> +{\n> + ASSERT(context.hw->numGammaOutSamples ==\n> + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);\n> +\n> + /*\n> + * The logarithmic segments as specified in the reference.\n> + * Plus an additional 0 to make the loop easier\n\nIs the '0' still required now that this uses utils::enumerate(segments)?\n\n> + */\n> + std::array segments = {\n> + 64, 64, 64, 64, 128, 128, 128, 128, 256,\n> + 256, 256, 512, 512, 512, 512, 512, 0\n> + };\n\nPresumably we'd have different segments for V12 ?\n\n> + auto gamma_y = params->others.goc_config.gamma_y;\n> +\n> + if (frame > 0 && std::fabs(frameContext.gamma - gamma_) < 0.001)\n> + return;\n> +\n> + gamma_ = frameContext.gamma;\n> +\n> + unsigned x = 0;\n> + for (auto [i, size] : utils::enumerate(segments)) {\n> + gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> + x += size;\n\nAssuming the '0' is actually valid, as I think the last iteration\nthrough this loop will still have added x by 512 anyway,\n\n\nReviewed-by: Kieran Bingham \n\n> + }\n> +\n> + params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> + params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> + params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::process\n> + */\n> +void GammaOutCorrection::process([[maybe_unused]] IPAContext &context,\n> + [[maybe_unused]] const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> + ControlList &metadata)\n> +{\n> + metadata.set(controls::Gamma, frameContext.gamma);\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(GammaOutCorrection, \"GammaOutCorrection\")\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h\n> new file mode 100644\n> index 00000000..d45e4194\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/goc.h\n> @@ -0,0 +1,48 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * RkISP1 Gamma out control\n> + */\n> +\n> +#pragma once\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class GammaOutCorrection : public Algorithm\n> +{\n> +public:\n> + GammaOutCorrection();\n> + ~GammaOutCorrection() = default;\n> +\n> + int init(IPAContext &context, const YamlObject &tuningData) override;\n> + int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> + void queueRequest(IPAContext &context,\n> + const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + const ControlList &controls) override;\n> + void prepare(IPAContext &context, const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + rkisp1_params_cfg *params) override;\n> + void process(IPAContext &context, const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + const rkisp1_stat_buffer *stats,\n> + ControlList &metadata) override;\n> +\n> +private:\n> + /*\n> + * \\todo: gamma_ holds the current state of the hardware. context.activeState\n> + * can not be used as that holds the \"future state\" after applying all known\n> + * requests. The intended usage of activeState needs to be clarified.\n> + */\n> + double gamma_;\n> +\n> + double defaultGamma_;\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 93a48329..6ee71a9b 100644\n> --- a/src/ipa/rkisp1/algorithms/meson.build\n> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> @@ -8,6 +8,7 @@ rkisp1_ipa_algorithms = files([\n> 'dpcc.cpp',\n> 'dpf.cpp',\n> 'filter.cpp',\n> + 'goc.cpp',\n> 'gsl.cpp',\n> 'lsc.cpp',\n> ])\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index bd02a7a2..5252e222 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -104,6 +104,8 @@ struct IPAActiveState {\n> uint8_t denoise;\n> uint8_t sharpness;\n> } filter;\n> +\n> + double gamma;\n> };\n> \n> struct IPAFrameContext : public FrameContext {\n> @@ -146,6 +148,8 @@ struct IPAFrameContext : public FrameContext {\n> uint32_t exposure;\n> double gain;\n> } sensor;\n> +\n> + double gamma;\n> };\n> \n> struct IPAContext {\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 89621BD87C\n\tfor ;\n\tMon, 3 Jun 2024 15:57:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 73857634CA;\n\tMon, 3 Jun 2024 17:57:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E3F6761A3B\n\tfor ;\n\tMon, 3 Jun 2024 17:57:27 +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 9B51439F;\n\tMon, 3 Jun 2024 17:57:20 +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=\"VAYJ9aCH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717430240;\n\tbh=gHpbJx0ieZvbdAmrehOvigajT7RwjCM5VE4gSMnJHG8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VAYJ9aCH25h1BvME9JVcmz7UHgDG7YGWjvE/ORCvnYWQoaDfvVtUCkJpYBPtACRZe\n\t7N1oJNkLdkzVGZq9GUuC3oiBN4toQbpegbupfBISAX2oZkDL4UguVyh+d2WRaMfOnv\n\ti2wRD7CWlvri5PsGQpvTYDqLjnqCk1L6CKiXgqjI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240603140806.90045-4-stefan.klug@ideasonboard.com>","References":"<20240603140806.90045-1-stefan.klug@ideasonboard.com>\n\t<20240603140806.90045-4-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v3 3/3] ipa: rkisp1: Add GammaOutCorrection algorithm","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 03 Jun 2024 16:57:25 +0100","Message-ID":"<171743024565.205609.5835595704744600028@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":29758,"web_url":"https://patchwork.libcamera.org/comment/29758/","msgid":"","date":"2024-06-03T17:21:09","subject":"Re: [PATCH v3 3/3] ipa: rkisp1: Add GammaOutCorrection algorithm","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nthanks for the review.\n\nOn Mon, Jun 03, 2024 at 04:57:25PM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-06-03 15:06:30)\n> > Add a gamma algorithm for the rkisp1. It defaults to a gamma of 2.2 which\n> > closely resembles sRGB. No seperate sRGB mode was implemented because the pwl\n> > that models the gamma curve is so coarse that there is basically no difference\n> > between srgb and gamma=2.2. The default can be overridden within the tuning\n> > file or set at runtime using the gamma control.\n> > \n> > The gamma algorithm is not enabled by default. This will be done in future\n> > tuning file updates.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > v2 -> v3:\n> > - Squashed with patch 1/4 from previous series\n> > - Renamed from Gamma to GammaOutCorrection\n> > - Read default gamma value from tuning file\n> > - Some stylistic fixes from review\n> > \n> > src/ipa/rkisp1/algorithms/goc.cpp | 161 ++++++++++++++++++++++++++\n> > src/ipa/rkisp1/algorithms/goc.h | 48 ++++++++\n> > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > src/ipa/rkisp1/ipa_context.h | 4 +\n> > 4 files changed, 214 insertions(+)\n> > create mode 100644 src/ipa/rkisp1/algorithms/goc.cpp\n> > create mode 100644 src/ipa/rkisp1/algorithms/goc.h\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp\n> > new file mode 100644\n> > index 00000000..875c82e5\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/goc.cpp\n> > @@ -0,0 +1,161 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + *\n> > + * RkISP1 Gamma out control\n> > + */\n> > +#include \"goc.h\"\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +#include \"linux/rkisp1-config.h\"\n> > +\n> > +/**\n> > + * \\file goc.h\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +/**\n> > + * \\class GammaOutCorrection\n> > + * \\brief RkISP1 Gamma out correction\n> > + *\n> > + * This algorithm implements the gamma out curve for the RkISP1.\n> > + * It defaults to a gamma value of 2.2\n> > + * As gamma is internally represented as a piecewise linear function with only\n> > + * 17 knots, the difference between gamma=2.2 and sRGB gamma is minimal.\n> > + * Therefore sRGB gamma was not implemented as special case.\n> > + *\n> > + * Useful links:\n> > + * https://www.cambridgeincolour.com/tutorials/gamma-correction.htm\n> > + * https://en.wikipedia.org/wiki/SRGB\n> > + */\n> > +\n> > +LOG_DEFINE_CATEGORY(RkISP1Gamma)\n> > +\n> > +const double kDefaultGamma = 2.2;\n> > +\n> > +GammaOutCorrection::GammaOutCorrection()\n> > + : gamma_(0)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::init\n> > + */\n> > +int GammaOutCorrection::init([[maybe_unused]] IPAContext &context,\n> > + [[maybe_unused]] const YamlObject &tuningData)\n> > +{\n> > + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, 2.2f);\n> > + defaultGamma_ = tuningData[\"gamma\"].get(kDefaultGamma);\n> > +\n> > + if (context.hw->numGammaOutSamples !=\n> > + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {\n> > + LOG(RkISP1Gamma, Error)\n> > + << \"Gamma is not implemented for RkISP1 V12\";\n> \n> Is that because the size is different? Are there other differences that\n> prevent us supporting it ?\n\nYes, and I have no documentation on the segment sizes for the\nlogarithmic case. (And no device to test it).\n\n> \n> > + return -EINVAL;\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Configure the Gamma given a configInfo\n> > + * \\param[in] context The shared IPA context\n> > + * \\param[in] configInfo The IPA configuration data\n> > + *\n> > + * \\return 0\n> > + */\n> > +int GammaOutCorrection::configure(IPAContext &context,\n> > + [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> > +{\n> > + context.activeState.gamma = defaultGamma_;\n> > + return 0;\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > + */\n> > +void GammaOutCorrection::queueRequest([[maybe_unused]] IPAContext &context,\n> > + [[maybe_unused]] const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + const ControlList &controls)\n> > +{\n> > + const auto &gamma = controls.get(controls::Gamma);\n> > + if (gamma) {\n> > + /*\n> > + * \\todo This is not correct as it updates the current state with a\n> > + * future value. But it does no harm at the moment an allows us to\n> > + * track the last active gamma\n> > + */\n> > + context.activeState.gamma = *gamma;\n> > + LOG(RkISP1Gamma, Debug) << \"Set gamma to \" << *gamma;\n> > + }\n> > +\n> > + frameContext.gamma = context.activeState.gamma;\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > + */\n> > +void GammaOutCorrection::prepare([[maybe_unused]] IPAContext &context,\n> > + const uint32_t frame,\n> > + [[maybe_unused]] IPAFrameContext &frameContext,\n> > + rkisp1_params_cfg *params)\n> > +{\n> > + ASSERT(context.hw->numGammaOutSamples ==\n> > + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);\n> > +\n> > + /*\n> > + * The logarithmic segments as specified in the reference.\n> > + * Plus an additional 0 to make the loop easier\n> \n> Is the '0' still required now that this uses utils::enumerate(segments)?\n\nThe segments define the spacing between the kneepoints. So there is one\nsegment less than kneepoints. Therefore the 0 is still needed.\n\n> \n> > + */\n> > + std::array segments = {\n> > + 64, 64, 64, 64, 128, 128, 128, 128, 256,\n> > + 256, 256, 512, 512, 512, 512, 512, 0\n> > + };\n> \n> Presumably we'd have different segments for V12 ?\n\nYes, exactly. And I don't know the sizes.\n\n> \n> > + auto gamma_y = params->others.goc_config.gamma_y;\n> > +\n> > + if (frame > 0 && std::fabs(frameContext.gamma - gamma_) < 0.001)\n> > + return;\n> > +\n> > + gamma_ = frameContext.gamma;\n> > +\n> > + unsigned x = 0;\n> > + for (auto [i, size] : utils::enumerate(segments)) {\n> > + gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> > + x += size;\n> \n> Assuming the '0' is actually valid, as I think the last iteration\n> through this loop will still have added x by 512 anyway,\n> \n\nBut without the 0 the loop would run one iteration too short and I would\nneed to duplicate the gamma_y = ... oh wait in our usecase gamma_y[0] is\nalways zero. I can start with gamma_y[1] and remove the 0. Dang.\n\n> \n> Reviewed-by: Kieran Bingham \n\nThanks :-)\n\n> \n> > + }\n> > +\n> > + params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > + params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::process\n> > + */\n> > +void GammaOutCorrection::process([[maybe_unused]] IPAContext &context,\n> > + [[maybe_unused]] const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > + ControlList &metadata)\n> > +{\n> > + metadata.set(controls::Gamma, frameContext.gamma);\n> > +}\n> > +\n> > +REGISTER_IPA_ALGORITHM(GammaOutCorrection, \"GammaOutCorrection\")\n> > +\n> > +} /* namespace ipa::rkisp1::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h\n> > new file mode 100644\n> > index 00000000..d45e4194\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/goc.h\n> > @@ -0,0 +1,48 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + *\n> > + * RkISP1 Gamma out control\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \"algorithm.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +class GammaOutCorrection : public Algorithm\n> > +{\n> > +public:\n> > + GammaOutCorrection();\n> > + ~GammaOutCorrection() = default;\n> > +\n> > + int init(IPAContext &context, const YamlObject &tuningData) override;\n> > + int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > + void queueRequest(IPAContext &context,\n> > + const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + const ControlList &controls) override;\n> > + void prepare(IPAContext &context, const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + rkisp1_params_cfg *params) override;\n> > + void process(IPAContext &context, const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + const rkisp1_stat_buffer *stats,\n> > + ControlList &metadata) override;\n> > +\n> > +private:\n> > + /*\n> > + * \\todo: gamma_ holds the current state of the hardware. context.activeState\n> > + * can not be used as that holds the \"future state\" after applying all known\n> > + * requests. The intended usage of activeState needs to be clarified.\n> > + */\n> > + double gamma_;\n> > +\n> > + double defaultGamma_;\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 93a48329..6ee71a9b 100644\n> > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > @@ -8,6 +8,7 @@ rkisp1_ipa_algorithms = files([\n> > 'dpcc.cpp',\n> > 'dpf.cpp',\n> > 'filter.cpp',\n> > + 'goc.cpp',\n> > 'gsl.cpp',\n> > 'lsc.cpp',\n> > ])\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index bd02a7a2..5252e222 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -104,6 +104,8 @@ struct IPAActiveState {\n> > uint8_t denoise;\n> > uint8_t sharpness;\n> > } filter;\n> > +\n> > + double gamma;\n> > };\n> > \n> > struct IPAFrameContext : public FrameContext {\n> > @@ -146,6 +148,8 @@ struct IPAFrameContext : public FrameContext {\n> > uint32_t exposure;\n> > double gain;\n> > } sensor;\n> > +\n> > + double gamma;\n> > };\n> > \n> > struct IPAContext {\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 751F9BD87C\n\tfor ;\n\tMon, 3 Jun 2024 17:21:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76391634CA;\n\tMon, 3 Jun 2024 19:21:13 +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 33AD961A3B\n\tfor ;\n\tMon, 3 Jun 2024 19:21:12 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:6067:153a:95aa:2e07])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F3B50A38;\n\tMon, 3 Jun 2024 19:21:04 +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=\"lBKrs+I/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717435265;\n\tbh=sgFuW2YybX8yIsR1erTTYI1HOjIrR+ICpDaKKgE6VwY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lBKrs+I/KCgue+BsjLTMdrHAEYGx7bRlgSzqHyusL/xyA3Fb7oYazSd2kcd7o+dOY\n\tYPqpr3urkcqQbPmDgSWTcz19XO+REKNA78ZO4+wVv/5QFoxey6lM5FYx/w9Ub6Imxg\n\tLkbK3tIE6qMK79Fiuwpux9tkY/FGNucd6JDNBjGA=","Date":"Mon, 3 Jun 2024 19:21:09 +0200","From":"Stefan Klug ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/3] ipa: rkisp1: Add GammaOutCorrection algorithm","Message-ID":"","References":"<20240603140806.90045-1-stefan.klug@ideasonboard.com>\n\t<20240603140806.90045-4-stefan.klug@ideasonboard.com>\n\t<171743024565.205609.5835595704744600028@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171743024565.205609.5835595704744600028@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":29759,"web_url":"https://patchwork.libcamera.org/comment/29759/","msgid":"","date":"2024-06-04T07:34:55","subject":"Re: [PATCH v3 3/3] ipa: rkisp1: Add GammaOutCorrection algorithm","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jun 03, 2024 at 07:21:09PM +0200, Stefan Klug wrote:\n> Hi Kieran,\n> \n> thanks for the review.\n> \n> On Mon, Jun 03, 2024 at 04:57:25PM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2024-06-03 15:06:30)\n> > > Add a gamma algorithm for the rkisp1. It defaults to a gamma of 2.2 which\n> > > closely resembles sRGB. No seperate sRGB mode was implemented because the pwl\n> > > that models the gamma curve is so coarse that there is basically no difference\n> > > between srgb and gamma=2.2. The default can be overridden within the tuning\n> > > file or set at runtime using the gamma control.\n> > > \n> > > The gamma algorithm is not enabled by default. This will be done in future\n> > > tuning file updates.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > v2 -> v3:\n> > > - Squashed with patch 1/4 from previous series\n> > > - Renamed from Gamma to GammaOutCorrection\n> > > - Read default gamma value from tuning file\n> > > - Some stylistic fixes from review\n> > > \n> > > src/ipa/rkisp1/algorithms/goc.cpp | 161 ++++++++++++++++++++++++++\n> > > src/ipa/rkisp1/algorithms/goc.h | 48 ++++++++\n> > > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > > src/ipa/rkisp1/ipa_context.h | 4 +\n> > > 4 files changed, 214 insertions(+)\n> > > create mode 100644 src/ipa/rkisp1/algorithms/goc.cpp\n> > > create mode 100644 src/ipa/rkisp1/algorithms/goc.h\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp\n> > > new file mode 100644\n> > > index 00000000..875c82e5\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/algorithms/goc.cpp\n> > > @@ -0,0 +1,161 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas On Board\n> > > + *\n> > > + * RkISP1 Gamma out control\n> > > + */\n> > > +#include \"goc.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > +\n> > > +#include \"linux/rkisp1-config.h\"\n> > > +\n> > > +/**\n> > > + * \\file goc.h\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::algorithms {\n> > > +\n> > > +/**\n> > > + * \\class GammaOutCorrection\n> > > + * \\brief RkISP1 Gamma out correction\n> > > + *\n> > > + * This algorithm implements the gamma out curve for the RkISP1.\n> > > + * It defaults to a gamma value of 2.2\n> > > + * As gamma is internally represented as a piecewise linear function with only\n> > > + * 17 knots, the difference between gamma=2.2 and sRGB gamma is minimal.\n> > > + * Therefore sRGB gamma was not implemented as special case.\n> > > + *\n> > > + * Useful links:\n> > > + * https://www.cambridgeincolour.com/tutorials/gamma-correction.htm\n> > > + * https://en.wikipedia.org/wiki/SRGB\n> > > + */\n> > > +\n> > > +LOG_DEFINE_CATEGORY(RkISP1Gamma)\n> > > +\n> > > +const double kDefaultGamma = 2.2;\n> > > +\n> > > +GammaOutCorrection::GammaOutCorrection()\n> > > + : gamma_(0)\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > + */\n> > > +int GammaOutCorrection::init([[maybe_unused]] IPAContext &context,\n> > > + [[maybe_unused]] const YamlObject &tuningData)\n> > > +{\n> > > + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, 2.2f);\n> > > + defaultGamma_ = tuningData[\"gamma\"].get(kDefaultGamma);\n> > > +\n> > > + if (context.hw->numGammaOutSamples !=\n> > > + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {\n> > > + LOG(RkISP1Gamma, Error)\n> > > + << \"Gamma is not implemented for RkISP1 V12\";\n> > \n> > Is that because the size is different? Are there other differences that\n> > prevent us supporting it ?\n> \n> Yes, and I have no documentation on the segment sizes for the\n> logarithmic case. (And no device to test it).\n> \n> > \n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Configure the Gamma given a configInfo\n> > > + * \\param[in] context The shared IPA context\n> > > + * \\param[in] configInfo The IPA configuration data\n> > > + *\n> > > + * \\return 0\n> > > + */\n> > > +int GammaOutCorrection::configure(IPAContext &context,\n> > > + [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> > > +{\n> > > + context.activeState.gamma = defaultGamma_;\n> > > + return 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > > + */\n> > > +void GammaOutCorrection::queueRequest([[maybe_unused]] IPAContext &context,\n> > > + [[maybe_unused]] const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + const ControlList &controls)\n> > > +{\n> > > + const auto &gamma = controls.get(controls::Gamma);\n> > > + if (gamma) {\n> > > + /*\n> > > + * \\todo This is not correct as it updates the current state with a\n> > > + * future value. But it does no harm at the moment an allows us to\n> > > + * track the last active gamma\n> > > + */\n> > > + context.activeState.gamma = *gamma;\n> > > + LOG(RkISP1Gamma, Debug) << \"Set gamma to \" << *gamma;\n> > > + }\n> > > +\n> > > + frameContext.gamma = context.activeState.gamma;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > + */\n> > > +void GammaOutCorrection::prepare([[maybe_unused]] IPAContext &context,\n> > > + const uint32_t frame,\n> > > + [[maybe_unused]] IPAFrameContext &frameContext,\n> > > + rkisp1_params_cfg *params)\n> > > +{\n> > > + ASSERT(context.hw->numGammaOutSamples ==\n> > > + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);\n> > > +\n> > > + /*\n> > > + * The logarithmic segments as specified in the reference.\n> > > + * Plus an additional 0 to make the loop easier\n> > \n> > Is the '0' still required now that this uses utils::enumerate(segments)?\n> \n> The segments define the spacing between the kneepoints. So there is one\n> segment less than kneepoints. Therefore the 0 is still needed.\n> \n> > \n> > > + */\n> > > + std::array segments = {\n> > > + 64, 64, 64, 64, 128, 128, 128, 128, 256,\n> > > + 256, 256, 512, 512, 512, 512, 512, 0\n> > > + };\n> > \n> > Presumably we'd have different segments for V12 ?\n> \n> Yes, exactly. And I don't know the sizes.\n> \n> > \n> > > + auto gamma_y = params->others.goc_config.gamma_y;\n> > > +\n> > > + if (frame > 0 && std::fabs(frameContext.gamma - gamma_) < 0.001)\n> > > + return;\n> > > +\n> > > + gamma_ = frameContext.gamma;\n> > > +\n> > > + unsigned x = 0;\n> > > + for (auto [i, size] : utils::enumerate(segments)) {\n> > > + gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> > > + x += size;\n> > \n> > Assuming the '0' is actually valid, as I think the last iteration\n> > through this loop will still have added x by 512 anyway,\n> > \n> \n> But without the 0 the loop would run one iteration too short and I would\n> need to duplicate the gamma_y = ... oh wait in our usecase gamma_y[0] is\n> always zero. I can start with gamma_y[1] and remove the 0. Dang.\n\nI did that. The code looks like this:\n----\n\n\t/* The logarithmic segments as specified in the reference. */\n\tstd::array segments = {\n\t\t64, 64, 64, 64, 128, 128, 128, 128, 256,\n\t\t256, 256, 512, 512, 512, 512, 512\n\t};\n\tauto gamma_y = params->others.goc_config.gamma_y;\n\n\tif (frame > 0 && std::fabs(frameContext.gamma - gamma_) < 0.001)\n\t\treturn;\n\n\tgamma_ = frameContext.gamma;\n\n\tunsigned x = 0;\n\tgamma_y[0] = 0;\n\tfor (auto [i, size] : utils::enumerate(segments)) {\n\t\tx += size;\n\t\tgamma_y[i+1] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n\t}\n\n---\n\nIs that really better, with that speciall handling of gamma_y[0]?\n\nRegards,\nStefan\n\n> \n> > \n> > Reviewed-by: Kieran Bingham \n> \n> Thanks :-)\n> \n> > \n> > > + }\n> > > +\n> > > + params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::process\n> > > + */\n> > > +void GammaOutCorrection::process([[maybe_unused]] IPAContext &context,\n> > > + [[maybe_unused]] const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > + ControlList &metadata)\n> > > +{\n> > > + metadata.set(controls::Gamma, frameContext.gamma);\n> > > +}\n> > > +\n> > > +REGISTER_IPA_ALGORITHM(GammaOutCorrection, \"GammaOutCorrection\")\n> > > +\n> > > +} /* namespace ipa::rkisp1::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h\n> > > new file mode 100644\n> > > index 00000000..d45e4194\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/algorithms/goc.h\n> > > @@ -0,0 +1,48 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas On Board\n> > > + *\n> > > + * RkISP1 Gamma out control\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \"algorithm.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::algorithms {\n> > > +\n> > > +class GammaOutCorrection : public Algorithm\n> > > +{\n> > > +public:\n> > > + GammaOutCorrection();\n> > > + ~GammaOutCorrection() = default;\n> > > +\n> > > + int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > + int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > > + void queueRequest(IPAContext &context,\n> > > + const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + const ControlList &controls) override;\n> > > + void prepare(IPAContext &context, const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + rkisp1_params_cfg *params) override;\n> > > + void process(IPAContext &context, const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + const rkisp1_stat_buffer *stats,\n> > > + ControlList &metadata) override;\n> > > +\n> > > +private:\n> > > + /*\n> > > + * \\todo: gamma_ holds the current state of the hardware. context.activeState\n> > > + * can not be used as that holds the \"future state\" after applying all known\n> > > + * requests. The intended usage of activeState needs to be clarified.\n> > > + */\n> > > + double gamma_;\n> > > +\n> > > + double defaultGamma_;\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 93a48329..6ee71a9b 100644\n> > > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > > @@ -8,6 +8,7 @@ rkisp1_ipa_algorithms = files([\n> > > 'dpcc.cpp',\n> > > 'dpf.cpp',\n> > > 'filter.cpp',\n> > > + 'goc.cpp',\n> > > 'gsl.cpp',\n> > > 'lsc.cpp',\n> > > ])\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index bd02a7a2..5252e222 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -104,6 +104,8 @@ struct IPAActiveState {\n> > > uint8_t denoise;\n> > > uint8_t sharpness;\n> > > } filter;\n> > > +\n> > > + double gamma;\n> > > };\n> > > \n> > > struct IPAFrameContext : public FrameContext {\n> > > @@ -146,6 +148,8 @@ struct IPAFrameContext : public FrameContext {\n> > > uint32_t exposure;\n> > > double gain;\n> > > } sensor;\n> > > +\n> > > + double gamma;\n> > > };\n> > > \n> > > struct IPAContext {\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 19D63BDE6B\n\tfor ;\n\tTue, 4 Jun 2024 07:35:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AADA4634CB;\n\tTue, 4 Jun 2024 09:35:00 +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 83D73634BA\n\tfor ;\n\tTue, 4 Jun 2024 09:34:58 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:a102:b099:ebdc:c91c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BE02D15B5;\n\tTue, 4 Jun 2024 09:34:50 +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=\"A2ipzfJI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717486490;\n\tbh=1Yd0V4l5dQdj9969XMRG68yaP9cR3nl/pDd0//KsXT4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A2ipzfJIR1DC40E7faMWtK64CDLu6n3wCNXMO/mhPi2jNpHwQR1gATctH6l0D9ViF\n\ty9iLkP8yL0sXiYFsOix7ZBjx8xGGNyH/dzr2X3GNFXG3iw3KtwkVBl72xJVlJfFn+W\n\tB2MZAAN8rxI6wcpvXI+ClDuwPG2VdawwTrRI2XLE=","Date":"Tue, 4 Jun 2024 09:34:55 +0200","From":"Stefan Klug ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/3] ipa: rkisp1: Add GammaOutCorrection algorithm","Message-ID":"","References":"<20240603140806.90045-1-stefan.klug@ideasonboard.com>\n\t<20240603140806.90045-4-stefan.klug@ideasonboard.com>\n\t<171743024565.205609.5835595704744600028@ping.linuxembedded.co.uk>\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":29760,"web_url":"https://patchwork.libcamera.org/comment/29760/","msgid":"<171748788805.205609.5397197884753615812@ping.linuxembedded.co.uk>","date":"2024-06-04T07:58:08","subject":"Re: [PATCH v3 3/3] ipa: rkisp1: Add GammaOutCorrection algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-06-04 08:34:55)\n> Hi Kieran,\n> \n> On Mon, Jun 03, 2024 at 07:21:09PM +0200, Stefan Klug wrote:\n> > Hi Kieran,\n> > \n> > thanks for the review.\n> > \n> > On Mon, Jun 03, 2024 at 04:57:25PM +0100, Kieran Bingham wrote:\n> > > Quoting Stefan Klug (2024-06-03 15:06:30)\n> > > > Add a gamma algorithm for the rkisp1. It defaults to a gamma of 2.2 which\n> > > > closely resembles sRGB. No seperate sRGB mode was implemented because the pwl\n> > > > that models the gamma curve is so coarse that there is basically no difference\n> > > > between srgb and gamma=2.2. The default can be overridden within the tuning\n> > > > file or set at runtime using the gamma control.\n> > > > \n> > > > The gamma algorithm is not enabled by default. This will be done in future\n> > > > tuning file updates.\n> > > > \n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > v2 -> v3:\n> > > > - Squashed with patch 1/4 from previous series\n> > > > - Renamed from Gamma to GammaOutCorrection\n> > > > - Read default gamma value from tuning file\n> > > > - Some stylistic fixes from review\n> > > > \n> > > > src/ipa/rkisp1/algorithms/goc.cpp | 161 ++++++++++++++++++++++++++\n> > > > src/ipa/rkisp1/algorithms/goc.h | 48 ++++++++\n> > > > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > > > src/ipa/rkisp1/ipa_context.h | 4 +\n> > > > 4 files changed, 214 insertions(+)\n> > > > create mode 100644 src/ipa/rkisp1/algorithms/goc.cpp\n> > > > create mode 100644 src/ipa/rkisp1/algorithms/goc.h\n> > > > \n> > > > diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp\n> > > > new file mode 100644\n> > > > index 00000000..875c82e5\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/rkisp1/algorithms/goc.cpp\n> > > > @@ -0,0 +1,161 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Ideas On Board\n> > > > + *\n> > > > + * RkISP1 Gamma out control\n> > > > + */\n> > > > +#include \"goc.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > > +\n> > > > +#include \"linux/rkisp1-config.h\"\n> > > > +\n> > > > +/**\n> > > > + * \\file goc.h\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa::rkisp1::algorithms {\n> > > > +\n> > > > +/**\n> > > > + * \\class GammaOutCorrection\n> > > > + * \\brief RkISP1 Gamma out correction\n> > > > + *\n> > > > + * This algorithm implements the gamma out curve for the RkISP1.\n> > > > + * It defaults to a gamma value of 2.2\n> > > > + * As gamma is internally represented as a piecewise linear function with only\n> > > > + * 17 knots, the difference between gamma=2.2 and sRGB gamma is minimal.\n> > > > + * Therefore sRGB gamma was not implemented as special case.\n> > > > + *\n> > > > + * Useful links:\n> > > > + * https://www.cambridgeincolour.com/tutorials/gamma-correction.htm\n> > > > + * https://en.wikipedia.org/wiki/SRGB\n> > > > + */\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(RkISP1Gamma)\n> > > > +\n> > > > +const double kDefaultGamma = 2.2;\n> > > > +\n> > > > +GammaOutCorrection::GammaOutCorrection()\n> > > > + : gamma_(0)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > > + */\n> > > > +int GammaOutCorrection::init([[maybe_unused]] IPAContext &context,\n> > > > + [[maybe_unused]] const YamlObject &tuningData)\n> > > > +{\n> > > > + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, 2.2f);\n> > > > + defaultGamma_ = tuningData[\"gamma\"].get(kDefaultGamma);\n> > > > +\n> > > > + if (context.hw->numGammaOutSamples !=\n> > > > + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {\n> > > > + LOG(RkISP1Gamma, Error)\n> > > > + << \"Gamma is not implemented for RkISP1 V12\";\n> > > \n> > > Is that because the size is different? Are there other differences that\n> > > prevent us supporting it ?\n> > \n> > Yes, and I have no documentation on the segment sizes for the\n> > logarithmic case. (And no device to test it).\n> > \n> > > \n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Configure the Gamma given a configInfo\n> > > > + * \\param[in] context The shared IPA context\n> > > > + * \\param[in] configInfo The IPA configuration data\n> > > > + *\n> > > > + * \\return 0\n> > > > + */\n> > > > +int GammaOutCorrection::configure(IPAContext &context,\n> > > > + [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> > > > +{\n> > > > + context.activeState.gamma = defaultGamma_;\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > > > + */\n> > > > +void GammaOutCorrection::queueRequest([[maybe_unused]] IPAContext &context,\n> > > > + [[maybe_unused]] const uint32_t frame,\n> > > > + IPAFrameContext &frameContext,\n> > > > + const ControlList &controls)\n> > > > +{\n> > > > + const auto &gamma = controls.get(controls::Gamma);\n> > > > + if (gamma) {\n> > > > + /*\n> > > > + * \\todo This is not correct as it updates the current state with a\n> > > > + * future value. But it does no harm at the moment an allows us to\n> > > > + * track the last active gamma\n> > > > + */\n> > > > + context.activeState.gamma = *gamma;\n> > > > + LOG(RkISP1Gamma, Debug) << \"Set gamma to \" << *gamma;\n> > > > + }\n> > > > +\n> > > > + frameContext.gamma = context.activeState.gamma;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > + */\n> > > > +void GammaOutCorrection::prepare([[maybe_unused]] IPAContext &context,\n> > > > + const uint32_t frame,\n> > > > + [[maybe_unused]] IPAFrameContext &frameContext,\n> > > > + rkisp1_params_cfg *params)\n> > > > +{\n> > > > + ASSERT(context.hw->numGammaOutSamples ==\n> > > > + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);\n> > > > +\n> > > > + /*\n> > > > + * The logarithmic segments as specified in the reference.\n> > > > + * Plus an additional 0 to make the loop easier\n> > > \n> > > Is the '0' still required now that this uses utils::enumerate(segments)?\n> > \n> > The segments define the spacing between the kneepoints. So there is one\n> > segment less than kneepoints. Therefore the 0 is still needed.\n> > \n> > > \n> > > > + */\n> > > > + std::array segments = {\n> > > > + 64, 64, 64, 64, 128, 128, 128, 128, 256,\n> > > > + 256, 256, 512, 512, 512, 512, 512, 0\n> > > > + };\n> > > \n> > > Presumably we'd have different segments for V12 ?\n> > \n> > Yes, exactly. And I don't know the sizes.\n> > \n> > > \n> > > > + auto gamma_y = params->others.goc_config.gamma_y;\n> > > > +\n> > > > + if (frame > 0 && std::fabs(frameContext.gamma - gamma_) < 0.001)\n> > > > + return;\n> > > > +\n> > > > + gamma_ = frameContext.gamma;\n> > > > +\n> > > > + unsigned x = 0;\n> > > > + for (auto [i, size] : utils::enumerate(segments)) {\n> > > > + gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> > > > + x += size;\n> > > \n> > > Assuming the '0' is actually valid, as I think the last iteration\n> > > through this loop will still have added x by 512 anyway,\n> > > \n> > \n> > But without the 0 the loop would run one iteration too short and I would\n> > need to duplicate the gamma_y = ... oh wait in our usecase gamma_y[0] is\n> > always zero. I can start with gamma_y[1] and remove the 0. Dang.\n> \n> I did that. The code looks like this:\n> ----\n> \n> /* The logarithmic segments as specified in the reference. */\n> std::array segments = {\n> 64, 64, 64, 64, 128, 128, 128, 128, 256,\n> 256, 256, 512, 512, 512, 512, 512\n> };\n> auto gamma_y = params->others.goc_config.gamma_y;\n> \n> if (frame > 0 && std::fabs(frameContext.gamma - gamma_) < 0.001)\n> return;\n> \n> gamma_ = frameContext.gamma;\n> \n> unsigned x = 0;\n> gamma_y[0] = 0;\n> for (auto [i, size] : utils::enumerate(segments)) {\n> x += size;\n> gamma_y[i+1] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> }\n> \n> ---\n> \n> Is that really better, with that speciall handling of gamma_y[0]?\n\nMaybe not, you can choose whichever you prefer ;-)\n\n--\nKieran","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 6E78BBD87C\n\tfor ;\n\tTue, 4 Jun 2024 07:58:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72395634C5;\n\tTue, 4 Jun 2024 09:58: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 995CA634BA\n\tfor ;\n\tTue, 4 Jun 2024 09:58:11 +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 D76DD15B5;\n\tTue, 4 Jun 2024 09:58:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VmvAUZwz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717487883;\n\tbh=TOLApmT/v1RQ9/gLU3hAKs6Wfah0aCOsqe5/skWAruM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VmvAUZwzt1VdTg69+7zz0rYa1RhQijgYuR0wDbNN+RrdiiCPxuWofCw5RuQxBM5Bo\n\tkCVntSxYcPKYB37ylLpw4D3V2z4F6AbIblvIlXK4F+YBJz4+mCou2/+S/3nUlpDLBd\n\tZ21+eSD7oOrvd6+Rbf6cB4eOW6NieVg+oA/zH2YQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240603140806.90045-1-stefan.klug@ideasonboard.com>\n\t<20240603140806.90045-4-stefan.klug@ideasonboard.com>\n\t<171743024565.205609.5835595704744600028@ping.linuxembedded.co.uk>\n\t\n\t","Subject":"Re: [PATCH v3 3/3] ipa: rkisp1: Add GammaOutCorrection algorithm","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Stefan Klug ","Date":"Tue, 04 Jun 2024 08:58:08 +0100","Message-ID":"<171748788805.205609.5397197884753615812@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\" "}}]