[libcamera-devel] [RFC 2/3] libcamera: v4l2_device: Add methods to get/set format

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-01-26 16:13:17 +0100, Jacopo Mondi wrote:
> Add methods to set and get the image format programmed on a V4L2Device
> for both the single and multi planar use case.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/include/v4l2_device.h |  13 +++
>  src/libcamera/v4l2_device.cpp       | 131 ++++++++++++++++++++++++++++
>  2 files changed, 144 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index ee0c4eb..01dbf45 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -14,6 +14,7 @@
>  
>  #include <libcamera/buffer.h>
>  #include <libcamera/event_notifier.h>
> +#include <libcamera/stream.h>
>  
>  namespace libcamera {
>  
> @@ -81,6 +82,9 @@ public:
>  	int requestBuffers(unsigned int qty = 8);
>  	int requestBuffers(unsigned int qty, std::vector<FrameBuffer *> &buffers);
>  
> +	int format(StreamConfiguration *config);
> +	int setFormat(StreamConfiguration *config);
> +

I'm not sure these should take StreamConfiguration as an argument. Maybe 
the PipelineHandler should translate the StreamConfigurations it 
receives into a structure which is V4L2 specific?

>  	int queueBuffer(FrameBuffer *frame);
>  	FrameBuffer *dequeueBuffer();
>  
> @@ -93,6 +97,7 @@ private:
>  	std::string deviceNode_;
>  	int fd_;
>  	V4L2Capability caps_;
> +	unsigned int buftype_;
>  
>  	enum v4l2_memory memoryType_;
>  	enum v4l2_buf_type bufferType_;
> @@ -101,6 +106,14 @@ private:
>  	void bufferAvailable(EventNotifier *notifier);
>  
>  	EventNotifier *fdEvent_;
> +
> +	unsigned int getPlanesFromFormat(unsigned int pixfmt);
> +	unsigned int getBppFromFormat(unsigned int pixfmt);

Should these methods be part of the V4L2Device or some other v4l2 util 
collection? I wonder if this even could be useful for applications as we 
currently expose the pixel format as a V4L2 FOURCC there. I'm not sure 
what the end design will look like so this is just an open question.


> +	int getFormatSingleplane(StreamConfiguration *config);
> +	int setFormatSingleplane(StreamConfiguration *config);
> +
> +	int getFormatMultiplane(StreamConfiguration *config);
> +	int setFormatMultiplane(StreamConfiguration *config);

Here I like the deviation from our usual style of not using get*().

>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 4d39c85..d16691d 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -203,6 +203,15 @@ int V4L2Device::open()
>  		return -EINVAL;
>  	}
>  
> +	if (caps_.isCapture())
> +		buftype_ = caps_.isMultiplanar() ?
> +			   V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE :
> +			   V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	else
> +		buftype_ = caps_.isMultiplanar() ?
> +			   V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
> +			   V4L2_BUF_TYPE_VIDEO_OUTPUT;
> +
>  	return 0;
>  }
>  
> @@ -314,6 +323,26 @@ int V4L2Device::requestBuffers(unsigned int qty, std::vector<FrameBuffer *> &buf
>  	return 0;
>  }
>  
> +/**
> + * \brief Retrieve the image format set on the V4L2 device
> + * \return 0 for success, a negative error code otherwise
> + */
> +int V4L2Device::format(StreamConfiguration *config)
> +{
> +	return caps_.isMultiplanar() ? getFormatMultiplane(config) :
> +				       getFormatSingleplane(config);
> +}
> +
> +/**
> + * \brief Program an image format on the V4L2 device
> + * \return 0 for success, a negative error code otherwise
> + */
> +int V4L2Device::setFormat(StreamConfiguration *config)
> +{
> +	return caps_.isMultiplanar() ? setFormatMultiplane(config) :
> +				       setFormatSingleplane(config);
> +}

Looking at the implementation of the get*() and set*() bellow I'm 
wondering if it's possible to handle the difference between the two 
inside just one {get,set}Format() function. Or do you expect these 
functions to grow over time?

If you really want to keep them separate maybe it's possible to split 
out the creation of the v4l2_format strucutre and keep the actual ioctl 
and config->set*() common somehow? Or maybe I'm just obsessing about two 
things that looks similar but really are not.

> +
>  int V4L2Device::queueBuffer(FrameBuffer *frame)
>  {
>  	struct v4l2_buffer buf = {};
> @@ -422,6 +451,108 @@ int V4L2Device::streamOff()
>  	}
>  
>  	delete fdEvent_;
> +}
> +
> +/* TODO: this is a simple stub at the moment. */
> +unsigned int V4L2Device::getPlanesFromFormat(unsigned int pixfmt)
> +{
> +	return 1;
> +}
> +
> +/* TODO: this is a simple stub at the moment. */
> +unsigned int V4L2Device::getBppFromFormat(unsigned int pixfmt)
> +{
> +	return 16;
> +}
> +
> +int V4L2Device::getFormatSingleplane(StreamConfiguration *config)
> +{
> +	struct v4l2_format v4l2Fmt;
> +	struct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;
> +	int ret;
> +
> +	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(Error) << "Unable to get format: " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	config->setWidth(pix->width);
> +	config->setHeight(pix->height);
> +	config->setPixelFormat(pix->pixelformat);
> +
> +	return 0;
> +}
> +
> +int V4L2Device::setFormatSingleplane(StreamConfiguration *config)
> +{
> +	struct v4l2_format v4l2Fmt;
> +	struct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;
> +	int ret;
> +
> +	v4l2Fmt.type = buftype_;
> +	pix->width = config->width();
> +	pix->height = config->height();
> +
> +	pix->width = config->width();
> +	pix->height = config->height();
> +	pix->pixelformat = config->pixelformat();
> +
> +	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(Error) << "Unable to set format: " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int V4L2Device::getFormatMultiplane(StreamConfiguration *config)
> +{
> +	struct v4l2_format v4l2Fmt;
> +	struct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;
> +	int ret;
> +
> +	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Fmt);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(Error) << "Unable to get format: " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	config->setWidth(pix->width);
> +	config->setHeight(pix->height);
> +	config->setPixelFormat(pix->pixelformat);
> +
> +	return 0;
> +}
> +
> +int V4L2Device::setFormatMultiplane(StreamConfiguration *config)
> +{
> +	struct v4l2_format v4l2Fmt;
> +	struct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;
> +	int ret;
> +
> +	v4l2Fmt.type = buftype_;
> +	pix->width = config->width();
> +	pix->height = config->height();
> +	pix->pixelformat = config->pixelformat();
> +
> +	unsigned int numPlanes = getPlanesFromFormat(pix->pixelformat);
> +	unsigned int bpp = getBppFromFormat(pix->pixelformat);
> +	for (unsigned int i = 0; i < numPlanes; ++i) {
> +		pix->plane_fmt[i].bytesperline = bpp * pix->width;
> +		pix->plane_fmt[i].sizeimage = bpp * pix->width * pix->height;
> +	}
> +
> +	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(Error) << "Unable to set format: " << strerror(-ret);
> +		return ret;
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund