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

[Laurent Pinchart]

Hi Kieran,

On Tue, Feb 01, 2022 at 10:26:06AM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-02-01 07:20:52)
> > 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.

Ah yes I didn't notice. Do applications typically return an error status
with -h ("exit 1") or a success status ("exit 0") ? I usually code the
latter, but I may be wrong.

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

Here it should be "exit 1".

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

That was my thinking too, as there's now a script, the binary path is
less important.

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

-- 
Regards,

Laurent Pinchart