<div dir="ltr"><div dir="ltr">Hi Jacopo and Umang,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 19 Jul 2022 at 18:07, Umang Jain <<a href="mailto:umang.jain@ideasonboard.com">umang.jain@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On 7/19/22 22:16, Jacopo Mondi via libcamera-devel wrote:<br>
> Hi Naush,<br>
><br>
> On Tue, Jul 19, 2022 at 05:13:15PM +0100, Naushir Patuck wrote:<br>
>> Hi Jacopo,<br>
>><br>
>> On Tue, 19 Jul 2022 at 16:23, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
>><br>
>>> Hi Naush<br>
>>><br>
>>> On Tue, Jul 19, 2022 at 03:45:17PM +0100, Naushir Patuck via<br>
>>> libcamera-devel wrote:<br>
>>>> Now that controls::get() returns a std::optional<T> handle invalid<br>
>>> controls,<br>
>>>> the error log message in ControlList::find() is unnecessary and likely<br>
>>> invalid<br>
>>>> in the case when calling controls::get() on a missing control. Remove<br>
>>> this<br>
>>>> error message.<br>
>>> Isn't anyway up to the application to validate the returned<br>
>>> std::optional<> ? What if an application doesn't do so an tries to<br>
>>> access an std::nullopt ? At least the messages here would provide a<br>
>>> trace that something went wrong ?<br>
>>><br>
>> I was seeing this error message continually raised in one of our<br>
>> applications.<br>
>> The RPi pipeline handler checks if we have a ScalerCrop control set in the<br>
>> Request. This is done by a ControlList::get with the new update [1]. If<br>
>> this is not<br>
>> set, the error message pops up. I would want to defer the error handling<br>
>> in this<br>
>> case to the pipeline handler as not having ScalerCrop is perfectly valid.<br>
>> In the previous code, we called Controllist::contains(), which I can re-use<br>
>> but<br>
>> that sort of defeats the purpose of the std::optional change.<br>
> No, you're right, as contains() has been dropped, get() is meant to<br>
<br>
<br>
contains() hasn't been dropped yet, it's still present.<br>
<br>
<a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/controls.cpp#n934" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/controls.cpp#n934</a><br>
<br>
However, I have proposed dropping of it in the review although I missed <br>
to see ControlList::find() behaviour.<br>
<br>
> be called unconditionally and the control presence tested on the<br>
> returned optional pointer.<br>
><br>
> This is common for pipeline handlers, but I do expect applications to<br>
> inspect metadata using the same pattern, and an error message is<br>
> indeed misleading.<br>
><br>
> Let's see what others think. I'm a bit skeptic about removing the<br>
> message completely as it could be an indication of an un-checked error<br>
> condition, it could be demoted to Warn or Debug but again it seems<br>
> to be a sub-optimal solution..<br>
<br>
<br>
The distinction between invalid control and a missing control is an <br>
interesting one for ControlList::get()<br>
<br>
From the current documentation on master for get()<br>
<br>
* The behaviour is undefined if the control \a ctrl is not present in the<br>
* list. Use ControlList::contains() to test for the presence of a <br>
control in<br>
* the list before retrieving its value.<br>
<br>
Is this still valid with the ::get() change that got introduced? If yes, <br>
then I think this patch would make sense. But we can preserve the log <br>
and demote it to DEBUG/WARN as suggested.<br></blockquote><div><br></div><div>IMHO ::get() should supersede ::contains() as using std::optional gives us </div><div>everything we need to validate if a control is present or not. Additionally,</div><div>using ::contains() implies a possible double lookup which Laurent carefully</div><div>removed from the codebase in his last patch :-)</div><div><br></div><div>If we do keep a log message here (again I think this we shouldn't as the</div><div>absence is a valid condition) then I would prefer it at DEBUG level.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
>> I also may have misunderstood the new mechanism completely... :-)<br>
>> ...but this change does fix my spew of error messages.<br>
<br>
<br>
Now that I'm reading too, it makes me a bit fuzzy as well. With ::get(), <br>
contains() and find() and the roles of each.<br>
<br>
/me puts on thinking cap!<br>
<br>
>><br>
>> Regards,<br>
>> Naush<br>
>><br>
>> [1]:<br>
>> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2055" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2055</a><br>
>><br>
>><br>
>>>> Fixes: 1c4d48018505 ("libcamera: controls: Use std::optional to handle<br>
>>> invalid control values")<br>
>>>> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
>>>> ---<br>
>>>> src/libcamera/controls.cpp | 12 ++----------<br>
>>>> 1 file changed, 2 insertions(+), 10 deletions(-)<br>
>>>><br>
>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp<br>
>>>> index 03ac6345247c..701a872185e3 100644<br>
>>>> --- a/src/libcamera/controls.cpp<br>
>>>> +++ b/src/libcamera/controls.cpp<br>
>>>> @@ -1051,24 +1051,16 @@ void ControlList::set(unsigned int id, const<br>
>>> ControlValue &value)<br>
>>>> const ControlValue *ControlList::find(unsigned int id) const<br>
>>>> {<br>
>>>> const auto iter = controls_.find(id);<br>
>>>> - if (iter == controls_.end()) {<br>
>>>> - LOG(Controls, Error)<br>
>>>> - << "Control " << utils::hex(id) << " not found";<br>
>>>> -<br>
>>>> + if (iter == controls_.end())<br>
>>>> return nullptr;<br>
>>>> - }<br>
>>>><br>
>>>> return &iter->second;<br>
>>>> }<br>
>>>><br>
>>>> ControlValue *ControlList::find(unsigned int id)<br>
>>>> {<br>
>>>> - if (validator_ && !validator_->validate(id)) {<br>
>>>> - LOG(Controls, Error)<br>
>>>> - << "Control " << utils::hex(id)<br>
>>>> - << " is not valid for " << validator_->name();<br>
>>>> + if (validator_ && !validator_->validate(id))<br>
>>>> return nullptr;<br>
>>>> - }<br>
>>>><br>
>>>> return &controls_[id];<br>
>>>> }<br>
>>>> --<br>
>>>> 2.25.1<br>
>>>><br>
</blockquote></div></div>