<div dir="ltr"><div dir="ltr"><div>Hi Laurent,</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Aug 18, 2021 at 12:04 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Nicolas,<br>
<br>
On Tue, Aug 17, 2021 at 12:24:05PM -0400, Nicolas Dufresne wrote:<br>
> Le samedi 14 août 2021 à 02:46 +0300, Laurent Pinchart a écrit :<br>
> > From: Vedant Paranjape <<a href="mailto:vedantparanjape160201@gmail.com" target="_blank">vedantparanjape160201@gmail.com</a>><br>
> > <br>
> > This patch adds a test to test if single stream using libcamera's<br>
> > gstreamer element works.<br>
> > <br>
> > We need to work around two distinct issues with ASan when enabled in the<br>
> > build:<br>
> > <br>
> > - glib has a known leak at initialization time. This is covered by the<br>
> > suppression file shipped with glib, but it's not clear how to use it<br>
> > automatically. For now, disable leak detection to avoid test failures.<br>
> <br>
> Are Valgrind suppression usable for Asan ? Glib installs it into:<br>
> <br>
> /usr/share/glib-2.0/valgrind/glib.supp<br>
> <br>
> For GStreamer suppressions, we don't install them (yet, under discussion). They<br>
> are located in each repos. I would enable GStreamer leak tracer though, it does<br>
> not have such a high run-time overhead, and will detect any GObject or<br>
> GstMiniObject leak, as long as the test calls gst_deinit() at the end (all tests<br>
> should).<br>
<br>
Yes, I think they are usable. I was however unsure whether the path to<br>
the supression file was standard, especially considering embedded<br>
systems. I thus went for a workaround. It would be nice if there was a<br>
standard mechanism to pick suppressions automatically.<br>
<br>
> > - GStreamer spawns a child process to scan plugins. If GStreamer is<br>
> > compiled without ASan (which is likely) but libcamera is, dlopen()ing<br>
> > the libcamera plugin will cause an ASan link order verification<br>
> > failure. Disable the verification child processes to work around the<br>
> > problem. This requires gcc 8 or newer.<br>
> <br>
> Have you considered simply disabling forks ? See gst_registry_fork_set_enabled()<br>
<br>
No, as I had no idea that existed :-)<br></blockquote><div><br></div><div>Tested this, it works as expected. Now need to figure out to patch gstreamer as it still leaks memory</div><div><br>=================================================================<br>==676137==ERROR: LeakSanitizer: detected memory leaks<br><br>Direct leak of 16384 byte(s) in 1 object(s) allocated from:<br> #0 0x7f4faf3b8bc8 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10dbc8)<br> #1 0x7f4fae763e98 in g_malloc (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)<br><br>SUMMARY: AddressSanitizer: 16384 byte(s) leaked in 1 allocation(s).<br>――――――――――――――――――――――――――――――――――――――――</div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > Signed-off-by: Vedant Paranjape <<a href="mailto:vedantparanjape160201@gmail.com" target="_blank">vedantparanjape160201@gmail.com</a>><br>
> > Reviewed-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
> > Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> > Tested-by: Kieran Bingham <kieran.bingham@@<a href="http://ideasonboard.com" rel="noreferrer" target="_blank">ideasonboard.com</a>><br>
> > Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> > Signed-off-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> > ---<br>
> > This version incorporates changes coming from my review of v10, and<br>
> > fixes for ASan issues. ASan is a bit of a pain with GStreamer, for two<br>
> > independent reasons as explained in the commit message. I'd like to find<br>
> > a way to use leak suppression files to handle the glib initialization<br>
> > leak, but that's a big rabbit hole. The workaround for the second issue<br>
> > is acceptable in my opinion.<br>
> > <br>
> > The biggest trouble with these workarounds is that they don't work with<br>
> > gcc version older than 8. As the stable version of the most common Linux<br>
> > distributions ship gcc 8 or newer, skipping this test for gcc 7 (which<br>
> > is the oldest version that libcamera supports) is also acceptable in my<br>
> > opinion.<br>
> > <br>
> > Changes since v10:<br>
> > <br>
> > - Disable ASan leak detection<br>
> > - Disable ASan link order verification for child processes<br>
> > - Include source_path.h<br>
> > - Remove unneeded explicit std::string construction<br>
> > - Declare variables at usage site<br>
> > - Make constants constexpr<br>
> > - Add a variable for the message type<br>
> > - Blank space fixes<br>
> > ---<br>
> > .../gstreamer_single_stream_test.cpp | 188 ++++++++++++++++++<br>
> > test/gstreamer/meson.build | 19 ++<br>
> > test/meson.build | 1 +<br>
> > 3 files changed, 208 insertions(+)<br>
> > create mode 100644 test/gstreamer/gstreamer_single_stream_test.cpp<br>
> > create mode 100644 test/gstreamer/meson.build<br>
> > <br>
> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp<br>
> > new file mode 100644<br>
> > index 000000000000..e26673b3471a<br>
> > --- /dev/null<br>
> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp<br>
> > @@ -0,0 +1,188 @@<br>
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */<br>
> > +/*<br>
> > + * Copyright (C) 2021, Vedant Paranjape<br>
> > + *<br>
> > + * ipa_interface_test.cpp - Test the IPA interface<br>
> > + */<br>
> > +<br>
> > +#include <iostream><br>
> > +#include <unistd.h><br>
> > +<br>
> > +#include <libcamera/base/utils.h><br>
> > +<br>
> > +#include "libcamera/internal/source_paths.h"<br>
> > +<br>
> > +#include <gst/gst.h><br>
> > +<br>
> > +#include "test.h"<br>
> > +<br>
> > +using namespace std;<br>
> > +<br>
> > +extern "C" {<br>
> > +const char *__asan_default_options()<br>
> > +{<br>
> > + /*<br>
> > + * Disable leak detection due to a known global variable initialization<br>
> > + * leak in glib's g_quark_init(). This should ideally be handled by<br>
> > + * using a suppression file instead of disabling leak detection.<br>
> > + */<br>
> > + return "detect_leaks=false";<br>
> <br>
> Have you consider GST_TRACERS=leaks ? It will heck for refcounted object leaks<br>
> on gstreamer/glib side, with very low overhead, and will abort on gst_deinit()<br>
> if it found some leaks.<br>
<br>
Again, no idea it existed :-) It could be enabled in init() below.<br>
<br>
> > +}<br>
> > +}<br>
> > +<br>
> > +class GstreamerSingleStreamTest : public Test<br>
> > +{<br>
> > +protected:<br>
> > + int init() override<br>
> > + {<br>
> > + /*<br>
> > + * GStreamer spawns a process to run the gst-plugin-scanner<br>
> > + * helper. If libcamera is compiled with ASan enabled, and as<br>
> > + * GStreamer is most likely not, this will cause the ASan link<br>
> > + * order check to fail when gst-plugin-scanner dlopen()s the<br>
> > + * plugin as many libraries will have already been loaded by<br>
> > + * then. Work around this issue by disabling the link order<br>
> > + * check. This will only affect child processes, as ASan is<br>
> > + * already loaded for this process by the time this code is<br>
> > + * executed, and should thus hopefully be safe.<br>
> > + *<br>
> > + * This option is not available in gcc older than 8, the only<br>
> > + * option in that case is to skip the test.<br>
> > + */<br>
> > +#if defined(__SANITIZE_ADDRESS__) && !defined(__clang__) && __GNUC__ < 8<br>
> > + return TestSkip;<br>
> > +#endif<br>
> > + setenv("ASAN_OPTIONS", "verify_asan_link_order=0", 1);<br>
> <br>
> I think gst_registry_fork_set_enabled(FALSE) would be much cleaner, what do you<br>
> think ?<br>
<br>
If it does the job and has no drawback, that's fine with me.<br>
<br>
> > +<br>
> > + /* Initialize GStreamer */<br>
> > + GError *errInit = nullptr;<br>
> > + if (!gst_init_check(nullptr, nullptr, &errInit)) {<br>
> > + g_printerr("Could not initialize GStreamer: %s\n",<br>
> > + errInit ? errInit->message : "unknown error");<br>
> > + if (errInit)<br>
> > + g_error_free(errInit);<br>
> > +<br>
> > + return TestFail;<br>
> > + }<br>
> > +<br>
> > + /*<br>
> > + * Remove the system libcamera plugin, if any, and add the<br>
> > + * plugin from the build directory.<br>
> > + */<br>
> > + GstRegistry *registry = gst_registry_get();<br>
> > + GstPlugin *plugin = gst_registry_lookup(registry, "libgstlibcamera.so");<br>
> > + if (plugin) {<br>
> > + gst_registry_remove_plugin(registry, plugin);<br>
> > + gst_object_unref(plugin);<br>
> > + }<br>
> > +<br>
> > + std::string path = libcamera::utils::libcameraBuildPath()<br>
> > + + "src/gstreamer";<br>
> > + if (!gst_registry_scan_path(registry, path.c_str())) {<br>
> > + g_printerr("Failed to add plugin to registry\n");<br>
> > + gst_deinit();<br>
> > + return TestFail;<br>
> > + }<br>
> > +<br>
> > + /* Create the elements */<br>
> > + libcameraSrc_ = gst_element_factory_make("libcamerasrc", "libcamera");<br>
> > + convert0_ = gst_element_factory_make("videoconvert", "convert0");<br>
> > + sink0_ = gst_element_factory_make("fakesink", "sink0");<br>
> <br>
> Please use fakevideosink instead.<br>
<br>
That's what Vedant was doing, and the element wasn't present on Kieran's<br>
system. For my own culture, what's the advantage of using fakevideosink<br>
over fakesink ?<br>
<br>
> > +<br>
> > + /* Create the empty pipeline_ */<br>
> > + pipeline_ = gst_pipeline_new("test-pipeline");<br>
> > +<br>
> > + if (!pipeline_ || !convert0_ || !sink0_ || !libcameraSrc_) {<br>
> <br>
> pipeline_ cannot be NULL, glib will abort if malloc failed.<br>
<br>
Good point.<br>
<br>
> > + g_printerr("Not all elements could be created. %p.%p.%p.%p\n",<br>
> > + pipeline_, convert0_, sink0_, libcameraSrc_);<br>
> > + if (pipeline_)<br>
> > + gst_object_unref(pipeline_);<br>
> > + if (convert0_)<br>
> > + gst_object_unref(convert0_);<br>
> > + if (sink0_)<br>
> > + gst_object_unref(sink0_);<br>
> > + if (libcameraSrc_)<br>
> > + gst_object_unref(libcameraSrc_);<br>
> <br>
> Use local g_autoptr() and g_steal_pointer() on success perhaps ? This will<br>
> reduce a lot the about of error prone code in failure case. Note that in other<br>
> software I've seen, an abort is used instead, as clean exit is just coding<br>
> overhead for a test.<br>
> <br>
> An alternative, add them immediately to the pipeline, they are not owned by your<br>
> class anyway.<br>
> <br>
> > + gst_deinit();<br>
> > +<br>
> > + return TestFail;<br>
> > + }<br>
> > +<br>
> > + return TestPass;<br>
> > + }<br>
> > +<br>
> > + void cleanup() override<br>
> > + {<br>
> > + gst_object_unref(pipeline_);<br>
> > + gst_deinit();<br>
> > + }<br>
> > +<br>
> > + int run() override<br>
> > + {<br>
> > + GstStateChangeReturn ret;<br>
> > +<br>
> > + /* Build the pipeline */<br>
> > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL);<br>
> <br>
> This can fail (e.g. duplicate name).<br>
> <br>
> > + if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) {<br>
> > + g_printerr("Elements could not be linked.\n");<br>
> > + return TestFail;<br>
> > + }<br>
> > +<br>
> > + /* Start playing */<br>
> > + ret = gst_element_set_state(pipeline_, GST_STATE_PLAYING);<br>
> > + if (ret == GST_STATE_CHANGE_FAILURE) {<br>
> > + g_printerr("Unable to set the pipeline to the playing state.\n");<br>
> > + return TestFail;<br>
> > + }<br>
> > +<br>
> > + /* Wait until error or EOS or timeout after 2 seconds */<br>
> > + constexpr GstMessageType msgType =<br>
> > + static_cast<GstMessageType>(GST_MESSAGE_ERROR | GST_MESSAGE_EOS);<br>
> > + constexpr GstClockTime timeout = 2000000000;<br>
<br>
> Perhaps 2 * GST_SECOND.<br>
<br>
Much better.<br>
<br>
> > +<br>
> > + g_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);<br>
> > + g_autoptr(GstMessage) msg = gst_bus_timed_pop_filtered(bus, timeout, msgType);<br>
> > +<br>
> > + gst_element_set_state(pipeline_, GST_STATE_NULL);<br>
> > +<br>
> > + /* Parse error message */<br>
> > + if (msg == NULL)<br>
> > + return TestPass;<br>
> <br>
> I would like some minimal validation. I would expect that after 2s some frames<br>
> got "rendered" properly. You can read the GstStructure property "stats" from<br>
> fakevideosink / fakesink, and read the "rendered" field. Make sure this not zero<br>
> perhaps ?<br>
<br>
Very good idea.<br>
<br>
Vedant, I think there's work for you :-)<br>
<br>
> > +<br>
> > + switch (GST_MESSAGE_TYPE(msg)) {<br>
> > + case GST_MESSAGE_ERROR:<br>
> > + gstreamer_print_error(msg);<br>
> > + break;<br>
> > + case GST_MESSAGE_EOS:<br>
> > + g_print("End-Of-Stream reached.\n");<br>
> > + break;<br>
> > + default:<br>
> > + g_printerr("Unexpected message received.\n");<br>
> > + break;<br>
> > + }<br>
> > +<br>
> > + return TestFail;<br>
> > + }<br>
> > +<br>
> > +private:<br>
> > + void gstreamer_print_error(GstMessage *msg)<br>
> > + {<br>
> > + GError *err;<br>
> > + gchar *debug_info;<br>
> > +<br>
> > + gst_message_parse_error(msg, &err, &debug_info);<br>
> > + g_printerr("Error received from element %s: %s\n",<br>
> > + GST_OBJECT_NAME(msg->src), err->message);<br>
> > + g_printerr("Debugging information: %s\n",<br>
> > + debug_info ? debug_info : "none");<br>
> > + g_clear_error(&err);<br>
> > + g_free(debug_info);<br>
> > + }<br>
> > +<br>
> > + GstElement *pipeline_;<br>
> > + GstElement *libcameraSrc_;<br>
> > + GstElement *convert0_;<br>
> > + GstElement *sink0_;<br>
> > +};<br>
> > +<br>
> > +TEST_REGISTER(GstreamerSingleStreamTest)<br>
> > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build<br>
> > new file mode 100644<br>
> > index 000000000000..b99aa0da0ba3<br>
> > --- /dev/null<br>
> > +++ b/test/gstreamer/meson.build<br>
> > @@ -0,0 +1,19 @@<br>
> > +# SPDX-License-Identifier: CC0-1.0<br>
> > +<br>
> > +if not gst_enabled<br>
> > + subdir_done()<br>
> > +endif<br>
> > +<br>
> > +gstreamer_tests = [<br>
> > + ['single_stream_test', 'gstreamer_single_stream_test.cpp'],<br>
> > +]<br>
> > +gstreamer_dep = dependency('gstreamer-1.0', required: true)<br>
> > +<br>
> > +foreach t : gstreamer_tests<br>
> > + exe = executable(t[0], t[1],<br>
> > + dependencies : [libcamera_private, gstreamer_dep],<br>
> > + link_with : test_libraries,<br>
> > + include_directories : test_includes_internal)<br>
> > +<br>
> > + test(t[0], exe, suite : 'gstreamer', is_parallel : false)<br>
> > +endforeach<br>
> > diff --git a/test/meson.build b/test/meson.build<br>
> > index 3bceb5df586f..d0466f17d7b6 100644<br>
> > --- a/test/meson.build<br>
> > +++ b/test/meson.build<br>
> > @@ -11,6 +11,7 @@ subdir('libtest')<br>
> > <br>
> > subdir('camera')<br>
> > subdir('controls')<br>
> > +subdir('gstreamer')<br>
> > subdir('ipa')<br>
> > subdir('ipc')<br>
> > subdir('log')<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br></blockquote><div><br></div><div>Regards,</div><div>Vedant Paranjape<br></div></div></div>