[{"id":31034,"web_url":"https://patchwork.libcamera.org/comment/31034/","msgid":"<20240901214720.GC31148@pendragon.ideasonboard.com>","date":"2024-09-01T21:47:20","subject":"Re: [PATCH v5 14/18] libcamera: software_isp: Move color handling to\n\tan algorithm module","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Fri, Aug 30, 2024 at 09:25:50AM +0200, Milan Zamazal wrote:\n> After black level handling has been moved to an algorithm module, white\n> balance and the construction of color tables can be moved to algorithm\n> modules too.\n> \n> This time, the moved code is split between stats processing and\n> parameter construction methods. It is also split to three algorithm\n> modules:\n> \n> - Gamma table computation. This is actually independent of color\n> handling.\n> \n> - White balance computation.\n> \n> - Color lookup tables construction. While this applies the color gains\n> computed by the white balance algorithm, it is not directly related to\n> white balance. We may want to modify the color lookup tables in\n> future according to other parameters than just gamma and white balance\n> gains.\n> \n> This is the only part of the software ISP algorithms that sets the\n> parameters so emitting setIspParams can be moved to prepare() method.\n> \n> A more natural representation of the gains (and black level) would be\n> floating point numbers. This is not done here in order to minimize\n> changes in code movements. It will be addressed in a followup patch.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> src/ipa/simple/algorithms/awb.cpp | 70 +++++++++++++++++++++++++++\n> src/ipa/simple/algorithms/awb.h | 38 +++++++++++++++\n> src/ipa/simple/algorithms/gamma.cpp | 64 ++++++++++++++++++++++++\n> src/ipa/simple/algorithms/gamma.h | 42 ++++++++++++++++\n> src/ipa/simple/algorithms/lut.cpp | 57 ++++++++++++++++++++++\n> src/ipa/simple/algorithms/lut.h | 37 ++++++++++++++\n> src/ipa/simple/algorithms/meson.build | 3 ++\n> src/ipa/simple/data/uncalibrated.yaml | 3 ++\n> src/ipa/simple/ipa_context.cpp | 30 ++++++++++++\n> src/ipa/simple/ipa_context.h | 12 +++++\n> src/ipa/simple/soft_simple.cpp | 66 ++-----------------------\n> 11 files changed, 360 insertions(+), 62 deletions(-)\n> create mode 100644 src/ipa/simple/algorithms/awb.cpp\n> create mode 100644 src/ipa/simple/algorithms/awb.h\n> create mode 100644 src/ipa/simple/algorithms/gamma.cpp\n> create mode 100644 src/ipa/simple/algorithms/gamma.h\n> create mode 100644 src/ipa/simple/algorithms/lut.cpp\n> create mode 100644 src/ipa/simple/algorithms/lut.h\n> \n> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp\n> new file mode 100644\n> index 00000000..4b49996a\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/awb.cpp\n> @@ -0,0 +1,70 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * Auto white balance\n> + */\n> +\n> +#include \"awb.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"simple/ipa_context.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPASoftAwb)\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +int Awb::init(IPAContext &context,\n> +\t [[maybe_unused]] const YamlObject &tuningData)\n> +{\n> +\tauto &gains = context.activeState.gains;\n> +\tgains.red = gains.green = gains.blue = 256;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void Awb::process(IPAContext &context,\n> +\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> +\t\t const SwIspStats *stats,\n> +\t\t [[maybe_unused]] ControlList &metadata)\n> +{\n> +\tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n> +\tconst uint8_t blackLevel = context.activeState.black.level;\n> +\n> +\t/*\n> +\t * Black level must be subtracted to get the correct AWB ratios, they\n> +\t * would be off if they were computed from the whole brightness range\n> +\t * rather than from the sensor range.\n> +\t */\n> +\tconst uint64_t nPixels = std::accumulate(\n> +\t\thistogram.begin(), histogram.end(), 0);\n> +\tconst uint64_t offset = blackLevel * nPixels;\n> +\tconst uint64_t sumR = stats->sumR_ - offset / 4;\n> +\tconst uint64_t sumG = stats->sumG_ - offset / 2;\n> +\tconst uint64_t sumB = stats->sumB_ - offset / 4;\n> +\n> +\t/*\n> +\t * Calculate red and blue gains for AWB.\n> +\t * Clamp max gain at 4.0, this also avoids 0 division.\n> +\t * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n> +\t */\n> +\tauto &gains = context.activeState.gains;\n> +\tgains.red = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> +\tgains.blue = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n> +\t/* Green gain is fixed to 256 */\n> +\n> +\tLOG(IPASoftAwb, Debug) << \"gain R/B \" << gains.red << \"/\" << gains.blue;\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Awb, \"Awb\")\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h\n> new file mode 100644\n> index 00000000..c0b88bec\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/awb.h\n> @@ -0,0 +1,38 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * Auto white balance\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n> +\n> +#include \"algorithm.h\"\n> +#include \"ipa_context.h\"\n\nAs with the previous patch, I think you can drop all the includes except\nalgorithm.h. Same for the other algorithms.\n\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +class Awb : public Algorithm\n> +{\n> +public:\n> +\tAwb() = default;\n> +\t~Awb() = default;\n> +\n> +\tint init(IPAContext &context, const YamlObject &tuningData)\n> +\t\toverride;\n\nNo need to split this on two lines. Same below.\n\n> +\tvoid process(IPAContext &context,\n> +\t\t const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t const SwIspStats *stats,\n> +\t\t ControlList &metadata) override;\n> +};\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/gamma.cpp b/src/ipa/simple/algorithms/gamma.cpp\n> new file mode 100644\n> index 00000000..0b8ec5ee\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/gamma.cpp\n> @@ -0,0 +1,64 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * Gamma table construction\n> + */\n> +\n> +#include \"gamma.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"simple/ipa_context.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +int Gamma::init(IPAContext &context,\n> +\t\t[[maybe_unused]] const YamlObject &tuningData)\n> +{\n> +\t/* Gamma value is fixed */\n> +\tcontext.configuration.gamma = 0.5;\n> +\tupdateGammaTable(context);\n\nSame question as before, does this belong to .init() ? Actually, given\nthat prepare() will always be called to calculate ISP parameters, do you\neven need to initialize the gamme table ?\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +void Gamma::updateGammaTable(IPAContext &context)\n> +{\n> +\tauto &gammaTable = context.activeState.gamma.gammaTable;\n> +\tauto blackLevel = context.activeState.black.level;\n> +\tconst unsigned int blackIndex =\n> +\t\tblackLevel * IPAActiveState::kGammaLookupSize / 256;\n\nUse gammaTable.size() instead of kGammaLookupSize.\n\n> +\tstd::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n> +\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n> +\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n\nSame here.\n\n> +\t\tgammaTable[i] = UINT8_MAX * powf((i - blackIndex) / divisor,\n\nstd::powf\n\n> +\t\t\t\t\t\t context.configuration.gamma);\n> +\tcontext.activeState.gamma.blackLevel = blackLevel;\n> +}\n> +\n> +void Gamma::prepare(IPAContext &context,\n> +\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> +\t\t [[maybe_unused]] DebayerParams *params)\n> +{\n> +\t/*\n> +\t * Update the gamma table if needed. This means if black level changes and\n> +\t * since the black level gets updated only if a lower value is observed,\n> +\t * it's not permanently prone to minor fluctuations or rounding errors.\n\nPlease reflow to 80 columns.\n\n> +\t */\n> +\tif (context.activeState.gamma.blackLevel != context.activeState.black.level)\n> +\t\tupdateGammaTable(context);\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Gamma, \"Gamma\")\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/gamma.h b/src/ipa/simple/algorithms/gamma.h\n> new file mode 100644\n> index 00000000..43e88e87\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/gamma.h\n> @@ -0,0 +1,42 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * Gamma table construction\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> +\n> +#include \"algorithm.h\"\n> +#include \"ipa_context.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +static constexpr unsigned int kGammaLookupSize = 1024;\n> +\n> +class Gamma : public Algorithm\n> +{\n> +public:\n> +\tGamma() = default;\n> +\t~Gamma() = default;\n> +\n> +\tint init(IPAContext &context, const YamlObject &tuningData)\n> +\t\toverride;\n> +\tvoid prepare(IPAContext &context,\n> +\t\t const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t DebayerParams *params) override;\n> +\n> +private:\n> +\tvoid updateGammaTable(IPAContext &context);\n> +};\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> new file mode 100644\n> index 00000000..748f10aa\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/lut.cpp\n> @@ -0,0 +1,57 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * Lookup table construction\n> + */\n> +\n> +#include \"lut.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"simple/ipa_context.h\"\n> +\n> +#include \"gamma.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +int Lut::init(IPAContext &context,\n> +\t [[maybe_unused]] const YamlObject &tuningData)\n> +{\n> +\tauto &gains = context.activeState.gains;\n> +\tgains.red = gains.green = gains.blue = 1.0;\n\nThat looks wrong, the Awb algorithm does this already.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +void Lut::prepare(IPAContext &context,\n> +\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> +\t\t DebayerParams *params)\n> +{\n> +\tauto &gains = context.activeState.gains;\n> +\tauto &gammaTable = context.activeState.gamma.gammaTable;\n> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\tconstexpr unsigned int div =\n> +\t\t\tstatic_cast(DebayerParams::kRGBLookupSize) * 256 / kGammaLookupSize;\n> +\t\t/* Apply gamma after gain! */\n> +\t\tunsigned int idx;\n> +\t\tidx = std::min({ i * gains.red / div, kGammaLookupSize - 1 });\n> +\t\tparams->red[i] = gammaTable[idx];\n> +\t\tidx = std::min({ i * gains.green / div, kGammaLookupSize - 1 });\n> +\t\tparams->green[i] = gammaTable[idx];\n> +\t\tidx = std::min({ i * gains.blue / div, kGammaLookupSize - 1 });\n> +\t\tparams->blue[i] = gammaTable[idx];\n> +\t}\n\nWhy is this a separate algorithm, why isn't it part of Gamma ?\n\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Lut, \"Lut\")\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n> new file mode 100644\n> index 00000000..5b5e554e\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/lut.h\n> @@ -0,0 +1,37 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * Lookup table construction\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> +\n> +#include \"algorithm.h\"\n> +#include \"ipa_context.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +class Lut : public Algorithm\n> +{\n> +public:\n> +\tLut() = default;\n> +\t~Lut() = default;\n> +\n> +\tint init(IPAContext &context, const YamlObject &tuningData)\n> +\t\toverride;\n> +\tvoid prepare(IPAContext &context,\n> +\t\t const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t DebayerParams *params) override;\n> +};\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n> index 1f63c220..68e5aa58 100644\n> --- a/src/ipa/simple/algorithms/meson.build\n> +++ b/src/ipa/simple/algorithms/meson.build\n> @@ -1,5 +1,8 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> soft_simple_ipa_algorithms = files([\n> + 'awb.cpp',\n> 'blc.cpp',\n> + 'gamma.cpp',\n> + 'lut.cpp',\n> ])\n> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n> index e0d03d96..deaea6b1 100644\n> --- a/src/ipa/simple/data/uncalibrated.yaml\n> +++ b/src/ipa/simple/data/uncalibrated.yaml\n> @@ -4,4 +4,7 @@\n> version: 1\n> algorithms:\n> - BlackLevel:\n> + - Gamma:\n> + - Awb:\n> + - Lut:\n> ...\n> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp\n> index 268cff32..5fa492cb 100644\n> --- a/src/ipa/simple/ipa_context.cpp\n> +++ b/src/ipa/simple/ipa_context.cpp\n> @@ -50,6 +50,11 @@ namespace libcamera::ipa::soft {\n> * \\brief The current state of IPA algorithms\n> */\n> \n> +/**\n> + * \\var IPASessionConfiguration::gamma\n> + * \\brief Gamma value to be used in the raw image processing\n> + */\n> +\n> /**\n> * \\var IPAActiveState::black\n> * \\brief Context for the Black Level algorithm\n> @@ -58,4 +63,29 @@ namespace libcamera::ipa::soft {\n> * \\brief Current determined black level\n> */\n> \n> +/**\n> + * \\var IPAActiveState::gains\n> + * \\brief Context for gains in the Colors algorithm\n> + *\n> + * \\var IPAActiveState::gains.red\n> + * \\brief Gain of red color\n> + *\n> + * \\var IPAActiveState::gains.green\n> + * \\brief Gain of green color\n> + *\n> + * \\var IPAActiveState::gains.blue\n> + * \\brief Gain of blue color\n> + */\n> +\n> +/**\n> + * \\var IPAActiveState::gamma\n> + * \\brief Context for gamma in the Colors algorithm\n> + *\n> + * \\var IPAActiveState::gamma.gammaTable\n> + * \\brief Computed gamma table\n> + *\n> + * \\var IPAActiveState::gamma.blackLevel\n> + * \\brief Black level used for the gamma table computation\n> + */\n> +\n> } /* namespace libcamera::ipa::soft */\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index 22156569..f8b8834d 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -7,6 +7,7 @@\n> \n> #pragma once\n> \n> +#include \n> #include \n> \n> #include \n> @@ -16,12 +17,23 @@ namespace libcamera {\n> namespace ipa::soft {\n> \n> struct IPASessionConfiguration {\n> +\tfloat gamma;\n> };\n> \n> struct IPAActiveState {\n> \tstruct {\n> \t\tuint8_t level;\n> \t} black;\n\nBlank line. Same below.\n\n> +\tstruct {\n> +\t\tunsigned int red;\n> +\t\tunsigned int green;\n> +\t\tunsigned int blue;\n> +\t} gains;\n> +\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n\nThis duplicates the kGammaLookupSize from\nsrc/ipa/simple/algorithms/gamma.h. \n\n> +\tstruct {\n> +\t\tstd::array gammaTable;\n> +\t\tuint8_t blackLevel;\n> +\t} gamma;\n> };\n> \n> struct IPAFrameContext : public FrameContext {\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index a1bd2f0c..8f01bf7d 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -5,8 +5,6 @@\n> * Simple Software Image Processing Algorithm module\n> */\n> \n> -#include \n> -#include \n> #include \n> #include \n> \n> @@ -93,9 +91,6 @@ private:\n> \tstd::unique_ptr camHelper_;\n> \tControlInfoMap sensorInfoMap_;\n> \n> -\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n> -\tstd::array gammaTable_;\n> -\tint lastBlackLevel_ = -1;\n> \t/* Local parameter storage */\n> \tstruct IPAContext context_;\n> \n> @@ -283,6 +278,7 @@ void IPASoftSimple::prepare(const uint32_t frame)\n> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> \tfor (auto const &algo : algorithms())\n> \t\talgo->prepare(context_, frame, frameContext, params_);\n> +\tsetIspParams.emit();\n> }\n> \n> void IPASoftSimple::processStats(const uint32_t frame,\n> @@ -300,62 +296,6 @@ void IPASoftSimple::processStats(const uint32_t frame,\n> \tfor (auto const &algo : algorithms())\n> \t\talgo->process(context_, frame, frameContext, stats_, metadata);\n> \n> -\tSwIspStats::Histogram histogram = stats_->yHistogram;\n> -\tconst uint8_t blackLevel = context_.activeState.black.level;\n> -\n> -\t/*\n> -\t * Black level must be subtracted to get the correct AWB ratios, they\n> -\t * would be off if they were computed from the whole brightness range\n> -\t * rather than from the sensor range.\n> -\t */\n> -\tconst uint64_t nPixels = std::accumulate(\n> -\t\thistogram.begin(), histogram.end(), 0);\n> -\tconst uint64_t offset = blackLevel * nPixels;\n> -\tconst uint64_t sumR = stats_->sumR_ - offset / 4;\n> -\tconst uint64_t sumG = stats_->sumG_ - offset / 2;\n> -\tconst uint64_t sumB = stats_->sumB_ - offset / 4;\n> -\n> -\t/*\n> -\t * Calculate red and blue gains for AWB.\n> -\t * Clamp max gain at 4.0, this also avoids 0 division.\n> -\t * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n> -\t */\n> -\tconst unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> -\tconst unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n> -\t/* Green gain and gamma values are fixed */\n> -\tconstexpr unsigned int gainG = 256;\n> -\n> -\t/* Update the gamma table if needed */\n> -\tif (blackLevel != lastBlackLevel_) {\n> -\t\tconstexpr float gamma = 0.5;\n> -\t\tconst unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;\n> -\t\tstd::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);\n> -\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n> -\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n> -\t\t\tgammaTable_[i] = UINT8_MAX *\n> -\t\t\t\t\t std::pow((i - blackIndex) / divisor, gamma);\n> -\n> -\t\tlastBlackLevel_ = blackLevel;\n> -\t}\n> -\n> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> -\t\tconstexpr unsigned int div =\n> -\t\t\tDebayerParams::kRGBLookupSize * 256 / kGammaLookupSize;\n> -\t\tunsigned int idx;\n> -\n> -\t\t/* Apply gamma after gain! */\n> -\t\tidx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });\n> -\t\tparams_->red[i] = gammaTable_[idx];\n> -\n> -\t\tidx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });\n> -\t\tparams_->green[i] = gammaTable_[idx];\n> -\n> -\t\tidx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });\n> -\t\tparams_->blue[i] = gammaTable_[idx];\n> -\t}\n> -\n> -\tsetIspParams.emit();\n> -\n> \t/* \\todo Switch to the libipa/algorithm.h API someday. */\n> \n> \t/*\n> @@ -372,6 +312,7 @@ void IPASoftSimple::processStats(const uint32_t frame,\n> \t * Calculate Mean Sample Value (MSV) according to formula from:\n> \t * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> \t */\n> +\tconst uint8_t blackLevel = context_.activeState.black.level;\n> \tconst unsigned int blackLevelHistIdx =\n> \t\tblackLevel / (256 / SwIspStats::kYHistogramSize);\n> \tconst unsigned int histogramSize =\n> @@ -421,7 +362,8 @@ void IPASoftSimple::processStats(const uint32_t frame,\n> \n> \tLOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n> \t\t\t << \" exp \" << exposure_ << \" again \" << again_\n> -\t\t\t << \" gain R/B \" << gainR << \"/\" << gainB\n> +\t\t\t << \" gain R/B \" << context_.activeState.gains.red\n> +\t\t\t << \"/\" << context_.activeState.gains.blue\n\nThat's already printed in Awb::process(), does it need to be duplicated\nhere ?\n\n> \t\t\t << \" black level \" << static_cast(blackLevel);\n> }\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 A6253C323E\n\tfor ;\n\tSun, 1 Sep 2024 21:47:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB53E63469;\n\tSun, 1 Sep 2024 23:47:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E246363466\n\tfor ;\n\tSun, 1 Sep 2024 23:47:52 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4CD774CF;\n\tSun, 1 Sep 2024 23:46:41 +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=\"bCwuPxj1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725227202;\n\tbh=YgtE9EAf/9sy77dcbZXrpCnloTFHYEsjcfomGN8lo8I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bCwuPxj1ajXvlznXl3FX34r4oe2QANzUcWzGaoCmQiYWQ69hfJ5afsIInUYPTWEUq\n\to7UL0xyqdHgKyY57/VK6RidjedG7U6CSLGgoT57Bs7EQ0YD2mvVKe0FOFLhkjftYjO\n\t9KlRgKppcyemJaFZmxRsreBWj72UoHkSQK9+ylRk=","Date":"Mon, 2 Sep 2024 00:47:20 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tUmang Jain ,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 14/18] libcamera: software_isp: Move color handling to\n\tan algorithm module","Message-ID":"<20240901214720.GC31148@pendragon.ideasonboard.com>","References":"<20240830072554.180672-1-mzamazal@redhat.com>\n\t<20240830072554.180672-15-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240830072554.180672-15-mzamazal@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31101,"web_url":"https://patchwork.libcamera.org/comment/31101/","msgid":"<87frqdysho.fsf@redhat.com>","date":"2024-09-06T10:41:07","subject":"Re: [PATCH v5 14/18] libcamera: software_isp: Move color handling\n\tto an algorithm module","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nthank you for review.\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Fri, Aug 30, 2024 at 09:25:50AM +0200, Milan Zamazal wrote:\n>> After black level handling has been moved to an algorithm module, white\n>> balance and the construction of color tables can be moved to algorithm\n>> modules too.\n>> \n>> This time, the moved code is split between stats processing and\n>> parameter construction methods. It is also split to three algorithm\n>> modules:\n>> \n>> - Gamma table computation. This is actually independent of color\n>> handling.\n>> \n>> - White balance computation.\n>> \n>> - Color lookup tables construction. While this applies the color gains\n>> computed by the white balance algorithm, it is not directly related to\n>> white balance. We may want to modify the color lookup tables in\n>> future according to other parameters than just gamma and white balance\n>> gains.\n>> \n>> This is the only part of the software ISP algorithms that sets the\n>> parameters so emitting setIspParams can be moved to prepare() method.\n>> \n>> A more natural representation of the gains (and black level) would be\n>> floating point numbers. This is not done here in order to minimize\n>> changes in code movements. It will be addressed in a followup patch.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/ipa/simple/algorithms/awb.cpp | 70 +++++++++++++++++++++++++++\n>> src/ipa/simple/algorithms/awb.h | 38 +++++++++++++++\n>> src/ipa/simple/algorithms/gamma.cpp | 64 ++++++++++++++++++++++++\n>> src/ipa/simple/algorithms/gamma.h | 42 ++++++++++++++++\n>> src/ipa/simple/algorithms/lut.cpp | 57 ++++++++++++++++++++++\n>> src/ipa/simple/algorithms/lut.h | 37 ++++++++++++++\n>> src/ipa/simple/algorithms/meson.build | 3 ++\n>> src/ipa/simple/data/uncalibrated.yaml | 3 ++\n>> src/ipa/simple/ipa_context.cpp | 30 ++++++++++++\n>> src/ipa/simple/ipa_context.h | 12 +++++\n>> src/ipa/simple/soft_simple.cpp | 66 ++-----------------------\n>> 11 files changed, 360 insertions(+), 62 deletions(-)\n>> create mode 100644 src/ipa/simple/algorithms/awb.cpp\n>> create mode 100644 src/ipa/simple/algorithms/awb.h\n>> create mode 100644 src/ipa/simple/algorithms/gamma.cpp\n>> create mode 100644 src/ipa/simple/algorithms/gamma.h\n>> create mode 100644 src/ipa/simple/algorithms/lut.cpp\n>> create mode 100644 src/ipa/simple/algorithms/lut.h\n>> \n>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp\n>> new file mode 100644\n>> index 00000000..4b49996a\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/awb.cpp\n>> @@ -0,0 +1,70 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Red Hat Inc.\n>> + *\n>> + * Auto white balance\n>> + */\n>> +\n>> +#include \"awb.h\"\n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \"simple/ipa_context.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(IPASoftAwb)\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +int Awb::init(IPAContext &context,\n>> +\t [[maybe_unused]] const YamlObject &tuningData)\n>> +{\n>> +\tauto &gains = context.activeState.gains;\n>> +\tgains.red = gains.green = gains.blue = 256;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +void Awb::process(IPAContext &context,\n>> +\t\t [[maybe_unused]] const uint32_t frame,\n>> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n>> +\t\t const SwIspStats *stats,\n>> +\t\t [[maybe_unused]] ControlList &metadata)\n>> +{\n>> +\tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n>> +\tconst uint8_t blackLevel = context.activeState.black.level;\n>> +\n>> +\t/*\n>> +\t * Black level must be subtracted to get the correct AWB ratios, they\n>> +\t * would be off if they were computed from the whole brightness range\n>> +\t * rather than from the sensor range.\n>> +\t */\n>> +\tconst uint64_t nPixels = std::accumulate(\n>> +\t\thistogram.begin(), histogram.end(), 0);\n>> +\tconst uint64_t offset = blackLevel * nPixels;\n>> +\tconst uint64_t sumR = stats->sumR_ - offset / 4;\n>> +\tconst uint64_t sumG = stats->sumG_ - offset / 2;\n>> +\tconst uint64_t sumB = stats->sumB_ - offset / 4;\n>> +\n>> +\t/*\n>> +\t * Calculate red and blue gains for AWB.\n>> +\t * Clamp max gain at 4.0, this also avoids 0 division.\n>> +\t * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n>> +\t */\n>> +\tauto &gains = context.activeState.gains;\n>> +\tgains.red = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n>> +\tgains.blue = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n>> +\t/* Green gain is fixed to 256 */\n>> +\n>> +\tLOG(IPASoftAwb, Debug) << \"gain R/B \" << gains.red << \"/\" << gains.blue;\n>> +}\n>> +\n>> +REGISTER_IPA_ALGORITHM(Awb, \"Awb\")\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h\n>> new file mode 100644\n>> index 00000000..c0b88bec\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/awb.h\n>> @@ -0,0 +1,38 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Red Hat Inc.\n>> + *\n>> + * Auto white balance\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n>> +\n>> +#include \"algorithm.h\"\n>> +#include \"ipa_context.h\"\n>\n> As with the previous patch, I think you can drop all the includes except\n> algorithm.h. Same for the other algorithms.\n>\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +class Awb : public Algorithm\n>> +{\n>> +public:\n>> +\tAwb() = default;\n>> +\t~Awb() = default;\n>> +\n>> +\tint init(IPAContext &context, const YamlObject &tuningData)\n>> +\t\toverride;\n>\n> No need to split this on two lines. Same below.\n>\n>> +\tvoid process(IPAContext &context,\n>> +\t\t const uint32_t frame,\n>> +\t\t IPAFrameContext &frameContext,\n>> +\t\t const SwIspStats *stats,\n>> +\t\t ControlList &metadata) override;\n>> +};\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/gamma.cpp b/src/ipa/simple/algorithms/gamma.cpp\n>> new file mode 100644\n>> index 00000000..0b8ec5ee\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/gamma.cpp\n>> @@ -0,0 +1,64 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Red Hat Inc.\n>> + *\n>> + * Gamma table construction\n>> + */\n>> +\n>> +#include \"gamma.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \"simple/ipa_context.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +int Gamma::init(IPAContext &context,\n>> +\t\t[[maybe_unused]] const YamlObject &tuningData)\n>> +{\n>> +\t/* Gamma value is fixed */\n>> +\tcontext.configuration.gamma = 0.5;\n>> +\tupdateGammaTable(context);\n>\n> Same question as before, does this belong to .init() ? Actually, given\n> that prepare() will always be called to calculate ISP parameters, do you\n> even need to initialize the gamme table ?\n\nThe update of the gamma table in prepare() is conditional and I wouldn't\nrely on the initial values of the variables used in the condition;\nmaking sure the table is initialized is safer and it doesn't harm to do\nit in init(). Once the gamma table is initialized, we can rely on the\nconditional update in prepare(), so there is no need to update the gamma\ntable in configure() again.\n\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +void Gamma::updateGammaTable(IPAContext &context)\n>> +{\n>> +\tauto &gammaTable = context.activeState.gamma.gammaTable;\n>> +\tauto blackLevel = context.activeState.black.level;\n>> +\tconst unsigned int blackIndex =\n>> +\t\tblackLevel * IPAActiveState::kGammaLookupSize / 256;\n>\n> Use gammaTable.size() instead of kGammaLookupSize.\n>\n>> +\tstd::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n>> +\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n>> +\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n>\n> Same here.\n>\n>> +\t\tgammaTable[i] = UINT8_MAX * powf((i - blackIndex) / divisor,\n>\n> std::powf\n\nstd::pow actually, I suppose.\n\n>> +\t\t\t\t\t\t context.configuration.gamma);\n>> +\tcontext.activeState.gamma.blackLevel = blackLevel;\n>> +}\n>> +\n>> +void Gamma::prepare(IPAContext &context,\n>> +\t\t [[maybe_unused]] const uint32_t frame,\n>> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n>> +\t\t [[maybe_unused]] DebayerParams *params)\n>> +{\n>> +\t/*\n>> +\t * Update the gamma table if needed. This means if black level changes and\n>> +\t * since the black level gets updated only if a lower value is observed,\n>> +\t * it's not permanently prone to minor fluctuations or rounding errors.\n>\n> Please reflow to 80 columns.\n>\n>> +\t */\n>> +\tif (context.activeState.gamma.blackLevel != context.activeState.black.level)\n>> +\t\tupdateGammaTable(context);\n>> +}\n>> +\n>> +REGISTER_IPA_ALGORITHM(Gamma, \"Gamma\")\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/gamma.h b/src/ipa/simple/algorithms/gamma.h\n>> new file mode 100644\n>> index 00000000..43e88e87\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/gamma.h\n>> @@ -0,0 +1,42 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Red Hat Inc.\n>> + *\n>> + * Gamma table construction\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>> +\n>> +#include \"algorithm.h\"\n>> +#include \"ipa_context.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +static constexpr unsigned int kGammaLookupSize = 1024;\n>> +\n>> +class Gamma : public Algorithm\n>> +{\n>> +public:\n>> +\tGamma() = default;\n>> +\t~Gamma() = default;\n>> +\n>> +\tint init(IPAContext &context, const YamlObject &tuningData)\n>> +\t\toverride;\n>> +\tvoid prepare(IPAContext &context,\n>> +\t\t const uint32_t frame,\n>> +\t\t IPAFrameContext &frameContext,\n>> +\t\t DebayerParams *params) override;\n>> +\n>> +private:\n>> +\tvoid updateGammaTable(IPAContext &context);\n>> +};\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n>> new file mode 100644\n>> index 00000000..748f10aa\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/lut.cpp\n>> @@ -0,0 +1,57 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Red Hat Inc.\n>> + *\n>> + * Lookup table construction\n>> + */\n>> +\n>> +#include \"lut.h\"\n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \"simple/ipa_context.h\"\n>> +\n>> +#include \"gamma.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +int Lut::init(IPAContext &context,\n>> +\t [[maybe_unused]] const YamlObject &tuningData)\n>> +{\n>> +\tauto &gains = context.activeState.gains;\n>> +\tgains.red = gains.green = gains.blue = 1.0;\n>\n> That looks wrong, the Awb algorithm does this already.\n\nIndeed, it doesn't belong here.\n\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +void Lut::prepare(IPAContext &context,\n>> +\t\t [[maybe_unused]] const uint32_t frame,\n>> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n>> +\t\t DebayerParams *params)\n>> +{\n>> +\tauto &gains = context.activeState.gains;\n>> +\tauto &gammaTable = context.activeState.gamma.gammaTable;\n>> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\tconstexpr unsigned int div =\n>> +\t\t\tstatic_cast(DebayerParams::kRGBLookupSize) * 256 / kGammaLookupSize;\n>> +\t\t/* Apply gamma after gain! */\n>> +\t\tunsigned int idx;\n>> +\t\tidx = std::min({ i * gains.red / div, kGammaLookupSize - 1 });\n>> +\t\tparams->red[i] = gammaTable[idx];\n>> +\t\tidx = std::min({ i * gains.green / div, kGammaLookupSize - 1 });\n>> +\t\tparams->green[i] = gammaTable[idx];\n>> +\t\tidx = std::min({ i * gains.blue / div, kGammaLookupSize - 1 });\n>> +\t\tparams->blue[i] = gammaTable[idx];\n>> +\t}\n>\n> Why is this a separate algorithm, why isn't it part of Gamma ?\n\nWell, constructing the gamma table and the color lookup tables are two\ndifferent things. While the gammaTable looks like a utility just for\nthe color lookup tables here, it may not always be the case, for\ninstance contrast adjustment may be implemented by modifying the gamma\ntable. \n\nBut I don't have a strong opinion on this and I can merge the two.\n\n>> +}\n>> +\n>> +REGISTER_IPA_ALGORITHM(Lut, \"Lut\")\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n>> new file mode 100644\n>> index 00000000..5b5e554e\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/lut.h\n>> @@ -0,0 +1,37 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Red Hat Inc.\n>> + *\n>> + * Lookup table construction\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>> +\n>> +#include \"algorithm.h\"\n>> +#include \"ipa_context.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +class Lut : public Algorithm\n>> +{\n>> +public:\n>> +\tLut() = default;\n>> +\t~Lut() = default;\n>> +\n>> +\tint init(IPAContext &context, const YamlObject &tuningData)\n>> +\t\toverride;\n>> +\tvoid prepare(IPAContext &context,\n>> +\t\t const uint32_t frame,\n>> +\t\t IPAFrameContext &frameContext,\n>> +\t\t DebayerParams *params) override;\n>> +};\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n>> index 1f63c220..68e5aa58 100644\n>> --- a/src/ipa/simple/algorithms/meson.build\n>> +++ b/src/ipa/simple/algorithms/meson.build\n>> @@ -1,5 +1,8 @@\n>> # SPDX-License-Identifier: CC0-1.0\n>> \n>> soft_simple_ipa_algorithms = files([\n>> + 'awb.cpp',\n>> 'blc.cpp',\n>> + 'gamma.cpp',\n>> + 'lut.cpp',\n>> ])\n>> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n>> index e0d03d96..deaea6b1 100644\n>> --- a/src/ipa/simple/data/uncalibrated.yaml\n>> +++ b/src/ipa/simple/data/uncalibrated.yaml\n>> @@ -4,4 +4,7 @@\n>> version: 1\n>> algorithms:\n>> - BlackLevel:\n>> + - Gamma:\n>> + - Awb:\n>> + - Lut:\n>> ...\n>> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp\n>> index 268cff32..5fa492cb 100644\n>> --- a/src/ipa/simple/ipa_context.cpp\n>> +++ b/src/ipa/simple/ipa_context.cpp\n>> @@ -50,6 +50,11 @@ namespace libcamera::ipa::soft {\n>> * \\brief The current state of IPA algorithms\n>> */\n>> \n>> +/**\n>> + * \\var IPASessionConfiguration::gamma\n>> + * \\brief Gamma value to be used in the raw image processing\n>> + */\n>> +\n>> /**\n>> * \\var IPAActiveState::black\n>> * \\brief Context for the Black Level algorithm\n>> @@ -58,4 +63,29 @@ namespace libcamera::ipa::soft {\n>> * \\brief Current determined black level\n>> */\n>> \n>> +/**\n>> + * \\var IPAActiveState::gains\n>> + * \\brief Context for gains in the Colors algorithm\n>> + *\n>> + * \\var IPAActiveState::gains.red\n>> + * \\brief Gain of red color\n>> + *\n>> + * \\var IPAActiveState::gains.green\n>> + * \\brief Gain of green color\n>> + *\n>> + * \\var IPAActiveState::gains.blue\n>> + * \\brief Gain of blue color\n>> + */\n>> +\n>> +/**\n>> + * \\var IPAActiveState::gamma\n>> + * \\brief Context for gamma in the Colors algorithm\n>> + *\n>> + * \\var IPAActiveState::gamma.gammaTable\n>> + * \\brief Computed gamma table\n>> + *\n>> + * \\var IPAActiveState::gamma.blackLevel\n>> + * \\brief Black level used for the gamma table computation\n>> + */\n>> +\n>> } /* namespace libcamera::ipa::soft */\n>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> index 22156569..f8b8834d 100644\n>> --- a/src/ipa/simple/ipa_context.h\n>> +++ b/src/ipa/simple/ipa_context.h\n>> @@ -7,6 +7,7 @@\n>> \n>> #pragma once\n>> \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -16,12 +17,23 @@ namespace libcamera {\n>> namespace ipa::soft {\n>> \n>> struct IPASessionConfiguration {\n>> +\tfloat gamma;\n>> };\n>> \n>> struct IPAActiveState {\n>> \tstruct {\n>> \t\tuint8_t level;\n>> \t} black;\n>\n> Blank line. Same below.\n>\n>> +\tstruct {\n>> +\t\tunsigned int red;\n>> +\t\tunsigned int green;\n>> +\t\tunsigned int blue;\n>> +\t} gains;\n>> +\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n>\n> This duplicates the kGammaLookupSize from\n> src/ipa/simple/algorithms/gamma.h. \n\nYes. The constant in gamma.h is actually not needed when replacing it\nwith size() calls, I'll remove it.\n\n>> +\tstruct {\n>> +\t\tstd::array gammaTable;\n>> +\t\tuint8_t blackLevel;\n>> +\t} gamma;\n>> };\n>> \n>> struct IPAFrameContext : public FrameContext {\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index a1bd2f0c..8f01bf7d 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -5,8 +5,6 @@\n>> * Simple Software Image Processing Algorithm module\n>> */\n>> \n>> -#include \n>> -#include \n>> #include \n>> #include \n>> \n>> @@ -93,9 +91,6 @@ private:\n>> \tstd::unique_ptr camHelper_;\n>> \tControlInfoMap sensorInfoMap_;\n>> \n>> -\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n>> -\tstd::array gammaTable_;\n>> -\tint lastBlackLevel_ = -1;\n>> \t/* Local parameter storage */\n>> \tstruct IPAContext context_;\n>> \n>> @@ -283,6 +278,7 @@ void IPASoftSimple::prepare(const uint32_t frame)\n>> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>> \tfor (auto const &algo : algorithms())\n>> \t\talgo->prepare(context_, frame, frameContext, params_);\n>> +\tsetIspParams.emit();\n>> }\n>> \n>> void IPASoftSimple::processStats(const uint32_t frame,\n>> @@ -300,62 +296,6 @@ void IPASoftSimple::processStats(const uint32_t frame,\n>> \tfor (auto const &algo : algorithms())\n>> \t\talgo->process(context_, frame, frameContext, stats_, metadata);\n>> \n>> -\tSwIspStats::Histogram histogram = stats_->yHistogram;\n>> -\tconst uint8_t blackLevel = context_.activeState.black.level;\n>> -\n>> -\t/*\n>> -\t * Black level must be subtracted to get the correct AWB ratios, they\n>> -\t * would be off if they were computed from the whole brightness range\n>> -\t * rather than from the sensor range.\n>> -\t */\n>> -\tconst uint64_t nPixels = std::accumulate(\n>> -\t\thistogram.begin(), histogram.end(), 0);\n>> -\tconst uint64_t offset = blackLevel * nPixels;\n>> -\tconst uint64_t sumR = stats_->sumR_ - offset / 4;\n>> -\tconst uint64_t sumG = stats_->sumG_ - offset / 2;\n>> -\tconst uint64_t sumB = stats_->sumB_ - offset / 4;\n>> -\n>> -\t/*\n>> -\t * Calculate red and blue gains for AWB.\n>> -\t * Clamp max gain at 4.0, this also avoids 0 division.\n>> -\t * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n>> -\t */\n>> -\tconst unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n>> -\tconst unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n>> -\t/* Green gain and gamma values are fixed */\n>> -\tconstexpr unsigned int gainG = 256;\n>> -\n>> -\t/* Update the gamma table if needed */\n>> -\tif (blackLevel != lastBlackLevel_) {\n>> -\t\tconstexpr float gamma = 0.5;\n>> -\t\tconst unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;\n>> -\t\tstd::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);\n>> -\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n>> -\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n>> -\t\t\tgammaTable_[i] = UINT8_MAX *\n>> -\t\t\t\t\t std::pow((i - blackIndex) / divisor, gamma);\n>> -\n>> -\t\tlastBlackLevel_ = blackLevel;\n>> -\t}\n>> -\n>> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> -\t\tconstexpr unsigned int div =\n>> -\t\t\tDebayerParams::kRGBLookupSize * 256 / kGammaLookupSize;\n>> -\t\tunsigned int idx;\n>> -\n>> -\t\t/* Apply gamma after gain! */\n>> -\t\tidx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });\n>> -\t\tparams_->red[i] = gammaTable_[idx];\n>> -\n>> -\t\tidx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });\n>> -\t\tparams_->green[i] = gammaTable_[idx];\n>> -\n>> -\t\tidx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });\n>> -\t\tparams_->blue[i] = gammaTable_[idx];\n>> -\t}\n>> -\n>> -\tsetIspParams.emit();\n>> -\n>> \t/* \\todo Switch to the libipa/algorithm.h API someday. */\n>> \n>> \t/*\n>> @@ -372,6 +312,7 @@ void IPASoftSimple::processStats(const uint32_t frame,\n>> \t * Calculate Mean Sample Value (MSV) according to formula from:\n>> \t * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n>> \t */\n>> +\tconst uint8_t blackLevel = context_.activeState.black.level;\n>> \tconst unsigned int blackLevelHistIdx =\n>> \t\tblackLevel / (256 / SwIspStats::kYHistogramSize);\n>> \tconst unsigned int histogramSize =\n>> @@ -421,7 +362,8 @@ void IPASoftSimple::processStats(const uint32_t frame,\n>> \n>> \tLOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n>> \t\t\t << \" exp \" << exposure_ << \" again \" << again_\n>> -\t\t\t << \" gain R/B \" << gainR << \"/\" << gainB\n>> +\t\t\t << \" gain R/B \" << context_.activeState.gains.red\n>> +\t\t\t << \"/\" << context_.activeState.gains.blue\n>\n> That's already printed in Awb::process(), does it need to be duplicated\n> here ?\n\nI don't think so, I'll remove it here.\n\n>> \t\t\t << \" black level \" << static_cast(blackLevel);\n>> }\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 257CCC324C\n\tfor ;\n\tFri, 6 Sep 2024 10:41:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AAFE2634E5;\n\tFri, 6 Sep 2024 12:41:16 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C25C633CC\n\tfor ;\n\tFri, 6 Sep 2024 12:41:14 +0200 (CEST)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-512-5Irs489lOZCcpinRtvXOaw-1; Fri, 06 Sep 2024 06:41:11 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-374b385b146so861725f8f.0\n\tfor ;\n\tFri, 06 Sep 2024 03:41:11 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-42ca05cc3dbsm16123065e9.20.2024.09.06.03.41.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 06 Sep 2024 03:41:08 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"JhPBXZnO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1725619273;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=JtY1qhWnnzVEWbqMhu5t0ZriEZdTkQTCMuiIz67IG2A=;\n\tb=JhPBXZnOKV8zk6m37oJQ8ZRtw7qBl+6S9zC6/j3WefgSgJ6tsKg41Zn7sZHTRP3zDyAKwg\n\tNXodlVwmf3qOAWL1Io9L+DJGvsJlDYiGsIHzjTDxcffQXyN28SYCP+eioWPJNX8q95M58k\n\tGSPCbxmmKa+dICkvnuFrjYH5pLMk3aA=","X-MC-Unique":"5Irs489lOZCcpinRtvXOaw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725619270; x=1726224070;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=JtY1qhWnnzVEWbqMhu5t0ZriEZdTkQTCMuiIz67IG2A=;\n\tb=SK1A2qmGJWbAhsX+mMH3bd8nsPrWznX6CypfijCtmHth8I9ssl46kcPeHIDxhcLZAq\n\tINzBEKKha0vF0D+ZyUIK6ls5MC6au2Wa1v+ofCvsBunMHBHgl31J0wDwDmsUQSpVXIBq\n\tspoE7jnghj5Tg6ou4AZYY+R3Hxew5LLWE9m3Ts3vAttCIGA5e8MTzI05WJIBd5hd+O2r\n\t/YqKEWuMdY/7gGbO7KjU2YRa2bisnYav872Ha85n6oeH1+IX7INTYiFfz9hSHxKzOLyu\n\tgqRMNWB4F/42HBiD2Q/nIrrjTQuu9he0zNC7iTq03m1udEFo0flbnMQhHlm377JqBjHC\n\taBVQ==","X-Gm-Message-State":"AOJu0Ywgz5WS5qBWxeBd1TRxq7/zwCg1ACJBnRsBl+nVw8jXFmrtxEx7\n\tph0KQdeFlbaib5Qn8zUlgcwrN/f1S+htMJtQ+KhwDXs8i2A2MqqCEu7n4/DEVBhvt0LJTucyqk5\n\tv/kfGCbtxeolkJ+/UskceTqIrRdO1a6ilYRlFgjiHxHXZCDQjmzGiO21XwquaFK+BjqAxisTF22\n\tU308o=","X-Received":["by 2002:a5d:68c1:0:b0:376:37e:2729 with SMTP id\n\tffacd0b85a97d-378895de760mr1252048f8f.31.1725619269886; \n\tFri, 06 Sep 2024 03:41:09 -0700 (PDT)","by 2002:a5d:68c1:0:b0:376:37e:2729 with SMTP id\n\tffacd0b85a97d-378895de760mr1252029f8f.31.1725619269065; \n\tFri, 06 Sep 2024 03:41:09 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEwArwp80a2Xc2Bw/Ffb+7D84b53Ap3bHAyeJ5Kz4T9wRwgi5fTt/rtIvrPtLHk5gFfev1uvw==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Umang Jain\n\t,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 14/18] libcamera: software_isp: Move color handling\n\tto an algorithm module","In-Reply-To":"<20240901214720.GC31148@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Mon, 2 Sep 2024 00:47:20 +0300\")","References":"<20240830072554.180672-1-mzamazal@redhat.com>\n\t<20240830072554.180672-15-mzamazal@redhat.com>\n\t<20240901214720.GC31148@pendragon.ideasonboard.com>","Date":"Fri, 06 Sep 2024 12:41:07 +0200","Message-ID":"<87frqdysho.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":31104,"web_url":"https://patchwork.libcamera.org/comment/31104/","msgid":"<20240906130227.GB27086@pendragon.ideasonboard.com>","date":"2024-09-06T13:02:27","subject":"Re: [PATCH v5 14/18] libcamera: software_isp: Move color handling to\n\tan algorithm module","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 06, 2024 at 12:41:07PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Fri, Aug 30, 2024 at 09:25:50AM +0200, Milan Zamazal wrote:\n> >> After black level handling has been moved to an algorithm module, white\n> >> balance and the construction of color tables can be moved to algorithm\n> >> modules too.\n> >> \n> >> This time, the moved code is split between stats processing and\n> >> parameter construction methods. It is also split to three algorithm\n> >> modules:\n> >> \n> >> - Gamma table computation. This is actually independent of color\n> >> handling.\n> >> \n> >> - White balance computation.\n> >> \n> >> - Color lookup tables construction. While this applies the color gains\n> >> computed by the white balance algorithm, it is not directly related to\n> >> white balance. We may want to modify the color lookup tables in\n> >> future according to other parameters than just gamma and white balance\n> >> gains.\n> >> \n> >> This is the only part of the software ISP algorithms that sets the\n> >> parameters so emitting setIspParams can be moved to prepare() method.\n> >> \n> >> A more natural representation of the gains (and black level) would be\n> >> floating point numbers. This is not done here in order to minimize\n> >> changes in code movements. It will be addressed in a followup patch.\n> >> \n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> src/ipa/simple/algorithms/awb.cpp | 70 +++++++++++++++++++++++++++\n> >> src/ipa/simple/algorithms/awb.h | 38 +++++++++++++++\n> >> src/ipa/simple/algorithms/gamma.cpp | 64 ++++++++++++++++++++++++\n> >> src/ipa/simple/algorithms/gamma.h | 42 ++++++++++++++++\n> >> src/ipa/simple/algorithms/lut.cpp | 57 ++++++++++++++++++++++\n> >> src/ipa/simple/algorithms/lut.h | 37 ++++++++++++++\n> >> src/ipa/simple/algorithms/meson.build | 3 ++\n> >> src/ipa/simple/data/uncalibrated.yaml | 3 ++\n> >> src/ipa/simple/ipa_context.cpp | 30 ++++++++++++\n> >> src/ipa/simple/ipa_context.h | 12 +++++\n> >> src/ipa/simple/soft_simple.cpp | 66 ++-----------------------\n> >> 11 files changed, 360 insertions(+), 62 deletions(-)\n> >> create mode 100644 src/ipa/simple/algorithms/awb.cpp\n> >> create mode 100644 src/ipa/simple/algorithms/awb.h\n> >> create mode 100644 src/ipa/simple/algorithms/gamma.cpp\n> >> create mode 100644 src/ipa/simple/algorithms/gamma.h\n> >> create mode 100644 src/ipa/simple/algorithms/lut.cpp\n> >> create mode 100644 src/ipa/simple/algorithms/lut.h\n> >> \n> >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp\n> >> new file mode 100644\n> >> index 00000000..4b49996a\n> >> --- /dev/null\n> >> +++ b/src/ipa/simple/algorithms/awb.cpp\n> >> @@ -0,0 +1,70 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2024, Red Hat Inc.\n> >> + *\n> >> + * Auto white balance\n> >> + */\n> >> +\n> >> +#include \"awb.h\"\n> >> +\n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +#include \"simple/ipa_context.h\"\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +LOG_DEFINE_CATEGORY(IPASoftAwb)\n> >> +\n> >> +namespace ipa::soft::algorithms {\n> >> +\n> >> +int Awb::init(IPAContext &context,\n> >> +\t [[maybe_unused]] const YamlObject &tuningData)\n> >> +{\n> >> +\tauto &gains = context.activeState.gains;\n> >> +\tgains.red = gains.green = gains.blue = 256;\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +void Awb::process(IPAContext &context,\n> >> +\t\t [[maybe_unused]] const uint32_t frame,\n> >> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> >> +\t\t const SwIspStats *stats,\n> >> +\t\t [[maybe_unused]] ControlList &metadata)\n> >> +{\n> >> +\tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n> >> +\tconst uint8_t blackLevel = context.activeState.black.level;\n> >> +\n> >> +\t/*\n> >> +\t * Black level must be subtracted to get the correct AWB ratios, they\n> >> +\t * would be off if they were computed from the whole brightness range\n> >> +\t * rather than from the sensor range.\n> >> +\t */\n> >> +\tconst uint64_t nPixels = std::accumulate(\n> >> +\t\thistogram.begin(), histogram.end(), 0);\n> >> +\tconst uint64_t offset = blackLevel * nPixels;\n> >> +\tconst uint64_t sumR = stats->sumR_ - offset / 4;\n> >> +\tconst uint64_t sumG = stats->sumG_ - offset / 2;\n> >> +\tconst uint64_t sumB = stats->sumB_ - offset / 4;\n> >> +\n> >> +\t/*\n> >> +\t * Calculate red and blue gains for AWB.\n> >> +\t * Clamp max gain at 4.0, this also avoids 0 division.\n> >> +\t * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n> >> +\t */\n> >> +\tauto &gains = context.activeState.gains;\n> >> +\tgains.red = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> >> +\tgains.blue = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n> >> +\t/* Green gain is fixed to 256 */\n> >> +\n> >> +\tLOG(IPASoftAwb, Debug) << \"gain R/B \" << gains.red << \"/\" << gains.blue;\n> >> +}\n> >> +\n> >> +REGISTER_IPA_ALGORITHM(Awb, \"Awb\")\n> >> +\n> >> +} /* namespace ipa::soft::algorithms */\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h\n> >> new file mode 100644\n> >> index 00000000..c0b88bec\n> >> --- /dev/null\n> >> +++ b/src/ipa/simple/algorithms/awb.h\n> >> @@ -0,0 +1,38 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2024, Red Hat Inc.\n> >> + *\n> >> + * Auto white balance\n> >> + */\n> >> +\n> >> +#pragma once\n> >> +\n> >> +#include \n> >> +\n> >> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n> >> +\n> >> +#include \"algorithm.h\"\n> >> +#include \"ipa_context.h\"\n> >\n> > As with the previous patch, I think you can drop all the includes except\n> > algorithm.h. Same for the other algorithms.\n> >\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa::soft::algorithms {\n> >> +\n> >> +class Awb : public Algorithm\n> >> +{\n> >> +public:\n> >> +\tAwb() = default;\n> >> +\t~Awb() = default;\n> >> +\n> >> +\tint init(IPAContext &context, const YamlObject &tuningData)\n> >> +\t\toverride;\n> >\n> > No need to split this on two lines. Same below.\n> >\n> >> +\tvoid process(IPAContext &context,\n> >> +\t\t const uint32_t frame,\n> >> +\t\t IPAFrameContext &frameContext,\n> >> +\t\t const SwIspStats *stats,\n> >> +\t\t ControlList &metadata) override;\n> >> +};\n> >> +\n> >> +} /* namespace ipa::soft::algorithms */\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/ipa/simple/algorithms/gamma.cpp b/src/ipa/simple/algorithms/gamma.cpp\n> >> new file mode 100644\n> >> index 00000000..0b8ec5ee\n> >> --- /dev/null\n> >> +++ b/src/ipa/simple/algorithms/gamma.cpp\n> >> @@ -0,0 +1,64 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2024, Red Hat Inc.\n> >> + *\n> >> + * Gamma table construction\n> >> + */\n> >> +\n> >> +#include \"gamma.h\"\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +#include \"simple/ipa_context.h\"\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa::soft::algorithms {\n> >> +\n> >> +int Gamma::init(IPAContext &context,\n> >> +\t\t[[maybe_unused]] const YamlObject &tuningData)\n> >> +{\n> >> +\t/* Gamma value is fixed */\n> >> +\tcontext.configuration.gamma = 0.5;\n> >> +\tupdateGammaTable(context);\n> >\n> > Same question as before, does this belong to .init() ? Actually, given\n> > that prepare() will always be called to calculate ISP parameters, do you\n> > even need to initialize the gamme table ?\n> \n> The update of the gamma table in prepare() is conditional and I wouldn't\n> rely on the initial values of the variables used in the condition;\n> making sure the table is initialized is safer and it doesn't harm to do\n> it in init(). Once the gamma table is initialized, we can rely on the\n> conditional update in prepare(), so there is no need to update the gamma\n> table in configure() again.\n\nThen I think it should be initialized in configure() instead, to avoid\nretaining state between capture sessions.\n\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +void Gamma::updateGammaTable(IPAContext &context)\n> >> +{\n> >> +\tauto &gammaTable = context.activeState.gamma.gammaTable;\n> >> +\tauto blackLevel = context.activeState.black.level;\n> >> +\tconst unsigned int blackIndex =\n> >> +\t\tblackLevel * IPAActiveState::kGammaLookupSize / 256;\n> >\n> > Use gammaTable.size() instead of kGammaLookupSize.\n> >\n> >> +\tstd::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n> >> +\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n> >> +\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n> >\n> > Same here.\n> >\n> >> +\t\tgammaTable[i] = UINT8_MAX * powf((i - blackIndex) / divisor,\n> >\n> > std::powf\n> \n> std::pow actually, I suppose.\n\nYes.\n\n> >> +\t\t\t\t\t\t context.configuration.gamma);\n> >> +\tcontext.activeState.gamma.blackLevel = blackLevel;\n> >> +}\n> >> +\n> >> +void Gamma::prepare(IPAContext &context,\n> >> +\t\t [[maybe_unused]] const uint32_t frame,\n> >> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> >> +\t\t [[maybe_unused]] DebayerParams *params)\n> >> +{\n> >> +\t/*\n> >> +\t * Update the gamma table if needed. This means if black level changes and\n> >> +\t * since the black level gets updated only if a lower value is observed,\n> >> +\t * it's not permanently prone to minor fluctuations or rounding errors.\n> >\n> > Please reflow to 80 columns.\n> >\n> >> +\t */\n> >> +\tif (context.activeState.gamma.blackLevel != context.activeState.black.level)\n> >> +\t\tupdateGammaTable(context);\n> >> +}\n> >> +\n> >> +REGISTER_IPA_ALGORITHM(Gamma, \"Gamma\")\n> >> +\n> >> +} /* namespace ipa::soft::algorithms */\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/ipa/simple/algorithms/gamma.h b/src/ipa/simple/algorithms/gamma.h\n> >> new file mode 100644\n> >> index 00000000..43e88e87\n> >> --- /dev/null\n> >> +++ b/src/ipa/simple/algorithms/gamma.h\n> >> @@ -0,0 +1,42 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2024, Red Hat Inc.\n> >> + *\n> >> + * Gamma table construction\n> >> + */\n> >> +\n> >> +#pragma once\n> >> +\n> >> +#include \n> >> +\n> >> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> >> +\n> >> +#include \"algorithm.h\"\n> >> +#include \"ipa_context.h\"\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa::soft::algorithms {\n> >> +\n> >> +static constexpr unsigned int kGammaLookupSize = 1024;\n> >> +\n> >> +class Gamma : public Algorithm\n> >> +{\n> >> +public:\n> >> +\tGamma() = default;\n> >> +\t~Gamma() = default;\n> >> +\n> >> +\tint init(IPAContext &context, const YamlObject &tuningData)\n> >> +\t\toverride;\n> >> +\tvoid prepare(IPAContext &context,\n> >> +\t\t const uint32_t frame,\n> >> +\t\t IPAFrameContext &frameContext,\n> >> +\t\t DebayerParams *params) override;\n> >> +\n> >> +private:\n> >> +\tvoid updateGammaTable(IPAContext &context);\n> >> +};\n> >> +\n> >> +} /* namespace ipa::soft::algorithms */\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> >> new file mode 100644\n> >> index 00000000..748f10aa\n> >> --- /dev/null\n> >> +++ b/src/ipa/simple/algorithms/lut.cpp\n> >> @@ -0,0 +1,57 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2024, Red Hat Inc.\n> >> + *\n> >> + * Lookup table construction\n> >> + */\n> >> +\n> >> +#include \"lut.h\"\n> >> +\n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +#include \"simple/ipa_context.h\"\n> >> +\n> >> +#include \"gamma.h\"\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa::soft::algorithms {\n> >> +\n> >> +int Lut::init(IPAContext &context,\n> >> +\t [[maybe_unused]] const YamlObject &tuningData)\n> >> +{\n> >> +\tauto &gains = context.activeState.gains;\n> >> +\tgains.red = gains.green = gains.blue = 1.0;\n> >\n> > That looks wrong, the Awb algorithm does this already.\n> \n> Indeed, it doesn't belong here.\n> \n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +void Lut::prepare(IPAContext &context,\n> >> +\t\t [[maybe_unused]] const uint32_t frame,\n> >> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> >> +\t\t DebayerParams *params)\n> >> +{\n> >> +\tauto &gains = context.activeState.gains;\n> >> +\tauto &gammaTable = context.activeState.gamma.gammaTable;\n> >> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> >> +\t\tconstexpr unsigned int div =\n> >> +\t\t\tstatic_cast(DebayerParams::kRGBLookupSize) * 256 / kGammaLookupSize;\n> >> +\t\t/* Apply gamma after gain! */\n> >> +\t\tunsigned int idx;\n> >> +\t\tidx = std::min({ i * gains.red / div, kGammaLookupSize - 1 });\n> >> +\t\tparams->red[i] = gammaTable[idx];\n> >> +\t\tidx = std::min({ i * gains.green / div, kGammaLookupSize - 1 });\n> >> +\t\tparams->green[i] = gammaTable[idx];\n> >> +\t\tidx = std::min({ i * gains.blue / div, kGammaLookupSize - 1 });\n> >> +\t\tparams->blue[i] = gammaTable[idx];\n> >> +\t}\n> >\n> > Why is this a separate algorithm, why isn't it part of Gamma ?\n> \n> Well, constructing the gamma table and the color lookup tables are two\n> different things. While the gammaTable looks like a utility just for\n> the color lookup tables here, it may not always be the case, for\n> instance contrast adjustment may be implemented by modifying the gamma\n> table. \n> \n> But I don't have a strong opinion on this and I can merge the two.\n\nI'd start simple and merge the two, and split them later if needed.\n\n> >> +}\n> >> +\n> >> +REGISTER_IPA_ALGORITHM(Lut, \"Lut\")\n> >> +\n> >> +} /* namespace ipa::soft::algorithms */\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n> >> new file mode 100644\n> >> index 00000000..5b5e554e\n> >> --- /dev/null\n> >> +++ b/src/ipa/simple/algorithms/lut.h\n> >> @@ -0,0 +1,37 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2024, Red Hat Inc.\n> >> + *\n> >> + * Lookup table construction\n> >> + */\n> >> +\n> >> +#pragma once\n> >> +\n> >> +#include \n> >> +\n> >> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> >> +\n> >> +#include \"algorithm.h\"\n> >> +#include \"ipa_context.h\"\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa::soft::algorithms {\n> >> +\n> >> +class Lut : public Algorithm\n> >> +{\n> >> +public:\n> >> +\tLut() = default;\n> >> +\t~Lut() = default;\n> >> +\n> >> +\tint init(IPAContext &context, const YamlObject &tuningData)\n> >> +\t\toverride;\n> >> +\tvoid prepare(IPAContext &context,\n> >> +\t\t const uint32_t frame,\n> >> +\t\t IPAFrameContext &frameContext,\n> >> +\t\t DebayerParams *params) override;\n> >> +};\n> >> +\n> >> +} /* namespace ipa::soft::algorithms */\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n> >> index 1f63c220..68e5aa58 100644\n> >> --- a/src/ipa/simple/algorithms/meson.build\n> >> +++ b/src/ipa/simple/algorithms/meson.build\n> >> @@ -1,5 +1,8 @@\n> >> # SPDX-License-Identifier: CC0-1.0\n> >> \n> >> soft_simple_ipa_algorithms = files([\n> >> + 'awb.cpp',\n> >> 'blc.cpp',\n> >> + 'gamma.cpp',\n> >> + 'lut.cpp',\n> >> ])\n> >> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n> >> index e0d03d96..deaea6b1 100644\n> >> --- a/src/ipa/simple/data/uncalibrated.yaml\n> >> +++ b/src/ipa/simple/data/uncalibrated.yaml\n> >> @@ -4,4 +4,7 @@\n> >> version: 1\n> >> algorithms:\n> >> - BlackLevel:\n> >> + - Gamma:\n> >> + - Awb:\n> >> + - Lut:\n> >> ...\n> >> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp\n> >> index 268cff32..5fa492cb 100644\n> >> --- a/src/ipa/simple/ipa_context.cpp\n> >> +++ b/src/ipa/simple/ipa_context.cpp\n> >> @@ -50,6 +50,11 @@ namespace libcamera::ipa::soft {\n> >> * \\brief The current state of IPA algorithms\n> >> */\n> >> \n> >> +/**\n> >> + * \\var IPASessionConfiguration::gamma\n> >> + * \\brief Gamma value to be used in the raw image processing\n> >> + */\n> >> +\n> >> /**\n> >> * \\var IPAActiveState::black\n> >> * \\brief Context for the Black Level algorithm\n> >> @@ -58,4 +63,29 @@ namespace libcamera::ipa::soft {\n> >> * \\brief Current determined black level\n> >> */\n> >> \n> >> +/**\n> >> + * \\var IPAActiveState::gains\n> >> + * \\brief Context for gains in the Colors algorithm\n> >> + *\n> >> + * \\var IPAActiveState::gains.red\n> >> + * \\brief Gain of red color\n> >> + *\n> >> + * \\var IPAActiveState::gains.green\n> >> + * \\brief Gain of green color\n> >> + *\n> >> + * \\var IPAActiveState::gains.blue\n> >> + * \\brief Gain of blue color\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var IPAActiveState::gamma\n> >> + * \\brief Context for gamma in the Colors algorithm\n> >> + *\n> >> + * \\var IPAActiveState::gamma.gammaTable\n> >> + * \\brief Computed gamma table\n> >> + *\n> >> + * \\var IPAActiveState::gamma.blackLevel\n> >> + * \\brief Black level used for the gamma table computation\n> >> + */\n> >> +\n> >> } /* namespace libcamera::ipa::soft */\n> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> >> index 22156569..f8b8834d 100644\n> >> --- a/src/ipa/simple/ipa_context.h\n> >> +++ b/src/ipa/simple/ipa_context.h\n> >> @@ -7,6 +7,7 @@\n> >> \n> >> #pragma once\n> >> \n> >> +#include \n> >> #include \n> >> \n> >> #include \n> >> @@ -16,12 +17,23 @@ namespace libcamera {\n> >> namespace ipa::soft {\n> >> \n> >> struct IPASessionConfiguration {\n> >> +\tfloat gamma;\n> >> };\n> >> \n> >> struct IPAActiveState {\n> >> \tstruct {\n> >> \t\tuint8_t level;\n> >> \t} black;\n> >\n> > Blank line. Same below.\n> >\n> >> +\tstruct {\n> >> +\t\tunsigned int red;\n> >> +\t\tunsigned int green;\n> >> +\t\tunsigned int blue;\n> >> +\t} gains;\n> >> +\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n> >\n> > This duplicates the kGammaLookupSize from\n> > src/ipa/simple/algorithms/gamma.h. \n> \n> Yes. The constant in gamma.h is actually not needed when replacing it\n> with size() calls, I'll remove it.\n> \n> >> +\tstruct {\n> >> +\t\tstd::array gammaTable;\n> >> +\t\tuint8_t blackLevel;\n> >> +\t} gamma;\n> >> };\n> >> \n> >> struct IPAFrameContext : public FrameContext {\n> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> >> index a1bd2f0c..8f01bf7d 100644\n> >> --- a/src/ipa/simple/soft_simple.cpp\n> >> +++ b/src/ipa/simple/soft_simple.cpp\n> >> @@ -5,8 +5,6 @@\n> >> * Simple Software Image Processing Algorithm module\n> >> */\n> >> \n> >> -#include \n> >> -#include \n> >> #include \n> >> #include \n> >> \n> >> @@ -93,9 +91,6 @@ private:\n> >> \tstd::unique_ptr camHelper_;\n> >> \tControlInfoMap sensorInfoMap_;\n> >> \n> >> -\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n> >> -\tstd::array gammaTable_;\n> >> -\tint lastBlackLevel_ = -1;\n> >> \t/* Local parameter storage */\n> >> \tstruct IPAContext context_;\n> >> \n> >> @@ -283,6 +278,7 @@ void IPASoftSimple::prepare(const uint32_t frame)\n> >> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> >> \tfor (auto const &algo : algorithms())\n> >> \t\talgo->prepare(context_, frame, frameContext, params_);\n> >> +\tsetIspParams.emit();\n> >> }\n> >> \n> >> void IPASoftSimple::processStats(const uint32_t frame,\n> >> @@ -300,62 +296,6 @@ void IPASoftSimple::processStats(const uint32_t frame,\n> >> \tfor (auto const &algo : algorithms())\n> >> \t\talgo->process(context_, frame, frameContext, stats_, metadata);\n> >> \n> >> -\tSwIspStats::Histogram histogram = stats_->yHistogram;\n> >> -\tconst uint8_t blackLevel = context_.activeState.black.level;\n> >> -\n> >> -\t/*\n> >> -\t * Black level must be subtracted to get the correct AWB ratios, they\n> >> -\t * would be off if they were computed from the whole brightness range\n> >> -\t * rather than from the sensor range.\n> >> -\t */\n> >> -\tconst uint64_t nPixels = std::accumulate(\n> >> -\t\thistogram.begin(), histogram.end(), 0);\n> >> -\tconst uint64_t offset = blackLevel * nPixels;\n> >> -\tconst uint64_t sumR = stats_->sumR_ - offset / 4;\n> >> -\tconst uint64_t sumG = stats_->sumG_ - offset / 2;\n> >> -\tconst uint64_t sumB = stats_->sumB_ - offset / 4;\n> >> -\n> >> -\t/*\n> >> -\t * Calculate red and blue gains for AWB.\n> >> -\t * Clamp max gain at 4.0, this also avoids 0 division.\n> >> -\t * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n> >> -\t */\n> >> -\tconst unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> >> -\tconst unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n> >> -\t/* Green gain and gamma values are fixed */\n> >> -\tconstexpr unsigned int gainG = 256;\n> >> -\n> >> -\t/* Update the gamma table if needed */\n> >> -\tif (blackLevel != lastBlackLevel_) {\n> >> -\t\tconstexpr float gamma = 0.5;\n> >> -\t\tconst unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;\n> >> -\t\tstd::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);\n> >> -\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n> >> -\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n> >> -\t\t\tgammaTable_[i] = UINT8_MAX *\n> >> -\t\t\t\t\t std::pow((i - blackIndex) / divisor, gamma);\n> >> -\n> >> -\t\tlastBlackLevel_ = blackLevel;\n> >> -\t}\n> >> -\n> >> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> >> -\t\tconstexpr unsigned int div =\n> >> -\t\t\tDebayerParams::kRGBLookupSize * 256 / kGammaLookupSize;\n> >> -\t\tunsigned int idx;\n> >> -\n> >> -\t\t/* Apply gamma after gain! */\n> >> -\t\tidx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });\n> >> -\t\tparams_->red[i] = gammaTable_[idx];\n> >> -\n> >> -\t\tidx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });\n> >> -\t\tparams_->green[i] = gammaTable_[idx];\n> >> -\n> >> -\t\tidx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });\n> >> -\t\tparams_->blue[i] = gammaTable_[idx];\n> >> -\t}\n> >> -\n> >> -\tsetIspParams.emit();\n> >> -\n> >> \t/* \\todo Switch to the libipa/algorithm.h API someday. */\n> >> \n> >> \t/*\n> >> @@ -372,6 +312,7 @@ void IPASoftSimple::processStats(const uint32_t frame,\n> >> \t * Calculate Mean Sample Value (MSV) according to formula from:\n> >> \t * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> >> \t */\n> >> +\tconst uint8_t blackLevel = context_.activeState.black.level;\n> >> \tconst unsigned int blackLevelHistIdx =\n> >> \t\tblackLevel / (256 / SwIspStats::kYHistogramSize);\n> >> \tconst unsigned int histogramSize =\n> >> @@ -421,7 +362,8 @@ void IPASoftSimple::processStats(const uint32_t frame,\n> >> \n> >> \tLOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n> >> \t\t\t << \" exp \" << exposure_ << \" again \" << again_\n> >> -\t\t\t << \" gain R/B \" << gainR << \"/\" << gainB\n> >> +\t\t\t << \" gain R/B \" << context_.activeState.gains.red\n> >> +\t\t\t << \"/\" << context_.activeState.gains.blue\n> >\n> > That's already printed in Awb::process(), does it need to be duplicated\n> > here ?\n> \n> I don't think so, I'll remove it here.\n> \n> >> \t\t\t << \" black level \" << static_cast(blackLevel);\n> >> }\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 93E48BF415\n\tfor ;\n\tFri, 6 Sep 2024 13:02:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 958FD634E3;\n\tFri, 6 Sep 2024 15:02:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0993633CC\n\tfor ;\n\tFri, 6 Sep 2024 15:02:29 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 216C43C7;\n\tFri, 6 Sep 2024 15:01:15 +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=\"tn231Uqy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725627675;\n\tbh=GA9vkM6qis+fQ/YVXNfDsZv0y4AGoj6JIM7G9NpYAuc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tn231UqyP/KFNhUrPuZ1i+R0vWmUXhi5AtlTy7zYTvxGLkVpxdPYDdyjvhVLGA6Zv\n\tqJ9EP8oJhaiRf5ksMlBzQKn/yqXJXNnBYG6IDexMg9IrRbi1Zgj6BHLNf43m9bwaca\n\t17VHNaEoqYPXDhmZRw+IzcyhzCh4PIC76tjy610o=","Date":"Fri, 6 Sep 2024 16:02:27 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tUmang Jain ,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 14/18] libcamera: software_isp: Move color handling to\n\tan algorithm module","Message-ID":"<20240906130227.GB27086@pendragon.ideasonboard.com>","References":"<20240830072554.180672-1-mzamazal@redhat.com>\n\t<20240830072554.180672-15-mzamazal@redhat.com>\n\t<20240901214720.GC31148@pendragon.ideasonboard.com>\n\t<87frqdysho.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87frqdysho.fsf@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]