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

[Julien Vuillaumier]

Hi Barnabás,

On 20/12/2024 17:44, Barnabás Pőcze wrote:
> 
> 
> Hi
> 
> 
> 2024. december 20., péntek 17:17 keltezéssel, Julien Vuillaumier <julien.vuillaumier at nxp.com> írta:
> 
>> Hi Barnabás,
>>
>> 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)
- vimc pipeline handler is already adding virtual cameras - vimc 
pipeline presence also depends on the 'test' meson option.

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

> 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 >
> 
> Regards,
> Barnabás Pőcze
> 

Thanks,
Julien

>>
>>> 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')
>>> --
>>
>>
>> Thanks,
>> Julien