[libcamera-devel,v5,3/9] libcamera: v4l2_subdevice: Implement ENUM_FRAME_SIZES

671 diff mbox series

Implement enumFormat() methods to enumerate the available image
resolutions on the subdevice.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/geometry.cpp             | 34 +++++++++++
 src/libcamera/include/geometry.h       | 12 ++++
 src/libcamera/include/v4l2_subdevice.h | 11 +++-
 src/libcamera/v4l2_subdevice.cpp       | 84 ++++++++++++++++++++++++++
 4 files changed, 139 insertions(+), 2 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Thu, Feb 28, 2019 at 09:01:45PM +0100, Jacopo Mondi wrote:
> Implement enumFormat() methods to enumerate the available image
> resolutions on the subdevice.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/geometry.cpp             | 34 +++++++++++
>  src/libcamera/include/geometry.h       | 12 ++++
>  src/libcamera/include/v4l2_subdevice.h | 11 +++-
>  src/libcamera/v4l2_subdevice.cpp       | 84 ++++++++++++++++++++++++++
>  4 files changed, 139 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index 57f4fc7716d9..f41f6975d4ce 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -46,4 +46,38 @@ namespace libcamera {
>   * \brief The distance between the top and bottom sides
>   */
>  
> +/**
> + * \struct SizeRange
> + * \brief Describe a range of image sizes
> + *
> + * SizeRange describes a range of image sizes included in the (maxWidth,
> + * maxHeight) - (minWidth, minHeight) interval. If the minimum and

Could you swap min and max ? Intervals are usually represented with
minimum first.

> + * maximum sizes are identical it represents a single image resolution.
> + */
> +
> +/**
> + * \fn SizeRange::SizeRange()
> + * \brief Construct a size range
> + */
> +
> +/**
> + * \var SizeRange::minWidth
> + * \brief The minimum image width
> + */
> +
> +/**
> + * \var SizeRange::minHeight
> + * \brief The minimum image height
> + */
> +
> +/**
> + * \var SizeRange::maxWidth
> + * \brief The maximum image width
> + */
> +
> +/**
> + * \var SizeRange::maxHeight
> + * \brief The maximum image height
> + */
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/geometry.h b/src/libcamera/include/geometry.h
> index cc146da7cb0d..eadc4ed4f9cb 100644
> --- a/src/libcamera/include/geometry.h
> +++ b/src/libcamera/include/geometry.h
> @@ -17,6 +17,18 @@ struct Rectangle {
>  	unsigned int h;
>  };
>  
> +struct SizeRange {
> +	SizeRange(unsigned int minW, unsigned int minH,
> +		  unsigned int maxW, unsigned int maxH)
> +		: minWidth(minW), minHeight(minH), maxWidth(maxW),
> +		  maxHeight(maxH) {}

Do we need a constructor ?

> +
> +	unsigned int minWidth;
> +	unsigned int minHeight;
> +	unsigned int maxWidth;
> +	unsigned int maxHeight;
> +};
> +
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_GEOMETRY_H__ */
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 669e79f9a9fd..aa7451e86962 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -7,15 +7,16 @@
>  #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__
>  #define __LIBCAMERA_V4L2_SUBDEVICE_H__
>  
> +#include <map>
>  #include <string>
> +#include <vector>
>  
> +#include "geometry.h"
>  #include "log.h"
>  #include "media_object.h"
>  
>  namespace libcamera {
>  
> -struct Rectangle;
> -
>  struct V4L2SubdeviceFormat {
>  	uint32_t mbus_code;
>  	uint32_t width;
> @@ -39,6 +40,9 @@ public:
>  	int setCrop(unsigned int pad, Rectangle *rect);
>  	int setCompose(unsigned int pad, Rectangle *rect);
>  
> +	const std::map<unsigned int, std::vector<SizeRange>>
> +						formats(unsigned int pad);
> +
>  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
>  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format);
>  
> @@ -46,6 +50,9 @@ protected:
>  	std::string logPrefix() const;
>  
>  private:
> +	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> +					    unsigned int code);
> +
>  	int setSelection(unsigned int pad, unsigned int target,
>  			 Rectangle *rect);
>  
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 4fe59c45f000..7f191e072c61 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -183,6 +183,58 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
>  	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
>  }
>  
> +/**
> + * \brief List the sub-device image resolutions and formats on \a pad
> + * \param[in] pad The 0-indexed pad number to enumerate formats on
> + *
> + * Retrieve a list of image formats and sizes on the \a pad of a video
> + * subdevice.  Subdevices are free to report a list of discrete sizes they

s/  Subdevices are free to report/ Subdevices can report either/

> + * support, or a single size interval expressed as a [min-max] sizes range.

Can't subdevices report a list of intervals ?

> + *
> + * Each image size list is associated with a media bus pixel code for which
> + * the reported resolutions are supported.
> + *
> + * \return A, map of image formats associated with a list of image sizes, or

s/A, /A /

> + * an empty map on error or if the pad does not exists

s/exists/exist/

> + * \sa SizeRange

I don't think you need this, doesn't Doxygen link to SizeRange as it is
used in the return type.

> + */
> +const std::map<unsigned int, std::vector<SizeRange>>
> +V4L2Subdevice::formats(unsigned int pad)
> +{
> +	std::map<unsigned int, std::vector<SizeRange>> formatMap = {};
> +	struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> +	int ret;
> +
> +	if (pad > entity_->pads().size()) {

Shouldn't this be >= ?

> +		LOG(V4L2Subdev, Error)
> +			<< "Format enumeration required on a non-existing pad: "

Did you mean "requested" ? How about just "Invalid pad " << pad (as the
log message will contain the function name already) ?

> +			<< pad;
> +		return formatMap;
> +	}
> +
> +	mbusEnum.pad = pad;
> +	mbusEnum.index = 0;
> +	mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	while (true) {
> +		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> +		if (ret)
> +			break;
> +
> +		formatMap[mbusEnum.code] = enumPadSizes(pad, mbusEnum.code);
> +
> +		mbusEnum.index++;
> +	}
> +
> +	if (ret && (errno != EINVAL && errno != ENOTTY)) {

		ret = -errnor;

and use strerror(-ret).

> +		LOG(V4L2Subdev, Error)
> +			<< "Unable to enumerate format on pad " << pad

s/format/formats/

> +			<< ": " << strerror(errno);
> +		formatMap.clear();
> +	}
> +
> +	return formatMap;
> +}
> +
>  /**
>   * \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
> @@ -248,6 +300,38 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
>  	return 0;
>  }
>  
> +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> +						   unsigned int code)
> +{
> +	struct v4l2_subdev_frame_size_enum sizeEnum = {};
> +	std::vector<SizeRange> sizes = {};
> +	int ret;
> +
> +	sizeEnum.index = 0;
> +	sizeEnum.pad = pad;
> +	sizeEnum.code = code;
> +	sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	while (true) {
> +		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> +		if (ret)
> +			break;
> +
> +		sizes.emplace_back(sizeEnum.min_width, sizeEnum.min_height,
> +				   sizeEnum.max_width, sizeEnum.max_height);

If you used

		sizes.emplace_back({ sizeEnum.min_width, sizeEnum.min_height,
				     sizeEnum.max_width, sizeEnum.max_height });

I think you could remove the SizeRange constructor.

> +
> +		sizeEnum.index++;
> +	}
> +
> +	if (ret && (errno != EINVAL && errno != ENOTTY)) {

Same errno dance here.

> +		LOG(V4L2Subdev, Error)
> +			<< "Unable to enumerate sizes on pad " << pad
> +			<< ": " << strerror(errno);
> +		sizes.clear();
> +	}
> +
> +	return sizes;
> +}
> +
>  int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
>  				Rectangle *rect)
>  {

Hi Laurent,
   thanks for your comments

On Fri, Mar 01, 2019 at 12:03:50AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Feb 28, 2019 at 09:01:45PM +0100, Jacopo Mondi wrote:
> > Implement enumFormat() methods to enumerate the available image
> > resolutions on the subdevice.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/geometry.cpp             | 34 +++++++++++
> >  src/libcamera/include/geometry.h       | 12 ++++
> >  src/libcamera/include/v4l2_subdevice.h | 11 +++-
> >  src/libcamera/v4l2_subdevice.cpp       | 84 ++++++++++++++++++++++++++
> >  4 files changed, 139 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index 57f4fc7716d9..f41f6975d4ce 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -46,4 +46,38 @@ namespace libcamera {
> >   * \brief The distance between the top and bottom sides
> >   */
> >
> > +/**
> > + * \struct SizeRange
> > + * \brief Describe a range of image sizes
> > + *
> > + * SizeRange describes a range of image sizes included in the (maxWidth,
> > + * maxHeight) - (minWidth, minHeight) interval. If the minimum and
>
> Could you swap min and max ? Intervals are usually represented with
> minimum first.

Yes, I initially had it like this, then I swapped it to (max - min) as
it would represent a differnce between two sizes, but I agree it is
more canonically seen with min first.

>
> > + * maximum sizes are identical it represents a single image resolution.
> > + */
> > +
> > +/**
> > + * \fn SizeRange::SizeRange()
> > + * \brief Construct a size range
> > + */
> > +
> > +/**
> > + * \var SizeRange::minWidth
> > + * \brief The minimum image width
> > + */
> > +
> > +/**
> > + * \var SizeRange::minHeight
> > + * \brief The minimum image height
> > + */
> > +
> > +/**
> > + * \var SizeRange::maxWidth
> > + * \brief The maximum image width
> > + */
> > +
> > +/**
> > + * \var SizeRange::maxHeight
> > + * \brief The maximum image height
> > + */
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/include/geometry.h b/src/libcamera/include/geometry.h
> > index cc146da7cb0d..eadc4ed4f9cb 100644
> > --- a/src/libcamera/include/geometry.h
> > +++ b/src/libcamera/include/geometry.h
> > @@ -17,6 +17,18 @@ struct Rectangle {
> >  	unsigned int h;
> >  };
> >
> > +struct SizeRange {
> > +	SizeRange(unsigned int minW, unsigned int minH,
> > +		  unsigned int maxW, unsigned int maxH)
> > +		: minWidth(minW), minHeight(minH), maxWidth(maxW),
> > +		  maxHeight(maxH) {}
>
> Do we need a constructor ?
>

If I use emplace_back like I did yes. With your below suggestion
probably not.

> > +
> > +	unsigned int minWidth;
> > +	unsigned int minHeight;
> > +	unsigned int maxWidth;
> > +	unsigned int maxHeight;
> > +};
> > +
> >  } /* namespace libcamera */
> >
> >  #endif /* __LIBCAMERA_GEOMETRY_H__ */
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index 669e79f9a9fd..aa7451e86962 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -7,15 +7,16 @@
> >  #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__
> >  #define __LIBCAMERA_V4L2_SUBDEVICE_H__
> >
> > +#include <map>
> >  #include <string>
> > +#include <vector>
> >
> > +#include "geometry.h"
> >  #include "log.h"
> >  #include "media_object.h"
> >
> >  namespace libcamera {
> >
> > -struct Rectangle;
> > -
> >  struct V4L2SubdeviceFormat {
> >  	uint32_t mbus_code;
> >  	uint32_t width;
> > @@ -39,6 +40,9 @@ public:
> >  	int setCrop(unsigned int pad, Rectangle *rect);
> >  	int setCompose(unsigned int pad, Rectangle *rect);
> >
> > +	const std::map<unsigned int, std::vector<SizeRange>>
> > +						formats(unsigned int pad);
> > +
> >  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> >  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> >
> > @@ -46,6 +50,9 @@ protected:
> >  	std::string logPrefix() const;
> >
> >  private:
> > +	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > +					    unsigned int code);
> > +
> >  	int setSelection(unsigned int pad, unsigned int target,
> >  			 Rectangle *rect);
> >
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 4fe59c45f000..7f191e072c61 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -183,6 +183,58 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> >  	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> >  }
> >
> > +/**
> > + * \brief List the sub-device image resolutions and formats on \a pad
> > + * \param[in] pad The 0-indexed pad number to enumerate formats on
> > + *
> > + * Retrieve a list of image formats and sizes on the \a pad of a video
> > + * subdevice.  Subdevices are free to report a list of discrete sizes they
>
> s/  Subdevices are free to report/ Subdevices can report either/
>
> > + * support, or a single size interval expressed as a [min-max] sizes range.
>
> Can't subdevices report a list of intervals ?
>

Possibly, yes, I'll change this.

> > + *
> > + * Each image size list is associated with a media bus pixel code for which
> > + * the reported resolutions are supported.
> > + *
> > + * \return A, map of image formats associated with a list of image sizes, or
>
> s/A, /A /
>
> > + * an empty map on error or if the pad does not exists
>
> s/exists/exist/
>
> > + * \sa SizeRange
>
> I don't think you need this, doesn't Doxygen link to SizeRange as it is
> used in the return type.
>
> > + */
> > +const std::map<unsigned int, std::vector<SizeRange>>
> > +V4L2Subdevice::formats(unsigned int pad)
> > +{
> > +	std::map<unsigned int, std::vector<SizeRange>> formatMap = {};
> > +	struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> > +	int ret;
> > +
> > +	if (pad > entity_->pads().size()) {
>
> Shouldn't this be >= ?
>

Ups, sorry, I had a different check here and forgot to update this.


> > +		LOG(V4L2Subdev, Error)
> > +			<< "Format enumeration required on a non-existing pad: "
>
> Did you mean "requested" ? How about just "Invalid pad " << pad (as the
> log message will contain the function name already) ?
>
> > +			<< pad;
> > +		return formatMap;
> > +	}
> > +
> > +	mbusEnum.pad = pad;
> > +	mbusEnum.index = 0;
> > +	mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	while (true) {
> > +		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > +		if (ret)
> > +			break;
> > +
> > +		formatMap[mbusEnum.code] = enumPadSizes(pad, mbusEnum.code);
> > +
> > +		mbusEnum.index++;
> > +	}
> > +
> > +	if (ret && (errno != EINVAL && errno != ENOTTY)) {
>
> 		ret = -errnor;
>
> and use strerror(-ret).
>
> > +		LOG(V4L2Subdev, Error)
> > +			<< "Unable to enumerate format on pad " << pad
>
> s/format/formats/
>
> > +			<< ": " << strerror(errno);
> > +		formatMap.clear();
> > +	}
> > +
> > +	return formatMap;
> > +}
> > +
> >  /**
> >   * \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
> > @@ -248,6 +300,38 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> >  	return 0;
> >  }
> >
> > +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > +						   unsigned int code)
> > +{
> > +	struct v4l2_subdev_frame_size_enum sizeEnum = {};
> > +	std::vector<SizeRange> sizes = {};
> > +	int ret;
> > +
> > +	sizeEnum.index = 0;
> > +	sizeEnum.pad = pad;
> > +	sizeEnum.code = code;
> > +	sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	while (true) {
> > +		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > +		if (ret)
> > +			break;
> > +
> > +		sizes.emplace_back(sizeEnum.min_width, sizeEnum.min_height,
> > +				   sizeEnum.max_width, sizeEnum.max_height);
>
> If you used
>
> 		sizes.emplace_back({ sizeEnum.min_width, sizeEnum.min_height,
> 				     sizeEnum.max_width, sizeEnum.max_height });
>
> I think you could remove the SizeRange constructor.

I'll try, but are you sure you're not constructing a SizeRange item
and copying it in the vector in this way, instead of letting the
emplace_back method construct it?

>
> > +
> > +		sizeEnum.index++;
> > +	}
> > +
> > +	if (ret && (errno != EINVAL && errno != ENOTTY)) {
>
> Same errno dance here.

Yes, thank you.

Once clarified the emplace_back() thing, I'll re-submit.

>
> > +		LOG(V4L2Subdev, Error)
> > +			<< "Unable to enumerate sizes on pad " << pad
> > +			<< ": " << strerror(errno);
> > +		sizes.clear();
> > +	}
> > +
> > +	return sizes;
> > +}
> > +
> >  int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> >  				Rectangle *rect)
> >  {
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Fri, Mar 01, 2019 at 10:16:24AM +0100, Jacopo Mondi wrote:
> On Fri, Mar 01, 2019 at 12:03:50AM +0200, Laurent Pinchart wrote:
> > On Thu, Feb 28, 2019 at 09:01:45PM +0100, Jacopo Mondi wrote:
> >> Implement enumFormat() methods to enumerate the available image
> >> resolutions on the subdevice.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  src/libcamera/geometry.cpp             | 34 +++++++++++
> >>  src/libcamera/include/geometry.h       | 12 ++++
> >>  src/libcamera/include/v4l2_subdevice.h | 11 +++-
> >>  src/libcamera/v4l2_subdevice.cpp       | 84 ++++++++++++++++++++++++++
> >>  4 files changed, 139 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> >> index 57f4fc7716d9..f41f6975d4ce 100644
> >> --- a/src/libcamera/geometry.cpp
> >> +++ b/src/libcamera/geometry.cpp
> >> @@ -46,4 +46,38 @@ namespace libcamera {
> >>   * \brief The distance between the top and bottom sides
> >>   */
> >>
> >> +/**
> >> + * \struct SizeRange
> >> + * \brief Describe a range of image sizes
> >> + *
> >> + * SizeRange describes a range of image sizes included in the (maxWidth,
> >> + * maxHeight) - (minWidth, minHeight) interval. If the minimum and
> >
> > Could you swap min and max ? Intervals are usually represented with
> > minimum first.
> 
> Yes, I initially had it like this, then I swapped it to (max - min) as
> it would represent a differnce between two sizes, but I agree it is
> more canonically seen with min first.
> 
> >> + * maximum sizes are identical it represents a single image resolution.
> >> + */
> >> +
> >> +/**
> >> + * \fn SizeRange::SizeRange()
> >> + * \brief Construct a size range
> >> + */
> >> +
> >> +/**
> >> + * \var SizeRange::minWidth
> >> + * \brief The minimum image width
> >> + */
> >> +
> >> +/**
> >> + * \var SizeRange::minHeight
> >> + * \brief The minimum image height
> >> + */
> >> +
> >> +/**
> >> + * \var SizeRange::maxWidth
> >> + * \brief The maximum image width
> >> + */
> >> +
> >> +/**
> >> + * \var SizeRange::maxHeight
> >> + * \brief The maximum image height
> >> + */
> >> +
> >>  } /* namespace libcamera */
> >> diff --git a/src/libcamera/include/geometry.h b/src/libcamera/include/geometry.h
> >> index cc146da7cb0d..eadc4ed4f9cb 100644
> >> --- a/src/libcamera/include/geometry.h
> >> +++ b/src/libcamera/include/geometry.h
> >> @@ -17,6 +17,18 @@ struct Rectangle {
> >>  	unsigned int h;
> >>  };
> >>
> >> +struct SizeRange {
> >> +	SizeRange(unsigned int minW, unsigned int minH,
> >> +		  unsigned int maxW, unsigned int maxH)
> >> +		: minWidth(minW), minHeight(minH), maxWidth(maxW),
> >> +		  maxHeight(maxH) {}
> >
> > Do we need a constructor ?
> 
> If I use emplace_back like I did yes. With your below suggestion
> probably not.
> 
> >> +
> >> +	unsigned int minWidth;
> >> +	unsigned int minHeight;
> >> +	unsigned int maxWidth;
> >> +	unsigned int maxHeight;
> >> +};
> >> +
> >>  } /* namespace libcamera */
> >>
> >>  #endif /* __LIBCAMERA_GEOMETRY_H__ */
> >> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> >> index 669e79f9a9fd..aa7451e86962 100644
> >> --- a/src/libcamera/include/v4l2_subdevice.h
> >> +++ b/src/libcamera/include/v4l2_subdevice.h
> >> @@ -7,15 +7,16 @@
> >>  #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__
> >>  #define __LIBCAMERA_V4L2_SUBDEVICE_H__
> >>
> >> +#include <map>
> >>  #include <string>
> >> +#include <vector>
> >>
> >> +#include "geometry.h"
> >>  #include "log.h"
> >>  #include "media_object.h"
> >>
> >>  namespace libcamera {
> >>
> >> -struct Rectangle;
> >> -
> >>  struct V4L2SubdeviceFormat {
> >>  	uint32_t mbus_code;
> >>  	uint32_t width;
> >> @@ -39,6 +40,9 @@ public:
> >>  	int setCrop(unsigned int pad, Rectangle *rect);
> >>  	int setCompose(unsigned int pad, Rectangle *rect);
> >>
> >> +	const std::map<unsigned int, std::vector<SizeRange>>
> >> +						formats(unsigned int pad);
> >> +
> >>  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> >>  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> >>
> >> @@ -46,6 +50,9 @@ protected:
> >>  	std::string logPrefix() const;
> >>
> >>  private:
> >> +	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> >> +					    unsigned int code);
> >> +
> >>  	int setSelection(unsigned int pad, unsigned int target,
> >>  			 Rectangle *rect);
> >>
> >> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> >> index 4fe59c45f000..7f191e072c61 100644
> >> --- a/src/libcamera/v4l2_subdevice.cpp
> >> +++ b/src/libcamera/v4l2_subdevice.cpp
> >> @@ -183,6 +183,58 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> >>  	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> >>  }
> >>
> >> +/**
> >> + * \brief List the sub-device image resolutions and formats on \a pad
> >> + * \param[in] pad The 0-indexed pad number to enumerate formats on
> >> + *
> >> + * Retrieve a list of image formats and sizes on the \a pad of a video
> >> + * subdevice.  Subdevices are free to report a list of discrete sizes they
> >
> > s/  Subdevices are free to report/ Subdevices can report either/
> >
> >> + * support, or a single size interval expressed as a [min-max] sizes range.
> >
> > Can't subdevices report a list of intervals ?
> >
> 
> Possibly, yes, I'll change this.
> 
> >> + *
> >> + * Each image size list is associated with a media bus pixel code for which
> >> + * the reported resolutions are supported.
> >> + *
> >> + * \return A, map of image formats associated with a list of image sizes, or
> >
> > s/A, /A /
> >
> >> + * an empty map on error or if the pad does not exists
> >
> > s/exists/exist/
> >
> >> + * \sa SizeRange
> >
> > I don't think you need this, doesn't Doxygen link to SizeRange as it is
> > used in the return type.
> >
> >> + */
> >> +const std::map<unsigned int, std::vector<SizeRange>>
> >> +V4L2Subdevice::formats(unsigned int pad)
> >> +{
> >> +	std::map<unsigned int, std::vector<SizeRange>> formatMap = {};
> >> +	struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> >> +	int ret;
> >> +
> >> +	if (pad > entity_->pads().size()) {
> >
> > Shouldn't this be >= ?
> >
> 
> Ups, sorry, I had a different check here and forgot to update this.
> 
> >> +		LOG(V4L2Subdev, Error)
> >> +			<< "Format enumeration required on a non-existing pad: "
> >
> > Did you mean "requested" ? How about just "Invalid pad " << pad (as the
> > log message will contain the function name already) ?
> >
> >> +			<< pad;
> >> +		return formatMap;
> >> +	}
> >> +
> >> +	mbusEnum.pad = pad;
> >> +	mbusEnum.index = 0;
> >> +	mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >> +	while (true) {
> >> +		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> >> +		if (ret)
> >> +			break;
> >> +
> >> +		formatMap[mbusEnum.code] = enumPadSizes(pad, mbusEnum.code);
> >> +
> >> +		mbusEnum.index++;
> >> +	}
> >> +
> >> +	if (ret && (errno != EINVAL && errno != ENOTTY)) {
> >
> > 		ret = -errnor;
> >
> > and use strerror(-ret).
> >
> >> +		LOG(V4L2Subdev, Error)
> >> +			<< "Unable to enumerate format on pad " << pad
> >
> > s/format/formats/
> >
> >> +			<< ": " << strerror(errno);
> >> +		formatMap.clear();
> >> +	}
> >> +
> >> +	return formatMap;
> >> +}
> >> +
> >>  /**
> >>   * \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
> >> @@ -248,6 +300,38 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> >>  	return 0;
> >>  }
> >>
> >> +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> >> +						   unsigned int code)
> >> +{
> >> +	struct v4l2_subdev_frame_size_enum sizeEnum = {};
> >> +	std::vector<SizeRange> sizes = {};
> >> +	int ret;
> >> +
> >> +	sizeEnum.index = 0;
> >> +	sizeEnum.pad = pad;
> >> +	sizeEnum.code = code;
> >> +	sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >> +	while (true) {
> >> +		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> >> +		if (ret)
> >> +			break;
> >> +
> >> +		sizes.emplace_back(sizeEnum.min_width, sizeEnum.min_height,
> >> +				   sizeEnum.max_width, sizeEnum.max_height);
> >
> > If you used
> >
> > 		sizes.emplace_back({ sizeEnum.min_width, sizeEnum.min_height,
> > 				     sizeEnum.max_width, sizeEnum.max_height });
> >
> > I think you could remove the SizeRange constructor.
> 
> I'll try, but are you sure you're not constructing a SizeRange item
> and copying it in the vector in this way, instead of letting the
> emplace_back method construct it?

You're right, it doesn't work. Let's keep the constructor then.

> >> +
> >> +		sizeEnum.index++;
> >> +	}
> >> +
> >> +	if (ret && (errno != EINVAL && errno != ENOTTY)) {
> >
> > Same errno dance here.
> 
> Yes, thank you.
> 
> Once clarified the emplace_back() thing, I'll re-submit.
> 
> >> +		LOG(V4L2Subdev, Error)
> >> +			<< "Unable to enumerate sizes on pad " << pad
> >> +			<< ": " << strerror(errno);
> >> +		sizes.clear();
> >> +	}
> >> +
> >> +	return sizes;
> >> +}
> >> +
> >>  int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> >>  				Rectangle *rect)
> >>  {