[{"id":39842,"web_url":"https://patchwork.libcamera.org/comment/39842/","msgid":"","date":"2026-07-24T15:01:29","subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:\n> Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`\n> based on the rkisp1 `Agc` algorithm, with slight adjustments.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/ipa/libipa/agc.cpp | 639 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/agc.h | 98 ++++++\n> src/ipa/libipa/meson.build | 1 +\n> 3 files changed, 738 insertions(+)\n> create mode 100644 src/ipa/libipa/agc.cpp\n>\n> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n> new file mode 100644\n> index 0000000000..e16a02fdde\n> --- /dev/null\n> +++ b/src/ipa/libipa/agc.cpp\n> @@ -0,0 +1,639 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, 2026 Ideas On Board\n> + *\n> + * Generic AGC algorithm\n> + */\n> +\n> +#include \"agc.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +using namespace std::chrono_literals;\n> +\n> +LOG_DEFINE_CATEGORY(Agc)\n> +\n> +/**\n> + * \\class AgcAlgorithm\n> + * \\brief AgcMeanLuminance wrapper for implementing the Algorithm interface\n> + *\n> + * \\todo DigitalGain, DigitalGainMode\n> + */\n> +\n> +/**\n> + * \\struct agc::Session\n> + * \\brief Session configuration for AgcAlgorithm\n> + *\n> + * \\var agc::Session::minExposureTime\n> + * \\brief Minimum exposure time supported with the configured sensor\n> + *\n> + * \\var agc::Session::maxExposureTime\n> + * \\brief Maximum exposure time supported with the configured sensor\n> + *\n> + * \\var agc::Session::minAnalogueGain\n> + * \\brief Minimum analogue gain supported with the configured sensor\n> + *\n> + * \\var agc::Session::maxAnalogueGain\n> + * \\brief Maximum analogue gain supported with the configured sensor\n> + *\n> + * \\var agc::Session::minFrameDuration\n> + * \\brief Minimum frame duration supported with the configured sensor\n> + *\n> + * \\var agc::Session::maxFrameDuration\n> + * \\brief Maximum frame duration supported with the configured sensor\n\nI would use \"for the streaming session\" in place of \"with the\nconfigured sensor\" in all previous entries.\n\nyes, those settings depend on the sensor configuration but I found the\nwording a bit confusing\n\nOr maybe \"for the current sensor configuration\" ?\n\n> + *\n> + * \\var agc::Session::lineDuration\n> + * \\brief Line duration with the configured sensor and output size\n> + *\n> + * \\var agc::Session::sensor\n> + * \\brief Details of the sensor configuration\n> + *\n> + * \\var agc::Session::sensor.outputSize\n> + * \\brief Configured output size of the sensor\n> + *\n> + * \\var agc::Session::autoAllowed\n> + * \\brief Whether automatic controls are allowed\n\nAs I was really confused by this, is it worth adding one line saying\nthat auto is not allowed when capturing raw frames ?\n\n> + */\n> +\n> +/**\n> + * \\struct agc::ActiveState\n> + * \\brief Active state for AgcAlgorithm\n> + *\n> + * The \\a automatic variables track the latest values computed by algorithm\n> + * based on the latest processed statistics. All other variables track the\n> + * consolidated controls requested in queued requests.\n> + *\n> + * \\var agc::ActiveState::manual\n> + * \\brief Manual exposure time and analog gain (set through requests)\n> + *\n> + * \\var agc::ActiveState::manual.exposure\n> + * \\brief Manual exposure time expressed as a number of lines as set by the\n> + * ExposureTime control\n> + *\n> + * \\var agc::ActiveState::manual.gain\n> + * \\brief Manual analogue gain as set by the AnalogueGain control\n> + *\n> + * \\var agc::ActiveState::automatic\n> + * \\brief Automatic exposure time and analog gain (computed by the algorithm)\n> + *\n> + * \\var agc::ActiveState::automatic.exposure\n> + * \\brief Automatic exposure time expressed as a number of lines\n> + *\n> + * \\var agc::ActiveState::automatic.gain\n> + * \\brief Automatic analogue gain multiplier\n> + *\n> + * \\var agc::ActiveState::automatic.quantizationGain\n> + * \\brief Automatic quantization gain multiplier\n> + *\n> + * \\var agc::ActiveState::automatic.yTarget\n> + * \\brief Automatically determined luminance target\n> + *\n> + * \\var agc::ActiveState::autoExposureEnabled\n> + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n\ns/AGC state (exposure)/exposure mode\n\n> + *\n> + * \\var agc::ActiveState::autoGainEnabled\n> + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n\nsame here ?\n\n> + *\n> + * \\var agc::ActiveState::exposureValue\n> + * \\brief Exposure value as set by the ExposureValue control\n> + *\n> + * \\var agc::ActiveState::constraintMode\n> + * \\brief Constraint mode as set by the AeConstraintMode control\n> + *\n> + * \\var agc::ActiveState::exposureMode\n> + * \\brief Exposure mode as set by the AeExposureMode control\n> + *\n> + * \\var agc::ActiveState::minFrameDuration\n> + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> + *\n> + * \\var agc::ActiveState::maxFrameDuration\n> + * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> + */\n> +\n> +/**\n> + * \\struct agc::FrameContext\n> + * \\brief Per-frame context for AgcAlgorithm\n> + *\n> + * \\var agc::FrameContext::exposure\n> + * \\brief Exposure time expressed as a number of lines computed by the algorithm\n> + *\n> + * \\var agc::FrameContext::gain\n> + * \\brief Analogue gain multiplier computed by the algorithm\n\nAre these computed by the algorithms or can these be the manually\nprogrammed values when running in manual mode ?\n\nI would replace \"computed by the algorithm\" with \"for the frame\" or\nsimilar\n\n> + *\n> + * The gain should be adapted to the sensor specific gain code before applying.\n\ns/adapted/translated ?\n\n> + *\n> + * \\var agc::FrameContext::quantizationGain\n> + * \\brief Quantization gain multiplier computed by the algorithm\n> + *\n> + * \\var agc::FrameContext::exposureValue\n> + * \\brief Exposure value as set by the ExposureValue control\n> + *\n> + * \\var agc::FrameContext::yTarget\n> + * \\brief Luminance target computed by the algorithm\n> + *\n> + * \\var agc::FrameContext::vblank\n> + * \\brief Vertical blanking parameter computed by the algorithm\n> + *\n> + * \\var agc::FrameContext::autoExposureEnabled\n> + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n> + *\n> + * \\var agc::FrameContext::autoGainEnabled\n> + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n> + *\n> + * \\var agc::FrameContext::constraintMode\n> + * \\brief Constraint mode as set by the AeConstraintMode control\n> + *\n> + * \\var agc::FrameContext::exposureMode\n> + * \\brief Exposure mode as set by the AeExposureMode control\n> + *\n> + * \\var agc::FrameContext::minFrameDuration\n> + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> + *\n> + * \\var agc::FrameContext::maxFrameDuration\n> + * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> + *\n> + * \\var agc::FrameContext::frameDuration\n> + * \\brief The actual FrameDuration used by the algorithm for the frame\n> + *\n> + * \\var agc::FrameContext::autoExposureModeChange\n> + * \\brief Indicate if autoExposureEnabled has changed from true in the previous\n> + * frame to false in the current frame, and no manual exposure value has been\n> + * supplied in the current frame.\n\ndoes this only track auto->manual transaction or does it track the\nother way around.\n\nKind of a long brief (and no '.' at the end).\n\nI would\n\n \\brief Exposure mode change flag\n\n Indicate if the exposure mode has changed compared to the\n previous frame.\n\n> + *\n> + * \\var agc::FrameContext::autoGainModeChange\n> + * \\brief Indicate if autoGainEnabled has changed from true in the previous\n> + * frame to false in the current frame, and no manual gain value has been\n> + * supplied in the current frame.\n> + */\n> +\n> +/**\n> + * \\struct AgcAlgorithm::ConfigurationParams\n> + * \\brief Parameters for AgcAlgorithm::configure()\n> + *\n> + * \\var AgcAlgorithm::ConfigurationParams::sensor\n> + * \\brief CameraSensorHelper for the sensor\n> + *\n> + * \\var AgcAlgorithm::ConfigurationParams::sensorInfo\n> + * \\brief Details of the sensor\n\nThe sensor configuration description\n\n> + *\n> + * \\var AgcAlgorithm::ConfigurationParams::sensorControls\n> + * \\brief ControlInfoMap of the sensor\n> + *\n> + * \\var AgcAlgorithm::ConfigurationParams::ctrlMap\n> + * \\brief ControlMap to update with controls\n> + *\n> + * \\var AgcAlgorithm::ConfigurationParams::autoAllowed\n> + * \\brief Whether to enable auto controls\n> + */\n> +\n> +/**\n> + * \\struct AgcAlgorithm::ProcessParams\n> + * \\brief Parameters for AgcAlgorithm::process()\n> + *\n> + * \\var AgcAlgorithm::ProcessParams::traits\n> + * \\brief Implementation of AgcMeanLuminance::Traits\n> + *\n> + * \\var AgcAlgorithm::ProcessParams::yHist\n> + * \\brief Luminance histogram of the frame\n> + *\n> + * \\var AgcAlgorithm::ProcessParams::exposure\n> + * \\brief Effective exposure of the frame\n> + *\n> + * \\var AgcAlgorithm::ProcessParams::gain\n> + * \\brief Effective gain of the frame\n> + *\n> + * \\var AgcAlgorithm::ProcessParams::additionalConstraints\n> + * \\brief Additional AgcMeanLuminance::AgcConstraints to apply\n> + *\n> + * \\var AgcAlgorithm::ProcessParams::lux\n> + * \\brief Effective lux value of the frame\n> + */\n> +\n> +/**\n> + * \\brief Load tuning data\n> + */\n> +int AgcAlgorithm::init(const ValueNode &tuningData)\n> +{\n> +\tint ret = impl_.parseTuningData(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Initialize the session configuration and active state\n> + */\n> +int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config)\n\nEasy to shorten to\n\nint AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,\n\t\t\t const ConfigurationParams &config)\n\n\nSeems like session is an out parameter.\n\nWe used to have a rule: const & for input params, * for output ones.\nNot sure anymore how much we enforce it these days..\n\n> +{\n> +\tsession = {};\n> +\tsession.lineDuration = config.sensorInfo.minLineLength * 1.0s\n> +\t\t/ config.sensorInfo.pixelRate;\n> +\tsession.sensor.outputSize = config.sensorInfo.outputSize;\n> +\tsession.autoAllowed = config.autoAllowed;\n> +\n> +\tconst double lineDurationUs = session.lineDuration.get();\n> +\n> +\t/*\n> +\t * Compute exposure time limits from the V4L2_CID_EXPOSURE control\n> +\t * limits and the line duration.\n> +\t */\n> +\n> +\tconst ControlInfo &v4l2Exposure = config.sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> +\tint32_t minExposure = v4l2Exposure.min().get();\n> +\tint32_t maxExposure = v4l2Exposure.max().get();\n> +\tint32_t defExposure = v4l2Exposure.def().get();\n> +\tconfig.ctrlMap[&controls::ExposureTime] = ControlInfo{\n> +\t\tstatic_cast(minExposure * lineDurationUs),\n> +\t\tstatic_cast(maxExposure * lineDurationUs),\n> +\t\tstatic_cast(defExposure * lineDurationUs),\n> +\t};\n> +\n> +\t/* Compute the analogue gain limits. */\n> +\tconst ControlInfo &v4l2Gain = config.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> +\tfloat minGain = config.sensor->gain(v4l2Gain.min().get());\n> +\tfloat maxGain = config.sensor->gain(v4l2Gain.max().get());\n> +\tfloat defGain = config.sensor->gain(v4l2Gain.def().get());\n> +\tconfig.ctrlMap[&controls::AnalogueGain] = ControlInfo{\n> +\t\tminGain,\n> +\t\tmaxGain,\n> +\t\tdefGain,\n> +\t};\n> +\n> +\tLOG(Agc, Debug)\n> +\t\t<< \"Exposure: [\" << minExposure << \", \" << maxExposure\n> +\t\t<< \"], gain: [\" << minGain << \", \" << maxGain << \"]\";\n> +\n> +\t/*\n> +\t * Compute the frame duration limits.\n> +\t *\n> +\t * The frame length is computed assuming a fixed line length combined\n> +\t * with the vertical frame sizes.\n> +\t */\n> +\tconst ControlInfo &v4l2HBlank = config.sensorControls.find(V4L2_CID_HBLANK)->second;\n> +\tuint32_t hblank = v4l2HBlank.def().get();\n> +\tuint32_t lineLength = config.sensorInfo.outputSize.width + hblank;\n> +\n> +\tconst ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;\n> +\tstd::array frameHeights{\n> +\t\tv4l2VBlank.min().get() + config.sensorInfo.outputSize.height,\n> +\t\tv4l2VBlank.max().get() + config.sensorInfo.outputSize.height,\n> +\t\tv4l2VBlank.def().get() + config.sensorInfo.outputSize.height,\n> +\t};\n> +\n> +\tstd::array frameDurations;\n> +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> +\t\tframeDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U);\n> +\t}\n> +\n> +\tconfig.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{\n> +\t\tframeDurations[0],\n> +\t\tframeDurations[1],\n> +\t\tSpan{ { frameDurations[2], frameDurations[2] } },\n> +\t};\n> +\n> +\tsession.minFrameDuration = std::chrono::microseconds(frameDurations[0]);\n> +\tsession.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);\n> +\n> +\t/*\n> +\t * When the AGC computes the new exposure values for a frame, it needs\n> +\t * to know the limits for exposure time and analogue gain. As it depends\n> +\t * on the sensor, update it with the controls.\n> +\t *\n> +\t * \\todo take VBLANK into account for maximum exposure time\n> +\t */\n> +\tsession.minExposureTime = minExposure * session.lineDuration;\n> +\tsession.maxExposureTime = maxExposure * session.lineDuration;\n> +\tsession.minAnalogueGain = minGain;\n> +\tsession.maxAnalogueGain = maxGain;\n\nThe above matches the current RkISP1Agc::reconfigure() content\n\n> +\n> +\timpl_.configure(session.lineDuration, config.sensor);\n> +\timpl_.setLimits(session.minExposureTime, session.maxExposureTime,\n> +\t\t\tsession.minAnalogueGain, session.maxAnalogueGain,\n> +\t\t\t{});\n> +\timpl_.resetFrameCount();\n> +\n> +\t/* Configure the default exposure and gain. */\n> +\tstate = {};\n> +\tstate.automatic.gain = session.minAnalogueGain;\n> +\tstate.automatic.exposure = 10ms / session.lineDuration;\n> +\tstate.automatic.quantizationGain = 1;\n> +\tstate.automatic.yTarget = impl_.effectiveYTarget(0, 1);\n\nAnd this matches RkISP1::Agc() apart from this early yTarget\ncomputation which I presume is intentional\n\n> +\tstate.manual.gain = state.automatic.gain;\n> +\tstate.manual.exposure = state.automatic.exposure;\n> +\tstate.autoExposureEnabled = session.autoAllowed;\n> +\tstate.autoGainEnabled = session.autoAllowed;\n> +\tstate.exposureValue = 0;\n> +\n> +\tstate.constraintMode =\n> +\t\tstatic_cast(impl_.constraintModes().begin()->first);\n> +\tstate.exposureMode =\n> +\t\tstatic_cast(impl_.exposureModeHelpers().begin()->first);\n> +\n> +\tstate.minFrameDuration = session.minFrameDuration;\n> +\tstate.maxFrameDuration = session.maxFrameDuration;\n> +\n> +\tconst auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) {\n> +\t\tstd::array values;\n> +\t\tsize_t count = 0;\n> +\n> +\t\tif (session.autoAllowed)\n> +\t\t\tvalues[count++] = ControlValue(automatic);\n> +\n> +\t\tvalues[count++] = ControlValue(manual);\n> +\n> +\t\tconfig.ctrlMap[&cid] = ControlInfo{\n> +\t\t\t{ values.data(), count },\n> +\t\t\tControlValue(session.autoAllowed ? automatic : manual),\n> +\t\t};\n> +\t};\n> +\n> +\tadd(controls::ExposureTimeMode, controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);\n> +\tadd(controls::AnalogueGainMode, controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);\n\nCould you please break trivially re-adjustable long lines ?\n\n> +\n> +\t/* \\todo Move this to the `Camera` class. */\n> +\tconfig.ctrlMap[&controls::AeEnable] = ControlInfo{\n> +\t\tfalse,\n> +\t\tsession.autoAllowed,\n> +\t\tsession.autoAllowed,\n> +\t};\n\nRkISP1Agc register controls in init() but I think what you're doing\nhere it's better, as some control such as ExposureTimeMode depend on\nthe session configuration.\n\nIs there any risk of having leftovers from a previous session in the\ncontrol list ?\n\n> +\n> +\tif (session.autoAllowed) {\n> +\t\tconfig.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);\n> +\n> +\t\tfor (const auto &[id, info] : impl_.controls())\n> +\t\t\tconfig.ctrlMap[id] = info;\n> +\t} else {\n> +\t\tconfig.ctrlMap.erase(&controls::ExposureValue);\n> +\n> +\t\tfor (const auto &[id, info] : impl_.controls())\n> +\t\t\tconfig.ctrlMap.erase(id);\n> +\t}\n\nMaybe it happens later on, but this is a bit cumbersome. Making\nAgcMeanLuminance aware of autoAllowed would avoid this\n\n> +\n> +\n\nDouble empty line\n\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Handle a \\a queueRequest operation\n> + */\n> +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,\n> +\t\t\t\tagc::FrameContext &frameContext, const ControlList &controls)\n> +{\n> +\tif (session.autoAllowed) {\n\nNow that you register controls at configure() time and not init()\ntime, am I correct that if !autoAllowed applications won't be able to select\ncontrols::ExposureTimeModeAuto ?\n\nWhich makes me wonder if controls::ExposureTimeMode should be\nregistered at all if !autoAllowed\n\n> +\t\tconst auto &aeEnable = controls.get(controls::ExposureTimeMode);\n> +\t\tif (aeEnable &&\n> +\t\t (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {\n> +\t\t\tstate.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);\n> +\n> +\t\t\tLOG(Agc, Debug)\n> +\t\t\t\t<< (state.autoExposureEnabled ? \"Enabling\" : \"Disabling\")\n> +\t\t\t\t<< \" AGC (exposure)\";\n> +\n> +\t\t\t/*\n> +\t\t\t * If we go from auto -> manual with no manual control\n> +\t\t\t * set, use the last computed value, which we don't\n> +\t\t\t * know until prepare() so save this information.\n> +\t\t\t *\n> +\t\t\t * \\todo Check the previous frame at prepare() time\n> +\t\t\t * instead of saving a flag here\n> +\t\t\t */\n> +\t\t\tif (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))\n> +\t\t\t\tframeContext.autoExposureModeChange = true;\n> +\t\t}\n> +\n> +\t\tconst auto &agEnable = controls.get(controls::AnalogueGainMode);\n\nSame reasoning for AnalogueGainMode\n\n> +\t\tif (agEnable &&\n> +\t\t (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {\n> +\t\t\tstate.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);\n> +\n> +\t\t\tLOG(Agc, Debug)\n> +\t\t\t\t<< (state.autoGainEnabled ? \"Enabling\" : \"Disabling\")\n> +\t\t\t\t<< \" AGC (gain)\";\n> +\t\t\t/*\n> +\t\t\t * If we go from auto -> manual with no manual control\n> +\t\t\t * set, use the last computed value, which we don't\n> +\t\t\t * know until prepare() so save this information.\n> +\t\t\t */\n> +\t\t\tif (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))\n> +\t\t\t\tframeContext.autoGainModeChange = true;\n> +\t\t}\n> +\t}\n> +\n> +\tconst auto &exposure = controls.get(controls::ExposureTime);\n> +\tif (exposure && !state.autoExposureEnabled) {\n> +\t\tstate.manual.exposure = *exposure * 1.0us / session.lineDuration;\n> +\n> +\t\tLOG(Agc, Debug)\n> +\t\t\t<< \"Set exposure to \" << state.manual.exposure;\n> +\t}\n> +\n> +\tconst auto &gain = controls.get(controls::AnalogueGain);\n> +\tif (gain && !state.autoGainEnabled) {\n> +\t\tstate.manual.gain = *gain;\n> +\n> +\t\tLOG(Agc, Debug) << \"Set gain to \" << state.manual.gain;\n> +\t}\n> +\n> +\tframeContext.autoExposureEnabled = state.autoExposureEnabled;\n> +\tframeContext.autoGainEnabled = state.autoGainEnabled;\n> +\n> +\tif (!frameContext.autoExposureEnabled)\n> +\t\tframeContext.exposure = state.manual.exposure;\n> +\tif (!frameContext.autoGainEnabled)\n> +\t\tframeContext.gain = state.manual.gain;\n> +\n> +\tif (!frameContext.autoExposureEnabled &&\n> +\t !frameContext.autoGainEnabled)\n> +\t\tframeContext.quantizationGain = 1.0;\n> +\n> +\tconst auto &exposureMode = controls.get(controls::AeExposureMode);\n> +\tif (exposureMode)\n> +\t\tstate.exposureMode =\n> +\t\t\tstatic_cast(*exposureMode);\n> +\tframeContext.exposureMode = state.exposureMode;\n> +\n> +\tconst auto &constraintMode = controls.get(controls::AeConstraintMode);\n> +\tif (constraintMode)\n> +\t\tstate.constraintMode =\n> +\t\t\tstatic_cast(*constraintMode);\n> +\tframeContext.constraintMode = state.constraintMode;\n> +\n> +\tconst auto &exposureValue = controls.get(controls::ExposureValue);\n> +\tif (exposureValue)\n> +\t\tstate.exposureValue = *exposureValue;\n> +\tframeContext.exposureValue = state.exposureValue;\n> +\n> +\tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> +\tif (frameDurationLimits) {\n> +\t\t/* Limit the control value to the limits in ControlInfo */\n> +\t\tstate.minFrameDuration = std::clamp(\n> +\t\t\tstd::chrono::microseconds((*frameDurationLimits).front()),\n> +\t\t\tsession.minFrameDuration,\n> +\t\t\tsession.maxFrameDuration\n> +\t\t);\n> +\n> +\t\tstate.maxFrameDuration = std::clamp(\n> +\t\t\tstd::chrono::microseconds((*frameDurationLimits).back()),\n> +\t\t\tsession.minFrameDuration,\n> +\t\t\tsession.maxFrameDuration\n> +\t\t);\n> +\t}\n> +\tframeContext.minFrameDuration = state.minFrameDuration;\n> +\tframeContext.maxFrameDuration = state.maxFrameDuration;\n> +}\n> +\n\nThis matches RkISP1Agc::queueRequest() (metering mode handling apart)\n\n> +/**\n> + * \\brief Handle a \\a prepare operation\n> + */\n> +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)\n> +{\n> +\tuint32_t activeAutoExposure = state.automatic.exposure;\n> +\tdouble activeAutoGain = state.automatic.gain;\n> +\tdouble activeAutoQGain = state.automatic.quantizationGain;\n> +\n> +\t/* Populate exposure and gain in auto mode */\n> +\tif (frameContext.autoExposureEnabled) {\n> +\t\tframeContext.exposure = activeAutoExposure;\n> +\t\tframeContext.quantizationGain = activeAutoQGain;\n> +\t}\n> +\tif (frameContext.autoGainEnabled) {\n> +\t\tframeContext.gain = activeAutoGain;\n> +\t\tframeContext.quantizationGain = activeAutoQGain;\n> +\t}\n> +\n> +\t/*\n> +\t * Populate manual exposure and gain from the active auto values when\n> +\t * transitioning from auto to manual\n> +\t */\n\nI guess this answers to my question on the auto->manual transition\ntracking for the frameContext.autoExposureModeChange flag.\n\nIt tracks both state changes, but we use it to propagate auto values\nto the manual state if they have not been overriden by an application\nprovide control\n\n> +\tif (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {\n> +\t\tstate.manual.exposure = activeAutoExposure;\n> +\t\tframeContext.exposure = activeAutoExposure;\n> +\t}\n> +\tif (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {\n> +\t\tstate.manual.gain = activeAutoGain;\n> +\t\tframeContext.gain = activeAutoGain;\n> +\t\tframeContext.quantizationGain = activeAutoQGain;\n> +\t}\n> +\n> +\tframeContext.yTarget = state.automatic.yTarget;\n> +}\n\nAck, this matches RkISP1Agc::prepare()\n\n> +\n> +/**\n> + * \\brief Handle a \\a process operation\n> + */\n> +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,\n> +\t\t\t agc::FrameContext &frameContext, std::optional &¶ms,\n> +\t\t\t ControlList &metadata)\n> +{\n> +\tconst utils::Duration &lineDuration = session.lineDuration;\n> +\tutils::Duration newExposureTime = {};\n> +\n> +\tif (params) {\n\nI presume (well, you told me) that this matches the\n\n if (!stats)\n\ncase in RkISP1.\n\nI wonder, should\n\nThe RkISP1 IPA does\n\n\tif (!stats) {\n\t\tprocessFrameDuration(context, frameContext,\n\t\t\t\t frameContext.agc.minFrameDuration);\n\t\tfillMetadata(context, frameContext, metadata);\n\t\treturn;\n\t}\n\nI guess it's fine for IPA to call AgcAlgorithm::process() even when\n!stats, as it is simpler for them to make the call unconditionally, so\nI'm not against this.\n\nBut maybe we can do like RkISP1 does (split processFrameDuration() and\nfillMetadata() to helpers) to save some indentation here.\n\n> +\t\tASSERT(session.autoAllowed);\n\nDoes it mean IPA should not call process if (autoAllowed) ?\n\nCan't we simply bail out here to make IPAs simpler ?\n\n> +\n> +\t\t/*\n> +\t\t* Set the AGC limits using the fixed exposure time and/or gain in\n> +\t\t* manual mode, or the sensor limits in auto mode.\n> +\t\t*/\n\nWeird indent\n\n> +\t\tutils::Duration minExposureTime;\n> +\t\tutils::Duration maxExposureTime;\n> +\t\tdouble minAnalogueGain;\n> +\t\tdouble maxAnalogueGain;\n> +\n> +\t\tif (frameContext.autoExposureEnabled) {\n> +\t\t\tminExposureTime = session.minExposureTime;\n> +\t\t\tmaxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);\n\nvery long line\n\n> +\t\t} else {\n> +\t\t\tminExposureTime = lineDuration * frameContext.exposure;\n> +\t\t\tmaxExposureTime = minExposureTime;\n> +\t\t}\n> +\n> +\t\tif (frameContext.autoGainEnabled) {\n> +\t\t\tminAnalogueGain = session.minAnalogueGain;\n> +\t\t\tmaxAnalogueGain = session.maxAnalogueGain;\n> +\t\t} else {\n> +\t\t\tminAnalogueGain = frameContext.gain;\n> +\t\t\tmaxAnalogueGain = frameContext.gain;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t* The Agc algorithm needs to know the effective exposure value that was\n> +\t\t* applied to the sensor when the statistics were collected.\n> +\t\t*/\n> +\t\tutils::Duration effectiveExposureValue =\n> +\t\t\tlineDuration * params->exposure * params->gain;\n\nRkISP1 gets exposure and gain from the frameContext.\n\nI see in the next patches that port RkISP1 to AgcAlgorithm this hunk\n\n agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc,\n\n ...\n\n .exposure = frameContext.sensor.exposure,\n /*\n * Include the quantization gain if it was applied. Do not use\n * compress.gain because it will include gains that shall not be\n * reported to the user when HDR is implemented.\n */\n .gain = frameContext.sensor.gain\n * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),\n\nIs the reason you can't take exposure and gain from the FrameContext\nthe gain adjustment ?\n\n> +\n> +\t\timpl_.setLimits(minExposureTime, maxExposureTime,\n> +\t\t\t\tminAnalogueGain, maxAnalogueGain,\n> +\t\t\t\tstd::move(params->additionalConstraints));\n> +\n> +\t\tconst auto &newEv = impl_.calculateNewEv({\n> +\t\t\t.traits = params->traits,\n> +\t\t\t.yHist = params->yHist,\n> +\t\t\t.effectiveExposureValue = effectiveExposureValue,\n> +\t\t\t.constraintModeIndex = frameContext.constraintMode,\n> +\t\t\t.exposureModeIndex = frameContext.exposureMode,\n> +\t\t\t.lux = params->lux,\n> +\t\t\t.exposureCompensation = pow(2.0, frameContext.exposureValue),\n> +\t\t});\n> +\n> +\t\tLOG(Agc, Debug)\n> +\t\t\t<< \"Divided up exposure time, analogue gain, quantization gain\"\n> +\t\t\t<< \" and digital gain are \" << newEv.exposureTime << \", \" << newEv.analogueGain\n> +\t\t\t<< \", \" << newEv.quantizationGain << \" and \" << newEv.digitalGain;\n> +\n> +\t\t/* Update the estimated exposure and gain. */\n> +\t\tstate.automatic.exposure = newEv.exposureTime / lineDuration;\n> +\t\tstate.automatic.gain = newEv.analogueGain;\n> +\t\tstate.automatic.quantizationGain = newEv.quantizationGain;\n> +\t\tstate.automatic.yTarget = newEv.yTarget;\n> +\n> +\t\tnewExposureTime = newEv.exposureTime;\n> +\t}\n> +\n> +\t/*\n> +\t * Expand the target frame duration so that we do not run faster than\n> +\t * the minimum frame duration when we have short exposures.\n> +\t */\n> +\tconst auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);\n> +\tframeContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;\n> +\n> +\t/* Update frame duration accounting for line length quantization. */\n> +\tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;\n\nEasy to shorten to\n\n\tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank)\n\t\t\t\t * lineDuration;\n\n> +\n> +\tmetadata.set(controls::AnalogueGain, frameContext.gain);\n> +\tmetadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get());\n\nI'm not sure I understand the strategy here.. this line is very long\nand could be easily made\n\n\tmetadata.set(controls::ExposureTime,\n\t\t utils::Duration(lineDuration * frameContext.exposure).get());\n\n\n> +\tmetadata.set(controls::FrameDuration, frameContext.frameDuration.get());\n> +\tmetadata.set(controls::ExposureTimeMode,\n> +\t\t frameContext.autoExposureEnabled\n> +\t\t ? controls::ExposureTimeModeAuto\n> +\t\t : controls::ExposureTimeModeManual);\n> +\tmetadata.set(controls::AnalogueGainMode,\n> +\t\t frameContext.autoGainEnabled\n> +\t\t ? controls::AnalogueGainModeAuto\n> +\t\t : controls::AnalogueGainModeManual);\n\nThese ones instead are very short and could easily be made\n\n\tmetadata.set(controls::ExposureTimeMode, frameContext.autoExposureEnabled ?\n\t\t controls::ExposureTimeModeAuto : controls::ExposureTimeModeManual);\n\n> +\n> +\tmetadata.set(controls::AeExposureMode, frameContext.exposureMode);\n> +\tmetadata.set(controls::AeConstraintMode, frameContext.constraintMode);\n> +\tmetadata.set(controls::ExposureValue, frameContext.exposureValue);\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h\n> index 5247425952..1eace12908 100644\n> --- a/src/ipa/libipa/agc.h\n> +++ b/src/ipa/libipa/agc.h\n> @@ -7,13 +7,19 @@\n>\n> #pragma once\n>\n> +#include \n> #include \n>\n> #include \n>\n> +#include \n> #include \n>\n> +#include \n> +\n> +#include \"agc_mean_luminance.h\"\n> #include \"camera_sensor_helper.h\"\n> +#include \"histogram.h\"\n>\n> namespace libcamera {\n>\n> @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,\n> \tcontrols.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));\n> }\n>\n> +struct Session {\n> +\tutils::Duration minExposureTime;\n> +\tutils::Duration maxExposureTime;\n> +\tdouble minAnalogueGain;\n> +\tdouble maxAnalogueGain;\n> +\tutils::Duration minFrameDuration;\n> +\tutils::Duration maxFrameDuration;\n> +\n> +\tutils::Duration lineDuration;\n> +\n> +\tstruct {\n> +\t\tSize outputSize;\n> +\t} sensor;\n> +\n> +\tbool autoAllowed;\n> +};\n> +\n> +struct ActiveState {\n> +\tstruct {\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t} manual;\n> +\tstruct {\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t\tdouble quantizationGain;\n> +\t\tdouble yTarget;\n> +\t} automatic;\n> +\n> +\tbool autoExposureEnabled;\n> +\tbool autoGainEnabled;\n> +\tdouble exposureValue;\n> +\tcontrols::AeConstraintModeEnum constraintMode;\n> +\tcontrols::AeExposureModeEnum exposureMode;\n> +\tutils::Duration minFrameDuration;\n> +\tutils::Duration maxFrameDuration;\n> +};\n> +\n> +struct FrameContext {\n> +\tuint32_t exposure;\n> +\tdouble gain;\n> +\tdouble quantizationGain;\n> +\tdouble exposureValue;\n> +\tdouble yTarget;\n> +\tuint32_t vblank;\n> +\tbool autoExposureEnabled;\n> +\tbool autoGainEnabled;\n> +\tcontrols::AeConstraintModeEnum constraintMode;\n> +\tcontrols::AeExposureModeEnum exposureMode;\n> +\tutils::Duration minFrameDuration;\n> +\tutils::Duration maxFrameDuration;\n> +\tutils::Duration frameDuration;\n> +\tbool autoExposureModeChange;\n> +\tbool autoGainModeChange;\n> +};\n> +\n> } /* namespace agc */\n>\n> +class AgcAlgorithm\n> +{\n> +public:\n> +\tstruct ConfigurationParams {\n> +\t\tconst CameraSensorHelper *sensor;\n> +\t\tconst IPACameraSensorInfo &sensorInfo;\n> +\t\tconst ControlInfoMap &sensorControls;\n> +\t\tControlInfoMap::Map &ctrlMap;\n> +\t\tbool autoAllowed = true;\n> +\t};\n> +\n> +\tint init(const ValueNode &tuningData);\n> +\n> +\tint configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);\n\neasy to shorten\n\n> +\n> +\tvoid queueRequest(const agc::Session &session, agc::ActiveState &state,\n> +\t\t\t agc::FrameContext &frameContext, const ControlList &controls);\n> +\n> +\tvoid prepare(agc::ActiveState &state, agc::FrameContext &frameContext);\n> +\n> +\tstruct ProcessParams {\n> +\t\tconst AgcMeanLuminance::Traits &traits;\n> +\t\tconst Histogram &yHist;\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t\tstd::vector &&additionalConstraints = {};\n> +\t\tdouble lux = 0;\n> +\t};\n> +\n> +\tvoid process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,\n\nsame here\n\n> +\t\t std::optional &¶ms, ControlList &metadata);\n> +\n> +private:\n> +\tAgcMeanLuminance impl_;\n> +};\n> +\n> } /* namespace ipa */\n>\n> } /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 05f1a8749c..ca681fa5af 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -23,6 +23,7 @@ libipa_headers = files([\n> ])\n>\n> libipa_sources = files([\n> + 'agc.cpp',\n> 'agc_mean_luminance.cpp',\n> 'algorithm.cpp',\n> 'awb_bayes.cpp',\n\nEnormous work overall!\n\nAgc is the most complex beast we have and I'm looking forwards to\ngetting this merged already!\n\n\n> --\n> 2.55.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F1D2EBDE4C\n\tfor ;\n\tFri, 24 Jul 2026 15:01:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4CDE67F2F;\n\tFri, 24 Jul 2026 17:01:32 +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 C687D67E89\n\tfor ;\n\tFri, 24 Jul 2026 17:01:31 +0200 (CEST)","from ideasonboard.com (93-46-82-201.ip106.fastwebnet.it\n\t[93.46.82.201])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB59D9A4;\n\tFri, 24 Jul 2026 17:00:29 +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=\"mKxCWZze\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1784905230;\n\tbh=sP6pthtXbfa8xT/Gdq4GHDHj0DoffHp8wgsdEXIXPCY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mKxCWZzeTqTrW6GV0OSrlwDg9LhLDQ6h2dBg2iS8cgitEYPxho8IC7acvfp0P2bMj\n\tS/JGStJ0+POzMPvA3yMLWm/pzEI9DfVwRZgruxWehOOob+P05nVZ1gXbHmdhbsQaaE\n\tbSpRLv2wjymGXwFAykVFzJ+yLYRPHKG+i2ppt59w=","Date":"Fri, 24 Jul 2026 17:01:29 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","Message-ID":"","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-22-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20260723154327.1357866-22-barnabas.pocze@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":39863,"web_url":"https://patchwork.libcamera.org/comment/39863/","msgid":"","date":"2026-07-27T08:53:33","subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2026. 07. 24. 17:01 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:\n>> Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`\n>> based on the rkisp1 `Agc` algorithm, with slight adjustments.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/ipa/libipa/agc.cpp | 639 +++++++++++++++++++++++++++++++++++++\n>> src/ipa/libipa/agc.h | 98 ++++++\n>> src/ipa/libipa/meson.build | 1 +\n>> 3 files changed, 738 insertions(+)\n>> create mode 100644 src/ipa/libipa/agc.cpp\n>>\n>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n>> new file mode 100644\n>> index 0000000000..e16a02fdde\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/agc.cpp\n>> @@ -0,0 +1,639 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021-2022, 2026 Ideas On Board\n>> + *\n>> + * Generic AGC algorithm\n>> + */\n>> +\n>> +#include \"agc.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +using namespace std::chrono_literals;\n>> +\n>> +LOG_DEFINE_CATEGORY(Agc)\n>> +\n>> +/**\n>> + * \\class AgcAlgorithm\n>> + * \\brief AgcMeanLuminance wrapper for implementing the Algorithm interface\n>> + *\n>> + * \\todo DigitalGain, DigitalGainMode\n>> + */\n>> +\n>> +/**\n>> + * \\struct agc::Session\n>> + * \\brief Session configuration for AgcAlgorithm\n>> + *\n>> + * \\var agc::Session::minExposureTime\n>> + * \\brief Minimum exposure time supported with the configured sensor\n>> + *\n>> + * \\var agc::Session::maxExposureTime\n>> + * \\brief Maximum exposure time supported with the configured sensor\n>> + *\n>> + * \\var agc::Session::minAnalogueGain\n>> + * \\brief Minimum analogue gain supported with the configured sensor\n>> + *\n>> + * \\var agc::Session::maxAnalogueGain\n>> + * \\brief Maximum analogue gain supported with the configured sensor\n>> + *\n>> + * \\var agc::Session::minFrameDuration\n>> + * \\brief Minimum frame duration supported with the configured sensor\n>> + *\n>> + * \\var agc::Session::maxFrameDuration\n>> + * \\brief Maximum frame duration supported with the configured sensor\n> \n> I would use \"for the streaming session\" in place of \"with the\n> configured sensor\" in all previous entries.\n> \n> yes, those settings depend on the sensor configuration but I found the\n> wording a bit confusing\n> \n> Or maybe \"for the current sensor configuration\" ?\n\nI'll drop \"supported\" and use \"for the streaming session\".\n\n\n> \n>> + *\n>> + * \\var agc::Session::lineDuration\n>> + * \\brief Line duration with the configured sensor and output size\n>> + *\n>> + * \\var agc::Session::sensor\n>> + * \\brief Details of the sensor configuration\n>> + *\n>> + * \\var agc::Session::sensor.outputSize\n>> + * \\brief Configured output size of the sensor\n>> + *\n>> + * \\var agc::Session::autoAllowed\n>> + * \\brief Whether automatic controls are allowed\n> \n> As I was really confused by this, is it worth adding one line saying\n> that auto is not allowed when capturing raw frames ?\n\nI suppose if statistics are provided, then it can work for raw capture as well.\nSo my suggestion:\n\n \\brief Whether automatic controls are allowed\n \n Determines whether statistics are generally expected to be available for\n automatic exposure and gain control. If \\a false, statistics must not\n be provided to \\a AgcAlgorithm::process(), and \\a ExposureTimeMode and\n \\a AnalogueGainMode will only avertise manual control, and multiple other\n controls will also be omitted.\n\n\n> \n>> + */\n>> +\n>> +/**\n>> + * \\struct agc::ActiveState\n>> + * \\brief Active state for AgcAlgorithm\n>> + *\n>> + * The \\a automatic variables track the latest values computed by algorithm\n>> + * based on the latest processed statistics. All other variables track the\n>> + * consolidated controls requested in queued requests.\n>> + *\n>> + * \\var agc::ActiveState::manual\n>> + * \\brief Manual exposure time and analog gain (set through requests)\n>> + *\n>> + * \\var agc::ActiveState::manual.exposure\n>> + * \\brief Manual exposure time expressed as a number of lines as set by the\n>> + * ExposureTime control\n>> + *\n>> + * \\var agc::ActiveState::manual.gain\n>> + * \\brief Manual analogue gain as set by the AnalogueGain control\n>> + *\n>> + * \\var agc::ActiveState::automatic\n>> + * \\brief Automatic exposure time and analog gain (computed by the algorithm)\n>> + *\n>> + * \\var agc::ActiveState::automatic.exposure\n>> + * \\brief Automatic exposure time expressed as a number of lines\n>> + *\n>> + * \\var agc::ActiveState::automatic.gain\n>> + * \\brief Automatic analogue gain multiplier\n>> + *\n>> + * \\var agc::ActiveState::automatic.quantizationGain\n>> + * \\brief Automatic quantization gain multiplier\n>> + *\n>> + * \\var agc::ActiveState::automatic.yTarget\n>> + * \\brief Automatically determined luminance target\n>> + *\n>> + * \\var agc::ActiveState::autoExposureEnabled\n>> + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n> \n> s/AGC state (exposure)/exposure mode\n> \n>> + *\n>> + * \\var agc::ActiveState::autoGainEnabled\n>> + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n> \n> same here ?\n\nThese were copied verbatim.\n\n\n> \n>> + *\n>> + * \\var agc::ActiveState::exposureValue\n>> + * \\brief Exposure value as set by the ExposureValue control\n>> + *\n>> + * \\var agc::ActiveState::constraintMode\n>> + * \\brief Constraint mode as set by the AeConstraintMode control\n>> + *\n>> + * \\var agc::ActiveState::exposureMode\n>> + * \\brief Exposure mode as set by the AeExposureMode control\n>> + *\n>> + * \\var agc::ActiveState::minFrameDuration\n>> + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n>> + *\n>> + * \\var agc::ActiveState::maxFrameDuration\n>> + * \\brief Maximum frame duration as set by the FrameDurationLimits control\n>> + */\n>> +\n>> +/**\n>> + * \\struct agc::FrameContext\n>> + * \\brief Per-frame context for AgcAlgorithm\n>> + *\n>> + * \\var agc::FrameContext::exposure\n>> + * \\brief Exposure time expressed as a number of lines computed by the algorithm\n>> + *\n>> + * \\var agc::FrameContext::gain\n>> + * \\brief Analogue gain multiplier computed by the algorithm\n> \n> Are these computed by the algorithms or can these be the manually\n> programmed values when running in manual mode ?\n\nThis is always the gain that needs to be applied to the frame, so\nthe manual gain in manual mode, the automatic gain in automatic mode.\n\n\n> \n> I would replace \"computed by the algorithm\" with \"for the frame\" or\n> similar\n> \n>> + *\n>> + * The gain should be adapted to the sensor specific gain code before applying.\n> \n> s/adapted/translated ?\n\nThese were also all copied verbatim, but I'll adjust it.\n\n\n> \n>> + *\n>> + * \\var agc::FrameContext::quantizationGain\n>> + * \\brief Quantization gain multiplier computed by the algorithm\n>> + *\n>> + * \\var agc::FrameContext::exposureValue\n>> + * \\brief Exposure value as set by the ExposureValue control\n>> + *\n>> + * \\var agc::FrameContext::yTarget\n>> + * \\brief Luminance target computed by the algorithm\n>> + *\n>> + * \\var agc::FrameContext::vblank\n>> + * \\brief Vertical blanking parameter computed by the algorithm\n>> + *\n>> + * \\var agc::FrameContext::autoExposureEnabled\n>> + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n>> + *\n>> + * \\var agc::FrameContext::autoGainEnabled\n>> + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n>> + *\n>> + * \\var agc::FrameContext::constraintMode\n>> + * \\brief Constraint mode as set by the AeConstraintMode control\n>> + *\n>> + * \\var agc::FrameContext::exposureMode\n>> + * \\brief Exposure mode as set by the AeExposureMode control\n>> + *\n>> + * \\var agc::FrameContext::minFrameDuration\n>> + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n>> + *\n>> + * \\var agc::FrameContext::maxFrameDuration\n>> + * \\brief Maximum frame duration as set by the FrameDurationLimits control\n>> + *\n>> + * \\var agc::FrameContext::frameDuration\n>> + * \\brief The actual FrameDuration used by the algorithm for the frame\n>> + *\n>> + * \\var agc::FrameContext::autoExposureModeChange\n>> + * \\brief Indicate if autoExposureEnabled has changed from true in the previous\n>> + * frame to false in the current frame, and no manual exposure value has been\n>> + * supplied in the current frame.\n> \n> does this only track auto->manual transaction or does it track the\n> other way around.\n> \n> Kind of a long brief (and no '.' at the end).\n\nThese were also copied verbatim, but I have removed the dots.\n\n\n> \n> I would\n> \n> \\brief Exposure mode change flag\n> \n> Indicate if the exposure mode has changed compared to the\n> previous frame.\n\nIt only tracks auto -> manual transitions without manual `ExposureTime`,\nso I think the above wouldn't be entirely correct.\n\n\n> \n>> + *\n>> + * \\var agc::FrameContext::autoGainModeChange\n>> + * \\brief Indicate if autoGainEnabled has changed from true in the previous\n>> + * frame to false in the current frame, and no manual gain value has been\n>> + * supplied in the current frame.\n>> + */\n>> +\n>> +/**\n>> + * \\struct AgcAlgorithm::ConfigurationParams\n>> + * \\brief Parameters for AgcAlgorithm::configure()\n>> + *\n>> + * \\var AgcAlgorithm::ConfigurationParams::sensor\n>> + * \\brief CameraSensorHelper for the sensor\n>> + *\n>> + * \\var AgcAlgorithm::ConfigurationParams::sensorInfo\n>> + * \\brief Details of the sensor\n> \n> The sensor configuration description\n\nI have adjusted it to\n\n Current configuration of the sensor\n\nhope it is also acceptable.\n\n\n> \n>> + *\n>> + * \\var AgcAlgorithm::ConfigurationParams::sensorControls\n>> + * \\brief ControlInfoMap of the sensor\n>> + *\n>> + * \\var AgcAlgorithm::ConfigurationParams::ctrlMap\n>> + * \\brief ControlMap to update with controls\n>> + *\n>> + * \\var AgcAlgorithm::ConfigurationParams::autoAllowed\n>> + * \\brief Whether to enable auto controls\n>> + */\n>> +\n>> +/**\n>> + * \\struct AgcAlgorithm::ProcessParams\n>> + * \\brief Parameters for AgcAlgorithm::process()\n>> + *\n>> + * \\var AgcAlgorithm::ProcessParams::traits\n>> + * \\brief Implementation of AgcMeanLuminance::Traits\n>> + *\n>> + * \\var AgcAlgorithm::ProcessParams::yHist\n>> + * \\brief Luminance histogram of the frame\n>> + *\n>> + * \\var AgcAlgorithm::ProcessParams::exposure\n>> + * \\brief Effective exposure of the frame\n>> + *\n>> + * \\var AgcAlgorithm::ProcessParams::gain\n>> + * \\brief Effective gain of the frame\n>> + *\n>> + * \\var AgcAlgorithm::ProcessParams::additionalConstraints\n>> + * \\brief Additional AgcMeanLuminance::AgcConstraints to apply\n>> + *\n>> + * \\var AgcAlgorithm::ProcessParams::lux\n>> + * \\brief Effective lux value of the frame\n>> + */\n>> +\n>> +/**\n>> + * \\brief Load tuning data\n>> + */\n>> +int AgcAlgorithm::init(const ValueNode &tuningData)\n>> +{\n>> +\tint ret = impl_.parseTuningData(tuningData);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Initialize the session configuration and active state\n>> + */\n>> +int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config)\n> \n> Easy to shorten to\n> \n> int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,\n> \t\t\t const ConfigurationParams &config)\n\nDone.\n\n\n> \n> \n> Seems like session is an out parameter.\n> \n> We used to have a rule: const & for input params, * for output ones.\n> Not sure anymore how much we enforce it these days..\n\nI'm not sure I would consider it an out param in the traditional sense\n(i.e. the caller is not really expected to use/inspect the result), and\nI really prefer the consistency of always passing by reference to every\nfunction of `AgcAlgorithm`.\n\n\n> \n>> +{\n>> +\tsession = {};\n>> +\tsession.lineDuration = config.sensorInfo.minLineLength * 1.0s\n>> +\t\t/ config.sensorInfo.pixelRate;\n>> +\tsession.sensor.outputSize = config.sensorInfo.outputSize;\n>> +\tsession.autoAllowed = config.autoAllowed;\n>> +\n>> +\tconst double lineDurationUs = session.lineDuration.get();\n>> +\n>> +\t/*\n>> +\t * Compute exposure time limits from the V4L2_CID_EXPOSURE control\n>> +\t * limits and the line duration.\n>> +\t */\n>> +\n>> +\tconst ControlInfo &v4l2Exposure = config.sensorControls.find(V4L2_CID_EXPOSURE)->second;\n>> +\tint32_t minExposure = v4l2Exposure.min().get();\n>> +\tint32_t maxExposure = v4l2Exposure.max().get();\n>> +\tint32_t defExposure = v4l2Exposure.def().get();\n>> +\tconfig.ctrlMap[&controls::ExposureTime] = ControlInfo{\n>> +\t\tstatic_cast(minExposure * lineDurationUs),\n>> +\t\tstatic_cast(maxExposure * lineDurationUs),\n>> +\t\tstatic_cast(defExposure * lineDurationUs),\n>> +\t};\n>> +\n>> +\t/* Compute the analogue gain limits. */\n>> +\tconst ControlInfo &v4l2Gain = config.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>> +\tfloat minGain = config.sensor->gain(v4l2Gain.min().get());\n>> +\tfloat maxGain = config.sensor->gain(v4l2Gain.max().get());\n>> +\tfloat defGain = config.sensor->gain(v4l2Gain.def().get());\n>> +\tconfig.ctrlMap[&controls::AnalogueGain] = ControlInfo{\n>> +\t\tminGain,\n>> +\t\tmaxGain,\n>> +\t\tdefGain,\n>> +\t};\n>> +\n>> +\tLOG(Agc, Debug)\n>> +\t\t<< \"Exposure: [\" << minExposure << \", \" << maxExposure\n>> +\t\t<< \"], gain: [\" << minGain << \", \" << maxGain << \"]\";\n>> +\n>> +\t/*\n>> +\t * Compute the frame duration limits.\n>> +\t *\n>> +\t * The frame length is computed assuming a fixed line length combined\n>> +\t * with the vertical frame sizes.\n>> +\t */\n>> +\tconst ControlInfo &v4l2HBlank = config.sensorControls.find(V4L2_CID_HBLANK)->second;\n>> +\tuint32_t hblank = v4l2HBlank.def().get();\n>> +\tuint32_t lineLength = config.sensorInfo.outputSize.width + hblank;\n>> +\n>> +\tconst ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;\n>> +\tstd::array frameHeights{\n>> +\t\tv4l2VBlank.min().get() + config.sensorInfo.outputSize.height,\n>> +\t\tv4l2VBlank.max().get() + config.sensorInfo.outputSize.height,\n>> +\t\tv4l2VBlank.def().get() + config.sensorInfo.outputSize.height,\n>> +\t};\n>> +\n>> +\tstd::array frameDurations;\n>> +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n>> +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n>> +\t\tframeDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U);\n>> +\t}\n>> +\n>> +\tconfig.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{\n>> +\t\tframeDurations[0],\n>> +\t\tframeDurations[1],\n>> +\t\tSpan{ { frameDurations[2], frameDurations[2] } },\n>> +\t};\n>> +\n>> +\tsession.minFrameDuration = std::chrono::microseconds(frameDurations[0]);\n>> +\tsession.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);\n>> +\n>> +\t/*\n>> +\t * When the AGC computes the new exposure values for a frame, it needs\n>> +\t * to know the limits for exposure time and analogue gain. As it depends\n>> +\t * on the sensor, update it with the controls.\n>> +\t *\n>> +\t * \\todo take VBLANK into account for maximum exposure time\n>> +\t */\n>> +\tsession.minExposureTime = minExposure * session.lineDuration;\n>> +\tsession.maxExposureTime = maxExposure * session.lineDuration;\n>> +\tsession.minAnalogueGain = minGain;\n>> +\tsession.maxAnalogueGain = maxGain;\n> \n> The above matches the current RkISP1Agc::reconfigure() content\n> \n>> +\n>> +\timpl_.configure(session.lineDuration, config.sensor);\n>> +\timpl_.setLimits(session.minExposureTime, session.maxExposureTime,\n>> +\t\t\tsession.minAnalogueGain, session.maxAnalogueGain,\n>> +\t\t\t{});\n>> +\timpl_.resetFrameCount();\n>> +\n>> +\t/* Configure the default exposure and gain. */\n>> +\tstate = {};\n>> +\tstate.automatic.gain = session.minAnalogueGain;\n>> +\tstate.automatic.exposure = 10ms / session.lineDuration;\n>> +\tstate.automatic.quantizationGain = 1;\n>> +\tstate.automatic.yTarget = impl_.effectiveYTarget(0, 1);\n> \n> And this matches RkISP1::Agc() apart from this early yTarget\n> computation which I presume is intentional\n\nHmmm... that should match as well. There is\n\n context.activeState.agc.automatic.yTarget = agc_.effectiveYTarget(0, 1);\n\nat the end of `rkisp1.cpp:Agc::configure()`.\n\n\n> \n>> +\tstate.manual.gain = state.automatic.gain;\n>> +\tstate.manual.exposure = state.automatic.exposure;\n>> +\tstate.autoExposureEnabled = session.autoAllowed;\n>> +\tstate.autoGainEnabled = session.autoAllowed;\n>> +\tstate.exposureValue = 0;\n>> +\n>> +\tstate.constraintMode =\n>> +\t\tstatic_cast(impl_.constraintModes().begin()->first);\n>> +\tstate.exposureMode =\n>> +\t\tstatic_cast(impl_.exposureModeHelpers().begin()->first);\n>> +\n>> +\tstate.minFrameDuration = session.minFrameDuration;\n>> +\tstate.maxFrameDuration = session.maxFrameDuration;\n>> +\n>> +\tconst auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) {\n>> +\t\tstd::array values;\n>> +\t\tsize_t count = 0;\n>> +\n>> +\t\tif (session.autoAllowed)\n>> +\t\t\tvalues[count++] = ControlValue(automatic);\n>> +\n>> +\t\tvalues[count++] = ControlValue(manual);\n>> +\n>> +\t\tconfig.ctrlMap[&cid] = ControlInfo{\n>> +\t\t\t{ values.data(), count },\n>> +\t\t\tControlValue(session.autoAllowed ? automatic : manual),\n>> +\t\t};\n>> +\t};\n>> +\n>> +\tadd(controls::ExposureTimeMode, controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);\n>> +\tadd(controls::AnalogueGainMode, controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);\n> \n> Could you please break trivially re-adjustable long lines ?\n\nAdjusted to\n\n add(controls::ExposureTimeMode,\n controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);\n add(controls::AnalogueGainMode,\n controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);\n\nDoes that look better?\n\n\n> \n>> +\n>> +\t/* \\todo Move this to the `Camera` class. */\n>> +\tconfig.ctrlMap[&controls::AeEnable] = ControlInfo{\n>> +\t\tfalse,\n>> +\t\tsession.autoAllowed,\n>> +\t\tsession.autoAllowed,\n>> +\t};\n> \n> RkISP1Agc register controls in init() but I think what you're doing\n> here it's better, as some control such as ExposureTimeMode depend on\n> the session configuration.\n> \n> Is there any risk of having leftovers from a previous session in the\n> control list ?\n\nEvery control should either be overwritten or removed.\n\n\n> \n>> +\n>> +\tif (session.autoAllowed) {\n>> +\t\tconfig.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);\n>> +\n>> +\t\tfor (const auto &[id, info] : impl_.controls())\n>> +\t\t\tconfig.ctrlMap[id] = info;\n>> +\t} else {\n>> +\t\tconfig.ctrlMap.erase(&controls::ExposureValue);\n>> +\n>> +\t\tfor (const auto &[id, info] : impl_.controls())\n>> +\t\t\tconfig.ctrlMap.erase(id);\n>> +\t}\n> \n> Maybe it happens later on, but this is a bit cumbersome. Making\n> AgcMeanLuminance aware of autoAllowed would avoid this\n\nPossibly, but my preference is to keep more control handling out of AgcMeanLuminance.\nAnd in any case, there will be a loop like this or equivalent, and\nI'm not sure it matters too much where it is.\n\n\n> \n>> +\n>> +\n> \n> Double empty line\n\nFixed.\n\n\n> \n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Handle a \\a queueRequest operation\n>> + */\n>> +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,\n>> +\t\t\t\tagc::FrameContext &frameContext, const ControlList &controls)\n>> +{\n>> +\tif (session.autoAllowed) {\n> \n> Now that you register controls at configure() time and not init()\n> time, am I correct that if !autoAllowed applications won't be able to select\n> controls::ExposureTimeModeAuto ?\n\nYes.\n\n\n> \n> Which makes me wonder if controls::ExposureTimeMode should be\n> registered at all if !autoAllowed\n\nI think that's more of a philosophical question. My preference is to\nregister controls even if they only really have one valid value. Maybe\nthis needs more discussion to arrive at a libcamera-wide policy.\n\n\n> \n>> +\t\tconst auto &aeEnable = controls.get(controls::ExposureTimeMode);\n>> +\t\tif (aeEnable &&\n>> +\t\t (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {\n>> +\t\t\tstate.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);\n>> +\n>> +\t\t\tLOG(Agc, Debug)\n>> +\t\t\t\t<< (state.autoExposureEnabled ? \"Enabling\" : \"Disabling\")\n>> +\t\t\t\t<< \" AGC (exposure)\";\n>> +\n>> +\t\t\t/*\n>> +\t\t\t * If we go from auto -> manual with no manual control\n>> +\t\t\t * set, use the last computed value, which we don't\n>> +\t\t\t * know until prepare() so save this information.\n>> +\t\t\t *\n>> +\t\t\t * \\todo Check the previous frame at prepare() time\n>> +\t\t\t * instead of saving a flag here\n>> +\t\t\t */\n>> +\t\t\tif (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))\n>> +\t\t\t\tframeContext.autoExposureModeChange = true;\n>> +\t\t}\n>> +\n>> +\t\tconst auto &agEnable = controls.get(controls::AnalogueGainMode);\n> \n> Same reasoning for AnalogueGainMode\n> \n>> +\t\tif (agEnable &&\n>> +\t\t (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {\n>> +\t\t\tstate.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);\n>> +\n>> +\t\t\tLOG(Agc, Debug)\n>> +\t\t\t\t<< (state.autoGainEnabled ? \"Enabling\" : \"Disabling\")\n>> +\t\t\t\t<< \" AGC (gain)\";\n>> +\t\t\t/*\n>> +\t\t\t * If we go from auto -> manual with no manual control\n>> +\t\t\t * set, use the last computed value, which we don't\n>> +\t\t\t * know until prepare() so save this information.\n>> +\t\t\t */\n>> +\t\t\tif (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))\n>> +\t\t\t\tframeContext.autoGainModeChange = true;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\tconst auto &exposure = controls.get(controls::ExposureTime);\n>> +\tif (exposure && !state.autoExposureEnabled) {\n>> +\t\tstate.manual.exposure = *exposure * 1.0us / session.lineDuration;\n>> +\n>> +\t\tLOG(Agc, Debug)\n>> +\t\t\t<< \"Set exposure to \" << state.manual.exposure;\n>> +\t}\n>> +\n>> +\tconst auto &gain = controls.get(controls::AnalogueGain);\n>> +\tif (gain && !state.autoGainEnabled) {\n>> +\t\tstate.manual.gain = *gain;\n>> +\n>> +\t\tLOG(Agc, Debug) << \"Set gain to \" << state.manual.gain;\n>> +\t}\n>> +\n>> +\tframeContext.autoExposureEnabled = state.autoExposureEnabled;\n>> +\tframeContext.autoGainEnabled = state.autoGainEnabled;\n>> +\n>> +\tif (!frameContext.autoExposureEnabled)\n>> +\t\tframeContext.exposure = state.manual.exposure;\n>> +\tif (!frameContext.autoGainEnabled)\n>> +\t\tframeContext.gain = state.manual.gain;\n>> +\n>> +\tif (!frameContext.autoExposureEnabled &&\n>> +\t !frameContext.autoGainEnabled)\n>> +\t\tframeContext.quantizationGain = 1.0;\n>> +\n>> +\tconst auto &exposureMode = controls.get(controls::AeExposureMode);\n>> +\tif (exposureMode)\n>> +\t\tstate.exposureMode =\n>> +\t\t\tstatic_cast(*exposureMode);\n>> +\tframeContext.exposureMode = state.exposureMode;\n>> +\n>> +\tconst auto &constraintMode = controls.get(controls::AeConstraintMode);\n>> +\tif (constraintMode)\n>> +\t\tstate.constraintMode =\n>> +\t\t\tstatic_cast(*constraintMode);\n>> +\tframeContext.constraintMode = state.constraintMode;\n>> +\n>> +\tconst auto &exposureValue = controls.get(controls::ExposureValue);\n>> +\tif (exposureValue)\n>> +\t\tstate.exposureValue = *exposureValue;\n>> +\tframeContext.exposureValue = state.exposureValue;\n>> +\n>> +\tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n>> +\tif (frameDurationLimits) {\n>> +\t\t/* Limit the control value to the limits in ControlInfo */\n>> +\t\tstate.minFrameDuration = std::clamp(\n>> +\t\t\tstd::chrono::microseconds((*frameDurationLimits).front()),\n>> +\t\t\tsession.minFrameDuration,\n>> +\t\t\tsession.maxFrameDuration\n>> +\t\t);\n>> +\n>> +\t\tstate.maxFrameDuration = std::clamp(\n>> +\t\t\tstd::chrono::microseconds((*frameDurationLimits).back()),\n>> +\t\t\tsession.minFrameDuration,\n>> +\t\t\tsession.maxFrameDuration\n>> +\t\t);\n>> +\t}\n>> +\tframeContext.minFrameDuration = state.minFrameDuration;\n>> +\tframeContext.maxFrameDuration = state.maxFrameDuration;\n>> +}\n>> +\n> \n> This matches RkISP1Agc::queueRequest() (metering mode handling apart)\n> \n>> +/**\n>> + * \\brief Handle a \\a prepare operation\n>> + */\n>> +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)\n>> +{\n>> +\tuint32_t activeAutoExposure = state.automatic.exposure;\n>> +\tdouble activeAutoGain = state.automatic.gain;\n>> +\tdouble activeAutoQGain = state.automatic.quantizationGain;\n>> +\n>> +\t/* Populate exposure and gain in auto mode */\n>> +\tif (frameContext.autoExposureEnabled) {\n>> +\t\tframeContext.exposure = activeAutoExposure;\n>> +\t\tframeContext.quantizationGain = activeAutoQGain;\n>> +\t}\n>> +\tif (frameContext.autoGainEnabled) {\n>> +\t\tframeContext.gain = activeAutoGain;\n>> +\t\tframeContext.quantizationGain = activeAutoQGain;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Populate manual exposure and gain from the active auto values when\n>> +\t * transitioning from auto to manual\n>> +\t */\n> \n> I guess this answers to my question on the auto->manual transition\n> tracking for the frameContext.autoExposureModeChange flag.\n> \n> It tracks both state changes, but we use it to propagate auto values\n> to the manual state if they have not been overriden by an application\n> provide control\n\nAs mentioned above, I think it very specifically only tracks those auto->manual\ntransition that don't have an associated manual `ExposureTime`.\n\n\n> \n>> +\tif (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {\n>> +\t\tstate.manual.exposure = activeAutoExposure;\n>> +\t\tframeContext.exposure = activeAutoExposure;\n>> +\t}\n>> +\tif (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {\n>> +\t\tstate.manual.gain = activeAutoGain;\n>> +\t\tframeContext.gain = activeAutoGain;\n>> +\t\tframeContext.quantizationGain = activeAutoQGain;\n>> +\t}\n>> +\n>> +\tframeContext.yTarget = state.automatic.yTarget;\n>> +}\n> \n> Ack, this matches RkISP1Agc::prepare()\n> \n>> +\n>> +/**\n>> + * \\brief Handle a \\a process operation\n>> + */\n>> +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,\n>> +\t\t\t agc::FrameContext &frameContext, std::optional &¶ms,\n>> +\t\t\t ControlList &metadata)\n>> +{\n>> +\tconst utils::Duration &lineDuration = session.lineDuration;\n>> +\tutils::Duration newExposureTime = {};\n>> +\n>> +\tif (params) {\n> \n> I presume (well, you told me) that this matches the\n> \n> if (!stats)\n> \n> case in RkISP1.\n> \n> I wonder, should\n> \n> The RkISP1 IPA does\n> \n> \tif (!stats) {\n> \t\tprocessFrameDuration(context, frameContext,\n> \t\t\t\t frameContext.agc.minFrameDuration);\n> \t\tfillMetadata(context, frameContext, metadata);\n> \t\treturn;\n> \t}\n> \n> I guess it's fine for IPA to call AgcAlgorithm::process() even when\n> !stats, as it is simpler for them to make the call unconditionally, so\n> I'm not against this.\n> \n> But maybe we can do like RkISP1 does (split processFrameDuration() and\n> fillMetadata() to helpers) to save some indentation here.\n\nMy preference would be not to add back those separate functions. Maybe\nmoving the `if (params) { ... }` block into a separate function if\nthat is acceptable.\n\n\n> \n>> +\t\tASSERT(session.autoAllowed);\n> \n> Does it mean IPA should not call process if (autoAllowed) ?\n\nYes.\n\n\n> \n> Can't we simply bail out here to make IPAs simpler ?\n\nPossibly, but I don't see how that would make them simpler. The assumption\nis that `autoAllowed == false` means that statistics will not be available\n(and hence no automatic control) (usually for raw capture - but there is no\ntechnical reason why agc could not run even for raw capture if statistics\nare available), so providing statistics is an api violation in a sense, so\nI think the assert is reasonable.\n\n\n> \n>> +\n>> +\t\t/*\n>> +\t\t* Set the AGC limits using the fixed exposure time and/or gain in\n>> +\t\t* manual mode, or the sensor limits in auto mode.\n>> +\t\t*/\n> \n> Weird indent\n\nFixed.\n\n> \n>> +\t\tutils::Duration minExposureTime;\n>> +\t\tutils::Duration maxExposureTime;\n>> +\t\tdouble minAnalogueGain;\n>> +\t\tdouble maxAnalogueGain;\n>> +\n>> +\t\tif (frameContext.autoExposureEnabled) {\n>> +\t\t\tminExposureTime = session.minExposureTime;\n>> +\t\t\tmaxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);\n> \n> very long line\n\nAdjusted.\n\n\n> \n>> +\t\t} else {\n>> +\t\t\tminExposureTime = lineDuration * frameContext.exposure;\n>> +\t\t\tmaxExposureTime = minExposureTime;\n>> +\t\t}\n>> +\n>> +\t\tif (frameContext.autoGainEnabled) {\n>> +\t\t\tminAnalogueGain = session.minAnalogueGain;\n>> +\t\t\tmaxAnalogueGain = session.maxAnalogueGain;\n>> +\t\t} else {\n>> +\t\t\tminAnalogueGain = frameContext.gain;\n>> +\t\t\tmaxAnalogueGain = frameContext.gain;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t* The Agc algorithm needs to know the effective exposure value that was\n>> +\t\t* applied to the sensor when the statistics were collected.\n>> +\t\t*/\n>> +\t\tutils::Duration effectiveExposureValue =\n>> +\t\t\tlineDuration * params->exposure * params->gain;\n> \n> RkISP1 gets exposure and gain from the frameContext.\n> \n> I see in the next patches that port RkISP1 to AgcAlgorithm this hunk\n> \n> agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc,\n> \n> ...\n> \n> .exposure = frameContext.sensor.exposure,\n> /*\n> * Include the quantization gain if it was applied. Do not use\n> * compress.gain because it will include gains that shall not be\n> * reported to the user when HDR is implemented.\n> */\n> .gain = frameContext.sensor.gain\n> * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),\n> \n> Is the reason you can't take exposure and gain from the FrameContext\n> the gain adjustment ?\n\nThat, and the fact that it is `frameContext.sensor`, which is separate\nfrom `frameContext.agc` (i.e. `agc::FrameContext`), because it needs\nthe exposure/gain that was actually in effect.\n\n\n> \n>> +\n>> +\t\timpl_.setLimits(minExposureTime, maxExposureTime,\n>> +\t\t\t\tminAnalogueGain, maxAnalogueGain,\n>> +\t\t\t\tstd::move(params->additionalConstraints));\n>> +\n>> +\t\tconst auto &newEv = impl_.calculateNewEv({\n>> +\t\t\t.traits = params->traits,\n>> +\t\t\t.yHist = params->yHist,\n>> +\t\t\t.effectiveExposureValue = effectiveExposureValue,\n>> +\t\t\t.constraintModeIndex = frameContext.constraintMode,\n>> +\t\t\t.exposureModeIndex = frameContext.exposureMode,\n>> +\t\t\t.lux = params->lux,\n>> +\t\t\t.exposureCompensation = pow(2.0, frameContext.exposureValue),\n>> +\t\t});\n>> +\n>> +\t\tLOG(Agc, Debug)\n>> +\t\t\t<< \"Divided up exposure time, analogue gain, quantization gain\"\n>> +\t\t\t<< \" and digital gain are \" << newEv.exposureTime << \", \" << newEv.analogueGain\n>> +\t\t\t<< \", \" << newEv.quantizationGain << \" and \" << newEv.digitalGain;\n>> +\n>> +\t\t/* Update the estimated exposure and gain. */\n>> +\t\tstate.automatic.exposure = newEv.exposureTime / lineDuration;\n>> +\t\tstate.automatic.gain = newEv.analogueGain;\n>> +\t\tstate.automatic.quantizationGain = newEv.quantizationGain;\n>> +\t\tstate.automatic.yTarget = newEv.yTarget;\n>> +\n>> +\t\tnewExposureTime = newEv.exposureTime;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Expand the target frame duration so that we do not run faster than\n>> +\t * the minimum frame duration when we have short exposures.\n>> +\t */\n>> +\tconst auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);\n>> +\tframeContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;\n>> +\n>> +\t/* Update frame duration accounting for line length quantization. */\n>> +\tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;\n> \n> Easy to shorten to\n> \n> \tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank)\n> \t\t\t\t * lineDuration;\n> \n\nThis was also copied verbatim.\n\n\n>> +\n>> +\tmetadata.set(controls::AnalogueGain, frameContext.gain);\n>> +\tmetadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get());\n> \n> I'm not sure I understand the strategy here.. this line is very long\n> and could be easily made\n> \n> \tmetadata.set(controls::ExposureTime,\n> \t\t utils::Duration(lineDuration * frameContext.exposure).get());\n> \n\nAdjusted.\n\n\n> \n>> +\tmetadata.set(controls::FrameDuration, frameContext.frameDuration.get());\n>> +\tmetadata.set(controls::ExposureTimeMode,\n>> +\t\t frameContext.autoExposureEnabled\n>> +\t\t ? controls::ExposureTimeModeAuto\n>> +\t\t : controls::ExposureTimeModeManual);\n>> +\tmetadata.set(controls::AnalogueGainMode,\n>> +\t\t frameContext.autoGainEnabled\n>> +\t\t ? controls::AnalogueGainModeAuto\n>> +\t\t : controls::AnalogueGainModeManual);\n> \n> These ones instead are very short and could easily be made\n> \n> \tmetadata.set(controls::ExposureTimeMode, frameContext.autoExposureEnabled ?\n> \t\t controls::ExposureTimeModeAuto : controls::ExposureTimeModeManual);\n\n\nThese were copied verbatim. So please confirm if you want me to adjust the\nverbatim copies here and in the whole file.\n\n\n> \n>> +\n>> +\tmetadata.set(controls::AeExposureMode, frameContext.exposureMode);\n>> +\tmetadata.set(controls::AeConstraintMode, frameContext.constraintMode);\n>> +\tmetadata.set(controls::ExposureValue, frameContext.exposureValue);\n>> +}\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h\n>> index 5247425952..1eace12908 100644\n>> --- a/src/ipa/libipa/agc.h\n>> +++ b/src/ipa/libipa/agc.h\n>> @@ -7,13 +7,19 @@\n>>\n>> #pragma once\n>>\n>> +#include \n>> #include \n>>\n>> #include \n>>\n>> +#include \n>> #include \n>>\n>> +#include \n>> +\n>> +#include \"agc_mean_luminance.h\"\n>> #include \"camera_sensor_helper.h\"\n>> +#include \"histogram.h\"\n>>\n>> namespace libcamera {\n>>\n>> @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,\n>> \tcontrols.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));\n>> }\n>>\n>> +struct Session {\n>> +\tutils::Duration minExposureTime;\n>> +\tutils::Duration maxExposureTime;\n>> +\tdouble minAnalogueGain;\n>> +\tdouble maxAnalogueGain;\n>> +\tutils::Duration minFrameDuration;\n>> +\tutils::Duration maxFrameDuration;\n>> +\n>> +\tutils::Duration lineDuration;\n>> +\n>> +\tstruct {\n>> +\t\tSize outputSize;\n>> +\t} sensor;\n>> +\n>> +\tbool autoAllowed;\n>> +};\n>> +\n>> +struct ActiveState {\n>> +\tstruct {\n>> +\t\tuint32_t exposure;\n>> +\t\tdouble gain;\n>> +\t} manual;\n>> +\tstruct {\n>> +\t\tuint32_t exposure;\n>> +\t\tdouble gain;\n>> +\t\tdouble quantizationGain;\n>> +\t\tdouble yTarget;\n>> +\t} automatic;\n>> +\n>> +\tbool autoExposureEnabled;\n>> +\tbool autoGainEnabled;\n>> +\tdouble exposureValue;\n>> +\tcontrols::AeConstraintModeEnum constraintMode;\n>> +\tcontrols::AeExposureModeEnum exposureMode;\n>> +\tutils::Duration minFrameDuration;\n>> +\tutils::Duration maxFrameDuration;\n>> +};\n>> +\n>> +struct FrameContext {\n>> +\tuint32_t exposure;\n>> +\tdouble gain;\n>> +\tdouble quantizationGain;\n>> +\tdouble exposureValue;\n>> +\tdouble yTarget;\n>> +\tuint32_t vblank;\n>> +\tbool autoExposureEnabled;\n>> +\tbool autoGainEnabled;\n>> +\tcontrols::AeConstraintModeEnum constraintMode;\n>> +\tcontrols::AeExposureModeEnum exposureMode;\n>> +\tutils::Duration minFrameDuration;\n>> +\tutils::Duration maxFrameDuration;\n>> +\tutils::Duration frameDuration;\n>> +\tbool autoExposureModeChange;\n>> +\tbool autoGainModeChange;\n>> +};\n>> +\n>> } /* namespace agc */\n>>\n>> +class AgcAlgorithm\n>> +{\n>> +public:\n>> +\tstruct ConfigurationParams {\n>> +\t\tconst CameraSensorHelper *sensor;\n>> +\t\tconst IPACameraSensorInfo &sensorInfo;\n>> +\t\tconst ControlInfoMap &sensorControls;\n>> +\t\tControlInfoMap::Map &ctrlMap;\n>> +\t\tbool autoAllowed = true;\n>> +\t};\n>> +\n>> +\tint init(const ValueNode &tuningData);\n>> +\n>> +\tint configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);\n> \n> easy to shorten\n\nDone.\n\n\n> \n>> +\n>> +\tvoid queueRequest(const agc::Session &session, agc::ActiveState &state,\n>> +\t\t\t agc::FrameContext &frameContext, const ControlList &controls);\n>> +\n>> +\tvoid prepare(agc::ActiveState &state, agc::FrameContext &frameContext);\n>> +\n>> +\tstruct ProcessParams {\n>> +\t\tconst AgcMeanLuminance::Traits &traits;\n>> +\t\tconst Histogram &yHist;\n>> +\t\tuint32_t exposure;\n>> +\t\tdouble gain;\n>> +\t\tstd::vector &&additionalConstraints = {};\n>> +\t\tdouble lux = 0;\n>> +\t};\n>> +\n>> +\tvoid process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,\n> \n> same here\n\nDone.\n\n\n> \n>> +\t\t std::optional &¶ms, ControlList &metadata);\n>> +\n>> +private:\n>> +\tAgcMeanLuminance impl_;\n>> +};\n>> +\n>> } /* namespace ipa */\n>>\n>> } /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>> index 05f1a8749c..ca681fa5af 100644\n>> --- a/src/ipa/libipa/meson.build\n>> +++ b/src/ipa/libipa/meson.build\n>> @@ -23,6 +23,7 @@ libipa_headers = files([\n>> ])\n>>\n>> libipa_sources = files([\n>> + 'agc.cpp',\n>> 'agc_mean_luminance.cpp',\n>> 'algorithm.cpp',\n>> 'awb_bayes.cpp',\n> \n> Enormous work overall!\n> \n> Agc is the most complex beast we have and I'm looking forwards to\n> getting this merged already!\n> \n> \n>> --\n>> 2.55.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5D1B5BDE4C\n\tfor ;\n\tMon, 27 Jul 2026 08:53:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5632F67F61;\n\tMon, 27 Jul 2026 10:53:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3616C67F4C\n\tfor ;\n\tMon, 27 Jul 2026 10:53:37 +0200 (CEST)","from [192.168.33.46] (185.182.215.156.nat.pool.zt.hu\n\t[185.182.215.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58930492;\n\tMon, 27 Jul 2026 10:52:33 +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=\"t7inaC3a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785142353;\n\tbh=dacUl9zdIq+TgPStPEPeMkhpIDV5Zqfn1pI7MF7cmZQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=t7inaC3aivOei7JkY5yn3KzeCFhGmoU8NAfcIdqL8zsR3954PjqgMzovx689z1vTE\n\tQwbo1zrcMbmtXhqlT8QpZg6xTCoL3F4pKp10rrmfl37cgB8hqfYVpzwzPVQJtLbekn\n\t8cFJP4PaTKJHEKkVYenKTNZHWcqU7sGmTnSe7hu0=","Message-ID":"","Date":"Mon, 27 Jul 2026 10:53:33 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-22-barnabas.pocze@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":39872,"web_url":"https://patchwork.libcamera.org/comment/39872/","msgid":"","date":"2026-07-27T13:25:20","subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Mon, Jul 27, 2026 at 10:53:33AM +0200, Barnabás Pőcze wrote:\n> 2026. 07. 24. 17:01 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:\n> > > Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`\n> > > based on the rkisp1 `Agc` algorithm, with slight adjustments.\n> > >\n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > src/ipa/libipa/agc.cpp | 639 +++++++++++++++++++++++++++++++++++++\n> > > src/ipa/libipa/agc.h | 98 ++++++\n> > > src/ipa/libipa/meson.build | 1 +\n> > > 3 files changed, 738 insertions(+)\n> > > create mode 100644 src/ipa/libipa/agc.cpp\n> > >\n> > > diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n> > > new file mode 100644\n> > > index 0000000000..e16a02fdde\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/agc.cpp\n> > > @@ -0,0 +1,639 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021-2022, 2026 Ideas On Board\n> > > + *\n> > > + * Generic AGC algorithm\n> > > + */\n> > > +\n> > > +#include \"agc.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa {\n> > > +\n> > > +using namespace std::chrono_literals;\n> > > +\n> > > +LOG_DEFINE_CATEGORY(Agc)\n> > > +\n> > > +/**\n> > > + * \\class AgcAlgorithm\n> > > + * \\brief AgcMeanLuminance wrapper for implementing the Algorithm interface\n> > > + *\n> > > + * \\todo DigitalGain, DigitalGainMode\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\struct agc::Session\n> > > + * \\brief Session configuration for AgcAlgorithm\n> > > + *\n> > > + * \\var agc::Session::minExposureTime\n> > > + * \\brief Minimum exposure time supported with the configured sensor\n> > > + *\n> > > + * \\var agc::Session::maxExposureTime\n> > > + * \\brief Maximum exposure time supported with the configured sensor\n> > > + *\n> > > + * \\var agc::Session::minAnalogueGain\n> > > + * \\brief Minimum analogue gain supported with the configured sensor\n> > > + *\n> > > + * \\var agc::Session::maxAnalogueGain\n> > > + * \\brief Maximum analogue gain supported with the configured sensor\n> > > + *\n> > > + * \\var agc::Session::minFrameDuration\n> > > + * \\brief Minimum frame duration supported with the configured sensor\n> > > + *\n> > > + * \\var agc::Session::maxFrameDuration\n> > > + * \\brief Maximum frame duration supported with the configured sensor\n> >\n> > I would use \"for the streaming session\" in place of \"with the\n> > configured sensor\" in all previous entries.\n> >\n> > yes, those settings depend on the sensor configuration but I found the\n> > wording a bit confusing\n> >\n> > Or maybe \"for the current sensor configuration\" ?\n>\n> I'll drop \"supported\" and use \"for the streaming session\".\n>\n>\n\nthanks\n\n> >\n> > > + *\n> > > + * \\var agc::Session::lineDuration\n> > > + * \\brief Line duration with the configured sensor and output size\n> > > + *\n> > > + * \\var agc::Session::sensor\n> > > + * \\brief Details of the sensor configuration\n> > > + *\n> > > + * \\var agc::Session::sensor.outputSize\n> > > + * \\brief Configured output size of the sensor\n> > > + *\n> > > + * \\var agc::Session::autoAllowed\n> > > + * \\brief Whether automatic controls are allowed\n> >\n> > As I was really confused by this, is it worth adding one line saying\n> > that auto is not allowed when capturing raw frames ?\n>\n> I suppose if statistics are provided, then it can work for raw capture as well.\n> So my suggestion:\n>\n> \\brief Whether automatic controls are allowed\n> Determines whether statistics are generally expected to be available for\n\ns/generally//\n\n> automatic exposure and gain control. If \\a false, statistics must not\n> be provided to \\a AgcAlgorithm::process(), and \\a ExposureTimeMode and\n> \\a AnalogueGainMode will only avertise manual control, and multiple other\n> controls will also be omitted.\n\nOk.\n\nI would however drop everything after ', and multiple ..\"\nbecause either we list them or this might sound vague\n\n>\n>\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\struct agc::ActiveState\n> > > + * \\brief Active state for AgcAlgorithm\n> > > + *\n> > > + * The \\a automatic variables track the latest values computed by algorithm\n> > > + * based on the latest processed statistics. All other variables track the\n> > > + * consolidated controls requested in queued requests.\n> > > + *\n> > > + * \\var agc::ActiveState::manual\n> > > + * \\brief Manual exposure time and analog gain (set through requests)\n> > > + *\n> > > + * \\var agc::ActiveState::manual.exposure\n> > > + * \\brief Manual exposure time expressed as a number of lines as set by the\n> > > + * ExposureTime control\n> > > + *\n> > > + * \\var agc::ActiveState::manual.gain\n> > > + * \\brief Manual analogue gain as set by the AnalogueGain control\n> > > + *\n> > > + * \\var agc::ActiveState::automatic\n> > > + * \\brief Automatic exposure time and analog gain (computed by the algorithm)\n> > > + *\n> > > + * \\var agc::ActiveState::automatic.exposure\n> > > + * \\brief Automatic exposure time expressed as a number of lines\n> > > + *\n> > > + * \\var agc::ActiveState::automatic.gain\n> > > + * \\brief Automatic analogue gain multiplier\n> > > + *\n> > > + * \\var agc::ActiveState::automatic.quantizationGain\n> > > + * \\brief Automatic quantization gain multiplier\n> > > + *\n> > > + * \\var agc::ActiveState::automatic.yTarget\n> > > + * \\brief Automatically determined luminance target\n> > > + *\n> > > + * \\var agc::ActiveState::autoExposureEnabled\n> > > + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n> >\n> > s/AGC state (exposure)/exposure mode\n> >\n> > > + *\n> > > + * \\var agc::ActiveState::autoGainEnabled\n> > > + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n> >\n> > same here ?\n>\n> These were copied verbatim.\n>\n\nI don't think we should stick too strictly to what's in rkisp1\nespecially when it's about documentation which can so easily be\nimproved\n\n>\n> >\n> > > + *\n> > > + * \\var agc::ActiveState::exposureValue\n> > > + * \\brief Exposure value as set by the ExposureValue control\n> > > + *\n> > > + * \\var agc::ActiveState::constraintMode\n> > > + * \\brief Constraint mode as set by the AeConstraintMode control\n> > > + *\n> > > + * \\var agc::ActiveState::exposureMode\n> > > + * \\brief Exposure mode as set by the AeExposureMode control\n> > > + *\n> > > + * \\var agc::ActiveState::minFrameDuration\n> > > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > > + *\n> > > + * \\var agc::ActiveState::maxFrameDuration\n> > > + * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\struct agc::FrameContext\n> > > + * \\brief Per-frame context for AgcAlgorithm\n> > > + *\n> > > + * \\var agc::FrameContext::exposure\n> > > + * \\brief Exposure time expressed as a number of lines computed by the algorithm\n> > > + *\n> > > + * \\var agc::FrameContext::gain\n> > > + * \\brief Analogue gain multiplier computed by the algorithm\n> >\n> > Are these computed by the algorithms or can these be the manually\n> > programmed values when running in manual mode ?\n>\n> This is always the gain that needs to be applied to the frame, so\n> the manual gain in manual mode, the automatic gain in automatic mode.\n>\n\nSo maybe \"computed by the algorithm\" isn't exactly correct ?\n\n\n * \\brief Analogue gain multiplier applied to the frame\n\n?\n\n>\n> >\n> > I would replace \"computed by the algorithm\" with \"for the frame\" or\n> > similar\n> >\n> > > + *\n> > > + * The gain should be adapted to the sensor specific gain code before applying.\n> >\n> > s/adapted/translated ?\n>\n> These were also all copied verbatim, but I'll adjust it.\n>\n>\n> >\n> > > + *\n> > > + * \\var agc::FrameContext::quantizationGain\n> > > + * \\brief Quantization gain multiplier computed by the algorithm\n> > > + *\n> > > + * \\var agc::FrameContext::exposureValue\n> > > + * \\brief Exposure value as set by the ExposureValue control\n> > > + *\n> > > + * \\var agc::FrameContext::yTarget\n> > > + * \\brief Luminance target computed by the algorithm\n> > > + *\n> > > + * \\var agc::FrameContext::vblank\n> > > + * \\brief Vertical blanking parameter computed by the algorithm\n> > > + *\n> > > + * \\var agc::FrameContext::autoExposureEnabled\n> > > + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n> > > + *\n> > > + * \\var agc::FrameContext::autoGainEnabled\n> > > + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n> > > + *\n> > > + * \\var agc::FrameContext::constraintMode\n> > > + * \\brief Constraint mode as set by the AeConstraintMode control\n> > > + *\n> > > + * \\var agc::FrameContext::exposureMode\n> > > + * \\brief Exposure mode as set by the AeExposureMode control\n> > > + *\n> > > + * \\var agc::FrameContext::minFrameDuration\n> > > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > > + *\n> > > + * \\var agc::FrameContext::maxFrameDuration\n> > > + * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > > + *\n> > > + * \\var agc::FrameContext::frameDuration\n> > > + * \\brief The actual FrameDuration used by the algorithm for the frame\n> > > + *\n> > > + * \\var agc::FrameContext::autoExposureModeChange\n> > > + * \\brief Indicate if autoExposureEnabled has changed from true in the previous\n> > > + * frame to false in the current frame, and no manual exposure value has been\n> > > + * supplied in the current frame.\n> >\n> > does this only track auto->manual transaction or does it track the\n> > other way around.\n> >\n> > Kind of a long brief (and no '.' at the end).\n>\n> These were also copied verbatim, but I have removed the dots.\n>\n>\n> >\n> > I would\n> >\n> > \\brief Exposure mode change flag\n> >\n> > Indicate if the exposure mode has changed compared to the\n> > previous frame.\n>\n> It only tracks auto -> manual transitions without manual `ExposureTime`,\n> so I think the above wouldn't be entirely correct.\n>\n\nYeah, you're right\n\n if (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))\n frameContext.autoExposureModeChange = true;\n\nThe purpose of the flag is to keep track if the auto value should be\npropagated to manual, so it's a bit of an unfortunate name. However,\nnot something that should be changed in this patch\n\n>\n> >\n> > > + *\n> > > + * \\var agc::FrameContext::autoGainModeChange\n> > > + * \\brief Indicate if autoGainEnabled has changed from true in the previous\n> > > + * frame to false in the current frame, and no manual gain value has been\n> > > + * supplied in the current frame.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\struct AgcAlgorithm::ConfigurationParams\n> > > + * \\brief Parameters for AgcAlgorithm::configure()\n> > > + *\n> > > + * \\var AgcAlgorithm::ConfigurationParams::sensor\n> > > + * \\brief CameraSensorHelper for the sensor\n> > > + *\n> > > + * \\var AgcAlgorithm::ConfigurationParams::sensorInfo\n> > > + * \\brief Details of the sensor\n> >\n> > The sensor configuration description\n>\n> I have adjusted it to\n>\n> Current configuration of the sensor\n>\n> hope it is also acceptable.\n>\n\nIt is indeed\n\n>\n> >\n> > > + *\n> > > + * \\var AgcAlgorithm::ConfigurationParams::sensorControls\n> > > + * \\brief ControlInfoMap of the sensor\n> > > + *\n> > > + * \\var AgcAlgorithm::ConfigurationParams::ctrlMap\n> > > + * \\brief ControlMap to update with controls\n> > > + *\n> > > + * \\var AgcAlgorithm::ConfigurationParams::autoAllowed\n> > > + * \\brief Whether to enable auto controls\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\struct AgcAlgorithm::ProcessParams\n> > > + * \\brief Parameters for AgcAlgorithm::process()\n> > > + *\n> > > + * \\var AgcAlgorithm::ProcessParams::traits\n> > > + * \\brief Implementation of AgcMeanLuminance::Traits\n> > > + *\n> > > + * \\var AgcAlgorithm::ProcessParams::yHist\n> > > + * \\brief Luminance histogram of the frame\n> > > + *\n> > > + * \\var AgcAlgorithm::ProcessParams::exposure\n> > > + * \\brief Effective exposure of the frame\n> > > + *\n> > > + * \\var AgcAlgorithm::ProcessParams::gain\n> > > + * \\brief Effective gain of the frame\n> > > + *\n> > > + * \\var AgcAlgorithm::ProcessParams::additionalConstraints\n> > > + * \\brief Additional AgcMeanLuminance::AgcConstraints to apply\n> > > + *\n> > > + * \\var AgcAlgorithm::ProcessParams::lux\n> > > + * \\brief Effective lux value of the frame\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Load tuning data\n> > > + */\n> > > +int AgcAlgorithm::init(const ValueNode &tuningData)\n> > > +{\n> > > +\tint ret = impl_.parseTuningData(tuningData);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Initialize the session configuration and active state\n> > > + */\n> > > +int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config)\n> >\n> > Easy to shorten to\n> >\n> > int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,\n> > \t\t\t const ConfigurationParams &config)\n>\n> Done.\n>\n>\n> >\n> >\n> > Seems like session is an out parameter.\n> >\n> > We used to have a rule: const & for input params, * for output ones.\n> > Not sure anymore how much we enforce it these days..\n>\n> I'm not sure I would consider it an out param in the traditional sense\n> (i.e. the caller is not really expected to use/inspect the result), and\n> I really prefer the consistency of always passing by reference to every\n> function of `AgcAlgorithm`.\n>\n\nOk, as said, I don't exactly know how much that rule is even enforced\nnowadays\n\n>\n> >\n> > > +{\n> > > +\tsession = {};\n> > > +\tsession.lineDuration = config.sensorInfo.minLineLength * 1.0s\n> > > +\t\t/ config.sensorInfo.pixelRate;\n> > > +\tsession.sensor.outputSize = config.sensorInfo.outputSize;\n> > > +\tsession.autoAllowed = config.autoAllowed;\n> > > +\n> > > +\tconst double lineDurationUs = session.lineDuration.get();\n> > > +\n> > > +\t/*\n> > > +\t * Compute exposure time limits from the V4L2_CID_EXPOSURE control\n> > > +\t * limits and the line duration.\n> > > +\t */\n> > > +\n> > > +\tconst ControlInfo &v4l2Exposure = config.sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > > +\tint32_t minExposure = v4l2Exposure.min().get();\n> > > +\tint32_t maxExposure = v4l2Exposure.max().get();\n> > > +\tint32_t defExposure = v4l2Exposure.def().get();\n> > > +\tconfig.ctrlMap[&controls::ExposureTime] = ControlInfo{\n> > > +\t\tstatic_cast(minExposure * lineDurationUs),\n> > > +\t\tstatic_cast(maxExposure * lineDurationUs),\n> > > +\t\tstatic_cast(defExposure * lineDurationUs),\n> > > +\t};\n> > > +\n> > > +\t/* Compute the analogue gain limits. */\n> > > +\tconst ControlInfo &v4l2Gain = config.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> > > +\tfloat minGain = config.sensor->gain(v4l2Gain.min().get());\n> > > +\tfloat maxGain = config.sensor->gain(v4l2Gain.max().get());\n> > > +\tfloat defGain = config.sensor->gain(v4l2Gain.def().get());\n> > > +\tconfig.ctrlMap[&controls::AnalogueGain] = ControlInfo{\n> > > +\t\tminGain,\n> > > +\t\tmaxGain,\n> > > +\t\tdefGain,\n> > > +\t};\n> > > +\n> > > +\tLOG(Agc, Debug)\n> > > +\t\t<< \"Exposure: [\" << minExposure << \", \" << maxExposure\n> > > +\t\t<< \"], gain: [\" << minGain << \", \" << maxGain << \"]\";\n> > > +\n> > > +\t/*\n> > > +\t * Compute the frame duration limits.\n> > > +\t *\n> > > +\t * The frame length is computed assuming a fixed line length combined\n> > > +\t * with the vertical frame sizes.\n> > > +\t */\n> > > +\tconst ControlInfo &v4l2HBlank = config.sensorControls.find(V4L2_CID_HBLANK)->second;\n> > > +\tuint32_t hblank = v4l2HBlank.def().get();\n> > > +\tuint32_t lineLength = config.sensorInfo.outputSize.width + hblank;\n> > > +\n> > > +\tconst ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;\n> > > +\tstd::array frameHeights{\n> > > +\t\tv4l2VBlank.min().get() + config.sensorInfo.outputSize.height,\n> > > +\t\tv4l2VBlank.max().get() + config.sensorInfo.outputSize.height,\n> > > +\t\tv4l2VBlank.def().get() + config.sensorInfo.outputSize.height,\n> > > +\t};\n> > > +\n> > > +\tstd::array frameDurations;\n> > > +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > > +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > > +\t\tframeDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U);\n> > > +\t}\n> > > +\n> > > +\tconfig.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{\n> > > +\t\tframeDurations[0],\n> > > +\t\tframeDurations[1],\n> > > +\t\tSpan{ { frameDurations[2], frameDurations[2] } },\n> > > +\t};\n> > > +\n> > > +\tsession.minFrameDuration = std::chrono::microseconds(frameDurations[0]);\n> > > +\tsession.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);\n> > > +\n> > > +\t/*\n> > > +\t * When the AGC computes the new exposure values for a frame, it needs\n> > > +\t * to know the limits for exposure time and analogue gain. As it depends\n> > > +\t * on the sensor, update it with the controls.\n> > > +\t *\n> > > +\t * \\todo take VBLANK into account for maximum exposure time\n> > > +\t */\n> > > +\tsession.minExposureTime = minExposure * session.lineDuration;\n> > > +\tsession.maxExposureTime = maxExposure * session.lineDuration;\n> > > +\tsession.minAnalogueGain = minGain;\n> > > +\tsession.maxAnalogueGain = maxGain;\n> >\n> > The above matches the current RkISP1Agc::reconfigure() content\n> >\n> > > +\n> > > +\timpl_.configure(session.lineDuration, config.sensor);\n> > > +\timpl_.setLimits(session.minExposureTime, session.maxExposureTime,\n> > > +\t\t\tsession.minAnalogueGain, session.maxAnalogueGain,\n> > > +\t\t\t{});\n> > > +\timpl_.resetFrameCount();\n> > > +\n> > > +\t/* Configure the default exposure and gain. */\n> > > +\tstate = {};\n> > > +\tstate.automatic.gain = session.minAnalogueGain;\n> > > +\tstate.automatic.exposure = 10ms / session.lineDuration;\n> > > +\tstate.automatic.quantizationGain = 1;\n> > > +\tstate.automatic.yTarget = impl_.effectiveYTarget(0, 1);\n> >\n> > And this matches RkISP1::Agc() apart from this early yTarget\n> > computation which I presume is intentional\n>\n> Hmmm... that should match as well. There is\n>\n> context.activeState.agc.automatic.yTarget = agc_.effectiveYTarget(0, 1);\n>\n> at the end of `rkisp1.cpp:Agc::configure()`.\n\nAh yes, it's at the end of the function indeed\n\n>\n>\n> >\n> > > +\tstate.manual.gain = state.automatic.gain;\n> > > +\tstate.manual.exposure = state.automatic.exposure;\n> > > +\tstate.autoExposureEnabled = session.autoAllowed;\n> > > +\tstate.autoGainEnabled = session.autoAllowed;\n> > > +\tstate.exposureValue = 0;\n> > > +\n> > > +\tstate.constraintMode =\n> > > +\t\tstatic_cast(impl_.constraintModes().begin()->first);\n> > > +\tstate.exposureMode =\n> > > +\t\tstatic_cast(impl_.exposureModeHelpers().begin()->first);\n> > > +\n> > > +\tstate.minFrameDuration = session.minFrameDuration;\n> > > +\tstate.maxFrameDuration = session.maxFrameDuration;\n> > > +\n> > > +\tconst auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) {\n> > > +\t\tstd::array values;\n> > > +\t\tsize_t count = 0;\n> > > +\n> > > +\t\tif (session.autoAllowed)\n> > > +\t\t\tvalues[count++] = ControlValue(automatic);\n> > > +\n> > > +\t\tvalues[count++] = ControlValue(manual);\n> > > +\n> > > +\t\tconfig.ctrlMap[&cid] = ControlInfo{\n> > > +\t\t\t{ values.data(), count },\n> > > +\t\t\tControlValue(session.autoAllowed ? automatic : manual),\n> > > +\t\t};\n> > > +\t};\n> > > +\n> > > +\tadd(controls::ExposureTimeMode, controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);\n> > > +\tadd(controls::AnalogueGainMode, controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);\n> >\n> > Could you please break trivially re-adjustable long lines ?\n>\n> Adjusted to\n>\n> add(controls::ExposureTimeMode,\n> controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);\n> add(controls::AnalogueGainMode,\n> controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);\n>\n> Does that look better?\n>\n\nindeed\n\n>\n> >\n> > > +\n> > > +\t/* \\todo Move this to the `Camera` class. */\n> > > +\tconfig.ctrlMap[&controls::AeEnable] = ControlInfo{\n> > > +\t\tfalse,\n> > > +\t\tsession.autoAllowed,\n> > > +\t\tsession.autoAllowed,\n> > > +\t};\n> >\n> > RkISP1Agc register controls in init() but I think what you're doing\n> > here it's better, as some control such as ExposureTimeMode depend on\n> > the session configuration.\n> >\n> > Is there any risk of having leftovers from a previous session in the\n> > control list ?\n>\n> Every control should either be overwritten or removed.\n>\n\nIs that because the Camera::controls info map gets reset between\nconfigure() calls ? I don't really see that happening in Camera or\nPipelineHandler.\n\nSame as per the data->ipaControls_ passed by the RkISP1 pipeline handle\nto the IPA and used to populate Camera::controls().\n\n>\n> >\n> > > +\n> > > +\tif (session.autoAllowed) {\n> > > +\t\tconfig.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);\n> > > +\n> > > +\t\tfor (const auto &[id, info] : impl_.controls())\n> > > +\t\t\tconfig.ctrlMap[id] = info;\n> > > +\t} else {\n> > > +\t\tconfig.ctrlMap.erase(&controls::ExposureValue);\n> > > +\n> > > +\t\tfor (const auto &[id, info] : impl_.controls())\n> > > +\t\t\tconfig.ctrlMap.erase(id);\n> > > +\t}\n> >\n> > Maybe it happens later on, but this is a bit cumbersome. Making\n> > AgcMeanLuminance aware of autoAllowed would avoid this\n>\n> Possibly, but my preference is to keep more control handling out of AgcMeanLuminance.\n> And in any case, there will be a loop like this or equivalent, and\n\nOk, I thought it was AgcMeanLuminance registering ExposureValue but it\nactually is this class doing so it here. What we're erasing are\npossible leftovers from a previous session maybe ?\n\n> I'm not sure it matters too much where it is.\n\nI was bothered by the need to erase, as it seems like a lot of\nbookeeping that could possibile go wrong.\n\nBut if I get this right this is about removing controls from a\nprevious session ? If that's the case should we pass a fresh list of\ncontrols at every ::configure() call to avoid leftovers instead of\nerasing them ?\n\n>\n>\n> >\n> > > +\n> > > +\n> >\n> > Double empty line\n>\n> Fixed.\n>\n>\n> >\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Handle a \\a queueRequest operation\n> > > + */\n> > > +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,\n> > > +\t\t\t\tagc::FrameContext &frameContext, const ControlList &controls)\n> > > +{\n> > > +\tif (session.autoAllowed) {\n> >\n> > Now that you register controls at configure() time and not init()\n> > time, am I correct that if !autoAllowed applications won't be able to select\n> > controls::ExposureTimeModeAuto ?\n>\n> Yes.\n>\n>\n> >\n> > Which makes me wonder if controls::ExposureTimeMode should be\n> > registered at all if !autoAllowed\n>\n> I think that's more of a philosophical question. My preference is to\n> register controls even if they only really have one valid value. Maybe\n> this needs more discussion to arrive at a libcamera-wide policy.\n\nAck, let's ask others\n\n>\n>\n> >\n> > > +\t\tconst auto &aeEnable = controls.get(controls::ExposureTimeMode);\n> > > +\t\tif (aeEnable &&\n> > > +\t\t (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {\n> > > +\t\t\tstate.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);\n> > > +\n> > > +\t\t\tLOG(Agc, Debug)\n> > > +\t\t\t\t<< (state.autoExposureEnabled ? \"Enabling\" : \"Disabling\")\n> > > +\t\t\t\t<< \" AGC (exposure)\";\n> > > +\n> > > +\t\t\t/*\n> > > +\t\t\t * If we go from auto -> manual with no manual control\n> > > +\t\t\t * set, use the last computed value, which we don't\n> > > +\t\t\t * know until prepare() so save this information.\n> > > +\t\t\t *\n> > > +\t\t\t * \\todo Check the previous frame at prepare() time\n> > > +\t\t\t * instead of saving a flag here\n> > > +\t\t\t */\n> > > +\t\t\tif (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))\n> > > +\t\t\t\tframeContext.autoExposureModeChange = true;\n> > > +\t\t}\n> > > +\n> > > +\t\tconst auto &agEnable = controls.get(controls::AnalogueGainMode);\n> >\n> > Same reasoning for AnalogueGainMode\n> >\n> > > +\t\tif (agEnable &&\n> > > +\t\t (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {\n> > > +\t\t\tstate.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);\n> > > +\n> > > +\t\t\tLOG(Agc, Debug)\n> > > +\t\t\t\t<< (state.autoGainEnabled ? \"Enabling\" : \"Disabling\")\n> > > +\t\t\t\t<< \" AGC (gain)\";\n> > > +\t\t\t/*\n> > > +\t\t\t * If we go from auto -> manual with no manual control\n> > > +\t\t\t * set, use the last computed value, which we don't\n> > > +\t\t\t * know until prepare() so save this information.\n> > > +\t\t\t */\n> > > +\t\t\tif (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))\n> > > +\t\t\t\tframeContext.autoGainModeChange = true;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tconst auto &exposure = controls.get(controls::ExposureTime);\n> > > +\tif (exposure && !state.autoExposureEnabled) {\n> > > +\t\tstate.manual.exposure = *exposure * 1.0us / session.lineDuration;\n> > > +\n> > > +\t\tLOG(Agc, Debug)\n> > > +\t\t\t<< \"Set exposure to \" << state.manual.exposure;\n> > > +\t}\n> > > +\n> > > +\tconst auto &gain = controls.get(controls::AnalogueGain);\n> > > +\tif (gain && !state.autoGainEnabled) {\n> > > +\t\tstate.manual.gain = *gain;\n> > > +\n> > > +\t\tLOG(Agc, Debug) << \"Set gain to \" << state.manual.gain;\n> > > +\t}\n> > > +\n> > > +\tframeContext.autoExposureEnabled = state.autoExposureEnabled;\n> > > +\tframeContext.autoGainEnabled = state.autoGainEnabled;\n> > > +\n> > > +\tif (!frameContext.autoExposureEnabled)\n> > > +\t\tframeContext.exposure = state.manual.exposure;\n> > > +\tif (!frameContext.autoGainEnabled)\n> > > +\t\tframeContext.gain = state.manual.gain;\n> > > +\n> > > +\tif (!frameContext.autoExposureEnabled &&\n> > > +\t !frameContext.autoGainEnabled)\n> > > +\t\tframeContext.quantizationGain = 1.0;\n> > > +\n> > > +\tconst auto &exposureMode = controls.get(controls::AeExposureMode);\n> > > +\tif (exposureMode)\n> > > +\t\tstate.exposureMode =\n> > > +\t\t\tstatic_cast(*exposureMode);\n> > > +\tframeContext.exposureMode = state.exposureMode;\n> > > +\n> > > +\tconst auto &constraintMode = controls.get(controls::AeConstraintMode);\n> > > +\tif (constraintMode)\n> > > +\t\tstate.constraintMode =\n> > > +\t\t\tstatic_cast(*constraintMode);\n> > > +\tframeContext.constraintMode = state.constraintMode;\n> > > +\n> > > +\tconst auto &exposureValue = controls.get(controls::ExposureValue);\n> > > +\tif (exposureValue)\n> > > +\t\tstate.exposureValue = *exposureValue;\n> > > +\tframeContext.exposureValue = state.exposureValue;\n> > > +\n> > > +\tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > > +\tif (frameDurationLimits) {\n> > > +\t\t/* Limit the control value to the limits in ControlInfo */\n> > > +\t\tstate.minFrameDuration = std::clamp(\n> > > +\t\t\tstd::chrono::microseconds((*frameDurationLimits).front()),\n> > > +\t\t\tsession.minFrameDuration,\n> > > +\t\t\tsession.maxFrameDuration\n> > > +\t\t);\n> > > +\n> > > +\t\tstate.maxFrameDuration = std::clamp(\n> > > +\t\t\tstd::chrono::microseconds((*frameDurationLimits).back()),\n> > > +\t\t\tsession.minFrameDuration,\n> > > +\t\t\tsession.maxFrameDuration\n> > > +\t\t);\n> > > +\t}\n> > > +\tframeContext.minFrameDuration = state.minFrameDuration;\n> > > +\tframeContext.maxFrameDuration = state.maxFrameDuration;\n> > > +}\n> > > +\n> >\n> > This matches RkISP1Agc::queueRequest() (metering mode handling apart)\n> >\n> > > +/**\n> > > + * \\brief Handle a \\a prepare operation\n> > > + */\n> > > +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)\n> > > +{\n> > > +\tuint32_t activeAutoExposure = state.automatic.exposure;\n> > > +\tdouble activeAutoGain = state.automatic.gain;\n> > > +\tdouble activeAutoQGain = state.automatic.quantizationGain;\n> > > +\n> > > +\t/* Populate exposure and gain in auto mode */\n> > > +\tif (frameContext.autoExposureEnabled) {\n> > > +\t\tframeContext.exposure = activeAutoExposure;\n> > > +\t\tframeContext.quantizationGain = activeAutoQGain;\n> > > +\t}\n> > > +\tif (frameContext.autoGainEnabled) {\n> > > +\t\tframeContext.gain = activeAutoGain;\n> > > +\t\tframeContext.quantizationGain = activeAutoQGain;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Populate manual exposure and gain from the active auto values when\n> > > +\t * transitioning from auto to manual\n> > > +\t */\n> >\n> > I guess this answers to my question on the auto->manual transition\n> > tracking for the frameContext.autoExposureModeChange flag.\n> >\n> > It tracks both state changes, but we use it to propagate auto values\n> > to the manual state if they have not been overriden by an application\n> > provide control\n>\n> As mentioned above, I think it very specifically only tracks those auto->manual\n> transition that don't have an associated manual `ExposureTime`.\n>\n\nYes you're right\n\n>\n> >\n> > > +\tif (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {\n> > > +\t\tstate.manual.exposure = activeAutoExposure;\n> > > +\t\tframeContext.exposure = activeAutoExposure;\n> > > +\t}\n> > > +\tif (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {\n> > > +\t\tstate.manual.gain = activeAutoGain;\n> > > +\t\tframeContext.gain = activeAutoGain;\n> > > +\t\tframeContext.quantizationGain = activeAutoQGain;\n> > > +\t}\n> > > +\n> > > +\tframeContext.yTarget = state.automatic.yTarget;\n> > > +}\n> >\n> > Ack, this matches RkISP1Agc::prepare()\n> >\n> > > +\n> > > +/**\n> > > + * \\brief Handle a \\a process operation\n> > > + */\n> > > +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,\n> > > +\t\t\t agc::FrameContext &frameContext, std::optional &¶ms,\n> > > +\t\t\t ControlList &metadata)\n> > > +{\n> > > +\tconst utils::Duration &lineDuration = session.lineDuration;\n> > > +\tutils::Duration newExposureTime = {};\n> > > +\n> > > +\tif (params) {\n> >\n> > I presume (well, you told me) that this matches the\n> >\n> > if (!stats)\n> >\n> > case in RkISP1.\n> >\n> > I wonder, should\n> >\n> > The RkISP1 IPA does\n> >\n> > \tif (!stats) {\n> > \t\tprocessFrameDuration(context, frameContext,\n> > \t\t\t\t frameContext.agc.minFrameDuration);\n> > \t\tfillMetadata(context, frameContext, metadata);\n> > \t\treturn;\n> > \t}\n> >\n> > I guess it's fine for IPA to call AgcAlgorithm::process() even when\n> > !stats, as it is simpler for them to make the call unconditionally, so\n> > I'm not against this.\n> >\n> > But maybe we can do like RkISP1 does (split processFrameDuration() and\n> > fillMetadata() to helpers) to save some indentation here.\n>\n> My preference would be not to add back those separate functions. Maybe\n> moving the `if (params) { ... }` block into a separate function if\n> that is acceptable.\n\nI guess we're discussing tastes, but I usually get suspicious when\ncode gets to 4 indentation levels and we could easily avoid that\n\n>\n>\n> >\n> > > +\t\tASSERT(session.autoAllowed);\n> >\n> > Does it mean IPA should not call process if (autoAllowed) ?\n>\n> Yes.\n>\n\nWell, not exactly.\n\nThe IPA shall not provide stats if !autoAllowed\n\n>\n> >\n> > Can't we simply bail out here to make IPAs simpler ?\n>\n> Possibly, but I don't see how that would make them simpler. The assumption\n\nI would have liked for IPAs to be able to call ::process() with an\noptionally initialized ProcessParams which in case of !stats\nwould be empty. If the core is running with !autoAllowed it will\nignore the params.\n\nThis doesn't work in case the system is running with autoAllowed but\nfor some reasons you don't have valid stats. RkISP1 seems to assume it\nmight happen you can get bad/corrupted stats, however in practice I'm\nnot sure how it could happen.\n\n\nWhat you have right now (RkISP1 as an example)\n\n\tif (stats) {\n\t\tif (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)\n\t\t\tparams = &stats->params;\n\t\telse\n\t\t\tLOG(RkISP1Agc, Error) << \"AUTOEXP data is missing in statistics\";\n\t}\n\n\tif (params) {\n\t\tstd::vector additionalConstraints;\n\t\tif (context.activeState.wdr.mode != controls::WdrOff)\n\t\t\tadditionalConstraints.push_back(context.activeState.wdr.constraint);\n\n\t\tagc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{\n\t\t\t.traits = AgcTraits{\n\t\t\t\t{ params->ae.exp_mean, context.hw.numAeCells },\n\t\t\t\tmeteringModes_.at(frameContext.agc.meteringMode),\n\t\t\t},\n\t\t\t.yHist = {\n\t\t\t\t/* The lower 4 bits are fractional and meant to be discarded. */\n\t\t\t\t{ params->hist.hist_bins, context.hw.numHistogramBins },\n\t\t\t\t[](uint32_t x) { return x >> 4; },\n\t\t\t},\n\t\t\t.exposure = frameContext.sensor.exposure,\n\t\t\t/*\n\t\t\t * Include the quantization gain if it was applied. Do not use\n\t\t\t * compress.gain because it will include gains that shall not be\n\t\t\t * reported to the user when HDR is implemented.\n\t\t\t */\n\t\t\t.gain = frameContext.sensor.gain\n\t\t\t * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),\n\t\t\t.additionalConstraints = std::move(additionalConstraints),\n\t\t\t.lux = frameContext.lux.lux,\n\t\t}}, metadata);\n\t} else {\n\t\tagc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);\n\t}\n\nRequires the IPA to validate stats and have to distinct calls to\nagc_.process()\n\nwhile\n\n agc::ProcessParams params {};\n if (stats && stats & ... )\n /* populate params. */\n }\n\n agc_.process( ... , ..., params, ... )\n\nis less verbose, but would require the core to bail out in case\n(params.empty() || !autoAllowed)\n\nThis mean you would need a way to distinguish empty param blocks and\nyou can't use the declarative style you have here, which I kind of\nlike as well..\n\nnot sure, I have a feeling IPA could be made simpler, but at the same\ntime this seems more robust\n\n> is that `autoAllowed == false` means that statistics will not be available\n> (and hence no automatic control) (usually for raw capture - but there is no\n> technical reason why agc could not run even for raw capture if statistics\n> are available), so providing statistics is an api violation in a sense, so\n> I think the assert is reasonable.\n>\n\nWe can start strict and see. After all this is not something\napplication could trigger.\n\n>\n> >\n> > > +\n> > > +\t\t/*\n> > > +\t\t* Set the AGC limits using the fixed exposure time and/or gain in\n> > > +\t\t* manual mode, or the sensor limits in auto mode.\n> > > +\t\t*/\n> >\n> > Weird indent\n>\n> Fixed.\n>\n> >\n> > > +\t\tutils::Duration minExposureTime;\n> > > +\t\tutils::Duration maxExposureTime;\n> > > +\t\tdouble minAnalogueGain;\n> > > +\t\tdouble maxAnalogueGain;\n> > > +\n> > > +\t\tif (frameContext.autoExposureEnabled) {\n> > > +\t\t\tminExposureTime = session.minExposureTime;\n> > > +\t\t\tmaxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);\n> >\n> > very long line\n>\n> Adjusted.\n>\n>\n> >\n> > > +\t\t} else {\n> > > +\t\t\tminExposureTime = lineDuration * frameContext.exposure;\n> > > +\t\t\tmaxExposureTime = minExposureTime;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (frameContext.autoGainEnabled) {\n> > > +\t\t\tminAnalogueGain = session.minAnalogueGain;\n> > > +\t\t\tmaxAnalogueGain = session.maxAnalogueGain;\n> > > +\t\t} else {\n> > > +\t\t\tminAnalogueGain = frameContext.gain;\n> > > +\t\t\tmaxAnalogueGain = frameContext.gain;\n> > > +\t\t}\n> > > +\n> > > +\t\t/*\n> > > +\t\t* The Agc algorithm needs to know the effective exposure value that was\n> > > +\t\t* applied to the sensor when the statistics were collected.\n> > > +\t\t*/\n> > > +\t\tutils::Duration effectiveExposureValue =\n> > > +\t\t\tlineDuration * params->exposure * params->gain;\n> >\n> > RkISP1 gets exposure and gain from the frameContext.\n> >\n> > I see in the next patches that port RkISP1 to AgcAlgorithm this hunk\n> >\n> > agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc,\n> >\n> > ...\n> >\n> > .exposure = frameContext.sensor.exposure,\n> > /*\n> > * Include the quantization gain if it was applied. Do not use\n> > * compress.gain because it will include gains that shall not be\n> > * reported to the user when HDR is implemented.\n> > */\n> > .gain = frameContext.sensor.gain\n> > * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),\n> >\n> > Is the reason you can't take exposure and gain from the FrameContext\n> > the gain adjustment ?\n>\n> That, and the fact that it is `frameContext.sensor`, which is separate\n> from `frameContext.agc` (i.e. `agc::FrameContext`), because it needs\n> the exposure/gain that was actually in effect.\n>\n\nAck, too bad, the Agc::process() interface is quite verbose already :(\n\n>\n> >\n> > > +\n> > > +\t\timpl_.setLimits(minExposureTime, maxExposureTime,\n> > > +\t\t\t\tminAnalogueGain, maxAnalogueGain,\n> > > +\t\t\t\tstd::move(params->additionalConstraints));\n> > > +\n> > > +\t\tconst auto &newEv = impl_.calculateNewEv({\n> > > +\t\t\t.traits = params->traits,\n> > > +\t\t\t.yHist = params->yHist,\n> > > +\t\t\t.effectiveExposureValue = effectiveExposureValue,\n> > > +\t\t\t.constraintModeIndex = frameContext.constraintMode,\n> > > +\t\t\t.exposureModeIndex = frameContext.exposureMode,\n> > > +\t\t\t.lux = params->lux,\n> > > +\t\t\t.exposureCompensation = pow(2.0, frameContext.exposureValue),\n> > > +\t\t});\n> > > +\n> > > +\t\tLOG(Agc, Debug)\n> > > +\t\t\t<< \"Divided up exposure time, analogue gain, quantization gain\"\n> > > +\t\t\t<< \" and digital gain are \" << newEv.exposureTime << \", \" << newEv.analogueGain\n> > > +\t\t\t<< \", \" << newEv.quantizationGain << \" and \" << newEv.digitalGain;\n> > > +\n> > > +\t\t/* Update the estimated exposure and gain. */\n> > > +\t\tstate.automatic.exposure = newEv.exposureTime / lineDuration;\n> > > +\t\tstate.automatic.gain = newEv.analogueGain;\n> > > +\t\tstate.automatic.quantizationGain = newEv.quantizationGain;\n> > > +\t\tstate.automatic.yTarget = newEv.yTarget;\n> > > +\n> > > +\t\tnewExposureTime = newEv.exposureTime;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Expand the target frame duration so that we do not run faster than\n> > > +\t * the minimum frame duration when we have short exposures.\n> > > +\t */\n> > > +\tconst auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);\n> > > +\tframeContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;\n> > > +\n> > > +\t/* Update frame duration accounting for line length quantization. */\n> > > +\tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;\n> >\n> > Easy to shorten to\n> >\n> > \tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank)\n> > \t\t\t\t * lineDuration;\n> >\n>\n> This was also copied verbatim.\n>\n>\n> > > +\n> > > +\tmetadata.set(controls::AnalogueGain, frameContext.gain);\n> > > +\tmetadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get());\n> >\n> > I'm not sure I understand the strategy here.. this line is very long\n> > and could be easily made\n> >\n> > \tmetadata.set(controls::ExposureTime,\n> > \t\t utils::Duration(lineDuration * frameContext.exposure).get());\n> >\n>\n> Adjusted.\n>\n>\n> >\n> > > +\tmetadata.set(controls::FrameDuration, frameContext.frameDuration.get());\n> > > +\tmetadata.set(controls::ExposureTimeMode,\n> > > +\t\t frameContext.autoExposureEnabled\n> > > +\t\t ? controls::ExposureTimeModeAuto\n> > > +\t\t : controls::ExposureTimeModeManual);\n> > > +\tmetadata.set(controls::AnalogueGainMode,\n> > > +\t\t frameContext.autoGainEnabled\n> > > +\t\t ? controls::AnalogueGainModeAuto\n> > > +\t\t : controls::AnalogueGainModeManual);\n> >\n> > These ones instead are very short and could easily be made\n> >\n> > \tmetadata.set(controls::ExposureTimeMode, frameContext.autoExposureEnabled ?\n> > \t\t controls::ExposureTimeModeAuto : controls::ExposureTimeModeManual);\n>\n>\n> These were copied verbatim. So please confirm if you want me to adjust the\n> verbatim copies here and in the whole file.\n\nAs said, it's not that we're moving code in a single patch where we\ncan compare the lines moved around in the diff.\n\nThe way I did the review was looking at the code in Agc and RkISP1Agc\nand they are of course different already, so I don't think we should\nstick to verbatim copies when there's something trivially improvable\nlike spanning to the whole line length\n\n>\n>\n> >\n> > > +\n> > > +\tmetadata.set(controls::AeExposureMode, frameContext.exposureMode);\n> > > +\tmetadata.set(controls::AeConstraintMode, frameContext.constraintMode);\n> > > +\tmetadata.set(controls::ExposureValue, frameContext.exposureValue);\n> > > +}\n> > > +\n> > > +} /* namespace ipa */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h\n> > > index 5247425952..1eace12908 100644\n> > > --- a/src/ipa/libipa/agc.h\n> > > +++ b/src/ipa/libipa/agc.h\n> > > @@ -7,13 +7,19 @@\n> > >\n> > > #pragma once\n> > >\n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > +#include \n> > > +\n> > > +#include \"agc_mean_luminance.h\"\n> > > #include \"camera_sensor_helper.h\"\n> > > +#include \"histogram.h\"\n> > >\n> > > namespace libcamera {\n> > >\n> > > @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,\n> > > \tcontrols.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));\n> > > }\n> > >\n> > > +struct Session {\n> > > +\tutils::Duration minExposureTime;\n> > > +\tutils::Duration maxExposureTime;\n> > > +\tdouble minAnalogueGain;\n> > > +\tdouble maxAnalogueGain;\n> > > +\tutils::Duration minFrameDuration;\n> > > +\tutils::Duration maxFrameDuration;\n> > > +\n> > > +\tutils::Duration lineDuration;\n> > > +\n> > > +\tstruct {\n> > > +\t\tSize outputSize;\n> > > +\t} sensor;\n> > > +\n> > > +\tbool autoAllowed;\n> > > +};\n> > > +\n> > > +struct ActiveState {\n> > > +\tstruct {\n> > > +\t\tuint32_t exposure;\n> > > +\t\tdouble gain;\n> > > +\t} manual;\n> > > +\tstruct {\n> > > +\t\tuint32_t exposure;\n> > > +\t\tdouble gain;\n> > > +\t\tdouble quantizationGain;\n> > > +\t\tdouble yTarget;\n> > > +\t} automatic;\n> > > +\n> > > +\tbool autoExposureEnabled;\n> > > +\tbool autoGainEnabled;\n> > > +\tdouble exposureValue;\n> > > +\tcontrols::AeConstraintModeEnum constraintMode;\n> > > +\tcontrols::AeExposureModeEnum exposureMode;\n> > > +\tutils::Duration minFrameDuration;\n> > > +\tutils::Duration maxFrameDuration;\n> > > +};\n> > > +\n> > > +struct FrameContext {\n> > > +\tuint32_t exposure;\n> > > +\tdouble gain;\n> > > +\tdouble quantizationGain;\n> > > +\tdouble exposureValue;\n> > > +\tdouble yTarget;\n> > > +\tuint32_t vblank;\n> > > +\tbool autoExposureEnabled;\n> > > +\tbool autoGainEnabled;\n> > > +\tcontrols::AeConstraintModeEnum constraintMode;\n> > > +\tcontrols::AeExposureModeEnum exposureMode;\n> > > +\tutils::Duration minFrameDuration;\n> > > +\tutils::Duration maxFrameDuration;\n> > > +\tutils::Duration frameDuration;\n> > > +\tbool autoExposureModeChange;\n> > > +\tbool autoGainModeChange;\n> > > +};\n> > > +\n> > > } /* namespace agc */\n> > >\n> > > +class AgcAlgorithm\n> > > +{\n> > > +public:\n> > > +\tstruct ConfigurationParams {\n> > > +\t\tconst CameraSensorHelper *sensor;\n> > > +\t\tconst IPACameraSensorInfo &sensorInfo;\n> > > +\t\tconst ControlInfoMap &sensorControls;\n> > > +\t\tControlInfoMap::Map &ctrlMap;\n> > > +\t\tbool autoAllowed = true;\n> > > +\t};\n> > > +\n> > > +\tint init(const ValueNode &tuningData);\n> > > +\n> > > +\tint configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);\n> >\n> > easy to shorten\n>\n> Done.\n>\n>\n> >\n> > > +\n> > > +\tvoid queueRequest(const agc::Session &session, agc::ActiveState &state,\n> > > +\t\t\t agc::FrameContext &frameContext, const ControlList &controls);\n> > > +\n> > > +\tvoid prepare(agc::ActiveState &state, agc::FrameContext &frameContext);\n> > > +\n> > > +\tstruct ProcessParams {\n> > > +\t\tconst AgcMeanLuminance::Traits &traits;\n> > > +\t\tconst Histogram &yHist;\n> > > +\t\tuint32_t exposure;\n> > > +\t\tdouble gain;\n> > > +\t\tstd::vector &&additionalConstraints = {};\n> > > +\t\tdouble lux = 0;\n> > > +\t};\n> > > +\n> > > +\tvoid process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,\n> >\n> > same here\n>\n> Done.\n>\n>\n> >\n> > > +\t\t std::optional &¶ms, ControlList &metadata);\n> > > +\n> > > +private:\n> > > +\tAgcMeanLuminance impl_;\n> > > +};\n> > > +\n> > > } /* namespace ipa */\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > index 05f1a8749c..ca681fa5af 100644\n> > > --- a/src/ipa/libipa/meson.build\n> > > +++ b/src/ipa/libipa/meson.build\n> > > @@ -23,6 +23,7 @@ libipa_headers = files([\n> > > ])\n> > >\n> > > libipa_sources = files([\n> > > + 'agc.cpp',\n> > > 'agc_mean_luminance.cpp',\n> > > 'algorithm.cpp',\n> > > 'awb_bayes.cpp',\n> >\n> > Enormous work overall!\n> >\n> > Agc is the most complex beast we have and I'm looking forwards to\n> > getting this merged already!\n> >\n> >\n> > > --\n> > > 2.55.0\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 561C6BE080\n\tfor ;\n\tMon, 27 Jul 2026 13:25:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67FE667F69;\n\tMon, 27 Jul 2026 15:25:27 +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 2731067EB2\n\tfor ;\n\tMon, 27 Jul 2026 15:25:26 +0200 (CEST)","from ideasonboard.com (mob-5-90-50-102.net.vodafone.it\n\t[5.90.50.102])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 74F81492;\n\tMon, 27 Jul 2026 15:24:21 +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=\"figUnQgU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785158662;\n\tbh=yqRJvA9ymvZwhgORPv4ZUccBY7m3dbHxU0kQJmHKTcA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=figUnQgU4YYhuisP9t1wNwOvBN9cM3HYeUJONtQM7u9IV16DYT2t4nvCtAbIl5ygy\n\tJDStXC4c99NoynUcrAEKYfmI6Qlv3s7Y8Rgf2e6hWV8UjsVOzM7MGToxg9r+Wyfjc5\n\tijUzKwDCa++cGxbx91K+e7r94N6kovcykfxnkjFs=","Date":"Mon, 27 Jul 2026 15:25:20 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","Message-ID":"","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-22-barnabas.pocze@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":39877,"web_url":"https://patchwork.libcamera.org/comment/39877/","msgid":"","date":"2026-07-27T14:06:31","subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2026. 07. 27. 15:25 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Mon, Jul 27, 2026 at 10:53:33AM +0200, Barnabás Pőcze wrote:\n>> 2026. 07. 24. 17:01 keltezéssel, Jacopo Mondi írta:\n>>> Hi Barnabás\n>>>\n>>> On Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:\n>>>> Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`\n>>>> based on the rkisp1 `Agc` algorithm, with slight adjustments.\n>>>>\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> src/ipa/libipa/agc.cpp | 639 +++++++++++++++++++++++++++++++++++++\n>>>> src/ipa/libipa/agc.h | 98 ++++++\n>>>> src/ipa/libipa/meson.build | 1 +\n>>>> 3 files changed, 738 insertions(+)\n>>>> create mode 100644 src/ipa/libipa/agc.cpp\n>>>>\n>>>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n>>>> new file mode 100644\n>>>> index 0000000000..e16a02fdde\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/libipa/agc.cpp\n>>>> @@ -0,0 +1,639 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2021-2022, 2026 Ideas On Board\n>>>> + *\n>>>> + * Generic AGC algorithm\n>>>> + */\n>>>> +\n>>>> +#include \"agc.h\"\n>>>> +\n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +#include \n>>>> +\n>>>> +#include \n>>>> +\n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +namespace ipa {\n>>>> +\n>>>> +using namespace std::chrono_literals;\n>>>> +\n>>>> +LOG_DEFINE_CATEGORY(Agc)\n>>>> +\n>>>> +/**\n>>>> + * \\class AgcAlgorithm\n>>>> + * \\brief AgcMeanLuminance wrapper for implementing the Algorithm interface\n>>>> + *\n>>>> + * \\todo DigitalGain, DigitalGainMode\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\struct agc::Session\n>>>> + * \\brief Session configuration for AgcAlgorithm\n>>>> + *\n>>>> + * \\var agc::Session::minExposureTime\n>>>> + * \\brief Minimum exposure time supported with the configured sensor\n>>>> + *\n>>>> + * \\var agc::Session::maxExposureTime\n>>>> + * \\brief Maximum exposure time supported with the configured sensor\n>>>> + *\n>>>> + * \\var agc::Session::minAnalogueGain\n>>>> + * \\brief Minimum analogue gain supported with the configured sensor\n>>>> + *\n>>>> + * \\var agc::Session::maxAnalogueGain\n>>>> + * \\brief Maximum analogue gain supported with the configured sensor\n>>>> + *\n>>>> + * \\var agc::Session::minFrameDuration\n>>>> + * \\brief Minimum frame duration supported with the configured sensor\n>>>> + *\n>>>> + * \\var agc::Session::maxFrameDuration\n>>>> + * \\brief Maximum frame duration supported with the configured sensor\n>>>\n>>> I would use \"for the streaming session\" in place of \"with the\n>>> configured sensor\" in all previous entries.\n>>>\n>>> yes, those settings depend on the sensor configuration but I found the\n>>> wording a bit confusing\n>>>\n>>> Or maybe \"for the current sensor configuration\" ?\n>>\n>> I'll drop \"supported\" and use \"for the streaming session\".\n>>\n>>\n> \n> thanks\n> \n>>>\n>>>> + *\n>>>> + * \\var agc::Session::lineDuration\n>>>> + * \\brief Line duration with the configured sensor and output size\n>>>> + *\n>>>> + * \\var agc::Session::sensor\n>>>> + * \\brief Details of the sensor configuration\n>>>> + *\n>>>> + * \\var agc::Session::sensor.outputSize\n>>>> + * \\brief Configured output size of the sensor\n>>>> + *\n>>>> + * \\var agc::Session::autoAllowed\n>>>> + * \\brief Whether automatic controls are allowed\n>>>\n>>> As I was really confused by this, is it worth adding one line saying\n>>> that auto is not allowed when capturing raw frames ?\n>>\n>> I suppose if statistics are provided, then it can work for raw capture as well.\n>> So my suggestion:\n>>\n>> \\brief Whether automatic controls are allowed\n>> Determines whether statistics are generally expected to be available for\n> \n> s/generally//\n> \n>> automatic exposure and gain control. If \\a false, statistics must not\n>> be provided to \\a AgcAlgorithm::process(), and \\a ExposureTimeMode and\n>> \\a AnalogueGainMode will only avertise manual control, and multiple other\n>> controls will also be omitted.\n> \n> Ok.\n> \n> I would however drop everything after ', and multiple ..\"\n> because either we list them or this might sound vague\n\nCurrent version:\n\n * \\brief Whether to enable auto controls\n *\n * If \\a false, the algorithm is set up for manual exposure and gain\n * control only, without automatic adjustments. In this mode statistics\n * must not be provided to AgcAlgorithm::process(), and ExposureTimeMode\n * and AnalogueGainMode will only advertise manual control.\n\n\n> \n>>\n>>\n>>>\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\struct agc::ActiveState\n>>>> + * \\brief Active state for AgcAlgorithm\n>>>> + *\n>>>> + * The \\a automatic variables track the latest values computed by algorithm\n>>>> + * based on the latest processed statistics. All other variables track the\n>>>> + * consolidated controls requested in queued requests.\n>>>> + *\n>>>> + * \\var agc::ActiveState::manual\n>>>> + * \\brief Manual exposure time and analog gain (set through requests)\n>>>> + *\n>>>> + * \\var agc::ActiveState::manual.exposure\n>>>> + * \\brief Manual exposure time expressed as a number of lines as set by the\n>>>> + * ExposureTime control\n>>>> + *\n>>>> + * \\var agc::ActiveState::manual.gain\n>>>> + * \\brief Manual analogue gain as set by the AnalogueGain control\n>>>> + *\n>>>> + * \\var agc::ActiveState::automatic\n>>>> + * \\brief Automatic exposure time and analog gain (computed by the algorithm)\n>>>> + *\n>>>> + * \\var agc::ActiveState::automatic.exposure\n>>>> + * \\brief Automatic exposure time expressed as a number of lines\n>>>> + *\n>>>> + * \\var agc::ActiveState::automatic.gain\n>>>> + * \\brief Automatic analogue gain multiplier\n>>>> + *\n>>>> + * \\var agc::ActiveState::automatic.quantizationGain\n>>>> + * \\brief Automatic quantization gain multiplier\n>>>> + *\n>>>> + * \\var agc::ActiveState::automatic.yTarget\n>>>> + * \\brief Automatically determined luminance target\n>>>> + *\n>>>> + * \\var agc::ActiveState::autoExposureEnabled\n>>>> + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n>>>\n>>> s/AGC state (exposure)/exposure mode\n>>>\n>>>> + *\n>>>> + * \\var agc::ActiveState::autoGainEnabled\n>>>> + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n>>>\n>>> same here ?\n>>\n>> These were copied verbatim.\n>>\n> \n> I don't think we should stick too strictly to what's in rkisp1\n> especially when it's about documentation which can so easily be\n> improved\n\nCurrent version:\n\n * \\brief Whether automatic exposure control is enabled by the ExposureTimeMode control\n\n\n> \n>>\n>>>\n>>>> + *\n>>>> + * \\var agc::ActiveState::exposureValue\n>>>> + * \\brief Exposure value as set by the ExposureValue control\n>>>> + *\n>>>> + * \\var agc::ActiveState::constraintMode\n>>>> + * \\brief Constraint mode as set by the AeConstraintMode control\n>>>> + *\n>>>> + * \\var agc::ActiveState::exposureMode\n>>>> + * \\brief Exposure mode as set by the AeExposureMode control\n>>>> + *\n>>>> + * \\var agc::ActiveState::minFrameDuration\n>>>> + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n>>>> + *\n>>>> + * \\var agc::ActiveState::maxFrameDuration\n>>>> + * \\brief Maximum frame duration as set by the FrameDurationLimits control\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\struct agc::FrameContext\n>>>> + * \\brief Per-frame context for AgcAlgorithm\n>>>> + *\n>>>> + * \\var agc::FrameContext::exposure\n>>>> + * \\brief Exposure time expressed as a number of lines computed by the algorithm\n>>>> + *\n>>>> + * \\var agc::FrameContext::gain\n>>>> + * \\brief Analogue gain multiplier computed by the algorithm\n>>>\n>>> Are these computed by the algorithms or can these be the manually\n>>> programmed values when running in manual mode ?\n>>\n>> This is always the gain that needs to be applied to the frame, so\n>> the manual gain in manual mode, the automatic gain in automatic mode.\n>>\n> \n> So maybe \"computed by the algorithm\" isn't exactly correct ?\n\nI think is a bit of a philosophical question again, for the user,\nthe important thing to know is from where to retrieve the final\nexposure and gain to apply to the frame. Even if manual control\nis in effect, these values are in a sense calculated.\n\nI think \"computed by the algorithm\" is fine, but maybe \"to apply to the frame\"\nwould be clearer?\n\n\n> \n> \n> * \\brief Analogue gain multiplier applied to the frame\n> \n> ?\n> \n>>\n>>>\n>>> I would replace \"computed by the algorithm\" with \"for the frame\" or\n>>> similar\n>>>\n>>>> + *\n>>>> + * The gain should be adapted to the sensor specific gain code before applying.\n>>>\n>>> s/adapted/translated ?\n>>\n>> These were also all copied verbatim, but I'll adjust it.\n>>\n>>\n>>>\n>>>> + *\n>>>> + * \\var agc::FrameContext::quantizationGain\n>>>> + * \\brief Quantization gain multiplier computed by the algorithm\n>>>> + *\n>>>> + * \\var agc::FrameContext::exposureValue\n>>>> + * \\brief Exposure value as set by the ExposureValue control\n>>>> + *\n>>>> + * \\var agc::FrameContext::yTarget\n>>>> + * \\brief Luminance target computed by the algorithm\n>>>> + *\n>>>> + * \\var agc::FrameContext::vblank\n>>>> + * \\brief Vertical blanking parameter computed by the algorithm\n>>>> + *\n>>>> + * \\var agc::FrameContext::autoExposureEnabled\n>>>> + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n>>>> + *\n>>>> + * \\var agc::FrameContext::autoGainEnabled\n>>>> + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n>>>> + *\n>>>> + * \\var agc::FrameContext::constraintMode\n>>>> + * \\brief Constraint mode as set by the AeConstraintMode control\n>>>> + *\n>>>> + * \\var agc::FrameContext::exposureMode\n>>>> + * \\brief Exposure mode as set by the AeExposureMode control\n>>>> + *\n>>>> + * \\var agc::FrameContext::minFrameDuration\n>>>> + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n>>>> + *\n>>>> + * \\var agc::FrameContext::maxFrameDuration\n>>>> + * \\brief Maximum frame duration as set by the FrameDurationLimits control\n>>>> + *\n>>>> + * \\var agc::FrameContext::frameDuration\n>>>> + * \\brief The actual FrameDuration used by the algorithm for the frame\n>>>> + *\n>>>> + * \\var agc::FrameContext::autoExposureModeChange\n>>>> + * \\brief Indicate if autoExposureEnabled has changed from true in the previous\n>>>> + * frame to false in the current frame, and no manual exposure value has been\n>>>> + * supplied in the current frame.\n>>>\n>>> does this only track auto->manual transaction or does it track the\n>>> other way around.\n>>>\n>>> Kind of a long brief (and no '.' at the end).\n>>\n>> These were also copied verbatim, but I have removed the dots.\n>>\n>>\n>>>\n>>> I would\n>>>\n>>> \\brief Exposure mode change flag\n>>>\n>>> Indicate if the exposure mode has changed compared to the\n>>> previous frame.\n>>\n>> It only tracks auto -> manual transitions without manual `ExposureTime`,\n>> so I think the above wouldn't be entirely correct.\n>>\n> \n> Yeah, you're right\n> \n> if (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))\n> frameContext.autoExposureModeChange = true;\n> \n> The purpose of the flag is to keep track if the auto value should be\n> propagated to manual, so it's a bit of an unfortunate name. However,\n> not something that should be changed in this patch\n> \n>>\n>>>\n>>>> + *\n>>>> + * \\var agc::FrameContext::autoGainModeChange\n>>>> + * \\brief Indicate if autoGainEnabled has changed from true in the previous\n>>>> + * frame to false in the current frame, and no manual gain value has been\n>>>> + * supplied in the current frame.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\struct AgcAlgorithm::ConfigurationParams\n>>>> + * \\brief Parameters for AgcAlgorithm::configure()\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ConfigurationParams::sensor\n>>>> + * \\brief CameraSensorHelper for the sensor\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ConfigurationParams::sensorInfo\n>>>> + * \\brief Details of the sensor\n>>>\n>>> The sensor configuration description\n>>\n>> I have adjusted it to\n>>\n>> Current configuration of the sensor\n>>\n>> hope it is also acceptable.\n>>\n> \n> It is indeed\n> \n>>\n>>>\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ConfigurationParams::sensorControls\n>>>> + * \\brief ControlInfoMap of the sensor\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ConfigurationParams::ctrlMap\n>>>> + * \\brief ControlMap to update with controls\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ConfigurationParams::autoAllowed\n>>>> + * \\brief Whether to enable auto controls\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\struct AgcAlgorithm::ProcessParams\n>>>> + * \\brief Parameters for AgcAlgorithm::process()\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ProcessParams::traits\n>>>> + * \\brief Implementation of AgcMeanLuminance::Traits\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ProcessParams::yHist\n>>>> + * \\brief Luminance histogram of the frame\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ProcessParams::exposure\n>>>> + * \\brief Effective exposure of the frame\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ProcessParams::gain\n>>>> + * \\brief Effective gain of the frame\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ProcessParams::additionalConstraints\n>>>> + * \\brief Additional AgcMeanLuminance::AgcConstraints to apply\n>>>> + *\n>>>> + * \\var AgcAlgorithm::ProcessParams::lux\n>>>> + * \\brief Effective lux value of the frame\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\brief Load tuning data\n>>>> + */\n>>>> +int AgcAlgorithm::init(const ValueNode &tuningData)\n>>>> +{\n>>>> +\tint ret = impl_.parseTuningData(tuningData);\n>>>> +\tif (ret)\n>>>> +\t\treturn ret;\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Initialize the session configuration and active state\n>>>> + */\n>>>> +int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config)\n>>>\n>>> Easy to shorten to\n>>>\n>>> int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,\n>>> \t\t\t const ConfigurationParams &config)\n>>\n>> Done.\n>>\n>>\n>>>\n>>>\n>>> Seems like session is an out parameter.\n>>>\n>>> We used to have a rule: const & for input params, * for output ones.\n>>> Not sure anymore how much we enforce it these days..\n>>\n>> I'm not sure I would consider it an out param in the traditional sense\n>> (i.e. the caller is not really expected to use/inspect the result), and\n>> I really prefer the consistency of always passing by reference to every\n>> function of `AgcAlgorithm`.\n>>\n> \n> Ok, as said, I don't exactly know how much that rule is even enforced\n> nowadays\n> \n>>\n>>>\n>>>> +{\n>>>> +\tsession = {};\n>>>> +\tsession.lineDuration = config.sensorInfo.minLineLength * 1.0s\n>>>> +\t\t/ config.sensorInfo.pixelRate;\n>>>> +\tsession.sensor.outputSize = config.sensorInfo.outputSize;\n>>>> +\tsession.autoAllowed = config.autoAllowed;\n>>>> +\n>>>> +\tconst double lineDurationUs = session.lineDuration.get();\n>>>> +\n>>>> +\t/*\n>>>> +\t * Compute exposure time limits from the V4L2_CID_EXPOSURE control\n>>>> +\t * limits and the line duration.\n>>>> +\t */\n>>>> +\n>>>> +\tconst ControlInfo &v4l2Exposure = config.sensorControls.find(V4L2_CID_EXPOSURE)->second;\n>>>> +\tint32_t minExposure = v4l2Exposure.min().get();\n>>>> +\tint32_t maxExposure = v4l2Exposure.max().get();\n>>>> +\tint32_t defExposure = v4l2Exposure.def().get();\n>>>> +\tconfig.ctrlMap[&controls::ExposureTime] = ControlInfo{\n>>>> +\t\tstatic_cast(minExposure * lineDurationUs),\n>>>> +\t\tstatic_cast(maxExposure * lineDurationUs),\n>>>> +\t\tstatic_cast(defExposure * lineDurationUs),\n>>>> +\t};\n>>>> +\n>>>> +\t/* Compute the analogue gain limits. */\n>>>> +\tconst ControlInfo &v4l2Gain = config.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>>>> +\tfloat minGain = config.sensor->gain(v4l2Gain.min().get());\n>>>> +\tfloat maxGain = config.sensor->gain(v4l2Gain.max().get());\n>>>> +\tfloat defGain = config.sensor->gain(v4l2Gain.def().get());\n>>>> +\tconfig.ctrlMap[&controls::AnalogueGain] = ControlInfo{\n>>>> +\t\tminGain,\n>>>> +\t\tmaxGain,\n>>>> +\t\tdefGain,\n>>>> +\t};\n>>>> +\n>>>> +\tLOG(Agc, Debug)\n>>>> +\t\t<< \"Exposure: [\" << minExposure << \", \" << maxExposure\n>>>> +\t\t<< \"], gain: [\" << minGain << \", \" << maxGain << \"]\";\n>>>> +\n>>>> +\t/*\n>>>> +\t * Compute the frame duration limits.\n>>>> +\t *\n>>>> +\t * The frame length is computed assuming a fixed line length combined\n>>>> +\t * with the vertical frame sizes.\n>>>> +\t */\n>>>> +\tconst ControlInfo &v4l2HBlank = config.sensorControls.find(V4L2_CID_HBLANK)->second;\n>>>> +\tuint32_t hblank = v4l2HBlank.def().get();\n>>>> +\tuint32_t lineLength = config.sensorInfo.outputSize.width + hblank;\n>>>> +\n>>>> +\tconst ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;\n>>>> +\tstd::array frameHeights{\n>>>> +\t\tv4l2VBlank.min().get() + config.sensorInfo.outputSize.height,\n>>>> +\t\tv4l2VBlank.max().get() + config.sensorInfo.outputSize.height,\n>>>> +\t\tv4l2VBlank.def().get() + config.sensorInfo.outputSize.height,\n>>>> +\t};\n>>>> +\n>>>> +\tstd::array frameDurations;\n>>>> +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n>>>> +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n>>>> +\t\tframeDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U);\n>>>> +\t}\n>>>> +\n>>>> +\tconfig.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{\n>>>> +\t\tframeDurations[0],\n>>>> +\t\tframeDurations[1],\n>>>> +\t\tSpan{ { frameDurations[2], frameDurations[2] } },\n>>>> +\t};\n>>>> +\n>>>> +\tsession.minFrameDuration = std::chrono::microseconds(frameDurations[0]);\n>>>> +\tsession.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);\n>>>> +\n>>>> +\t/*\n>>>> +\t * When the AGC computes the new exposure values for a frame, it needs\n>>>> +\t * to know the limits for exposure time and analogue gain. As it depends\n>>>> +\t * on the sensor, update it with the controls.\n>>>> +\t *\n>>>> +\t * \\todo take VBLANK into account for maximum exposure time\n>>>> +\t */\n>>>> +\tsession.minExposureTime = minExposure * session.lineDuration;\n>>>> +\tsession.maxExposureTime = maxExposure * session.lineDuration;\n>>>> +\tsession.minAnalogueGain = minGain;\n>>>> +\tsession.maxAnalogueGain = maxGain;\n>>>\n>>> The above matches the current RkISP1Agc::reconfigure() content\n>>>\n>>>> +\n>>>> +\timpl_.configure(session.lineDuration, config.sensor);\n>>>> +\timpl_.setLimits(session.minExposureTime, session.maxExposureTime,\n>>>> +\t\t\tsession.minAnalogueGain, session.maxAnalogueGain,\n>>>> +\t\t\t{});\n>>>> +\timpl_.resetFrameCount();\n>>>> +\n>>>> +\t/* Configure the default exposure and gain. */\n>>>> +\tstate = {};\n>>>> +\tstate.automatic.gain = session.minAnalogueGain;\n>>>> +\tstate.automatic.exposure = 10ms / session.lineDuration;\n>>>> +\tstate.automatic.quantizationGain = 1;\n>>>> +\tstate.automatic.yTarget = impl_.effectiveYTarget(0, 1);\n>>>\n>>> And this matches RkISP1::Agc() apart from this early yTarget\n>>> computation which I presume is intentional\n>>\n>> Hmmm... that should match as well. There is\n>>\n>> context.activeState.agc.automatic.yTarget = agc_.effectiveYTarget(0, 1);\n>>\n>> at the end of `rkisp1.cpp:Agc::configure()`.\n> \n> Ah yes, it's at the end of the function indeed\n> \n>>\n>>\n>>>\n>>>> +\tstate.manual.gain = state.automatic.gain;\n>>>> +\tstate.manual.exposure = state.automatic.exposure;\n>>>> +\tstate.autoExposureEnabled = session.autoAllowed;\n>>>> +\tstate.autoGainEnabled = session.autoAllowed;\n>>>> +\tstate.exposureValue = 0;\n>>>> +\n>>>> +\tstate.constraintMode =\n>>>> +\t\tstatic_cast(impl_.constraintModes().begin()->first);\n>>>> +\tstate.exposureMode =\n>>>> +\t\tstatic_cast(impl_.exposureModeHelpers().begin()->first);\n>>>> +\n>>>> +\tstate.minFrameDuration = session.minFrameDuration;\n>>>> +\tstate.maxFrameDuration = session.maxFrameDuration;\n>>>> +\n>>>> +\tconst auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) {\n>>>> +\t\tstd::array values;\n>>>> +\t\tsize_t count = 0;\n>>>> +\n>>>> +\t\tif (session.autoAllowed)\n>>>> +\t\t\tvalues[count++] = ControlValue(automatic);\n>>>> +\n>>>> +\t\tvalues[count++] = ControlValue(manual);\n>>>> +\n>>>> +\t\tconfig.ctrlMap[&cid] = ControlInfo{\n>>>> +\t\t\t{ values.data(), count },\n>>>> +\t\t\tControlValue(session.autoAllowed ? automatic : manual),\n>>>> +\t\t};\n>>>> +\t};\n>>>> +\n>>>> +\tadd(controls::ExposureTimeMode, controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);\n>>>> +\tadd(controls::AnalogueGainMode, controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);\n>>>\n>>> Could you please break trivially re-adjustable long lines ?\n>>\n>> Adjusted to\n>>\n>> add(controls::ExposureTimeMode,\n>> controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual);\n>> add(controls::AnalogueGainMode,\n>> controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual);\n>>\n>> Does that look better?\n>>\n> \n> indeed\n> \n>>\n>>>\n>>>> +\n>>>> +\t/* \\todo Move this to the `Camera` class. */\n>>>> +\tconfig.ctrlMap[&controls::AeEnable] = ControlInfo{\n>>>> +\t\tfalse,\n>>>> +\t\tsession.autoAllowed,\n>>>> +\t\tsession.autoAllowed,\n>>>> +\t};\n>>>\n>>> RkISP1Agc register controls in init() but I think what you're doing\n>>> here it's better, as some control such as ExposureTimeMode depend on\n>>> the session configuration.\n>>>\n>>> Is there any risk of having leftovers from a previous session in the\n>>> control list ?\n>>\n>> Every control should either be overwritten or removed.\n>>\n> \n> Is that because the Camera::controls info map gets reset between\n> configure() calls ? I don't really see that happening in Camera or\n> PipelineHandler.\n\nAs far as I can tell, currently no IPA module clears its `ControlInfoMap::Map`\nbecause most controls are added in each algorithm's `init()`. I think this is\nagain a policy question. But adjusting this might be inconvenient because I think\nit is expected to have registered controls even before the first configuration,\nso in effect controls will have to be added in both `init()` and `configure()`\nby each algorithm. Or alternatively each IPA module should call `configure()`\nright after `init()`.\n\n\n> \n> Same as per the data->ipaControls_ passed by the RkISP1 pipeline handle\n> to the IPA and used to populate Camera::controls().\n> \n>>\n>>>\n>>>> +\n>>>> +\tif (session.autoAllowed) {\n>>>> +\t\tconfig.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);\n>>>> +\n>>>> +\t\tfor (const auto &[id, info] : impl_.controls())\n>>>> +\t\t\tconfig.ctrlMap[id] = info;\n>>>> +\t} else {\n>>>> +\t\tconfig.ctrlMap.erase(&controls::ExposureValue);\n>>>> +\n>>>> +\t\tfor (const auto &[id, info] : impl_.controls())\n>>>> +\t\t\tconfig.ctrlMap.erase(id);\n>>>> +\t}\n>>>\n>>> Maybe it happens later on, but this is a bit cumbersome. Making\n>>> AgcMeanLuminance aware of autoAllowed would avoid this\n>>\n>> Possibly, but my preference is to keep more control handling out of AgcMeanLuminance.\n>> And in any case, there will be a loop like this or equivalent, and\n> \n> Ok, I thought it was AgcMeanLuminance registering ExposureValue but it\n> actually is this class doing so it here. What we're erasing are\n> possible leftovers from a previous session maybe ?\n> \n>> I'm not sure it matters too much where it is.\n> \n> I was bothered by the need to erase, as it seems like a lot of\n> bookeeping that could possibile go wrong.\n> \n> But if I get this right this is about removing controls from a\n> previous session ? If that's the case should we pass a fresh list of\n> controls at every ::configure() call to avoid leftovers instead of\n> erasing them ?\n\nSee above.\n\n\n> \n>>\n>>\n>>>\n>>>> +\n>>>> +\n>>>\n>>> Double empty line\n>>\n>> Fixed.\n>>\n>>\n>>>\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Handle a \\a queueRequest operation\n>>>> + */\n>>>> +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,\n>>>> +\t\t\t\tagc::FrameContext &frameContext, const ControlList &controls)\n>>>> +{\n>>>> +\tif (session.autoAllowed) {\n>>>\n>>> Now that you register controls at configure() time and not init()\n>>> time, am I correct that if !autoAllowed applications won't be able to select\n>>> controls::ExposureTimeModeAuto ?\n>>\n>> Yes.\n>>\n>>\n>>>\n>>> Which makes me wonder if controls::ExposureTimeMode should be\n>>> registered at all if !autoAllowed\n>>\n>> I think that's more of a philosophical question. My preference is to\n>> register controls even if they only really have one valid value. Maybe\n>> this needs more discussion to arrive at a libcamera-wide policy.\n> \n> Ack, let's ask others\n> \n>>\n>>\n>>>\n>>>> +\t\tconst auto &aeEnable = controls.get(controls::ExposureTimeMode);\n>>>> +\t\tif (aeEnable &&\n>>>> +\t\t (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {\n>>>> +\t\t\tstate.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);\n>>>> +\n>>>> +\t\t\tLOG(Agc, Debug)\n>>>> +\t\t\t\t<< (state.autoExposureEnabled ? \"Enabling\" : \"Disabling\")\n>>>> +\t\t\t\t<< \" AGC (exposure)\";\n>>>> +\n>>>> +\t\t\t/*\n>>>> +\t\t\t * If we go from auto -> manual with no manual control\n>>>> +\t\t\t * set, use the last computed value, which we don't\n>>>> +\t\t\t * know until prepare() so save this information.\n>>>> +\t\t\t *\n>>>> +\t\t\t * \\todo Check the previous frame at prepare() time\n>>>> +\t\t\t * instead of saving a flag here\n>>>> +\t\t\t */\n>>>> +\t\t\tif (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))\n>>>> +\t\t\t\tframeContext.autoExposureModeChange = true;\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tconst auto &agEnable = controls.get(controls::AnalogueGainMode);\n>>>\n>>> Same reasoning for AnalogueGainMode\n>>>\n>>>> +\t\tif (agEnable &&\n>>>> +\t\t (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {\n>>>> +\t\t\tstate.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);\n>>>> +\n>>>> +\t\t\tLOG(Agc, Debug)\n>>>> +\t\t\t\t<< (state.autoGainEnabled ? \"Enabling\" : \"Disabling\")\n>>>> +\t\t\t\t<< \" AGC (gain)\";\n>>>> +\t\t\t/*\n>>>> +\t\t\t * If we go from auto -> manual with no manual control\n>>>> +\t\t\t * set, use the last computed value, which we don't\n>>>> +\t\t\t * know until prepare() so save this information.\n>>>> +\t\t\t */\n>>>> +\t\t\tif (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))\n>>>> +\t\t\t\tframeContext.autoGainModeChange = true;\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +\tconst auto &exposure = controls.get(controls::ExposureTime);\n>>>> +\tif (exposure && !state.autoExposureEnabled) {\n>>>> +\t\tstate.manual.exposure = *exposure * 1.0us / session.lineDuration;\n>>>> +\n>>>> +\t\tLOG(Agc, Debug)\n>>>> +\t\t\t<< \"Set exposure to \" << state.manual.exposure;\n>>>> +\t}\n>>>> +\n>>>> +\tconst auto &gain = controls.get(controls::AnalogueGain);\n>>>> +\tif (gain && !state.autoGainEnabled) {\n>>>> +\t\tstate.manual.gain = *gain;\n>>>> +\n>>>> +\t\tLOG(Agc, Debug) << \"Set gain to \" << state.manual.gain;\n>>>> +\t}\n>>>> +\n>>>> +\tframeContext.autoExposureEnabled = state.autoExposureEnabled;\n>>>> +\tframeContext.autoGainEnabled = state.autoGainEnabled;\n>>>> +\n>>>> +\tif (!frameContext.autoExposureEnabled)\n>>>> +\t\tframeContext.exposure = state.manual.exposure;\n>>>> +\tif (!frameContext.autoGainEnabled)\n>>>> +\t\tframeContext.gain = state.manual.gain;\n>>>> +\n>>>> +\tif (!frameContext.autoExposureEnabled &&\n>>>> +\t !frameContext.autoGainEnabled)\n>>>> +\t\tframeContext.quantizationGain = 1.0;\n>>>> +\n>>>> +\tconst auto &exposureMode = controls.get(controls::AeExposureMode);\n>>>> +\tif (exposureMode)\n>>>> +\t\tstate.exposureMode =\n>>>> +\t\t\tstatic_cast(*exposureMode);\n>>>> +\tframeContext.exposureMode = state.exposureMode;\n>>>> +\n>>>> +\tconst auto &constraintMode = controls.get(controls::AeConstraintMode);\n>>>> +\tif (constraintMode)\n>>>> +\t\tstate.constraintMode =\n>>>> +\t\t\tstatic_cast(*constraintMode);\n>>>> +\tframeContext.constraintMode = state.constraintMode;\n>>>> +\n>>>> +\tconst auto &exposureValue = controls.get(controls::ExposureValue);\n>>>> +\tif (exposureValue)\n>>>> +\t\tstate.exposureValue = *exposureValue;\n>>>> +\tframeContext.exposureValue = state.exposureValue;\n>>>> +\n>>>> +\tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n>>>> +\tif (frameDurationLimits) {\n>>>> +\t\t/* Limit the control value to the limits in ControlInfo */\n>>>> +\t\tstate.minFrameDuration = std::clamp(\n>>>> +\t\t\tstd::chrono::microseconds((*frameDurationLimits).front()),\n>>>> +\t\t\tsession.minFrameDuration,\n>>>> +\t\t\tsession.maxFrameDuration\n>>>> +\t\t);\n>>>> +\n>>>> +\t\tstate.maxFrameDuration = std::clamp(\n>>>> +\t\t\tstd::chrono::microseconds((*frameDurationLimits).back()),\n>>>> +\t\t\tsession.minFrameDuration,\n>>>> +\t\t\tsession.maxFrameDuration\n>>>> +\t\t);\n>>>> +\t}\n>>>> +\tframeContext.minFrameDuration = state.minFrameDuration;\n>>>> +\tframeContext.maxFrameDuration = state.maxFrameDuration;\n>>>> +}\n>>>> +\n>>>\n>>> This matches RkISP1Agc::queueRequest() (metering mode handling apart)\n>>>\n>>>> +/**\n>>>> + * \\brief Handle a \\a prepare operation\n>>>> + */\n>>>> +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)\n>>>> +{\n>>>> +\tuint32_t activeAutoExposure = state.automatic.exposure;\n>>>> +\tdouble activeAutoGain = state.automatic.gain;\n>>>> +\tdouble activeAutoQGain = state.automatic.quantizationGain;\n>>>> +\n>>>> +\t/* Populate exposure and gain in auto mode */\n>>>> +\tif (frameContext.autoExposureEnabled) {\n>>>> +\t\tframeContext.exposure = activeAutoExposure;\n>>>> +\t\tframeContext.quantizationGain = activeAutoQGain;\n>>>> +\t}\n>>>> +\tif (frameContext.autoGainEnabled) {\n>>>> +\t\tframeContext.gain = activeAutoGain;\n>>>> +\t\tframeContext.quantizationGain = activeAutoQGain;\n>>>> +\t}\n>>>> +\n>>>> +\t/*\n>>>> +\t * Populate manual exposure and gain from the active auto values when\n>>>> +\t * transitioning from auto to manual\n>>>> +\t */\n>>>\n>>> I guess this answers to my question on the auto->manual transition\n>>> tracking for the frameContext.autoExposureModeChange flag.\n>>>\n>>> It tracks both state changes, but we use it to propagate auto values\n>>> to the manual state if they have not been overriden by an application\n>>> provide control\n>>\n>> As mentioned above, I think it very specifically only tracks those auto->manual\n>> transition that don't have an associated manual `ExposureTime`.\n>>\n> \n> Yes you're right\n> \n>>\n>>>\n>>>> +\tif (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {\n>>>> +\t\tstate.manual.exposure = activeAutoExposure;\n>>>> +\t\tframeContext.exposure = activeAutoExposure;\n>>>> +\t}\n>>>> +\tif (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {\n>>>> +\t\tstate.manual.gain = activeAutoGain;\n>>>> +\t\tframeContext.gain = activeAutoGain;\n>>>> +\t\tframeContext.quantizationGain = activeAutoQGain;\n>>>> +\t}\n>>>> +\n>>>> +\tframeContext.yTarget = state.automatic.yTarget;\n>>>> +}\n>>>\n>>> Ack, this matches RkISP1Agc::prepare()\n>>>\n>>>> +\n>>>> +/**\n>>>> + * \\brief Handle a \\a process operation\n>>>> + */\n>>>> +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,\n>>>> +\t\t\t agc::FrameContext &frameContext, std::optional &¶ms,\n>>>> +\t\t\t ControlList &metadata)\n>>>> +{\n>>>> +\tconst utils::Duration &lineDuration = session.lineDuration;\n>>>> +\tutils::Duration newExposureTime = {};\n>>>> +\n>>>> +\tif (params) {\n>>>\n>>> I presume (well, you told me) that this matches the\n>>>\n>>> if (!stats)\n>>>\n>>> case in RkISP1.\n>>>\n>>> I wonder, should\n>>>\n>>> The RkISP1 IPA does\n>>>\n>>> \tif (!stats) {\n>>> \t\tprocessFrameDuration(context, frameContext,\n>>> \t\t\t\t frameContext.agc.minFrameDuration);\n>>> \t\tfillMetadata(context, frameContext, metadata);\n>>> \t\treturn;\n>>> \t}\n>>>\n>>> I guess it's fine for IPA to call AgcAlgorithm::process() even when\n>>> !stats, as it is simpler for them to make the call unconditionally, so\n>>> I'm not against this.\n>>>\n>>> But maybe we can do like RkISP1 does (split processFrameDuration() and\n>>> fillMetadata() to helpers) to save some indentation here.\n>>\n>> My preference would be not to add back those separate functions. Maybe\n>> moving the `if (params) { ... }` block into a separate function if\n>> that is acceptable.\n> \n> I guess we're discussing tastes, but I usually get suspicious when\n> code gets to 4 indentation levels and we could easily avoid that\n> \n>>\n>>\n>>>\n>>>> +\t\tASSERT(session.autoAllowed);\n>>>\n>>> Does it mean IPA should not call process if (autoAllowed) ?\n>>\n>> Yes.\n>>\n> \n> Well, not exactly.\n> \n> The IPA shall not provide stats if !autoAllowed\n\nRight, I don't know why I wrote that.\n\n\n> \n>>\n>>>\n>>> Can't we simply bail out here to make IPAs simpler ?\n>>\n>> Possibly, but I don't see how that would make them simpler. The assumption\n> \n> I would have liked for IPAs to be able to call ::process() with an\n> optionally initialized ProcessParams which in case of !stats\n> would be empty. If the core is running with !autoAllowed it will\n> ignore the params.\n> \n> This doesn't work in case the system is running with autoAllowed but\n> for some reasons you don't have valid stats. RkISP1 seems to assume it\n> might happen you can get bad/corrupted stats, however in practice I'm\n> not sure how it could happen.\n> \n> \n> What you have right now (RkISP1 as an example)\n> \n> \tif (stats) {\n> \t\tif (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)\n> \t\t\tparams = &stats->params;\n> \t\telse\n> \t\t\tLOG(RkISP1Agc, Error) << \"AUTOEXP data is missing in statistics\";\n> \t}\n> \n> \tif (params) {\n> \t\tstd::vector additionalConstraints;\n> \t\tif (context.activeState.wdr.mode != controls::WdrOff)\n> \t\t\tadditionalConstraints.push_back(context.activeState.wdr.constraint);\n> \n> \t\tagc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{\n> \t\t\t.traits = AgcTraits{\n> \t\t\t\t{ params->ae.exp_mean, context.hw.numAeCells },\n> \t\t\t\tmeteringModes_.at(frameContext.agc.meteringMode),\n> \t\t\t},\n> \t\t\t.yHist = {\n> \t\t\t\t/* The lower 4 bits are fractional and meant to be discarded. */\n> \t\t\t\t{ params->hist.hist_bins, context.hw.numHistogramBins },\n> \t\t\t\t[](uint32_t x) { return x >> 4; },\n> \t\t\t},\n> \t\t\t.exposure = frameContext.sensor.exposure,\n> \t\t\t/*\n> \t\t\t * Include the quantization gain if it was applied. Do not use\n> \t\t\t * compress.gain because it will include gains that shall not be\n> \t\t\t * reported to the user when HDR is implemented.\n> \t\t\t */\n> \t\t\t.gain = frameContext.sensor.gain\n> \t\t\t * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),\n> \t\t\t.additionalConstraints = std::move(additionalConstraints),\n> \t\t\t.lux = frameContext.lux.lux,\n> \t\t}}, metadata);\n> \t} else {\n> \t\tagc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);\n> \t}\n> \n> Requires the IPA to validate stats and have to distinct calls to\n> agc_.process()\n> \n> while\n> \n> agc::ProcessParams params {};\n> if (stats && stats & ... )\n> /* populate params. */\n> }\n> \n> agc_.process( ... , ..., params, ... )\n\nThe issue is the \"traits\" and \"yHist\" members, they are references,\nso the referenced things must stay alive for the duration of the call.\nThe above snippet you suggested is syntactically possible, but avoiding\nuse-after-free issues makes it a bit cumbersome.\n\n\n> \n> is less verbose, but would require the core to bail out in case\n> (params.empty() || !autoAllowed)\n> \n> This mean you would need a way to distinguish empty param blocks and\n> you can't use the declarative style you have here, which I kind of\n> like as well..\n> \n> not sure, I have a feeling IPA could be made simpler, but at the same\n> time this seems more robust\n> \n>> is that `autoAllowed == false` means that statistics will not be available\n>> (and hence no automatic control) (usually for raw capture - but there is no\n>> technical reason why agc could not run even for raw capture if statistics\n>> are available), so providing statistics is an api violation in a sense, so\n>> I think the assert is reasonable.\n>>\n> \n> We can start strict and see. After all this is not something\n> application could trigger.\n> \n>>\n>>>\n>>>> +\n>>>> +\t\t/*\n>>>> +\t\t* Set the AGC limits using the fixed exposure time and/or gain in\n>>>> +\t\t* manual mode, or the sensor limits in auto mode.\n>>>> +\t\t*/\n>>>\n>>> Weird indent\n>>\n>> Fixed.\n>>\n>>>\n>>>> +\t\tutils::Duration minExposureTime;\n>>>> +\t\tutils::Duration maxExposureTime;\n>>>> +\t\tdouble minAnalogueGain;\n>>>> +\t\tdouble maxAnalogueGain;\n>>>> +\n>>>> +\t\tif (frameContext.autoExposureEnabled) {\n>>>> +\t\t\tminExposureTime = session.minExposureTime;\n>>>> +\t\t\tmaxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);\n>>>\n>>> very long line\n>>\n>> Adjusted.\n>>\n>>\n>>>\n>>>> +\t\t} else {\n>>>> +\t\t\tminExposureTime = lineDuration * frameContext.exposure;\n>>>> +\t\t\tmaxExposureTime = minExposureTime;\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tif (frameContext.autoGainEnabled) {\n>>>> +\t\t\tminAnalogueGain = session.minAnalogueGain;\n>>>> +\t\t\tmaxAnalogueGain = session.maxAnalogueGain;\n>>>> +\t\t} else {\n>>>> +\t\t\tminAnalogueGain = frameContext.gain;\n>>>> +\t\t\tmaxAnalogueGain = frameContext.gain;\n>>>> +\t\t}\n>>>> +\n>>>> +\t\t/*\n>>>> +\t\t* The Agc algorithm needs to know the effective exposure value that was\n>>>> +\t\t* applied to the sensor when the statistics were collected.\n>>>> +\t\t*/\n>>>> +\t\tutils::Duration effectiveExposureValue =\n>>>> +\t\t\tlineDuration * params->exposure * params->gain;\n>>>\n>>> RkISP1 gets exposure and gain from the frameContext.\n>>>\n>>> I see in the next patches that port RkISP1 to AgcAlgorithm this hunk\n>>>\n>>> agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc,\n>>>\n>>> ...\n>>>\n>>> .exposure = frameContext.sensor.exposure,\n>>> /*\n>>> * Include the quantization gain if it was applied. Do not use\n>>> * compress.gain because it will include gains that shall not be\n>>> * reported to the user when HDR is implemented.\n>>> */\n>>> .gain = frameContext.sensor.gain\n>>> * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),\n>>>\n>>> Is the reason you can't take exposure and gain from the FrameContext\n>>> the gain adjustment ?\n>>\n>> That, and the fact that it is `frameContext.sensor`, which is separate\n>> from `frameContext.agc` (i.e. `agc::FrameContext`), because it needs\n>> the exposure/gain that was actually in effect.\n>>\n> \n> Ack, too bad, the Agc::process() interface is quite verbose already :(\n> \n>>\n>>>\n>>>> +\n>>>> +\t\timpl_.setLimits(minExposureTime, maxExposureTime,\n>>>> +\t\t\t\tminAnalogueGain, maxAnalogueGain,\n>>>> +\t\t\t\tstd::move(params->additionalConstraints));\n>>>> +\n>>>> +\t\tconst auto &newEv = impl_.calculateNewEv({\n>>>> +\t\t\t.traits = params->traits,\n>>>> +\t\t\t.yHist = params->yHist,\n>>>> +\t\t\t.effectiveExposureValue = effectiveExposureValue,\n>>>> +\t\t\t.constraintModeIndex = frameContext.constraintMode,\n>>>> +\t\t\t.exposureModeIndex = frameContext.exposureMode,\n>>>> +\t\t\t.lux = params->lux,\n>>>> +\t\t\t.exposureCompensation = pow(2.0, frameContext.exposureValue),\n>>>> +\t\t});\n>>>> +\n>>>> +\t\tLOG(Agc, Debug)\n>>>> +\t\t\t<< \"Divided up exposure time, analogue gain, quantization gain\"\n>>>> +\t\t\t<< \" and digital gain are \" << newEv.exposureTime << \", \" << newEv.analogueGain\n>>>> +\t\t\t<< \", \" << newEv.quantizationGain << \" and \" << newEv.digitalGain;\n>>>> +\n>>>> +\t\t/* Update the estimated exposure and gain. */\n>>>> +\t\tstate.automatic.exposure = newEv.exposureTime / lineDuration;\n>>>> +\t\tstate.automatic.gain = newEv.analogueGain;\n>>>> +\t\tstate.automatic.quantizationGain = newEv.quantizationGain;\n>>>> +\t\tstate.automatic.yTarget = newEv.yTarget;\n>>>> +\n>>>> +\t\tnewExposureTime = newEv.exposureTime;\n>>>> +\t}\n>>>> +\n>>>> +\t/*\n>>>> +\t * Expand the target frame duration so that we do not run faster than\n>>>> +\t * the minimum frame duration when we have short exposures.\n>>>> +\t */\n>>>> +\tconst auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);\n>>>> +\tframeContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;\n>>>> +\n>>>> +\t/* Update frame duration accounting for line length quantization. */\n>>>> +\tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;\n>>>\n>>> Easy to shorten to\n>>>\n>>> \tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank)\n>>> \t\t\t\t * lineDuration;\n>>>\n>>\n>> This was also copied verbatim.\n>>\n>>\n>>>> +\n>>>> +\tmetadata.set(controls::AnalogueGain, frameContext.gain);\n>>>> +\tmetadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get());\n>>>\n>>> I'm not sure I understand the strategy here.. this line is very long\n>>> and could be easily made\n>>>\n>>> \tmetadata.set(controls::ExposureTime,\n>>> \t\t utils::Duration(lineDuration * frameContext.exposure).get());\n>>>\n>>\n>> Adjusted.\n>>\n>>\n>>>\n>>>> +\tmetadata.set(controls::FrameDuration, frameContext.frameDuration.get());\n>>>> +\tmetadata.set(controls::ExposureTimeMode,\n>>>> +\t\t frameContext.autoExposureEnabled\n>>>> +\t\t ? controls::ExposureTimeModeAuto\n>>>> +\t\t : controls::ExposureTimeModeManual);\n>>>> +\tmetadata.set(controls::AnalogueGainMode,\n>>>> +\t\t frameContext.autoGainEnabled\n>>>> +\t\t ? controls::AnalogueGainModeAuto\n>>>> +\t\t : controls::AnalogueGainModeManual);\n>>>\n>>> These ones instead are very short and could easily be made\n>>>\n>>> \tmetadata.set(controls::ExposureTimeMode, frameContext.autoExposureEnabled ?\n>>> \t\t controls::ExposureTimeModeAuto : controls::ExposureTimeModeManual);\n>>\n>>\n>> These were copied verbatim. So please confirm if you want me to adjust the\n>> verbatim copies here and in the whole file.\n> \n> As said, it's not that we're moving code in a single patch where we\n> can compare the lines moved around in the diff.\n> \n> The way I did the review was looking at the code in Agc and RkISP1Agc\n> and they are of course different already, so I don't think we should\n> stick to verbatim copies when there's something trivially improvable\n> like spanning to the whole line length\n> \n>>\n>>\n>>>\n>>>> +\n>>>> +\tmetadata.set(controls::AeExposureMode, frameContext.exposureMode);\n>>>> +\tmetadata.set(controls::AeConstraintMode, frameContext.constraintMode);\n>>>> +\tmetadata.set(controls::ExposureValue, frameContext.exposureValue);\n>>>> +}\n>>>> +\n>>>> +} /* namespace ipa */\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h\n>>>> index 5247425952..1eace12908 100644\n>>>> --- a/src/ipa/libipa/agc.h\n>>>> +++ b/src/ipa/libipa/agc.h\n>>>> @@ -7,13 +7,19 @@\n>>>>\n>>>> #pragma once\n>>>>\n>>>> +#include \n>>>> #include \n>>>>\n>>>> #include \n>>>>\n>>>> +#include \n>>>> #include \n>>>>\n>>>> +#include \n>>>> +\n>>>> +#include \"agc_mean_luminance.h\"\n>>>> #include \"camera_sensor_helper.h\"\n>>>> +#include \"histogram.h\"\n>>>>\n>>>> namespace libcamera {\n>>>>\n>>>> @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,\n>>>> \tcontrols.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));\n>>>> }\n>>>>\n>>>> +struct Session {\n>>>> +\tutils::Duration minExposureTime;\n>>>> +\tutils::Duration maxExposureTime;\n>>>> +\tdouble minAnalogueGain;\n>>>> +\tdouble maxAnalogueGain;\n>>>> +\tutils::Duration minFrameDuration;\n>>>> +\tutils::Duration maxFrameDuration;\n>>>> +\n>>>> +\tutils::Duration lineDuration;\n>>>> +\n>>>> +\tstruct {\n>>>> +\t\tSize outputSize;\n>>>> +\t} sensor;\n>>>> +\n>>>> +\tbool autoAllowed;\n>>>> +};\n>>>> +\n>>>> +struct ActiveState {\n>>>> +\tstruct {\n>>>> +\t\tuint32_t exposure;\n>>>> +\t\tdouble gain;\n>>>> +\t} manual;\n>>>> +\tstruct {\n>>>> +\t\tuint32_t exposure;\n>>>> +\t\tdouble gain;\n>>>> +\t\tdouble quantizationGain;\n>>>> +\t\tdouble yTarget;\n>>>> +\t} automatic;\n>>>> +\n>>>> +\tbool autoExposureEnabled;\n>>>> +\tbool autoGainEnabled;\n>>>> +\tdouble exposureValue;\n>>>> +\tcontrols::AeConstraintModeEnum constraintMode;\n>>>> +\tcontrols::AeExposureModeEnum exposureMode;\n>>>> +\tutils::Duration minFrameDuration;\n>>>> +\tutils::Duration maxFrameDuration;\n>>>> +};\n>>>> +\n>>>> +struct FrameContext {\n>>>> +\tuint32_t exposure;\n>>>> +\tdouble gain;\n>>>> +\tdouble quantizationGain;\n>>>> +\tdouble exposureValue;\n>>>> +\tdouble yTarget;\n>>>> +\tuint32_t vblank;\n>>>> +\tbool autoExposureEnabled;\n>>>> +\tbool autoGainEnabled;\n>>>> +\tcontrols::AeConstraintModeEnum constraintMode;\n>>>> +\tcontrols::AeExposureModeEnum exposureMode;\n>>>> +\tutils::Duration minFrameDuration;\n>>>> +\tutils::Duration maxFrameDuration;\n>>>> +\tutils::Duration frameDuration;\n>>>> +\tbool autoExposureModeChange;\n>>>> +\tbool autoGainModeChange;\n>>>> +};\n>>>> +\n>>>> } /* namespace agc */\n>>>>\n>>>> +class AgcAlgorithm\n>>>> +{\n>>>> +public:\n>>>> +\tstruct ConfigurationParams {\n>>>> +\t\tconst CameraSensorHelper *sensor;\n>>>> +\t\tconst IPACameraSensorInfo &sensorInfo;\n>>>> +\t\tconst ControlInfoMap &sensorControls;\n>>>> +\t\tControlInfoMap::Map &ctrlMap;\n>>>> +\t\tbool autoAllowed = true;\n>>>> +\t};\n>>>> +\n>>>> +\tint init(const ValueNode &tuningData);\n>>>> +\n>>>> +\tint configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);\n>>>\n>>> easy to shorten\n>>\n>> Done.\n>>\n>>\n>>>\n>>>> +\n>>>> +\tvoid queueRequest(const agc::Session &session, agc::ActiveState &state,\n>>>> +\t\t\t agc::FrameContext &frameContext, const ControlList &controls);\n>>>> +\n>>>> +\tvoid prepare(agc::ActiveState &state, agc::FrameContext &frameContext);\n>>>> +\n>>>> +\tstruct ProcessParams {\n>>>> +\t\tconst AgcMeanLuminance::Traits &traits;\n>>>> +\t\tconst Histogram &yHist;\n>>>> +\t\tuint32_t exposure;\n>>>> +\t\tdouble gain;\n>>>> +\t\tstd::vector &&additionalConstraints = {};\n>>>> +\t\tdouble lux = 0;\n>>>> +\t};\n>>>> +\n>>>> +\tvoid process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,\n>>>\n>>> same here\n>>\n>> Done.\n>>\n>>\n>>>\n>>>> +\t\t std::optional &¶ms, ControlList &metadata);\n>>>> +\n>>>> +private:\n>>>> +\tAgcMeanLuminance impl_;\n>>>> +};\n>>>> +\n>>>> } /* namespace ipa */\n>>>>\n>>>> } /* namespace libcamera */\n>>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>>>> index 05f1a8749c..ca681fa5af 100644\n>>>> --- a/src/ipa/libipa/meson.build\n>>>> +++ b/src/ipa/libipa/meson.build\n>>>> @@ -23,6 +23,7 @@ libipa_headers = files([\n>>>> ])\n>>>>\n>>>> libipa_sources = files([\n>>>> + 'agc.cpp',\n>>>> 'agc_mean_luminance.cpp',\n>>>> 'algorithm.cpp',\n>>>> 'awb_bayes.cpp',\n>>>\n>>> Enormous work overall!\n>>>\n>>> Agc is the most complex beast we have and I'm looking forwards to\n>>> getting this merged already!\n>>>\n>>>\n>>>> --\n>>>> 2.55.0\n>>>>\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 501ECBDE4C\n\tfor ;\n\tMon, 27 Jul 2026 14:06:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A78F767F80;\n\tMon, 27 Jul 2026 16:06:36 +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 1AAA667EB2\n\tfor ;\n\tMon, 27 Jul 2026 16:06:35 +0200 (CEST)","from [192.168.33.46] (185.182.215.156.nat.pool.zt.hu\n\t[185.182.215.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 119083A4;\n\tMon, 27 Jul 2026 16:05:31 +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=\"WUcPOaD1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785161131;\n\tbh=9Q4OUoK4ibC0k6hyisAw8Jvtk4HuYkaIQQak/CUEAFw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=WUcPOaD12ByqsTF6b6pqjazEhRTYsLloyby9OE8SiAh2Zh5A/99vg/VqjSMv6nJJM\n\tTjnJQ4cxUELIXZ4Wjhxh6Co/vLp3aOfoCf2Tb1gFj4j/o13nrXfjG1ybl1eZ1EvkrM\n\t+QxV9tTeQKIyzdJSpGeDTmiFgnoOXNNzZ2Az4KWY=","Message-ID":"","Date":"Mon, 27 Jul 2026 16:06:31 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-22-barnabas.pocze@ideasonboard.com>\n\t\n\t\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":39879,"web_url":"https://patchwork.libcamera.org/comment/39879/","msgid":"","date":"2026-07-27T14:25:55","subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Mon, Jul 27, 2026 at 04:06:31PM +0200, Barnabás Pőcze wrote:\n> 2026. 07. 27. 15:25 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Mon, Jul 27, 2026 at 10:53:33AM +0200, Barnabás Pőcze wrote:\n\n[snip]\n\n> > >\n> > > >\n> > > > > +\n> > > > > +\t/* \\todo Move this to the `Camera` class. */\n> > > > > +\tconfig.ctrlMap[&controls::AeEnable] = ControlInfo{\n> > > > > +\t\tfalse,\n> > > > > +\t\tsession.autoAllowed,\n> > > > > +\t\tsession.autoAllowed,\n> > > > > +\t};\n> > > >\n> > > > RkISP1Agc register controls in init() but I think what you're doing\n> > > > here it's better, as some control such as ExposureTimeMode depend on\n> > > > the session configuration.\n> > > >\n> > > > Is there any risk of having leftovers from a previous session in the\n> > > > control list ?\n> > >\n> > > Every control should either be overwritten or removed.\n> > >\n> >\n> > Is that because the Camera::controls info map gets reset between\n> > configure() calls ? I don't really see that happening in Camera or\n> > PipelineHandler.\n>\n> As far as I can tell, currently no IPA module clears its `ControlInfoMap::Map`\n> because most controls are added in each algorithm's `init()`. I think this is\n> again a policy question. But adjusting this might be inconvenient because I think\n> it is expected to have registered controls even before the first configuration,\n> so in effect controls will have to be added in both `init()` and `configure()`\n> by each algorithm. Or alternatively each IPA module should call `configure()`\n> right after `init()`.\n>\n\nmm, this might be tricky...\n\nOne possibilty, at the expense of efficiency, is to pass to\n:configure() a fresh control list from the IPA module, and then copy\nit back to the pipeline.\n\nBut we need to enforce a global policy for that..\n\n>\n> >\n> > Same as per the data->ipaControls_ passed by the RkISP1 pipeline handle\n> > to the IPA and used to populate Camera::controls().\n> >\n> > >\n> > > >\n> > > > > +\n> > > > > +\tif (session.autoAllowed) {\n> > > > > +\t\tconfig.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);\n> > > > > +\n> > > > > +\t\tfor (const auto &[id, info] : impl_.controls())\n> > > > > +\t\t\tconfig.ctrlMap[id] = info;\n> > > > > +\t} else {\n> > > > > +\t\tconfig.ctrlMap.erase(&controls::ExposureValue);\n> > > > > +\n> > > > > +\t\tfor (const auto &[id, info] : impl_.controls())\n> > > > > +\t\t\tconfig.ctrlMap.erase(id);\n> > > > > +\t}\n> > > >\n> > > > Maybe it happens later on, but this is a bit cumbersome. Making\n> > > > AgcMeanLuminance aware of autoAllowed would avoid this\n> > >\n> > > Possibly, but my preference is to keep more control handling out of AgcMeanLuminance.\n> > > And in any case, there will be a loop like this or equivalent, and\n> >\n> > Ok, I thought it was AgcMeanLuminance registering ExposureValue but it\n> > actually is this class doing so it here. What we're erasing are\n> > possible leftovers from a previous session maybe ?\n> >\n> > > I'm not sure it matters too much where it is.\n> >\n> > I was bothered by the need to erase, as it seems like a lot of\n> > bookeeping that could possibile go wrong.\n> >\n> > But if I get this right this is about removing controls from a\n> > previous session ? If that's the case should we pass a fresh list of\n> > controls at every ::configure() call to avoid leftovers instead of\n> > erasing them ?\n>\n> See above.\n>\n\n[snip]\n\n> > > > > +\t\tASSERT(session.autoAllowed);\n> > > >\n> > > > Does it mean IPA should not call process if (autoAllowed) ?\n> > >\n> > > Yes.\n> > >\n> >\n> > Well, not exactly.\n> >\n> > The IPA shall not provide stats if !autoAllowed\n>\n> Right, I don't know why I wrote that.\n>\n>\n> >\n> > >\n> > > >\n> > > > Can't we simply bail out here to make IPAs simpler ?\n> > >\n> > > Possibly, but I don't see how that would make them simpler. The assumption\n> >\n> > I would have liked for IPAs to be able to call ::process() with an\n> > optionally initialized ProcessParams which in case of !stats\n> > would be empty. If the core is running with !autoAllowed it will\n> > ignore the params.\n> >\n> > This doesn't work in case the system is running with autoAllowed but\n> > for some reasons you don't have valid stats. RkISP1 seems to assume it\n> > might happen you can get bad/corrupted stats, however in practice I'm\n> > not sure how it could happen.\n> >\n> >\n> > What you have right now (RkISP1 as an example)\n> >\n> > \tif (stats) {\n> > \t\tif (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)\n> > \t\t\tparams = &stats->params;\n> > \t\telse\n> > \t\t\tLOG(RkISP1Agc, Error) << \"AUTOEXP data is missing in statistics\";\n> > \t}\n> >\n> > \tif (params) {\n> > \t\tstd::vector additionalConstraints;\n> > \t\tif (context.activeState.wdr.mode != controls::WdrOff)\n> > \t\t\tadditionalConstraints.push_back(context.activeState.wdr.constraint);\n> >\n> > \t\tagc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{\n> > \t\t\t.traits = AgcTraits{\n> > \t\t\t\t{ params->ae.exp_mean, context.hw.numAeCells },\n> > \t\t\t\tmeteringModes_.at(frameContext.agc.meteringMode),\n> > \t\t\t},\n> > \t\t\t.yHist = {\n> > \t\t\t\t/* The lower 4 bits are fractional and meant to be discarded. */\n> > \t\t\t\t{ params->hist.hist_bins, context.hw.numHistogramBins },\n> > \t\t\t\t[](uint32_t x) { return x >> 4; },\n> > \t\t\t},\n> > \t\t\t.exposure = frameContext.sensor.exposure,\n> > \t\t\t/*\n> > \t\t\t * Include the quantization gain if it was applied. Do not use\n> > \t\t\t * compress.gain because it will include gains that shall not be\n> > \t\t\t * reported to the user when HDR is implemented.\n> > \t\t\t */\n> > \t\t\t.gain = frameContext.sensor.gain\n> > \t\t\t * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),\n> > \t\t\t.additionalConstraints = std::move(additionalConstraints),\n> > \t\t\t.lux = frameContext.lux.lux,\n> > \t\t}}, metadata);\n> > \t} else {\n> > \t\tagc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);\n> > \t}\n> >\n> > Requires the IPA to validate stats and have to distinct calls to\n> > agc_.process()\n> >\n> > while\n> >\n> > agc::ProcessParams params {};\n> > if (stats && stats & ... )\n> > /* populate params. */\n> > }\n> >\n> > agc_.process( ... , ..., params, ... )\n>\n> The issue is the \"traits\" and \"yHist\" members, they are references,\n\nI tried to compile the above snippet and quickly found that out\n\n> so the referenced things must stay alive for the duration of the call.\n> The above snippet you suggested is syntactically possible, but avoiding\n> use-after-free issues makes it a bit cumbersome.\n\nWell, we can construct/copy inside ProcessParams..\n\nRight now, you're taking a reference to an object constructed inside\nthe call location\n\n\t.traits = AgcTraits{\n\t\t{ params->ae.exp_mean, context.hw.numAeCells },\n\t\tmeteringModes_.at(frameContext.agc.meteringMode),\n\t},\n\t.yHist = {\n\t\t/* The lower 4 bits are fractional and meant to be discarded. */\n\t\t{ params->hist.hist_bins, context.hw.numHistogramBins },\n\t\t[](uint32_t x) { return x >> 4; },\n\t},\n\nI don't think constructing them in place would be too different.\n\nAnyway, don't bother for now..\n\n\n\n>\n>\n> >\n> > is less verbose, but would require the core to bail out in case\n> > (params.empty() || !autoAllowed)\n> >\n> > This mean you would need a way to distinguish empty param blocks and\n> > you can't use the declarative style you have here, which I kind of\n> > like as well..\n> >\n> > not sure, I have a feeling IPA could be made simpler, but at the same\n> > time this seems more robust\n> >\n> > > is that `autoAllowed == false` means that statistics will not be available\n> > > (and hence no automatic control) (usually for raw capture - but there is no\n> > > technical reason why agc could not run even for raw capture if statistics\n> > > are available), so providing statistics is an api violation in a sense, so\n> > > I think the assert is reasonable.\n> > >\n> >\n> > We can start strict and see. After all this is not something\n> > application could trigger.\n> >\n> > >\n> > > >\n> > > > > +\n> > > > > +\t\t/*\n> > > > > +\t\t* Set the AGC limits using the fixed exposure time and/or gain in\n> > > > > +\t\t* manual mode, or the sensor limits in auto mode.\n> > > > > +\t\t*/\n> > > >\n> > > > Weird indent\n> > >\n> > > Fixed.\n> > >\n> > > >\n> > > > > +\t\tutils::Duration minExposureTime;\n> > > > > +\t\tutils::Duration maxExposureTime;\n> > > > > +\t\tdouble minAnalogueGain;\n> > > > > +\t\tdouble maxAnalogueGain;\n> > > > > +\n> > > > > +\t\tif (frameContext.autoExposureEnabled) {\n> > > > > +\t\t\tminExposureTime = session.minExposureTime;\n> > > > > +\t\t\tmaxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);\n> > > >\n> > > > very long line\n> > >\n> > > Adjusted.\n> > >\n> > >\n> > > >\n> > > > > +\t\t} else {\n> > > > > +\t\t\tminExposureTime = lineDuration * frameContext.exposure;\n> > > > > +\t\t\tmaxExposureTime = minExposureTime;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tif (frameContext.autoGainEnabled) {\n> > > > > +\t\t\tminAnalogueGain = session.minAnalogueGain;\n> > > > > +\t\t\tmaxAnalogueGain = session.maxAnalogueGain;\n> > > > > +\t\t} else {\n> > > > > +\t\t\tminAnalogueGain = frameContext.gain;\n> > > > > +\t\t\tmaxAnalogueGain = frameContext.gain;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\t/*\n> > > > > +\t\t* The Agc algorithm needs to know the effective exposure value that was\n> > > > > +\t\t* applied to the sensor when the statistics were collected.\n> > > > > +\t\t*/\n> > > > > +\t\tutils::Duration effectiveExposureValue =\n> > > > > +\t\t\tlineDuration * params->exposure * params->gain;\n> > > >\n> > > > RkISP1 gets exposure and gain from the frameContext.\n> > > >\n> > > > I see in the next patches that port RkISP1 to AgcAlgorithm this hunk\n> > > >\n> > > > agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc,\n> > > >\n> > > > ...\n> > > >\n> > > > .exposure = frameContext.sensor.exposure,\n> > > > /*\n> > > > * Include the quantization gain if it was applied. Do not use\n> > > > * compress.gain because it will include gains that shall not be\n> > > > * reported to the user when HDR is implemented.\n> > > > */\n> > > > .gain = frameContext.sensor.gain\n> > > > * (frameContext.compress.enable ? frameContext.agc.quantizationGain : 1),\n> > > >\n> > > > Is the reason you can't take exposure and gain from the FrameContext\n> > > > the gain adjustment ?\n> > >\n> > > That, and the fact that it is `frameContext.sensor`, which is separate\n> > > from `frameContext.agc` (i.e. `agc::FrameContext`), because it needs\n> > > the exposure/gain that was actually in effect.\n> > >\n> >\n> > Ack, too bad, the Agc::process() interface is quite verbose already :(\n> >\n> > >\n> > > >\n> > > > > +\n> > > > > +\t\timpl_.setLimits(minExposureTime, maxExposureTime,\n> > > > > +\t\t\t\tminAnalogueGain, maxAnalogueGain,\n> > > > > +\t\t\t\tstd::move(params->additionalConstraints));\n> > > > > +\n> > > > > +\t\tconst auto &newEv = impl_.calculateNewEv({\n> > > > > +\t\t\t.traits = params->traits,\n> > > > > +\t\t\t.yHist = params->yHist,\n> > > > > +\t\t\t.effectiveExposureValue = effectiveExposureValue,\n> > > > > +\t\t\t.constraintModeIndex = frameContext.constraintMode,\n> > > > > +\t\t\t.exposureModeIndex = frameContext.exposureMode,\n> > > > > +\t\t\t.lux = params->lux,\n> > > > > +\t\t\t.exposureCompensation = pow(2.0, frameContext.exposureValue),\n> > > > > +\t\t});\n> > > > > +\n> > > > > +\t\tLOG(Agc, Debug)\n> > > > > +\t\t\t<< \"Divided up exposure time, analogue gain, quantization gain\"\n> > > > > +\t\t\t<< \" and digital gain are \" << newEv.exposureTime << \", \" << newEv.analogueGain\n> > > > > +\t\t\t<< \", \" << newEv.quantizationGain << \" and \" << newEv.digitalGain;\n> > > > > +\n> > > > > +\t\t/* Update the estimated exposure and gain. */\n> > > > > +\t\tstate.automatic.exposure = newEv.exposureTime / lineDuration;\n> > > > > +\t\tstate.automatic.gain = newEv.analogueGain;\n> > > > > +\t\tstate.automatic.quantizationGain = newEv.quantizationGain;\n> > > > > +\t\tstate.automatic.yTarget = newEv.yTarget;\n> > > > > +\n> > > > > +\t\tnewExposureTime = newEv.exposureTime;\n> > > > > +\t}\n> > > > > +\n> > > > > +\t/*\n> > > > > +\t * Expand the target frame duration so that we do not run faster than\n> > > > > +\t * the minimum frame duration when we have short exposures.\n> > > > > +\t */\n> > > > > +\tconst auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);\n> > > > > +\tframeContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;\n> > > > > +\n> > > > > +\t/* Update frame duration accounting for line length quantization. */\n> > > > > +\tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;\n> > > >\n> > > > Easy to shorten to\n> > > >\n> > > > \tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank)\n> > > > \t\t\t\t * lineDuration;\n> > > >\n> > >\n> > > This was also copied verbatim.\n> > >\n> > >\n> > > > > +\n> > > > > +\tmetadata.set(controls::AnalogueGain, frameContext.gain);\n> > > > > +\tmetadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get());\n> > > >\n> > > > I'm not sure I understand the strategy here.. this line is very long\n> > > > and could be easily made\n> > > >\n> > > > \tmetadata.set(controls::ExposureTime,\n> > > > \t\t utils::Duration(lineDuration * frameContext.exposure).get());\n> > > >\n> > >\n> > > Adjusted.\n> > >\n> > >\n> > > >\n> > > > > +\tmetadata.set(controls::FrameDuration, frameContext.frameDuration.get());\n> > > > > +\tmetadata.set(controls::ExposureTimeMode,\n> > > > > +\t\t frameContext.autoExposureEnabled\n> > > > > +\t\t ? controls::ExposureTimeModeAuto\n> > > > > +\t\t : controls::ExposureTimeModeManual);\n> > > > > +\tmetadata.set(controls::AnalogueGainMode,\n> > > > > +\t\t frameContext.autoGainEnabled\n> > > > > +\t\t ? controls::AnalogueGainModeAuto\n> > > > > +\t\t : controls::AnalogueGainModeManual);\n> > > >\n> > > > These ones instead are very short and could easily be made\n> > > >\n> > > > \tmetadata.set(controls::ExposureTimeMode, frameContext.autoExposureEnabled ?\n> > > > \t\t controls::ExposureTimeModeAuto : controls::ExposureTimeModeManual);\n> > >\n> > >\n> > > These were copied verbatim. So please confirm if you want me to adjust the\n> > > verbatim copies here and in the whole file.\n> >\n> > As said, it's not that we're moving code in a single patch where we\n> > can compare the lines moved around in the diff.\n> >\n> > The way I did the review was looking at the code in Agc and RkISP1Agc\n> > and they are of course different already, so I don't think we should\n> > stick to verbatim copies when there's something trivially improvable\n> > like spanning to the whole line length\n> >\n> > >\n> > >\n> > > >\n> > > > > +\n> > > > > +\tmetadata.set(controls::AeExposureMode, frameContext.exposureMode);\n> > > > > +\tmetadata.set(controls::AeConstraintMode, frameContext.constraintMode);\n> > > > > +\tmetadata.set(controls::ExposureValue, frameContext.exposureValue);\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace ipa */\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h\n> > > > > index 5247425952..1eace12908 100644\n> > > > > --- a/src/ipa/libipa/agc.h\n> > > > > +++ b/src/ipa/libipa/agc.h\n> > > > > @@ -7,13 +7,19 @@\n> > > > >\n> > > > > #pragma once\n> > > > >\n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"agc_mean_luminance.h\"\n> > > > > #include \"camera_sensor_helper.h\"\n> > > > > +#include \"histogram.h\"\n> > > > >\n> > > > > namespace libcamera {\n> > > > >\n> > > > > @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,\n> > > > > \tcontrols.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));\n> > > > > }\n> > > > >\n> > > > > +struct Session {\n> > > > > +\tutils::Duration minExposureTime;\n> > > > > +\tutils::Duration maxExposureTime;\n> > > > > +\tdouble minAnalogueGain;\n> > > > > +\tdouble maxAnalogueGain;\n> > > > > +\tutils::Duration minFrameDuration;\n> > > > > +\tutils::Duration maxFrameDuration;\n> > > > > +\n> > > > > +\tutils::Duration lineDuration;\n> > > > > +\n> > > > > +\tstruct {\n> > > > > +\t\tSize outputSize;\n> > > > > +\t} sensor;\n> > > > > +\n> > > > > +\tbool autoAllowed;\n> > > > > +};\n> > > > > +\n> > > > > +struct ActiveState {\n> > > > > +\tstruct {\n> > > > > +\t\tuint32_t exposure;\n> > > > > +\t\tdouble gain;\n> > > > > +\t} manual;\n> > > > > +\tstruct {\n> > > > > +\t\tuint32_t exposure;\n> > > > > +\t\tdouble gain;\n> > > > > +\t\tdouble quantizationGain;\n> > > > > +\t\tdouble yTarget;\n> > > > > +\t} automatic;\n> > > > > +\n> > > > > +\tbool autoExposureEnabled;\n> > > > > +\tbool autoGainEnabled;\n> > > > > +\tdouble exposureValue;\n> > > > > +\tcontrols::AeConstraintModeEnum constraintMode;\n> > > > > +\tcontrols::AeExposureModeEnum exposureMode;\n> > > > > +\tutils::Duration minFrameDuration;\n> > > > > +\tutils::Duration maxFrameDuration;\n> > > > > +};\n> > > > > +\n> > > > > +struct FrameContext {\n> > > > > +\tuint32_t exposure;\n> > > > > +\tdouble gain;\n> > > > > +\tdouble quantizationGain;\n> > > > > +\tdouble exposureValue;\n> > > > > +\tdouble yTarget;\n> > > > > +\tuint32_t vblank;\n> > > > > +\tbool autoExposureEnabled;\n> > > > > +\tbool autoGainEnabled;\n> > > > > +\tcontrols::AeConstraintModeEnum constraintMode;\n> > > > > +\tcontrols::AeExposureModeEnum exposureMode;\n> > > > > +\tutils::Duration minFrameDuration;\n> > > > > +\tutils::Duration maxFrameDuration;\n> > > > > +\tutils::Duration frameDuration;\n> > > > > +\tbool autoExposureModeChange;\n> > > > > +\tbool autoGainModeChange;\n> > > > > +};\n> > > > > +\n> > > > > } /* namespace agc */\n> > > > >\n> > > > > +class AgcAlgorithm\n> > > > > +{\n> > > > > +public:\n> > > > > +\tstruct ConfigurationParams {\n> > > > > +\t\tconst CameraSensorHelper *sensor;\n> > > > > +\t\tconst IPACameraSensorInfo &sensorInfo;\n> > > > > +\t\tconst ControlInfoMap &sensorControls;\n> > > > > +\t\tControlInfoMap::Map &ctrlMap;\n> > > > > +\t\tbool autoAllowed = true;\n> > > > > +\t};\n> > > > > +\n> > > > > +\tint init(const ValueNode &tuningData);\n> > > > > +\n> > > > > +\tint configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);\n> > > >\n> > > > easy to shorten\n> > >\n> > > Done.\n> > >\n> > >\n> > > >\n> > > > > +\n> > > > > +\tvoid queueRequest(const agc::Session &session, agc::ActiveState &state,\n> > > > > +\t\t\t agc::FrameContext &frameContext, const ControlList &controls);\n> > > > > +\n> > > > > +\tvoid prepare(agc::ActiveState &state, agc::FrameContext &frameContext);\n> > > > > +\n> > > > > +\tstruct ProcessParams {\n> > > > > +\t\tconst AgcMeanLuminance::Traits &traits;\n> > > > > +\t\tconst Histogram &yHist;\n> > > > > +\t\tuint32_t exposure;\n> > > > > +\t\tdouble gain;\n> > > > > +\t\tstd::vector &&additionalConstraints = {};\n> > > > > +\t\tdouble lux = 0;\n> > > > > +\t};\n> > > > > +\n> > > > > +\tvoid process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,\n> > > >\n> > > > same here\n> > >\n> > > Done.\n> > >\n> > >\n> > > >\n> > > > > +\t\t std::optional &¶ms, ControlList &metadata);\n> > > > > +\n> > > > > +private:\n> > > > > +\tAgcMeanLuminance impl_;\n> > > > > +};\n> > > > > +\n> > > > > } /* namespace ipa */\n> > > > >\n> > > > > } /* namespace libcamera */\n> > > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > > > index 05f1a8749c..ca681fa5af 100644\n> > > > > --- a/src/ipa/libipa/meson.build\n> > > > > +++ b/src/ipa/libipa/meson.build\n> > > > > @@ -23,6 +23,7 @@ libipa_headers = files([\n> > > > > ])\n> > > > >\n> > > > > libipa_sources = files([\n> > > > > + 'agc.cpp',\n> > > > > 'agc_mean_luminance.cpp',\n> > > > > 'algorithm.cpp',\n> > > > > 'awb_bayes.cpp',\n> > > >\n> > > > Enormous work overall!\n> > > >\n> > > > Agc is the most complex beast we have and I'm looking forwards to\n> > > > getting this merged already!\n> > > >\n> > > >\n> > > > > --\n> > > > > 2.55.0\n> > > > >\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 00ADEBDE4C\n\tfor ;\n\tMon, 27 Jul 2026 14:26:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7958067F76;\n\tMon, 27 Jul 2026 16:26:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B2C667EB2\n\tfor ;\n\tMon, 27 Jul 2026 16:25:59 +0200 (CEST)","from ideasonboard.com (mob-5-90-50-102.net.vodafone.it\n\t[5.90.50.102])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 298663A4;\n\tMon, 27 Jul 2026 16:24:55 +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=\"SBYQxXAA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785162295;\n\tbh=vPGW8c0e6fJM0YAZWPOD1YD2zSwR2/bXpLg4gTzCtxw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SBYQxXAAQG1YRi76Vdc9Ve0/Nr7AFTv8R656E187jaQDs/WI2yCtaM7TqOYbfeOAA\n\td/95GAPjSiJPws5X2NukSGIvdPxVVqCVTHTtIn4C6EQ2CuEOdGlsR0eg8CVjo6F9nO\n\tfq/8FMZ2yaYjJ/IhKs5JmbBHUhrnocoe3gPLWeqs=","Date":"Mon, 27 Jul 2026 16:25:55 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","Message-ID":"","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-22-barnabas.pocze@ideasonboard.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":39880,"web_url":"https://patchwork.libcamera.org/comment/39880/","msgid":"","date":"2026-07-27T14:29:23","subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n let me start a fresh reply, on a separate little thing\n\nOn Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:\n> Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`\n> based on the rkisp1 `Agc` algorithm, with slight adjustments.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/ipa/libipa/agc.cpp | 639 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/agc.h | 98 ++++++\n> src/ipa/libipa/meson.build | 1 +\n> 3 files changed, 738 insertions(+)\n> create mode 100644 src/ipa/libipa/agc.cpp\n>\n> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n> new file mode 100644\n> index 0000000000..e16a02fdde\n> --- /dev/null\n> +++ b/src/ipa/libipa/agc.cpp\n> @@ -0,0 +1,639 @@\n\n[snip]\n\n> + * \\brief Handle a \\a queueRequest operation\n> + */\n> +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,\n> +\t\t\t\tagc::FrameContext &frameContext, const ControlList &controls)\n> +{\n> +\tif (session.autoAllowed) {\n> +\t\tconst auto &aeEnable = controls.get(controls::ExposureTimeMode);\n> +\t\tif (aeEnable &&\n> +\t\t (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {\n\nif !autoAllowed we initialize\n\n state.autoExposureEnabled = false;\n\nand only register controls::ExposureTimeModeManual as an option for\ncontrol::ExposureTimeMode.\n\nThe above condition will always result then in\n\n if (aeEnable && (false != false))\n\nmaking this a nop.\n\nDo we then need to wrap this code with:\n\n\tif (session.autoAllowed) {\n\n\n }\n\nat all ?\n\n> +\t\t\tstate.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);\n> +\n> +\t\t\tLOG(Agc, Debug)\n> +\t\t\t\t<< (state.autoExposureEnabled ? \"Enabling\" : \"Disabling\")\n> +\t\t\t\t<< \" AGC (exposure)\";\n> +\n> +\t\t\t/*\n> +\t\t\t * If we go from auto -> manual with no manual control\n> +\t\t\t * set, use the last computed value, which we don't\n> +\t\t\t * know until prepare() so save this information.\n> +\t\t\t *\n> +\t\t\t * \\todo Check the previous frame at prepare() time\n> +\t\t\t * instead of saving a flag here\n> +\t\t\t */\n> +\t\t\tif (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))\n> +\t\t\t\tframeContext.autoExposureModeChange = true;\n> +\t\t}\n> +\n> +\t\tconst auto &agEnable = controls.get(controls::AnalogueGainMode);\n> +\t\tif (agEnable &&\n> +\t\t (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {\n> +\t\t\tstate.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);\n> +\n> +\t\t\tLOG(Agc, Debug)\n> +\t\t\t\t<< (state.autoGainEnabled ? \"Enabling\" : \"Disabling\")\n> +\t\t\t\t<< \" AGC (gain)\";\n> +\t\t\t/*\n> +\t\t\t * If we go from auto -> manual with no manual control\n> +\t\t\t * set, use the last computed value, which we don't\n> +\t\t\t * know until prepare() so save this information.\n> +\t\t\t */\n> +\t\t\tif (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))\n> +\t\t\t\tframeContext.autoGainModeChange = true;\n> +\t\t}\n> +\t}\n> +\n> +\tconst auto &exposure = controls.get(controls::ExposureTime);\n> +\tif (exposure && !state.autoExposureEnabled) {\n> +\t\tstate.manual.exposure = *exposure * 1.0us / session.lineDuration;\n> +\n> +\t\tLOG(Agc, Debug)\n> +\t\t\t<< \"Set exposure to \" << state.manual.exposure;\n> +\t}\n> +\n> +\tconst auto &gain = controls.get(controls::AnalogueGain);\n> +\tif (gain && !state.autoGainEnabled) {\n> +\t\tstate.manual.gain = *gain;\n> +\n> +\t\tLOG(Agc, Debug) << \"Set gain to \" << state.manual.gain;\n> +\t}\n> +\n> +\tframeContext.autoExposureEnabled = state.autoExposureEnabled;\n> +\tframeContext.autoGainEnabled = state.autoGainEnabled;\n> +\n> +\tif (!frameContext.autoExposureEnabled)\n> +\t\tframeContext.exposure = state.manual.exposure;\n> +\tif (!frameContext.autoGainEnabled)\n> +\t\tframeContext.gain = state.manual.gain;\n> +\n> +\tif (!frameContext.autoExposureEnabled &&\n> +\t !frameContext.autoGainEnabled)\n> +\t\tframeContext.quantizationGain = 1.0;\n> +\n> +\tconst auto &exposureMode = controls.get(controls::AeExposureMode);\n> +\tif (exposureMode)\n> +\t\tstate.exposureMode =\n> +\t\t\tstatic_cast(*exposureMode);\n> +\tframeContext.exposureMode = state.exposureMode;\n> +\n> +\tconst auto &constraintMode = controls.get(controls::AeConstraintMode);\n> +\tif (constraintMode)\n> +\t\tstate.constraintMode =\n> +\t\t\tstatic_cast(*constraintMode);\n> +\tframeContext.constraintMode = state.constraintMode;\n> +\n> +\tconst auto &exposureValue = controls.get(controls::ExposureValue);\n> +\tif (exposureValue)\n> +\t\tstate.exposureValue = *exposureValue;\n> +\tframeContext.exposureValue = state.exposureValue;\n> +\n> +\tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> +\tif (frameDurationLimits) {\n> +\t\t/* Limit the control value to the limits in ControlInfo */\n> +\t\tstate.minFrameDuration = std::clamp(\n> +\t\t\tstd::chrono::microseconds((*frameDurationLimits).front()),\n> +\t\t\tsession.minFrameDuration,\n> +\t\t\tsession.maxFrameDuration\n> +\t\t);\n> +\n> +\t\tstate.maxFrameDuration = std::clamp(\n> +\t\t\tstd::chrono::microseconds((*frameDurationLimits).back()),\n> +\t\t\tsession.minFrameDuration,\n> +\t\t\tsession.maxFrameDuration\n> +\t\t);\n> +\t}\n> +\tframeContext.minFrameDuration = state.minFrameDuration;\n> +\tframeContext.maxFrameDuration = state.maxFrameDuration;\n> +}\n> +\n> +/**\n> + * \\brief Handle a \\a prepare operation\n> + */\n> +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)\n> +{\n> +\tuint32_t activeAutoExposure = state.automatic.exposure;\n> +\tdouble activeAutoGain = state.automatic.gain;\n> +\tdouble activeAutoQGain = state.automatic.quantizationGain;\n> +\n> +\t/* Populate exposure and gain in auto mode */\n> +\tif (frameContext.autoExposureEnabled) {\n> +\t\tframeContext.exposure = activeAutoExposure;\n> +\t\tframeContext.quantizationGain = activeAutoQGain;\n> +\t}\n> +\tif (frameContext.autoGainEnabled) {\n> +\t\tframeContext.gain = activeAutoGain;\n> +\t\tframeContext.quantizationGain = activeAutoQGain;\n> +\t}\n> +\n> +\t/*\n> +\t * Populate manual exposure and gain from the active auto values when\n> +\t * transitioning from auto to manual\n> +\t */\n> +\tif (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {\n> +\t\tstate.manual.exposure = activeAutoExposure;\n> +\t\tframeContext.exposure = activeAutoExposure;\n> +\t}\n> +\tif (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {\n> +\t\tstate.manual.gain = activeAutoGain;\n> +\t\tframeContext.gain = activeAutoGain;\n> +\t\tframeContext.quantizationGain = activeAutoQGain;\n> +\t}\n> +\n> +\tframeContext.yTarget = state.automatic.yTarget;\n> +}\n> +\n> +/**\n> + * \\brief Handle a \\a process operation\n> + */\n> +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,\n> +\t\t\t agc::FrameContext &frameContext, std::optional &¶ms,\n> +\t\t\t ControlList &metadata)\n> +{\n> +\tconst utils::Duration &lineDuration = session.lineDuration;\n> +\tutils::Duration newExposureTime = {};\n> +\n> +\tif (params) {\n> +\t\tASSERT(session.autoAllowed);\n> +\n> +\t\t/*\n> +\t\t* Set the AGC limits using the fixed exposure time and/or gain in\n> +\t\t* manual mode, or the sensor limits in auto mode.\n> +\t\t*/\n> +\t\tutils::Duration minExposureTime;\n> +\t\tutils::Duration maxExposureTime;\n> +\t\tdouble minAnalogueGain;\n> +\t\tdouble maxAnalogueGain;\n> +\n> +\t\tif (frameContext.autoExposureEnabled) {\n> +\t\t\tminExposureTime = session.minExposureTime;\n> +\t\t\tmaxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);\n> +\t\t} else {\n> +\t\t\tminExposureTime = lineDuration * frameContext.exposure;\n> +\t\t\tmaxExposureTime = minExposureTime;\n> +\t\t}\n> +\n> +\t\tif (frameContext.autoGainEnabled) {\n> +\t\t\tminAnalogueGain = session.minAnalogueGain;\n> +\t\t\tmaxAnalogueGain = session.maxAnalogueGain;\n> +\t\t} else {\n> +\t\t\tminAnalogueGain = frameContext.gain;\n> +\t\t\tmaxAnalogueGain = frameContext.gain;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t* The Agc algorithm needs to know the effective exposure value that was\n> +\t\t* applied to the sensor when the statistics were collected.\n> +\t\t*/\n> +\t\tutils::Duration effectiveExposureValue =\n> +\t\t\tlineDuration * params->exposure * params->gain;\n> +\n> +\t\timpl_.setLimits(minExposureTime, maxExposureTime,\n> +\t\t\t\tminAnalogueGain, maxAnalogueGain,\n> +\t\t\t\tstd::move(params->additionalConstraints));\n> +\n> +\t\tconst auto &newEv = impl_.calculateNewEv({\n> +\t\t\t.traits = params->traits,\n> +\t\t\t.yHist = params->yHist,\n> +\t\t\t.effectiveExposureValue = effectiveExposureValue,\n> +\t\t\t.constraintModeIndex = frameContext.constraintMode,\n> +\t\t\t.exposureModeIndex = frameContext.exposureMode,\n> +\t\t\t.lux = params->lux,\n> +\t\t\t.exposureCompensation = pow(2.0, frameContext.exposureValue),\n> +\t\t});\n> +\n> +\t\tLOG(Agc, Debug)\n> +\t\t\t<< \"Divided up exposure time, analogue gain, quantization gain\"\n> +\t\t\t<< \" and digital gain are \" << newEv.exposureTime << \", \" << newEv.analogueGain\n> +\t\t\t<< \", \" << newEv.quantizationGain << \" and \" << newEv.digitalGain;\n> +\n> +\t\t/* Update the estimated exposure and gain. */\n> +\t\tstate.automatic.exposure = newEv.exposureTime / lineDuration;\n> +\t\tstate.automatic.gain = newEv.analogueGain;\n> +\t\tstate.automatic.quantizationGain = newEv.quantizationGain;\n> +\t\tstate.automatic.yTarget = newEv.yTarget;\n> +\n> +\t\tnewExposureTime = newEv.exposureTime;\n> +\t}\n> +\n> +\t/*\n> +\t * Expand the target frame duration so that we do not run faster than\n> +\t * the minimum frame duration when we have short exposures.\n> +\t */\n> +\tconst auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);\n> +\tframeContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;\n> +\n> +\t/* Update frame duration accounting for line length quantization. */\n> +\tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;\n> +\n> +\tmetadata.set(controls::AnalogueGain, frameContext.gain);\n> +\tmetadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get());\n> +\tmetadata.set(controls::FrameDuration, frameContext.frameDuration.get());\n> +\tmetadata.set(controls::ExposureTimeMode,\n> +\t\t frameContext.autoExposureEnabled\n> +\t\t ? controls::ExposureTimeModeAuto\n> +\t\t : controls::ExposureTimeModeManual);\n> +\tmetadata.set(controls::AnalogueGainMode,\n> +\t\t frameContext.autoGainEnabled\n> +\t\t ? controls::AnalogueGainModeAuto\n> +\t\t : controls::AnalogueGainModeManual);\n> +\n> +\tmetadata.set(controls::AeExposureMode, frameContext.exposureMode);\n> +\tmetadata.set(controls::AeConstraintMode, frameContext.constraintMode);\n> +\tmetadata.set(controls::ExposureValue, frameContext.exposureValue);\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h\n> index 5247425952..1eace12908 100644\n> --- a/src/ipa/libipa/agc.h\n> +++ b/src/ipa/libipa/agc.h\n> @@ -7,13 +7,19 @@\n>\n> #pragma once\n>\n> +#include \n> #include \n>\n> #include \n>\n> +#include \n> #include \n>\n> +#include \n> +\n> +#include \"agc_mean_luminance.h\"\n> #include \"camera_sensor_helper.h\"\n> +#include \"histogram.h\"\n>\n> namespace libcamera {\n>\n> @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,\n> \tcontrols.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));\n> }\n>\n> +struct Session {\n> +\tutils::Duration minExposureTime;\n> +\tutils::Duration maxExposureTime;\n> +\tdouble minAnalogueGain;\n> +\tdouble maxAnalogueGain;\n> +\tutils::Duration minFrameDuration;\n> +\tutils::Duration maxFrameDuration;\n> +\n> +\tutils::Duration lineDuration;\n> +\n> +\tstruct {\n> +\t\tSize outputSize;\n> +\t} sensor;\n> +\n> +\tbool autoAllowed;\n> +};\n> +\n> +struct ActiveState {\n> +\tstruct {\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t} manual;\n> +\tstruct {\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t\tdouble quantizationGain;\n> +\t\tdouble yTarget;\n> +\t} automatic;\n> +\n> +\tbool autoExposureEnabled;\n> +\tbool autoGainEnabled;\n> +\tdouble exposureValue;\n> +\tcontrols::AeConstraintModeEnum constraintMode;\n> +\tcontrols::AeExposureModeEnum exposureMode;\n> +\tutils::Duration minFrameDuration;\n> +\tutils::Duration maxFrameDuration;\n> +};\n> +\n> +struct FrameContext {\n> +\tuint32_t exposure;\n> +\tdouble gain;\n> +\tdouble quantizationGain;\n> +\tdouble exposureValue;\n> +\tdouble yTarget;\n> +\tuint32_t vblank;\n> +\tbool autoExposureEnabled;\n> +\tbool autoGainEnabled;\n> +\tcontrols::AeConstraintModeEnum constraintMode;\n> +\tcontrols::AeExposureModeEnum exposureMode;\n> +\tutils::Duration minFrameDuration;\n> +\tutils::Duration maxFrameDuration;\n> +\tutils::Duration frameDuration;\n> +\tbool autoExposureModeChange;\n> +\tbool autoGainModeChange;\n> +};\n> +\n> } /* namespace agc */\n>\n> +class AgcAlgorithm\n> +{\n> +public:\n> +\tstruct ConfigurationParams {\n> +\t\tconst CameraSensorHelper *sensor;\n> +\t\tconst IPACameraSensorInfo &sensorInfo;\n> +\t\tconst ControlInfoMap &sensorControls;\n> +\t\tControlInfoMap::Map &ctrlMap;\n> +\t\tbool autoAllowed = true;\n> +\t};\n> +\n> +\tint init(const ValueNode &tuningData);\n> +\n> +\tint configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);\n> +\n> +\tvoid queueRequest(const agc::Session &session, agc::ActiveState &state,\n> +\t\t\t agc::FrameContext &frameContext, const ControlList &controls);\n> +\n> +\tvoid prepare(agc::ActiveState &state, agc::FrameContext &frameContext);\n> +\n> +\tstruct ProcessParams {\n> +\t\tconst AgcMeanLuminance::Traits &traits;\n> +\t\tconst Histogram &yHist;\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t\tstd::vector &&additionalConstraints = {};\n> +\t\tdouble lux = 0;\n> +\t};\n> +\n> +\tvoid process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,\n> +\t\t std::optional &¶ms, ControlList &metadata);\n> +\n> +private:\n> +\tAgcMeanLuminance impl_;\n> +};\n> +\n> } /* namespace ipa */\n>\n> } /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 05f1a8749c..ca681fa5af 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -23,6 +23,7 @@ libipa_headers = files([\n> ])\n>\n> libipa_sources = files([\n> + 'agc.cpp',\n> 'agc_mean_luminance.cpp',\n> 'algorithm.cpp',\n> 'awb_bayes.cpp',\n> --\n> 2.55.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D3052BE080\n\tfor ;\n\tMon, 27 Jul 2026 14:29:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C30B267F85;\n\tMon, 27 Jul 2026 16:29:32 +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 F250C67EB2\n\tfor ;\n\tMon, 27 Jul 2026 16:29:30 +0200 (CEST)","from ideasonboard.com (mob-5-90-50-102.net.vodafone.it\n\t[5.90.50.102])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ECA723A4;\n\tMon, 27 Jul 2026 16:28:26 +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=\"QPGaSYHN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785162507;\n\tbh=l2dZ4PzDSIVdSbUfSetooc1X4NP1L6/sW3Ec6LReeXM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QPGaSYHNbx5Hau0Wm1rzEubSORAU6EeL6KT9raDgKY9Ti0OOLRRrmM/rxJneL1AlS\n\tamtiCsIz3mK3ttc6+4T58e8HG3NCzUwqGjHyCWnZodYenY5mlS3YtrMKdewmbpxYeq\n\thvd2P12nRHhQNW4ElzRv0pprZj8gpJCANVzPsllY=","Date":"Mon, 27 Jul 2026 16:29:23 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","Message-ID":"","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-22-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20260723154327.1357866-22-barnabas.pocze@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":39885,"web_url":"https://patchwork.libcamera.org/comment/39885/","msgid":"<1280fb2f-df68-47da-8093-2873814a065e@ideasonboard.com>","date":"2026-07-27T15:01:20","subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2026. 07. 27. 16:29 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> let me start a fresh reply, on a separate little thing\n> \n> On Thu, Jul 23, 2026 at 05:43:04PM +0200, Barnabás Pőcze wrote:\n>> Add a class that implements the `Algorithm` interface using `AgcMeanLuminance`\n>> based on the rkisp1 `Agc` algorithm, with slight adjustments.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/ipa/libipa/agc.cpp | 639 +++++++++++++++++++++++++++++++++++++\n>> src/ipa/libipa/agc.h | 98 ++++++\n>> src/ipa/libipa/meson.build | 1 +\n>> 3 files changed, 738 insertions(+)\n>> create mode 100644 src/ipa/libipa/agc.cpp\n>>\n>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n>> new file mode 100644\n>> index 0000000000..e16a02fdde\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/agc.cpp\n>> @@ -0,0 +1,639 @@\n> \n> [snip]\n> \n>> + * \\brief Handle a \\a queueRequest operation\n>> + */\n>> +void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &state,\n>> +\t\t\t\tagc::FrameContext &frameContext, const ControlList &controls)\n>> +{\n>> +\tif (session.autoAllowed) {\n>> +\t\tconst auto &aeEnable = controls.get(controls::ExposureTimeMode);\n>> +\t\tif (aeEnable &&\n>> +\t\t (*aeEnable == controls::ExposureTimeModeAuto) != state.autoExposureEnabled) {\n> \n> if !autoAllowed we initialize\n> \n> state.autoExposureEnabled = false;\n> \n> and only register controls::ExposureTimeModeManual as an option for\n> control::ExposureTimeMode.\n> \n> The above condition will always result then in\n> \n> if (aeEnable && (false != false))\n> \n> making this a nop.\n> \n> Do we then need to wrap this code with:\n> \n> \tif (session.autoAllowed) {\n> \n> \n> }\n> \n> at all ?\n\nThere is no control value validation, so the question is how to handle invalid\nvalues. If it's \"undefined\", then the check can be dropped. I think this is also\na policy question that's worth discussing.\n\n\n\n> \n>> +\t\t\tstate.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);\n>> +\n>> +\t\t\tLOG(Agc, Debug)\n>> +\t\t\t\t<< (state.autoExposureEnabled ? \"Enabling\" : \"Disabling\")\n>> +\t\t\t\t<< \" AGC (exposure)\";\n>> +\n>> +\t\t\t/*\n>> +\t\t\t * If we go from auto -> manual with no manual control\n>> +\t\t\t * set, use the last computed value, which we don't\n>> +\t\t\t * know until prepare() so save this information.\n>> +\t\t\t *\n>> +\t\t\t * \\todo Check the previous frame at prepare() time\n>> +\t\t\t * instead of saving a flag here\n>> +\t\t\t */\n>> +\t\t\tif (!state.autoExposureEnabled && !controls.get(controls::ExposureTime))\n>> +\t\t\t\tframeContext.autoExposureModeChange = true;\n>> +\t\t}\n>> +\n>> +\t\tconst auto &agEnable = controls.get(controls::AnalogueGainMode);\n>> +\t\tif (agEnable &&\n>> +\t\t (*agEnable == controls::AnalogueGainModeAuto) != state.autoGainEnabled) {\n>> +\t\t\tstate.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);\n>> +\n>> +\t\t\tLOG(Agc, Debug)\n>> +\t\t\t\t<< (state.autoGainEnabled ? \"Enabling\" : \"Disabling\")\n>> +\t\t\t\t<< \" AGC (gain)\";\n>> +\t\t\t/*\n>> +\t\t\t * If we go from auto -> manual with no manual control\n>> +\t\t\t * set, use the last computed value, which we don't\n>> +\t\t\t * know until prepare() so save this information.\n>> +\t\t\t */\n>> +\t\t\tif (!state.autoGainEnabled && !controls.get(controls::AnalogueGain))\n>> +\t\t\t\tframeContext.autoGainModeChange = true;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\tconst auto &exposure = controls.get(controls::ExposureTime);\n>> +\tif (exposure && !state.autoExposureEnabled) {\n>> +\t\tstate.manual.exposure = *exposure * 1.0us / session.lineDuration;\n>> +\n>> +\t\tLOG(Agc, Debug)\n>> +\t\t\t<< \"Set exposure to \" << state.manual.exposure;\n>> +\t}\n>> +\n>> +\tconst auto &gain = controls.get(controls::AnalogueGain);\n>> +\tif (gain && !state.autoGainEnabled) {\n>> +\t\tstate.manual.gain = *gain;\n>> +\n>> +\t\tLOG(Agc, Debug) << \"Set gain to \" << state.manual.gain;\n>> +\t}\n>> +\n>> +\tframeContext.autoExposureEnabled = state.autoExposureEnabled;\n>> +\tframeContext.autoGainEnabled = state.autoGainEnabled;\n>> +\n>> +\tif (!frameContext.autoExposureEnabled)\n>> +\t\tframeContext.exposure = state.manual.exposure;\n>> +\tif (!frameContext.autoGainEnabled)\n>> +\t\tframeContext.gain = state.manual.gain;\n>> +\n>> +\tif (!frameContext.autoExposureEnabled &&\n>> +\t !frameContext.autoGainEnabled)\n>> +\t\tframeContext.quantizationGain = 1.0;\n>> +\n>> +\tconst auto &exposureMode = controls.get(controls::AeExposureMode);\n>> +\tif (exposureMode)\n>> +\t\tstate.exposureMode =\n>> +\t\t\tstatic_cast(*exposureMode);\n>> +\tframeContext.exposureMode = state.exposureMode;\n>> +\n>> +\tconst auto &constraintMode = controls.get(controls::AeConstraintMode);\n>> +\tif (constraintMode)\n>> +\t\tstate.constraintMode =\n>> +\t\t\tstatic_cast(*constraintMode);\n>> +\tframeContext.constraintMode = state.constraintMode;\n>> +\n>> +\tconst auto &exposureValue = controls.get(controls::ExposureValue);\n>> +\tif (exposureValue)\n>> +\t\tstate.exposureValue = *exposureValue;\n>> +\tframeContext.exposureValue = state.exposureValue;\n>> +\n>> +\tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n>> +\tif (frameDurationLimits) {\n>> +\t\t/* Limit the control value to the limits in ControlInfo */\n>> +\t\tstate.minFrameDuration = std::clamp(\n>> +\t\t\tstd::chrono::microseconds((*frameDurationLimits).front()),\n>> +\t\t\tsession.minFrameDuration,\n>> +\t\t\tsession.maxFrameDuration\n>> +\t\t);\n>> +\n>> +\t\tstate.maxFrameDuration = std::clamp(\n>> +\t\t\tstd::chrono::microseconds((*frameDurationLimits).back()),\n>> +\t\t\tsession.minFrameDuration,\n>> +\t\t\tsession.maxFrameDuration\n>> +\t\t);\n>> +\t}\n>> +\tframeContext.minFrameDuration = state.minFrameDuration;\n>> +\tframeContext.maxFrameDuration = state.maxFrameDuration;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Handle a \\a prepare operation\n>> + */\n>> +void AgcAlgorithm::prepare(agc::ActiveState &state, agc::FrameContext &frameContext)\n>> +{\n>> +\tuint32_t activeAutoExposure = state.automatic.exposure;\n>> +\tdouble activeAutoGain = state.automatic.gain;\n>> +\tdouble activeAutoQGain = state.automatic.quantizationGain;\n>> +\n>> +\t/* Populate exposure and gain in auto mode */\n>> +\tif (frameContext.autoExposureEnabled) {\n>> +\t\tframeContext.exposure = activeAutoExposure;\n>> +\t\tframeContext.quantizationGain = activeAutoQGain;\n>> +\t}\n>> +\tif (frameContext.autoGainEnabled) {\n>> +\t\tframeContext.gain = activeAutoGain;\n>> +\t\tframeContext.quantizationGain = activeAutoQGain;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Populate manual exposure and gain from the active auto values when\n>> +\t * transitioning from auto to manual\n>> +\t */\n>> +\tif (!frameContext.autoExposureEnabled && frameContext.autoExposureModeChange) {\n>> +\t\tstate.manual.exposure = activeAutoExposure;\n>> +\t\tframeContext.exposure = activeAutoExposure;\n>> +\t}\n>> +\tif (!frameContext.autoGainEnabled && frameContext.autoGainModeChange) {\n>> +\t\tstate.manual.gain = activeAutoGain;\n>> +\t\tframeContext.gain = activeAutoGain;\n>> +\t\tframeContext.quantizationGain = activeAutoQGain;\n>> +\t}\n>> +\n>> +\tframeContext.yTarget = state.automatic.yTarget;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Handle a \\a process operation\n>> + */\n>> +void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,\n>> +\t\t\t agc::FrameContext &frameContext, std::optional &¶ms,\n>> +\t\t\t ControlList &metadata)\n>> +{\n>> +\tconst utils::Duration &lineDuration = session.lineDuration;\n>> +\tutils::Duration newExposureTime = {};\n>> +\n>> +\tif (params) {\n>> +\t\tASSERT(session.autoAllowed);\n>> +\n>> +\t\t/*\n>> +\t\t* Set the AGC limits using the fixed exposure time and/or gain in\n>> +\t\t* manual mode, or the sensor limits in auto mode.\n>> +\t\t*/\n>> +\t\tutils::Duration minExposureTime;\n>> +\t\tutils::Duration maxExposureTime;\n>> +\t\tdouble minAnalogueGain;\n>> +\t\tdouble maxAnalogueGain;\n>> +\n>> +\t\tif (frameContext.autoExposureEnabled) {\n>> +\t\t\tminExposureTime = session.minExposureTime;\n>> +\t\t\tmaxExposureTime = std::clamp(frameContext.maxFrameDuration, session.minExposureTime, session.maxExposureTime);\n>> +\t\t} else {\n>> +\t\t\tminExposureTime = lineDuration * frameContext.exposure;\n>> +\t\t\tmaxExposureTime = minExposureTime;\n>> +\t\t}\n>> +\n>> +\t\tif (frameContext.autoGainEnabled) {\n>> +\t\t\tminAnalogueGain = session.minAnalogueGain;\n>> +\t\t\tmaxAnalogueGain = session.maxAnalogueGain;\n>> +\t\t} else {\n>> +\t\t\tminAnalogueGain = frameContext.gain;\n>> +\t\t\tmaxAnalogueGain = frameContext.gain;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t* The Agc algorithm needs to know the effective exposure value that was\n>> +\t\t* applied to the sensor when the statistics were collected.\n>> +\t\t*/\n>> +\t\tutils::Duration effectiveExposureValue =\n>> +\t\t\tlineDuration * params->exposure * params->gain;\n>> +\n>> +\t\timpl_.setLimits(minExposureTime, maxExposureTime,\n>> +\t\t\t\tminAnalogueGain, maxAnalogueGain,\n>> +\t\t\t\tstd::move(params->additionalConstraints));\n>> +\n>> +\t\tconst auto &newEv = impl_.calculateNewEv({\n>> +\t\t\t.traits = params->traits,\n>> +\t\t\t.yHist = params->yHist,\n>> +\t\t\t.effectiveExposureValue = effectiveExposureValue,\n>> +\t\t\t.constraintModeIndex = frameContext.constraintMode,\n>> +\t\t\t.exposureModeIndex = frameContext.exposureMode,\n>> +\t\t\t.lux = params->lux,\n>> +\t\t\t.exposureCompensation = pow(2.0, frameContext.exposureValue),\n>> +\t\t});\n>> +\n>> +\t\tLOG(Agc, Debug)\n>> +\t\t\t<< \"Divided up exposure time, analogue gain, quantization gain\"\n>> +\t\t\t<< \" and digital gain are \" << newEv.exposureTime << \", \" << newEv.analogueGain\n>> +\t\t\t<< \", \" << newEv.quantizationGain << \" and \" << newEv.digitalGain;\n>> +\n>> +\t\t/* Update the estimated exposure and gain. */\n>> +\t\tstate.automatic.exposure = newEv.exposureTime / lineDuration;\n>> +\t\tstate.automatic.gain = newEv.analogueGain;\n>> +\t\tstate.automatic.quantizationGain = newEv.quantizationGain;\n>> +\t\tstate.automatic.yTarget = newEv.yTarget;\n>> +\n>> +\t\tnewExposureTime = newEv.exposureTime;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Expand the target frame duration so that we do not run faster than\n>> +\t * the minimum frame duration when we have short exposures.\n>> +\t */\n>> +\tconst auto frameDuration = std::max(frameContext.minFrameDuration, newExposureTime);\n>> +\tframeContext.vblank = (frameDuration / lineDuration) - session.sensor.outputSize.height;\n>> +\n>> +\t/* Update frame duration accounting for line length quantization. */\n>> +\tframeContext.frameDuration = (session.sensor.outputSize.height + frameContext.vblank) * lineDuration;\n>> +\n>> +\tmetadata.set(controls::AnalogueGain, frameContext.gain);\n>> +\tmetadata.set(controls::ExposureTime, utils::Duration(lineDuration * frameContext.exposure).get());\n>> +\tmetadata.set(controls::FrameDuration, frameContext.frameDuration.get());\n>> +\tmetadata.set(controls::ExposureTimeMode,\n>> +\t\t frameContext.autoExposureEnabled\n>> +\t\t ? controls::ExposureTimeModeAuto\n>> +\t\t : controls::ExposureTimeModeManual);\n>> +\tmetadata.set(controls::AnalogueGainMode,\n>> +\t\t frameContext.autoGainEnabled\n>> +\t\t ? controls::AnalogueGainModeAuto\n>> +\t\t : controls::AnalogueGainModeManual);\n>> +\n>> +\tmetadata.set(controls::AeExposureMode, frameContext.exposureMode);\n>> +\tmetadata.set(controls::AeConstraintMode, frameContext.constraintMode);\n>> +\tmetadata.set(controls::ExposureValue, frameContext.exposureValue);\n>> +}\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h\n>> index 5247425952..1eace12908 100644\n>> --- a/src/ipa/libipa/agc.h\n>> +++ b/src/ipa/libipa/agc.h\n>> @@ -7,13 +7,19 @@\n>>\n>> #pragma once\n>>\n>> +#include \n>> #include \n>>\n>> #include \n>>\n>> +#include \n>> #include \n>>\n>> +#include \n>> +\n>> +#include \"agc_mean_luminance.h\"\n>> #include \"camera_sensor_helper.h\"\n>> +#include \"histogram.h\"\n>>\n>> namespace libcamera {\n>>\n>> @@ -42,8 +48,100 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,\n>> \tcontrols.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));\n>> }\n>>\n>> +struct Session {\n>> +\tutils::Duration minExposureTime;\n>> +\tutils::Duration maxExposureTime;\n>> +\tdouble minAnalogueGain;\n>> +\tdouble maxAnalogueGain;\n>> +\tutils::Duration minFrameDuration;\n>> +\tutils::Duration maxFrameDuration;\n>> +\n>> +\tutils::Duration lineDuration;\n>> +\n>> +\tstruct {\n>> +\t\tSize outputSize;\n>> +\t} sensor;\n>> +\n>> +\tbool autoAllowed;\n>> +};\n>> +\n>> +struct ActiveState {\n>> +\tstruct {\n>> +\t\tuint32_t exposure;\n>> +\t\tdouble gain;\n>> +\t} manual;\n>> +\tstruct {\n>> +\t\tuint32_t exposure;\n>> +\t\tdouble gain;\n>> +\t\tdouble quantizationGain;\n>> +\t\tdouble yTarget;\n>> +\t} automatic;\n>> +\n>> +\tbool autoExposureEnabled;\n>> +\tbool autoGainEnabled;\n>> +\tdouble exposureValue;\n>> +\tcontrols::AeConstraintModeEnum constraintMode;\n>> +\tcontrols::AeExposureModeEnum exposureMode;\n>> +\tutils::Duration minFrameDuration;\n>> +\tutils::Duration maxFrameDuration;\n>> +};\n>> +\n>> +struct FrameContext {\n>> +\tuint32_t exposure;\n>> +\tdouble gain;\n>> +\tdouble quantizationGain;\n>> +\tdouble exposureValue;\n>> +\tdouble yTarget;\n>> +\tuint32_t vblank;\n>> +\tbool autoExposureEnabled;\n>> +\tbool autoGainEnabled;\n>> +\tcontrols::AeConstraintModeEnum constraintMode;\n>> +\tcontrols::AeExposureModeEnum exposureMode;\n>> +\tutils::Duration minFrameDuration;\n>> +\tutils::Duration maxFrameDuration;\n>> +\tutils::Duration frameDuration;\n>> +\tbool autoExposureModeChange;\n>> +\tbool autoGainModeChange;\n>> +};\n>> +\n>> } /* namespace agc */\n>>\n>> +class AgcAlgorithm\n>> +{\n>> +public:\n>> +\tstruct ConfigurationParams {\n>> +\t\tconst CameraSensorHelper *sensor;\n>> +\t\tconst IPACameraSensorInfo &sensorInfo;\n>> +\t\tconst ControlInfoMap &sensorControls;\n>> +\t\tControlInfoMap::Map &ctrlMap;\n>> +\t\tbool autoAllowed = true;\n>> +\t};\n>> +\n>> +\tint init(const ValueNode &tuningData);\n>> +\n>> +\tint configure(agc::Session &session, agc::ActiveState &state, const ConfigurationParams &config);\n>> +\n>> +\tvoid queueRequest(const agc::Session &session, agc::ActiveState &state,\n>> +\t\t\t agc::FrameContext &frameContext, const ControlList &controls);\n>> +\n>> +\tvoid prepare(agc::ActiveState &state, agc::FrameContext &frameContext);\n>> +\n>> +\tstruct ProcessParams {\n>> +\t\tconst AgcMeanLuminance::Traits &traits;\n>> +\t\tconst Histogram &yHist;\n>> +\t\tuint32_t exposure;\n>> +\t\tdouble gain;\n>> +\t\tstd::vector &&additionalConstraints = {};\n>> +\t\tdouble lux = 0;\n>> +\t};\n>> +\n>> +\tvoid process(const agc::Session &session, agc::ActiveState &state, agc::FrameContext &frameContext,\n>> +\t\t std::optional &¶ms, ControlList &metadata);\n>> +\n>> +private:\n>> +\tAgcMeanLuminance impl_;\n>> +};\n>> +\n>> } /* namespace ipa */\n>>\n>> } /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>> index 05f1a8749c..ca681fa5af 100644\n>> --- a/src/ipa/libipa/meson.build\n>> +++ b/src/ipa/libipa/meson.build\n>> @@ -23,6 +23,7 @@ libipa_headers = files([\n>> ])\n>>\n>> libipa_sources = files([\n>> + 'agc.cpp',\n>> 'agc_mean_luminance.cpp',\n>> 'algorithm.cpp',\n>> 'awb_bayes.cpp',\n>> --\n>> 2.55.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 70DF6BDE4C\n\tfor ;\n\tMon, 27 Jul 2026 15:01:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13FB867F8D;\n\tMon, 27 Jul 2026 17:01:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E69A367EB2\n\tfor ;\n\tMon, 27 Jul 2026 17:01:23 +0200 (CEST)","from [192.168.33.46] (185.182.215.156.nat.pool.zt.hu\n\t[185.182.215.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 12D423A4;\n\tMon, 27 Jul 2026 17:00:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ph2pwA+v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785164420;\n\tbh=ob3skrPUz4hI2QslcPUfZierfod87YcEUWXHuqSIgyw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Ph2pwA+vjN4GKVzYv6TEuGZ9HbQHSEl2DSskBiE0m/kn/hl4yB+bKV3KepY4y3llV\n\tlWyEnEgdeQ/X80jSogRtObglsLZxG5FbZte8EN2Hm0DfsdT0umw1NXF1QiYozdt5Lv\n\t2AUXMw6Pt+tGEqTT3AP5LWJyEamCibsQ3/4D22O8=","Message-ID":"<1280fb2f-df68-47da-8093-2873814a065e@ideasonboard.com>","Date":"Mon, 27 Jul 2026 17:01:20 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2 21/43] ipa: libipa: Add `AgcAlgorithm`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-22-barnabas.pocze@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]