[{"id":18747,"web_url":"https://patchwork.libcamera.org/comment/18747/","msgid":"","date":"2021-08-13T00:25:02","subject":"Re: [libcamera-devel] [PATCH v7] test: gstreamer: Add test for\n\tgstreamer single 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 Thu, Aug 12, 2021 at 04:10:30PM +0530, Vedant Paranjape wrote:\n> This patch adds a test to test if single stream using\n> libcamera's gstreamer element works.\n> \n> Signed-off-by: Vedant Paranjape \n> Reviewed-by: Paul Elder \n> ---\n> .../gstreamer_single_stream_test.cpp | 134 ++++++++++++++++++\n> test/gstreamer/meson.build | 19 +++\n> test/meson.build | 1 +\n> 3 files changed, 154 insertions(+)\n> create mode 100644 test/gstreamer/gstreamer_single_stream_test.cpp\n> create mode 100644 test/gstreamer/meson.build\n> \n> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> new file mode 100644\n> index 00000000..040b1e7e\n> --- /dev/null\n> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> @@ -0,0 +1,134 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Vedant Paranjape\n> + *\n> + * ipa_interface_test.cpp - Test the IPA interface\n> + */\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +\n> +class GstreamerSingleStreamTest : public Test\n> +{\n> +protected:\n> +\tint init() override\n> +\t{\n> +\t\t/* Initialize GStreamer */\n> +\t\tGError *err_init = nullptr;\n\nLet's use the libcamera coding style, this should be errInit. Same for\nlibcamera_src_.\n\n> +\t\tif (!gst_init_check(nullptr, nullptr, &err_init)) {\n> +\t\t\tg_printerr(\"Could not initialize GStreamer: %s\\n\",\n> +\t\t\t\t err_init ? err_init->message : \"unknown error\");\n> +\t\t\tif (err_init)\n> +\t\t\t\tg_error_free(err_init);\n> +\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (!gst_registry_scan_path(gst_registry_get(),\n> +\t\t\t\t\t std::string(libcamera::utils::libcameraBuildPath() +\n> +\t\t\t\t\t\t\t\"src/gstreamer\")\n> +\t\t\t\t\t\t .c_str())) {\n\nLet's use a local variable to make this a bit more readable.\n\n\t\tstd::string path = libcamera::utils::libcameraBuildPath()\n\t\t\t\t + \"src/gstreamer\";\n\t\tif (!gst_registry_scan_path(gst_registry_get(), path.c_str()) {\n\n> +\t\t\tg_printerr(\"Failed to add plugin to registry\\n\");\n\nYou need to call gst_deinit() here.\n\n> +\t\t\treturn TestFail;\n> +\t\t}\n\nThis won't work if the libcamerasrc plugin is already installed on the\nsystem, gst_registry_scan_path() will not add anything in that case. You\nneed to remove the plugin first. I've tested the following code:\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\n> +\n> +\t\t/* Create the elements */\n> +\t\tlibcamera_src_ = gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> +\t\tconvert0_ = gst_element_factory_make(\"videoconvert\", \"convert0\");\n> +\t\tsink0_ = gst_element_factory_make(\"fakevideosink\", \"sink0\");\n> +\n> +\t\t/* Create the empty pipeline_ */\n> +\t\tpipeline_ = gst_pipeline_new(\"test-pipeline\");\n> +\n> +\t\tif (!pipeline_ || !convert0_ || !sink0_ || !libcamera_src_) {\n> +\t\t\tg_printerr(\"Not all elements could be created.\\n\");\n\ncleanup() isn't called if init() fails, so you need to unref pipeline_,\nbut also libcamera_src_, convert0_ and sink0_ here. The last three\nelements don't need to be unrefered in cleanup(), because\ngst_bin_add_many() makes the pipeline take ownership of them.\n\ngst_deinit() is also needed here.\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(bus_);\n> +\t\tgst_element_set_state(pipeline_, GST_STATE_NULL);\n\nI'd move this call to the run() function, just before the\n/* Parse error message */ comment, as it shouldn't be executed if the\npipeline isn't playing.\n\n> +\t\tgst_object_unref(pipeline_);\n> +\t\tgst_deinit();\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\t/* Build the pipeline */\n> +\t\tgst_bin_add_many(GST_BIN(pipeline_), libcamera_src_, convert0_, sink0_, NULL);\n> +\t\tif (gst_element_link_many(libcamera_src_, convert0_, sink0_, NULL) != TRUE) {\n> +\t\t\tg_printerr(\"Elements could not be linked.\\n\");\n> +\t\t\tgst_object_unref(pipeline_);\n\nThis will cause a double unref, as cleanup() will be called.\n\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Start playing */\n> +\t\tret_ = gst_element_set_state(pipeline_, GST_STATE_PLAYING);\n\nThis can be a local variable.\n\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\tgst_object_unref(pipeline_);\n\nSame here, double unref.\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\tGstClockTime timeout = 2000000000;\n> +\t\tbus_ = gst_element_get_bus(pipeline_);\n> +\t\tmsg_ = gst_bus_timed_pop_filtered(bus_, timeout,\n\nBoth bus and msg can be local variables. Otherwise, for bus_, you'd have\nto initialize it to nullptr, or cleanup() will crash if one of the above\ntwo conditions is false.\n\n> +\t\t\t\t\t\t GstMessageType((uint)GST_MESSAGE_ERROR | (uint)GST_MESSAGE_EOS));\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\treturn TestFail;\n> +\t\t\tbreak;\n> +\t\tcase GST_MESSAGE_EOS:\n> +\t\t\tg_print(\"End-Of-Stream reached.\\n\");\n> +\t\t\treturn TestFail;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tg_printerr(\"Unexpected message received.\\n\");\n> +\t\t\treturn TestFail;\n> +\t\t\tbreak;\n> +\t\t}\n\nA blank line would be nice here.\n\n> +\t\tgst_message_unref(msg_);\n\nThis will never be called, as all the cases above return. Furthermore,\nmsg_ is leaked in all those cases. I'd remove the return statements, and\nturn the next line into\n\n\t\treturn TestFail;\n\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +private:\n> +\tGstElement *pipeline_, *libcamera_src_, *convert0_, *sink0_;\n\nOne variable per line please:\n\n\tGstElement *pipeline_;\n\tGstElement *libcamera_src_;\n\tGstElement *convert0_;\n\tGstElement *sink0_;\n\n> +\tGstBus *bus_;\n> +\tGstMessage *msg_;\n> +\tGstStateChangeReturn ret_;\n> +\n> +\tvoid gstreamer_print_error(GstMessage *msg)\n> +\t{\n> +\t\tGError *err;\n> +\t\tgchar *debug_info;\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 GST_OBJECT_NAME(msg->src), err->message);\n> +\t\tg_printerr(\"Debugging information: %s\\n\",\n> +\t\t\t debug_info ? debug_info : \"none\");\n> +\t\tg_clear_error(&err);\n> +\t\tg_free(debug_info);\n> +\t}\n\nWe usually put functions before variables.\n\n> +};\n> +\n> +TEST_REGISTER(GstreamerSingleStreamTest)\n> diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> new file mode 100644\n> index 00000000..b99aa0da\n> --- /dev/null\n> +++ b/test/gstreamer/meson.build\n> @@ -0,0 +1,19 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +if not gst_enabled\n> + subdir_done()\n> +endif\n> +\n> +gstreamer_tests = [\n> + ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> +]\n> +gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> +\n> +foreach t : gstreamer_tests\n> + exe = executable(t[0], t[1],\n> + dependencies : [libcamera_private, gstreamer_dep],\n> + link_with : test_libraries,\n> + include_directories : test_includes_internal)\n> +\n> + test(t[0], exe, suite : 'gstreamer', is_parallel : false)\n> +endforeach\n> diff --git a/test/meson.build b/test/meson.build\n> index 3bceb5df..d0466f17 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -11,6 +11,7 @@ subdir('libtest')\n> \n> subdir('camera')\n> subdir('controls')\n> +subdir('gstreamer')\n> subdir('ipa')\n> subdir('ipc')\n> subdir('log')","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 55B06BD87D\n\tfor ;\n\tFri, 13 Aug 2021 00:25:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F43768888;\n\tFri, 13 Aug 2021 02:25:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41D9168826\n\tfor ;\n\tFri, 13 Aug 2021 02:25:07 +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 AFE85EE;\n\tFri, 13 Aug 2021 02:25:06 +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=\"lY6fE9Ny\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628814306;\n\tbh=FXBepYTCVyHvNRPkr2c/X2D9tbCMGNV8Cp0T0kPq8Ig=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lY6fE9NypmIWpozHQgqUmpoI6DrWjoUTgMDLxnAHasHNsqxm2JRDjnh8z/mjb3iGC\n\tZkoTpvM1eWyV3GUx1bz06XxSl4msCX1oILZlxExHNL17qHAWqFSkTbTyHYEOvzCC8H\n\tYT+uHpUBQNQtJEbZUB3CQm5MoFdwDABTwOkwciVM=","Date":"Fri, 13 Aug 2021 03:25:02 +0300","From":"Laurent Pinchart ","To":"Vedant Paranjape ","Message-ID":"","References":"<20210812104030.557246-1-vedantparanjape160201@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210812104030.557246-1-vedantparanjape160201@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v7] test: gstreamer: Add test for\n\tgstreamer single 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":18751,"web_url":"https://patchwork.libcamera.org/comment/18751/","msgid":"","date":"2021-08-13T06:14:19","subject":"Re: [libcamera-devel] [PATCH v7] test: gstreamer: Add test for\n\tgstreamer single 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 detailed review.\n\nOn Fri, Aug 13, 2021 at 5:55 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Vedant,\n>\n> Thank you for the patch.\n>\n> On Thu, Aug 12, 2021 at 04:10:30PM +0530, Vedant Paranjape wrote:\n> > This patch adds a test to test if single stream using\n> > libcamera's gstreamer element works.\n> >\n> > Signed-off-by: Vedant Paranjape \n> > Reviewed-by: Paul Elder \n> > ---\n> > .../gstreamer_single_stream_test.cpp | 134 ++++++++++++++++++\n> > test/gstreamer/meson.build | 19 +++\n> > test/meson.build | 1 +\n> > 3 files changed, 154 insertions(+)\n> > create mode 100644 test/gstreamer/gstreamer_single_stream_test.cpp\n> > create mode 100644 test/gstreamer/meson.build\n> >\n> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > new file mode 100644\n> > index 00000000..040b1e7e\n> > --- /dev/null\n> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > @@ -0,0 +1,134 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Vedant Paranjape\n> > + *\n> > + * ipa_interface_test.cpp - Test the IPA interface\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> > +\n> > +class GstreamerSingleStreamTest : public Test\n> > +{\n> > +protected:\n> > + int init() override\n> > + {\n> > + /* Initialize GStreamer */\n> > + GError *err_init = nullptr;\n>\n> Let's use the libcamera coding style, this should be errInit. Same for\n> libcamera_src_.\n>\n> > + if (!gst_init_check(nullptr, nullptr, &err_init)) {\n> > + g_printerr(\"Could not initialize GStreamer: %s\\n\",\n> > + err_init ? err_init->message : \"unknown\n> error\");\n> > + if (err_init)\n> > + g_error_free(err_init);\n> > +\n> > + return TestFail;\n> > + }\n> > +\n> > + if (!gst_registry_scan_path(gst_registry_get(),\n> > +\n> std::string(libcamera::utils::libcameraBuildPath() +\n> > + \"src/gstreamer\")\n> > + .c_str())) {\n>\n> Let's use a local variable to make this a bit more readable.\n>\n> std::string path = libcamera::utils::libcameraBuildPath()\n> + \"src/gstreamer\";\n> if (!gst_registry_scan_path(gst_registry_get(),\n> path.c_str()) {\n>\n> > + g_printerr(\"Failed to add plugin to registry\\n\");\n>\n> You need to call gst_deinit() here.\n>\n> > + return TestFail;\n> > + }\n>\n> This won't work if the libcamerasrc plugin is already installed on the\n> system, gst_registry_scan_path() will not add anything in that case. You\n> need to remove the plugin first. I've tested the following code:\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> > +\n> > + /* Create the elements */\n> > + libcamera_src_ = gst_element_factory_make(\"libcamerasrc\",\n> \"libcamera\");\n> > + convert0_ = gst_element_factory_make(\"videoconvert\",\n> \"convert0\");\n> > + sink0_ = gst_element_factory_make(\"fakevideosink\",\n> \"sink0\");\n> > +\n> > + /* Create the empty pipeline_ */\n> > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > +\n> > + if (!pipeline_ || !convert0_ || !sink0_ ||\n> !libcamera_src_) {\n> > + g_printerr(\"Not all elements could be created.\\n\");\n>\n> cleanup() isn't called if init() fails, so you need to unref pipeline_,\n> but also libcamera_src_, convert0_ and sink0_ here. The last three\n> elements don't need to be unrefered in cleanup(), because\n> gst_bin_add_many() makes the pipeline take ownership of them.\n>\n\nI could make all those pointers g_autoptr, right ?\n\ngst_deinit() is also needed here.\n>\n> > + return TestFail;\n> > + }\n> > +\n> > + return TestPass;\n> > + }\n> > +\n> > + void cleanup() override\n> > + {\n> > + gst_object_unref(bus_);\n> > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n>\n> I'd move this call to the run() function, just before the\n> /* Parse error message */ comment, as it shouldn't be executed if the\n> pipeline isn't playing.\n>\n> > + gst_object_unref(pipeline_);\n> > + gst_deinit();\n> > + }\n> > +\n> > + int run() override\n> > + {\n> > + /* Build the pipeline */\n> > + gst_bin_add_many(GST_BIN(pipeline_), libcamera_src_,\n> convert0_, sink0_, NULL);\n> > + if (gst_element_link_many(libcamera_src_, convert0_,\n> sink0_, NULL) != TRUE) {\n> > + g_printerr(\"Elements could not be linked.\\n\");\n> > + gst_object_unref(pipeline_);\n>\n> This will cause a double unref, as cleanup() will be called.\n>\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Start playing */\n> > + ret_ = gst_element_set_state(pipeline_, GST_STATE_PLAYING);\n>\n> This can be a local variable.\n>\n> > + if (ret_ == GST_STATE_CHANGE_FAILURE) {\n> > + g_printerr(\"Unable to set the pipeline to the\n> playing state.\\n\");\n> > + gst_object_unref(pipeline_);\n>\n> Same here, double unref.\n>\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Wait until error or EOS or timeout after 2 seconds */\n> > + GstClockTime timeout = 2000000000;\n> > + bus_ = gst_element_get_bus(pipeline_);\n> > + msg_ = gst_bus_timed_pop_filtered(bus_, timeout,\n>\n> Both bus and msg can be local variables. Otherwise, for bus_, you'd have\n> to initialize it to nullptr, or cleanup() will crash if one of the above\n> two conditions is false.\n>\n> > +\n> GstMessageType((uint)GST_MESSAGE_ERROR | (uint)GST_MESSAGE_EOS));\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> > + return TestFail;\n> > + break;\n> > + case GST_MESSAGE_EOS:\n> > + g_print(\"End-Of-Stream reached.\\n\");\n> > + return TestFail;\n> > + break;\n> > + default:\n> > + g_printerr(\"Unexpected message received.\\n\");\n> > + return TestFail;\n> > + break;\n> > + }\n>\n> A blank line would be nice here.\n>\n> > + gst_message_unref(msg_);\n>\n> This will never be called, as all the cases above return. Furthermore,\n> msg_ is leaked in all those cases. I'd remove the return statements, and\n> turn the next line into\n>\n> return TestFail;\n>\n> > +\n> > + return TestPass;\n> > + }\n> > +\n> > +private:\n> > + GstElement *pipeline_, *libcamera_src_, *convert0_, *sink0_;\n>\n> One variable per line please:\n>\n> GstElement *pipeline_;\n> GstElement *libcamera_src_;\n> GstElement *convert0_;\n> GstElement *sink0_;\n>\n> > + GstBus *bus_;\n> > + GstMessage *msg_;\n> > + GstStateChangeReturn ret_;\n> > +\n> > + void gstreamer_print_error(GstMessage *msg)\n> > + {\n> > + GError *err;\n> > + gchar *debug_info;\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> > + g_clear_error(&err);\n> > + g_free(debug_info);\n> > + }\n>\n> We usually put functions before variables.\n>\n> > +};\n> > +\n> > +TEST_REGISTER(GstreamerSingleStreamTest)\n> > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > new file mode 100644\n> > index 00000000..b99aa0da\n> > --- /dev/null\n> > +++ b/test/gstreamer/meson.build\n> > @@ -0,0 +1,19 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +if not gst_enabled\n> > + subdir_done()\n> > +endif\n> > +\n> > +gstreamer_tests = [\n> > + ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> > +]\n> > +gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > +\n> > +foreach t : gstreamer_tests\n> > + exe = executable(t[0], t[1],\n> > + dependencies : [libcamera_private, gstreamer_dep],\n> > + link_with : test_libraries,\n> > + include_directories : test_includes_internal)\n> > +\n> > + test(t[0], exe, suite : 'gstreamer', is_parallel : false)\n> > +endforeach\n> > diff --git a/test/meson.build b/test/meson.build\n> > index 3bceb5df..d0466f17 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -11,6 +11,7 @@ subdir('libtest')\n> >\n> > subdir('camera')\n> > subdir('controls')\n> > +subdir('gstreamer')\n> > subdir('ipa')\n> > subdir('ipc')\n> > subdir('log')\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 93328C3240\n\tfor ;\n\tFri, 13 Aug 2021 06:14:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B180568891;\n\tFri, 13 Aug 2021 08:14:33 +0200 (CEST)","from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com\n\t[IPv6:2607:f8b0:4864:20::b36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB0E46025F\n\tfor ;\n\tFri, 13 Aug 2021 08:14:32 +0200 (CEST)","by mail-yb1-xb36.google.com with SMTP id k11so16838309ybf.6\n\tfor ;\n\tThu, 12 Aug 2021 23:14:32 -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=\"PrvCEmWv\"; 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=XicYs6fuvItD/6PG5E1KEvKx11qmhuSLSu95D3YuPMs=;\n\tb=PrvCEmWvzTCLqg4WLiU192nITbEtFr7VU89VylMrWBQ7TSGDvlo9Iub9sHH89dicZ/\n\tVWnHvrtvU42PGtQ4YQ5SZu99r+n/g1nEeaAIaq3cuSdWdSrdD6FB95w20OxnF6w6WryA\n\tGXeurSZWGZlQ9AJ69iTbwD+RJdjzR+lyMZtegASmJtxLYhwek3QDwwOS/gc9wiXVS2v6\n\t19ZkbUSXaWIPcjFBuL9HN87Np0ag5idyuZS9VGSpoy6oqrmi+5J7/YJsCCTB+SUG2vQk\n\tNl+mXES6C/QF2k/TeCth09HxhcjU8JO22h0IwIjEdad1aaePAhiRtZIm1tzj47QwLNga\n\tXErQ==","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=XicYs6fuvItD/6PG5E1KEvKx11qmhuSLSu95D3YuPMs=;\n\tb=EfmUfovLy0QUN3/5FiXPwzTVUkv8C0W6Y0kq6KeRmZCnIMI19AtB5F334A/filqXi3\n\tGRsqca8uDqCv/PSIiQdvBAJAKvoEBPCgcgsDkfjFRs78+gMTDQw1ZTLfOHKraF13bWPO\n\tSkcvVR/VywgBahi88v4BMZrK+dcHe7JC4Qia/CY7WCAZXQ22CRPyV6Z71D6N9sgiZKcy\n\tLFl01Tltq+9SRNkqxHmCrgSvHonZ7MR8D8uwE2uRKn76bVViHeyHmiDrxkkYtiSG8L1k\n\tymCgHssIYJ7BfrHOK4LMsFp0+cokoPL+DwQbiEKw3VK+roqaCwm1QAnZKG5Qj9effgl4\n\tFtHQ==","X-Gm-Message-State":"AOAM533u5unReQz4pg6LPLZ8F0izVs6DYrx5E2ATOKraOtJHfx9VcoTt\n\th/XCzBdVLp52bCsHZsOd1jIM5Of0NEJ4Yv3DnQc=","X-Google-Smtp-Source":"ABdhPJy7+RFMkHL61cwEZQW91rZg2ymQEWSbaoER9Zjl3a1z4Af2cseQNGChzTFTCNZTbLfAXXcgr9cWpskDcg4Xinw=","X-Received":"by 2002:a25:da4e:: with SMTP id n75mr1077055ybf.63.1628835271183;\n\tThu, 12 Aug 2021 23:14:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20210812104030.557246-1-vedantparanjape160201@gmail.com>\n\t","In-Reply-To":"","From":"Vedant Paranjape ","Date":"Fri, 13 Aug 2021 11:44:19 +0530","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000bc92ac05c96ac3e0\"","Subject":"Re: [libcamera-devel] [PATCH v7] test: gstreamer: Add test for\n\tgstreamer single 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":18768,"web_url":"https://patchwork.libcamera.org/comment/18768/","msgid":"","date":"2021-08-13T11:56:39","subject":"Re: [libcamera-devel] [PATCH v7] test: gstreamer: Add test for\n\tgstreamer single 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 Fri, Aug 13, 2021 at 11:44:19AM +0530, Vedant Paranjape wrote:\n> On Fri, Aug 13, 2021 at 5:55 AM Laurent Pinchart wrote:\n> > On Thu, Aug 12, 2021 at 04:10:30PM +0530, Vedant Paranjape wrote:\n> > > This patch adds a test to test if single stream using\n> > > libcamera's gstreamer element works.\n> > >\n> > > Signed-off-by: Vedant Paranjape \n> > > Reviewed-by: Paul Elder \n> > > ---\n> > > .../gstreamer_single_stream_test.cpp | 134 ++++++++++++++++++\n> > > test/gstreamer/meson.build | 19 +++\n> > > test/meson.build | 1 +\n> > > 3 files changed, 154 insertions(+)\n> > > create mode 100644 test/gstreamer/gstreamer_single_stream_test.cpp\n> > > create mode 100644 test/gstreamer/meson.build\n> > >\n> > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > new file mode 100644\n> > > index 00000000..040b1e7e\n> > > --- /dev/null\n> > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > @@ -0,0 +1,134 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Vedant Paranjape\n> > > + *\n> > > + * ipa_interface_test.cpp - Test the IPA interface\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"test.h\"\n> > > +\n> > > +using namespace std;\n> > > +\n> > > +class GstreamerSingleStreamTest : public Test\n> > > +{\n> > > +protected:\n> > > + int init() override\n> > > + {\n> > > + /* Initialize GStreamer */\n> > > + GError *err_init = nullptr;\n> >\n> > Let's use the libcamera coding style, this should be errInit. Same for\n> > libcamera_src_.\n> >\n> > > + if (!gst_init_check(nullptr, nullptr, &err_init)) {\n> > > + g_printerr(\"Could not initialize GStreamer: %s\\n\",\n> > > + err_init ? err_init->message : \"unknown error\");\n> > > + if (err_init)\n> > > + g_error_free(err_init);\n> > > +\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (!gst_registry_scan_path(gst_registry_get(),\n> > > +\n> > std::string(libcamera::utils::libcameraBuildPath() +\n> > > + \"src/gstreamer\")\n> > > + .c_str())) {\n> >\n> > Let's use a local variable to make this a bit more readable.\n> >\n> > std::string path = libcamera::utils::libcameraBuildPath()\n> > + \"src/gstreamer\";\n> > if (!gst_registry_scan_path(gst_registry_get(), path.c_str()) {\n> >\n> > > + g_printerr(\"Failed to add plugin to registry\\n\");\n> >\n> > You need to call gst_deinit() here.\n> >\n> > > + return TestFail;\n> > > + }\n> >\n> > This won't work if the libcamerasrc plugin is already installed on the\n> > system, gst_registry_scan_path() will not add anything in that case. You\n> > need to remove the plugin first. I've tested the following code:\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> > > +\n> > > + /* Create the elements */\n> > > + libcamera_src_ = gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > > + convert0_ = gst_element_factory_make(\"videoconvert\", \"convert0\");\n> > > + sink0_ = gst_element_factory_make(\"fakevideosink\", \"sink0\");\n> > > +\n> > > + /* Create the empty pipeline_ */\n> > > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > +\n> > > + if (!pipeline_ || !convert0_ || !sink0_ || !libcamera_src_) {\n> > > + g_printerr(\"Not all elements could be created.\\n\");\n> >\n> > cleanup() isn't called if init() fails, so you need to unref pipeline_,\n> > but also libcamera_src_, convert0_ and sink0_ here. The last three\n> > elements don't need to be unrefered in cleanup(), because\n> > gst_bin_add_many() makes the pipeline take ownership of them.\n> \n> I could make all those pointers g_autoptr, right ?\n\nI think so, at least if they can be helpful, I'm not a glib specialist\n:-)\n\n> > gst_deinit() is also needed here.\n> >\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + return TestPass;\n> > > + }\n> > > +\n> > > + void cleanup() override\n> > > + {\n> > > + gst_object_unref(bus_);\n> > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> >\n> > I'd move this call to the run() function, just before the\n> > /* Parse error message */ comment, as it shouldn't be executed if the\n> > pipeline isn't playing.\n> >\n> > > + gst_object_unref(pipeline_);\n> > > + gst_deinit();\n> > > + }\n> > > +\n> > > + int run() override\n> > > + {\n> > > + /* Build the pipeline */\n> > > + gst_bin_add_many(GST_BIN(pipeline_), libcamera_src_, convert0_, sink0_, NULL);\n> > > + if (gst_element_link_many(libcamera_src_, convert0_, sink0_, NULL) != TRUE) {\n> > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > + gst_object_unref(pipeline_);\n> >\n> > This will cause a double unref, as cleanup() will be called.\n> >\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Start playing */\n> > > + ret_ = gst_element_set_state(pipeline_, GST_STATE_PLAYING);\n> >\n> > This can be a local variable.\n> >\n> > > + if (ret_ == GST_STATE_CHANGE_FAILURE) {\n> > > + g_printerr(\"Unable to set the pipeline to the playing state.\\n\");\n> > > + gst_object_unref(pipeline_);\n> >\n> > Same here, double unref.\n> >\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Wait until error or EOS or timeout after 2 seconds */\n> > > + GstClockTime timeout = 2000000000;\n> > > + bus_ = gst_element_get_bus(pipeline_);\n> > > + msg_ = gst_bus_timed_pop_filtered(bus_, timeout,\n> >\n> > Both bus and msg can be local variables. Otherwise, for bus_, you'd have\n> > to initialize it to nullptr, or cleanup() will crash if one of the above\n> > two conditions is false.\n> >\n> > > +\n> > GstMessageType((uint)GST_MESSAGE_ERROR | (uint)GST_MESSAGE_EOS));\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> > > + return TestFail;\n> > > + break;\n> > > + case GST_MESSAGE_EOS:\n> > > + g_print(\"End-Of-Stream reached.\\n\");\n> > > + return TestFail;\n> > > + break;\n> > > + default:\n> > > + g_printerr(\"Unexpected message received.\\n\");\n> > > + return TestFail;\n> > > + break;\n> > > + }\n> >\n> > A blank line would be nice here.\n> >\n> > > + gst_message_unref(msg_);\n> >\n> > This will never be called, as all the cases above return. Furthermore,\n> > msg_ is leaked in all those cases. I'd remove the return statements, and\n> > turn the next line into\n> >\n> > return TestFail;\n> >\n> > > +\n> > > + return TestPass;\n> > > + }\n> > > +\n> > > +private:\n> > > + GstElement *pipeline_, *libcamera_src_, *convert0_, *sink0_;\n> >\n> > One variable per line please:\n> >\n> > GstElement *pipeline_;\n> > GstElement *libcamera_src_;\n> > GstElement *convert0_;\n> > GstElement *sink0_;\n> >\n> > > + GstBus *bus_;\n> > > + GstMessage *msg_;\n> > > + GstStateChangeReturn ret_;\n> > > +\n> > > + void gstreamer_print_error(GstMessage *msg)\n> > > + {\n> > > + GError *err;\n> > > + gchar *debug_info;\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> > > + g_clear_error(&err);\n> > > + g_free(debug_info);\n> > > + }\n> >\n> > We usually put functions before variables.\n> >\n> > > +};\n> > > +\n> > > +TEST_REGISTER(GstreamerSingleStreamTest)\n> > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > new file mode 100644\n> > > index 00000000..b99aa0da\n> > > --- /dev/null\n> > > +++ b/test/gstreamer/meson.build\n> > > @@ -0,0 +1,19 @@\n> > > +# SPDX-License-Identifier: CC0-1.0\n> > > +\n> > > +if not gst_enabled\n> > > + subdir_done()\n> > > +endif\n> > > +\n> > > +gstreamer_tests = [\n> > > + ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> > > +]\n> > > +gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > > +\n> > > +foreach t : gstreamer_tests\n> > > + exe = executable(t[0], t[1],\n> > > + dependencies : [libcamera_private, gstreamer_dep],\n> > > + link_with : test_libraries,\n> > > + include_directories : test_includes_internal)\n> > > +\n> > > + test(t[0], exe, suite : 'gstreamer', is_parallel : false)\n> > > +endforeach\n> > > diff --git a/test/meson.build b/test/meson.build\n> > > index 3bceb5df..d0466f17 100644\n> > > --- a/test/meson.build\n> > > +++ b/test/meson.build\n> > > @@ -11,6 +11,7 @@ subdir('libtest')\n> > >\n> > > subdir('camera')\n> > > subdir('controls')\n> > > +subdir('gstreamer')\n> > > subdir('ipa')\n> > > subdir('ipc')\n> > > subdir('log')","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 E0907BD87D\n\tfor ;\n\tFri, 13 Aug 2021 11:56:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53EAF6888D;\n\tFri, 13 Aug 2021 13:56:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB23A687FA\n\tfor ;\n\tFri, 13 Aug 2021 13:56:44 +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 5AF3D4A1;\n\tFri, 13 Aug 2021 13:56:44 +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=\"PwXeq8sw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628855804;\n\tbh=ebw8BR2DBntIhUrtvkS7M/Jb/1uuTHkUBGMWLBvWw1k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PwXeq8swGM4LEb+XtTwzeMuwJ0LbxhET5LD7zmsorpyNrn0QPodqYIsuy0LMI5giU\n\t7iNYrr8xupFE8TswqHZD9r1+nyYtz+uV8neN9WWFsu4pcvmFBZ8yuCdLVxRCrxfGve\n\tLvqV+6s4h3BQ9u7shXgnnOgos0Pzlf3sup2qhajQ=","Date":"Fri, 13 Aug 2021 14:56:39 +0300","From":"Laurent Pinchart ","To":"Vedant Paranjape ","Message-ID":"","References":"<20210812104030.557246-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 v7] test: gstreamer: Add test for\n\tgstreamer single 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":18769,"web_url":"https://patchwork.libcamera.org/comment/18769/","msgid":"","date":"2021-08-13T12:15:11","subject":"Re: [libcamera-devel] [PATCH v7] test: gstreamer: Add test for\n\tgstreamer single stream","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Laurent,\nI tried doing this, it threw a compiler error:\nhttps://paste.debian.net/1207499/\nI assume it must have something to do with vars being defined in a class.\nAnd g_autoptr uses the cleanup attribute.\n\nRegards,\nVedant\n\nOn Fri, Aug 13, 2021 at 5:26 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Vedant,\n>\n> On Fri, Aug 13, 2021 at 11:44:19AM +0530, Vedant Paranjape wrote:\n> > On Fri, Aug 13, 2021 at 5:55 AM Laurent Pinchart wrote:\n> > > On Thu, Aug 12, 2021 at 04:10:30PM +0530, Vedant Paranjape wrote:\n> > > > This patch adds a test to test if single stream using\n> > > > libcamera's gstreamer element works.\n> > > >\n> > > > Signed-off-by: Vedant Paranjape \n> > > > Reviewed-by: Paul Elder \n> > > > ---\n> > > > .../gstreamer_single_stream_test.cpp | 134\n> ++++++++++++++++++\n> > > > test/gstreamer/meson.build | 19 +++\n> > > > test/meson.build | 1 +\n> > > > 3 files changed, 154 insertions(+)\n> > > > create mode 100644 test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > create mode 100644 test/gstreamer/meson.build\n> > > >\n> > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > new file mode 100644\n> > > > index 00000000..040b1e7e\n> > > > --- /dev/null\n> > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > @@ -0,0 +1,134 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > + *\n> > > > + * ipa_interface_test.cpp - Test the IPA interface\n> > > > + */\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"test.h\"\n> > > > +\n> > > > +using namespace std;\n> > > > +\n> > > > +class GstreamerSingleStreamTest : public Test\n> > > > +{\n> > > > +protected:\n> > > > + int init() override\n> > > > + {\n> > > > + /* Initialize GStreamer */\n> > > > + GError *err_init = nullptr;\n> > >\n> > > Let's use the libcamera coding style, this should be errInit. Same for\n> > > libcamera_src_.\n> > >\n> > > > + if (!gst_init_check(nullptr, nullptr, &err_init)) {\n> > > > + g_printerr(\"Could not initialize GStreamer:\n> %s\\n\",\n> > > > + err_init ? err_init->message :\n> \"unknown error\");\n> > > > + if (err_init)\n> > > > + g_error_free(err_init);\n> > > > +\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + if (!gst_registry_scan_path(gst_registry_get(),\n> > > > +\n> > > std::string(libcamera::utils::libcameraBuildPath() +\n> > > > +\n> \"src/gstreamer\")\n> > > > + .c_str())) {\n> > >\n> > > Let's use a local variable to make this a bit more readable.\n> > >\n> > > std::string path =\n> libcamera::utils::libcameraBuildPath()\n> > > + \"src/gstreamer\";\n> > > if (!gst_registry_scan_path(gst_registry_get(),\n> path.c_str()) {\n> > >\n> > > > + g_printerr(\"Failed to add plugin to\n> registry\\n\");\n> > >\n> > > You need to call gst_deinit() here.\n> > >\n> > > > + return TestFail;\n> > > > + }\n> > >\n> > > This won't work if the libcamerasrc plugin is already installed on the\n> > > system, gst_registry_scan_path() will not add anything in that case.\n> You\n> > > need to remove the plugin first. I've tested the following code:\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> > > > +\n> > > > + /* Create the elements */\n> > > > + libcamera_src_ =\n> gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > > > + convert0_ = gst_element_factory_make(\"videoconvert\",\n> \"convert0\");\n> > > > + sink0_ = gst_element_factory_make(\"fakevideosink\",\n> \"sink0\");\n> > > > +\n> > > > + /* Create the empty pipeline_ */\n> > > > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > +\n> > > > + if (!pipeline_ || !convert0_ || !sink0_ ||\n> !libcamera_src_) {\n> > > > + g_printerr(\"Not all elements could be\n> created.\\n\");\n> > >\n> > > cleanup() isn't called if init() fails, so you need to unref pipeline_,\n> > > but also libcamera_src_, convert0_ and sink0_ here. The last three\n> > > elements don't need to be unrefered in cleanup(), because\n> > > gst_bin_add_many() makes the pipeline take ownership of them.\n> >\n> > I could make all those pointers g_autoptr, right ?\n>\n> I think so, at least if they can be helpful, I'm not a glib specialist\n> :-)\n>\n> > > gst_deinit() is also needed here.\n> > >\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + return TestPass;\n> > > > + }\n> > > > +\n> > > > + void cleanup() override\n> > > > + {\n> > > > + gst_object_unref(bus_);\n> > > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > >\n> > > I'd move this call to the run() function, just before the\n> > > /* Parse error message */ comment, as it shouldn't be executed if the\n> > > pipeline isn't playing.\n> > >\n> > > > + gst_object_unref(pipeline_);\n> > > > + gst_deinit();\n> > > > + }\n> > > > +\n> > > > + int run() override\n> > > > + {\n> > > > + /* Build the pipeline */\n> > > > + gst_bin_add_many(GST_BIN(pipeline_), libcamera_src_,\n> convert0_, sink0_, NULL);\n> > > > + if (gst_element_link_many(libcamera_src_, convert0_,\n> sink0_, NULL) != TRUE) {\n> > > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > > + gst_object_unref(pipeline_);\n> > >\n> > > This will cause a double unref, as cleanup() will be called.\n> > >\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + /* Start playing */\n> > > > + ret_ = gst_element_set_state(pipeline_,\n> GST_STATE_PLAYING);\n> > >\n> > > This can be a local variable.\n> > >\n> > > > + if (ret_ == GST_STATE_CHANGE_FAILURE) {\n> > > > + g_printerr(\"Unable to set the pipeline to the\n> playing state.\\n\");\n> > > > + gst_object_unref(pipeline_);\n> > >\n> > > Same here, double unref.\n> > >\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + /* Wait until error or EOS or timeout after 2 seconds\n> */\n> > > > + GstClockTime timeout = 2000000000;\n> > > > + bus_ = gst_element_get_bus(pipeline_);\n> > > > + msg_ = gst_bus_timed_pop_filtered(bus_, timeout,\n> > >\n> > > Both bus and msg can be local variables. Otherwise, for bus_, you'd\n> have\n> > > to initialize it to nullptr, or cleanup() will crash if one of the\n> above\n> > > two conditions is false.\n> > >\n> > > > +\n> > > GstMessageType((uint)GST_MESSAGE_ERROR | (uint)GST_MESSAGE_EOS));\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> > > > + return TestFail;\n> > > > + break;\n> > > > + case GST_MESSAGE_EOS:\n> > > > + g_print(\"End-Of-Stream reached.\\n\");\n> > > > + return TestFail;\n> > > > + break;\n> > > > + default:\n> > > > + g_printerr(\"Unexpected message received.\\n\");\n> > > > + return TestFail;\n> > > > + break;\n> > > > + }\n> > >\n> > > A blank line would be nice here.\n> > >\n> > > > + gst_message_unref(msg_);\n> > >\n> > > This will never be called, as all the cases above return. Furthermore,\n> > > msg_ is leaked in all those cases. I'd remove the return statements,\n> and\n> > > turn the next line into\n> > >\n> > > return TestFail;\n> > >\n> > > > +\n> > > > + return TestPass;\n> > > > + }\n> > > > +\n> > > > +private:\n> > > > + GstElement *pipeline_, *libcamera_src_, *convert0_, *sink0_;\n> > >\n> > > One variable per line please:\n> > >\n> > > GstElement *pipeline_;\n> > > GstElement *libcamera_src_;\n> > > GstElement *convert0_;\n> > > GstElement *sink0_;\n> > >\n> > > > + GstBus *bus_;\n> > > > + GstMessage *msg_;\n> > > > + GstStateChangeReturn ret_;\n> > > > +\n> > > > + void gstreamer_print_error(GstMessage *msg)\n> > > > + {\n> > > > + GError *err;\n> > > > + gchar *debug_info;\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> > > > + g_clear_error(&err);\n> > > > + g_free(debug_info);\n> > > > + }\n> > >\n> > > We usually put functions before variables.\n> > >\n> > > > +};\n> > > > +\n> > > > +TEST_REGISTER(GstreamerSingleStreamTest)\n> > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > new file mode 100644\n> > > > index 00000000..b99aa0da\n> > > > --- /dev/null\n> > > > +++ b/test/gstreamer/meson.build\n> > > > @@ -0,0 +1,19 @@\n> > > > +# SPDX-License-Identifier: CC0-1.0\n> > > > +\n> > > > +if not gst_enabled\n> > > > + subdir_done()\n> > > > +endif\n> > > > +\n> > > > +gstreamer_tests = [\n> > > > + ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> > > > +]\n> > > > +gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > > > +\n> > > > +foreach t : gstreamer_tests\n> > > > + exe = executable(t[0], t[1],\n> > > > + dependencies : [libcamera_private,\n> gstreamer_dep],\n> > > > + link_with : test_libraries,\n> > > > + include_directories : test_includes_internal)\n> > > > +\n> > > > + test(t[0], exe, suite : 'gstreamer', is_parallel : false)\n> > > > +endforeach\n> > > > diff --git a/test/meson.build b/test/meson.build\n> > > > index 3bceb5df..d0466f17 100644\n> > > > --- a/test/meson.build\n> > > > +++ b/test/meson.build\n> > > > @@ -11,6 +11,7 @@ subdir('libtest')\n> > > >\n> > > > subdir('camera')\n> > > > subdir('controls')\n> > > > +subdir('gstreamer')\n> > > > subdir('ipa')\n> > > > subdir('ipc')\n> > > > subdir('log')\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 B3A78C3240\n\tfor ;\n\tFri, 13 Aug 2021 12:15:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18D2968892;\n\tFri, 13 Aug 2021 14:15:26 +0200 (CEST)","from mail-yb1-xb29.google.com (mail-yb1-xb29.google.com\n\t[IPv6:2607:f8b0:4864:20::b29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F8D1687FA\n\tfor ;\n\tFri, 13 Aug 2021 14:15:25 +0200 (CEST)","by mail-yb1-xb29.google.com with SMTP id m193so18385637ybf.9\n\tfor ;\n\tFri, 13 Aug 2021 05:15:25 -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=\"e5vlmgTm\"; 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=jcPGQQxN2OGuwAoggK6ubnl4qKCplrvc1Heh8kKQXy8=;\n\tb=e5vlmgTmJMIOsYh0IU5eqcMV/Ywg8c4kQjwS2eQ9+EY6FbsJylLt8A5IS41X3zDe4i\n\tWsPGQBrJRB+fKV2+Pf5mFKj2dDMzATvhY5O8AOMpd36vlVfadCW9IQlgjz9qEjehp0M/\n\tBBJHXTuRiwSv7WergvwfmaRAb8YVVmHLgSVQ6VYnOM1beACenxaGjNf20CRnaancg/Wz\n\t3iRpT9l70uMTzUmnji+PyPZ/vr04ULeL07sCftBGk7/JCKQEn79R5zRRgJhyXir5wOyU\n\tTiTMJlVSMRdjmfTKsTPqcYIxX3s1Jwr+Xx1gyV7fFaKn+CVHkfAurVlMjscV/sMbOG1N\n\tQmRw==","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=jcPGQQxN2OGuwAoggK6ubnl4qKCplrvc1Heh8kKQXy8=;\n\tb=bJxZLHxjwnhNrzAkYF3CLc6dHScEZM8NAdK1a8AvVq0+AjDJdum9RLNy/t7AI1zplT\n\tZmlsY+LkbyrZM0aBqLPTfwcsVIXtJRNS/X4GBvnPvBu04xstsSiOvN/WAWFwUN6+2on7\n\tv2MXa+tXsTuMz9wFgLVuodyWARXOQHgNxJ5HcybCoLulwDbYAyVHFcpnPocupZcaJor3\n\tdLs8rlmehY7TUJoQGSf7QUdCpO/00N68VbiEW30wa3L+TGPjKYb7TLV4IXyA17T2bO8z\n\tmAqgqWX1akxu4FcFIr/ynZ1ce6kvxnJVf3T8ThhLynDPZKbRbmI9JgzValnhUgZ54eVe\n\tivQQ==","X-Gm-Message-State":"AOAM532IXpHjj40cO9vlCYSaO0pGv+mPzptqeNaGRUNbqRO/qW32b45V\n\tCVCs9WBMSAxYRlbHL7Ny2Q08aArr6zRfqEvv+Y8=","X-Google-Smtp-Source":"ABdhPJxQafGYPVw9Ru5w3Zio4FbZdJpXxWO24Da2GWJqR4GwENkAkvKcH9dsNysU0fCKAg+9E1vHyrt7oh8el3lOS2g=","X-Received":"by 2002:a25:d6c6:: with SMTP id\n\tn189mr2693650ybg.432.1628856923920; \n\tFri, 13 Aug 2021 05:15:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20210812104030.557246-1-vedantparanjape160201@gmail.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Vedant Paranjape ","Date":"Fri, 13 Aug 2021 17:45:11 +0530","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000571ba905c96fcef5\"","Subject":"Re: [libcamera-devel] [PATCH v7] test: gstreamer: Add test for\n\tgstreamer single 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\" "}}]