[{"id":35350,"web_url":"https://patchwork.libcamera.org/comment/35350/","msgid":"<175493316838.1641235.2414681416886447452@ping.linuxembedded.co.uk>","date":"2025-08-11T17:26:08","subject":"Re: [PATCH v2 15/16] ipa: rkisp1: Add WDR algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Dan - hidden question to you way at the bottom of this one!\n\nQuoting Stefan Klug (2025-08-08 15:12:53)\n> Add a WDR algorithm to do global tone mapping. Global tone mapping is\n> used to increase the perceived dynamic range of an image. The typical\n> effect is that in areas that are normally overexposed, additional\n> structure becomes visible.\n> \n> The overall idea is that the algorithm applies an exposure value\n> correction to underexpose the image to the point where only a small\n> number of saturated pixels is left. This artificial underexposure is\n> then mitigated by applying a tone mapping curve.\n> \n> This algorithm implements 4 tone mapping strategies:\n> - Linear\n> - Power\n> - Exponential\n> - Histogram equalization\n> \n> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> \n> ---\n> \n> Changes in v2:\n> - Fixed default value for min bright pixels\n> - Added check for supported params type\n> - Reset PID controller\n> - Various fixes from Pauls review\n> ---\n>  src/ipa/rkisp1/algorithms/meson.build |   1 +\n>  src/ipa/rkisp1/algorithms/wdr.cpp     | 510 ++++++++++++++++++++++++++\n>  src/ipa/rkisp1/algorithms/wdr.h       |  64 ++++\n>  src/ipa/rkisp1/ipa_context.h          |  11 +\n>  src/ipa/rkisp1/params.cpp             |   1 +\n>  src/ipa/rkisp1/params.h               |   2 +\n>  src/libcamera/control_ids_debug.yaml  |   7 +-\n>  src/libcamera/control_ids_draft.yaml  |  70 ++++\n>  8 files changed, 665 insertions(+), 1 deletion(-)\n>  create mode 100644 src/ipa/rkisp1/algorithms/wdr.cpp\n>  create mode 100644 src/ipa/rkisp1/algorithms/wdr.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> index 2e42a80cf99d..d329dbfb432d 100644\n> --- a/src/ipa/rkisp1/algorithms/meson.build\n> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> @@ -14,4 +14,5 @@ rkisp1_ipa_algorithms = files([\n>      'gsl.cpp',\n>      'lsc.cpp',\n>      'lux.cpp',\n> +    'wdr.cpp',\n>  ])\n> diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp\n> new file mode 100644\n> index 000000000000..0c1b261d4373\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/wdr.cpp\n> @@ -0,0 +1,510 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board\n> + *\n> + * RkISP1 Wide Dynamic Range control\n> + */\n> +\n> +#include \"wdr.h\"\n> +\n> +#include <libcamera/base/log.h>\n> +#include <libcamera/base/utils.h>\n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include <libipa/histogram.h>\n> +#include <libipa/pwl.h>\n> +\n> +#include \"linux/rkisp1-config.h\"\n> +\n> +/**\n> + * \\file wdr.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class WideDynamicRange\n> + * \\brief RkISP1 Wide Dynamic Range algorithm\n> + *\n> + * This algorithm implements automatic global tone mapping for the RkISP1.\n> + * Global tone mapping is done by the GWDR hardware block and applies\n> + * a global tone mapping curve to the image to increase the perceived dynamic\n> + * range. Imagine an indoor scene with bright outside visible through the\n> + * windows. With normal exposure settings, the windows will be completely\n> + * saturated and no structure (sky/clouds) will be visible because the AEGC has\n> + * to increase overall exposure to reach a certain level of mean brightness. In\n> + * WDR mode, the algorithm will artifically reduce the exposure time so that the\n> + * texture and colours become visible in the formerly saturated areas. Then the\n> + * global tone mapping curve is applied to mitigate the loss of brightness.\n> + *\n> + * Calculating that tone mapping curve is the most difficult part. This\n> + * algorithm implements four tone mapping strategies:\n> + * - Linear: The tone mapping curve is a combination of two linear functions\n> + * with one kneepoint\n\nCan this 'with one kneepiont' be indented to match the Linear ?\n\n\n * - Linear: The tone mapping curve is a combination of two linear functions\n *   with one kneepoint\n\n\n> + * - Power: The tone mapping curve follows a power function\n> + * - Exponential: The tone mapping curve follows an exponential function\n> + * - HistogramEqualization: The tone mapping curve tries to equalize the\n> + * histogram\n\nSame here.?\n\n> + *\n> + * The overall strategy is the same in all cases: A negative exposure value is\n> + * applied to the AEGC regulation until the number of nearly saturated pixels go\n> + * below a given threshold (controllable via WdrMaxBrightPixels, default is 2%)\n> + * or the MinExposureValue specified in the tuning file is reached.\n> + *\n> + * The global tone mapping curve is then calculated so that it accounts for the\n> + * reduction of brightness due to the negative exposure value. As the result of\n> + * tone mapping is very difficult to quantize and is by definition a lossy\n> + * process there is not a single \"correct\" solution.\n> + *\n> + * The approach taken here is based on a simple linear model. Consider a pixel\n> + * that was originally 50% grey. It will have its exposure pushed down by the\n> + * WDR's initial exposure compensation. This value then needs to be pushed back\n> + * up by the tone mapping curve so that it is 50% grey again. This point serves\n> + * as our kneepoint. To get to this kneepoint, this pixel and all darker pixels\n> + * (to the left of the kneepoint on the tone mapping curve) will simply have the\n> + * exposure compensation undone by pow(2, -WDR_EV). This cancels out the\n> + * original exposure compensation, which was pow(2, WDR_EV). The remaining\n> + * brigher pixels (to the right of the kneepoint on the tone mapping curve) will\n> + * be compressed. The WdrStrength control adjusts the gain of the left part of\n> + * the tone mapping curve.\n> + *\n> + * In the Power and Exponential modes, the curves are calculated so that they\n> + * pass through that kneepoint.\n> + *\n> + * The histogram equalization mode tries to equalize the histogram of the\n> + * image and acts independently of the calculated exposure value.\n> + *\n> + * \\code{.unparsed}\n> + * algorithms:\n> + *   - WideDynamicRange:\n> + *       ExposureConstraint:\n> + *         MaxBrightPixels: 0.02\n> + *         yTarget: 0.95\n> + *       MinExposureValue: -4.0\n> + * \\endcode\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Wdr)\n> +\n> +static constexpr unsigned int kTonecurveXIntervals = RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV;\n> +\n> +/*\n> + * Increasing interval sizes. The intervals are crafted so that they sum\n> + * up to 4096. This results in better fitting curves than the constant intervals\n> + * (all entries are 4)\n> + */\n> +static constexpr std::array<int, kTonecurveXIntervals> kLoglikeIntervals = {\n> +       { 0, 0, 0, 0, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3, 3, 4,\n> +         4, 4, 4, 4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 5, 6, 6 }\n> +};\n> +\n> +WideDynamicRange::WideDynamicRange()\n> +{\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::init\n> + */\n> +int WideDynamicRange::init([[maybe_unused]] IPAContext &context,\n> +                          [[maybe_unused]] const YamlObject &tuningData)\n> +{\n> +       if (!(context.hw->supportedBlocks & 1 << RKISP1_EXT_PARAMS_BLOCK_TYPE_WDR)) {\n> +               LOG(RkISP1Wdr, Error)\n> +                       << \"Wide Dynamic Range not supported by the hardware or kernel.\";\n> +               return -ENOTSUP;\n> +       }\n> +\n> +       toneCurveIntervalValues_ = kLoglikeIntervals;\n> +\n> +       /* Calculate a list of normed x values */\n> +       toneCurveX_[0] = 0.0;\n> +       int lastValue = 0;\n> +       for (unsigned int i = 1; i < toneCurveX_.size(); i++) {\n> +               lastValue += std::pow(2, toneCurveIntervalValues_[i - 1] + 3);\n> +               lastValue = std::min(lastValue, 4096);\n> +               toneCurveX_[i] = lastValue / 4096.0;\n> +       }\n> +\n> +       strength_ = 1.0;\n> +       mode_ = controls::draft::WdrOff;\n> +       exposureConstraintMaxBrightPixels_ = 0.02;\n> +       exposureConstraintY_ = 0.95;\n> +       minExposureValue_ = -4.0;\n> +\n> +       const auto &constraint = tuningData[\"ExposureConstraint\"];\n> +       if (!constraint.isDictionary()) {\n> +               LOG(RkISP1Wdr, Warning)\n> +                       << \"ExposureConstraint not found in tuning data.\"\n> +                          \"Using default values MaxBrightPixels: \"\n> +                       << exposureConstraintMaxBrightPixels_\n> +                       << \" yTarget: \" << exposureConstraintY_;\n> +       } else {\n> +               exposureConstraintMaxBrightPixels_ =\n> +                       constraint[\"MaxBrightPixels\"]\n> +                               .get<double>()\n> +                               .value_or(exposureConstraintMaxBrightPixels_);\n> +               exposureConstraintY_ =\n> +                       constraint[\"yTarget\"]\n> +                               .get<double>()\n> +                               .value_or(exposureConstraintY_);\n> +       }\n> +\n> +       const auto &minExp = tuningData[\"MinExposureValue\"];\n> +       minExposureValue_ = minExp.get<double>().value_or(minExposureValue_);\n> +       if (!minExp) {\n> +               LOG(RkISP1Wdr, Warning)\n> +                       << \"MinExposureValue not found in tuning data.\"\n> +                          \" Using default value: \"\n> +                       << minExposureValue_;\n> +       }\n\n{ } aren't required for single statement...\n\n> +\n> +       context.ctrlMap[&controls::draft::WdrMode] =\n> +               ControlInfo(controls::draft::WdrModeValues, controls::draft::WdrOff);\n> +       context.ctrlMap[&controls::draft::WdrStrength] =\n> +               ControlInfo(0.0f, 2.0f, static_cast<float>(strength_));\n> +       context.ctrlMap[&controls::draft::WdrMaxBrightPixels] =\n> +               ControlInfo(0.0f, 1.0f, static_cast<float>(exposureConstraintMaxBrightPixels_));\n> +\n> +       applyCompensationLinear(1.0, 0.0);\n> +\n> +       return 0;\n> +}\n> +\n> +void WideDynamicRange::applyHistogramEqualization(double strength)\n> +{\n> +       if (hist_.empty())\n> +               return;\n> +\n> +       strength *= 0.65;\n\nWhy 0.65 ?\n\n> +\n> +       /*\n> +        * In a fully equalized histogram, all bins have the same value. Try\n> +        * to equalize the histogram by applying a gain or damping depending on\n> +        * the distance of the actual bin value from that norm.\n> +        */\n> +       std::vector<double> gains;\n> +       gains.resize(hist_.size());\n> +       double sum = 0;\n> +       double norm = 1.0 / (gains.size());\n> +       for (unsigned i = 0; i < hist_.size(); i++) {\n> +               double diff = 1.0 + strength * (hist_[i] - norm) / norm;\n> +               gains[i] = diff;\n> +               sum += diff;\n> +       }\n> +       /* Never amplify the last entry. */\n> +       gains.back() = std::max(gains.back(), 1.0);\n> +\n> +       double scale = gains.size() / sum;\n> +       for (auto &v : gains)\n> +               v *= scale;\n> +\n> +       Pwl pwl;\n> +       double step = 1.0 / gains.size();\n> +       double lastX = 0;\n> +       double lastY = 0;\n> +\n> +       pwl.append(lastX, lastY);\n> +       for (unsigned int i = 0; i < gains.size() - 1; i++) {\n> +               lastY += gains[i] * step;\n> +               lastX += step;\n> +               pwl.append(lastX, lastY);\n> +       }\n> +       pwl.append(1.0, 1.0);\n> +\n> +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> +               toneCurveY_[i] = pwl.eval(toneCurveX_[i]);\n> +}\n> +\n> +Vector<double, 2> WideDynamicRange::kneePoint(double gain, double strength)\n> +{\n> +       gain = std::pow(gain, strength);\n> +       double y = 0.5;\n> +       double x = y / gain;\n> +\n> +       return { { x, y } };\n> +}\n> +\n> +void WideDynamicRange::applyCompensationLinear(double gain, double strength)\n> +{\n> +       auto kp = kneePoint(gain, strength);\n> +       double g1 = kp.y() / kp.x();\n> +       double g2 = (kp.y() - 1) / (kp.x() - 1);\n> +\n> +       for (unsigned int i = 0; i < toneCurveX_.size(); i++) {\n> +               double x = toneCurveX_[i];\n> +               double y;\n> +               if (x <= kp.x()) {\n> +                       y = g1 * x;\n> +               } else {\n> +                       y = g2 * x + 1 - g2;\n> +               }\n> +               toneCurveY_[i] = y;\n> +       }\n> +}\n> +\n> +void WideDynamicRange::applyCompensationPower(double gain, double strength)\n> +{\n> +       double e = 1.0;\n> +       if (strength > 1e-6) {\n> +               auto kp = kneePoint(gain, strength);\n> +               /* Calculate an exponent to go through the knee point. */\n> +               e = log(kp.y()) / log(kp.x());\n> +       }\n> +\n> +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> +               toneCurveY_[i] = std::pow(toneCurveX_[i], e);\n> +}\n> +\n> +void WideDynamicRange::applyCompensationExponential(double gain, double strength)\n> +{\n> +       double k = 0.1;\n> +       auto kp = kneePoint(gain, strength);\n> +       double kx = kp.x();\n> +       double ky = kp.y();\n> +\n> +       if (kx > ky) {\n> +               LOG(RkISP1Wdr, Warning) << \"Invalid knee point: \" << kp;\n> +               kx = ky;\n> +       }\n> +\n> +       /*\n> +        * The exponential curve is based on the function proposed by Glozman\n> +        * et al. in\n> +        * S. Glozman, T. Kats, and O. Yadid-Pecht, \"Exponent Operator Based\n> +        * Tone Mapping Algorithm for Color Wide Dynamic Range Images,\" 2011.\n> +        *\n> +        * That function uses a k factor as parameter for the WDR compression\n> +        * curve:\n> +        * k=0: maximum compression\n> +        * k=infinity: linear curve\n> +        *\n> +        * To calculate a k factor that results in a curve that passes through\n> +        * the kneepoint, the equation needs to be solved for k after inserting\n> +        * the kneepoint.  This can be formulated as search for a zero point.\n> +        * Unfortunately there is no closed solution for that transformation.\n> +        * Using newton's method to approximate the value is numerically\n> +        * unstable.\n> +        *\n> +        * Luckily the function only crosses the x axis once and for the set of\n> +        * possible kneepoints, a negative and a positive point can be guessed.\n> +        * The approximation is then implemented using bisection.\n> +        */\n> +       if (std::abs(kx - ky) < 0.001) {\n> +               k = 1e8;\n> +       } else {\n> +               double kl = 0.0001;\n> +               double kh = 1000;\n> +\n> +               auto fk = [=](double v) {\n> +                       return std::exp(-kx / v) -\n> +                              ky * std::exp(-1.0 / v) + ky - 1.0;\n> +               };\n> +\n> +               ASSERT(fk(kl) < 0);\n> +               ASSERT(fk(kh) > 0);\n> +\n> +               k = kh / 10.0;\n> +               while (fk(k) > 0) {\n> +                       kh = k;\n> +                       k /= 10.0;\n> +               }\n> +\n> +               do {\n> +                       k = (kl + kh) / 2;\n> +                       if (fk(k) < 0)\n> +                               kl = k;\n> +                       else\n> +                               kh = k;\n> +               } while (std::abs(kh - kl) > 1e-3);\n> +       }\n> +\n> +       double a = 1.0 / (1.0 - std::exp(-1.0 / k));\n> +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> +               toneCurveY_[i] = a * (1.0 - std::exp(-toneCurveX_[i] / k));\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void WideDynamicRange::queueRequest([[maybe_unused]] IPAContext &context,\n> +                                   [[maybe_unused]] const uint32_t frame,\n> +                                   IPAFrameContext &frameContext,\n> +                                   const ControlList &controls)\n> +{\n> +       strength_ = controls.get(controls::draft::WdrStrength).value_or(strength_);\n> +       exposureConstraintMaxBrightPixels_ =\n> +               controls.get(controls::draft::WdrMaxBrightPixels)\n> +                       .value_or(exposureConstraintMaxBrightPixels_);\n> +\n> +       const auto &mode = controls.get(controls::draft::WdrMode);\n> +       if (mode) {\n> +               mode_ = static_cast<controls::draft::WdrModeEnum>(*mode);\n> +       }\n\nNo { } required here.\n\n> +\n> +       frameContext.wdr.mode = mode_;\n> +       frameContext.wdr.strength = strength_;\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void WideDynamicRange::prepare(IPAContext &context,\n> +                              [[maybe_unused]] const uint32_t frame,\n> +                              IPAFrameContext &frameContext,\n> +                              RkISP1Params *params)\n> +{\n> +       if (!params) {\n> +               LOG(RkISP1Wdr, Warning) << \"Params is null\";\n> +               return;\n> +       }\n> +\n> +       auto config = params->block<BlockType::Wdr>();\n> +\n> +       auto mode = frameContext.wdr.mode;\n> +\n> +       config.setEnabled(mode != controls::draft::WdrOff);\n> +\n> +       double comp = 0;\n> +\n> +       /*\n> +        * Todo: This overwrites frameContext.agc.exposureValue so that\n> +        * in the next call to Agc::process() that exposureValue get's\n> +        * applied. In the future it is planned to move the exposure\n> +        * calculations from Agc::process() to Agc::prepare(). In this\n> +        * case, we need to ensure that this code get's called early enough.\n> +        */\n\nEeep ...\n  /me looks away for the moment.\n\n> +       if (mode != controls::draft::WdrOff) {\n> +               frameContext.wdr.wdrExposureValue = context.activeState.wdr.exposureValue;\n> +               frameContext.wdr.agcExposureValue = frameContext.agc.exposureValue;\n> +\n> +               /*\n> +                * When WDR is enabled, the maxBrightPixels constraint is always\n> +                * active. This is problematic when the user sets a positive\n> +                * exposureValue. As this would mean that the negative WDR\n> +                * exposure value and the user provided positive value should\n> +                * cancel each other out. But as the regulation is not absolute\n> +                * and only dependent on the measured changes in the histogram,\n> +                * reducing the wdr EV would only lead to an even stronger pull\n> +                * from regulation. Positive user supplied EVs must therefore be\n> +                * handled using the wdr curve.\n> +                */\n> +               if (frameContext.wdr.wdrExposureValue < 0) {\n> +                       frameContext.agc.exposureValue =\n> +                               std::min(frameContext.agc.exposureValue,\n> +                                        frameContext.wdr.wdrExposureValue);\n> +                       comp = frameContext.agc.exposureValue - frameContext.wdr.agcExposureValue;\n> +               }\n> +       }\n> +\n> +       /* Calculate how much EV we need to compensate with the WDR curve. */\n> +       double gain = pow(2.0, -comp);\n> +\n> +       if (mode == controls::draft::WdrOff) {\n> +               applyCompensationLinear(1.0, 0.0);\n> +       } else if (mode == controls::draft::WdrLinear) {\n> +               applyCompensationLinear(gain, frameContext.wdr.strength);\n> +       } else if (mode == controls::draft::WdrPower) {\n> +               applyCompensationPower(gain, frameContext.wdr.strength);\n> +       } else if (mode == controls::draft::WdrExponential) {\n> +               applyCompensationExponential(gain, frameContext.wdr.strength);\n> +       } else if (mode == controls::draft::WdrHistogramEqualization) {\n> +               applyHistogramEqualization(frameContext.wdr.strength);\n> +       }\n> +\n> +       /* Reset value */\n> +       config->dmin_strength = 0x10;\n> +       config->dmin_thresh = 0;\n> +\n> +       for (unsigned int i = 0; i < kTonecurveXIntervals; i++) {\n> +               int v = toneCurveIntervalValues_[i];\n> +               config->tone_curve.dY[i / 8] |= (v & 0x07) << ((i % 8) * 4);\n> +       }\n> +\n> +       std::vector<Point> debugCurve;\n> +\n> +       /*\n> +        * Fix the curve to adhere to the hardware constraints. Don't apply a\n> +        * constraint on the first element, which is most likely zero anyways.\n> +        */\n> +       int lastY = toneCurveY_[0] * 4096.0;\n> +       for (unsigned int i = 0; i < toneCurveX_.size(); i++) {\n> +               int diff = static_cast<int>(toneCurveY_[i] * 4096.0) - lastY;\n> +               diff = std::clamp(diff, -2048, 2048);\n> +               lastY = lastY + diff;\n> +               config->tone_curve.ym[i] = lastY;\n> +               debugCurve.push_back({ static_cast<int>(toneCurveX_[i] * 4096.0),\n> +                                      lastY });\n> +       }\n> +\n> +       context.debugMetadata.set<Span<const Point>>(controls::debug::WdrCurve, debugCurve);\n> +}\n> +\n> +void WideDynamicRange::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> +                              IPAFrameContext &frameContext,\n> +                              const rkisp1_stat_buffer *stats,\n> +                              ControlList &metadata)\n> +{\n> +       if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_HIST)) {\n> +               LOG(RkISP1Wdr, Error) << \"No histogram data in statistics\";\n\nIs this a full error if this happens? or just a debug print ? I thought\nsometimes on say a too dark image we might not get any (usable) stats ?\n\n> +               return;\n> +       }\n> +\n> +       if (!started_) {\n> +               started_ = true;\n> +               pid_.setStandardParameters(1, 5.0, 3.0);\n> +               pid_.setOutputLimits(minExposureValue_, 0.0);\n\nCan the minExposureValue_ ever be changed for instance if we get called\nwith different modes in configure() or if the frame duration limits get\nchanged ?\n\n> +               context.activeState.wdr.exposureValue = 0.0;\n\nShould those happen during configure() ? or init() ?\n\n - init just once when constructed... (current behaviour)\n - or configure if every time the camera is reconfigured..\n\nThen we wouldn't need a started_ bool tracker ?\n\n> +       }\n> +\n> +       pid_.setTarget(exposureConstraintY_);\n> +\n> +       const rkisp1_cif_isp_stat *params = &stats->params;\n> +       auto mode = frameContext.wdr.mode;\n> +\n> +       metadata.set(controls::draft::WdrMode, mode);\n> +\n> +       Histogram cumHist({ params->hist.hist_bins, context.hw->numHistogramBins },\n> +                         [](uint32_t x) { return x >> 4; });\n> +\n> +       std::vector<double> hist;\n> +       double sum = 0;\n> +       for (unsigned i = 0; i < context.hw->numHistogramBins; i++) {\n> +               double v = params->hist.hist_bins[i] >> 4;\n> +               hist.push_back(v);\n> +               sum += v;\n> +       }\n> +\n> +       /* Scale so that the entries sum up to 1. */\n> +       double scale = 1.0 / sum;\n> +       for (auto &v : hist)\n> +               v *= scale;\n> +       hist_.swap(hist);\n> +\n> +       double mean = cumHist.quantile(1.0 - exposureConstraintMaxBrightPixels_) /\n> +                     cumHist.bins();\n> +       LOG(RkISP1Wdr, Debug) << \"Mean y of bright pixels: \" << mean;\n> +\n> +       if (mode == controls::draft::WdrOff) {\n> +               metadata.set(controls::draft::WdrExposureValue, 0);\n> +               return;\n> +       }\n> +\n> +       context.activeState.wdr.exposureValue = pid_.process(mean);\n> +       LOG(RkISP1Wdr, Debug) << \"Active state WDR ev: \" << context.activeState.wdr.exposureValue;\n> +       metadata.set(controls::draft::WdrExposureValue, frameContext.wdr.wdrExposureValue);\n> +\n> +       /*\n> +        * We overwrote agc exposure value in prepare() to create an\n> +        * underexposure for WDR. Report the original exposure value in metadata.\n> +        */\n> +       metadata.set(controls::ExposureValue, frameContext.wdr.agcExposureValue);\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(WideDynamicRange, \"WideDynamicRange\")\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/wdr.h b/src/ipa/rkisp1/algorithms/wdr.h\n> new file mode 100644\n> index 000000000000..bdea4eb5492a\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/wdr.h\n> @@ -0,0 +1,64 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * RkISP1 Wide Dynamic Range control\n> + */\n> +\n> +#pragma once\n> +\n> +#include <libcamera/control_ids.h>\n> +\n> +#include \"libcamera/internal/pid_controller.h\"\n> +\n> +#include \"linux/rkisp1-config.h\"\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class WideDynamicRange : public Algorithm\n> +{\n> +public:\n> +       WideDynamicRange();\n> +       ~WideDynamicRange() = default;\n> +\n> +       int init(IPAContext &context, const YamlObject &tuningData) override;\n> +\n> +       void queueRequest(IPAContext &context, 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> +                    RkISP1Params *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> +       Vector<double, 2> kneePoint(double gain, double strength);\n> +       void applyCompensationLinear(double gain, double strength);\n> +       void applyCompensationPower(double gain, double strength);\n> +       void applyCompensationExponential(double gain, double strength);\n> +       void applyHistogramEqualization(double strength);\n> +\n> +       double exposureConstraintMaxBrightPixels_;\n> +       double exposureConstraintY_;\n> +       double minExposureValue_;\n> +       double strength_;\n> +       bool started_ = false;\n> +\n> +       controls::draft::WdrModeEnum mode_;\n> +       std::vector<double> hist_;\n> +       PidController pid_;\n> +\n> +       std::array<int, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV> toneCurveIntervalValues_;\n> +       std::array<double, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1> toneCurveX_;\n> +       std::array<double, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1> toneCurveY_;\n> +};\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 60cfab228edf..32f6db30bbdb 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -130,6 +130,10 @@ struct IPAActiveState {\n>         struct {\n>                 double gamma;\n>         } goc;\n> +\n> +       struct {\n> +               double exposureValue;\n> +       } wdr;\n>  };\n>  \n>  struct IPAFrameContext : public FrameContext {\n> @@ -198,6 +202,13 @@ struct IPAFrameContext : public FrameContext {\n>         struct {\n>                 double lux;\n>         } lux;\n> +\n> +       struct {\n> +               controls::draft::WdrModeEnum mode;\n> +               double wdrExposureValue;\n> +               double agcExposureValue;\n> +               double strength;\n> +       } wdr;\n>  };\n>  \n>  struct IPAContext {\n> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> index 4c0b051ce65d..5edb36c91b87 100644\n> --- a/src/ipa/rkisp1/params.cpp\n> +++ b/src/ipa/rkisp1/params.cpp\n> @@ -74,6 +74,7 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {\n>         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandBls, COMPAND_BLS, compand_bls),\n>         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandExpand, COMPAND_EXPAND, compand_curve),\n>         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandCompress, COMPAND_COMPRESS, compand_curve),\n> +       RKISP1_BLOCK_TYPE_ENTRY_EXT(Wdr, WDR, wdr),\n>  };\n>  \n>  } /* namespace */\n> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> index 40450e34497a..2e60528d102e 100644\n> --- a/src/ipa/rkisp1/params.h\n> +++ b/src/ipa/rkisp1/params.h\n> @@ -40,6 +40,7 @@ enum class BlockType {\n>         CompandBls,\n>         CompandExpand,\n>         CompandCompress,\n> +       Wdr,\n>  };\n>  \n>  namespace details {\n> @@ -74,6 +75,7 @@ RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)\n>  RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)\n>  RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)\n>  RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n> +RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr)\n>  \n>  } /* namespace details */\n>  \n> diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml\n> index 797532712099..9489e677402e 100644\n> --- a/src/libcamera/control_ids_debug.yaml\n> +++ b/src/libcamera/control_ids_debug.yaml\n> @@ -3,4 +3,9 @@\n>  %YAML 1.1\n>  ---\n>  vendor: debug\n> -controls: []\n> +controls:\n> +- WdrCurve:\n> +    type: Point\n> +    direction: out\n> +    description: Debug control WdrCurve found in src/ipa/rkisp1/algorithms/wdr.cpp\n> +    size: '[n]'\n\nI love the idea of having debug controls that let us plot debug\ninternals - but if these are generated by script - do we need a way to\nmake sure they are only ever append only - if we ever merge a debug\ncontrol - suddenly the id number becomes part of the ABI - and adding\nnew debug controls can't change the existing ABI. Could be handled with\nmanual review ... but could then irritate the automatic tools for\ncreateing the controls in the first place ...\n\n\n> diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> index 03309eeac34f..0c0af7a19968 100644\n> --- a/src/libcamera/control_ids_draft.yaml\n> +++ b/src/libcamera/control_ids_draft.yaml\n> @@ -293,5 +293,75 @@ controls:\n>  \n>          Currently identical to ANDROID_STATISTICS_FACE_IDS.\n>        size: [n]\n> +  - WdrMode:\n\nCan we avoid these going into ::draft:: at all ?\n\n> +      type: int32_t\n> +      direction: inout\n> +      description: |\n> +        Set the WDR mode.\n> +\n> +        The WDR mode is used to select the algorithm used for global tone\n> +        mapping. It will automatically reduce the exposure time of the sensor\n> +        so that there are only a small number of saturated pixels in the image.\n> +        The algorithm then compensates for the loss of brightness by applying a\n> +        global tone mapping curve to the image.\n> +      enum:\n> +        - name: WdrOff\n> +          value: 0\n> +          description: Wdr is disabled.\n> +        - name: WdrLinear\n> +          value: 1\n> +          description:\n> +            Apply a linear global tone mapping curve.\n\nI'm torn here on naming. Cue the bikeshed door opening ...\n\nBut we use the control name 'Wdr' and say it applies 'Global Tone\nMapping' configurations.\n\nIs WDR sufficiently known to represent Global Tone Mapping - are the\nterms sufficiently interchangable ?\n\n\nDo we have other hardware with 'WDR' capabilities to compare if the\ncontrols make sense on other devices already?\n\nDan - I expect the Mali-C55 to be quite HDR capable ... is there a WDR\nblock there that would benefit/utilise the same controls and concepts?\n\n\n\n> +\n> +            The curve with two linear sections is applied. This produces good\n> +            results at the expense of a slightly artificial look.\n> +        - name: WdrPower\n> +          value: 2\n> +          description: |\n> +            Apply a power global tone mapping curve.\n> +\n> +            This curve has high gain values on the dark areas of an image and\n> +            high compression values on the bright area. It therefore tends to\n> +            produce noticeable noise artifacts.\n> +        - name: WdrExponential\n> +          value: 3\n> +          description: |\n> +            Apply a exponential global tone mapping curve.\n> +\n> +            This curve has lower gain values in dark areas compared to the power\n> +            curve but produces a more natural look compared to the linear curve.\n> +            It is therefore the best choice for most scenes.\n> +        - name: WdrHistogramEqualization\n> +          value: 4\n> +          description: |\n> +            Apply histogram equalization.\n> +\n> +            This curve preserves most of the information of the image at the\n> +            expense of a very artificial look. It is therefore best suited for\n> +            technical analysis.\n> +  - WdrStrength:\n> +      type: float\n> +      direction: in\n> +      description: |\n> +        Specify the strength of the wdr algorithm. The exact meaning of this\n> +        value is specific to the algorithm in use. Usually a value of 0 means no\n> +        global tone mapping is applied. A values of 1 is the default value and\n> +        the correct value for most scenes. A value above 1 increases the global\n> +        tone mapping effect and can lead to unrealistic image effects.\n> +  - WdrMaxBrightPixels:\n> +      type: float\n> +      direction: in\n> +      description: |\n> +        Percentage of allowed (nearly) saturated pixels. The WDR algorithm\n> +        reduces the WdrExposureValue until the amount of pixels that are close\n> +        to saturation is lower than this value.\n> +  - WdrExposureValue:\n> +      type: float\n> +      direction: out\n> +      description: |\n> +        Reports the Exposure Value that was applied to the AEGC regulation so\n> +        that the WdrMaxBrightPixels limit is reached.\n\nIs that /different/ to the ExposureValue already reported in metadata?\n\n> +\n> +        \\sa ExposureValue\n>  \n>  ...\n> -- \n> 2.48.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 AF3F5BDCC1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 11 Aug 2025 17:26:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68A3C69241;\n\tMon, 11 Aug 2025 19:26: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 AA71369233\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 11 Aug 2025 19:26: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 4EAFA82A;\n\tMon, 11 Aug 2025 19:25:19 +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=\"jz1ZJugJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754933119;\n\tbh=CdX9Yi68XGfu5EGZ8taD60xn5eAFxpJHbkaR992CpdM=;\n\th=In-Reply-To:References:Subject:From:To:Cc:Date:From;\n\tb=jz1ZJugJ8L1bv+UOV8xW7WRbDdVS5isu7YfL6fpmvxArlGDSUY6k6bmc6l2ZDaAmF\n\tDdpKxvlX2SpJQd+4dgNt/KuoxCDvM6YqAiu2xBDs5AyN55mplk1qaDdph/Xc3Q4mk/\n\t0fJ1dmCoTBhR8ixNUO1+29u9n+oGKujuR/gI4rMc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250808141315.413839-16-stefan.klug@ideasonboard.com>","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-16-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 15/16] ipa: rkisp1: Add WDR algorithm","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org,\n\tDan Scally <dan.scally@ideasonboard.com>, ","Cc":"Stefan Klug <stefan.klug@ideasonboard.com>","Date":"Mon, 11 Aug 2025 18:26:08 +0100","Message-ID":"<175493316838.1641235.2414681416886447452@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":35373,"web_url":"https://patchwork.libcamera.org/comment/35373/","msgid":"<175508169312.1225443.2627596737853254972@localhost>","date":"2025-08-13T10:41:33","subject":"Re: [PATCH v2 15/16] ipa: rkisp1: Add WDR algorithm","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the review.\n\nQuoting Kieran Bingham (2025-08-11 19:26:08)\n> Dan - hidden question to you way at the bottom of this one!\n> \n> Quoting Stefan Klug (2025-08-08 15:12:53)\n> > Add a WDR algorithm to do global tone mapping. Global tone mapping is\n> > used to increase the perceived dynamic range of an image. The typical\n> > effect is that in areas that are normally overexposed, additional\n> > structure becomes visible.\n> > \n> > The overall idea is that the algorithm applies an exposure value\n> > correction to underexpose the image to the point where only a small\n> > number of saturated pixels is left. This artificial underexposure is\n> > then mitigated by applying a tone mapping curve.\n> > \n> > This algorithm implements 4 tone mapping strategies:\n> > - Linear\n> > - Power\n> > - Exponential\n> > - Histogram equalization\n> > \n> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > \n> > ---\n> > \n> > Changes in v2:\n> > - Fixed default value for min bright pixels\n> > - Added check for supported params type\n> > - Reset PID controller\n> > - Various fixes from Pauls review\n> > ---\n> >  src/ipa/rkisp1/algorithms/meson.build |   1 +\n> >  src/ipa/rkisp1/algorithms/wdr.cpp     | 510 ++++++++++++++++++++++++++\n> >  src/ipa/rkisp1/algorithms/wdr.h       |  64 ++++\n> >  src/ipa/rkisp1/ipa_context.h          |  11 +\n> >  src/ipa/rkisp1/params.cpp             |   1 +\n> >  src/ipa/rkisp1/params.h               |   2 +\n> >  src/libcamera/control_ids_debug.yaml  |   7 +-\n> >  src/libcamera/control_ids_draft.yaml  |  70 ++++\n> >  8 files changed, 665 insertions(+), 1 deletion(-)\n> >  create mode 100644 src/ipa/rkisp1/algorithms/wdr.cpp\n> >  create mode 100644 src/ipa/rkisp1/algorithms/wdr.h\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > index 2e42a80cf99d..d329dbfb432d 100644\n> > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > @@ -14,4 +14,5 @@ rkisp1_ipa_algorithms = files([\n> >      'gsl.cpp',\n> >      'lsc.cpp',\n> >      'lux.cpp',\n> > +    'wdr.cpp',\n> >  ])\n> > diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp\n> > new file mode 100644\n> > index 000000000000..0c1b261d4373\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/wdr.cpp\n> > @@ -0,0 +1,510 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2025, Ideas On Board\n> > + *\n> > + * RkISP1 Wide Dynamic Range control\n> > + */\n> > +\n> > +#include \"wdr.h\"\n> > +\n> > +#include <libcamera/base/log.h>\n> > +#include <libcamera/base/utils.h>\n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +#include <libipa/histogram.h>\n> > +#include <libipa/pwl.h>\n> > +\n> > +#include \"linux/rkisp1-config.h\"\n> > +\n> > +/**\n> > + * \\file wdr.h\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +/**\n> > + * \\class WideDynamicRange\n> > + * \\brief RkISP1 Wide Dynamic Range algorithm\n> > + *\n> > + * This algorithm implements automatic global tone mapping for the RkISP1.\n> > + * Global tone mapping is done by the GWDR hardware block and applies\n> > + * a global tone mapping curve to the image to increase the perceived dynamic\n> > + * range. Imagine an indoor scene with bright outside visible through the\n> > + * windows. With normal exposure settings, the windows will be completely\n> > + * saturated and no structure (sky/clouds) will be visible because the AEGC has\n> > + * to increase overall exposure to reach a certain level of mean brightness. In\n> > + * WDR mode, the algorithm will artifically reduce the exposure time so that the\n> > + * texture and colours become visible in the formerly saturated areas. Then the\n> > + * global tone mapping curve is applied to mitigate the loss of brightness.\n> > + *\n> > + * Calculating that tone mapping curve is the most difficult part. This\n> > + * algorithm implements four tone mapping strategies:\n> > + * - Linear: The tone mapping curve is a combination of two linear functions\n> > + * with one kneepoint\n> \n> Can this 'with one kneepiont' be indented to match the Linear ?\n> \n> \n>  * - Linear: The tone mapping curve is a combination of two linear functions\n>  *   with one kneepoint\n> \n\nack\n\n> \n> > + * - Power: The tone mapping curve follows a power function\n> > + * - Exponential: The tone mapping curve follows an exponential function\n> > + * - HistogramEqualization: The tone mapping curve tries to equalize the\n> > + * histogram\n> \n> Same here.?\n\nack\n\n> \n> > + *\n> > + * The overall strategy is the same in all cases: A negative exposure value is\n> > + * applied to the AEGC regulation until the number of nearly saturated pixels go\n> > + * below a given threshold (controllable via WdrMaxBrightPixels, default is 2%)\n> > + * or the MinExposureValue specified in the tuning file is reached.\n> > + *\n> > + * The global tone mapping curve is then calculated so that it accounts for the\n> > + * reduction of brightness due to the negative exposure value. As the result of\n> > + * tone mapping is very difficult to quantize and is by definition a lossy\n> > + * process there is not a single \"correct\" solution.\n> > + *\n> > + * The approach taken here is based on a simple linear model. Consider a pixel\n> > + * that was originally 50% grey. It will have its exposure pushed down by the\n> > + * WDR's initial exposure compensation. This value then needs to be pushed back\n> > + * up by the tone mapping curve so that it is 50% grey again. This point serves\n> > + * as our kneepoint. To get to this kneepoint, this pixel and all darker pixels\n> > + * (to the left of the kneepoint on the tone mapping curve) will simply have the\n> > + * exposure compensation undone by pow(2, -WDR_EV). This cancels out the\n> > + * original exposure compensation, which was pow(2, WDR_EV). The remaining\n> > + * brigher pixels (to the right of the kneepoint on the tone mapping curve) will\n> > + * be compressed. The WdrStrength control adjusts the gain of the left part of\n> > + * the tone mapping curve.\n> > + *\n> > + * In the Power and Exponential modes, the curves are calculated so that they\n> > + * pass through that kneepoint.\n> > + *\n> > + * The histogram equalization mode tries to equalize the histogram of the\n> > + * image and acts independently of the calculated exposure value.\n> > + *\n> > + * \\code{.unparsed}\n> > + * algorithms:\n> > + *   - WideDynamicRange:\n> > + *       ExposureConstraint:\n> > + *         MaxBrightPixels: 0.02\n> > + *         yTarget: 0.95\n> > + *       MinExposureValue: -4.0\n> > + * \\endcode\n> > + */\n> > +\n> > +LOG_DEFINE_CATEGORY(RkISP1Wdr)\n> > +\n> > +static constexpr unsigned int kTonecurveXIntervals = RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV;\n> > +\n> > +/*\n> > + * Increasing interval sizes. The intervals are crafted so that they sum\n> > + * up to 4096. This results in better fitting curves than the constant intervals\n> > + * (all entries are 4)\n> > + */\n> > +static constexpr std::array<int, kTonecurveXIntervals> kLoglikeIntervals = {\n> > +       { 0, 0, 0, 0, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3, 3, 4,\n> > +         4, 4, 4, 4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 5, 6, 6 }\n> > +};\n> > +\n> > +WideDynamicRange::WideDynamicRange()\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::init\n> > + */\n> > +int WideDynamicRange::init([[maybe_unused]] IPAContext &context,\n> > +                          [[maybe_unused]] const YamlObject &tuningData)\n> > +{\n> > +       if (!(context.hw->supportedBlocks & 1 << RKISP1_EXT_PARAMS_BLOCK_TYPE_WDR)) {\n> > +               LOG(RkISP1Wdr, Error)\n> > +                       << \"Wide Dynamic Range not supported by the hardware or kernel.\";\n> > +               return -ENOTSUP;\n> > +       }\n> > +\n> > +       toneCurveIntervalValues_ = kLoglikeIntervals;\n> > +\n> > +       /* Calculate a list of normed x values */\n> > +       toneCurveX_[0] = 0.0;\n> > +       int lastValue = 0;\n> > +       for (unsigned int i = 1; i < toneCurveX_.size(); i++) {\n> > +               lastValue += std::pow(2, toneCurveIntervalValues_[i - 1] + 3);\n> > +               lastValue = std::min(lastValue, 4096);\n> > +               toneCurveX_[i] = lastValue / 4096.0;\n> > +       }\n> > +\n> > +       strength_ = 1.0;\n> > +       mode_ = controls::draft::WdrOff;\n> > +       exposureConstraintMaxBrightPixels_ = 0.02;\n> > +       exposureConstraintY_ = 0.95;\n> > +       minExposureValue_ = -4.0;\n> > +\n> > +       const auto &constraint = tuningData[\"ExposureConstraint\"];\n> > +       if (!constraint.isDictionary()) {\n> > +               LOG(RkISP1Wdr, Warning)\n> > +                       << \"ExposureConstraint not found in tuning data.\"\n> > +                          \"Using default values MaxBrightPixels: \"\n> > +                       << exposureConstraintMaxBrightPixels_\n> > +                       << \" yTarget: \" << exposureConstraintY_;\n> > +       } else {\n> > +               exposureConstraintMaxBrightPixels_ =\n> > +                       constraint[\"MaxBrightPixels\"]\n> > +                               .get<double>()\n> > +                               .value_or(exposureConstraintMaxBrightPixels_);\n> > +               exposureConstraintY_ =\n> > +                       constraint[\"yTarget\"]\n> > +                               .get<double>()\n> > +                               .value_or(exposureConstraintY_);\n> > +       }\n> > +\n> > +       const auto &minExp = tuningData[\"MinExposureValue\"];\n> > +       minExposureValue_ = minExp.get<double>().value_or(minExposureValue_);\n> > +       if (!minExp) {\n> > +               LOG(RkISP1Wdr, Warning)\n> > +                       << \"MinExposureValue not found in tuning data.\"\n> > +                          \" Using default value: \"\n> > +                       << minExposureValue_;\n> > +       }\n> \n> { } aren't required for single statement...\n\nArgh I don't get these to become a habit :-)\n\n> \n> > +\n> > +       context.ctrlMap[&controls::draft::WdrMode] =\n> > +               ControlInfo(controls::draft::WdrModeValues, controls::draft::WdrOff);\n> > +       context.ctrlMap[&controls::draft::WdrStrength] =\n> > +               ControlInfo(0.0f, 2.0f, static_cast<float>(strength_));\n> > +       context.ctrlMap[&controls::draft::WdrMaxBrightPixels] =\n> > +               ControlInfo(0.0f, 1.0f, static_cast<float>(exposureConstraintMaxBrightPixels_));\n> > +\n> > +       applyCompensationLinear(1.0, 0.0);\n> > +\n> > +       return 0;\n> > +}\n> > +\n> > +void WideDynamicRange::applyHistogramEqualization(double strength)\n> > +{\n> > +       if (hist_.empty())\n> > +               return;\n> > +\n> > +       strength *= 0.65;\n> \n> Why 0.65 ?\n\nThat value was chosen to provide a similar optical impression compared\nto the other algorithms. I'll revisit and provide some description.\n\n> \n> > +\n> > +       /*\n> > +        * In a fully equalized histogram, all bins have the same value. Try\n> > +        * to equalize the histogram by applying a gain or damping depending on\n> > +        * the distance of the actual bin value from that norm.\n> > +        */\n> > +       std::vector<double> gains;\n> > +       gains.resize(hist_.size());\n> > +       double sum = 0;\n> > +       double norm = 1.0 / (gains.size());\n> > +       for (unsigned i = 0; i < hist_.size(); i++) {\n> > +               double diff = 1.0 + strength * (hist_[i] - norm) / norm;\n> > +               gains[i] = diff;\n> > +               sum += diff;\n> > +       }\n> > +       /* Never amplify the last entry. */\n> > +       gains.back() = std::max(gains.back(), 1.0);\n> > +\n> > +       double scale = gains.size() / sum;\n> > +       for (auto &v : gains)\n> > +               v *= scale;\n> > +\n> > +       Pwl pwl;\n> > +       double step = 1.0 / gains.size();\n> > +       double lastX = 0;\n> > +       double lastY = 0;\n> > +\n> > +       pwl.append(lastX, lastY);\n> > +       for (unsigned int i = 0; i < gains.size() - 1; i++) {\n> > +               lastY += gains[i] * step;\n> > +               lastX += step;\n> > +               pwl.append(lastX, lastY);\n> > +       }\n> > +       pwl.append(1.0, 1.0);\n> > +\n> > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> > +               toneCurveY_[i] = pwl.eval(toneCurveX_[i]);\n> > +}\n> > +\n> > +Vector<double, 2> WideDynamicRange::kneePoint(double gain, double strength)\n> > +{\n> > +       gain = std::pow(gain, strength);\n> > +       double y = 0.5;\n> > +       double x = y / gain;\n> > +\n> > +       return { { x, y } };\n> > +}\n> > +\n> > +void WideDynamicRange::applyCompensationLinear(double gain, double strength)\n> > +{\n> > +       auto kp = kneePoint(gain, strength);\n> > +       double g1 = kp.y() / kp.x();\n> > +       double g2 = (kp.y() - 1) / (kp.x() - 1);\n> > +\n> > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++) {\n> > +               double x = toneCurveX_[i];\n> > +               double y;\n> > +               if (x <= kp.x()) {\n> > +                       y = g1 * x;\n> > +               } else {\n> > +                       y = g2 * x + 1 - g2;\n> > +               }\n> > +               toneCurveY_[i] = y;\n> > +       }\n> > +}\n> > +\n> > +void WideDynamicRange::applyCompensationPower(double gain, double strength)\n> > +{\n> > +       double e = 1.0;\n> > +       if (strength > 1e-6) {\n> > +               auto kp = kneePoint(gain, strength);\n> > +               /* Calculate an exponent to go through the knee point. */\n> > +               e = log(kp.y()) / log(kp.x());\n> > +       }\n> > +\n> > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> > +               toneCurveY_[i] = std::pow(toneCurveX_[i], e);\n> > +}\n> > +\n> > +void WideDynamicRange::applyCompensationExponential(double gain, double strength)\n> > +{\n> > +       double k = 0.1;\n> > +       auto kp = kneePoint(gain, strength);\n> > +       double kx = kp.x();\n> > +       double ky = kp.y();\n> > +\n> > +       if (kx > ky) {\n> > +               LOG(RkISP1Wdr, Warning) << \"Invalid knee point: \" << kp;\n> > +               kx = ky;\n> > +       }\n> > +\n> > +       /*\n> > +        * The exponential curve is based on the function proposed by Glozman\n> > +        * et al. in\n> > +        * S. Glozman, T. Kats, and O. Yadid-Pecht, \"Exponent Operator Based\n> > +        * Tone Mapping Algorithm for Color Wide Dynamic Range Images,\" 2011.\n> > +        *\n> > +        * That function uses a k factor as parameter for the WDR compression\n> > +        * curve:\n> > +        * k=0: maximum compression\n> > +        * k=infinity: linear curve\n> > +        *\n> > +        * To calculate a k factor that results in a curve that passes through\n> > +        * the kneepoint, the equation needs to be solved for k after inserting\n> > +        * the kneepoint.  This can be formulated as search for a zero point.\n> > +        * Unfortunately there is no closed solution for that transformation.\n> > +        * Using newton's method to approximate the value is numerically\n> > +        * unstable.\n> > +        *\n> > +        * Luckily the function only crosses the x axis once and for the set of\n> > +        * possible kneepoints, a negative and a positive point can be guessed.\n> > +        * The approximation is then implemented using bisection.\n> > +        */\n> > +       if (std::abs(kx - ky) < 0.001) {\n> > +               k = 1e8;\n> > +       } else {\n> > +               double kl = 0.0001;\n> > +               double kh = 1000;\n> > +\n> > +               auto fk = [=](double v) {\n> > +                       return std::exp(-kx / v) -\n> > +                              ky * std::exp(-1.0 / v) + ky - 1.0;\n> > +               };\n> > +\n> > +               ASSERT(fk(kl) < 0);\n> > +               ASSERT(fk(kh) > 0);\n> > +\n> > +               k = kh / 10.0;\n> > +               while (fk(k) > 0) {\n> > +                       kh = k;\n> > +                       k /= 10.0;\n> > +               }\n> > +\n> > +               do {\n> > +                       k = (kl + kh) / 2;\n> > +                       if (fk(k) < 0)\n> > +                               kl = k;\n> > +                       else\n> > +                               kh = k;\n> > +               } while (std::abs(kh - kl) > 1e-3);\n> > +       }\n> > +\n> > +       double a = 1.0 / (1.0 - std::exp(-1.0 / k));\n> > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> > +               toneCurveY_[i] = a * (1.0 - std::exp(-toneCurveX_[i] / k));\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > + */\n> > +void WideDynamicRange::queueRequest([[maybe_unused]] IPAContext &context,\n> > +                                   [[maybe_unused]] const uint32_t frame,\n> > +                                   IPAFrameContext &frameContext,\n> > +                                   const ControlList &controls)\n> > +{\n> > +       strength_ = controls.get(controls::draft::WdrStrength).value_or(strength_);\n> > +       exposureConstraintMaxBrightPixels_ =\n> > +               controls.get(controls::draft::WdrMaxBrightPixels)\n> > +                       .value_or(exposureConstraintMaxBrightPixels_);\n> > +\n> > +       const auto &mode = controls.get(controls::draft::WdrMode);\n> > +       if (mode) {\n> > +               mode_ = static_cast<controls::draft::WdrModeEnum>(*mode);\n> > +       }\n> \n> No { } required here.\n\nzzz\n\n> \n> > +\n> > +       frameContext.wdr.mode = mode_;\n> > +       frameContext.wdr.strength = strength_;\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > + */\n> > +void WideDynamicRange::prepare(IPAContext &context,\n> > +                              [[maybe_unused]] const uint32_t frame,\n> > +                              IPAFrameContext &frameContext,\n> > +                              RkISP1Params *params)\n> > +{\n> > +       if (!params) {\n> > +               LOG(RkISP1Wdr, Warning) << \"Params is null\";\n> > +               return;\n> > +       }\n> > +\n> > +       auto config = params->block<BlockType::Wdr>();\n> > +\n> > +       auto mode = frameContext.wdr.mode;\n> > +\n> > +       config.setEnabled(mode != controls::draft::WdrOff);\n> > +\n> > +       double comp = 0;\n> > +\n> > +       /*\n> > +        * Todo: This overwrites frameContext.agc.exposureValue so that\n> > +        * in the next call to Agc::process() that exposureValue get's\n> > +        * applied. In the future it is planned to move the exposure\n> > +        * calculations from Agc::process() to Agc::prepare(). In this\n> > +        * case, we need to ensure that this code get's called early enough.\n> > +        */\n> \n> Eeep ...\n>   /me looks away for the moment.\n\n/me At least I should fix \\todo. But the underlying problem is actually\nquite nasty as we might need to have multiple prepare runs and some kind\nof dependency tracking. That is for another day.\n\n> \n> > +       if (mode != controls::draft::WdrOff) {\n> > +               frameContext.wdr.wdrExposureValue = context.activeState.wdr.exposureValue;\n> > +               frameContext.wdr.agcExposureValue = frameContext.agc.exposureValue;\n> > +\n> > +               /*\n> > +                * When WDR is enabled, the maxBrightPixels constraint is always\n> > +                * active. This is problematic when the user sets a positive\n> > +                * exposureValue. As this would mean that the negative WDR\n> > +                * exposure value and the user provided positive value should\n> > +                * cancel each other out. But as the regulation is not absolute\n> > +                * and only dependent on the measured changes in the histogram,\n> > +                * reducing the wdr EV would only lead to an even stronger pull\n> > +                * from regulation. Positive user supplied EVs must therefore be\n> > +                * handled using the wdr curve.\n> > +                */\n> > +               if (frameContext.wdr.wdrExposureValue < 0) {\n> > +                       frameContext.agc.exposureValue =\n> > +                               std::min(frameContext.agc.exposureValue,\n> > +                                        frameContext.wdr.wdrExposureValue);\n> > +                       comp = frameContext.agc.exposureValue - frameContext.wdr.agcExposureValue;\n> > +               }\n> > +       }\n> > +\n> > +       /* Calculate how much EV we need to compensate with the WDR curve. */\n> > +       double gain = pow(2.0, -comp);\n> > +\n> > +       if (mode == controls::draft::WdrOff) {\n> > +               applyCompensationLinear(1.0, 0.0);\n> > +       } else if (mode == controls::draft::WdrLinear) {\n> > +               applyCompensationLinear(gain, frameContext.wdr.strength);\n> > +       } else if (mode == controls::draft::WdrPower) {\n> > +               applyCompensationPower(gain, frameContext.wdr.strength);\n> > +       } else if (mode == controls::draft::WdrExponential) {\n> > +               applyCompensationExponential(gain, frameContext.wdr.strength);\n> > +       } else if (mode == controls::draft::WdrHistogramEqualization) {\n> > +               applyHistogramEqualization(frameContext.wdr.strength);\n> > +       }\n> > +\n> > +       /* Reset value */\n> > +       config->dmin_strength = 0x10;\n> > +       config->dmin_thresh = 0;\n> > +\n> > +       for (unsigned int i = 0; i < kTonecurveXIntervals; i++) {\n> > +               int v = toneCurveIntervalValues_[i];\n> > +               config->tone_curve.dY[i / 8] |= (v & 0x07) << ((i % 8) * 4);\n> > +       }\n> > +\n> > +       std::vector<Point> debugCurve;\n> > +\n> > +       /*\n> > +        * Fix the curve to adhere to the hardware constraints. Don't apply a\n> > +        * constraint on the first element, which is most likely zero anyways.\n> > +        */\n> > +       int lastY = toneCurveY_[0] * 4096.0;\n> > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++) {\n> > +               int diff = static_cast<int>(toneCurveY_[i] * 4096.0) - lastY;\n> > +               diff = std::clamp(diff, -2048, 2048);\n> > +               lastY = lastY + diff;\n> > +               config->tone_curve.ym[i] = lastY;\n> > +               debugCurve.push_back({ static_cast<int>(toneCurveX_[i] * 4096.0),\n> > +                                      lastY });\n> > +       }\n> > +\n> > +       context.debugMetadata.set<Span<const Point>>(controls::debug::WdrCurve, debugCurve);\n> > +}\n> > +\n> > +void WideDynamicRange::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > +                              IPAFrameContext &frameContext,\n> > +                              const rkisp1_stat_buffer *stats,\n> > +                              ControlList &metadata)\n> > +{\n> > +       if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_HIST)) {\n> > +               LOG(RkISP1Wdr, Error) << \"No histogram data in statistics\";\n> \n> Is this a full error if this happens? or just a debug print ? I thought\n> sometimes on say a too dark image we might not get any (usable) stats ?\n\nWe can make it a warning. But histogram should always be filled. That is\ndifferent from the AWB measurements where pixels can be excluded from\nthe calculation.\n\n> \n> > +               return;\n> > +       }\n> > +\n> > +       if (!started_) {\n> > +               started_ = true;\n> > +               pid_.setStandardParameters(1, 5.0, 3.0);\n> > +               pid_.setOutputLimits(minExposureValue_, 0.0);\n> \n> Can the minExposureValue_ ever be changed for instance if we get called\n> with different modes in configure() or if the frame duration limits get\n> changed ?\n\nIMHO not as it is read from the tuning file only.\n\n> \n> > +               context.activeState.wdr.exposureValue = 0.0;\n> \n> Should those happen during configure() ? or init() ?\n> \n>  - init just once when constructed... (current behaviour)\n>  - or configure if every time the camera is reconfigured..\n> \n> Then we wouldn't need a started_ bool tracker ?\n\nGood point. I think it should be reset in configure() (which is missing\nat the moment). I would expect (correct me if you do differently) the\nalgorithm to keep state between stop()/start() calls on the camera but to\nreset internal state on configure(). That is actually something we do\nnot test very well (or at all). In camshark it is always a complete tear\ndown. That needs to get fixed.\n\nSo here I'd like to add started_=false to configure() for now. I could\nalso move the setup code into configure which would remove the need for\nthe started_ variable. The reason why I prefer to having it here is a\npractical one. While working on the regulation I'd like to keep all the\nrelevant pieces in one place and not scattered over multiple places. And\nI expect us to do more work on that regulation as it is still quite\nslow.\n\n> \n> > +       }\n> > +\n> > +       pid_.setTarget(exposureConstraintY_);\n> > +\n> > +       const rkisp1_cif_isp_stat *params = &stats->params;\n> > +       auto mode = frameContext.wdr.mode;\n> > +\n> > +       metadata.set(controls::draft::WdrMode, mode);\n> > +\n> > +       Histogram cumHist({ params->hist.hist_bins, context.hw->numHistogramBins },\n> > +                         [](uint32_t x) { return x >> 4; });\n> > +\n> > +       std::vector<double> hist;\n> > +       double sum = 0;\n> > +       for (unsigned i = 0; i < context.hw->numHistogramBins; i++) {\n> > +               double v = params->hist.hist_bins[i] >> 4;\n> > +               hist.push_back(v);\n> > +               sum += v;\n> > +       }\n> > +\n> > +       /* Scale so that the entries sum up to 1. */\n> > +       double scale = 1.0 / sum;\n> > +       for (auto &v : hist)\n> > +               v *= scale;\n> > +       hist_.swap(hist);\n> > +\n> > +       double mean = cumHist.quantile(1.0 - exposureConstraintMaxBrightPixels_) /\n> > +                     cumHist.bins();\n> > +       LOG(RkISP1Wdr, Debug) << \"Mean y of bright pixels: \" << mean;\n> > +\n> > +       if (mode == controls::draft::WdrOff) {\n> > +               metadata.set(controls::draft::WdrExposureValue, 0);\n> > +               return;\n> > +       }\n> > +\n> > +       context.activeState.wdr.exposureValue = pid_.process(mean);\n> > +       LOG(RkISP1Wdr, Debug) << \"Active state WDR ev: \" << context.activeState.wdr.exposureValue;\n> > +       metadata.set(controls::draft::WdrExposureValue, frameContext.wdr.wdrExposureValue);\n> > +\n> > +       /*\n> > +        * We overwrote agc exposure value in prepare() to create an\n> > +        * underexposure for WDR. Report the original exposure value in metadata.\n> > +        */\n> > +       metadata.set(controls::ExposureValue, frameContext.wdr.agcExposureValue);\n> > +}\n> > +\n> > +REGISTER_IPA_ALGORITHM(WideDynamicRange, \"WideDynamicRange\")\n> > +\n> > +} /* namespace ipa::rkisp1::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/algorithms/wdr.h b/src/ipa/rkisp1/algorithms/wdr.h\n> > new file mode 100644\n> > index 000000000000..bdea4eb5492a\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/wdr.h\n> > @@ -0,0 +1,64 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021-2022, Ideas On Board\n> > + *\n> > + * RkISP1 Wide Dynamic Range control\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include <libcamera/control_ids.h>\n> > +\n> > +#include \"libcamera/internal/pid_controller.h\"\n> > +\n> > +#include \"linux/rkisp1-config.h\"\n> > +\n> > +#include \"algorithm.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +class WideDynamicRange : public Algorithm\n> > +{\n> > +public:\n> > +       WideDynamicRange();\n> > +       ~WideDynamicRange() = default;\n> > +\n> > +       int init(IPAContext &context, const YamlObject &tuningData) override;\n> > +\n> > +       void queueRequest(IPAContext &context, 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> > +                    RkISP1Params *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> > +       Vector<double, 2> kneePoint(double gain, double strength);\n> > +       void applyCompensationLinear(double gain, double strength);\n> > +       void applyCompensationPower(double gain, double strength);\n> > +       void applyCompensationExponential(double gain, double strength);\n> > +       void applyHistogramEqualization(double strength);\n> > +\n> > +       double exposureConstraintMaxBrightPixels_;\n> > +       double exposureConstraintY_;\n> > +       double minExposureValue_;\n> > +       double strength_;\n> > +       bool started_ = false;\n> > +\n> > +       controls::draft::WdrModeEnum mode_;\n> > +       std::vector<double> hist_;\n> > +       PidController pid_;\n> > +\n> > +       std::array<int, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV> toneCurveIntervalValues_;\n> > +       std::array<double, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1> toneCurveX_;\n> > +       std::array<double, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1> toneCurveY_;\n> > +};\n> > +\n> > +} /* namespace ipa::rkisp1::algorithms */\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 60cfab228edf..32f6db30bbdb 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -130,6 +130,10 @@ struct IPAActiveState {\n> >         struct {\n> >                 double gamma;\n> >         } goc;\n> > +\n> > +       struct {\n> > +               double exposureValue;\n> > +       } wdr;\n> >  };\n> >  \n> >  struct IPAFrameContext : public FrameContext {\n> > @@ -198,6 +202,13 @@ struct IPAFrameContext : public FrameContext {\n> >         struct {\n> >                 double lux;\n> >         } lux;\n> > +\n> > +       struct {\n> > +               controls::draft::WdrModeEnum mode;\n> > +               double wdrExposureValue;\n> > +               double agcExposureValue;\n> > +               double strength;\n> > +       } wdr;\n> >  };\n> >  \n> >  struct IPAContext {\n> > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> > index 4c0b051ce65d..5edb36c91b87 100644\n> > --- a/src/ipa/rkisp1/params.cpp\n> > +++ b/src/ipa/rkisp1/params.cpp\n> > @@ -74,6 +74,7 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {\n> >         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandBls, COMPAND_BLS, compand_bls),\n> >         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandExpand, COMPAND_EXPAND, compand_curve),\n> >         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandCompress, COMPAND_COMPRESS, compand_curve),\n> > +       RKISP1_BLOCK_TYPE_ENTRY_EXT(Wdr, WDR, wdr),\n> >  };\n> >  \n> >  } /* namespace */\n> > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> > index 40450e34497a..2e60528d102e 100644\n> > --- a/src/ipa/rkisp1/params.h\n> > +++ b/src/ipa/rkisp1/params.h\n> > @@ -40,6 +40,7 @@ enum class BlockType {\n> >         CompandBls,\n> >         CompandExpand,\n> >         CompandCompress,\n> > +       Wdr,\n> >  };\n> >  \n> >  namespace details {\n> > @@ -74,6 +75,7 @@ RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)\n> >  RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)\n> >  RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)\n> >  RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n> > +RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr)\n> >  \n> >  } /* namespace details */\n> >  \n> > diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml\n> > index 797532712099..9489e677402e 100644\n> > --- a/src/libcamera/control_ids_debug.yaml\n> > +++ b/src/libcamera/control_ids_debug.yaml\n> > @@ -3,4 +3,9 @@\n> >  %YAML 1.1\n> >  ---\n> >  vendor: debug\n> > -controls: []\n> > +controls:\n> > +- WdrCurve:\n> > +    type: Point\n> > +    direction: out\n> > +    description: Debug control WdrCurve found in src/ipa/rkisp1/algorithms/wdr.cpp\n> > +    size: '[n]'\n> \n> I love the idea of having debug controls that let us plot debug\n> internals - but if these are generated by script - do we need a way to\n> make sure they are only ever append only - if we ever merge a debug\n> control - suddenly the id number becomes part of the ABI - and adding\n> new debug controls can't change the existing ABI. Could be handled with\n> manual review ... but could then irritate the automatic tools for\n> createing the controls in the first place ...\n\nOhhh bummer. I need to think about that. We definitely shouldn't require\nthem to be append only. That would lead to a massive mess. Do we do the\nsame for draft controls? Is there no way to deprecate a control?\n\n> \n> \n> > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > index 03309eeac34f..0c0af7a19968 100644\n> > --- a/src/libcamera/control_ids_draft.yaml\n> > +++ b/src/libcamera/control_ids_draft.yaml\n> > @@ -293,5 +293,75 @@ controls:\n> >  \n> >          Currently identical to ANDROID_STATISTICS_FACE_IDS.\n> >        size: [n]\n> > +  - WdrMode:\n> \n> Can we avoid these going into ::draft:: at all ?\n> \n> > +      type: int32_t\n> > +      direction: inout\n> > +      description: |\n> > +        Set the WDR mode.\n> > +\n> > +        The WDR mode is used to select the algorithm used for global tone\n> > +        mapping. It will automatically reduce the exposure time of the sensor\n> > +        so that there are only a small number of saturated pixels in the image.\n> > +        The algorithm then compensates for the loss of brightness by applying a\n> > +        global tone mapping curve to the image.\n> > +      enum:\n> > +        - name: WdrOff\n> > +          value: 0\n> > +          description: Wdr is disabled.\n> > +        - name: WdrLinear\n> > +          value: 1\n> > +          description:\n> > +            Apply a linear global tone mapping curve.\n> \n> I'm torn here on naming. Cue the bikeshed door opening ...\n\nI'm putting on my safety gear... :-)\n\n> \n> But we use the control name 'Wdr' and say it applies 'Global Tone\n> Mapping' configurations.\n> \n> Is WDR sufficiently known to represent Global Tone Mapping - are the\n> terms sufficiently interchangable ?\n\nThat is not really easy to solve. I googled \"WDR vs HDR\" which lead to\neverything. Articles that contradict each other, pure nonsense and\ncompany specific nomenclature. I would not limit it to global tone\nmapping only - that's only what we have right now but I'd love to see\nlocal tone mapping added to the area of \"libcamera WDR/HDR controls\". As\nanother side note, the upcoming HDR stitcher support will not expose own\ncontrols but just plug into these Wdr* controls.\n\nWe could discuss if we lean more to HDR instead of WDR... My gut feeling\nis that WDR is a broead spectrum of things to increase the perceived\ndynamic range (Local tone mapping, global tone mapping, single low\nexposure + digital gain, on-chip fusion, multi-exposure) while with HDR\nI think of at least two exposures. But I'm sure you'll find a lot of\narticles/people disagreeing here... so... don't know :-)\n\n> \n> \n> Do we have other hardware with 'WDR' capabilities to compare if the\n> controls make sense on other devices already?\n> \n> Dan - I expect the Mali-C55 to be quite HDR capable ... is there a WDR\n> block there that would benefit/utilise the same controls and concepts?\n> \n> \n> \n> > +\n> > +            The curve with two linear sections is applied. This produces good\n> > +            results at the expense of a slightly artificial look.\n> > +        - name: WdrPower\n> > +          value: 2\n> > +          description: |\n> > +            Apply a power global tone mapping curve.\n> > +\n> > +            This curve has high gain values on the dark areas of an image and\n> > +            high compression values on the bright area. It therefore tends to\n> > +            produce noticeable noise artifacts.\n> > +        - name: WdrExponential\n> > +          value: 3\n> > +          description: |\n> > +            Apply a exponential global tone mapping curve.\n> > +\n> > +            This curve has lower gain values in dark areas compared to the power\n> > +            curve but produces a more natural look compared to the linear curve.\n> > +            It is therefore the best choice for most scenes.\n> > +        - name: WdrHistogramEqualization\n> > +          value: 4\n> > +          description: |\n> > +            Apply histogram equalization.\n> > +\n> > +            This curve preserves most of the information of the image at the\n> > +            expense of a very artificial look. It is therefore best suited for\n> > +            technical analysis.\n> > +  - WdrStrength:\n> > +      type: float\n> > +      direction: in\n> > +      description: |\n> > +        Specify the strength of the wdr algorithm. The exact meaning of this\n> > +        value is specific to the algorithm in use. Usually a value of 0 means no\n> > +        global tone mapping is applied. A values of 1 is the default value and\n> > +        the correct value for most scenes. A value above 1 increases the global\n> > +        tone mapping effect and can lead to unrealistic image effects.\n> > +  - WdrMaxBrightPixels:\n> > +      type: float\n> > +      direction: in\n> > +      description: |\n> > +        Percentage of allowed (nearly) saturated pixels. The WDR algorithm\n> > +        reduces the WdrExposureValue until the amount of pixels that are close\n> > +        to saturation is lower than this value.\n> > +  - WdrExposureValue:\n> > +      type: float\n> > +      direction: out\n> > +      description: |\n> > +        Reports the Exposure Value that was applied to the AEGC regulation so\n> > +        that the WdrMaxBrightPixels limit is reached.\n> \n> Is that /different/ to the ExposureValue already reported in metadata?\n\nYes, the ExposureValue is applied by the user leading to a over/under\nexposed output image. WdrExposureValue is applied under the hood and\nequalized by tone mapping algorithm. So a WdrExposureValue of -2\nbasically says \"There are very bright areas in the image, I need to work\nhard\" but the output is still exposed as with ExposureValue=0. I don't\nknow if that will be the approach taken for other ISPs as it is based on\nthe limitations of the imx8mp ISP. I think time will tell.\n\nBest regards,\nStefan\n\n> \n> > +\n> > +        \\sa ExposureValue\n> >  \n> >  ...\n> > -- \n> > 2.48.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 87FF0BDCC1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 13 Aug 2025 10:41:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3854269251;\n\tWed, 13 Aug 2025 12:41:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 402FB69249\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 13 Aug 2025 12:41:36 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:2c15:6671:9c33:a3cb])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id BE9A2346;\n\tWed, 13 Aug 2025 12:40:42 +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=\"Oym0J6hu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755081642;\n\tbh=r7bQJFBa0+lXpIWj450OJ7+askjCUvXHFNT5wlJRPS8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Oym0J6huJzT5W0A0Ld7p5JvtVu0OznFvsJqUkI2Lv/oEalCgHWflNr/qw1zCMZafU\n\t7PVfluJc/84WCKNLqCuaWE/23evnGHVihl42ETmUevYXxtg3xI0oCmOHX70IdYy8bd\n\tmId/so/DXgi7YNpuM6u0PhJVH/roDnaU7Ctf98Hc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175493316838.1641235.2414681416886447452@ping.linuxembedded.co.uk>","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-16-stefan.klug@ideasonboard.com>\n\t<175493316838.1641235.2414681416886447452@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v2 15/16] ipa: rkisp1: Add WDR algorithm","From":"Stefan Klug <stefan.klug@ideasonboard.com>","Cc":"","To":"Dan Scally <dan.scally@ideasonboard.com>,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 13 Aug 2025 12:41:33 +0200","Message-ID":"<175508169312.1225443.2627596737853254972@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":35377,"web_url":"https://patchwork.libcamera.org/comment/35377/","msgid":"<175508279425.560048.16769889226335004715@ping.linuxembedded.co.uk>","date":"2025-08-13T10:59:54","subject":"Re: [PATCH v2 15/16] ipa: rkisp1: Add WDR algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-08-13 11:41:33)\n> Hi Kieran,\n> \n> Thank you for the review.\n> \n> Quoting Kieran Bingham (2025-08-11 19:26:08)\n> > Dan - hidden question to you way at the bottom of this one!\n> > \n> > Quoting Stefan Klug (2025-08-08 15:12:53)\n> > > Add a WDR algorithm to do global tone mapping. Global tone mapping is\n> > > used to increase the perceived dynamic range of an image. The typical\n> > > effect is that in areas that are normally overexposed, additional\n> > > structure becomes visible.\n> > > \n> > > The overall idea is that the algorithm applies an exposure value\n> > > correction to underexpose the image to the point where only a small\n> > > number of saturated pixels is left. This artificial underexposure is\n> > > then mitigated by applying a tone mapping curve.\n> > > \n> > > This algorithm implements 4 tone mapping strategies:\n> > > - Linear\n> > > - Power\n> > > - Exponential\n> > > - Histogram equalization\n> > > \n> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > > \n> > > ---\n> > > \n> > > Changes in v2:\n> > > - Fixed default value for min bright pixels\n> > > - Added check for supported params type\n> > > - Reset PID controller\n> > > - Various fixes from Pauls review\n> > > ---\n> > >  src/ipa/rkisp1/algorithms/meson.build |   1 +\n> > >  src/ipa/rkisp1/algorithms/wdr.cpp     | 510 ++++++++++++++++++++++++++\n> > >  src/ipa/rkisp1/algorithms/wdr.h       |  64 ++++\n> > >  src/ipa/rkisp1/ipa_context.h          |  11 +\n> > >  src/ipa/rkisp1/params.cpp             |   1 +\n> > >  src/ipa/rkisp1/params.h               |   2 +\n> > >  src/libcamera/control_ids_debug.yaml  |   7 +-\n> > >  src/libcamera/control_ids_draft.yaml  |  70 ++++\n> > >  8 files changed, 665 insertions(+), 1 deletion(-)\n> > >  create mode 100644 src/ipa/rkisp1/algorithms/wdr.cpp\n> > >  create mode 100644 src/ipa/rkisp1/algorithms/wdr.h\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > > index 2e42a80cf99d..d329dbfb432d 100644\n> > > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > > @@ -14,4 +14,5 @@ rkisp1_ipa_algorithms = files([\n> > >      'gsl.cpp',\n> > >      'lsc.cpp',\n> > >      'lux.cpp',\n> > > +    'wdr.cpp',\n> > >  ])\n> > > diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp\n> > > new file mode 100644\n> > > index 000000000000..0c1b261d4373\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/algorithms/wdr.cpp\n> > > @@ -0,0 +1,510 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2025, Ideas On Board\n> > > + *\n> > > + * RkISP1 Wide Dynamic Range control\n> > > + */\n> > > +\n> > > +#include \"wdr.h\"\n> > > +\n> > > +#include <libcamera/base/log.h>\n> > > +#include <libcamera/base/utils.h>\n> > > +\n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > +\n> > > +#include <libipa/histogram.h>\n> > > +#include <libipa/pwl.h>\n> > > +\n> > > +#include \"linux/rkisp1-config.h\"\n> > > +\n> > > +/**\n> > > + * \\file wdr.h\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::algorithms {\n> > > +\n> > > +/**\n> > > + * \\class WideDynamicRange\n> > > + * \\brief RkISP1 Wide Dynamic Range algorithm\n> > > + *\n> > > + * This algorithm implements automatic global tone mapping for the RkISP1.\n> > > + * Global tone mapping is done by the GWDR hardware block and applies\n> > > + * a global tone mapping curve to the image to increase the perceived dynamic\n> > > + * range. Imagine an indoor scene with bright outside visible through the\n> > > + * windows. With normal exposure settings, the windows will be completely\n> > > + * saturated and no structure (sky/clouds) will be visible because the AEGC has\n> > > + * to increase overall exposure to reach a certain level of mean brightness. In\n> > > + * WDR mode, the algorithm will artifically reduce the exposure time so that the\n> > > + * texture and colours become visible in the formerly saturated areas. Then the\n> > > + * global tone mapping curve is applied to mitigate the loss of brightness.\n> > > + *\n> > > + * Calculating that tone mapping curve is the most difficult part. This\n> > > + * algorithm implements four tone mapping strategies:\n> > > + * - Linear: The tone mapping curve is a combination of two linear functions\n> > > + * with one kneepoint\n> > \n> > Can this 'with one kneepiont' be indented to match the Linear ?\n> > \n> > \n> >  * - Linear: The tone mapping curve is a combination of two linear functions\n> >  *   with one kneepoint\n> > \n> \n> ack\n> \n> > \n> > > + * - Power: The tone mapping curve follows a power function\n> > > + * - Exponential: The tone mapping curve follows an exponential function\n> > > + * - HistogramEqualization: The tone mapping curve tries to equalize the\n> > > + * histogram\n> > \n> > Same here.?\n> \n> ack\n> \n> > \n> > > + *\n> > > + * The overall strategy is the same in all cases: A negative exposure value is\n> > > + * applied to the AEGC regulation until the number of nearly saturated pixels go\n> > > + * below a given threshold (controllable via WdrMaxBrightPixels, default is 2%)\n> > > + * or the MinExposureValue specified in the tuning file is reached.\n> > > + *\n> > > + * The global tone mapping curve is then calculated so that it accounts for the\n> > > + * reduction of brightness due to the negative exposure value. As the result of\n> > > + * tone mapping is very difficult to quantize and is by definition a lossy\n> > > + * process there is not a single \"correct\" solution.\n> > > + *\n> > > + * The approach taken here is based on a simple linear model. Consider a pixel\n> > > + * that was originally 50% grey. It will have its exposure pushed down by the\n> > > + * WDR's initial exposure compensation. This value then needs to be pushed back\n> > > + * up by the tone mapping curve so that it is 50% grey again. This point serves\n> > > + * as our kneepoint. To get to this kneepoint, this pixel and all darker pixels\n> > > + * (to the left of the kneepoint on the tone mapping curve) will simply have the\n> > > + * exposure compensation undone by pow(2, -WDR_EV). This cancels out the\n> > > + * original exposure compensation, which was pow(2, WDR_EV). The remaining\n> > > + * brigher pixels (to the right of the kneepoint on the tone mapping curve) will\n> > > + * be compressed. The WdrStrength control adjusts the gain of the left part of\n> > > + * the tone mapping curve.\n> > > + *\n> > > + * In the Power and Exponential modes, the curves are calculated so that they\n> > > + * pass through that kneepoint.\n> > > + *\n> > > + * The histogram equalization mode tries to equalize the histogram of the\n> > > + * image and acts independently of the calculated exposure value.\n> > > + *\n> > > + * \\code{.unparsed}\n> > > + * algorithms:\n> > > + *   - WideDynamicRange:\n> > > + *       ExposureConstraint:\n> > > + *         MaxBrightPixels: 0.02\n> > > + *         yTarget: 0.95\n> > > + *       MinExposureValue: -4.0\n> > > + * \\endcode\n> > > + */\n> > > +\n> > > +LOG_DEFINE_CATEGORY(RkISP1Wdr)\n> > > +\n> > > +static constexpr unsigned int kTonecurveXIntervals = RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV;\n> > > +\n> > > +/*\n> > > + * Increasing interval sizes. The intervals are crafted so that they sum\n> > > + * up to 4096. This results in better fitting curves than the constant intervals\n> > > + * (all entries are 4)\n> > > + */\n> > > +static constexpr std::array<int, kTonecurveXIntervals> kLoglikeIntervals = {\n> > > +       { 0, 0, 0, 0, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3, 3, 4,\n> > > +         4, 4, 4, 4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 5, 6, 6 }\n> > > +};\n> > > +\n> > > +WideDynamicRange::WideDynamicRange()\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > + */\n> > > +int WideDynamicRange::init([[maybe_unused]] IPAContext &context,\n> > > +                          [[maybe_unused]] const YamlObject &tuningData)\n> > > +{\n> > > +       if (!(context.hw->supportedBlocks & 1 << RKISP1_EXT_PARAMS_BLOCK_TYPE_WDR)) {\n> > > +               LOG(RkISP1Wdr, Error)\n> > > +                       << \"Wide Dynamic Range not supported by the hardware or kernel.\";\n> > > +               return -ENOTSUP;\n> > > +       }\n> > > +\n> > > +       toneCurveIntervalValues_ = kLoglikeIntervals;\n> > > +\n> > > +       /* Calculate a list of normed x values */\n> > > +       toneCurveX_[0] = 0.0;\n> > > +       int lastValue = 0;\n> > > +       for (unsigned int i = 1; i < toneCurveX_.size(); i++) {\n> > > +               lastValue += std::pow(2, toneCurveIntervalValues_[i - 1] + 3);\n> > > +               lastValue = std::min(lastValue, 4096);\n> > > +               toneCurveX_[i] = lastValue / 4096.0;\n> > > +       }\n> > > +\n> > > +       strength_ = 1.0;\n> > > +       mode_ = controls::draft::WdrOff;\n> > > +       exposureConstraintMaxBrightPixels_ = 0.02;\n> > > +       exposureConstraintY_ = 0.95;\n> > > +       minExposureValue_ = -4.0;\n> > > +\n> > > +       const auto &constraint = tuningData[\"ExposureConstraint\"];\n> > > +       if (!constraint.isDictionary()) {\n> > > +               LOG(RkISP1Wdr, Warning)\n> > > +                       << \"ExposureConstraint not found in tuning data.\"\n> > > +                          \"Using default values MaxBrightPixels: \"\n> > > +                       << exposureConstraintMaxBrightPixels_\n> > > +                       << \" yTarget: \" << exposureConstraintY_;\n> > > +       } else {\n> > > +               exposureConstraintMaxBrightPixels_ =\n> > > +                       constraint[\"MaxBrightPixels\"]\n> > > +                               .get<double>()\n> > > +                               .value_or(exposureConstraintMaxBrightPixels_);\n> > > +               exposureConstraintY_ =\n> > > +                       constraint[\"yTarget\"]\n> > > +                               .get<double>()\n> > > +                               .value_or(exposureConstraintY_);\n> > > +       }\n> > > +\n> > > +       const auto &minExp = tuningData[\"MinExposureValue\"];\n> > > +       minExposureValue_ = minExp.get<double>().value_or(minExposureValue_);\n> > > +       if (!minExp) {\n> > > +               LOG(RkISP1Wdr, Warning)\n> > > +                       << \"MinExposureValue not found in tuning data.\"\n> > > +                          \" Using default value: \"\n> > > +                       << minExposureValue_;\n> > > +       }\n> > \n> > { } aren't required for single statement...\n> \n> Argh I don't get these to become a habit :-)\n> \n> > \n> > > +\n> > > +       context.ctrlMap[&controls::draft::WdrMode] =\n> > > +               ControlInfo(controls::draft::WdrModeValues, controls::draft::WdrOff);\n> > > +       context.ctrlMap[&controls::draft::WdrStrength] =\n> > > +               ControlInfo(0.0f, 2.0f, static_cast<float>(strength_));\n> > > +       context.ctrlMap[&controls::draft::WdrMaxBrightPixels] =\n> > > +               ControlInfo(0.0f, 1.0f, static_cast<float>(exposureConstraintMaxBrightPixels_));\n> > > +\n> > > +       applyCompensationLinear(1.0, 0.0);\n> > > +\n> > > +       return 0;\n> > > +}\n> > > +\n> > > +void WideDynamicRange::applyHistogramEqualization(double strength)\n> > > +{\n> > > +       if (hist_.empty())\n> > > +               return;\n> > > +\n> > > +       strength *= 0.65;\n> > \n> > Why 0.65 ?\n> \n> That value was chosen to provide a similar optical impression compared\n> to the other algorithms. I'll revisit and provide some description.\n\nAck\n\n> \n> > \n> > > +\n> > > +       /*\n> > > +        * In a fully equalized histogram, all bins have the same value. Try\n> > > +        * to equalize the histogram by applying a gain or damping depending on\n> > > +        * the distance of the actual bin value from that norm.\n> > > +        */\n> > > +       std::vector<double> gains;\n> > > +       gains.resize(hist_.size());\n> > > +       double sum = 0;\n> > > +       double norm = 1.0 / (gains.size());\n> > > +       for (unsigned i = 0; i < hist_.size(); i++) {\n> > > +               double diff = 1.0 + strength * (hist_[i] - norm) / norm;\n> > > +               gains[i] = diff;\n> > > +               sum += diff;\n> > > +       }\n> > > +       /* Never amplify the last entry. */\n> > > +       gains.back() = std::max(gains.back(), 1.0);\n> > > +\n> > > +       double scale = gains.size() / sum;\n> > > +       for (auto &v : gains)\n> > > +               v *= scale;\n> > > +\n> > > +       Pwl pwl;\n> > > +       double step = 1.0 / gains.size();\n> > > +       double lastX = 0;\n> > > +       double lastY = 0;\n> > > +\n> > > +       pwl.append(lastX, lastY);\n> > > +       for (unsigned int i = 0; i < gains.size() - 1; i++) {\n> > > +               lastY += gains[i] * step;\n> > > +               lastX += step;\n> > > +               pwl.append(lastX, lastY);\n> > > +       }\n> > > +       pwl.append(1.0, 1.0);\n> > > +\n> > > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> > > +               toneCurveY_[i] = pwl.eval(toneCurveX_[i]);\n> > > +}\n> > > +\n> > > +Vector<double, 2> WideDynamicRange::kneePoint(double gain, double strength)\n> > > +{\n> > > +       gain = std::pow(gain, strength);\n> > > +       double y = 0.5;\n> > > +       double x = y / gain;\n> > > +\n> > > +       return { { x, y } };\n> > > +}\n> > > +\n> > > +void WideDynamicRange::applyCompensationLinear(double gain, double strength)\n> > > +{\n> > > +       auto kp = kneePoint(gain, strength);\n> > > +       double g1 = kp.y() / kp.x();\n> > > +       double g2 = (kp.y() - 1) / (kp.x() - 1);\n> > > +\n> > > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++) {\n> > > +               double x = toneCurveX_[i];\n> > > +               double y;\n> > > +               if (x <= kp.x()) {\n> > > +                       y = g1 * x;\n> > > +               } else {\n> > > +                       y = g2 * x + 1 - g2;\n> > > +               }\n> > > +               toneCurveY_[i] = y;\n> > > +       }\n> > > +}\n> > > +\n> > > +void WideDynamicRange::applyCompensationPower(double gain, double strength)\n> > > +{\n> > > +       double e = 1.0;\n> > > +       if (strength > 1e-6) {\n> > > +               auto kp = kneePoint(gain, strength);\n> > > +               /* Calculate an exponent to go through the knee point. */\n> > > +               e = log(kp.y()) / log(kp.x());\n> > > +       }\n> > > +\n> > > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> > > +               toneCurveY_[i] = std::pow(toneCurveX_[i], e);\n> > > +}\n> > > +\n> > > +void WideDynamicRange::applyCompensationExponential(double gain, double strength)\n> > > +{\n> > > +       double k = 0.1;\n> > > +       auto kp = kneePoint(gain, strength);\n> > > +       double kx = kp.x();\n> > > +       double ky = kp.y();\n> > > +\n> > > +       if (kx > ky) {\n> > > +               LOG(RkISP1Wdr, Warning) << \"Invalid knee point: \" << kp;\n> > > +               kx = ky;\n> > > +       }\n> > > +\n> > > +       /*\n> > > +        * The exponential curve is based on the function proposed by Glozman\n> > > +        * et al. in\n> > > +        * S. Glozman, T. Kats, and O. Yadid-Pecht, \"Exponent Operator Based\n> > > +        * Tone Mapping Algorithm for Color Wide Dynamic Range Images,\" 2011.\n> > > +        *\n> > > +        * That function uses a k factor as parameter for the WDR compression\n> > > +        * curve:\n> > > +        * k=0: maximum compression\n> > > +        * k=infinity: linear curve\n> > > +        *\n> > > +        * To calculate a k factor that results in a curve that passes through\n> > > +        * the kneepoint, the equation needs to be solved for k after inserting\n> > > +        * the kneepoint.  This can be formulated as search for a zero point.\n> > > +        * Unfortunately there is no closed solution for that transformation.\n> > > +        * Using newton's method to approximate the value is numerically\n> > > +        * unstable.\n> > > +        *\n> > > +        * Luckily the function only crosses the x axis once and for the set of\n> > > +        * possible kneepoints, a negative and a positive point can be guessed.\n> > > +        * The approximation is then implemented using bisection.\n> > > +        */\n> > > +       if (std::abs(kx - ky) < 0.001) {\n> > > +               k = 1e8;\n> > > +       } else {\n> > > +               double kl = 0.0001;\n> > > +               double kh = 1000;\n> > > +\n> > > +               auto fk = [=](double v) {\n> > > +                       return std::exp(-kx / v) -\n> > > +                              ky * std::exp(-1.0 / v) + ky - 1.0;\n> > > +               };\n> > > +\n> > > +               ASSERT(fk(kl) < 0);\n> > > +               ASSERT(fk(kh) > 0);\n> > > +\n> > > +               k = kh / 10.0;\n> > > +               while (fk(k) > 0) {\n> > > +                       kh = k;\n> > > +                       k /= 10.0;\n> > > +               }\n> > > +\n> > > +               do {\n> > > +                       k = (kl + kh) / 2;\n> > > +                       if (fk(k) < 0)\n> > > +                               kl = k;\n> > > +                       else\n> > > +                               kh = k;\n> > > +               } while (std::abs(kh - kl) > 1e-3);\n> > > +       }\n> > > +\n> > > +       double a = 1.0 / (1.0 - std::exp(-1.0 / k));\n> > > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> > > +               toneCurveY_[i] = a * (1.0 - std::exp(-toneCurveX_[i] / k));\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > > + */\n> > > +void WideDynamicRange::queueRequest([[maybe_unused]] IPAContext &context,\n> > > +                                   [[maybe_unused]] const uint32_t frame,\n> > > +                                   IPAFrameContext &frameContext,\n> > > +                                   const ControlList &controls)\n> > > +{\n> > > +       strength_ = controls.get(controls::draft::WdrStrength).value_or(strength_);\n> > > +       exposureConstraintMaxBrightPixels_ =\n> > > +               controls.get(controls::draft::WdrMaxBrightPixels)\n> > > +                       .value_or(exposureConstraintMaxBrightPixels_);\n> > > +\n> > > +       const auto &mode = controls.get(controls::draft::WdrMode);\n> > > +       if (mode) {\n> > > +               mode_ = static_cast<controls::draft::WdrModeEnum>(*mode);\n> > > +       }\n> > \n> > No { } required here.\n> \n> zzz\n> \n> > \n> > > +\n> > > +       frameContext.wdr.mode = mode_;\n> > > +       frameContext.wdr.strength = strength_;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > + */\n> > > +void WideDynamicRange::prepare(IPAContext &context,\n> > > +                              [[maybe_unused]] const uint32_t frame,\n> > > +                              IPAFrameContext &frameContext,\n> > > +                              RkISP1Params *params)\n> > > +{\n> > > +       if (!params) {\n> > > +               LOG(RkISP1Wdr, Warning) << \"Params is null\";\n> > > +               return;\n> > > +       }\n> > > +\n> > > +       auto config = params->block<BlockType::Wdr>();\n> > > +\n> > > +       auto mode = frameContext.wdr.mode;\n> > > +\n> > > +       config.setEnabled(mode != controls::draft::WdrOff);\n> > > +\n> > > +       double comp = 0;\n> > > +\n> > > +       /*\n> > > +        * Todo: This overwrites frameContext.agc.exposureValue so that\n> > > +        * in the next call to Agc::process() that exposureValue get's\n> > > +        * applied. In the future it is planned to move the exposure\n> > > +        * calculations from Agc::process() to Agc::prepare(). In this\n> > > +        * case, we need to ensure that this code get's called early enough.\n> > > +        */\n> > \n> > Eeep ...\n> >   /me looks away for the moment.\n> \n> /me At least I should fix \\todo. But the underlying problem is actually\n> quite nasty as we might need to have multiple prepare runs and some kind\n> of dependency tracking. That is for another day.\n> \n> > \n> > > +       if (mode != controls::draft::WdrOff) {\n> > > +               frameContext.wdr.wdrExposureValue = context.activeState.wdr.exposureValue;\n> > > +               frameContext.wdr.agcExposureValue = frameContext.agc.exposureValue;\n> > > +\n> > > +               /*\n> > > +                * When WDR is enabled, the maxBrightPixels constraint is always\n> > > +                * active. This is problematic when the user sets a positive\n> > > +                * exposureValue. As this would mean that the negative WDR\n> > > +                * exposure value and the user provided positive value should\n> > > +                * cancel each other out. But as the regulation is not absolute\n> > > +                * and only dependent on the measured changes in the histogram,\n> > > +                * reducing the wdr EV would only lead to an even stronger pull\n> > > +                * from regulation. Positive user supplied EVs must therefore be\n> > > +                * handled using the wdr curve.\n> > > +                */\n> > > +               if (frameContext.wdr.wdrExposureValue < 0) {\n> > > +                       frameContext.agc.exposureValue =\n> > > +                               std::min(frameContext.agc.exposureValue,\n> > > +                                        frameContext.wdr.wdrExposureValue);\n> > > +                       comp = frameContext.agc.exposureValue - frameContext.wdr.agcExposureValue;\n> > > +               }\n> > > +       }\n> > > +\n> > > +       /* Calculate how much EV we need to compensate with the WDR curve. */\n> > > +       double gain = pow(2.0, -comp);\n> > > +\n> > > +       if (mode == controls::draft::WdrOff) {\n> > > +               applyCompensationLinear(1.0, 0.0);\n> > > +       } else if (mode == controls::draft::WdrLinear) {\n> > > +               applyCompensationLinear(gain, frameContext.wdr.strength);\n> > > +       } else if (mode == controls::draft::WdrPower) {\n> > > +               applyCompensationPower(gain, frameContext.wdr.strength);\n> > > +       } else if (mode == controls::draft::WdrExponential) {\n> > > +               applyCompensationExponential(gain, frameContext.wdr.strength);\n> > > +       } else if (mode == controls::draft::WdrHistogramEqualization) {\n> > > +               applyHistogramEqualization(frameContext.wdr.strength);\n> > > +       }\n> > > +\n> > > +       /* Reset value */\n> > > +       config->dmin_strength = 0x10;\n> > > +       config->dmin_thresh = 0;\n> > > +\n> > > +       for (unsigned int i = 0; i < kTonecurveXIntervals; i++) {\n> > > +               int v = toneCurveIntervalValues_[i];\n> > > +               config->tone_curve.dY[i / 8] |= (v & 0x07) << ((i % 8) * 4);\n> > > +       }\n> > > +\n> > > +       std::vector<Point> debugCurve;\n> > > +\n> > > +       /*\n> > > +        * Fix the curve to adhere to the hardware constraints. Don't apply a\n> > > +        * constraint on the first element, which is most likely zero anyways.\n> > > +        */\n> > > +       int lastY = toneCurveY_[0] * 4096.0;\n> > > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++) {\n> > > +               int diff = static_cast<int>(toneCurveY_[i] * 4096.0) - lastY;\n> > > +               diff = std::clamp(diff, -2048, 2048);\n> > > +               lastY = lastY + diff;\n> > > +               config->tone_curve.ym[i] = lastY;\n> > > +               debugCurve.push_back({ static_cast<int>(toneCurveX_[i] * 4096.0),\n> > > +                                      lastY });\n> > > +       }\n> > > +\n> > > +       context.debugMetadata.set<Span<const Point>>(controls::debug::WdrCurve, debugCurve);\n> > > +}\n> > > +\n> > > +void WideDynamicRange::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > +                              IPAFrameContext &frameContext,\n> > > +                              const rkisp1_stat_buffer *stats,\n> > > +                              ControlList &metadata)\n> > > +{\n> > > +       if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_HIST)) {\n> > > +               LOG(RkISP1Wdr, Error) << \"No histogram data in statistics\";\n> > \n> > Is this a full error if this happens? or just a debug print ? I thought\n> > sometimes on say a too dark image we might not get any (usable) stats ?\n> \n> We can make it a warning. But histogram should always be filled. That is\n> different from the AWB measurements where pixels can be excluded from\n> the calculation.\n> \n> > \n> > > +               return;\n> > > +       }\n> > > +\n> > > +       if (!started_) {\n> > > +               started_ = true;\n> > > +               pid_.setStandardParameters(1, 5.0, 3.0);\n> > > +               pid_.setOutputLimits(minExposureValue_, 0.0);\n> > \n> > Can the minExposureValue_ ever be changed for instance if we get called\n> > with different modes in configure() or if the frame duration limits get\n> > changed ?\n> \n> IMHO not as it is read from the tuning file only.\n\nThat makes me think pid_.setOutputLimits(minExposureValue_, 0.0); should\nbe set at init() time then after the tuning file is read ?\n\n> \n> > \n> > > +               context.activeState.wdr.exposureValue = 0.0;\n> > \n> > Should those happen during configure() ? or init() ?\n> > \n> >  - init just once when constructed... (current behaviour)\n> >  - or configure if every time the camera is reconfigured..\n> > \n> > Then we wouldn't need a started_ bool tracker ?\n> \n> Good point. I think it should be reset in configure() (which is missing\n> at the moment). I would expect (correct me if you do differently) the\n> algorithm to keep state between stop()/start() calls on the camera but to\n> reset internal state on configure(). That is actually something we do\n> not test very well (or at all). In camshark it is always a complete tear\n> down. That needs to get fixed.\n\nAh yes - 'pausing' a stream in camshark would be nice to have. We can do\nthat in qcam already.\n\n> So here I'd like to add started_=false to configure() for now. I could\n> also move the setup code into configure which would remove the need for\n> the started_ variable. The reason why I prefer to having it here is a\n> practical one. While working on the regulation I'd like to keep all the\n> relevant pieces in one place and not scattered over multiple places. And\n> I expect us to do more work on that regulation as it is still quite\n> slow.\n\nI'll say ok ... but ...\n\nI still think we have an event based interface that performs tasks at\ngiven events and using a flag to make an extra 'state' simply means we\nare missing or have a poor event interface... (Except in this case I\ndon't think it's missing - it's init,configure,{prepare,process}?)\n\n> \n> > \n> > > +       }\n> > > +\n> > > +       pid_.setTarget(exposureConstraintY_);\n> > > +\n> > > +       const rkisp1_cif_isp_stat *params = &stats->params;\n> > > +       auto mode = frameContext.wdr.mode;\n> > > +\n> > > +       metadata.set(controls::draft::WdrMode, mode);\n> > > +\n> > > +       Histogram cumHist({ params->hist.hist_bins, context.hw->numHistogramBins },\n> > > +                         [](uint32_t x) { return x >> 4; });\n> > > +\n> > > +       std::vector<double> hist;\n> > > +       double sum = 0;\n> > > +       for (unsigned i = 0; i < context.hw->numHistogramBins; i++) {\n> > > +               double v = params->hist.hist_bins[i] >> 4;\n> > > +               hist.push_back(v);\n> > > +               sum += v;\n> > > +       }\n> > > +\n> > > +       /* Scale so that the entries sum up to 1. */\n> > > +       double scale = 1.0 / sum;\n> > > +       for (auto &v : hist)\n> > > +               v *= scale;\n> > > +       hist_.swap(hist);\n> > > +\n> > > +       double mean = cumHist.quantile(1.0 - exposureConstraintMaxBrightPixels_) /\n> > > +                     cumHist.bins();\n> > > +       LOG(RkISP1Wdr, Debug) << \"Mean y of bright pixels: \" << mean;\n> > > +\n> > > +       if (mode == controls::draft::WdrOff) {\n> > > +               metadata.set(controls::draft::WdrExposureValue, 0);\n> > > +               return;\n> > > +       }\n> > > +\n> > > +       context.activeState.wdr.exposureValue = pid_.process(mean);\n> > > +       LOG(RkISP1Wdr, Debug) << \"Active state WDR ev: \" << context.activeState.wdr.exposureValue;\n> > > +       metadata.set(controls::draft::WdrExposureValue, frameContext.wdr.wdrExposureValue);\n> > > +\n> > > +       /*\n> > > +        * We overwrote agc exposure value in prepare() to create an\n> > > +        * underexposure for WDR. Report the original exposure value in metadata.\n> > > +        */\n> > > +       metadata.set(controls::ExposureValue, frameContext.wdr.agcExposureValue);\n> > > +}\n> > > +\n> > > +REGISTER_IPA_ALGORITHM(WideDynamicRange, \"WideDynamicRange\")\n> > > +\n> > > +} /* namespace ipa::rkisp1::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/rkisp1/algorithms/wdr.h b/src/ipa/rkisp1/algorithms/wdr.h\n> > > new file mode 100644\n> > > index 000000000000..bdea4eb5492a\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/algorithms/wdr.h\n> > > @@ -0,0 +1,64 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021-2022, Ideas On Board\n> > > + *\n> > > + * RkISP1 Wide Dynamic Range control\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include <libcamera/control_ids.h>\n> > > +\n> > > +#include \"libcamera/internal/pid_controller.h\"\n> > > +\n> > > +#include \"linux/rkisp1-config.h\"\n> > > +\n> > > +#include \"algorithm.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::algorithms {\n> > > +\n> > > +class WideDynamicRange : public Algorithm\n> > > +{\n> > > +public:\n> > > +       WideDynamicRange();\n> > > +       ~WideDynamicRange() = default;\n> > > +\n> > > +       int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > +\n> > > +       void queueRequest(IPAContext &context, 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> > > +                    RkISP1Params *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> > > +       Vector<double, 2> kneePoint(double gain, double strength);\n> > > +       void applyCompensationLinear(double gain, double strength);\n> > > +       void applyCompensationPower(double gain, double strength);\n> > > +       void applyCompensationExponential(double gain, double strength);\n> > > +       void applyHistogramEqualization(double strength);\n> > > +\n> > > +       double exposureConstraintMaxBrightPixels_;\n> > > +       double exposureConstraintY_;\n> > > +       double minExposureValue_;\n> > > +       double strength_;\n> > > +       bool started_ = false;\n> > > +\n> > > +       controls::draft::WdrModeEnum mode_;\n> > > +       std::vector<double> hist_;\n> > > +       PidController pid_;\n> > > +\n> > > +       std::array<int, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV> toneCurveIntervalValues_;\n> > > +       std::array<double, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1> toneCurveX_;\n> > > +       std::array<double, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1> toneCurveY_;\n> > > +};\n> > > +\n> > > +} /* namespace ipa::rkisp1::algorithms */\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index 60cfab228edf..32f6db30bbdb 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -130,6 +130,10 @@ struct IPAActiveState {\n> > >         struct {\n> > >                 double gamma;\n> > >         } goc;\n> > > +\n> > > +       struct {\n> > > +               double exposureValue;\n> > > +       } wdr;\n> > >  };\n> > >  \n> > >  struct IPAFrameContext : public FrameContext {\n> > > @@ -198,6 +202,13 @@ struct IPAFrameContext : public FrameContext {\n> > >         struct {\n> > >                 double lux;\n> > >         } lux;\n> > > +\n> > > +       struct {\n> > > +               controls::draft::WdrModeEnum mode;\n> > > +               double wdrExposureValue;\n> > > +               double agcExposureValue;\n> > > +               double strength;\n> > > +       } wdr;\n> > >  };\n> > >  \n> > >  struct IPAContext {\n> > > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> > > index 4c0b051ce65d..5edb36c91b87 100644\n> > > --- a/src/ipa/rkisp1/params.cpp\n> > > +++ b/src/ipa/rkisp1/params.cpp\n> > > @@ -74,6 +74,7 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {\n> > >         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandBls, COMPAND_BLS, compand_bls),\n> > >         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandExpand, COMPAND_EXPAND, compand_curve),\n> > >         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandCompress, COMPAND_COMPRESS, compand_curve),\n> > > +       RKISP1_BLOCK_TYPE_ENTRY_EXT(Wdr, WDR, wdr),\n> > >  };\n> > >  \n> > >  } /* namespace */\n> > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> > > index 40450e34497a..2e60528d102e 100644\n> > > --- a/src/ipa/rkisp1/params.h\n> > > +++ b/src/ipa/rkisp1/params.h\n> > > @@ -40,6 +40,7 @@ enum class BlockType {\n> > >         CompandBls,\n> > >         CompandExpand,\n> > >         CompandCompress,\n> > > +       Wdr,\n> > >  };\n> > >  \n> > >  namespace details {\n> > > @@ -74,6 +75,7 @@ RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)\n> > >  RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)\n> > >  RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)\n> > >  RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr)\n> > >  \n> > >  } /* namespace details */\n> > >  \n> > > diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml\n> > > index 797532712099..9489e677402e 100644\n> > > --- a/src/libcamera/control_ids_debug.yaml\n> > > +++ b/src/libcamera/control_ids_debug.yaml\n> > > @@ -3,4 +3,9 @@\n> > >  %YAML 1.1\n> > >  ---\n> > >  vendor: debug\n> > > -controls: []\n> > > +controls:\n> > > +- WdrCurve:\n> > > +    type: Point\n> > > +    direction: out\n> > > +    description: Debug control WdrCurve found in src/ipa/rkisp1/algorithms/wdr.cpp\n> > > +    size: '[n]'\n> > \n> > I love the idea of having debug controls that let us plot debug\n> > internals - but if these are generated by script - do we need a way to\n> > make sure they are only ever append only - if we ever merge a debug\n> > control - suddenly the id number becomes part of the ABI - and adding\n> > new debug controls can't change the existing ABI. Could be handled with\n> > manual review ... but could then irritate the automatic tools for\n> > createing the controls in the first place ...\n> \n> Ohhh bummer. I need to think about that. We definitely shouldn't require\n> them to be append only. That would lead to a massive mess. Do we do the\n> same for draft controls? Is there no way to deprecate a control?\n\nWe can 'deprecate' it by saying it's not used any more and never\nexposing it ...\n\nBut the pain point is that each control is backed by an integer number\nwhich is based by created an enum and is therefore 'the order in which\nthey are defined'.\n\nIf one application thinks WdrCurve is control 10001 and anther thinks\nit's 10005 we have a problem :-(\n\nAnd I wish we could solve that better somehow. But it means we have to\nonly append controls to any namespace. Otherwise it's an ABI break and\nthe SONAME has to be bumped.\n\n> > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > > index 03309eeac34f..0c0af7a19968 100644\n> > > --- a/src/libcamera/control_ids_draft.yaml\n> > > +++ b/src/libcamera/control_ids_draft.yaml\n> > > @@ -293,5 +293,75 @@ controls:\n> > >  \n> > >          Currently identical to ANDROID_STATISTICS_FACE_IDS.\n> > >        size: [n]\n> > > +  - WdrMode:\n> > \n> > Can we avoid these going into ::draft:: at all ?\n> > \n> > > +      type: int32_t\n> > > +      direction: inout\n> > > +      description: |\n> > > +        Set the WDR mode.\n> > > +\n> > > +        The WDR mode is used to select the algorithm used for global tone\n> > > +        mapping. It will automatically reduce the exposure time of the sensor\n> > > +        so that there are only a small number of saturated pixels in the image.\n> > > +        The algorithm then compensates for the loss of brightness by applying a\n> > > +        global tone mapping curve to the image.\n> > > +      enum:\n> > > +        - name: WdrOff\n> > > +          value: 0\n> > > +          description: Wdr is disabled.\n> > > +        - name: WdrLinear\n> > > +          value: 1\n> > > +          description:\n> > > +            Apply a linear global tone mapping curve.\n> > \n> > I'm torn here on naming. Cue the bikeshed door opening ...\n> \n> I'm putting on my safety gear... :-)\n> \n> > \n> > But we use the control name 'Wdr' and say it applies 'Global Tone\n> > Mapping' configurations.\n> > \n> > Is WDR sufficiently known to represent Global Tone Mapping - are the\n> > terms sufficiently interchangable ?\n> \n> That is not really easy to solve. I googled \"WDR vs HDR\" which lead to\n> everything. Articles that contradict each other, pure nonsense and\n> company specific nomenclature. I would not limit it to global tone\n> mapping only - that's only what we have right now but I'd love to see\n> local tone mapping added to the area of \"libcamera WDR/HDR controls\". As\n> another side note, the upcoming HDR stitcher support will not expose own\n> controls but just plug into these Wdr* controls.\n> \n> We could discuss if we lean more to HDR instead of WDR... My gut feeling\n> is that WDR is a broead spectrum of things to increase the perceived\n> dynamic range (Local tone mapping, global tone mapping, single low\n> exposure + digital gain, on-chip fusion, multi-exposure) while with HDR\n> I think of at least two exposures. But I'm sure you'll find a lot of\n> articles/people disagreeing here... so... don't know :-)\n> \n\nI agree - HDR is definitely separate and to me really means multiple\nexposures in our contexts.\n\nI'm fine with 'WDR' meaning the processing on the single image ...\n\n> > Do we have other hardware with 'WDR' capabilities to compare if the\n> > controls make sense on other devices already?\n> > \n> > Dan - I expect the Mali-C55 to be quite HDR capable ... is there a WDR\n> > block there that would benefit/utilise the same controls and concepts?\n> > \n> > \n> > \n> > > +\n> > > +            The curve with two linear sections is applied. This produces good\n> > > +            results at the expense of a slightly artificial look.\n> > > +        - name: WdrPower\n> > > +          value: 2\n> > > +          description: |\n> > > +            Apply a power global tone mapping curve.\n> > > +\n> > > +            This curve has high gain values on the dark areas of an image and\n> > > +            high compression values on the bright area. It therefore tends to\n> > > +            produce noticeable noise artifacts.\n> > > +        - name: WdrExponential\n> > > +          value: 3\n> > > +          description: |\n> > > +            Apply a exponential global tone mapping curve.\n> > > +\n> > > +            This curve has lower gain values in dark areas compared to the power\n> > > +            curve but produces a more natural look compared to the linear curve.\n> > > +            It is therefore the best choice for most scenes.\n> > > +        - name: WdrHistogramEqualization\n> > > +          value: 4\n> > > +          description: |\n> > > +            Apply histogram equalization.\n> > > +\n> > > +            This curve preserves most of the information of the image at the\n> > > +            expense of a very artificial look. It is therefore best suited for\n> > > +            technical analysis.\n> > > +  - WdrStrength:\n> > > +      type: float\n> > > +      direction: in\n> > > +      description: |\n> > > +        Specify the strength of the wdr algorithm. The exact meaning of this\n> > > +        value is specific to the algorithm in use. Usually a value of 0 means no\n> > > +        global tone mapping is applied. A values of 1 is the default value and\n> > > +        the correct value for most scenes. A value above 1 increases the global\n> > > +        tone mapping effect and can lead to unrealistic image effects.\n> > > +  - WdrMaxBrightPixels:\n> > > +      type: float\n> > > +      direction: in\n> > > +      description: |\n> > > +        Percentage of allowed (nearly) saturated pixels. The WDR algorithm\n> > > +        reduces the WdrExposureValue until the amount of pixels that are close\n> > > +        to saturation is lower than this value.\n> > > +  - WdrExposureValue:\n> > > +      type: float\n> > > +      direction: out\n> > > +      description: |\n> > > +        Reports the Exposure Value that was applied to the AEGC regulation so\n> > > +        that the WdrMaxBrightPixels limit is reached.\n> > \n> > Is that /different/ to the ExposureValue already reported in metadata?\n> \n> Yes, the ExposureValue is applied by the user leading to a over/under\n> exposed output image. WdrExposureValue is applied under the hood and\n> equalized by tone mapping algorithm. So a WdrExposureValue of -2\n> basically says \"There are very bright areas in the image, I need to work\n> hard\" but the output is still exposed as with ExposureValue=0. I don't\n> know if that will be the approach taken for other ISPs as it is based on\n> the limitations of the imx8mp ISP. I think time will tell.\n\nAck.\n\n> \n> Best regards,\n> Stefan\n> \n> > \n> > > +\n> > > +        \\sa ExposureValue\n> > >  \n> > >  ...\n> > > -- \n> > > 2.48.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 4D7BCBDCC1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 13 Aug 2025 11:00:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3329369249;\n\tWed, 13 Aug 2025 13:00: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 DBA526922C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 13 Aug 2025 12:59:57 +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 56A56379;\n\tWed, 13 Aug 2025 12:59: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=\"ep4DAnOQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755082744;\n\tbh=ERoVHH8sojTK6BdPPsdwVCkaSdH5hDlBSOFRGMdljos=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ep4DAnOQaNUdJycIp6QAPm46UyTMYVFgfCuQFpW1o8WXYC6wZNZwjxCyrQ5E5FSD3\n\tl01zUYcGvZxcFLC5V9ujc9vHHkHnK3zRJk7cxdu4I0aBTyxo2ped+unHse7R++iqJV\n\tgOaOwfOFXmS3CiRZ9LRM+f+tBXjIUnS3jmowJobY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175508169312.1225443.2627596737853254972@localhost>","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-16-stefan.klug@ideasonboard.com>\n\t<175493316838.1641235.2414681416886447452@ping.linuxembedded.co.uk>\n\t<175508169312.1225443.2627596737853254972@localhost>","Subject":"Re: [PATCH v2 15/16] ipa: rkisp1: Add WDR algorithm","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"","To":"Dan Scally <dan.scally@ideasonboard.com>,\n\tStefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 13 Aug 2025 11:59:54 +0100","Message-ID":"<175508279425.560048.16769889226335004715@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":35383,"web_url":"https://patchwork.libcamera.org/comment/35383/","msgid":"<CAHW6GY+a7FWLrzuRZBtPNim=byKnxpv+a5G072r832DDm0aQEg@mail.gmail.com>","date":"2025-08-13T12:11:54","subject":"Re: [PATCH v2 15/16] ipa: rkisp1: Add WDR algorithm","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran, everyone\n\nOn Wed, 13 Aug 2025 at 12:00, Kieran Bingham\n<kieran.bingham@ideasonboard.com> wrote:\n>\n> Quoting Stefan Klug (2025-08-13 11:41:33)\n> > Hi Kieran,\n> >\n> > Thank you for the review.\n> >\n> > Quoting Kieran Bingham (2025-08-11 19:26:08)\n> > > Dan - hidden question to you way at the bottom of this one!\n> > >\n> > > Quoting Stefan Klug (2025-08-08 15:12:53)\n> > > > Add a WDR algorithm to do global tone mapping. Global tone mapping is\n> > > > used to increase the perceived dynamic range of an image. The typical\n> > > > effect is that in areas that are normally overexposed, additional\n> > > > structure becomes visible.\n> > > >\n> > > > The overall idea is that the algorithm applies an exposure value\n> > > > correction to underexpose the image to the point where only a small\n> > > > number of saturated pixels is left. This artificial underexposure is\n> > > > then mitigated by applying a tone mapping curve.\n> > > >\n> > > > This algorithm implements 4 tone mapping strategies:\n> > > > - Linear\n> > > > - Power\n> > > > - Exponential\n> > > > - Histogram equalization\n> > > >\n> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > > >\n> > > > ---\n> > > >\n> > > > Changes in v2:\n> > > > - Fixed default value for min bright pixels\n> > > > - Added check for supported params type\n> > > > - Reset PID controller\n> > > > - Various fixes from Pauls review\n> > > > ---\n> > > >  src/ipa/rkisp1/algorithms/meson.build |   1 +\n> > > >  src/ipa/rkisp1/algorithms/wdr.cpp     | 510 ++++++++++++++++++++++++++\n> > > >  src/ipa/rkisp1/algorithms/wdr.h       |  64 ++++\n> > > >  src/ipa/rkisp1/ipa_context.h          |  11 +\n> > > >  src/ipa/rkisp1/params.cpp             |   1 +\n> > > >  src/ipa/rkisp1/params.h               |   2 +\n> > > >  src/libcamera/control_ids_debug.yaml  |   7 +-\n> > > >  src/libcamera/control_ids_draft.yaml  |  70 ++++\n> > > >  8 files changed, 665 insertions(+), 1 deletion(-)\n> > > >  create mode 100644 src/ipa/rkisp1/algorithms/wdr.cpp\n> > > >  create mode 100644 src/ipa/rkisp1/algorithms/wdr.h\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > > > index 2e42a80cf99d..d329dbfb432d 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > > > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > > > @@ -14,4 +14,5 @@ rkisp1_ipa_algorithms = files([\n> > > >      'gsl.cpp',\n> > > >      'lsc.cpp',\n> > > >      'lux.cpp',\n> > > > +    'wdr.cpp',\n> > > >  ])\n> > > > diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..0c1b261d4373\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/rkisp1/algorithms/wdr.cpp\n> > > > @@ -0,0 +1,510 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2025, Ideas On Board\n> > > > + *\n> > > > + * RkISP1 Wide Dynamic Range control\n> > > > + */\n> > > > +\n> > > > +#include \"wdr.h\"\n> > > > +\n> > > > +#include <libcamera/base/log.h>\n> > > > +#include <libcamera/base/utils.h>\n> > > > +\n> > > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > > +\n> > > > +#include <libipa/histogram.h>\n> > > > +#include <libipa/pwl.h>\n> > > > +\n> > > > +#include \"linux/rkisp1-config.h\"\n> > > > +\n> > > > +/**\n> > > > + * \\file wdr.h\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa::rkisp1::algorithms {\n> > > > +\n> > > > +/**\n> > > > + * \\class WideDynamicRange\n> > > > + * \\brief RkISP1 Wide Dynamic Range algorithm\n> > > > + *\n> > > > + * This algorithm implements automatic global tone mapping for the RkISP1.\n> > > > + * Global tone mapping is done by the GWDR hardware block and applies\n> > > > + * a global tone mapping curve to the image to increase the perceived dynamic\n> > > > + * range. Imagine an indoor scene with bright outside visible through the\n> > > > + * windows. With normal exposure settings, the windows will be completely\n> > > > + * saturated and no structure (sky/clouds) will be visible because the AEGC has\n> > > > + * to increase overall exposure to reach a certain level of mean brightness. In\n> > > > + * WDR mode, the algorithm will artifically reduce the exposure time so that the\n> > > > + * texture and colours become visible in the formerly saturated areas. Then the\n> > > > + * global tone mapping curve is applied to mitigate the loss of brightness.\n> > > > + *\n> > > > + * Calculating that tone mapping curve is the most difficult part. This\n> > > > + * algorithm implements four tone mapping strategies:\n> > > > + * - Linear: The tone mapping curve is a combination of two linear functions\n> > > > + * with one kneepoint\n> > >\n> > > Can this 'with one kneepiont' be indented to match the Linear ?\n> > >\n> > >\n> > >  * - Linear: The tone mapping curve is a combination of two linear functions\n> > >  *   with one kneepoint\n> > >\n> >\n> > ack\n> >\n> > >\n> > > > + * - Power: The tone mapping curve follows a power function\n> > > > + * - Exponential: The tone mapping curve follows an exponential function\n> > > > + * - HistogramEqualization: The tone mapping curve tries to equalize the\n> > > > + * histogram\n> > >\n> > > Same here.?\n> >\n> > ack\n> >\n> > >\n> > > > + *\n> > > > + * The overall strategy is the same in all cases: A negative exposure value is\n> > > > + * applied to the AEGC regulation until the number of nearly saturated pixels go\n> > > > + * below a given threshold (controllable via WdrMaxBrightPixels, default is 2%)\n> > > > + * or the MinExposureValue specified in the tuning file is reached.\n> > > > + *\n> > > > + * The global tone mapping curve is then calculated so that it accounts for the\n> > > > + * reduction of brightness due to the negative exposure value. As the result of\n> > > > + * tone mapping is very difficult to quantize and is by definition a lossy\n> > > > + * process there is not a single \"correct\" solution.\n> > > > + *\n> > > > + * The approach taken here is based on a simple linear model. Consider a pixel\n> > > > + * that was originally 50% grey. It will have its exposure pushed down by the\n> > > > + * WDR's initial exposure compensation. This value then needs to be pushed back\n> > > > + * up by the tone mapping curve so that it is 50% grey again. This point serves\n> > > > + * as our kneepoint. To get to this kneepoint, this pixel and all darker pixels\n> > > > + * (to the left of the kneepoint on the tone mapping curve) will simply have the\n> > > > + * exposure compensation undone by pow(2, -WDR_EV). This cancels out the\n> > > > + * original exposure compensation, which was pow(2, WDR_EV). The remaining\n> > > > + * brigher pixels (to the right of the kneepoint on the tone mapping curve) will\n> > > > + * be compressed. The WdrStrength control adjusts the gain of the left part of\n> > > > + * the tone mapping curve.\n> > > > + *\n> > > > + * In the Power and Exponential modes, the curves are calculated so that they\n> > > > + * pass through that kneepoint.\n> > > > + *\n> > > > + * The histogram equalization mode tries to equalize the histogram of the\n> > > > + * image and acts independently of the calculated exposure value.\n> > > > + *\n> > > > + * \\code{.unparsed}\n> > > > + * algorithms:\n> > > > + *   - WideDynamicRange:\n> > > > + *       ExposureConstraint:\n> > > > + *         MaxBrightPixels: 0.02\n> > > > + *         yTarget: 0.95\n> > > > + *       MinExposureValue: -4.0\n> > > > + * \\endcode\n> > > > + */\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(RkISP1Wdr)\n> > > > +\n> > > > +static constexpr unsigned int kTonecurveXIntervals = RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV;\n> > > > +\n> > > > +/*\n> > > > + * Increasing interval sizes. The intervals are crafted so that they sum\n> > > > + * up to 4096. This results in better fitting curves than the constant intervals\n> > > > + * (all entries are 4)\n> > > > + */\n> > > > +static constexpr std::array<int, kTonecurveXIntervals> kLoglikeIntervals = {\n> > > > +       { 0, 0, 0, 0, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3, 3, 4,\n> > > > +         4, 4, 4, 4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 5, 6, 6 }\n> > > > +};\n> > > > +\n> > > > +WideDynamicRange::WideDynamicRange()\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > > + */\n> > > > +int WideDynamicRange::init([[maybe_unused]] IPAContext &context,\n> > > > +                          [[maybe_unused]] const YamlObject &tuningData)\n> > > > +{\n> > > > +       if (!(context.hw->supportedBlocks & 1 << RKISP1_EXT_PARAMS_BLOCK_TYPE_WDR)) {\n> > > > +               LOG(RkISP1Wdr, Error)\n> > > > +                       << \"Wide Dynamic Range not supported by the hardware or kernel.\";\n> > > > +               return -ENOTSUP;\n> > > > +       }\n> > > > +\n> > > > +       toneCurveIntervalValues_ = kLoglikeIntervals;\n> > > > +\n> > > > +       /* Calculate a list of normed x values */\n> > > > +       toneCurveX_[0] = 0.0;\n> > > > +       int lastValue = 0;\n> > > > +       for (unsigned int i = 1; i < toneCurveX_.size(); i++) {\n> > > > +               lastValue += std::pow(2, toneCurveIntervalValues_[i - 1] + 3);\n> > > > +               lastValue = std::min(lastValue, 4096);\n> > > > +               toneCurveX_[i] = lastValue / 4096.0;\n> > > > +       }\n> > > > +\n> > > > +       strength_ = 1.0;\n> > > > +       mode_ = controls::draft::WdrOff;\n> > > > +       exposureConstraintMaxBrightPixels_ = 0.02;\n> > > > +       exposureConstraintY_ = 0.95;\n> > > > +       minExposureValue_ = -4.0;\n> > > > +\n> > > > +       const auto &constraint = tuningData[\"ExposureConstraint\"];\n> > > > +       if (!constraint.isDictionary()) {\n> > > > +               LOG(RkISP1Wdr, Warning)\n> > > > +                       << \"ExposureConstraint not found in tuning data.\"\n> > > > +                          \"Using default values MaxBrightPixels: \"\n> > > > +                       << exposureConstraintMaxBrightPixels_\n> > > > +                       << \" yTarget: \" << exposureConstraintY_;\n> > > > +       } else {\n> > > > +               exposureConstraintMaxBrightPixels_ =\n> > > > +                       constraint[\"MaxBrightPixels\"]\n> > > > +                               .get<double>()\n> > > > +                               .value_or(exposureConstraintMaxBrightPixels_);\n> > > > +               exposureConstraintY_ =\n> > > > +                       constraint[\"yTarget\"]\n> > > > +                               .get<double>()\n> > > > +                               .value_or(exposureConstraintY_);\n> > > > +       }\n> > > > +\n> > > > +       const auto &minExp = tuningData[\"MinExposureValue\"];\n> > > > +       minExposureValue_ = minExp.get<double>().value_or(minExposureValue_);\n> > > > +       if (!minExp) {\n> > > > +               LOG(RkISP1Wdr, Warning)\n> > > > +                       << \"MinExposureValue not found in tuning data.\"\n> > > > +                          \" Using default value: \"\n> > > > +                       << minExposureValue_;\n> > > > +       }\n> > >\n> > > { } aren't required for single statement...\n> >\n> > Argh I don't get these to become a habit :-)\n> >\n> > >\n> > > > +\n> > > > +       context.ctrlMap[&controls::draft::WdrMode] =\n> > > > +               ControlInfo(controls::draft::WdrModeValues, controls::draft::WdrOff);\n> > > > +       context.ctrlMap[&controls::draft::WdrStrength] =\n> > > > +               ControlInfo(0.0f, 2.0f, static_cast<float>(strength_));\n> > > > +       context.ctrlMap[&controls::draft::WdrMaxBrightPixels] =\n> > > > +               ControlInfo(0.0f, 1.0f, static_cast<float>(exposureConstraintMaxBrightPixels_));\n> > > > +\n> > > > +       applyCompensationLinear(1.0, 0.0);\n> > > > +\n> > > > +       return 0;\n> > > > +}\n> > > > +\n> > > > +void WideDynamicRange::applyHistogramEqualization(double strength)\n> > > > +{\n> > > > +       if (hist_.empty())\n> > > > +               return;\n> > > > +\n> > > > +       strength *= 0.65;\n> > >\n> > > Why 0.65 ?\n> >\n> > That value was chosen to provide a similar optical impression compared\n> > to the other algorithms. I'll revisit and provide some description.\n>\n> Ack\n>\n> >\n> > >\n> > > > +\n> > > > +       /*\n> > > > +        * In a fully equalized histogram, all bins have the same value. Try\n> > > > +        * to equalize the histogram by applying a gain or damping depending on\n> > > > +        * the distance of the actual bin value from that norm.\n> > > > +        */\n> > > > +       std::vector<double> gains;\n> > > > +       gains.resize(hist_.size());\n> > > > +       double sum = 0;\n> > > > +       double norm = 1.0 / (gains.size());\n> > > > +       for (unsigned i = 0; i < hist_.size(); i++) {\n> > > > +               double diff = 1.0 + strength * (hist_[i] - norm) / norm;\n> > > > +               gains[i] = diff;\n> > > > +               sum += diff;\n> > > > +       }\n> > > > +       /* Never amplify the last entry. */\n> > > > +       gains.back() = std::max(gains.back(), 1.0);\n> > > > +\n> > > > +       double scale = gains.size() / sum;\n> > > > +       for (auto &v : gains)\n> > > > +               v *= scale;\n> > > > +\n> > > > +       Pwl pwl;\n> > > > +       double step = 1.0 / gains.size();\n> > > > +       double lastX = 0;\n> > > > +       double lastY = 0;\n> > > > +\n> > > > +       pwl.append(lastX, lastY);\n> > > > +       for (unsigned int i = 0; i < gains.size() - 1; i++) {\n> > > > +               lastY += gains[i] * step;\n> > > > +               lastX += step;\n> > > > +               pwl.append(lastX, lastY);\n> > > > +       }\n> > > > +       pwl.append(1.0, 1.0);\n> > > > +\n> > > > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> > > > +               toneCurveY_[i] = pwl.eval(toneCurveX_[i]);\n> > > > +}\n> > > > +\n> > > > +Vector<double, 2> WideDynamicRange::kneePoint(double gain, double strength)\n> > > > +{\n> > > > +       gain = std::pow(gain, strength);\n> > > > +       double y = 0.5;\n> > > > +       double x = y / gain;\n> > > > +\n> > > > +       return { { x, y } };\n> > > > +}\n> > > > +\n> > > > +void WideDynamicRange::applyCompensationLinear(double gain, double strength)\n> > > > +{\n> > > > +       auto kp = kneePoint(gain, strength);\n> > > > +       double g1 = kp.y() / kp.x();\n> > > > +       double g2 = (kp.y() - 1) / (kp.x() - 1);\n> > > > +\n> > > > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++) {\n> > > > +               double x = toneCurveX_[i];\n> > > > +               double y;\n> > > > +               if (x <= kp.x()) {\n> > > > +                       y = g1 * x;\n> > > > +               } else {\n> > > > +                       y = g2 * x + 1 - g2;\n> > > > +               }\n> > > > +               toneCurveY_[i] = y;\n> > > > +       }\n> > > > +}\n> > > > +\n> > > > +void WideDynamicRange::applyCompensationPower(double gain, double strength)\n> > > > +{\n> > > > +       double e = 1.0;\n> > > > +       if (strength > 1e-6) {\n> > > > +               auto kp = kneePoint(gain, strength);\n> > > > +               /* Calculate an exponent to go through the knee point. */\n> > > > +               e = log(kp.y()) / log(kp.x());\n> > > > +       }\n> > > > +\n> > > > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> > > > +               toneCurveY_[i] = std::pow(toneCurveX_[i], e);\n> > > > +}\n> > > > +\n> > > > +void WideDynamicRange::applyCompensationExponential(double gain, double strength)\n> > > > +{\n> > > > +       double k = 0.1;\n> > > > +       auto kp = kneePoint(gain, strength);\n> > > > +       double kx = kp.x();\n> > > > +       double ky = kp.y();\n> > > > +\n> > > > +       if (kx > ky) {\n> > > > +               LOG(RkISP1Wdr, Warning) << \"Invalid knee point: \" << kp;\n> > > > +               kx = ky;\n> > > > +       }\n> > > > +\n> > > > +       /*\n> > > > +        * The exponential curve is based on the function proposed by Glozman\n> > > > +        * et al. in\n> > > > +        * S. Glozman, T. Kats, and O. Yadid-Pecht, \"Exponent Operator Based\n> > > > +        * Tone Mapping Algorithm for Color Wide Dynamic Range Images,\" 2011.\n> > > > +        *\n> > > > +        * That function uses a k factor as parameter for the WDR compression\n> > > > +        * curve:\n> > > > +        * k=0: maximum compression\n> > > > +        * k=infinity: linear curve\n> > > > +        *\n> > > > +        * To calculate a k factor that results in a curve that passes through\n> > > > +        * the kneepoint, the equation needs to be solved for k after inserting\n> > > > +        * the kneepoint.  This can be formulated as search for a zero point.\n> > > > +        * Unfortunately there is no closed solution for that transformation.\n> > > > +        * Using newton's method to approximate the value is numerically\n> > > > +        * unstable.\n> > > > +        *\n> > > > +        * Luckily the function only crosses the x axis once and for the set of\n> > > > +        * possible kneepoints, a negative and a positive point can be guessed.\n> > > > +        * The approximation is then implemented using bisection.\n> > > > +        */\n> > > > +       if (std::abs(kx - ky) < 0.001) {\n> > > > +               k = 1e8;\n> > > > +       } else {\n> > > > +               double kl = 0.0001;\n> > > > +               double kh = 1000;\n> > > > +\n> > > > +               auto fk = [=](double v) {\n> > > > +                       return std::exp(-kx / v) -\n> > > > +                              ky * std::exp(-1.0 / v) + ky - 1.0;\n> > > > +               };\n> > > > +\n> > > > +               ASSERT(fk(kl) < 0);\n> > > > +               ASSERT(fk(kh) > 0);\n> > > > +\n> > > > +               k = kh / 10.0;\n> > > > +               while (fk(k) > 0) {\n> > > > +                       kh = k;\n> > > > +                       k /= 10.0;\n> > > > +               }\n> > > > +\n> > > > +               do {\n> > > > +                       k = (kl + kh) / 2;\n> > > > +                       if (fk(k) < 0)\n> > > > +                               kl = k;\n> > > > +                       else\n> > > > +                               kh = k;\n> > > > +               } while (std::abs(kh - kl) > 1e-3);\n> > > > +       }\n> > > > +\n> > > > +       double a = 1.0 / (1.0 - std::exp(-1.0 / k));\n> > > > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n> > > > +               toneCurveY_[i] = a * (1.0 - std::exp(-toneCurveX_[i] / k));\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > > > + */\n> > > > +void WideDynamicRange::queueRequest([[maybe_unused]] IPAContext &context,\n> > > > +                                   [[maybe_unused]] const uint32_t frame,\n> > > > +                                   IPAFrameContext &frameContext,\n> > > > +                                   const ControlList &controls)\n> > > > +{\n> > > > +       strength_ = controls.get(controls::draft::WdrStrength).value_or(strength_);\n> > > > +       exposureConstraintMaxBrightPixels_ =\n> > > > +               controls.get(controls::draft::WdrMaxBrightPixels)\n> > > > +                       .value_or(exposureConstraintMaxBrightPixels_);\n> > > > +\n> > > > +       const auto &mode = controls.get(controls::draft::WdrMode);\n> > > > +       if (mode) {\n> > > > +               mode_ = static_cast<controls::draft::WdrModeEnum>(*mode);\n> > > > +       }\n> > >\n> > > No { } required here.\n> >\n> > zzz\n> >\n> > >\n> > > > +\n> > > > +       frameContext.wdr.mode = mode_;\n> > > > +       frameContext.wdr.strength = strength_;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > + */\n> > > > +void WideDynamicRange::prepare(IPAContext &context,\n> > > > +                              [[maybe_unused]] const uint32_t frame,\n> > > > +                              IPAFrameContext &frameContext,\n> > > > +                              RkISP1Params *params)\n> > > > +{\n> > > > +       if (!params) {\n> > > > +               LOG(RkISP1Wdr, Warning) << \"Params is null\";\n> > > > +               return;\n> > > > +       }\n> > > > +\n> > > > +       auto config = params->block<BlockType::Wdr>();\n> > > > +\n> > > > +       auto mode = frameContext.wdr.mode;\n> > > > +\n> > > > +       config.setEnabled(mode != controls::draft::WdrOff);\n> > > > +\n> > > > +       double comp = 0;\n> > > > +\n> > > > +       /*\n> > > > +        * Todo: This overwrites frameContext.agc.exposureValue so that\n> > > > +        * in the next call to Agc::process() that exposureValue get's\n> > > > +        * applied. In the future it is planned to move the exposure\n> > > > +        * calculations from Agc::process() to Agc::prepare(). In this\n> > > > +        * case, we need to ensure that this code get's called early enough.\n> > > > +        */\n> > >\n> > > Eeep ...\n> > >   /me looks away for the moment.\n> >\n> > /me At least I should fix \\todo. But the underlying problem is actually\n> > quite nasty as we might need to have multiple prepare runs and some kind\n> > of dependency tracking. That is for another day.\n> >\n> > >\n> > > > +       if (mode != controls::draft::WdrOff) {\n> > > > +               frameContext.wdr.wdrExposureValue = context.activeState.wdr.exposureValue;\n> > > > +               frameContext.wdr.agcExposureValue = frameContext.agc.exposureValue;\n> > > > +\n> > > > +               /*\n> > > > +                * When WDR is enabled, the maxBrightPixels constraint is always\n> > > > +                * active. This is problematic when the user sets a positive\n> > > > +                * exposureValue. As this would mean that the negative WDR\n> > > > +                * exposure value and the user provided positive value should\n> > > > +                * cancel each other out. But as the regulation is not absolute\n> > > > +                * and only dependent on the measured changes in the histogram,\n> > > > +                * reducing the wdr EV would only lead to an even stronger pull\n> > > > +                * from regulation. Positive user supplied EVs must therefore be\n> > > > +                * handled using the wdr curve.\n> > > > +                */\n> > > > +               if (frameContext.wdr.wdrExposureValue < 0) {\n> > > > +                       frameContext.agc.exposureValue =\n> > > > +                               std::min(frameContext.agc.exposureValue,\n> > > > +                                        frameContext.wdr.wdrExposureValue);\n> > > > +                       comp = frameContext.agc.exposureValue - frameContext.wdr.agcExposureValue;\n> > > > +               }\n> > > > +       }\n> > > > +\n> > > > +       /* Calculate how much EV we need to compensate with the WDR curve. */\n> > > > +       double gain = pow(2.0, -comp);\n> > > > +\n> > > > +       if (mode == controls::draft::WdrOff) {\n> > > > +               applyCompensationLinear(1.0, 0.0);\n> > > > +       } else if (mode == controls::draft::WdrLinear) {\n> > > > +               applyCompensationLinear(gain, frameContext.wdr.strength);\n> > > > +       } else if (mode == controls::draft::WdrPower) {\n> > > > +               applyCompensationPower(gain, frameContext.wdr.strength);\n> > > > +       } else if (mode == controls::draft::WdrExponential) {\n> > > > +               applyCompensationExponential(gain, frameContext.wdr.strength);\n> > > > +       } else if (mode == controls::draft::WdrHistogramEqualization) {\n> > > > +               applyHistogramEqualization(frameContext.wdr.strength);\n> > > > +       }\n> > > > +\n> > > > +       /* Reset value */\n> > > > +       config->dmin_strength = 0x10;\n> > > > +       config->dmin_thresh = 0;\n> > > > +\n> > > > +       for (unsigned int i = 0; i < kTonecurveXIntervals; i++) {\n> > > > +               int v = toneCurveIntervalValues_[i];\n> > > > +               config->tone_curve.dY[i / 8] |= (v & 0x07) << ((i % 8) * 4);\n> > > > +       }\n> > > > +\n> > > > +       std::vector<Point> debugCurve;\n> > > > +\n> > > > +       /*\n> > > > +        * Fix the curve to adhere to the hardware constraints. Don't apply a\n> > > > +        * constraint on the first element, which is most likely zero anyways.\n> > > > +        */\n> > > > +       int lastY = toneCurveY_[0] * 4096.0;\n> > > > +       for (unsigned int i = 0; i < toneCurveX_.size(); i++) {\n> > > > +               int diff = static_cast<int>(toneCurveY_[i] * 4096.0) - lastY;\n> > > > +               diff = std::clamp(diff, -2048, 2048);\n> > > > +               lastY = lastY + diff;\n> > > > +               config->tone_curve.ym[i] = lastY;\n> > > > +               debugCurve.push_back({ static_cast<int>(toneCurveX_[i] * 4096.0),\n> > > > +                                      lastY });\n> > > > +       }\n> > > > +\n> > > > +       context.debugMetadata.set<Span<const Point>>(controls::debug::WdrCurve, debugCurve);\n> > > > +}\n> > > > +\n> > > > +void WideDynamicRange::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > +                              IPAFrameContext &frameContext,\n> > > > +                              const rkisp1_stat_buffer *stats,\n> > > > +                              ControlList &metadata)\n> > > > +{\n> > > > +       if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_HIST)) {\n> > > > +               LOG(RkISP1Wdr, Error) << \"No histogram data in statistics\";\n> > >\n> > > Is this a full error if this happens? or just a debug print ? I thought\n> > > sometimes on say a too dark image we might not get any (usable) stats ?\n> >\n> > We can make it a warning. But histogram should always be filled. That is\n> > different from the AWB measurements where pixels can be excluded from\n> > the calculation.\n> >\n> > >\n> > > > +               return;\n> > > > +       }\n> > > > +\n> > > > +       if (!started_) {\n> > > > +               started_ = true;\n> > > > +               pid_.setStandardParameters(1, 5.0, 3.0);\n> > > > +               pid_.setOutputLimits(minExposureValue_, 0.0);\n> > >\n> > > Can the minExposureValue_ ever be changed for instance if we get called\n> > > with different modes in configure() or if the frame duration limits get\n> > > changed ?\n> >\n> > IMHO not as it is read from the tuning file only.\n>\n> That makes me think pid_.setOutputLimits(minExposureValue_, 0.0); should\n> be set at init() time then after the tuning file is read ?\n>\n> >\n> > >\n> > > > +               context.activeState.wdr.exposureValue = 0.0;\n> > >\n> > > Should those happen during configure() ? or init() ?\n> > >\n> > >  - init just once when constructed... (current behaviour)\n> > >  - or configure if every time the camera is reconfigured..\n> > >\n> > > Then we wouldn't need a started_ bool tracker ?\n> >\n> > Good point. I think it should be reset in configure() (which is missing\n> > at the moment). I would expect (correct me if you do differently) the\n> > algorithm to keep state between stop()/start() calls on the camera but to\n> > reset internal state on configure(). That is actually something we do\n> > not test very well (or at all). In camshark it is always a complete tear\n> > down. That needs to get fixed.\n>\n> Ah yes - 'pausing' a stream in camshark would be nice to have. We can do\n> that in qcam already.\n>\n> > So here I'd like to add started_=false to configure() for now. I could\n> > also move the setup code into configure which would remove the need for\n> > the started_ variable. The reason why I prefer to having it here is a\n> > practical one. While working on the regulation I'd like to keep all the\n> > relevant pieces in one place and not scattered over multiple places. And\n> > I expect us to do more work on that regulation as it is still quite\n> > slow.\n>\n> I'll say ok ... but ...\n>\n> I still think we have an event based interface that performs tasks at\n> given events and using a flag to make an extra 'state' simply means we\n> are missing or have a poor event interface... (Except in this case I\n> don't think it's missing - it's init,configure,{prepare,process}?)\n>\n> >\n> > >\n> > > > +       }\n> > > > +\n> > > > +       pid_.setTarget(exposureConstraintY_);\n> > > > +\n> > > > +       const rkisp1_cif_isp_stat *params = &stats->params;\n> > > > +       auto mode = frameContext.wdr.mode;\n> > > > +\n> > > > +       metadata.set(controls::draft::WdrMode, mode);\n> > > > +\n> > > > +       Histogram cumHist({ params->hist.hist_bins, context.hw->numHistogramBins },\n> > > > +                         [](uint32_t x) { return x >> 4; });\n> > > > +\n> > > > +       std::vector<double> hist;\n> > > > +       double sum = 0;\n> > > > +       for (unsigned i = 0; i < context.hw->numHistogramBins; i++) {\n> > > > +               double v = params->hist.hist_bins[i] >> 4;\n> > > > +               hist.push_back(v);\n> > > > +               sum += v;\n> > > > +       }\n> > > > +\n> > > > +       /* Scale so that the entries sum up to 1. */\n> > > > +       double scale = 1.0 / sum;\n> > > > +       for (auto &v : hist)\n> > > > +               v *= scale;\n> > > > +       hist_.swap(hist);\n> > > > +\n> > > > +       double mean = cumHist.quantile(1.0 - exposureConstraintMaxBrightPixels_) /\n> > > > +                     cumHist.bins();\n> > > > +       LOG(RkISP1Wdr, Debug) << \"Mean y of bright pixels: \" << mean;\n> > > > +\n> > > > +       if (mode == controls::draft::WdrOff) {\n> > > > +               metadata.set(controls::draft::WdrExposureValue, 0);\n> > > > +               return;\n> > > > +       }\n> > > > +\n> > > > +       context.activeState.wdr.exposureValue = pid_.process(mean);\n> > > > +       LOG(RkISP1Wdr, Debug) << \"Active state WDR ev: \" << context.activeState.wdr.exposureValue;\n> > > > +       metadata.set(controls::draft::WdrExposureValue, frameContext.wdr.wdrExposureValue);\n> > > > +\n> > > > +       /*\n> > > > +        * We overwrote agc exposure value in prepare() to create an\n> > > > +        * underexposure for WDR. Report the original exposure value in metadata.\n> > > > +        */\n> > > > +       metadata.set(controls::ExposureValue, frameContext.wdr.agcExposureValue);\n> > > > +}\n> > > > +\n> > > > +REGISTER_IPA_ALGORITHM(WideDynamicRange, \"WideDynamicRange\")\n> > > > +\n> > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/rkisp1/algorithms/wdr.h b/src/ipa/rkisp1/algorithms/wdr.h\n> > > > new file mode 100644\n> > > > index 000000000000..bdea4eb5492a\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/rkisp1/algorithms/wdr.h\n> > > > @@ -0,0 +1,64 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021-2022, Ideas On Board\n> > > > + *\n> > > > + * RkISP1 Wide Dynamic Range control\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include <libcamera/control_ids.h>\n> > > > +\n> > > > +#include \"libcamera/internal/pid_controller.h\"\n> > > > +\n> > > > +#include \"linux/rkisp1-config.h\"\n> > > > +\n> > > > +#include \"algorithm.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa::rkisp1::algorithms {\n> > > > +\n> > > > +class WideDynamicRange : public Algorithm\n> > > > +{\n> > > > +public:\n> > > > +       WideDynamicRange();\n> > > > +       ~WideDynamicRange() = default;\n> > > > +\n> > > > +       int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > > +\n> > > > +       void queueRequest(IPAContext &context, 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> > > > +                    RkISP1Params *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> > > > +       Vector<double, 2> kneePoint(double gain, double strength);\n> > > > +       void applyCompensationLinear(double gain, double strength);\n> > > > +       void applyCompensationPower(double gain, double strength);\n> > > > +       void applyCompensationExponential(double gain, double strength);\n> > > > +       void applyHistogramEqualization(double strength);\n> > > > +\n> > > > +       double exposureConstraintMaxBrightPixels_;\n> > > > +       double exposureConstraintY_;\n> > > > +       double minExposureValue_;\n> > > > +       double strength_;\n> > > > +       bool started_ = false;\n> > > > +\n> > > > +       controls::draft::WdrModeEnum mode_;\n> > > > +       std::vector<double> hist_;\n> > > > +       PidController pid_;\n> > > > +\n> > > > +       std::array<int, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV> toneCurveIntervalValues_;\n> > > > +       std::array<double, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1> toneCurveX_;\n> > > > +       std::array<double, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1> toneCurveY_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index 60cfab228edf..32f6db30bbdb 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -130,6 +130,10 @@ struct IPAActiveState {\n> > > >         struct {\n> > > >                 double gamma;\n> > > >         } goc;\n> > > > +\n> > > > +       struct {\n> > > > +               double exposureValue;\n> > > > +       } wdr;\n> > > >  };\n> > > >\n> > > >  struct IPAFrameContext : public FrameContext {\n> > > > @@ -198,6 +202,13 @@ struct IPAFrameContext : public FrameContext {\n> > > >         struct {\n> > > >                 double lux;\n> > > >         } lux;\n> > > > +\n> > > > +       struct {\n> > > > +               controls::draft::WdrModeEnum mode;\n> > > > +               double wdrExposureValue;\n> > > > +               double agcExposureValue;\n> > > > +               double strength;\n> > > > +       } wdr;\n> > > >  };\n> > > >\n> > > >  struct IPAContext {\n> > > > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> > > > index 4c0b051ce65d..5edb36c91b87 100644\n> > > > --- a/src/ipa/rkisp1/params.cpp\n> > > > +++ b/src/ipa/rkisp1/params.cpp\n> > > > @@ -74,6 +74,7 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {\n> > > >         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandBls, COMPAND_BLS, compand_bls),\n> > > >         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandExpand, COMPAND_EXPAND, compand_curve),\n> > > >         RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandCompress, COMPAND_COMPRESS, compand_curve),\n> > > > +       RKISP1_BLOCK_TYPE_ENTRY_EXT(Wdr, WDR, wdr),\n> > > >  };\n> > > >\n> > > >  } /* namespace */\n> > > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> > > > index 40450e34497a..2e60528d102e 100644\n> > > > --- a/src/ipa/rkisp1/params.h\n> > > > +++ b/src/ipa/rkisp1/params.h\n> > > > @@ -40,6 +40,7 @@ enum class BlockType {\n> > > >         CompandBls,\n> > > >         CompandExpand,\n> > > >         CompandCompress,\n> > > > +       Wdr,\n> > > >  };\n> > > >\n> > > >  namespace details {\n> > > > @@ -74,6 +75,7 @@ RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)\n> > > >  RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)\n> > > >  RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)\n> > > >  RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n> > > > +RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr)\n> > > >\n> > > >  } /* namespace details */\n> > > >\n> > > > diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml\n> > > > index 797532712099..9489e677402e 100644\n> > > > --- a/src/libcamera/control_ids_debug.yaml\n> > > > +++ b/src/libcamera/control_ids_debug.yaml\n> > > > @@ -3,4 +3,9 @@\n> > > >  %YAML 1.1\n> > > >  ---\n> > > >  vendor: debug\n> > > > -controls: []\n> > > > +controls:\n> > > > +- WdrCurve:\n> > > > +    type: Point\n> > > > +    direction: out\n> > > > +    description: Debug control WdrCurve found in src/ipa/rkisp1/algorithms/wdr.cpp\n> > > > +    size: '[n]'\n> > >\n> > > I love the idea of having debug controls that let us plot debug\n> > > internals - but if these are generated by script - do we need a way to\n> > > make sure they are only ever append only - if we ever merge a debug\n> > > control - suddenly the id number becomes part of the ABI - and adding\n> > > new debug controls can't change the existing ABI. Could be handled with\n> > > manual review ... but could then irritate the automatic tools for\n> > > createing the controls in the first place ...\n> >\n> > Ohhh bummer. I need to think about that. We definitely shouldn't require\n> > them to be append only. That would lead to a massive mess. Do we do the\n> > same for draft controls? Is there no way to deprecate a control?\n>\n> We can 'deprecate' it by saying it's not used any more and never\n> exposing it ...\n>\n> But the pain point is that each control is backed by an integer number\n> which is based by created an enum and is therefore 'the order in which\n> they are defined'.\n>\n> If one application thinks WdrCurve is control 10001 and anther thinks\n> it's 10005 we have a problem :-(\n>\n> And I wish we could solve that better somehow. But it means we have to\n> only append controls to any namespace. Otherwise it's an ABI break and\n> the SONAME has to be bumped.\n>\n> > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > > > index 03309eeac34f..0c0af7a19968 100644\n> > > > --- a/src/libcamera/control_ids_draft.yaml\n> > > > +++ b/src/libcamera/control_ids_draft.yaml\n> > > > @@ -293,5 +293,75 @@ controls:\n> > > >\n> > > >          Currently identical to ANDROID_STATISTICS_FACE_IDS.\n> > > >        size: [n]\n> > > > +  - WdrMode:\n> > >\n> > > Can we avoid these going into ::draft:: at all ?\n> > >\n> > > > +      type: int32_t\n> > > > +      direction: inout\n> > > > +      description: |\n> > > > +        Set the WDR mode.\n> > > > +\n> > > > +        The WDR mode is used to select the algorithm used for global tone\n> > > > +        mapping. It will automatically reduce the exposure time of the sensor\n> > > > +        so that there are only a small number of saturated pixels in the image.\n> > > > +        The algorithm then compensates for the loss of brightness by applying a\n> > > > +        global tone mapping curve to the image.\n> > > > +      enum:\n> > > > +        - name: WdrOff\n> > > > +          value: 0\n> > > > +          description: Wdr is disabled.\n> > > > +        - name: WdrLinear\n> > > > +          value: 1\n> > > > +          description:\n> > > > +            Apply a linear global tone mapping curve.\n> > >\n> > > I'm torn here on naming. Cue the bikeshed door opening ...\n> >\n> > I'm putting on my safety gear... :-)\n> >\n> > >\n> > > But we use the control name 'Wdr' and say it applies 'Global Tone\n> > > Mapping' configurations.\n> > >\n> > > Is WDR sufficiently known to represent Global Tone Mapping - are the\n> > > terms sufficiently interchangable ?\n> >\n> > That is not really easy to solve. I googled \"WDR vs HDR\" which lead to\n> > everything. Articles that contradict each other, pure nonsense and\n> > company specific nomenclature. I would not limit it to global tone\n> > mapping only - that's only what we have right now but I'd love to see\n> > local tone mapping added to the area of \"libcamera WDR/HDR controls\". As\n> > another side note, the upcoming HDR stitcher support will not expose own\n> > controls but just plug into these Wdr* controls.\n> >\n> > We could discuss if we lean more to HDR instead of WDR... My gut feeling\n> > is that WDR is a broead spectrum of things to increase the perceived\n> > dynamic range (Local tone mapping, global tone mapping, single low\n> > exposure + digital gain, on-chip fusion, multi-exposure) while with HDR\n> > I think of at least two exposures. But I'm sure you'll find a lot of\n> > articles/people disagreeing here... so... don't know :-)\n> >\n>\n> I agree - HDR is definitely separate and to me really means multiple\n> exposures in our contexts.\n\nI certainly agree that confusion abounds. Whether we make better by\nsaying \"we mean *this*\", I don't know. You can apply exactly the same\nHDR procedures to a single image as to multiple fused images, the\nissue might be that you get a noisy or suboptimal result. Whether\ndifferent names makes sense for what is the same processing... not\nclear to me. But the distinction feels a bit artificial.\n\nAs another point of reference, the sensor HDR we have on the IMX708\n(fusing multiple exposures) is enabled by V4L2's \"Wide Dynamic Range\"\ncontrol (because, actually, there isn't a \"High Dynamic Range\"\ncontrol), which further messes with the nomenclature.\n\nFor what it's worth, I always think of WDR/HDR in two steps. There's\nthe creation of the wide/high dynamic range image, and then there's\ntonemapping.\n\nWhen you stitch a long and short image together (for example), you get\na wide/high dynamic range image, though it's probably still linear and\nyou haven't tonemapped anything.\n\nThere are other ways to get this though. We can merge multiple short\nexposure images together to get a few more fractional bits at the\nbottom of the range, and this gives a kind of wide/high dynamic range\nimage too.\n\nArguably, just scaling up a single short exposure image gives you a\nwide/high dynamic range image. It has more dynamic range than a long\nexposure image, though you don't have the corresponding signal\nresolution, along with noise/other problems.\n\nSo yes, all dead confusing!\n\nSorry to muddy the waters yet more...\n\nDavid\n\n>\n> I'm fine with 'WDR' meaning the processing on the single image ...\n>\n> > > Do we have other hardware with 'WDR' capabilities to compare if the\n> > > controls make sense on other devices already?\n> > >\n> > > Dan - I expect the Mali-C55 to be quite HDR capable ... is there a WDR\n> > > block there that would benefit/utilise the same controls and concepts?\n> > >\n> > >\n> > >\n> > > > +\n> > > > +            The curve with two linear sections is applied. This produces good\n> > > > +            results at the expense of a slightly artificial look.\n> > > > +        - name: WdrPower\n> > > > +          value: 2\n> > > > +          description: |\n> > > > +            Apply a power global tone mapping curve.\n> > > > +\n> > > > +            This curve has high gain values on the dark areas of an image and\n> > > > +            high compression values on the bright area. It therefore tends to\n> > > > +            produce noticeable noise artifacts.\n> > > > +        - name: WdrExponential\n> > > > +          value: 3\n> > > > +          description: |\n> > > > +            Apply a exponential global tone mapping curve.\n> > > > +\n> > > > +            This curve has lower gain values in dark areas compared to the power\n> > > > +            curve but produces a more natural look compared to the linear curve.\n> > > > +            It is therefore the best choice for most scenes.\n> > > > +        - name: WdrHistogramEqualization\n> > > > +          value: 4\n> > > > +          description: |\n> > > > +            Apply histogram equalization.\n> > > > +\n> > > > +            This curve preserves most of the information of the image at the\n> > > > +            expense of a very artificial look. It is therefore best suited for\n> > > > +            technical analysis.\n> > > > +  - WdrStrength:\n> > > > +      type: float\n> > > > +      direction: in\n> > > > +      description: |\n> > > > +        Specify the strength of the wdr algorithm. The exact meaning of this\n> > > > +        value is specific to the algorithm in use. Usually a value of 0 means no\n> > > > +        global tone mapping is applied. A values of 1 is the default value and\n> > > > +        the correct value for most scenes. A value above 1 increases the global\n> > > > +        tone mapping effect and can lead to unrealistic image effects.\n> > > > +  - WdrMaxBrightPixels:\n> > > > +      type: float\n> > > > +      direction: in\n> > > > +      description: |\n> > > > +        Percentage of allowed (nearly) saturated pixels. The WDR algorithm\n> > > > +        reduces the WdrExposureValue until the amount of pixels that are close\n> > > > +        to saturation is lower than this value.\n> > > > +  - WdrExposureValue:\n> > > > +      type: float\n> > > > +      direction: out\n> > > > +      description: |\n> > > > +        Reports the Exposure Value that was applied to the AEGC regulation so\n> > > > +        that the WdrMaxBrightPixels limit is reached.\n> > >\n> > > Is that /different/ to the ExposureValue already reported in metadata?\n> >\n> > Yes, the ExposureValue is applied by the user leading to a over/under\n> > exposed output image. WdrExposureValue is applied under the hood and\n> > equalized by tone mapping algorithm. So a WdrExposureValue of -2\n> > basically says \"There are very bright areas in the image, I need to work\n> > hard\" but the output is still exposed as with ExposureValue=0. I don't\n> > know if that will be the approach taken for other ISPs as it is based on\n> > the limitations of the imx8mp ISP. I think time will tell.\n>\n> Ack.\n>\n> >\n> > Best regards,\n> > Stefan\n> >\n> > >\n> > > > +\n> > > > +        \\sa ExposureValue\n> > > >\n> > > >  ...\n> > > > --\n> > > > 2.48.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 59EB4BEFBE\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 13 Aug 2025 12:12:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0274169249;\n\tWed, 13 Aug 2025 14:12:09 +0200 (CEST)","from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com\n\t[IPv6:2607:f8b0:4864:20::72f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97CBC61444\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 13 Aug 2025 14:12:07 +0200 (CEST)","by mail-qk1-x72f.google.com with SMTP id\n\taf79cd13be357-7e849d7a28dso333600585a.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 13 Aug 2025 05:12:07 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"KdcJCmI0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1755087126; x=1755691926;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=HiCGDyyCuESlNNvIJqZoflcp5/hohYCmMQ/eih+hx40=;\n\tb=KdcJCmI03cbl8VRlZ7KeVoWeBl7UHA196ld+YK9GeQ9x+/LqGMITMwSJOvRwklCOgi\n\tgrtmJrp5cmTfoR/qnUjqM/WPp/FHWyGPWVE4VVJHut7uHWeIKC9LNUFO8uqGPWbM5gub\n\tQ+xgkYKOgz1BT+OnGPbzA3i+bYG9d5cDONjlJywCyf+pKNjyIWTdR3Jq2nrX/+NiY7DA\n\thSHtdnaz5HXAaWOHBzKG9CcM81+7ioLXcKEiCmPAUhm1c7AAmbXblQgS3wLwUiw434yV\n\t2Nu7k9PMLBSPTEHA3R5STHpA0OfYl31q5f2ozKJ1G3wyi6ZplBAFb2qmm9F2wFFfTVXL\n\tFYKg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1755087126; x=1755691926;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=HiCGDyyCuESlNNvIJqZoflcp5/hohYCmMQ/eih+hx40=;\n\tb=N3NQprd3Hq1z4leNJFNO0yAdMMAQoHjWJs+2KmsF/1V3ICR3qlK5NiBCW6HgZ+fw8J\n\t6AIA3Xm7Mnfnvhy2/nYlWCvVEVbRTHETvBFWQ/b+gdHav/uteEIAGFuqmqJSJ4hF/G+q\n\tcojsMsmS9aElqA1Q/l4rn8x8irlhwARNoK4ClRtsWLdx6Nf4bYPrXUj5SBefLCLun9oH\n\todP5ODlj/oahaksihIQ5Epa9hpFiD5g5D+yVYTtmB2y242VCjkFgWk6LsFOHJ3m7Xaeo\n\tOt/7AVI/n7IX3FdbAcXmugbp71GFrbGlOr3IjWHBTquptZyujejCs3kHAGom9ssiRY89\n\tLcng==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCX/MuES6Zt62VJ6+fQ+nyOxupFC/ureFKhbMp1uNNwA0vvODM5e80fTYLFoVe1hE3sYfFqSMVxZbS5dhB532OI=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxToDaf9nJpfFM+htSb0GqH2HQmIoFhugyGr0uS2SBwXNmEJ9VH\n\tBdql+NUi/74B9NgKTPd5CY41rOsWXN2h1gFS7KFEZYCuqmPYzge4MMk0KBHYqrO3JIVGb82/9xB\n\tPptmKg6qb3FttcYovOoS0E/sPbqZ0vSmTSfUHtbM+lA==","X-Gm-Gg":"ASbGnctTK+vYd7EZwMWEJxEVvfiNBf/HbDCzf7dhvkSUNDB+cGVlZzryFo79VCVVaxp\n\t7KJcOKA/gjmsTo+FsEEpQkCFbjWUo+OqjI3Z15ZPjIU8M9DrN1y9nbSuUymJNsMru3iNLVTorLy\n\tOEPDD14DtTD9a5/R4pfcd94ScNuLJ+xaOI8TQNnKSMK7nCNk910GleJOCnwvwJ504FMTypCScY+\n\t8GNqVOz8CwPMmESvDrGpSoBkca5Zbcj62AYqj0=","X-Google-Smtp-Source":"AGHT+IFkmbQymiIEO1c+xIHfllfA/uv2WE+C+AqJxndZxcnQ4MBG72SUzsajiqnM5E1jpi/T34MCgU8bXfc8BuT19QU=","X-Received":"by 2002:a05:620a:9366:b0:7e8:5143:d640 with SMTP id\n\taf79cd13be357-7e8652fef3amr332730485a.49.1755087125691;\n\tWed, 13 Aug 2025 05:12:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-16-stefan.klug@ideasonboard.com>\n\t<175493316838.1641235.2414681416886447452@ping.linuxembedded.co.uk>\n\t<175508169312.1225443.2627596737853254972@localhost>\n\t<175508279425.560048.16769889226335004715@ping.linuxembedded.co.uk>","In-Reply-To":"<175508279425.560048.16769889226335004715@ping.linuxembedded.co.uk>","From":"David Plowman <david.plowman@raspberrypi.com>","Date":"Wed, 13 Aug 2025 13:11:54 +0100","X-Gm-Features":"Ac12FXycWSqMJknu6x6iRc2FBDq4E6B9lIv5ETSOmFMfj0MiHb7-yaus4pBtJSo","Message-ID":"<CAHW6GY+a7FWLrzuRZBtPNim=byKnxpv+a5G072r832DDm0aQEg@mail.gmail.com>","Subject":"Re: [PATCH v2 15/16] ipa: rkisp1: Add WDR algorithm","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Dan Scally <dan.scally@ideasonboard.com>,\n\tStefan Klug <stefan.klug@ideasonboard.com>, \n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":35408,"web_url":"https://patchwork.libcamera.org/comment/35408/","msgid":"<20e3adef-b03f-4078-9062-1e86d265c855@ideasonboard.com>","date":"2025-08-14T13:01:01","subject":"Re: [PATCH v2 15/16] ipa: rkisp1: Add WDR algorithm","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Kieran, Stefan\n\nOn 11/08/2025 18:26, Kieran Bingham wrote:\n> Dan - hidden question to you way at the bottom of this one!\n\nSorry this took a few days!\n\n> \n> Quoting Stefan Klug (2025-08-08 15:12:53)\n>> Add a WDR algorithm to do global tone mapping. Global tone mapping is\n>> used to increase the perceived dynamic range of an image. The typical\n>> effect is that in areas that are normally overexposed, additional\n>> structure becomes visible.\n>>\n>> The overall idea is that the algorithm applies an exposure value\n>> correction to underexpose the image to the point where only a small\n>> number of saturated pixels is left. This artificial underexposure is\n>> then mitigated by applying a tone mapping curve.\n>>\n>> This algorithm implements 4 tone mapping strategies:\n>> - Linear\n>> - Power\n>> - Exponential\n>> - Histogram equalization\n>>\n>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n>>\n>> ---\n>>\n>> Changes in v2:\n>> - Fixed default value for min bright pixels\n>> - Added check for supported params type\n>> - Reset PID controller\n>> - Various fixes from Pauls review\n>> ---\n>>   src/ipa/rkisp1/algorithms/meson.build |   1 +\n>>   src/ipa/rkisp1/algorithms/wdr.cpp     | 510 ++++++++++++++++++++++++++\n>>   src/ipa/rkisp1/algorithms/wdr.h       |  64 ++++\n>>   src/ipa/rkisp1/ipa_context.h          |  11 +\n>>   src/ipa/rkisp1/params.cpp             |   1 +\n>>   src/ipa/rkisp1/params.h               |   2 +\n>>   src/libcamera/control_ids_debug.yaml  |   7 +-\n>>   src/libcamera/control_ids_draft.yaml  |  70 ++++\n>>   8 files changed, 665 insertions(+), 1 deletion(-)\n>>   create mode 100644 src/ipa/rkisp1/algorithms/wdr.cpp\n>>   create mode 100644 src/ipa/rkisp1/algorithms/wdr.h\n>>\n>> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n>> index 2e42a80cf99d..d329dbfb432d 100644\n>> --- a/src/ipa/rkisp1/algorithms/meson.build\n>> +++ b/src/ipa/rkisp1/algorithms/meson.build\n>> @@ -14,4 +14,5 @@ rkisp1_ipa_algorithms = files([\n>>       'gsl.cpp',\n>>       'lsc.cpp',\n>>       'lux.cpp',\n>> +    'wdr.cpp',\n>>   ])\n>> diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp\n>> new file mode 100644\n>> index 000000000000..0c1b261d4373\n>> --- /dev/null\n>> +++ b/src/ipa/rkisp1/algorithms/wdr.cpp\n>> @@ -0,0 +1,510 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2025, Ideas On Board\n>> + *\n>> + * RkISP1 Wide Dynamic Range control\n>> + */\n>> +\n>> +#include \"wdr.h\"\n>> +\n>> +#include <libcamera/base/log.h>\n>> +#include <libcamera/base/utils.h>\n>> +\n>> +#include \"libcamera/internal/yaml_parser.h\"\n>> +\n>> +#include <libipa/histogram.h>\n>> +#include <libipa/pwl.h>\n>> +\n>> +#include \"linux/rkisp1-config.h\"\n>> +\n>> +/**\n>> + * \\file wdr.h\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::rkisp1::algorithms {\n>> +\n>> +/**\n>> + * \\class WideDynamicRange\n>> + * \\brief RkISP1 Wide Dynamic Range algorithm\n>> + *\n>> + * This algorithm implements automatic global tone mapping for the RkISP1.\n>> + * Global tone mapping is done by the GWDR hardware block and applies\n>> + * a global tone mapping curve to the image to increase the perceived dynamic\n>> + * range. Imagine an indoor scene with bright outside visible through the\n>> + * windows. With normal exposure settings, the windows will be completely\n>> + * saturated and no structure (sky/clouds) will be visible because the AEGC has\n>> + * to increase overall exposure to reach a certain level of mean brightness. In\n>> + * WDR mode, the algorithm will artifically reduce the exposure time so that the\n>> + * texture and colours become visible in the formerly saturated areas. Then the\n>> + * global tone mapping curve is applied to mitigate the loss of brightness.\n>> + *\n>> + * Calculating that tone mapping curve is the most difficult part. This\n>> + * algorithm implements four tone mapping strategies:\n>> + * - Linear: The tone mapping curve is a combination of two linear functions\n>> + * with one kneepoint\n> \n> Can this 'with one kneepiont' be indented to match the Linear ?\n> \n> \n>   * - Linear: The tone mapping curve is a combination of two linear functions\n>   *   with one kneepoint\n> \n> \n>> + * - Power: The tone mapping curve follows a power function\n>> + * - Exponential: The tone mapping curve follows an exponential function\n>> + * - HistogramEqualization: The tone mapping curve tries to equalize the\n>> + * histogram\n> \n> Same here.?\n> \n>> + *\n>> + * The overall strategy is the same in all cases: A negative exposure value is\n>> + * applied to the AEGC regulation until the number of nearly saturated pixels go\n>> + * below a given threshold (controllable via WdrMaxBrightPixels, default is 2%)\n>> + * or the MinExposureValue specified in the tuning file is reached.\n>> + *\n>> + * The global tone mapping curve is then calculated so that it accounts for the\n>> + * reduction of brightness due to the negative exposure value. As the result of\n>> + * tone mapping is very difficult to quantize and is by definition a lossy\n>> + * process there is not a single \"correct\" solution.\n>> + *\n>> + * The approach taken here is based on a simple linear model. Consider a pixel\n>> + * that was originally 50% grey. It will have its exposure pushed down by the\n>> + * WDR's initial exposure compensation. This value then needs to be pushed back\n>> + * up by the tone mapping curve so that it is 50% grey again. This point serves\n>> + * as our kneepoint. To get to this kneepoint, this pixel and all darker pixels\n>> + * (to the left of the kneepoint on the tone mapping curve) will simply have the\n>> + * exposure compensation undone by pow(2, -WDR_EV). This cancels out the\n>> + * original exposure compensation, which was pow(2, WDR_EV). The remaining\n>> + * brigher pixels (to the right of the kneepoint on the tone mapping curve) will\n>> + * be compressed. The WdrStrength control adjusts the gain of the left part of\n>> + * the tone mapping curve.\n>> + *\n>> + * In the Power and Exponential modes, the curves are calculated so that they\n>> + * pass through that kneepoint.\n>> + *\n>> + * The histogram equalization mode tries to equalize the histogram of the\n>> + * image and acts independently of the calculated exposure value.\n>> + *\n>> + * \\code{.unparsed}\n>> + * algorithms:\n>> + *   - WideDynamicRange:\n>> + *       ExposureConstraint:\n>> + *         MaxBrightPixels: 0.02\n>> + *         yTarget: 0.95\n>> + *       MinExposureValue: -4.0\n>> + * \\endcode\n>> + */\n\nDrive by comment to say that this explanation is really excellent\n\n>> +\n>> +LOG_DEFINE_CATEGORY(RkISP1Wdr)\n>> +\n>> +static constexpr unsigned int kTonecurveXIntervals = RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV;\n>> +\n>> +/*\n>> + * Increasing interval sizes. The intervals are crafted so that they sum\n>> + * up to 4096. This results in better fitting curves than the constant intervals\n>> + * (all entries are 4)\n>> + */\n>> +static constexpr std::array<int, kTonecurveXIntervals> kLoglikeIntervals = {\n>> +       { 0, 0, 0, 0, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3, 3, 4,\n>> +         4, 4, 4, 4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 5, 6, 6 }\n>> +};\n>> +\n>> +WideDynamicRange::WideDynamicRange()\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\copydoc libcamera::ipa::Algorithm::init\n>> + */\n>> +int WideDynamicRange::init([[maybe_unused]] IPAContext &context,\n>> +                          [[maybe_unused]] const YamlObject &tuningData)\n>> +{\n>> +       if (!(context.hw->supportedBlocks & 1 << RKISP1_EXT_PARAMS_BLOCK_TYPE_WDR)) {\n>> +               LOG(RkISP1Wdr, Error)\n>> +                       << \"Wide Dynamic Range not supported by the hardware or kernel.\";\n>> +               return -ENOTSUP;\n>> +       }\n>> +\n>> +       toneCurveIntervalValues_ = kLoglikeIntervals;\n>> +\n>> +       /* Calculate a list of normed x values */\n>> +       toneCurveX_[0] = 0.0;\n>> +       int lastValue = 0;\n>> +       for (unsigned int i = 1; i < toneCurveX_.size(); i++) {\n>> +               lastValue += std::pow(2, toneCurveIntervalValues_[i - 1] + 3);\n>> +               lastValue = std::min(lastValue, 4096);\n>> +               toneCurveX_[i] = lastValue / 4096.0;\n>> +       }\n>> +\n>> +       strength_ = 1.0;\n>> +       mode_ = controls::draft::WdrOff;\n>> +       exposureConstraintMaxBrightPixels_ = 0.02;\n>> +       exposureConstraintY_ = 0.95;\n>> +       minExposureValue_ = -4.0;\n>> +\n>> +       const auto &constraint = tuningData[\"ExposureConstraint\"];\n>> +       if (!constraint.isDictionary()) {\n>> +               LOG(RkISP1Wdr, Warning)\n>> +                       << \"ExposureConstraint not found in tuning data.\"\n>> +                          \"Using default values MaxBrightPixels: \"\n>> +                       << exposureConstraintMaxBrightPixels_\n>> +                       << \" yTarget: \" << exposureConstraintY_;\n>> +       } else {\n>> +               exposureConstraintMaxBrightPixels_ =\n>> +                       constraint[\"MaxBrightPixels\"]\n>> +                               .get<double>()\n>> +                               .value_or(exposureConstraintMaxBrightPixels_);\n>> +               exposureConstraintY_ =\n>> +                       constraint[\"yTarget\"]\n>> +                               .get<double>()\n>> +                               .value_or(exposureConstraintY_);\n>> +       }\n>> +\n>> +       const auto &minExp = tuningData[\"MinExposureValue\"];\n>> +       minExposureValue_ = minExp.get<double>().value_or(minExposureValue_);\n>> +       if (!minExp) {\n>> +               LOG(RkISP1Wdr, Warning)\n>> +                       << \"MinExposureValue not found in tuning data.\"\n>> +                          \" Using default value: \"\n>> +                       << minExposureValue_;\n>> +       }\n> \n> { } aren't required for single statement...\n> \n>> +\n>> +       context.ctrlMap[&controls::draft::WdrMode] =\n>> +               ControlInfo(controls::draft::WdrModeValues, controls::draft::WdrOff);\n>> +       context.ctrlMap[&controls::draft::WdrStrength] =\n>> +               ControlInfo(0.0f, 2.0f, static_cast<float>(strength_));\n>> +       context.ctrlMap[&controls::draft::WdrMaxBrightPixels] =\n>> +               ControlInfo(0.0f, 1.0f, static_cast<float>(exposureConstraintMaxBrightPixels_));\n>> +\n>> +       applyCompensationLinear(1.0, 0.0);\n>> +\n>> +       return 0;\n>> +}\n>> +\n>> +void WideDynamicRange::applyHistogramEqualization(double strength)\n>> +{\n>> +       if (hist_.empty())\n>> +               return;\n>> +\n>> +       strength *= 0.65;\n> \n> Why 0.65 ?\n> \n>> +\n>> +       /*\n>> +        * In a fully equalized histogram, all bins have the same value. Try\n>> +        * to equalize the histogram by applying a gain or damping depending on\n>> +        * the distance of the actual bin value from that norm.\n>> +        */\n>> +       std::vector<double> gains;\n>> +       gains.resize(hist_.size());\n>> +       double sum = 0;\n>> +       double norm = 1.0 / (gains.size());\n>> +       for (unsigned i = 0; i < hist_.size(); i++) {\n>> +               double diff = 1.0 + strength * (hist_[i] - norm) / norm;\n>> +               gains[i] = diff;\n>> +               sum += diff;\n>> +       }\n>> +       /* Never amplify the last entry. */\n>> +       gains.back() = std::max(gains.back(), 1.0);\n>> +\n>> +       double scale = gains.size() / sum;\n>> +       for (auto &v : gains)\n>> +               v *= scale;\n>> +\n>> +       Pwl pwl;\n>> +       double step = 1.0 / gains.size();\n>> +       double lastX = 0;\n>> +       double lastY = 0;\n>> +\n>> +       pwl.append(lastX, lastY);\n>> +       for (unsigned int i = 0; i < gains.size() - 1; i++) {\n>> +               lastY += gains[i] * step;\n>> +               lastX += step;\n>> +               pwl.append(lastX, lastY);\n>> +       }\n>> +       pwl.append(1.0, 1.0);\n>> +\n>> +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n>> +               toneCurveY_[i] = pwl.eval(toneCurveX_[i]);\n>> +}\n>> +\n>> +Vector<double, 2> WideDynamicRange::kneePoint(double gain, double strength)\n>> +{\n>> +       gain = std::pow(gain, strength);\n>> +       double y = 0.5;\n>> +       double x = y / gain;\n>> +\n>> +       return { { x, y } };\n>> +}\n>> +\n>> +void WideDynamicRange::applyCompensationLinear(double gain, double strength)\n>> +{\n>> +       auto kp = kneePoint(gain, strength);\n>> +       double g1 = kp.y() / kp.x();\n>> +       double g2 = (kp.y() - 1) / (kp.x() - 1);\n>> +\n>> +       for (unsigned int i = 0; i < toneCurveX_.size(); i++) {\n>> +               double x = toneCurveX_[i];\n>> +               double y;\n>> +               if (x <= kp.x()) {\n>> +                       y = g1 * x;\n>> +               } else {\n>> +                       y = g2 * x + 1 - g2;\n>> +               }\n>> +               toneCurveY_[i] = y;\n>> +       }\n>> +}\n>> +\n>> +void WideDynamicRange::applyCompensationPower(double gain, double strength)\n>> +{\n>> +       double e = 1.0;\n>> +       if (strength > 1e-6) {\n>> +               auto kp = kneePoint(gain, strength);\n>> +               /* Calculate an exponent to go through the knee point. */\n>> +               e = log(kp.y()) / log(kp.x());\n>> +       }\n>> +\n>> +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n>> +               toneCurveY_[i] = std::pow(toneCurveX_[i], e);\n>> +}\n>> +\n>> +void WideDynamicRange::applyCompensationExponential(double gain, double strength)\n>> +{\n>> +       double k = 0.1;\n>> +       auto kp = kneePoint(gain, strength);\n>> +       double kx = kp.x();\n>> +       double ky = kp.y();\n>> +\n>> +       if (kx > ky) {\n>> +               LOG(RkISP1Wdr, Warning) << \"Invalid knee point: \" << kp;\n>> +               kx = ky;\n>> +       }\n>> +\n>> +       /*\n>> +        * The exponential curve is based on the function proposed by Glozman\n>> +        * et al. in\n>> +        * S. Glozman, T. Kats, and O. Yadid-Pecht, \"Exponent Operator Based\n>> +        * Tone Mapping Algorithm for Color Wide Dynamic Range Images,\" 2011.\n>> +        *\n>> +        * That function uses a k factor as parameter for the WDR compression\n>> +        * curve:\n>> +        * k=0: maximum compression\n>> +        * k=infinity: linear curve\n>> +        *\n>> +        * To calculate a k factor that results in a curve that passes through\n>> +        * the kneepoint, the equation needs to be solved for k after inserting\n>> +        * the kneepoint.  This can be formulated as search for a zero point.\n>> +        * Unfortunately there is no closed solution for that transformation.\n>> +        * Using newton's method to approximate the value is numerically\n>> +        * unstable.\n>> +        *\n>> +        * Luckily the function only crosses the x axis once and for the set of\n>> +        * possible kneepoints, a negative and a positive point can be guessed.\n>> +        * The approximation is then implemented using bisection.\n>> +        */\n>> +       if (std::abs(kx - ky) < 0.001) {\n>> +               k = 1e8;\n>> +       } else {\n>> +               double kl = 0.0001;\n>> +               double kh = 1000;\n>> +\n>> +               auto fk = [=](double v) {\n>> +                       return std::exp(-kx / v) -\n>> +                              ky * std::exp(-1.0 / v) + ky - 1.0;\n>> +               };\n>> +\n>> +               ASSERT(fk(kl) < 0);\n>> +               ASSERT(fk(kh) > 0);\n>> +\n>> +               k = kh / 10.0;\n>> +               while (fk(k) > 0) {\n>> +                       kh = k;\n>> +                       k /= 10.0;\n>> +               }\n>> +\n>> +               do {\n>> +                       k = (kl + kh) / 2;\n>> +                       if (fk(k) < 0)\n>> +                               kl = k;\n>> +                       else\n>> +                               kh = k;\n>> +               } while (std::abs(kh - kl) > 1e-3);\n>> +       }\n>> +\n>> +       double a = 1.0 / (1.0 - std::exp(-1.0 / k));\n>> +       for (unsigned int i = 0; i < toneCurveX_.size(); i++)\n>> +               toneCurveY_[i] = a * (1.0 - std::exp(-toneCurveX_[i] / k));\n>> +}\n>> +\n>> +/**\n>> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n>> + */\n>> +void WideDynamicRange::queueRequest([[maybe_unused]] IPAContext &context,\n>> +                                   [[maybe_unused]] const uint32_t frame,\n>> +                                   IPAFrameContext &frameContext,\n>> +                                   const ControlList &controls)\n>> +{\n>> +       strength_ = controls.get(controls::draft::WdrStrength).value_or(strength_);\n>> +       exposureConstraintMaxBrightPixels_ =\n>> +               controls.get(controls::draft::WdrMaxBrightPixels)\n>> +                       .value_or(exposureConstraintMaxBrightPixels_);\n>> +\n>> +       const auto &mode = controls.get(controls::draft::WdrMode);\n>> +       if (mode) {\n>> +               mode_ = static_cast<controls::draft::WdrModeEnum>(*mode);\n>> +       }\n> \n> No { } required here.\n> \n>> +\n>> +       frameContext.wdr.mode = mode_;\n>> +       frameContext.wdr.strength = strength_;\n>> +}\n>> +\n>> +/**\n>> + * \\copydoc libcamera::ipa::Algorithm::prepare\n>> + */\n>> +void WideDynamicRange::prepare(IPAContext &context,\n>> +                              [[maybe_unused]] const uint32_t frame,\n>> +                              IPAFrameContext &frameContext,\n>> +                              RkISP1Params *params)\n>> +{\n>> +       if (!params) {\n>> +               LOG(RkISP1Wdr, Warning) << \"Params is null\";\n>> +               return;\n>> +       }\n>> +\n>> +       auto config = params->block<BlockType::Wdr>();\n>> +\n>> +       auto mode = frameContext.wdr.mode;\n>> +\n>> +       config.setEnabled(mode != controls::draft::WdrOff);\n>> +\n>> +       double comp = 0;\n>> +\n>> +       /*\n>> +        * Todo: This overwrites frameContext.agc.exposureValue so that\n>> +        * in the next call to Agc::process() that exposureValue get's\n>> +        * applied. In the future it is planned to move the exposure\n>> +        * calculations from Agc::process() to Agc::prepare(). In this\n>> +        * case, we need to ensure that this code get's called early enough.\n>> +        */\n> \n> Eeep ...\n>    /me looks away for the moment.\n> \n>> +       if (mode != controls::draft::WdrOff) {\n>> +               frameContext.wdr.wdrExposureValue = context.activeState.wdr.exposureValue;\n>> +               frameContext.wdr.agcExposureValue = frameContext.agc.exposureValue;\n>> +\n>> +               /*\n>> +                * When WDR is enabled, the maxBrightPixels constraint is always\n>> +                * active. This is problematic when the user sets a positive\n>> +                * exposureValue. As this would mean that the negative WDR\n>> +                * exposure value and the user provided positive value should\n>> +                * cancel each other out. But as the regulation is not absolute\n>> +                * and only dependent on the measured changes in the histogram,\n>> +                * reducing the wdr EV would only lead to an even stronger pull\n>> +                * from regulation. Positive user supplied EVs must therefore be\n>> +                * handled using the wdr curve.\n>> +                */\n>> +               if (frameContext.wdr.wdrExposureValue < 0) {\n>> +                       frameContext.agc.exposureValue =\n>> +                               std::min(frameContext.agc.exposureValue,\n>> +                                        frameContext.wdr.wdrExposureValue);\n>> +                       comp = frameContext.agc.exposureValue - frameContext.wdr.agcExposureValue;\n>> +               }\n>> +       }\n>> +\n>> +       /* Calculate how much EV we need to compensate with the WDR curve. */\n>> +       double gain = pow(2.0, -comp);\n>> +\n>> +       if (mode == controls::draft::WdrOff) {\n>> +               applyCompensationLinear(1.0, 0.0);\n>> +       } else if (mode == controls::draft::WdrLinear) {\n>> +               applyCompensationLinear(gain, frameContext.wdr.strength);\n>> +       } else if (mode == controls::draft::WdrPower) {\n>> +               applyCompensationPower(gain, frameContext.wdr.strength);\n>> +       } else if (mode == controls::draft::WdrExponential) {\n>> +               applyCompensationExponential(gain, frameContext.wdr.strength);\n>> +       } else if (mode == controls::draft::WdrHistogramEqualization) {\n>> +               applyHistogramEqualization(frameContext.wdr.strength);\n>> +       }\n>> +\n>> +       /* Reset value */\n>> +       config->dmin_strength = 0x10;\n>> +       config->dmin_thresh = 0;\n>> +\n>> +       for (unsigned int i = 0; i < kTonecurveXIntervals; i++) {\n>> +               int v = toneCurveIntervalValues_[i];\n>> +               config->tone_curve.dY[i / 8] |= (v & 0x07) << ((i % 8) * 4);\n>> +       }\n>> +\n>> +       std::vector<Point> debugCurve;\n>> +\n>> +       /*\n>> +        * Fix the curve to adhere to the hardware constraints. Don't apply a\n>> +        * constraint on the first element, which is most likely zero anyways.\n>> +        */\n>> +       int lastY = toneCurveY_[0] * 4096.0;\n>> +       for (unsigned int i = 0; i < toneCurveX_.size(); i++) {\n>> +               int diff = static_cast<int>(toneCurveY_[i] * 4096.0) - lastY;\n>> +               diff = std::clamp(diff, -2048, 2048);\n>> +               lastY = lastY + diff;\n>> +               config->tone_curve.ym[i] = lastY;\n>> +               debugCurve.push_back({ static_cast<int>(toneCurveX_[i] * 4096.0),\n>> +                                      lastY });\n>> +       }\n>> +\n>> +       context.debugMetadata.set<Span<const Point>>(controls::debug::WdrCurve, debugCurve);\n>> +}\n>> +\n>> +void WideDynamicRange::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> +                              IPAFrameContext &frameContext,\n>> +                              const rkisp1_stat_buffer *stats,\n>> +                              ControlList &metadata)\n>> +{\n>> +       if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_HIST)) {\n>> +               LOG(RkISP1Wdr, Error) << \"No histogram data in statistics\";\n> \n> Is this a full error if this happens? or just a debug print ? I thought\n> sometimes on say a too dark image we might not get any (usable) stats ?\n> \n>> +               return;\n>> +       }\n>> +\n>> +       if (!started_) {\n>> +               started_ = true;\n>> +               pid_.setStandardParameters(1, 5.0, 3.0);\n>> +               pid_.setOutputLimits(minExposureValue_, 0.0);\n> \n> Can the minExposureValue_ ever be changed for instance if we get called\n> with different modes in configure() or if the frame duration limits get\n> changed ?\n> \n>> +               context.activeState.wdr.exposureValue = 0.0;\n> \n> Should those happen during configure() ? or init() ?\n> \n>   - init just once when constructed... (current behaviour)\n>   - or configure if every time the camera is reconfigured..\n> \n> Then we wouldn't need a started_ bool tracker ?\n> \n>> +       }\n>> +\n>> +       pid_.setTarget(exposureConstraintY_);\n>> +\n>> +       const rkisp1_cif_isp_stat *params = &stats->params;\n>> +       auto mode = frameContext.wdr.mode;\n>> +\n>> +       metadata.set(controls::draft::WdrMode, mode);\n>> +\n>> +       Histogram cumHist({ params->hist.hist_bins, context.hw->numHistogramBins },\n>> +                         [](uint32_t x) { return x >> 4; });\n>> +\n>> +       std::vector<double> hist;\n>> +       double sum = 0;\n>> +       for (unsigned i = 0; i < context.hw->numHistogramBins; i++) {\n>> +               double v = params->hist.hist_bins[i] >> 4;\n>> +               hist.push_back(v);\n>> +               sum += v;\n>> +       }\n>> +\n>> +       /* Scale so that the entries sum up to 1. */\n>> +       double scale = 1.0 / sum;\n>> +       for (auto &v : hist)\n>> +               v *= scale;\n>> +       hist_.swap(hist);\n>> +\n>> +       double mean = cumHist.quantile(1.0 - exposureConstraintMaxBrightPixels_) /\n>> +                     cumHist.bins();\n>> +       LOG(RkISP1Wdr, Debug) << \"Mean y of bright pixels: \" << mean;\n>> +\n>> +       if (mode == controls::draft::WdrOff) {\n>> +               metadata.set(controls::draft::WdrExposureValue, 0);\n>> +               return;\n>> +       }\n>> +\n>> +       context.activeState.wdr.exposureValue = pid_.process(mean);\n>> +       LOG(RkISP1Wdr, Debug) << \"Active state WDR ev: \" << context.activeState.wdr.exposureValue;\n>> +       metadata.set(controls::draft::WdrExposureValue, frameContext.wdr.wdrExposureValue);\n>> +\n>> +       /*\n>> +        * We overwrote agc exposure value in prepare() to create an\n>> +        * underexposure for WDR. Report the original exposure value in metadata.\n>> +        */\n>> +       metadata.set(controls::ExposureValue, frameContext.wdr.agcExposureValue);\n>> +}\n>> +\n>> +REGISTER_IPA_ALGORITHM(WideDynamicRange, \"WideDynamicRange\")\n>> +\n>> +} /* namespace ipa::rkisp1::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/rkisp1/algorithms/wdr.h b/src/ipa/rkisp1/algorithms/wdr.h\n>> new file mode 100644\n>> index 000000000000..bdea4eb5492a\n>> --- /dev/null\n>> +++ b/src/ipa/rkisp1/algorithms/wdr.h\n>> @@ -0,0 +1,64 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021-2022, Ideas On Board\n>> + *\n>> + * RkISP1 Wide Dynamic Range control\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include <libcamera/control_ids.h>\n>> +\n>> +#include \"libcamera/internal/pid_controller.h\"\n>> +\n>> +#include \"linux/rkisp1-config.h\"\n>> +\n>> +#include \"algorithm.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::rkisp1::algorithms {\n>> +\n>> +class WideDynamicRange : public Algorithm\n>> +{\n>> +public:\n>> +       WideDynamicRange();\n>> +       ~WideDynamicRange() = default;\n>> +\n>> +       int init(IPAContext &context, const YamlObject &tuningData) override;\n>> +\n>> +       void queueRequest(IPAContext &context, 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>> +                    RkISP1Params *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>> +       Vector<double, 2> kneePoint(double gain, double strength);\n>> +       void applyCompensationLinear(double gain, double strength);\n>> +       void applyCompensationPower(double gain, double strength);\n>> +       void applyCompensationExponential(double gain, double strength);\n>> +       void applyHistogramEqualization(double strength);\n>> +\n>> +       double exposureConstraintMaxBrightPixels_;\n>> +       double exposureConstraintY_;\n>> +       double minExposureValue_;\n>> +       double strength_;\n>> +       bool started_ = false;\n>> +\n>> +       controls::draft::WdrModeEnum mode_;\n>> +       std::vector<double> hist_;\n>> +       PidController pid_;\n>> +\n>> +       std::array<int, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV> toneCurveIntervalValues_;\n>> +       std::array<double, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1> toneCurveX_;\n>> +       std::array<double, RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1> toneCurveY_;\n>> +};\n>> +\n>> +} /* namespace ipa::rkisp1::algorithms */\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n>> index 60cfab228edf..32f6db30bbdb 100644\n>> --- a/src/ipa/rkisp1/ipa_context.h\n>> +++ b/src/ipa/rkisp1/ipa_context.h\n>> @@ -130,6 +130,10 @@ struct IPAActiveState {\n>>          struct {\n>>                  double gamma;\n>>          } goc;\n>> +\n>> +       struct {\n>> +               double exposureValue;\n>> +       } wdr;\n>>   };\n>>   \n>>   struct IPAFrameContext : public FrameContext {\n>> @@ -198,6 +202,13 @@ struct IPAFrameContext : public FrameContext {\n>>          struct {\n>>                  double lux;\n>>          } lux;\n>> +\n>> +       struct {\n>> +               controls::draft::WdrModeEnum mode;\n>> +               double wdrExposureValue;\n>> +               double agcExposureValue;\n>> +               double strength;\n>> +       } wdr;\n>>   };\n>>   \n>>   struct IPAContext {\n>> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n>> index 4c0b051ce65d..5edb36c91b87 100644\n>> --- a/src/ipa/rkisp1/params.cpp\n>> +++ b/src/ipa/rkisp1/params.cpp\n>> @@ -74,6 +74,7 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {\n>>          RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandBls, COMPAND_BLS, compand_bls),\n>>          RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandExpand, COMPAND_EXPAND, compand_curve),\n>>          RKISP1_BLOCK_TYPE_ENTRY_EXT(CompandCompress, COMPAND_COMPRESS, compand_curve),\n>> +       RKISP1_BLOCK_TYPE_ENTRY_EXT(Wdr, WDR, wdr),\n>>   };\n>>   \n>>   } /* namespace */\n>> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n>> index 40450e34497a..2e60528d102e 100644\n>> --- a/src/ipa/rkisp1/params.h\n>> +++ b/src/ipa/rkisp1/params.h\n>> @@ -40,6 +40,7 @@ enum class BlockType {\n>>          CompandBls,\n>>          CompandExpand,\n>>          CompandCompress,\n>> +       Wdr,\n>>   };\n>>   \n>>   namespace details {\n>> @@ -74,6 +75,7 @@ RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)\n>>   RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)\n>>   RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)\n>>   RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr)\n>>   \n>>   } /* namespace details */\n>>   \n>> diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml\n>> index 797532712099..9489e677402e 100644\n>> --- a/src/libcamera/control_ids_debug.yaml\n>> +++ b/src/libcamera/control_ids_debug.yaml\n>> @@ -3,4 +3,9 @@\n>>   %YAML 1.1\n>>   ---\n>>   vendor: debug\n>> -controls: []\n>> +controls:\n>> +- WdrCurve:\n>> +    type: Point\n>> +    direction: out\n>> +    description: Debug control WdrCurve found in src/ipa/rkisp1/algorithms/wdr.cpp\n>> +    size: '[n]'\n> \n> I love the idea of having debug controls that let us plot debug\n> internals - but if these are generated by script - do we need a way to\n> make sure they are only ever append only - if we ever merge a debug\n> control - suddenly the id number becomes part of the ABI - and adding\n> new debug controls can't change the existing ABI. Could be handled with\n> manual review ... but could then irritate the automatic tools for\n> createing the controls in the first place ...\n> \n> \n>> diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n>> index 03309eeac34f..0c0af7a19968 100644\n>> --- a/src/libcamera/control_ids_draft.yaml\n>> +++ b/src/libcamera/control_ids_draft.yaml\n>> @@ -293,5 +293,75 @@ controls:\n>>   \n>>           Currently identical to ANDROID_STATISTICS_FACE_IDS.\n>>         size: [n]\n>> +  - WdrMode:\n> \n> Can we avoid these going into ::draft:: at all ?\n> \n>> +      type: int32_t\n>> +      direction: inout\n>> +      description: |\n>> +        Set the WDR mode.\n>> +\n>> +        The WDR mode is used to select the algorithm used for global tone\n>> +        mapping. It will automatically reduce the exposure time of the sensor\n>> +        so that there are only a small number of saturated pixels in the image.\n>> +        The algorithm then compensates for the loss of brightness by applying a\n>> +        global tone mapping curve to the image.\n>> +      enum:\n>> +        - name: WdrOff\n>> +          value: 0\n>> +          description: Wdr is disabled.\n>> +        - name: WdrLinear\n>> +          value: 1\n>> +          description:\n>> +            Apply a linear global tone mapping curve.\n> \n> I'm torn here on naming. Cue the bikeshed door opening ...\n> \n> But we use the control name 'Wdr' and say it applies 'Global Tone\n> Mapping' configurations.\n> \n> Is WDR sufficiently known to represent Global Tone Mapping - are the\n> terms sufficiently interchangable ?\n> \n> \n> Do we have other hardware with 'WDR' capabilities to compare if the\n> controls make sense on other devices already?\n> \n> Dan - I expect the Mali-C55 to be quite HDR capable ... is there a WDR\n> block there that would benefit/utilise the same controls and concepts?\n\nYes, but I'm not at all familiar with the ISP's specific implementation \n(we haven't touched those blocks yet) or even the concept really (I read \nthe discussion about the confusion between HDR and WDR with some relief \nas it confuses me totally). So this is a bit \"best guess\"...but I \n_think_ that what looks to be the equivalent of this in the C55 is the \nIridix tone mapping block, which purports to operate automatically - \ncalculating the curves that would be applied to a frame from the \nprevious frame...it does also allow the user to supply a curve though, \nso my best guess at my current state of knowledge is that yes we could \nuse the same concept and controls, but perhaps with the addition of a \n\"WdrAutomatic\" mode that left decisions up to the hardware instead.\n\n> \n> \n> \n>> +\n>> +            The curve with two linear sections is applied. This produces good\n>> +            results at the expense of a slightly artificial look.\n>> +        - name: WdrPower\n>> +          value: 2\n>> +          description: |\n>> +            Apply a power global tone mapping curve.\n>> +\n>> +            This curve has high gain values on the dark areas of an image and\n>> +            high compression values on the bright area. It therefore tends to\n>> +            produce noticeable noise artifacts.\n>> +        - name: WdrExponential\n>> +          value: 3\n>> +          description: |\n>> +            Apply a exponential global tone mapping curve.\n>> +\n>> +            This curve has lower gain values in dark areas compared to the power\n>> +            curve but produces a more natural look compared to the linear curve.\n>> +            It is therefore the best choice for most scenes.\n>> +        - name: WdrHistogramEqualization\n>> +          value: 4\n>> +          description: |\n>> +            Apply histogram equalization.\n>> +\n>> +            This curve preserves most of the information of the image at the\n>> +            expense of a very artificial look. It is therefore best suited for\n>> +            technical analysis.\n>> +  - WdrStrength:\n>> +      type: float\n>> +      direction: in\n>> +      description: |\n>> +        Specify the strength of the wdr algorithm. The exact meaning of this\n>> +        value is specific to the algorithm in use. Usually a value of 0 means no\n>> +        global tone mapping is applied. A values of 1 is the default value and\n>> +        the correct value for most scenes. A value above 1 increases the global\n>> +        tone mapping effect and can lead to unrealistic image effects.\n>> +  - WdrMaxBrightPixels:\n>> +      type: float\n>> +      direction: in\n>> +      description: |\n>> +        Percentage of allowed (nearly) saturated pixels. The WDR algorithm\n>> +        reduces the WdrExposureValue until the amount of pixels that are close\n>> +        to saturation is lower than this value.\n>> +  - WdrExposureValue:\n>> +      type: float\n>> +      direction: out\n>> +      description: |\n>> +        Reports the Exposure Value that was applied to the AEGC regulation so\n>> +        that the WdrMaxBrightPixels limit is reached.\n> \n> Is that /different/ to the ExposureValue already reported in metadata?\n> \n>> +\n>> +        \\sa ExposureValue\n>>   \n>>   ...\n>> -- \n>> 2.48.1\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 728E1BEFBE\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 14 Aug 2025 13:01:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BFF069247;\n\tThu, 14 Aug 2025 15:01:06 +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 81A8861444\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 14 Aug 2025 15:01:04 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F05CC3D5;\n\tThu, 14 Aug 2025 15:00:09 +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=\"Q1Mh6sRG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755176410;\n\tbh=VS34WPf7pCc0nbH3gCxBlky6s4gfS6Ts/w7YidX9ibE=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Q1Mh6sRG/Dopvfg1eP5Aet55qCkD7VaxjLaZScUQdoosmoIy93NzY4QcVZxWKbjzS\n\tfWwRz4P6vZZzhUg2T7qsEwqhbh704uNhasKGzQMtzzeVs5Ub+9nz4GkV7CMPx0MtCh\n\t7CrqljbjqhPel23FegcDFRbmCobJQGyR52Pcg0Co=","Message-ID":"<20e3adef-b03f-4078-9062-1e86d265c855@ideasonboard.com>","Date":"Thu, 14 Aug 2025 14:01:01 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 15/16] ipa: rkisp1: Add WDR algorithm","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tStefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-16-stefan.klug@ideasonboard.com>\n\t<175493316838.1641235.2414681416886447452@ping.linuxembedded.co.uk>","Content-Language":"en-US","From":"Dan Scally <dan.scally@ideasonboard.com>","In-Reply-To":"<175493316838.1641235.2414681416886447452@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]