[libcamera-devel] [PATCH 1/2] libcamera: controls: Return ControlValue reference from ControlList::set()

[Laurent Pinchart]

Hi Jacopo,

On Sun, Apr 26, 2020 at 04:24:54PM +0200, Jacopo Mondi wrote:
> On Sun, Apr 26, 2020 at 02:16:22AM +0300, Laurent Pinchart wrote:
> > It is useful for the caller of ControlList::set() to get a reference to
> > the ControlValue that stores the control in the control list, in order
> > to then modify that value directly. This allows creating an entry in
> > the ControlList and then reserving memory for the control when getting
> > V4L2 controls from a device. This is already possible by calling the
> > get() function right after set(), but results in two lookups. Extend the
> > id-based set() function to return the reference to the inserted
> > ControlValue to avoid the second lookup.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/controls.h | 2 +-
> >  src/libcamera/controls.cpp   | 7 +++++--
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 80944efc133a..5a5cfc3e52ca 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -394,7 +394,7 @@ public:
> >  	}
> >
> >  	const ControlValue &get(unsigned int id) const;
> > -	void set(unsigned int id, const ControlValue &value);
> > +	ControlValue &set(unsigned int id, const ControlValue &value);
> >
> >  	const ControlInfoMap *infoMap() const { return infoMap_; }
> >
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 08df7f29e938..12822e87a4d7 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -930,14 +930,17 @@ const ControlValue &ControlList::get(unsigned int id) const
> >   *
> >   * The behaviour is undefined if the control \a id is not supported by the
> >   * object that the list refers to.
> > + *
> > + * \return A reference to the ControlValue stored in the control list
> >   */
> > -void ControlList::set(unsigned int id, const ControlValue &value)
> > +ControlValue &ControlList::set(unsigned int id, const ControlValue &value)
> >  {
> >  	ControlValue *val = find(id);
> >  	if (!val)
> > -		return;
> > +		return *val;
> 
> Wouldn't you dereference a nullptr ? Should we create a

I agree. I actually dreamt about libcamera last night and figured out
this issue in my sleep. I wonder how bad that is :-)

>         static ControlValue empty{};
> 
> at function begin and return that one ?

The set() function is documented as having an undefined behaviour if the
control isn't supported, so this is within the range of possible
undefined behaviours (so would rm -rf /*). It's not very nice though.
Returning a reference to a local static variable isn't nice either, as
the caller won't know that an error occurred. I'm leaning towards
returning a pointer, until we refactor this API (getting and setting
controls by numerical ID isn't nice in general). I'll see what I can do.

> Overall, I think I'm still missing the use case for this tbh, but

It's to avoid a second lookup in patch 2/2.

> we're not losing anything, as the previous return type was void, so
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >
> >  	*val = value;
> > +	return *val;
> >  }
> >
> >  /**

-- 
Regards,

Laurent Pinchart