[libcamera-devel,PATCH/RFC,10/12] libcamera: v4l2_device: Add method to enumerate all discrete frame sizes

1227 diff mbox series

From: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Allow the video device to be interrogated about which discrete frame
sizes it supports.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/include/v4l2_device.h |  4 +++
 src/libcamera/v4l2_device.cpp       | 51 +++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

Hi Niklas,

Thank you for the patch.

On Sat, May 18, 2019 at 02:06:19AM +0300, Laurent Pinchart wrote:
> From: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> Allow the video device to be interrogated about which discrete frame
> sizes it supports.

The V4L2Subdevice class has a formats() method with a similar purpose. I
think we need to address them both, if not with a single API, at least
with APIs that work well together, and are based on the same design
concepts.

We should also keep in mind that the stream formats exposed to
applications (which is missing from this series), and the enumerated
formats used internally don't need to be a single class. They serve two
similar but different purposes, so I don't think they should necessarily
be identical.

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/include/v4l2_device.h |  4 +++
>  src/libcamera/v4l2_device.cpp       | 51 +++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 2e7bd1e7f4cc..481d63d9cc4c 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -8,6 +8,8 @@
>  #define __LIBCAMERA_V4L2_DEVICE_H__
>  
>  #include <atomic>
> +#include <map>
> +#include <set>
>  #include <string>
>  #include <vector>
>  
> @@ -126,6 +128,8 @@ public:
>  
>  	int getFormat(V4L2DeviceFormat *format);
>  	int setFormat(V4L2DeviceFormat *format);
> +	std::map<unsigned int, std::vector<Size>>
> +	enumerateFrameSizes(std::set<unsigned int> pixelformats);
>  
>  	int exportBuffers(BufferPool *pool);
>  	int importBuffers(BufferPool *pool);
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 8366ffc4db55..d26a89f4a27d 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -564,6 +564,57 @@ int V4L2Device::setFormatMultiplane(V4L2DeviceFormat *format)
>  	return 0;
>  }
>  
> +/**
> + * \brief Enumerate all discrete frame sizes for a set of pixel formats
> + * \param[in] pixelformats A set of pixel formats to enumerate sizes for
> + *
> + * Enumerate all discrete frame sizes reported by the video device.
> + *
> + * \return A map of pixel format to frame sizes
> + */
> +std::map<unsigned int, std::vector<Size>>
> +V4L2Device::enumerateFrameSizes(std::set<unsigned int> pixelformats)

Why do you pass a set of pixel formats instead of enumerating everything
?

> +{
> +	std::map<unsigned int, std::vector<Size>> sizes;
> +	int ret;
> +
> +	for (unsigned int pixelformat : pixelformats) {
> +		struct v4l2_frmsizeenum frameSize = {};
> +		unsigned int index = 0;
> +
> +		frameSize.pixel_format = pixelformat;
> +
> +		while (true) {

How about making this a for loop ?

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

> +			frameSize.index = index;

You should declare the frameSize variable within this loop, as it should
be fully initialised for each ioctl call.

			struct v4l2_frmsizeenum frameSize = {
				.pixel_format = pixelformat,
				.index = index,
			};

> +
> +			ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize);
> +			if (ret) {
> +				ret = -errno;
> +
> +				if (ret == -EINVAL)
> +					break;
> +
> +				if (ret != -ENOTTY)
> +					LOG(V4L2, Error)
> +						<< "Unable to enumerate frame size"
> +						<< strerror(-ret);

Why do you skip the error message when the error code is ENOTTY ?

> +
> +				return {};
> +			}
> +
> +			if (frameSize.type != V4L2_FRMSIZE_TYPE_DISCRETE)
> +				return {};

Don't we need to support ranges ?

> +
> +			sizes[pixelformat].push_back(Size(frameSize.discrete.width,
> +							  frameSize.discrete.height));
> +
> +			index++;
> +		}
> +	}
> +
> +	return sizes;
> +}
> +
>  int V4L2Device::requestBuffers(unsigned int count)
>  {
>  	struct v4l2_requestbuffers rb = {};