[libcamera-devel] [PATCH v2 8/8] test: v4l2_device: Provide buffer sharing test
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Feb 13 17:19:08 CET 2019
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 at 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
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list