[libcamera-devel,2/2] test: v4l2_videodevice: Add test for V4L2BufferCache

2837 diff mbox series

Add test to test the different modes and situations the V4L2BufferCache
can be put to. The tests verify that a FrameBuffer used with the cache
results in a V4L2 video device index can be resolved, and that the cache
implementation is capable of keeping buffers in a hot state.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 test/v4l2_videodevice/buffer_cache.cpp | 286 +++++++++++++++++++++++++
 test/v4l2_videodevice/meson.build      |   1 +
 2 files changed, 287 insertions(+)
 create mode 100644 test/v4l2_videodevice/buffer_cache.cpp

Hi Niklas,

Thank you for the patch.

On Sun, Feb 16, 2020 at 05:16:42PM +0100, Niklas Söderlund wrote:
> Add test to test the different modes and situations the V4L2BufferCache
> can be put to. The tests verify that a FrameBuffer used with the cache

s/put to/put in/ ?

> results in a V4L2 video device index can be resolved, and that the cache

s/can be resolved/that can be resolved/ ? Or did you mean something else
? I'm not sure what you mean by resolved here to be honest.

> implementation is capable of keeping buffers in a hot state.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Could you move this before patch 1/2 to see that it fails, and that the
fix actually fixes the problem ?

> ---
>  test/v4l2_videodevice/buffer_cache.cpp | 286 +++++++++++++++++++++++++
>  test/v4l2_videodevice/meson.build      |   1 +
>  2 files changed, 287 insertions(+)
>  create mode 100644 test/v4l2_videodevice/buffer_cache.cpp
> 
> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> new file mode 100644
> index 0000000000000000..6244361130525d29
> --- /dev/null
> +++ b/test/v4l2_videodevice/buffer_cache.cpp
> @@ -0,0 +1,286 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * Test the buffer cache different operation modes
> + */
> +
> +#include <iostream>
> +#include <random>
> +#include <vector>
> +
> +#include <libcamera/stream.h>
> +
> +#include "device_enumerator.h"
> +#include "media_device.h"
> +#include "v4l2_videodevice.h"
> +
> +#include "test.h"
> +
> +using namespace libcamera;
> +
> +namespace {
> +
> +/* A provider of source of external buffers, suitable for to use by V4L2BufferCache. */

Line wrap ?

A provider or a source ?

s/for to use/for use/

> +class BufferSource
> +{
> +public:
> +	BufferSource()
> +		: video_(nullptr)
> +	{
> +	}
> +
> +	~BufferSource()
> +	{
> +		if (video_) {
> +			video_->releaseBuffers();
> +			video_->close();
> +		}
> +
> +		delete video_;
> +		video_ = nullptr;
> +
> +		if (media_)
> +			media_->release();
> +	}
> +
> +	int allocate(const StreamConfiguration &config)
> +	{
> +		/* Locate and open the video device. */
> +		std::string videoDeviceName = "vivid-000-vid-out";
> +
> +		std::unique_ptr<DeviceEnumerator> enumerator =
> +			DeviceEnumerator::create();
> +		if (!enumerator) {
> +			std::cout << "Failed to create device enumerator" << std::endl;
> +			return TestFail;
> +		}
> +
> +		if (enumerator->enumerate()) {
> +			std::cout << "Failed to enumerate media devices" << std::endl;
> +			return TestFail;
> +		}
> +
> +		DeviceMatch dm("vivid");
> +		dm.add(videoDeviceName);
> +
> +		media_ = enumerator->search(dm);
> +		if (!media_) {
> +			std::cout << "No vivid output device available" << std::endl;
> +			return TestSkip;
> +		}
> +
> +		video_ = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);
> +		if (!video_) {
> +			std::cout << "Unable to open " << videoDeviceName << std::endl;

I think this error message is for the open() failure.

> +			return TestFail;
> +		}
> +
> +		if (video_->open())
> +			return TestFail;
> +
> +		/* Configure the format. */
> +		V4L2DeviceFormat format;
> +		if (video_->getFormat(&format)) {
> +			std::cout << "Failed to get format on output device" << std::endl;
> +			return TestFail;
> +		}
> +
> +		format.size = config.size;
> +		format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false);
> +		if (video_->setFormat(&format)) {
> +			std::cout << "Failed to set format on output device" << std::endl;
> +			return TestFail;
> +		}
> +
> +		if (video_->exportBuffers(config.bufferCount, &buffers_) < 0) {
> +			std::cout << "Failed to export buffers" << std::endl;
> +			return TestFail;
> +		}

We clearly need a better way to get hold of dmabufs from the kernel :-)

> +
> +		return TestPass;
> +	}
> +
> +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers()
> +	{
> +		return buffers_;
> +	}
> +
> +private:
> +	std::shared_ptr<MediaDevice> media_;
> +	V4L2VideoDevice *video_;
> +	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> +};
> +
> +class BufferCacheTest : public Test
> +{
> +public:
> +	/*
> +	 * Test that a cache with the same size as there are buffers results in
> +	 * a sequential run over; 0, 1, 2, 3, 4, 0, 1, 2, 3, 4, ...
> +	 *
> +	 * Test only valid when the cache size is as least as big as there are

s/Test/The test is/

You won't be charged per word.

> +	 * number of buffers.

"... as the number of buffers."

> +	 */
> +	int testSequential(V4L2BufferCache *cache,
> +			   const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> +	{
> +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> +			int nBuffer = i % buffers.size();
> +			int index = cache->get(*buffers[nBuffer].get());
> +
> +			if (index != nBuffer) {
> +				std::cout << "Expected index " << nBuffer
> +					  << " got " << index << std::endl;
> +				return TestFail;
> +			}
> +
> +			cache->put(index);
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	/*
> +	 * Test that randomly putting buffers to the cache always results in a
> +	 * valid index.
> +	 */
> +	int testRandom(V4L2BufferCache *cache,
> +		       const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> +	{

		std::random_device rd;
		std::mt19937 gen(rd());
		std::uniform_int_distribution<> dis(0, buffers.size());

(rd and gen should likely be class members as you need random number
generation in testHot() too, dis can stay local and be duplicated in
testHot()).

> +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> +			int nBuffer = rand() % buffers.size();

			int nBuffer = dis(gen);

Slightly longer to write, but will give you a real uniform distribution.
It's certainly overkill here but I think it's useful to get used to the
best practices as it won't hurt.

> +			int index = cache->get(*buffers[nBuffer].get());
> +
> +			if (index < 0) {
> +				std::cout << "Failed lookup from cache"
> +					  << std::endl;
> +				return TestFail;
> +			}
> +
> +			cache->put(index);
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	/*
> +	 * Test that using a buffer more frequently keeps it hot in the cache at
> +	 * all times.
> +	 */
> +	int testHot(V4L2BufferCache *cache,
> +		    const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> +		    unsigned int hotFrequency)
> +	{
> +		/* Run the random test on the cache to make it messy. */
> +		if (testRandom(cache, buffers) != TestPass)
> +			return TestFail;
> +
> +		/* Pick a hot buffer at random and store its index */

s/index/index./

> +		int hotBuffer = rand() % buffers.size();
> +		int hotIndex = cache->get(*buffers[hotBuffer].get());
> +		cache->put(hotIndex);
> +
> +		/*
> +		 * Queue hot buffer at the requested frequency and make sure
> +		 * it stays hot.
> +		 */
> +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> +			int nBuffer, index;
> +			bool hotQueue = i % hotFrequency == 0;
> +
> +			if (hotQueue)
> +				nBuffer = hotBuffer;
> +			else
> +				nBuffer = rand() % buffers.size();
> +
> +			index = cache->get(*buffers[nBuffer].get());
> +
> +			if (index < 0) {
> +				std::cout << "Failed lookup from cache"
> +					  << std::endl;
> +				return TestFail;
> +			}
> +
> +			if (hotQueue && index != hotIndex) {
> +				std::cout << "Hot buffer got cold"
> +					  << std::endl;
> +				return TestFail;
> +			}
> +
> +			cache->put(index);
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int run() override
> +	{
> +		const unsigned int numBuffers = 8;
> +
> +		StreamConfiguration cfg;
> +		cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> +		cfg.size = Size(600, 800);
> +		cfg.bufferCount = numBuffers;
> +
> +		BufferSource source;
> +		int ret = source.allocate(cfg);
> +		if (ret != TestPass)
> +			return ret;
> +

I would add

		const std::vector<std::unique_ptr<FrameBuffer>> &buffer = source.buffers();

as you use source.buffers() all the time.

> +		if (source.buffers().size() != numBuffers) {
> +			std::cout << "Got " << source.buffers().size()
> +				  << " buffers, expected " << numBuffers
> +				  << std::endl;
> +			return TestFail;
> +		}
> +
> +		/*
> +		 * Test cache of same size as there are buffers, cache is

s/cache is/the cache is/

Same below.

> +		 * created from a list of buffers and will be pre-populated.
> +		 */
> +		V4L2BufferCache cacheFromBuffers(source.buffers());
> +
> +		if (testSequential(&cacheFromBuffers, source.buffers()) != TestPass)
> +			return TestFail;
> +
> +		if (testRandom(&cacheFromBuffers, source.buffers()) != TestPass)
> +			return TestFail;
> +
> +		if (testHot(&cacheFromBuffers, source.buffers(), numBuffers - 1) != TestPass)

Is the -1 needed (here and below) ?

> +			return TestFail;
> +
> +		/*
> +		 * Test cache of same size as there are buffers, cache is not
> +		 * pre-populated.
> +		 */
> +		V4L2BufferCache cacheFromNumbers(numBuffers);
> +
> +		if (testSequential(&cacheFromNumbers, source.buffers()) != TestPass)
> +			return TestFail;
> +
> +		if (testRandom(&cacheFromNumbers, source.buffers()) != TestPass)
> +			return TestFail;
> +
> +		if (testHot(&cacheFromNumbers, source.buffers(), numBuffers - 1) != TestPass)
> +			return TestFail;
> +
> +		/*
> +		 * Test cache half the size of number of buffers used, cache is
> +		 * not pre-populated.
> +		 */
> +		V4L2BufferCache cacheHalf(numBuffers / 2);
> +
> +		if (testRandom(&cacheHalf, source.buffers()) != TestPass)
> +			return TestFail;
> +
> +		if (testHot(&cacheHalf, source.buffers(), numBuffers / 2 - 1) != TestPass)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +};
> +
> +} /* namespace */
> +
> +TEST_REGISTER(BufferCacheTest);
> diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> index 5c52da7219c21cc3..685fcf6d16d72182 100644
> --- a/test/v4l2_videodevice/meson.build
> +++ b/test/v4l2_videodevice/meson.build
> @@ -5,6 +5,7 @@ v4l2_videodevice_tests = [
>      [ 'controls',           'controls.cpp' ],
>      [ 'formats',            'formats.cpp' ],
>      [ 'request_buffers',    'request_buffers.cpp' ],
> +    [ 'buffer_cache',       'buffer_cache.cpp' ],
>      [ 'stream_on_off',      'stream_on_off.cpp' ],
>      [ 'capture_async',      'capture_async.cpp' ],
>      [ 'buffer_sharing',     'buffer_sharing.cpp' ],

Hi Niklas,

On 16/02/2020 16:16, Niklas Söderlund wrote:
> Add test to test the different modes and situations the V4L2BufferCache
> can be put to. The tests verify that a FrameBuffer used with the cache
> results in a V4L2 video device index can be resolved, and that the cache
> implementation is capable of keeping buffers in a hot state.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  test/v4l2_videodevice/buffer_cache.cpp | 286 +++++++++++++++++++++++++
>  test/v4l2_videodevice/meson.build      |   1 +
>  2 files changed, 287 insertions(+)
>  create mode 100644 test/v4l2_videodevice/buffer_cache.cpp
> 
> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> new file mode 100644
> index 0000000000000000..6244361130525d29
> --- /dev/null
> +++ b/test/v4l2_videodevice/buffer_cache.cpp
> @@ -0,0 +1,286 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * Test the buffer cache different operation modes
> + */
> +
> +#include <iostream>
> +#include <random>
> +#include <vector>
> +
> +#include <libcamera/stream.h>
> +
> +#include "device_enumerator.h"
> +#include "media_device.h"
> +#include "v4l2_videodevice.h"
> +
> +#include "test.h"
> +
> +using namespace libcamera;
> +
> +namespace {
> +
> +/* A provider of source of external buffers, suitable for to use by V4L2BufferCache. */
> +class BufferSource
> +{
> +public:
> +	BufferSource()
> +		: video_(nullptr)
> +	{
> +	}
> +
> +	~BufferSource()
> +	{
> +		if (video_) {
> +			video_->releaseBuffers();
> +			video_->close();
> +		}
> +
> +		delete video_;
> +		video_ = nullptr;
> +
> +		if (media_)
> +			media_->release();
> +	}
> +
> +	int allocate(const StreamConfiguration &config)
> +	{
> +		/* Locate and open the video device. */
> +		std::string videoDeviceName = "vivid-000-vid-out";
> +
> +		std::unique_ptr<DeviceEnumerator> enumerator =
> +			DeviceEnumerator::create();
> +		if (!enumerator) {
> +			std::cout << "Failed to create device enumerator" << std::endl;
> +			return TestFail;
> +		}
> +
> +		if (enumerator->enumerate()) {
> +			std::cout << "Failed to enumerate media devices" << std::endl;
> +			return TestFail;
> +		}
> +
> +		DeviceMatch dm("vivid");
> +		dm.add(videoDeviceName);
> +
> +		media_ = enumerator->search(dm);
> +		if (!media_) {
> +			std::cout << "No vivid output device available" << std::endl;
> +			return TestSkip;
> +		}
> +
> +		video_ = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);
> +		if (!video_) {
> +			std::cout << "Unable to open " << videoDeviceName << std::endl;
> +			return TestFail;
> +		}
> +
> +		if (video_->open())
> +			return TestFail;
> +
> +		/* Configure the format. */
> +		V4L2DeviceFormat format;
> +		if (video_->getFormat(&format)) {
> +			std::cout << "Failed to get format on output device" << std::endl;
> +			return TestFail;
> +		}
> +
> +		format.size = config.size;
> +		format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false);
> +		if (video_->setFormat(&format)) {
> +			std::cout << "Failed to set format on output device" << std::endl;
> +			return TestFail;
> +		}
> +
> +		if (video_->exportBuffers(config.bufferCount, &buffers_) < 0) {
> +			std::cout << "Failed to export buffers" << std::endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers()
> +	{
> +		return buffers_;
> +	}
> +
> +private:
> +	std::shared_ptr<MediaDevice> media_;
> +	V4L2VideoDevice *video_;
> +	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> +};
> +
> +class BufferCacheTest : public Test
> +{
> +public:
> +	/*
> +	 * Test that a cache with the same size as there are buffers results in
> +	 * a sequential run over; 0, 1, 2, 3, 4, 0, 1, 2, 3, 4, ...
> +	 *
> +	 * Test only valid when the cache size is as least as big as there are
> +	 * number of buffers.
> +	 */
> +	int testSequential(V4L2BufferCache *cache,
> +			   const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> +	{
> +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> +			int nBuffer = i % buffers.size();
> +			int index = cache->get(*buffers[nBuffer].get());
> +
> +			if (index != nBuffer) {
> +				std::cout << "Expected index " << nBuffer
> +					  << " got " << index << std::endl;
> +				return TestFail;
> +			}
> +
> +			cache->put(index);
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	/*
> +	 * Test that randomly putting buffers to the cache always results in a
> +	 * valid index.
> +	 */
> +	int testRandom(V4L2BufferCache *cache,
> +		       const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> +	{
> +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> +			int nBuffer = rand() % buffers.size();

Is rand() a prng? Or does it get arbitrary random data?

It would be useful if this was based around a prng (which is perhaps
seeded randomly) so that we can ensure we can reliably repeat a failing
test....

Then in any event of failure (and providing that the failure test case
prints it's random seed in the logs) we can hardcode the seed into the
binary and repeat the failing case to identify the root-cause.

That's mostly a more general comment about seeing random usage in tests
than this specific use-case of course, but I fear it may still be
applicable in some aspect.

--
Kieran




> +			int index = cache->get(*buffers[nBuffer].get());
> +
> +			if (index < 0) {
> +				std::cout << "Failed lookup from cache"
> +					  << std::endl;
> +				return TestFail;
> +			}
> +
> +			cache->put(index);
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	/*
> +	 * Test that using a buffer more frequently keeps it hot in the cache at
> +	 * all times.
> +	 */
> +	int testHot(V4L2BufferCache *cache,
> +		    const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> +		    unsigned int hotFrequency)
> +	{
> +		/* Run the random test on the cache to make it messy. */
> +		if (testRandom(cache, buffers) != TestPass)
> +			return TestFail;
> +
> +		/* Pick a hot buffer at random and store its index */
> +		int hotBuffer = rand() % buffers.size();
> +		int hotIndex = cache->get(*buffers[hotBuffer].get());
> +		cache->put(hotIndex);
> +
> +		/*
> +		 * Queue hot buffer at the requested frequency and make sure
> +		 * it stays hot.
> +		 */
> +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> +			int nBuffer, index;
> +			bool hotQueue = i % hotFrequency == 0;
> +
> +			if (hotQueue)
> +				nBuffer = hotBuffer;
> +			else
> +				nBuffer = rand() % buffers.size();
> +
> +			index = cache->get(*buffers[nBuffer].get());
> +
> +			if (index < 0) {
> +				std::cout << "Failed lookup from cache"
> +					  << std::endl;
> +				return TestFail;
> +			}
> +
> +			if (hotQueue && index != hotIndex) {
> +				std::cout << "Hot buffer got cold"
> +					  << std::endl;
> +				return TestFail;
> +			}
> +
> +			cache->put(index);
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int run() override
> +	{
> +		const unsigned int numBuffers = 8;
> +
> +		StreamConfiguration cfg;
> +		cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> +		cfg.size = Size(600, 800);
> +		cfg.bufferCount = numBuffers;
> +
> +		BufferSource source;
> +		int ret = source.allocate(cfg);
> +		if (ret != TestPass)
> +			return ret;
> +
> +		if (source.buffers().size() != numBuffers) {
> +			std::cout << "Got " << source.buffers().size()
> +				  << " buffers, expected " << numBuffers
> +				  << std::endl;
> +			return TestFail;
> +		}
> +
> +		/*
> +		 * Test cache of same size as there are buffers, cache is
> +		 * created from a list of buffers and will be pre-populated.
> +		 */
> +		V4L2BufferCache cacheFromBuffers(source.buffers());
> +
> +		if (testSequential(&cacheFromBuffers, source.buffers()) != TestPass)
> +			return TestFail;
> +
> +		if (testRandom(&cacheFromBuffers, source.buffers()) != TestPass)
> +			return TestFail;
> +
> +		if (testHot(&cacheFromBuffers, source.buffers(), numBuffers - 1) != TestPass)
> +			return TestFail;
> +
> +		/*
> +		 * Test cache of same size as there are buffers, cache is not
> +		 * pre-populated.
> +		 */
> +		V4L2BufferCache cacheFromNumbers(numBuffers);
> +
> +		if (testSequential(&cacheFromNumbers, source.buffers()) != TestPass)
> +			return TestFail;
> +
> +		if (testRandom(&cacheFromNumbers, source.buffers()) != TestPass)
> +			return TestFail;
> +
> +		if (testHot(&cacheFromNumbers, source.buffers(), numBuffers - 1) != TestPass)
> +			return TestFail;
> +
> +		/*
> +		 * Test cache half the size of number of buffers used, cache is
> +		 * not pre-populated.
> +		 */
> +		V4L2BufferCache cacheHalf(numBuffers / 2);
> +
> +		if (testRandom(&cacheHalf, source.buffers()) != TestPass)
> +			return TestFail;
> +
> +		if (testHot(&cacheHalf, source.buffers(), numBuffers / 2 - 1) != TestPass)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +};
> +
> +} /* namespace */
> +
> +TEST_REGISTER(BufferCacheTest);
> diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> index 5c52da7219c21cc3..685fcf6d16d72182 100644
> --- a/test/v4l2_videodevice/meson.build
> +++ b/test/v4l2_videodevice/meson.build
> @@ -5,6 +5,7 @@ v4l2_videodevice_tests = [
>      [ 'controls',           'controls.cpp' ],
>      [ 'formats',            'formats.cpp' ],
>      [ 'request_buffers',    'request_buffers.cpp' ],
> +    [ 'buffer_cache',       'buffer_cache.cpp' ],
>      [ 'stream_on_off',      'stream_on_off.cpp' ],
>      [ 'capture_async',      'capture_async.cpp' ],
>      [ 'buffer_sharing',     'buffer_sharing.cpp' ],
>

Hi Kieran,

Thanks for your feedback.

On 2020-02-17 10:12:30 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 16/02/2020 16:16, Niklas Söderlund wrote:
> > Add test to test the different modes and situations the V4L2BufferCache
> > can be put to. The tests verify that a FrameBuffer used with the cache
> > results in a V4L2 video device index can be resolved, and that the cache
> > implementation is capable of keeping buffers in a hot state.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  test/v4l2_videodevice/buffer_cache.cpp | 286 +++++++++++++++++++++++++
> >  test/v4l2_videodevice/meson.build      |   1 +
> >  2 files changed, 287 insertions(+)
> >  create mode 100644 test/v4l2_videodevice/buffer_cache.cpp
> > 
> > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> > new file mode 100644
> > index 0000000000000000..6244361130525d29
> > --- /dev/null
> > +++ b/test/v4l2_videodevice/buffer_cache.cpp
> > @@ -0,0 +1,286 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * Test the buffer cache different operation modes
> > + */
> > +
> > +#include <iostream>
> > +#include <random>
> > +#include <vector>
> > +
> > +#include <libcamera/stream.h>
> > +
> > +#include "device_enumerator.h"
> > +#include "media_device.h"
> > +#include "v4l2_videodevice.h"
> > +
> > +#include "test.h"
> > +
> > +using namespace libcamera;
> > +
> > +namespace {
> > +
> > +/* A provider of source of external buffers, suitable for to use by V4L2BufferCache. */
> > +class BufferSource
> > +{
> > +public:
> > +	BufferSource()
> > +		: video_(nullptr)
> > +	{
> > +	}
> > +
> > +	~BufferSource()
> > +	{
> > +		if (video_) {
> > +			video_->releaseBuffers();
> > +			video_->close();
> > +		}
> > +
> > +		delete video_;
> > +		video_ = nullptr;
> > +
> > +		if (media_)
> > +			media_->release();
> > +	}
> > +
> > +	int allocate(const StreamConfiguration &config)
> > +	{
> > +		/* Locate and open the video device. */
> > +		std::string videoDeviceName = "vivid-000-vid-out";
> > +
> > +		std::unique_ptr<DeviceEnumerator> enumerator =
> > +			DeviceEnumerator::create();
> > +		if (!enumerator) {
> > +			std::cout << "Failed to create device enumerator" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (enumerator->enumerate()) {
> > +			std::cout << "Failed to enumerate media devices" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		DeviceMatch dm("vivid");
> > +		dm.add(videoDeviceName);
> > +
> > +		media_ = enumerator->search(dm);
> > +		if (!media_) {
> > +			std::cout << "No vivid output device available" << std::endl;
> > +			return TestSkip;
> > +		}
> > +
> > +		video_ = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);
> > +		if (!video_) {
> > +			std::cout << "Unable to open " << videoDeviceName << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (video_->open())
> > +			return TestFail;
> > +
> > +		/* Configure the format. */
> > +		V4L2DeviceFormat format;
> > +		if (video_->getFormat(&format)) {
> > +			std::cout << "Failed to get format on output device" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		format.size = config.size;
> > +		format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false);
> > +		if (video_->setFormat(&format)) {
> > +			std::cout << "Failed to set format on output device" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (video_->exportBuffers(config.bufferCount, &buffers_) < 0) {
> > +			std::cout << "Failed to export buffers" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers()
> > +	{
> > +		return buffers_;
> > +	}
> > +
> > +private:
> > +	std::shared_ptr<MediaDevice> media_;
> > +	V4L2VideoDevice *video_;
> > +	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> > +};
> > +
> > +class BufferCacheTest : public Test
> > +{
> > +public:
> > +	/*
> > +	 * Test that a cache with the same size as there are buffers results in
> > +	 * a sequential run over; 0, 1, 2, 3, 4, 0, 1, 2, 3, 4, ...
> > +	 *
> > +	 * Test only valid when the cache size is as least as big as there are
> > +	 * number of buffers.
> > +	 */
> > +	int testSequential(V4L2BufferCache *cache,
> > +			   const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> > +	{
> > +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> > +			int nBuffer = i % buffers.size();
> > +			int index = cache->get(*buffers[nBuffer].get());
> > +
> > +			if (index != nBuffer) {
> > +				std::cout << "Expected index " << nBuffer
> > +					  << " got " << index << std::endl;
> > +				return TestFail;
> > +			}
> > +
> > +			cache->put(index);
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	/*
> > +	 * Test that randomly putting buffers to the cache always results in a
> > +	 * valid index.
> > +	 */
> > +	int testRandom(V4L2BufferCache *cache,
> > +		       const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> > +	{
> > +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> > +			int nBuffer = rand() % buffers.size();
> 
> Is rand() a prng? Or does it get arbitrary random data?

To be fully correct I think one need to seed rand() with srand().

> 
> It would be useful if this was based around a prng (which is perhaps
> seeded randomly) so that we can ensure we can reliably repeat a failing
> test....
> 
> Then in any event of failure (and providing that the failure test case
> prints it's random seed in the logs) we can hardcode the seed into the
> binary and repeat the failing case to identify the root-cause.
> 
> That's mostly a more general comment about seeing random usage in tests
> than this specific use-case of course, but I fear it may still be
> applicable in some aspect.

I agree there could be tests where this level of reproducibility would 
be highly desirable. I'm not so concerned about this particular test 
case tho.

If you feel strongly about it I'm not against it but then I think we 
shall provide a helper class that can be reused for all our testcases in 
case they need random numbers.

> 
> --
> Kieran
> 
> 
> 
> 
> > +			int index = cache->get(*buffers[nBuffer].get());
> > +
> > +			if (index < 0) {
> > +				std::cout << "Failed lookup from cache"
> > +					  << std::endl;
> > +				return TestFail;
> > +			}
> > +
> > +			cache->put(index);
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	/*
> > +	 * Test that using a buffer more frequently keeps it hot in the cache at
> > +	 * all times.
> > +	 */
> > +	int testHot(V4L2BufferCache *cache,
> > +		    const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> > +		    unsigned int hotFrequency)
> > +	{
> > +		/* Run the random test on the cache to make it messy. */
> > +		if (testRandom(cache, buffers) != TestPass)
> > +			return TestFail;
> > +
> > +		/* Pick a hot buffer at random and store its index */
> > +		int hotBuffer = rand() % buffers.size();
> > +		int hotIndex = cache->get(*buffers[hotBuffer].get());
> > +		cache->put(hotIndex);
> > +
> > +		/*
> > +		 * Queue hot buffer at the requested frequency and make sure
> > +		 * it stays hot.
> > +		 */
> > +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> > +			int nBuffer, index;
> > +			bool hotQueue = i % hotFrequency == 0;
> > +
> > +			if (hotQueue)
> > +				nBuffer = hotBuffer;
> > +			else
> > +				nBuffer = rand() % buffers.size();
> > +
> > +			index = cache->get(*buffers[nBuffer].get());
> > +
> > +			if (index < 0) {
> > +				std::cout << "Failed lookup from cache"
> > +					  << std::endl;
> > +				return TestFail;
> > +			}
> > +
> > +			if (hotQueue && index != hotIndex) {
> > +				std::cout << "Hot buffer got cold"
> > +					  << std::endl;
> > +				return TestFail;
> > +			}
> > +
> > +			cache->put(index);
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	int run() override
> > +	{
> > +		const unsigned int numBuffers = 8;
> > +
> > +		StreamConfiguration cfg;
> > +		cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> > +		cfg.size = Size(600, 800);
> > +		cfg.bufferCount = numBuffers;
> > +
> > +		BufferSource source;
> > +		int ret = source.allocate(cfg);
> > +		if (ret != TestPass)
> > +			return ret;
> > +
> > +		if (source.buffers().size() != numBuffers) {
> > +			std::cout << "Got " << source.buffers().size()
> > +				  << " buffers, expected " << numBuffers
> > +				  << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/*
> > +		 * Test cache of same size as there are buffers, cache is
> > +		 * created from a list of buffers and will be pre-populated.
> > +		 */
> > +		V4L2BufferCache cacheFromBuffers(source.buffers());
> > +
> > +		if (testSequential(&cacheFromBuffers, source.buffers()) != TestPass)
> > +			return TestFail;
> > +
> > +		if (testRandom(&cacheFromBuffers, source.buffers()) != TestPass)
> > +			return TestFail;
> > +
> > +		if (testHot(&cacheFromBuffers, source.buffers(), numBuffers - 1) != TestPass)
> > +			return TestFail;
> > +
> > +		/*
> > +		 * Test cache of same size as there are buffers, cache is not
> > +		 * pre-populated.
> > +		 */
> > +		V4L2BufferCache cacheFromNumbers(numBuffers);
> > +
> > +		if (testSequential(&cacheFromNumbers, source.buffers()) != TestPass)
> > +			return TestFail;
> > +
> > +		if (testRandom(&cacheFromNumbers, source.buffers()) != TestPass)
> > +			return TestFail;
> > +
> > +		if (testHot(&cacheFromNumbers, source.buffers(), numBuffers - 1) != TestPass)
> > +			return TestFail;
> > +
> > +		/*
> > +		 * Test cache half the size of number of buffers used, cache is
> > +		 * not pre-populated.
> > +		 */
> > +		V4L2BufferCache cacheHalf(numBuffers / 2);
> > +
> > +		if (testRandom(&cacheHalf, source.buffers()) != TestPass)
> > +			return TestFail;
> > +
> > +		if (testHot(&cacheHalf, source.buffers(), numBuffers / 2 - 1) != TestPass)
> > +			return TestFail;
> > +
> > +		return TestPass;
> > +	}
> > +};
> > +
> > +} /* namespace */
> > +
> > +TEST_REGISTER(BufferCacheTest);
> > diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> > index 5c52da7219c21cc3..685fcf6d16d72182 100644
> > --- a/test/v4l2_videodevice/meson.build
> > +++ b/test/v4l2_videodevice/meson.build
> > @@ -5,6 +5,7 @@ v4l2_videodevice_tests = [
> >      [ 'controls',           'controls.cpp' ],
> >      [ 'formats',            'formats.cpp' ],
> >      [ 'request_buffers',    'request_buffers.cpp' ],
> > +    [ 'buffer_cache',       'buffer_cache.cpp' ],
> >      [ 'stream_on_off',      'stream_on_off.cpp' ],
> >      [ 'capture_async',      'capture_async.cpp' ],
> >      [ 'buffer_sharing',     'buffer_sharing.cpp' ],
> > 
> 
> -- 
> Regards
> --
> Kieran

Hi Laurent,

Thanks for your feedback.

On 2020-02-17 00:18:35 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sun, Feb 16, 2020 at 05:16:42PM +0100, Niklas Söderlund wrote:
> > Add test to test the different modes and situations the V4L2BufferCache
> > can be put to. The tests verify that a FrameBuffer used with the cache
> 
> s/put to/put in/ ?
> 
> > results in a V4L2 video device index can be resolved, and that the cache
> 
> s/can be resolved/that can be resolved/ ? Or did you mean something else
> ? I'm not sure what you mean by resolved here to be honest.

Maybe s/resolved/produced/ ?

> 
> > implementation is capable of keeping buffers in a hot state.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> Could you move this before patch 1/2 to see that it fails, and that the
> fix actually fixes the problem ?

Sure.

> 
> > ---
> >  test/v4l2_videodevice/buffer_cache.cpp | 286 +++++++++++++++++++++++++
> >  test/v4l2_videodevice/meson.build      |   1 +
> >  2 files changed, 287 insertions(+)
> >  create mode 100644 test/v4l2_videodevice/buffer_cache.cpp
> > 
> > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> > new file mode 100644
> > index 0000000000000000..6244361130525d29
> > --- /dev/null
> > +++ b/test/v4l2_videodevice/buffer_cache.cpp
> > @@ -0,0 +1,286 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * Test the buffer cache different operation modes
> > + */
> > +
> > +#include <iostream>
> > +#include <random>
> > +#include <vector>
> > +
> > +#include <libcamera/stream.h>
> > +
> > +#include "device_enumerator.h"
> > +#include "media_device.h"
> > +#include "v4l2_videodevice.h"
> > +
> > +#include "test.h"
> > +
> > +using namespace libcamera;
> > +
> > +namespace {
> > +
> > +/* A provider of source of external buffers, suitable for to use by V4L2BufferCache. */
> 
> Line wrap ?
> 
> A provider or a source ?
> 
> s/for to use/for use/
> 
> > +class BufferSource
> > +{
> > +public:
> > +	BufferSource()
> > +		: video_(nullptr)
> > +	{
> > +	}
> > +
> > +	~BufferSource()
> > +	{
> > +		if (video_) {
> > +			video_->releaseBuffers();
> > +			video_->close();
> > +		}
> > +
> > +		delete video_;
> > +		video_ = nullptr;
> > +
> > +		if (media_)
> > +			media_->release();
> > +	}
> > +
> > +	int allocate(const StreamConfiguration &config)
> > +	{
> > +		/* Locate and open the video device. */
> > +		std::string videoDeviceName = "vivid-000-vid-out";
> > +
> > +		std::unique_ptr<DeviceEnumerator> enumerator =
> > +			DeviceEnumerator::create();
> > +		if (!enumerator) {
> > +			std::cout << "Failed to create device enumerator" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (enumerator->enumerate()) {
> > +			std::cout << "Failed to enumerate media devices" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		DeviceMatch dm("vivid");
> > +		dm.add(videoDeviceName);
> > +
> > +		media_ = enumerator->search(dm);
> > +		if (!media_) {
> > +			std::cout << "No vivid output device available" << std::endl;
> > +			return TestSkip;
> > +		}
> > +
> > +		video_ = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);
> > +		if (!video_) {
> > +			std::cout << "Unable to open " << videoDeviceName << std::endl;
> 
> I think this error message is for the open() failure.
> 
> > +			return TestFail;
> > +		}
> > +
> > +		if (video_->open())
> > +			return TestFail;
> > +
> > +		/* Configure the format. */
> > +		V4L2DeviceFormat format;
> > +		if (video_->getFormat(&format)) {
> > +			std::cout << "Failed to get format on output device" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		format.size = config.size;
> > +		format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false);
> > +		if (video_->setFormat(&format)) {
> > +			std::cout << "Failed to set format on output device" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (video_->exportBuffers(config.bufferCount, &buffers_) < 0) {
> > +			std::cout << "Failed to export buffers" << std::endl;
> > +			return TestFail;
> > +		}
> 
> We clearly need a better way to get hold of dmabufs from the kernel :-)

:-)

> 
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers()
> > +	{
> > +		return buffers_;
> > +	}
> > +
> > +private:
> > +	std::shared_ptr<MediaDevice> media_;
> > +	V4L2VideoDevice *video_;
> > +	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> > +};
> > +
> > +class BufferCacheTest : public Test
> > +{
> > +public:
> > +	/*
> > +	 * Test that a cache with the same size as there are buffers results in
> > +	 * a sequential run over; 0, 1, 2, 3, 4, 0, 1, 2, 3, 4, ...
> > +	 *
> > +	 * Test only valid when the cache size is as least as big as there are
> 
> s/Test/The test is/
> 
> You won't be charged per word.
> 
> > +	 * number of buffers.
> 
> "... as the number of buffers."
> 
> > +	 */
> > +	int testSequential(V4L2BufferCache *cache,
> > +			   const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> > +	{
> > +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> > +			int nBuffer = i % buffers.size();
> > +			int index = cache->get(*buffers[nBuffer].get());
> > +
> > +			if (index != nBuffer) {
> > +				std::cout << "Expected index " << nBuffer
> > +					  << " got " << index << std::endl;
> > +				return TestFail;
> > +			}
> > +
> > +			cache->put(index);
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	/*
> > +	 * Test that randomly putting buffers to the cache always results in a
> > +	 * valid index.
> > +	 */
> > +	int testRandom(V4L2BufferCache *cache,
> > +		       const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> > +	{
> 
> 		std::random_device rd;
> 		std::mt19937 gen(rd());
> 		std::uniform_int_distribution<> dis(0, buffers.size());
> 
> (rd and gen should likely be class members as you need random number
> generation in testHot() too, dis can stay local and be duplicated in
> testHot()).
> 
> > +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> > +			int nBuffer = rand() % buffers.size();
> 
> 			int nBuffer = dis(gen);
> 
> Slightly longer to write, but will give you a real uniform distribution.
> It's certainly overkill here but I think it's useful to get used to the
> best practices as it won't hurt.

I will see how the discussion about reproducible random numbers end and 
either to this or add a helper random number class which uses the std 
random numbers if they can be made reproducible.

> 
> > +			int index = cache->get(*buffers[nBuffer].get());
> > +
> > +			if (index < 0) {
> > +				std::cout << "Failed lookup from cache"
> > +					  << std::endl;
> > +				return TestFail;
> > +			}
> > +
> > +			cache->put(index);
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	/*
> > +	 * Test that using a buffer more frequently keeps it hot in the cache at
> > +	 * all times.
> > +	 */
> > +	int testHot(V4L2BufferCache *cache,
> > +		    const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> > +		    unsigned int hotFrequency)
> > +	{
> > +		/* Run the random test on the cache to make it messy. */
> > +		if (testRandom(cache, buffers) != TestPass)
> > +			return TestFail;
> > +
> > +		/* Pick a hot buffer at random and store its index */
> 
> s/index/index./
> 
> > +		int hotBuffer = rand() % buffers.size();
> > +		int hotIndex = cache->get(*buffers[hotBuffer].get());
> > +		cache->put(hotIndex);
> > +
> > +		/*
> > +		 * Queue hot buffer at the requested frequency and make sure
> > +		 * it stays hot.
> > +		 */
> > +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> > +			int nBuffer, index;
> > +			bool hotQueue = i % hotFrequency == 0;
> > +
> > +			if (hotQueue)
> > +				nBuffer = hotBuffer;
> > +			else
> > +				nBuffer = rand() % buffers.size();
> > +
> > +			index = cache->get(*buffers[nBuffer].get());
> > +
> > +			if (index < 0) {
> > +				std::cout << "Failed lookup from cache"
> > +					  << std::endl;
> > +				return TestFail;
> > +			}
> > +
> > +			if (hotQueue && index != hotIndex) {
> > +				std::cout << "Hot buffer got cold"
> > +					  << std::endl;
> > +				return TestFail;
> > +			}
> > +
> > +			cache->put(index);
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	int run() override
> > +	{
> > +		const unsigned int numBuffers = 8;
> > +
> > +		StreamConfiguration cfg;
> > +		cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> > +		cfg.size = Size(600, 800);
> > +		cfg.bufferCount = numBuffers;
> > +
> > +		BufferSource source;
> > +		int ret = source.allocate(cfg);
> > +		if (ret != TestPass)
> > +			return ret;
> > +
> 
> I would add
> 
> 		const std::vector<std::unique_ptr<FrameBuffer>> &buffer = source.buffers();
> 
> as you use source.buffers() all the time.
> 
> > +		if (source.buffers().size() != numBuffers) {
> > +			std::cout << "Got " << source.buffers().size()
> > +				  << " buffers, expected " << numBuffers
> > +				  << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/*
> > +		 * Test cache of same size as there are buffers, cache is
> 
> s/cache is/the cache is/
> 
> Same below.
> 
> > +		 * created from a list of buffers and will be pre-populated.
> > +		 */
> > +		V4L2BufferCache cacheFromBuffers(source.buffers());
> > +
> > +		if (testSequential(&cacheFromBuffers, source.buffers()) != TestPass)
> > +			return TestFail;
> > +
> > +		if (testRandom(&cacheFromBuffers, source.buffers()) != TestPass)
> > +			return TestFail;
> > +
> > +		if (testHot(&cacheFromBuffers, source.buffers(), numBuffers - 1) != TestPass)
> 
> Is the -1 needed (here and below) ?

No, good catch. They are a left over from an earlier state of 
development.

> 
> > +			return TestFail;
> > +
> > +		/*
> > +		 * Test cache of same size as there are buffers, cache is not
> > +		 * pre-populated.
> > +		 */
> > +		V4L2BufferCache cacheFromNumbers(numBuffers);
> > +
> > +		if (testSequential(&cacheFromNumbers, source.buffers()) != TestPass)
> > +			return TestFail;
> > +
> > +		if (testRandom(&cacheFromNumbers, source.buffers()) != TestPass)
> > +			return TestFail;
> > +
> > +		if (testHot(&cacheFromNumbers, source.buffers(), numBuffers - 1) != TestPass)
> > +			return TestFail;
> > +
> > +		/*
> > +		 * Test cache half the size of number of buffers used, cache is
> > +		 * not pre-populated.
> > +		 */
> > +		V4L2BufferCache cacheHalf(numBuffers / 2);
> > +
> > +		if (testRandom(&cacheHalf, source.buffers()) != TestPass)
> > +			return TestFail;
> > +
> > +		if (testHot(&cacheHalf, source.buffers(), numBuffers / 2 - 1) != TestPass)
> > +			return TestFail;
> > +
> > +		return TestPass;
> > +	}
> > +};
> > +
> > +} /* namespace */
> > +
> > +TEST_REGISTER(BufferCacheTest);
> > diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> > index 5c52da7219c21cc3..685fcf6d16d72182 100644
> > --- a/test/v4l2_videodevice/meson.build
> > +++ b/test/v4l2_videodevice/meson.build
> > @@ -5,6 +5,7 @@ v4l2_videodevice_tests = [
> >      [ 'controls',           'controls.cpp' ],
> >      [ 'formats',            'formats.cpp' ],
> >      [ 'request_buffers',    'request_buffers.cpp' ],
> > +    [ 'buffer_cache',       'buffer_cache.cpp' ],
> >      [ 'stream_on_off',      'stream_on_off.cpp' ],
> >      [ 'capture_async',      'capture_async.cpp' ],
> >      [ 'buffer_sharing',     'buffer_sharing.cpp' ],
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Hello,

On Mon, Feb 17, 2020 at 01:46:14PM +0100, Niklas Söderlund wrote:
> On 2020-02-17 10:12:30 +0000, Kieran Bingham wrote:
> > On 16/02/2020 16:16, Niklas Söderlund wrote:
> >> Add test to test the different modes and situations the V4L2BufferCache
> >> can be put to. The tests verify that a FrameBuffer used with the cache
> >> results in a V4L2 video device index can be resolved, and that the cache
> >> implementation is capable of keeping buffers in a hot state.
> >> 
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >> ---
> >>  test/v4l2_videodevice/buffer_cache.cpp | 286 +++++++++++++++++++++++++
> >>  test/v4l2_videodevice/meson.build      |   1 +
> >>  2 files changed, 287 insertions(+)
> >>  create mode 100644 test/v4l2_videodevice/buffer_cache.cpp
> >> 
> >> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> >> new file mode 100644
> >> index 0000000000000000..6244361130525d29
> >> --- /dev/null
> >> +++ b/test/v4l2_videodevice/buffer_cache.cpp
> >> @@ -0,0 +1,286 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2020, Google Inc.
> >> + *
> >> + * Test the buffer cache different operation modes
> >> + */
> >> +
> >> +#include <iostream>
> >> +#include <random>
> >> +#include <vector>
> >> +
> >> +#include <libcamera/stream.h>
> >> +
> >> +#include "device_enumerator.h"
> >> +#include "media_device.h"
> >> +#include "v4l2_videodevice.h"
> >> +
> >> +#include "test.h"
> >> +
> >> +using namespace libcamera;
> >> +
> >> +namespace {
> >> +
> >> +/* A provider of source of external buffers, suitable for to use by V4L2BufferCache. */
> >> +class BufferSource
> >> +{
> >> +public:
> >> +	BufferSource()
> >> +		: video_(nullptr)
> >> +	{
> >> +	}
> >> +
> >> +	~BufferSource()
> >> +	{
> >> +		if (video_) {
> >> +			video_->releaseBuffers();
> >> +			video_->close();
> >> +		}
> >> +
> >> +		delete video_;
> >> +		video_ = nullptr;
> >> +
> >> +		if (media_)
> >> +			media_->release();
> >> +	}
> >> +
> >> +	int allocate(const StreamConfiguration &config)
> >> +	{
> >> +		/* Locate and open the video device. */
> >> +		std::string videoDeviceName = "vivid-000-vid-out";
> >> +
> >> +		std::unique_ptr<DeviceEnumerator> enumerator =
> >> +			DeviceEnumerator::create();
> >> +		if (!enumerator) {
> >> +			std::cout << "Failed to create device enumerator" << std::endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		if (enumerator->enumerate()) {
> >> +			std::cout << "Failed to enumerate media devices" << std::endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		DeviceMatch dm("vivid");
> >> +		dm.add(videoDeviceName);
> >> +
> >> +		media_ = enumerator->search(dm);
> >> +		if (!media_) {
> >> +			std::cout << "No vivid output device available" << std::endl;
> >> +			return TestSkip;
> >> +		}
> >> +
> >> +		video_ = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);
> >> +		if (!video_) {
> >> +			std::cout << "Unable to open " << videoDeviceName << std::endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		if (video_->open())
> >> +			return TestFail;
> >> +
> >> +		/* Configure the format. */
> >> +		V4L2DeviceFormat format;
> >> +		if (video_->getFormat(&format)) {
> >> +			std::cout << "Failed to get format on output device" << std::endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		format.size = config.size;
> >> +		format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false);
> >> +		if (video_->setFormat(&format)) {
> >> +			std::cout << "Failed to set format on output device" << std::endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		if (video_->exportBuffers(config.bufferCount, &buffers_) < 0) {
> >> +			std::cout << "Failed to export buffers" << std::endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		return TestPass;
> >> +	}
> >> +
> >> +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers()
> >> +	{
> >> +		return buffers_;
> >> +	}
> >> +
> >> +private:
> >> +	std::shared_ptr<MediaDevice> media_;
> >> +	V4L2VideoDevice *video_;
> >> +	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> >> +};
> >> +
> >> +class BufferCacheTest : public Test
> >> +{
> >> +public:
> >> +	/*
> >> +	 * Test that a cache with the same size as there are buffers results in
> >> +	 * a sequential run over; 0, 1, 2, 3, 4, 0, 1, 2, 3, 4, ...
> >> +	 *
> >> +	 * Test only valid when the cache size is as least as big as there are
> >> +	 * number of buffers.
> >> +	 */
> >> +	int testSequential(V4L2BufferCache *cache,
> >> +			   const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> >> +	{
> >> +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> >> +			int nBuffer = i % buffers.size();
> >> +			int index = cache->get(*buffers[nBuffer].get());
> >> +
> >> +			if (index != nBuffer) {
> >> +				std::cout << "Expected index " << nBuffer
> >> +					  << " got " << index << std::endl;
> >> +				return TestFail;
> >> +			}
> >> +
> >> +			cache->put(index);
> >> +		}
> >> +
> >> +		return TestPass;
> >> +	}
> >> +
> >> +	/*
> >> +	 * Test that randomly putting buffers to the cache always results in a
> >> +	 * valid index.
> >> +	 */
> >> +	int testRandom(V4L2BufferCache *cache,
> >> +		       const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> >> +	{
> >> +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> >> +			int nBuffer = rand() % buffers.size();
> > 
> > Is rand() a prng? Or does it get arbitrary random data?
> 
> To be fully correct I think one need to seed rand() with srand().
> 
> > It would be useful if this was based around a prng (which is perhaps
> > seeded randomly) so that we can ensure we can reliably repeat a failing
> > test....
> > 
> > Then in any event of failure (and providing that the failure test case
> > prints it's random seed in the logs) we can hardcode the seed into the
> > binary and repeat the failing case to identify the root-cause.
> > 
> > That's mostly a more general comment about seeing random usage in tests
> > than this specific use-case of course, but I fear it may still be
> > applicable in some aspect.
> 
> I agree there could be tests where this level of reproducibility would 
> be highly desirable. I'm not so concerned about this particular test 
> case tho.
> 
> If you feel strongly about it I'm not against it but then I think we 
> shall provide a helper class that can be reused for all our testcases in 
> case they need random numbers.

I agree with Kieran that tests should be reproducible. How about

	std::mt19937 gen;
	std::uniform_int_distribution<> dist(0, buffers.size() - 1);

	...
		int nBuffer = dist(gen);

This will be a PRNG and should always produce the same results. We can
specify a seed when constructing gen, otherwise the value 5489 is used.

> >> +			int index = cache->get(*buffers[nBuffer].get());
> >> +
> >> +			if (index < 0) {
> >> +				std::cout << "Failed lookup from cache"
> >> +					  << std::endl;
> >> +				return TestFail;
> >> +			}
> >> +
> >> +			cache->put(index);
> >> +		}
> >> +
> >> +		return TestPass;
> >> +	}
> >> +
> >> +	/*
> >> +	 * Test that using a buffer more frequently keeps it hot in the cache at
> >> +	 * all times.
> >> +	 */
> >> +	int testHot(V4L2BufferCache *cache,
> >> +		    const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> >> +		    unsigned int hotFrequency)
> >> +	{
> >> +		/* Run the random test on the cache to make it messy. */
> >> +		if (testRandom(cache, buffers) != TestPass)
> >> +			return TestFail;
> >> +
> >> +		/* Pick a hot buffer at random and store its index */
> >> +		int hotBuffer = rand() % buffers.size();
> >> +		int hotIndex = cache->get(*buffers[hotBuffer].get());
> >> +		cache->put(hotIndex);
> >> +
> >> +		/*
> >> +		 * Queue hot buffer at the requested frequency and make sure
> >> +		 * it stays hot.
> >> +		 */
> >> +		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
> >> +			int nBuffer, index;
> >> +			bool hotQueue = i % hotFrequency == 0;
> >> +
> >> +			if (hotQueue)
> >> +				nBuffer = hotBuffer;
> >> +			else
> >> +				nBuffer = rand() % buffers.size();
> >> +
> >> +			index = cache->get(*buffers[nBuffer].get());
> >> +
> >> +			if (index < 0) {
> >> +				std::cout << "Failed lookup from cache"
> >> +					  << std::endl;
> >> +				return TestFail;
> >> +			}
> >> +
> >> +			if (hotQueue && index != hotIndex) {
> >> +				std::cout << "Hot buffer got cold"
> >> +					  << std::endl;
> >> +				return TestFail;
> >> +			}
> >> +
> >> +			cache->put(index);
> >> +		}
> >> +
> >> +		return TestPass;
> >> +	}
> >> +
> >> +	int run() override
> >> +	{
> >> +		const unsigned int numBuffers = 8;
> >> +
> >> +		StreamConfiguration cfg;
> >> +		cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> >> +		cfg.size = Size(600, 800);
> >> +		cfg.bufferCount = numBuffers;
> >> +
> >> +		BufferSource source;
> >> +		int ret = source.allocate(cfg);
> >> +		if (ret != TestPass)
> >> +			return ret;
> >> +
> >> +		if (source.buffers().size() != numBuffers) {
> >> +			std::cout << "Got " << source.buffers().size()
> >> +				  << " buffers, expected " << numBuffers
> >> +				  << std::endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		/*
> >> +		 * Test cache of same size as there are buffers, cache is
> >> +		 * created from a list of buffers and will be pre-populated.
> >> +		 */
> >> +		V4L2BufferCache cacheFromBuffers(source.buffers());
> >> +
> >> +		if (testSequential(&cacheFromBuffers, source.buffers()) != TestPass)
> >> +			return TestFail;
> >> +
> >> +		if (testRandom(&cacheFromBuffers, source.buffers()) != TestPass)
> >> +			return TestFail;
> >> +
> >> +		if (testHot(&cacheFromBuffers, source.buffers(), numBuffers - 1) != TestPass)
> >> +			return TestFail;
> >> +
> >> +		/*
> >> +		 * Test cache of same size as there are buffers, cache is not
> >> +		 * pre-populated.
> >> +		 */
> >> +		V4L2BufferCache cacheFromNumbers(numBuffers);
> >> +
> >> +		if (testSequential(&cacheFromNumbers, source.buffers()) != TestPass)
> >> +			return TestFail;
> >> +
> >> +		if (testRandom(&cacheFromNumbers, source.buffers()) != TestPass)
> >> +			return TestFail;
> >> +
> >> +		if (testHot(&cacheFromNumbers, source.buffers(), numBuffers - 1) != TestPass)
> >> +			return TestFail;
> >> +
> >> +		/*
> >> +		 * Test cache half the size of number of buffers used, cache is
> >> +		 * not pre-populated.
> >> +		 */
> >> +		V4L2BufferCache cacheHalf(numBuffers / 2);
> >> +
> >> +		if (testRandom(&cacheHalf, source.buffers()) != TestPass)
> >> +			return TestFail;
> >> +
> >> +		if (testHot(&cacheHalf, source.buffers(), numBuffers / 2 - 1) != TestPass)
> >> +			return TestFail;
> >> +
> >> +		return TestPass;
> >> +	}
> >> +};
> >> +
> >> +} /* namespace */
> >> +
> >> +TEST_REGISTER(BufferCacheTest);
> >> diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> >> index 5c52da7219c21cc3..685fcf6d16d72182 100644
> >> --- a/test/v4l2_videodevice/meson.build
> >> +++ b/test/v4l2_videodevice/meson.build
> >> @@ -5,6 +5,7 @@ v4l2_videodevice_tests = [
> >>      [ 'controls',           'controls.cpp' ],
> >>      [ 'formats',            'formats.cpp' ],
> >>      [ 'request_buffers',    'request_buffers.cpp' ],
> >> +    [ 'buffer_cache',       'buffer_cache.cpp' ],
> >>      [ 'stream_on_off',      'stream_on_off.cpp' ],
> >>      [ 'capture_async',      'capture_async.cpp' ],
> >>      [ 'buffer_sharing',     'buffer_sharing.cpp' ],