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

[Kieran Bingham]

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

-- 
Regards
--
Kieran