[libcamera-devel,9/9] libcamera: v4l2_controls: Add control validator
diff mbox series

Message ID 20191007224642.6597-10-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Use ControlList for both libcamera and V4L2 controls
Related show

Commit Message

Laurent Pinchart Oct. 7, 2019, 10:46 p.m. UTC 
Add a ControlValidator specialisation that validates controls based on a
V4L2 device.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/include/v4l2_controls.h | 14 ++++++++++++
 src/libcamera/v4l2_controls.cpp       | 33 +++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

Comments

Jacopo Mondi Oct. 9, 2019, 9:27 a.m. UTC | #1 
Hi Laurent,

On Tue, Oct 08, 2019 at 01:46:42AM +0300, Laurent Pinchart wrote:
> Add a ControlValidator specialisation that validates controls based on a
> V4L2 device.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/include/v4l2_controls.h | 14 ++++++++++++
>  src/libcamera/v4l2_controls.cpp       | 33 +++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>
> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> index 133ab9ec208b..5fd92e3ac4e5 100644
> --- a/src/libcamera/include/v4l2_controls.h
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -17,6 +17,8 @@
>
>  #include <libcamera/controls.h>
>
> +#include "control_validator.h"
> +
>  namespace libcamera {
>
>  class V4L2ControlId : public ControlId
> @@ -57,6 +59,18 @@ private:
>  	const V4L2ControlInfoMap &controls_;
>  };
>
> +class V4L2ControlValidator final : public ControlValidator
> +{
> +public:
> +	V4L2ControlValidator(V4L2Device *device);
> +
> +	const std::string &name() const override;
> +	bool validate(const ControlId &id) const override;
> +
> +private:
> +	V4L2Device *dev_;
> +};
> +
>  } /* namespace libcamera */
>
>  #endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> index d3f94709e821..498567ee0bf0 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -239,4 +239,37 @@ void V4L2ControlList::set(unsigned int id, const ControlValue &value)
>  	ControlList::set(ctrl->second.id(), value);
>  }
>
> +/**
> + * \class V4L2ControlValidator
> + * \brief A control validator for V4L2Devoe instances

V4L2Device

> + *
> + * This ControlValidator specialisation validates that controls exist in the
> + * V4L2Device associated with the validator.
> + */
> +
> +/**
> + * \brief Construst a V4L2ControlValidator for the V4L2 \a device
> + * \param[in] device The V4L2 device
> + */
> +V4L2ControlValidator::V4L2ControlValidator(V4L2Device *device)
> +	: dev_(device)
> +{
> +}
> +
> +const std::string &V4L2ControlValidator::name() const
> +{
> +	return dev_->deviceNode();
> +}
> +
> +/**
> + * \brief Validate a control
> + * \param[in] id The control ID
> + * \return True if the control is valid, false otherwise

I would expand what "valid" means in this context.
All validators 'validates' controls, this one does it against the list
of controls supported by the video device.

Thanks
  j


> + */
> +bool V4L2ControlValidator::validate(const ControlId &id) const
> +{
> +	const V4L2ControlInfoMap &controls = dev_->controls();
> +	return controls.find(id.id()) != controls.end();

The fact std::map::contains(T key) has only been introduced in C++ is
really a shame, we have this find() == end() pattern everywhere :(

> +}
> +
>  } /* namespace libcamera */
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Oct. 9, 2019, 9:48 a.m. UTC | #2 
Hi Jacopo,

On Wed, Oct 09, 2019 at 11:27:39AM +0200, Jacopo Mondi wrote:
> On Tue, Oct 08, 2019 at 01:46:42AM +0300, Laurent Pinchart wrote:
> > Add a ControlValidator specialisation that validates controls based on a
> > V4L2 device.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/include/v4l2_controls.h | 14 ++++++++++++
> >  src/libcamera/v4l2_controls.cpp       | 33 +++++++++++++++++++++++++++
> >  2 files changed, 47 insertions(+)
> >
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > index 133ab9ec208b..5fd92e3ac4e5 100644
> > --- a/src/libcamera/include/v4l2_controls.h
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -17,6 +17,8 @@
> >
> >  #include <libcamera/controls.h>
> >
> > +#include "control_validator.h"
> > +
> >  namespace libcamera {
> >
> >  class V4L2ControlId : public ControlId
> > @@ -57,6 +59,18 @@ private:
> >  	const V4L2ControlInfoMap &controls_;
> >  };
> >
> > +class V4L2ControlValidator final : public ControlValidator
> > +{
> > +public:
> > +	V4L2ControlValidator(V4L2Device *device);
> > +
> > +	const std::string &name() const override;
> > +	bool validate(const ControlId &id) const override;
> > +
> > +private:
> > +	V4L2Device *dev_;
> > +};
> > +
> >  } /* namespace libcamera */
> >
> >  #endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index d3f94709e821..498567ee0bf0 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -239,4 +239,37 @@ void V4L2ControlList::set(unsigned int id, const ControlValue &value)
> >  	ControlList::set(ctrl->second.id(), value);
> >  }
> >
> > +/**
> > + * \class V4L2ControlValidator
> > + * \brief A control validator for V4L2Devoe instances
> 
> V4L2Device

Probably better than introducing a V4L2Devoe class, yes.

> > + *
> > + * This ControlValidator specialisation validates that controls exist in the
> > + * V4L2Device associated with the validator.
> > + */
> > +
> > +/**
> > + * \brief Construst a V4L2ControlValidator for the V4L2 \a device
> > + * \param[in] device The V4L2 device
> > + */
> > +V4L2ControlValidator::V4L2ControlValidator(V4L2Device *device)
> > +	: dev_(device)
> > +{
> > +}
> > +
> > +const std::string &V4L2ControlValidator::name() const
> > +{
> > +	return dev_->deviceNode();
> > +}
> > +
> > +/**
> > + * \brief Validate a control
> > + * \param[in] id The control ID
> > + * \return True if the control is valid, false otherwise
> 
> I would expand what "valid" means in this context.
> All validators 'validates' controls, this one does it against the list
> of controls supported by the video device.

"A control is considered valid if it is supported by the V4L2Device
associated with the validator." ?

> > + */
> > +bool V4L2ControlValidator::validate(const ControlId &id) const
> > +{
> > +	const V4L2ControlInfoMap &controls = dev_->controls();
> > +	return controls.find(id.id()) != controls.end();
> 
> The fact std::map::contains(T key) has only been introduced in C++ is
> really a shame, we have this find() == end() pattern everywhere :(

I know :-( At some point we could bump our minimal C++ version, but
C++20 is definitely too recent.

> > +}
> > +
> >  } /* namespace libcamera */
Jacopo Mondi Oct. 9, 2019, 9:59 a.m. UTC | #3 
Hi Laurent,

On Wed, Oct 09, 2019 at 12:48:36PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Oct 09, 2019 at 11:27:39AM +0200, Jacopo Mondi wrote:
> > On Tue, Oct 08, 2019 at 01:46:42AM +0300, Laurent Pinchart wrote:
> > > Add a ControlValidator specialisation that validates controls based on a
> > > V4L2 device.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/include/v4l2_controls.h | 14 ++++++++++++
> > >  src/libcamera/v4l2_controls.cpp       | 33 +++++++++++++++++++++++++++
> > >  2 files changed, 47 insertions(+)
> > >
> > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > > index 133ab9ec208b..5fd92e3ac4e5 100644
> > > --- a/src/libcamera/include/v4l2_controls.h
> > > +++ b/src/libcamera/include/v4l2_controls.h
> > > @@ -17,6 +17,8 @@
> > >
> > >  #include <libcamera/controls.h>
> > >
> > > +#include "control_validator.h"
> > > +
> > >  namespace libcamera {
> > >
> > >  class V4L2ControlId : public ControlId
> > > @@ -57,6 +59,18 @@ private:
> > >  	const V4L2ControlInfoMap &controls_;
> > >  };
> > >
> > > +class V4L2ControlValidator final : public ControlValidator
> > > +{
> > > +public:
> > > +	V4L2ControlValidator(V4L2Device *device);
> > > +
> > > +	const std::string &name() const override;
> > > +	bool validate(const ControlId &id) const override;
> > > +
> > > +private:
> > > +	V4L2Device *dev_;
> > > +};
> > > +
> > >  } /* namespace libcamera */
> > >
> > >  #endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > > index d3f94709e821..498567ee0bf0 100644
> > > --- a/src/libcamera/v4l2_controls.cpp
> > > +++ b/src/libcamera/v4l2_controls.cpp
> > > @@ -239,4 +239,37 @@ void V4L2ControlList::set(unsigned int id, const ControlValue &value)
> > >  	ControlList::set(ctrl->second.id(), value);
> > >  }
> > >
> > > +/**
> > > + * \class V4L2ControlValidator
> > > + * \brief A control validator for V4L2Devoe instances
> >
> > V4L2Device
>
> Probably better than introducing a V4L2Devoe class, yes.
>
> > > + *
> > > + * This ControlValidator specialisation validates that controls exist in the
> > > + * V4L2Device associated with the validator.
> > > + */
> > > +
> > > +/**
> > > + * \brief Construst a V4L2ControlValidator for the V4L2 \a device
> > > + * \param[in] device The V4L2 device
> > > + */
> > > +V4L2ControlValidator::V4L2ControlValidator(V4L2Device *device)
> > > +	: dev_(device)
> > > +{
> > > +}
> > > +
> > > +const std::string &V4L2ControlValidator::name() const
> > > +{
> > > +	return dev_->deviceNode();
> > > +}
> > > +
> > > +/**
> > > + * \brief Validate a control
> > > + * \param[in] id The control ID
> > > + * \return True if the control is valid, false otherwise
> >
> > I would expand what "valid" means in this context.
> > All validators 'validates' controls, this one does it against the list
> > of controls supported by the video device.
>
> "A control is considered valid if it is supported by the V4L2Device
> associated with the validator." ?
>

Better, thanks!

> > > + */
> > > +bool V4L2ControlValidator::validate(const ControlId &id) const
> > > +{
> > > +	const V4L2ControlInfoMap &controls = dev_->controls();
> > > +	return controls.find(id.id()) != controls.end();
> >
> > The fact std::map::contains(T key) has only been introduced in C++ is
> > really a shame, we have this find() == end() pattern everywhere :(
>
> I know :-( At some point we could bump our minimal C++ version, but
> C++20 is definitely too recent.
>

Not just too recent, it's in the future!1!1

Thanks
   j

> > > +}
> > > +
> > >  } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
index 133ab9ec208b..5fd92e3ac4e5 100644
--- a/src/libcamera/include/v4l2_controls.h
+++ b/src/libcamera/include/v4l2_controls.h
@@ -17,6 +17,8 @@ 
 
 #include <libcamera/controls.h>
 
+#include "control_validator.h"
+
 namespace libcamera {
 
 class V4L2ControlId : public ControlId
@@ -57,6 +59,18 @@  private:
 	const V4L2ControlInfoMap &controls_;
 };
 
+class V4L2ControlValidator final : public ControlValidator
+{
+public:
+	V4L2ControlValidator(V4L2Device *device);
+
+	const std::string &name() const override;
+	bool validate(const ControlId &id) const override;
+
+private:
+	V4L2Device *dev_;
+};
+
 } /* namespace libcamera */
 
 #endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
index d3f94709e821..498567ee0bf0 100644
--- a/src/libcamera/v4l2_controls.cpp
+++ b/src/libcamera/v4l2_controls.cpp
@@ -239,4 +239,37 @@  void V4L2ControlList::set(unsigned int id, const ControlValue &value)
 	ControlList::set(ctrl->second.id(), value);
 }
 
+/**
+ * \class V4L2ControlValidator
+ * \brief A control validator for V4L2Devoe instances
+ *
+ * This ControlValidator specialisation validates that controls exist in the
+ * V4L2Device associated with the validator.
+ */
+
+/**
+ * \brief Construst a V4L2ControlValidator for the V4L2 \a device
+ * \param[in] device The V4L2 device
+ */
+V4L2ControlValidator::V4L2ControlValidator(V4L2Device *device)
+	: dev_(device)
+{
+}
+
+const std::string &V4L2ControlValidator::name() const
+{
+	return dev_->deviceNode();
+}
+
+/**
+ * \brief Validate a control
+ * \param[in] id The control ID
+ * \return True if the control is valid, false otherwise
+ */
+bool V4L2ControlValidator::validate(const ControlId &id) const
+{
+	const V4L2ControlInfoMap &controls = dev_->controls();
+	return controls.find(id.id()) != controls.end();
+}
+
 } /* namespace libcamera */