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

[Naushir Patuck]

Hi Laurent,

On Tue, 4 Feb 2025 at 19:37, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.

I'm not sure that's correct, but It could be because of my
interpretation of the default values...

AgcChannel::switchMode() is called at the time of Camera::start().
That point seems too late to set the ControlInfo default value.  From
a user's perspective, fetching a control default after the camera has
started does not seem intuitive to me.  Also noting that the default
value here is the default in manual mode, which is not the default AGC
mode :)  But again, this might just be my interpretation of how/when
default values are accessed by the user.

However, I have missed something in this patch - there are specific
default analogue gain, shutter and frame duration values that are
global to the IPA that get setup in ipa::configure().  Instead of
using the min values as default, I should explicitly use those values.

I'll post a v2 patch for folks to consider.

Regards,
Naush


>
> > > 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