[libcamera-devel] [PATCH v1 3/3] ipa: rpi: agc: Gracefully handle missing agc modes
Naushir Patuck
naush at raspberrypi.com
Mon Jun 12 11:15:42 CEST 2023
Hi David,
On Mon, 12 Jun 2023 at 09:54, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> Hi everyone
>
> I think this is a good idea. The only question I have is whether we
> might simply print a warning and do nothing, rather than change to the
> default mode.
>
> I think this is a slightly less surprising behaviour - if you make a
> mistake it has no effect, rather than change you to some possibly
> unrelated mode that you might even have to undo again. It's also a
> general rule that could be applied to all controls: ask for something
> that can't be done and it tells you and does nothing.
>
> Thoughts?
I agree, it's slightly less surprising if we don't change the mode (instead of
changing to default) on the error condition. This also matches the behaviour of
various other control handling that we have in the IPA.
I'll make the change and post it shortly.
Regards,
Naush
>
> Thanks!
> David
>
> On Wed, 7 Jun 2023 at 20:56, Kieran Bingham via libcamera-devel
> <libcamera-devel at lists.libcamera.org> 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()")
> >
> > 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.
> >
> >
> > > 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.
> >
> > --
> > Kieran
> >
> >
> > >
> > > 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
More information about the libcamera-devel
mailing list