[libcamera-devel] [PATCH v2 08/14] test: v4l2_videodevice: Add V4L2 control test

[Laurent Pinchart]

Hi Jacopo,

On Sun, Oct 13, 2019 at 04:07:59PM +0200, Jacopo Mondi wrote:
> On Sat, Oct 12, 2019 at 09:44:01PM +0300, Laurent Pinchart wrote:
> > Add a test that exercises the control enumeration, get and set APIs on a
> > V4L2Device.
> 
> This test was really needed, thanks.
> I would have however moved it after the V4L2ControlList API rework to
> avoid having it patch it later in the series.

I specifically added it before the rework, to test for regressions :-)

> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  test/v4l2_videodevice/controls.cpp | 98 ++++++++++++++++++++++++++++++
> >  test/v4l2_videodevice/meson.build  |  1 +
> >  2 files changed, 99 insertions(+)
> >  create mode 100644 test/v4l2_videodevice/controls.cpp
> >
> > diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
> > new file mode 100644
> > index 000000000000..4a7535245c00
> > --- /dev/null
> > +++ b/test/v4l2_videodevice/controls.cpp
> > @@ -0,0 +1,98 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * controls.cpp - V4L2 device controls handling test
> > + */
> > +
> > +#include <climits>
> > +#include <iostream>
> > +
> > +#include "v4l2_videodevice.h"
> > +
> > +#include "v4l2_videodevice_test.h"
> > +
> > +using namespace std;
> > +using namespace libcamera;
> > +
> > +class V4L2ControlTest : public V4L2VideoDeviceTest
> > +{
> > +public:
> > +	V4L2ControlTest()
> > +		: V4L2VideoDeviceTest("vivid", "vivid-000-vid-cap") {}
> 
> I still don't get what is the preferred way of doing this
> if {} on the same line
> or
>         {
>         }

The second is preferred, this comes from a copy&paste, I'll fix it.

> > +
> > +protected:
> > +	int run()
> > +	{
> > +		const V4L2ControlInfoMap &info = capture_->controls();
> > +
> > +		/* Test control enumeration. */
> > +		if (info.empty()) {
> > +			cerr << "Failed to enumerate controls" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (info.find(V4L2_CID_BRIGHTNESS) == info.end() ||
> > +		    info.find(V4L2_CID_CONTRAST) == info.end() ||
> > +		    info.find(V4L2_CID_SATURATION) == info.end()) {
> > +			cerr << "Missing controls" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		const V4L2ControlInfo &brightness = info.find(V4L2_CID_BRIGHTNESS)->second;
> > +		const V4L2ControlInfo &contrast = info.find(V4L2_CID_CONTRAST)->second;
> > +		const V4L2ControlInfo &saturation = info.find(V4L2_CID_SATURATION)->second;
> > +
> > +		/* Test getting controls. */
> > +		V4L2ControlList ctrls;
> > +		ctrls.add(V4L2_CID_BRIGHTNESS);
> > +		ctrls.add(V4L2_CID_CONTRAST);
> > +		ctrls.add(V4L2_CID_SATURATION);
> > +
> > +		int ret = capture_->getControls(&ctrls);
> > +		if (ret != 0) {
> 
> Just if (ret) ?

Will fix.

> > +			cerr << "Failed to get controls" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (ctrls[V4L2_CID_BRIGHTNESS]->value().get<int32_t>() == -1 ||
> > +		    ctrls[V4L2_CID_CONTRAST]->value().get<int32_t>() == -1 ||
> > +		    ctrls[V4L2_CID_SATURATION]->value().get<int32_t>() == -1) {
> 
> I'm missing where they're set to -1

They're not, they're set to 0. And I can't test for != 0 as 0 is a valid
value. I'll remove this.

> > +			cerr << "Incorrect value for retrieved controls" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/* Test setting controls. */
> > +		ctrls[V4L2_CID_BRIGHTNESS]->value() = brightness.range().min();
> > +		ctrls[V4L2_CID_CONTRAST]->value() = contrast.range().max();
> > +		ctrls[V4L2_CID_SATURATION]->value() = saturation.range().min();
> > +
> > +		ret = capture_->setControls(&ctrls);
> > +		if (ret != 0) {
> 
> same comment: just if (ret) ?

Will fix too.

> > +			cerr << "Failed to set controls" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/* Test setting controls outside of range. */
> > +		ctrls[V4L2_CID_BRIGHTNESS]->value() = brightness.range().min().get<int32_t>() - 1;
> > +		ctrls[V4L2_CID_CONTRAST]->value() = contrast.range().max().get<int32_t>() + 1;
> > +		ctrls[V4L2_CID_SATURATION]->value() = saturation.range().min().get<int32_t>() + 1;
> > +
> > +		ret = capture_->setControls(&ctrls);
> > +		if (ret != 0) {
> 
> Shouldn't setting controls out of range fail?

No, V4L2 controls are adjusted by the kernel. We may later perform the
adjustement in libcamera, or even fail, in which case I'll adapt the
test, but at the moment the test matches the implementation.

> Anyway, with these clarified
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +			cerr << "Failed to set controls (out of range)" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (ctrls[V4L2_CID_BRIGHTNESS]->value() != brightness.range().min() ||
> > +		    ctrls[V4L2_CID_CONTRAST]->value() != brightness.range().max() ||
> > +		    ctrls[V4L2_CID_SATURATION]->value() != saturation.range().min().get<int32_t>() + 1) {
> > +			cerr << "Controls not updated when set" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +};
> > +
> > +TEST_REGISTER(V4L2ControlTest);
> > diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> > index ad41898b5f8b..5c52da7219c2 100644
> > --- a/test/v4l2_videodevice/meson.build
> > +++ b/test/v4l2_videodevice/meson.build
> > @@ -2,6 +2,7 @@
> >  # They are not alphabetically sorted.
> >  v4l2_videodevice_tests = [
> >      [ 'double_open',        'double_open.cpp' ],
> > +    [ 'controls',           'controls.cpp' ],
> >      [ 'formats',            'formats.cpp' ],
> >      [ 'request_buffers',    'request_buffers.cpp' ],
> >      [ 'stream_on_off',      'stream_on_off.cpp' ],

-- 
Regards,

Laurent Pinchart