[libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device object
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jan 16 19:00:55 CET 2019
Hi Niklas,
Ok - here's an attempt at explaining my rationale and also hopefully
some updates which might make you like things more (I've dropped the 'get')
On 16/01/2019 14:59, Niklas Söderlund wrote:
> Hi Kieran,
>
> Thanks for your work.
>
> As Jacopo already provided a rich set of comments I try to comment on
> different things.
>
> On 2019-01-15 23:19:08 +0000, Kieran Bingham wrote:
>> Provide a helper V4L2 device object capable of interacting with the
>> V4L2 Linux Kernel APIs.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>> src/libcamera/include/v4l2_device.h | 45 +++++++++
>> src/libcamera/meson.build | 2 +
>> src/libcamera/v4l2_device.cpp | 150 ++++++++++++++++++++++++++++
>> 3 files changed, 197 insertions(+)
>> 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..b0f5e9689654
>> --- /dev/null
>> +++ b/src/libcamera/include/v4l2_device.h
>> @@ -0,0 +1,45 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * v4l2_device.h - V4L2 Device
>> + */
>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
>> +#define __LIBCAMERA_V4L2_DEVICE_H__
>> +
>> +#include <string>
>> +
>> +#include <linux/videodev2.h>
>> +
>> +namespace libcamera {
>> +
>> +class V4L2Capability : public v4l2_capability
>> +{
>> +public:
>> + std::string getDriver() const { return std::string((char *)driver); }
>> + std::string getCard() const { return std::string((char *)card); }
>
> I'm not loving this. I think this is a deal breaker in this case to
> inherit from struct v4l2_capability. I would make the struct a private
> member of the class and make this drivre() and card().
Can you explain /why/ you think it's a deal breaker please?
Is your objection to the 'get' prefix (as in getDriver())?
I now have a solution for that with appropriate casting:
std::string driver() const {
return reinterpret_cast<const char *>(v4l2_capability::driver);
}
Otherwise, I believe it does inherit from v4l2_capability.
If I could I would write write:
struct v4l2_capability : struct v4l2_capability {
public:
std::string driver() const;
std::string card() const;
bool isCapture() const;
};
The point is if I could add methods to the struct v4l2_capability, I'd
be doing that ... but I can't. And I certainly don't want to duplicate
the structure...
The V4L2Capability is simply providing an interface at the C++ level and
defining how to work with the data in the structure in an object
orientated way.
This means that any methods which need to interact with struct
v4l2_capability are contained in a common location - and those methods
can only operate on the structure ...
Putting a v4l2_capability as a private member in a class V4L2Capability
produces: [2] which I quite dislike. I've updated V4L2Capability to mark
it final to express that this class should not be further extended or
overridden.
[1] https://paste.debian.net/1060853/ {inheritance}
[2] https://paste.debian.net/1060852/ {composition}
[3] https://paste.debian.net/1060839/ {flat in V4L2Device}
My biggest reason for continuing to push this - is because I feel this
pattern will be beneficial for the many complex structure types in V4L2.
I'm thinking controls, pix_format, fmtdesc, requestbuffers...
(Not everything, just ones where it makes sense to define helpers or
bring the kernel API to a C++ interface)
They are all 'objects' which are managed by the kernel. It's just that
the data for them is in a c-struct because that's what the kernel has.
In my opinion - handling those types as objects in the C++ will keep the
code where it counts. On the object it deals with.
If we go with [3] (which I don't want), then the V4L2Device class is
going to be polluted with all sorts of helpers (helpful as they may be)
which operate on the various structures. It's also no good in the case
where there are multiple objects (like v4l2_buffers).
I don't think [2] is appropriate here. It doesn't protect us from
anything, and we are not extending any types or fields in the struct
v4l2_capability.
(I'd also envisage re-using this V4L2Device class externally too, so
having nice wrappings around things can provide further benefits)
>> +
>> + bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }
>> + bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }
>
> Not saying it's wrong but it confuses me a bit. Why is one capability
> check prefixed with 'is' and the other 'has'?
Grammar :)
The device 'is' a capture device.
(_CAPTURE | _CAPTURE_MPLANE) vs (_OUTPUT / _M2M)
The device 'has' streaming IO ioctls
The device 'has' async io ioctls
...
(Now technically an M2M 'is' both a _CAPTURE and an _OUTPUT but that's
differentiated by being an M2M...)
>
>> +};
>> +
>> +class V4L2Device
>> +{
>> +public:
>> + V4L2Device() = delete;
>> + V4L2Device(const std::string &devnode);
>> + ~V4L2Device();
>> +
>> + int open();
>> + bool isOpen() const;
>> + void close();
>> +
>> +private:
>> + std::string devnode_;
>> + int fd_;
>> + V4L2Capability caps_;
>> +};
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index abf9a71d4172..f9f25c0ecf15 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -11,6 +11,7 @@ libcamera_sources = files([
>> 'pipeline_handler.cpp',
>> 'signal.cpp',
>> 'timer.cpp',
>> + 'v4l2_device.cpp',
>> ])
>>
>> libcamera_headers = files([
>> @@ -21,6 +22,7 @@ libcamera_headers = files([
>> 'include/media_object.h',
>> 'include/pipeline_handler.h',
>> 'include/utils.h',
>> + 'include/v4l2_device.h',
>> ])
>>
>> libcamera_internal_includes = include_directories('include')
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> new file mode 100644
>> index 000000000000..8c49d3ff56e2
>> --- /dev/null
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -0,0 +1,150 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * v4l2_device.cpp - V4L2 Device
>> + */
>> +
>> +#include <fcntl.h>
>> +#include <string.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/mman.h>
>> +#include <unistd.h>
>> +
>> +#include "log.h"
>> +#include "v4l2_device.h"
>> +
>> +/**
>> + * \file v4l2_device.h
>> + * \brief V4L2 Device API
>> + */
>> +namespace libcamera {
>> +
>> +/**
>> + * \class V4L2Capability
>> + * \brief struct v4l2_capability object wrapper and helpers
>> + *
>> + * The V4L2Capability class manages the information returned by the
>> + * VIDIOC_QUERYCAP ioctl.
>> + */
>> +
>> +/**
>> + * \fn std::string V4L2Capability::getDriver()
>> + * \brief Retrieve the driver module name
>> + * \return The string containing the name of the driver module
>> + */
>> +
>> +/**
>> + * \fn std::string V4L2Capability::getCard()
>> + * \brief Retrieve the device card name
>> + * \return The string containing the device name
>> + */
>> +
>> +/**
>> + * \fn bool V4L2Capability::isCapture()
>> + * \brief Identify if the device is capable of capturing video
>> + * \return boolean true if the device provides video frames
>> + */
>> +
>> +/**
>> + * \fn bool V4L2Capability::hasStreaming()
>> + * \brief Determine if the device can perform Streaming IO
>> + * \return boolean true if the device provides Streaming IO IOCTLs
>> + */
>> +
>> +/**
>> + * \class V4L2Device
>> + * \brief V4L2Device object and API.
>> + *
>> + * The V4L2 Device API class models an instance of a V4L2 device node.
>> + * It is constructed with the path to a V4L2 video device node. The device node
>> + * is only opened upon a call to open() which must be checked for success.
>> + *
>> + * The device capabilities are validated and the device is rejected if it is
>> + * not a suitable capture device.
>> + *
>> + * No API call (except open(), isOpen() and close()) shall be called on an
>> + * unopened device instance.
>> + *
>> + * Upon destruction any device left open will be closed, and any resources
>> + * released.
>> + */
>> +
>> +/**
>> + * \brief Construct a V4L2Device
>> + * \param devnode The file-system path to the video device node
>> + */
>> +V4L2Device::V4L2Device(const std::string &devnode)
>> + : devnode_(devnode), fd_(-1)
>> +{
>> +}
>> +
>> +V4L2Device::~V4L2Device()
>> +{
>> + close();
>> +}
>> +
>> +/**
>> + * \brief Open a V4L2 device and query properties from the device.
>> + * \return 0 on success, or a negative error code otherwise
>> + */
>> +int V4L2Device::open()
>> +{
>> + int ret;
>> +
>> + if (isOpen()) {
>> + LOG(Error) << "Device already open";
>> + return -EBUSY;
>> + }
>> +
>> + ret = ::open(devnode_.c_str(), O_RDWR);
>> + if (ret < 0) {
>> + ret = -errno;
>> + LOG(Error) << "Failed to open V4L2 device " << devnode_
>> + << " : " << strerror(ret);
>> + return ret;
>> + }
>> + fd_ = ret;
>> +
>> + ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
>> + if (ret < 0) {
>> + LOG(Error) << "Failed to query device capabilities: " << strerror(errno);
>> + return -errno;
>> + }
>> +
>> + LOG(Debug) << "Opened '" << devnode_ << "' " << caps_.getDriver() << ": " << caps_.getCard();
>> +
>> + if (!(caps_.isCapture())) {
>> + LOG(Error) << "Device is not a capture device";
>> + return -EINVAL;
>> + }
>> +
>> + if (!(caps_.hasStreaming())) {
>> + LOG(Error) << "Device does not support streaming IO";
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * \brief Check if device is successfully opened
>> + */
>> +bool V4L2Device::isOpen() const
>> +{
>> + return (fd_ != -1);
>> +}
>> +
>> +/**
>> + * \brief Close the device, releasing any resources acquired by open()
>> + */
>> +void V4L2Device::close()
>> +{
>> + if (fd_ < 0)
>> + return;
>> +
>> + ::close(fd_);
>> + fd_ = -1;
>> +}
>> +
>> +} /* namespace libcamera */
>> --
>> 2.17.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list