[libcamera-devel] [PATCH v1 3/3] ipa: rpi: agc: Gracefully handle missing agc modes

[Kieran Bingham]

Quoting Naushir Patuck via libcamera-devel (2023-06-07 11:00:54)
> If a metering/exposure/constraint mode is not listed in the sensor
> tuning file, and a control for the missing mode is set on the agc, we
> terminate the application with a fatal log message.
> 
> Instead of this fatal termination, log a warning message and switch to
> the appropriate default mode so that the application continues running.

I think this will improve the user experience better, at least it goes
from "Oh ... the program crashed" to "Oh the program didn't give me the
image settings I expected" ...

As long as the actually applied configuration is reported in the
metadata, I think that's fine and reasonable - and the warnings here
will help someone when they investigate the 'why'.



> 
> Reported-on: https://github.com/raspberrypi/libcamera/issues/59
> Reported-on: https://github.com/ayufan/camera-streamer/issues/67
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index b611157af1f0..1b05d478818e 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -540,24 +540,36 @@ void Agc::housekeepConfig()
>          */
>         if (meteringModeName_ != status_.meteringMode) {
>                 auto it = config_.meteringModes.find(meteringModeName_);
> -               if (it == config_.meteringModes.end())
> -                       LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_;
> -               meteringMode_ = &it->second;
> +               if (it == config_.meteringModes.end()) {
> +                       LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_
> +                                            << ", defaulting to " << config_.defaultMeteringMode;

Taking the default here seems like a good fallback. I also wonder if we
should instead take the settings from uncalibrated.json. But that may be
more tricky to get the data in - and would still need a fallback in case
*that* couldn't be found!.

So this seems like a good step forwards to me.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +                       meteringModeName_ = config_.defaultMeteringMode;
> +                       meteringMode_ = &config_.meteringModes[meteringModeName_];
> +               } else
> +                       meteringMode_ = &it->second;
>                 status_.meteringMode = meteringModeName_;
>         }
>         if (exposureModeName_ != status_.exposureMode) {
>                 auto it = config_.exposureModes.find(exposureModeName_);
> -               if (it == config_.exposureModes.end())
> -                       LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_;
> -               exposureMode_ = &it->second;
> +               if (it == config_.exposureModes.end()) {
> +                       LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_
> +                                            << ", defaulting to " << config_.defaultExposureMode;
> +                       exposureModeName_ = config_.defaultExposureMode;
> +                       exposureMode_ = &config_.exposureModes[exposureModeName_];
> +               } else
> +                       exposureMode_ = &it->second;
>                 status_.exposureMode = exposureModeName_;
>         }
>         if (constraintModeName_ != status_.constraintMode) {
>                 auto it =
>                         config_.constraintModes.find(constraintModeName_);
> -               if (it == config_.constraintModes.end())
> -                       LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_;
> -               constraintMode_ = &it->second;
> +               if (it == config_.constraintModes.end()) {
> +                       LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_
> +                                            << ", defaulting to " << config_.defaultConstraintMode;
> +                       constraintModeName_ = config_.defaultConstraintMode;
> +                       constraintMode_ = &config_.constraintModes[constraintModeName_];
> +               } else
> +                       constraintMode_ = &it->second;
>                 status_.constraintMode = constraintModeName_;
>         }
>         LOG(RPiAgc, Debug) << "exposureMode "
> -- 
> 2.34.1
>