[libcamera-devel] [PATCH 4/6] libcamera: v4l2_device: Add single/multiplane formats

[Niklas Söderlund]

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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund