[libcamera-devel] [PATCH v2] tests: v4l2_compat: Add test for v4l2_compat

[Paul Elder]

Hi Laurent,

Thank you for the review.

On Sun, Jun 28, 2020 at 12:23:41PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> 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.

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


Thanks,

Paul