[libcamera-devel] [PATCH v2 2/8] test: delayed_controls: Add test case for DelayedControls
Jacopo Mondi
jacopo at jmondi.org
Tue Nov 24 12:31:18 CET 2020
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 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?
>
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 at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
More information about the libcamera-devel
mailing list