[libcamera-devel] [PATCH v3 09/16] libcamera: v4l2_device: Add enumeration of pixelformats and frame sizes
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jun 19 03:28:51 CEST 2019
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 at ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo at 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 at 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list