[libcamera-devel] [PATCH 3/5] test: camera: Add setting of format test

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-03-10 15:30:39 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Wed, Mar 06, 2019 at 03:47:53AM +0100, Niklas Söderlund wrote:
> > Try to set the default format, a modified valid format and an invalid
> > format.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  test/camera/format_set.cpp | 88 ++++++++++++++++++++++++++++++++++++++
> >  test/camera/meson.build    |  1 +
> >  2 files changed, 89 insertions(+)
> >  create mode 100644 test/camera/format_set.cpp
> > 
> > diff --git a/test/camera/format_set.cpp b/test/camera/format_set.cpp
> > new file mode 100644
> > index 0000000000000000..948428ca6b00d2d0
> > --- /dev/null
> > +++ b/test/camera/format_set.cpp
> > @@ -0,0 +1,88 @@
> > +/* 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 FormatSet : public CameraTest
> > +{
> > +protected:
> > +	int run()
> > +	{
> > +		if (camera_->acquire()) {
> > +			cout << "Acquiring the camera failed" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		std::set<Stream *> streams = { *camera_->streams().begin() };
> > +		std::map<Stream *, StreamConfiguration> conf =
> > +			camera_->streamConfiguration(streams);
> > +		StreamConfiguration *sconf = &conf.begin()->second;
> > +		if (conf.size() != 1) {
> > +			cout << "Reading default format failed" << endl;
> > +			return TestFail;
> > +		}
> 
> This could use the validation function I mentioned in patch 2/5. Or you
> could rely on the test from patch 2/5 performing this check, and skip it
> here, up to you.

Great idea, I think I will keep the new check here to catch potential 
errors early.

> 
> > +
> > +		/*
> > +		 * Test setting the default format works.
> s/Test/Test that/
> 
> > +		 */
> 
> This holds on a single line.

Thanks.

> 
> > +		if (camera_->configureStreams(conf)) {
> > +			cout << "Setting valid format test failed" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/*
> > +		 * Test setting the default format fails the camera is not
> 
> s/Test/Test that/
> s/the camera/if the camera/

Thanks.

> 
> > +		 * acquired.
> > +		 */
> > +		if (camera_->release()) {
> > +			cout << "Releasing the camera failed" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (!camera_->configureStreams(conf)) {
> > +			cout << "Setting valid format on a camera not acquired failed" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (camera_->acquire()) {
> > +			cout << "Acquiring the camera failed" << endl;
> > +			return TestFail;
> > +		}
> 
> If you moved this before the previous test you could drop the
> releast/acquire. On the other hand there's value in testing
> release/acquire. If you want to keep them I'd mention that in the
> comment above.

It was my intention to also test the release/acquire, I will add it to 
the comment.

> 
> > +
> > +		/*
> > +		 * Test doubling the resolution works.
> 
> s/Test/Test that/
> 
> You should expand this comment and mention that VIMC supports it,
> otherwise it may not be clear why a double resolution is expected to be
> valid.

Good point.

> 
> > +		 */
> > +		sconf->width *= 2;
> > +		sconf->height *= 2;
> > +		if (camera_->configureStreams(conf)) {
> > +			cout << "Setting modified valid format test failed" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/*
> > +		 * Test Setting an invalid format fails.
> > +		 */
> > +		sconf->width = 0;
> > +		sconf->height = 0;
> > +		if (!camera_->configureStreams(conf)) {
> > +			cout << "Setting invalid format test failed" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +};
> > +
> > +} /* namespace */
> > +
> > +TEST_REGISTER(FormatSet);
> > diff --git a/test/camera/meson.build b/test/camera/meson.build
> > index 4f2ed244a9240512..f5f27c4229ac307f 100644
> > --- a/test/camera/meson.build
> > +++ b/test/camera/meson.build
> > @@ -2,6 +2,7 @@
> >  # They are not alphabetically sorted.
> >  camera_tests = [
> >    [ 'format_default',        'format_default.cpp' ],
> > +  [ 'format_set',            'format_set.cpp' ],
> >  ]
> >  
> >  foreach t : camera_tests
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund