[libcamera-devel] [PATCH v2] v4l2: Provide libcamerify wrapper script

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Mon, Jan 31, 2022 at 09:07:57PM +0000, Kieran Bingham wrote:
> Support easier usage of the v4l2 compatibility layer with a script that

s/v4l2/V4L2/

> handles the LD_PRELOAD for applications.
> 
> The wrapper can be prefixed to launch any application with the preload
> set:
> 
>  $ libcamerify v4l2-ctl --list-devices
>  \_SB_.PCI0.GP13.XHC0.RHUB.PRT4- (libcamera:0):
>  	/dev/video0
> 
>  platform/vimc.0 Sensor B (libcamera:1):
>  	/dev/video2
>  	/dev/video3
>  	/dev/video4
> 
> Specifying '-d' once before the command to run will enable V4L2Compat
> layer debug output from libcamera.
> 
> Specifying '-d' twice will enable all debug levels from all libcamera
> components and provide a very detailed log for analysis.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> v2:
>  - rename to libcamerify (from libcamera-v4l2)
>  - add extending support for setting debug log levels.
> 
> 
>  src/v4l2/libcamerify.in | 34 ++++++++++++++++++++++++++++++++++
>  src/v4l2/meson.build    | 14 ++++++++++++++
>  2 files changed, 48 insertions(+)
>  create mode 100755 src/v4l2/libcamerify.in
> 
> diff --git a/src/v4l2/libcamerify.in b/src/v4l2/libcamerify.in
> new file mode 100755
> index 000000000000..161f5e49190e
> --- /dev/null
> +++ b/src/v4l2/libcamerify.in
> @@ -0,0 +1,34 @@
> +#!/bin/sh

License ?

> +
> +if [ "$LD_PRELOAD" = "" ] ; then
> +   LD_PRELOAD='@LIBCAMERA_V4L2_SO@'
> +else
> +   LD_PRELOAD="$LD_PRELOAD "'@LIBCAMERA_V4L2_SO@'
> +fi
> +
> +export LD_PRELOAD

Can you move this just before the exec call, to avoid LD_PRELOADing the
library in other commands executed by thie script ?

> +
> +help() {
> +	echo "$0: Load an application with libcamera V4L2 compatibility layer preload"
> +	echo " $0 [OPTIONS...] executable [args]"
> +	echo " -d, --debug	Increase log level"
> +	exit 1;
> +}
> +
> +debug=0
> +while [ $# -gt 0 ]; do
> +	case $1 in
> +		-d|--debug) debug=$((debug+1));;
> +		-h) help; break;;

"break" should be "exit 0".

> +		--) shift; break;;
> +		-*) echo "Unrecognised option: $1"; help; break;;

Same here.

> +		*) break;;

I'd find this more readable:

		-d|--debug)
			debug=$((debug+1))
			;;
		-h)
			help
			break
			;;
		--)
			shift
			break
			;;
		-*)
			echo "Unrecognised option: $1"
			help
			break
			;;
		*)
			break
			;;

> +	esac
> +	shift
> +done
> +
> +[ $debug -gt 0 ] && loglevel=V4L2Compat:0
> +[ $debug -gt 1 ] && loglevel=0
> +[ "$loglevel" != "" ] && export LIBCAMERA_LOG_LEVELS=$loglevel
> +
> +exec "$@"
> diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build
> index f78497b6799b..07ff9a6939d2 100644
> --- a/src/v4l2/meson.build
> +++ b/src/v4l2/meson.build
> @@ -33,3 +33,17 @@ v4l2_compat = shared_library('v4l2-compat',
>                               install : true,
>                               dependencies : [libcamera_private, libdl],
>                               cpp_args : v4l2_compat_cpp_args)
> +
> +# Provide a wrapper script to support easily loading applications with the V4L2
> +# adaptation layer
> +
> +config_h.set('LIBCAMERA_V4L2_SO', get_option('prefix') / get_option('libdir') / 'v4l2-compat.so')

I don't think this should be added to config.h. Can you create a
separate variable ?

cdata = configuration_data()
cdata.set('LIBCAMERA_V4L2_SO', get_option('prefix') / get_option('libdir') / 'v4l2-compat.so')

I also wonder if we could use v4l2_compat.name() instead of
'v4l2-compat.so' (but that's only available starting in meson 0.54).

> +
> +summary({
> +    'LIBCAMERA_V4L2_SO' : config_h.get('LIBCAMERA_V4L2_SO'),
> +}, section : 'Paths')

I would have skipped this. It's a bit out of place, with all the other
paths being directories.

> +
> +configure_file(input : 'libcamerify.in',
> +               output : 'libcamerify',
> +               configuration : config_h,
> +               install_dir : get_option('bindir'))

-- 
Regards,

Laurent Pinchart