[libcamera-devel] [PATCH] gstreamer: Add meson devenv support

[Nicolas Dufresne]

Hi,

Le mardi 02 janvier 2024 à 11:09 +0000, Kieran Bingham a écrit :
> Quoting Nicolas Dufresne via libcamera-devel (2023-12-05 20:18:47)
> > Hi,
> > 
> > Le mardi 05 décembre 2023 à 18:19 +0200, Laurent Pinchart a écrit :
> > > Hi Nicolas,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Tue, Dec 05, 2023 at 09:00:10AM -0500, Nicolas Dufresne via libcamera-devel wrote:
> > > > Le lundi 04 décembre 2023 à 16:27 -0500, Nicolas Dufresne via libcamera-devel a écrit :
> > > > > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > > > 
> > > > > This change to the build system will prepend the plugin build directory
> > > > > to GST_PLUGIN_PATH environment. This makes the built plugin visible to
> > > > > GStreamer inside meson devenv enabling uninstalled testing.
> > > > 
> > > > In case of positive feedback, I'd like to make a V2 that updates the
> > > > documentation. Instead of:
> > > > 
> > > >   export GST_PLUGIN_PATH=$(pwd)/build/src/gstreamer
> > > > 
> > > > I'd document:
> > > > 
> > > >   meson devenv -C build/
> 
> Ack.
> 
> > > > 
> > > > Devenv is nicer since it prepends to the path, it will also blend well if
> > > > libcamera is made a subproject of another project using devenv (notably mesa
> > > > and/or GStreamer). In cross-compilation, the env can be generated on the build
> > > > computer, and used as a script on the target with help of the --dump option.
> > > 
> > > I didn't know about meson devenv, it seems to be an interesting feature.
> > > We have a bit of code in libcamera and in the unit tests to explicitly
> > > support running the libcamera test executables and the unit tests from
> > > the build directory and find the correct resources. I would be happy to
> > > remove that if it could be replaced by meson devenv.
> > 
> > Tests have their own "env" option, which does not need to match the devenv.
> > 
> > https://mesonbuild.com/Unit-tests.html
> 
> Hrm ... looks like that would need to be added to every test (at least
> every test that needs it - but perhaps it's better to add it to all to
> be consistent).
> 
> So we might have to make this env global to libcamera as a
> libcamera_runtime_env or such.
> 
> Could be done later though ... Not this patch.

I'll give it some further thought while making v2, but as said, we can split
adding devenv from setting tests env initially.

> 
> > > Related to this patch, we have the following code in
> > > test/gstreamer/gstreamer_test.cpp:
> > > 
> > >       /*
> > >        * Remove the system libcamera plugin, if any, and add the plugin from
> > >        * the build directory.
> > >        */
> > >       GstRegistry *registry = gst_registry_get();
> > >       g_autoptr(GstPlugin) plugin = gst_registry_lookup(registry, "libgstlibcamera.so");
> > >       if (plugin)
> > >               gst_registry_remove_plugin(registry, plugin);
> > > 
> > >       std::string path = libcamera::utils::libcameraBuildPath() + "src/gstreamer";
> > >       if (!gst_registry_scan_path(registry, path.c_str())) {
> > >               g_printerr("Failed to add plugin to registry\n");
> > > 
> > >               status_ = TestFail;
> > >               return;
> > >       }
> > > 
> > > Do you think this could be dropped ? I haven't checked if the meson unit
> > > tests are run in a similar environment as devenv though.
> > 
> > We can certainly drop this and just use the same env for devenv and the meson
> > test. The registry cache is invalidate when the env changes and path order is
> > respected.
> 
> Sounds like we could look to removing the isLibcameraInstalled()
> DT_RUNPATH/DT_RPATH workaround too?

I did think about this, but the workaround is something that just work without
having to interact with meson. What would happen is that some people workflow
would be affected. For this reason another subject that is best handled
separately. Are you aware of known maintenance issue with the current approach ?

> 
> 
> > > > > ---
> > > > >  src/gstreamer/meson.build | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
> > > > > index 20784b71..3810a15b 100644
> > > > > --- a/src/gstreamer/meson.build
> > > > > +++ b/src/gstreamer/meson.build
> > > > > @@ -46,3 +46,10 @@ libcamera_gst = shared_library('gstlibcamera',
> > > > >      install : true,
> > > > >      install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),
> > > > >  )
> > > > > +
> > > > > +# Makes the plugin visble to GStreamer inside meson devenv
> > > > > +fs = import('fs')
> > > > > +plugin_path = fs.parent(libcamera_gst.full_path())
> > > > > +env = environment()
> > > > > +env.prepend('GST_PLUGIN_PATH', fs.parent(plugin_path))
> > > > > +meson.add_devenv(env)
> 
>  - Cc: Tomi - Is this something that should be added for Python too?
> 
> This patch does seem fairly self sufficient though, and otherwise
> unobtrusive, (and seemingly beneficial).
> 
> With or without your proposed update to README.rst
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Thank you, I'll refresh my memory and rebase locally and will soon come back
with at least an updated readme, which I want for my self too.

Nicolas

> 
> --
> Kieran