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

[Laurent Pinchart]

Hi Nicolas,

On Thu, Dec 07, 2023 at 01:13:10PM -0500, Nicolas Dufresne wrote:
> Le jeudi 07 décembre 2023 à 18:14 +0200, Laurent Pinchart a écrit :
> > 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

No offense taken.

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

Ah OK I understand your point now. Yes, it's a good point, and we could
indeed disable the fork only when running with ASan enabled.

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

I haven't looked at runtime detection, but worst case we can have
compile-time detection.

> > > > >  
> > > > >         /* 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

-- 
Regards,

Laurent Pinchart