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

[Niklas Söderlund]

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

-- 
Regards,
Niklas Söderlund