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

[Umang Jain]

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.

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