[libcamera-devel] [PATCH v5 3/9] libcamera: v4l2_subdevice: Implement ENUM_FRAME_SIZES
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Feb 28 23:03:50 CET 2019
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 at 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)
> {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list