[libcamera-devel] [PATCH v2 2/8] test: delayed_controls: Add test case for DelayedControls
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Nov 23 22:17:25 CET 2020
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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list