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