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

[Laurent Pinchart]

Hello,

On Sunday, 23 December 2018 22:54:09 EET Jacopo Mondi wrote:
> On Sat, Dec 22, 2018 at 08:30:37PM +0000, Kieran Bingham wrote:
> > On 21/12/2018 16:25, jacopo mondi wrote:
> >> 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)
> 
> I see, and I feel in this specific case it's not a big deal, but as a
> general rule having tests -in the same commit- that adds a specific
> feature is not something that holds in the general case.
> 
> Tests should accompany new features, and if a patch series adds
> anything that is worth being tested, just make sure they get merged
> together.
> 
> As an example, Niklas' last series is a 12 patches one, with one test
> at the end of the series. Your one might be a 3 patches one, with a
> test at the end. Or am I missing something of how you would like to
> use testing?
> 
> Anyway, that's not a big deal at all in this case :)

I'm also slightly more in favour of separating code and tests in different 
patches. It's indeed not a big deal in this case, but when the patches and 
series grow bigger, bundling everything together creates too large patches.

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

V4L2Device or V4l2Device ? The latter looks weird to me, but capitalizing only 
the first letter of an abbreviation makes sense as a rule (and that's what the 
Google C++ Coding Style mandates).

> >>> +{
> >>> +public:
> >>> +	V4L2Device(const std::string &);
> 
> nit: I've been suggested to use parameter's names in function
> declarations in header files. I know this is very minor stuff, but at
> the very beginning of the development is worth being quite picky on
> this minor detail to establish a consistent code base. So please bear
> with me here a little more :)

I was about to say the same :-) Parameter names make function prototypes more 
explicit.

> >>> +	~V4L2Device();
> >>> +
> >>> +	bool isOpen();

s/;/ const;

> >>> +	int open();
> >>> +	void close();
> >>> +
> >>> +	int queryCap();
> >>> +
> >>> +private:
> >>> +	std::string device_;
> >>> +	int fd_;
> >>> +	int capabilities_;
> 
> Ok, I'm saying it: "What about reverse-xmas-tree order for variable
> declarations?"
> 
> Here, let's the bike-shedding begin

It also makes sense to group them by purpose. I'd take the line size into 
account only when no other criterion applies. In this case the order proposed 
by Kieran makes sense to me.

Please also note that we can't reorganize members freely as they generally 
breaks binary compatibility (at least for classes exposed through the public 
API).

> >>> +};
> >>> +
> >>> +}
> >> 
> >> 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.
> 
> Oh thanks, I missed it was not there, but I think that rule comes from
> the C++ style guide cited in our coding style document.
> 
> >>> +
> >>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */

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

You can mix the five includes above, no need to separate sys/.

> >>> +#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.
> 
> It allows documenting the code in the cpp file where the implementation is,
> but to refer to definitions in the header file. I know, confusing.

To be exact, it tells Doxygen to generate documentation for all classes, 
functions and macros in the referenced file. We thus need a \file statement 
for the header, and no \file statement should be needed for .cpp files as we 
shouldn't define any API there (symbols that are fully private to a .cpp file 
are internal and I don't think they need to appear in the generated 
documentation).

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

s/API //
s/a single instance/an instance/ ?

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

when the function isn't overloaded. But for functions defined in .cpp files 
(as opposed as the inlined functions in the class definition), you can drop 
the \fn line completely, doxygen knows that the comment block is related to 
the function that follows immediately.

> Oh really? That's nice, I did it in all functions and I could have
> avoided that...
> 
> >>> + *
> >>> + * 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.

The default constructor would be V4L2Device::V4L2Device(), which you should = 
delete in the header.

> >>> + */
> >>> +V4L2Device::V4L2Device(const std::string &device)

	: device_(device), fd_(fd), capabilities_(0)

> >>> +{
> >>> +	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.
> 
> I think that would be better
> 
> >>> +}
> >>> +
> >>> +V4L2Device::~V4L2Device()
> >>> +{
> >>> +	close();
> >>> +
> >>> +	LOG(Debug) << "V4L2Device Destroyed";
> >> 
> >> ditto
> >> 
> >>> +}
> >>> +
> >>> +/**
> >>> + * \fn V4L2Device::isOpen(())
> >> 
> >> one () in excess?
> > 
> > Not sure how that happened?

You can drop the \fn line completely anyway (same for the functions below that 
are defined in this file).

> >>> + *
> >>> + * Checks to see if we have successfully opened a v4l2 video device.

\brief Check if the device is open
\sa open()
\return True if the device is open, closed otherwise

> >>> + */
> >>> +bool V4L2Device::isOpen()
> >>> +{
> >>> +	return (fd_ != -1);
> >> 
> >> nit: no need to () braces
> > 
> > Can I keep them ?
> 
> Sure!
> 
> > Returning more than a simple variable or statement 'openly' feels
> > horrible to me ...

You will however have to deal with all the code in libcamera that will make 
you fell horrible, so maybe you'll need to get used to it ;-)

> :)
>
> >>> +}
> >>> +
> >>> +/**
> >>> + * \fn V4L2Device::open()
> >>> + *
> >>> + * Opens a v4l2 device and queries properties from the device.

\brief and \return. Same for the functions below.

> >>> + */
> >>> +int V4L2Device::open()
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	if (isOpen()) {
> >>> +		LOG(Error) << "Device already open";
> >>> +		return -EBADF;

-EBUSY maybe ?

> >>> +	}
> >>> +
> >>> +	fd_ = ::open(device_.c_str(), O_RDWR);
> >>> +	if (fd_ < 0) {

		ret = -errno;

> >>> +		LOG(Error) << "Failed to open V4L2 device " << device_
> >>> +			   << " : " << strerror(errno);
> >>> +		return -errno;

			<< " : " << strerror(-ret);
	return ret;

> >>> +	}
> >>> +
> >>> +	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 ?
> 
> That's how I would interpret
> https://google.github.io/styleguide/cppguide.html#Local_Variables
> 
> It could be discussed though. Again, it is only worth discussing this
> minor things as we're at the beginning and trying to establish a well
> consistent code base

For variables used throughout a function (ret or a i loop counter for 
instance), I think declaring them at the top makes sense.

> >>> +	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?)
> 
> Well, 'queryCap()' would return -1 (or what more appropriate, like the
> errno value)
> 
> > 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)
> 
> Can it return values < 0 ?
> 
> >>> +		LOG(Error) << "Device does not support streaming IO";
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +/**
> >>> + * \fn V4L2Device::close()
> >>> + *
> >>> + * Close a v4l2 device handle.

The file handle isn't exposed as part of the API, so I wouldn't mention it
here. You can just explain that is closes the device, releases any resource 
acquired by open(), and that calling the rest of the API on a closed device 
isn't allowed.

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

I think you should restrict queryCap() to be called only from the open() 
function and make it private. You could then remove this check. Or, possibly, 
even inline this function in open() as it would then be a single ioctl call.

> >>> +	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?
> 
> Returning 'ret' is equivalent to return -1. -errno is the appropriate
> one.
> 
> > 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 ...
> 
> No, I think it is useful. Actually, more than the MediaDevice itself
> (which I expect to instantiate V4L2Devices and not extend it) I think
> it might be worth implementing a V4L2Object that provides protected
> functions such as 'doIOCTL()' or just 'ioctl()' and have V4L2Device
> and V4L2Subdevice sub-class it.

I think it's a bit of an overabstraction :-) I believe there will be very 
little opportunities to share code between V4l2Device, V4l2Subdevice and 
MediaDevice. I recommend developing them as separate objects, and then 
creating a common base (or common helper functions) later if we realize enough 
code is duplicated.

> I'm not sure right now how I would model that, if the base class should
> do things like "setFormat()" or either provides an "ioctl()" function and
> have sub-classes implement the "setFormat()" logic. This boils down to
> how different that would be the implementation between a device and a
> subdevice. What do you think?

The format setting model for V4L2 device and V4L2 subdevices is fundamentally 
different. Subdevices take an extra pad number argument, and they propagate 
formats internally within the subdevice, which creates a set of "subdev 
configuration" objects internally in the kernel (a global one for the active 
parameters and one per file handle for the test parameters). We will have to 
expose this one way or another to implement trying formats, and I expect the 
V4l2Device and V4l2Subdevice objects to offer different APIs for that reason.

> >>> +	}
> >>> +
> >>> +	capabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS
> >>> +		      ? cap.device_caps : cap.capabilities;

How about caching the full v4l2_capability structure in V4L2Device ?

> >>> +	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...)
> 
> Why would you pre-allocate this?
> 
> >>>  ]
> >>> 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

Maybe V4L2DeviceDoubleOpenTest to avoid namespace clashes when we'll compile 
multiple unit tests into a single binary for a test suite ? Or maybe using an 
anynomous namespace for this purpose ?

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

cout/cerr ?

> >>> +			dev->close();
> >>> +			return TEST_FAIL;
> >>> +		}
> >>> +
> >>> +		return TEST_PASS;
> >>> +	}
> > > 
> > > nit: < one indent level
> > 
> > Oh -- for all class functions?
> 
> Oh damn, I got confused by having the full implementation here.
> 
> For 'big' functions like this one, we don't want to have inlined by
> implementing them in the definition. Now, I'm struggling to find a
> final answer to the question "Are functions implemented in the class
> definition automatically inlined?" I bet the compiler is smart enough
> to find out if it makes sense to inline or not a function even when
> its implementation is in the header file, but in general you could define
> the class, and implement the function outside of the definition, even
> if in the same cpp file.
> 
> >>> +};
> >> 
> >> 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...
> 
> Fine then, thanks for the explanation.
> 
> > 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)

> >>> +{
> >>> +	dev = nullptr;
> >>> +}
> >>> +
> >>> +int V4L2DeviceTest::init()
> >>> +{
> >>> +	const char *device = "/dev/video0";

	std::string device("/dev/video0");

> >>> +
> >>> +	/* Validate the device node exists. */
> >>> +	if (!path_exists(device))

Please log a message to explain why the test is skipped.

> >>> +		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 ?
> 
> My point was that the object would not get destroyed here. Because the
> test base class does not call cleanup() after init fails
> and you don't not call it either. But I was confused actually, as the
> base test class does this
> 
> +#define TEST_REGISTER(klass)                                           \
> +int main(int argc, char *argv[])                                       \
> +{                                                                      \
> +       return klass().execute();                                       \
> +}
> 
> And this creates and destroys the object, which calls your destructor,
> so I -guess- this is fine actually.
> Not calling 'cleaup()' after a failed init is being discussed now
> elsewhere.
> 
> Anyway, your constructor cannot fail, so you could remove that
> check...
> 
> > >> +		return TEST_FAIL;
> > >> +
> > >> +	return dev->open();
> > >> +}
> > >> +
> > >> +void V4L2DeviceTest::cleanup()
> > >> +{
> > >> +	if (dev)
> > >> +		delete dev;

Can't you safely delete nullptr ?

> > >> +};
> >>> 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() {};

Is this needed, given that the base class already has a virtual destructor ?

> >>> +
> >>> +protected:
> >>> +	int init();
> >>> +	void cleanup();
> >>> +
> >>> +	virtual int run() = 0;

I don't think this is needed either.

> >>> +	V4L2Device *dev;

s/dev/dev_/

> >>> +};
> >>> +
> >>> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */

-- 
Regards,

Laurent Pinchart