[{"id":13941,"web_url":"https://patchwork.libcamera.org/comment/13941/","msgid":"<20201127095054.b7f5dae77mmv3ot2@uno.localdomain>","date":"2020-11-27T09:50:54","subject":"Re: [libcamera-devel] [PATCH v3 2/8] test: delayed_controls: Add\n\ttest case for DelayedControls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Nov 23, 2020 at 11:12:28PM +0100, Niklas Söderlund wrote:\n> Add a test-case for DelayedControls that exercise the setting of\n\nexercizes\n\n> controls and reading back what controls where used for a particular\n\ns/where/were\n\n> frame. Also exercise corner case such as a V4L2 devices that do not\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 <niklas.soderlund@ragnatech.se>\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 | 300 +++++++++++++++++++++++++++++++++++++++\n>  test/meson.build         |   1 +\n>  2 files changed, 301 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..aaf321bf4f1b4cd8\n> --- /dev/null\n> +++ b/test/delayed_contols.cpp\n> @@ -0,0 +1,300 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * libcamera delayed controls tests\n\nWe usually prefix this with the file name.\nie.\n    * delayed_controls.cpp - libcamera delayed controls test\n\n> + */\n> +\n> +#include <iostream>\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> +\nThese following methods are usually in the protected scope.\nAlso missing 'override' keyword\n\n> +\tint init()\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> +\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> +\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<uint32_t, unsigned int> delays = {\n> +\t\t\t{ V4L2_CID_BRIGHTNESS, 0 },\n> +\t\t};\n> +\t\tstd::unique_ptr<DelayedControls> delayed =\n> +\t\t\tstd::make_unique<DelayedControls>(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\tdelayed->reset(&ctrls);\n> +\n> +\t\t/* Test control without delay are set at once. */\n> +\t\tfor (int32_t i = 0; i < 100; i++) {\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->frameStart(i);\n> +\n> +\t\t\tControlList result = delayed->get(i);\n> +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\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<uint32_t, unsigned int> delays = {\n> +\t\t\t{ V4L2_CID_BRIGHTNESS, 1 },\n> +\t\t};\n> +\t\tstd::unique_ptr<DelayedControls> delayed =\n> +\t\t\tstd::make_unique<DelayedControls>(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\tdelayed->reset(&ctrls);\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->frameStart(i);\n> +\n> +\t\t\tControlList result = delayed->get(i);\n> +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\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> +\tint dualControlsWithDelay(uint32_t startOffset)\n> +\t{\n> +\t\tstd::unordered_map<uint32_t, unsigned int> 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<DelayedControls> delayed =\n> +\t\t\tstd::make_unique<DelayedControls>(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> +\t\tdelayed->reset(&ctrls);\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->frameStart(frame);\n> +\n> +\t\t\tControlList result = delayed->get(frame);\n> +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\n> +\t\t\tint32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();\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> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint dualControlsMultiQueue()\n> +\t{\n> +\t\tstd::unordered_map<uint32_t, unsigned int> 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<DelayedControls> delayed =\n> +\t\t\tstd::make_unique<DelayedControls>(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\tdelayed->reset(&ctrls);\n\nnit: the &ctrls does not align with the prototype of push(const\nControlList &ctrls);\n\nInstead of defining\n                DelayedControls::reset(ControlList *ctrls = nullptr);\n\nI would override this as:\n                DelayedControls::reset();\n                DelayedControls::reset(const ControlList &ctrls);\n\nit should be trivial to define one based on the other and you will\nhave a nicer:\n                DelayedControls::reset(const ControlList &ctrls);\n                DelayedControls::push(const ControlList &ctrls);\n\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 (int32_t i = 0; i < 15; i++) {\n\ni can be unsigned int here and in the next loop ?\n\nTest looks good\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n  j\n\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 (int32_t i = 0; i < 16; i++) {\n> +\t\t\tint32_t value = 10 + i;\n> +\n> +\t\t\tdelayed->frameStart(i);\n> +\n> +\t\t\tControlList result = delayed->get(i);\n> +\n> +\t\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\n> +\t\t\tint32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();\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()\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> +\t\tret = dualControlsMultiQueue();\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tvoid cleanup()\n> +\t{\n> +\t\tdelete dev_;\n> +\t}\n> +\n> +private:\n> +\tstd::unique_ptr<DeviceEnumerator> enumerator_;\n> +\tstd::shared_ptr<MediaDevice> 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> 2.29.2\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 7D93EBE08A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 27 Nov 2020 09:50:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0011463470;\n\tFri, 27 Nov 2020 10:50:50 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA2AF6345C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 27 Nov 2020 10:50:49 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id D108B1C0004;\n\tFri, 27 Nov 2020 09:50:48 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 27 Nov 2020 10:50:54 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20201127095054.b7f5dae77mmv3ot2@uno.localdomain>","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201123221234.485933-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]