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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sun, Jun 16, 2019 at 03:33:55PM +0200, 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.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/include/v4l2_device.h |   5 ++
>  src/libcamera/v4l2_device.cpp       | 110 ++++++++++++++++++++++++++++
>  2 files changed, 115 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index bdecc087fe5afae0..1a67850ac4c1088f 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 {
> @@ -132,6 +133,7 @@ public:
>  
>  	int getFormat(V4L2DeviceFormat *format);
>  	int setFormat(V4L2DeviceFormat *format);
> +	ImageFormats formats();
>  
>  	int exportBuffers(BufferPool *pool);
>  	int importBuffers(BufferPool *pool);
> @@ -163,6 +165,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 0821bd75fb428766..96aa525025629697 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -634,6 +634,29 @@ int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *format)
>  	return 0;
>  }
>  
> +/**
> + * \brief Enumerate all pixel formats and frame sizes
> + *
> + * Enumerate all pixel formats and frame sizes supported by the video device.
> + *
> + * \return A list of the supported device formats
> + */
> +ImageFormats V4L2Device::formats()
> +{
> +	ImageFormats formats;
> +
> +	for (unsigned int pixelformat : enumPixelformats()) {
> +		if (formats.addFormat(pixelformat, enumSizes(pixelformat))) {
> +			LOG(V4L2, Error)
> +				<< "Could not add sizes for pixel format "
> +				<< pixelformat;
> +			return {};
> +		}

I think you should catch the error from enumSizes() and return {}. I
would also extend the documentation a little bit, you can copy the one
from the V4L2Subdevice class.

> +	}
> +
> +	return formats;
> +}
> +
>  int V4L2Device::requestBuffers(unsigned int count)
>  {
>  	struct v4l2_requestbuffers rb = {};
> @@ -763,6 +786,93 @@ int V4L2Device::createPlane(Buffer *buffer, unsigned int planeIndex,
>  	return 0;
>  }
>  
> +std::vector<unsigned int> V4L2Device::enumPixelformats()
> +{
> +	std::vector<unsigned int> formats;
> +	int ret;
> +
> +	for (unsigned int index = 0; ; index++) {
> +		struct v4l2_fmtdesc pixelformatEnum = {};
> +		pixelformatEnum.index = index;
> +		pixelformatEnum.type = bufferType_;
> +
> +		ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum);
> +		if (ret)
> +			break;
> +
> +		formats.push_back(pixelformatEnum.pixelformat);
> +	}
> +
> +	if (ret && errno != EINVAL) {
> +		ret = -errno;
> +		LOG(V4L2, Error)
> +			<< "Unable to enumerate pixelformats: "

s/pixelformats/pixel formats/

> +			<< strerror(-ret);
> +		return {};
> +	}
> +
> +	return formats;
> +}
> +
> +std::vector<SizeRange> V4L2Device::enumSizes(unsigned int pixelFormat)
> +{
> +	std::vector<SizeRange> sizes;
> +	int ret;
> +
> +	for (unsigned int index = 0;; index++) {
> +		struct v4l2_frmsizeenum frameSize = {};
> +		frameSize.index = index;
> +		frameSize.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)
> +				<< "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)
> +				<< "Unknown VIDIOC_ENUM_FRAMESIZES type: "

Nit-picking, s/://

With these small issues fixed (and trusting your documentation skills
for the formats() method),

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +				<< frameSize.type;
> +			return {};
> +		}
> +	}
> +
> +	if (ret && errno != EINVAL) {
> +		ret = -errno;
> +		LOG(V4L2, Error)
> +			<< "Unable to enumerate frame sizes: "
> +			<< strerror(-ret);
> +		return {};
> +	}
> +
> +	return sizes;
> +}
> +
>  /**
>   * \brief Import the externally allocated \a pool of buffers
>   * \param[in] pool BufferPool of buffers to import

-- 
Regards,

Laurent Pinchart