[libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device object

[Niklas Söderlund]

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().

> +
> +	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'?

> +};
> +
> +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,
Niklas Söderlund