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

22775 diff mbox series

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

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

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