[libcamera-devel] [PATCH 2/5] test: camera: Add read default format test
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Mar 11 01:43:24 CET 2019
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 at 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list