| Message ID | 20190226162641.12116-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 26/02/2019 16:26, 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 | 9 ++ > src/libcamera/v4l2_subdevice.cpp | 118 +++++++++++++++++++++++++ > 2 files changed, 127 insertions(+) > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > index eac699a06109..6b21308d2087 100644 > --- a/src/libcamera/include/v4l2_subdevice.h > +++ b/src/libcamera/include/v4l2_subdevice.h > @@ -7,7 +7,9 @@ > #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__ > #define __LIBCAMERA_V4L2_SUBDEVICE_H__ > > +#include <map> > #include <string> > +#include <vector> > > #include "media_object.h" > > @@ -38,15 +40,22 @@ public: > int setCrop(unsigned int pad, Rectangle *rect); > int setCompose(unsigned int pad, Rectangle *rect); > > + std::vector<V4L2SubdeviceFormat> &formats(unsigned int pad); > int getFormat(unsigned int pad, V4L2SubdeviceFormat *format); > int setFormat(unsigned int pad, V4L2SubdeviceFormat *format); > > private: > + int listPadSizes(unsigned int pad, unsigned int mbus_code, > + std::vector<V4L2SubdeviceFormat> *formats); > + std::vector<V4L2SubdeviceFormat> listPadFormats(unsigned int pad); The word 'list' seems a bit redundant in 'listPadSizes()' and 'listPadFormats()' ... and seems a bit hungarian notation to me? Other than that, I trust that the tests that follow make sure the code is doing something sane :) I can't see anything jump out at me yet. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > int setSelection(unsigned int pad, unsigned int target, > Rectangle *rect); > > const MediaEntity *entity_; > int fd_; > + > + std::map<unsigned int, std::vector<V4L2SubdeviceFormat>> formats_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index 56ecf3851cb0..f81a521f9e2a 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -5,6 +5,10 @@ > * v4l2_subdevice.cpp - V4L2 Subdevice > */ > > +#include <map> > +#include <string> > +#include <vector> > + > #include <fcntl.h> > #include <string.h> > #include <sys/ioctl.h> > @@ -116,6 +120,9 @@ int V4L2Subdevice::open() > } > fd_ = ret; > > + for (MediaPad *pad : entity_->pads()) > + formats_[pad->index()] = listPadFormats(pad->index()); > + > return 0; > } > > @@ -178,6 +185,25 @@ 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 > + * > + * \return A vector of image formats, or an empty vector if the pad does not > + * exist > + */ > +std::vector<V4L2SubdeviceFormat> &V4L2Subdevice::formats(unsigned int pad) > +{ > + /* > + * If pad does not exist, return an empty vector at position > + * pads().size() > + */ > + if (pad > entity_->pads().size()) > + pad = entity_->pads().size(); > + > + return formats_[pad]; > +} > + > /** > * \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 > @@ -242,6 +268,98 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format) > return 0; > } > > +int V4L2Subdevice::listPadSizes(unsigned int pad, unsigned int mbus_code, > + std::vector<V4L2SubdeviceFormat> *formats) > +{ > + struct v4l2_subdev_frame_size_enum sizeEnum = {}; > + int ret; > + > + sizeEnum.index = 0; > + sizeEnum.pad = pad; > + sizeEnum.code = mbus_code; > + sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + > + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum))) { > + V4L2SubdeviceFormat minFormat = { > + .mbus_code = mbus_code, > + .width = sizeEnum.min_width, > + .height = sizeEnum.min_height, > + }; > + formats->push_back(minFormat); > + > + /* > + * Most subdevices report discrete frame resolutions, where > + * min and max sizes are identical. For continue frame > + * resolutions, store the min and max sizes interval. > + */ > + if (sizeEnum.min_width == sizeEnum.max_width && > + sizeEnum.min_height == sizeEnum.max_height) { > + sizeEnum.index++; > + continue; > + } > + > + V4L2SubdeviceFormat maxFormat = { > + .mbus_code = mbus_code, > + .width = sizeEnum.max_width, > + .height = sizeEnum.max_height, > + }; > + formats->push_back(maxFormat); > + > + sizeEnum.index++; > + } > + > + if (ret && (errno != EINVAL && errno != ENOTTY)) { > + LOG(V4L2Subdev, Error) > + << "Unable to enumerate format on pad " << pad > + << " of " << deviceName() << ": " > + << strerror(errno); > + return ret; > + } > + > + return 0; > +} > + > +std::vector<V4L2SubdeviceFormat> V4L2Subdevice::listPadFormats(unsigned int pad) > +{ > + struct v4l2_subdev_mbus_code_enum mbusEnum = {}; > + std::vector<V4L2SubdeviceFormat> formats = {}; > + int ret; > + > + mbusEnum.pad = pad; > + mbusEnum.index = 0; > + mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + > + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum))) { > + ret = listPadSizes(pad, mbusEnum.code, &formats); > + if (ret) > + return formats; > + > + mbusEnum.index++; > + } > + > + if (ret && (errno != EINVAL && errno != ENOTTY)) { > + LOG(V4L2Subdev, Error) > + << "Unable to enumerate format on pad " << pad > + << " of " << deviceName() << ": " << strerror(-ret); > + return formats; > + } > + > + if (mbusEnum.index == 0 && ret && errno == EINVAL) { > + /* > + * The subdevice might not support ENUM_MBUS_CODE but > + * might support ENUM_FRAME_SIZES. > + */ > + struct V4L2SubdeviceFormat subdevFormat; > + ret = getFormat(pad, &subdevFormat); > + if (ret) > + return formats; > + > + listPadSizes(pad, subdevFormat.mbus_code, &formats); > + } > + > + return formats; > +} > + > int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > Rectangle *rect) > { >
Hi Kieran, On Tue, Feb 26, 2019 at 11:19:40PM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 26/02/2019 16:26, 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 | 9 ++ > > src/libcamera/v4l2_subdevice.cpp | 118 +++++++++++++++++++++++++ > > 2 files changed, 127 insertions(+) > > > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > > index eac699a06109..6b21308d2087 100644 > > --- a/src/libcamera/include/v4l2_subdevice.h > > +++ b/src/libcamera/include/v4l2_subdevice.h > > @@ -7,7 +7,9 @@ > > #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__ > > #define __LIBCAMERA_V4L2_SUBDEVICE_H__ > > > > +#include <map> > > #include <string> > > +#include <vector> > > > > #include "media_object.h" > > > > @@ -38,15 +40,22 @@ public: > > int setCrop(unsigned int pad, Rectangle *rect); > > int setCompose(unsigned int pad, Rectangle *rect); > > > > + std::vector<V4L2SubdeviceFormat> &formats(unsigned int pad); > > int getFormat(unsigned int pad, V4L2SubdeviceFormat *format); > > int setFormat(unsigned int pad, V4L2SubdeviceFormat *format); > > > > private: > > + int listPadSizes(unsigned int pad, unsigned int mbus_code, > > + std::vector<V4L2SubdeviceFormat> *formats); > > + std::vector<V4L2SubdeviceFormat> listPadFormats(unsigned int pad); > > > The word 'list' seems a bit redundant in 'listPadSizes()' and > 'listPadFormats()' ... and seems a bit hungarian notation to me? > I interpreted "list" as a verb, so the function name reads like "list all formats on a pad". Is it weird to you? > Other than that, I trust that the tests that follow make sure the code > is doing something sane :) > > > I can't see anything jump out at me yet. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thanks j > > > > > + > > int setSelection(unsigned int pad, unsigned int target, > > Rectangle *rect); > > > > const MediaEntity *entity_; > > int fd_; > > + > > + std::map<unsigned int, std::vector<V4L2SubdeviceFormat>> formats_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > index 56ecf3851cb0..f81a521f9e2a 100644 > > --- a/src/libcamera/v4l2_subdevice.cpp > > +++ b/src/libcamera/v4l2_subdevice.cpp > > @@ -5,6 +5,10 @@ > > * v4l2_subdevice.cpp - V4L2 Subdevice > > */ > > > > +#include <map> > > +#include <string> > > +#include <vector> > > + > > #include <fcntl.h> > > #include <string.h> > > #include <sys/ioctl.h> > > @@ -116,6 +120,9 @@ int V4L2Subdevice::open() > > } > > fd_ = ret; > > > > + for (MediaPad *pad : entity_->pads()) > > + formats_[pad->index()] = listPadFormats(pad->index()); > > + > > return 0; > > } > > > > @@ -178,6 +185,25 @@ 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 > > + * > > + * \return A vector of image formats, or an empty vector if the pad does not > > + * exist > > + */ > > +std::vector<V4L2SubdeviceFormat> &V4L2Subdevice::formats(unsigned int pad) > > +{ > > + /* > > + * If pad does not exist, return an empty vector at position > > + * pads().size() > > + */ > > + if (pad > entity_->pads().size()) > > + pad = entity_->pads().size(); > > + > > + return formats_[pad]; > > +} > > + > > /** > > * \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 > > @@ -242,6 +268,98 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format) > > return 0; > > } > > > > +int V4L2Subdevice::listPadSizes(unsigned int pad, unsigned int mbus_code, > > + std::vector<V4L2SubdeviceFormat> *formats) > > +{ > > + struct v4l2_subdev_frame_size_enum sizeEnum = {}; > > + int ret; > > + > > + sizeEnum.index = 0; > > + sizeEnum.pad = pad; > > + sizeEnum.code = mbus_code; > > + sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > + > > + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum))) { > > + V4L2SubdeviceFormat minFormat = { > > + .mbus_code = mbus_code, > > + .width = sizeEnum.min_width, > > + .height = sizeEnum.min_height, > > + }; > > + formats->push_back(minFormat); > > + > > + /* > > + * Most subdevices report discrete frame resolutions, where > > + * min and max sizes are identical. For continue frame > > + * resolutions, store the min and max sizes interval. > > + */ > > + if (sizeEnum.min_width == sizeEnum.max_width && > > + sizeEnum.min_height == sizeEnum.max_height) { > > + sizeEnum.index++; > > + continue; > > + } > > + > > + V4L2SubdeviceFormat maxFormat = { > > + .mbus_code = mbus_code, > > + .width = sizeEnum.max_width, > > + .height = sizeEnum.max_height, > > + }; > > + formats->push_back(maxFormat); > > + > > + sizeEnum.index++; > > + } > > + > > + if (ret && (errno != EINVAL && errno != ENOTTY)) { > > + LOG(V4L2Subdev, Error) > > + << "Unable to enumerate format on pad " << pad > > + << " of " << deviceName() << ": " > > + << strerror(errno); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +std::vector<V4L2SubdeviceFormat> V4L2Subdevice::listPadFormats(unsigned int pad) > > +{ > > + struct v4l2_subdev_mbus_code_enum mbusEnum = {}; > > + std::vector<V4L2SubdeviceFormat> formats = {}; > > + int ret; > > + > > + mbusEnum.pad = pad; > > + mbusEnum.index = 0; > > + mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > + > > + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum))) { > > + ret = listPadSizes(pad, mbusEnum.code, &formats); > > + if (ret) > > + return formats; > > + > > + mbusEnum.index++; > > + } > > + > > + if (ret && (errno != EINVAL && errno != ENOTTY)) { > > + LOG(V4L2Subdev, Error) > > + << "Unable to enumerate format on pad " << pad > > + << " of " << deviceName() << ": " << strerror(-ret); > > + return formats; > > + } > > + > > + if (mbusEnum.index == 0 && ret && errno == EINVAL) { > > + /* > > + * The subdevice might not support ENUM_MBUS_CODE but > > + * might support ENUM_FRAME_SIZES. > > + */ > > + struct V4L2SubdeviceFormat subdevFormat; > > + ret = getFormat(pad, &subdevFormat); > > + if (ret) > > + return formats; > > + > > + listPadSizes(pad, subdevFormat.mbus_code, &formats); > > + } > > + > > + return formats; > > +} > > + > > int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > > Rectangle *rect) > > { > > > > -- > Regards > -- > Kieran
Hi Jacopo, Thank you for the patch. On Wed, Feb 27, 2019 at 09:18:04AM +0100, Jacopo Mondi wrote: > On Tue, Feb 26, 2019 at 11:19:40PM +0000, Kieran Bingham wrote: > > On 26/02/2019 16:26, 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 | 9 ++ > >> src/libcamera/v4l2_subdevice.cpp | 118 +++++++++++++++++++++++++ > >> 2 files changed, 127 insertions(+) > >> > >> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > >> index eac699a06109..6b21308d2087 100644 > >> --- a/src/libcamera/include/v4l2_subdevice.h > >> +++ b/src/libcamera/include/v4l2_subdevice.h > >> @@ -7,7 +7,9 @@ > >> #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__ > >> #define __LIBCAMERA_V4L2_SUBDEVICE_H__ > >> > >> +#include <map> > >> #include <string> > >> +#include <vector> > >> > >> #include "media_object.h" > >> > >> @@ -38,15 +40,22 @@ public: > >> int setCrop(unsigned int pad, Rectangle *rect); > >> int setCompose(unsigned int pad, Rectangle *rect); > >> > >> + std::vector<V4L2SubdeviceFormat> &formats(unsigned int pad); Shouldn't you return a const reference ? And make this function const ? > >> int getFormat(unsigned int pad, V4L2SubdeviceFormat *format); > >> int setFormat(unsigned int pad, V4L2SubdeviceFormat *format); > >> > >> private: > >> + int listPadSizes(unsigned int pad, unsigned int mbus_code, > >> + std::vector<V4L2SubdeviceFormat> *formats); > >> + std::vector<V4L2SubdeviceFormat> listPadFormats(unsigned int pad); > > > > The word 'list' seems a bit redundant in 'listPadSizes()' and > > 'listPadFormats()' ... and seems a bit hungarian notation to me? > > I interpreted "list" as a verb, so the function name reads like "list > all formats on a pad". Is it weird to you? How about s/list/enumerate/ to remove all ambiguity ? > > Other than that, I trust that the tests that follow make sure the code > > is doing something sane :) > > > > I can't see anything jump out at me yet. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> + > >> int setSelection(unsigned int pad, unsigned int target, > >> Rectangle *rect); > >> > >> const MediaEntity *entity_; > >> int fd_; > >> + > >> + std::map<unsigned int, std::vector<V4L2SubdeviceFormat>> formats_; > >> }; > >> > >> } /* namespace libcamera */ > >> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > >> index 56ecf3851cb0..f81a521f9e2a 100644 > >> --- a/src/libcamera/v4l2_subdevice.cpp > >> +++ b/src/libcamera/v4l2_subdevice.cpp > >> @@ -5,6 +5,10 @@ > >> * v4l2_subdevice.cpp - V4L2 Subdevice > >> */ > >> > >> +#include <map> > >> +#include <string> > >> +#include <vector> > >> + Not strictly needed as the header includes them. > >> #include <fcntl.h> > >> #include <string.h> > >> #include <sys/ioctl.h> > >> @@ -116,6 +120,9 @@ int V4L2Subdevice::open() > >> } > >> fd_ = ret; > >> > >> + for (MediaPad *pad : entity_->pads()) > >> + formats_[pad->index()] = listPadFormats(pad->index()); > >> + Should you do a formats_.clear() on close() ? > >> return 0; > >> } > >> > >> @@ -178,6 +185,25 @@ 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 s/List/Retrieve/ ? > >> + * \param[in] pad The 0-indexed pad number to enumerate formats on "to retrieve formats for" ? > >> + * > >> + * \return A vector of image formats, or an empty vector if the pad does not > >> + * exist > >> + */ > >> +std::vector<V4L2SubdeviceFormat> &V4L2Subdevice::formats(unsigned int pad) > >> +{ > >> + /* > >> + * If pad does not exist, return an empty vector at position > >> + * pads().size() > >> + */ This will prevent differentiating between a non-existing pad and a pad that has no format. How about returning a pointer instead of a reference ? > >> + if (pad > entity_->pads().size()) > >> + pad = entity_->pads().size(); > >> + > >> + return formats_[pad]; > >> +} > >> + > >> /** > >> * \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 > >> @@ -242,6 +268,98 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format) > >> return 0; > >> } > >> > >> +int V4L2Subdevice::listPadSizes(unsigned int pad, unsigned int mbus_code, > >> + std::vector<V4L2SubdeviceFormat> *formats) > >> +{ > >> + struct v4l2_subdev_frame_size_enum sizeEnum = {}; > >> + int ret; > >> + > >> + sizeEnum.index = 0; > >> + sizeEnum.pad = pad; > >> + sizeEnum.code = mbus_code; > >> + sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > >> + > >> + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum))) { Assignments in conditional expressions are discouraged. How about while (1) { ret = ioctl(...); if (ret < 0) break; > >> + V4L2SubdeviceFormat minFormat = { > >> + .mbus_code = mbus_code, > >> + .width = sizeEnum.min_width, > >> + .height = sizeEnum.min_height, > >> + }; > >> + formats->push_back(minFormat); You can use emplace_back() instead of creating a local temporary variable. > >> + > >> + /* > >> + * Most subdevices report discrete frame resolutions, where > >> + * min and max sizes are identical. For continue frame > >> + * resolutions, store the min and max sizes interval. > >> + */ That's a bit of a hack. How is a caller supposed to tell if two consecutive entries in the list are min, max values or discrete values ? How about creating a SizeRange class (in geometry.h) to store min and max width and height, and using a std::vector<SizeRange> ? That would also have the benefit of not storing the mbus code in all entries. > >> + if (sizeEnum.min_width == sizeEnum.max_width && > >> + sizeEnum.min_height == sizeEnum.max_height) { > >> + sizeEnum.index++; > >> + continue; > >> + } > >> + > >> + V4L2SubdeviceFormat maxFormat = { > >> + .mbus_code = mbus_code, > >> + .width = sizeEnum.max_width, > >> + .height = sizeEnum.max_height, > >> + }; > >> + formats->push_back(maxFormat); emplace_back() here too. > >> + > >> + sizeEnum.index++; > >> + } > >> + > >> + if (ret && (errno != EINVAL && errno != ENOTTY)) { > >> + LOG(V4L2Subdev, Error) > >> + << "Unable to enumerate format on pad " << pad > >> + << " of " << deviceName() << ": " > >> + << strerror(errno); > >> + return ret; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +std::vector<V4L2SubdeviceFormat> V4L2Subdevice::listPadFormats(unsigned int pad) > >> +{ > >> + struct v4l2_subdev_mbus_code_enum mbusEnum = {}; > >> + std::vector<V4L2SubdeviceFormat> formats = {}; > >> + int ret; > >> + > >> + mbusEnum.pad = pad; > >> + mbusEnum.index = 0; > >> + mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > >> + > >> + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum))) { Same comment here. > >> + ret = listPadSizes(pad, mbusEnum.code, &formats); How about making format a std::map<unsigned int, std:vector<SizeRange>> to store the mbus code ? > >> + if (ret) > >> + return formats; > >> + > >> + mbusEnum.index++; > >> + } > >> + > >> + if (ret && (errno != EINVAL && errno != ENOTTY)) { > >> + LOG(V4L2Subdev, Error) > >> + << "Unable to enumerate format on pad " << pad > >> + << " of " << deviceName() << ": " << strerror(-ret); > >> + return formats; > >> + } > >> + > >> + if (mbusEnum.index == 0 && ret && errno == EINVAL) { > >> + /* > >> + * The subdevice might not support ENUM_MBUS_CODE but > >> + * might support ENUM_FRAME_SIZES. > >> + */ Can that happen ? > >> + struct V4L2SubdeviceFormat subdevFormat; > >> + ret = getFormat(pad, &subdevFormat); > >> + if (ret) > >> + return formats; > >> + > >> + listPadSizes(pad, subdevFormat.mbus_code, &formats); > >> + } > >> + > >> + return formats; > >> +} > >> + > >> int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > >> Rectangle *rect) > >> {
HI Laurent, thanks for your comments On Wed, Feb 27, 2019 at 07:51:20PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Feb 27, 2019 at 09:18:04AM +0100, Jacopo Mondi wrote: > > On Tue, Feb 26, 2019 at 11:19:40PM +0000, Kieran Bingham wrote: > > > On 26/02/2019 16:26, 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 | 9 ++ > > >> src/libcamera/v4l2_subdevice.cpp | 118 +++++++++++++++++++++++++ > > >> 2 files changed, 127 insertions(+) > > >> > > >> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > > >> index eac699a06109..6b21308d2087 100644 > > >> --- a/src/libcamera/include/v4l2_subdevice.h > > >> +++ b/src/libcamera/include/v4l2_subdevice.h > > >> @@ -7,7 +7,9 @@ > > >> #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__ > > >> #define __LIBCAMERA_V4L2_SUBDEVICE_H__ > > >> > > >> +#include <map> > > >> #include <string> > > >> +#include <vector> > > >> > > >> #include "media_object.h" > > >> > > >> @@ -38,15 +40,22 @@ public: > > >> int setCrop(unsigned int pad, Rectangle *rect); > > >> int setCompose(unsigned int pad, Rectangle *rect); > > >> > > >> + std::vector<V4L2SubdeviceFormat> &formats(unsigned int pad); > > Shouldn't you return a const reference ? And make this function const ? > > > >> int getFormat(unsigned int pad, V4L2SubdeviceFormat *format); > > >> int setFormat(unsigned int pad, V4L2SubdeviceFormat *format); > > >> > > >> private: > > >> + int listPadSizes(unsigned int pad, unsigned int mbus_code, > > >> + std::vector<V4L2SubdeviceFormat> *formats); > > >> + std::vector<V4L2SubdeviceFormat> listPadFormats(unsigned int pad); > > > > > > The word 'list' seems a bit redundant in 'listPadSizes()' and > > > 'listPadFormats()' ... and seems a bit hungarian notation to me? > > > > I interpreted "list" as a verb, so the function name reads like "list > > all formats on a pad". Is it weird to you? > > How about s/list/enumerate/ to remove all ambiguity ? > Ok > > > Other than that, I trust that the tests that follow make sure the code > > > is doing something sane :) > > > > > > I can't see anything jump out at me yet. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > >> + > > >> int setSelection(unsigned int pad, unsigned int target, > > >> Rectangle *rect); > > >> > > >> const MediaEntity *entity_; > > >> int fd_; > > >> + > > >> + std::map<unsigned int, std::vector<V4L2SubdeviceFormat>> formats_; > > >> }; > > >> > > >> } /* namespace libcamera */ > > >> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > >> index 56ecf3851cb0..f81a521f9e2a 100644 > > >> --- a/src/libcamera/v4l2_subdevice.cpp > > >> +++ b/src/libcamera/v4l2_subdevice.cpp > > >> @@ -5,6 +5,10 @@ > > >> * v4l2_subdevice.cpp - V4L2 Subdevice > > >> */ > > >> > > >> +#include <map> > > >> +#include <string> > > >> +#include <vector> > > >> + > > Not strictly needed as the header includes them. > > > >> #include <fcntl.h> > > >> #include <string.h> > > >> #include <sys/ioctl.h> > > >> @@ -116,6 +120,9 @@ int V4L2Subdevice::open() > > >> } > > >> fd_ = ret; > > >> > > >> + for (MediaPad *pad : entity_->pads()) > > >> + formats_[pad->index()] = listPadFormats(pad->index()); > > >> + > > Should you do a formats_.clear() on close() ? > Good point. But do we want to enumerate formats everytime we open a video device, or do it once and store them through open/close. If I use a pointer to store the format vector (or map as you suggested) I can easly find out if that's the first time the subdevice got opened, and create the format list once. > > >> return 0; > > >> } > > >> > > >> @@ -178,6 +185,25 @@ 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 > > s/List/Retrieve/ ? > > > >> + * \param[in] pad The 0-indexed pad number to enumerate formats on > > "to retrieve formats for" ? > > > >> + * > > >> + * \return A vector of image formats, or an empty vector if the pad does not > > >> + * exist > > >> + */ > > >> +std::vector<V4L2SubdeviceFormat> &V4L2Subdevice::formats(unsigned int pad) > > >> +{ > > >> + /* > > >> + * If pad does not exist, return an empty vector at position > > >> + * pads().size() > > >> + */ > > This will prevent differentiating between a non-existing pad and a pad > that has no format. How about returning a pointer instead of a reference > ? Ack, as per the above point, having the formats as pointers makes it easier to find out if they have been ever enumerated or not. > > > >> + if (pad > entity_->pads().size()) > > >> + pad = entity_->pads().size(); > > >> + > > >> + return formats_[pad]; > > >> +} > > >> + > > >> /** > > >> * \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 > > >> @@ -242,6 +268,98 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format) > > >> return 0; > > >> } > > >> > > >> +int V4L2Subdevice::listPadSizes(unsigned int pad, unsigned int mbus_code, > > >> + std::vector<V4L2SubdeviceFormat> *formats) > > >> +{ > > >> + struct v4l2_subdev_frame_size_enum sizeEnum = {}; > > >> + int ret; > > >> + > > >> + sizeEnum.index = 0; > > >> + sizeEnum.pad = pad; > > >> + sizeEnum.code = mbus_code; > > >> + sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > >> + > > >> + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum))) { > > Assignments in conditional expressions are discouraged. How about Are they? > > while (1) { > ret = ioctl(...); > if (ret < 0) > break; Sure, no problem > > > >> + V4L2SubdeviceFormat minFormat = { > > >> + .mbus_code = mbus_code, > > >> + .width = sizeEnum.min_width, > > >> + .height = sizeEnum.min_height, > > >> + }; > > >> + formats->push_back(minFormat); > > You can use emplace_back() instead of creating a local temporary > variable. Thanks. How does that work on structures with only the default compiler-created constructor? > > > >> + > > >> + /* > > >> + * Most subdevices report discrete frame resolutions, where > > >> + * min and max sizes are identical. For continue frame > > >> + * resolutions, store the min and max sizes interval. > > >> + */ > > That's a bit of a hack. How is a caller supposed to tell if two > consecutive entries in the list are min, max values or discrete values ? > How about creating a SizeRange class (in geometry.h) to store min and > max width and height, and using a std::vector<SizeRange> ? That would > also have the benefit of not storing the mbus code in all entries. > It really puzzled me how to better handle this. If we store min/max as well, how are users of the subdevice expected to handle this? They shall be aware that the formats can either be a single format with a min/max range, or an enumeration of discrete sizes. I mean, it's surely doable, I wonder how to express it clearly, in both documentation and code. It's not unexpected though, as it replicates the SUBDEV_ENUM_FRAME_SIZE behavior, so maybe it's enough to repeat the same here, but I wanted to avoid this ambiguity in our implementation. I guess it's not easily doable. > > >> + if (sizeEnum.min_width == sizeEnum.max_width && > > >> + sizeEnum.min_height == sizeEnum.max_height) { > > >> + sizeEnum.index++; > > >> + continue; > > >> + } > > >> + > > >> + V4L2SubdeviceFormat maxFormat = { > > >> + .mbus_code = mbus_code, > > >> + .width = sizeEnum.max_width, > > >> + .height = sizeEnum.max_height, > > >> + }; > > >> + formats->push_back(maxFormat); > > emplace_back() here too. > > > >> + > > >> + sizeEnum.index++; > > >> + } > > >> + > > >> + if (ret && (errno != EINVAL && errno != ENOTTY)) { > > >> + LOG(V4L2Subdev, Error) > > >> + << "Unable to enumerate format on pad " << pad > > >> + << " of " << deviceName() << ": " > > >> + << strerror(errno); > > >> + return ret; > > >> + } > > >> + > > >> + return 0; > > >> +} > > >> + > > >> +std::vector<V4L2SubdeviceFormat> V4L2Subdevice::listPadFormats(unsigned int pad) > > >> +{ > > >> + struct v4l2_subdev_mbus_code_enum mbusEnum = {}; > > >> + std::vector<V4L2SubdeviceFormat> formats = {}; > > >> + int ret; > > >> + > > >> + mbusEnum.pad = pad; > > >> + mbusEnum.index = 0; > > >> + mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > >> + > > >> + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum))) { > > Same comment here. > > > >> + ret = listPadSizes(pad, mbusEnum.code, &formats); > > How about making format a std::map<unsigned int, std:vector<SizeRange>> > to store the mbus code ? > That's a good suggestion, I'll try and see how it looks like. > > >> + if (ret) > > >> + return formats; > > >> + > > >> + mbusEnum.index++; > > >> + } > > >> + > > >> + if (ret && (errno != EINVAL && errno != ENOTTY)) { > > >> + LOG(V4L2Subdev, Error) > > >> + << "Unable to enumerate format on pad " << pad > > >> + << " of " << deviceName() << ": " << strerror(-ret); > > >> + return formats; > > >> + } > > >> + > > >> + if (mbusEnum.index == 0 && ret && errno == EINVAL) { > > >> + /* > > >> + * The subdevice might not support ENUM_MBUS_CODE but > > >> + * might support ENUM_FRAME_SIZES. > > >> + */ > > Can that happen ? > Is there anything in the V4L2 APIs preventing this? I haven't find anything in the documentation that says so. I would be happy to drop this, if we consider this case non-standard and not worth supporting. Thanks j > > >> + struct V4L2SubdeviceFormat subdevFormat; > > >> + ret = getFormat(pad, &subdevFormat); > > >> + if (ret) > > >> + return formats; > > >> + > > >> + listPadSizes(pad, subdevFormat.mbus_code, &formats); > > >> + } > > >> + > > >> + return formats; > > >> +} > > >> + > > >> int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > > >> Rectangle *rect) > > >> { > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Feb 28, 2019 at 09:44:31AM +0100, Jacopo Mondi wrote: > On Wed, Feb 27, 2019 at 07:51:20PM +0200, Laurent Pinchart wrote: > > On Wed, Feb 27, 2019 at 09:18:04AM +0100, Jacopo Mondi wrote: > >> On Tue, Feb 26, 2019 at 11:19:40PM +0000, Kieran Bingham wrote: > >>> On 26/02/2019 16:26, 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 | 9 ++ > >>>> src/libcamera/v4l2_subdevice.cpp | 118 +++++++++++++++++++++++++ > >>>> 2 files changed, 127 insertions(+) > >>>> > >>>> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > >>>> index eac699a06109..6b21308d2087 100644 > >>>> --- a/src/libcamera/include/v4l2_subdevice.h > >>>> +++ b/src/libcamera/include/v4l2_subdevice.h > >>>> @@ -7,7 +7,9 @@ > >>>> #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__ > >>>> #define __LIBCAMERA_V4L2_SUBDEVICE_H__ > >>>> > >>>> +#include <map> > >>>> #include <string> > >>>> +#include <vector> > >>>> > >>>> #include "media_object.h" > >>>> > >>>> @@ -38,15 +40,22 @@ public: > >>>> int setCrop(unsigned int pad, Rectangle *rect); > >>>> int setCompose(unsigned int pad, Rectangle *rect); > >>>> > >>>> + std::vector<V4L2SubdeviceFormat> &formats(unsigned int pad); > > > > Shouldn't you return a const reference ? And make this function const ? > > > >>>> int getFormat(unsigned int pad, V4L2SubdeviceFormat *format); > >>>> int setFormat(unsigned int pad, V4L2SubdeviceFormat *format); > >>>> > >>>> private: > >>>> + int listPadSizes(unsigned int pad, unsigned int mbus_code, > >>>> + std::vector<V4L2SubdeviceFormat> *formats); > >>>> + std::vector<V4L2SubdeviceFormat> listPadFormats(unsigned int pad); > >>> > >>> The word 'list' seems a bit redundant in 'listPadSizes()' and > >>> 'listPadFormats()' ... and seems a bit hungarian notation to me? > >> > >> I interpreted "list" as a verb, so the function name reads like "list > >> all formats on a pad". Is it weird to you? > > > > How about s/list/enumerate/ to remove all ambiguity ? > > Ok > > >>> Other than that, I trust that the tests that follow make sure the code > >>> is doing something sane :) > >>> > >>> I can't see anything jump out at me yet. > >>> > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> > >>>> + > >>>> int setSelection(unsigned int pad, unsigned int target, > >>>> Rectangle *rect); > >>>> > >>>> const MediaEntity *entity_; > >>>> int fd_; > >>>> + > >>>> + std::map<unsigned int, std::vector<V4L2SubdeviceFormat>> formats_; > >>>> }; > >>>> > >>>> } /* namespace libcamera */ > >>>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > >>>> index 56ecf3851cb0..f81a521f9e2a 100644 > >>>> --- a/src/libcamera/v4l2_subdevice.cpp > >>>> +++ b/src/libcamera/v4l2_subdevice.cpp > >>>> @@ -5,6 +5,10 @@ > >>>> * v4l2_subdevice.cpp - V4L2 Subdevice > >>>> */ > >>>> > >>>> +#include <map> > >>>> +#include <string> > >>>> +#include <vector> > >>>> + > > > > Not strictly needed as the header includes them. > > > >>>> #include <fcntl.h> > >>>> #include <string.h> > >>>> #include <sys/ioctl.h> > >>>> @@ -116,6 +120,9 @@ int V4L2Subdevice::open() > >>>> } > >>>> fd_ = ret; > >>>> > >>>> + for (MediaPad *pad : entity_->pads()) > >>>> + formats_[pad->index()] = listPadFormats(pad->index()); > >>>> + > > > > Should you do a formats_.clear() on close() ? > > > > Good point. But do we want to enumerate formats everytime we open a > video device, or do it once and store them through open/close. > > If I use a pointer to store the format vector (or map as you suggested) > I can easly find out if that's the first time the subdevice got > opened, and create the format list once. > > >>>> return 0; > >>>> } > >>>> > >>>> @@ -178,6 +185,25 @@ 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 > > > > s/List/Retrieve/ ? > > > >>>> + * \param[in] pad The 0-indexed pad number to enumerate formats on > > > > "to retrieve formats for" ? > > > >>>> + * > >>>> + * \return A vector of image formats, or an empty vector if the pad does not > >>>> + * exist > >>>> + */ > >>>> +std::vector<V4L2SubdeviceFormat> &V4L2Subdevice::formats(unsigned int pad) > >>>> +{ > >>>> + /* > >>>> + * If pad does not exist, return an empty vector at position > >>>> + * pads().size() > >>>> + */ > > > > This will prevent differentiating between a non-existing pad and a pad > > that has no format. How about returning a pointer instead of a reference > > ? > > Ack, as per the above point, having the formats as pointers makes it > easier to find out if they have been ever enumerated or not. > > >>>> + if (pad > entity_->pads().size()) > >>>> + pad = entity_->pads().size(); > >>>> + > >>>> + return formats_[pad]; > >>>> +} > >>>> + > >>>> /** > >>>> * \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 > >>>> @@ -242,6 +268,98 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format) > >>>> return 0; > >>>> } > >>>> > >>>> +int V4L2Subdevice::listPadSizes(unsigned int pad, unsigned int mbus_code, > >>>> + std::vector<V4L2SubdeviceFormat> *formats) > >>>> +{ > >>>> + struct v4l2_subdev_frame_size_enum sizeEnum = {}; > >>>> + int ret; > >>>> + > >>>> + sizeEnum.index = 0; > >>>> + sizeEnum.pad = pad; > >>>> + sizeEnum.code = mbus_code; > >>>> + sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > >>>> + > >>>> + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum))) { > > > > Assignments in conditional expressions are discouraged. How about > > Are they? It's too easy to forget an = and type = instead of == in a conditional expression, so forbidding assignments makes it easier to catch such errors as they would always be invalid. > > > > while (1) { > > ret = ioctl(...); > > if (ret < 0) > > break; > > Sure, no problem > > >>>> + V4L2SubdeviceFormat minFormat = { > >>>> + .mbus_code = mbus_code, > >>>> + .width = sizeEnum.min_width, > >>>> + .height = sizeEnum.min_height, > >>>> + }; > >>>> + formats->push_back(minFormat); > > > > You can use emplace_back() instead of creating a local temporary > > variable. > > Thanks. How does that work on structures with only the default > compiler-created constructor? I think you can use an initializer list. > >>>> + > >>>> + /* > >>>> + * Most subdevices report discrete frame resolutions, where > >>>> + * min and max sizes are identical. For continue frame > >>>> + * resolutions, store the min and max sizes interval. > >>>> + */ > > > > That's a bit of a hack. How is a caller supposed to tell if two > > consecutive entries in the list are min, max values or discrete values ? > > How about creating a SizeRange class (in geometry.h) to store min and > > max width and height, and using a std::vector<SizeRange> ? That would > > also have the benefit of not storing the mbus code in all entries. > > It really puzzled me how to better handle this. If we store min/max as > well, how are users of the subdevice expected to handle this? They > shall be aware that the formats can either be a single format with a > min/max range, or an enumeration of discrete sizes. I mean, it's > surely doable, I wonder how to express it clearly, in both > documentation and code. It's not unexpected though, as it replicates > the SUBDEV_ENUM_FRAME_SIZE behavior, so maybe it's enough to repeat > the same here, but I wanted to avoid this ambiguity in our > implementation. I guess it's not easily doable. I don't think we can do that easily, and your attempt created an even bigger problem :-) > >>>> + if (sizeEnum.min_width == sizeEnum.max_width && > >>>> + sizeEnum.min_height == sizeEnum.max_height) { > >>>> + sizeEnum.index++; > >>>> + continue; > >>>> + } > >>>> + > >>>> + V4L2SubdeviceFormat maxFormat = { > >>>> + .mbus_code = mbus_code, > >>>> + .width = sizeEnum.max_width, > >>>> + .height = sizeEnum.max_height, > >>>> + }; > >>>> + formats->push_back(maxFormat); > > > > emplace_back() here too. > > > >>>> + > >>>> + sizeEnum.index++; > >>>> + } > >>>> + > >>>> + if (ret && (errno != EINVAL && errno != ENOTTY)) { > >>>> + LOG(V4L2Subdev, Error) > >>>> + << "Unable to enumerate format on pad " << pad > >>>> + << " of " << deviceName() << ": " > >>>> + << strerror(errno); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +std::vector<V4L2SubdeviceFormat> V4L2Subdevice::listPadFormats(unsigned int pad) > >>>> +{ > >>>> + struct v4l2_subdev_mbus_code_enum mbusEnum = {}; > >>>> + std::vector<V4L2SubdeviceFormat> formats = {}; > >>>> + int ret; > >>>> + > >>>> + mbusEnum.pad = pad; > >>>> + mbusEnum.index = 0; > >>>> + mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; > >>>> + > >>>> + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum))) { > > > > Same comment here. > > > >>>> + ret = listPadSizes(pad, mbusEnum.code, &formats); > > > > How about making format a std::map<unsigned int, std:vector<SizeRange>> > > to store the mbus code ? > > That's a good suggestion, I'll try and see how it looks like. > > >>>> + if (ret) > >>>> + return formats; > >>>> + > >>>> + mbusEnum.index++; > >>>> + } > >>>> + > >>>> + if (ret && (errno != EINVAL && errno != ENOTTY)) { > >>>> + LOG(V4L2Subdev, Error) > >>>> + << "Unable to enumerate format on pad " << pad > >>>> + << " of " << deviceName() << ": " << strerror(-ret); > >>>> + return formats; > >>>> + } > >>>> + > >>>> + if (mbusEnum.index == 0 && ret && errno == EINVAL) { > >>>> + /* > >>>> + * The subdevice might not support ENUM_MBUS_CODE but > >>>> + * might support ENUM_FRAME_SIZES. > >>>> + */ > > > > Can that happen ? > > Is there anything in the V4L2 APIs preventing this? I haven't find > anything in the documentation that says so. I would be happy to drop > this, if we consider this case non-standard and not worth supporting. I think it would be a case of non-compliance, so I don't think we need to support it. > >>>> + struct V4L2SubdeviceFormat subdevFormat; > >>>> + ret = getFormat(pad, &subdevFormat); > >>>> + if (ret) > >>>> + return formats; > >>>> + > >>>> + listPadSizes(pad, subdevFormat.mbus_code, &formats); > >>>> + } > >>>> + > >>>> + return formats; > >>>> +} > >>>> + > >>>> int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > >>>> Rectangle *rect) > >>>> {
diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h index eac699a06109..6b21308d2087 100644 --- a/src/libcamera/include/v4l2_subdevice.h +++ b/src/libcamera/include/v4l2_subdevice.h @@ -7,7 +7,9 @@ #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__ #define __LIBCAMERA_V4L2_SUBDEVICE_H__ +#include <map> #include <string> +#include <vector> #include "media_object.h" @@ -38,15 +40,22 @@ public: int setCrop(unsigned int pad, Rectangle *rect); int setCompose(unsigned int pad, Rectangle *rect); + std::vector<V4L2SubdeviceFormat> &formats(unsigned int pad); int getFormat(unsigned int pad, V4L2SubdeviceFormat *format); int setFormat(unsigned int pad, V4L2SubdeviceFormat *format); private: + int listPadSizes(unsigned int pad, unsigned int mbus_code, + std::vector<V4L2SubdeviceFormat> *formats); + std::vector<V4L2SubdeviceFormat> listPadFormats(unsigned int pad); + int setSelection(unsigned int pad, unsigned int target, Rectangle *rect); const MediaEntity *entity_; int fd_; + + std::map<unsigned int, std::vector<V4L2SubdeviceFormat>> formats_; }; } /* namespace libcamera */ diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp index 56ecf3851cb0..f81a521f9e2a 100644 --- a/src/libcamera/v4l2_subdevice.cpp +++ b/src/libcamera/v4l2_subdevice.cpp @@ -5,6 +5,10 @@ * v4l2_subdevice.cpp - V4L2 Subdevice */ +#include <map> +#include <string> +#include <vector> + #include <fcntl.h> #include <string.h> #include <sys/ioctl.h> @@ -116,6 +120,9 @@ int V4L2Subdevice::open() } fd_ = ret; + for (MediaPad *pad : entity_->pads()) + formats_[pad->index()] = listPadFormats(pad->index()); + return 0; } @@ -178,6 +185,25 @@ 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 + * + * \return A vector of image formats, or an empty vector if the pad does not + * exist + */ +std::vector<V4L2SubdeviceFormat> &V4L2Subdevice::formats(unsigned int pad) +{ + /* + * If pad does not exist, return an empty vector at position + * pads().size() + */ + if (pad > entity_->pads().size()) + pad = entity_->pads().size(); + + return formats_[pad]; +} + /** * \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 @@ -242,6 +268,98 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format) return 0; } +int V4L2Subdevice::listPadSizes(unsigned int pad, unsigned int mbus_code, + std::vector<V4L2SubdeviceFormat> *formats) +{ + struct v4l2_subdev_frame_size_enum sizeEnum = {}; + int ret; + + sizeEnum.index = 0; + sizeEnum.pad = pad; + sizeEnum.code = mbus_code; + sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; + + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum))) { + V4L2SubdeviceFormat minFormat = { + .mbus_code = mbus_code, + .width = sizeEnum.min_width, + .height = sizeEnum.min_height, + }; + formats->push_back(minFormat); + + /* + * Most subdevices report discrete frame resolutions, where + * min and max sizes are identical. For continue frame + * resolutions, store the min and max sizes interval. + */ + if (sizeEnum.min_width == sizeEnum.max_width && + sizeEnum.min_height == sizeEnum.max_height) { + sizeEnum.index++; + continue; + } + + V4L2SubdeviceFormat maxFormat = { + .mbus_code = mbus_code, + .width = sizeEnum.max_width, + .height = sizeEnum.max_height, + }; + formats->push_back(maxFormat); + + sizeEnum.index++; + } + + if (ret && (errno != EINVAL && errno != ENOTTY)) { + LOG(V4L2Subdev, Error) + << "Unable to enumerate format on pad " << pad + << " of " << deviceName() << ": " + << strerror(errno); + return ret; + } + + return 0; +} + +std::vector<V4L2SubdeviceFormat> V4L2Subdevice::listPadFormats(unsigned int pad) +{ + struct v4l2_subdev_mbus_code_enum mbusEnum = {}; + std::vector<V4L2SubdeviceFormat> formats = {}; + int ret; + + mbusEnum.pad = pad; + mbusEnum.index = 0; + mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE; + + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum))) { + ret = listPadSizes(pad, mbusEnum.code, &formats); + if (ret) + return formats; + + mbusEnum.index++; + } + + if (ret && (errno != EINVAL && errno != ENOTTY)) { + LOG(V4L2Subdev, Error) + << "Unable to enumerate format on pad " << pad + << " of " << deviceName() << ": " << strerror(-ret); + return formats; + } + + if (mbusEnum.index == 0 && ret && errno == EINVAL) { + /* + * The subdevice might not support ENUM_MBUS_CODE but + * might support ENUM_FRAME_SIZES. + */ + struct V4L2SubdeviceFormat subdevFormat; + ret = getFormat(pad, &subdevFormat); + if (ret) + return formats; + + listPadSizes(pad, subdevFormat.mbus_code, &formats); + } + + return formats; +} + int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, Rectangle *rect) {
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 | 9 ++ src/libcamera/v4l2_subdevice.cpp | 118 +++++++++++++++++++++++++ 2 files changed, 127 insertions(+)