[libcamera-devel] [RFC PATCH] tests: v4l2_compat: Add test for v4l2_compat
Paul Elder
paul.elder at ideasonboard.com
Fri Jun 26 09:57:37 CEST 2020
Hi Laurent,
Thank you for the review.
On Thu, Jun 25, 2020 at 01:26:41AM +0300, Laurent Pinchart wrote:
> Hi Niklas and Paul,
>
> On Wed, Jun 24, 2020 at 07:38:04PM +0200, Niklas Söderlund wrote:
> > Hi Paul,
> >
> > Thanks for your work.
> >
> > On 2020-06-24 23:07:52 +0900, Paul Elder wrote:
> > > Test the V4L2 compatibility layer by running v4l2-compliance -s on every
> > > /dev/video* device.
> >
> > I wonder if this is not a tad to aggressive. Imagine the test being run
> > on a device where one video node is not covered by a libcamera pipeline
> > and also does not pass v4l2-compliance. Would this not lead to the
> > libcamera test suite to fail?
> >
> > Would it be possible you think to check the driver name of the video
> > device and check it against an whitelist (vimc, vivid, ?) before running
> > the v4l2-compliance suite?
> >
> > You can get the driver name from
> >
> > v4l2-ctl -D -d /dev/videoX
>
> Isn't this exactly what the code below is doing ? :-)
>
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > > test/meson.build | 1 +
> > > test/v4l2_compat/meson.build | 10 +++
> > > test/v4l2_compat/v4l2_compat_test.py | 125 +++++++++++++++++++++++++++
> > > 3 files changed, 136 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 a868813..591920f 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..a67aac4
> > > --- /dev/null
> > > +++ b/test/v4l2_compat/meson.build
> > > @@ -0,0 +1,10 @@
> > > +# SPDX-License-Identifier: CC0-1.0
> > > +
> > > +pymod = import('python')
> > > +py3 = pymod.find_installation('python3')
> > > +
> > > +v4l2_compat_test = files('v4l2_compat_test.py')
> > > +
> > > +test('v4l2_compat_test', py3,
> > > + args : v4l2_compat_test,
> > > + suite : 'v4l2_compat')
>
> Would this be simpler ?
>
> test('v4l2_compat_test', v4l2_compat_test,
> suite : 'v4l2_compat')
Ah yes, that is much better. I wasn't sure how tests with python worked
in meson.
> > > diff --git a/test/v4l2_compat/v4l2_compat_test.py b/test/v4l2_compat/v4l2_compat_test.py
> > > new file mode 100755
> > > index 0000000..f56db4e
> > > --- /dev/null
> > > +++ b/test/v4l2_compat/v4l2_compat_test.py
> > > @@ -0,0 +1,125 @@
> > > +#!/usr/bin/env python3
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +# Copyright (C) 2019, Google Inc.
>
> 2020 ?
Yes :)
> > > +#
> > > +# Author: Paul Elder <paul.elder at ideasonboard.com>
> > > +#
> > > +# v4l2_compat_test.py - Test the V4L2 compatibility layer
> > > +
> > > +import os
> > > +import re
> > > +from shutil import which
>
> I'd just import shutil and use shutil.which, up to you.
>
> > > +import subprocess
> > > +import sys
> > > +
> > > +TestPass = 0
> > > +TestFail = -1
> > > +TestSkip = 77
> > > +
> > > +
> > > +supported_pipelines = [
> > > + "uvcvideo",
>
> Our python scripts usually use single quotes for strings.
>
> > > + "vimc",
> > > +]
> > > +
> > > +
> > > +def find_file(name, path):
> > > + for root, dirs, files in os.walk(path):
> > > + if name in files:
> > > + return os.path.join(root, name)
>
> That's a bit of heavy artillery. How about passing the path to
> v4l2-compat.so to the test ? Something along the lines of
Ah yes, much better.
> diff --git a/test/v4l2_compat/meson.build b/test/v4l2_compat/meson.build
> index a67aac41b273..e24d10bd327c 100644
> --- a/test/v4l2_compat/meson.build
> +++ b/test/v4l2_compat/meson.build
> @@ -1,10 +1,9 @@
> # SPDX-License-Identifier: CC0-1.0
>
> -pymod = import('python')
> -py3 = pymod.find_installation('python3')
> +if is_variable('v4l2_compat')
> + v4l2_compat_test = files('v4l2_compat_test.py')
>
> -v4l2_compat_test = files('v4l2_compat_test.py')
> -
> -test('v4l2_compat_test', py3,
> - args : v4l2_compat_test,
> - suite : 'v4l2_compat')
> + test('v4l2_compat_test', v4l2_compat_test,
> + args : v4l2_compat,
> + suite : 'v4l2_compat')
> +endif
>
> > > +
> > > +
> > > +def grep(exp, arr):
> > > + return [s for s in arr if re.search(exp, s)]
> > > +
> > > +
> > > +def run_with_stdout(cmd, args="", env={}):
> > > + try:
> > > + with open(os.devnull, 'w') as devnull:
> > > + output = subprocess.check_output(f"{cmd} {args}", shell=True, env=env, stderr=devnull)
>
> Do you need to run this through a shell ? On my system, one of the video
> nodes related to my integrated UVC webcam isn't handled by libcamera.
> When v4l2-compliance is run on it with LD_PRELOAD=v4l2-compat.so, it
> hangs if run through a shell, and doesn't otherwise. I suspect it's the
> shell hanging, but if we don't need shell=True, then we may skip the
> investigation :-)
It looks like we don't need to run it through as shell, so I've dropped
it.
> You will need to turn the first argument into an array. I've tried the
> following, it seems to work.
>
> diff --git a/test/v4l2_compat/v4l2_compat_test.py b/test/v4l2_compat/v4l2_compat_test.py
> index f56db4e91890..d05b3235a78c 100755
> --- a/test/v4l2_compat/v4l2_compat_test.py
> +++ b/test/v4l2_compat/v4l2_compat_test.py
> @@ -33,10 +33,10 @@ def grep(exp, arr):
> return [s for s in arr if re.search(exp, s)]
>
>
> -def run_with_stdout(cmd, args="", env={}):
> +def run_with_stdout(*args, env={}):
> try:
> with open(os.devnull, 'w') as devnull:
> - output = subprocess.check_output(f"{cmd} {args}", shell=True, env=env, stderr=devnull)
> + output = subprocess.check_output(args, env=env, stderr=devnull)
> except subprocess.CalledProcessError as err:
> output = err.output
> return output.decode("utf-8").split("\n")
> @@ -59,7 +59,7 @@ def print_output_arr(output_arr):
>
>
> def test_v4l2_compliance(v4l2_compliance, v4l2_compat, device, base_driver):
> - output = run_with_stdout(v4l2_compliance, f"-s -d {device}", {"LD_PRELOAD": v4l2_compat})
> + output = run_with_stdout(v4l2_compliance, "-s", "-d", device, env={"LD_PRELOAD": v4l2_compat})
> result = extract_result(output[-2])
> if result["driver"] != "libcamera":
> return TestSkip
> @@ -104,7 +104,7 @@ def main():
>
> failed = []
> for device in dev_nodes:
> - out = run_with_stdout(v4l2_ctl, f"-D -d {device}")
> + 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
>
> And after further testing, it seems the test stills hangs from time to
> time :-S I'll let you investigate if you can reproduce the issue.
>
> I'm also getting a floating point exception when I run v4l2-compliance
> with LD_PRELOAD on my UVC webcam. Not too surprising as v4l2-compat
> reports
>
> WARN V4L2Compat v4l2_camera_proxy.cpp:515 sizeimage of at least one format is zero. Streaming this format will cause a floating point exception.
>
> The camera produces formats::R8, which isn't listed in pixelFormatInfo.
> I think that will be fixed when moving pixelFormatInfo out of the
> v4l2-compat layer, like we've discussed offline.
Yeah, this has to be fixed.
> > > + except subprocess.CalledProcessError as err:
> > > + output = err.output
> > > + return 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):
> > > + output = run_with_stdout(v4l2_compliance, f"-s -d {device}", {"LD_PRELOAD": v4l2_compat})
> > > + result = extract_result(output[-2])
> > > + if result["driver"] != "libcamera":
> > > + return TestSkip
> > > +
> > > + 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:
> > > + failures = grep("fail", output)
> > > + if re.search("S_FMT cannot handle an invalid format", failures[0]) is None:
>
> Is there a reason not to use your grep function ?
My grep function greps for a substring within a list of strings.
re.search("S_FMT cannot handle an invalid format", failures[0])
vs
grep("S_FMT cannot handle an invalid format", [failures[0]])
I think re.search is good enough for this.
> > > + print_output_arr(output)
> > > + return TestFail
> > > + return TestPass
> > > +
> > > + print_output_arr(output)
> > > + return TestFail
> > > +
> > > +
> > > +def main():
> > > + v4l2_compliance = which("v4l2-compliance")
> > > + if v4l2_compliance is None:
> > > + print("v4l2-compliance is not available")
> > > + return TestSkip
> > > +
> > > + v4l2_ctl = which("v4l2-ctl")
> > > + if v4l2_ctl is None:
> > > + print("v4l2-ctl is not available")
> > > + return TestSkip
> > > +
> > > + v4l2_compat = find_file("v4l2-compat.so", ".")
> > > + if v4l2_compat is None:
> > > + print("v4l2-compat.so is not built")
> > > + return TestSkip
> > > +
> > > + dev_nodes = grep("video", os.listdir("/dev"))
>
> import glob
>
> dev_nodes = glob.glob("/dev/video*")
>
> > > + dev_nodes = list(map(lambda s: f"/dev/{s}", dev_nodes))
>
> Or the more Pythonic syntax:
>
> dev_nodes = [f"/dev/{s]" for s in dev_nodes]
With glob, this is no longer necessary :)
> > > + 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, f"-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}")
> > > +
> > > + return TestPass if not failed else TestFail
> > > +
> > > +
> > > +if __name__ == '__main__':
> > > + sys.exit(main())
I've also figured out how to increase the test timeout in meson.
v2 coming up!
Thanks,
Paul
More information about the libcamera-devel
mailing list