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

403 diff mbox series

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@jmondi.org>
---
 src/libcamera/include/v4l2_device.h |  13 +++
 src/libcamera/v4l2_device.cpp       | 131 ++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+)

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

Hi Niklas,

On Sun, Jan 27, 2019 at 01:39:56AM +0100, Niklas Söderlund wrote:
> 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@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?
>

I agree with you and Laurent and we should have an intermediate v4l2
format class, that gets not exposed to applications.

It will probably be shared with v4l2-subdev, as it can be equally used
to configure a 'struct v4l2_pix_format', a 'struct
v4l2_pix_format_mplane' or a 'struct v4l2_mbus_framefmt', and possibly
to instruct the v4l2 device on crop/composing too.

It will probably merit a dedicated v4l2-format.h where to collect
those, and which can be included by other modules that requires
interactinc with v4l2 devices and subdevices format configuration (the
v4l2 devs themselves and reasonably pipeline handlers only right now).

> >  	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.
>

Actually, the number of planes and bpp of a v4l2-defined format should
come from the kernel iteself, am I wrong?

Should we provide an applications facing v4l2-formats, where V4L2
FOURCCs are associated with their bpp and number of planes? Is every
piece in userspace re-implementing this? :/

>
> > +	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*().

I considered this appropriate as this is a private API anyhow.

>
> >  };
> >
> >  } /* 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?
>

As the s/mplane use case use two different members of struct
v4l2_format.fmt union, the code gets un-necessarly complicated when I
tried that.

> 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.
>

Yeah that could be given a shot to.

Thanks
   j

> > +
> >  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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Hello,

On Sun, Jan 27, 2019 at 11:38:57AM +0100, Jacopo Mondi wrote:
> On Sun, Jan 27, 2019 at 01:39:56AM +0100, Niklas Söderlund wrote:
> > 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@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?
> 
> I agree with you and Laurent and we should have an intermediate v4l2
> format class, that gets not exposed to applications.
> 
> It will probably be shared with v4l2-subdev, as it can be equally used
> to configure a 'struct v4l2_pix_format', a 'struct
> v4l2_pix_format_mplane' or a 'struct v4l2_mbus_framefmt', and possibly
> to instruct the v4l2 device on crop/composing too.

I think we'll end up needing different structures for V4L2 devices and
subdevs, as the respective kernel APIs are quite different. I'll have a
look at the other patches you posted and comment there.

> It will probably merit a dedicated v4l2-format.h where to collect
> those, and which can be included by other modules that requires
> interactinc with v4l2 devices and subdevices format configuration (the
> v4l2 devs themselves and reasonably pipeline handlers only right now).
> 
> >>  	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.
> 
> Actually, the number of planes and bpp of a v4l2-defined format should
> come from the kernel iteself, am I wrong?

It could make sense, but the kernel doesn't provide that information.
The kernel will round the bytesperline value properly, but if you need
the direct bpp value, you need to get it from somewhere in userspace. We
will likely need more information than the bpp or number of planes, such
as whether the format is YUV or RGB (or something else ?), compressed or
not, ... Maybe we should create a database of format information
structures.

> Should we provide an applications facing v4l2-formats, where V4L2
> FOURCCs are associated with their bpp and number of planes? Is every
> piece in userspace re-implementing this? :/

Every piece in userspace is implementing that :-/ I think it would make
sense to provide that service to applications, but not name it
v4l2-formats, it should be libcamera formats, even if we use the V4L2
4CCs.

> >> +	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*().
> 
> I considered this appropriate as this is a private API anyhow.

Given that the V4L2 API defines G_FMT and S_FMT, I think this deviation
makes sense too. Those methods are not just a class accessors, they wrap
the V4L2 API.

> >>  };
> >>
> >>  } /* 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?
> 
> As the s/mplane use case use two different members of struct
> v4l2_format.fmt union, the code gets un-necessarly complicated when I
> tried that.
> 
> > 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.
> 
> Yeah that could be given a shot to.
> 
> >> +
> >>  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;
> >>  }