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

[Jacopo Mondi]

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 won't push anymore on this though.

> +
> +	bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }

You pointed out in your self-review this was meant to be changed,
right?

> +	bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }
> +};
> +
> +class V4L2Device
> +{
> +public:
> +	V4L2Device() = delete;

Should this be private?

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

> + */
> +
> +/**
> + * \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

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

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

> +		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);

Ah, I see what you've done here (return with no () )

	bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }

> +}
> +
> +/**
> + * \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
-------------- 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/20190116/ae345213/attachment-0001.sig>