[libcamera-devel] [PATCH v2 01/24] test: Extract CameraTest class out of camera tests to libtest
Jacopo Mondi
jacopo at jmondi.org
Fri Nov 15 16:51:18 CET 2019
HI Laurent,
On Thu, Nov 14, 2019 at 09:07:52AM +0100, Niklas Söderlund wrote:
> Hi Laurent,
>
> Thanks for your work.
>
> On 2019-11-08 22:53:46 +0200, Laurent Pinchart wrote:
> > Many tests other than the camera/ tests use a camera. To increase code
> > sharing, move the base CameraTest class to the test library. The class
> > becomes a helper that doesn't inherit from Test anymore (to avoid
> > diamond inheritance issues when more such helpers will exist).
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> I'm not overly found of how status_ is checked in users, but I like the
> overall change more and this is test code.
I agree this requires to be handled in the sub-classes' init() and
it's not super nice
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > ---
> > test/camera/buffer_import.cpp | 14 ++++-----
> > test/camera/capture.cpp | 15 ++++++---
> > test/camera/configuration_default.cpp | 16 ++++++++--
> > test/camera/configuration_set.cpp | 15 ++++++---
> > test/camera/meson.build | 2 +-
> > test/camera/statemachine.cpp | 15 ++++++---
> > test/controls/control_list.cpp | 39 +++++-------------------
> > test/{camera => libtest}/camera_test.cpp | 24 +++++++++------
> > test/{camera => libtest}/camera_test.h | 12 +++-----
> > test/libtest/meson.build | 1 +
> > 10 files changed, 77 insertions(+), 76 deletions(-)
> > rename test/{camera => libtest}/camera_test.cpp (55%)
> > rename test/{camera => libtest}/camera_test.h (77%)
> >
> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > index bbc5a25c4019..3efe02704c02 100644
> > --- a/test/camera/buffer_import.cpp
> > +++ b/test/camera/buffer_import.cpp
> > @@ -18,6 +18,7 @@
> > #include "v4l2_videodevice.h"
> >
> > #include "camera_test.h"
> > +#include "test.h"
> >
> > using namespace libcamera;
> >
> > @@ -254,11 +255,11 @@ private:
> > bool done_;
> > };
> >
> > -class BufferImportTest : public CameraTest
> > +class BufferImportTest : public CameraTest, public Test
> > {
> > public:
> > BufferImportTest()
> > - : CameraTest()
> > + : CameraTest("VIMC Sensor B")
> > {
> > }
> >
> > @@ -350,11 +351,10 @@ protected:
> >
> > int init()
> > {
> > - int ret = CameraTest::init();
> > - if (ret)
> > - return ret;
> > + if (status_ != TestPass)
> > + return status_;
> >
> > - ret = sink_.init();
> > + int ret = sink_.init();
> > if (ret != TestPass) {
> > cleanup();
> > return ret;
> > @@ -422,8 +422,6 @@ protected:
> >
> > camera_->stop();
> > camera_->freeBuffers();
> > -
> > - CameraTest::cleanup();
> > }
> >
> > private:
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index 791ccad15f70..08cce9c7cbaf 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -8,13 +8,20 @@
> > #include <iostream>
> >
> > #include "camera_test.h"
> > +#include "test.h"
> >
> > using namespace std;
> >
> > namespace {
> >
> > -class Capture : public CameraTest
> > +class Capture : public CameraTest, public Test
> > {
> > +public:
> > + Capture()
> > + : CameraTest("VIMC Sensor B")
> > + {
> > + }
> > +
> > protected:
> > unsigned int completeBuffersCount_;
> > unsigned int completeRequestsCount_;
> > @@ -46,14 +53,12 @@ protected:
> >
> > int init() override
> > {
> > - int ret = CameraTest::init();
> > - if (ret)
> > - return ret;
> > + if (status_ != TestPass)
> > + return status_;
> >
> > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > if (!config_ || config_->size() != 1) {
> > cout << "Failed to generate default configuration" << endl;
> > - CameraTest::cleanup();
> > return TestFail;
> > }
> >
> > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp
> > index ce2ec5d02e7b..31c908d2449e 100644
> > --- a/test/camera/configuration_default.cpp
> > +++ b/test/camera/configuration_default.cpp
> > @@ -8,15 +8,27 @@
> > #include <iostream>
> >
> > #include "camera_test.h"
> > +#include "test.h"
> >
> > using namespace std;
> >
> > namespace {
> >
> > -class ConfigurationDefault : public CameraTest
> > +class ConfigurationDefault : public CameraTest, public Test
> > {
> > +public:
> > + ConfigurationDefault()
> > + : CameraTest("VIMC Sensor B")
> > + {
> > + }
> > +
> > protected:
> > - int run()
> > + int init() override
> > + {
> > + return status_;
> > + }
> > +
> > + int run() override
> > {
> > std::unique_ptr<CameraConfiguration> config;
> >
> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> > index f88da96ca2b7..b4b5968115e8 100644
> > --- a/test/camera/configuration_set.cpp
> > +++ b/test/camera/configuration_set.cpp
> > @@ -8,24 +8,29 @@
> > #include <iostream>
> >
> > #include "camera_test.h"
> > +#include "test.h"
> >
> > using namespace std;
> >
> > namespace {
> >
> > -class ConfigurationSet : public CameraTest
> > +class ConfigurationSet : public CameraTest, public Test
> > {
> > +public:
> > + ConfigurationSet()
> > + : CameraTest("VIMC Sensor B")
> > + {
> > + }
> > +
> > protected:
> > int init() override
> > {
> > - int ret = CameraTest::init();
> > - if (ret)
> > - return ret;
> > + if (status_ != TestPass)
> > + return status_;
> >
> > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > if (!config_ || config_->size() != 1) {
> > cout << "Failed to generate default configuration" << endl;
> > - CameraTest::cleanup();
> > return TestFail;
> > }
> >
> > diff --git a/test/camera/meson.build b/test/camera/meson.build
> > index d6fd66b8f89e..e2a6660a7a92 100644
> > --- a/test/camera/meson.build
> > +++ b/test/camera/meson.build
> > @@ -9,7 +9,7 @@ camera_tests = [
> > ]
> >
> > foreach t : camera_tests
> > - exe = executable(t[0], [t[1], 'camera_test.cpp'],
> > + exe = executable(t[0], t[1],
> > dependencies : libcamera_dep,
> > link_with : test_libraries,
> > include_directories : test_includes_internal)
> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > index 12d5e0e1d534..afa13ba77b0b 100644
> > --- a/test/camera/statemachine.cpp
> > +++ b/test/camera/statemachine.cpp
> > @@ -8,13 +8,20 @@
> > #include <iostream>
> >
> > #include "camera_test.h"
> > +#include "test.h"
> >
> > using namespace std;
> >
> > namespace {
> >
> > -class Statemachine : public CameraTest
> > +class Statemachine : public CameraTest, public Test
> > {
> > +public:
> > + Statemachine()
> > + : CameraTest("VIMC Sensor B")
> > + {
> > + }
> > +
> > protected:
> > int testAvailable()
> > {
> > @@ -233,14 +240,12 @@ protected:
> >
> > int init() override
> > {
> > - int ret = CameraTest::init();
> > - if (ret)
> > - return ret;
> > + if (status_ != TestPass)
> > + return status_;
> >
> > defconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > if (!defconf_) {
> > cout << "Failed to generate default configuration" << endl;
> > - CameraTest::cleanup();
> > return TestFail;
> > }
> >
> > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp
> > index 5af53f64bb6c..4d212abd09e6 100644
> > --- a/test/controls/control_list.cpp
> > +++ b/test/controls/control_list.cpp
> > @@ -13,32 +13,22 @@
> > #include <libcamera/controls.h>
> >
> > #include "camera_controls.h"
> > +
> > +#include "camera_test.h"
> > #include "test.h"
> >
> > using namespace std;
> > using namespace libcamera;
> >
> > -class ControlListTest : public Test
> > +class ControlListTest : public CameraTest, public Test
> > {
> > -protected:
> > - int init()
> > +public:
> > + ControlListTest()
> > + : CameraTest("VIMC Sensor B")
> > {
> > - cm_ = new CameraManager();
> > -
> > - if (cm_->start()) {
> > - cout << "Failed to start camera manager" << endl;
> > - return TestFail;
> > - }
> > -
> > - camera_ = cm_->get("VIMC Sensor B");
> > - if (!camera_) {
> > - cout << "Can not find VIMC camera" << endl;
> > - return TestSkip;
> > - }
> > -
> > - return TestPass;
> > }
> >
> > +protected:
Don't we need to check status_ in init() ?
Apart from this:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> > int run()
> > {
> > CameraControlValidator validator(camera_.get());
> > @@ -156,21 +146,6 @@ protected:
> >
> > return TestPass;
> > }
> > -
> > - void cleanup()
> > - {
> > - if (camera_) {
> > - camera_->release();
> > - camera_.reset();
> > - }
> > -
> > - cm_->stop();
> > - delete cm_;
> > - }
> > -
> > -private:
> > - CameraManager *cm_;
> > - std::shared_ptr<Camera> camera_;
> > };
> >
> > TEST_REGISTER(ControlListTest)
> > diff --git a/test/camera/camera_test.cpp b/test/libtest/camera_test.cpp
> > similarity index 55%
> > rename from test/camera/camera_test.cpp
> > rename to test/libtest/camera_test.cpp
> > index 101e31fbce79..2ae4d6776f2e 100644
> > --- a/test/camera/camera_test.cpp
> > +++ b/test/libtest/camera_test.cpp
> > @@ -8,35 +8,39 @@
> > #include <iostream>
> >
> > #include "camera_test.h"
> > +#include "test.h"
> >
> > using namespace libcamera;
> > using namespace std;
> >
> > -int CameraTest::init()
> > +CameraTest::CameraTest(const char *name)
> > {
> > cm_ = new CameraManager();
> >
> > if (cm_->start()) {
> > - cout << "Failed to start camera manager" << endl;
> > - return TestFail;
> > + cerr << "Failed to start camera manager" << endl;
> > + status_ = TestFail;
> > + return;
> > }
> >
> > - camera_ = cm_->get("VIMC Sensor B");
> > + camera_ = cm_->get(name);
> > if (!camera_) {
> > - cout << "Can not find VIMC camera" << endl;
> > - return TestSkip;
> > + cerr << "Can not find '" << name << "' camera" << endl;
> > + status_ = TestSkip;
> > + return;
> > }
> >
> > /* Sanity check that the camera has streams. */
> > if (camera_->streams().empty()) {
> > - cout << "Camera has no stream" << endl;
> > - return TestFail;
> > + cerr << "Camera has no stream" << endl;
> > + status_ = TestFail;
> > + return;
> > }
> >
> > - return TestPass;
> > + status_ = TestPass;
> > }
> >
> > -void CameraTest::cleanup()
> > +CameraTest::~CameraTest()
> > {
> > if (camera_) {
> > camera_->release();
> > diff --git a/test/camera/camera_test.h b/test/libtest/camera_test.h
> > similarity index 77%
> > rename from test/camera/camera_test.h
> > rename to test/libtest/camera_test.h
> > index e57b05eb28a9..0b6bad05e37c 100644
> > --- a/test/camera/camera_test.h
> > +++ b/test/libtest/camera_test.h
> > @@ -9,22 +9,18 @@
> >
> > #include <libcamera/libcamera.h>
> >
> > -#include "test.h"
> > -
> > using namespace libcamera;
> >
> > -class CameraTest : public Test
> > +class CameraTest
> > {
> > public:
> > - CameraTest()
> > - : cm_(nullptr) {}
> > + CameraTest(const char *name);
> > + ~CameraTest();
> >
> > protected:
> > - int init();
> > - void cleanup();
> > -
> > CameraManager *cm_;
> > std::shared_ptr<Camera> camera_;
> > + int status_;
> > };
> >
> > #endif /* __LIBCAMERA_CAMERA_TEST_H__ */
> > diff --git a/test/libtest/meson.build b/test/libtest/meson.build
> > index ca762b4421c2..3e798ef3810e 100644
> > --- a/test/libtest/meson.build
> > +++ b/test/libtest/meson.build
> > @@ -1,4 +1,5 @@
> > libtest_sources = files([
> > + 'camera_test.cpp',
> > 'test.cpp',
> > ])
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191115/dafbeba2/attachment.sig>
More information about the libcamera-devel
mailing list