From patchwork Wed Sep 9 09:54:31 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Klug X-Patchwork-Id: 28218 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 3E4CCC3257 for ; Wed, 9 Sep 2026 09:57:46 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 3A0C36864A; Wed, 9 Sep 2026 11:57:45 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="jDYmwpaJ"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 39940685E3 for ; Wed, 9 Sep 2026 11:57:43 +0200 (CEST) Received: from ideasonboard.com (unknown [IPv6:2a00:6020:448c:6c00:8156:275a:e2cf:8f67]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id E42E44B0; Wed, 9 Sep 2026 11:56:06 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1788947767; bh=gB+sCwn1aVX8XIouvtao1R7HIaFp+9gAGqtlmROMytw=; h=From:To:Cc:Subject:Date:From; b=jDYmwpaJ/hf5ept9aXJrbM26KO2YzK1hPJDlQ8VHnDVfmsk3bAVCY8HWqed5j7xhb xTh6nQDrnnfWZzOmBnT2diA784vUisfadFHBET+ttQSSKrRgRDk4auv7C1ydfuZhrB v8+zMOJOlcpNAizOjNtaLvXFx5t5rFEdURTOlcC4= From: Stefan Klug To: libcamera-devel@lists.libcamera.org Cc: Stefan Klug , Jacopo Mondi , =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= Subject: [RFC PATCH] ipa: Reformat algorithms in a checkstyle compatible way Date: Wed, 9 Sep 2026 11:54:31 +0200 Message-ID: <20260909095616.256223-2-stefan.klug@ideasonboard.com> X-Mailer: git-send-email 2.53.0 MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Reformat some code sections in a checkstyle compatible way while trying to stay within the 80/100 column limit. Most of the changes are just reformatting. In libipa::AgcAlgorithm the visitor patterns were reformatted using intermediate variables. In rkisp1 Agc::process the code was reordered to spare one indentation level. Signed-off-by: Stefan Klug --- Hi, There have been several discussions around the question if checkstyle is mandatory or not with a tendency towards *not mandatory*. However during my daily work and many rebases it became even more painful to get constant warnings from checkstyle which eventually renders the tool useless. So I had a look at the code sections that popped up recently and tried to reformat them in a checkstyle compatible way, thereby keeping the indentation levels low. The result is this patch. I'd like to get some feedback from the group if these formattings could be a viable option. Best regards, Stefan --- src/ipa/ipu3/algorithms/agc.cpp | 67 ++++++----- src/ipa/libipa/agc.cpp | 174 ++++++++++++++-------------- src/ipa/mali-c55/algorithms/lsc.cpp | 14 ++- src/ipa/rkisp1/algorithms/agc.cpp | 113 ++++++++++-------- src/ipa/rkisp1/algorithms/lsc.cpp | 14 ++- 5 files changed, 203 insertions(+), 179 deletions(-) diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 57cfb325f1fc..323021cb9a4b 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -67,11 +67,13 @@ Agc::Agc() */ int Agc::init(IPAContext &context, const ValueNode &tuningData) { - return agc_.init(tuningData, context.camHelper.get(), { - .sensorInfo = context.sensorInfo, - .sensorControls = context.sensorControls, - .ctrlMap = context.ctrlMap, - }); + return agc_.init( + tuningData, context.camHelper.get(), + { + .sensorInfo = context.sensorInfo, + .sensorControls = context.sensorControls, + .ctrlMap = context.ctrlMap, + }); } /** @@ -84,14 +86,16 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) int Agc::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) { - stride_ = context.configuration.grid.stride; + stride_ = context.configuration.grid.stride; bdsGrid_ = context.configuration.grid.bdsGrid; - return agc_.configure(context.configuration.agc, context.activeState.agc, { - .sensorInfo = context.sensorInfo, - .sensorControls = context.sensorControls, - .ctrlMap = context.ctrlMap, - }); + return agc_.configure( + context.configuration.agc, context.activeState.agc, + { + .sensorInfo = context.sensorInfo, + .sensorControls = context.sensorControls, + .ctrlMap = context.ctrlMap, + }); } /** @@ -129,11 +133,10 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats, reinterpret_cast( &stats->awb_raw_buffer.meta_data[cellPosition]); - rgbTriples_.push_back({ - cell->R_avg, - (cell->Gr_avg + cell->Gb_avg) / 2, - cell->B_avg - }); + rgbTriples_.push_back( + { cell->R_avg, + (cell->Gr_avg + cell->Gb_avg) / 2, + cell->B_avg }); /* * Store the average green value to estimate the @@ -219,20 +222,24 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, { Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid); - agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{ - .traits = AgcTraits{ - rgbTriples_, - {{ - context.activeState.awb.gains.red, - context.activeState.awb.gains.blue, - context.activeState.awb.gains.green, - }}, - bdsGrid_, - }, - .yHist = hist, - .exposure = frameContext.sensor.exposure, - .gain = frameContext.sensor.gain, - }}, metadata); + agc_.process( + context.configuration.agc, context.activeState.agc, + frameContext.agc, + { { + .traits = AgcTraits{ + rgbTriples_, + { { + context.activeState.awb.gains.red, + context.activeState.awb.gains.blue, + context.activeState.awb.gains.green, + } }, + bdsGrid_, + }, + .yHist = hist, + .exposure = frameContext.sensor.exposure, + .gain = frameContext.sensor.gain, + } }, + metadata); } REGISTER_IPA_ALGORITHM(Agc, "Agc") diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp index 51b05e3c2acd..e1ded0f87097 100644 --- a/src/ipa/libipa/agc.cpp +++ b/src/ipa/libipa/agc.cpp @@ -485,51 +485,50 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, add(controls::AnalogueGainMode, controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual); - std::visit(utils::overloaded{ - [&](AgcMSV &) { - /* No constraint/exposure mode support. */ - state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal; - state.exposureMode = controls::AeExposureModeEnum::ExposureNormal; + auto configureMSV = [&](AgcMSV &) { + /* No constraint/exposure mode support. */ + state.constraintMode = controls::AeConstraintModeEnum::ConstraintNormal; + state.exposureMode = controls::AeExposureModeEnum::ExposureNormal; - state.automatic.yTarget = 0; /* Not supported. */ + state.automatic.yTarget = 0; /* Not supported. */ - if (!session.autoAllowed) - return; + if (!session.autoAllowed) + return; - config.ctrlMap[&controls::AeConstraintMode] = ControlInfo( - std::array{ ControlValue(state.constraintMode) } - ); + config.ctrlMap[&controls::AeConstraintMode] = ControlInfo( + std::array{ ControlValue(state.constraintMode) }); - config.ctrlMap[&controls::AeExposureMode] = ControlInfo( - std::array{ ControlValue(state.exposureMode) } - ); - }, - [&](AgcMeanLuminance &impl) { - state.constraintMode = - static_cast(impl.constraintModes().begin()->first); - state.exposureMode = - static_cast(impl.exposureModeHelpers().begin()->first); + config.ctrlMap[&controls::AeExposureMode] = ControlInfo( + std::array{ ControlValue(state.exposureMode) }); + }; - state.automatic.yTarget = impl.effectiveYTarget(0, 1); + auto configureMeanLuminance = [&](AgcMeanLuminance &impl) { + state.constraintMode = + static_cast(impl.constraintModes().begin()->first); + state.exposureMode = + static_cast(impl.exposureModeHelpers().begin()->first); - impl.configure(session.lineDuration, sensor_); + state.automatic.yTarget = impl.effectiveYTarget(0, 1); - if (!session.autoAllowed) - return; + impl.configure(session.lineDuration, sensor_); - config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); + if (!session.autoAllowed) + return; - std::vector options; - for (const auto &[id, _] : impl.constraintModes()) - options.emplace_back(id); - config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options); + config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); - options.clear(); - for (const auto &[id, _] : impl.exposureModeHelpers()) - options.emplace_back(id); - config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options); - }, - }, impl_); + std::vector options; + for (const auto &[id, _] : impl.constraintModes()) + options.emplace_back(id); + config.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options); + + options.clear(); + for (const auto &[id, _] : impl.exposureModeHelpers()) + options.emplace_back(id); + config.ctrlMap[&controls::AeExposureMode] = ControlInfo(options); + }; + + std::visit(utils::overloaded{ configureMSV, configureMeanLuminance }, impl_); return 0; } @@ -762,63 +761,64 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state, maxAnalogueGain = frameContext.gain; } - std::visit(utils::overloaded{ - [&](AgcMSV& impl) { - impl.setLimits({ - .exposure = { - static_cast(minExposureTime / lineDuration), - static_cast(maxExposureTime / lineDuration), - }, - .gain = { - minAnalogueGain, - maxAnalogueGain, - }, - /* gain codes -> step size of 1 */ - .gainMinStep = 1, - /* assume default gain is close to 1.0 */ - .gain1 = session.defAnalogueGain, - }); + auto processMSV = [&](AgcMSV &impl) { + impl.setLimits({ + .exposure = { + static_cast(minExposureTime / lineDuration), + static_cast(maxExposureTime / lineDuration), + }, + .gain = { + minAnalogueGain, + maxAnalogueGain, + }, + /* gain codes -> step size of 1 */ + .gainMinStep = 1, + /* assume default gain is close to 1.0 */ + .gain1 = session.defAnalogueGain, + }); - const auto& newEv = impl.calculateNewEv({ - .yHist = params->yHist, - .exposure = params->exposure, - .gain = params->gain, - }); + const auto &newEv = impl.calculateNewEv({ + .yHist = params->yHist, + .exposure = params->exposure, + .gain = params->gain, + }); - state.automatic.exposure = newEv.exposure; - state.automatic.gain = newEv.analogueGain; - }, - [&](AgcMeanLuminance& impl) { - /* - * The Agc algorithm needs to know the effective exposure - * value that was applied to the sensor when the statistics - * were collected. - */ - utils::Duration effectiveExposureValue = - lineDuration * params->exposure * params->gain; + state.automatic.exposure = newEv.exposure; + state.automatic.gain = newEv.analogueGain; + }; - impl.setLimits(minExposureTime, maxExposureTime, - minAnalogueGain, maxAnalogueGain, - std::move(params->additionalConstraints)); + auto processMeanLuminance = [&](AgcMeanLuminance &impl) { + /* + * The Agc algorithm needs to know the effective exposure value + * that was applied to the sensor when the statistics were + * collected. + */ + utils::Duration effectiveExposureValue = + lineDuration * params->exposure * params->gain; - const auto &newEv = impl.calculateNewEv({ - .traits = params->traits, - .yHist = params->yHist, - .effectiveExposureValue = effectiveExposureValue, - .constraintModeIndex = frameContext.constraintMode, - .exposureModeIndex = frameContext.exposureMode, - .lux = params->lux, - .exposureCompensation = std::pow(2.0, frameContext.exposureValue), - }); + impl.setLimits(minExposureTime, maxExposureTime, + minAnalogueGain, maxAnalogueGain, + std::move(params->additionalConstraints)); - /* Update the estimated exposure and gain. */ - state.automatic.exposure = newEv.exposureTime / lineDuration; - state.automatic.gain = newEv.analogueGain; - state.automatic.quantizationGain = newEv.quantizationGain; - state.automatic.digitalGain = newEv.digitalGain; - state.automatic.yTarget = newEv.yTarget; - }, - }, impl_); + const auto &newEv = impl.calculateNewEv({ + .traits = params->traits, + .yHist = params->yHist, + .effectiveExposureValue = effectiveExposureValue, + .constraintModeIndex = frameContext.constraintMode, + .exposureModeIndex = frameContext.exposureMode, + .lux = params->lux, + .exposureCompensation = std::pow(2.0, frameContext.exposureValue), + }); + + /* Update the estimated exposure and gain. */ + state.automatic.exposure = newEv.exposureTime / lineDuration; + state.automatic.gain = newEv.analogueGain; + state.automatic.quantizationGain = newEv.quantizationGain; + state.automatic.digitalGain = newEv.digitalGain; + state.automatic.yTarget = newEv.yTarget; + }; + + std::visit(utils::overloaded{ processMSV, processMeanLuminance }, impl_); const utils::Duration newExposureTime = state.automatic.exposure * lineDuration; diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp index fff0dc7d0e64..38c552e712b8 100644 --- a/src/ipa/mali-c55/algorithms/lsc.cpp +++ b/src/ipa/mali-c55/algorithms/lsc.cpp @@ -53,12 +53,14 @@ int Lsc::init(IPAContext &context, const ValueNode &tuningData) { gridPos_ = segmentsToPosition(); - return lscAlgo_.init(tuningData, context.ctrlMap, { - .keys = { "r", "g", "b" }, - .numHSamples = kMeshSize, - .numVSamples = kMeshSize, - .sensorSize = context.sensorInfo.activeAreaSize - }); + return lscAlgo_.init( + tuningData, context.ctrlMap, + { + .keys = { "r", "g", "b" }, + .numHSamples = kMeshSize, + .numVSamples = kMeshSize, + .sensorSize = context.sensorInfo.activeAreaSize, + }); } int Lsc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 1f1d6c96a3c6..a4a79cc792a7 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -162,12 +162,14 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) */ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) { - int ret = agc_.configure(context.configuration.agc, context.activeState.agc, { - .sensorInfo = context.sensorInfo, - .sensorControls = context.sensorControls, - .ctrlMap = context.ctrlMap, - .autoAllowed = !context.configuration.raw, - }); + int ret = agc_.configure( + context.configuration.agc, context.activeState.agc, + { + .sensorInfo = context.sensorInfo, + .sensorControls = context.sensorControls, + .ctrlMap = context.ctrlMap, + .autoAllowed = !context.configuration.raw, + }); if (ret) return ret; @@ -266,9 +268,9 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, struct rkisp1_cif_isp_window window = hstConfig->meas_window; Size windowSize = { window.h_size, window.v_size }; - hstConfig->histogram_predivider = - computeHistogramPredivider(windowSize, - static_cast(hstConfig->mode)); + hstConfig->histogram_predivider = computeHistogramPredivider( + windowSize, + static_cast(hstConfig->mode)); } namespace { @@ -285,19 +287,22 @@ public: * \brief Estimate the relative luminance of the frame with a given gain * \param[in] gain The gain to apply to the frame * - * This function estimates the average relative luminance of the frame that - * would be output by the sensor if an additional \a gain was applied. + * This function estimates the average relative luminance of the frame + * that would be output by the sensor if an additional \a gain was + * applied. * * The estimation is based on the AE statistics for the current frame. Y - * averages for all cells are first multiplied by the gain, and then saturated - * to approximate the sensor behaviour at high brightness values. The - * approximation is quite rough, as it doesn't take into account non-linearities - * when approaching saturation. In this case, saturating after the conversion to - * YUV doesn't take into account the fact that the R, G and B components - * contribute differently to the relative luminance. + * averages for all cells are first multiplied by the gain, and then + * saturated to approximate the sensor behaviour at high brightness + * values. The approximation is quite rough, as it doesn't take into + * account non-linearities when approaching saturation. In this case, + * saturating after the conversion to YUV doesn't take into account the + * fact that the R, G and B components contribute differently to the + * relative luminance. * - * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a - * theoretical perfect reflector of 100% reference white. + * The values are normalized to the [0.0, 1.0] range, where 1.0 + * corresponds to a theoretical perfect reflector of 100% reference + * white. * * More detailed information can be found in: * https://en.wikipedia.org/wiki/Relative_luminance @@ -358,40 +363,48 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) params = &stats->params; else - LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics"; - } - - if (params) { - std::vector additionalConstraints; - if (context.activeState.wdr.mode != controls::WdrOff) - additionalConstraints.push_back(context.activeState.wdr.constraint); - - agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{ - .traits = AgcTraits{ - { params->ae.exp_mean, context.hw.numAeCells }, - meteringModes_.at(frameContext.agc.meteringMode), - }, - .yHist = { - /* The lower 4 bits are fractional and meant to be discarded. */ - { params->hist.hist_bins, context.hw.numHistogramBins }, - [](uint32_t x) { return x >> 4; }, - }, - .exposure = frameContext.sensor.exposure, - /* - * Include the quantization gain if it was applied. Do not use - * compress.gain because it will include gains that shall not be - * reported to the user when HDR is implemented. - */ - .gain = frameContext.sensor.gain - * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1), - .additionalConstraints = std::move(additionalConstraints), - .lux = frameContext.lux.lux, - }}, metadata); - } else { - agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata); + LOG(RkISP1Agc, Error) + << "AUTOEXP data is missing in statistics"; } metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode); + + if (!params) { + agc_.process(context.configuration.agc, context.activeState.agc, + frameContext.agc, {}, metadata); + return; + } + + std::vector additionalConstraints; + if (context.activeState.wdr.mode != controls::WdrOff) + additionalConstraints.push_back(context.activeState.wdr.constraint); + + double qgain = (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1); + std::optional processParams{ { + .traits = AgcTraits{ + { params->ae.exp_mean, context.hw.numAeCells }, + meteringModes_.at(frameContext.agc.meteringMode), + }, + .yHist = { + /* + * The lower 4 bits are fractional and meant to be + * discarded. + */ + { params->hist.hist_bins, context.hw.numHistogramBins }, + [](uint32_t x) { return x >> 4; }, + }, + .exposure = frameContext.sensor.exposure, + /* + * Include the quantization gain if it was applied. Do not use + * compress.gain because it will include gains that shall not be + * reported to the user when HDR is implemented. + */ + .gain = frameContext.sensor.gain * qgain, + .additionalConstraints = std::move(additionalConstraints), + .lux = frameContext.lux.lux, + } }; + agc_.process(context.configuration.agc, context.activeState.agc, + frameContext.agc, std::move(processParams), metadata); } REGISTER_IPA_ALGORITHM(Agc, "Agc") diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp index 36f9a344ed3b..230fb70f4aa8 100644 --- a/src/ipa/rkisp1/algorithms/lsc.cpp +++ b/src/ipa/rkisp1/algorithms/lsc.cpp @@ -124,12 +124,14 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context, xPos_ = sizesListToPositions(xSize_); yPos_ = sizesListToPositions(ySize_); - return lscAlgo_.init(tuningData, context.ctrlMap, { - .keys = { "r", "gr", "gb", "b" }, - .numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX, - .numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX, - .sensorSize = context.sensorInfo.activeAreaSize - }); + return lscAlgo_.init( + tuningData, context.ctrlMap, + { + .keys = { "r", "gr", "gb", "b" }, + .numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX, + .numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX, + .sensorSize = context.sensorInfo.activeAreaSize, + }); } /**