[libcamera-devel,v2,2/8] test: delayed_controls: Add test case for DelayedControls
diff mbox series

Message ID 20201110002710.3233696-3-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • Add helper for controls that take effect with a delay
Related show

Commit Message

Niklas Söderlund Nov. 10, 2020, 12:27 a.m. UTC 
Add a test-case for DelayedControls that exercise the setting of
controls and reading back what controls where used for a particular
frame. Also exercise corner case such as a V4L2 devices that do not
reset it sequence number to 0 at stream on and sequence number wrapping
around the uint32_t value space.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 test/delayed_contols.cpp | 307 +++++++++++++++++++++++++++++++++++++++
 test/meson.build         |   1 +
 2 files changed, 308 insertions(+)
 create mode 100644 test/delayed_contols.cpp

Comments

Jacopo Mondi Nov. 18, 2020, 2:12 p.m. UTC | #1 
Hi Niklas,

On Tue, Nov 10, 2020 at 01:27:04AM +0100, Niklas Söderlund wrote:
> Add a test-case for DelayedControls that exercise the setting of
> controls and reading back what controls where used for a particular
> frame. Also exercise corner case such as a V4L2 devices that do not
> reset it sequence number to 0 at stream on and sequence number wrapping

s/it/its

> around the uint32_t value space.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  test/delayed_contols.cpp | 307 +++++++++++++++++++++++++++++++++++++++
>  test/meson.build         |   1 +
>  2 files changed, 308 insertions(+)
>  create mode 100644 test/delayed_contols.cpp
>
> diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
> new file mode 100644
> index 0000000000000000..e71da6662c30bbdc
> --- /dev/null
> +++ b/test/delayed_contols.cpp
> @@ -0,0 +1,307 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * libcamera delayed controls tests
> + */
> +
> +#include <iostream>
> +
> +#include "libcamera/internal/delayed_controls.h"
> +#include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/media_device.h"
> +#include "libcamera/internal/v4l2_videodevice.h"
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class DelayedControlsTest : public Test
> +{
> +public:
> +	DelayedControlsTest()
> +		: dev_(nullptr)
> +	{
> +	}
> +
> +	int init()
> +	{
> +		enumerator_ = DeviceEnumerator::create();
> +		if (!enumerator_) {
> +			cerr << "Failed to create device enumerator" << endl;
> +			return TestFail;
> +		}
> +
> +		if (enumerator_->enumerate()) {
> +			cerr << "Failed to enumerate media devices" << endl;
> +			return TestFail;
> +		}
> +
> +		DeviceMatch dm("vivid");
> +		dm.add("vivid-000-vid-cap");
> +
> +		media_ = enumerator_->search(dm);
> +		if (!media_) {
> +			cerr << "vivid video device found" << endl;
> +			return TestSkip;
> +		}
> +
> +		MediaEntity *entity = media_->getEntityByName("vivid-000-vid-cap");
> +		dev_ = new V4L2VideoDevice(entity->deviceNode());
> +		if (dev_->open()) {
> +			cerr << "Failed to open video device" << endl;
> +			return TestFail;
> +		}
> +
> +		const ControlInfoMap &infoMap = dev_->controls();
> +
> +		/* Test control enumeration. */
> +		if (infoMap.empty()) {
> +			cerr << "Failed to enumerate controls" << endl;
> +			return TestFail;
> +		}
> +
> +		if (infoMap.find(V4L2_CID_BRIGHTNESS) == infoMap.end() ||
> +		    infoMap.find(V4L2_CID_CONTRAST) == infoMap.end()) {
> +			cerr << "Missing controls" << endl;
> +			return TestFail;
> +		}
> +
> +		ControlList ctrls = dev_->getControls({ V4L2_CID_BRIGHTNESS });

What's ctrls for here ?

> +
> +		return TestPass;
> +	}
> +
> +	int singleControlNoDelay()
> +	{
> +		std::unordered_map<uint32_t, unsigned int> delays = {
> +			{ V4L2_CID_BRIGHTNESS, 0 },
> +		};
> +		std::unique_ptr<DelayedControls> delayed =
> +			std::make_unique<DelayedControls>(dev_, delays);

Should this just be
                DelayedControls delayed(dev_, delays) ?
it's not used outside of the function

> +

Additional empty line

> +		ControlList ctrls;

Do we want to create the ControlList with the device info map for
additional validation ?
                ControlList ctrl(dev_->controls());

> +
> +		/* Reset control to value not used in test. */
> +		ctrls.set(V4L2_CID_BRIGHTNESS, 1);
> +		delayed->reset(&ctrls);

I would use the ControlInfo.min()

Do you expect after reset() that BRIGHTNESS is equal to 1 ?

> +
> +		/* Test control without delay are set at once. */
> +		for (int32_t i = 0; i < 10; i++) {
> +			int32_t value = 100 + i;
> +
> +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> +			delayed->push(ctrls);
> +
> +			delayed->frameStart(i);
> +
> +			ControlList result = delayed->get(i);
> +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> +			if (brightness != value) {
> +				cerr << "Failed single control without delay"
> +				     << " frame " << i
> +				     << " expected " << value
> +				     << " got " << brightness
> +				     << endl;
> +				return TestFail;
> +			}
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int singleControlWithDelay()
> +	{
> +		std::unordered_map<uint32_t, unsigned int> delays = {
> +			{ V4L2_CID_BRIGHTNESS, 1 },
> +		};
> +		std::unique_ptr<DelayedControls> delayed =
> +			std::make_unique<DelayedControls>(dev_, delays);
> +
> +		ControlList ctrls;

Same questions

> +
> +		/* Reset control to value that will be first in test. */
> +		int32_t expected = 4;
> +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> +		delayed->reset(&ctrls);
> +
> +		/* Test single control with delay. */
> +		for (int32_t i = 0; i < 100; i++) {
> +			int32_t value = 10 + i;

Is swapping 10 and 100 in the for loop compared to the previous test
intended ?

> +
> +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> +			delayed->push(ctrls);
> +
> +			delayed->frameStart(i);
> +
> +			ControlList result = delayed->get(i);
> +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> +			if (brightness != expected) {

Are we're lucky and BRIGHTNESS.min() >= 4 ?

> +				cerr << "Failed single control with delay"
> +				     << " frame " << i
> +				     << " expected " << expected
> +				     << " got " << brightness
> +				     << endl;
> +				return TestFail;
> +			}
> +
> +			expected = value;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int dualControlsWithDelay(uint32_t startOffset)
> +	{
> +		std::unordered_map<uint32_t, unsigned int> delays = {
> +			{ V4L2_CID_BRIGHTNESS, 1 },
> +			{ V4L2_CID_CONTRAST, 2 },
> +		};
> +		std::unique_ptr<DelayedControls> delayed =
> +			std::make_unique<DelayedControls>(dev_, delays);
> +
> +		ControlList ctrls;
> +

Same :)

> +		/* Reset control to value that will be first two frames in test. */
> +		int32_t expected = 200;
> +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> +		ctrls.set(V4L2_CID_CONTRAST, expected);
> +		delayed->reset(&ctrls);
> +
> +		/* Test dual control with delay. */
> +		for (int32_t i = 0; i < 100; i++) {
> +			uint32_t frame = startOffset + i;
> +

Additional empty line

> +			int32_t value = 10 + i;
> +
> +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> +			ctrls.set(V4L2_CID_CONTRAST, value);
> +			delayed->push(ctrls);
> +
> +			delayed->frameStart(frame);
> +
> +			ControlList result = delayed->get(frame);
> +

In the previous functions you don't have an empty line here

> +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> +			int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
> +			if (brightness != expected || contrast != expected) {
> +				cerr << "Failed dual controls"
> +				     << " frame " << frame
> +				     << " brightness " << brightness
> +				     << " contrast " << contrast
> +				     << " expected " << expected
> +				     << endl;
> +				return TestFail;
> +			}
> +
> +			expected = i < 1 ? expected : value - 1;

Why do I expect this to fail ?

expected = 200
0:
        value = 10;
        set(BRIGHTNESS, 10);
        set(CONTRAST, 10);

        frameStart(0);
        brightness = 200;
        contrast = 200;
        (200 == 200; 200 == 200);
        expected = 200;

1:
        value = 11;
        set(BRIGHTNESS, 11);
        set(CONTRAST, 11);

        frameStart(1);
        brightness = 10; <- delay was 1
        contrast = 200;  <- delay was 2
        (10 != 200) <- Shouldn't this fail ?

        expected = 10;

> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int dualControlsMultiQueue()
> +	{
> +		std::unordered_map<uint32_t, unsigned int> delays = {
> +			{ V4L2_CID_BRIGHTNESS, 1 },
> +			{ V4L2_CID_CONTRAST, 2 },
> +		};
> +		std::unique_ptr<DelayedControls> delayed =
> +			std::make_unique<DelayedControls>(dev_, delays);
> +
> +		ControlList ctrls;
> +
> +		/* Reset control to value that will be first two frames in test. */
> +		int32_t expected = 100;
> +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> +		ctrls.set(V4L2_CID_CONTRAST, expected);
> +		delayed->reset(&ctrls);
> +
> +		/*
> +		 * Queue all controls before any fake frame start. Note we
> +		 * can't queue up more then the delayed controls history size
> +		 * which is 16. Where one spot is used by the reset control.
> +		 */
> +		for (int32_t i = 0; i < 15; i++) {
> +			int32_t value = 10 + i;
> +
> +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> +			ctrls.set(V4L2_CID_CONTRAST, value);
> +			delayed->push(ctrls);
> +		}
> +
> +		/* Process all queued controls. */
> +		for (int32_t i = 0; i < 16; i++) {
> +			int32_t value = 10 + i;
> +
> +			delayed->frameStart(i);
> +
> +			ControlList result = delayed->get(i);
> +
> +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> +			int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
> +			if (brightness != expected || contrast != expected) {
> +				cerr << "Failed multi queue"
> +				     << " frame " << i
> +				     << " brightness " << brightness
> +				     << " contrast " << contrast
> +				     << " expected " << expected
> +				     << endl;
> +				return TestFail;
> +			}
> +
> +			expected = i < 1 ? expected : value - 1;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int run()
> +	{
> +		int ret;
> +
> +		/* Test single control without delay. */
> +		ret = singleControlNoDelay();
> +		if (ret)
> +			return ret;
> +
> +		/* Test single control with delay. */
> +		ret = singleControlWithDelay();
> +		if (ret)
> +			return ret;
> +
> +		/* Test dual controls with different delays. */
> +		ret = dualControlsWithDelay(0);
> +		if (ret)
> +			return ret;
> +
> +		/* Test dual controls with non-zero sequence start. */
> +		ret = dualControlsWithDelay(10000);
> +		if (ret)
> +			return ret;
> +
> +		/* Test dual controls with sequence number wraparound. */
> +		ret = dualControlsWithDelay(UINT32_MAX - 50);
> +		if (ret)
> +			return ret;
> +
> +		/* Test control values produced faster then consumed. */
> +		ret = dualControlsMultiQueue();
> +		if (ret)
> +			return ret;

Maybe an empty line ?

Thanks
  j

> +		return TestPass;
> +	}
> +
> +	void cleanup()
> +	{
> +		delete dev_;
> +	}
> +
> +private:
> +	std::unique_ptr<DeviceEnumerator> enumerator_;
> +	std::shared_ptr<MediaDevice> media_;
> +	V4L2VideoDevice *dev_;
> +};
> +
> +TEST_REGISTER(DelayedControlsTest)
> diff --git a/test/meson.build b/test/meson.build
> index 0a1d434e399641bb..a683a657a439b4ff 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -25,6 +25,7 @@ public_tests = [
>  internal_tests = [
>      ['byte-stream-buffer',              'byte-stream-buffer.cpp'],
>      ['camera-sensor',                   'camera-sensor.cpp'],
> +    ['delayed_contols',                 'delayed_contols.cpp'],
>      ['event',                           'event.cpp'],
>      ['event-dispatcher',                'event-dispatcher.cpp'],
>      ['event-thread',                    'event-thread.cpp'],
> --
> 2.29.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Nov. 23, 2020, 9:17 p.m. UTC | #2 
Hi Jacopo,

Thanks for your feedback.

On 2020-11-18 15:12:53 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Nov 10, 2020 at 01:27:04AM +0100, Niklas Söderlund wrote:
> > Add a test-case for DelayedControls that exercise the setting of
> > controls and reading back what controls where used for a particular
> > frame. Also exercise corner case such as a V4L2 devices that do not
> > reset it sequence number to 0 at stream on and sequence number wrapping
> 
> s/it/its
> 
> > around the uint32_t value space.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  test/delayed_contols.cpp | 307 +++++++++++++++++++++++++++++++++++++++
> >  test/meson.build         |   1 +
> >  2 files changed, 308 insertions(+)
> >  create mode 100644 test/delayed_contols.cpp
> >
> > diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
> > new file mode 100644
> > index 0000000000000000..e71da6662c30bbdc
> > --- /dev/null
> > +++ b/test/delayed_contols.cpp
> > @@ -0,0 +1,307 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * libcamera delayed controls tests
> > + */
> > +
> > +#include <iostream>
> > +
> > +#include "libcamera/internal/delayed_controls.h"
> > +#include "libcamera/internal/device_enumerator.h"
> > +#include "libcamera/internal/media_device.h"
> > +#include "libcamera/internal/v4l2_videodevice.h"
> > +
> > +#include "test.h"
> > +
> > +using namespace std;
> > +using namespace libcamera;
> > +
> > +class DelayedControlsTest : public Test
> > +{
> > +public:
> > +	DelayedControlsTest()
> > +		: dev_(nullptr)
> > +	{
> > +	}
> > +
> > +	int init()
> > +	{
> > +		enumerator_ = DeviceEnumerator::create();
> > +		if (!enumerator_) {
> > +			cerr << "Failed to create device enumerator" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (enumerator_->enumerate()) {
> > +			cerr << "Failed to enumerate media devices" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		DeviceMatch dm("vivid");
> > +		dm.add("vivid-000-vid-cap");
> > +
> > +		media_ = enumerator_->search(dm);
> > +		if (!media_) {
> > +			cerr << "vivid video device found" << endl;
> > +			return TestSkip;
> > +		}
> > +
> > +		MediaEntity *entity = media_->getEntityByName("vivid-000-vid-cap");
> > +		dev_ = new V4L2VideoDevice(entity->deviceNode());
> > +		if (dev_->open()) {
> > +			cerr << "Failed to open video device" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		const ControlInfoMap &infoMap = dev_->controls();
> > +
> > +		/* Test control enumeration. */
> > +		if (infoMap.empty()) {
> > +			cerr << "Failed to enumerate controls" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (infoMap.find(V4L2_CID_BRIGHTNESS) == infoMap.end() ||
> > +		    infoMap.find(V4L2_CID_CONTRAST) == infoMap.end()) {
> > +			cerr << "Missing controls" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		ControlList ctrls = dev_->getControls({ V4L2_CID_BRIGHTNESS });
> 
> What's ctrls for here ?

Good catch, I will remove it for next version.

> 
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	int singleControlNoDelay()
> > +	{
> > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > +			{ V4L2_CID_BRIGHTNESS, 0 },
> > +		};
> > +		std::unique_ptr<DelayedControls> delayed =
> > +			std::make_unique<DelayedControls>(dev_, delays);
> 
> Should this just be
>                 DelayedControls delayed(dev_, delays) ?
> it's not used outside of the function

It could, I have no real preference. I picked this as I wanted to test 
some things during development and then I left it as such. Do you see an 
particular gain in reworking this?

> 
> > +
> 
> Additional empty line

Thanks.

> 
> > +		ControlList ctrls;
> 
> Do we want to create the ControlList with the device info map for
> additional validation ?
>                 ControlList ctrl(dev_->controls());

We could, but I don't see the value in it as the goal is not to test 
ControlList. The controls are carefully selected to have a value range 
of [min=0 max=255 step=1]. If this ever changes this test will fail in 
other ways.

> 
> > +
> > +		/* Reset control to value not used in test. */
> > +		ctrls.set(V4L2_CID_BRIGHTNESS, 1);
> > +		delayed->reset(&ctrls);
> 
> I would use the ControlInfo.min()
> 
> Do you expect after reset() that BRIGHTNESS is equal to 1 ?

Yes.

> 
> > +
> > +		/* Test control without delay are set at once. */
> > +		for (int32_t i = 0; i < 10; i++) {
> > +			int32_t value = 100 + i;
> > +
> > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > +			delayed->push(ctrls);
> > +
> > +			delayed->frameStart(i);
> > +
> > +			ControlList result = delayed->get(i);
> > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > +			if (brightness != value) {
> > +				cerr << "Failed single control without delay"
> > +				     << " frame " << i
> > +				     << " expected " << value
> > +				     << " got " << brightness
> > +				     << endl;
> > +				return TestFail;
> > +			}
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	int singleControlWithDelay()
> > +	{
> > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > +			{ V4L2_CID_BRIGHTNESS, 1 },
> > +		};
> > +		std::unique_ptr<DelayedControls> delayed =
> > +			std::make_unique<DelayedControls>(dev_, delays);
> > +
> > +		ControlList ctrls;
> 
> Same questions
> 
> > +
> > +		/* Reset control to value that will be first in test. */
> > +		int32_t expected = 4;
> > +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> > +		delayed->reset(&ctrls);
> > +
> > +		/* Test single control with delay. */
> > +		for (int32_t i = 0; i < 100; i++) {
> > +			int32_t value = 10 + i;
> 
> Is swapping 10 and 100 in the for loop compared to the previous test
> intended ?

No it should be 100 in the previous test.

> 
> > +
> > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > +			delayed->push(ctrls);
> > +
> > +			delayed->frameStart(i);
> > +
> > +			ControlList result = delayed->get(i);
> > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > +			if (brightness != expected) {
> 
> Are we're lucky and BRIGHTNESS.min() >= 4 ?

I'm sorry I don't think I understand this question fully I think. We are 
not lucky here, before the loop we set the control to 4 and this is 
whats tested here in the first iteration. But the vale 4 is arbitrary 
and we can set it to any value between 0 and 255 before the loop and the 
test works.

> 
> > +				cerr << "Failed single control with delay"
> > +				     << " frame " << i
> > +				     << " expected " << expected
> > +				     << " got " << brightness
> > +				     << endl;
> > +				return TestFail;
> > +			}
> > +
> > +			expected = value;
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	int dualControlsWithDelay(uint32_t startOffset)
> > +	{
> > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > +			{ V4L2_CID_BRIGHTNESS, 1 },
> > +			{ V4L2_CID_CONTRAST, 2 },
> > +		};
> > +		std::unique_ptr<DelayedControls> delayed =
> > +			std::make_unique<DelayedControls>(dev_, delays);
> > +
> > +		ControlList ctrls;
> > +
> 
> Same :)
> 
> > +		/* Reset control to value that will be first two frames in test. */
> > +		int32_t expected = 200;
> > +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> > +		ctrls.set(V4L2_CID_CONTRAST, expected);
> > +		delayed->reset(&ctrls);
> > +
> > +		/* Test dual control with delay. */
> > +		for (int32_t i = 0; i < 100; i++) {
> > +			uint32_t frame = startOffset + i;
> > +
> 
> Additional empty line
> 
> > +			int32_t value = 10 + i;
> > +
> > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > +			ctrls.set(V4L2_CID_CONTRAST, value);
> > +			delayed->push(ctrls);
> > +
> > +			delayed->frameStart(frame);
> > +
> > +			ControlList result = delayed->get(frame);
> > +
> 
> In the previous functions you don't have an empty line here
> 
> > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > +			int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
> > +			if (brightness != expected || contrast != expected) {
> > +				cerr << "Failed dual controls"
> > +				     << " frame " << frame
> > +				     << " brightness " << brightness
> > +				     << " contrast " << contrast
> > +				     << " expected " << expected
> > +				     << endl;
> > +				return TestFail;
> > +			}
> > +
> > +			expected = i < 1 ? expected : value - 1;
> 
> Why do I expect this to fail ?
> 
> expected = 200
> 0:
>         value = 10;
>         set(BRIGHTNESS, 10);
>         set(CONTRAST, 10);
> 
>         frameStart(0);
>         brightness = 200;
>         contrast = 200;
>         (200 == 200; 200 == 200);
>         expected = 200;
> 
> 1:
>         value = 11;
>         set(BRIGHTNESS, 11);
>         set(CONTRAST, 11);
> 
>         frameStart(1);
>         brightness = 10; <- delay was 1
>         contrast = 200;  <- delay was 2
>         (10 != 200) <- Shouldn't this fail ?
> 
>         expected = 10;

I agree it's confusing :-) The idea of DelayedControls is not to apply 
the controls as soon as possible but to sync the setting of all controls 
of a group to the worst delay.

The flow is:

expected = 200
0:
        value = 10;
        set(BRIGHTNESS, 10);
        set(CONTRAST, 10);

        frameStart(0);
        brightness = 200;
        contrast = 200;
        (200 == 200; 200 == 200);
        expected = 200;

1:
        value = 11;
        set(BRIGHTNESS, 11);
        set(CONTRAST, 11);

        frameStart(1);
        brightness = 200; <- brightness delay is 1 but for the group 2
        contrast = 200;  <- delay was 2
        (200 == 200; 200 == 200);
        expected = 10;

2:
        value = 12;
        set(BRIGHTNESS, 12);
        set(CONTRAST, 12);

        frameStart(2);
        brightness = 10;
        contrast = 10;
        (10 == 10; 10 == 10);
        expected = 11;

3:
        value = 13;
        set(BRIGHTNESS, 13);
        set(CONTRAST, 13);

        frameStart(3);
        brightness = 11;
        contrast = 11;
        (11 == 11; 11 == 11);
        expected = 12;

> 
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	int dualControlsMultiQueue()
> > +	{
> > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > +			{ V4L2_CID_BRIGHTNESS, 1 },
> > +			{ V4L2_CID_CONTRAST, 2 },
> > +		};
> > +		std::unique_ptr<DelayedControls> delayed =
> > +			std::make_unique<DelayedControls>(dev_, delays);
> > +
> > +		ControlList ctrls;
> > +
> > +		/* Reset control to value that will be first two frames in test. */
> > +		int32_t expected = 100;
> > +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> > +		ctrls.set(V4L2_CID_CONTRAST, expected);
> > +		delayed->reset(&ctrls);
> > +
> > +		/*
> > +		 * Queue all controls before any fake frame start. Note we
> > +		 * can't queue up more then the delayed controls history size
> > +		 * which is 16. Where one spot is used by the reset control.
> > +		 */
> > +		for (int32_t i = 0; i < 15; i++) {
> > +			int32_t value = 10 + i;
> > +
> > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > +			ctrls.set(V4L2_CID_CONTRAST, value);
> > +			delayed->push(ctrls);
> > +		}
> > +
> > +		/* Process all queued controls. */
> > +		for (int32_t i = 0; i < 16; i++) {
> > +			int32_t value = 10 + i;
> > +
> > +			delayed->frameStart(i);
> > +
> > +			ControlList result = delayed->get(i);
> > +
> > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > +			int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
> > +			if (brightness != expected || contrast != expected) {
> > +				cerr << "Failed multi queue"
> > +				     << " frame " << i
> > +				     << " brightness " << brightness
> > +				     << " contrast " << contrast
> > +				     << " expected " << expected
> > +				     << endl;
> > +				return TestFail;
> > +			}
> > +
> > +			expected = i < 1 ? expected : value - 1;
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	int run()
> > +	{
> > +		int ret;
> > +
> > +		/* Test single control without delay. */
> > +		ret = singleControlNoDelay();
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Test single control with delay. */
> > +		ret = singleControlWithDelay();
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Test dual controls with different delays. */
> > +		ret = dualControlsWithDelay(0);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Test dual controls with non-zero sequence start. */
> > +		ret = dualControlsWithDelay(10000);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Test dual controls with sequence number wraparound. */
> > +		ret = dualControlsWithDelay(UINT32_MAX - 50);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Test control values produced faster then consumed. */
> > +		ret = dualControlsMultiQueue();
> > +		if (ret)
> > +			return ret;
> 
> Maybe an empty line ?

Yes :-)

> 
> Thanks
>   j
> 
> > +		return TestPass;
> > +	}
> > +
> > +	void cleanup()
> > +	{
> > +		delete dev_;
> > +	}
> > +
> > +private:
> > +	std::unique_ptr<DeviceEnumerator> enumerator_;
> > +	std::shared_ptr<MediaDevice> media_;
> > +	V4L2VideoDevice *dev_;
> > +};
> > +
> > +TEST_REGISTER(DelayedControlsTest)
> > diff --git a/test/meson.build b/test/meson.build
> > index 0a1d434e399641bb..a683a657a439b4ff 100644
> > --- a/test/meson.build
> > +++ b/test/meson.build
> > @@ -25,6 +25,7 @@ public_tests = [
> >  internal_tests = [
> >      ['byte-stream-buffer',              'byte-stream-buffer.cpp'],
> >      ['camera-sensor',                   'camera-sensor.cpp'],
> > +    ['delayed_contols',                 'delayed_contols.cpp'],
> >      ['event',                           'event.cpp'],
> >      ['event-dispatcher',                'event-dispatcher.cpp'],
> >      ['event-thread',                    'event-thread.cpp'],
> > --
> > 2.29.2
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Nov. 24, 2020, 11:31 a.m. UTC | #3 
Hi Niklas,

On Mon, Nov 23, 2020 at 10:17:25PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2020-11-18 15:12:53 +0100, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Tue, Nov 10, 2020 at 01:27:04AM +0100, Niklas Söderlund wrote:
> > > Add a test-case for DelayedControls that exercise the setting of
> > > controls and reading back what controls where used for a particular
> > > frame. Also exercise corner case such as a V4L2 devices that do not
> > > reset it sequence number to 0 at stream on and sequence number wrapping
> >
> > s/it/its
> >
> > > around the uint32_t value space.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > >  test/delayed_contols.cpp | 307 +++++++++++++++++++++++++++++++++++++++
> > >  test/meson.build         |   1 +
> > >  2 files changed, 308 insertions(+)
> > >  create mode 100644 test/delayed_contols.cpp
> > >
> > > diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
> > > new file mode 100644
> > > index 0000000000000000..e71da6662c30bbdc
> > > --- /dev/null
> > > +++ b/test/delayed_contols.cpp
> > > @@ -0,0 +1,307 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Google Inc.
> > > + *
> > > + * libcamera delayed controls tests
> > > + */
> > > +
> > > +#include <iostream>
> > > +
> > > +#include "libcamera/internal/delayed_controls.h"
> > > +#include "libcamera/internal/device_enumerator.h"
> > > +#include "libcamera/internal/media_device.h"
> > > +#include "libcamera/internal/v4l2_videodevice.h"
> > > +
> > > +#include "test.h"
> > > +
> > > +using namespace std;
> > > +using namespace libcamera;
> > > +
> > > +class DelayedControlsTest : public Test
> > > +{
> > > +public:
> > > +	DelayedControlsTest()
> > > +		: dev_(nullptr)
> > > +	{
> > > +	}
> > > +
> > > +	int init()
> > > +	{
> > > +		enumerator_ = DeviceEnumerator::create();
> > > +		if (!enumerator_) {
> > > +			cerr << "Failed to create device enumerator" << endl;
> > > +			return TestFail;
> > > +		}
> > > +
> > > +		if (enumerator_->enumerate()) {
> > > +			cerr << "Failed to enumerate media devices" << endl;
> > > +			return TestFail;
> > > +		}
> > > +
> > > +		DeviceMatch dm("vivid");
> > > +		dm.add("vivid-000-vid-cap");
> > > +
> > > +		media_ = enumerator_->search(dm);
> > > +		if (!media_) {
> > > +			cerr << "vivid video device found" << endl;
> > > +			return TestSkip;
> > > +		}
> > > +
> > > +		MediaEntity *entity = media_->getEntityByName("vivid-000-vid-cap");
> > > +		dev_ = new V4L2VideoDevice(entity->deviceNode());
> > > +		if (dev_->open()) {
> > > +			cerr << "Failed to open video device" << endl;
> > > +			return TestFail;
> > > +		}
> > > +
> > > +		const ControlInfoMap &infoMap = dev_->controls();
> > > +
> > > +		/* Test control enumeration. */
> > > +		if (infoMap.empty()) {
> > > +			cerr << "Failed to enumerate controls" << endl;
> > > +			return TestFail;
> > > +		}
> > > +
> > > +		if (infoMap.find(V4L2_CID_BRIGHTNESS) == infoMap.end() ||
> > > +		    infoMap.find(V4L2_CID_CONTRAST) == infoMap.end()) {
> > > +			cerr << "Missing controls" << endl;
> > > +			return TestFail;
> > > +		}
> > > +
> > > +		ControlList ctrls = dev_->getControls({ V4L2_CID_BRIGHTNESS });
> >
> > What's ctrls for here ?
>
> Good catch, I will remove it for next version.
>
> >
> > > +
> > > +		return TestPass;
> > > +	}
> > > +
> > > +	int singleControlNoDelay()
> > > +	{
> > > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > > +			{ V4L2_CID_BRIGHTNESS, 0 },
> > > +		};
> > > +		std::unique_ptr<DelayedControls> delayed =
> > > +			std::make_unique<DelayedControls>(dev_, delays);
> >
> > Should this just be
> >                 DelayedControls delayed(dev_, delays) ?
> > it's not used outside of the function
>
> It could, I have no real preference. I picked this as I wanted to test
> some things during development and then I left it as such. Do you see an
> particular gain in reworking this?
>

It is more that I don't see any gain in using a unique_ptr. The scope
of the variable is local to the function, there's no lifetime
management at play, so allocating it on the stack has the same exact
effect (I won't go and mention the overhead related to using smart
pointer in term of performaces/memory occupation, as I cannot quantify
them for real and this is a test, so it's not a big deal at all).

> >
> > > +
> >
> > Additional empty line
>
> Thanks.
>
> >
> > > +		ControlList ctrls;
> >
> > Do we want to create the ControlList with the device info map for
> > additional validation ?
> >                 ControlList ctrl(dev_->controls());
>
> We could, but I don't see the value in it as the goal is not to test
> ControlList. The controls are carefully selected to have a value range
> of [min=0 max=255 step=1]. If this ever changes this test will fail in
> other ways.

Well, it will fail as you use hardcoded 1 and 255. If they're replaced
by min() and max() the test should be able to pass even if the
underlying driver implementation changes.

>
> >
> > > +
> > > +		/* Reset control to value not used in test. */
> > > +		ctrls.set(V4L2_CID_BRIGHTNESS, 1);
> > > +		delayed->reset(&ctrls);
> >
> > I would use the ControlInfo.min()
> >
> > Do you expect after reset() that BRIGHTNESS is equal to 1 ?
>
> Yes.
>

Unlikely to change, but I would avoid using hardcoded value.
Also setting it to min() makes the code more explicit.

> >
> > > +
> > > +		/* Test control without delay are set at once. */
> > > +		for (int32_t i = 0; i < 10; i++) {
> > > +			int32_t value = 100 + i;
> > > +
> > > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > > +			delayed->push(ctrls);
> > > +
> > > +			delayed->frameStart(i);
> > > +
> > > +			ControlList result = delayed->get(i);
> > > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > > +			if (brightness != value) {
> > > +				cerr << "Failed single control without delay"
> > > +				     << " frame " << i
> > > +				     << " expected " << value
> > > +				     << " got " << brightness
> > > +				     << endl;
> > > +				return TestFail;
> > > +			}
> > > +		}
> > > +
> > > +		return TestPass;
> > > +	}
> > > +
> > > +	int singleControlWithDelay()
> > > +	{
> > > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > > +			{ V4L2_CID_BRIGHTNESS, 1 },
> > > +		};
> > > +		std::unique_ptr<DelayedControls> delayed =
> > > +			std::make_unique<DelayedControls>(dev_, delays);
> > > +
> > > +		ControlList ctrls;
> >
> > Same questions
> >
> > > +
> > > +		/* Reset control to value that will be first in test. */
> > > +		int32_t expected = 4;
> > > +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> > > +		delayed->reset(&ctrls);
> > > +
> > > +		/* Test single control with delay. */
> > > +		for (int32_t i = 0; i < 100; i++) {
> > > +			int32_t value = 10 + i;
> >
> > Is swapping 10 and 100 in the for loop compared to the previous test
> > intended ?
>
> No it should be 100 in the previous test.
>
> >
> > > +
> > > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > > +			delayed->push(ctrls);
> > > +
> > > +			delayed->frameStart(i);
> > > +
> > > +			ControlList result = delayed->get(i);
> > > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > > +			if (brightness != expected) {
> >
> > Are we're lucky and BRIGHTNESS.min() >= 4 ?
>
> I'm sorry I don't think I understand this question fully I think. We are
> not lucky here, before the loop we set the control to 4 and this is
> whats tested here in the first iteration. But the vale 4 is arbitrary
> and we can set it to any value between 0 and 255 before the loop and the
> test works.
>

What I mean was probably "are we lucky and BRIGHTNESS.min() != 4".
I would use a generic min() + 10 to make sure the test adapts if the
driver changes.

> >
> > > +				cerr << "Failed single control with delay"
> > > +				     << " frame " << i
> > > +				     << " expected " << expected
> > > +				     << " got " << brightness
> > > +				     << endl;
> > > +				return TestFail;
> > > +			}
> > > +
> > > +			expected = value;
> > > +		}
> > > +
> > > +		return TestPass;
> > > +	}
> > > +
> > > +	int dualControlsWithDelay(uint32_t startOffset)
> > > +	{
> > > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > > +			{ V4L2_CID_BRIGHTNESS, 1 },
> > > +			{ V4L2_CID_CONTRAST, 2 },
> > > +		};
> > > +		std::unique_ptr<DelayedControls> delayed =
> > > +			std::make_unique<DelayedControls>(dev_, delays);
> > > +
> > > +		ControlList ctrls;
> > > +
> >
> > Same :)
> >
> > > +		/* Reset control to value that will be first two frames in test. */
> > > +		int32_t expected = 200;
> > > +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> > > +		ctrls.set(V4L2_CID_CONTRAST, expected);
> > > +		delayed->reset(&ctrls);
> > > +
> > > +		/* Test dual control with delay. */
> > > +		for (int32_t i = 0; i < 100; i++) {
> > > +			uint32_t frame = startOffset + i;
> > > +
> >
> > Additional empty line
> >
> > > +			int32_t value = 10 + i;
> > > +
> > > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > > +			ctrls.set(V4L2_CID_CONTRAST, value);
> > > +			delayed->push(ctrls);
> > > +
> > > +			delayed->frameStart(frame);
> > > +
> > > +			ControlList result = delayed->get(frame);
> > > +
> >
> > In the previous functions you don't have an empty line here
> >
> > > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > > +			int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
> > > +			if (brightness != expected || contrast != expected) {
> > > +				cerr << "Failed dual controls"
> > > +				     << " frame " << frame
> > > +				     << " brightness " << brightness
> > > +				     << " contrast " << contrast
> > > +				     << " expected " << expected
> > > +				     << endl;
> > > +				return TestFail;
> > > +			}
> > > +
> > > +			expected = i < 1 ? expected : value - 1;
> >
> > Why do I expect this to fail ?
> >
> > expected = 200
> > 0:
> >         value = 10;
> >         set(BRIGHTNESS, 10);
> >         set(CONTRAST, 10);
> >
> >         frameStart(0);
> >         brightness = 200;
> >         contrast = 200;
> >         (200 == 200; 200 == 200);
> >         expected = 200;
> >
> > 1:
> >         value = 11;
> >         set(BRIGHTNESS, 11);
> >         set(CONTRAST, 11);
> >
> >         frameStart(1);
> >         brightness = 10; <- delay was 1
> >         contrast = 200;  <- delay was 2
> >         (10 != 200) <- Shouldn't this fail ?
> >
> >         expected = 10;
>
> I agree it's confusing :-) The idea of DelayedControls is not to apply
> the controls as soon as possible but to sync the setting of all controls
> of a group to the worst delay.
>
> The flow is:
>
> expected = 200
> 0:
>         value = 10;
>         set(BRIGHTNESS, 10);
>         set(CONTRAST, 10);
>
>         frameStart(0);
>         brightness = 200;
>         contrast = 200;
>         (200 == 200; 200 == 200);
>         expected = 200;
>
> 1:
>         value = 11;
>         set(BRIGHTNESS, 11);
>         set(CONTRAST, 11);
>
>         frameStart(1);
>         brightness = 200; <- brightness delay is 1 but for the group 2
>         contrast = 200;  <- delay was 2
>         (200 == 200; 200 == 200);
>         expected = 10;
>
> 2:
>         value = 12;
>         set(BRIGHTNESS, 12);
>         set(CONTRAST, 12);
>
>         frameStart(2);
>         brightness = 10;
>         contrast = 10;
>         (10 == 10; 10 == 10);
>         expected = 11;
>
> 3:
>         value = 13;
>         set(BRIGHTNESS, 13);
>         set(CONTRAST, 13);
>
>         frameStart(3);
>         brightness = 11;
>         contrast = 11;
>         (11 == 11; 11 == 11);
>         expected = 12;
>

Thanks, I will run a few iterations and better understand this.

Thanks
  j

> >
> > > +		}
> > > +
> > > +		return TestPass;
> > > +	}
> > > +
> > > +	int dualControlsMultiQueue()
> > > +	{
> > > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > > +			{ V4L2_CID_BRIGHTNESS, 1 },
> > > +			{ V4L2_CID_CONTRAST, 2 },
> > > +		};
> > > +		std::unique_ptr<DelayedControls> delayed =
> > > +			std::make_unique<DelayedControls>(dev_, delays);
> > > +
> > > +		ControlList ctrls;
> > > +
> > > +		/* Reset control to value that will be first two frames in test. */
> > > +		int32_t expected = 100;
> > > +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> > > +		ctrls.set(V4L2_CID_CONTRAST, expected);
> > > +		delayed->reset(&ctrls);
> > > +
> > > +		/*
> > > +		 * Queue all controls before any fake frame start. Note we
> > > +		 * can't queue up more then the delayed controls history size
> > > +		 * which is 16. Where one spot is used by the reset control.
> > > +		 */
> > > +		for (int32_t i = 0; i < 15; i++) {
> > > +			int32_t value = 10 + i;
> > > +
> > > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > > +			ctrls.set(V4L2_CID_CONTRAST, value);
> > > +			delayed->push(ctrls);
> > > +		}
> > > +
> > > +		/* Process all queued controls. */
> > > +		for (int32_t i = 0; i < 16; i++) {
> > > +			int32_t value = 10 + i;
> > > +
> > > +			delayed->frameStart(i);
> > > +
> > > +			ControlList result = delayed->get(i);
> > > +
> > > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > > +			int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
> > > +			if (brightness != expected || contrast != expected) {
> > > +				cerr << "Failed multi queue"
> > > +				     << " frame " << i
> > > +				     << " brightness " << brightness
> > > +				     << " contrast " << contrast
> > > +				     << " expected " << expected
> > > +				     << endl;
> > > +				return TestFail;
> > > +			}
> > > +
> > > +			expected = i < 1 ? expected : value - 1;
> > > +		}
> > > +
> > > +		return TestPass;
> > > +	}
> > > +
> > > +	int run()
> > > +	{
> > > +		int ret;
> > > +
> > > +		/* Test single control without delay. */
> > > +		ret = singleControlNoDelay();
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		/* Test single control with delay. */
> > > +		ret = singleControlWithDelay();
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		/* Test dual controls with different delays. */
> > > +		ret = dualControlsWithDelay(0);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		/* Test dual controls with non-zero sequence start. */
> > > +		ret = dualControlsWithDelay(10000);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		/* Test dual controls with sequence number wraparound. */
> > > +		ret = dualControlsWithDelay(UINT32_MAX - 50);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		/* Test control values produced faster then consumed. */
> > > +		ret = dualControlsMultiQueue();
> > > +		if (ret)
> > > +			return ret;
> >
> > Maybe an empty line ?
>
> Yes :-)
>
> >
> > Thanks
> >   j
> >
> > > +		return TestPass;
> > > +	}
> > > +
> > > +	void cleanup()
> > > +	{
> > > +		delete dev_;
> > > +	}
> > > +
> > > +private:
> > > +	std::unique_ptr<DeviceEnumerator> enumerator_;
> > > +	std::shared_ptr<MediaDevice> media_;
> > > +	V4L2VideoDevice *dev_;
> > > +};
> > > +
> > > +TEST_REGISTER(DelayedControlsTest)
> > > diff --git a/test/meson.build b/test/meson.build
> > > index 0a1d434e399641bb..a683a657a439b4ff 100644
> > > --- a/test/meson.build
> > > +++ b/test/meson.build
> > > @@ -25,6 +25,7 @@ public_tests = [
> > >  internal_tests = [
> > >      ['byte-stream-buffer',              'byte-stream-buffer.cpp'],
> > >      ['camera-sensor',                   'camera-sensor.cpp'],
> > > +    ['delayed_contols',                 'delayed_contols.cpp'],
> > >      ['event',                           'event.cpp'],
> > >      ['event-dispatcher',                'event-dispatcher.cpp'],
> > >      ['event-thread',                    'event-thread.cpp'],
> > > --
> > > 2.29.2
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Niklas Söderlund Nov. 24, 2020, 11:46 a.m. UTC | #4 
Hi Jacopo,

On 2020-11-24 12:31:18 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Mon, Nov 23, 2020 at 10:17:25PM +0100, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your feedback.
> >
> > On 2020-11-18 15:12:53 +0100, Jacopo Mondi wrote:
> > > Hi Niklas,
> > >
> > > On Tue, Nov 10, 2020 at 01:27:04AM +0100, Niklas Söderlund wrote:
> > > > Add a test-case for DelayedControls that exercise the setting of
> > > > controls and reading back what controls where used for a particular
> > > > frame. Also exercise corner case such as a V4L2 devices that do not
> > > > reset it sequence number to 0 at stream on and sequence number wrapping
> > >
> > > s/it/its
> > >
> > > > around the uint32_t value space.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > ---
> > > >  test/delayed_contols.cpp | 307 +++++++++++++++++++++++++++++++++++++++
> > > >  test/meson.build         |   1 +
> > > >  2 files changed, 308 insertions(+)
> > > >  create mode 100644 test/delayed_contols.cpp
> > > >
> > > > diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
> > > > new file mode 100644
> > > > index 0000000000000000..e71da6662c30bbdc
> > > > --- /dev/null
> > > > +++ b/test/delayed_contols.cpp
> > > > @@ -0,0 +1,307 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2020, Google Inc.
> > > > + *
> > > > + * libcamera delayed controls tests
> > > > + */
> > > > +
> > > > +#include <iostream>
> > > > +
> > > > +#include "libcamera/internal/delayed_controls.h"
> > > > +#include "libcamera/internal/device_enumerator.h"
> > > > +#include "libcamera/internal/media_device.h"
> > > > +#include "libcamera/internal/v4l2_videodevice.h"
> > > > +
> > > > +#include "test.h"
> > > > +
> > > > +using namespace std;
> > > > +using namespace libcamera;
> > > > +
> > > > +class DelayedControlsTest : public Test
> > > > +{
> > > > +public:
> > > > +	DelayedControlsTest()
> > > > +		: dev_(nullptr)
> > > > +	{
> > > > +	}
> > > > +
> > > > +	int init()
> > > > +	{
> > > > +		enumerator_ = DeviceEnumerator::create();
> > > > +		if (!enumerator_) {
> > > > +			cerr << "Failed to create device enumerator" << endl;
> > > > +			return TestFail;
> > > > +		}
> > > > +
> > > > +		if (enumerator_->enumerate()) {
> > > > +			cerr << "Failed to enumerate media devices" << endl;
> > > > +			return TestFail;
> > > > +		}
> > > > +
> > > > +		DeviceMatch dm("vivid");
> > > > +		dm.add("vivid-000-vid-cap");
> > > > +
> > > > +		media_ = enumerator_->search(dm);
> > > > +		if (!media_) {
> > > > +			cerr << "vivid video device found" << endl;
> > > > +			return TestSkip;
> > > > +		}
> > > > +
> > > > +		MediaEntity *entity = media_->getEntityByName("vivid-000-vid-cap");
> > > > +		dev_ = new V4L2VideoDevice(entity->deviceNode());
> > > > +		if (dev_->open()) {
> > > > +			cerr << "Failed to open video device" << endl;
> > > > +			return TestFail;
> > > > +		}
> > > > +
> > > > +		const ControlInfoMap &infoMap = dev_->controls();
> > > > +
> > > > +		/* Test control enumeration. */
> > > > +		if (infoMap.empty()) {
> > > > +			cerr << "Failed to enumerate controls" << endl;
> > > > +			return TestFail;
> > > > +		}
> > > > +
> > > > +		if (infoMap.find(V4L2_CID_BRIGHTNESS) == infoMap.end() ||
> > > > +		    infoMap.find(V4L2_CID_CONTRAST) == infoMap.end()) {
> > > > +			cerr << "Missing controls" << endl;
> > > > +			return TestFail;
> > > > +		}
> > > > +
> > > > +		ControlList ctrls = dev_->getControls({ V4L2_CID_BRIGHTNESS });
> > >
> > > What's ctrls for here ?
> >
> > Good catch, I will remove it for next version.
> >
> > >
> > > > +
> > > > +		return TestPass;
> > > > +	}
> > > > +
> > > > +	int singleControlNoDelay()
> > > > +	{
> > > > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > > > +			{ V4L2_CID_BRIGHTNESS, 0 },
> > > > +		};
> > > > +		std::unique_ptr<DelayedControls> delayed =
> > > > +			std::make_unique<DelayedControls>(dev_, delays);
> > >
> > > Should this just be
> > >                 DelayedControls delayed(dev_, delays) ?
> > > it's not used outside of the function
> >
> > It could, I have no real preference. I picked this as I wanted to test
> > some things during development and then I left it as such. Do you see an
> > particular gain in reworking this?
> >
> 
> It is more that I don't see any gain in using a unique_ptr. The scope
> of the variable is local to the function, there's no lifetime
> management at play, so allocating it on the stack has the same exact
> effect (I won't go and mention the overhead related to using smart
> pointer in term of performaces/memory occupation, as I cannot quantify
> them for real and this is a test, so it's not a big deal at all).

I can rework it for v4 if it bothers you :-)

> 
> > >
> > > > +
> > >
> > > Additional empty line
> >
> > Thanks.
> >
> > >
> > > > +		ControlList ctrls;
> > >
> > > Do we want to create the ControlList with the device info map for
> > > additional validation ?
> > >                 ControlList ctrl(dev_->controls());
> >
> > We could, but I don't see the value in it as the goal is not to test
> > ControlList. The controls are carefully selected to have a value range
> > of [min=0 max=255 step=1]. If this ever changes this test will fail in
> > other ways.
> 
> Well, it will fail as you use hardcoded 1 and 255. If they're replaced
> by min() and max() the test should be able to pass even if the
> underlying driver implementation changes.

Yes and No ;-)

It will fail if the value space is not 0 - 255 as a lot of the loops 
used in this whole file depends on it to not go out of bounds. I agree 
if it was only here and in the example below I would switch to min() but 
as the whole file depends on [min=0 max=255 step=1] I think this is 
easier to read then mixing the two styles.

> 
> >
> > >
> > > > +
> > > > +		/* Reset control to value not used in test. */
> > > > +		ctrls.set(V4L2_CID_BRIGHTNESS, 1);
> > > > +		delayed->reset(&ctrls);
> > >
> > > I would use the ControlInfo.min()
> > >
> > > Do you expect after reset() that BRIGHTNESS is equal to 1 ?
> >
> > Yes.
> >
> 
> Unlikely to change, but I would avoid using hardcoded value.
> Also setting it to min() makes the code more explicit.
> 
> > >
> > > > +
> > > > +		/* Test control without delay are set at once. */
> > > > +		for (int32_t i = 0; i < 10; i++) {
> > > > +			int32_t value = 100 + i;
> > > > +
> > > > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > > > +			delayed->push(ctrls);
> > > > +
> > > > +			delayed->frameStart(i);
> > > > +
> > > > +			ControlList result = delayed->get(i);
> > > > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > > > +			if (brightness != value) {
> > > > +				cerr << "Failed single control without delay"
> > > > +				     << " frame " << i
> > > > +				     << " expected " << value
> > > > +				     << " got " << brightness
> > > > +				     << endl;
> > > > +				return TestFail;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		return TestPass;
> > > > +	}
> > > > +
> > > > +	int singleControlWithDelay()
> > > > +	{
> > > > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > > > +			{ V4L2_CID_BRIGHTNESS, 1 },
> > > > +		};
> > > > +		std::unique_ptr<DelayedControls> delayed =
> > > > +			std::make_unique<DelayedControls>(dev_, delays);
> > > > +
> > > > +		ControlList ctrls;
> > >
> > > Same questions
> > >
> > > > +
> > > > +		/* Reset control to value that will be first in test. */
> > > > +		int32_t expected = 4;
> > > > +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> > > > +		delayed->reset(&ctrls);
> > > > +
> > > > +		/* Test single control with delay. */
> > > > +		for (int32_t i = 0; i < 100; i++) {
> > > > +			int32_t value = 10 + i;
> > >
> > > Is swapping 10 and 100 in the for loop compared to the previous test
> > > intended ?
> >
> > No it should be 100 in the previous test.
> >
> > >
> > > > +
> > > > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > > > +			delayed->push(ctrls);
> > > > +
> > > > +			delayed->frameStart(i);
> > > > +
> > > > +			ControlList result = delayed->get(i);
> > > > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > > > +			if (brightness != expected) {
> > >
> > > Are we're lucky and BRIGHTNESS.min() >= 4 ?
> >
> > I'm sorry I don't think I understand this question fully I think. We are
> > not lucky here, before the loop we set the control to 4 and this is
> > whats tested here in the first iteration. But the vale 4 is arbitrary
> > and we can set it to any value between 0 and 255 before the loop and the
> > test works.
> >
> 
> What I mean was probably "are we lucky and BRIGHTNESS.min() != 4".
> I would use a generic min() + 10 to make sure the test adapts if the
> driver changes.
> 
> > >
> > > > +				cerr << "Failed single control with delay"
> > > > +				     << " frame " << i
> > > > +				     << " expected " << expected
> > > > +				     << " got " << brightness
> > > > +				     << endl;
> > > > +				return TestFail;
> > > > +			}
> > > > +
> > > > +			expected = value;
> > > > +		}
> > > > +
> > > > +		return TestPass;
> > > > +	}
> > > > +
> > > > +	int dualControlsWithDelay(uint32_t startOffset)
> > > > +	{
> > > > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > > > +			{ V4L2_CID_BRIGHTNESS, 1 },
> > > > +			{ V4L2_CID_CONTRAST, 2 },
> > > > +		};
> > > > +		std::unique_ptr<DelayedControls> delayed =
> > > > +			std::make_unique<DelayedControls>(dev_, delays);
> > > > +
> > > > +		ControlList ctrls;
> > > > +
> > >
> > > Same :)
> > >
> > > > +		/* Reset control to value that will be first two frames in test. */
> > > > +		int32_t expected = 200;
> > > > +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> > > > +		ctrls.set(V4L2_CID_CONTRAST, expected);
> > > > +		delayed->reset(&ctrls);
> > > > +
> > > > +		/* Test dual control with delay. */
> > > > +		for (int32_t i = 0; i < 100; i++) {
> > > > +			uint32_t frame = startOffset + i;
> > > > +
> > >
> > > Additional empty line
> > >
> > > > +			int32_t value = 10 + i;
> > > > +
> > > > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > > > +			ctrls.set(V4L2_CID_CONTRAST, value);
> > > > +			delayed->push(ctrls);
> > > > +
> > > > +			delayed->frameStart(frame);
> > > > +
> > > > +			ControlList result = delayed->get(frame);
> > > > +
> > >
> > > In the previous functions you don't have an empty line here
> > >
> > > > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > > > +			int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
> > > > +			if (brightness != expected || contrast != expected) {
> > > > +				cerr << "Failed dual controls"
> > > > +				     << " frame " << frame
> > > > +				     << " brightness " << brightness
> > > > +				     << " contrast " << contrast
> > > > +				     << " expected " << expected
> > > > +				     << endl;
> > > > +				return TestFail;
> > > > +			}
> > > > +
> > > > +			expected = i < 1 ? expected : value - 1;
> > >
> > > Why do I expect this to fail ?
> > >
> > > expected = 200
> > > 0:
> > >         value = 10;
> > >         set(BRIGHTNESS, 10);
> > >         set(CONTRAST, 10);
> > >
> > >         frameStart(0);
> > >         brightness = 200;
> > >         contrast = 200;
> > >         (200 == 200; 200 == 200);
> > >         expected = 200;
> > >
> > > 1:
> > >         value = 11;
> > >         set(BRIGHTNESS, 11);
> > >         set(CONTRAST, 11);
> > >
> > >         frameStart(1);
> > >         brightness = 10; <- delay was 1
> > >         contrast = 200;  <- delay was 2
> > >         (10 != 200) <- Shouldn't this fail ?
> > >
> > >         expected = 10;
> >
> > I agree it's confusing :-) The idea of DelayedControls is not to apply
> > the controls as soon as possible but to sync the setting of all controls
> > of a group to the worst delay.
> >
> > The flow is:
> >
> > expected = 200
> > 0:
> >         value = 10;
> >         set(BRIGHTNESS, 10);
> >         set(CONTRAST, 10);
> >
> >         frameStart(0);
> >         brightness = 200;
> >         contrast = 200;
> >         (200 == 200; 200 == 200);
> >         expected = 200;
> >
> > 1:
> >         value = 11;
> >         set(BRIGHTNESS, 11);
> >         set(CONTRAST, 11);
> >
> >         frameStart(1);
> >         brightness = 200; <- brightness delay is 1 but for the group 2
> >         contrast = 200;  <- delay was 2
> >         (200 == 200; 200 == 200);
> >         expected = 10;
> >
> > 2:
> >         value = 12;
> >         set(BRIGHTNESS, 12);
> >         set(CONTRAST, 12);
> >
> >         frameStart(2);
> >         brightness = 10;
> >         contrast = 10;
> >         (10 == 10; 10 == 10);
> >         expected = 11;
> >
> > 3:
> >         value = 13;
> >         set(BRIGHTNESS, 13);
> >         set(CONTRAST, 13);
> >
> >         frameStart(3);
> >         brightness = 11;
> >         contrast = 11;
> >         (11 == 11; 11 == 11);
> >         expected = 12;
> >
> 
> Thanks, I will run a few iterations and better understand this.
> 
> Thanks
>   j
> 
> > >
> > > > +		}
> > > > +
> > > > +		return TestPass;
> > > > +	}
> > > > +
> > > > +	int dualControlsMultiQueue()
> > > > +	{
> > > > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > > > +			{ V4L2_CID_BRIGHTNESS, 1 },
> > > > +			{ V4L2_CID_CONTRAST, 2 },
> > > > +		};
> > > > +		std::unique_ptr<DelayedControls> delayed =
> > > > +			std::make_unique<DelayedControls>(dev_, delays);
> > > > +
> > > > +		ControlList ctrls;
> > > > +
> > > > +		/* Reset control to value that will be first two frames in test. */
> > > > +		int32_t expected = 100;
> > > > +		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
> > > > +		ctrls.set(V4L2_CID_CONTRAST, expected);
> > > > +		delayed->reset(&ctrls);
> > > > +
> > > > +		/*
> > > > +		 * Queue all controls before any fake frame start. Note we
> > > > +		 * can't queue up more then the delayed controls history size
> > > > +		 * which is 16. Where one spot is used by the reset control.
> > > > +		 */
> > > > +		for (int32_t i = 0; i < 15; i++) {
> > > > +			int32_t value = 10 + i;
> > > > +
> > > > +			ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > > > +			ctrls.set(V4L2_CID_CONTRAST, value);
> > > > +			delayed->push(ctrls);
> > > > +		}
> > > > +
> > > > +		/* Process all queued controls. */
> > > > +		for (int32_t i = 0; i < 16; i++) {
> > > > +			int32_t value = 10 + i;
> > > > +
> > > > +			delayed->frameStart(i);
> > > > +
> > > > +			ControlList result = delayed->get(i);
> > > > +
> > > > +			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
> > > > +			int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
> > > > +			if (brightness != expected || contrast != expected) {
> > > > +				cerr << "Failed multi queue"
> > > > +				     << " frame " << i
> > > > +				     << " brightness " << brightness
> > > > +				     << " contrast " << contrast
> > > > +				     << " expected " << expected
> > > > +				     << endl;
> > > > +				return TestFail;
> > > > +			}
> > > > +
> > > > +			expected = i < 1 ? expected : value - 1;
> > > > +		}
> > > > +
> > > > +		return TestPass;
> > > > +	}
> > > > +
> > > > +	int run()
> > > > +	{
> > > > +		int ret;
> > > > +
> > > > +		/* Test single control without delay. */
> > > > +		ret = singleControlNoDelay();
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		/* Test single control with delay. */
> > > > +		ret = singleControlWithDelay();
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		/* Test dual controls with different delays. */
> > > > +		ret = dualControlsWithDelay(0);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		/* Test dual controls with non-zero sequence start. */
> > > > +		ret = dualControlsWithDelay(10000);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		/* Test dual controls with sequence number wraparound. */
> > > > +		ret = dualControlsWithDelay(UINT32_MAX - 50);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		/* Test control values produced faster then consumed. */
> > > > +		ret = dualControlsMultiQueue();
> > > > +		if (ret)
> > > > +			return ret;
> > >
> > > Maybe an empty line ?
> >
> > Yes :-)
> >
> > >
> > > Thanks
> > >   j
> > >
> > > > +		return TestPass;
> > > > +	}
> > > > +
> > > > +	void cleanup()
> > > > +	{
> > > > +		delete dev_;
> > > > +	}
> > > > +
> > > > +private:
> > > > +	std::unique_ptr<DeviceEnumerator> enumerator_;
> > > > +	std::shared_ptr<MediaDevice> media_;
> > > > +	V4L2VideoDevice *dev_;
> > > > +};
> > > > +
> > > > +TEST_REGISTER(DelayedControlsTest)
> > > > diff --git a/test/meson.build b/test/meson.build
> > > > index 0a1d434e399641bb..a683a657a439b4ff 100644
> > > > --- a/test/meson.build
> > > > +++ b/test/meson.build
> > > > @@ -25,6 +25,7 @@ public_tests = [
> > > >  internal_tests = [
> > > >      ['byte-stream-buffer',              'byte-stream-buffer.cpp'],
> > > >      ['camera-sensor',                   'camera-sensor.cpp'],
> > > > +    ['delayed_contols',                 'delayed_contols.cpp'],
> > > >      ['event',                           'event.cpp'],
> > > >      ['event-dispatcher',                'event-dispatcher.cpp'],
> > > >      ['event-thread',                    'event-thread.cpp'],
> > > > --
> > > > 2.29.2
> > > >
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel@lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund

Patch
diff mbox series 

diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
new file mode 100644
index 0000000000000000..e71da6662c30bbdc
--- /dev/null
+++ b/test/delayed_contols.cpp
@@ -0,0 +1,307 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2020, Google Inc.
+ *
+ * libcamera delayed controls tests
+ */
+
+#include <iostream>
+
+#include "libcamera/internal/delayed_controls.h"
+#include "libcamera/internal/device_enumerator.h"
+#include "libcamera/internal/media_device.h"
+#include "libcamera/internal/v4l2_videodevice.h"
+
+#include "test.h"
+
+using namespace std;
+using namespace libcamera;
+
+class DelayedControlsTest : public Test
+{
+public:
+	DelayedControlsTest()
+		: dev_(nullptr)
+	{
+	}
+
+	int init()
+	{
+		enumerator_ = DeviceEnumerator::create();
+		if (!enumerator_) {
+			cerr << "Failed to create device enumerator" << endl;
+			return TestFail;
+		}
+
+		if (enumerator_->enumerate()) {
+			cerr << "Failed to enumerate media devices" << endl;
+			return TestFail;
+		}
+
+		DeviceMatch dm("vivid");
+		dm.add("vivid-000-vid-cap");
+
+		media_ = enumerator_->search(dm);
+		if (!media_) {
+			cerr << "vivid video device found" << endl;
+			return TestSkip;
+		}
+
+		MediaEntity *entity = media_->getEntityByName("vivid-000-vid-cap");
+		dev_ = new V4L2VideoDevice(entity->deviceNode());
+		if (dev_->open()) {
+			cerr << "Failed to open video device" << endl;
+			return TestFail;
+		}
+
+		const ControlInfoMap &infoMap = dev_->controls();
+
+		/* Test control enumeration. */
+		if (infoMap.empty()) {
+			cerr << "Failed to enumerate controls" << endl;
+			return TestFail;
+		}
+
+		if (infoMap.find(V4L2_CID_BRIGHTNESS) == infoMap.end() ||
+		    infoMap.find(V4L2_CID_CONTRAST) == infoMap.end()) {
+			cerr << "Missing controls" << endl;
+			return TestFail;
+		}
+
+		ControlList ctrls = dev_->getControls({ V4L2_CID_BRIGHTNESS });
+
+		return TestPass;
+	}
+
+	int singleControlNoDelay()
+	{
+		std::unordered_map<uint32_t, unsigned int> delays = {
+			{ V4L2_CID_BRIGHTNESS, 0 },
+		};
+		std::unique_ptr<DelayedControls> delayed =
+			std::make_unique<DelayedControls>(dev_, delays);
+
+		ControlList ctrls;
+
+		/* Reset control to value not used in test. */
+		ctrls.set(V4L2_CID_BRIGHTNESS, 1);
+		delayed->reset(&ctrls);
+
+		/* Test control without delay are set at once. */
+		for (int32_t i = 0; i < 10; i++) {
+			int32_t value = 100 + i;
+
+			ctrls.set(V4L2_CID_BRIGHTNESS, value);
+			delayed->push(ctrls);
+
+			delayed->frameStart(i);
+
+			ControlList result = delayed->get(i);
+			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
+			if (brightness != value) {
+				cerr << "Failed single control without delay"
+				     << " frame " << i
+				     << " expected " << value
+				     << " got " << brightness
+				     << endl;
+				return TestFail;
+			}
+		}
+
+		return TestPass;
+	}
+
+	int singleControlWithDelay()
+	{
+		std::unordered_map<uint32_t, unsigned int> delays = {
+			{ V4L2_CID_BRIGHTNESS, 1 },
+		};
+		std::unique_ptr<DelayedControls> delayed =
+			std::make_unique<DelayedControls>(dev_, delays);
+
+		ControlList ctrls;
+
+		/* Reset control to value that will be first in test. */
+		int32_t expected = 4;
+		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
+		delayed->reset(&ctrls);
+
+		/* Test single control with delay. */
+		for (int32_t i = 0; i < 100; i++) {
+			int32_t value = 10 + i;
+
+			ctrls.set(V4L2_CID_BRIGHTNESS, value);
+			delayed->push(ctrls);
+
+			delayed->frameStart(i);
+
+			ControlList result = delayed->get(i);
+			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
+			if (brightness != expected) {
+				cerr << "Failed single control with delay"
+				     << " frame " << i
+				     << " expected " << expected
+				     << " got " << brightness
+				     << endl;
+				return TestFail;
+			}
+
+			expected = value;
+		}
+
+		return TestPass;
+	}
+
+	int dualControlsWithDelay(uint32_t startOffset)
+	{
+		std::unordered_map<uint32_t, unsigned int> delays = {
+			{ V4L2_CID_BRIGHTNESS, 1 },
+			{ V4L2_CID_CONTRAST, 2 },
+		};
+		std::unique_ptr<DelayedControls> delayed =
+			std::make_unique<DelayedControls>(dev_, delays);
+
+		ControlList ctrls;
+
+		/* Reset control to value that will be first two frames in test. */
+		int32_t expected = 200;
+		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
+		ctrls.set(V4L2_CID_CONTRAST, expected);
+		delayed->reset(&ctrls);
+
+		/* Test dual control with delay. */
+		for (int32_t i = 0; i < 100; i++) {
+			uint32_t frame = startOffset + i;
+
+			int32_t value = 10 + i;
+
+			ctrls.set(V4L2_CID_BRIGHTNESS, value);
+			ctrls.set(V4L2_CID_CONTRAST, value);
+			delayed->push(ctrls);
+
+			delayed->frameStart(frame);
+
+			ControlList result = delayed->get(frame);
+
+			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
+			int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
+			if (brightness != expected || contrast != expected) {
+				cerr << "Failed dual controls"
+				     << " frame " << frame
+				     << " brightness " << brightness
+				     << " contrast " << contrast
+				     << " expected " << expected
+				     << endl;
+				return TestFail;
+			}
+
+			expected = i < 1 ? expected : value - 1;
+		}
+
+		return TestPass;
+	}
+
+	int dualControlsMultiQueue()
+	{
+		std::unordered_map<uint32_t, unsigned int> delays = {
+			{ V4L2_CID_BRIGHTNESS, 1 },
+			{ V4L2_CID_CONTRAST, 2 },
+		};
+		std::unique_ptr<DelayedControls> delayed =
+			std::make_unique<DelayedControls>(dev_, delays);
+
+		ControlList ctrls;
+
+		/* Reset control to value that will be first two frames in test. */
+		int32_t expected = 100;
+		ctrls.set(V4L2_CID_BRIGHTNESS, expected);
+		ctrls.set(V4L2_CID_CONTRAST, expected);
+		delayed->reset(&ctrls);
+
+		/*
+		 * Queue all controls before any fake frame start. Note we
+		 * can't queue up more then the delayed controls history size
+		 * which is 16. Where one spot is used by the reset control.
+		 */
+		for (int32_t i = 0; i < 15; i++) {
+			int32_t value = 10 + i;
+
+			ctrls.set(V4L2_CID_BRIGHTNESS, value);
+			ctrls.set(V4L2_CID_CONTRAST, value);
+			delayed->push(ctrls);
+		}
+
+		/* Process all queued controls. */
+		for (int32_t i = 0; i < 16; i++) {
+			int32_t value = 10 + i;
+
+			delayed->frameStart(i);
+
+			ControlList result = delayed->get(i);
+
+			int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();
+			int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
+			if (brightness != expected || contrast != expected) {
+				cerr << "Failed multi queue"
+				     << " frame " << i
+				     << " brightness " << brightness
+				     << " contrast " << contrast
+				     << " expected " << expected
+				     << endl;
+				return TestFail;
+			}
+
+			expected = i < 1 ? expected : value - 1;
+		}
+
+		return TestPass;
+	}
+
+	int run()
+	{
+		int ret;
+
+		/* Test single control without delay. */
+		ret = singleControlNoDelay();
+		if (ret)
+			return ret;
+
+		/* Test single control with delay. */
+		ret = singleControlWithDelay();
+		if (ret)
+			return ret;
+
+		/* Test dual controls with different delays. */
+		ret = dualControlsWithDelay(0);
+		if (ret)
+			return ret;
+
+		/* Test dual controls with non-zero sequence start. */
+		ret = dualControlsWithDelay(10000);
+		if (ret)
+			return ret;
+
+		/* Test dual controls with sequence number wraparound. */
+		ret = dualControlsWithDelay(UINT32_MAX - 50);
+		if (ret)
+			return ret;
+
+		/* Test control values produced faster then consumed. */
+		ret = dualControlsMultiQueue();
+		if (ret)
+			return ret;
+		return TestPass;
+	}
+
+	void cleanup()
+	{
+		delete dev_;
+	}
+
+private:
+	std::unique_ptr<DeviceEnumerator> enumerator_;
+	std::shared_ptr<MediaDevice> media_;
+	V4L2VideoDevice *dev_;
+};
+
+TEST_REGISTER(DelayedControlsTest)
diff --git a/test/meson.build b/test/meson.build
index 0a1d434e399641bb..a683a657a439b4ff 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -25,6 +25,7 @@  public_tests = [
 internal_tests = [
     ['byte-stream-buffer',              'byte-stream-buffer.cpp'],
     ['camera-sensor',                   'camera-sensor.cpp'],
+    ['delayed_contols',                 'delayed_contols.cpp'],
     ['event',                           'event.cpp'],
     ['event-dispatcher',                'event-dispatcher.cpp'],
     ['event-thread',                    'event-thread.cpp'],