[libcamera-devel] [PATCH] libcamera: controls: Use template in the id-based interface
Jacopo Mondi
jacopo at jmondi.org
Thu Jan 2 09:10:38 CET 2020
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 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;
> > + }
> > +
> > + 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200102/7c227f92/attachment.sig>
More information about the libcamera-devel
mailing list