| Message ID | 20231206004425.18189-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | d61104fa7b50d6421109d302ead08c0af62c73df |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I think that's straightforward enough to just merge already. -- Kieran > --- > 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); > > /* 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 >
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@ideasonboard.com> > > > Reviewed-by: Kieran Bingham <kieran.bingham@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. Nicolas > > -- > Kieran > > > > --- > > 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); > > > > /* 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 > >
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@ideasonboard.com> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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. 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. 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); > > > > > > /* 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
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@ideasonboard.com> > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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 >
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@ideasonboard.com> > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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
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); /* 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)
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@ideasonboard.com> --- test/gstreamer/gstreamer_test.cpp | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) base-commit: 4eba2dc73c096d037a8a6390ff4a91ebbf1cedd4