[libcamera-devel] [PATCH] test: gstreamer: Fix indentation in comments

[Nicolas Dufresne]

Le jeudi 07 décembre 2023 à 18:14 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> On Thu, Dec 07, 2023 at 10:43:02AM -0500, Nicolas Dufresne wrote:
> > Le mercredi 06 décembre 2023 à 00:49 +0000, Kieran Bingham via libcamera-devel a écrit :
> > > Quoting Laurent Pinchart via libcamera-devel (2023-12-06 00:44:25)
> > > > A couple of comments are mis-indented in the gstreamer unit test. Fix
> > > > them, and reflow the text while at it.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > 
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > 
> > > I think that's straightforward enough to just merge already.
> > 
> > Ack. It highlights the code though, which I wasn't aware of. Seems generally a
> > bad idea to always disable the protection again a possibly broken/crashing
> > plugin init in the system. I might suggest to find a way to only enable it for
> > specific "ASAN" issue workaround.
> 
> Disabling the fork doesn't disable any protection, so I don't see any
> issue there. I may be missing something though.

If you disable the fork, and one of the plugin crash or abort, your program is
taken away with it. While if you keep the fork, the problematic plugin is (sorry
for the term, was implemented a long time ago) black listed, and a new fork is
started.

As we encourage running this test on whatever GStreamer installation you have,
I'd also encourage to keep that fork by default in case there is a a slightly
broken (but unrelated) installation issue. As we state, we don't care about
anything else then coreelements and libcamera plugins.

p.s. Note that this fork is today a separate program, typically
"/usr/libexec/gstreamer-1.0/gst-plugin-scanner".

> 
> Are you talking about the code that removes libgstlibcamera.so from the
> registry ? The point of that is to make sure we test the plugin from the
> build directory, and don't pick a plugin from the system by mistake.

No, not talking about this one, I have a patch to remove that code. see below..

> 
> Or are you talking about the __asan_default_options() function in the
> same file ? I would like to drop that indeed.
> 
> > > > ---
> > > >  test/gstreamer/gstreamer_test.cpp | 23 +++++++++++------------
> > > >  1 file changed, 11 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/test/gstreamer/gstreamer_test.cpp b/test/gstreamer/gstreamer_test.cpp
> > > > index 6ad0c15cd0e2..091f7bf70288 100644
> > > > --- a/test/gstreamer/gstreamer_test.cpp
> > > > +++ b/test/gstreamer/gstreamer_test.cpp
> > > > @@ -31,15 +31,14 @@ GstreamerTest::GstreamerTest(unsigned int numStreams)
> > > >         : pipeline_(nullptr), libcameraSrc_(nullptr)
> > > >  {
> > > >         /*
> > > > -       * GStreamer by default spawns a process to run the
> > > > -       * gst-plugin-scanner helper. If libcamera is compiled with ASan
> > > > -       * enabled, and as GStreamer is most likely not, this causes the
> > > > -       * ASan link order check to fail when gst-plugin-scanner
> > > > -       * dlopen()s the plugin as many libraries will have already been
> > > > -       * loaded by then. Fix this issue by disabling spawning of a
> > > > -       * child helper process when scanning the build directory for
> > > > -       * plugins.
> > > > -       */
> > > > +        * GStreamer by default spawns a process to run the gst-plugin-scanner
> > > > +        * helper. If libcamera is compiled with ASan enabled, and as GStreamer
> > > > +        * is most likely not, this causes the ASan link order check to fail
> > > > +        * when gst-plugin-scanner dlopen()s the plugin as many libraries will
> > > > +        * have already been loaded by then. Fix this issue by disabling
> > > > +        * spawning of a child helper process when scanning the build directory
> > > > +        * for plugins.
> > > > +        */
> > > >         gst_registry_fork_set_enabled(false);

This is the call I'd like to see go away, or be limited to whenever ASan is
going to be used. I'm not familiar with how ASan work and what it provides for
run-time detection (I only really know valgrind, which does offer method to
detect it).

> > > >  
> > > >         /* Initialize GStreamer */
> > > > @@ -53,9 +52,9 @@ GstreamerTest::GstreamerTest(unsigned int numStreams)
> > > >         }
> > > >  
> > > >         /*
> > > > -       * Remove the system libcamera plugin, if any, and add the
> > > > -       * plugin from the build directory.
> > > > -       */
> > > > +        * 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)
> > > > 
> > > > base-commit: 4eba2dc73c096d037a8a6390ff4a91ebbf1cedd4
>