[PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor controls

[Laurent Pinchart]

On Tue, Feb 04, 2025 at 09:27:26PM +0200, Laurent Pinchart wrote:
> On Tue, Feb 04, 2025 at 06:39:45PM +0000, Kieran Bingham wrote:
> > Quoting David Plowman (2025-02-04 16:05:06)
> > > Hi Naush
> > > 
> > > Thanks for the patch.
> > > 
> > > On Tue, 4 Feb 2025 at 15:09, Naushir Patuck <naush at raspberrypi.com> wrote:
> > > >
> > > > 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 min value.
> > > >
> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > 
> > > I suppose there is a bit of a question as to what these defaults mean,
> > > if anything. And if they don't mean much, then why do we need them.
> > > But this is obviously better than crashing, so:
> > > 
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > 
> > Yup, I agree, it's hard to know what the 'right' default should be in
> > all of these cases, and I think that's why defaults are not enforced on
> > the Control constructors, because sometimes I'm sure in some controls
> > it's really not possible to have a default.
> 
> But the IPA module will use a default when no value is specified. If the
> camera starts with AGC disabled, without specifying manual gain and
> exposure time, then the IPA will pick a value. Sometimes the default
> will make clear sense, sometimes it will just be arbitrary, but that's
> what the default reported in ControlInfo should be.
> 
> Does this patch report the correct values ? If so, it looks good to me.

Looking at AgcChannel::switchMode() initial fixed exposure time and
analogue gain are calculated and seem to produce different values than
the ones below. I think more work is needed.

> > But as this is better than crashing I think it's fine to merge this
> > patch - and we should try to determine which controls /require/ a
> > default value - and find a way to specify (and validate) such things.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > > Though it also feels like we need some better automated testing!! :)
> > > 
> > > 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..5924df4c1125 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>(mode_.minFrameDuration.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>(mode_.minAnalogueGain));
> > > >
> > > >         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>(mode_.minExposureTime.get<std::micro>()));
> > > >
> > > >         /* Declare colour processing related controls for non-mono sensors. */
> > > >         if (!monoSensor_)

-- 
Regards,

Laurent Pinchart