| Message ID | 20230322161317.18055-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via libcamera-devel wrote: > Populate all the fields of the SensorLimits structure in configure(). > This allows us to use the cached values instead of re-computing them > on every frame. > > For the gain -> code convertion, ensure we clamp to the analogue gain > limits set in the SensorLimits structure. I wonder if this can't be done by expanding your CameraMode structure. CameraMode already combines information from the SensorInfo and could very well include information from sensorControls. This would avoid introducing another type to move it around across different layers of your IPA. Have you considered that and decided you prefer introducing SensorLimits ? > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 67 ++++++++++++++++------------- > 1 file changed, 38 insertions(+), 29 deletions(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index c3b2c375036f..e8d8023bcfe7 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -224,8 +224,8 @@ private: > Duration minFrameDuration_; > Duration maxFrameDuration_; > > - /* Maximum gain code for the sensor. */ > - uint32_t maxSensorGainCode_; > + /* Mode specific sensor gain/exposure limits. */ > + RPiController::AgcAlgorithm::SensorLimits sensorLimits_; > > /* Track the frame length times over FrameLengthsQueueSize frames. */ > std::deque<Duration> frameLengths_; > @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > } > } > > - maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > - > /* Setup a metadata ControlList to output metadata. */ > libcameraMetadata_ = ControlList(controls::controls); > > @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > /* Pass the camera mode to the CamHelper to setup algorithms. */ > helper_->setCameraMode(mode_); > > + /* > + * Store the sensor gain and shutter limits for the mode. > + * > + * The maximum shutter value are calculated from the frame duration limit > + * as V4L2 will restrict the maximum control value based on the current > + * VBLANK value. > + * > + * These limits get set in the AGC algorithm through applyFrameDurations() > + * below. > + */ > + const ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN); > + const ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE); > + > + sensorLimits_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > + sensorLimits_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > + sensorLimits_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength; > + sensorLimits_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; > + sensorLimits_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > + sensorLimits_.maxShutter = Duration::max(); > + helper_->getBlanking(sensorLimits_.maxShutter, > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); > + > /* > * Initialise this ControlList correctly, even if empty, in case the IPA is > * running is isolation mode (passing the ControlList through the IPC layer). > @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > * based on the current sensor mode. > */ > ControlInfoMap::Map ctrlMap = ipaControls; > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; > ctrlMap[&controls::FrameDurationLimits] = > - ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()), > - static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>())); > + ControlInfo(static_cast<int64_t>(sensorLimits_.minFrameDuration.get<std::micro>()), > + static_cast<int64_t>(sensorLimits_.maxFrameDuration.get<std::micro>())); > > ctrlMap[&controls::AnalogueGain] = > - ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_))); > - > - /* > - * Calculate the max exposure limit from the frame duration limit as V4L2 > - * will limit the maximum control value based on the current VBLANK value. > - */ > - Duration maxShutter = Duration::max(); > - helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration); > - const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>(); > + ControlInfo(static_cast<float>(sensorLimits_.minAnalogueGain), > + static_cast<float>(sensorLimits_.maxAnalogueGain)); > > ctrlMap[&controls::ExposureTime] = > - ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()), > - static_cast<int32_t>(maxShutter.get<std::micro>())); > + ControlInfo(static_cast<int32_t>(sensorLimits_.minShutter.get<std::micro>()), > + static_cast<int32_t>(sensorLimits_.maxShutter.get<std::micro>())); > > /* Declare Autofocus controls, only if we have a controllable lens */ > if (lensPresent_) > @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) > > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration) > { > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; > - > /* > * This will only be applied once AGC recalculations occur. > * The values may be clamped based on the sensor mode capabilities as well. > @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur > minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration; > maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration; > minFrameDuration_ = std::clamp(minFrameDuration_, > - minSensorFrameDuration, maxSensorFrameDuration); > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); > maxFrameDuration_ = std::clamp(maxFrameDuration_, > - minSensorFrameDuration, maxSensorFrameDuration); > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_); > > /* Return the validated limits via metadata. */ > @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur > * getBlanking() will update maxShutter with the largest exposure > * value possible. > */ > - RPiController::AgcAlgorithm::SensorLimits limits; > - limits.maxShutter = Duration::max(); > - helper_->getBlanking(limits.maxShutter, minFrameDuration_, maxFrameDuration_); > + sensorLimits_.maxShutter = Duration::max(); > + helper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_, maxFrameDuration_); > > RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > controller_.getAlgorithm("agc")); > - agc->setSensorLimits(limits); > + agc->setSensorLimits(sensorLimits_); > } > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) > { > + const int32_t minGainCode = helper_->gainCode(sensorLimits_.minAnalogueGain); > + const int32_t maxGainCode = helper_->gainCode(sensorLimits_.maxAnalogueGain); > int32_t gainCode = helper_->gainCode(agcStatus->analogueGain); > > /* > @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) > * DelayedControls. The AGC will correctly handle a lower gain returned > * by the sensor, provided it knows the actual gain used. > */ > - gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); > + gainCode = std::clamp<int32_t>(gainCode, minGainCode, maxGainCode); > > /* getBlanking might clip exposure time to the fps limits. */ > Duration exposure = agcStatus->shutterTime; > -- > 2.34.1 >
Hi Jacopo, Thank you for the feedback. On Fri, 24 Mar 2023 at 14:23, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Naush > > On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via > libcamera-devel wrote: > > Populate all the fields of the SensorLimits structure in configure(). > > This allows us to use the cached values instead of re-computing them > > on every frame. > > > > For the gain -> code convertion, ensure we clamp to the analogue gain > > limits set in the SensorLimits structure. > > I wonder if this can't be done by expanding your CameraMode structure. > CameraMode already combines information from the SensorInfo and could > very well include information from sensorControls. > > This would avoid introducing another type to move it around across > different layers of your IPA. > > Have you considered that and decided you prefer introducing > SensorLimits ? > That's a great idea! It definitely fits well in the CameraMode structure. If there are no objection from David, I'll rework the series to move these fields into CameraMode. Regards, Naush > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/raspberrypi/raspberrypi.cpp | 67 ++++++++++++++++------------- > > 1 file changed, 38 insertions(+), 29 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index c3b2c375036f..e8d8023bcfe7 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -224,8 +224,8 @@ private: > > Duration minFrameDuration_; > > Duration maxFrameDuration_; > > > > - /* Maximum gain code for the sensor. */ > > - uint32_t maxSensorGainCode_; > > + /* Mode specific sensor gain/exposure limits. */ > > + RPiController::AgcAlgorithm::SensorLimits sensorLimits_; > > > > /* Track the frame length times over FrameLengthsQueueSize frames. > */ > > std::deque<Duration> frameLengths_; > > @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo > &sensorInfo, const IPAConfig &ip > > } > > } > > > > - maxSensorGainCode_ = > sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > > - > > /* Setup a metadata ControlList to output metadata. */ > > libcameraMetadata_ = ControlList(controls::controls); > > > > @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo > &sensorInfo, const IPAConfig &ip > > /* Pass the camera mode to the CamHelper to setup algorithms. */ > > helper_->setCameraMode(mode_); > > > > + /* > > + * Store the sensor gain and shutter limits for the mode. > > + * > > + * The maximum shutter value are calculated from the frame > duration limit > > + * as V4L2 will restrict the maximum control value based on the > current > > + * VBLANK value. > > + * > > + * These limits get set in the AGC algorithm through > applyFrameDurations() > > + * below. > > + */ > > + const ControlInfo &gainCtrl = > sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN); > > + const ControlInfo &shutterCtrl = > sensorCtrls_.at(V4L2_CID_EXPOSURE); > > + > > + sensorLimits_.minAnalogueGain = > helper_->gain(gainCtrl.min().get<int32_t>()); > > + sensorLimits_.maxAnalogueGain = > helper_->gain(gainCtrl.max().get<int32_t>()); > > + sensorLimits_.minFrameDuration = mode_.minFrameLength * > mode_.minLineLength; > > + sensorLimits_.maxFrameDuration = mode_.maxFrameLength * > mode_.maxLineLength; > > + sensorLimits_.minShutter = > helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > + sensorLimits_.maxShutter = Duration::max(); > > + helper_->getBlanking(sensorLimits_.maxShutter, > > + sensorLimits_.minFrameDuration, > sensorLimits_.maxFrameDuration); > > + > > /* > > * Initialise this ControlList correctly, even if empty, in case > the IPA is > > * running is isolation mode (passing the ControlList through the > IPC layer). > > @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo > &sensorInfo, const IPAConfig &ip > > * based on the current sensor mode. > > */ > > ControlInfoMap::Map ctrlMap = ipaControls; > > - const Duration minSensorFrameDuration = mode_.minFrameLength * > mode_.minLineLength; > > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * > mode_.maxLineLength; > > ctrlMap[&controls::FrameDurationLimits] = > > - > ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()), > > - > static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>())); > > + > ControlInfo(static_cast<int64_t>(sensorLimits_.minFrameDuration.get<std::micro>()), > > + > static_cast<int64_t>(sensorLimits_.maxFrameDuration.get<std::micro>())); > > > > ctrlMap[&controls::AnalogueGain] = > > - ControlInfo(1.0f, > static_cast<float>(helper_->gain(maxSensorGainCode_))); > > - > > - /* > > - * Calculate the max exposure limit from the frame duration limit > as V4L2 > > - * will limit the maximum control value based on the current > VBLANK value. > > - */ > > - Duration maxShutter = Duration::max(); > > - helper_->getBlanking(maxShutter, minSensorFrameDuration, > maxSensorFrameDuration); > > - const uint32_t exposureMin = > sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>(); > > + > ControlInfo(static_cast<float>(sensorLimits_.minAnalogueGain), > > + > static_cast<float>(sensorLimits_.maxAnalogueGain)); > > > > ctrlMap[&controls::ExposureTime] = > > - > ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, > mode_.minLineLength).get<std::micro>()), > > - > static_cast<int32_t>(maxShutter.get<std::micro>())); > > + > ControlInfo(static_cast<int32_t>(sensorLimits_.minShutter.get<std::micro>()), > > + > static_cast<int32_t>(sensorLimits_.maxShutter.get<std::micro>())); > > > > /* Declare Autofocus controls, only if we have a controllable lens > */ > > if (lensPresent_) > > @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus > *awbStatus, ControlList &ctrls) > > > > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration > maxFrameDuration) > > { > > - const Duration minSensorFrameDuration = mode_.minFrameLength * > mode_.minLineLength; > > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * > mode_.maxLineLength; > > - > > /* > > * This will only be applied once AGC recalculations occur. > > * The values may be clamped based on the sensor mode capabilities > as well. > > @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration > minFrameDuration, Duration maxFrameDur > > minFrameDuration_ = minFrameDuration ? minFrameDuration : > defaultMaxFrameDuration; > > maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : > defaultMinFrameDuration; > > minFrameDuration_ = std::clamp(minFrameDuration_, > > - minSensorFrameDuration, > maxSensorFrameDuration); > > + sensorLimits_.minFrameDuration, > sensorLimits_.maxFrameDuration); > > maxFrameDuration_ = std::clamp(maxFrameDuration_, > > - minSensorFrameDuration, > maxSensorFrameDuration); > > + sensorLimits_.minFrameDuration, > sensorLimits_.maxFrameDuration); > > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_); > > > > /* Return the validated limits via metadata. */ > > @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration > minFrameDuration, Duration maxFrameDur > > * getBlanking() will update maxShutter with the largest exposure > > * value possible. > > */ > > - RPiController::AgcAlgorithm::SensorLimits limits; > > - limits.maxShutter = Duration::max(); > > - helper_->getBlanking(limits.maxShutter, minFrameDuration_, > maxFrameDuration_); > > + sensorLimits_.maxShutter = Duration::max(); > > + helper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_, > maxFrameDuration_); > > > > RPiController::AgcAlgorithm *agc = > dynamic_cast<RPiController::AgcAlgorithm *>( > > controller_.getAlgorithm("agc")); > > - agc->setSensorLimits(limits); > > + agc->setSensorLimits(sensorLimits_); > > } > > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList > &ctrls) > > { > > + const int32_t minGainCode = > helper_->gainCode(sensorLimits_.minAnalogueGain); > > + const int32_t maxGainCode = > helper_->gainCode(sensorLimits_.maxAnalogueGain); > > int32_t gainCode = helper_->gainCode(agcStatus->analogueGain); > > > > /* > > @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus > *agcStatus, ControlList &ctrls) > > * DelayedControls. The AGC will correctly handle a lower gain > returned > > * by the sensor, provided it knows the actual gain used. > > */ > > - gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); > > + gainCode = std::clamp<int32_t>(gainCode, minGainCode, maxGainCode); > > > > /* getBlanking might clip exposure time to the fps limits. */ > > Duration exposure = agcStatus->shutterTime; > > -- > > 2.34.1 > > >
Hi Naush, Jacopo Just wanted to check whether there's no issue with these things changing dynamically (by which I mean not a mode-switch time). For example if someone sets a new framerate while things are running, or something. Thanks! David On Fri, 24 Mar 2023 at 14:54, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Hi Jacopo, > > Thank you for the feedback. > > > On Fri, 24 Mar 2023 at 14:23, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: >> >> Hi Naush >> >> On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via libcamera-devel wrote: >> > Populate all the fields of the SensorLimits structure in configure(). >> > This allows us to use the cached values instead of re-computing them >> > on every frame. >> > >> > For the gain -> code convertion, ensure we clamp to the analogue gain >> > limits set in the SensorLimits structure. >> >> I wonder if this can't be done by expanding your CameraMode structure. >> CameraMode already combines information from the SensorInfo and could >> very well include information from sensorControls. >> >> This would avoid introducing another type to move it around across >> different layers of your IPA. >> >> Have you considered that and decided you prefer introducing >> SensorLimits ? > > > That's a great idea! It definitely fits well in the CameraMode structure. > If there are no objection from David, I'll rework the series to move these > fields into CameraMode. > > Regards, > Naush > >> >> >> > >> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> >> > --- >> > src/ipa/raspberrypi/raspberrypi.cpp | 67 ++++++++++++++++------------- >> > 1 file changed, 38 insertions(+), 29 deletions(-) >> > >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp >> > index c3b2c375036f..e8d8023bcfe7 100644 >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp >> > @@ -224,8 +224,8 @@ private: >> > Duration minFrameDuration_; >> > Duration maxFrameDuration_; >> > >> > - /* Maximum gain code for the sensor. */ >> > - uint32_t maxSensorGainCode_; >> > + /* Mode specific sensor gain/exposure limits. */ >> > + RPiController::AgcAlgorithm::SensorLimits sensorLimits_; >> > >> > /* Track the frame length times over FrameLengthsQueueSize frames. */ >> > std::deque<Duration> frameLengths_; >> > @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip >> > } >> > } >> > >> > - maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); >> > - >> > /* Setup a metadata ControlList to output metadata. */ >> > libcameraMetadata_ = ControlList(controls::controls); >> > >> > @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip >> > /* Pass the camera mode to the CamHelper to setup algorithms. */ >> > helper_->setCameraMode(mode_); >> > >> > + /* >> > + * Store the sensor gain and shutter limits for the mode. >> > + * >> > + * The maximum shutter value are calculated from the frame duration limit >> > + * as V4L2 will restrict the maximum control value based on the current >> > + * VBLANK value. >> > + * >> > + * These limits get set in the AGC algorithm through applyFrameDurations() >> > + * below. >> > + */ >> > + const ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN); >> > + const ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE); >> > + >> > + sensorLimits_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); >> > + sensorLimits_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); >> > + sensorLimits_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength; >> > + sensorLimits_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; >> > + sensorLimits_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); >> > + sensorLimits_.maxShutter = Duration::max(); >> > + helper_->getBlanking(sensorLimits_.maxShutter, >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); >> > + >> > /* >> > * Initialise this ControlList correctly, even if empty, in case the IPA is >> > * running is isolation mode (passing the ControlList through the IPC layer). >> > @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip >> > * based on the current sensor mode. >> > */ >> > ControlInfoMap::Map ctrlMap = ipaControls; >> > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; >> > ctrlMap[&controls::FrameDurationLimits] = >> > - ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()), >> > - static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>())); >> > + ControlInfo(static_cast<int64_t>(sensorLimits_.minFrameDuration.get<std::micro>()), >> > + static_cast<int64_t>(sensorLimits_.maxFrameDuration.get<std::micro>())); >> > >> > ctrlMap[&controls::AnalogueGain] = >> > - ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_))); >> > - >> > - /* >> > - * Calculate the max exposure limit from the frame duration limit as V4L2 >> > - * will limit the maximum control value based on the current VBLANK value. >> > - */ >> > - Duration maxShutter = Duration::max(); >> > - helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration); >> > - const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>(); >> > + ControlInfo(static_cast<float>(sensorLimits_.minAnalogueGain), >> > + static_cast<float>(sensorLimits_.maxAnalogueGain)); >> > >> > ctrlMap[&controls::ExposureTime] = >> > - ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()), >> > - static_cast<int32_t>(maxShutter.get<std::micro>())); >> > + ControlInfo(static_cast<int32_t>(sensorLimits_.minShutter.get<std::micro>()), >> > + static_cast<int32_t>(sensorLimits_.maxShutter.get<std::micro>())); >> > >> > /* Declare Autofocus controls, only if we have a controllable lens */ >> > if (lensPresent_) >> > @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) >> > >> > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration) >> > { >> > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; >> > - >> > /* >> > * This will only be applied once AGC recalculations occur. >> > * The values may be clamped based on the sensor mode capabilities as well. >> > @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur >> > minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration; >> > maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration; >> > minFrameDuration_ = std::clamp(minFrameDuration_, >> > - minSensorFrameDuration, maxSensorFrameDuration); >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); >> > maxFrameDuration_ = std::clamp(maxFrameDuration_, >> > - minSensorFrameDuration, maxSensorFrameDuration); >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); >> > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_); >> > >> > /* Return the validated limits via metadata. */ >> > @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur >> > * getBlanking() will update maxShutter with the largest exposure >> > * value possible. >> > */ >> > - RPiController::AgcAlgorithm::SensorLimits limits; >> > - limits.maxShutter = Duration::max(); >> > - helper_->getBlanking(limits.maxShutter, minFrameDuration_, maxFrameDuration_); >> > + sensorLimits_.maxShutter = Duration::max(); >> > + helper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_, maxFrameDuration_); >> > >> > RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( >> > controller_.getAlgorithm("agc")); >> > - agc->setSensorLimits(limits); >> > + agc->setSensorLimits(sensorLimits_); >> > } >> > >> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) >> > { >> > + const int32_t minGainCode = helper_->gainCode(sensorLimits_.minAnalogueGain); >> > + const int32_t maxGainCode = helper_->gainCode(sensorLimits_.maxAnalogueGain); >> > int32_t gainCode = helper_->gainCode(agcStatus->analogueGain); >> > >> > /* >> > @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) >> > * DelayedControls. The AGC will correctly handle a lower gain returned >> > * by the sensor, provided it knows the actual gain used. >> > */ >> > - gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); >> > + gainCode = std::clamp<int32_t>(gainCode, minGainCode, maxGainCode); >> > >> > /* getBlanking might clip exposure time to the fps limits. */ >> > Duration exposure = agcStatus->shutterTime; >> > -- >> > 2.34.1 >> >
Hi David On Fri, Mar 24, 2023 at 06:11:06PM +0000, David Plowman via libcamera-devel wrote: > Hi Naush, Jacopo > > Just wanted to check whether there's no issue with these things > changing dynamically (by which I mean not a mode-switch time). For > example if someone sets a new framerate while things are running, or > something. Do I parse your questions right as: The 'sensor limits' passed to the AGC does not only change at configure() time, but also when a new FrameDurationLimits is set by an application. Particularly, setting a new FrameDurationLimits limits the maximum shutter time the AGC algorithm can use. As I read it, maxShutter is the only parameter that changes dynamically, all the rest of the structure remains un-changed between configurations. That's why it feels a bit of a duplication of CameraMode to me. This highlights that the maxShutter limit calculation doesn't probably belong to the main IPA module but is rather part of the AGC. The only other place where you use it is the ControlInfo limits initialization, again at configure() time. We handle cases like this in other IPA by having each algorithm implement queueRequest() and handling the controls they're interested in. In this way the state of an algorithm is as self-contained as possible, and the main reason I'm not thrilled by this patch is that is introduces yet-another state representation shared between multiple layers. I understand that to do the shutter calculation in the algorithm you would need to have access to the helper there, something you currently have not. Is this by design ? By the way your IPA is kind of "your territory" as you do most of the maintainance there, so if this feels right/better to you, I won't certainly oppose > > Thanks! > David > > On Fri, 24 Mar 2023 at 14:54, Naushir Patuck via libcamera-devel > <libcamera-devel@lists.libcamera.org> wrote: > > > > Hi Jacopo, > > > > Thank you for the feedback. > > > > > > On Fri, 24 Mar 2023 at 14:23, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > >> > >> Hi Naush > >> > >> On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via libcamera-devel wrote: > >> > Populate all the fields of the SensorLimits structure in configure(). > >> > This allows us to use the cached values instead of re-computing them > >> > on every frame. > >> > > >> > For the gain -> code convertion, ensure we clamp to the analogue gain > >> > limits set in the SensorLimits structure. > >> > >> I wonder if this can't be done by expanding your CameraMode structure. > >> CameraMode already combines information from the SensorInfo and could > >> very well include information from sensorControls. > >> > >> This would avoid introducing another type to move it around across > >> different layers of your IPA. > >> > >> Have you considered that and decided you prefer introducing > >> SensorLimits ? > > > > > > That's a great idea! It definitely fits well in the CameraMode structure. > > If there are no objection from David, I'll rework the series to move these > > fields into CameraMode. > > > > Regards, > > Naush > > > >> > >> > >> > > >> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > >> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > >> > --- > >> > src/ipa/raspberrypi/raspberrypi.cpp | 67 ++++++++++++++++------------- > >> > 1 file changed, 38 insertions(+), 29 deletions(-) > >> > > >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > >> > index c3b2c375036f..e8d8023bcfe7 100644 > >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp > >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > >> > @@ -224,8 +224,8 @@ private: > >> > Duration minFrameDuration_; > >> > Duration maxFrameDuration_; > >> > > >> > - /* Maximum gain code for the sensor. */ > >> > - uint32_t maxSensorGainCode_; > >> > + /* Mode specific sensor gain/exposure limits. */ > >> > + RPiController::AgcAlgorithm::SensorLimits sensorLimits_; > >> > > >> > /* Track the frame length times over FrameLengthsQueueSize frames. */ > >> > std::deque<Duration> frameLengths_; > >> > @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > >> > } > >> > } > >> > > >> > - maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > >> > - > >> > /* Setup a metadata ControlList to output metadata. */ > >> > libcameraMetadata_ = ControlList(controls::controls); > >> > > >> > @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > >> > /* Pass the camera mode to the CamHelper to setup algorithms. */ > >> > helper_->setCameraMode(mode_); > >> > > >> > + /* > >> > + * Store the sensor gain and shutter limits for the mode. > >> > + * > >> > + * The maximum shutter value are calculated from the frame duration limit > >> > + * as V4L2 will restrict the maximum control value based on the current > >> > + * VBLANK value. > >> > + * > >> > + * These limits get set in the AGC algorithm through applyFrameDurations() > >> > + * below. > >> > + */ > >> > + const ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN); > >> > + const ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE); > >> > + > >> > + sensorLimits_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > >> > + sensorLimits_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > >> > + sensorLimits_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength; > >> > + sensorLimits_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; > >> > + sensorLimits_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > >> > + sensorLimits_.maxShutter = Duration::max(); > >> > + helper_->getBlanking(sensorLimits_.maxShutter, > >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); > >> > + > >> > /* > >> > * Initialise this ControlList correctly, even if empty, in case the IPA is > >> > * running is isolation mode (passing the ControlList through the IPC layer). > >> > @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > >> > * based on the current sensor mode. > >> > */ > >> > ControlInfoMap::Map ctrlMap = ipaControls; > >> > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; > >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; > >> > ctrlMap[&controls::FrameDurationLimits] = > >> > - ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()), > >> > - static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>())); > >> > + ControlInfo(static_cast<int64_t>(sensorLimits_.minFrameDuration.get<std::micro>()), > >> > + static_cast<int64_t>(sensorLimits_.maxFrameDuration.get<std::micro>())); > >> > > >> > ctrlMap[&controls::AnalogueGain] = > >> > - ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_))); > >> > - > >> > - /* > >> > - * Calculate the max exposure limit from the frame duration limit as V4L2 > >> > - * will limit the maximum control value based on the current VBLANK value. > >> > - */ > >> > - Duration maxShutter = Duration::max(); > >> > - helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration); > >> > - const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>(); > >> > + ControlInfo(static_cast<float>(sensorLimits_.minAnalogueGain), > >> > + static_cast<float>(sensorLimits_.maxAnalogueGain)); > >> > > >> > ctrlMap[&controls::ExposureTime] = > >> > - ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()), > >> > - static_cast<int32_t>(maxShutter.get<std::micro>())); > >> > + ControlInfo(static_cast<int32_t>(sensorLimits_.minShutter.get<std::micro>()), > >> > + static_cast<int32_t>(sensorLimits_.maxShutter.get<std::micro>())); > >> > > >> > /* Declare Autofocus controls, only if we have a controllable lens */ > >> > if (lensPresent_) > >> > @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) > >> > > >> > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration) > >> > { > >> > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; > >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; > >> > - > >> > /* > >> > * This will only be applied once AGC recalculations occur. > >> > * The values may be clamped based on the sensor mode capabilities as well. > >> > @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur > >> > minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration; > >> > maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration; > >> > minFrameDuration_ = std::clamp(minFrameDuration_, > >> > - minSensorFrameDuration, maxSensorFrameDuration); > >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); > >> > maxFrameDuration_ = std::clamp(maxFrameDuration_, > >> > - minSensorFrameDuration, maxSensorFrameDuration); > >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); > >> > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_); > >> > > >> > /* Return the validated limits via metadata. */ > >> > @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur > >> > * getBlanking() will update maxShutter with the largest exposure > >> > * value possible. > >> > */ > >> > - RPiController::AgcAlgorithm::SensorLimits limits; > >> > - limits.maxShutter = Duration::max(); > >> > - helper_->getBlanking(limits.maxShutter, minFrameDuration_, maxFrameDuration_); > >> > + sensorLimits_.maxShutter = Duration::max(); > >> > + helper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_, maxFrameDuration_); > >> > > >> > RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > >> > controller_.getAlgorithm("agc")); > >> > - agc->setSensorLimits(limits); > >> > + agc->setSensorLimits(sensorLimits_); > >> > } > >> > > >> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) > >> > { > >> > + const int32_t minGainCode = helper_->gainCode(sensorLimits_.minAnalogueGain); > >> > + const int32_t maxGainCode = helper_->gainCode(sensorLimits_.maxAnalogueGain); > >> > int32_t gainCode = helper_->gainCode(agcStatus->analogueGain); > >> > > >> > /* > >> > @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) > >> > * DelayedControls. The AGC will correctly handle a lower gain returned > >> > * by the sensor, provided it knows the actual gain used. > >> > */ > >> > - gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); > >> > + gainCode = std::clamp<int32_t>(gainCode, minGainCode, maxGainCode); > >> > > >> > /* getBlanking might clip exposure time to the fps limits. */ > >> > Duration exposure = agcStatus->shutterTime; > >> > -- > >> > 2.34.1 > >> >
Hi Jacopo and David, On Sat, 25 Mar 2023 at 09:19, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi David > > On Fri, Mar 24, 2023 at 06:11:06PM +0000, David Plowman via > libcamera-devel wrote: > > Hi Naush, Jacopo > > > > Just wanted to check whether there's no issue with these things > > changing dynamically (by which I mean not a mode-switch time). For > > example if someone sets a new framerate while things are running, or > > something. > > Do I parse your questions right as: > > The 'sensor limits' passed to the AGC does not only change at configure() > time, but also when a new FrameDurationLimits is set by an application. > Particularly, setting a new FrameDurationLimits limits the maximum > shutter time the AGC algorithm can use. > > As I read it, maxShutter is the only parameter that changes > dynamically, all the rest of the structure remains un-changed between > configurations. That's why it feels a bit of a duplication of > CameraMode to me. This highlights that the maxShutter limit calculation > doesn't probably belong to the main IPA module but is rather part of > the AGC. The only other place where you use it is the ControlInfo > limits initialization, again at configure() time. > This is correct. At runtime, only the max shutter value will be updated. As such, I see no reason not to move the SensorLimits fields into the CameraMode structure. > > We handle cases like this in other IPA by having each algorithm implement > queueRequest() and handling the controls they're interested in. In > this way the state of an algorithm is as self-contained as possible, > and the main reason I'm not thrilled by this patch is that is > introduces yet-another state representation shared between multiple > layers. I understand that to do the shutter calculation in the algorithm > you would need to have access to the helper there, something you > currently have not. Is this by design ? > This is somewhat by design. We try and keep our algorithm code as clean as possible, i.e. mostly independent of libcamera dependencies like ControlLists as well as sensor dependencies like CamHelpers. The IPA code as a bridge between the algorithms and the rest of the world. > > By the way your IPA is kind of "your territory" as you do most of the > maintainance there, so if this feels right/better to you, I won't > certainly oppose > I'll explore the suggestion to move the fields to CameraMode, I think it's probably worth it for convenience. Regards, Naush > > > > Thanks! > > David > > > > On Fri, 24 Mar 2023 at 14:54, Naushir Patuck via libcamera-devel > > <libcamera-devel@lists.libcamera.org> wrote: > > > > > > Hi Jacopo, > > > > > > Thank you for the feedback. > > > > > > > > > On Fri, 24 Mar 2023 at 14:23, Jacopo Mondi < > jacopo.mondi@ideasonboard.com> wrote: > > >> > > >> Hi Naush > > >> > > >> On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via > libcamera-devel wrote: > > >> > Populate all the fields of the SensorLimits structure in > configure(). > > >> > This allows us to use the cached values instead of re-computing them > > >> > on every frame. > > >> > > > >> > For the gain -> code convertion, ensure we clamp to the analogue > gain > > >> > limits set in the SensorLimits structure. > > >> > > >> I wonder if this can't be done by expanding your CameraMode structure. > > >> CameraMode already combines information from the SensorInfo and could > > >> very well include information from sensorControls. > > >> > > >> This would avoid introducing another type to move it around across > > >> different layers of your IPA. > > >> > > >> Have you considered that and decided you prefer introducing > > >> SensorLimits ? > > > > > > > > > That's a great idea! It definitely fits well in the CameraMode > structure. > > > If there are no objection from David, I'll rework the series to move > these > > > fields into CameraMode. > > > > > > Regards, > > > Naush > > > > > >> > > >> > > >> > > > >> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > >> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > >> > --- > > >> > src/ipa/raspberrypi/raspberrypi.cpp | 67 > ++++++++++++++++------------- > > >> > 1 file changed, 38 insertions(+), 29 deletions(-) > > >> > > > >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > >> > index c3b2c375036f..e8d8023bcfe7 100644 > > >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > >> > @@ -224,8 +224,8 @@ private: > > >> > Duration minFrameDuration_; > > >> > Duration maxFrameDuration_; > > >> > > > >> > - /* Maximum gain code for the sensor. */ > > >> > - uint32_t maxSensorGainCode_; > > >> > + /* Mode specific sensor gain/exposure limits. */ > > >> > + RPiController::AgcAlgorithm::SensorLimits sensorLimits_; > > >> > > > >> > /* Track the frame length times over FrameLengthsQueueSize > frames. */ > > >> > std::deque<Duration> frameLengths_; > > >> > @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo > &sensorInfo, const IPAConfig &ip > > >> > } > > >> > } > > >> > > > >> > - maxSensorGainCode_ = > sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > > >> > - > > >> > /* Setup a metadata ControlList to output metadata. */ > > >> > libcameraMetadata_ = ControlList(controls::controls); > > >> > > > >> > @@ -473,6 +471,28 @@ int IPARPi::configure(const > IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > > >> > /* Pass the camera mode to the CamHelper to setup algorithms. > */ > > >> > helper_->setCameraMode(mode_); > > >> > > > >> > + /* > > >> > + * Store the sensor gain and shutter limits for the mode. > > >> > + * > > >> > + * The maximum shutter value are calculated from the frame > duration limit > > >> > + * as V4L2 will restrict the maximum control value based on > the current > > >> > + * VBLANK value. > > >> > + * > > >> > + * These limits get set in the AGC algorithm through > applyFrameDurations() > > >> > + * below. > > >> > + */ > > >> > + const ControlInfo &gainCtrl = > sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN); > > >> > + const ControlInfo &shutterCtrl = > sensorCtrls_.at(V4L2_CID_EXPOSURE); > > >> > + > > >> > + sensorLimits_.minAnalogueGain = > helper_->gain(gainCtrl.min().get<int32_t>()); > > >> > + sensorLimits_.maxAnalogueGain = > helper_->gain(gainCtrl.max().get<int32_t>()); > > >> > + sensorLimits_.minFrameDuration = mode_.minFrameLength * > mode_.minLineLength; > > >> > + sensorLimits_.maxFrameDuration = mode_.maxFrameLength * > mode_.maxLineLength; > > >> > + sensorLimits_.minShutter = > helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > >> > + sensorLimits_.maxShutter = Duration::max(); > > >> > + helper_->getBlanking(sensorLimits_.maxShutter, > > >> > + sensorLimits_.minFrameDuration, > sensorLimits_.maxFrameDuration); > > >> > + > > >> > /* > > >> > * Initialise this ControlList correctly, even if empty, in > case the IPA is > > >> > * running is isolation mode (passing the ControlList through > the IPC layer). > > >> > @@ -501,26 +521,17 @@ int IPARPi::configure(const > IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > > >> > * based on the current sensor mode. > > >> > */ > > >> > ControlInfoMap::Map ctrlMap = ipaControls; > > >> > - const Duration minSensorFrameDuration = mode_.minFrameLength > * mode_.minLineLength; > > >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength > * mode_.maxLineLength; > > >> > ctrlMap[&controls::FrameDurationLimits] = > > >> > - > ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()), > > >> > - > static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>())); > > >> > + > ControlInfo(static_cast<int64_t>(sensorLimits_.minFrameDuration.get<std::micro>()), > > >> > + > static_cast<int64_t>(sensorLimits_.maxFrameDuration.get<std::micro>())); > > >> > > > >> > ctrlMap[&controls::AnalogueGain] = > > >> > - ControlInfo(1.0f, > static_cast<float>(helper_->gain(maxSensorGainCode_))); > > >> > - > > >> > - /* > > >> > - * Calculate the max exposure limit from the frame duration > limit as V4L2 > > >> > - * will limit the maximum control value based on the current > VBLANK value. > > >> > - */ > > >> > - Duration maxShutter = Duration::max(); > > >> > - helper_->getBlanking(maxShutter, minSensorFrameDuration, > maxSensorFrameDuration); > > >> > - const uint32_t exposureMin = > sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>(); > > >> > + > ControlInfo(static_cast<float>(sensorLimits_.minAnalogueGain), > > >> > + > static_cast<float>(sensorLimits_.maxAnalogueGain)); > > >> > > > >> > ctrlMap[&controls::ExposureTime] = > > >> > - > ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, > mode_.minLineLength).get<std::micro>()), > > >> > - > static_cast<int32_t>(maxShutter.get<std::micro>())); > > >> > + > ControlInfo(static_cast<int32_t>(sensorLimits_.minShutter.get<std::micro>()), > > >> > + > static_cast<int32_t>(sensorLimits_.maxShutter.get<std::micro>())); > > >> > > > >> > /* Declare Autofocus controls, only if we have a controllable > lens */ > > >> > if (lensPresent_) > > >> > @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus > *awbStatus, ControlList &ctrls) > > >> > > > >> > void IPARPi::applyFrameDurations(Duration minFrameDuration, > Duration maxFrameDuration) > > >> > { > > >> > - const Duration minSensorFrameDuration = mode_.minFrameLength > * mode_.minLineLength; > > >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength > * mode_.maxLineLength; > > >> > - > > >> > /* > > >> > * This will only be applied once AGC recalculations occur. > > >> > * The values may be clamped based on the sensor mode > capabilities as well. > > >> > @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration > minFrameDuration, Duration maxFrameDur > > >> > minFrameDuration_ = minFrameDuration ? minFrameDuration : > defaultMaxFrameDuration; > > >> > maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : > defaultMinFrameDuration; > > >> > minFrameDuration_ = std::clamp(minFrameDuration_, > > >> > - minSensorFrameDuration, > maxSensorFrameDuration); > > >> > + > sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); > > >> > maxFrameDuration_ = std::clamp(maxFrameDuration_, > > >> > - minSensorFrameDuration, > maxSensorFrameDuration); > > >> > + > sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); > > >> > maxFrameDuration_ = std::max(maxFrameDuration_, > minFrameDuration_); > > >> > > > >> > /* Return the validated limits via metadata. */ > > >> > @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration > minFrameDuration, Duration maxFrameDur > > >> > * getBlanking() will update maxShutter with the largest > exposure > > >> > * value possible. > > >> > */ > > >> > - RPiController::AgcAlgorithm::SensorLimits limits; > > >> > - limits.maxShutter = Duration::max(); > > >> > - helper_->getBlanking(limits.maxShutter, minFrameDuration_, > maxFrameDuration_); > > >> > + sensorLimits_.maxShutter = Duration::max(); > > >> > + helper_->getBlanking(sensorLimits_.maxShutter, > minFrameDuration_, maxFrameDuration_); > > >> > > > >> > RPiController::AgcAlgorithm *agc = > dynamic_cast<RPiController::AgcAlgorithm *>( > > >> > controller_.getAlgorithm("agc")); > > >> > - agc->setSensorLimits(limits); > > >> > + agc->setSensorLimits(sensorLimits_); > > >> > } > > >> > > > >> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, > ControlList &ctrls) > > >> > { > > >> > + const int32_t minGainCode = > helper_->gainCode(sensorLimits_.minAnalogueGain); > > >> > + const int32_t maxGainCode = > helper_->gainCode(sensorLimits_.maxAnalogueGain); > > >> > int32_t gainCode = helper_->gainCode(agcStatus->analogueGain); > > >> > > > >> > /* > > >> > @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus > *agcStatus, ControlList &ctrls) > > >> > * DelayedControls. The AGC will correctly handle a lower > gain returned > > >> > * by the sensor, provided it knows the actual gain used. > > >> > */ > > >> > - gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); > > >> > + gainCode = std::clamp<int32_t>(gainCode, minGainCode, > maxGainCode); > > >> > > > >> > /* getBlanking might clip exposure time to the fps limits. */ > > >> > Duration exposure = agcStatus->shutterTime; > > >> > -- > > >> > 2.34.1 > > >> > >
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index c3b2c375036f..e8d8023bcfe7 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -224,8 +224,8 @@ private: Duration minFrameDuration_; Duration maxFrameDuration_; - /* Maximum gain code for the sensor. */ - uint32_t maxSensorGainCode_; + /* Mode specific sensor gain/exposure limits. */ + RPiController::AgcAlgorithm::SensorLimits sensorLimits_; /* Track the frame length times over FrameLengthsQueueSize frames. */ std::deque<Duration> frameLengths_; @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip } } - maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); - /* Setup a metadata ControlList to output metadata. */ libcameraMetadata_ = ControlList(controls::controls); @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip /* Pass the camera mode to the CamHelper to setup algorithms. */ helper_->setCameraMode(mode_); + /* + * Store the sensor gain and shutter limits for the mode. + * + * The maximum shutter value are calculated from the frame duration limit + * as V4L2 will restrict the maximum control value based on the current + * VBLANK value. + * + * These limits get set in the AGC algorithm through applyFrameDurations() + * below. + */ + const ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN); + const ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE); + + sensorLimits_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); + sensorLimits_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); + sensorLimits_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength; + sensorLimits_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; + sensorLimits_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); + sensorLimits_.maxShutter = Duration::max(); + helper_->getBlanking(sensorLimits_.maxShutter, + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); + /* * Initialise this ControlList correctly, even if empty, in case the IPA is * running is isolation mode (passing the ControlList through the IPC layer). @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip * based on the current sensor mode. */ ControlInfoMap::Map ctrlMap = ipaControls; - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; ctrlMap[&controls::FrameDurationLimits] = - ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()), - static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>())); + ControlInfo(static_cast<int64_t>(sensorLimits_.minFrameDuration.get<std::micro>()), + static_cast<int64_t>(sensorLimits_.maxFrameDuration.get<std::micro>())); ctrlMap[&controls::AnalogueGain] = - ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_))); - - /* - * Calculate the max exposure limit from the frame duration limit as V4L2 - * will limit the maximum control value based on the current VBLANK value. - */ - Duration maxShutter = Duration::max(); - helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration); - const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>(); + ControlInfo(static_cast<float>(sensorLimits_.minAnalogueGain), + static_cast<float>(sensorLimits_.maxAnalogueGain)); ctrlMap[&controls::ExposureTime] = - ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()), - static_cast<int32_t>(maxShutter.get<std::micro>())); + ControlInfo(static_cast<int32_t>(sensorLimits_.minShutter.get<std::micro>()), + static_cast<int32_t>(sensorLimits_.maxShutter.get<std::micro>())); /* Declare Autofocus controls, only if we have a controllable lens */ if (lensPresent_) @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration) { - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength; - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; - /* * This will only be applied once AGC recalculations occur. * The values may be clamped based on the sensor mode capabilities as well. @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration; maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration; minFrameDuration_ = std::clamp(minFrameDuration_, - minSensorFrameDuration, maxSensorFrameDuration); + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); maxFrameDuration_ = std::clamp(maxFrameDuration_, - minSensorFrameDuration, maxSensorFrameDuration); + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration); maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_); /* Return the validated limits via metadata. */ @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur * getBlanking() will update maxShutter with the largest exposure * value possible. */ - RPiController::AgcAlgorithm::SensorLimits limits; - limits.maxShutter = Duration::max(); - helper_->getBlanking(limits.maxShutter, minFrameDuration_, maxFrameDuration_); + sensorLimits_.maxShutter = Duration::max(); + helper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_, maxFrameDuration_); RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( controller_.getAlgorithm("agc")); - agc->setSensorLimits(limits); + agc->setSensorLimits(sensorLimits_); } void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) { + const int32_t minGainCode = helper_->gainCode(sensorLimits_.minAnalogueGain); + const int32_t maxGainCode = helper_->gainCode(sensorLimits_.maxAnalogueGain); int32_t gainCode = helper_->gainCode(agcStatus->analogueGain); /* @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) * DelayedControls. The AGC will correctly handle a lower gain returned * by the sensor, provided it knows the actual gain used. */ - gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); + gainCode = std::clamp<int32_t>(gainCode, minGainCode, maxGainCode); /* getBlanking might clip exposure time to the fps limits. */ Duration exposure = agcStatus->shutterTime;