[libcamera-devel] [PATCH v2 05/11] test: media_device: Create a common MediaDeviceTest base class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri May 17 01:51:52 CEST 2019
Hi Laurent,
Thanks for your feedback.
On 2019-05-11 15:46:21 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Wed, May 08, 2019 at 05:18:25PM +0200, Niklas Söderlund wrote:
> > Before adding more tests which will act on the vimc pipeline break out a
> > common base from media_device_link_test.cpp which already acts on vimc.
> > The new common base class will help reduce code duplication.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > test/media_device/media_device_link_test.cpp | 73 +++++---------------
> > test/media_device/media_device_test.cpp | 41 +++++++++++
> > test/media_device/media_device_test.h | 35 ++++++++++
> > test/media_device/meson.build | 2 +-
> > 4 files changed, 96 insertions(+), 55 deletions(-)
> > create mode 100644 test/media_device/media_device_test.cpp
> > create mode 100644 test/media_device/media_device_test.h
> >
> > diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp
> > index 334bb44a6fc14371..26aedf0ea23ec709 100644
> > --- a/test/media_device/media_device_link_test.cpp
> > +++ b/test/media_device/media_device_link_test.cpp
> > @@ -4,13 +4,10 @@
> > *
> > * media_device_link_test.cpp - Tests link handling on VIMC media device
> > */
> > +
> > #include <iostream>
> > -#include <memory>
> >
> > -#include "device_enumerator.h"
> > -#include "media_device.h"
> > -
> > -#include "test.h"
> > +#include "media_device_test.h"
> >
> > using namespace libcamera;
> > using namespace std;
> > @@ -29,44 +26,21 @@ using namespace std;
> > * loaded) the test is skipped.
> > */
> >
> > -class MediaDeviceLinkTest : public Test
> > +class MediaDeviceLinkTest : public MediaDeviceTest
> > {
> > - int init()
> > - {
> > - enumerator = unique_ptr<DeviceEnumerator>(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("vimc");
> > - dev_ = enumerator->search(dm);
> > - if (!dev_) {
> > - cerr << "No VIMC media device found: skip test" << endl;
> > - return TestSkip;
> > - }
> > -
> > - if (!dev_->acquire()) {
> > - cerr << "Unable to acquire media device "
> > - << dev_->deviceNode() << endl;
> > - return TestSkip;
> > - }
> > -
> > - return 0;
> > - }
> > -
> > int run()
> > {
> > + if (!media_->acquire()) {
> > + cerr << "Unable to acquire media device "
> > + << media_->deviceNode() << endl;
> > + return TestFail;
> > + }
>
> Shouldn't this go to he init() method of the base class, given that the
> release() call is in cleanup() ?
Wops, this is indeed wrong. However the release() should be moved to the
MediaDeviceLinkTest cleanup() function, and not the base class
MediaDeviceTest. Reason for this is that there are tests which we wish
to run without the media device acquired, so IMHO it's best for those
tests which needs to acquire it to do so themself.
Will fix for next version.
>
> > +
> > /*
> > * First of all disable all links in the media graph to
> > * ensure we start from a known state.
> > */
> > - if (dev_->disableLinks()) {
> > + if (media_->disableLinks()) {
> > cerr << "Failed to disable all links in the media graph";
> > return TestFail;
> > }
> > @@ -76,26 +50,26 @@ class MediaDeviceLinkTest : public Test
> > * different methods the media device offers.
> > */
> > string linkName("'Debayer A':[1] -> 'Scaler':[0]'");
> > - MediaLink *link = dev_->link("Debayer A", 1, "Scaler", 0);
> > + MediaLink *link = media_->link("Debayer A", 1, "Scaler", 0);
> > if (!link) {
> > cerr << "Unable to find link: " << linkName
> > << " using lookup by name" << endl;
> > return TestFail;
> > }
> >
> > - MediaEntity *source = dev_->getEntityByName("Debayer A");
> > + MediaEntity *source = media_->getEntityByName("Debayer A");
> > if (!source) {
> > cerr << "Unable to find entity: 'Debayer A'" << endl;
> > return TestFail;
> > }
> >
> > - MediaEntity *sink = dev_->getEntityByName("Scaler");
> > + MediaEntity *sink = media_->getEntityByName("Scaler");
> > if (!sink) {
> > cerr << "Unable to find entity: 'Scaler'" << endl;
> > return TestFail;
> > }
> >
> > - MediaLink *link2 = dev_->link(source, 1, sink, 0);
> > + MediaLink *link2 = media_->link(source, 1, sink, 0);
> > if (!link2) {
> > cerr << "Unable to find link: " << linkName
> > << " using lookup by entity" << endl;
> > @@ -108,7 +82,7 @@ class MediaDeviceLinkTest : public Test
> > return TestFail;
> > }
> >
> > - link2 = dev_->link(source->getPadByIndex(1),
> > + link2 = media_->link(source->getPadByIndex(1),
> > sink->getPadByIndex(0));
> > if (!link2) {
> > cerr << "Unable to find link: " << linkName
> > @@ -160,7 +134,7 @@ class MediaDeviceLinkTest : public Test
> >
> > /* Try to get a non existing link. */
> > linkName = "'Sensor A':[1] -> 'Scaler':[0]";
> > - link = dev_->link("Sensor A", 1, "Scaler", 0);
> > + link = media_->link("Sensor A", 1, "Scaler", 0);
> > if (link) {
> > cerr << "Link lookup for " << linkName
> > << " succeeded but link does not exist"
> > @@ -170,7 +144,7 @@ class MediaDeviceLinkTest : public Test
> >
> > /* Now get an immutable link and try to disable it. */
> > linkName = "'Sensor A':[0] -> 'Raw Capture 0':[0]";
> > - link = dev_->link("Sensor A", 0, "Raw Capture 0", 0);
> > + link = media_->link("Sensor A", 0, "Raw Capture 0", 0);
> > if (!link) {
> > cerr << "Unable to find link: " << linkName
> > << " using lookup by name" << endl;
> > @@ -195,7 +169,7 @@ class MediaDeviceLinkTest : public Test
> > * after disabling all links in the media graph.
> > */
> > linkName = "'Debayer B':[1] -> 'Scaler':[0]'";
> > - link = dev_->link("Debayer B", 1, "Scaler", 0);
> > + link = media_->link("Debayer B", 1, "Scaler", 0);
> > if (!link) {
> > cerr << "Unable to find link: " << linkName
> > << " using lookup by name" << endl;
> > @@ -215,7 +189,7 @@ class MediaDeviceLinkTest : public Test
> > return TestFail;
> > }
> >
> > - if (dev_->disableLinks()) {
> > + if (media_->disableLinks()) {
> > cerr << "Failed to disable all links in the media graph";
> > return TestFail;
> > }
> > @@ -229,15 +203,6 @@ class MediaDeviceLinkTest : public Test
> >
> > return 0;
> > }
> > -
> > - void cleanup()
> > - {
> > - dev_->release();
> > - }
> > -
> > -private:
> > - unique_ptr<DeviceEnumerator> enumerator;
> > - shared_ptr<MediaDevice> dev_;
> > };
> >
> > TEST_REGISTER(MediaDeviceLinkTest);
> > diff --git a/test/media_device/media_device_test.cpp b/test/media_device/media_device_test.cpp
> > new file mode 100644
> > index 0000000000000000..710613eeb80b6de1
> > --- /dev/null
> > +++ b/test/media_device/media_device_test.cpp
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * libcamera MediaDevice API tests
> > + */
> > +
> > +#include <iostream>
> > +
> > +#include "media_device_test.h"
> > +
> > +using namespace libcamera;
> > +using namespace std;
> > +
> > +int MediaDeviceTest::init()
> > +{
> > + enumerator_ = unique_ptr<DeviceEnumerator>(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("vimc");
> > + media_ = enumerator_->search(dm);
> > + if (!media_) {
> > + cerr << "No VIMC media device found: skip test" << endl;
> > + return TestSkip;
> > + }
> > +
> > + return TestPass;
> > +}
> > +
> > +void MediaDeviceTest::cleanup()
> > +{
> > + media_->release();
> > +}
> > diff --git a/test/media_device/media_device_test.h b/test/media_device/media_device_test.h
> > new file mode 100644
> > index 0000000000000000..af5c89f4fff2f63e
> > --- /dev/null
> > +++ b/test/media_device/media_device_test.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * media_device_test.h - libcamera media devie base class
>
> s/devie/device test/
>
> > + */
> > +#ifndef __LIBCAMERA_MEDIADEVICE_TEST_H__
> > +#define __LIBCAMERA_MEDIADEVICE_TEST_H__
> > +
> > +#include <memory>
> > +
> > +#include "device_enumerator.h"
> > +#include "media_device.h"
> > +
> > +#include "test.h"
> > +
> > +using namespace libcamera;
> > +
> > +class MediaDeviceTest : public Test
> > +{
> > +public:
> > + MediaDeviceTest()
> > + : media_(nullptr), enumerator_(nullptr) {}
> > +
> > +protected:
> > + int init();
> > + void cleanup();
> > +
> > + std::shared_ptr<MediaDevice> media_;
> > +
> > +private:
> > + std::unique_ptr<DeviceEnumerator> enumerator_;
> > +};
> > +
> > +#endif /* __LIBCAMERA_MEDIADEVICE_TEST_H__ */
> > diff --git a/test/media_device/meson.build b/test/media_device/meson.build
> > index d91a022fab190abf..1005685409d99aa7 100644
> > --- a/test/media_device/meson.build
> > +++ b/test/media_device/meson.build
> > @@ -4,7 +4,7 @@ media_device_tests = [
> > ]
> >
> > foreach t : media_device_tests
> > - exe = executable(t[0], t[1],
> > + exe = executable(t[0], [t[1], 'media_device_test.cpp'],
>
> This is a bit of a hack. How about creating a static library for this
> file, the same way we do with libtest ? It can live in this directory,
> there's no need for a subdirectory or a separate meson.build file.
>
> Alternatively you could include media_device_test.cpp in libtest, but
> that seems to be a bit of a hack too.
Good point, will fix for next version.
>
> > link_with : test_libraries,
> > include_directories : test_includes_internal)
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list