[libcamera-devel] [PATCH 4/6] test: v4l2_videodevice: Add M2M device test
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 9 20:18:04 CEST 2019
Hi Kieran,
On Fri, Aug 09, 2019 at 11:32:15AM +0100, Kieran Bingham wrote:
> On 08/08/2019 22:26, Laurent Pinchart wrote:
> > On Thu, Aug 08, 2019 at 04:12:19PM +0100, Kieran Bingham wrote:
> >> The V4L2M2MDevice requires two pipelines to be configured. This makes it unsuitable
> >> to reuse the existing V4L2DeviceTest test library in it's current form.
> >
> > s/it's/its/
> >
> > Commit messages should wrap at 72 columns.
> >
> >> Implement a full test to run the two M2M pipelines through VIM2M.
> >
> > Lovely, another driver for our test suite :-)
>
> Indeed! So many software devices to test :D
>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> test/v4l2_videodevice/meson.build | 1 +
> >> test/v4l2_videodevice/v4l2_m2mdevice.cpp | 210 +++++++++++++++++++++++
> >> 2 files changed, 211 insertions(+)
> >> create mode 100644 test/v4l2_videodevice/v4l2_m2mdevice.cpp
> >>
> >> diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> >> index 76be5e142bb6..ad41898b5f8b 100644
> >> --- a/test/v4l2_videodevice/meson.build
> >> +++ b/test/v4l2_videodevice/meson.build
> >> @@ -7,6 +7,7 @@ v4l2_videodevice_tests = [
> >> [ 'stream_on_off', 'stream_on_off.cpp' ],
> >> [ 'capture_async', 'capture_async.cpp' ],
> >> [ 'buffer_sharing', 'buffer_sharing.cpp' ],
> >> + [ 'v4l2_m2mdevice', 'v4l2_m2mdevice.cpp' ],
> >> ]
> >>
> >> foreach t : v4l2_videodevice_tests
> >> diff --git a/test/v4l2_videodevice/v4l2_m2mdevice.cpp b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> >> new file mode 100644
> >> index 000000000000..7a730f695ab7
> >> --- /dev/null
> >> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> >> @@ -0,0 +1,210 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * libcamera V4L2 API tests
> >
> > Copy & paste ?
>
> Of course ;D - I'm not writing all this from scratch hehe.
>
> >> + */
> >> +
> >> +#include <libcamera/buffer.h>
> >> +#include <libcamera/camera_manager.h>
> >> +#include <libcamera/event_dispatcher.h>
> >> +#include <libcamera/timer.h>
> >> +
> >> +#include <iostream>
> >> +#include <memory>
> >> +
> >> +#include "device_enumerator.h"
> >> +#include "media_device.h"
> >> +#include "v4l2_videodevice.h"
> >> +
> >> +#include "test.h"
> >> +
> >> +using namespace std;
> >> +using namespace libcamera;
> >> +
> >> +class V4L2M2MDeviceTest : public Test
> >> +{
> >> +public:
> >> + V4L2M2MDeviceTest()
> >> + : vim2m_(nullptr), outputFrames_(0), captureFrames_(0)
> >> + {
> >> + }
> >> +
> >> + void outputBufferComplete(Buffer *buffer)
> >> + {
> >> + std::cout << "Received output buffer " << buffer->index()
> >> + << std::endl;
> >
> > My preference goes with using the std:: prefix explicitly like here, in
> > which case you should use it everywhere and drop the using namespace std
> > statement. The alternative is to remove it everywhere.
>
> I'd prefer shorter lines and removing it.
>
> But really we just need some better test logging.
>
> >> +
> >> + outputFrames_++;
> >> +
> >> + /* Requeue the buffer for further use. */
> >> + vim2m_->output()->queueBuffer(buffer);
> >> + }
> >> +
> >> + void receiveCaptureBuffer(Buffer *buffer)
> >> + {
> >> + std::cout << "Received capture buffer " << buffer->index()
> >> + << std::endl;
> >> +
> >> + captureFrames_++;
> >> +
> >> + /* Requeue the buffer for further use. */
> >> + vim2m_->capture()->queueBuffer(buffer);
> >> + }
> >> +
> >> +protected:
> >> + int init()
> >> + {
> >> + enumerator_ = DeviceEnumerator::create();
> >> + if (!enumerator_) {
> >> + cerr << "Failed to create device enumerator" << endl;
> >> + return TestFail;
> >> + }
> >> +
> >> + if (enumerator_->enumerate()) {
> >> + cerr << "Failed to enumerate media devices" << endl;
> >> + return TestFail;
> >> + }
> >> +
> >> + DeviceMatch dm("vim2m");
> >> + dm.add("vim2m-source");
> >> + dm.add("vim2m-sink");
> >> +
> >> + media_ = enumerator_->search(dm);
> >> + if (!media_) {
> >> + cerr << "Failed to match device" << endl;
> >
> > Maybe "No vim2m device found" ?
>
> Updated.
>
> >> + return TestSkip;
> >> + }
> >> +
> >> + MediaEntity *entity = media_->getEntityByName("vim2m-source");
> >> + if (!entity) {
> >> + cerr << "Failed to get device entity" << endl;
> >
> > This can't happen due to the dm.add().
>
> Removed.
>
> >> + return TestSkip;
> >> + }
> >> +
> >
> > I would move the rest of the code to the run() function as it tests the
> > V4L2M2MDevice API.
>
> Done.
>
> >> + vim2m_ = new V4L2M2MDevice(entity->deviceNode());
> >> + if (vim2m_->status())
> >
> > You should add a message here (and below). It's difficult to debug test
> > failures when no message is printed.
>
> Maybe we should revisit the TestStatus patches I proposed.
> Then it would be
>
> return TestFail("Failed to open VIM2M device")
>
> I recall dropping the series because you didn't seem to like the concept.
I think it's a good idea, but I also think it should be part of a
logging infrastructure for tests. We shouldn't design the two parts
separately.
> >> + return TestFail;
> >> +
> >> + V4L2DeviceFormat format = {};
> >> + if (vim2m_->capture()->getFormat(&format))
> >> + return TestFail;
> >> +
> >> + format.size.width = 640;
> >> + format.size.height = 480;
> >> +
> >> + if (vim2m_->capture()->setFormat(&format))
> >> + return TestFail;
> >> +
> >> + if (vim2m_->output()->setFormat(&format))
> >> + return TestFail;
> >> +
> >> + cerr << "Initialised M2M ..." << endl;
> >
> > I'd drop this line.
>
> Dropped.
>
> >> +
> >> + return TestPass;
> >> + }
> >> +
> >> + int run()
> >> + {
> >> + const unsigned int bufferCount = 8;
> >
> > s/const/constexpr/
> >
> > Would 4 buffers be enough ?
>
> Sure.
>
> >> +
> >> + EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> >> + Timer timeout;
> >> + int ret;
> >> +
> >> + capturePool_.createBuffers(bufferCount);
> >> + outputPool_.createBuffers(bufferCount);
> >> +
> >> + ret = vim2m_->capture()->exportBuffers(&capturePool_);
> >> + if (ret) {
> >> + cerr << "Failed to export Capture Buffers" << endl;
> >> + return TestFail;
> >> + }
> >> +
> >> + ret = vim2m_->output()->exportBuffers(&outputPool_);
> >> + if (ret) {
> >> + cerr << "Failed to export Output Buffers" << endl;
> >> + return TestFail;
> >> + }
> >
> > I would store the capture and output devices to local variables to
> > shorten the lines.
>
> Sure.
>
> >> +
> >> + vim2m_->capture()->bufferReady.connect(this, &V4L2M2MDeviceTest::receiveCaptureBuffer);
> >> + vim2m_->output()->bufferReady.connect(this, &V4L2M2MDeviceTest::outputBufferComplete);
> >> +
> >> + /* We can't "queueAllBuffers()" on an output device, so we do it manually */
> >> + std::vector<std::unique_ptr<Buffer>> outputBuffers;
> >> + for (unsigned int i = 0; i < outputPool_.count(); ++i) {
> >> + Buffer *buffer = new Buffer(i);
> >> + outputBuffers.emplace_back(buffer);
> >> + ret = vim2m_->output()->queueBuffer(buffer);
> >> + if (ret)
> >> + return {};
> >> + }
> >> +
> >> + std::vector<std::unique_ptr<Buffer>> captureBuffers;
> >> + captureBuffers = vim2m_->capture()->queueAllBuffers();
> >> + if (captureBuffers.empty()) {
> >> + cerr << "Failed to queue all Capture Buffers" << endl;
> >> + return TestFail;
> >> + }
> >
> > Even if it makes little difference in practice, I would queue the
> > buffers on the capture side first,
> >
> >> +
> >> + ret = vim2m_->output()->streamOn();
> >> + if (ret) {
> >> + cerr << "Failed to streamOn output" << endl;
> >> + return TestFail;
> >> + }
> >> +
> >> + ret = vim2m_->capture()->streamOn();
> >> + if (ret) {
> >> + cerr << "Failed to streamOn capture" << endl;
> >> + return TestFail;
> >> + }
> >> +
> >> + timeout.start(10000);
> >> + while (timeout.isRunning()) {
> >> + dispatcher->processEvents();
> >> + if (captureFrames_ > 30)
> >> + break;
> >
> > How long does it take in practice to capture 30 frames ? Can we reduce
> > the timeout ?
>
> On my laptop:
>
> 27/37 libcamera:v4l2_videodevice / v4l2_m2mdevice OK 1.47 s
>
> On a RaspberryPi 3:
>
> 27/37 libcamera:v4l2_videodevice / v4l2_m2mdevice OK 1.64 s
>
> I'll drop to 5 seconds timeout. It's only going to happen in the event
> of a pipeline stall which is unlikely.
Seems good to me.
> >> + }
> >> +
> >> + if (captureFrames_ < 1) {
> >> + std::cout << "Failed to capture any frames within timeout." << std::endl;
> >
> > s/timeout\./timeout/
> > Line wrap.
> >
> >> + return TestFail;
> >> + }
> >> +
> >> + if (captureFrames_ < 30) {
> >> + std::cout << "Failed to capture 30 frames within timeout." << std::endl;
> >
> > Here too.
> >
> >> + return TestFail;
> >> + }
> >
> > You could merge the two checks and print the number of captured frames.
>
> Done.
>
> >> +
> >> + std::cout << "Output " << outputFrames_ << " frames" << std::endl;
> >> + std::cout << "Captured " << captureFrames_ << " frames" << std::endl;
> >> +
> >> + ret = vim2m_->capture()->streamOff();
> >> + if (ret)
> >
> > Error messages please.
>
> Done
>
> >> + return TestFail;
> >> +
> >> + ret = vim2m_->output()->streamOff();
> >> + if (ret)
> >> + return TestFail;
> >> +
> >> + return TestPass;
> >> + }
> >> +
> >> + void cleanup()
> >> + {
> >> + delete vim2m_;
> >> + };
> >> +
> >> +private:
> >> + std::unique_ptr<DeviceEnumerator> enumerator_;
> >> + std::shared_ptr<MediaDevice> media_;
> >> + V4L2M2MDevice *vim2m_;
> >> +
> >> + BufferPool capturePool_;
> >> + BufferPool outputPool_;
> >> +
> >> + unsigned int outputFrames_;
> >> + unsigned int captureFrames_;
> >> +};
> >> +
> >> +TEST_REGISTER(V4L2M2MDeviceTest);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list