[libcamera-devel] [PATCH v3 04/13] test: mapped-buffers: Provide MappedBuffer test

[Kieran Bingham]

Hi Laurent,

On 05/08/2020 01:43, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tue, Aug 04, 2020 at 10:47:02PM +0100, Kieran Bingham wrote:
>> Provide initial testing framework for the MappedBuffer component.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  test/mapped-buffers.cpp | 113 ++++++++++++++++++++++++++++++++++++++++
> 
> We usually don't pluralize the test file name.

Depluralised.

> 
>>  test/meson.build        |   1 +
>>  2 files changed, 114 insertions(+)
>>  create mode 100644 test/mapped-buffers.cpp
>>
>> diff --git a/test/mapped-buffers.cpp b/test/mapped-buffers.cpp
>> new file mode 100644
>> index 000000000000..4c0557070ca3
>> --- /dev/null
>> +++ b/test/mapped-buffers.cpp
>> @@ -0,0 +1,113 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * libcamera internal MappedBuffer tests
>> + */
>> +
>> +#include <iostream>
>> +
>> +#include "camera_test.h"
>> +#include "test.h"
>> +
>> +#include "libcamera/internal/buffer.h"
> 
> Should this go between iostream and camera_test.h ?

Ah yes,


>> +
>> +using namespace std;
>> +
>> +namespace {
>> +
>> +class MappedBuffers : public CameraTest, public Test
> 
> We usually name the test class with a Test suffix (MappedBufferTest).

Fixed.


> 
>> +{
>> +public:
>> +	MappedBuffers()
>> +		: CameraTest("VIMC Sensor B")
> 
> Either you or Niklas will have to solve this merge conflict :-) I wonder
> if CameraTest shouldn't default to VIMC Sensor, but that's for another
> patch.
> 

In this case I don't care about the source, just that I can get an
allocation for a 'real' buffer.



>> +	{
>> +	}
>> +
>> +protected:
>> +	int init() override
>> +	{
>> +		if (status_ != TestPass)
>> +			return status_;
>> +
>> +		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
>> +		if (!config_ || config_->size() != 1) {
>> +			cout << "Failed to generate default configuration" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		allocator_ = new FrameBufferAllocator(camera_);
>> +
>> +		StreamConfiguration &cfg = config_->at(0);
>> +
>> +		if (camera_->acquire()) {
>> +			cout << "Failed to acquire the camera" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		if (camera_->configure(config_.get())) {
>> +			cout << "Failed to set default configuration" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		stream_ = cfg.stream();
>> +
>> +		int ret = allocator_->allocate(stream_);
>> +		if (ret < 0)
>> +			return TestFail;
>> +
>> +		return TestPass;
>> +	}
>> +
>> +	void cleanup() override
>> +	{
>> +		delete allocator_;
>> +	}
>> +
>> +	int run() override
>> +	{
>> +		const std::unique_ptr<FrameBuffer> &buffer = allocator_->buffers(stream_).front();
>> +		std::vector<MappedBuffer> maps;
>> +
>> +		MappedFrameBuffer map(buffer.get(), PROT_READ);
>> +		if (!map.isValid()) {
>> +			cout << "Failed to successfully map buffer" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/* Make sure we can move it. */
>> +		maps.emplace_back(std::move(map));
>> +
>> +		/* But copying is prevented, it would cause double-unmap. */
>> +		// MappedFrameBuffer map_copy = map;
>> +
>> +		/* Local map should be invalid (after move). */
>> +		if (map.isValid()) {
>> +			cout << "Post-move map should not be valid" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/* Test for multiple successful maps on the same buffer. */
>> +		MappedFrameBuffer write_map(buffer.get(), PROT_WRITE);
>> +		if (!write_map.isValid()) {
>> +			cout << "Failed to map write buffer" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		MappedFrameBuffer rw_map(buffer.get(), PROT_READ | PROT_WRITE);
>> +		if (!rw_map.isValid()) {
>> +			cout << "Failed to map RW buffer" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		return TestPass;
>> +	}
>> +
> 
> private:

Fixed.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Thanks.



> 
>> +	std::unique_ptr<CameraConfiguration> config_;
>> +	FrameBufferAllocator *allocator_;
>> +	Stream *stream_;
>> +};
>> +
>> +} /* namespace */
>> +
>> +TEST_REGISTER(MappedBuffers);
>> diff --git a/test/meson.build b/test/meson.build
>> index 775187159dec..376ee6cee175 100644
>> --- a/test/meson.build
>> +++ b/test/meson.build
>> @@ -31,6 +31,7 @@ internal_tests = [
>>      ['file',                            'file.cpp'],
>>      ['file-descriptor',                 'file-descriptor.cpp'],
>>      ['hotplug-cameras',                 'hotplug-cameras.cpp'],
>> +    ['mapped-buffers',                  'mapped-buffers.cpp'],
>>      ['message',                         'message.cpp'],
>>      ['object',                          'object.cpp'],
>>      ['object-delete',                   'object-delete.cpp'],
> 

-- 
Regards
--
Kieran