Message ID | 20190616133402.21934-10-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Niklas, Thank you for the patch. On Sun, Jun 16, 2019 at 03:33:55PM +0200, Niklas Söderlund wrote: > Add methods to enumerate pixelformats and frame sizes from a v4l2 > device. The interface is similar to how V4L2Subdevice enumerates mbus > codes and frame sizes. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_device.h | 5 ++ > src/libcamera/v4l2_device.cpp | 110 ++++++++++++++++++++++++++++ > 2 files changed, 115 insertions(+) > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index bdecc087fe5afae0..1a67850ac4c1088f 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -16,6 +16,7 @@ > #include <libcamera/geometry.h> > #include <libcamera/signal.h> > > +#include "formats.h" > #include "log.h" > > namespace libcamera { > @@ -132,6 +133,7 @@ public: > > int getFormat(V4L2DeviceFormat *format); > int setFormat(V4L2DeviceFormat *format); > + ImageFormats formats(); > > int exportBuffers(BufferPool *pool); > int importBuffers(BufferPool *pool); > @@ -163,6 +165,9 @@ private: > int createPlane(Buffer *buffer, unsigned int plane, > unsigned int length); > > + std::vector<unsigned int> enumPixelformats(); > + std::vector<SizeRange> enumSizes(unsigned int pixelFormat); > + > Buffer *dequeueBuffer(); > void bufferAvailable(EventNotifier *notifier); > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 0821bd75fb428766..96aa525025629697 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -634,6 +634,29 @@ int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *format) > return 0; > } > > +/** > + * \brief Enumerate all pixel formats and frame sizes > + * > + * Enumerate all pixel formats and frame sizes supported by the video device. > + * > + * \return A list of the supported device formats > + */ > +ImageFormats V4L2Device::formats() > +{ > + ImageFormats formats; > + > + for (unsigned int pixelformat : enumPixelformats()) { > + if (formats.addFormat(pixelformat, enumSizes(pixelformat))) { > + LOG(V4L2, Error) > + << "Could not add sizes for pixel format " > + << pixelformat; > + return {}; > + } I think you should catch the error from enumSizes() and return {}. I would also extend the documentation a little bit, you can copy the one from the V4L2Subdevice class. > + } > + > + return formats; > +} > + > int V4L2Device::requestBuffers(unsigned int count) > { > struct v4l2_requestbuffers rb = {}; > @@ -763,6 +786,93 @@ int V4L2Device::createPlane(Buffer *buffer, unsigned int planeIndex, > return 0; > } > > +std::vector<unsigned int> V4L2Device::enumPixelformats() > +{ > + std::vector<unsigned int> formats; > + int ret; > + > + for (unsigned int index = 0; ; index++) { > + struct v4l2_fmtdesc pixelformatEnum = {}; > + pixelformatEnum.index = index; > + pixelformatEnum.type = bufferType_; > + > + ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum); > + if (ret) > + break; > + > + formats.push_back(pixelformatEnum.pixelformat); > + } > + > + if (ret && errno != EINVAL) { > + ret = -errno; > + LOG(V4L2, Error) > + << "Unable to enumerate pixelformats: " s/pixelformats/pixel formats/ > + << strerror(-ret); > + return {}; > + } > + > + return formats; > +} > + > +std::vector<SizeRange> V4L2Device::enumSizes(unsigned int pixelFormat) > +{ > + std::vector<SizeRange> sizes; > + int ret; > + > + for (unsigned int index = 0;; index++) { > + struct v4l2_frmsizeenum frameSize = {}; > + frameSize.index = index; > + frameSize.pixel_format = pixelFormat; > + > + ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize); > + if (ret) > + break; > + > + if (index != 0 && > + frameSize.type != V4L2_FRMSIZE_TYPE_DISCRETE) { > + LOG(V4L2, Error) > + << "Non-zero index for non discrete type"; > + return {}; > + } > + > + switch (frameSize.type) { > + case V4L2_FRMSIZE_TYPE_DISCRETE: > + sizes.emplace_back(frameSize.discrete.width, > + frameSize.discrete.height); > + break; > + case V4L2_FRMSIZE_TYPE_CONTINUOUS: > + sizes.emplace_back(frameSize.stepwise.min_width, > + frameSize.stepwise.min_height, > + frameSize.stepwise.max_width, > + frameSize.stepwise.max_height); > + break; > + case V4L2_FRMSIZE_TYPE_STEPWISE: > + sizes.emplace_back(frameSize.stepwise.min_width, > + frameSize.stepwise.min_height, > + frameSize.stepwise.max_width, > + frameSize.stepwise.max_height, > + frameSize.stepwise.step_width, > + frameSize.stepwise.step_height); > + break; > + default: > + LOG(V4L2, Error) > + << "Unknown VIDIOC_ENUM_FRAMESIZES type: " Nit-picking, s/:// With these small issues fixed (and trusting your documentation skills for the formats() method), Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + << frameSize.type; > + return {}; > + } > + } > + > + if (ret && errno != EINVAL) { > + ret = -errno; > + LOG(V4L2, Error) > + << "Unable to enumerate frame sizes: " > + << strerror(-ret); > + return {}; > + } > + > + return sizes; > +} > + > /** > * \brief Import the externally allocated \a pool of buffers > * \param[in] pool BufferPool of buffers to import
Hi Laurent, Thanks for your feedback, On 2019-06-19 02:07:27 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Sun, Jun 16, 2019 at 03:33:55PM +0200, Niklas Söderlund wrote: > > Add methods to enumerate pixelformats and frame sizes from a v4l2 > > device. The interface is similar to how V4L2Subdevice enumerates mbus > > codes and frame sizes. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/include/v4l2_device.h | 5 ++ > > src/libcamera/v4l2_device.cpp | 110 ++++++++++++++++++++++++++++ > > 2 files changed, 115 insertions(+) > > > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > index bdecc087fe5afae0..1a67850ac4c1088f 100644 > > --- a/src/libcamera/include/v4l2_device.h > > +++ b/src/libcamera/include/v4l2_device.h > > @@ -16,6 +16,7 @@ > > #include <libcamera/geometry.h> > > #include <libcamera/signal.h> > > > > +#include "formats.h" > > #include "log.h" > > > > namespace libcamera { > > @@ -132,6 +133,7 @@ public: > > > > int getFormat(V4L2DeviceFormat *format); > > int setFormat(V4L2DeviceFormat *format); > > + ImageFormats formats(); > > > > int exportBuffers(BufferPool *pool); > > int importBuffers(BufferPool *pool); > > @@ -163,6 +165,9 @@ private: > > int createPlane(Buffer *buffer, unsigned int plane, > > unsigned int length); > > > > + std::vector<unsigned int> enumPixelformats(); > > + std::vector<SizeRange> enumSizes(unsigned int pixelFormat); > > + > > Buffer *dequeueBuffer(); > > void bufferAvailable(EventNotifier *notifier); > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 0821bd75fb428766..96aa525025629697 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -634,6 +634,29 @@ int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *format) > > return 0; > > } > > > > +/** > > + * \brief Enumerate all pixel formats and frame sizes > > + * > > + * Enumerate all pixel formats and frame sizes supported by the video device. > > + * > > + * \return A list of the supported device formats > > + */ > > +ImageFormats V4L2Device::formats() > > +{ > > + ImageFormats formats; > > + > > + for (unsigned int pixelformat : enumPixelformats()) { > > + if (formats.addFormat(pixelformat, enumSizes(pixelformat))) { > > + LOG(V4L2, Error) > > + << "Could not add sizes for pixel format " > > + << pixelformat; > > + return {}; > > + } > > I think you should catch the error from enumSizes() and return {}. I > would also extend the documentation a little bit, you can copy the one > from the V4L2Subdevice class. I agree that enumSizes() should have a error check and I added that for next version. About the documentation I'm not sure what you are asking, the V4L2Device::formats() documentation is already an adapted copy of V4L2Subdevice::formats(). I have addressed all other comments in this patch but I have not collected your tag until I understand what you are asking here ;-) > > > + } > > + > > + return formats; > > +} > > + > > int V4L2Device::requestBuffers(unsigned int count) > > { > > struct v4l2_requestbuffers rb = {}; > > @@ -763,6 +786,93 @@ int V4L2Device::createPlane(Buffer *buffer, unsigned int planeIndex, > > return 0; > > } > > > > +std::vector<unsigned int> V4L2Device::enumPixelformats() > > +{ > > + std::vector<unsigned int> formats; > > + int ret; > > + > > + for (unsigned int index = 0; ; index++) { > > + struct v4l2_fmtdesc pixelformatEnum = {}; > > + pixelformatEnum.index = index; > > + pixelformatEnum.type = bufferType_; > > + > > + ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum); > > + if (ret) > > + break; > > + > > + formats.push_back(pixelformatEnum.pixelformat); > > + } > > + > > + if (ret && errno != EINVAL) { > > + ret = -errno; > > + LOG(V4L2, Error) > > + << "Unable to enumerate pixelformats: " > > s/pixelformats/pixel formats/ > > > + << strerror(-ret); > > + return {}; > > + } > > + > > + return formats; > > +} > > + > > +std::vector<SizeRange> V4L2Device::enumSizes(unsigned int pixelFormat) > > +{ > > + std::vector<SizeRange> sizes; > > + int ret; > > + > > + for (unsigned int index = 0;; index++) { > > + struct v4l2_frmsizeenum frameSize = {}; > > + frameSize.index = index; > > + frameSize.pixel_format = pixelFormat; > > + > > + ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize); > > + if (ret) > > + break; > > + > > + if (index != 0 && > > + frameSize.type != V4L2_FRMSIZE_TYPE_DISCRETE) { > > + LOG(V4L2, Error) > > + << "Non-zero index for non discrete type"; > > + return {}; > > + } > > + > > + switch (frameSize.type) { > > + case V4L2_FRMSIZE_TYPE_DISCRETE: > > + sizes.emplace_back(frameSize.discrete.width, > > + frameSize.discrete.height); > > + break; > > + case V4L2_FRMSIZE_TYPE_CONTINUOUS: > > + sizes.emplace_back(frameSize.stepwise.min_width, > > + frameSize.stepwise.min_height, > > + frameSize.stepwise.max_width, > > + frameSize.stepwise.max_height); > > + break; > > + case V4L2_FRMSIZE_TYPE_STEPWISE: > > + sizes.emplace_back(frameSize.stepwise.min_width, > > + frameSize.stepwise.min_height, > > + frameSize.stepwise.max_width, > > + frameSize.stepwise.max_height, > > + frameSize.stepwise.step_width, > > + frameSize.stepwise.step_height); > > + break; > > + default: > > + LOG(V4L2, Error) > > + << "Unknown VIDIOC_ENUM_FRAMESIZES type: " > > Nit-picking, s/:// > > With these small issues fixed (and trusting your documentation skills > for the formats() method), > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + << frameSize.type; > > + return {}; > > + } > > + } > > + > > + if (ret && errno != EINVAL) { > > + ret = -errno; > > + LOG(V4L2, Error) > > + << "Unable to enumerate frame sizes: " > > + << strerror(-ret); > > + return {}; > > + } > > + > > + return sizes; > > +} > > + > > /** > > * \brief Import the externally allocated \a pool of buffers > > * \param[in] pool BufferPool of buffers to import > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h index bdecc087fe5afae0..1a67850ac4c1088f 100644 --- a/src/libcamera/include/v4l2_device.h +++ b/src/libcamera/include/v4l2_device.h @@ -16,6 +16,7 @@ #include <libcamera/geometry.h> #include <libcamera/signal.h> +#include "formats.h" #include "log.h" namespace libcamera { @@ -132,6 +133,7 @@ public: int getFormat(V4L2DeviceFormat *format); int setFormat(V4L2DeviceFormat *format); + ImageFormats formats(); int exportBuffers(BufferPool *pool); int importBuffers(BufferPool *pool); @@ -163,6 +165,9 @@ private: int createPlane(Buffer *buffer, unsigned int plane, unsigned int length); + std::vector<unsigned int> enumPixelformats(); + std::vector<SizeRange> enumSizes(unsigned int pixelFormat); + Buffer *dequeueBuffer(); void bufferAvailable(EventNotifier *notifier); diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 0821bd75fb428766..96aa525025629697 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -634,6 +634,29 @@ int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *format) return 0; } +/** + * \brief Enumerate all pixel formats and frame sizes + * + * Enumerate all pixel formats and frame sizes supported by the video device. + * + * \return A list of the supported device formats + */ +ImageFormats V4L2Device::formats() +{ + ImageFormats formats; + + for (unsigned int pixelformat : enumPixelformats()) { + if (formats.addFormat(pixelformat, enumSizes(pixelformat))) { + LOG(V4L2, Error) + << "Could not add sizes for pixel format " + << pixelformat; + return {}; + } + } + + return formats; +} + int V4L2Device::requestBuffers(unsigned int count) { struct v4l2_requestbuffers rb = {}; @@ -763,6 +786,93 @@ int V4L2Device::createPlane(Buffer *buffer, unsigned int planeIndex, return 0; } +std::vector<unsigned int> V4L2Device::enumPixelformats() +{ + std::vector<unsigned int> formats; + int ret; + + for (unsigned int index = 0; ; index++) { + struct v4l2_fmtdesc pixelformatEnum = {}; + pixelformatEnum.index = index; + pixelformatEnum.type = bufferType_; + + ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum); + if (ret) + break; + + formats.push_back(pixelformatEnum.pixelformat); + } + + if (ret && errno != EINVAL) { + ret = -errno; + LOG(V4L2, Error) + << "Unable to enumerate pixelformats: " + << strerror(-ret); + return {}; + } + + return formats; +} + +std::vector<SizeRange> V4L2Device::enumSizes(unsigned int pixelFormat) +{ + std::vector<SizeRange> sizes; + int ret; + + for (unsigned int index = 0;; index++) { + struct v4l2_frmsizeenum frameSize = {}; + frameSize.index = index; + frameSize.pixel_format = pixelFormat; + + ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize); + if (ret) + break; + + if (index != 0 && + frameSize.type != V4L2_FRMSIZE_TYPE_DISCRETE) { + LOG(V4L2, Error) + << "Non-zero index for non discrete type"; + return {}; + } + + switch (frameSize.type) { + case V4L2_FRMSIZE_TYPE_DISCRETE: + sizes.emplace_back(frameSize.discrete.width, + frameSize.discrete.height); + break; + case V4L2_FRMSIZE_TYPE_CONTINUOUS: + sizes.emplace_back(frameSize.stepwise.min_width, + frameSize.stepwise.min_height, + frameSize.stepwise.max_width, + frameSize.stepwise.max_height); + break; + case V4L2_FRMSIZE_TYPE_STEPWISE: + sizes.emplace_back(frameSize.stepwise.min_width, + frameSize.stepwise.min_height, + frameSize.stepwise.max_width, + frameSize.stepwise.max_height, + frameSize.stepwise.step_width, + frameSize.stepwise.step_height); + break; + default: + LOG(V4L2, Error) + << "Unknown VIDIOC_ENUM_FRAMESIZES type: " + << frameSize.type; + return {}; + } + } + + if (ret && errno != EINVAL) { + ret = -errno; + LOG(V4L2, Error) + << "Unable to enumerate frame sizes: " + << strerror(-ret); + return {}; + } + + return sizes; +} + /** * \brief Import the externally allocated \a pool of buffers * \param[in] pool BufferPool of buffers to import