[libcamera-devel] [PATCH v2] tests: v4l2_compat: Add test for v4l2_compat
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 29 10:11:40 CEST 2020
Hi Paul,
On Mon, Jun 29, 2020 at 01:17:01PM +0900, Paul Elder wrote:
> On Sun, Jun 28, 2020 at 12:23:41PM +0300, Laurent Pinchart wrote:
> > On Fri, Jun 26, 2020 at 05:22:08PM +0900, Paul Elder wrote:
> > > Test the V4L2 compatibility layer by running v4l2-compliance -s on every
> > > /dev/video* device.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Note that as of v2, the tests will fail if the tester has a camera
> > > supported by libcamera that has unsupported formats, since they will
> > > cause a floating point exception. So I don't think this should be merged
> > > until that is fixed, otherwise we might get nasty test failures in
> > > bisection.
> >
> > I wonder if we shouldn't temporarily disable testing with UVC. There's
> > the additional issue that meson has a fixed timeout for tests, and
> > depending on the number of UVC devices plugged in the system, the
> > timeout could be exceeded.
>
> I increased the timeout to 60 seconds, which was enough for me with 14
> video nodes, of which 6 were supported by libcamera. :)
>
> > Or maybe it would make sense to restrict tests to a single video node
> > for each driver ? We could add an argument to the test to override that
> > behaviour and test all devices when run manually (or the other way
> > around, test all devices by default, but restrict to a smaller number of
> > devices when run from meson test).
>
> Yeah, I think this is a good idea.
>
> > > Changes in v2:
> > > - change all strings to single-quote
> > > - simplify meson.build
> > > - get path to v4l2-compat.so from cli arg, rather than running custom find()
> > > - remove find_file()
> > > - extend test timeout to 60 seconds (from default of 30)
> > > - don't run v4l2-compliance subprocesses in shell
> > > - check if v4l2-compliance runs are killed by signal (eg. the known
> > > SIGABRT due to floating point exception)
> > > - move the check to see if libcamera supports the camera to v4l2-ctl
> > > instead of v4l2-compliance (in v1 we only checked if the pipeline was
> > > supported with v4l2-ctl)
> > > ---
> > > test/meson.build | 1 +
> > > test/v4l2_compat/meson.build | 10 +++
> > > test/v4l2_compat/v4l2_compat_test.py | 127 +++++++++++++++++++++++++++
> > > 3 files changed, 138 insertions(+)
> > > create mode 100644 test/v4l2_compat/meson.build
> > > create mode 100755 test/v4l2_compat/v4l2_compat_test.py
> > >
> > > diff --git a/test/meson.build b/test/meson.build
> > > index 7808a26..f41d6e7 100644
> > > --- a/test/meson.build
> > > +++ b/test/meson.build
> > > @@ -12,6 +12,7 @@ subdir('pipeline')
> > > subdir('process')
> > > subdir('serialization')
> > > subdir('stream')
> > > +subdir('v4l2_compat')
> > > subdir('v4l2_subdevice')
> > > subdir('v4l2_videodevice')
> > >
> > > diff --git a/test/v4l2_compat/meson.build b/test/v4l2_compat/meson.build
> > > new file mode 100644
> > > index 0000000..5b29de7
> > > --- /dev/null
> > > +++ b/test/v4l2_compat/meson.build
> > > @@ -0,0 +1,10 @@
> > > +# SPDX-License-Identifier: CC0-1.0
> > > +
> > > +if is_variable('v4l2_compat')
> > > + v4l2_compat_test = files('v4l2_compat_test.py')
> > > +
> > > + test('v4l2_compat_test', v4l2_compat_test,
> > > + args : v4l2_compat,
> > > + suite : 'v4l2_compat',
> > > + timeout : 60)
> > > +endif
> > > diff --git a/test/v4l2_compat/v4l2_compat_test.py b/test/v4l2_compat/v4l2_compat_test.py
> > > new file mode 100755
> > > index 0000000..6c028e7
> > > --- /dev/null
> > > +++ b/test/v4l2_compat/v4l2_compat_test.py
> > > @@ -0,0 +1,127 @@
> > > +#!/usr/bin/env python3
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +# Copyright (C) 2020, Google Inc.
> > > +#
> > > +# Author: Paul Elder <paul.elder at ideasonboard.com>
> > > +#
> > > +# v4l2_compat_test.py - Test the V4L2 compatibility layer
> > > +
> > > +import glob
> > > +import os
> > > +import re
> > > +import shutil
> > > +import signal
> > > +import subprocess
> > > +import sys
> > > +
> > > +TestPass = 0
> > > +TestFail = -1
> > > +TestSkip = 77
> > > +
> > > +
> > > +supported_pipelines = [
> > > + 'uvcvideo',
> > > + 'vimc',
> > > +]
> > > +
> > > +
> > > +def grep(exp, arr):
> > > + return [s for s in arr if re.search(exp, s)]
> > > +
> > > +
> > > +def run_with_stdout(*args, env={}):
> > > + try:
> > > + with open(os.devnull, 'w') as devnull:
> > > + output = subprocess.check_output(args, env=env, stderr=devnull)
> > > + ret = 0
> > > + except subprocess.CalledProcessError as err:
> > > + output = err.output
> > > + ret = err.returncode
> > > + return ret, output.decode('utf-8').split('\n')
> > > +
> > > +
> > > +def extract_result(result):
> > > + res = result.split(', ')
> > > + ret = {}
> > > + ret['total'] = int(res[0].split(': ')[-1])
> > > + ret['succeeded'] = int(res[1].split(': ')[-1])
> > > + ret['failed'] = int(res[2].split(': ')[-1])
> > > + ret['warnings'] = int(res[3].split(': ')[-1])
> > > + ret['device'] = res[0].split()[4].strip(':')
> > > + ret['driver'] = res[0].split()[2]
> > > + return ret
> > > +
> > > +
> > > +def print_output_arr(output_arr):
> > > + print('\n'.join(output_arr))
> > > +
> > > +
> > > +def test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, base_driver):
> > > + ret, output = run_with_stdout(v4l2_compliance, '-s', '-d', device, env={'LD_PRELOAD': v4l2_compat})
> > > + # TODO fix format so that we don't die from floating point exceptions
> > > + if ret < 0:
> > > + print_output_arr(output)
> > > + print(f'Test for {device} terminated due to signal {signal.Signals(-ret).name}')
> > > + return TestFail
> > > +
> > > + result = extract_result(output[-2])
> > > + if result['failed'] == 0:
> > > + return TestPass
> > > +
> > > + # vimc will fail s_fmt because it only supports framesizes that are
> > > + # multiples of 3
> > > + if base_driver == 'vimc' and result['failed'] <= 1:
> >
> > I was trying to figure out if this code would work correctly when
> > v4l2-compliance doesn't report any error, and didn't notice the == 0
> > early return initially. Maybe == 1 here to make it clearer that you
> > don't handle the == 0 case ?
>
> Oh yeah, probably better just in case.
>
> > > + failures = grep('fail', output)
> > > + if re.search('S_FMT cannot handle an invalid format', failures[0]) is None:
> > > + print_output_arr(output)
> > > + return TestFail
> > > + return TestPass
> > > +
> > > + print_output_arr(output)
> > > + return TestFail
> > > +
> > > +
> > > +def main(v4l2_compat):
> > > + v4l2_compliance = shutil.which('v4l2-compliance')
> > > + if v4l2_compliance is None:
> > > + print('v4l2-compliance is not available')
> > > + return TestSkip
> > > +
> > > + v4l2_ctl = shutil.which('v4l2-ctl')
> > > + if v4l2_ctl is None:
> > > + print('v4l2-ctl is not available')
> > > + return TestSkip
> > > +
> > > + dev_nodes = glob.glob('/dev/video*')
> > > + if len(dev_nodes) == 0:
> > > + print('no video nodes available to test with')
> > > + return TestSkip
> > > +
> > > + failed = []
> > > + for device in dev_nodes:
> > > + _, out = run_with_stdout(v4l2_ctl, '-D', '-d', device, env={'LD_PRELOAD': v4l2_compat})
> > > + driver = grep('Driver name', out)[0].split(':')[-1].strip()
> > > + if driver != "libcamera":
> > > + continue
> > > +
> > > + _, out = run_with_stdout(v4l2_ctl, '-D', '-d', device)
> > > + driver = grep('Driver name', out)[0].split(':')[-1].strip()
> > > + if driver not in supported_pipelines:
> > > + continue
> > > +
> > > + ret = test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, driver)
> > > + if ret == TestFail:
> > > + failed.append(device)
> > > +
> > > + if len(failed) > 0:
> > > + print(f'Failed {len(failed)} tests:')
> > > + for device in failed:
> > > + print(f'- {device}')
> >
> > I didn't know about format strings in Python before reading v1 of this
> > patch. It's both scary and exciting.
>
> I learned it from my friends in all those group projects :)
>
> > > +
> > > + return TestPass if not failed else TestFail
> > > +
> > > +
> > > +if __name__ == '__main__':
> > > + if len(sys.argv) < 2:
> > > + sys.exit(TestSkip)
> > > + sys.exit(main(sys.argv[1]))
> >
> > I tend to write this
> >
> > if __name__ == '__main__':
> > sys.exit(main(sys.argv))
> >
> > to mimick C and have all arguments passed to the main function.
>
> Hm, I suppose. I don't think it's the job of the main function to
> validate the arguments, in python at least, since if __name__ ==
> '__main__' already serves as the entry point.
It's really a matter of convention, Python doesn't have a main function,
you could move all the code from main() to the top-level with one level
of indentation less. Using __name__ == '__main__' is common in Python
files that can be both imported (because they provide useful symbols to
other scripts) and executed standalone, to avoid code being run on
import, but is otherwise not strictly mandatory. I like having a main
function in all cases personally, but that's likely due to my C
background.
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list