| Message ID | 20191230155725.28691-1-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Mon, Dec 30, 2019 at 04:57:25PM +0100, Jacopo Mondi wrote: > The ControlList Control<> and id based interfaces to set or get controls > differ in the way they return or receive values. The Control<> based > interface allows the usage of raw values, while the id-based interface > always goes through ControlValue, resulting in one additional function > call in the caller when accessing controls by numerical id. > > Align the two implementations by providing access to raw values for the > id-based interface. > > - controls.get(V4L2_CID_CONTRAST).get<int32_t>() > + controls.get<int32_t>(V4L2_CID_CONTRAST) The commit message doesn't explain why this is desirable :-) This patch may make sense as part of a bigger set of changes, but in isolation I don't see anything wrong with accessing the ControlValue. > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/controls.h | 23 ++++++++++++++++++- > src/libcamera/controls.cpp | 37 +++++++++++++++++------------- > test/ipa/ipa_wrappers_test.cpp | 6 ++--- > test/v4l2_videodevice/controls.cpp | 12 +++++----- > 4 files changed, 52 insertions(+), 26 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index b1b73367e874..0b55359890e6 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -220,6 +220,18 @@ public: > return val->get<T>(); > } > > + template<typename T> > + const T &get(unsigned int id) const > + { > + const ControlValue *val = find(id); > + if (!val) { > + static T t(0); > + return t; > + } > + > + return val->get<T>(); > + } > + > template<typename T> > void set(const Control<T> &ctrl, const T &value) > { > @@ -230,7 +242,16 @@ public: > val->set<T>(value); > } > > - const ControlValue &get(unsigned int id) const; > + template<typename T> > + void set(unsigned int id, const T &value) > + { > + ControlValue *val = find(id); > + if (!val) > + return; > + > + val->set<T>(value); > + } > + > void 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 7d8a0e97ee3a..78729d55ee29 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -726,6 +726,18 @@ bool ControlList::contains(unsigned int id) const > * \return The control value > */ > > +/** > + * \fn template<typename T> const T &ControlList::get(unsigned int id) const > + * \brief Get the value of control \a id > + * \param[in] id The control numerical ID > + * > + * The behaviour is undefined if the control \a id is not present in the list. > + * Use ControlList::contains() to test for the presence of a control in the > + * list before retrieving its value. > + * > + * \return The control value > + */ > + > /** > * \fn template<typename T> void ControlList::set(const Control<T> &ctrl, const T &value) > * \brief Set the control \a ctrl value to \a value > @@ -741,25 +753,18 @@ bool ControlList::contains(unsigned int id) const > */ > > /** > - * \brief Get the value of control \a id > - * \param[in] id The control numerical ID > + * \fn template<typename T> void ControlList::set(unsigned int id, const T &value) > + * \brief Set the value of control \a id to \a value > + * \param[in] id The control ID > + * \param[in] value The control value > * > - * The behaviour is undefined if the control \a id is not present in the list. > - * Use ControlList::contains() to test for the presence of a control in the > - * list before retrieving its value. > + * This method sets the value of a control in the control list. If the control > + * is already present in the list, its value is updated, otherwise it is added > + * to the list. > * > - * \return The control value > + * The behaviour is undefined if the control \a id is not supported by the > + * object that the list refers to. > */ > -const ControlValue &ControlList::get(unsigned int id) const > -{ > - static ControlValue zero; > - > - const ControlValue *val = find(id); > - if (!val) > - return zero; > - > - return *val; > -} > > /** > * \brief Set the value of control \a id to \a value > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp > index a1e34ad52317..11fea2ccc506 100644 > --- a/test/ipa/ipa_wrappers_test.cpp > +++ b/test/ipa/ipa_wrappers_test.cpp > @@ -181,9 +181,9 @@ public: > } > > const ControlList &controls = data.controls[0]; > - if (controls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() != 10 || > - controls.get(V4L2_CID_CONTRAST).get<int32_t>() != 20 || > - controls.get(V4L2_CID_SATURATION).get<int32_t>() != 30) { > + if (controls.get<int32_t>(V4L2_CID_BRIGHTNESS) != 10 || > + controls.get<int32_t>(V4L2_CID_CONTRAST) != 20 || > + controls.get<int32_t>(V4L2_CID_SATURATION) != 30) { > cerr << "processEvent(): Invalid controls" << endl; > return report(Op_processEvent, TestFail); > } > diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp > index 42c653d4435a..e6ab1b4c11bc 100644 > --- a/test/v4l2_videodevice/controls.cpp > +++ b/test/v4l2_videodevice/controls.cpp > @@ -57,9 +57,9 @@ protected: > return TestFail; > } > > - if (ctrls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() == -1 || > - ctrls.get(V4L2_CID_CONTRAST).get<int32_t>() == -1 || > - ctrls.get(V4L2_CID_SATURATION).get<int32_t>() == -1) { > + if (ctrls.get<int32_t>(V4L2_CID_BRIGHTNESS) == -1 || > + ctrls.get<int32_t>(V4L2_CID_CONTRAST) == -1 || > + ctrls.get<int32_t>(V4L2_CID_SATURATION) == -1) { > cerr << "Incorrect value for retrieved controls" << endl; > return TestFail; > } > @@ -86,9 +86,9 @@ protected: > return TestFail; > } > > - if (ctrls.get(V4L2_CID_BRIGHTNESS) != brightness.min() || > - ctrls.get(V4L2_CID_CONTRAST) != contrast.max() || > - ctrls.get(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) { > + if (ctrls.get<int32_t>(V4L2_CID_BRIGHTNESS) != brightness.min().get<int32_t>() || > + ctrls.get<int32_t>(V4L2_CID_CONTRAST) != contrast.max().get<int32_t>() || > + ctrls.get<int32_t>(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) { > cerr << "Controls not updated when set" << endl; > return TestFail; > }
Hi Laurent, On Tue, Dec 31, 2019 at 02:32:50PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Dec 30, 2019 at 04:57:25PM +0100, Jacopo Mondi wrote: > > The ControlList Control<> and id based interfaces to set or get controls > > differ in the way they return or receive values. The Control<> based > > interface allows the usage of raw values, while the id-based interface > > always goes through ControlValue, resulting in one additional function > > call in the caller when accessing controls by numerical id. > > > > Align the two implementations by providing access to raw values for the > > id-based interface. > > > > - controls.get(V4L2_CID_CONTRAST).get<int32_t>() > > + controls.get<int32_t>(V4L2_CID_CONTRAST) > > The commit message doesn't explain why this is desirable :-) This patch > may make sense as part of a bigger set of changes, but in isolation I > don't see anything wrong with accessing the ControlValue. > Well, "The ControlList Control<> and id based interfaces to set or get controls differ in the way they return or receive values" explains that the API differs based on the usage of Control or numerical id and to me controls.get<int32_t>(V4L2_CID_SATURATION) is not only nicer to type than controls.get(V4L2_CID_SATURATION).get<int32_t>() but it's also consistent with controls.get<int32_t>(Controls::Brightness) I'll resend as part of a longer series if it makes more sense, but to me this discrepancy in the interface is worth being changed considering all the effort we put in having a single ControlList for libcamera and v4l2 controls. Thanks j > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/controls.h | 23 ++++++++++++++++++- > > src/libcamera/controls.cpp | 37 +++++++++++++++++------------- > > test/ipa/ipa_wrappers_test.cpp | 6 ++--- > > test/v4l2_videodevice/controls.cpp | 12 +++++----- > > 4 files changed, 52 insertions(+), 26 deletions(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index b1b73367e874..0b55359890e6 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -220,6 +220,18 @@ public: > > return val->get<T>(); > > } > > > > + template<typename T> > > + const T &get(unsigned int id) const > > + { > > + const ControlValue *val = find(id); > > + if (!val) { > > + static T t(0); > > + return t; > > + } > > + > > + return val->get<T>(); > > + } > > + > > template<typename T> > > void set(const Control<T> &ctrl, const T &value) > > { > > @@ -230,7 +242,16 @@ public: > > val->set<T>(value); > > } > > > > - const ControlValue &get(unsigned int id) const; > > + template<typename T> > > + void set(unsigned int id, const T &value) > > + { > > + ControlValue *val = find(id); > > + if (!val) > > + return; > > + > > + val->set<T>(value); > > + } > > + > > void 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 7d8a0e97ee3a..78729d55ee29 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -726,6 +726,18 @@ bool ControlList::contains(unsigned int id) const > > * \return The control value > > */ > > > > +/** > > + * \fn template<typename T> const T &ControlList::get(unsigned int id) const > > + * \brief Get the value of control \a id > > + * \param[in] id The control numerical ID > > + * > > + * The behaviour is undefined if the control \a id is not present in the list. > > + * Use ControlList::contains() to test for the presence of a control in the > > + * list before retrieving its value. > > + * > > + * \return The control value > > + */ > > + > > /** > > * \fn template<typename T> void ControlList::set(const Control<T> &ctrl, const T &value) > > * \brief Set the control \a ctrl value to \a value > > @@ -741,25 +753,18 @@ bool ControlList::contains(unsigned int id) const > > */ > > > > /** > > - * \brief Get the value of control \a id > > - * \param[in] id The control numerical ID > > + * \fn template<typename T> void ControlList::set(unsigned int id, const T &value) > > + * \brief Set the value of control \a id to \a value > > + * \param[in] id The control ID > > + * \param[in] value The control value > > * > > - * The behaviour is undefined if the control \a id is not present in the list. > > - * Use ControlList::contains() to test for the presence of a control in the > > - * list before retrieving its value. > > + * This method sets the value of a control in the control list. If the control > > + * is already present in the list, its value is updated, otherwise it is added > > + * to the list. > > * > > - * \return The control value > > + * The behaviour is undefined if the control \a id is not supported by the > > + * object that the list refers to. > > */ > > -const ControlValue &ControlList::get(unsigned int id) const > > -{ > > - static ControlValue zero; > > - > > - const ControlValue *val = find(id); > > - if (!val) > > - return zero; > > - > > - return *val; > > -} > > > > /** > > * \brief Set the value of control \a id to \a value > > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp > > index a1e34ad52317..11fea2ccc506 100644 > > --- a/test/ipa/ipa_wrappers_test.cpp > > +++ b/test/ipa/ipa_wrappers_test.cpp > > @@ -181,9 +181,9 @@ public: > > } > > > > const ControlList &controls = data.controls[0]; > > - if (controls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() != 10 || > > - controls.get(V4L2_CID_CONTRAST).get<int32_t>() != 20 || > > - controls.get(V4L2_CID_SATURATION).get<int32_t>() != 30) { > > + if (controls.get<int32_t>(V4L2_CID_BRIGHTNESS) != 10 || > > + controls.get<int32_t>(V4L2_CID_CONTRAST) != 20 || > > + controls.get<int32_t>(V4L2_CID_SATURATION) != 30) { > > cerr << "processEvent(): Invalid controls" << endl; > > return report(Op_processEvent, TestFail); > > } > > diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp > > index 42c653d4435a..e6ab1b4c11bc 100644 > > --- a/test/v4l2_videodevice/controls.cpp > > +++ b/test/v4l2_videodevice/controls.cpp > > @@ -57,9 +57,9 @@ protected: > > return TestFail; > > } > > > > - if (ctrls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() == -1 || > > - ctrls.get(V4L2_CID_CONTRAST).get<int32_t>() == -1 || > > - ctrls.get(V4L2_CID_SATURATION).get<int32_t>() == -1) { > > + if (ctrls.get<int32_t>(V4L2_CID_BRIGHTNESS) == -1 || > > + ctrls.get<int32_t>(V4L2_CID_CONTRAST) == -1 || > > + ctrls.get<int32_t>(V4L2_CID_SATURATION) == -1) { > > cerr << "Incorrect value for retrieved controls" << endl; > > return TestFail; > > } > > @@ -86,9 +86,9 @@ protected: > > return TestFail; > > } > > > > - if (ctrls.get(V4L2_CID_BRIGHTNESS) != brightness.min() || > > - ctrls.get(V4L2_CID_CONTRAST) != contrast.max() || > > - ctrls.get(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) { > > + if (ctrls.get<int32_t>(V4L2_CID_BRIGHTNESS) != brightness.min().get<int32_t>() || > > + ctrls.get<int32_t>(V4L2_CID_CONTRAST) != contrast.max().get<int32_t>() || > > + ctrls.get<int32_t>(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) { > > cerr << "Controls not updated when set" << endl; > > return TestFail; > > } > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Jan 02, 2020 at 09:10:38AM +0100, Jacopo Mondi wrote: > On Tue, Dec 31, 2019 at 02:32:50PM +0200, Laurent Pinchart wrote: > > On Mon, Dec 30, 2019 at 04:57:25PM +0100, Jacopo Mondi wrote: > > > The ControlList Control<> and id based interfaces to set or get controls > > > differ in the way they return or receive values. The Control<> based > > > interface allows the usage of raw values, while the id-based interface > > > always goes through ControlValue, resulting in one additional function > > > call in the caller when accessing controls by numerical id. > > > > > > Align the two implementations by providing access to raw values for the > > > id-based interface. > > > > > > - controls.get(V4L2_CID_CONTRAST).get<int32_t>() > > > + controls.get<int32_t>(V4L2_CID_CONTRAST) > > > > The commit message doesn't explain why this is desirable :-) This patch > > may make sense as part of a bigger set of changes, but in isolation I > > don't see anything wrong with accessing the ControlValue. > > Well, "The ControlList Control<> and id based interfaces to set or get controls > differ in the way they return or receive values" explains that the API > differs based on the usage of Control or numerical id and to me > controls.get<int32_t>(V4L2_CID_SATURATION) is not only nicer to type than > controls.get(V4L2_CID_SATURATION).get<int32_t>() but it's also > consistent with controls.get<int32_t>(Controls::Brightness) > > I'll resend as part of a longer series if it makes more sense, but to > me this discrepancy in the interface is worth being changed > considering all the effort we put in having a single ControlList for > libcamera and v4l2 controls. Note that the interface based on numerical IDs shouldn't be used by applications. I'm not necessarily opposed to this change, but I would like to assess its implications in a broader context. Please see below for two additional comments. > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/controls.h | 23 ++++++++++++++++++- > > > src/libcamera/controls.cpp | 37 +++++++++++++++++------------- > > > test/ipa/ipa_wrappers_test.cpp | 6 ++--- > > > test/v4l2_videodevice/controls.cpp | 12 +++++----- > > > 4 files changed, 52 insertions(+), 26 deletions(-) > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > index b1b73367e874..0b55359890e6 100644 > > > --- a/include/libcamera/controls.h > > > +++ b/include/libcamera/controls.h > > > @@ -220,6 +220,18 @@ public: > > > return val->get<T>(); > > > } > > > > > > + template<typename T> > > > + const T &get(unsigned int id) const > > > + { > > > + const ControlValue *val = find(id); > > > + if (!val) { > > > + static T t(0); > > > + return t; This is the part that bothers me the most in our template interface, and I wish we could solve it differently instead of duplicating it :-) > > > + } > > > + > > > + return val->get<T>(); > > > + } > > > + > > > template<typename T> > > > void set(const Control<T> &ctrl, const T &value) > > > { > > > @@ -230,7 +242,16 @@ public: > > > val->set<T>(value); > > > } > > > > > > - const ControlValue &get(unsigned int id) const; > > > + template<typename T> > > > + void set(unsigned int id, const T &value) > > > + { > > > + ControlValue *val = find(id); > > > + if (!val) > > > + return; > > > + > > > + val->set<T>(value); > > > + } > > > + > > > void set(unsigned int id, const ControlValue &value); Should this be removed too ? > > > > > > const ControlInfoMap *infoMap() const { return infoMap_; } > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index 7d8a0e97ee3a..78729d55ee29 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -726,6 +726,18 @@ bool ControlList::contains(unsigned int id) const > > > * \return The control value > > > */ > > > > > > +/** > > > + * \fn template<typename T> const T &ControlList::get(unsigned int id) const > > > + * \brief Get the value of control \a id > > > + * \param[in] id The control numerical ID > > > + * > > > + * The behaviour is undefined if the control \a id is not present in the list. > > > + * Use ControlList::contains() to test for the presence of a control in the > > > + * list before retrieving its value. > > > + * > > > + * \return The control value > > > + */ > > > + > > > /** > > > * \fn template<typename T> void ControlList::set(const Control<T> &ctrl, const T &value) > > > * \brief Set the control \a ctrl value to \a value > > > @@ -741,25 +753,18 @@ bool ControlList::contains(unsigned int id) const > > > */ > > > > > > /** > > > - * \brief Get the value of control \a id > > > - * \param[in] id The control numerical ID > > > + * \fn template<typename T> void ControlList::set(unsigned int id, const T &value) > > > + * \brief Set the value of control \a id to \a value > > > + * \param[in] id The control ID > > > + * \param[in] value The control value > > > * > > > - * The behaviour is undefined if the control \a id is not present in the list. > > > - * Use ControlList::contains() to test for the presence of a control in the > > > - * list before retrieving its value. > > > + * This method sets the value of a control in the control list. If the control > > > + * is already present in the list, its value is updated, otherwise it is added > > > + * to the list. > > > * > > > - * \return The control value > > > + * The behaviour is undefined if the control \a id is not supported by the > > > + * object that the list refers to. > > > */ > > > -const ControlValue &ControlList::get(unsigned int id) const > > > -{ > > > - static ControlValue zero; > > > - > > > - const ControlValue *val = find(id); > > > - if (!val) > > > - return zero; > > > - > > > - return *val; > > > -} > > > > > > /** > > > * \brief Set the value of control \a id to \a value > > > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp > > > index a1e34ad52317..11fea2ccc506 100644 > > > --- a/test/ipa/ipa_wrappers_test.cpp > > > +++ b/test/ipa/ipa_wrappers_test.cpp > > > @@ -181,9 +181,9 @@ public: > > > } > > > > > > const ControlList &controls = data.controls[0]; > > > - if (controls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() != 10 || > > > - controls.get(V4L2_CID_CONTRAST).get<int32_t>() != 20 || > > > - controls.get(V4L2_CID_SATURATION).get<int32_t>() != 30) { > > > + if (controls.get<int32_t>(V4L2_CID_BRIGHTNESS) != 10 || > > > + controls.get<int32_t>(V4L2_CID_CONTRAST) != 20 || > > > + controls.get<int32_t>(V4L2_CID_SATURATION) != 30) { > > > cerr << "processEvent(): Invalid controls" << endl; > > > return report(Op_processEvent, TestFail); > > > } > > > diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp > > > index 42c653d4435a..e6ab1b4c11bc 100644 > > > --- a/test/v4l2_videodevice/controls.cpp > > > +++ b/test/v4l2_videodevice/controls.cpp > > > @@ -57,9 +57,9 @@ protected: > > > return TestFail; > > > } > > > > > > - if (ctrls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() == -1 || > > > - ctrls.get(V4L2_CID_CONTRAST).get<int32_t>() == -1 || > > > - ctrls.get(V4L2_CID_SATURATION).get<int32_t>() == -1) { > > > + if (ctrls.get<int32_t>(V4L2_CID_BRIGHTNESS) == -1 || > > > + ctrls.get<int32_t>(V4L2_CID_CONTRAST) == -1 || > > > + ctrls.get<int32_t>(V4L2_CID_SATURATION) == -1) { > > > cerr << "Incorrect value for retrieved controls" << endl; > > > return TestFail; > > > } > > > @@ -86,9 +86,9 @@ protected: > > > return TestFail; > > > } > > > > > > - if (ctrls.get(V4L2_CID_BRIGHTNESS) != brightness.min() || > > > - ctrls.get(V4L2_CID_CONTRAST) != contrast.max() || > > > - ctrls.get(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) { > > > + if (ctrls.get<int32_t>(V4L2_CID_BRIGHTNESS) != brightness.min().get<int32_t>() || > > > + ctrls.get<int32_t>(V4L2_CID_CONTRAST) != contrast.max().get<int32_t>() || > > > + ctrls.get<int32_t>(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) { > > > cerr << "Controls not updated when set" << endl; > > > return TestFail; > > > }
Hi Jacopo, On Thu, Jan 02, 2020 at 12:04:42PM +0100, Jacopo Mondi wrote: > On Thu, Jan 02, 2020 at 12:40:23PM +0200, Laurent Pinchart wrote: > > On Thu, Jan 02, 2020 at 09:10:38AM +0100, Jacopo Mondi wrote: > >> On Tue, Dec 31, 2019 at 02:32:50PM +0200, Laurent Pinchart wrote: > >>> On Mon, Dec 30, 2019 at 04:57:25PM +0100, Jacopo Mondi wrote: > >>>> The ControlList Control<> and id based interfaces to set or get controls > >>>> differ in the way they return or receive values. The Control<> based > >>>> interface allows the usage of raw values, while the id-based interface > >>>> always goes through ControlValue, resulting in one additional function > >>>> call in the caller when accessing controls by numerical id. > >>>> > >>>> Align the two implementations by providing access to raw values for the > >>>> id-based interface. > >>>> > >>>> - controls.get(V4L2_CID_CONTRAST).get<int32_t>() > >>>> + controls.get<int32_t>(V4L2_CID_CONTRAST) > >>> > >>> The commit message doesn't explain why this is desirable :-) This patch > >>> may make sense as part of a bigger set of changes, but in isolation I > >>> don't see anything wrong with accessing the ControlValue. > >> > >> Well, "The ControlList Control<> and id based interfaces to set or get controls > >> differ in the way they return or receive values" explains that the API > >> differs based on the usage of Control or numerical id and to me > >> controls.get<int32_t>(V4L2_CID_SATURATION) is not only nicer to type than > >> controls.get(V4L2_CID_SATURATION).get<int32_t>() but it's also > >> consistent with controls.get<int32_t>(Controls::Brightness) > >> > >> I'll resend as part of a longer series if it makes more sense, but to > >> me this discrepancy in the interface is worth being changed > >> considering all the effort we put in having a single ControlList for > >> libcamera and v4l2 controls. > > > > Note that the interface based on numerical IDs shouldn't be used by > > applications. I'm not necessarily opposed to this change, but I would > > like to assess its implications in a broader context. Please see below > > for two additional comments. > > > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>> --- > >>>> include/libcamera/controls.h | 23 ++++++++++++++++++- > >>>> src/libcamera/controls.cpp | 37 +++++++++++++++++------------- > >>>> test/ipa/ipa_wrappers_test.cpp | 6 ++--- > >>>> test/v4l2_videodevice/controls.cpp | 12 +++++----- > >>>> 4 files changed, 52 insertions(+), 26 deletions(-) > >>>> > >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > >>>> index b1b73367e874..0b55359890e6 100644 > >>>> --- a/include/libcamera/controls.h > >>>> +++ b/include/libcamera/controls.h > >>>> @@ -220,6 +220,18 @@ public: > >>>> return val->get<T>(); > >>>> } > >>>> > >>>> + template<typename T> > >>>> + const T &get(unsigned int id) const > >>>> + { > >>>> + const ControlValue *val = find(id); > >>>> + if (!val) { > >>>> + static T t(0); > >>>> + return t; > > > > This is the part that bothers me the most in our template interface, and > > I wish we could solve it differently instead of duplicating it :-) > > Why is this worse than: > > >>>> -const ControlValue &ControlList::get(unsigned int id) const > >>>> -{ > >>>> - static ControlValue zero; > >>>> - > >>>> - const ControlValue *val = find(id); > >>>> - if (!val) > >>>> - return zero; > >>>> - > >>>> - return *val; > >>>> -} It's not, and that's my point, that's the part that bothers me the most in our current interface. Your patch doesn't make it worse.
Hi Laurent On Thu, Jan 02, 2020 at 12:40:23PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Thu, Jan 02, 2020 at 09:10:38AM +0100, Jacopo Mondi wrote: > > On Tue, Dec 31, 2019 at 02:32:50PM +0200, Laurent Pinchart wrote: > > > On Mon, Dec 30, 2019 at 04:57:25PM +0100, Jacopo Mondi wrote: > > > > The ControlList Control<> and id based interfaces to set or get controls > > > > differ in the way they return or receive values. The Control<> based > > > > interface allows the usage of raw values, while the id-based interface > > > > always goes through ControlValue, resulting in one additional function > > > > call in the caller when accessing controls by numerical id. > > > > > > > > Align the two implementations by providing access to raw values for the > > > > id-based interface. > > > > > > > > - controls.get(V4L2_CID_CONTRAST).get<int32_t>() > > > > + controls.get<int32_t>(V4L2_CID_CONTRAST) > > > > > > The commit message doesn't explain why this is desirable :-) This patch > > > may make sense as part of a bigger set of changes, but in isolation I > > > don't see anything wrong with accessing the ControlValue. > > > > Well, "The ControlList Control<> and id based interfaces to set or get controls > > differ in the way they return or receive values" explains that the API > > differs based on the usage of Control or numerical id and to me > > controls.get<int32_t>(V4L2_CID_SATURATION) is not only nicer to type than > > controls.get(V4L2_CID_SATURATION).get<int32_t>() but it's also > > consistent with controls.get<int32_t>(Controls::Brightness) > > > > I'll resend as part of a longer series if it makes more sense, but to > > me this discrepancy in the interface is worth being changed > > considering all the effort we put in having a single ControlList for > > libcamera and v4l2 controls. > > Note that the interface based on numerical IDs shouldn't be used by > applications. I'm not necessarily opposed to this change, but I would > like to assess its implications in a broader context. Please see below > for two additional comments. > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > include/libcamera/controls.h | 23 ++++++++++++++++++- > > > > src/libcamera/controls.cpp | 37 +++++++++++++++++------------- > > > > test/ipa/ipa_wrappers_test.cpp | 6 ++--- > > > > test/v4l2_videodevice/controls.cpp | 12 +++++----- > > > > 4 files changed, 52 insertions(+), 26 deletions(-) > > > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > > index b1b73367e874..0b55359890e6 100644 > > > > --- a/include/libcamera/controls.h > > > > +++ b/include/libcamera/controls.h > > > > @@ -220,6 +220,18 @@ public: > > > > return val->get<T>(); > > > > } > > > > > > > > + template<typename T> > > > > + const T &get(unsigned int id) const > > > > + { > > > > + const ControlValue *val = find(id); > > > > + if (!val) { > > > > + static T t(0); > > > > + return t; > > This is the part that bothers me the most in our template interface, and > I wish we could solve it differently instead of duplicating it :-) > Why is this worse than: > > > > -const ControlValue &ControlList::get(unsigned int id) const > > > > -{ > > > > - static ControlValue zero; > > > > - > > > > - const ControlValue *val = find(id); > > > > - if (!val) > > > > - return zero; > > > > - > > > > - return *val; > > > > -} Thanks j
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index b1b73367e874..0b55359890e6 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -220,6 +220,18 @@ public: return val->get<T>(); } + template<typename T> + const T &get(unsigned int id) const + { + const ControlValue *val = find(id); + if (!val) { + static T t(0); + return t; + } + + return val->get<T>(); + } + template<typename T> void set(const Control<T> &ctrl, const T &value) { @@ -230,7 +242,16 @@ public: val->set<T>(value); } - const ControlValue &get(unsigned int id) const; + template<typename T> + void set(unsigned int id, const T &value) + { + ControlValue *val = find(id); + if (!val) + return; + + val->set<T>(value); + } + void 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 7d8a0e97ee3a..78729d55ee29 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -726,6 +726,18 @@ bool ControlList::contains(unsigned int id) const * \return The control value */ +/** + * \fn template<typename T> const T &ControlList::get(unsigned int id) const + * \brief Get the value of control \a id + * \param[in] id The control numerical ID + * + * The behaviour is undefined if the control \a id is not present in the list. + * Use ControlList::contains() to test for the presence of a control in the + * list before retrieving its value. + * + * \return The control value + */ + /** * \fn template<typename T> void ControlList::set(const Control<T> &ctrl, const T &value) * \brief Set the control \a ctrl value to \a value @@ -741,25 +753,18 @@ bool ControlList::contains(unsigned int id) const */ /** - * \brief Get the value of control \a id - * \param[in] id The control numerical ID + * \fn template<typename T> void ControlList::set(unsigned int id, const T &value) + * \brief Set the value of control \a id to \a value + * \param[in] id The control ID + * \param[in] value The control value * - * The behaviour is undefined if the control \a id is not present in the list. - * Use ControlList::contains() to test for the presence of a control in the - * list before retrieving its value. + * This method sets the value of a control in the control list. If the control + * is already present in the list, its value is updated, otherwise it is added + * to the list. * - * \return The control value + * The behaviour is undefined if the control \a id is not supported by the + * object that the list refers to. */ -const ControlValue &ControlList::get(unsigned int id) const -{ - static ControlValue zero; - - const ControlValue *val = find(id); - if (!val) - return zero; - - return *val; -} /** * \brief Set the value of control \a id to \a value diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp index a1e34ad52317..11fea2ccc506 100644 --- a/test/ipa/ipa_wrappers_test.cpp +++ b/test/ipa/ipa_wrappers_test.cpp @@ -181,9 +181,9 @@ public: } const ControlList &controls = data.controls[0]; - if (controls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() != 10 || - controls.get(V4L2_CID_CONTRAST).get<int32_t>() != 20 || - controls.get(V4L2_CID_SATURATION).get<int32_t>() != 30) { + if (controls.get<int32_t>(V4L2_CID_BRIGHTNESS) != 10 || + controls.get<int32_t>(V4L2_CID_CONTRAST) != 20 || + controls.get<int32_t>(V4L2_CID_SATURATION) != 30) { cerr << "processEvent(): Invalid controls" << endl; return report(Op_processEvent, TestFail); } diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp index 42c653d4435a..e6ab1b4c11bc 100644 --- a/test/v4l2_videodevice/controls.cpp +++ b/test/v4l2_videodevice/controls.cpp @@ -57,9 +57,9 @@ protected: return TestFail; } - if (ctrls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() == -1 || - ctrls.get(V4L2_CID_CONTRAST).get<int32_t>() == -1 || - ctrls.get(V4L2_CID_SATURATION).get<int32_t>() == -1) { + if (ctrls.get<int32_t>(V4L2_CID_BRIGHTNESS) == -1 || + ctrls.get<int32_t>(V4L2_CID_CONTRAST) == -1 || + ctrls.get<int32_t>(V4L2_CID_SATURATION) == -1) { cerr << "Incorrect value for retrieved controls" << endl; return TestFail; } @@ -86,9 +86,9 @@ protected: return TestFail; } - if (ctrls.get(V4L2_CID_BRIGHTNESS) != brightness.min() || - ctrls.get(V4L2_CID_CONTRAST) != contrast.max() || - ctrls.get(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) { + if (ctrls.get<int32_t>(V4L2_CID_BRIGHTNESS) != brightness.min().get<int32_t>() || + ctrls.get<int32_t>(V4L2_CID_CONTRAST) != contrast.max().get<int32_t>() || + ctrls.get<int32_t>(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) { cerr << "Controls not updated when set" << endl; return TestFail; }
The ControlList Control<> and id based interfaces to set or get controls differ in the way they return or receive values. The Control<> based interface allows the usage of raw values, while the id-based interface always goes through ControlValue, resulting in one additional function call in the caller when accessing controls by numerical id. Align the two implementations by providing access to raw values for the id-based interface. - controls.get(V4L2_CID_CONTRAST).get<int32_t>() + controls.get<int32_t>(V4L2_CID_CONTRAST) Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/controls.h | 23 ++++++++++++++++++- src/libcamera/controls.cpp | 37 +++++++++++++++++------------- test/ipa/ipa_wrappers_test.cpp | 6 ++--- test/v4l2_videodevice/controls.cpp | 12 +++++----- 4 files changed, 52 insertions(+), 26 deletions(-)