| Message ID | 20191108205409.18845-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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. Reviewed-by: Niklas Söderlund <niklas.soderlund@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: > 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Fri, Nov 15, 2019 at 04:51:18PM +0100, Jacopo Mondi wrote: > On Thu, Nov 14, 2019 at 09:07:52AM +0100, Niklas Söderlund wrote: > > 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@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 I don't like it much either, I think we'll need to refactor the tests in the not too distant future, and this is something I believe we should address then. > > Reviewed-by: Niklas Söderlund <niklas.soderlund@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@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', > > > ]) > > >
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: 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', ])
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@ideasonboard.com> --- 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%)