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

[Jacopo Mondi]

Hi Naush,

On Tue, Jul 19, 2022 at 05:13:15PM +0100, Naushir Patuck wrote:
> 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.

No, you're right, as contains() has been dropped, get() is meant to
be called unconditionally and the control presence tested on the
returned optional pointer.

This is common for pipeline handlers, but I do expect applications to
inspect metadata using the same pattern, and an error message is
indeed misleading.

Let's see what others think. I'm a bit skeptic about removing the
message completely as it could be an indication of an un-checked error
condition, it could be demoted to Warn or Debug but again it seems
to be a sub-optimal solution..

>
> 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
> > >
> >