| Message ID | 20190219165620.2385-2-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Tue, Feb 19, 2019 at 05:56:18PM +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/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. > + * > + * \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; > + sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + > + 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; > + } Let's not repeat the mistake of the V4L2 API that requires calling the same ioctl in a loop. Instead of just wrapping the ioctl, let's make this function more useful by enumerating all supported formats and returning a list. You may even want to have two nested loops, one over formats, and the other over sizes. > + > + 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
Hi Niklas, On Thu, Feb 21, 2019 at 04:31:56PM +0100, Niklas Söderlund wrote: > 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@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? > Yes, you and Laurent had the same suggestion and is surely better. I'll re-implement this. > > + * > > + * \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. > All V4L2Subdevice methods accepts a pad as first argument. I would keep this consistent. > > + 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 > :-) I'm really not sure about this one, I could add a field to V4L2SubdeviceFormatEnum for this... > > > + > > + 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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
On Mon, Feb 25, 2019 at 10:28:33AM +0100, Jacopo Mondi wrote: > Hi Niklas, > > On Thu, Feb 21, 2019 at 04:31:56PM +0100, Niklas Söderlund wrote: > > 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@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? > > > > Yes, you and Laurent had the same suggestion and is surely better. > I'll re-implement this. > > > > + * > > > + * \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. > > > > All V4L2Subdevice methods accepts a pad as first argument. I would > keep this consistent. > > > > + 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 > > :-) > > I'm really not sure about this one, I could add a field to > V4L2SubdeviceFormatEnum for this... > Re-thinking about this, I would leave this out until we don't define a way to handle FORMAT_TRY globally, as otherwise I would have to expose that flag outside of V4L2Subdevice which I would prefer not to... > > > > > + > > > + 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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Mon, Feb 25, 2019 at 10:44:35AM +0100, Jacopo Mondi wrote: > On Mon, Feb 25, 2019 at 10:28:33AM +0100, Jacopo Mondi wrote: > > On Thu, Feb 21, 2019 at 04:31:56PM +0100, Niklas Söderlund wrote: > >> 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@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? > >> > > > > Yes, you and Laurent had the same suggestion and is surely better. > > I'll re-implement this. > > > >>> + * > >>> + * \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. > >> > > > > All V4L2Subdevice methods accepts a pad as first argument. I would > > keep this consistent. > > > >>> + 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 > >> :-) > > > > I'm really not sure about this one, I could add a field to > > V4L2SubdeviceFormatEnum for this... > > > > Re-thinking about this, I would leave this out until we don't define a > way to handle FORMAT_TRY globally, as otherwise I would have to expose > that flag outside of V4L2Subdevice which I would prefer not to... I think you made the right decision. > >>> + > >>> + 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
Hi Jacopo, Laurent, > > > > > > All V4L2Subdevice methods accepts a pad as first argument. I would > > > keep this consistent. > > > > > >>> + 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 > > >> :-) > > > > > > I'm really not sure about this one, I could add a field to > > > V4L2SubdeviceFormatEnum for this... > > > > > > > Re-thinking about this, I would leave this out until we don't define a > > way to handle FORMAT_TRY globally, as otherwise I would have to expose > > that flag outside of V4L2Subdevice which I would prefer not to... > > I think you made the right decision. After mocking around a bit more in the code I now agree with you both. Hard coding of V4L2_SUBDEV_FORMAT_ACTIVE is how it's done in other places so we can change this when we add V4L2_SUBDEV_FORMAT_TRY in the future. Sorry for not noticing this in my first review.
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. + * + * \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; + sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; + + 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
Implement enumFormat() methods to enumerate the available image resolutions on the subdevice. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_subdevice.h | 11 ++++ src/libcamera/v4l2_subdevice.cpp | 90 ++++++++++++++++++++++++++ 2 files changed, 101 insertions(+)