[libcamera-devel] [PATCH v2 07/14] libcamera: controls: Support accessing controls by numerical ID
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Oct 13 16:05:18 CEST 2019
Hi Jacopo,
On Sun, Oct 13, 2019 at 03:50:04PM +0200, Jacopo Mondi wrote:
> On Sat, Oct 12, 2019 at 09:44:00PM +0300, Laurent Pinchart wrote:
> > The ControlList class has template get() and set() methods to get and
> > set control values. The methods require a reference to a Control
> > instance, which is only available when calling them with a hardcoded
> > control. In order to support usage of ControlList for V4L2 controls, as
> > well as serialisation and deserialisation of ControlList, we need a way
> > to get and set control values based on a control numerical ID. Add new
> > contains(), get() and set() overload methods to do so.
> >
> > As this change prepares the ControlList to be used for other objects
> > than camera, update its documentation accordingly.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/controls.h | 10 ++-
> > src/ipa/rkisp1/rkisp1.cpp | 2 +-
> > src/libcamera/controls.cpp | 143 ++++++++++++++++++++++++++-------
> > src/libcamera/request.cpp | 5 +-
> > test/controls/control_list.cpp | 2 +-
> > 5 files changed, 125 insertions(+), 37 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 999fcf7a3a62..5e6708fe570b 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -126,7 +126,7 @@ private:
> > using ControlListMap = std::unordered_map<const ControlId *, ControlValue>;
> >
> > public:
> > - ControlList(ControlValidator *validator = nullptr);
> > + ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);
> >
> > using iterator = ControlListMap::iterator;
> > using const_iterator = ControlListMap::const_iterator;
> > @@ -136,11 +136,13 @@ public:
> > const_iterator begin() const { return controls_.begin(); }
> > const_iterator end() const { return controls_.end(); }
> >
> > - bool contains(const ControlId &id) const;
> > bool empty() const { return controls_.empty(); }
> > std::size_t size() const { return controls_.size(); }
> > void clear() { controls_.clear(); }
> >
> > + bool contains(const ControlId &id) const;
> > + bool contains(unsigned int id) const;
> > +
> > template<typename T>
> > const T &get(const Control<T> &ctrl) const
> > {
> > @@ -163,11 +165,15 @@ public:
> > val->set<T>(value);
> > }
> >
> > + const ControlValue &get(unsigned int id) const;
> > + void set(unsigned int id, const ControlValue &value);
> > +
> > private:
> > const ControlValue *find(const ControlId &id) const;
> > ControlValue *find(const ControlId &id);
> >
> > ControlValidator *validator_;
> > + const ControlIdMap *idmap_;
> > ControlListMap controls_;
> > };
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 80138f196184..b0d23dd154be 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -220,7 +220,7 @@ void IPARkISP1::setControls(unsigned int frame)
> >
> > void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> > {
> > - ControlList ctrls;
> > + ControlList ctrls(controls::controls);
>
> Just to note down that providing a default argument to the ControList
> contructor would shorten this. But I know you're in favour of having
> controls:controls explicitly passed.
Yes, I think it's best to get the callers to think about it and pass the
idmap explicitly. If applications were to create ControlList instances
themselves I could reconsider this (but possibly through a different
solution), but as that's not the case, I prefer keeping it explicit.
> >
> > if (aeState)
> > ctrls.set(controls::AeLocked, aeState == 2);
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 292e48cd6d25..ddd4e6680ce2 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -7,6 +7,7 @@
> >
> > #include <libcamera/controls.h>
> >
> > +#include <iomanip>
> > #include <sstream>
> > #include <string>
> >
> > @@ -16,13 +17,13 @@
> >
> > /**
> > * \file controls.h
> > - * \brief Describes control framework and controls supported by a camera
> > + * \brief Framework to handle manage controls related to an object
>
> "to handle -and- manage"
> Or just drop one of the two
Oops, fixed.
> > *
> > - * A control is a mean to govern or influence the operation of a camera. Every
> > - * control is defined by a unique numerical ID, a name string and the data type
> > - * of the value it stores. The libcamera API defines a set of standard controls
> > - * in the libcamera::controls namespace, as a set of instances of the Control
> > - * class.
> > + * A control is a mean to govern or influence the operation of an object, and in
> > + * particular of a camera. Every control is defined by a unique numerical ID, a
> > + * name string and the data type of the value it stores. The libcamera API
> > + * defines a set of standard controls in the libcamera::controls namespace, as
> > + * a set of instances of the Control class.
> > *
> > * The main way for applications to interact with controls is through the
> > * ControlList stored in the Request class:
> > @@ -274,7 +275,7 @@ bool ControlValue::operator==(const ControlValue &other) const
> > * \class Control
> > * \brief Describe a control and its intrinsic properties
> > *
> > - * The Control class models a control exposed by a camera. Its template type
> > + * The Control class models a control exposed by an object. Its template type
> > * name T refers to the control data type, and allows methods that operate on
> > * control values to be defined as template methods using the same type T for
> > * the control value. See for instance how the ControlList::get() method
> > @@ -293,8 +294,8 @@ bool ControlValue::operator==(const ControlValue &other) const
> > * long int).
> > *
> > * Controls IDs shall be unique. While nothing prevents multiple instances of
> > - * the Control class to be created with the same ID, this may lead to undefined
> > - * behaviour.
> > + * the Control class to be created with the same ID for the same object, doing
> > + * so may cause undefined behaviour.
> > */
> >
> > /**
> > @@ -398,18 +399,28 @@ std::string ControlRange::toString() const
> >
> > /**
> > * \class ControlList
> > - * \brief Associate a list of ControlId with their values for a camera
> > + * \brief Associate a list of ControlId with their values for an object
> > *
> > - * A ControlList wraps a map of ControlId to ControlValue and optionally
> > - * validates controls against a ControlValidator.
> > + * The ControlList class stores values of controls exposed by an object. The
> > + * lists returned by the Request::controls() and Request::metadata() methods
> > + * refer to the camera that the request belongs to.
>
> Do we want this ? It leaves V4L2 controls a bit out of the picture
I did it on purpose, as ControlList is exposed to applications, and V4L2
controls are not. I didn't want to mention V4L2 anywhere in this file,
while still keeping it generic. That's why I replaced instances of
"camera" with "object", and added an explicit note that the two
ControlList instances visible to applications are related to a camera.
> > + *
> > + * Control lists are constructed with a map of all the controls supported by
> > + * their object, and an optional ControlValidator to further validate the
>
> I would drop "by their object"
"with a map of all the controls supported" sounds to me that it's
missing something. Supported by what ? :-)
> > + * controls.
> > */
> >
> > /**
> > * \brief Construct a ControlList with an optional control validator
> > + * \param[in] idmap The ControlId map for the control list target object
> > * \param[in] validator The validator (may be null)
> > + *
> > + * For ControlList containing libcamera controls, a global map of all libcamera
> > + * controls is provided by controls::controls and can be used as the \a idmap
> > + * argument.
> > */
> > -ControlList::ControlList(ControlValidator *validator)
> > - : validator_(validator)
> > +ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)
> > + : validator_(validator), idmap_(&idmap)
> > {
> > }
> >
> > @@ -449,20 +460,6 @@ ControlList::ControlList(ControlValidator *validator)
> > * list
> > */
> >
> > -/**
> > - * \brief Check if the list contains a control with the specified \a id
> > - * \param[in] id The control ID
> > - *
> > - * The behaviour is undefined if the control \a id is not supported by the
> > - * camera that the ControlList refers to.
> > - *
> > - * \return True if the list contains a matching control, false otherwise
> > - */
> > -bool ControlList::contains(const ControlId &id) const
> > -{
> > - return controls_.find(&id) != controls_.end();
> > -}
> > -
> > /**
> > * \fn ControlList::empty()
> > * \brief Identify if the list is empty
> > @@ -481,7 +478,33 @@ bool ControlList::contains(const ControlId &id) const
> > */
> >
> > /**
> > - * \fn template<typename T> const T &ControlList::get() const
> > + * \brief Check if the list contains a control with the specified \a id
> > + * \param[in] id The control ID
> > + *
> > + * \return True if the list contains a matching control, false otherwise
> > + */
> > +bool ControlList::contains(const ControlId &id) const
> > +{
> > + return controls_.find(&id) != controls_.end();
> > +}
> > +
> > +/**
> > + * \brief Check if the list contains a control with the specified \a id
>
> The documentation of these two methods is the same. I assume it's ok,
> but I would mentioned "numeric id" in the second version.
Isn't it already specified in the param ?
> > + * \param[in] id The control numerical ID
> > + *
> > + * \return True if the list contains a matching control, false otherwise
> > + */
> > +bool ControlList::contains(unsigned int id) const
> > +{
> > + const auto iter = idmap_->find(id);
> > + if (iter == idmap_->end())
> > + return false;
> > +
> > + return contains(*iter->second);
> > +}
> > +
> > +/**
> > + * \fn template<typename T> const T &ControlList::get(const Control<T> &ctrl) const
> > * \brief Get the value of a control
> > * \param[in] ctrl The control
> > *
> > @@ -496,7 +519,7 @@ bool ControlList::contains(const ControlId &id) const
> > */
> >
> > /**
> > - * \fn template<typename T> void ControlList::set()
> > + * \fn template<typename T> void ControlList::set(const Control<T> &ctrl, const T &value)
> > * \brief Set the control value to \a value
> > * \param[in] ctrl The control
> > * \param[in] value The control value
> > @@ -506,9 +529,67 @@ bool ControlList::contains(const ControlId &id) const
> > * to the list.
> > *
> > * The behaviour is undefined if the control \a ctrl is not supported by the
> > - * camera that the list refers to.
> > + * object that the list refers to.
> > */
> >
> > +/**
> > + * \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
> > + */
> > +const ControlValue &ControlList::get(unsigned int id) const
>
> This, as the ControlId * oriented version, is very easy to miss if the
> control is not present. Not an issue with this patch, anyway.
>
> The pattern would be like
>
> const ControlValue &ctrl = list.get(ID);
> if (ctrl.isNone()) {
> ...
> }
>
> Should we document it?
No, we shouldn't, as that's incorrect code. The documentation of the
method clearly specifies that the behaiour is unspecified if the control
is not present in the list. We don't want to document a way to rely on
unspecified behaviour.
> > +{
> > + static ControlValue zero;
> > +
> > + const auto ctrl = idmap_->find(id);
> > + if (ctrl == idmap_->end()) {
> > + LOG(Controls, Error)
> > + << std::hex << std::setfill('0')
> > + << "Control 0x" << std::setw(8) << id << " is not valid";
> > + return zero;
> > + }
> > +
> > + const ControlValue *val = find(*ctrl->second);
> > + if (!val)
> > + return zero;
> > +
> > + return *val;
> > +}
> > +
> > +/**
> > + * \brief Set the value of control \a id to \a value
> > + * \param[in] id The control ID
> > + * \param[in] value The control 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.
> > + *
> > + * The behaviour is undefined if the control \a id is not supported by the
> > + * object that the list refers to.
> > + */
> > +void ControlList::set(unsigned int id, const ControlValue &value)
> > +{
> > + const auto ctrl = idmap_->find(id);
> > + if (ctrl == idmap_->end()) {
> > + LOG(Controls, Error)
> > + << std::hex << std::setfill('0')
> > + << "Control 0x" << std::setw(8) << id << " is not valid";
>
> Here and in the get operation "is not valid" or "is not supported" ?
"is not supported" seems better, I'll switch to it.
> Anyway, all minor issues, so please add
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thank you.
> > + return;
> > + }
> > +
> > + ControlValue *val = find(*ctrl->second);
> > + if (!val)
> > + return;
> > +
> > + *val = value;
> > +}
> > +
> > const ControlValue *ControlList::find(const ControlId &id) const
> > {
> > const auto iter = controls_.find(&id);
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index e800f1449888..c14ed1a4d3ce 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -11,6 +11,7 @@
> >
> > #include <libcamera/buffer.h>
> > #include <libcamera/camera.h>
> > +#include <libcamera/control_ids.h>
> > #include <libcamera/stream.h>
> >
> > #include "camera_controls.h"
> > @@ -64,12 +65,12 @@ Request::Request(Camera *camera, uint64_t cookie)
> > * creating a new instance for each request?
> > */
> > validator_ = new CameraControlValidator(camera);
> > - controls_ = new ControlList(validator_);
> > + controls_ = new ControlList(controls::controls, validator_);
> >
> > /**
> > * \todo: Add a validator for metadata controls.
> > */
> > - metadata_ = new ControlList();
> > + metadata_ = new ControlList(controls::controls);
> > }
> >
> > Request::~Request()
> > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp
> > index 1bcfecc467b5..5af53f64bb6c 100644
> > --- a/test/controls/control_list.cpp
> > +++ b/test/controls/control_list.cpp
> > @@ -42,7 +42,7 @@ protected:
> > int run()
> > {
> > CameraControlValidator validator(camera_.get());
> > - ControlList list(&validator);
> > + ControlList list(controls::controls, &validator);
> >
> > /* Test that the list is initially empty. */
> > if (!list.empty()) {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list