[libcamera-devel] [PATCH 09/17] libcamera: v4l2_device: Add enumeration of pixelformats and frame sizes

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wed, May 29, 2019 at 10:53:27PM +0100, Kieran Bingham wrote:
> On 27/05/2019 01:15, Niklas Söderlund wrote:
> > Add methods to enumerate pixelformats and frame sizes from a v4l2
> > device. The interface is similar to how V4L2Subdevice enumerates mbus
> > codes and frame sizes.
> 
> Could we add a test or two to test/v4l2_device/formats.cpp for this
> public API addition?
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/include/v4l2_device.h |   5 ++
> >  src/libcamera/v4l2_device.cpp       | 104 ++++++++++++++++++++++++++++
> >  2 files changed, 109 insertions(+)
> > 
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 2e7bd1e7f4cce276..db2d12757c6f564a 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -16,6 +16,7 @@
> >  #include <libcamera/geometry.h>
> >  #include <libcamera/signal.h>
> >  
> > +#include "formats.h"
> >  #include "log.h"
> >  
> >  namespace libcamera {
> > @@ -126,6 +127,7 @@ public:
> >  
> >  	int getFormat(V4L2DeviceFormat *format);
> >  	int setFormat(V4L2DeviceFormat *format);
> > +	V4L2DeviceFormats formats();
> >  
> >  	int exportBuffers(BufferPool *pool);
> >  	int importBuffers(BufferPool *pool);
> > @@ -154,6 +156,9 @@ private:
> >  	int createPlane(Buffer *buffer, unsigned int plane,
> >  			unsigned int length);
> >  
> > +	std::vector<unsigned int> enumPixelformats();
> > +	std::vector<SizeRange> enumSizes(unsigned int pixelformat);
> > +
> >  	Buffer *dequeueBuffer();
> >  	void bufferAvailable(EventNotifier *notifier);
> >  
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 8366ffc4db559520..a9667092a20505d9 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -564,6 +564,23 @@ int V4L2Device::setFormatMultiplane(V4L2DeviceFormat *format)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * \brief Enumerate all pixelformats and frame sizes

s/pixelformats/pixel formats/ (same below)

> > + *
> > + * Enumerate all pixelformats and frame sizes reported by the video device.

"reported" or "supported" ?

> > + *
> > + * \return All pixelformats and frame sizes
> 
> How about:
> 
>  \return A list of the supported device formats
> 
> <except s/list/some-other-type-description/ >
> 
> > + */
> > +V4L2DeviceFormats V4L2Device::formats()
> 
> Eeep, we have a V4L2DeviceFormat class and a V4L2DeviceFormats class
> which do (somewhat) different things ...
> 
> I hope that won't be too confusing... <I've gone back and noted this in
> reply to the patch that adds the types.>

It can be a bit confusing indeed, but so far I think this is the best
solution I've seen. If anyone can think of a better option, please say
so. We can always change it later anyway.

> > +{
> > +	std::map<unsigned int, std::vector<SizeRange>> formatMap = {};
> > +
> > +	for (unsigned int pixelformat : enumPixelformats())
> > +		formatMap[pixelformat] = enumSizes(pixelformat);
> > +
> > +	return V4L2DeviceFormats(formatMap);
> > +}
> > +
> >  int V4L2Device::requestBuffers(unsigned int count)
> >  {
> >  	struct v4l2_requestbuffers rb = {};
> > @@ -692,6 +709,93 @@ int V4L2Device::createPlane(Buffer *buffer, unsigned int planeIndex,
> >  
> >  	return 0;
> >  }
> 
> newline required here.
> 
> > +std::vector<unsigned int> V4L2Device::enumPixelformats()
> > +{
> > +	std::vector<unsigned int> pixelformats;

s/pixelformats/pixelFormats/ (or just formats) ?

> > +	int ret;
> > +
> > +	for (unsigned int index = 0;; index++) {

s/;;/; ;/

> > +		struct v4l2_fmtdesc pixelformatEnum = {
> > +			.index = index,
> > +			.type = bufferType_,
> > +		};
> > +
> > +		ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum);
> > +		if (ret)
> > +			break;
> > +
> > +		pixelformats.push_back(pixelformatEnum.pixelformat);
> > +	}
> > +
> > +	if (ret && errno != EINVAL) {
> > +		ret = errno;
> > +		LOG(V4L2, Error)
> > +			<< "Unable to enumerate pixelformats: "
> > +			<< strerror(ret);
> > +		return {};
> > +	}
> > +
> > +	return pixelformats;
> > +}
> > +
> > +std::vector<SizeRange> V4L2Device::enumSizes(unsigned int pixelformat)

s/pixelformat/pixelFormat/ ?

> > +{
> > +	std::vector<SizeRange> sizes;
> > +	int ret;
> > +
> > +	for (unsigned int index = 0;; index++) {
> > +		struct v4l2_frmsizeenum frameSize = {
> > +			.index = index,
> > +			.pixel_format = pixelformat,
> > +		};
> > +
> > +		ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > +		if (ret)
> > +			break;
> > +
> > +		if (index != 0 &&
> > +		    frameSize.type != V4L2_FRMSIZE_TYPE_DISCRETE) {
> > +			LOG(V4L2, Error)
> > +				<< "None 0 index for none discrete type";
> 
> Non-zero index for non discrete type
> 
> > +			return {};
> > +		}
> > +
> > +		switch (frameSize.type) {
> > +		case V4L2_FRMSIZE_TYPE_DISCRETE:
> > +			sizes.emplace_back(frameSize.discrete.width,
> > +					   frameSize.discrete.height);
> > +			break;
> > +		case V4L2_FRMSIZE_TYPE_CONTINUOUS:
> > +			sizes.emplace_back(frameSize.stepwise.min_width,
> > +					   frameSize.stepwise.min_height,
> > +					   frameSize.stepwise.max_width,
> > +					   frameSize.stepwise.max_height);
> > +			break;
> > +		case V4L2_FRMSIZE_TYPE_STEPWISE:
> > +			sizes.emplace_back(frameSize.stepwise.min_width,
> > +					   frameSize.stepwise.min_height,
> > +					   frameSize.stepwise.max_width,
> > +					   frameSize.stepwise.max_height,
> > +					   frameSize.stepwise.step_width,
> > +					   frameSize.stepwise.step_height);
> > +			break;
> > +		default:
> > +			LOG(V4L2, Error)
> > +				<< "Unkown VIDIOC_ENUM_FRAMESIZES type "
> 
> s/Unkown/Unknown/
> 
> > +				<< frameSize.type;
> > +			return {};
> > +		}
> > +	}
> > +
> > +	if (ret && errno != EINVAL) {
> > +		ret = errno;
> > +		LOG(V4L2, Error)
> > +			<< "Unable to enumerate frame sizes: " << strerror(ret);

While it makes no big difference as we then don't do anything with ret,
we usually assign ret with -errno and use strerror(-ret). It may make
sense to use this construct here as well, to avoid possible bugs in case
we later change the function to return ret. I won't push much though,
it's up to you.

> > +		return {};
> > +	}
> > +
> > +	return sizes;
> > +}
> >  
> >  /**
> >   * \brief Import the externally allocated \a pool of buffers

-- 
Regards,

Laurent Pinchart