| Message ID | 20190612163604.10037-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-06-12 18:36:04 +0200, Jacopo Mondi wrote: > he V4L2 devices and subdevices share a few common operations,like s/^he/The/ > 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. > > 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@jmondi.org> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@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 | 148 +++++++++++++++++++++++ > src/libcamera/v4l2_subdevice.cpp | 69 +++-------- > src/libcamera/v4l2_videodevice.cpp | 82 ++++--------- > 7 files changed, 227 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..584a570a4703 > --- /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(); } > + > +protected: > + V4L2Device(); > + > + int fd() { return fd_; } > + > + int open(const std::string &pathname, unsigned int flags); > + int close(); > + bool isOpen() const; > + > + 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..bddd10e6f83c > --- /dev/null > +++ b/src/libcamera/v4l2_device.cpp > @@ -0,0 +1,148 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * v4l2_device.cpp - Common base for V4L2 devices and subdevices One line summary is different form the .h file. v4l2_device.h - 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() > + : 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); > + return ret; > + } > + > + fd_ = ret; > + > + return 0; > +} > + > +/** > + * \brief Close the device node > + * > + * Reset the file descriptor to -1 > + * > + * \return 0 on success or a negative error code otherwise > + */ > +int V4L2Device::close() > +{ > + if (fd_ < 0) > + return 0; > + > + int ret = ::close(fd_); > + if (ret < 0) { > + ret = -errno; > + LOG(V4L2, Error) << "Failed to close V4L2 device: " > + << strerror(-ret); > + return ret; Is it really a good idea to return the error here and not set fd_ to -1 ? I wonder if we should not keep the same signature as before the introduction of the base class and V4L2Device::close() return a void. Looking in our code base not a single call site checks the return of any close() call. And I'm not sure how we would recover if a close would fail. Either we make turn the Error into Fatal or we reset the fd anyhow and potentially leak it. > + } > + > + fd_ = -1; > + > + return 0; > +} > + > +/** > + * \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; > +} > + > +/** > + * \var V4L2Device::fd_ > + * \brief The V4L2 device node file descriptor nit: This is a private member of V4L2Device, I'm not sure it really needs to be documented. > + * > + * The file descriptor is initialized to -1 and reset to this value once > + * the device node gets closed. > + */ > + > +} /* 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) I'm not sure what I think about using the same log category for the base and the specific implementation. What is the upside of only having the one? > > /** > * \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)) { > ret = -errno; > - 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; > - 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..bc5b306e5ca3 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,16 @@ 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) > return; > > releaseBuffers(); > delete fdEvent_; > - > - ::close(fd_); > - fd_ = -1; Should you not call V4L2Device::close() here? > } > > /** > @@ -462,9 +439,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 +464,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 +491,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 +528,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 +557,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 +585,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 +614,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 +665,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 +717,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 +823,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 +859,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 +918,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 +940,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; > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Wed, Jun 12, 2019 at 10:56:43PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2019-06-12 18:36:04 +0200, Jacopo Mondi wrote: > > he V4L2 devices and subdevices share a few common operations,like > > s/^he/The/ > Ups, missed that from Kieran's review. > > 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. > > > > 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@jmondi.org> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@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 | 148 +++++++++++++++++++++++ > > src/libcamera/v4l2_subdevice.cpp | 69 +++-------- > > src/libcamera/v4l2_videodevice.cpp | 82 ++++--------- > > 7 files changed, 227 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..584a570a4703 > > --- /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(); } > > + > > +protected: > > + V4L2Device(); > > + > > + int fd() { return fd_; } > > + > > + int open(const std::string &pathname, unsigned int flags); > > + int close(); > > + bool isOpen() const; > > + > > + 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..bddd10e6f83c > > --- /dev/null > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -0,0 +1,148 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * v4l2_device.cpp - Common base for V4L2 devices and subdevices > > One line summary is different form the .h file. > > v4l2_device.h - Common base for V4L2 video devices and subdevices > Thanks! nice catch! > > + */ > > + > > +#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() > > + : 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); > > + return ret; > > + } > > + > > + fd_ = ret; > > + > > + return 0; > > +} > > + > > +/** > > + * \brief Close the device node > > + * > > + * Reset the file descriptor to -1 > > + * > > + * \return 0 on success or a negative error code otherwise > > + */ > > +int V4L2Device::close() > > +{ > > + if (fd_ < 0) > > + return 0; > > + > > + int ret = ::close(fd_); > > + if (ret < 0) { > > + ret = -errno; > > + LOG(V4L2, Error) << "Failed to close V4L2 device: " > > + << strerror(-ret); > > + return ret; > > Is it really a good idea to return the error here and not set fd_ to -1 > ? I wonder if we should not keep the same signature as before the > introduction of the base class and V4L2Device::close() return a void. > Possibly, yes. > Looking in our code base not a single call site checks the return of any > close() call. And I'm not sure how we would recover if a close would > fail. Either we make turn the Error into Fatal or we reset the fd anyhow > and potentially leak it. I should probably just ignore the ::close error and reset the fd to -1 anyhow. However, I would print out a message in case of error. > > > + } > > + > > + fd_ = -1; > > + > > + return 0; > > +} > > + > > +/** > > + * \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; > > +} > > + > > +/** > > + * \var V4L2Device::fd_ > > + * \brief The V4L2 device node file descriptor > > nit: This is a private member of V4L2Device, I'm not sure it really > needs to be documented. > No, it does not, and the comment doesn't tell anything particularly useful. > > + * > > + * The file descriptor is initialized to -1 and reset to this value once > > + * the device node gets closed. > > + */ > > + > > +} /* 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) > > I'm not sure what I think about using the same log category for the base > and the specific implementation. What is the upside of only having the > one? > As the derived classes call into the base methods, we would end up with the call path to an error printed out with different tags. I think it's better to have the same in all three (the better would be for have the base class use the tag defined by the derived class the failing operation has been called on. I'm not sure it is possible though). > > > > /** > > * \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)) { > > ret = -errno; > > - 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; > > - 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..bc5b306e5ca3 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,16 @@ 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) > > return; > > > > releaseBuffers(); > > delete fdEvent_; > > - > > - ::close(fd_); > > - fd_ = -1; > > Should you not call V4L2Device::close() here? > Indeed I should! I thought I had it! Nice catch! Thanks j > > } > > > > /** > > @@ -462,9 +439,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 +464,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 +491,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 +528,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 +557,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 +585,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 +614,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 +665,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 +717,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 +823,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 +859,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 +918,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 +940,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; > > -- > > 2.21.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h new file mode 100644 index 000000000000..584a570a4703 --- /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(); } + +protected: + V4L2Device(); + + int fd() { return fd_; } + + int open(const std::string &pathname, unsigned int flags); + int close(); + bool isOpen() const; + + 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..bddd10e6f83c --- /dev/null +++ b/src/libcamera/v4l2_device.cpp @@ -0,0 +1,148 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * v4l2_device.cpp - Common base for V4L2 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() + : 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); + return ret; + } + + fd_ = ret; + + return 0; +} + +/** + * \brief Close the device node + * + * Reset the file descriptor to -1 + * + * \return 0 on success or a negative error code otherwise + */ +int V4L2Device::close() +{ + if (fd_ < 0) + return 0; + + int ret = ::close(fd_); + if (ret < 0) { + ret = -errno; + LOG(V4L2, Error) << "Failed to close V4L2 device: " + << strerror(-ret); + return ret; + } + + fd_ = -1; + + return 0; +} + +/** + * \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; +} + +/** + * \var V4L2Device::fd_ + * \brief The V4L2 device node file descriptor + * + * The file descriptor is initialized to -1 and reset to this value once + * the device node gets closed. + */ + +} /* 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)) { ret = -errno; - 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; - 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..bc5b306e5ca3 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,16 @@ 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) return; releaseBuffers(); delete fdEvent_; - - ::close(fd_); - fd_ = -1; } /** @@ -462,9 +439,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 +464,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 +491,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 +528,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 +557,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 +585,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 +614,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 +665,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 +717,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 +823,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 +859,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 +918,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 +940,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;