| Message ID | 20200425231623.6505-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Rejected |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, 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@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 static ControlValue empty{}; at function begin and return that one ? Overall, I think I'm still missing the use case for this tbh, but we're not losing anything, as the previous return type was void, so Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > *val = value; > + return *val; > } > > /** > -- > Regards, > > 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@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@jmondi.org> > > > > > *val = value; > > + return *val; > > } > > > > /**
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@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@jmondi.org> > > > > > > > > *val = value; > > > + return *val; > > > } > > > > > > /** > > -- > Regards, > > Laurent Pinchart
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; *val = value; + return *val; } /**
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@ideasonboard.com> --- include/libcamera/controls.h | 2 +- src/libcamera/controls.cpp | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-)