[libcamera-devel,v2,8/8] test: v4l2_device: Provide buffer sharing test

577 diff mbox series

Obtain two V4L2Devices and use one to obtain a BufferPool.

Propagate the formats from the first to the second device and then commence
sending buffers between the two devices in a ping-pong fashion.

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

Hi Kieran,

Thank you for the patch.

On Wed, Feb 13, 2019 at 03:10:27PM +0000, Kieran Bingham wrote:
> Obtain two V4L2Devices and use one to obtain a BufferPool.
> 
> Propagate the formats from the first to the second device and then commence
> sending buffers between the two devices in a ping-pong fashion.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  test/v4l2_device/buffer_sharing.cpp | 179 ++++++++++++++++++++++++++++
>  test/v4l2_device/meson.build        |   1 +
>  2 files changed, 180 insertions(+)
>  create mode 100644 test/v4l2_device/buffer_sharing.cpp
> 
> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp
> new file mode 100644
> index 000000000000..f03201e82084
> --- /dev/null
> +++ b/test/v4l2_device/buffer_sharing.cpp
> @@ -0,0 +1,179 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * libcamera V4L2 API tests
> + *
> + * Validate the function of exporting buffers from a V4L2Device and
> + * the ability to import them to another V4L2Device instance.
> + * Ensure that the Buffers can successfully be queued and dequeued
> + * between both devices.
> + */
> +
> +#include <iostream>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/timer.h>
> +
> +#include "v4l2_device_test.h"
> +
> +#include "log.h"
> +
> +LOG_DEFINE_CATEGORY(Test)

Can you use std::cout until we implement a test logger ?

> +class BufferSharingTest : public V4L2DeviceTest
> +{
> +public:
> +	BufferSharingTest()
> +		: output_(nullptr), framesCapture(0), framesOutput(0){};

	BufferSharingTest()
		: V4L2DeviceTest(), output_(nullptr), framesCapture(0),
		  framesOutput(0)
	{
	}

> +
> +private:
> +	const unsigned int bufferCount = 4;
> +
> +	V4L2Device *output_;
> +	std::shared_ptr<MediaDevice> secondMedia_;

This can be removed.

> +
> +	unsigned int framesCapture;

framesCaptured_ ?

> +	unsigned int framesOutput;

framesOutput_ ?

> +
> +protected:
> +	int init()
> +	{
> +		int ret = V4L2DeviceTest::init();
> +		if (ret)
> +			return ret;
> +
> +		/* media_ already represents VIVID */
> +		MediaEntity *entity = media_->getEntityByName("vivid-000-vid-out");
> +		if (!entity)
> +			return TestSkip;
> +
> +		output_ = new V4L2Device(entity);
> +		if (!output_)
> +			return TestFail;
> +
> +		ret = output_->open();
> +		if (ret)
> +			return TestFail;
> +
> +		V4L2DeviceFormat format;

You should either initialise format to all 0 here (format = {}) or do so
in V4L2Device::getFormat().

> +
> +		ret = dev_->getFormat(&format);
> +		if (ret) {
> +			return TestFail;
> +		}

No need for braces. Or rather please keep them and log a message
explaining the cause of the error. Same for the other TestFail above and
below.

> +
> +		LOG(Test, Info) << "Successfully obtained format from source";

If we log failures I think you can remove this and the other LOG()
instance below.

> +		ret = output_->setFormat(&format);
> +		if (ret)
> +			return TestFail;
> +
> +		LOG(Test, Info) << "Successfully set format to output";
> +
> +		pool_.createBuffers(bufferCount);
> +
> +		ret = dev_->exportBuffers(&pool_);

Should we rename dev_ to capture_ in the base class ?

> +		if (ret)
> +			return TestFail;
> +
> +		ret = output_->importBuffers(&pool_);
> +		if (ret) {
> +			std::cerr << "Failed to import buffers" << std::endl;
> +			return TestFail;
> +		}
> +
> +		return 0;
> +	}
> +
> +	void receiveSourceBuffer(Buffer *buffer)

How about captureBufferReady() ?

> +	{
> +		std::cout << "Received source buffer: " << buffer->index()
> +			  << " sequence " << buffer->sequence() << std::endl;
> +
> +		output_->queueBuffer(buffer);
> +		framesCapture++;
> +	}
> +
> +	void receiveDestinationBuffer(Buffer *buffer)

And outputBufferReady() ?

Source and destination could be a bit confusing. I'd rework the messages
accordingly.

> +	{
> +		std::cout << "Received destination buffer: " << buffer->index()
> +			  << " sequence " << buffer->sequence() << std::endl;
> +
> +		dev_->queueBuffer(buffer);
> +		framesOutput++;
> +	}
> +
> +	int run()
> +	{
> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> +		Timer timeout;
> +		int ret;
> +
> +		dev_->bufferReady.connect(this, &BufferSharingTest::receiveSourceBuffer);
> +		output_->bufferReady.connect(this, &BufferSharingTest::receiveDestinationBuffer);
> +
> +		/* Queue all the buffers to the device. */

... to the capture device.

> +		for (Buffer &b : pool_.buffers()) {

I'd spell buffer in full.

> +			if (dev_->queueBuffer(&b))
> +				return TestFail;
> +		}
> +
> +		ret = dev_->streamOn();
> +		if (ret)
> +			return TestFail;
> +
> +		ret = output_->streamOn();
> +		if (ret)
> +			return TestFail;
> +
> +		timeout.start(10000);
> +		while (timeout.isRunning()) {
> +			dispatcher->processEvents();
> +			if (framesCapture > 30 && framesOutput > 30)
> +				break;
> +		}
> +
> +		if ((framesCapture < 1) || (framesOutput < 1)) {
> +			std::cout << "Failed to process any frames within timeout." << std::endl;
> +			return TestFail;
> +		}
> +
> +		if ((framesCapture < 30) || (framesOutput < 30)) {
> +			std::cout << "Failed to process 30 frames within timeout." << std::endl;
> +			return TestFail;
> +		}
> +
> +		ret = dev_->streamOff();
> +		if (ret)
> +			return TestFail;
> +
> +		ret = output_->streamOff();
> +		if (ret)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +
> +	void cleanup()
> +	{
> +		std::cout
> +			<< "Processed " << framesCapture << " capture frames"
> +			<< " and " << framesOutput << " output frames"
> +			<< std::endl;

Maybe "Captured ... frames and output ... frames" ?

> +
> +		dev_->streamOff();
> +		output_->streamOff();
> +
> +		if (secondMedia_)
> +			secondMedia_->release();

You should free buffers on both devices (for the capture device likely
in the base class).

> +		delete output_;
> +
> +		V4L2DeviceTest::cleanup();
> +	}
> +};
> +
> +TEST_REGISTER(BufferSharingTest);
> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build
> index ec2c7f9f11ff..9f7a7545ac9b 100644
> --- a/test/v4l2_device/meson.build
> +++ b/test/v4l2_device/meson.build
> @@ -5,6 +5,7 @@ v4l2_device_tests = [
>    [ 'request_buffers',    'request_buffers.cpp' ],
>    [ 'stream_on_off',      'stream_on_off.cpp' ],
>    [ 'capture_async',      'capture_async.cpp' ],
> +  [ 'buffer_sharing',     'buffer_sharing.cpp' ],
>  ]
>  
>  foreach t : v4l2_device_tests

On 13/02/2019 16:01, Laurent Pinchart wrote:
> Can you use std::cout until we implement a test logger ?

Argh - yes - sorry - I was meant to have changed that already.

On 13/02/2019 16:01, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Feb 13, 2019 at 03:10:27PM +0000, Kieran Bingham wrote:
>> Obtain two V4L2Devices and use one to obtain a BufferPool.
>>
>> Propagate the formats from the first to the second device and then commence
>> sending buffers between the two devices in a ping-pong fashion.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  test/v4l2_device/buffer_sharing.cpp | 179 ++++++++++++++++++++++++++++
>>  test/v4l2_device/meson.build        |   1 +
>>  2 files changed, 180 insertions(+)
>>  create mode 100644 test/v4l2_device/buffer_sharing.cpp
>>
>> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp
>> new file mode 100644
>> index 000000000000..f03201e82084
>> --- /dev/null
>> +++ b/test/v4l2_device/buffer_sharing.cpp
>> @@ -0,0 +1,179 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * libcamera V4L2 API tests
>> + *
>> + * Validate the function of exporting buffers from a V4L2Device and
>> + * the ability to import them to another V4L2Device instance.
>> + * Ensure that the Buffers can successfully be queued and dequeued
>> + * between both devices.
>> + */
>> +
>> +#include <iostream>
>> +
>> +#include <libcamera/buffer.h>
>> +#include <libcamera/camera_manager.h>
>> +#include <libcamera/event_dispatcher.h>
>> +#include <libcamera/timer.h>
>> +
>> +#include "v4l2_device_test.h"
>> +
>> +#include "log.h"
>> +
>> +LOG_DEFINE_CATEGORY(Test)
> 
> Can you use std::cout until we implement a test logger ?

#include "log.h" removed :) (thus all LOGs)

> 
>> +class BufferSharingTest : public V4L2DeviceTest
>> +{
>> +public:
>> +	BufferSharingTest()
>> +		: output_(nullptr), framesCapture(0), framesOutput(0){};
> 
> 	BufferSharingTest()
> 		: V4L2DeviceTest(), output_(nullptr), framesCapture(0),
> 		  framesOutput(0)
> 	{
> 	}
> 
>> +
>> +private:
>> +	const unsigned int bufferCount = 4;
>> +
>> +	V4L2Device *output_;
>> +	std::shared_ptr<MediaDevice> secondMedia_;
> 
> This can be removed.
> 
>> +
>> +	unsigned int framesCapture;
> 
> framesCaptured_ ?

Sure.

> 
>> +	unsigned int framesOutput;
> 
> framesOutput_ ?
> 

Done.


>> +
>> +protected:
>> +	int init()
>> +	{
>> +		int ret = V4L2DeviceTest::init();
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* media_ already represents VIVID */
>> +		MediaEntity *entity = media_->getEntityByName("vivid-000-vid-out");
>> +		if (!entity)
>> +			return TestSkip;
>> +
>> +		output_ = new V4L2Device(entity);
>> +		if (!output_)
>> +			return TestFail;
>> +
>> +		ret = output_->open();
>> +		if (ret)
>> +			return TestFail;
>> +
>> +		V4L2DeviceFormat format;
> 
> You should either initialise format to all 0 here (format = {}) or do so
> in V4L2Device::getFormat().

Really? format is a class? Shouldn't it's constructor do that?
Hrm ... it has no constructor :(



>> +
>> +		ret = dev_->getFormat(&format);
>> +		if (ret) {
>> +			return TestFail;
>> +		}
> 
> No need for braces. Or rather please keep them and log a message
> explaining the cause of the error. Same for the other TestFail above and
> below.
> 
>> +
>> +		LOG(Test, Info) << "Successfully obtained format from source";
> 
> If we log failures I think you can remove this and the other LOG()
> instance below.
> 
>> +		ret = output_->setFormat(&format);
>> +		if (ret)
>> +			return TestFail;
>> +
>> +		LOG(Test, Info) << "Successfully set format to output";
>> +
>> +		pool_.createBuffers(bufferCount);
>> +
>> +		ret = dev_->exportBuffers(&pool_);
> 
> Should we rename dev_ to capture_ in the base class ?

That seems reasonable now.

Lets handle that separately though.


> 
>> +		if (ret)
>> +			return TestFail;
>> +
>> +		ret = output_->importBuffers(&pool_);
>> +		if (ret) {
>> +			std::cerr << "Failed to import buffers" << std::endl;
>> +			return TestFail;
>> +		}
>> +
>> +		return 0;
>> +	}
>> +
>> +	void receiveSourceBuffer(Buffer *buffer)
> 
> How about captureBufferReady() ?
> 
>> +	{
>> +		std::cout << "Received source buffer: " << buffer->index()
>> +			  << " sequence " << buffer->sequence() << std::endl;
>> +
>> +		output_->queueBuffer(buffer);
>> +		framesCapture++;
>> +	}
>> +
>> +	void receiveDestinationBuffer(Buffer *buffer)
> 
> And outputBufferReady() ?
> 
> Source and destination could be a bit confusing. I'd rework the messages
> accordingly.

Done

> 
>> +	{
>> +		std::cout << "Received destination buffer: " << buffer->index()
>> +			  << " sequence " << buffer->sequence() << std::endl;
>> +
>> +		dev_->queueBuffer(buffer);
>> +		framesOutput++;
>> +	}
>> +
>> +	int run()
>> +	{
>> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
>> +		Timer timeout;
>> +		int ret;
>> +
>> +		dev_->bufferReady.connect(this, &BufferSharingTest::receiveSourceBuffer);
>> +		output_->bufferReady.connect(this, &BufferSharingTest::receiveDestinationBuffer);
>> +
>> +		/* Queue all the buffers to the device. */
> 
> ... to the capture device.

done

> 
>> +		for (Buffer &b : pool_.buffers()) {
> 
> I'd spell buffer in full.

done

> 
>> +			if (dev_->queueBuffer(&b))
>> +				return TestFail;
>> +		}
>> +
>> +		ret = dev_->streamOn();
>> +		if (ret)
>> +			return TestFail;
>> +
>> +		ret = output_->streamOn();
>> +		if (ret)
>> +			return TestFail;
>> +
>> +		timeout.start(10000);
>> +		while (timeout.isRunning()) {
>> +			dispatcher->processEvents();
>> +			if (framesCapture > 30 && framesOutput > 30)
>> +				break;
>> +		}
>> +
>> +		if ((framesCapture < 1) || (framesOutput < 1)) {
>> +			std::cout << "Failed to process any frames within timeout." << std::endl;
>> +			return TestFail;
>> +		}
>> +
>> +		if ((framesCapture < 30) || (framesOutput < 30)) {
>> +			std::cout << "Failed to process 30 frames within timeout." << std::endl;
>> +			return TestFail;
>> +		}
>> +
>> +		ret = dev_->streamOff();
>> +		if (ret)
>> +			return TestFail;
>> +
>> +		ret = output_->streamOff();
>> +		if (ret)
>> +			return TestFail;
>> +
>> +		return TestPass;
>> +	}
>> +
>> +	void cleanup()
>> +	{
>> +		std::cout
>> +			<< "Processed " << framesCapture << " capture frames"
>> +			<< " and " << framesOutput << " output frames"
>> +			<< std::endl;
> 
> Maybe "Captured ... frames and output ... frames" ?

Sure.


> 
>> +
>> +		dev_->streamOff();
>> +		output_->streamOff();
>> +
>> +		if (secondMedia_)
>> +			secondMedia_->release();
> 
> You should free buffers on both devices (for the capture device likely
> in the base class).
Would you object to the destructor doing these things ? (as well?) as a
catch all?


>> +		delete output_;
>> +
>> +		V4L2DeviceTest::cleanup();
>> +	}
>> +};
>> +
>> +TEST_REGISTER(BufferSharingTest);
>> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build
>> index ec2c7f9f11ff..9f7a7545ac9b 100644
>> --- a/test/v4l2_device/meson.build
>> +++ b/test/v4l2_device/meson.build
>> @@ -5,6 +5,7 @@ v4l2_device_tests = [
>>    [ 'request_buffers',    'request_buffers.cpp' ],
>>    [ 'stream_on_off',      'stream_on_off.cpp' ],
>>    [ 'capture_async',      'capture_async.cpp' ],
>> +  [ 'buffer_sharing',     'buffer_sharing.cpp' ],
>>  ]
>>  
>>  foreach t : v4l2_device_tests
>

Hi Laurent,

On 13/02/2019 16:01, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Feb 13, 2019 at 03:10:27PM +0000, Kieran Bingham wrote:
>> Obtain two V4L2Devices and use one to obtain a BufferPool.
>>
>> Propagate the formats from the first to the second device and then commence
>> sending buffers between the two devices in a ping-pong fashion.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  test/v4l2_device/buffer_sharing.cpp | 179 ++++++++++++++++++++++++++++
>>  test/v4l2_device/meson.build        |   1 +
>>  2 files changed, 180 insertions(+)
>>  create mode 100644 test/v4l2_device/buffer_sharing.cpp
>>
>> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp
>> new file mode 100644
>> index 000000000000..f03201e82084
>> --- /dev/null
>> +++ b/test/v4l2_device/buffer_sharing.cpp
>> @@ -0,0 +1,179 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * libcamera V4L2 API tests
>> + *
>> + * Validate the function of exporting buffers from a V4L2Device and
>> + * the ability to import them to another V4L2Device instance.
>> + * Ensure that the Buffers can successfully be queued and dequeued
>> + * between both devices.
>> + */
>> +
>> +#include <iostream>
>> +
>> +#include <libcamera/buffer.h>
>> +#include <libcamera/camera_manager.h>
>> +#include <libcamera/event_dispatcher.h>
>> +#include <libcamera/timer.h>
>> +
>> +#include "v4l2_device_test.h"
>> +
>> +#include "log.h"
>> +
>> +LOG_DEFINE_CATEGORY(Test)
> 
> Can you use std::cout until we implement a test logger ?
> 
>> +class BufferSharingTest : public V4L2DeviceTest
>> +{
>> +public:
>> +	BufferSharingTest()
>> +		: output_(nullptr), framesCapture(0), framesOutput(0){};
> 
> 	BufferSharingTest()
> 		: V4L2DeviceTest(), output_(nullptr), framesCapture(0),
> 		  framesOutput(0)
> 	{
> 	}


Checkstyle complains at this change, and prefers to have the {} inline.

--- test/v4l2_device/buffer_sharing.cpp
+++ test/v4l2_device/buffer_sharing.cpp
@@ -23,9 +23,7 @@
 {
 public:
        BufferSharingTest()
-               : output_(nullptr), framesCaptured_(0), framesOutput_(0)
-       {
-       };
+               : output_(nullptr), framesCaptured_(0), framesOutput_(0){};


Would you like to override checkstyle here?

<snip> ...

Hi Kieran,

On Wed, Feb 13, 2019 at 04:19:08PM +0000, Kieran Bingham wrote:
> On 13/02/2019 16:01, Laurent Pinchart wrote:
> > On Wed, Feb 13, 2019 at 03:10:27PM +0000, Kieran Bingham wrote:
> >> Obtain two V4L2Devices and use one to obtain a BufferPool.
> >>
> >> Propagate the formats from the first to the second device and then commence
> >> sending buffers between the two devices in a ping-pong fashion.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  test/v4l2_device/buffer_sharing.cpp | 179 ++++++++++++++++++++++++++++
> >>  test/v4l2_device/meson.build        |   1 +
> >>  2 files changed, 180 insertions(+)
> >>  create mode 100644 test/v4l2_device/buffer_sharing.cpp
> >>
> >> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp
> >> new file mode 100644
> >> index 000000000000..f03201e82084
> >> --- /dev/null
> >> +++ b/test/v4l2_device/buffer_sharing.cpp
> >> @@ -0,0 +1,179 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * libcamera V4L2 API tests
> >> + *
> >> + * Validate the function of exporting buffers from a V4L2Device and
> >> + * the ability to import them to another V4L2Device instance.
> >> + * Ensure that the Buffers can successfully be queued and dequeued
> >> + * between both devices.
> >> + */
> >> +
> >> +#include <iostream>
> >> +
> >> +#include <libcamera/buffer.h>
> >> +#include <libcamera/camera_manager.h>
> >> +#include <libcamera/event_dispatcher.h>
> >> +#include <libcamera/timer.h>
> >> +
> >> +#include "v4l2_device_test.h"
> >> +
> >> +#include "log.h"
> >> +
> >> +LOG_DEFINE_CATEGORY(Test)
> > 
> > Can you use std::cout until we implement a test logger ?
> 
> #include "log.h" removed :) (thus all LOGs)
> 
> >> +class BufferSharingTest : public V4L2DeviceTest
> >> +{
> >> +public:
> >> +	BufferSharingTest()
> >> +		: output_(nullptr), framesCapture(0), framesOutput(0){};
> > 
> > 	BufferSharingTest()
> > 		: V4L2DeviceTest(), output_(nullptr), framesCapture(0),
> > 		  framesOutput(0)
> > 	{
> > 	}
> > 
> >> +
> >> +private:
> >> +	const unsigned int bufferCount = 4;
> >> +
> >> +	V4L2Device *output_;
> >> +	std::shared_ptr<MediaDevice> secondMedia_;
> > 
> > This can be removed.
> > 
> >> +
> >> +	unsigned int framesCapture;
> > 
> > framesCaptured_ ?
> 
> Sure.
> 
> >> +	unsigned int framesOutput;
> > 
> > framesOutput_ ?
> 
> Done.
> 
> >> +
> >> +protected:
> >> +	int init()
> >> +	{
> >> +		int ret = V4L2DeviceTest::init();
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		/* media_ already represents VIVID */
> >> +		MediaEntity *entity = media_->getEntityByName("vivid-000-vid-out");
> >> +		if (!entity)
> >> +			return TestSkip;
> >> +
> >> +		output_ = new V4L2Device(entity);
> >> +		if (!output_)
> >> +			return TestFail;
> >> +
> >> +		ret = output_->open();
> >> +		if (ret)
> >> +			return TestFail;
> >> +
> >> +		V4L2DeviceFormat format;
> > 
> > You should either initialise format to all 0 here (format = {}) or do so
> > in V4L2Device::getFormat().
> 
> Really? format is a class? Shouldn't it's constructor do that?
> Hrm ... it has no constructor :(

Maybe we should turn it into a struct.

> >> +
> >> +		ret = dev_->getFormat(&format);
> >> +		if (ret) {
> >> +			return TestFail;
> >> +		}
> > 
> > No need for braces. Or rather please keep them and log a message
> > explaining the cause of the error. Same for the other TestFail above and
> > below.
> > 
> >> +
> >> +		LOG(Test, Info) << "Successfully obtained format from source";
> > 
> > If we log failures I think you can remove this and the other LOG()
> > instance below.
> > 
> >> +		ret = output_->setFormat(&format);
> >> +		if (ret)
> >> +			return TestFail;
> >> +
> >> +		LOG(Test, Info) << "Successfully set format to output";
> >> +
> >> +		pool_.createBuffers(bufferCount);
> >> +
> >> +		ret = dev_->exportBuffers(&pool_);
> > 
> > Should we rename dev_ to capture_ in the base class ?
> 
> That seems reasonable now.
> 
> Lets handle that separately though.
> 
> >> +		if (ret)
> >> +			return TestFail;
> >> +
> >> +		ret = output_->importBuffers(&pool_);
> >> +		if (ret) {
> >> +			std::cerr << "Failed to import buffers" << std::endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		return 0;
> >> +	}
> >> +
> >> +	void receiveSourceBuffer(Buffer *buffer)
> > 
> > How about captureBufferReady() ?
> > 
> >> +	{
> >> +		std::cout << "Received source buffer: " << buffer->index()
> >> +			  << " sequence " << buffer->sequence() << std::endl;
> >> +
> >> +		output_->queueBuffer(buffer);
> >> +		framesCapture++;
> >> +	}
> >> +
> >> +	void receiveDestinationBuffer(Buffer *buffer)
> > 
> > And outputBufferReady() ?
> > 
> > Source and destination could be a bit confusing. I'd rework the messages
> > accordingly.
> 
> Done
> 
> >> +	{
> >> +		std::cout << "Received destination buffer: " << buffer->index()
> >> +			  << " sequence " << buffer->sequence() << std::endl;
> >> +
> >> +		dev_->queueBuffer(buffer);
> >> +		framesOutput++;
> >> +	}
> >> +
> >> +	int run()
> >> +	{
> >> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> >> +		Timer timeout;
> >> +		int ret;
> >> +
> >> +		dev_->bufferReady.connect(this, &BufferSharingTest::receiveSourceBuffer);
> >> +		output_->bufferReady.connect(this, &BufferSharingTest::receiveDestinationBuffer);
> >> +
> >> +		/* Queue all the buffers to the device. */
> > 
> > ... to the capture device.
> 
> done
> 
> >> +		for (Buffer &b : pool_.buffers()) {
> > 
> > I'd spell buffer in full.
> 
> done
> 
> >> +			if (dev_->queueBuffer(&b))
> >> +				return TestFail;
> >> +		}
> >> +
> >> +		ret = dev_->streamOn();
> >> +		if (ret)
> >> +			return TestFail;
> >> +
> >> +		ret = output_->streamOn();
> >> +		if (ret)
> >> +			return TestFail;
> >> +
> >> +		timeout.start(10000);
> >> +		while (timeout.isRunning()) {
> >> +			dispatcher->processEvents();
> >> +			if (framesCapture > 30 && framesOutput > 30)
> >> +				break;
> >> +		}
> >> +
> >> +		if ((framesCapture < 1) || (framesOutput < 1)) {
> >> +			std::cout << "Failed to process any frames within timeout." << std::endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		if ((framesCapture < 30) || (framesOutput < 30)) {
> >> +			std::cout << "Failed to process 30 frames within timeout." << std::endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		ret = dev_->streamOff();
> >> +		if (ret)
> >> +			return TestFail;
> >> +
> >> +		ret = output_->streamOff();
> >> +		if (ret)
> >> +			return TestFail;
> >> +
> >> +		return TestPass;
> >> +	}
> >> +
> >> +	void cleanup()
> >> +	{
> >> +		std::cout
> >> +			<< "Processed " << framesCapture << " capture frames"
> >> +			<< " and " << framesOutput << " output frames"
> >> +			<< std::endl;
> > 
> > Maybe "Captured ... frames and output ... frames" ?
> 
> Sure.
> 
> >> +
> >> +		dev_->streamOff();
> >> +		output_->streamOff();
> >> +
> >> +		if (secondMedia_)
> >> +			secondMedia_->release();
> > 
> > You should free buffers on both devices (for the capture device likely
> > in the base class).
> 
> Would you object to the destructor doing these things ? (as well?) as a
> catch all?

No objection, as long as we clean up properly and log errors
appropriately. It's important to test the cleanup paths too.

> >> +		delete output_;
> >> +
> >> +		V4L2DeviceTest::cleanup();
> >> +	}
> >> +};
> >> +
> >> +TEST_REGISTER(BufferSharingTest);
> >> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build
> >> index ec2c7f9f11ff..9f7a7545ac9b 100644
> >> --- a/test/v4l2_device/meson.build
> >> +++ b/test/v4l2_device/meson.build
> >> @@ -5,6 +5,7 @@ v4l2_device_tests = [
> >>    [ 'request_buffers',    'request_buffers.cpp' ],
> >>    [ 'stream_on_off',      'stream_on_off.cpp' ],
> >>    [ 'capture_async',      'capture_async.cpp' ],
> >> +  [ 'buffer_sharing',     'buffer_sharing.cpp' ],
> >>  ]
> >>  
> >>  foreach t : v4l2_device_tests

Hi Kieran,

On Wed, Feb 13, 2019 at 04:23:07PM +0000, Kieran Bingham wrote:
> On 13/02/2019 16:01, Laurent Pinchart wrote:
> > On Wed, Feb 13, 2019 at 03:10:27PM +0000, Kieran Bingham wrote:
> >> Obtain two V4L2Devices and use one to obtain a BufferPool.
> >>
> >> Propagate the formats from the first to the second device and then commence
> >> sending buffers between the two devices in a ping-pong fashion.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  test/v4l2_device/buffer_sharing.cpp | 179 ++++++++++++++++++++++++++++
> >>  test/v4l2_device/meson.build        |   1 +
> >>  2 files changed, 180 insertions(+)
> >>  create mode 100644 test/v4l2_device/buffer_sharing.cpp
> >>
> >> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp
> >> new file mode 100644
> >> index 000000000000..f03201e82084
> >> --- /dev/null
> >> +++ b/test/v4l2_device/buffer_sharing.cpp
> >> @@ -0,0 +1,179 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * libcamera V4L2 API tests
> >> + *
> >> + * Validate the function of exporting buffers from a V4L2Device and
> >> + * the ability to import them to another V4L2Device instance.
> >> + * Ensure that the Buffers can successfully be queued and dequeued
> >> + * between both devices.
> >> + */
> >> +
> >> +#include <iostream>
> >> +
> >> +#include <libcamera/buffer.h>
> >> +#include <libcamera/camera_manager.h>
> >> +#include <libcamera/event_dispatcher.h>
> >> +#include <libcamera/timer.h>
> >> +
> >> +#include "v4l2_device_test.h"
> >> +
> >> +#include "log.h"
> >> +
> >> +LOG_DEFINE_CATEGORY(Test)
> > 
> > Can you use std::cout until we implement a test logger ?
> > 
> >> +class BufferSharingTest : public V4L2DeviceTest
> >> +{
> >> +public:
> >> +	BufferSharingTest()
> >> +		: output_(nullptr), framesCapture(0), framesOutput(0){};
> > 
> > 	BufferSharingTest()
> > 		: V4L2DeviceTest(), output_(nullptr), framesCapture(0),
> > 		  framesOutput(0)
> > 	{
> > 	}
> 
> 
> Checkstyle complains at this change, and prefers to have the {} inline.
> 
> --- test/v4l2_device/buffer_sharing.cpp
> +++ test/v4l2_device/buffer_sharing.cpp
> @@ -23,9 +23,7 @@
>  {
>  public:
>         BufferSharingTest()
> -               : output_(nullptr), framesCaptured_(0), framesOutput_(0)
> -       {
> -       };
> +               : output_(nullptr), framesCaptured_(0), framesOutput_(0){};
> 
> 
> Would you like to override checkstyle here?

The trailing ; is really not needed, and we need at least a space before
{}. When we have a list of initialisers I prefer splitting { and } to
separate lines for readability. If clang-format can do that through an
option it would be best. Otherwise we can override checkstyle here, or
keep {} inline (provided we add the space and remove the ;) if you
prefer (it's your code after all :-)).

Heya,

On 13/02/2019 16:54, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wed, Feb 13, 2019 at 04:23:07PM +0000, Kieran Bingham wrote:
>> On 13/02/2019 16:01, Laurent Pinchart wrote:
>>> On Wed, Feb 13, 2019 at 03:10:27PM +0000, Kieran Bingham wrote:
>>>> Obtain two V4L2Devices and use one to obtain a BufferPool.
>>>>
>>>> Propagate the formats from the first to the second device and then commence
>>>> sending buffers between the two devices in a ping-pong fashion.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> ---
>>>>  test/v4l2_device/buffer_sharing.cpp | 179 ++++++++++++++++++++++++++++
>>>>  test/v4l2_device/meson.build        |   1 +
>>>>  2 files changed, 180 insertions(+)
>>>>  create mode 100644 test/v4l2_device/buffer_sharing.cpp
>>>>
>>>> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp
>>>> new file mode 100644
>>>> index 000000000000..f03201e82084
>>>> --- /dev/null
>>>> +++ b/test/v4l2_device/buffer_sharing.cpp
>>>> @@ -0,0 +1,179 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * Copyright (C) 2019, Google Inc.
>>>> + *
>>>> + * libcamera V4L2 API tests
>>>> + *
>>>> + * Validate the function of exporting buffers from a V4L2Device and
>>>> + * the ability to import them to another V4L2Device instance.
>>>> + * Ensure that the Buffers can successfully be queued and dequeued
>>>> + * between both devices.
>>>> + */
>>>> +
>>>> +#include <iostream>
>>>> +
>>>> +#include <libcamera/buffer.h>
>>>> +#include <libcamera/camera_manager.h>
>>>> +#include <libcamera/event_dispatcher.h>
>>>> +#include <libcamera/timer.h>
>>>> +
>>>> +#include "v4l2_device_test.h"
>>>> +
>>>> +#include "log.h"
>>>> +
>>>> +LOG_DEFINE_CATEGORY(Test)
>>>
>>> Can you use std::cout until we implement a test logger ?
>>>
>>>> +class BufferSharingTest : public V4L2DeviceTest
>>>> +{
>>>> +public:
>>>> +	BufferSharingTest()
>>>> +		: output_(nullptr), framesCapture(0), framesOutput(0){};
>>>
>>> 	BufferSharingTest()
>>> 		: V4L2DeviceTest(), output_(nullptr), framesCapture(0),
>>> 		  framesOutput(0)
>>> 	{
>>> 	}
>>
>>
>> Checkstyle complains at this change, and prefers to have the {} inline.
>>
>> --- test/v4l2_device/buffer_sharing.cpp
>> +++ test/v4l2_device/buffer_sharing.cpp
>> @@ -23,9 +23,7 @@
>>  {
>>  public:
>>         BufferSharingTest()
>> -               : output_(nullptr), framesCaptured_(0), framesOutput_(0)
>> -       {
>> -       };
>> +               : output_(nullptr), framesCaptured_(0), framesOutput_(0){};
>>
>>
>> Would you like to override checkstyle here?
> 
> The trailing ; is really not needed, and we need at least a space before
> {}. When we have a list of initialisers I prefer splitting { and } to
> separate lines for readability. If clang-format can do that through an
> option it would be best. Otherwise we can override checkstyle here, or
> keep {} inline (provided we add the space and remove the ;) if you
> prefer (it's your code after all :-)).

I don't really care as long as checkstyle is clean.

Removing the ; satisfies clang-format in putting the { } on new lines,
so I've done that.