[libcamera-devel] [PATCH v2 3/3] ipa: ipu3: Ensure controls exists in updateSessionConfiguration()

[Kieran Bingham]

Quoting Umang Jain via libcamera-devel (2022-03-05 17:28:49)
> Add a control-not-found error checking for the controls being
> queried in IPAIPU3::updateSessionConfiguration().
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 0df358ba..583d6b9a 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -180,16 +180,31 @@ private:
>   */
>  void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>  {
> -       const ControlInfo vBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
> -       context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>();
> +       const auto itVBlank = sensorControls.find(V4L2_CID_VBLANK);

I'm torn whether we should have a single list of mandatory controls that
we check at init()/configure() or this which at least checks and obtains
the control in one loop.


But I note that this function is returning void, and so you are not
returning an error if the controls are not found - So I think that's
bad. It needs ot have the error propogated back up - ... so that means I
think a single check in configure() (or if we can during init()) would
be better.


> +       if (itVBlank == sensorControls.end()) {
> +               LOG(IPAIPU3, Error) << "Can't find vblank control";
> +               return;
> +       }
>  
> -       const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> -       int32_t minExposure = v4l2Exposure.min().get<int32_t>();
> -       int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
> +       context_.configuration.sensor.defVBlank = itVBlank->second.def().get<int32_t>();
> +
> +       const auto itExp = sensorControls.find(V4L2_CID_EXPOSURE);
> +       if (itExp == sensorControls.end()) {
> +               LOG(IPAIPU3, Error) << "Can't find exposure control";
> +               return;
> +       }
> +
> +       int32_t minExposure = itExp->second.min().get<int32_t>();
> +       int32_t maxExposure = itExp->second.max().get<int32_t>();
> +
> +       const auto itGain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);
> +       if (itGain == sensorControls.end()) {
> +               LOG(IPAIPU3, Error) << "Can't find analogue gain control";
> +               return;
> +       }
>  
> -       const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;
> -       int32_t minGain = v4l2Gain.min().get<int32_t>();
> -       int32_t maxGain = v4l2Gain.max().get<int32_t>();
> +       int32_t minGain = itGain->second.min().get<int32_t>();
> +       int32_t maxGain = itGain->second.max().get<int32_t>();
>  
>         /*
>          * When the AGC computes the new exposure values for a frame, it needs
> -- 
> 2.31.0
>