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

[Kieran Bingham]

Quoting Kieran Bingham (2023-01-12 09:47:12)
> 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'],

Note I've intentionally kept rkisp1 and imx8-isi as aarch64 only, as I
believe that's how I interpret the current implementation.

I expect it should really be both for each of them?
We could wrap a x86 = ['x86', 'x86_64'], and an arm = ['arm', 'aarch64']
helper too.

>   '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