[libcamera-devel] [PATCH 2/6] libcamera: Provide a V4L2Base class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 10 10:22:52 CEST 2019
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 at 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()
> >>> */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list