| Message ID | 20190306024755.28726-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Wed, Mar 06, 2019 at 03:47:52AM +0100, Niklas Söderlund wrote: > Add a test to verify reading the default format from a camera works. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > test/camera/camera_test.cpp | 47 ++++++++++++++++++++++ > test/camera/camera_test.h | 32 +++++++++++++++ > test/camera/format_default.cpp | 71 ++++++++++++++++++++++++++++++++++ > test/camera/meson.build | 12 ++++++ > test/meson.build | 1 + > 5 files changed, 163 insertions(+) > create mode 100644 test/camera/camera_test.cpp > create mode 100644 test/camera/camera_test.h > create mode 100644 test/camera/format_default.cpp > create mode 100644 test/camera/meson.build > > diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp > new file mode 100644 > index 0000000000000000..d39a0ed066665946 > --- /dev/null > +++ b/test/camera/camera_test.cpp > @@ -0,0 +1,47 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * libcamera Camera API tests > + */ > + > +#include <iostream> > + > +#include "camera_test.h" > + > +using namespace std; > +using namespace libcamera; Alphabetically sorted please. > + > +int CameraTest::init() > +{ > + cm_ = CameraManager::instance(); > + > + 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, skip." << endl; > + return TestSkip; > + } > + > + /* Sanity check that camera have streams. */ s/camera have/the camera has/ > + if (camera_->streams().size() == 0) { empty() instead of size() == 0 ? > + cout << "Camera have no streams, fail." << endl; s/have no streams/has no stream/ Do we need the "fail" suffix ? Same for the skip suffix in the previous message. > + return TestFail; > + } > + > + return TestPass; > +} > + > +void CameraTest::cleanup() > +{ > + if (camera_) { > + camera_->release(); > + camera_.reset(); Is the reset needed ? > + } > + > + cm_->stop(); > +}; > diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h > new file mode 100644 > index 0000000000000000..4aadd027675a5dbd > --- /dev/null > +++ b/test/camera/camera_test.h > @@ -0,0 +1,32 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * camera_test.h - libcamera camera test base class > + */ > +#ifndef __LIBCAMERA_CAMERA_TEST_H_ > +#define __LIBCAMERA_CAMERA_TEST_H_ Double underscore at the end ? Same for the #endif down below. > + > +#include <libcamera/libcamera.h> > + > +#include "test.h" > + > +using namespace libcamera; > + > +class CameraTest : public Test > +{ > +public: > + CameraTest() > + : cm_(nullptr){}; Space before {} and no need for ; > + > +protected: > + int init(); > + void cleanup(); > + > + std::shared_ptr<Camera> camera_; > + > +private: > + CameraManager *cm_; > +}; > + > +#endif /* __LIBCAMERA_CAMERA_TEST_H_ */ > diff --git a/test/camera/format_default.cpp b/test/camera/format_default.cpp > new file mode 100644 > index 0000000000000000..11b947f44c39b40a > --- /dev/null > +++ b/test/camera/format_default.cpp > @@ -0,0 +1,71 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * libcamera Camera API tests > + */ > + > +#include <iostream> > + > +#include "camera_test.h" > + > +using namespace std; > + > +namespace { > + > +class FormatDefault : public CameraTest > +{ > +protected: > + int run() > + { > + std::map<Stream *, StreamConfiguration> conf; > + std::set<Stream *> streams = { *camera_->streams().begin() }; > + > + /* > + * Test that asking for default format for a valid array of > + * streams returns formats and that the formats are somewhat > + * sane. It's not just a default format, but a default configuration. Could you rename that through the whole patch ? > + */ > + conf = camera_->streamConfiguration(streams); > + if (!conf.size()) { .empty() ? > + cout << "Default format for valid streams test failed" << endl; > + return TestFail; > + } > + > + StreamConfiguration *sconf = &conf.begin()->second; > + if (sconf->width == 0 || sconf->height == 0 || > + sconf->pixelFormat == 0 || sconf->bufferCount == 0) { > + cout << "Default format is set test failed" << endl; > + return TestFail; > + } Should you also test that conf contains a single element, with the same stream as the one passed in streams ? You could make that generic for any number of streams, and extract the code to a separate method moved to the CameraTest class to make it reusable by multi-streams tests. > + > + /* > + * Test that asking for format for an empty array of streams > + * returns an empty list of configurations. > + */ > + std::set<Stream *> streams_empty = {}; You can reuse the streams variable declared above. > + conf = camera_->streamConfiguration(streams_empty); > + if (conf.size()) { !.empty() > + cout << "Default format for empty streams test failed" << endl; > + return TestFail; > + } > + > + /* > + * Test that asking for format for an array of streams bad streams > + * returns an empty list of configurations. > + */ > + Stream *stream_bad = *streams.end(); Ideally you should pass a stream pointer for another camera instance. Otherwise you can just pass a random value. *streams.end() isn't a good idea, as according to the std::set documentation "This element acts as a placeholder; attempting to access it results in undefined behavior". > + std::set<Stream *> streams_bad = { stream_bad }; > + conf = camera_->streamConfiguration(streams_bad); > + if (conf.size()) { !.empty() > + cout << "Default format for bad streams test failed" << endl; > + return TestFail; > + } > + > + return TestPass; > + } > +}; > + > +} /* namespace */ > + > +TEST_REGISTER(FormatDefault); > diff --git a/test/camera/meson.build b/test/camera/meson.build > new file mode 100644 > index 0000000000000000..4f2ed244a9240512 > --- /dev/null > +++ b/test/camera/meson.build > @@ -0,0 +1,12 @@ > +# Tests are listed in order of complexity. > +# They are not alphabetically sorted. > +camera_tests = [ > + [ 'format_default', 'format_default.cpp' ], > +] > + > +foreach t : camera_tests > + exe = executable(t[0], [t[1], 'camera_test.cpp'], > + link_with : test_libraries, > + include_directories : test_includes_internal) > + test(t[0], exe, suite: 'camera', is_parallel: false) > +endforeach > diff --git a/test/meson.build b/test/meson.build > index 5fb16fa6afb62f8d..71a96921697c0e9e 100644 > --- a/test/meson.build > +++ b/test/meson.build > @@ -1,5 +1,6 @@ > subdir('libtest') > > +subdir('camera') > subdir('media_device') > subdir('pipeline') > subdir('v4l2_device')
Hi Laurent, Thanks for your feedback. On 2019-03-10 15:26:32 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Wed, Mar 06, 2019 at 03:47:52AM +0100, Niklas Söderlund wrote: > > Add a test to verify reading the default format from a camera works. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > test/camera/camera_test.cpp | 47 ++++++++++++++++++++++ > > test/camera/camera_test.h | 32 +++++++++++++++ > > test/camera/format_default.cpp | 71 ++++++++++++++++++++++++++++++++++ > > test/camera/meson.build | 12 ++++++ > > test/meson.build | 1 + > > 5 files changed, 163 insertions(+) > > create mode 100644 test/camera/camera_test.cpp > > create mode 100644 test/camera/camera_test.h > > create mode 100644 test/camera/format_default.cpp > > create mode 100644 test/camera/meson.build > > > > diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp > > new file mode 100644 > > index 0000000000000000..d39a0ed066665946 > > --- /dev/null > > +++ b/test/camera/camera_test.cpp > > @@ -0,0 +1,47 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * libcamera Camera API tests > > + */ > > + > > +#include <iostream> > > + > > +#include "camera_test.h" > > + > > +using namespace std; > > +using namespace libcamera; > > Alphabetically sorted please. Thanks for spotting this. > > > + > > +int CameraTest::init() > > +{ > > + cm_ = CameraManager::instance(); > > + > > + 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, skip." << endl; > > + return TestSkip; > > + } > > + > > + /* Sanity check that camera have streams. */ > > s/camera have/the camera has/ > > > + if (camera_->streams().size() == 0) { > > empty() instead of size() == 0 ? Yes that is easier to read. > > > + cout << "Camera have no streams, fail." << endl; > > s/have no streams/has no stream/ > > Do we need the "fail" suffix ? Same for the skip suffix in the previous > message. Good point, I will drop it thru out this series. > > > + return TestFail; > > + } > > + > > + return TestPass; > > +} > > + > > +void CameraTest::cleanup() > > +{ > > + if (camera_) { > > + camera_->release(); > > + camera_.reset(); > > Is the reset needed ? No it's not, will remove for v2. > > > + } > > + > > + cm_->stop(); > > +}; > > diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h > > new file mode 100644 > > index 0000000000000000..4aadd027675a5dbd > > --- /dev/null > > +++ b/test/camera/camera_test.h > > @@ -0,0 +1,32 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * camera_test.h - libcamera camera test base class > > + */ > > +#ifndef __LIBCAMERA_CAMERA_TEST_H_ > > +#define __LIBCAMERA_CAMERA_TEST_H_ > > Double underscore at the end ? Same for the #endif down below. Good catch, thanks for noticing. > > > + > > +#include <libcamera/libcamera.h> > > + > > +#include "test.h" > > + > > +using namespace libcamera; > > + > > +class CameraTest : public Test > > +{ > > +public: > > + CameraTest() > > + : cm_(nullptr){}; > > Space before {} and no need for ; Odd that checkstyle.py did not complain about this, thanks for setting me straight. > > > + > > +protected: > > + int init(); > > + void cleanup(); > > + > > + std::shared_ptr<Camera> camera_; > > + > > +private: > > + CameraManager *cm_; > > +}; > > + > > +#endif /* __LIBCAMERA_CAMERA_TEST_H_ */ > > diff --git a/test/camera/format_default.cpp b/test/camera/format_default.cpp > > new file mode 100644 > > index 0000000000000000..11b947f44c39b40a > > --- /dev/null > > +++ b/test/camera/format_default.cpp > > @@ -0,0 +1,71 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * libcamera Camera API tests > > + */ > > + > > +#include <iostream> > > + > > +#include "camera_test.h" > > + > > +using namespace std; > > + > > +namespace { > > + > > +class FormatDefault : public CameraTest > > +{ > > +protected: > > + int run() > > + { > > + std::map<Stream *, StreamConfiguration> conf; > > + std::set<Stream *> streams = { *camera_->streams().begin() }; > > + > > + /* > > + * Test that asking for default format for a valid array of > > + * streams returns formats and that the formats are somewhat > > + * sane. > > It's not just a default format, but a default configuration. Could you > rename that through the whole patch ? Done. > > > + */ > > + conf = camera_->streamConfiguration(streams); > > + if (!conf.size()) { > > .empty() ? Better. > > > + cout << "Default format for valid streams test failed" << endl; > > + return TestFail; > > + } > > + > > + StreamConfiguration *sconf = &conf.begin()->second; > > + if (sconf->width == 0 || sconf->height == 0 || > > + sconf->pixelFormat == 0 || sconf->bufferCount == 0) { > > + cout << "Default format is set test failed" << endl; > > + return TestFail; > > + } > > Should you also test that conf contains a single element, with the same > stream as the one passed in streams ? You could make that generic for > any number of streams, and extract the code to a separate method moved > to the CameraTest class to make it reusable by multi-streams tests. Good idea, I will give it a short in v2. > > > + > > + /* > > + * Test that asking for format for an empty array of streams > > + * returns an empty list of configurations. > > + */ > > + std::set<Stream *> streams_empty = {}; > > You can reuse the streams variable declared above. I could, but I would prefer to keep one named streams_empty for clarity. If you feel strongly about it I'm willing to budge. > > > + conf = camera_->streamConfiguration(streams_empty); > > + if (conf.size()) { > > !.empty() Done. > > > + cout << "Default format for empty streams test failed" << endl; > > + return TestFail; > > + } > > + > > + /* > > + * Test that asking for format for an array of streams bad streams > > + * returns an empty list of configurations. > > + */ > > + Stream *stream_bad = *streams.end(); > > Ideally you should pass a stream pointer for another camera instance. > Otherwise you can just pass a random value. *streams.end() isn't a good > idea, as according to the std::set documentation "This element acts as a > placeholder; attempting to access it results in undefined behavior". I agree a stream from another camera would be nice, but I fear the extra requirement of finding another camera in this test case would be to much code for to little gain. I'm will go for 0xdeadbeef in v2. > > > + std::set<Stream *> streams_bad = { stream_bad }; > > + conf = camera_->streamConfiguration(streams_bad); > > + if (conf.size()) { > > !.empty() Done. > > > + cout << "Default format for bad streams test failed" << endl; > > + return TestFail; > > + } > > + > > + return TestPass; > > + } > > +}; > > + > > +} /* namespace */ > > + > > +TEST_REGISTER(FormatDefault); > > diff --git a/test/camera/meson.build b/test/camera/meson.build > > new file mode 100644 > > index 0000000000000000..4f2ed244a9240512 > > --- /dev/null > > +++ b/test/camera/meson.build > > @@ -0,0 +1,12 @@ > > +# Tests are listed in order of complexity. > > +# They are not alphabetically sorted. > > +camera_tests = [ > > + [ 'format_default', 'format_default.cpp' ], > > +] > > + > > +foreach t : camera_tests > > + exe = executable(t[0], [t[1], 'camera_test.cpp'], > > + link_with : test_libraries, > > + include_directories : test_includes_internal) > > + test(t[0], exe, suite: 'camera', is_parallel: false) > > +endforeach > > diff --git a/test/meson.build b/test/meson.build > > index 5fb16fa6afb62f8d..71a96921697c0e9e 100644 > > --- a/test/meson.build > > +++ b/test/meson.build > > @@ -1,5 +1,6 @@ > > subdir('libtest') > > > > +subdir('camera') > > subdir('media_device') > > subdir('pipeline') > > subdir('v4l2_device') > > -- > Regards, > > Laurent Pinchart
Hi again, On 2019-03-11 01:43:23 +0100, Niklas Söderlund wrote: [snip] > > > +void CameraTest::cleanup() > > > +{ > > > + if (camera_) { > > > + camera_->release(); > > > + camera_.reset(); > > > > Is the reset needed ? > > No it's not, will remove for v2. I take this back, it is needed. Without this running a test results in an error being printed ERR DeviceEnumerator device_enumerator.cpp:173 Removing media device while still in use This is because the shard_ptr design as we call CameraManager::stop() while we still hold a reference to a camera. Maybe we could improve this design somehow? For now I will keep this in v2 as it works with our current design.
Hi Niklas, On Mon, Mar 11, 2019 at 02:52:56AM +0100, Niklas Söderlund wrote: > On 2019-03-11 01:43:23 +0100, Niklas Söderlund wrote: > [snip] > >>> +void CameraTest::cleanup() > >>> +{ > >>> + if (camera_) { > >>> + camera_->release(); > >>> + camera_.reset(); > >> > >> Is the reset needed ? > > > > No it's not, will remove for v2. > > I take this back, it is needed. > > Without this running a test results in an error being printed > > ERR DeviceEnumerator device_enumerator.cpp:173 Removing media device while still in use > > This is because the shard_ptr design as we call CameraManager::stop() > while we still hold a reference to a camera. Maybe we could improve this > design somehow? For now I will keep this in v2 as it works with our > current design. I suspected that. Let's keep it then, and see if we can improve the situation when creating the camera facade object.
diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp new file mode 100644 index 0000000000000000..d39a0ed066665946 --- /dev/null +++ b/test/camera/camera_test.cpp @@ -0,0 +1,47 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * libcamera Camera API tests + */ + +#include <iostream> + +#include "camera_test.h" + +using namespace std; +using namespace libcamera; + +int CameraTest::init() +{ + cm_ = CameraManager::instance(); + + 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, skip." << endl; + return TestSkip; + } + + /* Sanity check that camera have streams. */ + if (camera_->streams().size() == 0) { + cout << "Camera have no streams, fail." << endl; + return TestFail; + } + + return TestPass; +} + +void CameraTest::cleanup() +{ + if (camera_) { + camera_->release(); + camera_.reset(); + } + + cm_->stop(); +}; diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h new file mode 100644 index 0000000000000000..4aadd027675a5dbd --- /dev/null +++ b/test/camera/camera_test.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * camera_test.h - libcamera camera test base class + */ +#ifndef __LIBCAMERA_CAMERA_TEST_H_ +#define __LIBCAMERA_CAMERA_TEST_H_ + +#include <libcamera/libcamera.h> + +#include "test.h" + +using namespace libcamera; + +class CameraTest : public Test +{ +public: + CameraTest() + : cm_(nullptr){}; + +protected: + int init(); + void cleanup(); + + std::shared_ptr<Camera> camera_; + +private: + CameraManager *cm_; +}; + +#endif /* __LIBCAMERA_CAMERA_TEST_H_ */ diff --git a/test/camera/format_default.cpp b/test/camera/format_default.cpp new file mode 100644 index 0000000000000000..11b947f44c39b40a --- /dev/null +++ b/test/camera/format_default.cpp @@ -0,0 +1,71 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * libcamera Camera API tests + */ + +#include <iostream> + +#include "camera_test.h" + +using namespace std; + +namespace { + +class FormatDefault : public CameraTest +{ +protected: + int run() + { + std::map<Stream *, StreamConfiguration> conf; + std::set<Stream *> streams = { *camera_->streams().begin() }; + + /* + * Test that asking for default format for a valid array of + * streams returns formats and that the formats are somewhat + * sane. + */ + conf = camera_->streamConfiguration(streams); + if (!conf.size()) { + cout << "Default format for valid streams test failed" << endl; + return TestFail; + } + + StreamConfiguration *sconf = &conf.begin()->second; + if (sconf->width == 0 || sconf->height == 0 || + sconf->pixelFormat == 0 || sconf->bufferCount == 0) { + cout << "Default format is set test failed" << endl; + return TestFail; + } + + /* + * Test that asking for format for an empty array of streams + * returns an empty list of configurations. + */ + std::set<Stream *> streams_empty = {}; + conf = camera_->streamConfiguration(streams_empty); + if (conf.size()) { + cout << "Default format for empty streams test failed" << endl; + return TestFail; + } + + /* + * Test that asking for format for an array of streams bad streams + * returns an empty list of configurations. + */ + Stream *stream_bad = *streams.end(); + std::set<Stream *> streams_bad = { stream_bad }; + conf = camera_->streamConfiguration(streams_bad); + if (conf.size()) { + cout << "Default format for bad streams test failed" << endl; + return TestFail; + } + + return TestPass; + } +}; + +} /* namespace */ + +TEST_REGISTER(FormatDefault); diff --git a/test/camera/meson.build b/test/camera/meson.build new file mode 100644 index 0000000000000000..4f2ed244a9240512 --- /dev/null +++ b/test/camera/meson.build @@ -0,0 +1,12 @@ +# Tests are listed in order of complexity. +# They are not alphabetically sorted. +camera_tests = [ + [ 'format_default', 'format_default.cpp' ], +] + +foreach t : camera_tests + exe = executable(t[0], [t[1], 'camera_test.cpp'], + link_with : test_libraries, + include_directories : test_includes_internal) + test(t[0], exe, suite: 'camera', is_parallel: false) +endforeach diff --git a/test/meson.build b/test/meson.build index 5fb16fa6afb62f8d..71a96921697c0e9e 100644 --- a/test/meson.build +++ b/test/meson.build @@ -1,5 +1,6 @@ subdir('libtest') +subdir('camera') subdir('media_device') subdir('pipeline') subdir('v4l2_device')
Add a test to verify reading the default format from a camera works. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- test/camera/camera_test.cpp | 47 ++++++++++++++++++++++ test/camera/camera_test.h | 32 +++++++++++++++ test/camera/format_default.cpp | 71 ++++++++++++++++++++++++++++++++++ test/camera/meson.build | 12 ++++++ test/meson.build | 1 + 5 files changed, 163 insertions(+) create mode 100644 test/camera/camera_test.cpp create mode 100644 test/camera/camera_test.h create mode 100644 test/camera/format_default.cpp create mode 100644 test/camera/meson.build