[libcamera-devel,4/6] libcamera: v4l2_device: Add single/multiplane formats
diff mbox series

Message ID 20190121172705.19985-5-jacopo@jmondi.org
State Rejected
Headers show
Series
  • libcamera: Augment V4L2 device
Related show

Commit Message

Jacopo Mondi Jan. 21, 2019, 5:27 p.m. UTC 
Add a class hierarchy internal to V4L2Device to handle set/get format
operations in the single/multi plane use case.

Provide stubs only for  get/setFormat methods at the moment.
---
 src/libcamera/include/v4l2_device.h | 34 +++++++++++++++++
 src/libcamera/v4l2_device.cpp       | 59 +++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)

Comments

Laurent Pinchart Jan. 21, 2019, 8:46 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Mon, Jan 21, 2019 at 06:27:03PM +0100, Jacopo Mondi wrote:
> Add a class hierarchy internal to V4L2Device to handle set/get format
> operations in the single/multi plane use case.
> 
> Provide stubs only for  get/setFormat methods at the moment.
> ---
>  src/libcamera/include/v4l2_device.h | 34 +++++++++++++++++
>  src/libcamera/v4l2_device.cpp       | 59 +++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 0101a4b..81992dc 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_V4L2_DEVICE_H__
>  #define __LIBCAMERA_V4L2_DEVICE_H__
>  
> +#include <memory>
>  #include <string>
>  
>  #include <linux/videodev2.h>
> @@ -54,11 +55,44 @@ public:
>  	const char *busName() const { return caps_.bus_info(); }
>  
>  	int setControl(unsigned int control, int value);
> +	int getFormat();

Should this be named just format() ?

> +	int setFormat();
>  
>  private:
> +	class V4L2Format
> +	{
> +	public:
> +		virtual ~V4L2Format() { }
> +		virtual int setFormat() = 0;
> +		virtual int getFormat() = 0;
> +
> +	protected:
> +		V4L2Format() { }
> +		V4L2Format(V4L2Format &) = delete;
> +	};
> +
> +	class V4L2FormatSPlane : public V4L2Format
> +	{
> +	public:
> +		V4L2FormatSPlane() { }
> +		~V4L2FormatSPlane() { }
> +		int setFormat();
> +		int getFormat();
> +	};
> +
> +	class V4L2FormatMPlane : public V4L2Format
> +	{
> +	public:
> +		V4L2FormatMPlane() { }
> +		~V4L2FormatMPlane() { }
> +		int setFormat();
> +		int getFormat();
> +	};
> +
>  	std::string devnode_;
>  	int fd_;
>  	V4L2Capability caps_;
> +	std::unique_ptr<V4L2Format> format_;

I have a bit of trouble reviewing this patch as I don't understand how
this is supposed to be used, but it seems overly complicated to me. I
think a simple

	if (single planaer) {
		...
	} else {
		...
	}

in getFormat() and setFormat() would be good enough.

>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 7cd89d7..126f6f2 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -5,6 +5,8 @@
>   * v4l2_device.cpp - V4L2 Device
>   */
>  
> +#include <memory>
> +
>  #include <fcntl.h>
>  #include <string.h>
>  #include <sys/ioctl.h>
> @@ -13,6 +15,7 @@
>  
>  #include "log.h"
>  #include "media_object.h"
> +#include "utils.h"
>  #include "v4l2_device.h"
>  
>  /**
> @@ -182,6 +185,12 @@ int V4L2Device::open()
>  		return -EINVAL;
>  	}
>  
> +	/* Create single/multi plane format handler. */
> +	if (caps_.isSinglePlane())
> +		format_ = utils::make_unique<V4L2FormatSPlane>();
> +	else
> +		format_ = utils::make_unique<V4L2FormatMPlane>();
> +
>  	return 0;
>  }
>  
> @@ -262,4 +271,54 @@ int V4L2Device::setControl(unsigned int control, int value)
>  	return v4l2_ctrl.value;
>  }
>  
> +/**
> + * \brief Retrieve the image format set on the V4L2 device
> + *
> + * TODO: define how the image format is encoded
> + * FIXME: this function is a stub at the moment
> + *
> + * \return 0 for success, a negative error code otherwise
> + */
> +int V4L2Device::getFormat()
> +{
> +	return format_->getFormat();
> +}
> +
> +/**
> + * \brief Program an image format on the V4L2 device
> + *
> + * TODO: define how the image format is encoded
> + * FIXME: this function is a stub at the moment
> + *
> + * \return 0 for success, a negative error code otherwise
> + */
> +int V4L2Device::setFormat()
> +{
> +	return format_->setFormat();
> +}
> +
> +/* FIXME: this function is a stub at the moment. */
> +int V4L2Device::V4L2FormatSPlane::getFormat()
> +{
> +	return 0;
> +}
> +
> +/* FIXME: this function is a stub at the moment. */
> +int V4L2Device::V4L2FormatSPlane::setFormat()
> +{
> +	return 0;
> +}
> +
> +/* FIXME: this function is a stub at the moment. */
> +int V4L2Device::V4L2FormatMPlane::getFormat()
> +{
> +	return 0;
> +}
> +
> +/* FIXME: this function is a stub at the moment. */
> +int V4L2Device::V4L2FormatMPlane::setFormat()
> +{
> +	return 0;
> +}
> +
>  } /* namespace libcamera */
Niklas Söderlund Jan. 22, 2019, 11:39 a.m. UTC | #2 
Hej,

On 2019-01-21 22:46:03 +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Jan 21, 2019 at 06:27:03PM +0100, Jacopo Mondi wrote:
> > Add a class hierarchy internal to V4L2Device to handle set/get format
> > operations in the single/multi plane use case.
> > 
> > Provide stubs only for  get/setFormat methods at the moment.
> > ---
> >  src/libcamera/include/v4l2_device.h | 34 +++++++++++++++++
> >  src/libcamera/v4l2_device.cpp       | 59 +++++++++++++++++++++++++++++
> >  2 files changed, 93 insertions(+)
> > 
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 0101a4b..81992dc 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __LIBCAMERA_V4L2_DEVICE_H__
> >  #define __LIBCAMERA_V4L2_DEVICE_H__
> >  
> > +#include <memory>
> >  #include <string>
> >  
> >  #include <linux/videodev2.h>
> > @@ -54,11 +55,44 @@ public:
> >  	const char *busName() const { return caps_.bus_info(); }
> >  
> >  	int setControl(unsigned int control, int value);
> > +	int getFormat();
> 
> Should this be named just format() ?
> 
> > +	int setFormat();
> >  
> >  private:
> > +	class V4L2Format
> > +	{
> > +	public:
> > +		virtual ~V4L2Format() { }
> > +		virtual int setFormat() = 0;
> > +		virtual int getFormat() = 0;
> > +
> > +	protected:
> > +		V4L2Format() { }
> > +		V4L2Format(V4L2Format &) = delete;
> > +	};
> > +
> > +	class V4L2FormatSPlane : public V4L2Format
> > +	{
> > +	public:
> > +		V4L2FormatSPlane() { }
> > +		~V4L2FormatSPlane() { }
> > +		int setFormat();
> > +		int getFormat();
> > +	};
> > +
> > +	class V4L2FormatMPlane : public V4L2Format
> > +	{
> > +	public:
> > +		V4L2FormatMPlane() { }
> > +		~V4L2FormatMPlane() { }
> > +		int setFormat();
> > +		int getFormat();
> > +	};
> > +
> >  	std::string devnode_;
> >  	int fd_;
> >  	V4L2Capability caps_;
> > +	std::unique_ptr<V4L2Format> format_;
> 
> I have a bit of trouble reviewing this patch as I don't understand how
> this is supposed to be used, but it seems overly complicated to me. I
> think a simple
> 
> 	if (single planaer) {
> 		...
> 	} else {
> 		...
> 	}
> 
> in getFormat() and setFormat() would be good enough.
> 

I tend to agree with this, but it's just my two euro cents.

> >  };
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 7cd89d7..126f6f2 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -5,6 +5,8 @@
> >   * v4l2_device.cpp - V4L2 Device
> >   */
> >  
> > +#include <memory>
> > +
> >  #include <fcntl.h>
> >  #include <string.h>
> >  #include <sys/ioctl.h>
> > @@ -13,6 +15,7 @@
> >  
> >  #include "log.h"
> >  #include "media_object.h"
> > +#include "utils.h"
> >  #include "v4l2_device.h"
> >  
> >  /**
> > @@ -182,6 +185,12 @@ int V4L2Device::open()
> >  		return -EINVAL;
> >  	}
> >  
> > +	/* Create single/multi plane format handler. */
> > +	if (caps_.isSinglePlane())
> > +		format_ = utils::make_unique<V4L2FormatSPlane>();
> > +	else
> > +		format_ = utils::make_unique<V4L2FormatMPlane>();
> > +
> >  	return 0;
> >  }
> >  
> > @@ -262,4 +271,54 @@ int V4L2Device::setControl(unsigned int control, int value)
> >  	return v4l2_ctrl.value;
> >  }
> >  
> > +/**
> > + * \brief Retrieve the image format set on the V4L2 device
> > + *
> > + * TODO: define how the image format is encoded
> > + * FIXME: this function is a stub at the moment
> > + *
> > + * \return 0 for success, a negative error code otherwise
> > + */
> > +int V4L2Device::getFormat()
> > +{
> > +	return format_->getFormat();
> > +}

Is not V4L2Device::getFormat() a virtual function which should not be 
implemented?

> > +
> > +/**
> > + * \brief Program an image format on the V4L2 device
> > + *
> > + * TODO: define how the image format is encoded
> > + * FIXME: this function is a stub at the moment
> > + *
> > + * \return 0 for success, a negative error code otherwise
> > + */
> > +int V4L2Device::setFormat()
> > +{
> > +	return format_->setFormat();
> > +}
> > +
> > +/* FIXME: this function is a stub at the moment. */
> > +int V4L2Device::V4L2FormatSPlane::getFormat()
> > +{
> > +	return 0;
> > +}
> > +
> > +/* FIXME: this function is a stub at the moment. */
> > +int V4L2Device::V4L2FormatSPlane::setFormat()
> > +{
> > +	return 0;
> > +}
> > +
> > +/* FIXME: this function is a stub at the moment. */
> > +int V4L2Device::V4L2FormatMPlane::getFormat()
> > +{
> > +	return 0;
> > +}
> > +
> > +/* FIXME: this function is a stub at the moment. */
> > +int V4L2Device::V4L2FormatMPlane::setFormat()
> > +{
> > +	return 0;
> > +}
> > +
> >  } /* namespace libcamera */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Jan. 22, 2019, 2:14 p.m. UTC | #3 
Hi Laurent

On Mon, Jan 21, 2019 at 10:46:03PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Jan 21, 2019 at 06:27:03PM +0100, Jacopo Mondi wrote:
> > Add a class hierarchy internal to V4L2Device to handle set/get format
> > operations in the single/multi plane use case.
> >
> > Provide stubs only for  get/setFormat methods at the moment.
> > ---
> >  src/libcamera/include/v4l2_device.h | 34 +++++++++++++++++
> >  src/libcamera/v4l2_device.cpp       | 59 +++++++++++++++++++++++++++++
> >  2 files changed, 93 insertions(+)
> >
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 0101a4b..81992dc 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __LIBCAMERA_V4L2_DEVICE_H__
> >  #define __LIBCAMERA_V4L2_DEVICE_H__
> >
> > +#include <memory>
> >  #include <string>
> >
> >  #include <linux/videodev2.h>
> > @@ -54,11 +55,44 @@ public:
> >  	const char *busName() const { return caps_.bus_info(); }
> >
> >  	int setControl(unsigned int control, int value);
> > +	int getFormat();
>
> Should this be named just format() ?
>
> > +	int setFormat();
> >
> >  private:
> > +	class V4L2Format
> > +	{
> > +	public:
> > +		virtual ~V4L2Format() { }
> > +		virtual int setFormat() = 0;
> > +		virtual int getFormat() = 0;
> > +
> > +	protected:
> > +		V4L2Format() { }
> > +		V4L2Format(V4L2Format &) = delete;
> > +	};
> > +
> > +	class V4L2FormatSPlane : public V4L2Format
> > +	{
> > +	public:
> > +		V4L2FormatSPlane() { }
> > +		~V4L2FormatSPlane() { }
> > +		int setFormat();
> > +		int getFormat();
> > +	};
> > +
> > +	class V4L2FormatMPlane : public V4L2Format
> > +	{
> > +	public:
> > +		V4L2FormatMPlane() { }
> > +		~V4L2FormatMPlane() { }
> > +		int setFormat();
> > +		int getFormat();
> > +	};
> > +
> >  	std::string devnode_;
> >  	int fd_;
> >  	V4L2Capability caps_;
> > +	std::unique_ptr<V4L2Format> format_;
>
> I have a bit of trouble reviewing this patch as I don't understand how
> this is supposed to be used, but it seems overly complicated to me. I
> think a simple
>
> 	if (single planaer) {
> 		...
> 	} else {
> 		...
> 	}
>
> in getFormat() and setFormat() would be good enough.

Maybe. I made a class hierarcy to handle the format set/get because I
thought that would have been useful to store in the class instances
informations on the currently configured format and isolate it from
the rest of the V4L2 device object.

Both you and Niklas think this is an overkill, and I can drop it.
I'm sure this is not "too complicated to review" for you, though.

Thanks
   j

>
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 7cd89d7..126f6f2 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -5,6 +5,8 @@
> >   * v4l2_device.cpp - V4L2 Device
> >   */
> >
> > +#include <memory>
> > +
> >  #include <fcntl.h>
> >  #include <string.h>
> >  #include <sys/ioctl.h>
> > @@ -13,6 +15,7 @@
> >
> >  #include "log.h"
> >  #include "media_object.h"
> > +#include "utils.h"
> >  #include "v4l2_device.h"
> >
> >  /**
> > @@ -182,6 +185,12 @@ int V4L2Device::open()
> >  		return -EINVAL;
> >  	}
> >
> > +	/* Create single/multi plane format handler. */
> > +	if (caps_.isSinglePlane())
> > +		format_ = utils::make_unique<V4L2FormatSPlane>();
> > +	else
> > +		format_ = utils::make_unique<V4L2FormatMPlane>();
> > +
> >  	return 0;
> >  }
> >
> > @@ -262,4 +271,54 @@ int V4L2Device::setControl(unsigned int control, int value)
> >  	return v4l2_ctrl.value;
> >  }
> >
> > +/**
> > + * \brief Retrieve the image format set on the V4L2 device
> > + *
> > + * TODO: define how the image format is encoded
> > + * FIXME: this function is a stub at the moment
> > + *
> > + * \return 0 for success, a negative error code otherwise
> > + */
> > +int V4L2Device::getFormat()
> > +{
> > +	return format_->getFormat();
> > +}
> > +
> > +/**
> > + * \brief Program an image format on the V4L2 device
> > + *
> > + * TODO: define how the image format is encoded
> > + * FIXME: this function is a stub at the moment
> > + *
> > + * \return 0 for success, a negative error code otherwise
> > + */
> > +int V4L2Device::setFormat()
> > +{
> > +	return format_->setFormat();
> > +}
> > +
> > +/* FIXME: this function is a stub at the moment. */
> > +int V4L2Device::V4L2FormatSPlane::getFormat()
> > +{
> > +	return 0;
> > +}
> > +
> > +/* FIXME: this function is a stub at the moment. */
> > +int V4L2Device::V4L2FormatSPlane::setFormat()
> > +{
> > +	return 0;
> > +}
> > +
> > +/* FIXME: this function is a stub at the moment. */
> > +int V4L2Device::V4L2FormatMPlane::getFormat()
> > +{
> > +	return 0;
> > +}
> > +
> > +/* FIXME: this function is a stub at the moment. */
> > +int V4L2Device::V4L2FormatMPlane::setFormat()
> > +{
> > +	return 0;
> > +}
> > +
> >  } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
index 0101a4b..81992dc 100644
--- a/src/libcamera/include/v4l2_device.h
+++ b/src/libcamera/include/v4l2_device.h
@@ -7,6 +7,7 @@ 
 #ifndef __LIBCAMERA_V4L2_DEVICE_H__
 #define __LIBCAMERA_V4L2_DEVICE_H__
 
+#include <memory>
 #include <string>
 
 #include <linux/videodev2.h>
@@ -54,11 +55,44 @@  public:
 	const char *busName() const { return caps_.bus_info(); }
 
 	int setControl(unsigned int control, int value);
+	int getFormat();
+	int setFormat();
 
 private:
+	class V4L2Format
+	{
+	public:
+		virtual ~V4L2Format() { }
+		virtual int setFormat() = 0;
+		virtual int getFormat() = 0;
+
+	protected:
+		V4L2Format() { }
+		V4L2Format(V4L2Format &) = delete;
+	};
+
+	class V4L2FormatSPlane : public V4L2Format
+	{
+	public:
+		V4L2FormatSPlane() { }
+		~V4L2FormatSPlane() { }
+		int setFormat();
+		int getFormat();
+	};
+
+	class V4L2FormatMPlane : public V4L2Format
+	{
+	public:
+		V4L2FormatMPlane() { }
+		~V4L2FormatMPlane() { }
+		int setFormat();
+		int getFormat();
+	};
+
 	std::string devnode_;
 	int fd_;
 	V4L2Capability caps_;
+	std::unique_ptr<V4L2Format> format_;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 7cd89d7..126f6f2 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -5,6 +5,8 @@ 
  * v4l2_device.cpp - V4L2 Device
  */
 
+#include <memory>
+
 #include <fcntl.h>
 #include <string.h>
 #include <sys/ioctl.h>
@@ -13,6 +15,7 @@ 
 
 #include "log.h"
 #include "media_object.h"
+#include "utils.h"
 #include "v4l2_device.h"
 
 /**
@@ -182,6 +185,12 @@  int V4L2Device::open()
 		return -EINVAL;
 	}
 
+	/* Create single/multi plane format handler. */
+	if (caps_.isSinglePlane())
+		format_ = utils::make_unique<V4L2FormatSPlane>();
+	else
+		format_ = utils::make_unique<V4L2FormatMPlane>();
+
 	return 0;
 }
 
@@ -262,4 +271,54 @@  int V4L2Device::setControl(unsigned int control, int value)
 	return v4l2_ctrl.value;
 }
 
+/**
+ * \brief Retrieve the image format set on the V4L2 device
+ *
+ * TODO: define how the image format is encoded
+ * FIXME: this function is a stub at the moment
+ *
+ * \return 0 for success, a negative error code otherwise
+ */
+int V4L2Device::getFormat()
+{
+	return format_->getFormat();
+}
+
+/**
+ * \brief Program an image format on the V4L2 device
+ *
+ * TODO: define how the image format is encoded
+ * FIXME: this function is a stub at the moment
+ *
+ * \return 0 for success, a negative error code otherwise
+ */
+int V4L2Device::setFormat()
+{
+	return format_->setFormat();
+}
+
+/* FIXME: this function is a stub at the moment. */
+int V4L2Device::V4L2FormatSPlane::getFormat()
+{
+	return 0;
+}
+
+/* FIXME: this function is a stub at the moment. */
+int V4L2Device::V4L2FormatSPlane::setFormat()
+{
+	return 0;
+}
+
+/* FIXME: this function is a stub at the moment. */
+int V4L2Device::V4L2FormatMPlane::getFormat()
+{
+	return 0;
+}
+
+/* FIXME: this function is a stub at the moment. */
+int V4L2Device::V4L2FormatMPlane::setFormat()
+{
+	return 0;
+}
+
 } /* namespace libcamera */