[libcamera-devel] [PATCH v5 2/4] libcamera: Introduce V4L2Device base class
Jacopo Mondi
jacopo at jmondi.org
Wed Jun 19 14:26:54 CEST 2019
Hi Laurent,
On Wed, Jun 19, 2019 at 03:20:29PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jun 19, 2019 at 01:05:46PM +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 increase, as the control support
> > implementation is identical for the two derived classes.
> >
> > 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 | 43 +++++++
> > src/libcamera/include/v4l2_subdevice.h | 9 +-
> > src/libcamera/include/v4l2_videodevice.h | 10 +-
> > src/libcamera/meson.build | 2 +
> > src/libcamera/v4l2_device.cpp | 144 +++++++++++++++++++++++
> > src/libcamera/v4l2_subdevice.cpp | 84 +++----------
> > src/libcamera/v4l2_videodevice.cpp | 103 +++++-----------
> > 7 files changed, 239 insertions(+), 156 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..2c26f3eae2b4
> > --- /dev/null
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -0,0 +1,43 @@
> > +/* 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>
> > +
> > +#include "log.h"
> > +
> > +namespace libcamera {
> > +
> > +class V4L2Device : protected Loggable
> > +{
> > +public:
> > + void close();
> > + bool isOpen() const { return fd_ != -1; }
> > + const std::string &deviceNode() const { return deviceNode_; }
> > +
> > +protected:
> > + V4L2Device(const std::string &deviceNode, const std::string &logTag);
> > + ~V4L2Device() {}
> > +
> > + int fd() { return fd_; }
> > +
> > + int open(unsigned int flags);
> > +
> > + int ioctl(unsigned long request, void *argp);
> > +
> > + std::string logPrefix() const { return "'" + logTag_ + "'"; }
>
> Let's try not to inline methods that are more complex than just
> returning a member of the class.
>
Ok. I always assumed the compiler knows better, and for simple methods
like this one, implementing it here or in the cpp file it's a matter
of tastes
> I didn't notice in my previous review that V4L2Subdevice was using the
> entity name as a prefix. I'm afraid my comment about moving logPrefix()
> to V4L2Device was a bad one :-/ However, V4L2Device should still inherit
> from Loggable. It will thus have a pure virtual logPrefix() inherited
> from Loggable, requiring the derived classes to implement that method,
> but allowing LOG() statements in V4L2Device to use the log prefix.
>
Why is this (using the entity name as log prefix) bad for you? The
tag is passed to the base class constructor. Might be a bit of a
shortcut maybe, is this your concern ?
> Here's the diff I came up with, it can be squashed with this patch if
> you agree on the comments.
>
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 2c26f3eae2b4..8c83adab4d43 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -21,7 +21,7 @@ public:
> const std::string &deviceNode() const { return deviceNode_; }
>
> protected:
> - V4L2Device(const std::string &deviceNode, const std::string &logTag);
> + V4L2Device(const std::string &deviceNode);
> ~V4L2Device() {}
>
> int fd() { return fd_; }
> @@ -30,11 +30,8 @@ protected:
>
> int ioctl(unsigned long request, void *argp);
>
> - std::string logPrefix() const { return "'" + logTag_ + "'"; }
> -
> private:
> std::string deviceNode_;
> - std::string logTag_;
> int fd_;
> };
>
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index b1da4a87c553..9c077674f997 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -52,6 +52,9 @@ public:
> static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> const std::string &entity);
>
> +protected:
> + std::string logPrefix() const;
> +
> private:
> std::vector<unsigned int> enumPadCodes(unsigned int pad);
> std::vector<SizeRange> enumPadSizes(unsigned int pad,
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index e73dec13f847..734b34f1dc53 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -147,6 +147,9 @@ public:
> static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> const std::string &entity);
>
> +protected:
> + std::string logPrefix() const;
> +
> private:
> int getFormatMeta(V4L2DeviceFormat *format);
> int setFormatMeta(V4L2DeviceFormat *format);
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index eeed0a70fb0e..1f113780ba45 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -68,17 +68,15 @@ void V4L2Device::close()
> /**
> * \brief Construct a V4L2Device
> * \param[in] deviceNode The device node filesystem path
> - * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> - * the device in the log output
> *
> * The V4L2Device constructor is protected and can only be accessed by the
> * V4L2VideoDevice and V4L2Subdevice derived classes.
> *
> * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> * at open() time, and the \a logTag to prefix log messages with.
> */
> -V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> - : deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> +V4L2Device::V4L2Device(const std::string &deviceNode)
> + : deviceNode_(deviceNode), fd_(-1)
> {
> }
>
> @@ -99,6 +94,7 @@ V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> */
> int V4L2Device::open(unsigned int flags)
> {
> + LOG(V4L2, Info) << "Opening device";
> if (isOpen()) {
> LOG(V4L2, Error) << "Device already open";
> return -EBUSY;
> @@ -135,10 +131,4 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> return 0;
> }
>
> -/**
> - * \fn V4L2Device::logPrefix()
> - * \brief Retrieve the tag to prefix log messages with
> - * \return The tag to prefix log messages with
> - */
> -
> } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index d0e1d717b26c..a188298de34c 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> * path
> */
> V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> - : V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> + : V4L2Device(entity->deviceNode()), entity_(entity)
> {
> }
>
> @@ -265,6 +265,11 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> return new V4L2Subdevice(mediaEntity);
> }
>
> +std::string V4L2Subdevice::logPrefix() const
> +{
> + return "'" + entity_->name() + "'";
> +}
> +
> std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> {
> std::vector<unsigned int> codes;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 403e0b2e0653..2de3751f467e 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -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)
> - : V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> + : V4L2Device(deviceNode), bufferPool_(nullptr),
> queuedBuffersCount_(0), fdEvent_(nullptr)
> {
> /*
> @@ -389,6 +389,11 @@ void V4L2VideoDevice::close()
> * \return The string containing the device location
> */
>
> +std::string V4L2VideoDevice::logPrefix() const
> +{
> + return deviceNode() + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> +}
> +
> /**
> * \brief Retrieve the image format set on the V4L2 device
> * \param[out] format The image format applied on the device
>
> > +
> > +private:
> > + std::string deviceNode_;
> > + std::string logTag_;
> > + 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 9afd28b6db02..b1da4a87c553 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
> > {
> > 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_; }
> >
> > @@ -53,9 +52,6 @@ public:
> > static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> > const std::string &entity);
> >
> > -protected:
> > - std::string logPrefix() const;
> > -
> > private:
> > std::vector<unsigned int> enumPadCodes(unsigned int pad);
> > std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > @@ -65,7 +61,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 895658805b47..e73dec13f847 100644
> > --- a/src/libcamera/include/v4l2_videodevice.h
> > +++ b/src/libcamera/include/v4l2_videodevice.h
> > @@ -18,6 +18,7 @@
> >
> > #include "formats.h"
> > #include "log.h"
> > +#include "v4l2_device.h"
> >
> > namespace libcamera {
> >
> > @@ -112,7 +113,7 @@ public:
> > const std::string toString() const;
> > };
> >
> > -class V4L2VideoDevice : protected Loggable
> > +class V4L2VideoDevice : public V4L2Device
> > {
> > public:
> > explicit V4L2VideoDevice(const std::string &deviceNode);
> > @@ -123,13 +124,11 @@ public:
> > V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
> >
> > int open();
> > - bool isOpen() const;
> > void close();
> >
> > const char *driverName() const { return caps_.driver(); }
> > const char *deviceName() const { return caps_.card(); }
> > const char *busName() const { return caps_.bus_info(); }
> > - const std::string &deviceNode() const { return deviceNode_; }
> >
> > int getFormat(V4L2DeviceFormat *format);
> > int setFormat(V4L2DeviceFormat *format);
> > @@ -148,9 +147,6 @@ public:
> > static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> > const std::string &entity);
> >
> > -protected:
> > - std::string logPrefix() const;
> > -
> > private:
> > int getFormatMeta(V4L2DeviceFormat *format);
> > int setFormatMeta(V4L2DeviceFormat *format);
> > @@ -171,8 +167,6 @@ private:
> > Buffer *dequeueBuffer();
> > 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..eeed0a70fb0e
> > --- /dev/null
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -0,0 +1,144 @@
> > +/* 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 V4L2VideoDevice and V4L2Subdevice
> > + *
> > + * The V4L2Device class groups together the methods and fields common to
> > + * both the V4L2VideoDevice and V4L2Subdevice classes, and provides a base
> > + * class whith 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 instead create instances of one the derived
> > + * classes to model either a V4L2 video device or a V4L2 subdevice.
> > + */
> > +
> > +/**
> > + * \brief Close the device node
> > + *
> > + * Reset the file descriptor to -1
> > + */
> > +void V4L2Device::close()
> > +{
> > + if (!isOpen())
> > + return;
> > +
> > + if (::close(fd_) < 0)
> > + LOG(V4L2, Error) << "Failed to close V4L2 device: "
> > + << strerror(errno);
> > + fd_ = -1;
> > +}
> > +
> > +/**
> > + * \fn V4L2Device::isOpen()
> > + * \brief Check if the V4L2 device node is open
> > + * \return True if the V4L2 device node is open, false otherwise
> > + */
> > +
> > +/**
> > + * \fn V4L2Device::deviceNode()
> > + * \brief Retrieve the device node path
> > + * \return The device node path
> > + */
> > +
> > +/**
> > + * \brief Construct a V4L2Device
> > + * \param[in] deviceNode The device node filesystem path
> > + * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> > + * the device in the log output
> > + *
> > + * The V4L2Device constructor is protected and can only be accessed by the
> > + * V4L2VideoDevice and V4L2Subdevice derived classes.
>
> I would drop this sentence.
>
> > + *
> > + * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> > + * at open() time, and the \a logTag to prefix log messages with.
> > + */
> > +V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> > + : deviceNode_(deviceNode), logTag_(logTag), 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[in] flags Access mode flags
> > + *
> > + * Open the device node path with the provided access mode \a flags and
> > + * initialize the file descriptor, which was initially set to -1.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int V4L2Device::open(unsigned int flags)
> > +{
> > + if (isOpen()) {
> > + LOG(V4L2, Error) << "Device already open";
> > + return -EBUSY;
> > + }
> > +
> > + int ret = ::open(deviceNode_.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 Perform an IOCTL system call on the device node
> > + * \param[in] request The IOCTL request code
> > + * \param[in] 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;
> > +}
> > +
> > +/**
> > + * \fn V4L2Device::logPrefix()
> > + * \brief Retrieve the tag to prefix log messages with
> > + * \return The tag to prefix log messages with
> > + */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 48f1fcb30ed4..d0e1d717b26c 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->deviceNode(), entity->name()), 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(O_RDWR);
> > }
> >
> > /**
> > @@ -200,7 +162,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> > ImageFormats formats;
> >
> > if (pad >= entity_->pads().size()) {
> > - LOG(V4L2Subdev, Error) << "Invalid pad: " << pad;
> > + LOG(V4L2, Error) << "Invalid pad: " << pad;
> > return {};
> > }
> >
> > @@ -210,7 +172,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> > return {};
> >
> > if (formats.addFormat(code, sizes)) {
> > - LOG(V4L2Subdev, Error)
> > + LOG(V4L2, Error)
> > << "Could not add sizes for media bus code "
> > << code << " on pad " << pad;
> > return {};
> > @@ -232,10 +194,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;
> > @@ -268,10 +229,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;
> > @@ -305,11 +265,6 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> > return new V4L2Subdevice(mediaEntity);
> > }
> >
> > -std::string V4L2Subdevice::logPrefix() const
> > -{
> > - return "'" + entity_->name() + "'";
> > -}
> > -
> > std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > {
> > std::vector<unsigned int> codes;
> > @@ -321,18 +276,17 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > mbusEnum.index = index;
> > mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >
> > - ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > + ret = ioctl(VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > if (ret)
> > break;
> >
> > codes.push_back(mbusEnum.code);
> > }
> >
> > - if (ret && errno != EINVAL) {
> > - ret = errno;
> > - LOG(V4L2Subdev, Error)
> > + if (ret < 0 && ret != -EINVAL) {
> > + LOG(V4L2, Error)
> > << "Unable to enumerate formats on pad " << pad
> > - << ": " << strerror(ret);
> > + << ": " << strerror(-ret);
> > return {};
> > }
> >
> > @@ -352,7 +306,7 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > sizeEnum.code = code;
> > sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >
> > - ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > + ret = ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > if (ret)
> > break;
> >
> > @@ -360,9 +314,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > sizeEnum.max_width, sizeEnum.max_height);
> > }
> >
> > - if (ret && (errno != EINVAL && errno != ENOTTY)) {
> > - ret = -errno;
> > - LOG(V4L2Subdev, Error)
> > + if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> > + LOG(V4L2, Error)
> > << "Unable to enumerate sizes on pad " << pad
> > << ": " << strerror(-ret);
> > return {};
> > @@ -386,10 +339,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 0e70498c8bb1..403e0b2e0653 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(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 (!isOpen())
> > return;
> >
> > releaseBuffers();
> > delete fdEvent_;
> >
> > - ::close(fd_);
> > - fd_ = -1;
> > + V4L2Device::close();
> > }
> >
> > /**
> > @@ -410,17 +389,6 @@ void V4L2VideoDevice::close()
> > * \return The string containing the device location
> > */
> >
> > -/**
> > - * \fn V4L2VideoDevice::deviceNode()
> > - * \brief Retrieve the video device node path
> > - * \return The video device device node path
> > - */
> > -
> > -std::string V4L2VideoDevice::logPrefix() const
> > -{
> > - return deviceNode_ + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> > -}
> > -
> > /**
> > * \brief Retrieve the image format set on the V4L2 device
> > * \param[out] format The image format applied on the device
> > @@ -462,9 +430,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 +455,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 +482,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 +519,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 +548,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 +576,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;
> > }
> > @@ -670,9 +632,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);
> > @@ -722,9 +683,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);
> > @@ -775,9 +735,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;
> > @@ -801,15 +760,14 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats()
> > pixelformatEnum.index = index;
> > pixelformatEnum.type = bufferType_;
> >
> > - ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum);
> > + ret = ioctl(VIDIOC_ENUM_FMT, &pixelformatEnum);
> > if (ret)
> > break;
> >
> > formats.push_back(pixelformatEnum.pixelformat);
> > }
> >
> > - if (ret && errno != EINVAL) {
> > - ret = -errno;
> > + if (ret && ret != -EINVAL) {
> > LOG(V4L2, Error)
> > << "Unable to enumerate pixel formats: "
> > << strerror(-ret);
> > @@ -829,7 +787,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> > frameSize.index = index;
> > frameSize.pixel_format = pixelFormat;
> >
> > - ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > + ret = ioctl(VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > if (ret)
> > break;
> >
> > @@ -867,8 +825,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> > }
> > }
> >
> > - if (ret && errno != EINVAL) {
> > - ret = -errno;
> > + if (ret && ret != -EINVAL) {
> > LOG(V4L2, Error)
> > << "Unable to enumerate frame sizes: "
> > << strerror(-ret);
> > @@ -969,9 +926,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);
> > @@ -1006,9 +962,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;
> > @@ -1066,9 +1021,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;
> > @@ -1089,9 +1043,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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190619/a4653633/attachment-0001.sig>
More information about the libcamera-devel
mailing list