[libcamera-devel] [PATCH v2 1/2] lib: Add V4L2 Device object
Jacopo Mondi
jacopo at jmondi.org
Tue Jan 15 17:39:51 CET 2019
Hi Kieran,
thanks for the patch
On Tue, Jan 15, 2019 at 04:02:11PM +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 | 43 ++++++++++
> src/libcamera/meson.build | 2 +
> src/libcamera/v4l2_device.cpp | 127 ++++++++++++++++++++++++++++
> 3 files changed, 172 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..ddcb17af2187
> --- /dev/null
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_device.h - V4L2 Device API Abstractions
I would leave "API" out (and "abstraction" too tbh...)
> + */
> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> +#define __LIBCAMERA_V4L2_DEVICE_H__
> +
> +#include <string>
> +
> +#include <linux/videodev2.h>
Please add this header to include/linux, otherwise you're relying on
the system wide installed one
> +
> +namespace libcamera {
> +
> +class V4L2Capability : public v4l2_capability
Is this the kernel header provided one? If so, I'm not that sure is a
good idea to extend it directly. True, you get compatibility with the
current header version automatically, but that might hide other issues
if the kernel API changes. I would prefer accessing the
v4l2_capability directly, and fail at compile time if there have been
changes.
Furthermore, you only use the 'capabilities' field of this structure,
am I wrong?
> +{
> +public:
> + bool isCapture() { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }
> + bool isMplane() { return capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE; }
> + bool hasStreaming() { return capabilities & V4L2_CAP_STREAMING; }
> +};
> +
> +class V4L2Device
> +{
> +public:
> + V4L2Device() = delete;
> + V4L2Device(const std::string &device);
> + ~V4L2Device();
> +
> + int open();
> + bool isOpen() const;
> + void close();
> +
> +private:
> + std::string device_;
devnode_ to standardize with MediaDevice ?
> + 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..3133bfe4ffb0
> --- /dev/null
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_device.cpp - V4L2 Device API
Keep in sync with the header please
> + */
> +
> +#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 wraps the V4L2 API interactions for capabilities and
> + * device flag parsing.
> + */
I would mention that the class uses informations provided by the V4L2
APIs defined 'struct v4l2_capabilities', but if you agree with what
I've said above, this is not a wrapper.
> +
> +/**
> + * \fn bool V4L2Capability::isCapture()
> + * \brief Identify if the device is capable of capturing video
For getter we used the "Retrieve ... " syntax in \brief descriptions.
This is not strictly a getters, I'm fine with "Identify" as long as it
is used consistently in other 'identity' function descriptions.
> + * \return boolean true if the device provides video frames
s/boolean//
", false otherwise" at the end
> + */
> +
> +/**
> + * \fn bool V4L2Capability::isMplane()
> + * \brief Identify if the device uses MPLANE formats
> + * \return boolean true if the device uses multiplanar buffer formats
s/boolean//
", false otherwise" at the end
no need for a isSinglePlane() ?
Do we need to define isMulti/isSingle, or should we return a flag from
a 'V4L2Capability::plane()' function ?
> + */
> +
> +/**
> + * \fn bool V4L2Capability::hasStreaming()
> + * \brief Determine if the device can perform Streaming IO
> + * \return boolean true if the device provides Streaming IO IOCTLs
s/boolean//
", false otherwise" at the end
> + */
> +
> +/**
> + * \class V4L2Device
> + * \brief V4L2Device object and API.
> + *
> + * The V4L2 Device API class models an instance of a v4l2 device node.
Please expand this with indications on how the class is expected to be
constructed (by its devnode path, if the devnode is checked to be
valid), if it has to be opened before it can be operated on, how and who
shall destroy it etc..
And I think it's either V4L2 or v4l2.
> + */
> +
> +/**
> + * \brief Constructor for a V4L2Device object. The \a device specifies the path
> + * to the video device node.
Following the suggestions I have received on MediaObject
\brief Construct a V4L2Device
\param The V4L2 device node path
Ah, and the 'object' term comes from Java. I tried to avoid using it
and refer to classes/instances
> + * \param device The file-system path to the video device node
> + */
> +V4L2Device::V4L2Device(const std::string &device)
> +{
> + device_ = device;
> + fd_ = -1;
> + caps_ = { };
Initializers list?
> +}
> +
> +V4L2Device::~V4L2Device()
> +{
> + close();
> +}
> +
> +/**
> + * \brief Opens a v4l2 device and queries properties from the device.
I dont' see other \briefs using the third person. We can change this
though
> + * \return 0 on success, or a negative errno
Copying from the classes we have in at the moment:
"or a negative error code otherwise"
> + */
> +int V4L2Device::open()
> +{
> + int ret;
> +
> + if (isOpen()) {
> + LOG(Error) << "Device already open";
> + return -EBUSY;
> + }
> +
> + fd_ = ::open(device_.c_str(), O_RDWR);
> + if (fd_ < 0) {
> + LOG(Error) << "Failed to open V4L2 device " << device_
> + << " : " << strerror(errno);
> + return -errno;
We standardized on the following error handling pattern:
int ret = ::open(devnode_.c_str(), O_RDWR);
if (ret < 0) {
ret = -errno;
LOG(Error) << "Failed to open v4l2 device at " << devnode_
<< ": " << strerror(-ret);
return ret;
}
fd_ = ret;
So that you don't have to assign fd_ before we know open succeeded
> + }
> +
> + ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
Is this the V4L2Capability instance? Is it safe to pass to an IOCTL
something larger that what it might expect? I'm genuinely asking this,
not sure...
> + if (ret < 0) {
> + LOG(Error) << "Failed to query device capabilities: " << strerror(errno);
> + return -errno;
> + }
> +
> + if (!(caps_.hasStreaming())) {
> + LOG(Error) << "Device does not support streaming IO";
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * \brief Checks to see if we have successfully opened a v4l2 video device.
Check (or Verify, or Return) if the device is open
?
> + */
> +bool V4L2Device::isOpen() const
> +{
> + return (fd_ != -1);
> +}
> +
> +/**
>+ * \brief Close the device, releasing any resources acquired by \a open().
open() is not an argument. You might use \sa open() but I don't think
it is necessary.
> + */
> +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
-------------- 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/20190115/85965dc1/attachment.sig>
More information about the libcamera-devel
mailing list