| Message ID | 20220719144517.15898-1-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 ? > > Fixes: 1c4d48018505 ("libcamera: controls: Use std::optional to handle invalid control values") > Signed-off-by: Naushir Patuck <naush@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 >
Hi Jacopo, On Tue, 19 Jul 2022 at 16:23, Jacopo Mondi <jacopo@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. I also may have misunderstood the new mechanism completely... :-) ...but this change does fix my spew of error messages. 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@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 > > >
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@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 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.. > > I also may have misunderstood the new mechanism completely... :-) > ...but this change does fix my spew of error messages. > > 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@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 > > > > >
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@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@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 >>>>
Hi Jacopo and Umang, On Tue, 19 Jul 2022 at 18:07, Umang Jain <umang.jain@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@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@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 > >>>> >
Hello, On Tue, Jul 19, 2022 at 06:16:03PM +0100, Naushir Patuck via libcamera-devel wrote: > On Tue, 19 Jul 2022 at 18:07, Umang Jain wrote: > > On 7/19/22 22:16, Jacopo Mondi via libcamera-devel wrote: > > > On Tue, Jul 19, 2022 at 05:13:15PM +0100, Naushir Patuck wrote: > > >> On Tue, 19 Jul 2022 at 16:23, Jacopo Mondi wrote: > > >>> 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. That's not valid, it looks like the documentation hasn't been updated when switching to std::optional<T>. I'll send a fix. > 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 :-) We have two variants of get(): template<typename T> std::optional<T> get(const Control<T> &ctrl) const; and const ControlValue &get(unsigned int id) const; The latter is used when looking up controls by numerical ID, which is used internally but not meant to be used by applications. That still needs a contains() check first. For the first variant, I agree contains() shouldn't be used. I'd like to drop the second variant, but that's all we have today to handle V4L2 controls. Jacopo is working on moving the IPA module interfaces to non-V4L2 controls, so there's hope there, but we'll need more time. > 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. I agree with the removal of the error message for the first variant of get(), but we should keep it for the second variant, as well as for set() which also calls find(). Note that we have const and non-const versions of find(), used in get() and set() respectively. The non-const version should keep the error message as it's used for set() only. To drop the message only from the std::optional version of get(), how about the following ? diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 192be78434dc..3e31b3633583 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -376,11 +376,11 @@ public: template<typename T> std::optional<T> get(const Control<T> &ctrl) const { - const ControlValue *val = find(ctrl.id()); - if (!val) + const auto entry = controls_.find(id); + if (entry == controls_.end()) return std::nullopt; - return val->get<T>(); + return entry->second.get<T>(); } template<typename T, typename V> > > >> 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! > > > > >> [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@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]; > > >>>> }
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]; }
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. Fixes: 1c4d48018505 ("libcamera: controls: Use std::optional to handle invalid control values") Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/controls.cpp | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)