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

[Naushir Patuck]

Hi Jacopo and Umang,

On Tue, 19 Jul 2022 at 18:07, Umang Jain <umang.jain at ideasonboard.com>
wrote:

> Hi,
>
> On 7/19/22 22:16, Jacopo Mondi via libcamera-devel wrote:
> > 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
>
>
> contains() hasn't been dropped yet, it's still present.
>
>
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/controls.cpp#n934
>
> However, I have proposed dropping of it in the review although I missed
> to see ControlList::find() behaviour.
>
> > 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..
>
>
> The distinction between invalid control and a missing control is an
> interesting one for ControlList::get()
>
>  From the current documentation on master for get()
>
>   * The behaviour is undefined if the control \a ctrl is not present in the
>   * list. Use ControlList::contains() to test for the presence of a
> control in
>   * the list before retrieving its value.
>
> Is this still valid with the ::get() change that got introduced? If yes,
> then I think this patch would make sense. But we can preserve the log
> and demote it to DEBUG/WARN as suggested.
>

IMHO ::get() should supersede ::contains() as using std::optional gives us
everything we need to validate if a control is present or not.
Additionally,
using ::contains() implies a possible double lookup which Laurent carefully
removed from the codebase in his last patch :-)

If we do keep a log message here (again I think this we shouldn't as the
absence is a valid condition) then I would prefer it at DEBUG level.

Regards,
Naush



> >
> >> I also may have misunderstood the new mechanism completely... :-)
> >> ...but this change does fix my spew of error messages.
>
>
> Now that I'm reading too, it makes me a bit fuzzy as well. With ::get(),
> contains() and find() and the roles of each.
>
> /me puts on thinking cap!
>
> >>
> >> 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/3a5c5d23/attachment.htm>