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

[Niklas Söderlund]

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

-- 
Regards,
Niklas Söderlund