[libcamera-devel] [PATCH 2/6] libcamera: Provide a V4L2Base class
Jacopo Mondi
jacopo at jmondi.org
Mon Jun 10 09:06:05 CEST 2019
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 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?
> > > 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
-------------- 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/20190610/eb524b69/attachment.sig>
More information about the libcamera-devel
mailing list