| Message ID | 20190602130435.18780-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Just some overview comments so far ... On 02/06/2019 14:04, Jacopo Mondi wrote: > Provide a base class for V4L2 Devices and Subdevices to group common > methods and fields. > > At the moment the code shared between V4L2Device and V4L2Subdevice is > quite limited, but with the forthcoming introduction of control it will > grow consistently. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_base.h | 31 +++++++++++++ > src/libcamera/include/v4l2_device.h | 9 ++-- > src/libcamera/include/v4l2_subdevice.h | 9 ++-- > src/libcamera/meson.build | 2 + > src/libcamera/v4l2_base.cpp | 64 ++++++++++++++++++++++++++ > src/libcamera/v4l2_device.cpp | 13 ++---- > src/libcamera/v4l2_subdevice.cpp | 12 +---- Perhaps we should have a v4l2/ subdir in here now? > 7 files changed, 110 insertions(+), 30 deletions(-) > create mode 100644 src/libcamera/include/v4l2_base.h > create mode 100644 src/libcamera/v4l2_base.cpp > > diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h > new file mode 100644 > index 000000000000..2fda81a960d2 > --- /dev/null > +++ b/src/libcamera/include/v4l2_base.h > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * v4l2_base.h - Common base for V4L2 devices and subdevices > + */ > +#ifndef __LIBCAMERA_V4L2_BASE__ > +#define __LIBCAMERA_V4L2_BASE__ I wonder if we could just call this 'v4l2'? Equally it might be better to keep the base too ... It should be clear that we can't instantiate the base object of course. > + > +#include <vector> > + > +namespace libcamera { > + > +class V4L2Base > +{ > +public: Are you avoiding a constructor here to initialise the fd_? (for something I haven't yet thought of?) > + virtual ~V4L2Base() > + { > + } > + > + virtual int open() = 0; > + virtual void close() = 0; > + bool isOpen() const; > + > +protected: > + int fd_; > +}; > + > +}; /* namespace libcamera */ > + > +#endif /* __LIBCAMERA_V4L2_BASE__ */ > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index bdecc087fe5a..d9bcdb921b8c 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -17,6 +17,7 @@ > #include <libcamera/signal.h> > > #include "log.h" > +#include "v4l2_base.h" > > namespace libcamera { > > @@ -111,7 +112,7 @@ public: > const std::string toString() const; > }; > > -class V4L2Device : protected Loggable > +class V4L2Device : public V4L2Base, protected Loggable > { > public: > explicit V4L2Device(const std::string &deviceNode); > @@ -121,9 +122,8 @@ public: > > V4L2Device &operator=(const V4L2Device &) = delete; > > - int open(); > - bool isOpen() const; > - void close(); > + int open() override; > + void close() override; > > const char *driverName() const { return caps_.driver(); } > const char *deviceName() const { return caps_.card(); } > @@ -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/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > index 3e4e5107aebe..bdef3e69dd8c 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_base.h" > > namespace libcamera { > > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat { > const std::string toString() const; > }; > > -class V4L2Subdevice : protected Loggable > +class V4L2Subdevice : public V4L2Base, protected Loggable > { > public: > explicit V4L2Subdevice(const MediaEntity *entity); > @@ -36,9 +37,8 @@ public: > V4L2Subdevice &operator=(const V4L2Subdevice &) = delete; > ~V4L2Subdevice(); > > - int open(); > - bool isOpen() const; > - void close(); > + int open() override; > + void close() override; > > const MediaEntity *entity() const { return entity_; } > > @@ -64,7 +64,6 @@ private: > Rectangle *rect); > > const MediaEntity *entity_; > - int fd_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index 6a73580d71f5..6d858a22531e 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -21,6 +21,7 @@ libcamera_sources = files([ > 'stream.cpp', > 'timer.cpp', > 'utils.cpp', > + 'v4l2_base.cpp', > 'v4l2_device.cpp', > 'v4l2_subdevice.cpp', > ]) > @@ -38,6 +39,7 @@ libcamera_headers = files([ > 'include/media_object.h', > 'include/pipeline_handler.h', > 'include/utils.h', > + 'include/v4l2_base.h', > 'include/v4l2_device.h', > 'include/v4l2_subdevice.h', > ]) > diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp > new file mode 100644 > index 000000000000..7d05a3c82e4d > --- /dev/null > +++ b/src/libcamera/v4l2_base.cpp > @@ -0,0 +1,64 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * v4l2_base.cpp - Common base for V4L2 devices and subdevices > + */ > + > +#include "v4l2_base.h" > + > +/** > + * \file v4l2_base.h > + * \brief Common base for V4L2 devices and subdevices > + */ > + > +namespace libcamera { > + > +/** > + * \class V4L2Base > + * \brief Base class for V4L2Device and V4L2Subdevice > + * > + * The V4L2Base class groups together the methods and fields common to > + * both V4L2Device and V4L2Subdevice, and provide a base abstract class which > + * defines a streamlined interface that both the derived class have to implement. > + * > + * The interface defined by V4L2Base only requires derived classes to implement > + * methods that modifies the status of the file descriptor associated with > + * the video device or subdevice, such as \a open() and \a close(). > + * > + * Methods common to V4L2Device and V4L2Subdevice, such as V4L2 controls > + * support are implemented in the base class to maximize code re-use. > + */ > + > +/** > + * \brief Open a V4L2 device or subdevice > + * > + * Pure virtual method to be implemented by the derived classes. > + * > + * \return 0 on success or a negative error code otherwise > + */ > + > +/** > + * \brief Close the device or subdevice > + * > + * Pure virtual method to be implemented by the derived classes. > + */ > + > +/** > + * \brief Check if the V4L2 device or subdevice is open > + * \return True if the V4L2 device or subdevice is open, false otherwise > + */ > +bool V4L2Base::isOpen() const > +{ > + return fd_ != -1; > +} > + > +/** > + * \var V4L2Base::fd_ > + * \brief The V4L2 device or subdevice device node file descriptor > + * > + * The file descriptor is initialized to -1 and reset to this value once > + * the device or subdevice gets closed. > + */ > + > +}; /* namespace libcamera */ > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 0821bd75fb42..64533e88a512 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -270,9 +270,11 @@ const std::string V4L2DeviceFormat::toString() const > * \param[in] deviceNode The file-system path to the video device node > */ > V4L2Device::V4L2Device(const std::string &deviceNode) > - : deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr), > + : deviceNode_(deviceNode), bufferPool_(nullptr), > queuedBuffersCount_(0), fdEvent_(nullptr) > { > + fd_ = -1; Shouldn't this be initialised in the base class? > + > /* > * We default to an MMAP based CAPTURE device, however this will be > * updated based upon the device capabilities. > @@ -368,15 +370,6 @@ int V4L2Device::open() > return 0; > } > > -/** > - * \brief Check if device is successfully opened > - * \return True if the device is open, false otherwise > - */ > -bool V4L2Device::isOpen() const > -{ > - return fd_ != -1; > -} > - > /** > * \brief Close the device, releasing any resources acquired by open() > */ > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index fceee33156e9..3a053fadb3f6 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -102,8 +102,9 @@ const std::string V4L2SubdeviceFormat::toString() const > * path > */ > V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity) > - : entity_(entity), fd_(-1) > + : entity_(entity) > { > + fd_ = -1; > } > > V4L2Subdevice::~V4L2Subdevice() > @@ -137,15 +138,6 @@ int V4L2Subdevice::open() > 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() > */ >
Hello, On Mon, Jun 03, 2019 at 10:22:29AM +0100, Kieran Bingham wrote: > Hi Jacopo, > > Just some overview comments so far ... > > On 02/06/2019 14:04, Jacopo Mondi wrote: > > Provide a base class for V4L2 Devices and Subdevices to group common > > methods and fields. > > > > At the moment the code shared between V4L2Device and V4L2Subdevice is > > quite limited, but with the forthcoming introduction of control it will > > grow consistently. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/include/v4l2_base.h | 31 +++++++++++++ > > src/libcamera/include/v4l2_device.h | 9 ++-- > > src/libcamera/include/v4l2_subdevice.h | 9 ++-- > > src/libcamera/meson.build | 2 + > > src/libcamera/v4l2_base.cpp | 64 ++++++++++++++++++++++++++ > > src/libcamera/v4l2_device.cpp | 13 ++---- > > src/libcamera/v4l2_subdevice.cpp | 12 +---- > > Perhaps we should have a v4l2/ subdir in here now? > > > 7 files changed, 110 insertions(+), 30 deletions(-) > > create mode 100644 src/libcamera/include/v4l2_base.h > > create mode 100644 src/libcamera/v4l2_base.cpp > > > > diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h > > new file mode 100644 > > index 000000000000..2fda81a960d2 > > --- /dev/null > > +++ b/src/libcamera/include/v4l2_base.h > > @@ -0,0 +1,31 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * v4l2_base.h - Common base for V4L2 devices and subdevices > > + */ > > +#ifndef __LIBCAMERA_V4L2_BASE__ > > +#define __LIBCAMERA_V4L2_BASE__ > > I wonder if we could just call this 'v4l2'? > > Equally it might be better to keep the base too ... It should be clear > that we can't instantiate the base object of course. We could also rename V4L2Device to V4L2VideoDevice and name the base class V4L2Device (or V4L2DeviceBase). I don't mind either way. > > + > > +#include <vector> > > + > > +namespace libcamera { > > + > > +class V4L2Base > > +{ > > +public: > > Are you avoiding a constructor here to initialise the fd_? (for > something I haven't yet thought of?) > > > + virtual ~V4L2Base() > > + { > > + } > > + > > + virtual int open() = 0; > > + virtual void close() = 0; There's no need to make the open() and close() functions virtual, or even to declare them in the base class, as you never call them on a V4L2Base instance. All you need is an fd_ in the base. It could also possibly make sense to add an ioctl() function in the base class. However, I'm not too fond of storing fd_ in the base class and implementing open() and close() in derived classes. One option would be to implement protected open() and close() functions in the base class that would just open and close the fd, and call them from the open() and close() function of the derived classes. You wouldn't need to make the base methods virtual in this case, and I think I would make fd_ private and provide a protected fd() accessor. > > + bool isOpen() const; > > + > > +protected: > > + int fd_; > > +}; > > + > > +}; /* namespace libcamera */ > > + > > +#endif /* __LIBCAMERA_V4L2_BASE__ */ > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > index bdecc087fe5a..d9bcdb921b8c 100644 > > --- a/src/libcamera/include/v4l2_device.h > > +++ b/src/libcamera/include/v4l2_device.h > > @@ -17,6 +17,7 @@ > > #include <libcamera/signal.h> > > > > #include "log.h" > > +#include "v4l2_base.h" > > > > namespace libcamera { > > > > @@ -111,7 +112,7 @@ public: > > const std::string toString() const; > > }; > > > > -class V4L2Device : protected Loggable > > +class V4L2Device : public V4L2Base, protected Loggable > > { > > public: > > explicit V4L2Device(const std::string &deviceNode); > > @@ -121,9 +122,8 @@ public: > > > > V4L2Device &operator=(const V4L2Device &) = delete; > > > > - int open(); > > - bool isOpen() const; > > - void close(); > > + int open() override; > > + void close() override; > > > > const char *driverName() const { return caps_.driver(); } > > const char *deviceName() const { return caps_.card(); } > > @@ -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/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > > index 3e4e5107aebe..bdef3e69dd8c 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_base.h" > > > > namespace libcamera { > > > > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat { > > const std::string toString() const; > > }; > > > > -class V4L2Subdevice : protected Loggable > > +class V4L2Subdevice : public V4L2Base, protected Loggable > > { > > public: > > explicit V4L2Subdevice(const MediaEntity *entity); > > @@ -36,9 +37,8 @@ public: > > V4L2Subdevice &operator=(const V4L2Subdevice &) = delete; > > ~V4L2Subdevice(); > > > > - int open(); > > - bool isOpen() const; > > - void close(); > > + int open() override; > > + void close() override; > > > > const MediaEntity *entity() const { return entity_; } > > > > @@ -64,7 +64,6 @@ private: > > Rectangle *rect); > > > > const MediaEntity *entity_; > > - int fd_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index 6a73580d71f5..6d858a22531e 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -21,6 +21,7 @@ libcamera_sources = files([ > > 'stream.cpp', > > 'timer.cpp', > > 'utils.cpp', > > + 'v4l2_base.cpp', > > 'v4l2_device.cpp', > > 'v4l2_subdevice.cpp', > > ]) > > @@ -38,6 +39,7 @@ libcamera_headers = files([ > > 'include/media_object.h', > > 'include/pipeline_handler.h', > > 'include/utils.h', > > + 'include/v4l2_base.h', > > 'include/v4l2_device.h', > > 'include/v4l2_subdevice.h', > > ]) > > diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp > > new file mode 100644 > > index 000000000000..7d05a3c82e4d > > --- /dev/null > > +++ b/src/libcamera/v4l2_base.cpp > > @@ -0,0 +1,64 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * v4l2_base.cpp - Common base for V4L2 devices and subdevices > > + */ > > + > > +#include "v4l2_base.h" > > + > > +/** > > + * \file v4l2_base.h > > + * \brief Common base for V4L2 devices and subdevices > > + */ > > + > > +namespace libcamera { > > + > > +/** > > + * \class V4L2Base > > + * \brief Base class for V4L2Device and V4L2Subdevice > > + * > > + * The V4L2Base class groups together the methods and fields common to > > + * both V4L2Device and V4L2Subdevice, and provide a base abstract class which > > + * defines a streamlined interface that both the derived class have to implement. > > + * > > + * The interface defined by V4L2Base only requires derived classes to implement > > + * methods that modifies the status of the file descriptor associated with > > + * the video device or subdevice, such as \a open() and \a close(). > > + * > > + * Methods common to V4L2Device and V4L2Subdevice, such as V4L2 controls > > + * support are implemented in the base class to maximize code re-use. > > + */ > > + > > +/** > > + * \brief Open a V4L2 device or subdevice > > + * > > + * Pure virtual method to be implemented by the derived classes. > > + * > > + * \return 0 on success or a negative error code otherwise > > + */ > > + > > +/** > > + * \brief Close the device or subdevice > > + * > > + * Pure virtual method to be implemented by the derived classes. > > + */ > > + > > +/** > > + * \brief Check if the V4L2 device or subdevice is open > > + * \return True if the V4L2 device or subdevice is open, false otherwise > > + */ > > +bool V4L2Base::isOpen() const > > +{ > > + return fd_ != -1; > > +} > > + > > +/** > > + * \var V4L2Base::fd_ > > + * \brief The V4L2 device or subdevice device node file descriptor > > + * > > + * The file descriptor is initialized to -1 and reset to this value once > > + * the device or subdevice gets closed. > > + */ > > + > > +}; /* namespace libcamera */ > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 0821bd75fb42..64533e88a512 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -270,9 +270,11 @@ const std::string V4L2DeviceFormat::toString() const > > * \param[in] deviceNode The file-system path to the video device node > > */ > > V4L2Device::V4L2Device(const std::string &deviceNode) > > - : deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr), > > + : deviceNode_(deviceNode), bufferPool_(nullptr), > > queuedBuffersCount_(0), fdEvent_(nullptr) > > { > > + fd_ = -1; > > Shouldn't this be initialised in the base class? > > > + > > /* > > * We default to an MMAP based CAPTURE device, however this will be > > * updated based upon the device capabilities. > > @@ -368,15 +370,6 @@ int V4L2Device::open() > > return 0; > > } > > > > -/** > > - * \brief Check if device is successfully opened > > - * \return True if the device is open, false otherwise > > - */ > > -bool V4L2Device::isOpen() const > > -{ > > - return fd_ != -1; > > -} > > - > > /** > > * \brief Close the device, releasing any resources acquired by open() > > */ > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > index fceee33156e9..3a053fadb3f6 100644 > > --- a/src/libcamera/v4l2_subdevice.cpp > > +++ b/src/libcamera/v4l2_subdevice.cpp > > @@ -102,8 +102,9 @@ const std::string V4L2SubdeviceFormat::toString() const > > * path > > */ > > V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity) > > - : entity_(entity), fd_(-1) > > + : entity_(entity) > > { > > + fd_ = -1; > > } > > > > V4L2Subdevice::~V4L2Subdevice() > > @@ -137,15 +138,6 @@ int V4L2Subdevice::open() > > 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() > > */
Hi Laurent/Kieran, On Sat, Jun 08, 2019 at 06:48:05PM +0300, Laurent Pinchart wrote: > Hello, > > On Mon, Jun 03, 2019 at 10:22:29AM +0100, Kieran Bingham wrote: > > Hi Jacopo, > > > > Just some overview comments so far ... > > > > On 02/06/2019 14:04, Jacopo Mondi wrote: > > > Provide a base class for V4L2 Devices and Subdevices to group common > > > methods and fields. > > > > > > At the moment the code shared between V4L2Device and V4L2Subdevice is > > > quite limited, but with the forthcoming introduction of control it will > > > grow consistently. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/include/v4l2_base.h | 31 +++++++++++++ > > > src/libcamera/include/v4l2_device.h | 9 ++-- > > > src/libcamera/include/v4l2_subdevice.h | 9 ++-- > > > src/libcamera/meson.build | 2 + > > > src/libcamera/v4l2_base.cpp | 64 ++++++++++++++++++++++++++ > > > src/libcamera/v4l2_device.cpp | 13 ++---- > > > src/libcamera/v4l2_subdevice.cpp | 12 +---- > > > > Perhaps we should have a v4l2/ subdir in here now? > > I like the idea. Anyone is against this? > > > 7 files changed, 110 insertions(+), 30 deletions(-) > > > create mode 100644 src/libcamera/include/v4l2_base.h > > > create mode 100644 src/libcamera/v4l2_base.cpp > > > > > > diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h > > > new file mode 100644 > > > index 000000000000..2fda81a960d2 > > > --- /dev/null > > > +++ b/src/libcamera/include/v4l2_base.h > > > @@ -0,0 +1,31 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2019, Google Inc. > > > + * > > > + * v4l2_base.h - Common base for V4L2 devices and subdevices > > > + */ > > > +#ifndef __LIBCAMERA_V4L2_BASE__ > > > +#define __LIBCAMERA_V4L2_BASE__ > > > > I wonder if we could just call this 'v4l2'? > > > > Equally it might be better to keep the base too ... It should be clear > > that we can't instantiate the base object of course. > > We could also rename V4L2Device to V4L2VideoDevice and name the base > class V4L2Device (or V4L2DeviceBase). I don't mind either way. > What if we have V4L2Device as base class and V4l2Videodev and V4L2Subdev as derived ones ? > > > + > > > +#include <vector> > > > + > > > +namespace libcamera { > > > + > > > +class V4L2Base > > > +{ > > > +public: > > > > Are you avoiding a constructor here to initialise the fd_? (for > > something I haven't yet thought of?) > > It's (wrongly?) implemented in the derived classes. > > > + virtual ~V4L2Base() > > > + { > > > + } > > > + > > > + virtual int open() = 0; > > > + virtual void close() = 0; > > There's no need to make the open() and close() functions virtual, or > even to declare them in the base class, as you never call them on a > V4L2Base instance. All you need is an fd_ in the base. It could also > possibly make sense to add an ioctl() function in the base class. > +1 for ioctl wrapper. It would save some code. > However, I'm not too fond of storing fd_ in the base class and > implementing open() and close() in derived classes. One option would be > to implement protected open() and close() functions in the base class > that would just open and close the fd, and call them from the open() and > close() function of the derived classes. You wouldn't need to make the > base methods virtual in this case, and I think I would make fd_ private > and provide a protected fd() accessor. Isn't this over-engineered? I mean, the derived classes would have to call the base class methods that just wrap open/close without doing much more, and have to call a method to access the fd_, without any real benefit in my opinion. I would rather move fd_ to the derived classes and have open()/close() implemented there independently. Unless I'm missing some obvious reason not to do so... > > > > + bool isOpen() const; > > > + > > > +protected: > > > + int fd_; > > > +}; > > > + > > > +}; /* namespace libcamera */ > > > + > > > +#endif /* __LIBCAMERA_V4L2_BASE__ */ > > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > > index bdecc087fe5a..d9bcdb921b8c 100644 > > > --- a/src/libcamera/include/v4l2_device.h > > > +++ b/src/libcamera/include/v4l2_device.h > > > @@ -17,6 +17,7 @@ > > > #include <libcamera/signal.h> > > > > > > #include "log.h" > > > +#include "v4l2_base.h" > > > > > > namespace libcamera { > > > > > > @@ -111,7 +112,7 @@ public: > > > const std::string toString() const; > > > }; > > > > > > -class V4L2Device : protected Loggable > > > +class V4L2Device : public V4L2Base, protected Loggable > > > { > > > public: > > > explicit V4L2Device(const std::string &deviceNode); > > > @@ -121,9 +122,8 @@ public: > > > > > > V4L2Device &operator=(const V4L2Device &) = delete; > > > > > > - int open(); > > > - bool isOpen() const; > > > - void close(); > > > + int open() override; > > > + void close() override; > > > > > > const char *driverName() const { return caps_.driver(); } > > > const char *deviceName() const { return caps_.card(); } > > > @@ -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/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > > > index 3e4e5107aebe..bdef3e69dd8c 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_base.h" > > > > > > namespace libcamera { > > > > > > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat { > > > const std::string toString() const; > > > }; > > > > > > -class V4L2Subdevice : protected Loggable > > > +class V4L2Subdevice : public V4L2Base, protected Loggable > > > { > > > public: > > > explicit V4L2Subdevice(const MediaEntity *entity); > > > @@ -36,9 +37,8 @@ public: > > > V4L2Subdevice &operator=(const V4L2Subdevice &) = delete; > > > ~V4L2Subdevice(); > > > > > > - int open(); > > > - bool isOpen() const; > > > - void close(); > > > + int open() override; > > > + void close() override; > > > > > > const MediaEntity *entity() const { return entity_; } > > > > > > @@ -64,7 +64,6 @@ private: > > > Rectangle *rect); > > > > > > const MediaEntity *entity_; > > > - int fd_; > > > }; > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > index 6a73580d71f5..6d858a22531e 100644 > > > --- a/src/libcamera/meson.build > > > +++ b/src/libcamera/meson.build > > > @@ -21,6 +21,7 @@ libcamera_sources = files([ > > > 'stream.cpp', > > > 'timer.cpp', > > > 'utils.cpp', > > > + 'v4l2_base.cpp', > > > 'v4l2_device.cpp', > > > 'v4l2_subdevice.cpp', > > > ]) > > > @@ -38,6 +39,7 @@ libcamera_headers = files([ > > > 'include/media_object.h', > > > 'include/pipeline_handler.h', > > > 'include/utils.h', > > > + 'include/v4l2_base.h', > > > 'include/v4l2_device.h', > > > 'include/v4l2_subdevice.h', > > > ]) > > > diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp > > > new file mode 100644 > > > index 000000000000..7d05a3c82e4d > > > --- /dev/null > > > +++ b/src/libcamera/v4l2_base.cpp > > > @@ -0,0 +1,64 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2019, Google Inc. > > > + * > > > + * v4l2_base.cpp - Common base for V4L2 devices and subdevices > > > + */ > > > + > > > +#include "v4l2_base.h" > > > + > > > +/** > > > + * \file v4l2_base.h > > > + * \brief Common base for V4L2 devices and subdevices > > > + */ > > > + > > > +namespace libcamera { > > > + > > > +/** > > > + * \class V4L2Base > > > + * \brief Base class for V4L2Device and V4L2Subdevice > > > + * > > > + * The V4L2Base class groups together the methods and fields common to > > > + * both V4L2Device and V4L2Subdevice, and provide a base abstract class which > > > + * defines a streamlined interface that both the derived class have to implement. > > > + * > > > + * The interface defined by V4L2Base only requires derived classes to implement > > > + * methods that modifies the status of the file descriptor associated with > > > + * the video device or subdevice, such as \a open() and \a close(). > > > + * > > > + * Methods common to V4L2Device and V4L2Subdevice, such as V4L2 controls > > > + * support are implemented in the base class to maximize code re-use. > > > + */ > > > + > > > +/** > > > + * \brief Open a V4L2 device or subdevice > > > + * > > > + * Pure virtual method to be implemented by the derived classes. > > > + * > > > + * \return 0 on success or a negative error code otherwise > > > + */ > > > + > > > +/** > > > + * \brief Close the device or subdevice > > > + * > > > + * Pure virtual method to be implemented by the derived classes. > > > + */ > > > + > > > +/** > > > + * \brief Check if the V4L2 device or subdevice is open > > > + * \return True if the V4L2 device or subdevice is open, false otherwise > > > + */ > > > +bool V4L2Base::isOpen() const > > > +{ > > > + return fd_ != -1; > > > +} > > > + > > > +/** > > > + * \var V4L2Base::fd_ > > > + * \brief The V4L2 device or subdevice device node file descriptor > > > + * > > > + * The file descriptor is initialized to -1 and reset to this value once > > > + * the device or subdevice gets closed. > > > + */ > > > + > > > +}; /* namespace libcamera */ > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index 0821bd75fb42..64533e88a512 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -270,9 +270,11 @@ const std::string V4L2DeviceFormat::toString() const > > > * \param[in] deviceNode The file-system path to the video device node > > > */ > > > V4L2Device::V4L2Device(const std::string &deviceNode) > > > - : deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr), > > > + : deviceNode_(deviceNode), bufferPool_(nullptr), > > > queuedBuffersCount_(0), fdEvent_(nullptr) > > > { > > > + fd_ = -1; > > > > Shouldn't this be initialised in the base class? > > > > > + > > > /* > > > * We default to an MMAP based CAPTURE device, however this will be > > > * updated based upon the device capabilities. > > > @@ -368,15 +370,6 @@ int V4L2Device::open() > > > return 0; > > > } > > > > > > -/** > > > - * \brief Check if device is successfully opened > > > - * \return True if the device is open, false otherwise > > > - */ > > > -bool V4L2Device::isOpen() const > > > -{ > > > - return fd_ != -1; > > > -} > > > - > > > /** > > > * \brief Close the device, releasing any resources acquired by open() > > > */ > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > > index fceee33156e9..3a053fadb3f6 100644 > > > --- a/src/libcamera/v4l2_subdevice.cpp > > > +++ b/src/libcamera/v4l2_subdevice.cpp > > > @@ -102,8 +102,9 @@ const std::string V4L2SubdeviceFormat::toString() const > > > * path > > > */ > > > V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity) > > > - : entity_(entity), fd_(-1) > > > + : entity_(entity) > > > { > > > + fd_ = -1; > > > } > > > > > > V4L2Subdevice::~V4L2Subdevice() > > > @@ -137,15 +138,6 @@ int V4L2Subdevice::open() > > > 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() > > > */ > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Jun 10, 2019 at 09:06:05AM +0200, Jacopo Mondi wrote: > On Sat, Jun 08, 2019 at 06:48:05PM +0300, Laurent Pinchart wrote: > > On Mon, Jun 03, 2019 at 10:22:29AM +0100, Kieran Bingham wrote: > >> On 02/06/2019 14:04, Jacopo Mondi wrote: > >>> Provide a base class for V4L2 Devices and Subdevices to group common > >>> methods and fields. > >>> > >>> At the moment the code shared between V4L2Device and V4L2Subdevice is > >>> quite limited, but with the forthcoming introduction of control it will > >>> grow consistently. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> src/libcamera/include/v4l2_base.h | 31 +++++++++++++ > >>> src/libcamera/include/v4l2_device.h | 9 ++-- > >>> src/libcamera/include/v4l2_subdevice.h | 9 ++-- > >>> src/libcamera/meson.build | 2 + > >>> src/libcamera/v4l2_base.cpp | 64 ++++++++++++++++++++++++++ > >>> src/libcamera/v4l2_device.cpp | 13 ++---- > >>> src/libcamera/v4l2_subdevice.cpp | 12 +---- > >> > >> Perhaps we should have a v4l2/ subdir in here now? > > I like the idea. Anyone is against this? I don't think it's necessary yet, but I'm not against it either. The files will need to start with the v4l2_ prefix anyway. > >>> 7 files changed, 110 insertions(+), 30 deletions(-) > >>> create mode 100644 src/libcamera/include/v4l2_base.h > >>> create mode 100644 src/libcamera/v4l2_base.cpp > >>> > >>> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h > >>> new file mode 100644 > >>> index 000000000000..2fda81a960d2 > >>> --- /dev/null > >>> +++ b/src/libcamera/include/v4l2_base.h > >>> @@ -0,0 +1,31 @@ > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >>> +/* > >>> + * Copyright (C) 2019, Google Inc. > >>> + * > >>> + * v4l2_base.h - Common base for V4L2 devices and subdevices > >>> + */ > >>> +#ifndef __LIBCAMERA_V4L2_BASE__ > >>> +#define __LIBCAMERA_V4L2_BASE__ > >> > >> I wonder if we could just call this 'v4l2'? > >> > >> Equally it might be better to keep the base too ... It should be clear > >> that we can't instantiate the base object of course. > > > > We could also rename V4L2Device to V4L2VideoDevice and name the base > > class V4L2Device (or V4L2DeviceBase). I don't mind either way. > > What if we have V4L2Device as base class and V4l2Videodev and > V4L2Subdev as derived ones ? That works for me. > >>> + > >>> +#include <vector> > >>> + > >>> +namespace libcamera { > >>> + > >>> +class V4L2Base > >>> +{ > >>> +public: > >> > >> Are you avoiding a constructor here to initialise the fd_? (for > >> something I haven't yet thought of?) > > It's (wrongly?) implemented in the derived classes. > > >>> + virtual ~V4L2Base() > >>> + { > >>> + } > >>> + > >>> + virtual int open() = 0; > >>> + virtual void close() = 0; > > > > There's no need to make the open() and close() functions virtual, or > > even to declare them in the base class, as you never call them on a > > V4L2Base instance. All you need is an fd_ in the base. It could also > > possibly make sense to add an ioctl() function in the base class. > > +1 for ioctl wrapper. It would save some code. > > > However, I'm not too fond of storing fd_ in the base class and > > implementing open() and close() in derived classes. One option would be > > to implement protected open() and close() functions in the base class > > that would just open and close the fd, and call them from the open() and > > close() function of the derived classes. You wouldn't need to make the > > base methods virtual in this case, and I think I would make fd_ private > > and provide a protected fd() accessor. > > Isn't this over-engineered? I mean, the derived classes would have to > call the base class methods that just wrap open/close without doing > much more, and have to call a method to access the fd_, without any > real benefit in my opinion. I would rather move fd_ to the derived > classes and have open()/close() implemented there independently. > > Unless I'm missing some obvious reason not to do so... You can't move fd_ to the derived classes as you need it in the ioctl() and control functions in the base class. I don't think an accessor for the fd would be a big deal, as it can be inlined and would thus be free. open() and close() would indeed be a bit small, but I think that grouping them with fd_ in the same class would be a better design. > >>> + bool isOpen() const; > >>> + > >>> +protected: > >>> + int fd_; > >>> +}; > >>> + > >>> +}; /* namespace libcamera */ > >>> + > >>> +#endif /* __LIBCAMERA_V4L2_BASE__ */ > >>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > >>> index bdecc087fe5a..d9bcdb921b8c 100644 > >>> --- a/src/libcamera/include/v4l2_device.h > >>> +++ b/src/libcamera/include/v4l2_device.h > >>> @@ -17,6 +17,7 @@ > >>> #include <libcamera/signal.h> > >>> > >>> #include "log.h" > >>> +#include "v4l2_base.h" > >>> > >>> namespace libcamera { > >>> > >>> @@ -111,7 +112,7 @@ public: > >>> const std::string toString() const; > >>> }; > >>> > >>> -class V4L2Device : protected Loggable > >>> +class V4L2Device : public V4L2Base, protected Loggable > >>> { > >>> public: > >>> explicit V4L2Device(const std::string &deviceNode); > >>> @@ -121,9 +122,8 @@ public: > >>> > >>> V4L2Device &operator=(const V4L2Device &) = delete; > >>> > >>> - int open(); > >>> - bool isOpen() const; > >>> - void close(); > >>> + int open() override; > >>> + void close() override; > >>> > >>> const char *driverName() const { return caps_.driver(); } > >>> const char *deviceName() const { return caps_.card(); } > >>> @@ -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/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > >>> index 3e4e5107aebe..bdef3e69dd8c 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_base.h" > >>> > >>> namespace libcamera { > >>> > >>> @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat { > >>> const std::string toString() const; > >>> }; > >>> > >>> -class V4L2Subdevice : protected Loggable > >>> +class V4L2Subdevice : public V4L2Base, protected Loggable > >>> { > >>> public: > >>> explicit V4L2Subdevice(const MediaEntity *entity); > >>> @@ -36,9 +37,8 @@ public: > >>> V4L2Subdevice &operator=(const V4L2Subdevice &) = delete; > >>> ~V4L2Subdevice(); > >>> > >>> - int open(); > >>> - bool isOpen() const; > >>> - void close(); > >>> + int open() override; > >>> + void close() override; > >>> > >>> const MediaEntity *entity() const { return entity_; } > >>> > >>> @@ -64,7 +64,6 @@ private: > >>> Rectangle *rect); > >>> > >>> const MediaEntity *entity_; > >>> - int fd_; > >>> }; > >>> > >>> } /* namespace libcamera */ > >>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > >>> index 6a73580d71f5..6d858a22531e 100644 > >>> --- a/src/libcamera/meson.build > >>> +++ b/src/libcamera/meson.build > >>> @@ -21,6 +21,7 @@ libcamera_sources = files([ > >>> 'stream.cpp', > >>> 'timer.cpp', > >>> 'utils.cpp', > >>> + 'v4l2_base.cpp', > >>> 'v4l2_device.cpp', > >>> 'v4l2_subdevice.cpp', > >>> ]) > >>> @@ -38,6 +39,7 @@ libcamera_headers = files([ > >>> 'include/media_object.h', > >>> 'include/pipeline_handler.h', > >>> 'include/utils.h', > >>> + 'include/v4l2_base.h', > >>> 'include/v4l2_device.h', > >>> 'include/v4l2_subdevice.h', > >>> ]) > >>> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp > >>> new file mode 100644 > >>> index 000000000000..7d05a3c82e4d > >>> --- /dev/null > >>> +++ b/src/libcamera/v4l2_base.cpp > >>> @@ -0,0 +1,64 @@ > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >>> +/* > >>> + * Copyright (C) 2019, Google Inc. > >>> + * > >>> + * v4l2_base.cpp - Common base for V4L2 devices and subdevices > >>> + */ > >>> + > >>> +#include "v4l2_base.h" > >>> + > >>> +/** > >>> + * \file v4l2_base.h > >>> + * \brief Common base for V4L2 devices and subdevices > >>> + */ > >>> + > >>> +namespace libcamera { > >>> + > >>> +/** > >>> + * \class V4L2Base > >>> + * \brief Base class for V4L2Device and V4L2Subdevice > >>> + * > >>> + * The V4L2Base class groups together the methods and fields common to > >>> + * both V4L2Device and V4L2Subdevice, and provide a base abstract class which > >>> + * defines a streamlined interface that both the derived class have to implement. > >>> + * > >>> + * The interface defined by V4L2Base only requires derived classes to implement > >>> + * methods that modifies the status of the file descriptor associated with > >>> + * the video device or subdevice, such as \a open() and \a close(). > >>> + * > >>> + * Methods common to V4L2Device and V4L2Subdevice, such as V4L2 controls > >>> + * support are implemented in the base class to maximize code re-use. > >>> + */ > >>> + > >>> +/** > >>> + * \brief Open a V4L2 device or subdevice > >>> + * > >>> + * Pure virtual method to be implemented by the derived classes. > >>> + * > >>> + * \return 0 on success or a negative error code otherwise > >>> + */ > >>> + > >>> +/** > >>> + * \brief Close the device or subdevice > >>> + * > >>> + * Pure virtual method to be implemented by the derived classes. > >>> + */ > >>> + > >>> +/** > >>> + * \brief Check if the V4L2 device or subdevice is open > >>> + * \return True if the V4L2 device or subdevice is open, false otherwise > >>> + */ > >>> +bool V4L2Base::isOpen() const > >>> +{ > >>> + return fd_ != -1; > >>> +} > >>> + > >>> +/** > >>> + * \var V4L2Base::fd_ > >>> + * \brief The V4L2 device or subdevice device node file descriptor > >>> + * > >>> + * The file descriptor is initialized to -1 and reset to this value once > >>> + * the device or subdevice gets closed. > >>> + */ > >>> + > >>> +}; /* namespace libcamera */ > >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > >>> index 0821bd75fb42..64533e88a512 100644 > >>> --- a/src/libcamera/v4l2_device.cpp > >>> +++ b/src/libcamera/v4l2_device.cpp > >>> @@ -270,9 +270,11 @@ const std::string V4L2DeviceFormat::toString() const > >>> * \param[in] deviceNode The file-system path to the video device node > >>> */ > >>> V4L2Device::V4L2Device(const std::string &deviceNode) > >>> - : deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr), > >>> + : deviceNode_(deviceNode), bufferPool_(nullptr), > >>> queuedBuffersCount_(0), fdEvent_(nullptr) > >>> { > >>> + fd_ = -1; > >> > >> Shouldn't this be initialised in the base class? > >> > >>> + > >>> /* > >>> * We default to an MMAP based CAPTURE device, however this will be > >>> * updated based upon the device capabilities. > >>> @@ -368,15 +370,6 @@ int V4L2Device::open() > >>> return 0; > >>> } > >>> > >>> -/** > >>> - * \brief Check if device is successfully opened > >>> - * \return True if the device is open, false otherwise > >>> - */ > >>> -bool V4L2Device::isOpen() const > >>> -{ > >>> - return fd_ != -1; > >>> -} > >>> - > >>> /** > >>> * \brief Close the device, releasing any resources acquired by open() > >>> */ > >>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > >>> index fceee33156e9..3a053fadb3f6 100644 > >>> --- a/src/libcamera/v4l2_subdevice.cpp > >>> +++ b/src/libcamera/v4l2_subdevice.cpp > >>> @@ -102,8 +102,9 @@ const std::string V4L2SubdeviceFormat::toString() const > >>> * path > >>> */ > >>> V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity) > >>> - : entity_(entity), fd_(-1) > >>> + : entity_(entity) > >>> { > >>> + fd_ = -1; > >>> } > >>> > >>> V4L2Subdevice::~V4L2Subdevice() > >>> @@ -137,15 +138,6 @@ int V4L2Subdevice::open() > >>> 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() > >>> */
diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h new file mode 100644 index 000000000000..2fda81a960d2 --- /dev/null +++ b/src/libcamera/include/v4l2_base.h @@ -0,0 +1,31 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * v4l2_base.h - Common base for V4L2 devices and subdevices + */ +#ifndef __LIBCAMERA_V4L2_BASE__ +#define __LIBCAMERA_V4L2_BASE__ + +#include <vector> + +namespace libcamera { + +class V4L2Base +{ +public: + virtual ~V4L2Base() + { + } + + virtual int open() = 0; + virtual void close() = 0; + bool isOpen() const; + +protected: + int fd_; +}; + +}; /* namespace libcamera */ + +#endif /* __LIBCAMERA_V4L2_BASE__ */ diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h index bdecc087fe5a..d9bcdb921b8c 100644 --- a/src/libcamera/include/v4l2_device.h +++ b/src/libcamera/include/v4l2_device.h @@ -17,6 +17,7 @@ #include <libcamera/signal.h> #include "log.h" +#include "v4l2_base.h" namespace libcamera { @@ -111,7 +112,7 @@ public: const std::string toString() const; }; -class V4L2Device : protected Loggable +class V4L2Device : public V4L2Base, protected Loggable { public: explicit V4L2Device(const std::string &deviceNode); @@ -121,9 +122,8 @@ public: V4L2Device &operator=(const V4L2Device &) = delete; - int open(); - bool isOpen() const; - void close(); + int open() override; + void close() override; const char *driverName() const { return caps_.driver(); } const char *deviceName() const { return caps_.card(); } @@ -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/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h index 3e4e5107aebe..bdef3e69dd8c 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_base.h" namespace libcamera { @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat { const std::string toString() const; }; -class V4L2Subdevice : protected Loggable +class V4L2Subdevice : public V4L2Base, protected Loggable { public: explicit V4L2Subdevice(const MediaEntity *entity); @@ -36,9 +37,8 @@ public: V4L2Subdevice &operator=(const V4L2Subdevice &) = delete; ~V4L2Subdevice(); - int open(); - bool isOpen() const; - void close(); + int open() override; + void close() override; const MediaEntity *entity() const { return entity_; } @@ -64,7 +64,6 @@ private: Rectangle *rect); const MediaEntity *entity_; - int fd_; }; } /* namespace libcamera */ diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index 6a73580d71f5..6d858a22531e 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -21,6 +21,7 @@ libcamera_sources = files([ 'stream.cpp', 'timer.cpp', 'utils.cpp', + 'v4l2_base.cpp', 'v4l2_device.cpp', 'v4l2_subdevice.cpp', ]) @@ -38,6 +39,7 @@ libcamera_headers = files([ 'include/media_object.h', 'include/pipeline_handler.h', 'include/utils.h', + 'include/v4l2_base.h', 'include/v4l2_device.h', 'include/v4l2_subdevice.h', ]) diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp new file mode 100644 index 000000000000..7d05a3c82e4d --- /dev/null +++ b/src/libcamera/v4l2_base.cpp @@ -0,0 +1,64 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * v4l2_base.cpp - Common base for V4L2 devices and subdevices + */ + +#include "v4l2_base.h" + +/** + * \file v4l2_base.h + * \brief Common base for V4L2 devices and subdevices + */ + +namespace libcamera { + +/** + * \class V4L2Base + * \brief Base class for V4L2Device and V4L2Subdevice + * + * The V4L2Base class groups together the methods and fields common to + * both V4L2Device and V4L2Subdevice, and provide a base abstract class which + * defines a streamlined interface that both the derived class have to implement. + * + * The interface defined by V4L2Base only requires derived classes to implement + * methods that modifies the status of the file descriptor associated with + * the video device or subdevice, such as \a open() and \a close(). + * + * Methods common to V4L2Device and V4L2Subdevice, such as V4L2 controls + * support are implemented in the base class to maximize code re-use. + */ + +/** + * \brief Open a V4L2 device or subdevice + * + * Pure virtual method to be implemented by the derived classes. + * + * \return 0 on success or a negative error code otherwise + */ + +/** + * \brief Close the device or subdevice + * + * Pure virtual method to be implemented by the derived classes. + */ + +/** + * \brief Check if the V4L2 device or subdevice is open + * \return True if the V4L2 device or subdevice is open, false otherwise + */ +bool V4L2Base::isOpen() const +{ + return fd_ != -1; +} + +/** + * \var V4L2Base::fd_ + * \brief The V4L2 device or subdevice device node file descriptor + * + * The file descriptor is initialized to -1 and reset to this value once + * the device or subdevice gets closed. + */ + +}; /* namespace libcamera */ diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 0821bd75fb42..64533e88a512 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -270,9 +270,11 @@ const std::string V4L2DeviceFormat::toString() const * \param[in] deviceNode The file-system path to the video device node */ V4L2Device::V4L2Device(const std::string &deviceNode) - : deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr), + : deviceNode_(deviceNode), bufferPool_(nullptr), queuedBuffersCount_(0), fdEvent_(nullptr) { + fd_ = -1; + /* * We default to an MMAP based CAPTURE device, however this will be * updated based upon the device capabilities. @@ -368,15 +370,6 @@ int V4L2Device::open() return 0; } -/** - * \brief Check if device is successfully opened - * \return True if the device is open, false otherwise - */ -bool V4L2Device::isOpen() const -{ - return fd_ != -1; -} - /** * \brief Close the device, releasing any resources acquired by open() */ diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp index fceee33156e9..3a053fadb3f6 100644 --- a/src/libcamera/v4l2_subdevice.cpp +++ b/src/libcamera/v4l2_subdevice.cpp @@ -102,8 +102,9 @@ const std::string V4L2SubdeviceFormat::toString() const * path */ V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity) - : entity_(entity), fd_(-1) + : entity_(entity) { + fd_ = -1; } V4L2Subdevice::~V4L2Subdevice() @@ -137,15 +138,6 @@ int V4L2Subdevice::open() 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() */
Provide a base class for V4L2 Devices and Subdevices to group common methods and fields. At the moment the code shared between V4L2Device and V4L2Subdevice is quite limited, but with the forthcoming introduction of control it will grow consistently. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_base.h | 31 +++++++++++++ src/libcamera/include/v4l2_device.h | 9 ++-- src/libcamera/include/v4l2_subdevice.h | 9 ++-- src/libcamera/meson.build | 2 + src/libcamera/v4l2_base.cpp | 64 ++++++++++++++++++++++++++ src/libcamera/v4l2_device.cpp | 13 ++---- src/libcamera/v4l2_subdevice.cpp | 12 +---- 7 files changed, 110 insertions(+), 30 deletions(-) create mode 100644 src/libcamera/include/v4l2_base.h create mode 100644 src/libcamera/v4l2_base.cpp