[libcamera-devel] [PATCH] meson: Remove pipelines list duplication

[Kieran Bingham]

Quoting Laurent Pinchart (2023-01-12 09:30:46)
> Hi Javier,
> 
> Thank you for the patch.
> 
> On Thu, Jan 12, 2023 at 12:22:21AM +0100, Javier Martinez Canillas wrote:
> > The supported pipelines are listed in three places: the meson_options.txt
> > file, the defined array when a user selects -Dpipelines="all", and arrays
> > defined when the default -Dpipelines="auto" is selected.
> > 
> > This is hard to maintain and error prone, let's at least in the meson file
> > have a single place where these pipelines lists are defined.
> > 
> > Signed-off-by: Javier Martinez Canillas <javierm at redhat.com>
> > ---
> > 
> >  meson.build | 31 +++++++++++++++++--------------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> Looking at the diffstat, I'm not sure if it's worth it. It feels a bit
> overengineered, especially given that the pipeline handlers also have to
> be added to meson_options.txt too. If we had thousands of pipeline
> handlers we would need to do better, but at the moment, I'm not entirely
> convinced.
> 
> If you want to continue in this direction, I would at least turn the
> pipelines_* variables into a single dictionary indexed by architecture,
> or do it the other way around, indexing by pipeline handler name and
> listing the supported architectures as the value. This could help making
> the x86 vs. x86_64 and arm vs. aarch64 a bit less special cases.

Precisely what I had in mind last night.

Sketched out that would be:

pipelines = get_option('pipelines')

pipes_support = {
  'imx8-isi':     ['aarch64'],
  'ipu3':         ['x86', 'x86_64'],
  'raspberrypi':  ['arm', 'aarch64'],
  'rkisp1':       ['aarch64'],
  'simple':       ['arm', 'aarch64'],
  'uvcvideo':     ['any'],
  'vimc':         ['test'],
  'virtual':      ['test'],
}

if pipelines.contains('all')
    pipelines = []
    foreach pipe, archs : pipes_support
        pipelines += pipe
    endforeach
endif

if pipelines.contains('auto')
    host_cpu = host_machine.cpu_family()
    pipelines = []
    foreach pipe, archs : pipes_support
        if host_cpu in archs or 'any' in archs
            message('Auto: Enabling ' + pipe + ' pipeline handler')
            pipelines += pipe
        endif
    endforeach
endif

if get_option('test')
    foreach pipe, archs : pipes_support
        if 'test' in archs and pipe not in pipelines
            message('Test Support: Enabling ' + pipe + ' pipeline handler')
            pipelines += pipe
        endif
    endforeach
endif


> 
> > diff --git a/meson.build b/meson.build
> > index 389c547206fb..f9e3280ec0f2 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -168,38 +168,41 @@ liblttng = dependency('lttng-ust', required : get_option('tracing'))
> >  # are enabled.
> >  pipelines = get_option('pipelines')
> >  
> > +pipelines_aarch64 = ['imx8-isi', 'rkisp1']
> > +pipelines_arm = ['raspberrypi', 'simple']
> > +pipelines_agnostic = ['uvcvideo']
> > +pipelines_test = ['vimc']
> > +pipelines_x86 = ['ipu3']
> > +
> >  if pipelines.contains('all')
> > -    pipelines = [
> > -        'imx8-isi',
> > -        'ipu3',
> > -        'raspberrypi',
> > -        'rkisp1',
> > -        'simple',
> > -        'uvcvideo',
> > -        'vimc',
> > -    ]
> > +    pipelines = []
> > +    pipelines += pipelines_aarch64
> > +    pipelines += pipelines_arm
> > +    pipelines += pipelines_agnostic
> > +    pipelines += pipelines_test
> > +    pipelines += pipelines_x86
> >  endif
> >  
> >  if pipelines.contains('auto')
> >      host_cpu = host_machine.cpu_family()
> >      pipelines = []
> >      if host_cpu == 'x86' or host_cpu == 'x86_64'
> > -        pipelines += ['ipu3']
> > +        pipelines += pipelines_x86
> >      elif host_cpu == 'aarch64'
> > -        pipelines += ['imx8-isi', 'rkisp1']
> > +        pipelines += pipelines_aarch64
> >      endif
> >  
> >      if host_cpu == 'arm' or host_cpu == 'aarch64'
> > -        pipelines += ['raspberrypi', 'simple']
> > +        pipelines += pipeines_arm
> >      endif
> >  
> >      # Always include the uvcvideo pipeline handler.
> > -    pipelines += ['uvcvideo']
> > +    pipelines += pipelines_agnostic
> >  endif
> >  
> >  if get_option('test') and 'vimc' not in pipelines
> >      message('Enabling vimc pipeline handler to support tests')
> > -    pipelines += ['vimc']
> > +    pipelines += pipelines_test
> >  endif
> >  
> >  # Utilities are parsed first to provide support for other components.
> 
> -- 
> Regards,
> 
> Laurent Pinchart