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

[Kieran Bingham]

Hi Jacopo,

Thanks for the review,

On 21/12/2018 16:25, jacopo mondi wrote:
> Hi Kieran,
>     thanks for the patch
> 
> On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:
>> Provide a helper V4L2 device object capable of interacting with the
>> V4L2 Linux Kernel APIs.
>>
>> A test suite is added at test/v4l2_device/ to utilise and validate the
>> code base.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/libcamera/include/v4l2_device.h   |  36 +++++++
>>  src/libcamera/meson.build             |   2 +
>>  src/libcamera/v4l2_device.cpp         | 137 ++++++++++++++++++++++++++
> 
> I would really split test and library code in 2 differen commits.

I'm happy to go with group consensus here, but I felt that tests and
code could be in the same commit here, as the test directly affects the
code added.

This means that when doing rebase actions or such, I can do 'git rebase
-i -x "ninja; ninja test"' and validate every commit as I rework things.

(I have a script which does that for me)


>>  test/meson.build                      |   2 +
>>  test/v4l2_device/double_open.cpp      |  32 ++++++
>>  test/v4l2_device/meson.build          |  12 +++
>>  test/v4l2_device/v4l2_device_test.cpp |  36 +++++++
>>  test/v4l2_device/v4l2_device_test.h   |  31 ++++++
>>  8 files changed, 288 insertions(+)
>>  create mode 100644 src/libcamera/include/v4l2_device.h
>>  create mode 100644 src/libcamera/v4l2_device.cpp
>>  create mode 100644 test/v4l2_device/double_open.cpp
>>  create mode 100644 test/v4l2_device/meson.build
>>  create mode 100644 test/v4l2_device/v4l2_device_test.cpp
>>  create mode 100644 test/v4l2_device/v4l2_device_test.h
>>
>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
>> new file mode 100644
>> index 000000000000..619d932d3c82
>> --- /dev/null
>> +++ b/src/libcamera/include/v4l2_device.h
>> @@ -0,0 +1,36 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * v4l2_device.h - V4L2 Device API Abstractions
>> + */
>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
>> +#define __LIBCAMERA_V4L2_DEVICE_H__
>> +
>> +#include <string>
>> +
>> +#include <linux/videodev2.h>
>> +
>> +namespace libcamera {
>> +
>> +class V4L2Device
>> +{
>> +public:
>> +	V4L2Device(const std::string &);
>> +	~V4L2Device();
>> +
>> +	bool isOpen();
>> +	int open();
>> +	void close();
>> +
>> +	int queryCap();
>> +
>> +private:
>> +	std::string device_;
>> +	int fd_;
>> +	int capabilities_;
>> +};
>> +
>> +}
> 
> nit:
> } /* namespace libcamera */
> 

Ah yes - that might be nice.

How did I miss this?
Do we have this in our templates?

Hrm ... nope, this end comment isn't there.

I've added it to the template document.


>> +
>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index f632eb5dd779..bbaf9a05ec2c 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -1,11 +1,13 @@
>>  libcamera_sources = files([
>>      'log.cpp',
>>      'main.cpp',
>> +    'v4l2_device.cpp',
>>  ])
>>
>>  libcamera_headers = files([
>>      'include/log.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..eea2514f343c
>> --- /dev/null
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -0,0 +1,137 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * v4l2_device.cpp - V4L2 Device API
>> + */
>> +
>> +#include <fcntl.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +
>> +#include <sys/ioctl.h>
>> +#include <sys/mman.h>
>> +
>> +#include "log.h"
>> +#include "v4l2_device.h"
>> +
>> +/**
>> + * \file v4l2_device.cpp
> 
> file v4l2_device.h to document here what is defined in the header file

Oh - So this is supposed to reference the header not declare the current
file. I guess that kind of makes sense :) Always pointless to state the
name of the current file.

> 
>> + * \brief V4L2 Device API
>> + */
>> +namespace libcamera {
>> +
>> +/**
>> + * \class V4L2Device
>> + * \brief V4L2Device object and API.
>> + *
>> + * The V4L2 Device API class models a single instance of a v4l2 device node.
>> + */
>> +
>> +/**
>> + * \fn V4L2Device::V4L2Device(device)
> 
> nit: parameter type?

I believe doxygen collects that automatically?

It generates this in the output:
  libcamera::V4L2Device::V4L2Device(const std::string & device)


>> + *
>> + * Default constructor for a V4L2Device object. The \a device specifies a
>> + * string to representation of the device object.
> 
> My poor English skill do not allow me to say if this is a typo or it
> actually makes sense :)

Ahem - there's certainly a typo there - good spot.

It's also not necessarily correct, and is too close from my Camera class
constructor text.

I'll adapt this to :

 * Default constructor for a V4L2Device object. The \a device specifies
 * the path to the video device node.

> 
>> + */
>> +V4L2Device::V4L2Device(const std::string &device)
>> +{
>> +	device_ = device;
>> +	fd_ = -1;
>> +	capabilities_ = 0;
>> +
>> +	LOG(Debug) << "V4L2Device Constructed for " << device_;
> 
> Not sure a debug printout is worth here

Certainly not for long... they're just here for early dev.

I can just drop these - they're not useful.

> 
>> +}
>> +
>> +V4L2Device::~V4L2Device()
>> +{
>> +	close();
>> +
>> +	LOG(Debug) << "V4L2Device Destroyed";
> 
> ditto
> 
>> +}
>> +
>> +/**
>> + * \fn V4L2Device::isOpen(())
> 
> one () in excess?

Not sure how that happened?

> 
>> + *
>> + * Checks to see if we have successfully opened a v4l2 video device.
>> + */
>> +bool V4L2Device::isOpen()
>> +{
>> +	return (fd_ != -1);
> 
> nit: no need to () braces

Can I keep them ?

Returning more than a simple variable or statement 'openly' feels
horrible to me ...


>> +}
>> +
>> +/**
>> + * \fn V4L2Device::open()
>> + *
>> + * Opens a v4l2 device and queries properties from the device.
>> + */
>> +int V4L2Device::open()
>> +{
>> +	int ret;
>> +
>> +	if (isOpen()) {
>> +		LOG(Error) << "Device already open";
>> +		return -EBADF;
>> +	}
>> +
>> +	fd_ = ::open(device_.c_str(), O_RDWR);
>> +	if (fd_ < 0) {
>> +		LOG(Error) << "Failed to open V4L2 device " << device_
>> +			   << " : " << strerror(errno);
>> +		return -errno;
>> +	}
>> +
>> +	ret = queryCap();
> 
> I don't like it much either, but this might have been "int ret = "

Hrm ... that certainly is late instantiation.

Is that what we're going for ?


> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!(capabilities_ & V4L2_CAP_STREAMING)) {
> 
> I wonder if it wouldn't be better to return the capabilities from
> queryCap, and assign them to the class member capabilities_ after this
> check here.
> 
> Looking at the code I -knew- that the queryCap() function had modified
> the capabilities_ and then you're inspecting them here, but it's all a
> bit implicit to me...

How will we determine if the ioctl call succeeded?
 (while avoiding exceptions?)

Capabilities flag can return any free-form int value. (ok - so it's
restricted to the flags available in V4L2 ... but... that's very
function specific)



>> +		LOG(Error) << "Device does not support streaming IO";
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \fn V4L2Device::close()
>> + *
>> + * Close a v4l2 device handle.
>> + */
>> +void V4L2Device::close()
>> +{
>> +	if (fd_ < 0)
>> +		return;
>> +
>> +	::close(fd_);
>> +	fd_ = -1;
>> +}
>> +
>> +/**
>> + * \fn V4L2Device::queryCap()
>> + *
>> + * Obtains the capabilities information from the V4L2 Device.
>> + */
>> +int V4L2Device::queryCap()
>> +{
>> +	struct v4l2_capability cap = {};
>> +	int ret;
>> +
>> +	if (!isOpen()) {
>> +		LOG(Error) << "Attempt to query unopened device";
>> +		return -EBADF;
>> +	}
>> +
>> +	ret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);
>> +	if (ret < 0) {
>> +		LOG(Error) << "Failed to query device capabilities";
>> +		return -1;
> 
> It would be useful to return the errno number and possibly print it
> out with strerror

Good point!

Should this be -errno? Or just return ret?


The more I think about it - the more I'd like to wrap the ioctl call in
the class (or more likely a Device base class) which will handle the
error checks...

That could then be a parent class for your MediaDevice object too, and
the expected V4L2SubDevice ...

What are your thoughts? Over-abstracting? or useful ...


> 
>> +	}
>> +
>> +	capabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS
>> +		      ? cap.device_caps : cap.capabilities;
>> +
>> +	return 0;
>> +}
>> +
>> +} /* namespace libcamera */
>> diff --git a/test/meson.build b/test/meson.build
>> index 754527324c7d..2bc76c289eea 100644
>> --- a/test/meson.build
>> +++ b/test/meson.build
>> @@ -12,6 +12,8 @@ test_includes_internal = [
>>    libcamera_internal_includes,
>>  ]
>>
>> +subdir('v4l2_device')
>> +
>>  public_tests = [
>>    [ 'test_init',      'init.cpp' ],
> 
> Not related to this, but weird spacing here

This was intentional ... pre-allocation of table spacing. (The table
will have multiple entries...)

> 
>>  ]
>> diff --git a/test/v4l2_device/double_open.cpp b/test/v4l2_device/double_open.cpp
>> new file mode 100644
>> index 000000000000..1b7c7bfe14b8
>> --- /dev/null
>> +++ b/test/v4l2_device/double_open.cpp
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * libcamera V4L2 API tests
>> + */
>> +
>> +#include "v4l2_device_test.h"
>> +
>> +class DoubleOpen : public V4L2DeviceTest
>> +{
>> +protected:
>> +	int run()
>> +	{
>> +		int ret;
>> +
>> +		/*
>> +		 * Expect failure: The device has already been opened by the
>> +		 * V4L2DeviceTest base class
>> +		 */
>> +		ret = dev->open();
>> +		if (!ret) {
>> +			fprintf(stderr, "Double open erroneously succeeded\n");
>> +			dev->close();
>> +			return TEST_FAIL;
>> +		}
>> +
>> +		return TEST_PASS;
>> +	}
> 
> nit: < one indent level

Oh -- for all class functions?


>> +};
> 
> I welcome testing, but do we really need tests to make sure a function
> fails when called twice? 

Yes - I felt this was important.

It's there because once I added the open() and close() calls - it
provided a way to leak fd's if you call open(), open().

The first fd would get silently dropped ...

So I added a test to call open twice, then I added the isOpen() checks.

In other words - the fix for the leak came about directly because I
added the double_open test...


Another reason we I think we should base class the common device
operations (then this test would only apply to the Device object unit
tests anyway.)


> I'm sure you could have sketched it by
> calling open twice in the V4L2DeviceTest class

No - the V4L2DeviceTest class is the base class common to all further
tests for the V4L2Device Object.

I have other patches which add further tests using the V4L2DeviceTest
base class.

>> +
>> +TEST_REGISTER(DoubleOpen);
>> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build
>> new file mode 100644
>> index 000000000000..41675a303498
>> --- /dev/null
>> +++ b/test/v4l2_device/meson.build
>> @@ -0,0 +1,12 @@
>> +# Tests are listed in order of complexity.
>> +# They are not alphabetically sorted.
>> +v4l2_device_tests = [
>> +  [ 'double_open',        'double_open.cpp' ],
> 
> Again weird spacing, I'm now thinking this is intentional and I don't
> know something

Table pre-allocation for multiple tests.

This isn't my only test, and I'd rather not have to reformat the table
if I add tests with longer filenames.

> 
>> +]
>> +
>> +foreach t : v4l2_device_tests
>> +  exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],
>> +		   link_with : test_libraries,
>> +		   include_directories : test_includes_internal)
>> +  test(t[0], exe, suite: 'v4l2_device', is_parallel: false)
>> +endforeach
>> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
>> new file mode 100644
>> index 000000000000..ae317a9519c5
>> --- /dev/null
>> +++ b/test/v4l2_device/v4l2_device_test.cpp
>> @@ -0,0 +1,36 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * libcamera V4L2 API tests
>> + */
>> +
>> +#include "v4l2_device_test.h"
>> +
>> +using namespace libcamera;
>> +
>> +V4L2DeviceTest::V4L2DeviceTest()
>> +{
>> +	dev = nullptr;
>> +}
>> +
>> +int V4L2DeviceTest::init()
>> +{
>> +	const char *device = "/dev/video0";
>> +
>> +	/* Validate the device node exists. */
>> +	if (!path_exists(device))
>> +		return TEST_SKIP;
>> +
>> +	dev = new V4L2Device(device);
>> +	if (!dev)
> 
> You should call cleanup here if you fail
> Ah no, the Test class should call it if init() fails!
> Ah no^2, your constructor cannot fail (good!). My previous comment
> still hold though.
> 

Should I instead call cleanup() from my destructor ?



>> +		return TEST_FAIL;
>> +
>> +	return dev->open();
>> +}
>> +
>> +void V4L2DeviceTest::cleanup()
>> +{
>> +	if (dev)
>> +		delete dev;
>> +};
>> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h
>> new file mode 100644
>> index 000000000000..4374ddc7e932
>> --- /dev/null
>> +++ b/test/v4l2_device/v4l2_device_test.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * vl42device_test.h - libcamera v4l2device test base class
>> + */
>> +
>> +#include "test.h"
>> +#include "v4l2_device.h"
> 
> Includes after the inclusion guards

Ack.


>> +
>> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_
>> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_
>> +
>> +using namespace libcamera;
>> +
>> +class V4L2DeviceTest : public Test
>> +{
>> +public:
>> +	V4L2DeviceTest();
>> +	virtual ~V4L2DeviceTest() {};
>> +
>> +protected:
>> +	int init();
>> +	void cleanup();
>> +
>> +	virtual int run() = 0;
>> +
>> +	V4L2Device *dev;
>> +};
>> +
>> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */
>> --
>> 2.17.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran