[libcamera-devel,4/6] test: v4l2_videodevice: Add M2M device test

1753 diff mbox series

The V4L2M2MDevice requires two pipelines to be configured. This makes it unsuitable
to reuse the existing V4L2DeviceTest test library in it's current form.

Implement a full test to run the two M2M pipelines through VIM2M.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 test/v4l2_videodevice/meson.build        |   1 +
 test/v4l2_videodevice/v4l2_m2mdevice.cpp | 210 +++++++++++++++++++++++
 2 files changed, 211 insertions(+)
 create mode 100644 test/v4l2_videodevice/v4l2_m2mdevice.cpp

Hi Kieran,

Thank you for the patch.

On Thu, Aug 08, 2019 at 04:12:19PM +0100, Kieran Bingham wrote:
> The V4L2M2MDevice requires two pipelines to be configured. This makes it unsuitable
> to reuse the existing V4L2DeviceTest test library in it's current form.

s/it's/its/

Commit messages should wrap at 72 columns.

> Implement a full test to run the two M2M pipelines through VIM2M.

Lovely, another driver for our test suite :-)

> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  test/v4l2_videodevice/meson.build        |   1 +
>  test/v4l2_videodevice/v4l2_m2mdevice.cpp | 210 +++++++++++++++++++++++
>  2 files changed, 211 insertions(+)
>  create mode 100644 test/v4l2_videodevice/v4l2_m2mdevice.cpp
> 
> diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> index 76be5e142bb6..ad41898b5f8b 100644
> --- a/test/v4l2_videodevice/meson.build
> +++ b/test/v4l2_videodevice/meson.build
> @@ -7,6 +7,7 @@ v4l2_videodevice_tests = [
>      [ 'stream_on_off',      'stream_on_off.cpp' ],
>      [ 'capture_async',      'capture_async.cpp' ],
>      [ 'buffer_sharing',     'buffer_sharing.cpp' ],
> +    [ 'v4l2_m2mdevice',     'v4l2_m2mdevice.cpp' ],
>  ]
>  
>  foreach t : v4l2_videodevice_tests
> diff --git a/test/v4l2_videodevice/v4l2_m2mdevice.cpp b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> new file mode 100644
> index 000000000000..7a730f695ab7
> --- /dev/null
> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> @@ -0,0 +1,210 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * libcamera V4L2 API tests

Copy & paste ?

> + */
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/timer.h>
> +
> +#include <iostream>
> +#include <memory>
> +
> +#include "device_enumerator.h"
> +#include "media_device.h"
> +#include "v4l2_videodevice.h"
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class V4L2M2MDeviceTest : public Test
> +{
> +public:
> +	V4L2M2MDeviceTest()
> +		: vim2m_(nullptr), outputFrames_(0), captureFrames_(0)
> +	{
> +	}
> +
> +	void outputBufferComplete(Buffer *buffer)
> +	{
> +		std::cout << "Received output buffer " << buffer->index()
> +			  << std::endl;

My preference goes with using the std:: prefix explicitly like here, in
which case you should use it everywhere and drop the using namespace std
statement. The alternative is to remove it everywhere.

> +
> +		outputFrames_++;
> +
> +		/* Requeue the buffer for further use. */
> +		vim2m_->output()->queueBuffer(buffer);
> +	}
> +
> +	void receiveCaptureBuffer(Buffer *buffer)
> +	{
> +		std::cout << "Received capture buffer " << buffer->index()
> +			  << std::endl;
> +
> +		captureFrames_++;
> +
> +		/* Requeue the buffer for further use. */
> +		vim2m_->capture()->queueBuffer(buffer);
> +	}
> +
> +protected:
> +	int init()
> +	{
> +		enumerator_ = DeviceEnumerator::create();
> +		if (!enumerator_) {
> +			cerr << "Failed to create device enumerator" << endl;
> +			return TestFail;
> +		}
> +
> +		if (enumerator_->enumerate()) {
> +			cerr << "Failed to enumerate media devices" << endl;
> +			return TestFail;
> +		}
> +
> +		DeviceMatch dm("vim2m");
> +		dm.add("vim2m-source");
> +		dm.add("vim2m-sink");
> +
> +		media_ = enumerator_->search(dm);
> +		if (!media_) {
> +			cerr << "Failed to match device" << endl;

Maybe "No vim2m device found" ?

> +			return TestSkip;
> +		}
> +
> +		MediaEntity *entity = media_->getEntityByName("vim2m-source");
> +		if (!entity) {
> +			cerr << "Failed to get device entity" << endl;

This can't happen due to the dm.add().

> +			return TestSkip;
> +		}
> +

I would move the rest of the code to the run() function as it tests the
V4L2M2MDevice API.

> +		vim2m_ = new V4L2M2MDevice(entity->deviceNode());
> +		if (vim2m_->status())

You should add a message here (and below). It's difficult to debug test
failures when no message is printed.

> +			return TestFail;
> +
> +		V4L2DeviceFormat format = {};
> +		if (vim2m_->capture()->getFormat(&format))
> +			return TestFail;
> +
> +		format.size.width = 640;
> +		format.size.height = 480;
> +
> +		if (vim2m_->capture()->setFormat(&format))
> +			return TestFail;
> +
> +		if (vim2m_->output()->setFormat(&format))
> +			return TestFail;
> +
> +		cerr << "Initialised M2M ..." << endl;

I'd drop this line.

> +
> +		return TestPass;
> +	}
> +
> +	int run()
> +	{
> +		const unsigned int bufferCount = 8;

s/const/constexpr/

Would 4 buffers be enough ?

> +
> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> +		Timer timeout;
> +		int ret;
> +
> +		capturePool_.createBuffers(bufferCount);
> +		outputPool_.createBuffers(bufferCount);
> +
> +		ret = vim2m_->capture()->exportBuffers(&capturePool_);
> +		if (ret) {
> +			cerr << "Failed to export Capture Buffers" << endl;
> +			return TestFail;
> +		}
> +
> +		ret = vim2m_->output()->exportBuffers(&outputPool_);
> +		if (ret) {
> +			cerr << "Failed to export Output Buffers" << endl;
> +			return TestFail;
> +		}

I would store the capture and output devices to local variables to
shorten the lines.

> +
> +		vim2m_->capture()->bufferReady.connect(this, &V4L2M2MDeviceTest::receiveCaptureBuffer);
> +		vim2m_->output()->bufferReady.connect(this, &V4L2M2MDeviceTest::outputBufferComplete);
> +
> +		/* We can't "queueAllBuffers()" on an output device, so we do it manually */
> +		std::vector<std::unique_ptr<Buffer>> outputBuffers;
> +		for (unsigned int i = 0; i < outputPool_.count(); ++i) {
> +			Buffer *buffer = new Buffer(i);
> +			outputBuffers.emplace_back(buffer);
> +			ret = vim2m_->output()->queueBuffer(buffer);
> +			if (ret)
> +				return {};
> +		}
> +
> +		std::vector<std::unique_ptr<Buffer>> captureBuffers;
> +		captureBuffers = vim2m_->capture()->queueAllBuffers();
> +		if (captureBuffers.empty()) {
> +			cerr << "Failed to queue all Capture Buffers" << endl;
> +			return TestFail;
> +		}

Even if it makes little difference in practice, I would queue the
buffers on the capture side first, 

> +
> +		ret = vim2m_->output()->streamOn();
> +		if (ret) {
> +			cerr << "Failed to streamOn output" << endl;
> +			return TestFail;
> +		}
> +
> +		ret = vim2m_->capture()->streamOn();
> +		if (ret) {
> +			cerr << "Failed to streamOn capture" << endl;
> +			return TestFail;
> +		}
> +
> +		timeout.start(10000);
> +		while (timeout.isRunning()) {
> +			dispatcher->processEvents();
> +			if (captureFrames_ > 30)
> +				break;

How long does it take in practice to capture 30 frames ? Can we reduce
the timeout ?

> +		}
> +
> +		if (captureFrames_ < 1) {
> +			std::cout << "Failed to capture any frames within timeout." << std::endl;

s/timeout\./timeout/
Line wrap.

> +			return TestFail;
> +		}
> +
> +		if (captureFrames_ < 30) {
> +			std::cout << "Failed to capture 30 frames within timeout." << std::endl;

Here too.

> +			return TestFail;
> +		}

You could merge the two checks and print the number of captured frames.

> +
> +		std::cout << "Output " << outputFrames_ << " frames" << std::endl;
> +		std::cout << "Captured " << captureFrames_ << " frames" << std::endl;
> +
> +		ret = vim2m_->capture()->streamOff();
> +		if (ret)

Error messages please.

> +			return TestFail;
> +
> +		ret = vim2m_->output()->streamOff();
> +		if (ret)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +
> +	void cleanup()
> +	{
> +		delete vim2m_;
> +	};
> +
> +private:
> +	std::unique_ptr<DeviceEnumerator> enumerator_;
> +	std::shared_ptr<MediaDevice> media_;
> +	V4L2M2MDevice *vim2m_;
> +
> +	BufferPool capturePool_;
> +	BufferPool outputPool_;
> +
> +	unsigned int outputFrames_;
> +	unsigned int captureFrames_;
> +};
> +
> +TEST_REGISTER(V4L2M2MDeviceTest);

On 08/08/2019 22:26, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Aug 08, 2019 at 04:12:19PM +0100, Kieran Bingham wrote:
>> The V4L2M2MDevice requires two pipelines to be configured. This makes it unsuitable
>> to reuse the existing V4L2DeviceTest test library in it's current form.
> 
> s/it's/its/
> 
> Commit messages should wrap at 72 columns.
> 
>> Implement a full test to run the two M2M pipelines through VIM2M.
> 
> Lovely, another driver for our test suite :-)

Indeed! So many software devices to test :D


>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  test/v4l2_videodevice/meson.build        |   1 +
>>  test/v4l2_videodevice/v4l2_m2mdevice.cpp | 210 +++++++++++++++++++++++
>>  2 files changed, 211 insertions(+)
>>  create mode 100644 test/v4l2_videodevice/v4l2_m2mdevice.cpp
>>
>> diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
>> index 76be5e142bb6..ad41898b5f8b 100644
>> --- a/test/v4l2_videodevice/meson.build
>> +++ b/test/v4l2_videodevice/meson.build
>> @@ -7,6 +7,7 @@ v4l2_videodevice_tests = [
>>      [ 'stream_on_off',      'stream_on_off.cpp' ],
>>      [ 'capture_async',      'capture_async.cpp' ],
>>      [ 'buffer_sharing',     'buffer_sharing.cpp' ],
>> +    [ 'v4l2_m2mdevice',     'v4l2_m2mdevice.cpp' ],
>>  ]
>>  
>>  foreach t : v4l2_videodevice_tests
>> diff --git a/test/v4l2_videodevice/v4l2_m2mdevice.cpp b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
>> new file mode 100644
>> index 000000000000..7a730f695ab7
>> --- /dev/null
>> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
>> @@ -0,0 +1,210 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * libcamera V4L2 API tests
> 
> Copy & paste ?

Of course ;D - I'm not writing all this from scratch hehe.



>> + */
>> +
>> +#include <libcamera/buffer.h>
>> +#include <libcamera/camera_manager.h>
>> +#include <libcamera/event_dispatcher.h>
>> +#include <libcamera/timer.h>
>> +
>> +#include <iostream>
>> +#include <memory>
>> +
>> +#include "device_enumerator.h"
>> +#include "media_device.h"
>> +#include "v4l2_videodevice.h"
>> +
>> +#include "test.h"
>> +
>> +using namespace std;
>> +using namespace libcamera;
>> +
>> +class V4L2M2MDeviceTest : public Test
>> +{
>> +public:
>> +	V4L2M2MDeviceTest()
>> +		: vim2m_(nullptr), outputFrames_(0), captureFrames_(0)
>> +	{
>> +	}
>> +
>> +	void outputBufferComplete(Buffer *buffer)
>> +	{
>> +		std::cout << "Received output buffer " << buffer->index()
>> +			  << std::endl;
> 
> My preference goes with using the std:: prefix explicitly like here, in
> which case you should use it everywhere and drop the using namespace std
> statement. The alternative is to remove it everywhere.

I'd prefer shorter lines and removing it.

But really we just need some better test logging.


>> +
>> +		outputFrames_++;
>> +
>> +		/* Requeue the buffer for further use. */
>> +		vim2m_->output()->queueBuffer(buffer);
>> +	}
>> +
>> +	void receiveCaptureBuffer(Buffer *buffer)
>> +	{
>> +		std::cout << "Received capture buffer " << buffer->index()
>> +			  << std::endl;
>> +
>> +		captureFrames_++;
>> +
>> +		/* Requeue the buffer for further use. */
>> +		vim2m_->capture()->queueBuffer(buffer);
>> +	}
>> +
>> +protected:
>> +	int init()
>> +	{
>> +		enumerator_ = DeviceEnumerator::create();
>> +		if (!enumerator_) {
>> +			cerr << "Failed to create device enumerator" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		if (enumerator_->enumerate()) {
>> +			cerr << "Failed to enumerate media devices" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		DeviceMatch dm("vim2m");
>> +		dm.add("vim2m-source");
>> +		dm.add("vim2m-sink");
>> +
>> +		media_ = enumerator_->search(dm);
>> +		if (!media_) {
>> +			cerr << "Failed to match device" << endl;
> 
> Maybe "No vim2m device found" ?
> 

Updated.

>> +			return TestSkip;
>> +		}
>> +
>> +		MediaEntity *entity = media_->getEntityByName("vim2m-source");
>> +		if (!entity) {
>> +			cerr << "Failed to get device entity" << endl;
> 
> This can't happen due to the dm.add().
> 

Removed.

>> +			return TestSkip;
>> +		}
>> +
> 
> I would move the rest of the code to the run() function as it tests the
> V4L2M2MDevice API.

Done.

> 
>> +		vim2m_ = new V4L2M2MDevice(entity->deviceNode());
>> +		if (vim2m_->status())
> 
> You should add a message here (and below). It's difficult to debug test
> failures when no message is printed.


Maybe we should revisit the TestStatus patches I proposed.
Then it would be

			return TestFail("Failed to open VIM2M device")

I recall dropping the series because you didn't seem to like the concept.



>> +			return TestFail;
>> +
>> +		V4L2DeviceFormat format = {};
>> +		if (vim2m_->capture()->getFormat(&format))
>> +			return TestFail;
>> +
>> +		format.size.width = 640;
>> +		format.size.height = 480;
>> +
>> +		if (vim2m_->capture()->setFormat(&format))
>> +			return TestFail;
>> +
>> +		if (vim2m_->output()->setFormat(&format))
>> +			return TestFail;
>> +
>> +		cerr << "Initialised M2M ..." << endl;
> 
> I'd drop this line.

Dropped.

> 
>> +
>> +		return TestPass;
>> +	}
>> +
>> +	int run()
>> +	{
>> +		const unsigned int bufferCount = 8;
> 
> s/const/constexpr/
> 
> Would 4 buffers be enough ?

Sure.

> 
>> +
>> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
>> +		Timer timeout;
>> +		int ret;
>> +
>> +		capturePool_.createBuffers(bufferCount);
>> +		outputPool_.createBuffers(bufferCount);
>> +
>> +		ret = vim2m_->capture()->exportBuffers(&capturePool_);
>> +		if (ret) {
>> +			cerr << "Failed to export Capture Buffers" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		ret = vim2m_->output()->exportBuffers(&outputPool_);
>> +		if (ret) {
>> +			cerr << "Failed to export Output Buffers" << endl;
>> +			return TestFail;
>> +		}
> 
> I would store the capture and output devices to local variables to
> shorten the lines.

Sure.

> 
>> +
>> +		vim2m_->capture()->bufferReady.connect(this, &V4L2M2MDeviceTest::receiveCaptureBuffer);
>> +		vim2m_->output()->bufferReady.connect(this, &V4L2M2MDeviceTest::outputBufferComplete);
>> +
>> +		/* We can't "queueAllBuffers()" on an output device, so we do it manually */
>> +		std::vector<std::unique_ptr<Buffer>> outputBuffers;
>> +		for (unsigned int i = 0; i < outputPool_.count(); ++i) {
>> +			Buffer *buffer = new Buffer(i);
>> +			outputBuffers.emplace_back(buffer);
>> +			ret = vim2m_->output()->queueBuffer(buffer);
>> +			if (ret)
>> +				return {};
>> +		}
>> +
>> +		std::vector<std::unique_ptr<Buffer>> captureBuffers;
>> +		captureBuffers = vim2m_->capture()->queueAllBuffers();
>> +		if (captureBuffers.empty()) {
>> +			cerr << "Failed to queue all Capture Buffers" << endl;
>> +			return TestFail;
>> +		}
> 
> Even if it makes little difference in practice, I would queue the
> buffers on the capture side first, 
> 
>> +
>> +		ret = vim2m_->output()->streamOn();
>> +		if (ret) {
>> +			cerr << "Failed to streamOn output" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		ret = vim2m_->capture()->streamOn();
>> +		if (ret) {
>> +			cerr << "Failed to streamOn capture" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		timeout.start(10000);
>> +		while (timeout.isRunning()) {
>> +			dispatcher->processEvents();
>> +			if (captureFrames_ > 30)
>> +				break;
> 
> How long does it take in practice to capture 30 frames ? Can we reduce
> the timeout ?

On my laptop:

27/37 libcamera:v4l2_videodevice / v4l2_m2mdevice  OK       1.47 s


On a RaspberryPi 3:

27/37 libcamera:v4l2_videodevice / v4l2_m2mdevice  OK       1.64 s


I'll drop to 5 seconds timeout. It's only going to happen in the event
of a pipeline stall which is unlikely.



>> +		}
>> +
>> +		if (captureFrames_ < 1) {
>> +			std::cout << "Failed to capture any frames within timeout." << std::endl;
> 
> s/timeout\./timeout/
> Line wrap.
> 
>> +			return TestFail;
>> +		}
>> +
>> +		if (captureFrames_ < 30) {
>> +			std::cout << "Failed to capture 30 frames within timeout." << std::endl;
> 
> Here too.
> 
>> +			return TestFail;
>> +		}
> 
> You could merge the two checks and print the number of captured frames.

Done.


> 
>> +
>> +		std::cout << "Output " << outputFrames_ << " frames" << std::endl;
>> +		std::cout << "Captured " << captureFrames_ << " frames" << std::endl;
>> +
>> +		ret = vim2m_->capture()->streamOff();
>> +		if (ret)
> 
> Error messages please.

Done


> 
>> +			return TestFail;
>> +
>> +		ret = vim2m_->output()->streamOff();
>> +		if (ret)
>> +			return TestFail;
>> +
>> +		return TestPass;
>> +	}
>> +
>> +	void cleanup()
>> +	{
>> +		delete vim2m_;
>> +	};
>> +
>> +private:
>> +	std::unique_ptr<DeviceEnumerator> enumerator_;
>> +	std::shared_ptr<MediaDevice> media_;
>> +	V4L2M2MDevice *vim2m_;
>> +
>> +	BufferPool capturePool_;
>> +	BufferPool outputPool_;
>> +
>> +	unsigned int outputFrames_;
>> +	unsigned int captureFrames_;
>> +};
>> +
>> +TEST_REGISTER(V4L2M2MDeviceTest);
>

Hi Kieran,

On Thu, Aug 08, 2019 at 04:12:19PM +0100, Kieran Bingham wrote:
> The V4L2M2MDevice requires two pipelines to be configured. This makes it unsuitable
> to reuse the existing V4L2DeviceTest test library in it's current form.
>
> Implement a full test to run the two M2M pipelines through VIM2M.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  test/v4l2_videodevice/meson.build        |   1 +
>  test/v4l2_videodevice/v4l2_m2mdevice.cpp | 210 +++++++++++++++++++++++
>  2 files changed, 211 insertions(+)
>  create mode 100644 test/v4l2_videodevice/v4l2_m2mdevice.cpp
>
> diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> index 76be5e142bb6..ad41898b5f8b 100644
> --- a/test/v4l2_videodevice/meson.build
> +++ b/test/v4l2_videodevice/meson.build
> @@ -7,6 +7,7 @@ v4l2_videodevice_tests = [
>      [ 'stream_on_off',      'stream_on_off.cpp' ],
>      [ 'capture_async',      'capture_async.cpp' ],
>      [ 'buffer_sharing',     'buffer_sharing.cpp' ],
> +    [ 'v4l2_m2mdevice',     'v4l2_m2mdevice.cpp' ],
>  ]
>
>  foreach t : v4l2_videodevice_tests
> diff --git a/test/v4l2_videodevice/v4l2_m2mdevice.cpp b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> new file mode 100644
> index 000000000000..7a730f695ab7
> --- /dev/null
> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> @@ -0,0 +1,210 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * libcamera V4L2 API tests
> + */
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/timer.h>
> +
> +#include <iostream>
> +#include <memory>

C++ includes before library includes

> +
> +#include "device_enumerator.h"
> +#include "media_device.h"
> +#include "v4l2_videodevice.h"
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class V4L2M2MDeviceTest : public Test
> +{
> +public:
> +	V4L2M2MDeviceTest()
> +		: vim2m_(nullptr), outputFrames_(0), captureFrames_(0)
> +	{
> +	}
> +
> +	void outputBufferComplete(Buffer *buffer)
> +	{
> +		std::cout << "Received output buffer " << buffer->index()
> +			  << std::endl;
> +
> +		outputFrames_++;
> +
> +		/* Requeue the buffer for further use. */
> +		vim2m_->output()->queueBuffer(buffer);
> +	}
> +
> +	void receiveCaptureBuffer(Buffer *buffer)
> +	{
> +		std::cout << "Received capture buffer " << buffer->index()
> +			  << std::endl;
> +
> +		captureFrames_++;
> +
> +		/* Requeue the buffer for further use. */
> +		vim2m_->capture()->queueBuffer(buffer);
> +	}
> +
> +protected:
> +	int init()
> +	{
> +		enumerator_ = DeviceEnumerator::create();
> +		if (!enumerator_) {
> +			cerr << "Failed to create device enumerator" << endl;
> +			return TestFail;
> +		}
> +
> +		if (enumerator_->enumerate()) {
> +			cerr << "Failed to enumerate media devices" << endl;
> +			return TestFail;
> +		}
> +
> +		DeviceMatch dm("vim2m");
> +		dm.add("vim2m-source");
> +		dm.add("vim2m-sink");
> +
> +		media_ = enumerator_->search(dm);
> +		if (!media_) {
> +			cerr << "Failed to match device" << endl;
> +			return TestSkip;
> +		}
> +
> +		MediaEntity *entity = media_->getEntityByName("vim2m-source");
> +		if (!entity) {
> +			cerr << "Failed to get device entity" << endl;
> +			return TestSkip;
> +		}
> +
> +		vim2m_ = new V4L2M2MDevice(entity->deviceNode());
> +		if (vim2m_->status())
> +			return TestFail;

Do we want a static function M2MFromEntityName like we have for
devices and subdevices?

> +
> +		V4L2DeviceFormat format = {};
> +		if (vim2m_->capture()->getFormat(&format))
> +			return TestFail;
> +
> +		format.size.width = 640;
> +		format.size.height = 480;
> +
> +		if (vim2m_->capture()->setFormat(&format))
> +			return TestFail;
> +
> +		if (vim2m_->output()->setFormat(&format))
> +			return TestFail;

Does setFormat deserves a single helper as open()/close() are?

> +
> +		cerr << "Initialised M2M ..." << endl;

s/...//
Not sure it makes a difference, but that should be cout.

Also, as Laurent said, what about keeping the std namespace explicit ?

> +
> +		return TestPass;
> +	}
> +
> +	int run()
> +	{
> +		const unsigned int bufferCount = 8;
> +
> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> +		Timer timeout;
> +		int ret;

Since you don't initialize it, could you declare them at the time
they're used?

> +
> +		capturePool_.createBuffers(bufferCount);
> +		outputPool_.createBuffers(bufferCount);
> +
> +		ret = vim2m_->capture()->exportBuffers(&capturePool_);
> +		if (ret) {
> +			cerr << "Failed to export Capture Buffers" << endl;
> +			return TestFail;
> +		}
> +
> +		ret = vim2m_->output()->exportBuffers(&outputPool_);
> +		if (ret) {
> +			cerr << "Failed to export Output Buffers" << endl;
> +			return TestFail;

Should we delete the pools ?

> +		}
> +
> +		vim2m_->capture()->bufferReady.connect(this, &V4L2M2MDeviceTest::receiveCaptureBuffer);
> +		vim2m_->output()->bufferReady.connect(this, &V4L2M2MDeviceTest::outputBufferComplete);
> +
> +		/* We can't "queueAllBuffers()" on an output device, so we do it manually */
> +		std::vector<std::unique_ptr<Buffer>> outputBuffers;
> +		for (unsigned int i = 0; i < outputPool_.count(); ++i) {
> +			Buffer *buffer = new Buffer(i);
> +			outputBuffers.emplace_back(buffer);
> +			ret = vim2m_->output()->queueBuffer(buffer);
> +			if (ret)
> +				return {};

return TestFail ? And maybe an error message for simmetry with the
below one?

> +		}
> +
> +		std::vector<std::unique_ptr<Buffer>> captureBuffers;
> +		captureBuffers = vim2m_->capture()->queueAllBuffers();
> +		if (captureBuffers.empty()) {
> +			cerr << "Failed to queue all Capture Buffers" << endl;
> +			return TestFail;
> +		}
> +
> +		ret = vim2m_->output()->streamOn();
> +		if (ret) {
> +			cerr << "Failed to streamOn output" << endl;
> +			return TestFail;
> +		}
> +
> +		ret = vim2m_->capture()->streamOn();
> +		if (ret) {
> +			cerr << "Failed to streamOn capture" << endl;
> +			return TestFail;
> +		}
> +
> +		timeout.start(10000);
> +		while (timeout.isRunning()) {
> +			dispatcher->processEvents();
> +			if (captureFrames_ > 30)
> +				break;
> +		}
> +
> +		if (captureFrames_ < 1) {
> +			std::cout << "Failed to capture any frames within timeout." << std::endl;
> +			return TestFail;
> +		}
> +
> +		if (captureFrames_ < 30) {
> +			std::cout << "Failed to capture 30 frames within timeout." << std::endl;
> +			return TestFail;
> +		}

Over 80 columns, I know you're not too concerned about that ;)

> +
> +		std::cout << "Output " << outputFrames_ << " frames" << std::endl;
> +		std::cout << "Captured " << captureFrames_ << " frames" << std::endl;
> +
> +		ret = vim2m_->capture()->streamOff();
> +		if (ret)
> +			return TestFail;
> +
> +		ret = vim2m_->output()->streamOff();
> +		if (ret)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +
> +	void cleanup()
> +	{
> +		delete vim2m_;
> +	};
> +
> +private:
> +	std::unique_ptr<DeviceEnumerator> enumerator_;
> +	std::shared_ptr<MediaDevice> media_;
> +	V4L2M2MDevice *vim2m_;
> +
> +	BufferPool capturePool_;
> +	BufferPool outputPool_;
> +
> +	unsigned int outputFrames_;
> +	unsigned int captureFrames_;
> +};
> +
> +TEST_REGISTER(V4L2M2MDeviceTest);
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thanks for the review.

I've picked up a couple of fixups and applied them ready for v2.


On 09/08/2019 15:30, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Aug 08, 2019 at 04:12:19PM +0100, Kieran Bingham wrote:
>> The V4L2M2MDevice requires two pipelines to be configured. This makes it unsuitable
>> to reuse the existing V4L2DeviceTest test library in it's current form.
>>
>> Implement a full test to run the two M2M pipelines through VIM2M.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  test/v4l2_videodevice/meson.build        |   1 +
>>  test/v4l2_videodevice/v4l2_m2mdevice.cpp | 210 +++++++++++++++++++++++
>>  2 files changed, 211 insertions(+)
>>  create mode 100644 test/v4l2_videodevice/v4l2_m2mdevice.cpp
>>
>> diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
>> index 76be5e142bb6..ad41898b5f8b 100644
>> --- a/test/v4l2_videodevice/meson.build
>> +++ b/test/v4l2_videodevice/meson.build
>> @@ -7,6 +7,7 @@ v4l2_videodevice_tests = [
>>      [ 'stream_on_off',      'stream_on_off.cpp' ],
>>      [ 'capture_async',      'capture_async.cpp' ],
>>      [ 'buffer_sharing',     'buffer_sharing.cpp' ],
>> +    [ 'v4l2_m2mdevice',     'v4l2_m2mdevice.cpp' ],
>>  ]
>>
>>  foreach t : v4l2_videodevice_tests
>> diff --git a/test/v4l2_videodevice/v4l2_m2mdevice.cpp b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
>> new file mode 100644
>> index 000000000000..7a730f695ab7
>> --- /dev/null
>> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
>> @@ -0,0 +1,210 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * libcamera V4L2 API tests
>> + */
>> +
>> +#include <libcamera/buffer.h>
>> +#include <libcamera/camera_manager.h>
>> +#include <libcamera/event_dispatcher.h>
>> +#include <libcamera/timer.h>
>> +
>> +#include <iostream>
>> +#include <memory>
> 
> C++ includes before library includes

Moved, and dropped <memory> I don't think it's used.


> 
>> +
>> +#include "device_enumerator.h"
>> +#include "media_device.h"
>> +#include "v4l2_videodevice.h"
>> +
>> +#include "test.h"
>> +
>> +using namespace std;
>> +using namespace libcamera;
>> +
>> +class V4L2M2MDeviceTest : public Test
>> +{
>> +public:
>> +	V4L2M2MDeviceTest()
>> +		: vim2m_(nullptr), outputFrames_(0), captureFrames_(0)
>> +	{
>> +	}
>> +
>> +	void outputBufferComplete(Buffer *buffer)
>> +	{
>> +		std::cout << "Received output buffer " << buffer->index()
>> +			  << std::endl;
>> +
>> +		outputFrames_++;
>> +
>> +		/* Requeue the buffer for further use. */
>> +		vim2m_->output()->queueBuffer(buffer);
>> +	}
>> +
>> +	void receiveCaptureBuffer(Buffer *buffer)
>> +	{
>> +		std::cout << "Received capture buffer " << buffer->index()
>> +			  << std::endl;
>> +
>> +		captureFrames_++;
>> +
>> +		/* Requeue the buffer for further use. */
>> +		vim2m_->capture()->queueBuffer(buffer);
>> +	}
>> +
>> +protected:
>> +	int init()
>> +	{
>> +		enumerator_ = DeviceEnumerator::create();
>> +		if (!enumerator_) {
>> +			cerr << "Failed to create device enumerator" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		if (enumerator_->enumerate()) {
>> +			cerr << "Failed to enumerate media devices" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		DeviceMatch dm("vim2m");
>> +		dm.add("vim2m-source");
>> +		dm.add("vim2m-sink");
>> +
>> +		media_ = enumerator_->search(dm);
>> +		if (!media_) {
>> +			cerr << "Failed to match device" << endl;
>> +			return TestSkip;
>> +		}
>> +
>> +		MediaEntity *entity = media_->getEntityByName("vim2m-source");
>> +		if (!entity) {
>> +			cerr << "Failed to get device entity" << endl;
>> +			return TestSkip;
>> +		}
>> +
>> +		vim2m_ = new V4L2M2MDevice(entity->deviceNode());
>> +		if (vim2m_->status())
>> +			return TestFail;
> 
> Do we want a static function M2MFromEntityName like we have for
> devices and subdevices?

I think this looks clean, but we can add the helper later if you think
it's useful.


>> +
>> +		V4L2DeviceFormat format = {};
>> +		if (vim2m_->capture()->getFormat(&format))
>> +			return TestFail;
>> +
>> +		format.size.width = 640;
>> +		format.size.height = 480;
>> +
>> +		if (vim2m_->capture()->setFormat(&format))
>> +			return TestFail;
>> +
>> +		if (vim2m_->output()->setFormat(&format))
>> +			return TestFail;
> 
> Does setFormat deserves a single helper as open()/close() are?

No - We can set different formats on the two pipes.

It's just this test where I set the same format on both for simplicity.
I don't want to test the conversion capabilities of vim2m - just the
buffer flow.


> 
>> +
>> +		cerr << "Initialised M2M ..." << endl;
> 
> s/...//
> Not sure it makes a difference, but that should be cout.
> 
> Also, as Laurent said, what about keeping the std namespace explicit ?

It was there to help me when it wasn't running. I've removed it now.


> 
>> +
>> +		return TestPass;
>> +	}
>> +
>> +	int run()
>> +	{
>> +		const unsigned int bufferCount = 8;
>> +
>> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
>> +		Timer timeout;
>> +		int ret;
> 
> Since you don't initialize it, could you declare them at the time
> they're used?
> 

The ret is used by multiple statements, so I'd rather keep that separate.

I've moved the Timer declaration to the usage.



>> +
>> +		capturePool_.createBuffers(bufferCount);
>> +		outputPool_.createBuffers(bufferCount);
>> +
>> +		ret = vim2m_->capture()->exportBuffers(&capturePool_);
>> +		if (ret) {
>> +			cerr << "Failed to export Capture Buffers" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		ret = vim2m_->output()->exportBuffers(&outputPool_);
>> +		if (ret) {
>> +			cerr << "Failed to export Output Buffers" << endl;
>> +			return TestFail;
> 
> Should we delete the pools ?

The pools are class members. Their destructors will run when the class
is destroyed.


> 
>> +		}
>> +
>> +		vim2m_->capture()->bufferReady.connect(this, &V4L2M2MDeviceTest::receiveCaptureBuffer);
>> +		vim2m_->output()->bufferReady.connect(this, &V4L2M2MDeviceTest::outputBufferComplete);
>> +
>> +		/* We can't "queueAllBuffers()" on an output device, so we do it manually */
>> +		std::vector<std::unique_ptr<Buffer>> outputBuffers;
>> +		for (unsigned int i = 0; i < outputPool_.count(); ++i) {
>> +			Buffer *buffer = new Buffer(i);
>> +			outputBuffers.emplace_back(buffer);
>> +			ret = vim2m_->output()->queueBuffer(buffer);
>> +			if (ret)
>> +				return {};
> 
> return TestFail ? And maybe an error message for simmetry with the
> below one?

Yes, I've fixed this one already.



> 
>> +		}
>> +
>> +		std::vector<std::unique_ptr<Buffer>> captureBuffers;
>> +		captureBuffers = vim2m_->capture()->queueAllBuffers();
>> +		if (captureBuffers.empty()) {
>> +			cerr << "Failed to queue all Capture Buffers" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		ret = vim2m_->output()->streamOn();
>> +		if (ret) {
>> +			cerr << "Failed to streamOn output" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		ret = vim2m_->capture()->streamOn();
>> +		if (ret) {
>> +			cerr << "Failed to streamOn capture" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		timeout.start(10000);
>> +		while (timeout.isRunning()) {
>> +			dispatcher->processEvents();
>> +			if (captureFrames_ > 30)
>> +				break;
>> +		}
>> +
>> +		if (captureFrames_ < 1) {
>> +			std::cout << "Failed to capture any frames within timeout." << std::endl;
>> +			return TestFail;
>> +		}
>> +
>> +		if (captureFrames_ < 30) {
>> +			std::cout << "Failed to capture 30 frames within timeout." << std::endl;
>> +			return TestFail;
>> +		}
> 
> Over 80 columns, I know you're not too concerned about that ;)

For debug lines in test cases : no - but those lines have already been
deleted for the next version.

> 
>> +
>> +		std::cout << "Output " << outputFrames_ << " frames" << std::endl;
>> +		std::cout << "Captured " << captureFrames_ << " frames" << std::endl;
>> +
>> +		ret = vim2m_->capture()->streamOff();
>> +		if (ret)
>> +			return TestFail;
>> +
>> +		ret = vim2m_->output()->streamOff();
>> +		if (ret)
>> +			return TestFail;
>> +
>> +		return TestPass;
>> +	}
>> +
>> +	void cleanup()
>> +	{
>> +		delete vim2m_;
>> +	};
>> +
>> +private:
>> +	std::unique_ptr<DeviceEnumerator> enumerator_;
>> +	std::shared_ptr<MediaDevice> media_;
>> +	V4L2M2MDevice *vim2m_;
>> +
>> +	BufferPool capturePool_;
>> +	BufferPool outputPool_;
>> +
>> +	unsigned int outputFrames_;
>> +	unsigned int captureFrames_;
>> +};
>> +
>> +TEST_REGISTER(V4L2M2MDeviceTest);
>> --
>> 2.20.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Kieran,

On Fri, Aug 09, 2019 at 11:32:15AM +0100, Kieran Bingham wrote:
> On 08/08/2019 22:26, Laurent Pinchart wrote:
> > On Thu, Aug 08, 2019 at 04:12:19PM +0100, Kieran Bingham wrote:
> >> The V4L2M2MDevice requires two pipelines to be configured. This makes it unsuitable
> >> to reuse the existing V4L2DeviceTest test library in it's current form.
> > 
> > s/it's/its/
> > 
> > Commit messages should wrap at 72 columns.
> > 
> >> Implement a full test to run the two M2M pipelines through VIM2M.
> > 
> > Lovely, another driver for our test suite :-)
> 
> Indeed! So many software devices to test :D
> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  test/v4l2_videodevice/meson.build        |   1 +
> >>  test/v4l2_videodevice/v4l2_m2mdevice.cpp | 210 +++++++++++++++++++++++
> >>  2 files changed, 211 insertions(+)
> >>  create mode 100644 test/v4l2_videodevice/v4l2_m2mdevice.cpp
> >>
> >> diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> >> index 76be5e142bb6..ad41898b5f8b 100644
> >> --- a/test/v4l2_videodevice/meson.build
> >> +++ b/test/v4l2_videodevice/meson.build
> >> @@ -7,6 +7,7 @@ v4l2_videodevice_tests = [
> >>      [ 'stream_on_off',      'stream_on_off.cpp' ],
> >>      [ 'capture_async',      'capture_async.cpp' ],
> >>      [ 'buffer_sharing',     'buffer_sharing.cpp' ],
> >> +    [ 'v4l2_m2mdevice',     'v4l2_m2mdevice.cpp' ],
> >>  ]
> >>  
> >>  foreach t : v4l2_videodevice_tests
> >> diff --git a/test/v4l2_videodevice/v4l2_m2mdevice.cpp b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> >> new file mode 100644
> >> index 000000000000..7a730f695ab7
> >> --- /dev/null
> >> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> >> @@ -0,0 +1,210 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * libcamera V4L2 API tests
> > 
> > Copy & paste ?
> 
> Of course ;D - I'm not writing all this from scratch hehe.
> 
> >> + */
> >> +
> >> +#include <libcamera/buffer.h>
> >> +#include <libcamera/camera_manager.h>
> >> +#include <libcamera/event_dispatcher.h>
> >> +#include <libcamera/timer.h>
> >> +
> >> +#include <iostream>
> >> +#include <memory>
> >> +
> >> +#include "device_enumerator.h"
> >> +#include "media_device.h"
> >> +#include "v4l2_videodevice.h"
> >> +
> >> +#include "test.h"
> >> +
> >> +using namespace std;
> >> +using namespace libcamera;
> >> +
> >> +class V4L2M2MDeviceTest : public Test
> >> +{
> >> +public:
> >> +	V4L2M2MDeviceTest()
> >> +		: vim2m_(nullptr), outputFrames_(0), captureFrames_(0)
> >> +	{
> >> +	}
> >> +
> >> +	void outputBufferComplete(Buffer *buffer)
> >> +	{
> >> +		std::cout << "Received output buffer " << buffer->index()
> >> +			  << std::endl;
> > 
> > My preference goes with using the std:: prefix explicitly like here, in
> > which case you should use it everywhere and drop the using namespace std
> > statement. The alternative is to remove it everywhere.
> 
> I'd prefer shorter lines and removing it.
> 
> But really we just need some better test logging.
> 
> >> +
> >> +		outputFrames_++;
> >> +
> >> +		/* Requeue the buffer for further use. */
> >> +		vim2m_->output()->queueBuffer(buffer);
> >> +	}
> >> +
> >> +	void receiveCaptureBuffer(Buffer *buffer)
> >> +	{
> >> +		std::cout << "Received capture buffer " << buffer->index()
> >> +			  << std::endl;
> >> +
> >> +		captureFrames_++;
> >> +
> >> +		/* Requeue the buffer for further use. */
> >> +		vim2m_->capture()->queueBuffer(buffer);
> >> +	}
> >> +
> >> +protected:
> >> +	int init()
> >> +	{
> >> +		enumerator_ = DeviceEnumerator::create();
> >> +		if (!enumerator_) {
> >> +			cerr << "Failed to create device enumerator" << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		if (enumerator_->enumerate()) {
> >> +			cerr << "Failed to enumerate media devices" << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		DeviceMatch dm("vim2m");
> >> +		dm.add("vim2m-source");
> >> +		dm.add("vim2m-sink");
> >> +
> >> +		media_ = enumerator_->search(dm);
> >> +		if (!media_) {
> >> +			cerr << "Failed to match device" << endl;
> > 
> > Maybe "No vim2m device found" ?
> 
> Updated.
> 
> >> +			return TestSkip;
> >> +		}
> >> +
> >> +		MediaEntity *entity = media_->getEntityByName("vim2m-source");
> >> +		if (!entity) {
> >> +			cerr << "Failed to get device entity" << endl;
> > 
> > This can't happen due to the dm.add().
> 
> Removed.
> 
> >> +			return TestSkip;
> >> +		}
> >> +
> > 
> > I would move the rest of the code to the run() function as it tests the
> > V4L2M2MDevice API.
> 
> Done.
> 
> >> +		vim2m_ = new V4L2M2MDevice(entity->deviceNode());
> >> +		if (vim2m_->status())
> > 
> > You should add a message here (and below). It's difficult to debug test
> > failures when no message is printed.
> 
> Maybe we should revisit the TestStatus patches I proposed.
> Then it would be
> 
> 			return TestFail("Failed to open VIM2M device")
> 
> I recall dropping the series because you didn't seem to like the concept.

I think it's a good idea, but I also think it should be part of a
logging infrastructure for tests. We shouldn't design the two parts
separately.

> >> +			return TestFail;
> >> +
> >> +		V4L2DeviceFormat format = {};
> >> +		if (vim2m_->capture()->getFormat(&format))
> >> +			return TestFail;
> >> +
> >> +		format.size.width = 640;
> >> +		format.size.height = 480;
> >> +
> >> +		if (vim2m_->capture()->setFormat(&format))
> >> +			return TestFail;
> >> +
> >> +		if (vim2m_->output()->setFormat(&format))
> >> +			return TestFail;
> >> +
> >> +		cerr << "Initialised M2M ..." << endl;
> > 
> > I'd drop this line.
> 
> Dropped.
> 
> >> +
> >> +		return TestPass;
> >> +	}
> >> +
> >> +	int run()
> >> +	{
> >> +		const unsigned int bufferCount = 8;
> > 
> > s/const/constexpr/
> > 
> > Would 4 buffers be enough ?
> 
> Sure.
> 
> >> +
> >> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> >> +		Timer timeout;
> >> +		int ret;
> >> +
> >> +		capturePool_.createBuffers(bufferCount);
> >> +		outputPool_.createBuffers(bufferCount);
> >> +
> >> +		ret = vim2m_->capture()->exportBuffers(&capturePool_);
> >> +		if (ret) {
> >> +			cerr << "Failed to export Capture Buffers" << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		ret = vim2m_->output()->exportBuffers(&outputPool_);
> >> +		if (ret) {
> >> +			cerr << "Failed to export Output Buffers" << endl;
> >> +			return TestFail;
> >> +		}
> > 
> > I would store the capture and output devices to local variables to
> > shorten the lines.
> 
> Sure.
> 
> >> +
> >> +		vim2m_->capture()->bufferReady.connect(this, &V4L2M2MDeviceTest::receiveCaptureBuffer);
> >> +		vim2m_->output()->bufferReady.connect(this, &V4L2M2MDeviceTest::outputBufferComplete);
> >> +
> >> +		/* We can't "queueAllBuffers()" on an output device, so we do it manually */
> >> +		std::vector<std::unique_ptr<Buffer>> outputBuffers;
> >> +		for (unsigned int i = 0; i < outputPool_.count(); ++i) {
> >> +			Buffer *buffer = new Buffer(i);
> >> +			outputBuffers.emplace_back(buffer);
> >> +			ret = vim2m_->output()->queueBuffer(buffer);
> >> +			if (ret)
> >> +				return {};
> >> +		}
> >> +
> >> +		std::vector<std::unique_ptr<Buffer>> captureBuffers;
> >> +		captureBuffers = vim2m_->capture()->queueAllBuffers();
> >> +		if (captureBuffers.empty()) {
> >> +			cerr << "Failed to queue all Capture Buffers" << endl;
> >> +			return TestFail;
> >> +		}
> > 
> > Even if it makes little difference in practice, I would queue the
> > buffers on the capture side first, 
> > 
> >> +
> >> +		ret = vim2m_->output()->streamOn();
> >> +		if (ret) {
> >> +			cerr << "Failed to streamOn output" << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		ret = vim2m_->capture()->streamOn();
> >> +		if (ret) {
> >> +			cerr << "Failed to streamOn capture" << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		timeout.start(10000);
> >> +		while (timeout.isRunning()) {
> >> +			dispatcher->processEvents();
> >> +			if (captureFrames_ > 30)
> >> +				break;
> > 
> > How long does it take in practice to capture 30 frames ? Can we reduce
> > the timeout ?
> 
> On my laptop:
> 
> 27/37 libcamera:v4l2_videodevice / v4l2_m2mdevice  OK       1.47 s
> 
> On a RaspberryPi 3:
> 
> 27/37 libcamera:v4l2_videodevice / v4l2_m2mdevice  OK       1.64 s
> 
> I'll drop to 5 seconds timeout. It's only going to happen in the event
> of a pipeline stall which is unlikely.

Seems good to me.

> >> +		}
> >> +
> >> +		if (captureFrames_ < 1) {
> >> +			std::cout << "Failed to capture any frames within timeout." << std::endl;
> > 
> > s/timeout\./timeout/
> > Line wrap.
> > 
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		if (captureFrames_ < 30) {
> >> +			std::cout << "Failed to capture 30 frames within timeout." << std::endl;
> > 
> > Here too.
> > 
> >> +			return TestFail;
> >> +		}
> > 
> > You could merge the two checks and print the number of captured frames.
> 
> Done.
> 
> >> +
> >> +		std::cout << "Output " << outputFrames_ << " frames" << std::endl;
> >> +		std::cout << "Captured " << captureFrames_ << " frames" << std::endl;
> >> +
> >> +		ret = vim2m_->capture()->streamOff();
> >> +		if (ret)
> > 
> > Error messages please.
> 
> Done
> 
> >> +			return TestFail;
> >> +
> >> +		ret = vim2m_->output()->streamOff();
> >> +		if (ret)
> >> +			return TestFail;
> >> +
> >> +		return TestPass;
> >> +	}
> >> +
> >> +	void cleanup()
> >> +	{
> >> +		delete vim2m_;
> >> +	};
> >> +
> >> +private:
> >> +	std::unique_ptr<DeviceEnumerator> enumerator_;
> >> +	std::shared_ptr<MediaDevice> media_;
> >> +	V4L2M2MDevice *vim2m_;
> >> +
> >> +	BufferPool capturePool_;
> >> +	BufferPool outputPool_;
> >> +
> >> +	unsigned int outputFrames_;
> >> +	unsigned int captureFrames_;
> >> +};
> >> +
> >> +TEST_REGISTER(V4L2M2MDeviceTest);