[libcamera-devel,v2,1/2] lib: Add V4L2 Device object

242 diff mbox series

Provide a helper V4L2 device object capable of interacting with the
V4L2 Linux Kernel APIs.

Signed-off-by: Kieran Bingham <kieran.bingham@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

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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On 15/01/2019 16:39, Jacopo Mondi wrote:
> 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@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...)

That leaves v4l2_device.h - V4L2 Device....

Seems to make "V4L2 Device" a bit redundant, as that's expressed by the
filename?

We are abstracting the V4L2 device node API ... which is what I think I
meant in the file brief...



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

/me shudders again

Ok :-(

Which version? 4.19? 4.20? (git-master?)
Which version are the other files? (the README states 4.19)
How do we keep them in sync?


>> +
>> +namespace libcamera {
>> +
>> +class V4L2Capability : public v4l2_capability
> 
> Is this the kernel header provided one?

Yes, the public v4l2_capability is the
   linux/videodev2.h::struct v4l2_capability

> 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'm not sure I understand that point?

> I would prefer accessing the
> v4l2_capability directly, and fail at compile time if there have been
> changes.

If the v4l2_capability struct changes drastically it will still fail at
compile time.

I'm only adding member functions which operate on the struct
v4l2_capability.

Wrapping them in the class V4L2Capability means that the functions are
tied to the type.

The V4L2Capability could be extended to add more features and helpers as
necessary as they are needed.


> Furthermore, you only use the 'capabilities' field of this structure,
> am I wrong?


Yes, currently I do - because this is the simplified version based on
Laurent's review comments to my previous queryCap().


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

I guess this implementation is wrong. It should probably read:

 capabilites & (V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE);

The check is to tell if the device being opened will provide frames...
as opposed to being an M2M device or an Output device.



>> +	bool isMplane() { return capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE; }

This then determines if the device can support the mplane API...

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


Another reason why I think we should have a base Device class!

It's not just about code sharing - it's about defining the common interface!

To me a device node is a major and minor device number... Shouldn't this
instead be devicePath_ ?


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

This is not a getter. It does not return an internal value. It parses
the internal state for information and classifies that state to
determine if the device is a capture device ...



> 
>> + * \return boolean true if the device provides video frames
> 
> s/boolean//
> ", false otherwise" at the end

Really ?

 Do we have to say that a boolean returns either true or false?


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

... er ...

Oh - I see you were removing the worked boolean. But that's the part
that describes it will be false otherwise..


> no need for a isSinglePlane() ?
> Do we need to define isMulti/isSingle, or should we return a flag from
> a 'V4L2Capability::plane()' function ?

This isn't a full implementation of the parsing of the
v4l2_capabilities. It can grow with time based on what is needed.

I very soon need to know if the device is an MPLANE device (i.e.


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

Good spot - I should make this 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 yes - I should move to the \param ...

> 
> Ah, and the 'object' term comes from Java. I tried to avoid using it
> and refer to classes/instances

Does it (come from Java?) ? I thought C++ was an object orientated
language.. where an object was an instantiation of the class type.

This this is the constructor for that object instantiation...


>> + * \param device The file-system path to the video device node
>> + */
>> +V4L2Device::V4L2Device(const std::string &device)
>> +{
>> +	device_ = device;
>> +	fd_ = -1;
>> +	caps_ = { };
> 
> Initializers list?

Yup.


> 
>> +}
>> +
>> +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"

Sure that's a bit clearer


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

It's not larger. It's the same size. I validated that
sizeof(V4L2Capability) and sizeof(struct v4l2_capability) were identical
when putting the code together.

I was actually trying to work out how to make a function to 'get' a
pointer to the base class type, i.e. a (struct v4l2_capabillity *)self
getter - and I somehow ended up with this which generates no compile
warnings and works.

I've also validated that the data is aligned the same etc whether you
pass a struct v4l2_capabiltiy or a V4L2Capability.

I originally thought that this was the compiler perfoming the
polymorphism correctly to the base type because of the type information
in the ioctl() parameter - but now I'm not sure if it's perhaps just
coincidental (or rather design) that the base class is the first part of
the subclass.

The V4L2Capability is not expecting to store extra data anyway. It's
just a way to tie methods to the type.


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

Is your question based on the tense again here?


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

Oh - I thought \a was 'anchor' as in to generate a hyperlink anchor to a
named thing.

I want open() to be linked to the open() documentation.
Maybe \sa is also appropriate - I'll see how it affects the Doxygen output.

>> + */
>> +void V4L2Device::close()
>> +{
>> +	if (fd_ < 0)
>> +		return;
>> +
>> +	::close(fd_);
>> +	fd_ = -1;
>> +}
>> +
>> +} /* namespace libcamera */
>> --
>> 2.17.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

On 15/01/2019 20:46, Kieran Bingham wrote:
> Hi Jacopo,

<snip>


>>> + * \param device The file-system path to the video device node
>>> + */
>>> +V4L2Device::V4L2Device(const std::string &device)
>>> +{
>>> +	device_ = device;
>>> +	fd_ = -1;
>>> +	caps_ = { };

Interestingly, because the V4L2Capability caps_ is a class - it
automatically gets initialised to 0, where as a struct v4l2_capability
here does not.

>> Initializers list?
> 
> Yup.

Fixed.

Working through the rest now.

<snip>