[{"id":1606,"web_url":"https://patchwork.libcamera.org/comment/1606/","msgid":"<20190517090212.GA4960@pendragon.ideasonboard.com>","date":"2019-05-17T09:02:12","subject":"Re: [libcamera-devel] [PATCH v4 05/11] test: media_device: Create a\n\tcommon MediaDeviceTest base class","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 Fri, May 17, 2019 at 02:54:41AM +0200, Niklas Söderlund wrote:\n> Before adding more tests which will act on the vimc pipeline break out a\n> common base from media_device_link_test.cpp which already acts on vimc.\n> The new common base class will help reduce code duplication.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> test/media_device/media_device_link_test.cpp | 71 ++++++--------------\n> test/media_device/media_device_test.cpp | 36 ++++++++++\n> test/media_device/media_device_test.h | 34 ++++++++++\n> test/media_device/meson.build | 9 ++-\n> 4 files changed, 99 insertions(+), 51 deletions(-)\n> create mode 100644 test/media_device/media_device_test.cpp\n> create mode 100644 test/media_device/media_device_test.h\n> \n> diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp\n> index 334bb44a6fc14371..60e502df8043f979 100644\n> --- a/test/media_device/media_device_link_test.cpp\n> +++ b/test/media_device/media_device_link_test.cpp\n> @@ -4,13 +4,10 @@\n> *\n> * media_device_link_test.cpp - Tests link handling on VIMC media device\n> */\n> +\n> #include \n> -#include \n> \n> -#include \"device_enumerator.h\"\n> -#include \"media_device.h\"\n> -\n> -#include \"test.h\"\n> +#include \"media_device_test.h\"\n> \n> using namespace libcamera;\n> using namespace std;\n> @@ -29,44 +26,21 @@ using namespace std;\n> * loaded) the test is skipped.\n> */\n> \n> -class MediaDeviceLinkTest : public Test\n> +class MediaDeviceLinkTest : public MediaDeviceTest\n> {\n> -\tint init()\n> -\t{\n> -\t\tenumerator = unique_ptr(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(\"vimc\");\n> -\t\tdev_ = enumerator->search(dm);\n> -\t\tif (!dev_) {\n> -\t\t\tcerr << \"No VIMC media device found: skip test\" << endl;\n> -\t\t\treturn TestSkip;\n> -\t\t}\n> -\n> -\t\tif (!dev_->acquire()) {\n> -\t\t\tcerr << \"Unable to acquire media device \"\n> -\t\t\t << dev_->deviceNode() << endl;\n> -\t\t\treturn TestSkip;\n> -\t\t}\n> -\n> -\t\treturn 0;\n> -\t}\n> -\n> \tint run()\n> \t{\n> +\t\tif (!media_->acquire()) {\n> +\t\t\tcerr << \"Unable to acquire media device \"\n> +\t\t\t << media_->deviceNode() << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n\nThis should be in MediaDeviceLinkTest::init() (which should call\nMediaDeviceTest::init()) as the release call is in cleanup().\n\n> +\n> \t\t/*\n> \t\t * First of all disable all links in the media graph to\n> \t\t * ensure we start from a known state.\n> \t\t */\n> -\t\tif (dev_->disableLinks()) {\n> +\t\tif (media_->disableLinks()) {\n> \t\t\tcerr << \"Failed to disable all links in the media graph\";\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -76,26 +50,26 @@ class MediaDeviceLinkTest : public Test\n> \t\t * different methods the media device offers.\n> \t\t */\n> \t\tstring linkName(\"'Debayer A':[1] -> 'Scaler':[0]'\");\n> -\t\tMediaLink *link = dev_->link(\"Debayer A\", 1, \"Scaler\", 0);\n> +\t\tMediaLink *link = media_->link(\"Debayer A\", 1, \"Scaler\", 0);\n> \t\tif (!link) {\n> \t\t\tcerr << \"Unable to find link: \" << linkName\n> \t\t\t << \" using lookup by name\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tMediaEntity *source = dev_->getEntityByName(\"Debayer A\");\n> +\t\tMediaEntity *source = media_->getEntityByName(\"Debayer A\");\n> \t\tif (!source) {\n> \t\t\tcerr << \"Unable to find entity: 'Debayer A'\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tMediaEntity *sink = dev_->getEntityByName(\"Scaler\");\n> +\t\tMediaEntity *sink = media_->getEntityByName(\"Scaler\");\n> \t\tif (!sink) {\n> \t\t\tcerr << \"Unable to find entity: 'Scaler'\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tMediaLink *link2 = dev_->link(source, 1, sink, 0);\n> +\t\tMediaLink *link2 = media_->link(source, 1, sink, 0);\n> \t\tif (!link2) {\n> \t\t\tcerr << \"Unable to find link: \" << linkName\n> \t\t\t << \" using lookup by entity\" << endl;\n> @@ -108,7 +82,7 @@ class MediaDeviceLinkTest : public Test\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tlink2 = dev_->link(source->getPadByIndex(1),\n> +\t\tlink2 = media_->link(source->getPadByIndex(1),\n> \t\t\t\t sink->getPadByIndex(0));\n> \t\tif (!link2) {\n> \t\t\tcerr << \"Unable to find link: \" << linkName\n> @@ -160,7 +134,7 @@ class MediaDeviceLinkTest : public Test\n> \n> \t\t/* Try to get a non existing link. */\n> \t\tlinkName = \"'Sensor A':[1] -> 'Scaler':[0]\";\n> -\t\tlink = dev_->link(\"Sensor A\", 1, \"Scaler\", 0);\n> +\t\tlink = media_->link(\"Sensor A\", 1, \"Scaler\", 0);\n> \t\tif (link) {\n> \t\t\tcerr << \"Link lookup for \" << linkName\n> \t\t\t << \" succeeded but link does not exist\"\n> @@ -170,7 +144,7 @@ class MediaDeviceLinkTest : public Test\n> \n> \t\t/* Now get an immutable link and try to disable it. */\n> \t\tlinkName = \"'Sensor A':[0] -> 'Raw Capture 0':[0]\";\n> -\t\tlink = dev_->link(\"Sensor A\", 0, \"Raw Capture 0\", 0);\n> +\t\tlink = media_->link(\"Sensor A\", 0, \"Raw Capture 0\", 0);\n> \t\tif (!link) {\n> \t\t\tcerr << \"Unable to find link: \" << linkName\n> \t\t\t << \" using lookup by name\" << endl;\n> @@ -195,7 +169,7 @@ class MediaDeviceLinkTest : public Test\n> \t\t * after disabling all links in the media graph.\n> \t\t */\n> \t\tlinkName = \"'Debayer B':[1] -> 'Scaler':[0]'\";\n> -\t\tlink = dev_->link(\"Debayer B\", 1, \"Scaler\", 0);\n> +\t\tlink = media_->link(\"Debayer B\", 1, \"Scaler\", 0);\n> \t\tif (!link) {\n> \t\t\tcerr << \"Unable to find link: \" << linkName\n> \t\t\t << \" using lookup by name\" << endl;\n> @@ -215,7 +189,7 @@ class MediaDeviceLinkTest : public Test\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tif (dev_->disableLinks()) {\n> +\t\tif (media_->disableLinks()) {\n> \t\t\tcerr << \"Failed to disable all links in the media graph\";\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -227,17 +201,14 @@ class MediaDeviceLinkTest : public Test\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> +\n\nUnneeded blank line.\n\n> \t\treturn 0;\n> \t}\n> \n> \tvoid cleanup()\n> \t{\n> -\t\tdev_->release();\n> +\t\tmedia_->release();\n> \t}\n> -\n> -private:\n> -\tunique_ptr enumerator;\n> -\tshared_ptr dev_;\n> };\n> \n> TEST_REGISTER(MediaDeviceLinkTest);\n> diff --git a/test/media_device/media_device_test.cpp b/test/media_device/media_device_test.cpp\n> new file mode 100644\n> index 0000000000000000..055069ad1c331cbb\n> --- /dev/null\n> +++ b/test/media_device/media_device_test.cpp\n> @@ -0,0 +1,36 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * libcamera MediaDevice API tests\n\n * media_device_test.cpp - libcamera media device test base class\n\n> + */\n> +\n> +#include \n> +\n> +#include \"media_device_test.h\"\n> +\n> +using namespace libcamera;\n> +using namespace std;\n> +\n> +int MediaDeviceTest::init()\n> +{\n> +\tenumerator_ = unique_ptr(DeviceEnumerator::create());\n> +\tif (!enumerator_) {\n> +\t\tcerr << \"Failed to create device enumerator\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tif (enumerator_->enumerate()) {\n> +\t\tcerr << \"Failed to enumerate media devices\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tDeviceMatch dm(\"vimc\");\n> +\tmedia_ = enumerator_->search(dm);\n> +\tif (!media_) {\n> +\t\tcerr << \"No VIMC media device found: skip test\" << endl;\n> +\t\treturn TestSkip;\n> +\t}\n> +\n> +\treturn TestPass;\n> +}\n> diff --git a/test/media_device/media_device_test.h b/test/media_device/media_device_test.h\n> new file mode 100644\n> index 0000000000000000..cdbd14841d5ccca2\n> --- /dev/null\n> +++ b/test/media_device/media_device_test.h\n> @@ -0,0 +1,34 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * media_device_test.h - libcamera media device test base class\n> + */\n> +#ifndef __LIBCAMERA_MEDIADEVICE_TEST_H__\n> +#define __LIBCAMERA_MEDIADEVICE_TEST_H__\n> +\n> +#include \n> +\n> +#include \"device_enumerator.h\"\n> +#include \"media_device.h\"\n> +\n> +#include \"test.h\"\n> +\n> +using namespace libcamera;\n> +\n> +class MediaDeviceTest : public Test\n> +{\n> +public:\n> +\tMediaDeviceTest()\n> +\t\t: media_(nullptr), enumerator_(nullptr) {}\n> +\n> +protected:\n> +\tint init();\n> +\n> +\tstd::shared_ptr media_;\n> +\n> +private:\n> +\tstd::unique_ptr enumerator_;\n> +};\n> +\n> +#endif /* __LIBCAMERA_MEDIADEVICE_TEST_H__ */\n> diff --git a/test/media_device/meson.build b/test/media_device/meson.build\n> index d91a022fab190abf..364b4ecf662077ac 100644\n> --- a/test/media_device/meson.build\n> +++ b/test/media_device/meson.build\n> @@ -1,11 +1,18 @@\n> +libmediadevicetest_sources = files([\n> + 'media_device_test.cpp',\n> +])\n> +\n> media_device_tests = [\n> ['media_device_print_test', 'media_device_print_test.cpp'],\n> ['media_device_link_test', 'media_device_link_test.cpp'],\n> ]\n> \n> +libmediadevicetest = static_library('libmediadevicetest', libmediadevicetest_sources,\n> + include_directories : test_includes_internal)\n\nThat's a long name, do you think lib_mdev_test would be better ? Up to\nyou.\n\nWith these small issues fixed,\n\nReviewed-by: Laurent Pinchart \n\n> +\n> foreach t : media_device_tests\n> exe = executable(t[0], t[1],\n> - link_with : test_libraries,\n> + link_with : [test_libraries, libmediadevicetest],\n> include_directories : test_includes_internal)\n> \n> test(t[0], exe, suite: 'media_device', is_parallel: false)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E8EA060C02\n\tfor ;\n\tFri, 17 May 2019 11:02:29 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 617832FD;\n\tFri, 17 May 2019 11:02:29 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558083749;\n\tbh=5iuSaL+jz+/PJS9m9fmTjaonG5ykqzaJtI6hbmlaPk0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NGAGgvKr8aSEgTFOqFPU5227uV6PCEYNqzXkDkKgNzp1L79fPrZRTnGH+fvwvD3dE\n\t3knzKaPCXzBlUBCEJjChz7itNlEtOopLrPPIIH46kIeh6QXjQ39oe+Xx7t1n6XNAFS\n\tfK4gLpGu96aDlqnz+Wy6LsxV5JgEZG0zjEiT4IHg=","Date":"Fri, 17 May 2019 12:02:12 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190517090212.GA4960@pendragon.ideasonboard.com>","References":"<20190517005447.27171-1-niklas.soderlund@ragnatech.se>\n\t<20190517005447.27171-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190517005447.27171-6-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 05/11] test: media_device: Create a\n\tcommon MediaDeviceTest base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 17 May 2019 09:02:30 -0000"}}]