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

[Laurent Pinchart]

On Wed, Jun 07, 2023 at 08:56:50PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2023-06-07 16:10:59)
> > Hi Naush,
> > 
> > Thank you for the patch.
> > 
> > On Wed, Jun 07, 2023 at 11:00:54AM +0100, Naushir Patuck via libcamera-devel wrote:
> > > 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.
> > > 
> > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59
> > > Reported-on: https://github.com/ayufan/camera-streamer/issues/67
> > 
> > We haven't used "Reported-on" before, and grepping in the whole Linux
> 
> Err ... we have:
> 
> 2a261d911f50 ("pipeline: raspberrypi: Iterate over all Unicam instances in match()")

Oops, indeed :-/

> Other references we've used for github issues include:
> 
>     Reported-on: https://github.com/raspberrypi/libcamera/issues/59
>     Reported-on: https://github.com/ayufan/camera-streamer/issues/67
>     Reported-on: https://github.com/raspberrypi/libcamera/issues/44
>     Bug: https://github.com/raspberrypi/libcamera/issues/29
>     Reported-by: https://github.com/kralo
>     Bug: https://github.com/raspberrypi/libcamera-apps/issues/217
>     Bug: https://github.com/raspberrypi/libcamera-apps/issues/236
>     Bug: https://github.com/raspberrypi/libcamera-apps/issues/238
>     Bug: https://github.com/raspberrypi/libcamera-apps/issues/240
>     See https://github.com/PyCQA/pycodestyle/issues/466
> 
> I think Bug: is the most appropriate still, so I'll use that.

In order to avoid manual handling if similar issues in the future, I've
implemented a git commit message trailers checker for checkstyle.py.
I'll post patches shortly.

> > kernel history there's a single occurrence. The tags used in the kernel
> > with github issues links are
> > 
> >       1 Ref.
> >       1 regression
> >       1 Reported-on
> >       1 see
> >       1 URL
> >       2 Issue
> >       2 link
> >       2 Ref
> >       3 Buglink
> >       3 Related
> >       3 Reported-by
> >       3 Resolves
> >       5 Bug
> >       5 References
> >       6 Fixes
> >       7 Reference
> >      17 Addresses-KSPP-ID
> >      43 See
> >      89 Closes
> >     158 BugLink
> >    1426 Link
> > 
> > We don't use BugLink and have used Link once only to refer to a mailing
> > list entry, while we use Bug for bugzilla entries. I would vote for Bug
> > or Link in this case.
> > 
> > > 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;
> > > +                     meteringModeName_ = config_.defaultMeteringMode;
> > > +                     meteringMode_ = &config_.meteringModes[meteringModeName_];
> > > +             } else
> > > +                     meteringMode_ = &it->second;
> > 
> > Please use curly braces here and below. This can be updated when
> > applying the patch, no need to resubmit.
> 
> Updated, and running through the integration tests.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > >               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 "

-- 
Regards,

Laurent Pinchart