<div dir="ltr"><div dir="ltr"><div>Hi Umang,</div><div>Thanks for the Review. I feel the changes are appropriate. I will send a patch with the changes.</div><div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Aug 16, 2021 at 11:17 AM Umang Jain <<a href="mailto:umang.jain@ideasonboard.com">umang.jain@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">
<div>
<p>Hi Vedant and Laurent,</p>
<p>Thanks for the patch. Overall looks good to me,<br>
</p>
<p>a few comments below for cleanup paths :)<br>
</p>
<div>On 8/14/21 5:16 AM, Laurent Pinchart
wrote:<br>
</div>
<blockquote type="cite">
<pre>From: Vedant Paranjape <a href="mailto:vedantparanjape160201@gmail.com" target="_blank"><vedantparanjape160201@gmail.com></a>
This patch adds a test to test if single stream using libcamera's
gstreamer element works.
We need to work around two distinct issues with ASan when enabled in the
build:
- glib has a known leak at initialization time. This is covered by the
suppression file shipped with glib, but it's not clear how to use it
automatically. For now, disable leak detection to avoid test failures.
- GStreamer spawns a child process to scan plugins. If GStreamer is
compiled without ASan (which is likely) but libcamera is, dlopen()ing
the libcamera plugin will cause an ASan link order verification
failure. Disable the verification child processes to work around the
problem. This requires gcc 8 or newer.
Signed-off-by: Vedant Paranjape <a href="mailto:vedantparanjape160201@gmail.com" target="_blank"><vedantparanjape160201@gmail.com></a>
Reviewed-by: Paul Elder <a href="mailto:paul.elder@ideasonboard.com" target="_blank"><paul.elder@ideasonboard.com></a>
Reviewed-by: Kieran Bingham <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank"><kieran.bingham@ideasonboard.com></a>
Tested-by: Kieran Bingham <a href="mailto:kieran.bingham@@ideasonboard.com" target="_blank"><kieran.bingham@@ideasonboard.com></a>
Reviewed-by: Laurent Pinchart <a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank"><laurent.pinchart@ideasonboard.com></a>
Signed-off-by: Laurent Pinchart <a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank"><laurent.pinchart@ideasonboard.com></a>
---
This version incorporates changes coming from my review of v10, and
fixes for ASan issues. ASan is a bit of a pain with GStreamer, for two
independent reasons as explained in the commit message. I'd like to find
a way to use leak suppression files to handle the glib initialization
leak, but that's a big rabbit hole. The workaround for the second issue
is acceptable in my opinion.
The biggest trouble with these workarounds is that they don't work with
gcc version older than 8. As the stable version of the most common Linux
distributions ship gcc 8 or newer, skipping this test for gcc 7 (which
is the oldest version that libcamera supports) is also acceptable in my
opinion.
Changes since v10:
- Disable ASan leak detection
- Disable ASan link order verification for child processes
- Include source_path.h
- Remove unneeded explicit <a>std::string</a> construction
- Declare variables at usage site
- Make constants constexpr
- Add a variable for the message type
- Blank space fixes
---
.../gstreamer_single_stream_test.cpp | 188 ++++++++++++++++++
test/gstreamer/meson.build | 19 ++
test/meson.build | 1 +
3 files changed, 208 insertions(+)
create mode 100644 test/gstreamer/gstreamer_single_stream_test.cpp
create mode 100644 test/gstreamer/meson.build
diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp
new file mode 100644
index 000000000000..e26673b3471a
--- /dev/null
+++ b/test/gstreamer/gstreamer_single_stream_test.cpp
@@ -0,0 +1,188 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2021, Vedant Paranjape
+ *
+ * ipa_interface_test.cpp - Test the IPA interface
+ */
+
+#include <iostream>
+#include <unistd.h>
+
+#include <libcamera/base/utils.h>
+
+#include "libcamera/internal/source_paths.h"
+
+#include <gst/gst.h>
+
+#include "test.h"
+
+using namespace std;
+
+extern "C" {
+const char *__asan_default_options()
+{
+ /*
+ * Disable leak detection due to a known global variable initialization
+ * leak in glib's g_quark_init(). This should ideally be handled by
+ * using a suppression file instead of disabling leak detection.
+ */
+ return "detect_leaks=false";
+}
+}
+
+class GstreamerSingleStreamTest : public Test
+{
+protected:
+ int init() override
+ {
+ /*
+ * GStreamer 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 will cause 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. Work around this issue by disabling the link order
+ * check. This will only affect child processes, as ASan is
+ * already loaded for this process by the time this code is
+ * executed, and should thus hopefully be safe.
+ *
+ * This option is not available in gcc older than 8, the only
+ * option in that case is to skip the test.
+ */
+#if defined(__SANITIZE_ADDRESS__) && !defined(__clang__) && __GNUC__ < 8
+ return TestSkip;
+#endif
+ setenv("ASAN_OPTIONS", "verify_asan_link_order=0", 1);
+
+ /* Initialize GStreamer */
+ GError *errInit = nullptr;</pre>
</blockquote>
One can use:<br>
<p> g_autoptr(GError) errInit = NULL;</p>
<p>...<br>
</p>
<blockquote type="cite">
<pre>+ if (!gst_init_check(nullptr, nullptr, &errInit)) {
+ g_printerr("Could not initialize GStreamer: %s\n",
+ errInit ? errInit->message : "unknown error");
+ if (errInit)
+ g_error_free(errInit);</pre>
</blockquote>
And remove manual free here.<br>
<blockquote type="cite">
<pre>+
+ return TestFail;
+ }
+
+ /*
+ * Remove the system libcamera plugin, if any, and add the
+ * plugin from the build directory.
+ */
+ GstRegistry *registry = gst_registry_get();
+ GstPlugin *plugin = gst_registry_lookup(registry, "libgstlibcamera.so");
+ if (plugin) {
+ gst_registry_remove_plugin(registry, plugin);
+ gst_object_unref(plugin);
+ }
+
+ <a>std::string</a> path = <a>libcamera::utils::libcameraBuildPath()</a>
+ + "src/gstreamer";
+ if (!gst_registry_scan_path(registry, path.c_str())) {
+ g_printerr("Failed to add plugin to registry\n");
+ gst_deinit();
+ return TestFail;
+ }
+
+ /* Create the elements */
+ libcameraSrc_ = gst_element_factory_make("libcamerasrc", "libcamera");
+ convert0_ = gst_element_factory_make("videoconvert", "convert0");
+ sink0_ = gst_element_factory_make("fakesink", "sink0");
+
+ /* Create the empty pipeline_ */
+ pipeline_ = gst_pipeline_new("test-pipeline");
+
+ if (!pipeline_ || !convert0_ || !sink0_ || !libcameraSrc_) {
+ g_printerr("Not all elements could be created. %p.%p.%p.%p\n",
+ pipeline_, convert0_, sink0_, libcameraSrc_);
+ if (pipeline_)
+ gst_object_unref(pipeline_);
+ if (convert0_)
+ gst_object_unref(convert0_);
+ if (sink0_)
+ gst_object_unref(sink0_);
+ if (libcameraSrc_)
+ gst_object_unref(libcameraSrc_);
+ gst_deinit();
+
+ return TestFail;
+ }
+
+ return TestPass;
+ }
+
+ void cleanup() override
+ {
+ gst_object_unref(pipeline_);
+ gst_deinit();
+ }
+
+ int run() override
+ {
+ GstStateChangeReturn ret;
+
+ /* Build the pipeline */
+ gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL);
+ if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) {
+ g_printerr("Elements could not be linked.\n");
+ return TestFail;
+ }
+
+ /* Start playing */
+ ret = gst_element_set_state(pipeline_, GST_STATE_PLAYING);
+ if (ret == GST_STATE_CHANGE_FAILURE) {
+ g_printerr("Unable to set the pipeline to the playing state.\n");
+ return TestFail;
+ }
+
+ /* Wait until error or EOS or timeout after 2 seconds */
+ constexpr GstMessageType msgType =
+ static_cast<GstMessageType>(GST_MESSAGE_ERROR | GST_MESSAGE_EOS);
+ constexpr GstClockTime timeout = 2000000000;
+
+ g_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);
+ g_autoptr(GstMessage) msg = gst_bus_timed_pop_filtered(bus, timeout, msgType);
+
+ gst_element_set_state(pipeline_, GST_STATE_NULL);
+
+ /* Parse error message */
+ if (msg == NULL)
+ return TestPass;
+
+ switch (GST_MESSAGE_TYPE(msg)) {
+ case GST_MESSAGE_ERROR:
+ gstreamer_print_error(msg);
+ break;
+ case GST_MESSAGE_EOS:
+ g_print("End-Of-Stream reached.\n");</pre>
</blockquote>
EOS doesn't sound like a error/Test-fail condition to me. If there
is a possibility that EOS will be reached before timeout, what's
ensures(in the first place) that the pipeline will be in playing
state for entire timeout, before we request masked EOS/Error
messages from the bus? What am I missing?<br></div></blockquote><div><br></div><div>I am not a gstreamer expert, to the best of my knowledge, EOS won't be ever sent by libcamerasrc, as it is the element which drives the pipeline. I think this event can be dropped, I just want to confirm it with Nicolas once he's back.<br></div><div><br></div><div>> It is important to note that <em>only elements driving the pipeline should
ever send an EOS event</em>. If your element is chain-based, it is not
driving the pipeline. Chain-based elements should just return
GST_FLOW_EOS from their chain function at the end of the stream (or
the configured segment)</div><div><br></div><div><a href="https://gstreamer.freedesktop.org/documentation/plugin-development/advanced/events.html?gi-language=c#end-of-stream-eos">https://gstreamer.freedesktop.org/documentation/plugin-development/advanced/events.html?gi-language=c#end-of-stream-eos</a></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"><div>
<blockquote type="cite">
<pre>+ break;
+ default:
+ g_printerr("Unexpected message received.\n");
+ break;
+ }
+
+ return TestFail;
+ }
+
+private:
+ void gstreamer_print_error(GstMessage *msg)
+ {
+ GError *err;</pre>
</blockquote>
<p>This can use:</p>
<p> g_autoptr(GError) err = NULL;<br>
</p>
<blockquote type="cite">
<pre>+ gchar *debug_info;</pre>
</blockquote>
<p>and</p>
<p> <span>g_autofree</span> <span>char</span>
*debug_info = <span>NULL</span>;</p>
<blockquote type="cite">
<pre>+
+ gst_message_parse_error(msg, &err, &debug_info);
+ g_printerr("Error received from element %s: %s\n",
+ GST_OBJECT_NAME(msg->src), err->message);
+ g_printerr("Debugging information: %s\n",
+ debug_info ? debug_info : "none");
+ g_clear_error(&err);
+ g_free(debug_info);</pre>
</blockquote>
<p>So, that will lead us to dropping manual free of err and debug
info</p>
<p><br>
</p>
<p>Minor things, so:</p>
<p>Reviewed-by: Umang Jain <a href="mailto:umang.jain@ideasonboard.com" target="_blank"><umang.jain@ideasonboard.com></a><br>
</p>
<blockquote type="cite">
<pre>+ }
+
+ GstElement *pipeline_;
+ GstElement *libcameraSrc_;
+ GstElement *convert0_;
+ GstElement *sink0_;
+};
+
+TEST_REGISTER(GstreamerSingleStreamTest)
diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build
new file mode 100644
index 000000000000..b99aa0da0ba3
--- /dev/null
+++ b/test/gstreamer/meson.build
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: CC0-1.0
+
+if not gst_enabled
+ subdir_done()
+endif
+
+gstreamer_tests = [
+ ['single_stream_test', 'gstreamer_single_stream_test.cpp'],
+]
+gstreamer_dep = dependency('gstreamer-1.0', required: true)
+
+foreach t : gstreamer_tests
+ exe = executable(t[0], t[1],
+ dependencies : [libcamera_private, gstreamer_dep],
+ link_with : test_libraries,
+ include_directories : test_includes_internal)
+
+ test(t[0], exe, suite : 'gstreamer', is_parallel : false)
+endforeach
diff --git a/test/meson.build b/test/meson.build
index 3bceb5df586f..d0466f17d7b6 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -11,6 +11,7 @@ subdir('libtest')
subdir('camera')
subdir('controls')
+subdir('gstreamer')
subdir('ipa')
subdir('ipc')
subdir('log')
</pre></blockquote></div></blockquote><div><br></div><div>Regards,</div><div><i><b>Vedant Paranjape</b></i><br></div></div></div>