[{"id":14494,"web_url":"https://patchwork.libcamera.org/comment/14494/","msgid":"","date":"2021-01-10T15:21:08","subject":"Re: [libcamera-devel] [PATCH v4 2/8] test: delayed_controls: Add\n\ttest case for DelayedControls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Tue, Dec 15, 2020 at 01:48:05AM +0100, Niklas Söderlund wrote:\n> Add a test-case for DelayedControls that exercise the setting of\n\ns/test-case/test case/\ns/exercise/exercises/\n\n> controls and reading back what controls were used for a particular\n> frame. Also exercise corner case such as a V4L2 devices that do not\n\ns/case/cases/\ns/devices/device/\ns/do/does/\n\n> reset its sequence number to 0 at stream on and sequence number wrapping\n> around the uint32_t value space.\n> \n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> ---\n> * Changes since v3\n> - Update commit message.\n> - Update header documentation.\n> - Add protected and override.\n> - Use unsigned int instead of int32_t in loops.\n> \n> * Changes since v2\n> - Remove a unused LoC.\n> - Add and remove blank lines.\n> - Align number of loop iterations.\n> ---\n> test/delayed_contols.cpp | 304 +++++++++++++++++++++++++++++++++++++++\n> test/meson.build | 1 +\n> 2 files changed, 305 insertions(+)\n> create mode 100644 test/delayed_contols.cpp\n> \n> diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp\n> new file mode 100644\n> index 0000000000000000..0a2b47aebf06b7fb\n> --- /dev/null\n> +++ b/test/delayed_contols.cpp\n> @@ -0,0 +1,304 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * delayed_controls.cpp - libcamera delayed controls test\n> + */\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/delayed_controls.h\"\n> +#include \"libcamera/internal/device_enumerator.h\"\n> +#include \"libcamera/internal/media_device.h\"\n> +#include \"libcamera/internal/v4l2_videodevice.h\"\n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +class DelayedControlsTest : public Test\n> +{\n> +public:\n> +\tDelayedControlsTest()\n> +\t\t: dev_(nullptr)\n> +\t{\n> +\t}\n> +\n> +protected:\n> +\tint init() override\n> +\t{\n> +\t\tenumerator_ = DeviceEnumerator::create();\n> +\t\tif (!enumerator_) {\n> +\t\t\tcerr << \"Failed to create device enumerator\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (enumerator_->enumerate()) {\n> +\t\t\tcerr << \"Failed to enumerate media devices\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tDeviceMatch dm(\"vivid\");\n> +\t\tdm.add(\"vivid-000-vid-cap\");\n> +\n> +\t\tmedia_ = enumerator_->search(dm);\n> +\t\tif (!media_) {\n> +\t\t\tcerr << \"vivid video device found\" << endl;\n> +\t\t\treturn TestSkip;\n> +\t\t}\n> +\n> +\t\tMediaEntity *entity = media_->getEntityByName(\"vivid-000-vid-cap\");\n> +\t\tdev_ = new V4L2VideoDevice(entity->deviceNode());\n\nYou can use V4L2VideoDevice::fromEntityName() and turn dev_ into a\nstd::unique_ptr<>.\n\n> +\t\tif (dev_->open()) {\n> +\t\t\tcerr << \"Failed to open video device\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tconst ControlInfoMap &infoMap = dev_->controls();\n> +\n> +\t\t/* Test control enumeration. */\n\nI'd write\n\n\t\t/* Make sure the controls we require are present. */\n\nas this isn't a test.\n\n> +\t\tif (infoMap.empty()) {\n> +\t\t\tcerr << \"Failed to enumerate controls\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (infoMap.find(V4L2_CID_BRIGHTNESS) == infoMap.end() ||\n> +\t\t infoMap.find(V4L2_CID_CONTRAST) == infoMap.end()) {\n> +\t\t\tcerr << \"Missing controls\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint singleControlNoDelay()\n> +\t{\n> +\t\tstd::unordered_map delays = {\n> +\t\t\t{ V4L2_CID_BRIGHTNESS, 0 },\n> +\t\t};\n> +\t\tstd::unique_ptr delayed =\n> +\t\t\tstd::make_unique(dev_, delays);\n> +\t\tControlList ctrls;\n> +\n> +\t\t/* Reset control to value not used in test. */\n> +\t\tctrls.set(V4L2_CID_BRIGHTNESS, 1);\n> +\t\tdev_->setControls(&ctrls);\n> +\n> +\t\t/* Test control without delay are set at once. */\n> +\t\tfor (int32_t i = 0; i < 100; i++) {\n\nunsigned int i would do. Same below.\n\n> +\t\t\tint32_t value = 100 + i;\n> +\n> +\t\t\tctrls.set(V4L2_CID_BRIGHTNESS, value);\n> +\t\t\tdelayed->push(ctrls);\n> +\n> +\t\t\tdelayed->applyControls(i);\n> +\n> +\t\t\tControlList result = delayed->get(i);\n> +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get();\n\nShould we also read controls back from dev_ to test that the correct\nvalues is applied ?\n\n> +\t\t\tif (brightness != value) {\n> +\t\t\t\tcerr << \"Failed single control without delay\"\n> +\t\t\t\t << \" frame \" << i\n> +\t\t\t\t << \" expected \" << value\n> +\t\t\t\t << \" got \" << brightness\n> +\t\t\t\t << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint singleControlWithDelay()\n> +\t{\n> +\t\tstd::unordered_map delays = {\n> +\t\t\t{ V4L2_CID_BRIGHTNESS, 1 },\n> +\t\t};\n> +\t\tstd::unique_ptr delayed =\n> +\t\t\tstd::make_unique(dev_, delays);\n> +\t\tControlList ctrls;\n> +\n> +\t\t/* Reset control to value that will be first in test. */\n> +\t\tint32_t expected = 4;\n> +\t\tctrls.set(V4L2_CID_BRIGHTNESS, expected);\n> +\t\tdev_->setControls(&ctrls);\n> +\t\tdelayed->reset();\n\nIf you set the controls on the device before creating the\nDelayedControls instance, you could save the reset() call. Same for the\nother test cases.\n\n> +\n> +\t\t/* Test single control with delay. */\n> +\t\tfor (int32_t i = 0; i < 100; i++) {\n> +\t\t\tint32_t value = 10 + i;\n> +\n> +\t\t\tctrls.set(V4L2_CID_BRIGHTNESS, value);\n> +\t\t\tdelayed->push(ctrls);\n> +\n> +\t\t\tdelayed->applyControls(i);\n> +\n> +\t\t\tControlList result = delayed->get(i);\n> +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get();\n> +\t\t\tif (brightness != expected) {\n> +\t\t\t\tcerr << \"Failed single control with delay\"\n> +\t\t\t\t << \" frame \" << i\n> +\t\t\t\t << \" expected \" << expected\n> +\t\t\t\t << \" got \" << brightness\n> +\t\t\t\t << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\texpected = value;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n\nI wonder if these two functions could be merged, with the delay passed\nas a parameter.\n\n> +\n> +\tint dualControlsWithDelay(uint32_t startOffset)\n> +\t{\n> +\t\tstd::unordered_map delays = {\n> +\t\t\t{ V4L2_CID_BRIGHTNESS, 1 },\n> +\t\t\t{ V4L2_CID_CONTRAST, 2 },\n> +\t\t};\n> +\t\tstd::unique_ptr delayed =\n> +\t\t\tstd::make_unique(dev_, delays);\n> +\t\tControlList ctrls;\n> +\n> +\t\t/* Reset control to value that will be first two frames in test. */\n> +\t\tint32_t expected = 200;\n> +\t\tctrls.set(V4L2_CID_BRIGHTNESS, expected);\n> +\t\tctrls.set(V4L2_CID_CONTRAST, expected);\n\nIt could be worth setting different values for the two controls (and\nbelow too) to test the right value is applied to the right control.\n\n> +\t\tdev_->setControls(&ctrls);\n> +\t\tdelayed->reset();\n> +\n> +\t\t/* Test dual control with delay. */\n> +\t\tfor (int32_t i = 0; i < 100; i++) {\n> +\t\t\tuint32_t frame = startOffset + i;\n> +\t\t\tint32_t value = 10 + i;\n> +\n> +\t\t\tctrls.set(V4L2_CID_BRIGHTNESS, value);\n> +\t\t\tctrls.set(V4L2_CID_CONTRAST, value);\n> +\t\t\tdelayed->push(ctrls);\n> +\n> +\t\t\tdelayed->applyControls(frame);\n> +\n> +\t\t\tControlList result = delayed->get(frame);\n> +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get();\n> +\t\t\tint32_t contrast = result.get(V4L2_CID_CONTRAST).get();\n> +\t\t\tif (brightness != expected || contrast != expected) {\n> +\t\t\t\tcerr << \"Failed dual controls\"\n> +\t\t\t\t << \" frame \" << frame\n> +\t\t\t\t << \" brightness \" << brightness\n> +\t\t\t\t << \" contrast \" << contrast\n> +\t\t\t\t << \" expected \" << expected\n> +\t\t\t\t << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\texpected = i < 1 ? expected : value - 1;\n\nThis puzzles me. get() is documented as returning the controls in effect\nat a sequence number. I'd expect this to work for the brightness control\nas the corresponding delay is 1, but not for the contrast control as the\ndelay is 2.\n\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint dualControlsMultiQueue()\n> +\t{\n> +\t\tstd::unordered_map delays = {\n> +\t\t\t{ V4L2_CID_BRIGHTNESS, 1 },\n> +\t\t\t{ V4L2_CID_CONTRAST, 2 },\n> +\t\t};\n> +\t\tstd::unique_ptr delayed =\n> +\t\t\tstd::make_unique(dev_, delays);\n> +\t\tControlList ctrls;\n> +\n> +\t\t/* Reset control to value that will be first two frames in test. */\n> +\t\tint32_t expected = 100;\n> +\t\tctrls.set(V4L2_CID_BRIGHTNESS, expected);\n> +\t\tctrls.set(V4L2_CID_CONTRAST, expected);\n> +\t\tdev_->setControls(&ctrls);\n> +\t\tdelayed->reset();\n> +\n> +\t\t/*\n> +\t\t * Queue all controls before any fake frame start. Note we\n> +\t\t * can't queue up more then the delayed controls history size\n> +\t\t * which is 16. Where one spot is used by the reset control.\n> +\t\t */\n> +\t\tfor (unsigned int i = 0; i < 15; i++) {\n> +\t\t\tint32_t value = 10 + i;\n> +\n> +\t\t\tctrls.set(V4L2_CID_BRIGHTNESS, value);\n> +\t\t\tctrls.set(V4L2_CID_CONTRAST, value);\n> +\t\t\tdelayed->push(ctrls);\n> +\t\t}\n> +\n> +\t\t/* Process all queued controls. */\n> +\t\tfor (unsigned int i = 0; i < 16; i++) {\n> +\t\t\tint32_t value = 10 + i;\n> +\n> +\t\t\tdelayed->applyControls(i);\n> +\n> +\t\t\tControlList result = delayed->get(i);\n> +\n> +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get();\n> +\t\t\tint32_t contrast = result.get(V4L2_CID_CONTRAST).get();\n> +\t\t\tif (brightness != expected || contrast != expected) {\n> +\t\t\t\tcerr << \"Failed multi queue\"\n> +\t\t\t\t << \" frame \" << i\n> +\t\t\t\t << \" brightness \" << brightness\n> +\t\t\t\t << \" contrast \" << contrast\n> +\t\t\t\t << \" expected \" << expected\n> +\t\t\t\t << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\texpected = i < 1 ? expected : value - 1;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\tint ret;\n> +\n> +\t\t/* Test single control without delay. */\n> +\t\tret = singleControlNoDelay();\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\t/* Test single control with delay. */\n> +\t\tret = singleControlWithDelay();\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\t/* Test dual controls with different delays. */\n> +\t\tret = dualControlsWithDelay(0);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\t/* Test dual controls with non-zero sequence start. */\n> +\t\tret = dualControlsWithDelay(10000);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\t/* Test dual controls with sequence number wraparound. */\n> +\t\tret = dualControlsWithDelay(UINT32_MAX - 50);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\t/* Test control values produced faster then consumed. */\n\ns/then/than/\n\n> +\t\tret = dualControlsMultiQueue();\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tvoid cleanup() override\n> +\t{\n> +\t\tdelete dev_;\n> +\t}\n> +\n> +private:\n> +\tstd::unique_ptr enumerator_;\n> +\tstd::shared_ptr media_;\n> +\tV4L2VideoDevice *dev_;\n> +};\n> +\n> +TEST_REGISTER(DelayedControlsTest)\n> diff --git a/test/meson.build b/test/meson.build\n> index 0a1d434e399641bb..a683a657a439b4ff 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -25,6 +25,7 @@ public_tests = [\n> internal_tests = [\n> ['byte-stream-buffer', 'byte-stream-buffer.cpp'],\n> ['camera-sensor', 'camera-sensor.cpp'],\n> + ['delayed_contols', 'delayed_contols.cpp'],\n> ['event', 'event.cpp'],\n> ['event-dispatcher', 'event-dispatcher.cpp'],\n> ['event-thread', 'event-thread.cpp'],","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 89906BD808\n\tfor ;\n\tSun, 10 Jan 2021 15:21:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03E1568069;\n\tSun, 10 Jan 2021 16:21:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D46C160523\n\tfor ;\n\tSun, 10 Jan 2021 16:21:22 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 548E2DA;\n\tSun, 10 Jan 2021 16:21:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uCSbzYYN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610292082;\n\tbh=qn+W+wS/1epY4mktZCnBM+ZiSC4t+6rUms/ok7CTtPQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uCSbzYYNDxrIRM1KnEvjjZ9wP89wwkurAVeKnYgVpK85IviLYgYb1AuuxsOG6BKnE\n\tALODqE7hLgg8h58kuVXYenkErs0g2x13cbv92wKVSEYmWZrzPUMSdr5cw1F378vnnQ\n\tVZXFKlnz6dOOs8jqi/zROTYwlWVDUyxkeIWCQf30=","Date":"Sun, 10 Jan 2021 17:21:08 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201215004811.602429-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v4 2/8] test: delayed_controls: Add\n\ttest case for DelayedControls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14563,"web_url":"https://patchwork.libcamera.org/comment/14563/","msgid":"","date":"2021-01-16T11:42:18","subject":"Re: [libcamera-devel] [PATCH v4 2/8] test: delayed_controls: Add\n\ttest case for DelayedControls","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your comments,\n\nOn 2021-01-10 17:21:08 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Tue, Dec 15, 2020 at 01:48:05AM +0100, Niklas Söderlund wrote:\n> > Add a test-case for DelayedControls that exercise the setting of\n> \n> s/test-case/test case/\n> s/exercise/exercises/\n> \n> > controls and reading back what controls were used for a particular\n> > frame. Also exercise corner case such as a V4L2 devices that do not\n> \n> s/case/cases/\n> s/devices/device/\n> s/do/does/\n> \n> > reset its sequence number to 0 at stream on and sequence number wrapping\n> > around the uint32_t value space.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > * Changes since v3\n> > - Update commit message.\n> > - Update header documentation.\n> > - Add protected and override.\n> > - Use unsigned int instead of int32_t in loops.\n> > \n> > * Changes since v2\n> > - Remove a unused LoC.\n> > - Add and remove blank lines.\n> > - Align number of loop iterations.\n> > ---\n> > test/delayed_contols.cpp | 304 +++++++++++++++++++++++++++++++++++++++\n> > test/meson.build | 1 +\n> > 2 files changed, 305 insertions(+)\n> > create mode 100644 test/delayed_contols.cpp\n> > \n> > diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp\n> > new file mode 100644\n> > index 0000000000000000..0a2b47aebf06b7fb\n> > --- /dev/null\n> > +++ b/test/delayed_contols.cpp\n> > @@ -0,0 +1,304 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * delayed_controls.cpp - libcamera delayed controls test\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/delayed_controls.h\"\n> > +#include \"libcamera/internal/device_enumerator.h\"\n> > +#include \"libcamera/internal/media_device.h\"\n> > +#include \"libcamera/internal/v4l2_videodevice.h\"\n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> > +using namespace libcamera;\n> > +\n> > +class DelayedControlsTest : public Test\n> > +{\n> > +public:\n> > +\tDelayedControlsTest()\n> > +\t\t: dev_(nullptr)\n> > +\t{\n> > +\t}\n> > +\n> > +protected:\n> > +\tint init() override\n> > +\t{\n> > +\t\tenumerator_ = DeviceEnumerator::create();\n> > +\t\tif (!enumerator_) {\n> > +\t\t\tcerr << \"Failed to create device enumerator\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (enumerator_->enumerate()) {\n> > +\t\t\tcerr << \"Failed to enumerate media devices\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tDeviceMatch dm(\"vivid\");\n> > +\t\tdm.add(\"vivid-000-vid-cap\");\n> > +\n> > +\t\tmedia_ = enumerator_->search(dm);\n> > +\t\tif (!media_) {\n> > +\t\t\tcerr << \"vivid video device found\" << endl;\n> > +\t\t\treturn TestSkip;\n> > +\t\t}\n> > +\n> > +\t\tMediaEntity *entity = media_->getEntityByName(\"vivid-000-vid-cap\");\n> > +\t\tdev_ = new V4L2VideoDevice(entity->deviceNode());\n> \n> You can use V4L2VideoDevice::fromEntityName() and turn dev_ into a\n> std::unique_ptr<>.\n> \n> > +\t\tif (dev_->open()) {\n> > +\t\t\tcerr << \"Failed to open video device\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tconst ControlInfoMap &infoMap = dev_->controls();\n> > +\n> > +\t\t/* Test control enumeration. */\n> \n> I'd write\n> \n> \t\t/* Make sure the controls we require are present. */\n> \n> as this isn't a test.\n> \n> > +\t\tif (infoMap.empty()) {\n> > +\t\t\tcerr << \"Failed to enumerate controls\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (infoMap.find(V4L2_CID_BRIGHTNESS) == infoMap.end() ||\n> > +\t\t infoMap.find(V4L2_CID_CONTRAST) == infoMap.end()) {\n> > +\t\t\tcerr << \"Missing controls\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tint singleControlNoDelay()\n> > +\t{\n> > +\t\tstd::unordered_map delays = {\n> > +\t\t\t{ V4L2_CID_BRIGHTNESS, 0 },\n> > +\t\t};\n> > +\t\tstd::unique_ptr delayed =\n> > +\t\t\tstd::make_unique(dev_, delays);\n> > +\t\tControlList ctrls;\n> > +\n> > +\t\t/* Reset control to value not used in test. */\n> > +\t\tctrls.set(V4L2_CID_BRIGHTNESS, 1);\n> > +\t\tdev_->setControls(&ctrls);\n> > +\n> > +\t\t/* Test control without delay are set at once. */\n> > +\t\tfor (int32_t i = 0; i < 100; i++) {\n> \n> unsigned int i would do. Same below.\n> \n> > +\t\t\tint32_t value = 100 + i;\n> > +\n> > +\t\t\tctrls.set(V4L2_CID_BRIGHTNESS, value);\n> > +\t\t\tdelayed->push(ctrls);\n> > +\n> > +\t\t\tdelayed->applyControls(i);\n> > +\n> > +\t\t\tControlList result = delayed->get(i);\n> > +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get();\n> \n> Should we also read controls back from dev_ to test that the correct\n> values is applied ?\n\nWe could, but is that not a test for V4L2VideoDevice?\n\n> \n> > +\t\t\tif (brightness != value) {\n> > +\t\t\t\tcerr << \"Failed single control without delay\"\n> > +\t\t\t\t << \" frame \" << i\n> > +\t\t\t\t << \" expected \" << value\n> > +\t\t\t\t << \" got \" << brightness\n> > +\t\t\t\t << endl;\n> > +\t\t\t\treturn TestFail;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tint singleControlWithDelay()\n> > +\t{\n> > +\t\tstd::unordered_map delays = {\n> > +\t\t\t{ V4L2_CID_BRIGHTNESS, 1 },\n> > +\t\t};\n> > +\t\tstd::unique_ptr delayed =\n> > +\t\t\tstd::make_unique(dev_, delays);\n> > +\t\tControlList ctrls;\n> > +\n> > +\t\t/* Reset control to value that will be first in test. */\n> > +\t\tint32_t expected = 4;\n> > +\t\tctrls.set(V4L2_CID_BRIGHTNESS, expected);\n> > +\t\tdev_->setControls(&ctrls);\n> > +\t\tdelayed->reset();\n> \n> If you set the controls on the device before creating the\n> DelayedControls instance, you could save the reset() call. Same for the\n> other test cases.\n\nTrue, but I want to test reset() here as I suspect the usage pattern in \npipeline will be to create the DelayedControls at init time but reset() \nthe controls at start() time.\n\n> \n> > +\n> > +\t\t/* Test single control with delay. */\n> > +\t\tfor (int32_t i = 0; i < 100; i++) {\n> > +\t\t\tint32_t value = 10 + i;\n> > +\n> > +\t\t\tctrls.set(V4L2_CID_BRIGHTNESS, value);\n> > +\t\t\tdelayed->push(ctrls);\n> > +\n> > +\t\t\tdelayed->applyControls(i);\n> > +\n> > +\t\t\tControlList result = delayed->get(i);\n> > +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get();\n> > +\t\t\tif (brightness != expected) {\n> > +\t\t\t\tcerr << \"Failed single control with delay\"\n> > +\t\t\t\t << \" frame \" << i\n> > +\t\t\t\t << \" expected \" << expected\n> > +\t\t\t\t << \" got \" << brightness\n> > +\t\t\t\t << endl;\n> > +\t\t\t\treturn TestFail;\n> > +\t\t\t}\n> > +\n> > +\t\t\texpected = value;\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> \n> I wonder if these two functions could be merged, with the delay passed\n> as a parameter.\n\nI'm sure they could but one test 1 parameter and the other 2. So the \ncode IMHO would be hard to read if we add 'if (dual) ...;' checks all \nover the place.\n\n> \n> > +\n> > +\tint dualControlsWithDelay(uint32_t startOffset)\n> > +\t{\n> > +\t\tstd::unordered_map delays = {\n> > +\t\t\t{ V4L2_CID_BRIGHTNESS, 1 },\n> > +\t\t\t{ V4L2_CID_CONTRAST, 2 },\n> > +\t\t};\n> > +\t\tstd::unique_ptr delayed =\n> > +\t\t\tstd::make_unique(dev_, delays);\n> > +\t\tControlList ctrls;\n> > +\n> > +\t\t/* Reset control to value that will be first two frames in test. */\n> > +\t\tint32_t expected = 200;\n> > +\t\tctrls.set(V4L2_CID_BRIGHTNESS, expected);\n> > +\t\tctrls.set(V4L2_CID_CONTRAST, expected);\n> \n> It could be worth setting different values for the two controls (and\n> below too) to test the right value is applied to the right control.\n\nGood idea, will do so for v5.\n\n> \n> > +\t\tdev_->setControls(&ctrls);\n> > +\t\tdelayed->reset();\n> > +\n> > +\t\t/* Test dual control with delay. */\n> > +\t\tfor (int32_t i = 0; i < 100; i++) {\n> > +\t\t\tuint32_t frame = startOffset + i;\n> > +\t\t\tint32_t value = 10 + i;\n> > +\n> > +\t\t\tctrls.set(V4L2_CID_BRIGHTNESS, value);\n> > +\t\t\tctrls.set(V4L2_CID_CONTRAST, value);\n> > +\t\t\tdelayed->push(ctrls);\n> > +\n> > +\t\t\tdelayed->applyControls(frame);\n> > +\n> > +\t\t\tControlList result = delayed->get(frame);\n> > +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get();\n> > +\t\t\tint32_t contrast = result.get(V4L2_CID_CONTRAST).get();\n> > +\t\t\tif (brightness != expected || contrast != expected) {\n> > +\t\t\t\tcerr << \"Failed dual controls\"\n> > +\t\t\t\t << \" frame \" << frame\n> > +\t\t\t\t << \" brightness \" << brightness\n> > +\t\t\t\t << \" contrast \" << contrast\n> > +\t\t\t\t << \" expected \" << expected\n> > +\t\t\t\t << endl;\n> > +\t\t\t\treturn TestFail;\n> > +\t\t\t}\n> > +\n> > +\t\t\texpected = i < 1 ? expected : value - 1;\n> \n> This puzzles me. get() is documented as returning the controls in effect\n> at a sequence number. I'd expect this to work for the brightness control\n> as the corresponding delay is 1, but not for the contrast control as the\n> delay is 2.\n\nThis needs to documented better, Delayed controls return garbage data \nuntil _pipeline depth_ frames have been completed. In the RPI pipeline \nhandler this is handled by dropping the N first frames. I'm sure we can \nimprove here but as stated in previous patches the goal of this series \nis to profligate the StaggerdCtls design as-is as it improves the design \nin other ares. I'm sure we will rework this in the future.\n\n> \n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tint dualControlsMultiQueue()\n> > +\t{\n> > +\t\tstd::unordered_map delays = {\n> > +\t\t\t{ V4L2_CID_BRIGHTNESS, 1 },\n> > +\t\t\t{ V4L2_CID_CONTRAST, 2 },\n> > +\t\t};\n> > +\t\tstd::unique_ptr delayed =\n> > +\t\t\tstd::make_unique(dev_, delays);\n> > +\t\tControlList ctrls;\n> > +\n> > +\t\t/* Reset control to value that will be first two frames in test. */\n> > +\t\tint32_t expected = 100;\n> > +\t\tctrls.set(V4L2_CID_BRIGHTNESS, expected);\n> > +\t\tctrls.set(V4L2_CID_CONTRAST, expected);\n> > +\t\tdev_->setControls(&ctrls);\n> > +\t\tdelayed->reset();\n> > +\n> > +\t\t/*\n> > +\t\t * Queue all controls before any fake frame start. Note we\n> > +\t\t * can't queue up more then the delayed controls history size\n> > +\t\t * which is 16. Where one spot is used by the reset control.\n> > +\t\t */\n> > +\t\tfor (unsigned int i = 0; i < 15; i++) {\n> > +\t\t\tint32_t value = 10 + i;\n> > +\n> > +\t\t\tctrls.set(V4L2_CID_BRIGHTNESS, value);\n> > +\t\t\tctrls.set(V4L2_CID_CONTRAST, value);\n> > +\t\t\tdelayed->push(ctrls);\n> > +\t\t}\n> > +\n> > +\t\t/* Process all queued controls. */\n> > +\t\tfor (unsigned int i = 0; i < 16; i++) {\n> > +\t\t\tint32_t value = 10 + i;\n> > +\n> > +\t\t\tdelayed->applyControls(i);\n> > +\n> > +\t\t\tControlList result = delayed->get(i);\n> > +\n> > +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get();\n> > +\t\t\tint32_t contrast = result.get(V4L2_CID_CONTRAST).get();\n> > +\t\t\tif (brightness != expected || contrast != expected) {\n> > +\t\t\t\tcerr << \"Failed multi queue\"\n> > +\t\t\t\t << \" frame \" << i\n> > +\t\t\t\t << \" brightness \" << brightness\n> > +\t\t\t\t << \" contrast \" << contrast\n> > +\t\t\t\t << \" expected \" << expected\n> > +\t\t\t\t << endl;\n> > +\t\t\t\treturn TestFail;\n> > +\t\t\t}\n> > +\n> > +\t\t\texpected = i < 1 ? expected : value - 1;\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tint run() override\n> > +\t{\n> > +\t\tint ret;\n> > +\n> > +\t\t/* Test single control without delay. */\n> > +\t\tret = singleControlNoDelay();\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\t/* Test single control with delay. */\n> > +\t\tret = singleControlWithDelay();\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\t/* Test dual controls with different delays. */\n> > +\t\tret = dualControlsWithDelay(0);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\t/* Test dual controls with non-zero sequence start. */\n> > +\t\tret = dualControlsWithDelay(10000);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\t/* Test dual controls with sequence number wraparound. */\n> > +\t\tret = dualControlsWithDelay(UINT32_MAX - 50);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\t/* Test control values produced faster then consumed. */\n> \n> s/then/than/\n> \n> > +\t\tret = dualControlsMultiQueue();\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tvoid cleanup() override\n> > +\t{\n> > +\t\tdelete dev_;\n> > +\t}\n> > +\n> > +private:\n> > +\tstd::unique_ptr enumerator_;\n> > +\tstd::shared_ptr media_;\n> > +\tV4L2VideoDevice *dev_;\n> > +};\n> > +\n> > +TEST_REGISTER(DelayedControlsTest)\n> > diff --git a/test/meson.build b/test/meson.build\n> > index 0a1d434e399641bb..a683a657a439b4ff 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -25,6 +25,7 @@ public_tests = [\n> > internal_tests = [\n> > ['byte-stream-buffer', 'byte-stream-buffer.cpp'],\n> > ['camera-sensor', 'camera-sensor.cpp'],\n> > + ['delayed_contols', 'delayed_contols.cpp'],\n> > ['event', 'event.cpp'],\n> > ['event-dispatcher', 'event-dispatcher.cpp'],\n> > ['event-thread', 'event-thread.cpp'],\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5D819C0F1C\n\tfor ;\n\tSat, 16 Jan 2021 11:42:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CED0D68105;\n\tSat, 16 Jan 2021 12:42:21 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C674360109\n\tfor ;\n\tSat, 16 Jan 2021 12:42:20 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id b10so13181214ljp.6\n\tfor ;\n\tSat, 16 Jan 2021 03:42:20 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tc14sm1239625lfd.186.2021.01.16.03.42.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 16 Jan 2021 03:42:19 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"hTp1OcJ/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=zLyOyGJ9p6MPW4NU18pOkUjbdsLnGw65NHq1OFv2J50=;\n\tb=hTp1OcJ/2MzkqdUtN2swmP0nVPFqv9ED12PBks7nWOxNISfuH0YCZoXL0lNP4CRmVs\n\twEl1Ib0d/AQ9qzASKQQBlIbRymSH2Kmgeh3yM/0T+RPMjGBEgN0BCbmBIP7bJchCFeE6\n\tSIybeHmBpoM8/J7Hte1MLoRUIQVzPUPgOXZApWaeMAdP9AjDQckSAGixsDjcs2UIJw3y\n\tbfjOAH1Rl9UaSyeIA9zhFvEZpGaLvFABjeif6oJNTOjm/VjYg2yI4GCGfDqImmUmXGKE\n\tvIFnCYZjhmNAlPQs7CuLdQWlT89c5IjZO8sY1VAq86TzT1vE8HilLpWyDqoAnfdCbmVf\n\tlEsw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=zLyOyGJ9p6MPW4NU18pOkUjbdsLnGw65NHq1OFv2J50=;\n\tb=DVZJdToSykQtvitTBpMsJEY3fhHELSYfFvx+635rs/SuFKfHb/VzplRcVjbM5pNMj7\n\t93PyBd1JkWUDqP9cLzrRXFZOl6b6jvWePYpoysfgNTizOwSZcf5HWa3AvvVi13QYOSYM\n\tfgAE+hQNroOVL/Hl7zXOXhCtYPexy94uXA5tiMl+YV7ui4dWoP+4RnIjd9rU6AG7uS8w\n\t20JpNDQ0QR9K2lWqnW9AHidtF6EMadLOuX4XL8zCGF8oWlCbVSWjbMnKhrO9IgSpQ9aa\n\tOJJg7TszwSFpcBcJCmZCYZQLeRcn8ArJCao46oSwZbgQClVah7C7LxvdU1xQcOgQL8lz\n\trsyQ==","X-Gm-Message-State":"AOAM5334M03oZz7JFGsCPegDvv30KGZokUM8SYEP+13EbfbIIHR6B8OD\n\tp/8PrllBhy2P+lauiCxEJNKhWw==","X-Google-Smtp-Source":"ABdhPJxihoToquVVSLJoHYnGXH3icGSy3q9RABym06m8EIV0pO6SxleqUoWLwr39A4xM+TVGQfm65Q==","X-Received":"by 2002:a2e:b1d4:: with SMTP id\n\te20mr6683965lja.304.1610797340140; \n\tSat, 16 Jan 2021 03:42:20 -0800 (PST)","Date":"Sat, 16 Jan 2021 12:42:18 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-3-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 2/8] test: delayed_controls: Add\n\ttest case for DelayedControls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]