[EXT] [PATCH v2] libcamera: virtual: Install configuration file

[Laurent Pinchart]

On Fri, Jan 03, 2025 at 06:43:18PM +0100, Julien Vuillaumier wrote:
> On 20/12/2024 17:44, Barnabás Pőcze wrote:
> > 2024. december 20., péntek 17:17 keltezéssel, Julien Vuillaumier írta:
> >> On 20/12/2024 14:54, Barnabás Pőcze wrote:
> >>
> >>> Install the example configuration file of the virtual pipeline
> >>> handler as it serves documentation purposes, and to make the
> >>> virtual pipeline handler easily usable in CI.
> >>>
> >>> Nonetheless, the file is installed with the ".example" suffix
> >>> so that it will not be used by default, to avoid cluttering
> >>> the camera lists of users whose distributions decide to
> >>> enable the virtual pipeline handler.
> >>>
> >>> The file is installed in the proper location for convenience:
> >>> (1) is is easier to use it in the CI;
> >>> (2) users need not browse documentation to determine where
> >>> they should place the file.
> >>
> >> Virtual pipeline, when enabled, outputs a noisy error message during the
> >> match() when its configuration file is not installed:
> >>
> >> DEBUG Camera camera_manager.cpp:140 Found registered pipeline handler
> >> 'virtual'
> >> ERROR Pipeline pipeline_handler.cpp:629 Configuration file
> >> '/usr/share/libcamera/pipeline/virtual/virtual.yaml' not found for
> >> pipeline handler 'virtual'
> >> ERROR Virtual virtual.cpp:336 Failed to open config file:
> >>
> >> Should virtual.yaml be installed with its actual name without the
> >> '.example' suffix. That would clear the error, though augmenting the
> >> camera list.
> > 
> > That is indeed correct, but there should be no issues apart from that.
> > Is it causing any problems?
> 
> There is no functional issue - however it is confusing to have a 
> pipeline handler reporting errors in the libcamera log during the match.
> 
> > I don't believe adding the virtual cameras is acceptable for any end user.
> 
> I am not sure that adding virtual cameras is really an issue for the end 
> user:
> - virtual pipeline being present means that 'test' meson config option 
> equals true - option may not be used by distributions (just speculating)

The virtual pipeline handler can also be enabled by setting the
'pipelines' meson option, without enabling the 'test' option.

> - vimc pipeline handler is already adding virtual cameras

Correct, but only if the vimc driver is loaded.

> - vimc 
> pipeline presence also depends on the 'test' meson option.

It can also be enabled through the 'pipelines' option.

> It is only my view but I see it preferable for the end user to have 
> virtual cameras added and no error messages.

I think we all agree that the error message is wrong.

By default, I believe no virtual camera should be created. That can be
achieved by ensuring that distributions either do not enable the virtual
pipeline handler in their libcamera package (either explicitly, or
through the 'test' option), or do not install the virtual pipeline
handler configuration file.

I also believe that end users could find the virtual pipeline handler
useful, which would call for enabling it in distribution packages. If
that's the case, distributions should not install the configuration
file.

At the end of the day distributions will decide what to do, regardless
of what we do in meson.build. They can decide not to package the virtual
pipeline handler configuration file even if we install it. Conversely,
they can also decide to strip the .example suffix from the file we
install, if we decide to install an example file only.

My personal opinion is that we should install the sample configuration
file with a .example suffix, and demote the error message to an info
message. When running libcamera from the build directory the virtual
pipeline handler configuration file will be loaded, making it easy to
develop the pipeline handler without having to install libcamera or
rename files.

> > Installing it with the ".example" suffix means that it is trivial for
> > distributions to ship the virtual pipeline with an example configuration
> > without it interfering normal operations.
> > 
> > I suppose a placeholder `virtual.yaml` file could be installed with the
> > following content: { }
> > 
> > But I am not entirely sure if it's worth it >
> > 
> >>> Signed-off-by: Barnabás Pőcze pobrn at protonmail.com
> >>> Reviewed-by: Kieran Bingham kieran.bingham at ideasonboard.com
> >>> Reviewed-by: Harvey Yang chenghaoyang at chromium.org
> >>> Reviewed-by: Laurent Pinchart laurent.pinchart at ideasonboard.com
> >>> ---
> >>> src/libcamera/pipeline/virtual/data/meson.build | 4 ++++
> >>> src/libcamera/pipeline/virtual/meson.build | 2 ++
> >>> 2 files changed, 6 insertions(+)
> >>> create mode 100644 src/libcamera/pipeline/virtual/data/meson.build
> >>>
> >>> diff --git a/src/libcamera/pipeline/virtual/data/meson.build b/src/libcamera/pipeline/virtual/data/meson.build
> >>> new file mode 100644
> >>> index 000000000..ce63f9a27
> >>> --- /dev/null
> >>> +++ b/src/libcamera/pipeline/virtual/data/meson.build
> >>> @@ -0,0 +1,4 @@
> >>> +install_data('virtual.yaml',
> >>> + install_dir : pipeline_data_dir / 'virtual',
> >>> + install_tag : 'runtime',
> >>> + rename: 'virtual.yaml.example')
> >>> diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build
> >>> index 4786fe2e0..c84345936 100644
> >>> --- a/src/libcamera/pipeline/virtual/meson.build
> >>> +++ b/src/libcamera/pipeline/virtual/meson.build
> >>> @@ -11,3 +11,5 @@ libjpeg = dependency('libjpeg', required : true)
> >>>
> >>> libcamera_deps += [libyuv_dep]
> >>> libcamera_deps += [libjpeg]
> >>> +
> >>> +subdir('data')

-- 
Regards,

Laurent Pinchart