| Message ID | 20260703153819.1088752-9-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2026-07-03 16:38:10) > Move the `ControlInfo` setup and related initialization into the AGC > algorithm instead of having it in the main IPA file. Interesting! I don't think I'd ever considered moving these bits /into/ the AGC always thinking of them as part of the control for the sensor - but it sounds like a good way to move towards factoring out all the common duplication indeed. > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 91 ++++++++++++++++++++++++++++++- > src/ipa/rkisp1/rkisp1.cpp | 88 +----------------------------- > 2 files changed, 91 insertions(+), 88 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 540b64ff99..689d045b7a 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -33,13 +33,96 @@ using namespace std::literals::chrono_literals; > > namespace ipa::rkisp1::algorithms { > > +LOG_DEFINE_CATEGORY(RkISP1Agc) > + > +namespace { > + > +void reconfigure(IPAContext &context) > +{ > + context.configuration.sensor.lineDuration = > + context.sensorInfo.minLineLength * 1.0s / context.sensorInfo.pixelRate; > + > + double lineDurationUs = context.configuration.sensor.lineDuration.get<std::micro>(); > + > + /* > + * Compute exposure time limits from the V4L2_CID_EXPOSURE control > + * limits and the line duration. > + */ > + > + const ControlInfo &v4l2Exposure = context.sensorControls.find(V4L2_CID_EXPOSURE)->second; > + int32_t minExposure = v4l2Exposure.min().get<int32_t>(); > + int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); > + int32_t defExposure = v4l2Exposure.def().get<int32_t>(); > + context.ctrlMap[&controls::ExposureTime] = ControlInfo{ > + static_cast<int32_t>(minExposure * lineDurationUs), > + static_cast<int32_t>(maxExposure * lineDurationUs), > + static_cast<int32_t>(defExposure * lineDurationUs), > + }; > + > + /* Compute the analogue gain limits. */ > + const ControlInfo &v4l2Gain = context.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; > + float minGain = context.camHelper->gain(v4l2Gain.min().get<int32_t>()); > + float maxGain = context.camHelper->gain(v4l2Gain.max().get<int32_t>()); > + float defGain = context.camHelper->gain(v4l2Gain.def().get<int32_t>()); > + context.ctrlMap[&controls::AnalogueGain] = ControlInfo{ > + minGain, > + maxGain, > + defGain, > + }; > + > + LOG(RkISP1Agc, Debug) > + << "Exposure: [" << minExposure << ", " << maxExposure > + << "], gain: [" << minGain << ", " << maxGain << "]"; > + > + /* > + * Compute the frame duration limits. > + * > + * The frame length is computed assuming a fixed line length combined > + * with the vertical frame sizes. > + */ Absolutely trivial, but theres some whitespace fail on the comment. But I bet it won't stay here long enough to care: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Hi Barnabás On Fri, Jul 03, 2026 at 05:38:10PM +0200, Barnabás Pőcze wrote: > Move the `ControlInfo` setup and related initialization into the AGC > algorithm instead of having it in the main IPA file. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 91 ++++++++++++++++++++++++++++++- > src/ipa/rkisp1/rkisp1.cpp | 88 +----------------------------- > 2 files changed, 91 insertions(+), 88 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 540b64ff99..689d045b7a 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -33,13 +33,96 @@ using namespace std::literals::chrono_literals; > > namespace ipa::rkisp1::algorithms { > > +LOG_DEFINE_CATEGORY(RkISP1Agc) > + > +namespace { > + > +void reconfigure(IPAContext &context) One question: why a static helper ? Do we have a rule on when a function should be an helper or a class member ? See, to me, and maybe I'm wrong, if there's no reason not to do so, functions like this should be class members, they're integral to the class functioning it doesn't matter if they don't change the class state. Am I wrong. Also, if you could maybe find a better name :) > +{ > + context.configuration.sensor.lineDuration = > + context.sensorInfo.minLineLength * 1.0s / context.sensorInfo.pixelRate; > + > + double lineDurationUs = context.configuration.sensor.lineDuration.get<std::micro>(); > + > + /* > + * Compute exposure time limits from the V4L2_CID_EXPOSURE control > + * limits and the line duration. > + */ > + > + const ControlInfo &v4l2Exposure = context.sensorControls.find(V4L2_CID_EXPOSURE)->second; > + int32_t minExposure = v4l2Exposure.min().get<int32_t>(); > + int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); > + int32_t defExposure = v4l2Exposure.def().get<int32_t>(); > + context.ctrlMap[&controls::ExposureTime] = ControlInfo{ > + static_cast<int32_t>(minExposure * lineDurationUs), > + static_cast<int32_t>(maxExposure * lineDurationUs), > + static_cast<int32_t>(defExposure * lineDurationUs), > + }; > + > + /* Compute the analogue gain limits. */ > + const ControlInfo &v4l2Gain = context.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; > + float minGain = context.camHelper->gain(v4l2Gain.min().get<int32_t>()); > + float maxGain = context.camHelper->gain(v4l2Gain.max().get<int32_t>()); > + float defGain = context.camHelper->gain(v4l2Gain.def().get<int32_t>()); > + context.ctrlMap[&controls::AnalogueGain] = ControlInfo{ > + minGain, > + maxGain, > + defGain, > + }; > + > + LOG(RkISP1Agc, Debug) > + << "Exposure: [" << minExposure << ", " << maxExposure > + << "], gain: [" << minGain << ", " << maxGain << "]"; > + > + /* > + * Compute the frame duration limits. > + * > + * The frame length is computed assuming a fixed line length combined > + * with the vertical frame sizes. > + */ weid comment indent > + const ControlInfo &v4l2HBlank = context.sensorControls.find(V4L2_CID_HBLANK)->second; > + uint32_t hblank = v4l2HBlank.def().get<int32_t>(); > + uint32_t lineLength = context.sensorInfo.outputSize.width + hblank; > + > + const ControlInfo &v4l2VBlank = context.sensorControls.find(V4L2_CID_VBLANK)->second; > + std::array<uint32_t, 3> frameHeights{ > + v4l2VBlank.min().get<int32_t>() + context.sensorInfo.outputSize.height, > + v4l2VBlank.max().get<int32_t>() + context.sensorInfo.outputSize.height, > + v4l2VBlank.def().get<int32_t>() + context.sensorInfo.outputSize.height, > + }; > + > + std::array<int64_t, 3> frameDurations; > + for (unsigned int i = 0; i < frameHeights.size(); ++i) { > + uint64_t frameSize = lineLength * frameHeights[i]; > + frameDurations[i] = frameSize / (context.sensorInfo.pixelRate / 1000000U); > + } > + > + context.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{ > + frameDurations[0], > + frameDurations[1], > + Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } }, > + }; > + > + /* > + * When the AGC computes the new exposure values for a frame, it needs > + * to know the limits for exposure time and analogue gain. As it depends > + * on the sensor, update it with the controls. > + * > + * \todo take VBLANK into account for maximum exposure time > + */ > + context.configuration.sensor.minExposureTime = minExposure * context.configuration.sensor.lineDuration; > + context.configuration.sensor.maxExposureTime = maxExposure * context.configuration.sensor.lineDuration; > + context.configuration.sensor.minAnalogueGain = context.camHelper->gain(minGain); > + context.configuration.sensor.maxAnalogueGain = context.camHelper->gain(maxGain); > +} > + > +} /* namespace */ > + > /** > * \class Agc > * \brief A mean-based auto-exposure algorithm > */ > > -LOG_DEFINE_CATEGORY(RkISP1Agc) > - > int Agc::parseMeteringModes(IPAContext &context, const ValueNode &tuningData) > { > if (!tuningData.isDictionary()) > @@ -160,6 +243,8 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) > context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); > context.ctrlMap.merge(agc_.controls()); > > + reconfigure(context); > + > return 0; > } > > @@ -172,6 +257,8 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) > */ > int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > { > + reconfigure(context); > + > /* Configure the default exposure and gain. */ > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; > context.activeState.agc.automatic.exposure = > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 66a91c4d79..38f55b1d86 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -169,9 +169,6 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > return -ENODEV; > } > > - context_.configuration.sensor.lineDuration = > - sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate; > - > /* Load the tuning data file. */ > File file(settings.configurationFile); > if (!file.open(File::OpenModeFlag::ReadOnly)) { > @@ -227,18 +224,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > context_.sensorInfo = ipaConfig.sensorInfo; > context_.sensorControls = ipaConfig.sensorControls; > > - const auto itExp = context_.sensorControls.find(V4L2_CID_EXPOSURE); > - int32_t minExposure = itExp->second.min().get<int32_t>(); > - int32_t maxExposure = itExp->second.max().get<int32_t>(); > - > - const auto itGain = context_.sensorControls.find(V4L2_CID_ANALOGUE_GAIN); > - int32_t minGain = itGain->second.min().get<int32_t>(); > - int32_t maxGain = itGain->second.max().get<int32_t>(); > - > - LOG(IPARkISP1, Debug) > - << "Exposure: [" << minExposure << ", " << maxExposure > - << "], gain: [" << minGain << ", " << maxGain << "]"; > - so we initialized these here but never really used them in init ? > /* Clear the IPA context before the streaming session. */ > context_.configuration = {}; > context_.activeState = {}; > @@ -247,27 +232,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > context_.configuration.paramFormat = ipaConfig.paramFormat; > > context_.configuration.sensor.size = context_.sensorInfo.outputSize; > - context_.configuration.sensor.lineDuration = context_.sensorInfo.minLineLength * 1.0s > - / context_.sensorInfo.pixelRate; > - > - /* Update the camera controls using the new sensor settings. */ > - updateControls(ipaControls); > - > - /* > - * When the AGC computes the new exposure values for a frame, it needs > - * to know the limits for exposure time and analogue gain. As it depends > - * on the sensor, update it with the controls. > - * > - * \todo take VBLANK into account for maximum exposure time > - */ > - context_.configuration.sensor.minExposureTime = > - minExposure * context_.configuration.sensor.lineDuration; > - context_.configuration.sensor.maxExposureTime = > - maxExposure * context_.configuration.sensor.lineDuration; > - context_.configuration.sensor.minAnalogueGain = > - context_.camHelper->gain(minGain); > - context_.configuration.sensor.maxAnalogueGain = > - context_.camHelper->gain(maxGain); > > context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(), > [](auto &cfg) -> bool { > @@ -289,6 +253,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > return ret; > } > > + updateControls(ipaControls); > + > return 0; > } > > @@ -386,56 +352,6 @@ void IPARkISP1::updateControls(ControlInfoMap *ipaControls) > { > ControlInfoMap::Map ctrlMap = rkisp1Controls; > > - /* > - * Compute exposure time limits from the V4L2_CID_EXPOSURE control > - * limits and the line duration. > - */ > - double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); > - const ControlInfo &v4l2Exposure = context_.sensorControls.find(V4L2_CID_EXPOSURE)->second; > - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; > - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; > - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; > - ctrlMap.emplace(std::piecewise_construct, > - std::forward_as_tuple(&controls::ExposureTime), > - std::forward_as_tuple(minExposure, maxExposure, defExposure)); > - > - /* Compute the analogue gain limits. */ > - const ControlInfo &v4l2Gain = context_.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; > - float minGain = context_.camHelper->gain(v4l2Gain.min().get<int32_t>()); > - float maxGain = context_.camHelper->gain(v4l2Gain.max().get<int32_t>()); > - float defGain = context_.camHelper->gain(v4l2Gain.def().get<int32_t>()); > - ctrlMap.emplace(std::piecewise_construct, > - std::forward_as_tuple(&controls::AnalogueGain), > - std::forward_as_tuple(minGain, maxGain, defGain)); > - > - /* > - * Compute the frame duration limits. > - * > - * The frame length is computed assuming a fixed line length combined > - * with the vertical frame sizes. > - */ > - const ControlInfo &v4l2HBlank = context_.sensorControls.find(V4L2_CID_HBLANK)->second; > - uint32_t hblank = v4l2HBlank.def().get<int32_t>(); > - uint32_t lineLength = context_.sensorInfo.outputSize.width + hblank; > - > - const ControlInfo &v4l2VBlank = context_.sensorControls.find(V4L2_CID_VBLANK)->second; > - std::array<uint32_t, 3> frameHeights{ > - v4l2VBlank.min().get<int32_t>() + context_.sensorInfo.outputSize.height, > - v4l2VBlank.max().get<int32_t>() + context_.sensorInfo.outputSize.height, > - v4l2VBlank.def().get<int32_t>() + context_.sensorInfo.outputSize.height, > - }; > - > - std::array<int64_t, 3> frameDurations; > - for (unsigned int i = 0; i < frameHeights.size(); ++i) { > - uint64_t frameSize = lineLength * frameHeights[i]; > - frameDurations[i] = frameSize / (context_.sensorInfo.pixelRate / 1000000U); > - } > - > - /* \todo Move this (and other agc-related controls) to agc */ Yey! Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > - context_.ctrlMap[&controls::FrameDurationLimits] = > - ControlInfo(frameDurations[0], frameDurations[1], > - ControlValue(Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } })); > - > ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > } > -- > 2.54.0 >
2026. 07. 16. 18:24 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Fri, Jul 03, 2026 at 05:38:10PM +0200, Barnabás Pőcze wrote: >> Move the `ControlInfo` setup and related initialization into the AGC >> algorithm instead of having it in the main IPA file. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/ipa/rkisp1/algorithms/agc.cpp | 91 ++++++++++++++++++++++++++++++- >> src/ipa/rkisp1/rkisp1.cpp | 88 +----------------------------- >> 2 files changed, 91 insertions(+), 88 deletions(-) >> >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp >> index 540b64ff99..689d045b7a 100644 >> --- a/src/ipa/rkisp1/algorithms/agc.cpp >> +++ b/src/ipa/rkisp1/algorithms/agc.cpp >> @@ -33,13 +33,96 @@ using namespace std::literals::chrono_literals; >> >> namespace ipa::rkisp1::algorithms { >> >> +LOG_DEFINE_CATEGORY(RkISP1Agc) >> + >> +namespace { >> + >> +void reconfigure(IPAContext &context) > > One question: why a static helper ? Do we have a rule on when a > function should be an helper or a class member ? See, to me, and maybe > I'm wrong, if there's no reason not to do so, functions like this > should be class members, they're integral to the class functioning it > doesn't matter if they don't change the class state. Am I wrong. I don't know of such rule. But I take essentially the opposite view: push as much out of classes into local free functions as possible (and convenient). > > Also, if you could maybe find a better name :) This is the best I could do, suggestions welcome. > >> +{ >> + context.configuration.sensor.lineDuration = >> + context.sensorInfo.minLineLength * 1.0s / context.sensorInfo.pixelRate; >> + >> + double lineDurationUs = context.configuration.sensor.lineDuration.get<std::micro>(); >> + >> + /* >> + * Compute exposure time limits from the V4L2_CID_EXPOSURE control >> + * limits and the line duration. >> + */ >> + >> + const ControlInfo &v4l2Exposure = context.sensorControls.find(V4L2_CID_EXPOSURE)->second; >> + int32_t minExposure = v4l2Exposure.min().get<int32_t>(); >> + int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); >> + int32_t defExposure = v4l2Exposure.def().get<int32_t>(); >> + context.ctrlMap[&controls::ExposureTime] = ControlInfo{ >> + static_cast<int32_t>(minExposure * lineDurationUs), >> + static_cast<int32_t>(maxExposure * lineDurationUs), >> + static_cast<int32_t>(defExposure * lineDurationUs), >> + }; >> + >> + /* Compute the analogue gain limits. */ >> + const ControlInfo &v4l2Gain = context.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; >> + float minGain = context.camHelper->gain(v4l2Gain.min().get<int32_t>()); >> + float maxGain = context.camHelper->gain(v4l2Gain.max().get<int32_t>()); >> + float defGain = context.camHelper->gain(v4l2Gain.def().get<int32_t>()); >> + context.ctrlMap[&controls::AnalogueGain] = ControlInfo{ >> + minGain, >> + maxGain, >> + defGain, >> + }; >> + >> + LOG(RkISP1Agc, Debug) >> + << "Exposure: [" << minExposure << ", " << maxExposure >> + << "], gain: [" << minGain << ", " << maxGain << "]"; >> + >> + /* >> + * Compute the frame duration limits. >> + * >> + * The frame length is computed assuming a fixed line length combined >> + * with the vertical frame sizes. >> + */ > > weid comment indent > >> + const ControlInfo &v4l2HBlank = context.sensorControls.find(V4L2_CID_HBLANK)->second; >> + uint32_t hblank = v4l2HBlank.def().get<int32_t>(); >> + uint32_t lineLength = context.sensorInfo.outputSize.width + hblank; >> + >> + const ControlInfo &v4l2VBlank = context.sensorControls.find(V4L2_CID_VBLANK)->second; >> + std::array<uint32_t, 3> frameHeights{ >> + v4l2VBlank.min().get<int32_t>() + context.sensorInfo.outputSize.height, >> + v4l2VBlank.max().get<int32_t>() + context.sensorInfo.outputSize.height, >> + v4l2VBlank.def().get<int32_t>() + context.sensorInfo.outputSize.height, >> + }; >> + >> + std::array<int64_t, 3> frameDurations; >> + for (unsigned int i = 0; i < frameHeights.size(); ++i) { >> + uint64_t frameSize = lineLength * frameHeights[i]; >> + frameDurations[i] = frameSize / (context.sensorInfo.pixelRate / 1000000U); >> + } >> + >> + context.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{ >> + frameDurations[0], >> + frameDurations[1], >> + Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } }, >> + }; >> + >> + /* >> + * When the AGC computes the new exposure values for a frame, it needs >> + * to know the limits for exposure time and analogue gain. As it depends >> + * on the sensor, update it with the controls. >> + * >> + * \todo take VBLANK into account for maximum exposure time >> + */ >> + context.configuration.sensor.minExposureTime = minExposure * context.configuration.sensor.lineDuration; >> + context.configuration.sensor.maxExposureTime = maxExposure * context.configuration.sensor.lineDuration; >> + context.configuration.sensor.minAnalogueGain = context.camHelper->gain(minGain); >> + context.configuration.sensor.maxAnalogueGain = context.camHelper->gain(maxGain); >> +} >> + >> +} /* namespace */ >> + >> /** >> * \class Agc >> * \brief A mean-based auto-exposure algorithm >> */ >> >> -LOG_DEFINE_CATEGORY(RkISP1Agc) >> - >> int Agc::parseMeteringModes(IPAContext &context, const ValueNode &tuningData) >> { >> if (!tuningData.isDictionary()) >> @@ -160,6 +243,8 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) >> context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); >> context.ctrlMap.merge(agc_.controls()); >> >> + reconfigure(context); >> + >> return 0; >> } >> >> @@ -172,6 +257,8 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) >> */ >> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) >> { >> + reconfigure(context); >> + >> /* Configure the default exposure and gain. */ >> context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; >> context.activeState.agc.automatic.exposure = >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp >> index 66a91c4d79..38f55b1d86 100644 >> --- a/src/ipa/rkisp1/rkisp1.cpp >> +++ b/src/ipa/rkisp1/rkisp1.cpp >> @@ -169,9 +169,6 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, >> return -ENODEV; >> } >> >> - context_.configuration.sensor.lineDuration = >> - sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate; >> - >> /* Load the tuning data file. */ >> File file(settings.configurationFile); >> if (!file.open(File::OpenModeFlag::ReadOnly)) { >> @@ -227,18 +224,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, >> context_.sensorInfo = ipaConfig.sensorInfo; >> context_.sensorControls = ipaConfig.sensorControls; >> >> - const auto itExp = context_.sensorControls.find(V4L2_CID_EXPOSURE); >> - int32_t minExposure = itExp->second.min().get<int32_t>(); >> - int32_t maxExposure = itExp->second.max().get<int32_t>(); >> - >> - const auto itGain = context_.sensorControls.find(V4L2_CID_ANALOGUE_GAIN); >> - int32_t minGain = itGain->second.min().get<int32_t>(); >> - int32_t maxGain = itGain->second.max().get<int32_t>(); >> - >> - LOG(IPARkISP1, Debug) >> - << "Exposure: [" << minExposure << ", " << maxExposure >> - << "], gain: [" << minGain << ", " << maxGain << "]"; >> - > > so we initialized these here but never really used them in init ? Some parts need to run in `init()` to provide data for `Camera::controls()`. But I decided to merge everything into a single function that does all the necessary calculations for `init()` and `configure()`. Or you mean something different? > >> /* Clear the IPA context before the streaming session. */ >> context_.configuration = {}; >> context_.activeState = {}; >> @@ -247,27 +232,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, >> context_.configuration.paramFormat = ipaConfig.paramFormat; >> >> context_.configuration.sensor.size = context_.sensorInfo.outputSize; >> - context_.configuration.sensor.lineDuration = context_.sensorInfo.minLineLength * 1.0s >> - / context_.sensorInfo.pixelRate; >> - >> - /* Update the camera controls using the new sensor settings. */ >> - updateControls(ipaControls); >> - >> - /* >> - * When the AGC computes the new exposure values for a frame, it needs >> - * to know the limits for exposure time and analogue gain. As it depends >> - * on the sensor, update it with the controls. >> - * >> - * \todo take VBLANK into account for maximum exposure time >> - */ >> - context_.configuration.sensor.minExposureTime = >> - minExposure * context_.configuration.sensor.lineDuration; >> - context_.configuration.sensor.maxExposureTime = >> - maxExposure * context_.configuration.sensor.lineDuration; >> - context_.configuration.sensor.minAnalogueGain = >> - context_.camHelper->gain(minGain); >> - context_.configuration.sensor.maxAnalogueGain = >> - context_.camHelper->gain(maxGain); >> >> context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(), >> [](auto &cfg) -> bool { >> @@ -289,6 +253,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, >> return ret; >> } >> >> + updateControls(ipaControls); >> + >> return 0; >> } >> >> @@ -386,56 +352,6 @@ void IPARkISP1::updateControls(ControlInfoMap *ipaControls) >> { >> ControlInfoMap::Map ctrlMap = rkisp1Controls; >> >> - /* >> - * Compute exposure time limits from the V4L2_CID_EXPOSURE control >> - * limits and the line duration. >> - */ >> - double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); >> - const ControlInfo &v4l2Exposure = context_.sensorControls.find(V4L2_CID_EXPOSURE)->second; >> - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; >> - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; >> - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; >> - ctrlMap.emplace(std::piecewise_construct, >> - std::forward_as_tuple(&controls::ExposureTime), >> - std::forward_as_tuple(minExposure, maxExposure, defExposure)); >> - >> - /* Compute the analogue gain limits. */ >> - const ControlInfo &v4l2Gain = context_.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; >> - float minGain = context_.camHelper->gain(v4l2Gain.min().get<int32_t>()); >> - float maxGain = context_.camHelper->gain(v4l2Gain.max().get<int32_t>()); >> - float defGain = context_.camHelper->gain(v4l2Gain.def().get<int32_t>()); >> - ctrlMap.emplace(std::piecewise_construct, >> - std::forward_as_tuple(&controls::AnalogueGain), >> - std::forward_as_tuple(minGain, maxGain, defGain)); >> - >> - /* >> - * Compute the frame duration limits. >> - * >> - * The frame length is computed assuming a fixed line length combined >> - * with the vertical frame sizes. >> - */ >> - const ControlInfo &v4l2HBlank = context_.sensorControls.find(V4L2_CID_HBLANK)->second; >> - uint32_t hblank = v4l2HBlank.def().get<int32_t>(); >> - uint32_t lineLength = context_.sensorInfo.outputSize.width + hblank; >> - >> - const ControlInfo &v4l2VBlank = context_.sensorControls.find(V4L2_CID_VBLANK)->second; >> - std::array<uint32_t, 3> frameHeights{ >> - v4l2VBlank.min().get<int32_t>() + context_.sensorInfo.outputSize.height, >> - v4l2VBlank.max().get<int32_t>() + context_.sensorInfo.outputSize.height, >> - v4l2VBlank.def().get<int32_t>() + context_.sensorInfo.outputSize.height, >> - }; >> - >> - std::array<int64_t, 3> frameDurations; >> - for (unsigned int i = 0; i < frameHeights.size(); ++i) { >> - uint64_t frameSize = lineLength * frameHeights[i]; >> - frameDurations[i] = frameSize / (context_.sensorInfo.pixelRate / 1000000U); >> - } >> - >> - /* \todo Move this (and other agc-related controls) to agc */ > > Yey! > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >> - context_.ctrlMap[&controls::FrameDurationLimits] = >> - ControlInfo(frameDurations[0], frameDurations[1], >> - ControlValue(Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } })); >> - >> ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); >> *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); >> } >> -- >> 2.54.0 >>
Hi Barnabás On Thu, Jul 16, 2026 at 06:49:06PM +0200, Barnabás Pőcze wrote: > 2026. 07. 16. 18:24 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Fri, Jul 03, 2026 at 05:38:10PM +0200, Barnabás Pőcze wrote: > > > Move the `ControlInfo` setup and related initialization into the AGC > > > algorithm instead of having it in the main IPA file. > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/agc.cpp | 91 ++++++++++++++++++++++++++++++- > > > src/ipa/rkisp1/rkisp1.cpp | 88 +----------------------------- > > > 2 files changed, 91 insertions(+), 88 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > index 540b64ff99..689d045b7a 100644 > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > @@ -33,13 +33,96 @@ using namespace std::literals::chrono_literals; > > > > > > namespace ipa::rkisp1::algorithms { > > > > > > +LOG_DEFINE_CATEGORY(RkISP1Agc) > > > + > > > +namespace { > > > + > > > +void reconfigure(IPAContext &context) > > > > One question: why a static helper ? Do we have a rule on when a > > function should be an helper or a class member ? See, to me, and maybe > > I'm wrong, if there's no reason not to do so, functions like this > > should be class members, they're integral to the class functioning it > > doesn't matter if they don't change the class state. Am I wrong. > > I don't know of such rule. But I take essentially the opposite view: > push as much out of classes into local free functions as possible (and convenient). > The Google C++ rules do not help much here https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions and I don't find any clear guidance. I'll adopt your approach as Stefan had a similar comment as well in my libIPA series then. > > > > > Also, if you could maybe find a better name :) > > This is the best I could do, suggestions welcome. > I'm sure you can do better :) Maybe just updateConfiguration() would be more expressive ? A minor detail anyway. Thanks j > > > > > > +{ > > > + context.configuration.sensor.lineDuration = > > > + context.sensorInfo.minLineLength * 1.0s / context.sensorInfo.pixelRate; > > > + > > > + double lineDurationUs = context.configuration.sensor.lineDuration.get<std::micro>(); > > > + > > > + /* > > > + * Compute exposure time limits from the V4L2_CID_EXPOSURE control > > > + * limits and the line duration. > > > + */ > > > + > > > + const ControlInfo &v4l2Exposure = context.sensorControls.find(V4L2_CID_EXPOSURE)->second; > > > + int32_t minExposure = v4l2Exposure.min().get<int32_t>(); > > > + int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); > > > + int32_t defExposure = v4l2Exposure.def().get<int32_t>(); > > > + context.ctrlMap[&controls::ExposureTime] = ControlInfo{ > > > + static_cast<int32_t>(minExposure * lineDurationUs), > > > + static_cast<int32_t>(maxExposure * lineDurationUs), > > > + static_cast<int32_t>(defExposure * lineDurationUs), > > > + }; > > > + > > > + /* Compute the analogue gain limits. */ > > > + const ControlInfo &v4l2Gain = context.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; > > > + float minGain = context.camHelper->gain(v4l2Gain.min().get<int32_t>()); > > > + float maxGain = context.camHelper->gain(v4l2Gain.max().get<int32_t>()); > > > + float defGain = context.camHelper->gain(v4l2Gain.def().get<int32_t>()); > > > + context.ctrlMap[&controls::AnalogueGain] = ControlInfo{ > > > + minGain, > > > + maxGain, > > > + defGain, > > > + }; > > > + > > > + LOG(RkISP1Agc, Debug) > > > + << "Exposure: [" << minExposure << ", " << maxExposure > > > + << "], gain: [" << minGain << ", " << maxGain << "]"; > > > + > > > + /* > > > + * Compute the frame duration limits. > > > + * > > > + * The frame length is computed assuming a fixed line length combined > > > + * with the vertical frame sizes. > > > + */ > > > > weid comment indent > > > > > + const ControlInfo &v4l2HBlank = context.sensorControls.find(V4L2_CID_HBLANK)->second; > > > + uint32_t hblank = v4l2HBlank.def().get<int32_t>(); > > > + uint32_t lineLength = context.sensorInfo.outputSize.width + hblank; > > > + > > > + const ControlInfo &v4l2VBlank = context.sensorControls.find(V4L2_CID_VBLANK)->second; > > > + std::array<uint32_t, 3> frameHeights{ > > > + v4l2VBlank.min().get<int32_t>() + context.sensorInfo.outputSize.height, > > > + v4l2VBlank.max().get<int32_t>() + context.sensorInfo.outputSize.height, > > > + v4l2VBlank.def().get<int32_t>() + context.sensorInfo.outputSize.height, > > > + }; > > > + > > > + std::array<int64_t, 3> frameDurations; > > > + for (unsigned int i = 0; i < frameHeights.size(); ++i) { > > > + uint64_t frameSize = lineLength * frameHeights[i]; > > > + frameDurations[i] = frameSize / (context.sensorInfo.pixelRate / 1000000U); > > > + } > > > + > > > + context.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{ > > > + frameDurations[0], > > > + frameDurations[1], > > > + Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } }, > > > + }; > > > + > > > + /* > > > + * When the AGC computes the new exposure values for a frame, it needs > > > + * to know the limits for exposure time and analogue gain. As it depends > > > + * on the sensor, update it with the controls. > > > + * > > > + * \todo take VBLANK into account for maximum exposure time > > > + */ > > > + context.configuration.sensor.minExposureTime = minExposure * context.configuration.sensor.lineDuration; > > > + context.configuration.sensor.maxExposureTime = maxExposure * context.configuration.sensor.lineDuration; > > > + context.configuration.sensor.minAnalogueGain = context.camHelper->gain(minGain); > > > + context.configuration.sensor.maxAnalogueGain = context.camHelper->gain(maxGain); > > > +} > > > + > > > +} /* namespace */ > > > + > > > /** > > > * \class Agc > > > * \brief A mean-based auto-exposure algorithm > > > */ > > > > > > -LOG_DEFINE_CATEGORY(RkISP1Agc) > > > - > > > int Agc::parseMeteringModes(IPAContext &context, const ValueNode &tuningData) > > > { > > > if (!tuningData.isDictionary()) > > > @@ -160,6 +243,8 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) > > > context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); > > > context.ctrlMap.merge(agc_.controls()); > > > > > > + reconfigure(context); > > > + > > > return 0; > > > } > > > > > > @@ -172,6 +257,8 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) > > > */ > > > int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > { > > > + reconfigure(context); > > > + > > > /* Configure the default exposure and gain. */ > > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; > > > context.activeState.agc.automatic.exposure = > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > index 66a91c4d79..38f55b1d86 100644 > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > @@ -169,9 +169,6 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > > return -ENODEV; > > > } > > > > > > - context_.configuration.sensor.lineDuration = > > > - sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate; > > > - > > > /* Load the tuning data file. */ > > > File file(settings.configurationFile); > > > if (!file.open(File::OpenModeFlag::ReadOnly)) { > > > @@ -227,18 +224,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > > context_.sensorInfo = ipaConfig.sensorInfo; > > > context_.sensorControls = ipaConfig.sensorControls; > > > > > > - const auto itExp = context_.sensorControls.find(V4L2_CID_EXPOSURE); > > > - int32_t minExposure = itExp->second.min().get<int32_t>(); > > > - int32_t maxExposure = itExp->second.max().get<int32_t>(); > > > - > > > - const auto itGain = context_.sensorControls.find(V4L2_CID_ANALOGUE_GAIN); > > > - int32_t minGain = itGain->second.min().get<int32_t>(); > > > - int32_t maxGain = itGain->second.max().get<int32_t>(); > > > - > > > - LOG(IPARkISP1, Debug) > > > - << "Exposure: [" << minExposure << ", " << maxExposure > > > - << "], gain: [" << minGain << ", " << maxGain << "]"; > > > - > > > > so we initialized these here but never really used them in init ? > > Some parts need to run in `init()` to provide data for `Camera::controls()`. > But I decided to merge everything into a single function that does all > the necessary calculations for `init()` and `configure()`. Or you mean > something different? > > > > > > > /* Clear the IPA context before the streaming session. */ > > > context_.configuration = {}; > > > context_.activeState = {}; > > > @@ -247,27 +232,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > > context_.configuration.paramFormat = ipaConfig.paramFormat; > > > > > > context_.configuration.sensor.size = context_.sensorInfo.outputSize; > > > - context_.configuration.sensor.lineDuration = context_.sensorInfo.minLineLength * 1.0s > > > - / context_.sensorInfo.pixelRate; > > > - > > > - /* Update the camera controls using the new sensor settings. */ > > > - updateControls(ipaControls); > > > - > > > - /* > > > - * When the AGC computes the new exposure values for a frame, it needs > > > - * to know the limits for exposure time and analogue gain. As it depends > > > - * on the sensor, update it with the controls. > > > - * > > > - * \todo take VBLANK into account for maximum exposure time > > > - */ > > > - context_.configuration.sensor.minExposureTime = > > > - minExposure * context_.configuration.sensor.lineDuration; > > > - context_.configuration.sensor.maxExposureTime = > > > - maxExposure * context_.configuration.sensor.lineDuration; > > > - context_.configuration.sensor.minAnalogueGain = > > > - context_.camHelper->gain(minGain); > > > - context_.configuration.sensor.maxAnalogueGain = > > > - context_.camHelper->gain(maxGain); > > > > > > context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(), > > > [](auto &cfg) -> bool { > > > @@ -289,6 +253,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > > return ret; > > > } > > > > > > + updateControls(ipaControls); > > > + > > > return 0; > > > } > > > > > > @@ -386,56 +352,6 @@ void IPARkISP1::updateControls(ControlInfoMap *ipaControls) > > > { > > > ControlInfoMap::Map ctrlMap = rkisp1Controls; > > > > > > - /* > > > - * Compute exposure time limits from the V4L2_CID_EXPOSURE control > > > - * limits and the line duration. > > > - */ > > > - double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); > > > - const ControlInfo &v4l2Exposure = context_.sensorControls.find(V4L2_CID_EXPOSURE)->second; > > > - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; > > > - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; > > > - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; > > > - ctrlMap.emplace(std::piecewise_construct, > > > - std::forward_as_tuple(&controls::ExposureTime), > > > - std::forward_as_tuple(minExposure, maxExposure, defExposure)); > > > - > > > - /* Compute the analogue gain limits. */ > > > - const ControlInfo &v4l2Gain = context_.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; > > > - float minGain = context_.camHelper->gain(v4l2Gain.min().get<int32_t>()); > > > - float maxGain = context_.camHelper->gain(v4l2Gain.max().get<int32_t>()); > > > - float defGain = context_.camHelper->gain(v4l2Gain.def().get<int32_t>()); > > > - ctrlMap.emplace(std::piecewise_construct, > > > - std::forward_as_tuple(&controls::AnalogueGain), > > > - std::forward_as_tuple(minGain, maxGain, defGain)); > > > - > > > - /* > > > - * Compute the frame duration limits. > > > - * > > > - * The frame length is computed assuming a fixed line length combined > > > - * with the vertical frame sizes. > > > - */ > > > - const ControlInfo &v4l2HBlank = context_.sensorControls.find(V4L2_CID_HBLANK)->second; > > > - uint32_t hblank = v4l2HBlank.def().get<int32_t>(); > > > - uint32_t lineLength = context_.sensorInfo.outputSize.width + hblank; > > > - > > > - const ControlInfo &v4l2VBlank = context_.sensorControls.find(V4L2_CID_VBLANK)->second; > > > - std::array<uint32_t, 3> frameHeights{ > > > - v4l2VBlank.min().get<int32_t>() + context_.sensorInfo.outputSize.height, > > > - v4l2VBlank.max().get<int32_t>() + context_.sensorInfo.outputSize.height, > > > - v4l2VBlank.def().get<int32_t>() + context_.sensorInfo.outputSize.height, > > > - }; > > > - > > > - std::array<int64_t, 3> frameDurations; > > > - for (unsigned int i = 0; i < frameHeights.size(); ++i) { > > > - uint64_t frameSize = lineLength * frameHeights[i]; > > > - frameDurations[i] = frameSize / (context_.sensorInfo.pixelRate / 1000000U); > > > - } > > > - > > > - /* \todo Move this (and other agc-related controls) to agc */ > > > > Yey! > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > - context_.ctrlMap[&controls::FrameDurationLimits] = > > > - ControlInfo(frameDurations[0], frameDurations[1], > > > - ControlValue(Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } })); > > > - > > > ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); > > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > > > } > > > -- > > > 2.54.0 > > > >
2026. 07. 20. 8:46 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Thu, Jul 16, 2026 at 06:49:06PM +0200, Barnabás Pőcze wrote: >> 2026. 07. 16. 18:24 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Fri, Jul 03, 2026 at 05:38:10PM +0200, Barnabás Pőcze wrote: >>>> Move the `ControlInfo` setup and related initialization into the AGC >>>> algorithm instead of having it in the main IPA file. >>>> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> src/ipa/rkisp1/algorithms/agc.cpp | 91 ++++++++++++++++++++++++++++++- >>>> src/ipa/rkisp1/rkisp1.cpp | 88 +----------------------------- >>>> 2 files changed, 91 insertions(+), 88 deletions(-) >>>> >>>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp >>>> index 540b64ff99..689d045b7a 100644 >>>> --- a/src/ipa/rkisp1/algorithms/agc.cpp >>>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp >>>> @@ -33,13 +33,96 @@ using namespace std::literals::chrono_literals; >>>> >>>> namespace ipa::rkisp1::algorithms { >>>> >>>> +LOG_DEFINE_CATEGORY(RkISP1Agc) >>>> + >>>> +namespace { >>>> + >>>> +void reconfigure(IPAContext &context) >>> >>> One question: why a static helper ? Do we have a rule on when a >>> function should be an helper or a class member ? See, to me, and maybe >>> I'm wrong, if there's no reason not to do so, functions like this >>> should be class members, they're integral to the class functioning it >>> doesn't matter if they don't change the class state. Am I wrong. >> >> I don't know of such rule. But I take essentially the opposite view: >> push as much out of classes into local free functions as possible (and convenient). >> > > The Google C++ rules do not help much here > https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions > > and I don't find any clear guidance. > > I'll adopt your approach as Stefan had a similar comment as well in my > libIPA series then. > >> >>> >>> Also, if you could maybe find a better name :) >> >> This is the best I could do, suggestions welcome. >> > > I'm sure you can do better :) > > Maybe just updateConfiguration() would be more expressive ? > A minor detail anyway. I just realized that this function is dropped later, so I think name is fine for those couple moments. > > Thanks > j > > >> >>> >>>> +{ >>>> + context.configuration.sensor.lineDuration = >>>> + context.sensorInfo.minLineLength * 1.0s / context.sensorInfo.pixelRate; >>>> + >>>> + double lineDurationUs = context.configuration.sensor.lineDuration.get<std::micro>(); >>>> + >>>> + /* >>>> + * Compute exposure time limits from the V4L2_CID_EXPOSURE control >>>> + * limits and the line duration. >>>> + */ >>>> + >>>> + const ControlInfo &v4l2Exposure = context.sensorControls.find(V4L2_CID_EXPOSURE)->second; >>>> + int32_t minExposure = v4l2Exposure.min().get<int32_t>(); >>>> + int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); >>>> + int32_t defExposure = v4l2Exposure.def().get<int32_t>(); >>>> + context.ctrlMap[&controls::ExposureTime] = ControlInfo{ >>>> + static_cast<int32_t>(minExposure * lineDurationUs), >>>> + static_cast<int32_t>(maxExposure * lineDurationUs), >>>> + static_cast<int32_t>(defExposure * lineDurationUs), >>>> + }; >>>> + >>>> + /* Compute the analogue gain limits. */ >>>> + const ControlInfo &v4l2Gain = context.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; >>>> + float minGain = context.camHelper->gain(v4l2Gain.min().get<int32_t>()); >>>> + float maxGain = context.camHelper->gain(v4l2Gain.max().get<int32_t>()); >>>> + float defGain = context.camHelper->gain(v4l2Gain.def().get<int32_t>()); >>>> + context.ctrlMap[&controls::AnalogueGain] = ControlInfo{ >>>> + minGain, >>>> + maxGain, >>>> + defGain, >>>> + }; >>>> + >>>> + LOG(RkISP1Agc, Debug) >>>> + << "Exposure: [" << minExposure << ", " << maxExposure >>>> + << "], gain: [" << minGain << ", " << maxGain << "]"; >>>> + >>>> + /* >>>> + * Compute the frame duration limits. >>>> + * >>>> + * The frame length is computed assuming a fixed line length combined >>>> + * with the vertical frame sizes. >>>> + */ >>> >>> weid comment indent >>> >>>> + const ControlInfo &v4l2HBlank = context.sensorControls.find(V4L2_CID_HBLANK)->second; >>>> + uint32_t hblank = v4l2HBlank.def().get<int32_t>(); >>>> + uint32_t lineLength = context.sensorInfo.outputSize.width + hblank; >>>> + >>>> + const ControlInfo &v4l2VBlank = context.sensorControls.find(V4L2_CID_VBLANK)->second; >>>> + std::array<uint32_t, 3> frameHeights{ >>>> + v4l2VBlank.min().get<int32_t>() + context.sensorInfo.outputSize.height, >>>> + v4l2VBlank.max().get<int32_t>() + context.sensorInfo.outputSize.height, >>>> + v4l2VBlank.def().get<int32_t>() + context.sensorInfo.outputSize.height, >>>> + }; >>>> + >>>> + std::array<int64_t, 3> frameDurations; >>>> + for (unsigned int i = 0; i < frameHeights.size(); ++i) { >>>> + uint64_t frameSize = lineLength * frameHeights[i]; >>>> + frameDurations[i] = frameSize / (context.sensorInfo.pixelRate / 1000000U); >>>> + } >>>> + >>>> + context.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{ >>>> + frameDurations[0], >>>> + frameDurations[1], >>>> + Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } }, >>>> + }; >>>> + >>>> + /* >>>> + * When the AGC computes the new exposure values for a frame, it needs >>>> + * to know the limits for exposure time and analogue gain. As it depends >>>> + * on the sensor, update it with the controls. >>>> + * >>>> + * \todo take VBLANK into account for maximum exposure time >>>> + */ >>>> + context.configuration.sensor.minExposureTime = minExposure * context.configuration.sensor.lineDuration; >>>> + context.configuration.sensor.maxExposureTime = maxExposure * context.configuration.sensor.lineDuration; >>>> + context.configuration.sensor.minAnalogueGain = context.camHelper->gain(minGain); >>>> + context.configuration.sensor.maxAnalogueGain = context.camHelper->gain(maxGain); >>>> +} >>>> + >>>> +} /* namespace */ >>>> + >>>> /** >>>> * \class Agc >>>> * \brief A mean-based auto-exposure algorithm >>>> */ >>>> >>>> -LOG_DEFINE_CATEGORY(RkISP1Agc) >>>> - >>>> int Agc::parseMeteringModes(IPAContext &context, const ValueNode &tuningData) >>>> { >>>> if (!tuningData.isDictionary()) >>>> @@ -160,6 +243,8 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) >>>> context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); >>>> context.ctrlMap.merge(agc_.controls()); >>>> >>>> + reconfigure(context); >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -172,6 +257,8 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) >>>> */ >>>> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) >>>> { >>>> + reconfigure(context); >>>> + >>>> /* Configure the default exposure and gain. */ >>>> context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; >>>> context.activeState.agc.automatic.exposure = >>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp >>>> index 66a91c4d79..38f55b1d86 100644 >>>> --- a/src/ipa/rkisp1/rkisp1.cpp >>>> +++ b/src/ipa/rkisp1/rkisp1.cpp >>>> @@ -169,9 +169,6 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, >>>> return -ENODEV; >>>> } >>>> >>>> - context_.configuration.sensor.lineDuration = >>>> - sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate; >>>> - >>>> /* Load the tuning data file. */ >>>> File file(settings.configurationFile); >>>> if (!file.open(File::OpenModeFlag::ReadOnly)) { >>>> @@ -227,18 +224,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, >>>> context_.sensorInfo = ipaConfig.sensorInfo; >>>> context_.sensorControls = ipaConfig.sensorControls; >>>> >>>> - const auto itExp = context_.sensorControls.find(V4L2_CID_EXPOSURE); >>>> - int32_t minExposure = itExp->second.min().get<int32_t>(); >>>> - int32_t maxExposure = itExp->second.max().get<int32_t>(); >>>> - >>>> - const auto itGain = context_.sensorControls.find(V4L2_CID_ANALOGUE_GAIN); >>>> - int32_t minGain = itGain->second.min().get<int32_t>(); >>>> - int32_t maxGain = itGain->second.max().get<int32_t>(); >>>> - >>>> - LOG(IPARkISP1, Debug) >>>> - << "Exposure: [" << minExposure << ", " << maxExposure >>>> - << "], gain: [" << minGain << ", " << maxGain << "]"; >>>> - >>> >>> so we initialized these here but never really used them in init ? >> >> Some parts need to run in `init()` to provide data for `Camera::controls()`. >> But I decided to merge everything into a single function that does all >> the necessary calculations for `init()` and `configure()`. Or you mean >> something different? >> >> >>> >>>> /* Clear the IPA context before the streaming session. */ >>>> context_.configuration = {}; >>>> context_.activeState = {}; >>>> @@ -247,27 +232,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, >>>> context_.configuration.paramFormat = ipaConfig.paramFormat; >>>> >>>> context_.configuration.sensor.size = context_.sensorInfo.outputSize; >>>> - context_.configuration.sensor.lineDuration = context_.sensorInfo.minLineLength * 1.0s >>>> - / context_.sensorInfo.pixelRate; >>>> - >>>> - /* Update the camera controls using the new sensor settings. */ >>>> - updateControls(ipaControls); >>>> - >>>> - /* >>>> - * When the AGC computes the new exposure values for a frame, it needs >>>> - * to know the limits for exposure time and analogue gain. As it depends >>>> - * on the sensor, update it with the controls. >>>> - * >>>> - * \todo take VBLANK into account for maximum exposure time >>>> - */ >>>> - context_.configuration.sensor.minExposureTime = >>>> - minExposure * context_.configuration.sensor.lineDuration; >>>> - context_.configuration.sensor.maxExposureTime = >>>> - maxExposure * context_.configuration.sensor.lineDuration; >>>> - context_.configuration.sensor.minAnalogueGain = >>>> - context_.camHelper->gain(minGain); >>>> - context_.configuration.sensor.maxAnalogueGain = >>>> - context_.camHelper->gain(maxGain); >>>> >>>> context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(), >>>> [](auto &cfg) -> bool { >>>> @@ -289,6 +253,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, >>>> return ret; >>>> } >>>> >>>> + updateControls(ipaControls); >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -386,56 +352,6 @@ void IPARkISP1::updateControls(ControlInfoMap *ipaControls) >>>> { >>>> ControlInfoMap::Map ctrlMap = rkisp1Controls; >>>> >>>> - /* >>>> - * Compute exposure time limits from the V4L2_CID_EXPOSURE control >>>> - * limits and the line duration. >>>> - */ >>>> - double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); >>>> - const ControlInfo &v4l2Exposure = context_.sensorControls.find(V4L2_CID_EXPOSURE)->second; >>>> - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; >>>> - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; >>>> - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; >>>> - ctrlMap.emplace(std::piecewise_construct, >>>> - std::forward_as_tuple(&controls::ExposureTime), >>>> - std::forward_as_tuple(minExposure, maxExposure, defExposure)); >>>> - >>>> - /* Compute the analogue gain limits. */ >>>> - const ControlInfo &v4l2Gain = context_.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; >>>> - float minGain = context_.camHelper->gain(v4l2Gain.min().get<int32_t>()); >>>> - float maxGain = context_.camHelper->gain(v4l2Gain.max().get<int32_t>()); >>>> - float defGain = context_.camHelper->gain(v4l2Gain.def().get<int32_t>()); >>>> - ctrlMap.emplace(std::piecewise_construct, >>>> - std::forward_as_tuple(&controls::AnalogueGain), >>>> - std::forward_as_tuple(minGain, maxGain, defGain)); >>>> - >>>> - /* >>>> - * Compute the frame duration limits. >>>> - * >>>> - * The frame length is computed assuming a fixed line length combined >>>> - * with the vertical frame sizes. >>>> - */ >>>> - const ControlInfo &v4l2HBlank = context_.sensorControls.find(V4L2_CID_HBLANK)->second; >>>> - uint32_t hblank = v4l2HBlank.def().get<int32_t>(); >>>> - uint32_t lineLength = context_.sensorInfo.outputSize.width + hblank; >>>> - >>>> - const ControlInfo &v4l2VBlank = context_.sensorControls.find(V4L2_CID_VBLANK)->second; >>>> - std::array<uint32_t, 3> frameHeights{ >>>> - v4l2VBlank.min().get<int32_t>() + context_.sensorInfo.outputSize.height, >>>> - v4l2VBlank.max().get<int32_t>() + context_.sensorInfo.outputSize.height, >>>> - v4l2VBlank.def().get<int32_t>() + context_.sensorInfo.outputSize.height, >>>> - }; >>>> - >>>> - std::array<int64_t, 3> frameDurations; >>>> - for (unsigned int i = 0; i < frameHeights.size(); ++i) { >>>> - uint64_t frameSize = lineLength * frameHeights[i]; >>>> - frameDurations[i] = frameSize / (context_.sensorInfo.pixelRate / 1000000U); >>>> - } >>>> - >>>> - /* \todo Move this (and other agc-related controls) to agc */ >>> >>> Yey! >>> >>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> >>>> - context_.ctrlMap[&controls::FrameDurationLimits] = >>>> - ControlInfo(frameDurations[0], frameDurations[1], >>>> - ControlValue(Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } })); >>>> - >>>> ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); >>>> *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); >>>> } >>>> -- >>>> 2.54.0 >>>> >>
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 540b64ff99..689d045b7a 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -33,13 +33,96 @@ using namespace std::literals::chrono_literals; namespace ipa::rkisp1::algorithms { +LOG_DEFINE_CATEGORY(RkISP1Agc) + +namespace { + +void reconfigure(IPAContext &context) +{ + context.configuration.sensor.lineDuration = + context.sensorInfo.minLineLength * 1.0s / context.sensorInfo.pixelRate; + + double lineDurationUs = context.configuration.sensor.lineDuration.get<std::micro>(); + + /* + * Compute exposure time limits from the V4L2_CID_EXPOSURE control + * limits and the line duration. + */ + + const ControlInfo &v4l2Exposure = context.sensorControls.find(V4L2_CID_EXPOSURE)->second; + int32_t minExposure = v4l2Exposure.min().get<int32_t>(); + int32_t maxExposure = v4l2Exposure.max().get<int32_t>(); + int32_t defExposure = v4l2Exposure.def().get<int32_t>(); + context.ctrlMap[&controls::ExposureTime] = ControlInfo{ + static_cast<int32_t>(minExposure * lineDurationUs), + static_cast<int32_t>(maxExposure * lineDurationUs), + static_cast<int32_t>(defExposure * lineDurationUs), + }; + + /* Compute the analogue gain limits. */ + const ControlInfo &v4l2Gain = context.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; + float minGain = context.camHelper->gain(v4l2Gain.min().get<int32_t>()); + float maxGain = context.camHelper->gain(v4l2Gain.max().get<int32_t>()); + float defGain = context.camHelper->gain(v4l2Gain.def().get<int32_t>()); + context.ctrlMap[&controls::AnalogueGain] = ControlInfo{ + minGain, + maxGain, + defGain, + }; + + LOG(RkISP1Agc, Debug) + << "Exposure: [" << minExposure << ", " << maxExposure + << "], gain: [" << minGain << ", " << maxGain << "]"; + + /* + * Compute the frame duration limits. + * + * The frame length is computed assuming a fixed line length combined + * with the vertical frame sizes. + */ + const ControlInfo &v4l2HBlank = context.sensorControls.find(V4L2_CID_HBLANK)->second; + uint32_t hblank = v4l2HBlank.def().get<int32_t>(); + uint32_t lineLength = context.sensorInfo.outputSize.width + hblank; + + const ControlInfo &v4l2VBlank = context.sensorControls.find(V4L2_CID_VBLANK)->second; + std::array<uint32_t, 3> frameHeights{ + v4l2VBlank.min().get<int32_t>() + context.sensorInfo.outputSize.height, + v4l2VBlank.max().get<int32_t>() + context.sensorInfo.outputSize.height, + v4l2VBlank.def().get<int32_t>() + context.sensorInfo.outputSize.height, + }; + + std::array<int64_t, 3> frameDurations; + for (unsigned int i = 0; i < frameHeights.size(); ++i) { + uint64_t frameSize = lineLength * frameHeights[i]; + frameDurations[i] = frameSize / (context.sensorInfo.pixelRate / 1000000U); + } + + context.ctrlMap[&controls::FrameDurationLimits] = ControlInfo{ + frameDurations[0], + frameDurations[1], + Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } }, + }; + + /* + * When the AGC computes the new exposure values for a frame, it needs + * to know the limits for exposure time and analogue gain. As it depends + * on the sensor, update it with the controls. + * + * \todo take VBLANK into account for maximum exposure time + */ + context.configuration.sensor.minExposureTime = minExposure * context.configuration.sensor.lineDuration; + context.configuration.sensor.maxExposureTime = maxExposure * context.configuration.sensor.lineDuration; + context.configuration.sensor.minAnalogueGain = context.camHelper->gain(minGain); + context.configuration.sensor.maxAnalogueGain = context.camHelper->gain(maxGain); +} + +} /* namespace */ + /** * \class Agc * \brief A mean-based auto-exposure algorithm */ -LOG_DEFINE_CATEGORY(RkISP1Agc) - int Agc::parseMeteringModes(IPAContext &context, const ValueNode &tuningData) { if (!tuningData.isDictionary()) @@ -160,6 +243,8 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); context.ctrlMap.merge(agc_.controls()); + reconfigure(context); + return 0; } @@ -172,6 +257,8 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData) */ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) { + reconfigure(context); + /* Configure the default exposure and gain. */ context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; context.activeState.agc.automatic.exposure = diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 66a91c4d79..38f55b1d86 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -169,9 +169,6 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, return -ENODEV; } - context_.configuration.sensor.lineDuration = - sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate; - /* Load the tuning data file. */ File file(settings.configurationFile); if (!file.open(File::OpenModeFlag::ReadOnly)) { @@ -227,18 +224,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, context_.sensorInfo = ipaConfig.sensorInfo; context_.sensorControls = ipaConfig.sensorControls; - const auto itExp = context_.sensorControls.find(V4L2_CID_EXPOSURE); - int32_t minExposure = itExp->second.min().get<int32_t>(); - int32_t maxExposure = itExp->second.max().get<int32_t>(); - - const auto itGain = context_.sensorControls.find(V4L2_CID_ANALOGUE_GAIN); - int32_t minGain = itGain->second.min().get<int32_t>(); - int32_t maxGain = itGain->second.max().get<int32_t>(); - - LOG(IPARkISP1, Debug) - << "Exposure: [" << minExposure << ", " << maxExposure - << "], gain: [" << minGain << ", " << maxGain << "]"; - /* Clear the IPA context before the streaming session. */ context_.configuration = {}; context_.activeState = {}; @@ -247,27 +232,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, context_.configuration.paramFormat = ipaConfig.paramFormat; context_.configuration.sensor.size = context_.sensorInfo.outputSize; - context_.configuration.sensor.lineDuration = context_.sensorInfo.minLineLength * 1.0s - / context_.sensorInfo.pixelRate; - - /* Update the camera controls using the new sensor settings. */ - updateControls(ipaControls); - - /* - * When the AGC computes the new exposure values for a frame, it needs - * to know the limits for exposure time and analogue gain. As it depends - * on the sensor, update it with the controls. - * - * \todo take VBLANK into account for maximum exposure time - */ - context_.configuration.sensor.minExposureTime = - minExposure * context_.configuration.sensor.lineDuration; - context_.configuration.sensor.maxExposureTime = - maxExposure * context_.configuration.sensor.lineDuration; - context_.configuration.sensor.minAnalogueGain = - context_.camHelper->gain(minGain); - context_.configuration.sensor.maxAnalogueGain = - context_.camHelper->gain(maxGain); context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(), [](auto &cfg) -> bool { @@ -289,6 +253,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, return ret; } + updateControls(ipaControls); + return 0; } @@ -386,56 +352,6 @@ void IPARkISP1::updateControls(ControlInfoMap *ipaControls) { ControlInfoMap::Map ctrlMap = rkisp1Controls; - /* - * Compute exposure time limits from the V4L2_CID_EXPOSURE control - * limits and the line duration. - */ - double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); - const ControlInfo &v4l2Exposure = context_.sensorControls.find(V4L2_CID_EXPOSURE)->second; - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; - ctrlMap.emplace(std::piecewise_construct, - std::forward_as_tuple(&controls::ExposureTime), - std::forward_as_tuple(minExposure, maxExposure, defExposure)); - - /* Compute the analogue gain limits. */ - const ControlInfo &v4l2Gain = context_.sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; - float minGain = context_.camHelper->gain(v4l2Gain.min().get<int32_t>()); - float maxGain = context_.camHelper->gain(v4l2Gain.max().get<int32_t>()); - float defGain = context_.camHelper->gain(v4l2Gain.def().get<int32_t>()); - ctrlMap.emplace(std::piecewise_construct, - std::forward_as_tuple(&controls::AnalogueGain), - std::forward_as_tuple(minGain, maxGain, defGain)); - - /* - * Compute the frame duration limits. - * - * The frame length is computed assuming a fixed line length combined - * with the vertical frame sizes. - */ - const ControlInfo &v4l2HBlank = context_.sensorControls.find(V4L2_CID_HBLANK)->second; - uint32_t hblank = v4l2HBlank.def().get<int32_t>(); - uint32_t lineLength = context_.sensorInfo.outputSize.width + hblank; - - const ControlInfo &v4l2VBlank = context_.sensorControls.find(V4L2_CID_VBLANK)->second; - std::array<uint32_t, 3> frameHeights{ - v4l2VBlank.min().get<int32_t>() + context_.sensorInfo.outputSize.height, - v4l2VBlank.max().get<int32_t>() + context_.sensorInfo.outputSize.height, - v4l2VBlank.def().get<int32_t>() + context_.sensorInfo.outputSize.height, - }; - - std::array<int64_t, 3> frameDurations; - for (unsigned int i = 0; i < frameHeights.size(); ++i) { - uint64_t frameSize = lineLength * frameHeights[i]; - frameDurations[i] = frameSize / (context_.sensorInfo.pixelRate / 1000000U); - } - - /* \todo Move this (and other agc-related controls) to agc */ - context_.ctrlMap[&controls::FrameDurationLimits] = - ControlInfo(frameDurations[0], frameDurations[1], - ControlValue(Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } })); - ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); }
Move the `ControlInfo` setup and related initialization into the AGC algorithm instead of having it in the main IPA file. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/rkisp1/algorithms/agc.cpp | 91 ++++++++++++++++++++++++++++++- src/ipa/rkisp1/rkisp1.cpp | 88 +----------------------------- 2 files changed, 91 insertions(+), 88 deletions(-)