[libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device object
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jan 16 11:50:13 CET 2019
Hi Jacopo,
On 16/01/2019 08:44, Jacopo Mondi wrote:
> Hi Kieran,
> a few more comments, please bear with me a little longer and thanks
> for this series
>
> On Tue, Jan 15, 2019 at 11:19:08PM +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 still feel extending a kernel provided struct into an object is meh.
> Here you could have returned references to two members, intialized
> with the values from struct v4l2_capability....
I think this is a really good way to keep methods and code which
interacts with the struct v4l2_capability together, and treats the C
structure like an object (which it essentially is, it has defined data
structure and it's usage is defined by the kernel API).
It doesn't extend the size of storage - and only defines how to interact
with the data structures.
The getDriver() and getCard() wraps the casting required again into the
class definition so that it doesn't have to be done in arbitrary places
in the code.
I expect to add next:
V4L2Device::driverName() { return caps_.getDriver(); }
V4L2Device::deviceName() { return caps_.getCard(); }
// Or ? getDeviceName()?
V4L2Device::busName() { return caps_.getBus(); }
// (Yes, I'll add this one in to the V4L2Capabiltiy)
I anticipate that these strings will be useful to you in the pipeline
classes.
The UVC pipeline manager should certainly use them.
> I won't push anymore on this though.
I don't mind the push ... I knew this 'wrapping' would be potentially
controversial it's partly me trying to experiment with good ways to
interact with the Kernel API.
At the moment, I must say I really like it though ...
<waits for the tumbleweed to pass if I'm the only one>
Especially with the ease and cleanliness for exposing the capability
strings.
>> +
>> + bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }
>
> You pointed out in your self-review this was meant to be changed,
> right?
Half-right.
Then based on your other comments regarding MPLANE/Single Plane I
decided to drop MPLANE support.
We don't need it yet, the IPU3 doesn't support MPLANE, the RCAR-VIN
doesn't support MPLANE, UVC doesn't support MPLANE ...
>> + bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }
>> +};
>> +
>> +class V4L2Device
>> +{
>> +public:
>> + V4L2Device() = delete;
>
> Should this be private?
I wondered that - but I figured - it doesn't matter if it's public or
private. It's declaring that it's gone. And I thought it was better to
keep the constructors (or lack of) and destructors together.
If this is something we want to standardize (keeping deletions in
private:) I can move it.
>
>> + 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
>
> s/card/device ?
I want to respect the existing naming of the V4L2 API. It's called a
card there.
It can be exposed through the V4L2Device API as a deviceName() as
mentioned above.
>> + */
>> +
>> +/**
>> + * \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);
>
> strerror(-ret)
>
> Even if in this case you could have used errno directly. Sorry, my
> comment might has mis-lead you
Ahh oops - and good spot.
But yes - I'd much rather reference the errno directly for strerror().
>
>> + return ret;
>> + }
>> + fd_ = ret;
>> +
>> + ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
>
> As you pointed out, this might be the compiler performing down-casting
> to a plain 'struct v4l2_capability'
Yes, which is the desired result.
>
>> + 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())) {
>
> if (!caps_.isCapture())
Yes :)
>
>> + LOG(Error) << "Device is not a capture device";
>> + return -EINVAL;
>> + }
>> +
>> + if (!(caps_.hasStreaming())) {
And here...
>> + 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);
>
> Ah, I see what you've done here (return with no () )
>
> bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }
Ah I've been caught! (ok well it wasn't intentional hehe)
I /almost/ want to add () to the isCapture() but I don't want to
increase that line length...
/me cries a little and does:
s/(fd_ != -1)/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