[{"id":19157,"web_url":"https://patchwork.libcamera.org/comment/19157/","msgid":"","date":"2021-08-29T01:09:11","subject":"Re: [libcamera-devel] [PATCH v2] test: gstreamer: Add a test for\n\tgstreamer multi stream","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Vedant,\n\nThank you for the patch.\n\nOn Sat, Aug 28, 2021 at 11:07:53PM +0530, Vedant Paranjape wrote:\n> This patch adds a test to test if multi stream using libcamera's\n> gstreamer element works.\n> \n> We need to work around one issue with ASan when enabled in the\n> build:\n> \n> - glib has a known leak at initialization time. This is covered by the\n> suppression file shipped with glib, but it's not clear how to use it\n> automatically. For now, disable leak detection to avoid test failures.\n> \n> Signed-off-by: Vedant Paranjape \n> ---\n> .../gstreamer/gstreamer_multi_stream_test.cpp | 225 ++++++++++++++++++\n> test/gstreamer/meson.build | 1 +\n> 2 files changed, 226 insertions(+)\n> create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp\n> \n> diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> new file mode 100644\n> index 00000000..e32b07a4\n> --- /dev/null\n> +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> @@ -0,0 +1,225 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Vedant Paranjape\n> + *\n> + * gstreamer_single_stream_test.cpp - GStreamer single stream capture test\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/source_paths.h\"\n> +\n> +#include \n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +\n> +extern \"C\" {\n> +const char *__asan_default_options()\n> +{\n> +\t/*\n> +\t * Disable leak detection due to a known global variable initialization\n> +\t * leak in glib's g_quark_init(). This should ideally be handled by\n> +\t * using a suppression file instead of disabling leak detection.\n> +\t */\n> +\treturn \"detect_leaks=false\";\n> +}\n> +}\n> +\n> +class GstreamerSingleStreamTest : public Test\n> +{\n> +protected:\n> +\tint init() override\n> +\t{\n> +\t\t/*\n> +\t\t * GStreamer by default spawns a process to run the\n> +\t\t * gst-plugin-scanner helper. If libcamera is compiled with ASan\n> +\t\t * enabled, and as GStreamer is most likely not, this causes the\n> +\t\t * ASan link order check to fail when gst-plugin-scanner\n> +\t\t * dlopen()s the plugin as many libraries will have already been\n> +\t\t * loaded by then. Fix this issue by disabling spawning of a\n> +\t\t * child helper process when scanning the build directory for\n> +\t\t * plugins.\n> +\t\t */\n> +\t\tgst_registry_fork_set_enabled(false);\n> +\n> +\t\t/* Initialize GStreamer */\n> +\t\tg_autoptr(GError) errInit = NULL;\n> +\t\tif (!gst_init_check(nullptr, nullptr, &errInit)) {\n> +\t\t\tg_printerr(\"Could not initialize GStreamer: %s\\n\",\n> +\t\t\t\t errInit ? errInit->message : \"unknown error\");\n> +\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Remove the system libcamera plugin, if any, and add the\n> +\t\t * plugin from the build directory.\n> +\t\t */\n> +\t\tGstRegistry *registry = gst_registry_get();\n> +\t\tGstPlugin *plugin = gst_registry_lookup(registry, \"libgstlibcamera.so\");\n> +\t\tif (plugin) {\n> +\t\t\tgst_registry_remove_plugin(registry, plugin);\n> +\t\t\tgst_object_unref(plugin);\n> +\t\t}\n> +\n> +\t\tstd::string path = libcamera::utils::libcameraBuildPath()\n> +\t\t\t\t + \"src/gstreamer\";\n> +\t\tif (!gst_registry_scan_path(registry, path.c_str())) {\n> +\t\t\tg_printerr(\"Failed to add plugin to registry\\n\");\n> +\t\t\tgst_deinit();\n> +\t\t\treturn TestFail;\n> +\t\t}\n\nThere's quite a bit of duplicated code between this and the single\nstream test. Could you refactor the single stream test to create a base\nclass that can be shared with this one ?\n\n> +\n> +\t\t/* Check if platform support multistream output */\n> +\t\tlibcamera::CameraManager cm;\n> +\t\tcm.start();\n> +\t\tif (cm.cameras()[0]->streams().size() <= 1) {\n> +\t\t\tcm.stop();\n> +\t\t\treturn TestSkip;\n> +\t\t}\n> +\t\tcm.stop();\n\nIs there a guarantee that libcamerasrc will pick the same camera ? It\nseems quite fragile to me. Isn't there a way to instead query the\nlibcamerasrc element for the number of pads it supports ? More than\nthat, as not all cameras support multiple streams, it would be nice if\nthe test could iterate over all cameras to find one that does.\n\n> +\n> +\t\t/* Create the elements */\n> +\t\tlibcameraSrc_ = gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> +\t\tconvert0_ = gst_element_factory_make(\"videoconvert\", \"convert0\");\n> +\t\tconvert1_ = gst_element_factory_make(\"videoconvert\", \"convert1\");\n> +\t\tsink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> +\t\tsink1_ = gst_element_factory_make(\"fakesink\", \"sink1\");\n> +\t\tqueue0_ = gst_element_factory_make(\"queue\", \"camera_queue0\");\n> +\t\tqueue1_ = gst_element_factory_make(\"queue\", \"camera_queue1\");\n\nLooks like there's room to make the code more generic, posibly storing\nthe elements in containers.\n\n> +\n> +\t\t/* Create the empty pipeline_ */\n> +\t\tpipeline_ = gst_pipeline_new(\"test-pipeline\");\n> +\n> +\t\tif (!pipeline_ || !convert0_ || !convert1_ || !sink0_ ||\n> +\t\t\t!sink1_ || !queue0_ || !queue1_ || !libcameraSrc_) {\n> +\t\t\tg_printerr(\"Not all elements could be created. %p.%p.%p.%p.%p.%p.%p.%p\\n\",\n> +\t\t\t\t\t\tpipeline_, convert0_, convert1_, sink0_,\n> +\t\t\t\t\t\tsink1_, queue0_, queue1_, libcameraSrc_);\n> +\t\t\tif (pipeline_)\n> +\t\t\t\tgst_object_unref(pipeline_);\n> +\t\t\tif (convert0_)\n> +\t\t\t\tgst_object_unref(convert0_);\n> +\t\t\tif (convert1_)\n> +\t\t\t\tgst_object_unref(convert1_);\n> +\t\t\tif (sink0_)\n> +\t\t\t\tgst_object_unref(sink0_);\n> +\t\t\tif (sink1_)\n> +\t\t\t\tgst_object_unref(sink1_);\n> +\t\t\tif (queue0_)\n> +\t\t\t\tgst_object_unref(queue0_);\n> +\t\t\tif (queue1_)\n> +\t\t\t\tgst_object_unref(queue1_);\n> +\t\t\tif (libcameraSrc_)\n> +\t\t\t\tgst_object_unref(libcameraSrc_);\n> +\t\t\tgst_deinit();\n> +\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tvoid cleanup() override\n> +\t{\n> +\t\tgst_object_unref(pipeline_);\n> +\t\tgst_deinit();\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\tGstStateChangeReturn ret;\n> +\n> +\t\t/* Build the pipeline */\n> +\t\tgst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, queue0_, queue1_,\n> +\t\t\t\t\t\t\tconvert0_, convert1_, sink0_, sink1_, NULL);\n> +\t\tif (gst_element_link_many(queue0_, convert0_, sink0_, NULL) != TRUE\n> +\t\t\t\t|| gst_element_link_many(queue1_, convert1_, sink1_, NULL) != TRUE) {\n> +\t\t\tg_printerr(\"Elements could not be linked.\\n\");\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tg_autoptr(GstPad) src_pad = gst_element_get_static_pad(libcameraSrc_, \"src\");\n> +\t\tg_autoptr(GstPad) request_pad = gst_element_get_request_pad(libcameraSrc_, \"src_%u\");\n> +\t\tGstPad *queue0_sink_pad = gst_element_get_static_pad(queue0_, \"sink\");\n> +\t\tGstPad *queue1_sink_pad = gst_element_get_static_pad(queue1_, \"sink\");\n> +\n> +\t\tif (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK\n> +\t\t\t\t|| gst_pad_link(request_pad, queue1_sink_pad) != GST_PAD_LINK_OK) {\n> +\t\t\tif (queue0_sink_pad)\n> +\t\t\t\tgst_object_unref(queue0_sink_pad);\n> +\t\t\tif (queue1_sink_pad)\n> +\t\t\t\tgst_object_unref(queue1_sink_pad);\n> +\t\t\tg_printerr(\"Pads could not be linked.\\n\");\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\t\tgst_object_unref(queue0_sink_pad);\n> +\t\tgst_object_unref(queue1_sink_pad);\n> +\n> +\t\t/* Start playing */\n> +\t\tret = gst_element_set_state(pipeline_, GST_STATE_PLAYING);\n> +\t\tif (ret == GST_STATE_CHANGE_FAILURE) {\n> +\t\t\tg_printerr(\"Unable to set the pipeline to the playing state.\\n\");\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Wait until error or EOS or timeout after 2 seconds */\n> +\t\tconstexpr GstMessageType msgType =\n> +\t\t\tstatic_cast(GST_MESSAGE_ERROR | GST_MESSAGE_EOS);\n> +\t\tconstexpr GstClockTime timeout = 2 * GST_SECOND;\n> +\n> +\t\tg_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);\n> +\t\tg_autoptr(GstMessage) msg = gst_bus_timed_pop_filtered(bus, timeout, msgType);\n> +\n> +\t\tgst_element_set_state(pipeline_, GST_STATE_NULL);\n> +\n> +\t\t/* Parse error message */\n> +\t\tif (msg == NULL)\n> +\t\t\treturn TestPass;\n> +\n> +\t\tswitch (GST_MESSAGE_TYPE(msg)) {\n> +\t\tcase GST_MESSAGE_ERROR:\n> +\t\t\tgstreamer_print_error(msg);\n> +\t\t\tbreak;\n> +\t\tcase GST_MESSAGE_EOS:\n> +\t\t\tg_print(\"End-Of-Stream reached.\\n\");\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tg_printerr(\"Unexpected message received.\\n\");\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +private:\n> +\tvoid gstreamer_print_error(GstMessage *msg)\n> +\t{\n> +\t\tg_autoptr(GError) err = NULL;\n> +\t\tg_autofree gchar *debug_info = NULL;\n> +\n> +\t\tgst_message_parse_error(msg, &err, &debug_info);\n> +\t\tg_printerr(\"Error received from element %s: %s\\n\",\n> +\t\t\t\tGST_OBJECT_NAME(msg->src), err->message);\n> +\t\tg_printerr(\"Debugging information: %s\\n\",\n> +\t\t\t\tdebug_info ? debug_info : \"none\");\n> +\t}\n> +\n> +\tGstElement *pipeline_;\n> +\tGstElement *libcameraSrc_;\n> +\tGstElement *queue0_;\n> +\tGstElement *queue1_;\n> +\tGstElement *convert0_;\n> +\tGstElement *convert1_;\n> +\tGstElement *sink0_;\n> +\tGstElement *sink1_;\n> +};\n> +\n> +TEST_REGISTER(GstreamerSingleStreamTest)\n> diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> index b99aa0da..e28225b2 100644\n> --- a/test/gstreamer/meson.build\n> +++ b/test/gstreamer/meson.build\n> @@ -6,6 +6,7 @@ endif\n> \n> gstreamer_tests = [\n> ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> + ['multi_stream_test', 'gstreamer_multi_stream_test.cpp'],\n> ]\n> gstreamer_dep = dependency('gstreamer-1.0', required: true)\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 18514BDC71\n\tfor ;\n\tSun, 29 Aug 2021 01:09:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4EEA86916A;\n\tSun, 29 Aug 2021 03:09:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2960F60256\n\tfor ;\n\tSun, 29 Aug 2021 03:09:26 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7B2863D7;\n\tSun, 29 Aug 2021 03:09:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gj9VH0t4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630199365;\n\tbh=8Rz4SmjInYdgtOQzfsyaKiOsb9CjduatNM9/lQ2Gzb0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gj9VH0t4wYb9JSpyITh4OwrdoHR9giuYPkH3RkL4SI3NwKQHnj8bghCCGIdyD+Z5V\n\tUvlqLnFT5H5nJwYMOaoJoA8Nuu/i//THGY3Asg3HpqculVst3L6UfLKqvXowq8iIPy\n\tuBJRGIsdOs+IJW1ypc1kO2KNDFjMTPrtgepCS1K0=","Date":"Sun, 29 Aug 2021 04:09:11 +0300","From":"Laurent Pinchart ","To":"Vedant Paranjape ","Message-ID":"","References":"<20210828173753.336859-1-vedantparanjape160201@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210828173753.336859-1-vedantparanjape160201@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2] test: gstreamer: Add a test for\n\tgstreamer multi stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19158,"web_url":"https://patchwork.libcamera.org/comment/19158/","msgid":"","date":"2021-08-29T05:23:34","subject":"Re: [libcamera-devel] [PATCH v2] test: gstreamer: Add a test for\n\tgstreamer multi stream","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Laurent,\nThanks for the review.\nOn Sun, Aug 29, 2021 at 6:39 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Vedant,\n>\n> Thank you for the patch.\n>\n> On Sat, Aug 28, 2021 at 11:07:53PM +0530, Vedant Paranjape wrote:\n> > This patch adds a test to test if multi stream using libcamera's\n> > gstreamer element works.\n> >\n> > We need to work around one issue with ASan when enabled in the\n> > build:\n> >\n> > - glib has a known leak at initialization time. This is covered by the\n> > suppression file shipped with glib, but it's not clear how to use it\n> > automatically. For now, disable leak detection to avoid test failures.\n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > .../gstreamer/gstreamer_multi_stream_test.cpp | 225 ++++++++++++++++++\n> > test/gstreamer/meson.build | 1 +\n> > 2 files changed, 226 insertions(+)\n> > create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp\n> >\n> > diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp\n> b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> > new file mode 100644\n> > index 00000000..e32b07a4\n> > --- /dev/null\n> > +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> > @@ -0,0 +1,225 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Vedant Paranjape\n> > + *\n> > + * gstreamer_single_stream_test.cpp - GStreamer single stream capture\n> test\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/source_paths.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> > +\n> > +extern \"C\" {\n> > +const char *__asan_default_options()\n> > +{\n> > + /*\n> > + * Disable leak detection due to a known global variable\n> initialization\n> > + * leak in glib's g_quark_init(). This should ideally be handled by\n> > + * using a suppression file instead of disabling leak detection.\n> > + */\n> > + return \"detect_leaks=false\";\n> > +}\n> > +}\n> > +\n> > +class GstreamerSingleStreamTest : public Test\n> > +{\n> > +protected:\n> > + int init() override\n> > + {\n> > + /*\n> > + * GStreamer by default spawns a process to run the\n> > + * gst-plugin-scanner helper. If libcamera is compiled\n> with ASan\n> > + * enabled, and as GStreamer is most likely not, this\n> causes the\n> > + * ASan link order check to fail when gst-plugin-scanner\n> > + * dlopen()s the plugin as many libraries will have\n> already been\n> > + * loaded by then. Fix this issue by disabling spawning of\n> a\n> > + * child helper process when scanning the build directory\n> for\n> > + * plugins.\n> > + */\n> > + gst_registry_fork_set_enabled(false);\n> > +\n> > + /* Initialize GStreamer */\n> > + g_autoptr(GError) errInit = NULL;\n> > + if (!gst_init_check(nullptr, nullptr, &errInit)) {\n> > + g_printerr(\"Could not initialize GStreamer: %s\\n\",\n> > + errInit ? errInit->message : \"unknown\n> error\");\n> > +\n> > + return TestFail;\n> > + }\n> > +\n> > + /*\n> > + * Remove the system libcamera plugin, if any, and add the\n> > + * plugin from the build directory.\n> > + */\n> > + GstRegistry *registry = gst_registry_get();\n> > + GstPlugin *plugin = gst_registry_lookup(registry,\n> \"libgstlibcamera.so\");\n> > + if (plugin) {\n> > + gst_registry_remove_plugin(registry, plugin);\n> > + gst_object_unref(plugin);\n> > + }\n> > +\n> > + std::string path = libcamera::utils::libcameraBuildPath()\n> > + + \"src/gstreamer\";\n> > + if (!gst_registry_scan_path(registry, path.c_str())) {\n> > + g_printerr(\"Failed to add plugin to registry\\n\");\n> > + gst_deinit();\n> > + return TestFail;\n> > + }\n>\n> There's quite a bit of duplicated code between this and the single\n> stream test. Could you refactor the single stream test to create a base\n> class that can be shared with this one ?\n>\n\nYes, I am going to do this, but can we go about it as follows, merge this\npatch with duplicated code,\nthen I think over it a bit and then submit new patch which refactors a lot\nof the duplicated code,\nalso I don't think going ahead with making a base class is a good idea as,\nthere's much functional difference between two codes,\nthey rather have isolated code that repeats, but no so much as to roll it\ninto a class.\n\n> +\n> > + /* Check if platform support multistream output */\n> > + libcamera::CameraManager cm;\n> > + cm.start();\n> > + if (cm.cameras()[0]->streams().size() <= 1) {\n> > + cm.stop();\n> > + return TestSkip;\n> > + }\n> > + cm.stop();\n>\n> Is there a guarantee that libcamerasrc will pick the same camera ? It\n>\n\nYes, it is guaranteed if we don't pass it a camera-name which we aren't see\nhere:\nhttps://git.linuxtv.org/libcamera.git/tree/src/gstreamer/gstlibcamerasrc.cpp#n237\n\n\n> seems quite fragile to me. Isn't there a way to instead query the\n> libcamerasrc element for the number of pads it supports ?\n\n\nI think this won't be possible unless we add code to the\ngstreamer-element, @Nicolas\nDufresne can\nyou answer this please, I am not sure :)\n\nMore than\n> that, as not all cameras support multiple streams, it would be nice if\n> the test could iterate over all cameras to find one that does.\n>\n\nThis sounds like a nice idea, I could store the camera-name of the specific\ncamera and set the camera-name property\nof the libcamera element and start on it, but now I don't a multicamera\nsetup.\n\n> +\n> > + /* Create the elements */\n> > + libcameraSrc_ = gst_element_factory_make(\"libcamerasrc\",\n> \"libcamera\");\n> > + convert0_ = gst_element_factory_make(\"videoconvert\",\n> \"convert0\");\n> > + convert1_ = gst_element_factory_make(\"videoconvert\",\n> \"convert1\");\n> > + sink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > + sink1_ = gst_element_factory_make(\"fakesink\", \"sink1\");\n> > + queue0_ = gst_element_factory_make(\"queue\",\n> \"camera_queue0\");\n> > + queue1_ = gst_element_factory_make(\"queue\",\n> \"camera_queue1\");\n>\n> Looks like there's room to make the code more generic, posibly storing\n> the elements in containers.\n>\n\nOkay something like a vector to hold the elements and then a map to hold\n.\nBut this is a test code, is this fancy stuff really needed ? !!\n\n> +\n> > + /* Create the empty pipeline_ */\n> > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > +\n> > + if (!pipeline_ || !convert0_ || !convert1_ || !sink0_ ||\n> > + !sink1_ || !queue0_ || !queue1_ || !libcameraSrc_)\n> {\n> > + g_printerr(\"Not all elements could be created.\n> %p.%p.%p.%p.%p.%p.%p.%p\\n\",\n> > + pipeline_, convert0_,\n> convert1_, sink0_,\n> > + sink1_, queue0_, queue1_,\n> libcameraSrc_);\n> > + if (pipeline_)\n> > + gst_object_unref(pipeline_);\n> > + if (convert0_)\n> > + gst_object_unref(convert0_);\n> > + if (convert1_)\n> > + gst_object_unref(convert1_);\n> > + if (sink0_)\n> > + gst_object_unref(sink0_);\n> > + if (sink1_)\n> > + gst_object_unref(sink1_);\n> > + if (queue0_)\n> > + gst_object_unref(queue0_);\n> > + if (queue1_)\n> > + gst_object_unref(queue1_);\n> > + if (libcameraSrc_)\n> > + gst_object_unref(libcameraSrc_);\n> > + gst_deinit();\n> > +\n> > + return TestFail;\n> > + }\n> > +\n> > + return TestPass;\n> > + }\n> > +\n> > + void cleanup() override\n> > + {\n> > + gst_object_unref(pipeline_);\n> > + gst_deinit();\n> > + }\n> > +\n> > + int run() override\n> > + {\n> > + GstStateChangeReturn ret;\n> > +\n> > + /* Build the pipeline */\n> > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n> queue0_, queue1_,\n> > + convert0_,\n> convert1_, sink0_, sink1_, NULL);\n> > + if (gst_element_link_many(queue0_, convert0_, sink0_,\n> NULL) != TRUE\n> > + || gst_element_link_many(queue1_,\n> convert1_, sink1_, NULL) != TRUE) {\n> > + g_printerr(\"Elements could not be linked.\\n\");\n> > + return TestFail;\n> > + }\n> > +\n> > + g_autoptr(GstPad) src_pad =\n> gst_element_get_static_pad(libcameraSrc_, \"src\");\n> > + g_autoptr(GstPad) request_pad =\n> gst_element_get_request_pad(libcameraSrc_, \"src_%u\");\n> > + GstPad *queue0_sink_pad =\n> gst_element_get_static_pad(queue0_, \"sink\");\n> > + GstPad *queue1_sink_pad =\n> gst_element_get_static_pad(queue1_, \"sink\");\n> > +\n> > + if (gst_pad_link(src_pad, queue0_sink_pad) !=\n> GST_PAD_LINK_OK\n> > + || gst_pad_link(request_pad,\n> queue1_sink_pad) != GST_PAD_LINK_OK) {\n> > + if (queue0_sink_pad)\n> > + gst_object_unref(queue0_sink_pad);\n> > + if (queue1_sink_pad)\n> > + gst_object_unref(queue1_sink_pad);\n> > + g_printerr(\"Pads could not be linked.\\n\");\n> > + return TestFail;\n> > + }\n> > + gst_object_unref(queue0_sink_pad);\n> > + gst_object_unref(queue1_sink_pad);\n> > +\n> > + /* Start playing */\n> > + ret = gst_element_set_state(pipeline_, GST_STATE_PLAYING);\n> > + if (ret == GST_STATE_CHANGE_FAILURE) {\n> > + g_printerr(\"Unable to set the pipeline to the\n> playing state.\\n\");\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Wait until error or EOS or timeout after 2 seconds */\n> > + constexpr GstMessageType msgType =\n> > + static_cast(GST_MESSAGE_ERROR |\n> GST_MESSAGE_EOS);\n> > + constexpr GstClockTime timeout = 2 * GST_SECOND;\n> > +\n> > + g_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);\n> > + g_autoptr(GstMessage) msg =\n> gst_bus_timed_pop_filtered(bus, timeout, msgType);\n> > +\n> > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > +\n> > + /* Parse error message */\n> > + if (msg == NULL)\n> > + return TestPass;\n> > +\n> > + switch (GST_MESSAGE_TYPE(msg)) {\n> > + case GST_MESSAGE_ERROR:\n> > + gstreamer_print_error(msg);\n> > + break;\n> > + case GST_MESSAGE_EOS:\n> > + g_print(\"End-Of-Stream reached.\\n\");\n> > + break;\n> > + default:\n> > + g_printerr(\"Unexpected message received.\\n\");\n> > + break;\n> > + }\n> > +\n> > + return TestFail;\n> > + }\n> > +\n> > +private:\n> > + void gstreamer_print_error(GstMessage *msg)\n> > + {\n> > + g_autoptr(GError) err = NULL;\n> > + g_autofree gchar *debug_info = NULL;\n> > +\n> > + gst_message_parse_error(msg, &err, &debug_info);\n> > + g_printerr(\"Error received from element %s: %s\\n\",\n> > + GST_OBJECT_NAME(msg->src), err->message);\n> > + g_printerr(\"Debugging information: %s\\n\",\n> > + debug_info ? debug_info : \"none\");\n> > + }\n> > +\n> > + GstElement *pipeline_;\n> > + GstElement *libcameraSrc_;\n> > + GstElement *queue0_;\n> > + GstElement *queue1_;\n> > + GstElement *convert0_;\n> > + GstElement *convert1_;\n> > + GstElement *sink0_;\n> > + GstElement *sink1_;\n> > +};\n> > +\n> > +TEST_REGISTER(GstreamerSingleStreamTest)\n> > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > index b99aa0da..e28225b2 100644\n> > --- a/test/gstreamer/meson.build\n> > +++ b/test/gstreamer/meson.build\n> > @@ -6,6 +6,7 @@ endif\n> >\n> > gstreamer_tests = [\n> > ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> > + ['multi_stream_test', 'gstreamer_multi_stream_test.cpp'],\n> > ]\n> > gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n\nRegards\n*Vedant Paranjape*","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2C614BDC71\n\tfor ;\n\tSun, 29 Aug 2021 05:23:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 70A1469167;\n\tSun, 29 Aug 2021 07:23:49 +0200 (CEST)","from mail-yb1-xb35.google.com (mail-yb1-xb35.google.com\n\t[IPv6:2607:f8b0:4864:20::b35])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 141B360254\n\tfor ;\n\tSun, 29 Aug 2021 07:23:48 +0200 (CEST)","by mail-yb1-xb35.google.com with SMTP id k78so17866407ybf.10\n\tfor ;\n\tSat, 28 Aug 2021 22:23:47 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"GENDA4SI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=kRopN9J0jtxCe/My3/4mfbeKGIvbP3E46DRpckC0b/o=;\n\tb=GENDA4SIPt2lL4vhOKGuuuniGrqiiHzp9w/nBq6pZyKrERpZg01va2niim3zhatn+0\n\tjg5RpoXJt7SBgJkrYDFZBhgzwz9QTMwMRaXbAO1syb/3HZlJVhxgH8Cz4t1E2HC7EKre\n\toJqRqtkU/dbVFseLF6fQ6miTQNbubNpHbOWNirxnQfv0jLk1mLORWCePjZspvyp7YhPn\n\tE8LtUyrZtv35VVQ8SkLcC/owfuUNGwc0yIgFjRzkvL2eZm4vSZknbbNrz6fRvmTv+xDO\n\tKA6dNevz7MiCsS3rB+TeRgMuPRhF8EA1pywbfdGfxWopK7Tna64GShyAej6gB8GjemVb\n\trk9Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=kRopN9J0jtxCe/My3/4mfbeKGIvbP3E46DRpckC0b/o=;\n\tb=Ona0CWIDmQI5ifJcI0T+63aPa1NLyR2j+EF5BQl5ChDFrltynoKPh959amMWdjfVCl\n\tj8u9XxFfm6QuYpgZb/pVhBW1uvFa2RjBeLQj689sQD90z4NcO5PF8qM2YwG8ApUqwBIM\n\tx9O/eS0nDYZTtUPr1NGr4j6OYqhXGSdy7H0A7wMG/sASXst5HSf7t8z2Q0k8BuW+oGLq\n\tBpn/eju/SwrfmpsA4On9yVwE0eGY/coVH+KOtuTyEVafyVTvDfPBZ0ycwgxSUpE6Nqpy\n\tkJAtG/IxU2+OUU+GITLeibD3rpFpkGrsxCiCshRmC73zUbIuAr8Fy5h0qdnwc9az3eWe\n\tLohg==","X-Gm-Message-State":"AOAM530EUhbkE2TtiUVCuyAE3pTB0mUFoxiPkKHOVUCnDlLloFn+y9/c\n\tBKqk1d5UEYybt5GxLOg8qfupMuw8L79wE28t2+s=","X-Google-Smtp-Source":"ABdhPJwcj2j1Aw3lcmdFZiIhCUznBGGWYaiQBzTZJDfRku4wLVTnG6J18IfU3y55uODIeGNjPP+NiVxNYdVfvG5yHkU=","X-Received":"by 2002:a25:b787:: with SMTP id\n\tn7mr15585884ybh.468.1630214626615; \n\tSat, 28 Aug 2021 22:23:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20210828173753.336859-1-vedantparanjape160201@gmail.com>\n\t","In-Reply-To":"","From":"Vedant Paranjape ","Date":"Sun, 29 Aug 2021 10:53:34 +0530","Message-ID":"","To":"Laurent Pinchart , \n\tNicolas Dufresne ","Content-Type":"multipart/alternative; boundary=\"000000000000ba25d305caabeb17\"","Subject":"Re: [libcamera-devel] [PATCH v2] test: gstreamer: Add a test for\n\tgstreamer multi stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19159,"web_url":"https://patchwork.libcamera.org/comment/19159/","msgid":"","date":"2021-08-29T09:25:51","subject":"Re: [libcamera-devel] [PATCH v2] test: gstreamer: Add a test for\n\tgstreamer multi stream","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Vedant,\n\nOn Sun, Aug 29, 2021 at 10:53:34AM +0530, Vedant Paranjape wrote:\n> On Sun, Aug 29, 2021 at 6:39 AM Laurent Pinchart wrote:\n> > On Sat, Aug 28, 2021 at 11:07:53PM +0530, Vedant Paranjape wrote:\n> > > This patch adds a test to test if multi stream using libcamera's\n> > > gstreamer element works.\n> > >\n> > > We need to work around one issue with ASan when enabled in the\n> > > build:\n> > >\n> > > - glib has a known leak at initialization time. This is covered by the\n> > > suppression file shipped with glib, but it's not clear how to use it\n> > > automatically. For now, disable leak detection to avoid test failures.\n> > >\n> > > Signed-off-by: Vedant Paranjape \n> > > ---\n> > > .../gstreamer/gstreamer_multi_stream_test.cpp | 225 ++++++++++++++++++\n> > > test/gstreamer/meson.build | 1 +\n> > > 2 files changed, 226 insertions(+)\n> > > create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp\n> > >\n> > > diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> > > new file mode 100644\n> > > index 00000000..e32b07a4\n> > > --- /dev/null\n> > > +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> > > @@ -0,0 +1,225 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Vedant Paranjape\n> > > + *\n> > > + * gstreamer_single_stream_test.cpp - GStreamer single stream capture test\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/source_paths.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"test.h\"\n> > > +\n> > > +using namespace std;\n> > > +\n> > > +extern \"C\" {\n> > > +const char *__asan_default_options()\n> > > +{\n> > > + /*\n> > > + * Disable leak detection due to a known global variable initialization\n> > > + * leak in glib's g_quark_init(). This should ideally be handled by\n> > > + * using a suppression file instead of disabling leak detection.\n> > > + */\n> > > + return \"detect_leaks=false\";\n> > > +}\n> > > +}\n> > > +\n> > > +class GstreamerSingleStreamTest : public Test\n> > > +{\n> > > +protected:\n> > > + int init() override\n> > > + {\n> > > + /*\n> > > + * GStreamer by default spawns a process to run the\n> > > + * gst-plugin-scanner helper. If libcamera is compiled with ASan\n> > > + * enabled, and as GStreamer is most likely not, this causes the\n> > > + * ASan link order check to fail when gst-plugin-scanner\n> > > + * dlopen()s the plugin as many libraries will have already been\n> > > + * loaded by then. Fix this issue by disabling spawning of a\n> > > + * child helper process when scanning the build directory for\n> > > + * plugins.\n> > > + */\n> > > + gst_registry_fork_set_enabled(false);\n> > > +\n> > > + /* Initialize GStreamer */\n> > > + g_autoptr(GError) errInit = NULL;\n> > > + if (!gst_init_check(nullptr, nullptr, &errInit)) {\n> > > + g_printerr(\"Could not initialize GStreamer: %s\\n\",\n> > > + errInit ? errInit->message : \"unknown error\");\n> > > +\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Remove the system libcamera plugin, if any, and add the\n> > > + * plugin from the build directory.\n> > > + */\n> > > + GstRegistry *registry = gst_registry_get();\n> > > + GstPlugin *plugin = gst_registry_lookup(registry, \"libgstlibcamera.so\");\n> > > + if (plugin) {\n> > > + gst_registry_remove_plugin(registry, plugin);\n> > > + gst_object_unref(plugin);\n> > > + }\n> > > +\n> > > + std::string path = libcamera::utils::libcameraBuildPath()\n> > > + + \"src/gstreamer\";\n> > > + if (!gst_registry_scan_path(registry, path.c_str())) {\n> > > + g_printerr(\"Failed to add plugin to registry\\n\");\n> > > + gst_deinit();\n> > > + return TestFail;\n> > > + }\n> >\n> > There's quite a bit of duplicated code between this and the single\n> > stream test. Could you refactor the single stream test to create a base\n> > class that can be shared with this one ?\n> \n> Yes, I am going to do this, but can we go about it as follows, merge this\n> patch with duplicated code,\n> then I think over it a bit and then submit new patch which refactors a lot\n> of the duplicated code,\n\nWhy ? :-) I'm fine merging work in progress when there's a reason (for\ninstance to split very large partch series in chunks, or to avoid\nblocking development when other developers depend on that code), but in\nthis case I think a refactoring before merging the second test is\nbetter.\n\n> also I don't think going ahead with making a base class is a good idea as,\n> there's much functional difference between two codes,\n> they rather have isolated code that repeats, but no so much as to roll it\n> into a class.\n\nI don't agree, I think the tests are very similar. diffstat agrees with\nme.\n\n$ diff -u gstreamer_single_stream_test.cpp gstreamer_multi_stream_test.cpp | diffstat\n gstreamer_multi_stream_test.cpp | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 55 insertions(+), 7 deletions(-)\n\n> > +\n> > > + /* Check if platform support multistream output */\n> > > + libcamera::CameraManager cm;\n> > > + cm.start();\n> > > + if (cm.cameras()[0]->streams().size() <= 1) {\n> > > + cm.stop();\n> > > + return TestSkip;\n> > > + }\n> > > + cm.stop();\n> >\n> > Is there a guarantee that libcamerasrc will pick the same camera ? It\n> \n> Yes, it is guaranteed if we don't pass it a camera-name which we aren't see\n> here:\n> https://git.linuxtv.org/libcamera.git/tree/src/gstreamer/gstlibcamerasrc.cpp#n237\n> \n> > seems quite fragile to me. Isn't there a way to instead query the\n> > libcamerasrc element for the number of pads it supports ?\n> \n> I think this won't be possible unless we add code to the\n> gstreamer-element, @Nicolas\n> Dufresne can\n> you answer this please, I am not sure :)\n\nThe GstElement documentation mentions a numsrcpads field. I have little\nexperience with GStreamer, but it sounds like something that could help.\nHave you tried it ?\n\n> > More than\n> > that, as not all cameras support multiple streams, it would be nice if\n> > the test could iterate over all cameras to find one that does.\n> \n> This sounds like a nice idea, I could store the camera-name of the specific\n> camera and set the camera-name property\n> of the libcamera element and start on it, but now I don't a multicamera\n> setup.\n> \n> > +\n> > > + /* Create the elements */\n> > > + libcameraSrc_ = gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > > + convert0_ = gst_element_factory_make(\"videoconvert\", \"convert0\");\n> > > + convert1_ = gst_element_factory_make(\"videoconvert\", \"convert1\");\n> > > + sink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > + sink1_ = gst_element_factory_make(\"fakesink\", \"sink1\");\n> > > + queue0_ = gst_element_factory_make(\"queue\", \"camera_queue0\");\n> > > + queue1_ = gst_element_factory_make(\"queue\", \"camera_queue1\");\n> >\n> > Looks like there's room to make the code more generic, posibly storing\n> > the elements in containers.\n> \n> Okay something like a vector to hold the elements and then a map to hold\n> .\n> But this is a test code, is this fancy stuff really needed ? !!\n\nTest code doesn't mean that it has to be dirty and throw away all\nsoftware development good practices. Tests are first class citizens.\nIt's not about \"fancy stuff\", it's about making the code more readable\nand maintainable.\n\n> > > +\n> > > + /* Create the empty pipeline_ */\n> > > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > +\n> > > + if (!pipeline_ || !convert0_ || !convert1_ || !sink0_ ||\n> > > + !sink1_ || !queue0_ || !queue1_ || !libcameraSrc_) {\n> > > + g_printerr(\"Not all elements could be created.> %p.%p.%p.%p.%p.%p.%p.%p\\n\",\n> > > + pipeline_, convert0_, convert1_, sink0_,\n> > > + sink1_, queue0_, queue1_, libcameraSrc_);\n> > > + if (pipeline_)\n> > > + gst_object_unref(pipeline_);\n> > > + if (convert0_)\n> > > + gst_object_unref(convert0_);\n> > > + if (convert1_)\n> > > + gst_object_unref(convert1_);\n> > > + if (sink0_)\n> > > + gst_object_unref(sink0_);\n> > > + if (sink1_)\n> > > + gst_object_unref(sink1_);\n> > > + if (queue0_)\n> > > + gst_object_unref(queue0_);\n> > > + if (queue1_)\n> > > + gst_object_unref(queue1_);\n> > > + if (libcameraSrc_)\n> > > + gst_object_unref(libcameraSrc_);\n> > > + gst_deinit();\n> > > +\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + return TestPass;\n> > > + }\n> > > +\n> > > + void cleanup() override\n> > > + {\n> > > + gst_object_unref(pipeline_);\n> > > + gst_deinit();\n> > > + }\n> > > +\n> > > + int run() override\n> > > + {\n> > > + GstStateChangeReturn ret;\n> > > +\n> > > + /* Build the pipeline */\n> > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, queue0_, queue1_,\n> > > + convert0_, convert1_, sink0_, sink1_, NULL);\n> > > + if (gst_element_link_many(queue0_, convert0_, sink0_, NULL) != TRUE\n> > > + || gst_element_link_many(queue1_, convert1_, sink1_, NULL) != TRUE) {\n> > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + g_autoptr(GstPad) src_pad = gst_element_get_static_pad(libcameraSrc_, \"src\");\n> > > + g_autoptr(GstPad) request_pad = gst_element_get_request_pad(libcameraSrc_, \"src_%u\");\n> > > + GstPad *queue0_sink_pad = gst_element_get_static_pad(queue0_, \"sink\");\n> > > + GstPad *queue1_sink_pad = gst_element_get_static_pad(queue1_, \"sink\");\n> > > +\n> > > + if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK\n> > > + || gst_pad_link(request_pad, queue1_sink_pad) != GST_PAD_LINK_OK) {\n> > > + if (queue0_sink_pad)\n> > > + gst_object_unref(queue0_sink_pad);\n> > > + if (queue1_sink_pad)\n> > > + gst_object_unref(queue1_sink_pad);\n> > > + g_printerr(\"Pads could not be linked.\\n\");\n> > > + return TestFail;\n> > > + }\n> > > + gst_object_unref(queue0_sink_pad);\n> > > + gst_object_unref(queue1_sink_pad);\n> > > +\n> > > + /* Start playing */\n> > > + ret = gst_element_set_state(pipeline_, GST_STATE_PLAYING);\n> > > + if (ret == GST_STATE_CHANGE_FAILURE) {\n> > > + g_printerr(\"Unable to set the pipeline to the playing state.\\n\");\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Wait until error or EOS or timeout after 2 seconds */\n> > > + constexpr GstMessageType msgType =\n> > > + static_cast(GST_MESSAGE_ERROR | GST_MESSAGE_EOS);\n> > > + constexpr GstClockTime timeout = 2 * GST_SECOND;\n> > > +\n> > > + g_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);\n> > > + g_autoptr(GstMessage) msg = gst_bus_timed_pop_filtered(bus, timeout, msgType);\n> > > +\n> > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > > +\n> > > + /* Parse error message */\n> > > + if (msg == NULL)\n> > > + return TestPass;\n> > > +\n> > > + switch (GST_MESSAGE_TYPE(msg)) {\n> > > + case GST_MESSAGE_ERROR:\n> > > + gstreamer_print_error(msg);\n> > > + break;\n> > > + case GST_MESSAGE_EOS:\n> > > + g_print(\"End-Of-Stream reached.\\n\");\n> > > + break;\n> > > + default:\n> > > + g_printerr(\"Unexpected message received.\\n\");\n> > > + break;\n> > > + }\n> > > +\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > +private:\n> > > + void gstreamer_print_error(GstMessage *msg)\n> > > + {\n> > > + g_autoptr(GError) err = NULL;\n> > > + g_autofree gchar *debug_info = NULL;\n> > > +\n> > > + gst_message_parse_error(msg, &err, &debug_info);\n> > > + g_printerr(\"Error received from element %s: %s\\n\",\n> > > + GST_OBJECT_NAME(msg->src), err->message);\n> > > + g_printerr(\"Debugging information: %s\\n\",\n> > > + debug_info ? debug_info : \"none\");\n> > > + }\n> > > +\n> > > + GstElement *pipeline_;\n> > > + GstElement *libcameraSrc_;\n> > > + GstElement *queue0_;\n> > > + GstElement *queue1_;\n> > > + GstElement *convert0_;\n> > > + GstElement *convert1_;\n> > > + GstElement *sink0_;\n> > > + GstElement *sink1_;\n> > > +};\n> > > +\n> > > +TEST_REGISTER(GstreamerSingleStreamTest)\n> > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > index b99aa0da..e28225b2 100644\n> > > --- a/test/gstreamer/meson.build\n> > > +++ b/test/gstreamer/meson.build\n> > > @@ -6,6 +6,7 @@ endif\n> > >\n> > > gstreamer_tests = [\n> > > ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> > > + ['multi_stream_test', 'gstreamer_multi_stream_test.cpp'],\n> > > ]\n> > > gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A8AF2BD87D\n\tfor ;\n\tSun, 29 Aug 2021 09:26:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BCF469167;\n\tSun, 29 Aug 2021 11:26:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4461468890\n\tfor ;\n\tSun, 29 Aug 2021 11:26:06 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A15B53D7;\n\tSun, 29 Aug 2021 11:26:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AquF4f87\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630229165;\n\tbh=IKF2zutDi5CeThhx4X5jG3cmytpgn67l7VXhhkcLsCQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AquF4f876y5iVV728eq9rUvxnrfdmFQzCAa4VXC7U4MwP3eHqP+zfV6vihtB+3k+9\n\tQR05DCtL0kn9I1ah0H/Jg184d9wHNiDBlfOYQhV84q6OPxfQfwO4xRhahXodGuk1le\n\tq5Ag4bPMQvBfuZ2u3TKqBq8SGp0GZ3zSAgqFQ2ws=","Date":"Sun, 29 Aug 2021 12:25:51 +0300","From":"Laurent Pinchart ","To":"Vedant Paranjape ","Message-ID":"","References":"<20210828173753.336859-1-vedantparanjape160201@gmail.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] test: gstreamer: Add a test for\n\tgstreamer multi stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19160,"web_url":"https://patchwork.libcamera.org/comment/19160/","msgid":"","date":"2021-08-29T10:04:46","subject":"Re: [libcamera-devel] [PATCH v2] test: gstreamer: Add a test for\n\tgstreamer multi stream","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hello Laurent,\n\nOn Sun, Aug 29, 2021 at 2:56 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Vedant,\n>\n> On Sun, Aug 29, 2021 at 10:53:34AM +0530, Vedant Paranjape wrote:\n> > On Sun, Aug 29, 2021 at 6:39 AM Laurent Pinchart wrote:\n> > > On Sat, Aug 28, 2021 at 11:07:53PM +0530, Vedant Paranjape wrote:\n> > > > This patch adds a test to test if multi stream using libcamera's\n> > > > gstreamer element works.\n> > > >\n> > > > We need to work around one issue with ASan when enabled in the\n> > > > build:\n> > > >\n> > > > - glib has a known leak at initialization time. This is covered by\n> the\n> > > > suppression file shipped with glib, but it's not clear how to use\n> it\n> > > > automatically. For now, disable leak detection to avoid test\n> failures.\n> > > >\n> > > > Signed-off-by: Vedant Paranjape \n> > > > ---\n> > > > .../gstreamer/gstreamer_multi_stream_test.cpp | 225\n> ++++++++++++++++++\n> > > > test/gstreamer/meson.build | 1 +\n> > > > 2 files changed, 226 insertions(+)\n> > > > create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp\n> > > >\n> > > > diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp\n> b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> > > > new file mode 100644\n> > > > index 00000000..e32b07a4\n> > > > --- /dev/null\n> > > > +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> > > > @@ -0,0 +1,225 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > + *\n> > > > + * gstreamer_single_stream_test.cpp - GStreamer single stream\n> capture test\n> > > > + */\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/source_paths.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"test.h\"\n> > > > +\n> > > > +using namespace std;\n> > > > +\n> > > > +extern \"C\" {\n> > > > +const char *__asan_default_options()\n> > > > +{\n> > > > + /*\n> > > > + * Disable leak detection due to a known global variable\n> initialization\n> > > > + * leak in glib's g_quark_init(). This should ideally be\n> handled by\n> > > > + * using a suppression file instead of disabling leak\n> detection.\n> > > > + */\n> > > > + return \"detect_leaks=false\";\n> > > > +}\n> > > > +}\n> > > > +\n> > > > +class GstreamerSingleStreamTest : public Test\n> > > > +{\n> > > > +protected:\n> > > > + int init() override\n> > > > + {\n> > > > + /*\n> > > > + * GStreamer by default spawns a process to run the\n> > > > + * gst-plugin-scanner helper. If libcamera is compiled\n> with ASan\n> > > > + * enabled, and as GStreamer is most likely not, this\n> causes the\n> > > > + * ASan link order check to fail when\n> gst-plugin-scanner\n> > > > + * dlopen()s the plugin as many libraries will have\n> already been\n> > > > + * loaded by then. Fix this issue by disabling\n> spawning of a\n> > > > + * child helper process when scanning the build\n> directory for\n> > > > + * plugins.\n> > > > + */\n> > > > + gst_registry_fork_set_enabled(false);\n> > > > +\n> > > > + /* Initialize GStreamer */\n> > > > + g_autoptr(GError) errInit = NULL;\n> > > > + if (!gst_init_check(nullptr, nullptr, &errInit)) {\n> > > > + g_printerr(\"Could not initialize GStreamer:\n> %s\\n\",\n> > > > + errInit ? errInit->message :\n> \"unknown error\");\n> > > > +\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + /*\n> > > > + * Remove the system libcamera plugin, if any, and add\n> the\n> > > > + * plugin from the build directory.\n> > > > + */\n> > > > + GstRegistry *registry = gst_registry_get();\n> > > > + GstPlugin *plugin = gst_registry_lookup(registry,\n> \"libgstlibcamera.so\");\n> > > > + if (plugin) {\n> > > > + gst_registry_remove_plugin(registry, plugin);\n> > > > + gst_object_unref(plugin);\n> > > > + }\n> > > > +\n> > > > + std::string path =\n> libcamera::utils::libcameraBuildPath()\n> > > > + + \"src/gstreamer\";\n> > > > + if (!gst_registry_scan_path(registry, path.c_str())) {\n> > > > + g_printerr(\"Failed to add plugin to\n> registry\\n\");\n> > > > + gst_deinit();\n> > > > + return TestFail;\n> > > > + }\n> > >\n> > > There's quite a bit of duplicated code between this and the single\n> > > stream test. Could you refactor the single stream test to create a base\n> > > class that can be shared with this one ?\n> >\n> > Yes, I am going to do this, but can we go about it as follows, merge this\n> > patch with duplicated code,\n> > then I think over it a bit and then submit new patch which refactors a\n> lot\n> > of the duplicated code,\n>\n> Why ? :-) I'm fine merging work in progress when there's a reason (for\n> instance to split very large partch series in chunks, or to avoid\n> blocking development when other developers depend on that code), but in\n> this case I think a refactoring before merging the second test is\n> better.\n>\n\nBecause that will make reviewing difficult.\n\n> also I don't think going ahead with making a base class is a good idea as,\n> > there's much functional difference between two codes,\n> > they rather have isolated code that repeats, but no so much as to roll it\n> > into a class.\n>\n> I don't agree, I think the tests are very similar. diffstat agrees with\n> me.\n>\n> $ diff -u gstreamer_single_stream_test.cpp gstreamer_multi_stream_test.cpp\n> | diffstat\n> gstreamer_multi_stream_test.cpp | 62\n> +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n> 1 file changed, 55 insertions(+), 7 deletions(-)\n>\n\nOther than the code that loads gstreamer libcamera library, none of the\nother code can be modularized. small blocks of code are similar\nin between, but \"similar\" not \"same\". I am unable to comeup with a way to\nrefactor it more.\n\n> > +\n> > > > + /* Check if platform support multistream output */\n> > > > + libcamera::CameraManager cm;\n> > > > + cm.start();\n> > > > + if (cm.cameras()[0]->streams().size() <= 1) {\n> > > > + cm.stop();\n> > > > + return TestSkip;\n> > > > + }\n> > > > + cm.stop();\n> > >\n> > > Is there a guarantee that libcamerasrc will pick the same camera ? It\n> >\n> > Yes, it is guaranteed if we don't pass it a camera-name which we aren't\n> see\n> > here:\n> >\n> https://git.linuxtv.org/libcamera.git/tree/src/gstreamer/gstlibcamerasrc.cpp#n237\n> >\n> > > seems quite fragile to me. Isn't there a way to instead query the\n> > > libcamerasrc element for the number of pads it supports ?\n> >\n> > I think this won't be possible unless we add code to the\n> > gstreamer-element, @Nicolas\n> > Dufresne can\n> > you answer this please, I am not sure :)\n>\n> The GstElement documentation mentions a numsrcpads field. I have little\n> experience with GStreamer, but it sounds like something that could help.\n> Have you tried it ?\n>\n\nNo, this won't work. An element by default will generate a single source\npad, after that I'd need\nto explicitly request a request pad, which will map to a new stream. I\nguess this variable stores the\nthe number of active source pads and not the theoretical supported pads.\n\n> > More than\n> > > that, as not all cameras support multiple streams, it would be nice if\n> > > the test could iterate over all cameras to find one that does.\n> >\n> > This sounds like a nice idea, I could store the camera-name of the\n> specific\n> > camera and set the camera-name property\n> > of the libcamera element and start on it, but now I don't a multicamera\n> > setup.\n> >\n> > > +\n> > > > + /* Create the elements */\n> > > > + libcameraSrc_ =\n> gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > > > + convert0_ = gst_element_factory_make(\"videoconvert\",\n> \"convert0\");\n> > > > + convert1_ = gst_element_factory_make(\"videoconvert\",\n> \"convert1\");\n> > > > + sink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > > + sink1_ = gst_element_factory_make(\"fakesink\", \"sink1\");\n> > > > + queue0_ = gst_element_factory_make(\"queue\",\n> \"camera_queue0\");\n> > > > + queue1_ = gst_element_factory_make(\"queue\",\n> \"camera_queue1\");\n> > >\n> > > Looks like there's room to make the code more generic, posibly storing\n> > > the elements in containers.\n> >\n> > Okay something like a vector to hold the elements and then a map to hold\n> > .\n> > But this is a test code, is this fancy stuff really needed ? !!\n>\n> Test code doesn't mean that it has to be dirty and throw away all\n> software development good practices. Tests are first class citizens.\n> It's not about \"fancy stuff\", it's about making the code more readable\n> and maintainable.\n>\n\nI am not sure what is the ideal way for you to do it, please let me know.\nJust adding all these elements in\na struct ?\n\n> > > +\n> > > > + /* Create the empty pipeline_ */\n> > > > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > +\n> > > > + if (!pipeline_ || !convert0_ || !convert1_ || !sink0_\n> ||\n> > > > + !sink1_ || !queue0_ || !queue1_ ||\n> !libcameraSrc_) {\n> > > > + g_printerr(\"Not all elements could be\n> created.> %p.%p.%p.%p.%p.%p.%p.%p\\n\",\n> > > > + pipeline_, convert0_,\n> convert1_, sink0_,\n> > > > + sink1_, queue0_,\n> queue1_, libcameraSrc_);\n> > > > + if (pipeline_)\n> > > > + gst_object_unref(pipeline_);\n> > > > + if (convert0_)\n> > > > + gst_object_unref(convert0_);\n> > > > + if (convert1_)\n> > > > + gst_object_unref(convert1_);\n> > > > + if (sink0_)\n> > > > + gst_object_unref(sink0_);\n> > > > + if (sink1_)\n> > > > + gst_object_unref(sink1_);\n> > > > + if (queue0_)\n> > > > + gst_object_unref(queue0_);\n> > > > + if (queue1_)\n> > > > + gst_object_unref(queue1_);\n> > > > + if (libcameraSrc_)\n> > > > + gst_object_unref(libcameraSrc_);\n> > > > + gst_deinit();\n> > > > +\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + return TestPass;\n> > > > + }\n> > > > +\n> > > > + void cleanup() override\n> > > > + {\n> > > > + gst_object_unref(pipeline_);\n> > > > + gst_deinit();\n> > > > + }\n> > > > +\n> > > > + int run() override\n> > > > + {\n> > > > + GstStateChangeReturn ret;\n> > > > +\n> > > > + /* Build the pipeline */\n> > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n> queue0_, queue1_,\n> > > > + convert0_,\n> convert1_, sink0_, sink1_, NULL);\n> > > > + if (gst_element_link_many(queue0_, convert0_, sink0_,\n> NULL) != TRUE\n> > > > + || gst_element_link_many(queue1_,\n> convert1_, sink1_, NULL) != TRUE) {\n> > > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + g_autoptr(GstPad) src_pad =\n> gst_element_get_static_pad(libcameraSrc_, \"src\");\n> > > > + g_autoptr(GstPad) request_pad =\n> gst_element_get_request_pad(libcameraSrc_, \"src_%u\");\n> > > > + GstPad *queue0_sink_pad =\n> gst_element_get_static_pad(queue0_, \"sink\");\n> > > > + GstPad *queue1_sink_pad =\n> gst_element_get_static_pad(queue1_, \"sink\");\n> > > > +\n> > > > + if (gst_pad_link(src_pad, queue0_sink_pad) !=\n> GST_PAD_LINK_OK\n> > > > + || gst_pad_link(request_pad,\n> queue1_sink_pad) != GST_PAD_LINK_OK) {\n> > > > + if (queue0_sink_pad)\n> > > > + gst_object_unref(queue0_sink_pad);\n> > > > + if (queue1_sink_pad)\n> > > > + gst_object_unref(queue1_sink_pad);\n> > > > + g_printerr(\"Pads could not be linked.\\n\");\n> > > > + return TestFail;\n> > > > + }\n> > > > + gst_object_unref(queue0_sink_pad);\n> > > > + gst_object_unref(queue1_sink_pad);\n> > > > +\n> > > > + /* Start playing */\n> > > > + ret = gst_element_set_state(pipeline_,\n> GST_STATE_PLAYING);\n> > > > + if (ret == GST_STATE_CHANGE_FAILURE) {\n> > > > + g_printerr(\"Unable to set the pipeline to the\n> playing state.\\n\");\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + /* Wait until error or EOS or timeout after 2 seconds\n> */\n> > > > + constexpr GstMessageType msgType =\n> > > > + static_cast(GST_MESSAGE_ERROR\n> | GST_MESSAGE_EOS);\n> > > > + constexpr GstClockTime timeout = 2 * GST_SECOND;\n> > > > +\n> > > > + g_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);\n> > > > + g_autoptr(GstMessage) msg =\n> gst_bus_timed_pop_filtered(bus, timeout, msgType);\n> > > > +\n> > > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > > > +\n> > > > + /* Parse error message */\n> > > > + if (msg == NULL)\n> > > > + return TestPass;\n> > > > +\n> > > > + switch (GST_MESSAGE_TYPE(msg)) {\n> > > > + case GST_MESSAGE_ERROR:\n> > > > + gstreamer_print_error(msg);\n> > > > + break;\n> > > > + case GST_MESSAGE_EOS:\n> > > > + g_print(\"End-Of-Stream reached.\\n\");\n> > > > + break;\n> > > > + default:\n> > > > + g_printerr(\"Unexpected message received.\\n\");\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > +private:\n> > > > + void gstreamer_print_error(GstMessage *msg)\n> > > > + {\n> > > > + g_autoptr(GError) err = NULL;\n> > > > + g_autofree gchar *debug_info = NULL;\n> > > > +\n> > > > + gst_message_parse_error(msg, &err, &debug_info);\n> > > > + g_printerr(\"Error received from element %s: %s\\n\",\n> > > > + GST_OBJECT_NAME(msg->src),\n> err->message);\n> > > > + g_printerr(\"Debugging information: %s\\n\",\n> > > > + debug_info ? debug_info : \"none\");\n> > > > + }\n> > > > +\n> > > > + GstElement *pipeline_;\n> > > > + GstElement *libcameraSrc_;\n> > > > + GstElement *queue0_;\n> > > > + GstElement *queue1_;\n> > > > + GstElement *convert0_;\n> > > > + GstElement *convert1_;\n> > > > + GstElement *sink0_;\n> > > > + GstElement *sink1_;\n> > > > +};\n> > > > +\n> > > > +TEST_REGISTER(GstreamerSingleStreamTest)\n> > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > index b99aa0da..e28225b2 100644\n> > > > --- a/test/gstreamer/meson.build\n> > > > +++ b/test/gstreamer/meson.build\n> > > > @@ -6,6 +6,7 @@ endif\n> > > >\n> > > > gstreamer_tests = [\n> > > > ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> > > > + ['multi_stream_test', 'gstreamer_multi_stream_test.cpp'],\n> > > > ]\n> > > > gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n\nRegards,\n*Vedant Paranjape*","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 38388BDC71\n\tfor ;\n\tSun, 29 Aug 2021 10:05:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A2ED68933;\n\tSun, 29 Aug 2021 12:05:03 +0200 (CEST)","from mail-yb1-xb31.google.com (mail-yb1-xb31.google.com\n\t[IPv6:2607:f8b0:4864:20::b31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6010868891\n\tfor ;\n\tSun, 29 Aug 2021 12:05:01 +0200 (CEST)","by mail-yb1-xb31.google.com with SMTP id r4so21835187ybp.4\n\tfor ;\n\tSun, 29 Aug 2021 03:05:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"q+5guwW+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=cJ+HMZ5Sf1NS2aBTgK5eW/5kiK9TFmlvBGnIM8Zxre4=;\n\tb=q+5guwW+lEoowD0B3aQNeiiK4ysMq+ILuEdUR4S/sbIq5aHZodECLj+pNdcYljJ9f3\n\tWWaZkI8/tex1Jsz0U0kKn8IqyiYgY5OMNJA+OgjRgbbduN61gQeJKc00T1pEKxt0dFyq\n\t4xhPopYRz4iX1SwUye0+ovU9XEFYw0WtaonsnCJgJgbzIF/PoXRi75OSFCX4K622JrPt\n\tt2TuqnRdnAZJvWNv6HzSl7exY37QjsHF9o4oKPc+sdNEZWHpDKulj3EuF1/k1/op5wlJ\n\tLMOZQjokVScZBBIvx+hEOmVaekqXTSGOvt70Fx3vERv1RTcf790/5sNo15+Hw2xKDn78\n\trrEQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=cJ+HMZ5Sf1NS2aBTgK5eW/5kiK9TFmlvBGnIM8Zxre4=;\n\tb=dkifWuANYxSiae1PwIZzI8tjwHEL7+8+/wGS1xttiM3DWQUsDEp7WPXofEgjJ0oDPK\n\tcSTKERArkLH8+Fr5mVIF8SE3lnj4kheJbO02t9NJBuo4VY8Em5tGwC09hP4sGwI8KTyw\n\thkHvuuk/4qiGDXV06kDtBEyjZhiLZGDq+dsaiHgx0EAiBx8KcLCCV1oPC91uqvfNA9M9\n\t+c1k471VSTp+O/mAUQl4c08eIxTM5uHsyJ/at3QmljZwAoCeItBABX2YrHmOKsWvS+MC\n\t1QmD98mJjjXdBGfRt4PciZq8nessBdfBoJJWHaGy7GTwFNHAVCsFXpa/j7GhjI9xGM77\n\t3J4A==","X-Gm-Message-State":"AOAM530/OcYXi2eZN+6EX9E1rxV92WnYYczne3Z/x12V9gj8vwCVlV++\n\tNsOPlhzrupU/NtR1TW2YLaCq8J2z6pceqSHZEU8=","X-Google-Smtp-Source":"ABdhPJzyhCVu373UM9mFh6dyZ38o4bdfbk1EjxYDVHGlf5PetpLbHAVMtqW0DL+w2caCOwmDDVdZNGVRLROt7cr05kc=","X-Received":"by 2002:a25:b787:: with SMTP id\n\tn7mr16676559ybh.468.1630231500024; \n\tSun, 29 Aug 2021 03:05:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20210828173753.336859-1-vedantparanjape160201@gmail.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Vedant Paranjape ","Date":"Sun, 29 Aug 2021 15:34:46 +0530","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000075ee7d05caafd9ee\"","Subject":"Re: [libcamera-devel] [PATCH v2] test: gstreamer: Add a test for\n\tgstreamer multi stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19169,"web_url":"https://patchwork.libcamera.org/comment/19169/","msgid":"","date":"2021-08-30T11:18:54","subject":"Re: [libcamera-devel] [PATCH v2] test: gstreamer: Add a test for\n\tgstreamer multi stream","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Vedant,\n\nOn Sun, Aug 29, 2021 at 03:34:46PM +0530, Vedant Paranjape wrote:\n> On Sun, Aug 29, 2021 at 2:56 PM Laurent Pinchart wrote:\n> > On Sun, Aug 29, 2021 at 10:53:34AM +0530, Vedant Paranjape wrote:\n> > > On Sun, Aug 29, 2021 at 6:39 AM Laurent Pinchart wrote:\n> > > > On Sat, Aug 28, 2021 at 11:07:53PM +0530, Vedant Paranjape wrote:\n> > > > > This patch adds a test to test if multi stream using libcamera's\n> > > > > gstreamer element works.\n> > > > >\n> > > > > We need to work around one issue with ASan when enabled in the\n> > > > > build:\n> > > > >\n> > > > > - glib has a known leak at initialization time. This is covered by > he\n> > > > > suppression file shipped with glib, but it's not clear how to use > t\n> > > > > automatically. For now, disable leak detection to avoid test > ailures.\n> > > > >\n> > > > > Signed-off-by: Vedant Paranjape \n> > > > > ---\n> > > > > .../gstreamer/gstreamer_multi_stream_test.cpp | 225 > +++++++++++++++++\n> > > > > test/gstreamer/meson.build | 1 +\n> > > > > 2 files changed, 226 insertions(+)\n> > > > > create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp\n> > > > >\n> > > > > diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp > /test/gstreamer/gstreamer_multi_stream_test.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..e32b07a4\n> > > > > --- /dev/null\n> > > > > +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> > > > > @@ -0,0 +1,225 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > > + *\n> > > > > + * gstreamer_single_stream_test.cpp - GStreamer single stream > apture test\n> > > > > + */\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/source_paths.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"test.h\"\n> > > > > +\n> > > > > +using namespace std;\n> > > > > +\n> > > > > +extern \"C\" {\n> > > > > +const char *__asan_default_options()\n> > > > > +{\n> > > > > + /*\n> > > > > + * Disable leak detection due to a known global variable > nitialization\n> > > > > + * leak in glib's g_quark_init(). This should ideally be > andled by\n> > > > > + * using a suppression file instead of disabling leak > etection.\n> > > > > + */\n> > > > > + return \"detect_leaks=false\";\n> > > > > +}\n> > > > > +}\n> > > > > +\n> > > > > +class GstreamerSingleStreamTest : public Test\n> > > > > +{\n> > > > > +protected:\n> > > > > + int init() override\n> > > > > + {\n> > > > > + /*\n> > > > > + * GStreamer by default spawns a process to run the\n> > > > > + * gst-plugin-scanner helper. If libcamera is compiled > ith ASan\n> > > > > + * enabled, and as GStreamer is most likely not, this > auses the\n> > > > > + * ASan link order check to fail when > st-plugin-scanner\n> > > > > + * dlopen()s the plugin as many libraries will have > lready been\n> > > > > + * loaded by then. Fix this issue by disabling > pawning of a\n> > > > > + * child helper process when scanning the build > irectory for\n> > > > > + * plugins.\n> > > > > + */\n> > > > > + gst_registry_fork_set_enabled(false);\n> > > > > +\n> > > > > + /* Initialize GStreamer */\n> > > > > + g_autoptr(GError) errInit = NULL;\n> > > > > + if (!gst_init_check(nullptr, nullptr, &errInit)) {\n> > > > > + g_printerr(\"Could not initialize GStreamer: > s\\n\",\n> > > > > + errInit ? errInit->message : > unknown error\");\n> > > > > +\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > +\n> > > > > + /*\n> > > > > + * Remove the system libcamera plugin, if any, and add > he\n> > > > > + * plugin from the build directory.\n> > > > > + */\n> > > > > + GstRegistry *registry = gst_registry_get();\n> > > > > + GstPlugin *plugin = gst_registry_lookup(registry, > libgstlibcamera.so\");\n> > > > > + if (plugin) {\n> > > > > + gst_registry_remove_plugin(registry, plugin);\n> > > > > + gst_object_unref(plugin);\n> > > > > + }\n> > > > > +\n> > > > > + std::string path = > ibcamera::utils::libcameraBuildPath()\n> > > > > + + \"src/gstreamer\";\n> > > > > + if (!gst_registry_scan_path(registry, path.c_str())) {\n> > > > > + g_printerr(\"Failed to add plugin to > egistry\\n\");\n> > > > > + gst_deinit();\n> > > > > + return TestFail;\n> > > > > + }\n> > > >\n> > > > There's quite a bit of duplicated code between this and the single\n> > > > stream test. Could you refactor the single stream test to create a base\n> > > > class that can be shared with this one ?\n> > >\n> > > Yes, I am going to do this, but can we go about it as follows, merge this\n> > > patch with duplicated code,\n> > > then I think over it a bit and then submit new patch which refactors a lot\n> > > of the duplicated code,\n> >\n> > Why ? :-) I'm fine merging work in progress when there's a reason (for\n> > instance to split very large partch series in chunks, or to avoid\n> > blocking development when other developers depend on that code), but in\n> > this case I think a refactoring before merging the second test is\n> > better.\n> \n> Because that will make reviewing difficult.\n\nBeing on the reviewer side, I respectfully disagree :-)\n\n> > also I don't think going ahead with making a base class is a good idea as,\n> > > there's much functional difference between two codes,\n> > > they rather have isolated code that repeats, but no so much as to roll it\n> > > into a class.\n> >\n> > I don't agree, I think the tests are very similar. diffstat agrees with\n> > me.\n> >\n> > $ diff -u gstreamer_single_stream_test.cpp gstreamer_multi_stream_test.cpp | diffstat\n> > gstreamer_multi_stream_test.cpp | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n> > 1 file changed, 55 insertions(+), 7 deletions(-)\n> \n> Other than the code that loads gstreamer libcamera library, none of the\n> other code can be modularized. small blocks of code are similar\n> in between, but \"similar\" not \"same\". I am unable to comeup with a way to\n> refactor it more.\n> \n> > > +\n> > > > > + /* Check if platform support multistream output */\n> > > > > + libcamera::CameraManager cm;\n> > > > > + cm.start();\n> > > > > + if (cm.cameras()[0]->streams().size() <= 1) {\n> > > > > + cm.stop();\n> > > > > + return TestSkip;\n> > > > > + }\n> > > > > + cm.stop();\n> > > >\n> > > > Is there a guarantee that libcamerasrc will pick the same camera ? It\n> > >\n> > > Yes, it is guaranteed if we don't pass it a camera-name which we aren't see\n> > > here:\n> > >\n> > https://git.linuxtv.org/libcamera.git/tree/src/gstreamer/gstlibcamerasrc.cpp#n237\n> > >\n> > > > seems quite fragile to me. Isn't there a way to instead query the\n> > > > libcamerasrc element for the number of pads it supports ?\n> > >\n> > > I think this won't be possible unless we add code to the\n> > > gstreamer-element, @Nicolas\n> > > Dufresne can\n> > > you answer this please, I am not sure :)\n> >\n> > The GstElement documentation mentions a numsrcpads field. I have little\n> > experience with GStreamer, but it sounds like something that could help.\n> > Have you tried it ?\n> \n> No, this won't work. An element by default will generate a single source pad, after that I'd need\n> to explicitly request a request pad, which will map to a new stream. I guess this variable stores the\n> the number of active source pads and not the theoretical supported pads.\n\nGood point. I'll defer to Nicolas on this topic.\n\n> > > > More than\n> > > > that, as not all cameras support multiple streams, it would be nice if\n> > > > the test could iterate over all cameras to find one that does.\n> > >\n> > > This sounds like a nice idea, I could store the camera-name of the specific\n> > > camera and set the camera-name property\n> > > of the libcamera element and start on it, but now I don't a multicamera\n> > > setup.\n> > >\n> > > > +\n> > > > > + /* Create the elements */\n> > > > > + libcameraSrc_ = > st_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > > > > + convert0_ = gst_element_factory_make(\"videoconvert\", > convert0\");\n> > > > > + convert1_ = gst_element_factory_make(\"videoconvert\", > convert1\");\n> > > > > + sink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > > > + sink1_ = gst_element_factory_make(\"fakesink\", \"sink1\");\n> > > > > + queue0_ = gst_element_factory_make(\"queue\", > camera_queue0\");\n> > > > > + queue1_ = gst_element_factory_make(\"queue\", > camera_queue1\");\n> > > >\n> > > > Looks like there's room to make the code more generic, posibly storing\n> > > > the elements in containers.\n> > >\n> > > Okay something like a vector to hold the elements and then a map to hold\n> > > .\n> > > But this is a test code, is this fancy stuff really needed ? !!\n> >\n> > Test code doesn't mean that it has to be dirty and throw away all\n> > software development good practices. Tests are first class citizens.\n> > It's not about \"fancy stuff\", it's about making the code more readable\n> > and maintainable.\n> \n> I am not sure what is the ideal way for you to do it, please let me know.\n> Just adding all these elements in\n> a struct ?\n\nI'll review the patch you've sent, it's easier to discuss it in that\ncontext.\n\n> > > > +\n> > > > > + /* Create the empty pipeline_ */\n> > > > > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > > +\n> > > > > + if (!pipeline_ || !convert0_ || !convert1_ || !sink0_ ||\n> > > > > + !sink1_ || !queue0_ || !queue1_ || !libcameraSrc_) {\n> > > > > + g_printerr(\"Not all elements could be > reated.> %p.%p.%p.%p.%p.%p.%p.%p\\n\",\n> > > > > + pipeline_, convert0_, > onvert1_, sink0_,\n> > > > > + sink1_, queue0_, > ueue1_, libcameraSrc_);\n> > > > > + if (pipeline_)\n> > > > > + gst_object_unref(pipeline_);\n> > > > > + if (convert0_)\n> > > > > + gst_object_unref(convert0_);\n> > > > > + if (convert1_)\n> > > > > + gst_object_unref(convert1_);\n> > > > > + if (sink0_)\n> > > > > + gst_object_unref(sink0_);\n> > > > > + if (sink1_)\n> > > > > + gst_object_unref(sink1_);\n> > > > > + if (queue0_)\n> > > > > + gst_object_unref(queue0_);\n> > > > > + if (queue1_)\n> > > > > + gst_object_unref(queue1_);\n> > > > > + if (libcameraSrc_)\n> > > > > + gst_object_unref(libcameraSrc_);\n> > > > > + gst_deinit();\n> > > > > +\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > +\n> > > > > + return TestPass;\n> > > > > + }\n> > > > > +\n> > > > > + void cleanup() override\n> > > > > + {\n> > > > > + gst_object_unref(pipeline_);\n> > > > > + gst_deinit();\n> > > > > + }\n> > > > > +\n> > > > > + int run() override\n> > > > > + {\n> > > > > + GstStateChangeReturn ret;\n> > > > > +\n> > > > > + /* Build the pipeline */\n> > > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > ueue0_, queue1_,\n> > > > > + convert0_, > onvert1_, sink0_, sink1_, NULL);\n> > > > > + if (gst_element_link_many(queue0_, convert0_, sink0_, > ULL) != TRUE\n> > > > > + || gst_element_link_many(queue1_, > onvert1_, sink1_, NULL) != TRUE) {\n> > > > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > +\n> > > > > + g_autoptr(GstPad) src_pad = > st_element_get_static_pad(libcameraSrc_, \"src\");\n> > > > > + g_autoptr(GstPad) request_pad = > st_element_get_request_pad(libcameraSrc_, \"src_%u\");\n> > > > > + GstPad *queue0_sink_pad = > st_element_get_static_pad(queue0_, \"sink\");\n> > > > > + GstPad *queue1_sink_pad = > st_element_get_static_pad(queue1_, \"sink\");\n> > > > > +\n> > > > > + if (gst_pad_link(src_pad, queue0_sink_pad) != > ST_PAD_LINK_OK\n> > > > > + || gst_pad_link(request_pad, > ueue1_sink_pad) != GST_PAD_LINK_OK) {\n> > > > > + if (queue0_sink_pad)\n> > > > > + gst_object_unref(queue0_sink_pad);\n> > > > > + if (queue1_sink_pad)\n> > > > > + gst_object_unref(queue1_sink_pad);\n> > > > > + g_printerr(\"Pads could not be linked.\\n\");\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > + gst_object_unref(queue0_sink_pad);\n> > > > > + gst_object_unref(queue1_sink_pad);\n> > > > > +\n> > > > > + /* Start playing */\n> > > > > + ret = gst_element_set_state(pipeline_, > ST_STATE_PLAYING);\n> > > > > + if (ret == GST_STATE_CHANGE_FAILURE) {\n> > > > > + g_printerr(\"Unable to set the pipeline to the > laying state.\\n\");\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > +\n> > > > > + /* Wait until error or EOS or timeout after 2 seconds > /\n> > > > > + constexpr GstMessageType msgType =\n> > > > > + static_cast(GST_MESSAGE_ERROR > GST_MESSAGE_EOS);\n> > > > > + constexpr GstClockTime timeout = 2 * GST_SECOND;\n> > > > > +\n> > > > > + g_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);\n> > > > > + g_autoptr(GstMessage) msg = > st_bus_timed_pop_filtered(bus, timeout, msgType);\n> > > > > +\n> > > > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > > > > +\n> > > > > + /* Parse error message */\n> > > > > + if (msg == NULL)\n> > > > > + return TestPass;\n> > > > > +\n> > > > > + switch (GST_MESSAGE_TYPE(msg)) {\n> > > > > + case GST_MESSAGE_ERROR:\n> > > > > + gstreamer_print_error(msg);\n> > > > > + break;\n> > > > > + case GST_MESSAGE_EOS:\n> > > > > + g_print(\"End-Of-Stream reached.\\n\");\n> > > > > + break;\n> > > > > + default:\n> > > > > + g_printerr(\"Unexpected message received.\\n\");\n> > > > > + break;\n> > > > > + }\n> > > > > +\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > +\n> > > > > +private:\n> > > > > + void gstreamer_print_error(GstMessage *msg)\n> > > > > + {\n> > > > > + g_autoptr(GError) err = NULL;\n> > > > > + g_autofree gchar *debug_info = NULL;\n> > > > > +\n> > > > > + gst_message_parse_error(msg, &err, &debug_info);\n> > > > > + g_printerr(\"Error received from element %s: %s\\n\",\n> > > > > + GST_OBJECT_NAME(msg->src), > rr->message);\n> > > > > + g_printerr(\"Debugging information: %s\\n\",\n> > > > > + debug_info ? debug_info : \"none\");\n> > > > > + }\n> > > > > +\n> > > > > + GstElement *pipeline_;\n> > > > > + GstElement *libcameraSrc_;\n> > > > > + GstElement *queue0_;\n> > > > > + GstElement *queue1_;\n> > > > > + GstElement *convert0_;\n> > > > > + GstElement *convert1_;\n> > > > > + GstElement *sink0_;\n> > > > > + GstElement *sink1_;\n> > > > > +};\n> > > > > +\n> > > > > +TEST_REGISTER(GstreamerSingleStreamTest)\n> > > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > > index b99aa0da..e28225b2 100644\n> > > > > --- a/test/gstreamer/meson.build\n> > > > > +++ b/test/gstreamer/meson.build\n> > > > > @@ -6,6 +6,7 @@ endif\n> > > > >\n> > > > > gstreamer_tests = [\n> > > > > ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> > > > > + ['multi_stream_test', 'gstreamer_multi_stream_test.cpp'],\n> > > > > ]\n> > > > > gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 238C4BDC71\n\tfor ;\n\tMon, 30 Aug 2021 11:19:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C24C69166;\n\tMon, 30 Aug 2021 13:19:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47CE760258\n\tfor ;\n\tMon, 30 Aug 2021 13:19:08 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B0A8D5A7;\n\tMon, 30 Aug 2021 13:19:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"t+XtVaxP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630322347;\n\tbh=ay6SMxdlVCWvpH6ptieYOMV/eWXikFxfcqxb7z/y8a0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=t+XtVaxP3XE6wjes/dR2fsqh3e9gLvdbDEzWdUMESZNscUIM4kbJeuOpQ04YxOZGP\n\tNLeOLTrp5HnoqJGIoysEnzMNA63fIRJz72YebvVwQ7Ny+veLGXWt2JP08Y39ggHEy2\n\tXdJtk4RGDHH0ihhxPaaf0P80sAev7/VQB/jrXPjE=","Date":"Mon, 30 Aug 2021 14:18:54 +0300","From":"Laurent Pinchart ","To":"Vedant Paranjape ","Message-ID":"","References":"<20210828173753.336859-1-vedantparanjape160201@gmail.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] test: gstreamer: Add a test for\n\tgstreamer multi stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]