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

[Laurent Pinchart]

Hi Nicolas,

On Mon, Jan 08, 2024 at 10:42:06AM -0500, Nicolas Dufresne wrote:
> Le mardi 02 janvier 2024 à 11:09 +0000, Kieran Bingham a écrit :
> > Quoting Nicolas Dufresne via libcamera-devel (2023-12-05 20:18:47)
> > > Le mardi 05 décembre 2023 à 18:19 +0200, Laurent Pinchart a écrit :
> > > > 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 ?

Not really. I just think the current mechanism is a bit of a hack, and
is likely fragile, I would be happy to drop it if we could. There's no
urgency though.

When it comes to running binaries from the build directory for quick
testing, it's not a workflow we've committed to support in the same way
forever. I think requiring usage of meson devenv would be reasonable, if
it gives us all we need in a simple way.

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

-- 
Regards,

Laurent Pinchart