| Message ID | 20250206092446.36237-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck (2025-02-06 09:24:36) > 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> > --- > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index bd3c22000df5..3c77665e6ab4 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -259,15 +259,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>())); > Is there any risk or concern now that the default could be reported outside of the min/max values when configured? But reading the if (firstStart_) {} code block, I think these are the values that will be used on start so I expect they are at least the current representation of the defaults. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > 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 Naush Thanks for the revised version. On Thu, 6 Feb 2025 at 11:33, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Naushir Patuck (2025-02-06 09:24:36) > > 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> David > > --- > > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > index bd3c22000df5..3c77665e6ab4 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -259,15 +259,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>())); > > > > Is there any risk or concern now that the default could be reported > outside of the min/max values when configured? > > But reading the if (firstStart_) {} code block, I think these are the > values that will be used on start so I expect they are at least the > current representation of the defaults. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > 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..3c77665e6ab4 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -259,15 +259,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_)
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> --- src/ipa/rpi/common/ipa_base.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)