[libcamera-devel] [PATCH] libcamera: controls: Suppress error message from ControlList::find()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jul 19 22:19:20 CEST 2022
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 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];
> > >>>> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list