<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 19 Jul 2022 at 16:23, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</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 Naush<br>
<br>
On Tue, Jul 19, 2022 at 03:45:17PM +0100, Naushir Patuck via libcamera-devel wrote:<br>
> Now that controls::get() returns a std::optional<T> handle invalid controls,<br>
> the error log message in ControlList::find() is unnecessary and likely invalid<br>
> in the case when calling controls::get() on a missing control. Remove this<br>
> error message.<br>
<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></blockquote><div><br></div><div>I was seeing this error message continually raised in one of our applications.</div><div>The RPi pipeline handler checks if we have a ScalerCrop control set in the</div><div>Request.  This is done by a ControlList::get with the new update [1]. If this is not</div><div>set, the error message pops up.  I would want to defer the error handling in this</div><div>case to the pipeline handler as not having ScalerCrop is perfectly valid.</div><div>In the previous code, we called Controllist::contains(), which I can re-use but<br></div><div>that sort of defeats the purpose of the std::optional change.</div><div><br></div><div>I also may have misunderstood the new mechanism completely... :-)</div><div>...but this change does fix my spew of error messages.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div>[1]: <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2055">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2055</a></div><div> </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>
> Fixes: 1c4d48018505 ("libcamera: controls: Use std::optional to handle 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 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>