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

246 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 |  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

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

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

Hi Kieran,

On Wed, Jan 16, 2019 at 10:50:13AM +0000, Kieran Bingham wrote:
> 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@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.
>

One note I missed in both my previous comments: getter methods are named as
the member field they access, without the "get" prefix.

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

I'll let other express concerns, if any. Otherwise that's fine with
me.

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

Are you sure?

- entity 4: ipu3-cio2 0 (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video0


# v4l2-ctl -D -d /dev/video0
Driver Info (not using libv4l2):
	Driver name   : ipu3-cio2
	Card type     : Intel IPU3 CIO2
	Bus info      : PCI:0000:00:14.3
	Driver version: 4.20.0
	Capabilities  : 0x84201000
		Video Capture Multiplanar       <---------------
		Streaming
		Extended Pix Format
		Device Capabilities
	Device Caps   : 0x04201000
		Video Capture Multiplanar
		Streaming
		Extended Pix Format

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

No big deal, right...

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

As you like the most!

Thanks
  j

> >
> >> +}
> >> +
> >> +/**
> >> + * \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@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran

Hi Jacopo,

On 16/01/2019 11:11, Jacopo Mondi wrote:

<snip>
>>
>> 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.
>>
> 
> One note I missed in both my previous comments: getter methods are named as
> the member field they access, without the "get" prefix.

I can't do that here.
If I name the method the same as the base struct member I get:

../src/libcamera/include/v4l2_device.h: In member function
‘std::__cxx11::string libcamera::V4L2Capability::driver() const’:
../src/libcamera/include/v4l2_device.h:23:58: error: invalid use of
member function ‘std::__cxx11::string
libcamera::V4L2Capability::driver() const’ (did you forget the ‘()’ ?)
  std::string driver() const { return std::string((char *)driver); }


>>> 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.
>>
> 
> I'll let other express concerns, if any. Otherwise that's fine with
> me.

Ok thanks,


>>>> +
>>>> +	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 ...
>>
> 
> Are you sure?
> 
> - entity 4: ipu3-cio2 0 (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
> 
> 
> # v4l2-ctl -D -d /dev/video0
> Driver Info (not using libv4l2):
> 	Driver name   : ipu3-cio2
> 	Card type     : Intel IPU3 CIO2
> 	Bus info      : PCI:0000:00:14.3
> 	Driver version: 4.20.0
> 	Capabilities  : 0x84201000
> 		Video Capture Multiplanar       <---------------

Hrm ... my grep foo failed me then ...

OK - so MPLANE /is/ back on the requirements list :)

> 		Streaming
> 		Extended Pix Format
> 		Device Capabilities
> 	Device Caps   : 0x04201000
> 		Video Capture Multiplanar
> 		Streaming
> 		Extended Pix Format
> 
>>
>>>> +	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.
>>
> 
> No big deal, right...

I also realise I should add a deletion for the copy constructor...

<added for next spin>

<snip>

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

This assignment feels a little redundant.

I agree on only setting the fd_ = ret if success is good - but I'm
feeling like I can save a line (think of the bits) and just return -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_);
>>>

<snip>

>>>> +/**
>>>> + * \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/
>>
> 
> As you like the most!

I've removed the brackets to simplify.


> 
> Thanks
>   j

<snip>

Hi Kieran,

On Wed, Jan 16, 2019 at 12:07:23PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 16/01/2019 11:11, Jacopo Mondi wrote:
>
> <snip>
> >>
> >> 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.
> >>
> >
> > One note I missed in both my previous comments: getter methods are named as
> > the member field they access, without the "get" prefix.
>
> I can't do that here.
> If I name the method the same as the base struct member I get:
>
> ../src/libcamera/include/v4l2_device.h: In member function
> ‘std::__cxx11::string libcamera::V4L2Capability::driver() const’:
> ../src/libcamera/include/v4l2_device.h:23:58: error: invalid use of
> member function ‘std::__cxx11::string
> libcamera::V4L2Capability::driver() const’ (did you forget the ‘()’ ?)
>   std::string driver() const { return std::string((char *)driver); }
>

Ah! that's because of the v4l2_capability 'driver' field.
That would be called driver_ in a standalone V4L2Capabilities class...
>
> >>> 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.
> >>
> >
> > I'll let other express concerns, if any. Otherwise that's fine with
> > me.
>
> Ok thanks,
>
>
> >>>> +
> >>>> +	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 ...
> >>
> >
> > Are you sure?
> >
> > - entity 4: ipu3-cio2 0 (1 pad, 1 link)
> >             type Node subtype V4L flags 0
> >             device node name /dev/video0
> >
> >
> > # v4l2-ctl -D -d /dev/video0
> > Driver Info (not using libv4l2):
> > 	Driver name   : ipu3-cio2
> > 	Card type     : Intel IPU3 CIO2
> > 	Bus info      : PCI:0000:00:14.3
> > 	Driver version: 4.20.0
> > 	Capabilities  : 0x84201000
> > 		Video Capture Multiplanar       <---------------
>
> Hrm ... my grep foo failed me then ...
>
> OK - so MPLANE /is/ back on the requirements list :)
>

Yes please. MPLANE support should be there from the very beginning
imho.

> > 		Streaming
> > 		Extended Pix Format
> > 		Device Capabilities
> > 	Device Caps   : 0x04201000
> > 		Video Capture Multiplanar
> > 		Streaming
> > 		Extended Pix Format
> >
> >>
> >>>> +	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.
> >>
> >
> > No big deal, right...
>
> I also realise I should add a deletion for the copy constructor...
>
> <added for next spin>
>
> <snip>

Good point. Thanks.

>
> >>>> +/**
> >>>> + * \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;
>
> This assignment feels a little redundant.
>
> I agree on only setting the fd_ = ret if success is good - but I'm
> feeling like I can save a line (think of the bits) and just return -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_);
> >>>
>
> <snip>
>
> >>>> +/**
> >>>> + * \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/
> >>
> >
> > As you like the most!
>
> I've removed the brackets to simplify.

Thanks
   j

>
>
> >
> > Thanks
> >   j
>
> <snip>
> --
> Regards
> --
> Kieran

Hi Kieran, Jacopo



On 2019-01-16 09:44:55 +0100, Jacopo Mondi wrote:

[snip]

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

No you can't use errno in the call to strerror() as it might already 
have been been changed by the previous strings passed to LOG(Error), 
unfortunately it needs to be cached.

> 
> > +		return ret;
> > +	}

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

Hi Niklas,

On Wed, Jan 16, 2019 at 03:54:23PM +0100, Niklas Söderlund wrote:
> Hi Kieran, Jacopo
>
>
>
> On 2019-01-16 09:44:55 +0100, Jacopo Mondi wrote:
>
> [snip]
>
> > > +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
>
> No you can't use errno in the call to strerror() as it might already
> have been been changed by the previous strings passed to LOG(Error),
> unfortunately it needs to be cached.
>

Right, that's why we standardized on that patter... sorry, I've been
sloppy commenting this...

Kieran, pay attention to this in the IOCTL error handling you have a
few lines below these ones...

Thanks
  j

> >
> > > +		return ret;
> > > +	}
>
> --
> Regards,
> Niklas Söderlund

On 16/01/2019 15:00, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Wed, Jan 16, 2019 at 03:54:23PM +0100, Niklas Söderlund wrote:
>> Hi Kieran, Jacopo
>>
>>
>>
>> On 2019-01-16 09:44:55 +0100, Jacopo Mondi wrote:
>>
>> [snip]
>>
>>>> +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
>>
>> No you can't use errno in the call to strerror() as it might already
>> have been been changed by the previous strings passed to LOG(Error),
>> unfortunately it needs to be cached.
>>
> 
> Right, that's why we standardized on that patter... sorry, I've been
> sloppy commenting this...
> 
> Kieran, pay attention to this in the IOCTL error handling you have a
> few lines below these ones...
> 

Ok - updated. Can I change the location of the negative inversions?

Setting ret = -errno, to then re-negate it in strerror() feels horrible
to me.

Negating it for the return code seems more acceptable as we know that
return codes are checked for if(ret < 0)...



{
	...

	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_.caps());
	if (ret < 0) {
		ret = errno;
		LOG(Error) << "Failed to query device capabilities: " << strerror(ret);
		return -ret;
	}
}



> Thanks
>   j
> 
>>>
>>>> +		return ret;
>>>> +	}
>>
>> --
>> Regards,
>> Niklas Söderlund

Hi Niklas,

Ok - here's an attempt at explaining my rationale and also hopefully
some updates which might make you like things more (I've dropped the 'get')

On 16/01/2019 14:59, Niklas Söderlund wrote:
> 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@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().

Can you explain /why/ you think it's a deal breaker please?


Is your objection to the 'get' prefix (as in getDriver())?
I now have a solution for that with appropriate casting:

 std::string driver() const {
	return reinterpret_cast<const char *>(v4l2_capability::driver);
 }


Otherwise, I believe it does inherit from v4l2_capability.

If I could I would write write:

struct v4l2_capability : struct v4l2_capability {
public:
	std::string driver() const;
	std::string card() const;
	bool isCapture() const;
};


The point is if I could add methods to the struct v4l2_capability, I'd
be doing that ... but I can't. And I certainly don't want to duplicate
the structure...

The V4L2Capability is simply providing an interface at the C++ level and
defining how to work with the data in the structure in an object
orientated way.

This means that any methods which need to interact with struct
v4l2_capability are contained in a common location - and those methods
can only operate on the structure ...

Putting a v4l2_capability as a private member in a class V4L2Capability
produces: [2] which I quite dislike. I've updated V4L2Capability to mark
it final to express that this class should not be further extended or
overridden.

[1] https://paste.debian.net/1060853/ {inheritance}
[2] https://paste.debian.net/1060852/ {composition}
[3] https://paste.debian.net/1060839/ {flat in V4L2Device}


My biggest reason for continuing to push this - is because I feel this
pattern will be beneficial for the many complex structure types in V4L2.
I'm thinking controls, pix_format, fmtdesc, requestbuffers...

(Not everything, just ones where it makes sense to define helpers or
bring the kernel API to a C++ interface)

They are all 'objects' which are managed by the kernel. It's just that
the data for them is in a c-struct because that's what the kernel has.

In my opinion - handling those types as objects in the C++ will keep the
code where it counts. On the object it deals with.

If we go with [3] (which I don't want), then the V4L2Device class is
going to be polluted with all sorts of helpers (helpful as they may be)
which operate on the various structures. It's also no good in the case
where there are multiple objects (like v4l2_buffers).


I don't think [2] is appropriate here. It doesn't protect us from
anything, and we are not extending any types or fields in the struct
v4l2_capability.


(I'd also envisage re-using this V4L2Device class externally too, so
having nice wrappings around things can provide further benefits)



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

Grammar :)

The device 'is' a capture device.
	(_CAPTURE | _CAPTURE_MPLANE) vs (_OUTPUT / _M2M)

The device 'has' streaming IO ioctls
The device 'has' async io ioctls

...

(Now technically an M2M 'is' both a _CAPTURE and an _OUTPUT but that's
differentiated by being an M2M...)

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

Hi Kieran,

On Wed, Jan 16, 2019 at 12:07:23PM +0000, Kieran Bingham wrote:
> On 16/01/2019 11:11, Jacopo Mondi wrote:

[snip]

> >>>> +/**
> >>>> + * \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;
> 
> This assignment feels a little redundant.
> 
> I agree on only setting the fd_ = ret if success is good - but I'm
> feeling like I can save a line (think of the bits) and just return -errno?

You can't because LOG(Error) << ... may change errno. That's also why
you need to pass -ret to strerror and not errno, as it may change by the
time strerror is called.

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

[snip]

Hi Kieran,

On Wed, Jan 16, 2019 at 06:00:55PM +0000, Kieran Bingham wrote:
> On 16/01/2019 14:59, Niklas Söderlund wrote:
> > 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@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().
> 
> Can you explain /why/ you think it's a deal breaker please?
> 
> 
> Is your objection to the 'get' prefix (as in getDriver())?
> I now have a solution for that with appropriate casting:
> 
>  std::string driver() const {
> 	return reinterpret_cast<const char *>(v4l2_capability::driver);
>  }
> 
> 
> Otherwise, I believe it does inherit from v4l2_capability.
> 
> If I could I would write write:
> 
> struct v4l2_capability : struct v4l2_capability {
> public:
> 	std::string driver() const;
> 	std::string card() const;
> 	bool isCapture() const;
> };
> 
> 
> The point is if I could add methods to the struct v4l2_capability, I'd
> be doing that ... but I can't. And I certainly don't want to duplicate
> the structure...
> 
> The V4L2Capability is simply providing an interface at the C++ level and
> defining how to work with the data in the structure in an object
> orientated way.
> 
> This means that any methods which need to interact with struct
> v4l2_capability are contained in a common location - and those methods
> can only operate on the structure ...
> 
> Putting a v4l2_capability as a private member in a class V4L2Capability
> produces: [2] which I quite dislike. I've updated V4L2Capability to mark
> it final to express that this class should not be further extended or
> overridden.
> 
> [1] https://paste.debian.net/1060853/ {inheritance}
> [2] https://paste.debian.net/1060852/ {composition}
> [3] https://paste.debian.net/1060839/ {flat in V4L2Device}
> 
> 
> My biggest reason for continuing to push this - is because I feel this
> pattern will be beneficial for the many complex structure types in V4L2.
> I'm thinking controls, pix_format, fmtdesc, requestbuffers...
> 
> (Not everything, just ones where it makes sense to define helpers or
> bring the kernel API to a C++ interface)
> 
> They are all 'objects' which are managed by the kernel. It's just that
> the data for them is in a c-struct because that's what the kernel has.
> 
> In my opinion - handling those types as objects in the C++ will keep the
> code where it counts. On the object it deals with.
> 
> If we go with [3] (which I don't want), then the V4L2Device class is
> going to be polluted with all sorts of helpers (helpful as they may be)
> which operate on the various structures. It's also no good in the case
> where there are multiple objects (like v4l2_buffers).
> 
> 
> I don't think [2] is appropriate here. It doesn't protect us from
> anything, and we are not extending any types or fields in the struct
> v4l2_capability.
> 
> 
> (I'd also envisage re-using this V4L2Device class externally too, so
> having nice wrappings around things can provide further benefits)

FOr what it's worth, I think wrapping v4l2_capabilities is useful in
this case, and I like the proposed implementation, especially with the
solution that creates getters with a get prefix.

> >> +
> >> +	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'?
> 
> Grammar :)
> 
> The device 'is' a capture device.
> 	(_CAPTURE | _CAPTURE_MPLANE) vs (_OUTPUT / _M2M)
> 
> The device 'has' streaming IO ioctls
> The device 'has' async io ioctls

And the device 'has' capture capability ;-) Both prefixes are valid, I'm
slightly tempted to standardize on has in this case, but I won't push
for it.

> ...
> 
> (Now technically an M2M 'is' both a _CAPTURE and an _OUTPUT but that's
> differentiated by being an M2M...)
> 
> > 
> >> +};

[snip]

Hi Kieran,

On Wed, Jan 16, 2019 at 03:42:50PM +0000, Kieran Bingham wrote:
> On 16/01/2019 15:00, Jacopo Mondi wrote:
> > On Wed, Jan 16, 2019 at 03:54:23PM +0100, Niklas Söderlund wrote:
> >> On 2019-01-16 09:44:55 +0100, Jacopo Mondi wrote:
> >>
> >> [snip]
> >>
> >>>> +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
> >>
> >> No you can't use errno in the call to strerror() as it might already
> >> have been been changed by the previous strings passed to LOG(Error),
> >> unfortunately it needs to be cached.
> >>
> > 
> > Right, that's why we standardized on that patter... sorry, I've been
> > sloppy commenting this...
> > 
> > Kieran, pay attention to this in the IOCTL error handling you have a
> > few lines below these ones...
> > 
> 
> Ok - updated. Can I change the location of the negative inversions?
> 
> Setting ret = -errno, to then re-negate it in strerror() feels horrible
> to me.
> 
> Negating it for the return code seems more acceptable as we know that
> return codes are checked for if(ret < 0)...
> 
> {
> 	...
> 
> 	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_.caps());
> 	if (ret < 0) {
> 		ret = errno;
> 		LOG(Error) << "Failed to query device capabilities: " << strerror(ret);
> 		return -ret;
> 	}
> }

I think both options have their pros and cons. I'm fine with this
proposal, but in that case, please change it library-wide.