[libcamera-devel] [PATCH] libcamera: controls: Suppress error message from ControlList::find()

[Naushir Patuck]

Hi Jacopo,

On Tue, 19 Jul 2022 at 16:23, Jacopo Mondi <jacopo at jmondi.org> wrote:

> Hi Naush
>
> On Tue, Jul 19, 2022 at 03:45:17PM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > Now that controls::get() returns a std::optional<T> handle invalid
> controls,
> > the error log message in ControlList::find() is unnecessary and likely
> invalid
> > in the case when calling controls::get() on a missing control. Remove
> this
> > error message.
>
> Isn't anyway up to the application to validate the returned
> std::optional<> ? What if an application doesn't do so an tries to
> access an std::nullopt ? At least the messages here would provide a
> trace that something went wrong ?
>

I was seeing this error message continually raised in one of our
applications.
The RPi pipeline handler checks if we have a ScalerCrop control set in the
Request.  This is done by a ControlList::get with the new update [1]. If
this is not
set, the error message pops up.  I would want to defer the error handling
in this
case to the pipeline handler as not having ScalerCrop is perfectly valid.
In the previous code, we called Controllist::contains(), which I can re-use
but
that sort of defeats the purpose of the std::optional change.

I also may have misunderstood the new mechanism completely... :-)
...but this change does fix my spew of error messages.

Regards,
Naush

[1]:
https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2055


>
> >
> > Fixes: 1c4d48018505 ("libcamera: controls: Use std::optional to handle
> invalid control values")
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/libcamera/controls.cpp | 12 ++----------
> >  1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 03ac6345247c..701a872185e3 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -1051,24 +1051,16 @@ void ControlList::set(unsigned int id, const
> ControlValue &value)
> >  const ControlValue *ControlList::find(unsigned int id) const
> >  {
> >       const auto iter = controls_.find(id);
> > -     if (iter == controls_.end()) {
> > -             LOG(Controls, Error)
> > -                     << "Control " << utils::hex(id) << " not found";
> > -
> > +     if (iter == controls_.end())
> >               return nullptr;
> > -     }
> >
> >       return &iter->second;
> >  }
> >
> >  ControlValue *ControlList::find(unsigned int id)
> >  {
> > -     if (validator_ && !validator_->validate(id)) {
> > -             LOG(Controls, Error)
> > -                     << "Control " << utils::hex(id)
> > -                     << " is not valid for " << validator_->name();
> > +     if (validator_ && !validator_->validate(id))
> >               return nullptr;
> > -     }
> >
> >       return &controls_[id];
> >  }
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220719/035ee847/attachment.htm>