[libcamera-devel] [PATCH 1/3] libcamera: v4l2_subdevice: Implement ENUM_FRAME_SIZES

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2019-02-19 17:56:18 +0100, Jacopo Mondi wrote:
> Implement enumFormat() methods to enumerate the available image
> resolutions on the subdevice.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/include/v4l2_subdevice.h | 11 ++++
>  src/libcamera/v4l2_subdevice.cpp       | 90 ++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 82fa6685ab52..c7045776555c 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -14,6 +14,16 @@ namespace libcamera {
>  class MediaEntity;
>  struct Rectangle;
>  
> +struct V4L2SubdeviceFormatEnum {
> +	unsigned int index;
> +	uint32_t mbus_code;
> +
> +	uint32_t minWidth;
> +	uint32_t maxWidth;
> +	uint32_t minHeight;
> +	uint32_t maxHeight;
> +};
> +
>  struct V4L2SubdeviceFormat {
>  	uint32_t mbus_code;
>  	uint32_t width;
> @@ -36,6 +46,7 @@ public:
>  	int setCrop(unsigned int pad, Rectangle *rect);
>  	int setCompose(unsigned int pad, Rectangle *rect);
>  
> +	int enumFormat(unsigned int pad, V4L2SubdeviceFormatEnum *formatEnum);
>  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
>  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format);
>  
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index b436f73cc75f..5665154a2762 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -26,6 +26,52 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(V4L2Subdev)
>  
> +/**
> + * \struct V4L2SubdeviceFormatEnum
> + * \brief The V4L2 sub-device image size enumeration
> + *
> + * This structure describes an image resolution as enumerated by the V4L2
> + * sub-device. The structure is used as format exchange between the caller and
> + * the enumFormat() method. The caller is responsible to fill the media bus
> + * code to use and the index of the format to be enumerated.
> + */
> +
> +/**
> + * \var V4L2SubdeviceFormatEnum::index
> + * \brief The index of the format to be enumerated
> + */
> +
> +/**
> + * \var V4L2SubdeviceFormatEnum::mbus_code
> + * \brief The pixel format identification code for the formats to enumerate
> + *
> + * \sa V4L2SubdeviceFormat for media bus format pixel code description.
> + */
> +
> +/**
> + * \var V4L2SubdeviceFormatEnum::minWidth
> + * \brief The minimum width of the enumerated format as reported by the V4L2
> + * sub-device
> + */
> +
> +/**
> + * \var V4L2SubdeviceFormatEnum::maxWidth
> + * \brief The maximum width of the enumerated format as reported by the V4L2
> + * sub-device
> + */
> +
> +/**
> + * \var V4L2SubdeviceFormatEnum::minHeight
> + * \brief The minimum height of the enumerated format as reported by the V4L2
> + * sub-device
> + */
> +
> +/**
> + * \var V4L2SubdeviceFormatEnum::maxHeight
> + * \brief The maximum height of the enumerated format as reported by the V4L2
> + * sub-device
> + */
> +
>  /**
>   * \struct V4L2SubdeviceFormat
>   * \brief The V4L2 sub-device image format and sizes
> @@ -171,6 +217,50 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
>  	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
>  }
>  
> +/**
> + * \brief Enumerate the sub-device image resolutions
> + * \param[in] pad The 0-indexed pad number to enumerate formats on
> + * \param[inout] formatEnum The format enumeration description
> + *
> + * The method retrieve the image resolution of the image format with index
> + * formatEnum.index for the pixel format specified by formatEnum.mbus_code.
> + *
> + * To enumerate image resolutions, the caller shall start enumerating all
> + * formats by setting the formatEnum.index field to 0, and increasing it by
> + * one for any successive call, until the -EINVAL return code is returned.

Should we not help pipeline handlers by hiding this? And instead return 
a vector of V4L2SubdeviceFormatEnum with all formats?

> + *
> + * \return 0 on success, or a negative error code otherwise
> + */
> +int V4L2Subdevice::enumFormat(unsigned int pad,
> +			      V4L2SubdeviceFormatEnum *formatEnum)
> +{
> +	struct v4l2_subdev_frame_size_enum sizeEnum = {};
> +
> +	sizeEnum.index = formatEnum->index;
> +	sizeEnum.code = formatEnum->mbus_code;
> +	sizeEnum.pad = pad;

This is confusing, index and code comes from formatEnum while pad is a 
separate argument.

> +	sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;

I think this should be configurable by the caller or other way 
customisable. In the not so distant future we will need to add support 
for V4L2_SUBDEV_FORMAT_TRY in a lot of places better to prepare for it 
:-)

> +
> +	int ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> +	if (ret) {
> +		ret = -errno;
> +		if (ret == -EINVAL)
> +			return ret;
> +
> +		LOG(V4L2Subdev, Error)
> +			<< "Unable to enumerate format on pad " << pad
> +			<< " of " << deviceNode_ << ": " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	formatEnum->maxWidth = sizeEnum.max_width;
> +	formatEnum->minWidth = sizeEnum.min_width;
> +	formatEnum->maxHeight = sizeEnum.max_height;
> +	formatEnum->minHeight = sizeEnum.min_height;
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Retrieve the image format set on one of the V4L2 subdevice pads
>   * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund