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

[Jacopo Mondi]

Hi Laurent,

On Sun, Apr 26, 2020 at 08:30:52PM +0300, Laurent Pinchart wrote:
> 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.

Please be aware the same happens already in
        const ControlValue &ControlList::get(unsigned int id) const

>
> > 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