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

[Laurent Pinchart]

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 */

-- 
Regards,

Laurent Pinchart