[libcamera-devel] [PATCH] libcamera: controls: Suppress error message from ControlList::find()
Umang Jain
umang.jain at ideasonboard.com
Tue Jul 19 19:06:52 CEST 2022
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
>>>>
More information about the libcamera-devel
mailing list