Message ID | 20250211102327.55021-1-naush@raspberrypi.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Quoting Naushir Patuck (2025-02-11 10:20:02) > The existing IPA initialisation code did not set default values for > some sensor related controls. This caused a crash using libcamerify > when the it was trying to access the default value for > controls::FrameDurationLimits as part of a recent change. > > Ensure controls::FrameDurationLimits, controls::AnalogueGain and > controls::ExposureTime advertise default values along with the existing > min/max values. The default is set to the defaults defined in the IPA > set during initialisation. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253 > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> If you're ready with this I can merge it. I saw some githubs fly by about the default values of AWB/AGC enable. Should they be set to report they are enabled by default too? (Let's treat that as a separate topic from this patch though). > --- > > Changes in v3: > > Need to apply the same default in the init phase since the test in the > V4L2CameraProxy::setFmtFromConfig() happens before calling ipa::configure(). > > --- > src/ipa/rpi/common/ipa_base.cpp | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index bd3c22000df5..5d2c6d40da3e 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -61,12 +61,13 @@ const ControlInfoMap::Map ipaControls{ > ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto), > static_cast<int32_t>(controls::ExposureTimeModeManual), > static_cast<int32_t>(controls::ExposureTimeModeAuto)) }, > - { &controls::ExposureTime, ControlInfo(0, 66666) }, > + { &controls::ExposureTime, > + ControlInfo(1, 66666, static_cast<int64_t>(defaultExposureTime.get<std::micro>())) }, > { &controls::AnalogueGainMode, > ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto), > static_cast<int32_t>(controls::AnalogueGainModeManual), > static_cast<int32_t>(controls::AnalogueGainModeAuto)) }, > - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) }, > + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) }, > { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) }, > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > @@ -80,7 +81,9 @@ const ControlInfoMap::Map ipaControls{ > { &controls::HdrMode, ControlInfo(controls::HdrModeValues) }, > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) }, > + { &controls::FrameDurationLimits, > + ControlInfo(INT64_C(33333), INT64_C(120000), > + static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())) }, > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > { &controls::rpi::StatsOutputEnable, ControlInfo(false, true, false) }, > }; > @@ -259,15 +262,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa > ControlInfoMap::Map ctrlMap = ipaControls; > ctrlMap[&controls::FrameDurationLimits] = > ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), > - static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); > + static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()), > + static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())); > > ctrlMap[&controls::AnalogueGain] = > ControlInfo(static_cast<float>(mode_.minAnalogueGain), > - static_cast<float>(mode_.maxAnalogueGain)); > + static_cast<float>(mode_.maxAnalogueGain), > + static_cast<float>(defaultAnalogueGain)); > > ctrlMap[&controls::ExposureTime] = > ControlInfo(static_cast<int32_t>(mode_.minExposureTime.get<std::micro>()), > - static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>())); > + static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()), > + static_cast<int32_t>(defaultExposureTime.get<std::micro>())); > > /* Declare colour processing related controls for non-mono sensors. */ > if (!monoSensor_) > -- > 2.43.0 >
Hi Kieran, On Tue, 11 Feb 2025 at 12:22, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Naushir Patuck (2025-02-11 10:20:02) > > The existing IPA initialisation code did not set default values for > > some sensor related controls. This caused a crash using libcamerify > > when the it was trying to access the default value for > > controls::FrameDurationLimits as part of a recent change. > > > > Ensure controls::FrameDurationLimits, controls::AnalogueGain and > > controls::ExposureTime advertise default values along with the existing > > min/max values. The default is set to the defaults defined in the IPA > > set during initialisation. > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253 > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > If you're ready with this I can merge it. > > I saw some githubs fly by about the default values of AWB/AGC enable. > Should they be set to report they are enabled by default too? Actually some of those have already been fixed upstream by Paul, but I haven't rebased our downstream tree with the commits yet. There are still others that I have questions above. > > (Let's treat that as a separate topic from this patch though). Agreed, feel free to merge this patch now. Regards, Naush > > > > > --- > > > > Changes in v3: > > > > Need to apply the same default in the init phase since the test in the > > V4L2CameraProxy::setFmtFromConfig() happens before calling ipa::configure(). > > > > --- > > src/ipa/rpi/common/ipa_base.cpp | 18 ++++++++++++------ > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > index bd3c22000df5..5d2c6d40da3e 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -61,12 +61,13 @@ const ControlInfoMap::Map ipaControls{ > > ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto), > > static_cast<int32_t>(controls::ExposureTimeModeManual), > > static_cast<int32_t>(controls::ExposureTimeModeAuto)) }, > > - { &controls::ExposureTime, ControlInfo(0, 66666) }, > > + { &controls::ExposureTime, > > + ControlInfo(1, 66666, static_cast<int64_t>(defaultExposureTime.get<std::micro>())) }, > > { &controls::AnalogueGainMode, > > ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto), > > static_cast<int32_t>(controls::AnalogueGainModeManual), > > static_cast<int32_t>(controls::AnalogueGainModeAuto)) }, > > - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) }, > > + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) }, > > { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) }, > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > > @@ -80,7 +81,9 @@ const ControlInfoMap::Map ipaControls{ > > { &controls::HdrMode, ControlInfo(controls::HdrModeValues) }, > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) }, > > + { &controls::FrameDurationLimits, > > + ControlInfo(INT64_C(33333), INT64_C(120000), > > + static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())) }, > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > > { &controls::rpi::StatsOutputEnable, ControlInfo(false, true, false) }, > > }; > > @@ -259,15 +262,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa > > ControlInfoMap::Map ctrlMap = ipaControls; > > ctrlMap[&controls::FrameDurationLimits] = > > ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), > > - static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); > > + static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()), > > + static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())); > > > > ctrlMap[&controls::AnalogueGain] = > > ControlInfo(static_cast<float>(mode_.minAnalogueGain), > > - static_cast<float>(mode_.maxAnalogueGain)); > > + static_cast<float>(mode_.maxAnalogueGain), > > + static_cast<float>(defaultAnalogueGain)); > > > > ctrlMap[&controls::ExposureTime] = > > ControlInfo(static_cast<int32_t>(mode_.minExposureTime.get<std::micro>()), > > - static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>())); > > + static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()), > > + static_cast<int32_t>(defaultExposureTime.get<std::micro>())); > > > > /* Declare colour processing related controls for non-mono sensors. */ > > if (!monoSensor_) > > -- > > 2.43.0 > >
diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index bd3c22000df5..5d2c6d40da3e 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -61,12 +61,13 @@ const ControlInfoMap::Map ipaControls{ ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto), static_cast<int32_t>(controls::ExposureTimeModeManual), static_cast<int32_t>(controls::ExposureTimeModeAuto)) }, - { &controls::ExposureTime, ControlInfo(0, 66666) }, + { &controls::ExposureTime, + ControlInfo(1, 66666, static_cast<int64_t>(defaultExposureTime.get<std::micro>())) }, { &controls::AnalogueGainMode, ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto), static_cast<int32_t>(controls::AnalogueGainModeManual), static_cast<int32_t>(controls::AnalogueGainModeAuto)) }, - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) }, + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) }, { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) }, { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, @@ -80,7 +81,9 @@ const ControlInfoMap::Map ipaControls{ { &controls::HdrMode, ControlInfo(controls::HdrModeValues) }, { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) }, + { &controls::FrameDurationLimits, + ControlInfo(INT64_C(33333), INT64_C(120000), + static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())) }, { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, { &controls::rpi::StatsOutputEnable, ControlInfo(false, true, false) }, }; @@ -259,15 +262,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa ControlInfoMap::Map ctrlMap = ipaControls; ctrlMap[&controls::FrameDurationLimits] = ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), - static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); + static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()), + static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())); ctrlMap[&controls::AnalogueGain] = ControlInfo(static_cast<float>(mode_.minAnalogueGain), - static_cast<float>(mode_.maxAnalogueGain)); + static_cast<float>(mode_.maxAnalogueGain), + static_cast<float>(defaultAnalogueGain)); ctrlMap[&controls::ExposureTime] = ControlInfo(static_cast<int32_t>(mode_.minExposureTime.get<std::micro>()), - static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>())); + static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()), + static_cast<int32_t>(defaultExposureTime.get<std::micro>())); /* Declare colour processing related controls for non-mono sensors. */ if (!monoSensor_)