[libcamera-devel] [PATCH v4 2/2] libcamera: Introduce V4L2Device base class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jun 19 11:24:04 CEST 2019
Hi Jacopo,
On Wed, Jun 19, 2019 at 09:34:23AM +0200, Jacopo Mondi wrote:
> On Wed, Jun 19, 2019 at 12:07:45AM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 13, 2019 at 09:49:55AM +0200, Jacopo Mondi wrote:
> > > The V4L2 devices and subdevices share a few common operations,like
> > > opening and closing a device node, and perform IOCTLs on the device.
> > >
> > > With the forthcoming introduction of support for V4L2 controls, the
> > > quantity of shared code will drastically increase, as the control
> > > implementation is identical for the two.
> >
> > Drastically may be a bit of an overstatement ;-)
>
> Well, considering how thin it is now... :)
>
> > > To maximize code re-use and avoid duplications, provide a V4L2Device
> > > base class which groups the common operations and members.
> > >
> > > The newly introduced base class provides methods to open/close a device
> > > node, access the file descriptor, and perform IOCTLs on the device.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > > src/libcamera/include/v4l2_device.h | 36 +++++++
> > > src/libcamera/include/v4l2_subdevice.h | 6 +-
> > > src/libcamera/include/v4l2_videodevice.h | 5 +-
> > > src/libcamera/meson.build | 2 +
> > > src/libcamera/v4l2_device.cpp | 131 +++++++++++++++++++++++
> > > src/libcamera/v4l2_subdevice.cpp | 69 +++---------
> > > src/libcamera/v4l2_videodevice.cpp | 84 +++++----------
> > > 7 files changed, 212 insertions(+), 121 deletions(-)
> > > create mode 100644 src/libcamera/include/v4l2_device.h
> > > create mode 100644 src/libcamera/v4l2_device.cpp
> > >
> > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > > new file mode 100644
> > > index 000000000000..a73e1b600500
> > > --- /dev/null
> > > +++ b/src/libcamera/include/v4l2_device.h
> > > @@ -0,0 +1,36 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_device.h - Common base for V4L2 video devices and subdevices
> > > + */
> > > +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> > > +#define __LIBCAMERA_V4L2_DEVICE_H__
> > > +
> > > +#include <string>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class V4L2Device
> > > +{
> > > +public:
> > > + virtual ~V4L2Device() { close(); }
> >
> > I don't think the constructor needs to be virtual, as there are no other
> > virtual methods in the class, and we're not going to delete the
> > V4L2Device instance directly (I would make the destructor protected to
> > ensure that).
>
> Indeed, V4L2Device instances cannot be created directly, so no need
> for a virtual destructor.
It's not about whether they can be created directly or not.
class Base
{
public:
~Base() { }
}
class Derived : public Base
{
public:
Derived() { resource = new char[100]; }
~Derived() { delete[] resource; }
private:
char *resource;
}
int main
{
Derived *d = new Derived();
Base *b = d;
delete b;
return 0;
}
This will leak the resource, as deleting b will call Base::~Base() only.
If ~Base() was virtual, deleting b would call Derived::~Derived(),
correctly freeing resource.
In this case the destructor doesn't need to be virtual because no
instance of V4L2VideoDevice or V4L2Subdevice is deleted using a pointer
to V4L2Device.
> > > +
> > > +protected:
> > > + V4L2Device();
> > > +
> > > + int fd() { return fd_; }
> > > +
> > > + int open(const std::string &pathname, unsigned int flags);
> > > + void close();
> > > + bool isOpen() const;
> >
> > Shouldn't isOpen() and close() be public ? Otherwise a user of
> > V4L2VideoDevice or V4L2Subdevice won't be able to call them.
>
> No one calls them at the moment, appartently..
I thought so, otherwise the compiler would have told you. I think they
should still be made public.
> > I think you can inline isOpen() too.
> >
> > > +
> > > + int ioctl(unsigned long request, void *argp);
> > > +
> > > +private:
> > > + int fd_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > > index 3e4e5107aebe..a34b719f8c52 100644
> > > --- a/src/libcamera/include/v4l2_subdevice.h
> > > +++ b/src/libcamera/include/v4l2_subdevice.h
> > > @@ -16,6 +16,7 @@
> > > #include "formats.h"
> > > #include "log.h"
> > > #include "media_object.h"
> > > +#include "v4l2_device.h"
> > >
> > > namespace libcamera {
> > >
> > > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {
> > > const std::string toString() const;
> > > };
> > >
> > > -class V4L2Subdevice : protected Loggable
> > > +class V4L2Subdevice : public V4L2Device, protected Loggable
> > > {
> > > public:
> > > explicit V4L2Subdevice(const MediaEntity *entity);
> > > @@ -37,8 +38,6 @@ public:
> > > ~V4L2Subdevice();
> > >
> > > int open();
> > > - bool isOpen() const;
> > > - void close();
> > >
> > > const MediaEntity *entity() const { return entity_; }
> > >
> > > @@ -64,7 +63,6 @@ private:
> > > Rectangle *rect);
> > >
> > > const MediaEntity *entity_;
> > > - int fd_;
> > > };
> > >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > > index 6ecdb64e5f3c..15a83635b9bf 100644
> > > --- a/src/libcamera/include/v4l2_videodevice.h
> > > +++ b/src/libcamera/include/v4l2_videodevice.h
> > > @@ -17,6 +17,7 @@
> > > #include <libcamera/signal.h>
> > >
> > > #include "log.h"
> > > +#include "v4l2_device.h"
> > >
> > > namespace libcamera {
> > >
> > > @@ -111,7 +112,7 @@ public:
> > > const std::string toString() const;
> > > };
> > >
> > > -class V4L2VideoDevice : protected Loggable
> > > +class V4L2VideoDevice : public V4L2Device, protected Loggable
> > > {
> > > public:
> > > explicit V4L2VideoDevice(const std::string &deviceNode);
> > > @@ -122,7 +123,6 @@ public:
> > > V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
> > >
> > > int open();
> > > - bool isOpen() const;
> > > void close();
> > >
> > > const char *driverName() const { return caps_.driver(); }
> > > @@ -167,7 +167,6 @@ private:
> > > void bufferAvailable(EventNotifier *notifier);
> > >
> > > std::string deviceNode_;
> > > - int fd_;
> > > V4L2Capability caps_;
> > >
> > > enum v4l2_buf_type bufferType_;
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 15ab53b1abbe..f26ad5b2dc57 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -23,6 +23,7 @@ libcamera_sources = files([
> > > 'stream.cpp',
> > > 'timer.cpp',
> > > 'utils.cpp',
> > > + 'v4l2_device.cpp',
> > > 'v4l2_subdevice.cpp',
> > > 'v4l2_videodevice.cpp',
> > > ])
> > > @@ -41,6 +42,7 @@ libcamera_headers = files([
> > > 'include/media_object.h',
> > > 'include/pipeline_handler.h',
> > > 'include/utils.h',
> > > + 'include/v4l2_device.h',
> > > 'include/v4l2_subdevice.h',
> > > 'include/v4l2_videodevice.h',
> > > ])
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > new file mode 100644
> > > index 000000000000..6cbfc212a725
> > > --- /dev/null
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -0,0 +1,131 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_device.cpp - Common base for V4L2 video devices and subdevices
> > > + */
> > > +
> > > +#include "v4l2_device.h"
> > > +
> > > +#include <fcntl.h>
> > > +#include <string.h>
> > > +#include <sys/ioctl.h>
> > > +#include <unistd.h>
> > > +
> > > +#include "log.h"
> > > +
> > > +/**
> > > + * \file v4l2_device.h
> > > + * \brief Common base for V4L2 devices and subdevices
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(V4L2)
> > > +
> > > +/**
> > > + * \class V4L2Device
> > > + * \brief Base class for V4L2Videodev and V4L2Subdev
> > > + *
> > > + * The V4L2Device class groups together the methods and fields common to
> > > + * both V4L2Videodev and V4L2Subdev, and provide a base class which
> > > + * provides methods to open and close the device node associated with the
> > > + * device and to perform IOCTL system calls on it.
> > > + *
> > > + * The V4L2Device class cannot be instantiated directly, as its constructor
> > > + * is protected. Users should use instead one the derived classes to model
> > > + * either a V4L2 video device or a V4L2 subdevice.
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct a V4L2Device
> > > + *
> > > + * The V4L2Device constructor is protected and can only be accessed by the
> > > + * V4L2Videodev and V4L2Subdev derived classes.
> > > + *
> > > + * Initialize the file descriptor to -1.
> > > + */
> > > +V4L2Device::V4L2Device()
> >
> > Would it make sense to pass the device node name to the constructor and
> > remove it from the open() method ?
>
> We then would have to store it, and access it at open time.
> Currently we're not storing it, not sure it is worth doing it,
> considering all the users would then have to be changed.
>
> > > + : fd_(-1)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \fn V4L2Device::fd()
> > > + * \brief Retrieve the V4L2 device file descriptor
> > > + * \return The V4L2 device file descriptor, -1 if the device node is not open
> > > + */
> > > +
> > > +/**
> > > + * \brief Open a V4L2 device node
> > > + * \param pathname The filesystem path of the device node to open
> > > + * \param flags Access mode flags
> > > + *
> > > + * Initialize the file descriptor, which was initially set to -1.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int V4L2Device::open(const std::string &pathname, unsigned int flags)
> > > +{
> > > + if (isOpen()) {
> > > + LOG(V4L2, Error) << "Device already open";
> > > + return -EBUSY;
> > > + }
> > > +
> > > + int ret = ::open(pathname.c_str(), flags);
> > > + if (ret < 0) {
> > > + ret = -errno;
> > > + LOG(V4L2, Error) << "Failed to open V4L2 device: "
> > > + << strerror(-ret);
> >
> > I think I would keep this message in the callers, otherwise it won't
> > benefit from the inheritance from Loggable. Another option would be to
> > make V4L2Device inherit from loggable and move the logPrefix() method
> > there, while still keeping V4L2VideoDevice::logPrefix() to also print
> > the direction. I think that would be my favourite option.
>
> This maybe then call for storing the device node path
Yes it would. I think that would simplify the code.
> > > + return ret;
> > > + }
> > > +
> > > + fd_ = ret;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * \brief Close the device node
> > > + *
> > > + * Reset the file descriptor to -1
> > > + */
> > > +void V4L2Device::close()
> > > +{
> > > + if (fd_ < 0)
> > > + return;
> > > +
> > > + if (::close(fd_) < 0)
> > > + LOG(V4L2, Error) << "Failed to close V4L2 device: "
> > > + << strerror(errno);
> > > + fd_ = -1;
> > > +}
> > > +
> > > +/**
> > > + * \brief Check if the V4L2 device node is open
> > > + * \return True if the V4L2 device node is open, false otherwise
> > > + */
> > > +bool V4L2Device::isOpen() const
> > > +{
> > > + return fd_ != -1;
> > > +}
> > > +
> > > +/**
> > > + * \brief Perform an IOCTL system call on the device node
> > > + * \param request The IOCTL request code
> > > + * \param argp A pointer to the IOCTL argument
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int V4L2Device::ioctl(unsigned long request, void *argp)
> > > +{
> > > + /*
> > > + * Printing out an error message is usually better performed
> > > + * in the caller, which can provide more context.
> > > + */
> > > + if (::ioctl(fd_, request, argp) < 0)
> > > + return -errno;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 6681c1920065..52244d2ac3e1 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -29,7 +29,7 @@
> > >
> > > namespace libcamera {
> > >
> > > -LOG_DEFINE_CATEGORY(V4L2Subdev)
> > > +LOG_DECLARE_CATEGORY(V4L2)
> > >
> > > /**
> > > * \struct V4L2SubdeviceFormat
> > > @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> > > * path
> > > */
> > > V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> > > - : entity_(entity), fd_(-1)
> > > + : V4L2Device(), entity_(entity)
> > > {
> > > }
> > >
> > > @@ -117,45 +117,7 @@ V4L2Subdevice::~V4L2Subdevice()
> > > */
> > > int V4L2Subdevice::open()
> > > {
> > > - int ret;
> > > -
> > > - if (isOpen()) {
> > > - LOG(V4L2Subdev, Error) << "Device already open";
> > > - return -EBUSY;
> > > - }
> > > -
> > > - ret = ::open(entity_->deviceNode().c_str(), O_RDWR);
> > > - if (ret < 0) {
> > > - ret = -errno;
> > > - LOG(V4L2Subdev, Error)
> > > - << "Failed to open V4L2 subdevice '"
> > > - << entity_->deviceNode() << "': " << strerror(-ret);
> > > - return ret;
> > > - }
> > > - fd_ = ret;
> > > -
> > > - return 0;
> > > -}
> > > -
> > > -/**
> > > - * \brief Check if the subdevice is open
> > > - * \return True if the subdevice is open, false otherwise
> > > - */
> > > -bool V4L2Subdevice::isOpen() const
> > > -{
> > > - return fd_ != -1;
> > > -}
> > > -
> > > -/**
> > > - * \brief Close the subdevice, releasing any resources acquired by open()
> > > - */
> > > -void V4L2Subdevice::close()
> > > -{
> > > - if (!isOpen())
> > > - return;
> > > -
> > > - ::close(fd_);
> > > - fd_ = -1;
> > > + return V4L2Device::open(entity_->deviceNode(), O_RDWR);
> > > }
> > >
> > > /**
> > > @@ -207,7 +169,7 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)
> > > int ret;
> > >
> > > if (pad >= entity_->pads().size()) {
> > > - LOG(V4L2Subdev, Error) << "Invalid pad: " << pad;
> > > + LOG(V4L2, Error) << "Invalid pad: " << pad;
> > > return formatMap;
> > > }
> > >
> > > @@ -215,7 +177,7 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)
> > > mbusEnum.index = 0;
> > > mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > while (true) {
> > > - ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > > + ret = ioctl(VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > > if (ret)
> > > break;
> > >
> > > @@ -229,7 +191,7 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)
> > >
> > > if (ret && (errno != EINVAL && errno != ENOTTY)) {
> >
> > This should be replaced with
> >
> > if (ret < 0 && ret != -EINVAL && ret != -ENOTTY)
> >
> > > ret = -errno;
> >
> > and this line should be removed.
>
> Correct, thanks!
>
> > > - LOG(V4L2Subdev, Error)
> > > + LOG(V4L2, Error)
> > > << "Unable to enumerate formats on pad " << pad
> > > << ": " << strerror(-ret);
> > > formatMap.clear();
> > > @@ -250,10 +212,9 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> > > subdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > subdevFmt.pad = pad;
> > >
> > > - int ret = ioctl(fd_, VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > > + int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > > if (ret) {
> > > - ret = -errno;
> > > - LOG(V4L2Subdev, Error)
> > > + LOG(V4L2, Error)
> > > << "Unable to get format on pad " << pad
> > > << ": " << strerror(-ret);
> > > return ret;
> > > @@ -286,10 +247,9 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> > > subdevFmt.format.height = format->size.height;
> > > subdevFmt.format.code = format->mbus_code;
> > >
> > > - int ret = ioctl(fd_, VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > > + int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > > if (ret) {
> > > - ret = -errno;
> > > - LOG(V4L2Subdev, Error)
> > > + LOG(V4L2, Error)
> > > << "Unable to set format on pad " << pad
> > > << ": " << strerror(-ret);
> > > return ret;
> > > @@ -339,7 +299,7 @@ int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,
> > > sizeEnum.code = code;
> > > sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > while (true) {
> > > - ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > > + ret = ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > > if (ret)
> > > break;
> > >
> > > @@ -351,7 +311,7 @@ int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,
> > >
> > > if (ret && (errno != EINVAL && errno != ENOTTY)) {
> > > ret = -errno;
> >
> > Same here.
> >
> > > - LOG(V4L2Subdev, Error)
> > > + LOG(V4L2, Error)
> > > << "Unable to enumerate sizes on pad " << pad
> > > << ": " << strerror(-ret);
> > > sizes->clear();
> > > @@ -377,10 +337,9 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > > sel.r.width = rect->w;
> > > sel.r.height = rect->h;
> > >
> > > - int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
> > > + int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
> > > if (ret < 0) {
> > > - ret = -errno;
> > > - LOG(V4L2Subdev, Error)
> > > + LOG(V4L2, Error)
> > > << "Unable to set rectangle " << target << " on pad "
> > > << pad << ": " << strerror(-ret);
> > > return ret;
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 8957cf8f97d3..2172cf25bfb6 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -30,7 +30,7 @@
> > > */
> > > namespace libcamera {
> > >
> > > -LOG_DEFINE_CATEGORY(V4L2)
> > > +LOG_DECLARE_CATEGORY(V4L2)
> > >
> > > /**
> > > * \struct V4L2Capability
> > > @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
> > > * \param[in] deviceNode The file-system path to the video device node
> > > */
> > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > > - : deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),
> > > + : V4L2Device(), deviceNode_(deviceNode), bufferPool_(nullptr),
> > > queuedBuffersCount_(0), fdEvent_(nullptr)
> > > {
> > > /*
> > > @@ -305,23 +305,12 @@ int V4L2VideoDevice::open()
> > > {
> > > int ret;
> > >
> > > - if (isOpen()) {
> > > - LOG(V4L2, Error) << "Device already open";
> > > - return -EBUSY;
> > > - }
> > > -
> > > - ret = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);
> > > - if (ret < 0) {
> > > - ret = -errno;
> > > - LOG(V4L2, Error)
> > > - << "Failed to open V4L2 device: " << strerror(-ret);
> > > + ret = V4L2Device::open(deviceNode_, O_RDWR | O_NONBLOCK);
> > > + if (ret < 0)
> > > return ret;
> > > - }
> > > - fd_ = ret;
> > >
> > > - ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
> > > + ret = ioctl(VIDIOC_QUERYCAP, &caps_);
> > > if (ret < 0) {
> > > - ret = -errno;
> > > LOG(V4L2, Error)
> > > << "Failed to query device capabilities: "
> > > << strerror(-ret);
> > > @@ -342,20 +331,20 @@ int V4L2VideoDevice::open()
> > > * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).
> > > */
> > > if (caps_.isVideoCapture()) {
> > > - fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > > + fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> > > bufferType_ = caps_.isMultiplanar()
> > > ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> > > : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > } else if (caps_.isVideoOutput()) {
> > > - fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > > + fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> > > bufferType_ = caps_.isMultiplanar()
> > > ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> > > : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > > } else if (caps_.isMetaCapture()) {
> > > - fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > > + fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> > > bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> > > } else if (caps_.isMetaOutput()) {
> > > - fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > > + fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> > > bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> > > } else {
> > > LOG(V4L2, Error) << "Device is not a supported type";
> > > @@ -368,28 +357,18 @@ int V4L2VideoDevice::open()
> > > return 0;
> > > }
> > >
> > > -/**
> > > - * \brief Check if device is successfully opened
> > > - * \return True if the device is open, false otherwise
> > > - */
> > > -bool V4L2VideoDevice::isOpen() const
> > > -{
> > > - return fd_ != -1;
> > > -}
> > > -
> > > /**
> > > * \brief Close the device, releasing any resources acquired by open()
> > > */
> > > void V4L2VideoDevice::close()
> > > {
> > > - if (fd_ < 0)
> > > + if (fd() < 0)
> >
> > How about
> >
> > if (isOpen())
> >
> > > return;
> > >
> > > + close();
> >
> > Won't the compiler call V4L2VideoDevice::close() ? Shouldn't you write
> >
> > V4L2Device::close();
>
> This is the V4L2VideoDevice::close() method :)
Which then calls itself back, leading to infinite recursion :-) I wonder
why this hasn't been caught by tests, it would be worth checking if
V4L2Device::close() gets called (just log a message there, it should
then be obvious).
And now that I wrote this, the reason is quite clear. close() is called
on an instance of V4L2Device by V4L2Device::V4L2Device(), so it calls
V4L2Device::close(), not V4L2VideoDevice::close(). You need to make the
close method virtual in V4L2Device.
> > > +
> > > releaseBuffers();
> >
> > This calls an ioctl, so the close() call should be moved to the end of
> > the function.
>
> Ouch, correct, I'll change it
>
> > > delete fdEvent_;
> > > -
> > > - ::close(fd_);
> > > - fd_ = -1;
> > > }
> > >
> > > /**
> > > @@ -462,9 +441,8 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> > > int ret;
> > >
> > > v4l2Format.type = bufferType_;
> > > - ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > + ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > > if (ret) {
> > > - ret = -errno;
> > > LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > > return ret;
> > > }
> > > @@ -488,9 +466,8 @@ int V4L2VideoDevice::setFormatMeta(V4L2DeviceFormat *format)
> > > v4l2Format.type = bufferType_;
> > > pix->dataformat = format->fourcc;
> > > pix->buffersize = format->planes[0].size;
> > > - ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > + ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > > if (ret) {
> > > - ret = -errno;
> > > LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > > return ret;
> > > }
> > > @@ -516,9 +493,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
> > > int ret;
> > >
> > > v4l2Format.type = bufferType_;
> > > - ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > + ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > > if (ret) {
> > > - ret = -errno;
> > > LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > > return ret;
> > > }
> > > @@ -554,9 +530,8 @@ int V4L2VideoDevice::setFormatMultiplane(V4L2DeviceFormat *format)
> > > pix->plane_fmt[i].sizeimage = format->planes[i].size;
> > > }
> > >
> > > - ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > + ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > > if (ret) {
> > > - ret = -errno;
> > > LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > > return ret;
> > > }
> > > @@ -584,9 +559,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
> > > int ret;
> > >
> > > v4l2Format.type = bufferType_;
> > > - ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > + ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > > if (ret) {
> > > - ret = -errno;
> > > LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > > return ret;
> > > }
> > > @@ -613,9 +587,8 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)
> > > pix->pixelformat = format->fourcc;
> > > pix->bytesperline = format->planes[0].bpl;
> > > pix->field = V4L2_FIELD_NONE;
> > > - ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > + ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > > if (ret) {
> > > - ret = -errno;
> > > LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > > return ret;
> > > }
> > > @@ -643,9 +616,8 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)
> > > rb.type = bufferType_;
> > > rb.memory = memoryType_;
> > >
> > > - ret = ioctl(fd_, VIDIOC_REQBUFS, &rb);
> > > + ret = ioctl(VIDIOC_REQBUFS, &rb);
> > > if (ret < 0) {
> > > - ret = -errno;
> > > LOG(V4L2, Error)
> > > << "Unable to request " << count << " buffers: "
> > > << strerror(-ret);
> > > @@ -695,9 +667,8 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)
> > > buf.length = VIDEO_MAX_PLANES;
> > > buf.m.planes = planes;
> > >
> > > - ret = ioctl(fd_, VIDIOC_QUERYBUF, &buf);
> > > + ret = ioctl(VIDIOC_QUERYBUF, &buf);
> > > if (ret < 0) {
> > > - ret = -errno;
> > > LOG(V4L2, Error)
> > > << "Unable to query buffer " << i << ": "
> > > << strerror(-ret);
> > > @@ -748,9 +719,8 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,
> > > expbuf.plane = planeIndex;
> > > expbuf.flags = O_RDWR;
> > >
> > > - ret = ioctl(fd_, VIDIOC_EXPBUF, &expbuf);
> > > + ret = ioctl(VIDIOC_EXPBUF, &expbuf);
> > > if (ret < 0) {
> > > - ret = -errno;
> > > LOG(V4L2, Error)
> > > << "Failed to export buffer: " << strerror(-ret);
> > > return ret;
> > > @@ -855,9 +825,8 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> > >
> > > LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
> > >
> > > - ret = ioctl(fd_, VIDIOC_QBUF, &buf);
> > > + ret = ioctl(VIDIOC_QBUF, &buf);
> > > if (ret < 0) {
> > > - ret = -errno;
> > > LOG(V4L2, Error)
> > > << "Failed to queue buffer " << buf.index << ": "
> > > << strerror(-ret);
> > > @@ -892,9 +861,8 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> > > buf.m.planes = planes;
> > > }
> > >
> > > - ret = ioctl(fd_, VIDIOC_DQBUF, &buf);
> > > + ret = ioctl(VIDIOC_DQBUF, &buf);
> > > if (ret < 0) {
> > > - ret = -errno;
> > > LOG(V4L2, Error)
> > > << "Failed to dequeue buffer: " << strerror(-ret);
> > > return nullptr;
> > > @@ -952,9 +920,8 @@ int V4L2VideoDevice::streamOn()
> > > {
> > > int ret;
> > >
> > > - ret = ioctl(fd_, VIDIOC_STREAMON, &bufferType_);
> > > + ret = ioctl(VIDIOC_STREAMON, &bufferType_);
> > > if (ret < 0) {
> > > - ret = -errno;
> > > LOG(V4L2, Error)
> > > << "Failed to start streaming: " << strerror(-ret);
> > > return ret;
> > > @@ -975,9 +942,8 @@ int V4L2VideoDevice::streamOff()
> > > {
> > > int ret;
> > >
> > > - ret = ioctl(fd_, VIDIOC_STREAMOFF, &bufferType_);
> > > + ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> > > if (ret < 0) {
> > > - ret = -errno;
> > > LOG(V4L2, Error)
> > > << "Failed to stop streaming: " << strerror(-ret);
> > > return ret;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list