[libcamera-devel] [PATCH] libcamera: controls: Use template in the id-based interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jan 2 11:40:23 CET 2020
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 at 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;
> > > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list