[{"id":10926,"web_url":"https://patchwork.libcamera.org/comment/10926/","msgid":"<20200628092341.GK6954@pendragon.ideasonboard.com>","date":"2020-06-28T09:23:41","subject":"Re: [libcamera-devel] [PATCH v2] tests: v4l2_compat: Add test for\n\tv4l2_compat","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Jun 26, 2020 at 05:22:08PM +0900, Paul Elder wrote:\n> Test the V4L2 compatibility layer by running v4l2-compliance -s on every\n> /dev/video* device.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Note that as of v2, the tests will fail if the tester has a camera\n> supported by libcamera that has unsupported formats, since they will\n> cause a floating point exception. So I don't think this should be merged\n> until that is fixed, otherwise we might get nasty test failures in\n> bisection.\n\nI wonder if we shouldn't temporarily disable testing with UVC. There's\nthe additional issue that meson has a fixed timeout for tests, and\ndepending on the number of UVC devices plugged in the system, the\ntimeout could be exceeded.\n\nOr maybe it would make sense to restrict tests to a single video node\nfor each driver ? We could add an argument to the test to override that\nbehaviour and test all devices when run manually (or the other way\naround, test all devices by default, but restrict to a smaller number of\ndevices when run from meson test).\n\n> Changes in v2:\n> - change all strings to single-quote\n> - simplify meson.build\n> - get path to v4l2-compat.so from cli arg, rather than running custom find()\n> - remove find_file()\n> - extend test timeout to 60 seconds (from default of 30)\n> - don't run v4l2-compliance subprocesses in shell\n> - check if v4l2-compliance runs are killed by signal (eg. the known\n> SIGABRT due to floating point exception)\n> - move the check to see if libcamera supports the camera to v4l2-ctl\n> instead of v4l2-compliance (in v1 we only checked if the pipeline was\n> supported with v4l2-ctl)\n> ---\n> test/meson.build | 1 +\n> test/v4l2_compat/meson.build | 10 +++\n> test/v4l2_compat/v4l2_compat_test.py | 127 +++++++++++++++++++++++++++\n> 3 files changed, 138 insertions(+)\n> create mode 100644 test/v4l2_compat/meson.build\n> create mode 100755 test/v4l2_compat/v4l2_compat_test.py\n> \n> diff --git a/test/meson.build b/test/meson.build\n> index 7808a26..f41d6e7 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -12,6 +12,7 @@ subdir('pipeline')\n> subdir('process')\n> subdir('serialization')\n> subdir('stream')\n> +subdir('v4l2_compat')\n> subdir('v4l2_subdevice')\n> subdir('v4l2_videodevice')\n> \n> diff --git a/test/v4l2_compat/meson.build b/test/v4l2_compat/meson.build\n> new file mode 100644\n> index 0000000..5b29de7\n> --- /dev/null\n> +++ b/test/v4l2_compat/meson.build\n> @@ -0,0 +1,10 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +if is_variable('v4l2_compat')\n> + v4l2_compat_test = files('v4l2_compat_test.py')\n> +\n> + test('v4l2_compat_test', v4l2_compat_test,\n> + args : v4l2_compat,\n> + suite : 'v4l2_compat',\n> + timeout : 60)\n> +endif\n> diff --git a/test/v4l2_compat/v4l2_compat_test.py b/test/v4l2_compat/v4l2_compat_test.py\n> new file mode 100755\n> index 0000000..6c028e7\n> --- /dev/null\n> +++ b/test/v4l2_compat/v4l2_compat_test.py\n> @@ -0,0 +1,127 @@\n> +#!/usr/bin/env python3\n> +# SPDX-License-Identifier: GPL-2.0-or-later\n> +# Copyright (C) 2020, Google Inc.\n> +#\n> +# Author: Paul Elder \n> +#\n> +# v4l2_compat_test.py - Test the V4L2 compatibility layer\n> +\n> +import glob\n> +import os\n> +import re\n> +import shutil\n> +import signal\n> +import subprocess\n> +import sys\n> +\n> +TestPass = 0\n> +TestFail = -1\n> +TestSkip = 77\n> +\n> +\n> +supported_pipelines = [\n> + 'uvcvideo',\n> + 'vimc',\n> +]\n> +\n> +\n> +def grep(exp, arr):\n> + return [s for s in arr if re.search(exp, s)]\n> +\n> +\n> +def run_with_stdout(*args, env={}):\n> + try:\n> + with open(os.devnull, 'w') as devnull:\n> + output = subprocess.check_output(args, env=env, stderr=devnull)\n> + ret = 0\n> + except subprocess.CalledProcessError as err:\n> + output = err.output\n> + ret = err.returncode\n> + return ret, output.decode('utf-8').split('\\n')\n> +\n> +\n> +def extract_result(result):\n> + res = result.split(', ')\n> + ret = {}\n> + ret['total'] = int(res[0].split(': ')[-1])\n> + ret['succeeded'] = int(res[1].split(': ')[-1])\n> + ret['failed'] = int(res[2].split(': ')[-1])\n> + ret['warnings'] = int(res[3].split(': ')[-1])\n> + ret['device'] = res[0].split()[4].strip(':')\n> + ret['driver'] = res[0].split()[2]\n> + return ret\n> +\n> +\n> +def print_output_arr(output_arr):\n> + print('\\n'.join(output_arr))\n> +\n> +\n> +def test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, base_driver):\n> + ret, output = run_with_stdout(v4l2_compliance, '-s', '-d', device, env={'LD_PRELOAD': v4l2_compat})\n> + # TODO fix format so that we don't die from floating point exceptions\n> + if ret < 0:\n> + print_output_arr(output)\n> + print(f'Test for {device} terminated due to signal {signal.Signals(-ret).name}')\n> + return TestFail\n> +\n> + result = extract_result(output[-2])\n> + if result['failed'] == 0:\n> + return TestPass\n> +\n> + # vimc will fail s_fmt because it only supports framesizes that are\n> + # multiples of 3\n> + if base_driver == 'vimc' and result['failed'] <= 1:\n\nI was trying to figure out if this code would work correctly when\nv4l2-compliance doesn't report any error, and didn't notice the == 0\nearly return initially. Maybe == 1 here to make it clearer that you\ndon't handle the == 0 case ?\n\n> + failures = grep('fail', output)\n> + if re.search('S_FMT cannot handle an invalid format', failures[0]) is None:\n> + print_output_arr(output)\n> + return TestFail\n> + return TestPass\n> +\n> + print_output_arr(output)\n> + return TestFail\n> +\n> +\n> +def main(v4l2_compat):\n> + v4l2_compliance = shutil.which('v4l2-compliance')\n> + if v4l2_compliance is None:\n> + print('v4l2-compliance is not available')\n> + return TestSkip\n> +\n> + v4l2_ctl = shutil.which('v4l2-ctl')\n> + if v4l2_ctl is None:\n> + print('v4l2-ctl is not available')\n> + return TestSkip\n> +\n> + dev_nodes = glob.glob('/dev/video*')\n> + if len(dev_nodes) == 0:\n> + print('no video nodes available to test with')\n> + return TestSkip\n> +\n> + failed = []\n> + for device in dev_nodes:\n> + _, out = run_with_stdout(v4l2_ctl, '-D', '-d', device, env={'LD_PRELOAD': v4l2_compat})\n> + driver = grep('Driver name', out)[0].split(':')[-1].strip()\n> + if driver != \"libcamera\":\n> + continue\n> +\n> + _, out = run_with_stdout(v4l2_ctl, '-D', '-d', device)\n> + driver = grep('Driver name', out)[0].split(':')[-1].strip()\n> + if driver not in supported_pipelines:\n> + continue\n> +\n> + ret = test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, driver)\n> + if ret == TestFail:\n> + failed.append(device)\n> +\n> + if len(failed) > 0:\n> + print(f'Failed {len(failed)} tests:')\n> + for device in failed:\n> + print(f'- {device}')\n\nI didn't know about format strings in Python before reading v1 of this\npatch. It's both scary and exciting.\n\n> +\n> + return TestPass if not failed else TestFail\n> +\n> +\n> +if __name__ == '__main__':\n> + if len(sys.argv) < 2:\n> + sys.exit(TestSkip)\n> + sys.exit(main(sys.argv[1]))\n\nI tend to write this\n\nif __name__ == '__main__':\n sys.exit(main(sys.argv))\n\nto mimick C and have all arguments passed to the main function.\n\nReviewed-by: 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 8674BC2E66\n\tfor ;\n\tSun, 28 Jun 2020 09:23:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E1BD7609C8;\n\tSun, 28 Jun 2020 11:23:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1593A603B2\n\tfor ;\n\tSun, 28 Jun 2020 11:23:45 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 817E74FB;\n\tSun, 28 Jun 2020 11:23:44 +0200 (CEST)"],"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=\"hIHUO+2F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593336224;\n\tbh=W424Kuhrsys7sBDeJfNlwr73BLZVhxIPP/srBuUn2JA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hIHUO+2FKLgGP+fjfDcnc5jxG9v+TExwWYUpKNfgNXxYtAS9cQ4v/V56/e4hzW7b/\n\tpRNZ5YR2A3Btr6xbJ7W51Zb1qfibqre5kHuNqnI9HiDbwx1jqn/UaSXB73GBDq1ABr\n\trPedkf7HQrb2LTcAbCRC/RzTya3MF3WiptVrDllc=","Date":"Sun, 28 Jun 2020 12:23:41 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200628092341.GK6954@pendragon.ideasonboard.com>","References":"<20200626082208.122802-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200626082208.122802-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] tests: v4l2_compat: Add test for\n\tv4l2_compat","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":10949,"web_url":"https://patchwork.libcamera.org/comment/10949/","msgid":"<20200629041701.GC108994@pyrite.rasen.tech>","date":"2020-06-29T04:17:01","subject":"Re: [libcamera-devel] [PATCH v2] tests: v4l2_compat: Add test for\n\tv4l2_compat","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Sun, Jun 28, 2020 at 12:23:41PM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Fri, Jun 26, 2020 at 05:22:08PM +0900, Paul Elder wrote:\n> > Test the V4L2 compatibility layer by running v4l2-compliance -s on every\n> > /dev/video* device.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Note that as of v2, the tests will fail if the tester has a camera\n> > supported by libcamera that has unsupported formats, since they will\n> > cause a floating point exception. So I don't think this should be merged\n> > until that is fixed, otherwise we might get nasty test failures in\n> > bisection.\n> \n> I wonder if we shouldn't temporarily disable testing with UVC. There's\n> the additional issue that meson has a fixed timeout for tests, and\n> depending on the number of UVC devices plugged in the system, the\n> timeout could be exceeded.\n\nI increased the timeout to 60 seconds, which was enough for me with 14\nvideo nodes, of which 6 were supported by libcamera. :)\n\n> Or maybe it would make sense to restrict tests to a single video node\n> for each driver ? We could add an argument to the test to override that\n> behaviour and test all devices when run manually (or the other way\n> around, test all devices by default, but restrict to a smaller number of\n> devices when run from meson test).\n\nYeah, I think this is a good idea.\n\n> > Changes in v2:\n> > - change all strings to single-quote\n> > - simplify meson.build\n> > - get path to v4l2-compat.so from cli arg, rather than running custom find()\n> > - remove find_file()\n> > - extend test timeout to 60 seconds (from default of 30)\n> > - don't run v4l2-compliance subprocesses in shell\n> > - check if v4l2-compliance runs are killed by signal (eg. the known\n> > SIGABRT due to floating point exception)\n> > - move the check to see if libcamera supports the camera to v4l2-ctl\n> > instead of v4l2-compliance (in v1 we only checked if the pipeline was\n> > supported with v4l2-ctl)\n> > ---\n> > test/meson.build | 1 +\n> > test/v4l2_compat/meson.build | 10 +++\n> > test/v4l2_compat/v4l2_compat_test.py | 127 +++++++++++++++++++++++++++\n> > 3 files changed, 138 insertions(+)\n> > create mode 100644 test/v4l2_compat/meson.build\n> > create mode 100755 test/v4l2_compat/v4l2_compat_test.py\n> > \n> > diff --git a/test/meson.build b/test/meson.build\n> > index 7808a26..f41d6e7 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -12,6 +12,7 @@ subdir('pipeline')\n> > subdir('process')\n> > subdir('serialization')\n> > subdir('stream')\n> > +subdir('v4l2_compat')\n> > subdir('v4l2_subdevice')\n> > subdir('v4l2_videodevice')\n> > \n> > diff --git a/test/v4l2_compat/meson.build b/test/v4l2_compat/meson.build\n> > new file mode 100644\n> > index 0000000..5b29de7\n> > --- /dev/null\n> > +++ b/test/v4l2_compat/meson.build\n> > @@ -0,0 +1,10 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +if is_variable('v4l2_compat')\n> > + v4l2_compat_test = files('v4l2_compat_test.py')\n> > +\n> > + test('v4l2_compat_test', v4l2_compat_test,\n> > + args : v4l2_compat,\n> > + suite : 'v4l2_compat',\n> > + timeout : 60)\n> > +endif\n> > diff --git a/test/v4l2_compat/v4l2_compat_test.py b/test/v4l2_compat/v4l2_compat_test.py\n> > new file mode 100755\n> > index 0000000..6c028e7\n> > --- /dev/null\n> > +++ b/test/v4l2_compat/v4l2_compat_test.py\n> > @@ -0,0 +1,127 @@\n> > +#!/usr/bin/env python3\n> > +# SPDX-License-Identifier: GPL-2.0-or-later\n> > +# Copyright (C) 2020, Google Inc.\n> > +#\n> > +# Author: Paul Elder \n> > +#\n> > +# v4l2_compat_test.py - Test the V4L2 compatibility layer\n> > +\n> > +import glob\n> > +import os\n> > +import re\n> > +import shutil\n> > +import signal\n> > +import subprocess\n> > +import sys\n> > +\n> > +TestPass = 0\n> > +TestFail = -1\n> > +TestSkip = 77\n> > +\n> > +\n> > +supported_pipelines = [\n> > + 'uvcvideo',\n> > + 'vimc',\n> > +]\n> > +\n> > +\n> > +def grep(exp, arr):\n> > + return [s for s in arr if re.search(exp, s)]\n> > +\n> > +\n> > +def run_with_stdout(*args, env={}):\n> > + try:\n> > + with open(os.devnull, 'w') as devnull:\n> > + output = subprocess.check_output(args, env=env, stderr=devnull)\n> > + ret = 0\n> > + except subprocess.CalledProcessError as err:\n> > + output = err.output\n> > + ret = err.returncode\n> > + return ret, output.decode('utf-8').split('\\n')\n> > +\n> > +\n> > +def extract_result(result):\n> > + res = result.split(', ')\n> > + ret = {}\n> > + ret['total'] = int(res[0].split(': ')[-1])\n> > + ret['succeeded'] = int(res[1].split(': ')[-1])\n> > + ret['failed'] = int(res[2].split(': ')[-1])\n> > + ret['warnings'] = int(res[3].split(': ')[-1])\n> > + ret['device'] = res[0].split()[4].strip(':')\n> > + ret['driver'] = res[0].split()[2]\n> > + return ret\n> > +\n> > +\n> > +def print_output_arr(output_arr):\n> > + print('\\n'.join(output_arr))\n> > +\n> > +\n> > +def test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, base_driver):\n> > + ret, output = run_with_stdout(v4l2_compliance, '-s', '-d', device, env={'LD_PRELOAD': v4l2_compat})\n> > + # TODO fix format so that we don't die from floating point exceptions\n> > + if ret < 0:\n> > + print_output_arr(output)\n> > + print(f'Test for {device} terminated due to signal {signal.Signals(-ret).name}')\n> > + return TestFail\n> > +\n> > + result = extract_result(output[-2])\n> > + if result['failed'] == 0:\n> > + return TestPass\n> > +\n> > + # vimc will fail s_fmt because it only supports framesizes that are\n> > + # multiples of 3\n> > + if base_driver == 'vimc' and result['failed'] <= 1:\n> \n> I was trying to figure out if this code would work correctly when\n> v4l2-compliance doesn't report any error, and didn't notice the == 0\n> early return initially. Maybe == 1 here to make it clearer that you\n> don't handle the == 0 case ?\n\nOh yeah, probably better just in case.\n\n> > + failures = grep('fail', output)\n> > + if re.search('S_FMT cannot handle an invalid format', failures[0]) is None:\n> > + print_output_arr(output)\n> > + return TestFail\n> > + return TestPass\n> > +\n> > + print_output_arr(output)\n> > + return TestFail\n> > +\n> > +\n> > +def main(v4l2_compat):\n> > + v4l2_compliance = shutil.which('v4l2-compliance')\n> > + if v4l2_compliance is None:\n> > + print('v4l2-compliance is not available')\n> > + return TestSkip\n> > +\n> > + v4l2_ctl = shutil.which('v4l2-ctl')\n> > + if v4l2_ctl is None:\n> > + print('v4l2-ctl is not available')\n> > + return TestSkip\n> > +\n> > + dev_nodes = glob.glob('/dev/video*')\n> > + if len(dev_nodes) == 0:\n> > + print('no video nodes available to test with')\n> > + return TestSkip\n> > +\n> > + failed = []\n> > + for device in dev_nodes:\n> > + _, out = run_with_stdout(v4l2_ctl, '-D', '-d', device, env={'LD_PRELOAD': v4l2_compat})\n> > + driver = grep('Driver name', out)[0].split(':')[-1].strip()\n> > + if driver != \"libcamera\":\n> > + continue\n> > +\n> > + _, out = run_with_stdout(v4l2_ctl, '-D', '-d', device)\n> > + driver = grep('Driver name', out)[0].split(':')[-1].strip()\n> > + if driver not in supported_pipelines:\n> > + continue\n> > +\n> > + ret = test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, driver)\n> > + if ret == TestFail:\n> > + failed.append(device)\n> > +\n> > + if len(failed) > 0:\n> > + print(f'Failed {len(failed)} tests:')\n> > + for device in failed:\n> > + print(f'- {device}')\n> \n> I didn't know about format strings in Python before reading v1 of this\n> patch. It's both scary and exciting.\n\nI learned it from my friends in all those group projects :)\n\n> > +\n> > + return TestPass if not failed else TestFail\n> > +\n> > +\n> > +if __name__ == '__main__':\n> > + if len(sys.argv) < 2:\n> > + sys.exit(TestSkip)\n> > + sys.exit(main(sys.argv[1]))\n> \n> I tend to write this\n> \n> if __name__ == '__main__':\n> sys.exit(main(sys.argv))\n> \n> to mimick C and have all arguments passed to the main function.\n\nHm, I suppose. I don't think it's the job of the main function to\nvalidate the arguments, in python at least, since if __name__ ==\n'__main__' already serves as the entry point.\n\n> Reviewed-by: Laurent Pinchart \n\n\nThanks,\n\nPaul","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 65D19C2E69\n\tfor ;\n\tMon, 29 Jun 2020 04:17:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7140609DB;\n\tMon, 29 Jun 2020 06:17:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 342B7603B1\n\tfor ;\n\tMon, 29 Jun 2020 06:17:09 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 807112B3;\n\tMon, 29 Jun 2020 06:17:07 +0200 (CEST)"],"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=\"QALVaehM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593404228;\n\tbh=j5LqDz7JQXcPtchriDn4dH26SzlGYawo1jlmgs/VOo0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QALVaehMY3n6ESusgHGSVxXgqWn8Ki3F5Yl2IylNoBUtT5nCkEYJyU1DBuv/Pcz9W\n\tmWbLDzZvhZ5M0TParKdV+4topXM6SJOOwAZgED6CNmjTKASKp3oWF6c817kPBTM+mb\n\tHhCtgmjixP9nfOsR4KELwGdao1/TleKec8Ot3JOM=","Date":"Mon, 29 Jun 2020 13:17:01 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Message-ID":"<20200629041701.GC108994@pyrite.rasen.tech>","References":"<20200626082208.122802-1-paul.elder@ideasonboard.com>\n\t<20200628092341.GK6954@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200628092341.GK6954@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] tests: v4l2_compat: Add test for\n\tv4l2_compat","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":10953,"web_url":"https://patchwork.libcamera.org/comment/10953/","msgid":"<20200629081140.GA6012@pendragon.ideasonboard.com>","date":"2020-06-29T08:11:40","subject":"Re: [libcamera-devel] [PATCH v2] tests: v4l2_compat: Add test for\n\tv4l2_compat","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Mon, Jun 29, 2020 at 01:17:01PM +0900, Paul Elder wrote:\n> On Sun, Jun 28, 2020 at 12:23:41PM +0300, Laurent Pinchart wrote:\n> > On Fri, Jun 26, 2020 at 05:22:08PM +0900, Paul Elder wrote:\n> > > Test the V4L2 compatibility layer by running v4l2-compliance -s on every\n> > > /dev/video* device.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > \n> > > ---\n> > > Note that as of v2, the tests will fail if the tester has a camera\n> > > supported by libcamera that has unsupported formats, since they will\n> > > cause a floating point exception. So I don't think this should be merged\n> > > until that is fixed, otherwise we might get nasty test failures in\n> > > bisection.\n> > \n> > I wonder if we shouldn't temporarily disable testing with UVC. There's\n> > the additional issue that meson has a fixed timeout for tests, and\n> > depending on the number of UVC devices plugged in the system, the\n> > timeout could be exceeded.\n> \n> I increased the timeout to 60 seconds, which was enough for me with 14\n> video nodes, of which 6 were supported by libcamera. :)\n> \n> > Or maybe it would make sense to restrict tests to a single video node\n> > for each driver ? We could add an argument to the test to override that\n> > behaviour and test all devices when run manually (or the other way\n> > around, test all devices by default, but restrict to a smaller number of\n> > devices when run from meson test).\n> \n> Yeah, I think this is a good idea.\n> \n> > > Changes in v2:\n> > > - change all strings to single-quote\n> > > - simplify meson.build\n> > > - get path to v4l2-compat.so from cli arg, rather than running custom find()\n> > > - remove find_file()\n> > > - extend test timeout to 60 seconds (from default of 30)\n> > > - don't run v4l2-compliance subprocesses in shell\n> > > - check if v4l2-compliance runs are killed by signal (eg. the known\n> > > SIGABRT due to floating point exception)\n> > > - move the check to see if libcamera supports the camera to v4l2-ctl\n> > > instead of v4l2-compliance (in v1 we only checked if the pipeline was\n> > > supported with v4l2-ctl)\n> > > ---\n> > > test/meson.build | 1 +\n> > > test/v4l2_compat/meson.build | 10 +++\n> > > test/v4l2_compat/v4l2_compat_test.py | 127 +++++++++++++++++++++++++++\n> > > 3 files changed, 138 insertions(+)\n> > > create mode 100644 test/v4l2_compat/meson.build\n> > > create mode 100755 test/v4l2_compat/v4l2_compat_test.py\n> > > \n> > > diff --git a/test/meson.build b/test/meson.build\n> > > index 7808a26..f41d6e7 100644\n> > > --- a/test/meson.build\n> > > +++ b/test/meson.build\n> > > @@ -12,6 +12,7 @@ subdir('pipeline')\n> > > subdir('process')\n> > > subdir('serialization')\n> > > subdir('stream')\n> > > +subdir('v4l2_compat')\n> > > subdir('v4l2_subdevice')\n> > > subdir('v4l2_videodevice')\n> > > \n> > > diff --git a/test/v4l2_compat/meson.build b/test/v4l2_compat/meson.build\n> > > new file mode 100644\n> > > index 0000000..5b29de7\n> > > --- /dev/null\n> > > +++ b/test/v4l2_compat/meson.build\n> > > @@ -0,0 +1,10 @@\n> > > +# SPDX-License-Identifier: CC0-1.0\n> > > +\n> > > +if is_variable('v4l2_compat')\n> > > + v4l2_compat_test = files('v4l2_compat_test.py')\n> > > +\n> > > + test('v4l2_compat_test', v4l2_compat_test,\n> > > + args : v4l2_compat,\n> > > + suite : 'v4l2_compat',\n> > > + timeout : 60)\n> > > +endif\n> > > diff --git a/test/v4l2_compat/v4l2_compat_test.py b/test/v4l2_compat/v4l2_compat_test.py\n> > > new file mode 100755\n> > > index 0000000..6c028e7\n> > > --- /dev/null\n> > > +++ b/test/v4l2_compat/v4l2_compat_test.py\n> > > @@ -0,0 +1,127 @@\n> > > +#!/usr/bin/env python3\n> > > +# SPDX-License-Identifier: GPL-2.0-or-later\n> > > +# Copyright (C) 2020, Google Inc.\n> > > +#\n> > > +# Author: Paul Elder \n> > > +#\n> > > +# v4l2_compat_test.py - Test the V4L2 compatibility layer\n> > > +\n> > > +import glob\n> > > +import os\n> > > +import re\n> > > +import shutil\n> > > +import signal\n> > > +import subprocess\n> > > +import sys\n> > > +\n> > > +TestPass = 0\n> > > +TestFail = -1\n> > > +TestSkip = 77\n> > > +\n> > > +\n> > > +supported_pipelines = [\n> > > + 'uvcvideo',\n> > > + 'vimc',\n> > > +]\n> > > +\n> > > +\n> > > +def grep(exp, arr):\n> > > + return [s for s in arr if re.search(exp, s)]\n> > > +\n> > > +\n> > > +def run_with_stdout(*args, env={}):\n> > > + try:\n> > > + with open(os.devnull, 'w') as devnull:\n> > > + output = subprocess.check_output(args, env=env, stderr=devnull)\n> > > + ret = 0\n> > > + except subprocess.CalledProcessError as err:\n> > > + output = err.output\n> > > + ret = err.returncode\n> > > + return ret, output.decode('utf-8').split('\\n')\n> > > +\n> > > +\n> > > +def extract_result(result):\n> > > + res = result.split(', ')\n> > > + ret = {}\n> > > + ret['total'] = int(res[0].split(': ')[-1])\n> > > + ret['succeeded'] = int(res[1].split(': ')[-1])\n> > > + ret['failed'] = int(res[2].split(': ')[-1])\n> > > + ret['warnings'] = int(res[3].split(': ')[-1])\n> > > + ret['device'] = res[0].split()[4].strip(':')\n> > > + ret['driver'] = res[0].split()[2]\n> > > + return ret\n> > > +\n> > > +\n> > > +def print_output_arr(output_arr):\n> > > + print('\\n'.join(output_arr))\n> > > +\n> > > +\n> > > +def test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, base_driver):\n> > > + ret, output = run_with_stdout(v4l2_compliance, '-s', '-d', device, env={'LD_PRELOAD': v4l2_compat})\n> > > + # TODO fix format so that we don't die from floating point exceptions\n> > > + if ret < 0:\n> > > + print_output_arr(output)\n> > > + print(f'Test for {device} terminated due to signal {signal.Signals(-ret).name}')\n> > > + return TestFail\n> > > +\n> > > + result = extract_result(output[-2])\n> > > + if result['failed'] == 0:\n> > > + return TestPass\n> > > +\n> > > + # vimc will fail s_fmt because it only supports framesizes that are\n> > > + # multiples of 3\n> > > + if base_driver == 'vimc' and result['failed'] <= 1:\n> > \n> > I was trying to figure out if this code would work correctly when\n> > v4l2-compliance doesn't report any error, and didn't notice the == 0\n> > early return initially. Maybe == 1 here to make it clearer that you\n> > don't handle the == 0 case ?\n> \n> Oh yeah, probably better just in case.\n> \n> > > + failures = grep('fail', output)\n> > > + if re.search('S_FMT cannot handle an invalid format', failures[0]) is None:\n> > > + print_output_arr(output)\n> > > + return TestFail\n> > > + return TestPass\n> > > +\n> > > + print_output_arr(output)\n> > > + return TestFail\n> > > +\n> > > +\n> > > +def main(v4l2_compat):\n> > > + v4l2_compliance = shutil.which('v4l2-compliance')\n> > > + if v4l2_compliance is None:\n> > > + print('v4l2-compliance is not available')\n> > > + return TestSkip\n> > > +\n> > > + v4l2_ctl = shutil.which('v4l2-ctl')\n> > > + if v4l2_ctl is None:\n> > > + print('v4l2-ctl is not available')\n> > > + return TestSkip\n> > > +\n> > > + dev_nodes = glob.glob('/dev/video*')\n> > > + if len(dev_nodes) == 0:\n> > > + print('no video nodes available to test with')\n> > > + return TestSkip\n> > > +\n> > > + failed = []\n> > > + for device in dev_nodes:\n> > > + _, out = run_with_stdout(v4l2_ctl, '-D', '-d', device, env={'LD_PRELOAD': v4l2_compat})\n> > > + driver = grep('Driver name', out)[0].split(':')[-1].strip()\n> > > + if driver != \"libcamera\":\n> > > + continue\n> > > +\n> > > + _, out = run_with_stdout(v4l2_ctl, '-D', '-d', device)\n> > > + driver = grep('Driver name', out)[0].split(':')[-1].strip()\n> > > + if driver not in supported_pipelines:\n> > > + continue\n> > > +\n> > > + ret = test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, driver)\n> > > + if ret == TestFail:\n> > > + failed.append(device)\n> > > +\n> > > + if len(failed) > 0:\n> > > + print(f'Failed {len(failed)} tests:')\n> > > + for device in failed:\n> > > + print(f'- {device}')\n> > \n> > I didn't know about format strings in Python before reading v1 of this\n> > patch. It's both scary and exciting.\n> \n> I learned it from my friends in all those group projects :)\n> \n> > > +\n> > > + return TestPass if not failed else TestFail\n> > > +\n> > > +\n> > > +if __name__ == '__main__':\n> > > + if len(sys.argv) < 2:\n> > > + sys.exit(TestSkip)\n> > > + sys.exit(main(sys.argv[1]))\n> > \n> > I tend to write this\n> > \n> > if __name__ == '__main__':\n> > sys.exit(main(sys.argv))\n> > \n> > to mimick C and have all arguments passed to the main function.\n> \n> Hm, I suppose. I don't think it's the job of the main function to\n> validate the arguments, in python at least, since if __name__ ==\n> '__main__' already serves as the entry point.\n\nIt's really a matter of convention, Python doesn't have a main function,\nyou could move all the code from main() to the top-level with one level\nof indentation less. Using __name__ == '__main__' is common in Python\nfiles that can be both imported (because they provide useful symbols to\nother scripts) and executed standalone, to avoid code being run on\nimport, but is otherwise not strictly mandatory. I like having a main\nfunction in all cases personally, but that's likely due to my C\nbackground.\n\n> > Reviewed-by: 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 6B12DC2E6B\n\tfor ;\n\tMon, 29 Jun 2020 08:11:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDE35609C9;\n\tMon, 29 Jun 2020 10:11:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE84B603B2\n\tfor ;\n\tMon, 29 Jun 2020 10:11:44 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35260734;\n\tMon, 29 Jun 2020 10:11:44 +0200 (CEST)"],"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=\"HmF+UxIU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593418304;\n\tbh=4NAbehVLD3SXBEVO8hjKtdAZYWG+35DV8fiYZ+rkYL4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HmF+UxIU2HluIXCUspJvgGYyUe5FeiW8hQ8s9ckAO7avk5ezQka8+vXr9nQStp8Q+\n\tfvgRdMli+apLmOklHEmDL3rLbJs+DYRE7h7n/ZWFdeLhwxqJyERJKsvdiBAhu3516A\n\tNI7MoMAExrsHUdXBPvRxRhe9dkYxiWnrIm5eAFV0=","Date":"Mon, 29 Jun 2020 11:11:40 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200629081140.GA6012@pendragon.ideasonboard.com>","References":"<20200626082208.122802-1-paul.elder@ideasonboard.com>\n\t<20200628092341.GK6954@pendragon.ideasonboard.com>\n\t<20200629041701.GC108994@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200629041701.GC108994@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v2] tests: v4l2_compat: Add test for\n\tv4l2_compat","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]