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

[Kieran Bingham]

Quoting Laurent Pinchart (2022-02-01 07:20:52)
> 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 ?

Eeep yes. How did I miss that.

> 
> > +
> > +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".

help() was already calling exit 1;, but yes, I can move that in here.


> 
> > +             --) 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
>                         ;;
> 

sure

> > +     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).

I also asked in IRC #meson about this, hoping I could get a binary
target to tell me it's full installation path but alas...

> 
> > +
> > +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.

But it's also a path/file that (at least without this script) people need to
know, so that tehy can do LD_PRELOAD= themselves.

I can drop it though, as I expect this script will help exactly those
users.


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