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

[Jacopo Mondi]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190122/de1414c7/attachment.sig>