[libcamera-devel] [PATCH] ipa: rpi: agc: Do not switch to a default if a mode is unavailable

[David Plowman]

Hi Naush

Thanks for the patch!

On Mon, 12 Jun 2023 at 14:30, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> In commit 0ee9339331c6, a default metering/exposure/constraint mode is
> used if a control sets a mode that is not listed in the camera tuning
> file.
>
> Setting a default mode may be undesirable in these cases, so instead
> keep the agc mode unchanged. This also matches the behaviour for other
> IPA controls where no changes are made in error conditions.
>
> Fixes: 0ee9339331c6 ("ipa: rpi: agc: Gracefully handle missing agc modes")
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>

It's only a marginal thing but yes, I think I prefer this version!

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

Thanks!
David

> ---
>  src/ipa/rpi/controller/rpi/agc.cpp | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index e60ca33f867b..ae9ff219fa03 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -541,39 +541,32 @@ void Agc::housekeepConfig()
>         if (meteringModeName_ != status_.meteringMode) {
>                 auto it = config_.meteringModes.find(meteringModeName_);
>                 if (it == config_.meteringModes.end()) {
> -                       LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_
> -                                            << ", defaulting to " << config_.defaultMeteringMode;
> -                       meteringModeName_ = config_.defaultMeteringMode;
> -                       meteringMode_ = &config_.meteringModes[meteringModeName_];
> +                       LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_;
> +                       meteringModeName_ = status_.meteringMode;
>                 } else {
>                         meteringMode_ = &it->second;
> +                       status_.meteringMode = meteringModeName_;
>                 }
> -               status_.meteringMode = meteringModeName_;
>         }
>         if (exposureModeName_ != status_.exposureMode) {
>                 auto it = config_.exposureModes.find(exposureModeName_);
>                 if (it == config_.exposureModes.end()) {
> -                       LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_
> -                                            << ", defaulting to " << config_.defaultExposureMode;
> -                       exposureModeName_ = config_.defaultExposureMode;
> -                       exposureMode_ = &config_.exposureModes[exposureModeName_];
> +                       LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_;
> +                       exposureModeName_ = status_.exposureMode;
>                 } else {
>                         exposureMode_ = &it->second;
> +                       status_.exposureMode = exposureModeName_;
>                 }
> -               status_.exposureMode = exposureModeName_;
>         }
>         if (constraintModeName_ != status_.constraintMode) {
> -               auto it =
> -                       config_.constraintModes.find(constraintModeName_);
> +               auto it = config_.constraintModes.find(constraintModeName_);
>                 if (it == config_.constraintModes.end()) {
> -                       LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_
> -                                            << ", defaulting to " << config_.defaultConstraintMode;
> -                       constraintModeName_ = config_.defaultConstraintMode;
> -                       constraintMode_ = &config_.constraintModes[constraintModeName_];
> +                       LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_;
> +                       constraintModeName_ = status_.constraintMode;
>                 } else {
>                         constraintMode_ = &it->second;
> +                       status_.constraintMode = constraintModeName_;
>                 }
> -               status_.constraintMode = constraintModeName_;
>         }
>         LOG(RPiAgc, Debug) << "exposureMode "
>                            << exposureModeName_ << " constraintMode "
> --
> 2.34.1
>