[libcamera-devel] [PATCH v1 3/3] ipa: rpi: agc: Gracefully handle missing agc modes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 13 00:48:10 CEST 2023
Hello,
On Mon, Jun 12, 2023 at 10:15:42AM +0100, Naushir Patuck wrote:
> On Mon, 12 Jun 2023 at 09:54, David Plowman 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.
Sounds good to me too. Thanks David for the suggestion.
> I'll make the change and post it shortly.
>
> > On Wed, 7 Jun 2023 at 20:56, Kieran Bingham via libcamera-devel 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.
> > >
> > > > 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