[libcamera-devel] [PATCH 6/9] libcamera: v4l2_controls: Add V4L2ControlId

[Laurent Pinchart]

Hi Jacopo,

On Wed, Oct 09, 2019 at 10:35:20AM +0200, Jacopo Mondi wrote:
> On Tue, Oct 08, 2019 at 01:46:39AM +0300, Laurent Pinchart wrote:
> > Add a V4L2 specialisation of the ControlId class, in order to construct
> > a ControlId from a v4l2_query_ext_ctrl. This is needed in order to use
> > ControlList for V4L2 controls.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/include/v4l2_controls.h | 12 +++--
> >  src/libcamera/v4l2_controls.cpp       | 67 +++++++++++++++++++++++----
> >  src/libcamera/v4l2_device.cpp         |  4 +-
> >  3 files changed, 68 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > index b39370b2e90e..71ce377fe4c5 100644
> > --- a/src/libcamera/include/v4l2_controls.h
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -20,23 +20,27 @@
> >
> >  namespace libcamera {
> >
> > +class V4L2ControlId : public ControlId
> > +{
> > +public:
> > +	V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);
> > +};
> > +
> 
> This only add a constructor without any field. Do we need inheritance
> or can we just add a constructor to ControlId ?

We could, but I think it's best to keep the ControlId constructor
protected, otherwise applications could create new ControlId instances.

> >  class V4L2ControlInfo
> >  {
> >  public:
> >  	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> >
> > -	unsigned int id() const { return id_; }
> > +	const ControlId &id() const { return id_; }
> >  	unsigned int type() const { return type_; }
> >  	size_t size() const { return size_; }
> > -	const std::string &name() const { return name_; }
> >
> >  	const ControlRange &range() const { return range_; }
> >
> >  private:
> > -	unsigned int id_;
> > +	V4L2ControlId id_;
> >  	unsigned int type_;
> >  	size_t size_;
> > -	std::string name_;
> >
> >  	ControlRange range_;
> >  };
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index 6f5f1578b139..a630a2583d33 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -7,6 +7,8 @@
> >
> >  #include "v4l2_controls.h"
> >
> > +#include <string.h>
> > +
> >  /**
> >   * \file v4l2_controls.h
> >   * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs
> > @@ -47,6 +49,60 @@
> >
> >  namespace libcamera {
> >
> > +namespace {
> > +
> > +std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)
> > +{
> > +	size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> > +	return std::string(static_cast<const char *>(ctrl.name), len);
> > +}
> > +
> > +ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)
> > +{
> > +	switch (ctrl.type) {
> > +	case V4L2_CTRL_TYPE_BOOLEAN:
> > +		return ControlTypeBool;
> > +
> > +	case V4L2_CTRL_TYPE_INTEGER:
> > +		return ControlTypeInteger32;
> > +
> > +	case V4L2_CTRL_TYPE_INTEGER64:
> > +		return ControlTypeInteger64;
> > +
> > +	case V4L2_CTRL_TYPE_MENU:
> > +	case V4L2_CTRL_TYPE_BUTTON:
> > +	case V4L2_CTRL_TYPE_BITMASK:
> > +	case V4L2_CTRL_TYPE_INTEGER_MENU:
> > +		/*
> > +		 * More precise types may be needed, for now use a 32-bit
> > +		 * integer type.
> > +		 */
> > +		return ControlTypeInteger32;
> > +
> > +	default:
> > +		return ControlTypeNone;
> > +	}
> > +}
> 
> Probably it's ok to have inheritance as we need this in
> v4l2_controls.cpp
> 
> This considered
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \class V4L2ControlId
> > + * \brief V4L2 control static metadata
> > + *
> > + * The V4L2ControlId class is a specialisation of the ControlId for V4L2
> > + * controls.
> > + */
> > +
> > +/**
> > + * \brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl
> > + * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> > + */
> > +V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
> > +	: ControlId(ctrl.id, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))
> > +{
> > +}
> > +
> >  /**
> >   * \class V4L2ControlInfo
> >   * \brief Information on a V4L2 control
> > @@ -66,13 +122,12 @@ namespace libcamera {
> >
> >  /**
> >   * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> > - * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> > + * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> >   */
> >  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > +	: id_(ctrl)
> >  {
> > -	id_ = ctrl.id;
> >  	type_ = ctrl.type;
> > -	name_ = static_cast<const char *>(ctrl.name);
> >  	size_ = ctrl.elem_size * ctrl.elems;
> >
> >  	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> > @@ -101,12 +156,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >   * \return The V4L2 control value data size
> >   */
> >
> > -/**
> > - * \fn V4L2ControlInfo::name()
> > - * \brief Retrieve the control user readable name
> > - * \return The V4L2 control user readable name
> > - */
> > -
> >  /**
> >   * \fn V4L2ControlInfo::range()
> >   * \brief Retrieve the control value range
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 3bd82ff23212..466c3d41f6e3 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -184,7 +184,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
> >
> >  		const V4L2ControlInfo *info = &iter->second;
> >  		controlInfo[i] = info;
> > -		v4l2Ctrls[i].id = info->id();
> > +		v4l2Ctrls[i].id = ctrl->id();
> >  	}
> >
> >  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> > @@ -259,7 +259,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
> >
> >  		const V4L2ControlInfo *info = &iter->second;
> >  		controlInfo[i] = info;
> > -		v4l2Ctrls[i].id = info->id();
> > +		v4l2Ctrls[i].id = ctrl->id();
> >
> >  		/* Set the v4l2_ext_control value for the write operation. */
> >  		switch (info->type()) {

-- 
Regards,

Laurent Pinchart