[{"id":19222,"web_url":"https://patchwork.libcamera.org/comment/19222/","msgid":"<4c8aba66992febb60e9003bb6037295518c12c48.camel@ndufresne.ca>","date":"2021-08-31T20:29:21","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 01 septembre 2021 à 01:32 +0530, Vedant Paranjape a écrit :\n> Lot of code used in single stream test is boiler plate and common across\n> every gstreamer test. Factored out this code into a base class called\n> GstreamerTest.\n> \n> Also updated the gstreamer_single_stream_test to use the GstreamerTest\n> base class\n> \n> Signed-off-by: Vedant Paranjape \n\nFor my part, this looks like a good start, we will likely modify and adapt test\nwhile adding few more and it will get more stable later.\n\nReviewed-by: Nicolas Dufresne \n\np.s. I'm starting a discussion inline about the test design itself, why they\nneed to 2s instead of stop whenever a success condition is met. But this isn't a\nchange from the already merged test, so not a blocker for this.\n\n> ---\n> .../gstreamer_single_stream_test.cpp | 145 +++-------------\n> test/gstreamer/gstreamer_test.cpp | 157 ++++++++++++++++++\n> test/gstreamer/gstreamer_test.h | 39 +++++\n> test/gstreamer/meson.build | 2 +-\n> 4 files changed, 221 insertions(+), 122 deletions(-)\n> create mode 100644 test/gstreamer/gstreamer_test.cpp\n> create mode 100644 test/gstreamer/gstreamer_test.h\n> \n> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> index 4c8d4804..c27e4d36 100644\n> --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> @@ -14,104 +14,48 @@\n> \n> #include \n> \n> +#include \"gstreamer_test.h\"\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> +class GstreamerSingleStreamTest : public GstreamerTest, public Test\n> {\n> +public:\n> +\tGstreamerSingleStreamTest()\n> +\t\t: GstreamerTest()\n> +\t{\n> +\t}\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> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n> \n> -\t\t\treturn TestFail;\n> -\t\t}\n> +\t\tg_autoptr(GstElement) convert0__ = gst_element_factory_make(\"videoconvert\", \"convert0\");\n> +\t\tg_autoptr(GstElement) sink0__ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> +\t\tg_object_ref_sink(convert0__);\n> +\t\tg_object_ref_sink(sink0__);\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> +\t\tif (!convert0__ || !sink0__) {\n> +\t\t\tg_printerr(\"Not all elements could be created. %p.%p\\n\",\n> +\t\t\t\t convert0__, sink0__);\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> -\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\tsink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> -\n> -\t\t/* Create the empty pipeline_ */\n> -\t\tpipeline_ = gst_pipeline_new(\"test-pipeline\");\n> -\n> -\t\tif (!pipeline_ || !convert0_ || !sink0_ || !libcameraSrc_) {\n> -\t\t\tg_printerr(\"Not all elements could be created. %p.%p.%p.%p\\n\",\n> -\t\t\t\t pipeline_, convert0_, sink0_, 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 (sink0_)\n> -\t\t\t\tgst_object_unref(sink0_);\n> -\t\t\tif (libcameraSrc_)\n> -\t\t\t\tgst_object_unref(libcameraSrc_);\n> -\t\t\tgst_deinit();\n> +\t\tconvert0_ = reinterpret_cast(g_steal_pointer(&convert0__));\n> +\t\tsink0_ = reinterpret_cast(g_steal_pointer(&sink0__));\n> \n> +\t\tif (gstreamer_create_pipeline() != TestPass)\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_, convert0_, sink0_, NULL);\n> \t\tif (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) {\n> @@ -119,57 +63,16 @@ protected:\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> -\t\tif (ret == GST_STATE_CHANGE_FAILURE) {\n> -\t\t\tg_printerr(\"Unable to set the pipeline to the playing state.\\n\");\n> +\t\tif (gstreamer_start_pipeline() != TestPass)\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> +\t\tif (gstreamer_process_event() != TestPass)\n> +\t\t\treturn TestFail;\n> \n> -\t\treturn TestFail;\n> +\t\treturn TestPass;\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 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}\n> -\n> -\tGstElement *pipeline_;\n> -\tGstElement *libcameraSrc_;\n> \tGstElement *convert0_;\n> \tGstElement *sink0_;\n> };\n> diff --git a/test/gstreamer/gstreamer_test.cpp b/test/gstreamer/gstreamer_test.cpp\n> new file mode 100644\n> index 00000000..22128c4c\n> --- /dev/null\n> +++ b/test/gstreamer/gstreamer_test.cpp\n> @@ -0,0 +1,157 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Vedant Paranjape\n> + *\n> + * libcamera Gstreamer element API tests\n> + */\n> +\n> +#include \"gstreamer_test.h\"\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> +GstreamerTest::GstreamerTest()\n> +{\n> +\t/*\n> +\t* GStreamer by default spawns a process to run the\n> +\t* gst-plugin-scanner helper. If libcamera is compiled with ASan\n> +\t* enabled, and as GStreamer is most likely not, this causes the\n> +\t* ASan link order check to fail when gst-plugin-scanner\n> +\t* dlopen()s the plugin as many libraries will have already been\n> +\t* loaded by then. Fix this issue by disabling spawning of a\n> +\t* child helper process when scanning the build directory for\n> +\t* plugins.\n> +\t*/\n> +\tgst_registry_fork_set_enabled(false);\n> +\n> +\t/* Initialize GStreamer */\n> +\tg_autoptr(GError) errInit = NULL;\n> +\tif (!gst_init_check(nullptr, nullptr, &errInit)) {\n> +\t\tg_printerr(\"Could not initialize GStreamer: %s\\n\",\n> +\t\t\t errInit ? errInit->message : \"unknown error\");\n> +\n> +\t\tstatus_ = TestFail;\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t* Remove the system libcamera plugin, if any, and add the\n> +\t* plugin from the build directory.\n> +\t*/\n> +\tGstRegistry *registry = gst_registry_get();\n> +\tg_autoptr(GstPlugin) plugin = gst_registry_lookup(registry, \"libgstlibcamera.so\");\n> +\tif (plugin) {\n> +\t\tgst_registry_remove_plugin(registry, plugin);\n> +\t}\n> +\n> +\tstd::string path = libcamera::utils::libcameraBuildPath() + \"src/gstreamer\";\n> +\tif (!gst_registry_scan_path(registry, path.c_str())) {\n> +\t\tg_printerr(\"Failed to add plugin to registry\\n\");\n> +\n> +\t\tstatus_ = TestFail;\n> +\t\treturn;\n> +\t}\n> +\n> +\tstatus_ = TestPass;\n> +}\n> +\n> +GstreamerTest::~GstreamerTest()\n> +{\n> +\tif (pipeline_)\n> +\t\tgst_object_unref(pipeline_);\n> +\tif (status_ == TestFail) {\n> +\t\tgst_object_unref(libcameraSrc_);\n> +\t}\n> +\n> +\tgst_deinit();\n> +}\n> +\n> +int GstreamerTest::gstreamer_create_pipeline()\n> +{\n> +\tg_autoptr(GstElement) libcameraSrc__ = gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> +\tpipeline_ = gst_pipeline_new(\"test-pipeline\");\n> +\tg_object_ref_sink(libcameraSrc__);\n> +\n> +\tif (!libcameraSrc__ || !pipeline_) {\n> +\t\tg_printerr(\"Unable to create create pipeline %p.%p\\n\",\n> +\t\t\t\t\t\tlibcameraSrc__, pipeline_);\n> +\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tlibcameraSrc_ = reinterpret_cast(g_steal_pointer(&libcameraSrc__));\n> +\n> +\treturn TestPass;\n> +}\n> +\n> +int GstreamerTest::gstreamer_start_pipeline()\n> +{\n> +\tGstStateChangeReturn ret;\n> +\n> +\t/* Start playing */\n> +\tret = gst_element_set_state(pipeline_, GST_STATE_PLAYING);\n> +\tif (ret == GST_STATE_CHANGE_FAILURE) {\n> +\t\tg_printerr(\"Unable to set the pipeline to the playing state.\\n\");\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\treturn TestPass;\n> +}\n> +\n> +int GstreamerTest::gstreamer_process_event()\n> +{\n> +\t/* Wait until error or EOS or timeout after 2 seconds */\n> +\tconstexpr GstMessageType msgType =\n> +\t\tstatic_cast(GST_MESSAGE_ERROR | GST_MESSAGE_EOS);\n> +\tconstexpr GstClockTime timeout = 2 * GST_SECOND;\n> +\n> +\tg_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);\n> +\tg_autoptr(GstMessage) msg = gst_bus_timed_pop_filtered(bus, timeout, msgType);\n> +\n> +\tgst_element_set_state(pipeline_, GST_STATE_NULL);\n> +\n> +\t/* Parse error message */\n> +\tif (msg == NULL) {\n> +\t\treturn TestPass;\n> +\t}\n\nIsn't it unfortunate design that all test must last 2s ? Just opening the\ndiscussion, this was like this before.\n\n> +\n> +\tswitch (GST_MESSAGE_TYPE(msg)) {\n> +\tcase GST_MESSAGE_ERROR:\n> +\t\tgstreamer_print_error(msg);\n> +\t\tbreak;\n> +\tcase GST_MESSAGE_EOS:\n> +\t\tg_print(\"End-Of-Stream reached.\\n\");\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tg_printerr(\"Unexpected message received.\\n\");\n> +\t\tbreak;\n> +\t}\n> +\n> +\treturn TestFail;\n> +}\n> +\n> +void GstreamerTest::gstreamer_print_error(GstMessage *msg)\n> +{\n> +\tg_autoptr(GError) err = NULL;\n> +\tg_autofree gchar *debug_info = NULL;\n> +\n> +\tgst_message_parse_error(msg, &err, &debug_info);\n> +\tg_printerr(\"Error received from element %s: %s\\n\",\n> +\t\t GST_OBJECT_NAME(msg->src), err->message);\n> +\tg_printerr(\"Debugging information: %s\\n\",\n> +\t\t debug_info ? debug_info : \"none\");\n> +}\n> +\n> diff --git a/test/gstreamer/gstreamer_test.h b/test/gstreamer/gstreamer_test.h\n> new file mode 100644\n> index 00000000..cdffdea9\n> --- /dev/null\n> +++ b/test/gstreamer/gstreamer_test.h\n> @@ -0,0 +1,39 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Vedant Paranjape\n> + *\n> + * gstreamer_test.cpp - GStreamer test base class\n> + */\n> +\n> +#ifndef __LIBCAMERA_GSTREAMER_TEST_H__\n> +#define __LIBCAMERA_GSTREAMER_TEST_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/source_paths.h\"\n> +\n> +#include \n> +\n> +using namespace std;\n> +\n> +class GstreamerTest\n> +{\n> +public:\n> +\tGstreamerTest();\n> +\tvirtual ~GstreamerTest();\n> +\n> +protected:\n> +\tvirtual int gstreamer_create_pipeline();\n> +\tint gstreamer_start_pipeline();\n> +\tint gstreamer_process_event();\n> +\tvoid gstreamer_print_error(GstMessage *msg);\n> +\n> +\tGstElement *pipeline_;\n> +\tGstElement *libcameraSrc_;\n> +\tint status_;\n> +};\n> +\n> +#endif /* __LIBCAMERA_GSTREAMER_TEST_H__ */\n> diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> index b99aa0da..aca53b92 100644\n> --- a/test/gstreamer/meson.build\n> +++ b/test/gstreamer/meson.build\n> @@ -10,7 +10,7 @@ gstreamer_tests = [\n> gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> \n> foreach t : gstreamer_tests\n> - exe = executable(t[0], t[1],\n> + exe = executable(t[0], t[1], 'gstreamer_test.cpp',\n> dependencies : [libcamera_private, gstreamer_dep],\n> link_with : test_libraries,\n> include_directories : test_includes_internal)","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 8903FBDC71\n\tfor ;\n\tTue, 31 Aug 2021 20:29:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC0596916D;\n\tTue, 31 Aug 2021 22:29:26 +0200 (CEST)","from mail-il1-x12c.google.com (mail-il1-x12c.google.com\n\t[IPv6:2607:f8b0:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CFE1B68890\n\tfor ;\n\tTue, 31 Aug 2021 22:29:24 +0200 (CEST)","by mail-il1-x12c.google.com with SMTP id z2so680196iln.0\n\tfor ;\n\tTue, 31 Aug 2021 13:29:24 -0700 (PDT)","from nicolas-tpx395.localdomain (mtl.collabora.ca. [66.171.169.34])\n\tby smtp.gmail.com with ESMTPSA id\n\ty12sm5793878ior.43.2021.08.31.13.29.22\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 31 Aug 2021 13:29:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20150623.gappssmtp.com\n\theader.i=@ndufresne-ca.20150623.gappssmtp.com\n\theader.b=\"xE9ohDG3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:date:in-reply-to:references:user-agent\n\t:mime-version:content-transfer-encoding;\n\tbh=IWC9CqhTPg9GPc4A6MZMQQHo+R2zNCyTxQoWMGZSIrg=;\n\tb=xE9ohDG3UKzDPCjRTuSVB1v3vhnrjjZAC1wlVfBnnC3iHuc5vvAb6uMdxasBFDjeUg\n\tITJDQHfJpTzfjhZn4ZXHeDcl5uMjMk+tgyi/+xotMaCjNkftkDHCm3iiRJo+hUzbppmQ\n\tl8FMGyWM22Ns+TiRALbK31hWYSoCOeIp2sHRbBZgG+X2QGnTIS8HGe9+DiHqZuJQNGs2\n\t2K6l9hJF66KU/9rrMtSSE5pInTl5WnXD6COP0cPiTkSREx+nsQO6Fpq3Zx9FIwmSQPnZ\n\tWD4redlJahSGhkiMz1KzssLYHHVAH7MGnYFn7iFdG7aHnkDrHb8RFhG5ndmFLDG2HAaa\n\tZGSg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=IWC9CqhTPg9GPc4A6MZMQQHo+R2zNCyTxQoWMGZSIrg=;\n\tb=J5xTr9GLFjyhb2VyEheSsSoPAXf5NXZ5s3tUGR8RhjZRdt37cUlUdZOaIIF1XTVVdO\n\t/9vJYMi+j4X4eM7pHvq1wqlRphj720KPS0azG2p9o04yKRaN+rSSJAfU13NEJI8BwLoq\n\tP9gY5NeGFcjnrN6G/X6SuRrPh8fVqtbqb9/vRq17GPArRZqBv2aRTcwhSc7VR41Tu+LW\n\twp2rOOJ0X+M05eEkvcVTu/77ME2RIgCl+cKLnltR6VYdrztwXuiB409DNj6yyghKH9oP\n\tl1eTQTcW9k0i5MWZJBCCll4dwyHWyuoSDvFTB+NikCNZnp/Ueg4DmixP7AUGKHzyUw+w\n\tAHVQ==","X-Gm-Message-State":"AOAM532n7GhkNbMVb+ET/EA6XeVE8c7xV9kBmFDbatwxuwfT4PrRHvt4\n\t44eaUV9qgq5npeLpZNyHQVKugQ==","X-Google-Smtp-Source":"ABdhPJxk8kHV0bIuwQKoXaHw+mxyRa+FbesQwypSaTr3zbps8wwWvBipO9vAow9VHAIcFtF1dZHxFw==","X-Received":"by 2002:a92:c841:: with SMTP id\n\tb1mr21964277ilq.300.1630441763508; \n\tTue, 31 Aug 2021 13:29:23 -0700 (PDT)","Message-ID":"<4c8aba66992febb60e9003bb6037295518c12c48.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Vedant Paranjape , \n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 31 Aug 2021 16:29:21 -0400","In-Reply-To":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>","References":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.40.4 (3.40.4-1.fc34) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19224,"web_url":"https://patchwork.libcamera.org/comment/19224/","msgid":"","date":"2021-08-31T20:34:21","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"On Wed, 1 Sep, 2021, 01:59 Nicolas Dufresne, wrote:\n\n> Le mercredi 01 septembre 2021 à 01:32 +0530, Vedant Paranjape a écrit :\n> > Lot of code used in single stream test is boiler plate and common across\n> > every gstreamer test. Factored out this code into a base class called\n> > GstreamerTest.\n> >\n> > Also updated the gstreamer_single_stream_test to use the GstreamerTest\n> > base class\n> >\n> > Signed-off-by: Vedant Paranjape \n>\n> For my part, this looks like a good start, we will likely modify and adapt\n> test\n> while adding few more and it will get more stable later.\n>\n> Reviewed-by: Nicolas Dufresne \n>\n> p.s. I'm starting a discussion inline about the test design itself, why\n> they\n> need to 2s instead of stop whenever a success condition is met. But this\n> isn't a\n> change from the already merged test, so not a blocker for this.\n>\n\nI was the one who made it timeout at 2s, as without timeout the test kept\non running forever, what's the success condition here?\n\n> ---\n> > .../gstreamer_single_stream_test.cpp | 145 +++-------------\n> > test/gstreamer/gstreamer_test.cpp | 157 ++++++++++++++++++\n> > test/gstreamer/gstreamer_test.h | 39 +++++\n> > test/gstreamer/meson.build | 2 +-\n> > 4 files changed, 221 insertions(+), 122 deletions(-)\n> > create mode 100644 test/gstreamer/gstreamer_test.cpp\n> > create mode 100644 test/gstreamer/gstreamer_test.h\n> >\n> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > index 4c8d4804..c27e4d36 100644\n> > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > @@ -14,104 +14,48 @@\n> >\n> > #include \n> >\n> > +#include \"gstreamer_test.h\"\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> > +class GstreamerSingleStreamTest : public GstreamerTest, public Test\n> > {\n> > +public:\n> > + GstreamerSingleStreamTest()\n> > + : GstreamerTest()\n> > + {\n> > + }\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> > + if (status_ != TestPass)\n> > + return status_;\n> >\n> > - return TestFail;\n> > - }\n> > + g_autoptr(GstElement) convert0__ =\n> gst_element_factory_make(\"videoconvert\", \"convert0\");\n> > + g_autoptr(GstElement) sink0__ =\n> gst_element_factory_make(\"fakesink\", \"sink0\");\n> > + g_object_ref_sink(convert0__);\n> > + g_object_ref_sink(sink0__);\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> > + if (!convert0__ || !sink0__) {\n> > + g_printerr(\"Not all elements could be created.\n> %p.%p\\n\",\n> > + convert0__, sink0__);\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> > - /* Create the elements */\n> > - libcameraSrc_ = gst_element_factory_make(\"libcamerasrc\",\n> \"libcamera\");\n> > - convert0_ = gst_element_factory_make(\"videoconvert\",\n> \"convert0\");\n> > - sink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > -\n> > - /* Create the empty pipeline_ */\n> > - pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > -\n> > - if (!pipeline_ || !convert0_ || !sink0_ || !libcameraSrc_)\n> {\n> > - g_printerr(\"Not all elements could be created.\n> %p.%p.%p.%p\\n\",\n> > - pipeline_, convert0_, sink0_,\n> libcameraSrc_);\n> > - if (pipeline_)\n> > - gst_object_unref(pipeline_);\n> > - if (convert0_)\n> > - gst_object_unref(convert0_);\n> > - if (sink0_)\n> > - gst_object_unref(sink0_);\n> > - if (libcameraSrc_)\n> > - gst_object_unref(libcameraSrc_);\n> > - gst_deinit();\n> > + convert0_ = reinterpret_cast *>(g_steal_pointer(&convert0__));\n> > + sink0_ = reinterpret_cast *>(g_steal_pointer(&sink0__));\n> >\n> > + if (gstreamer_create_pipeline() != TestPass)\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> convert0_, sink0_, NULL);\n> > if (gst_element_link_many(libcameraSrc_, convert0_,\n> sink0_, NULL) != TRUE) {\n> > @@ -119,57 +63,16 @@ protected:\n> > return TestFail;\n> > }\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> > + if (gstreamer_start_pipeline() != TestPass)\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> > + if (gstreamer_process_event() != TestPass)\n> > + return TestFail;\n> >\n> > - return TestFail;\n> > + return TestPass;\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 *convert0_;\n> > GstElement *sink0_;\n> > };\n> > diff --git a/test/gstreamer/gstreamer_test.cpp\n> b/test/gstreamer/gstreamer_test.cpp\n> > new file mode 100644\n> > index 00000000..22128c4c\n> > --- /dev/null\n> > +++ b/test/gstreamer/gstreamer_test.cpp\n> > @@ -0,0 +1,157 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Vedant Paranjape\n> > + *\n> > + * libcamera Gstreamer element API tests\n> > + */\n> > +\n> > +#include \"gstreamer_test.h\"\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> > +GstreamerTest::GstreamerTest()\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> > + status_ = TestFail;\n> > + return;\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> > + g_autoptr(GstPlugin) plugin = gst_registry_lookup(registry,\n> \"libgstlibcamera.so\");\n> > + if (plugin) {\n> > + gst_registry_remove_plugin(registry, 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> > +\n> > + status_ = TestFail;\n> > + return;\n> > + }\n> > +\n> > + status_ = TestPass;\n> > +}\n> > +\n> > +GstreamerTest::~GstreamerTest()\n> > +{\n> > + if (pipeline_)\n> > + gst_object_unref(pipeline_);\n> > + if (status_ == TestFail) {\n> > + gst_object_unref(libcameraSrc_);\n> > + }\n> > +\n> > + gst_deinit();\n> > +}\n> > +\n> > +int GstreamerTest::gstreamer_create_pipeline()\n> > +{\n> > + g_autoptr(GstElement) libcameraSrc__ =\n> gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > + g_object_ref_sink(libcameraSrc__);\n> > +\n> > + if (!libcameraSrc__ || !pipeline_) {\n> > + g_printerr(\"Unable to create create pipeline %p.%p\\n\",\n> > + libcameraSrc__, pipeline_);\n> > +\n> > + return TestFail;\n> > + }\n> > +\n> > + libcameraSrc_ = reinterpret_cast *>(g_steal_pointer(&libcameraSrc__));\n> > +\n> > + return TestPass;\n> > +}\n> > +\n> > +int GstreamerTest::gstreamer_start_pipeline()\n> > +{\n> > + GstStateChangeReturn ret;\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\n> state.\\n\");\n> > + return TestFail;\n> > + }\n> > +\n> > + return TestPass;\n> > +}\n> > +\n> > +int GstreamerTest::gstreamer_process_event()\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 = gst_bus_timed_pop_filtered(bus,\n> 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>\n> Isn't it unfortunate design that all test must last 2s ? Just opening the\n> discussion, this was like this before.\n>\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> > +void GstreamerTest::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> > diff --git a/test/gstreamer/gstreamer_test.h\n> b/test/gstreamer/gstreamer_test.h\n> > new file mode 100644\n> > index 00000000..cdffdea9\n> > --- /dev/null\n> > +++ b/test/gstreamer/gstreamer_test.h\n> > @@ -0,0 +1,39 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Vedant Paranjape\n> > + *\n> > + * gstreamer_test.cpp - GStreamer test base class\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_GSTREAMER_TEST_H__\n> > +#define __LIBCAMERA_GSTREAMER_TEST_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/source_paths.h\"\n> > +\n> > +#include \n> > +\n> > +using namespace std;\n> > +\n> > +class GstreamerTest\n> > +{\n> > +public:\n> > + GstreamerTest();\n> > + virtual ~GstreamerTest();\n> > +\n> > +protected:\n> > + virtual int gstreamer_create_pipeline();\n> > + int gstreamer_start_pipeline();\n> > + int gstreamer_process_event();\n> > + void gstreamer_print_error(GstMessage *msg);\n> > +\n> > + GstElement *pipeline_;\n> > + GstElement *libcameraSrc_;\n> > + int status_;\n> > +};\n> > +\n> > +#endif /* __LIBCAMERA_GSTREAMER_TEST_H__ */\n> > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > index b99aa0da..aca53b92 100644\n> > --- a/test/gstreamer/meson.build\n> > +++ b/test/gstreamer/meson.build\n> > @@ -10,7 +10,7 @@ gstreamer_tests = [\n> > gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> >\n> > foreach t : gstreamer_tests\n> > - exe = executable(t[0], t[1],\n> > + exe = executable(t[0], t[1], 'gstreamer_test.cpp',\n> > dependencies : [libcamera_private, gstreamer_dep],\n> > link_with : test_libraries,\n> > include_directories : test_includes_internal)\n>\n>\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 49FD0BD87D\n\tfor ;\n\tTue, 31 Aug 2021 20:34:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03D5769166;\n\tTue, 31 Aug 2021 22:34:35 +0200 (CEST)","from mail-yb1-xb32.google.com (mail-yb1-xb32.google.com\n\t[IPv6:2607:f8b0:4864:20::b32])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BCC4568890\n\tfor ;\n\tTue, 31 Aug 2021 22:34:33 +0200 (CEST)","by mail-yb1-xb32.google.com with SMTP id e133so959529ybh.0\n\tfor ;\n\tTue, 31 Aug 2021 13:34:33 -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=\"NmNOQfGe\"; 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=gNp0IK+1ziEZ6IZ1QIUWFjjGrbmvhuJvPSmOpdyxBxc=;\n\tb=NmNOQfGe9KaIscWeQD1mgA6u226G8xiGo76T0CWSj6sDpnEZ+1ytrjHdCHGZYs10/l\n\tZhCuWMOayJluPeY5s5Ni0MS2U0NiTM+8F9nIkkxgqn2QVpOJ4nOcUpxDN2x61tqvLoJB\n\tbuN4Ks2rAg5RU7iTUqJCaU1IHZx6WpSI6FdH6MM8wvEd24zR2nhktplJExlrypALXCgz\n\te3xqm8ET9raFLhzNu+Elyv7u39l7r3t1ZvCUsyLwgMgyYwdf35gsXVXwAD/8Y0oSKzdF\n\tLJ1Cz+qnCC+tv32M+/Se8SEsTRnVGBmuZ0nN2Wowt0kpbhZG+OLyuAKH5nti4MDmNnFU\n\tC4dQ==","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=gNp0IK+1ziEZ6IZ1QIUWFjjGrbmvhuJvPSmOpdyxBxc=;\n\tb=tUGF7fJXUUva9VradsLJg+f715i+BxIq1p96JVchlofFDosyjcpZlrhDVTIswoEgbE\n\t6ycNjABAX3GXDvHx5k3WUI5sLnKXdNW4xTv0wkXw0h8biwDN58XIEb19spaC4WX8fz/x\n\tXQ02uFwpeRcn9hfLqaW7jeoVvinanZOSwiH2Or6VVYzm2XnC5RL8bywP5CVqylj5ZZSk\n\tiNTB4jlnjRqzmXKfvNmf76/mhIU0BOWJWvltT+nOpRYZ/6dwqNtgZG1MAFkNPkPxW7on\n\tzGlqahQzcw7BNxI/8396LezjeHrQsyRALrCkl+/nfo2T0VaPZo7ds37MtoDrujQ3Flol\n\tspkA==","X-Gm-Message-State":"AOAM531faE53xF9/+i9FWN9T9ZHqoOIlpnqaz7oNUd5+Df44OH5G1Du1\n\t12YZ2RME0LMveTwnqCt7IeUeyYuD6HO1VsFnTax6JHRQtkrOjg==","X-Google-Smtp-Source":"ABdhPJwgjPGp3PEDFWZxXGBt4yEMibtYm1BUsAHkrjqUskx0g7T6o0gCY+tukwX6LcN5bQz7ACVSGNuvcLUV9Facl7Q=","X-Received":"by 2002:a25:d648:: with SMTP id\n\tn69mr32808497ybg.175.1630442072564; \n\tTue, 31 Aug 2021 13:34:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>\n\t<4c8aba66992febb60e9003bb6037295518c12c48.camel@ndufresne.ca>","In-Reply-To":"<4c8aba66992febb60e9003bb6037295518c12c48.camel@ndufresne.ca>","From":"Vedant Paranjape ","Date":"Wed, 1 Sep 2021 02:04:21 +0530","Message-ID":"","To":"Nicolas Dufresne ","Content-Type":"multipart/alternative; boundary=\"0000000000008fe83d05cae0e001\"","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","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":19226,"web_url":"https://patchwork.libcamera.org/comment/19226/","msgid":"","date":"2021-08-31T20:41:06","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 01 septembre 2021 à 02:04 +0530, Vedant Paranjape a écrit :\n> \n> \n> On Wed, 1 Sep, 2021, 01:59 Nicolas Dufresne, wrote:\n> > Le mercredi 01 septembre 2021 à 01:32 +0530, Vedant Paranjape a écrit :\n> > > Lot of code used in single stream test is boiler plate and common across\n> > > every gstreamer test. Factored out this code into a base class called\n> > > GstreamerTest.\n> > > \n> > > Also updated the gstreamer_single_stream_test to use the GstreamerTest\n> > > base class\n> > > \n> > > Signed-off-by: Vedant Paranjape \n> > \n> > For my part, this looks like a good start, we will likely modify and adapt\n> > test\n> > while adding few more and it will get more stable later.\n> > \n> > Reviewed-by: Nicolas Dufresne \n> > \n> > p.s. I'm starting a discussion inline about the test design itself, why they\n> > need to 2s instead of stop whenever a success condition is met. But this\n> > isn't\n> > a\n> > change from the already merged test, so not a blocker for this.\n> \n> I was the one who made it timeout at 2s, as without timeout the test kept on\n> running forever, what's the success condition here?\n\nThis is just a test run, I'd use pad probe, and after couple of non-zero frames,\nI'd stop and quit. This can be done with a GMainLoop, using g_main_loop_quit(),\nor using an application message, that get catched as success in your\nprocess_event() function.\n\nThe second way is thinner I believe.\n\n> \n> > > ---\n> > >   .../gstreamer_single_stream_test.cpp          | 145 +++-------------\n> > >   test/gstreamer/gstreamer_test.cpp             | 157 ++++++++++++++++++\n> > >   test/gstreamer/gstreamer_test.h               |  39 +++++\n> > >   test/gstreamer/meson.build                    |   2 +-\n> > >   4 files changed, 221 insertions(+), 122 deletions(-)\n> > >   create mode 100644 test/gstreamer/gstreamer_test.cpp\n> > >   create mode 100644 test/gstreamer/gstreamer_test.h\n> > > \n> > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > index 4c8d4804..c27e4d36 100644\n> > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > @@ -14,104 +14,48 @@\n> > >   \n> > >   #include \n> > >   \n> > > +#include \"gstreamer_test.h\"\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> > > +class GstreamerSingleStreamTest : public GstreamerTest, public Test\n> > >   {\n> > > +public:\n> > > +     GstreamerSingleStreamTest()\n> > > +             : GstreamerTest()\n> > > +     {\n> > > +     }\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\n> > ASan\n> > > -              * enabled, and as GStreamer is most likely not, this causes\n> > the\n> > > -              * ASan link order check to fail when gst-plugin-scanner\n> > > -              * dlopen()s the plugin as many libraries will have already\n> > been\n> > > -              * loaded by then. Fix this issue by disabling spawning of 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> > > +             if (status_ != TestPass)\n> > > +                     return status_;\n> > >   \n> > > -                     return TestFail;\n> > > -             }\n> > > +             g_autoptr(GstElement) convert0__ =\n> > gst_element_factory_make(\"videoconvert\", \"convert0\");\n> > > +             g_autoptr(GstElement) sink0__ =\n> > gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > +             g_object_ref_sink(convert0__);\n> > > +             g_object_ref_sink(sink0__);\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> > > +             if (!convert0__ || !sink0__) {\n> > > +                     g_printerr(\"Not all elements could be created.\n> > %p.%p\\n\",\n> > > +                                convert0__, sink0__);\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> > > -             /* Create the elements */\n> > > -             libcameraSrc_ = gst_element_factory_make(\"libcamerasrc\",\n> > \"libcamera\");\n> > > -             convert0_ = gst_element_factory_make(\"videoconvert\",\n> > \"convert0\");\n> > > -             sink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > -\n> > > -             /* Create the empty pipeline_ */\n> > > -             pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > -\n> > > -             if (!pipeline_ || !convert0_ || !sink0_ || !libcameraSrc_) {\n> > > -                     g_printerr(\"Not all elements could be created.\n> > %p.%p.%p.%p\\n\",\n> > > -                                pipeline_, convert0_, sink0_,\n> > libcameraSrc_);\n> > > -                     if (pipeline_)\n> > > -                             gst_object_unref(pipeline_);\n> > > -                     if (convert0_)\n> > > -                             gst_object_unref(convert0_);\n> > > -                     if (sink0_)\n> > > -                             gst_object_unref(sink0_);\n> > > -                     if (libcameraSrc_)\n> > > -                             gst_object_unref(libcameraSrc_);\n> > > -                     gst_deinit();\n> > > +             convert0_ = reinterpret_cast > *>(g_steal_pointer(&convert0__));\n> > > +             sink0_ = reinterpret_cast > *>(g_steal_pointer(&sink0__));\n> > >   \n> > > +             if (gstreamer_create_pipeline() != TestPass)\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> > > convert0_,\n> > sink0_, NULL);\n> > >                if (gst_element_link_many(libcameraSrc_, convert0_, sink0_,\n> > NULL) != TRUE) {\n> > > @@ -119,57 +63,16 @@ protected:\n> > >                        return TestFail;\n> > >                }\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\n> > state.\\n\");\n> > > +             if (gstreamer_start_pipeline() != TestPass)\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 = gst_bus_timed_pop_filtered(bus,\n> > 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> > > +             if (gstreamer_process_event() != TestPass)\n> > > +                     return TestFail;\n> > >   \n> > > -             return TestFail;\n> > > +             return TestPass;\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 *convert0_;\n> > >        GstElement *sink0_;\n> > >   };\n> > > diff --git a/test/gstreamer/gstreamer_test.cpp\n> > b/test/gstreamer/gstreamer_test.cpp\n> > > new file mode 100644\n> > > index 00000000..22128c4c\n> > > --- /dev/null\n> > > +++ b/test/gstreamer/gstreamer_test.cpp\n> > > @@ -0,0 +1,157 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Vedant Paranjape\n> > > + *\n> > > + * libcamera Gstreamer element API tests\n> > > + */\n> > > +\n> > > +#include \"gstreamer_test.h\"\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> > > +GstreamerTest::GstreamerTest()\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> > > +             status_ = TestFail;\n> > > +             return;\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> > > +     g_autoptr(GstPlugin) plugin = gst_registry_lookup(registry,\n> > \"libgstlibcamera.so\");\n> > > +     if (plugin) {\n> > > +             gst_registry_remove_plugin(registry, 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> > > +\n> > > +             status_ = TestFail;\n> > > +             return;\n> > > +     }\n> > > +\n> > > +     status_ = TestPass;\n> > > +}\n> > > +\n> > > +GstreamerTest::~GstreamerTest()\n> > > +{\n> > > +     if (pipeline_)\n> > > +             gst_object_unref(pipeline_);\n> > > +     if (status_ == TestFail) {\n> > > +             gst_object_unref(libcameraSrc_);\n> > > +     }\n> > > +\n> > > +     gst_deinit();\n> > > +}\n> > > +\n> > > +int GstreamerTest::gstreamer_create_pipeline()\n> > > +{\n> > > +     g_autoptr(GstElement) libcameraSrc__ =\n> > gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > > +     pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > +     g_object_ref_sink(libcameraSrc__);\n> > > +\n> > > +     if (!libcameraSrc__ || !pipeline_) {\n> > > +             g_printerr(\"Unable to create create pipeline %p.%p\\n\",\n> > > +                                             libcameraSrc__, pipeline_);\n> > > +\n> > > +             return TestFail;\n> > > +     }\n> > > +\n> > > +     libcameraSrc_ = reinterpret_cast > *>(g_steal_pointer(&libcameraSrc__));\n> > > +\n> > > +     return TestPass;\n> > > +}\n> > > +\n> > > +int GstreamerTest::gstreamer_start_pipeline()\n> > > +{\n> > > +     GstStateChangeReturn ret;\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\n> > state.\\n\");\n> > > +             return TestFail;\n> > > +     }\n> > > +\n> > > +     return TestPass;\n> > > +}\n> > > +\n> > > +int GstreamerTest::gstreamer_process_event()\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 = gst_bus_timed_pop_filtered(bus, timeout,\n> > 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> > \n> > Isn't it unfortunate design that all test must last 2s ? Just opening the\n> > discussion, this was like this before.\n> > \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> > > +void GstreamerTest::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> > > diff --git a/test/gstreamer/gstreamer_test.h\n> > b/test/gstreamer/gstreamer_test.h\n> > > new file mode 100644\n> > > index 00000000..cdffdea9\n> > > --- /dev/null\n> > > +++ b/test/gstreamer/gstreamer_test.h\n> > > @@ -0,0 +1,39 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Vedant Paranjape\n> > > + *\n> > > + * gstreamer_test.cpp - GStreamer test base class\n> > > + */\n> > > +\n> > > +#ifndef __LIBCAMERA_GSTREAMER_TEST_H__\n> > > +#define __LIBCAMERA_GSTREAMER_TEST_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/source_paths.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +using namespace std;\n> > > +\n> > > +class GstreamerTest\n> > > +{\n> > > +public:\n> > > +     GstreamerTest();\n> > > +     virtual ~GstreamerTest();\n> > > +\n> > > +protected:\n> > > +     virtual int gstreamer_create_pipeline();\n> > > +     int gstreamer_start_pipeline();\n> > > +     int gstreamer_process_event();\n> > > +     void gstreamer_print_error(GstMessage *msg);\n> > > +\n> > > +     GstElement *pipeline_;\n> > > +     GstElement *libcameraSrc_;\n> > > +     int status_;\n> > > +};\n> > > +\n> > > +#endif /* __LIBCAMERA_GSTREAMER_TEST_H__ */\n> > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > index b99aa0da..aca53b92 100644\n> > > --- a/test/gstreamer/meson.build\n> > > +++ b/test/gstreamer/meson.build\n> > > @@ -10,7 +10,7 @@ gstreamer_tests = [\n> > >   gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > >   \n> > >   foreach t : gstreamer_tests\n> > > -    exe = executable(t[0], t[1],\n> > > +    exe = executable(t[0], t[1], 'gstreamer_test.cpp',\n> > >                        dependencies : [libcamera_private, gstreamer_dep],\n> > >                        link_with : test_libraries,\n> > >                        include_directories : test_includes_internal)\n> > \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 5303EBDC71\n\tfor ;\n\tTue, 31 Aug 2021 20:41:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB38E6916A;\n\tTue, 31 Aug 2021 22:41:11 +0200 (CEST)","from mail-io1-xd35.google.com (mail-io1-xd35.google.com\n\t[IPv6:2607:f8b0:4864:20::d35])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8EBD68890\n\tfor ;\n\tTue, 31 Aug 2021 22:41:09 +0200 (CEST)","by mail-io1-xd35.google.com with SMTP id b7so731723iob.4\n\tfor ;\n\tTue, 31 Aug 2021 13:41:09 -0700 (PDT)","from nicolas-tpx395.localdomain (mtl.collabora.ca. [66.171.169.34])\n\tby smtp.gmail.com with ESMTPSA id\n\ti6sm10491759ilb.30.2021.08.31.13.41.07\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 31 Aug 2021 13:41:07 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20150623.gappssmtp.com\n\theader.i=@ndufresne-ca.20150623.gappssmtp.com\n\theader.b=\"k2t/IwBM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=qDxH+PhOXJb/btqaABsx78n4ZHeRy5M5wXQ62ooUaS8=;\n\tb=k2t/IwBMh/6TgkiIsCiri3PstTa0nwr8xweKeUYUPz5MBOgQgdt7Ee8HptereqgvQC\n\tHyWrT8QztA5wBlbykyZaffqGHNaYm2qWY+Qa9rnqcEaHcIqN5CBDW5rsydWxjZZScK+G\n\thHK9WQ2PyxmVd96WQj99oJQwcdUIjGOkYL+RhC/cR19jCocxxyWa64fiwRmcNoL0N5Fi\n\tamzP1LcQqq6b04x9sU0pKNty8ozPa4XAFDTTnIzuu5m6n83ohEy/ELi3Fmv3pyi6cnTQ\n\tyEfaGk5m/q2GV5eND59KnqhV6ykbzxmEB4Mo03xDrNe2U8pR/TymCob7bjNpUrfV0oVA\n\tgSIw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=qDxH+PhOXJb/btqaABsx78n4ZHeRy5M5wXQ62ooUaS8=;\n\tb=iXFYKOHvUjkNnQoLzFCO2Lw9iLpuLFXabM+4cr0J0Cl4cYbpANX7TVhcSJOUh9350V\n\tTv3OHbKFZ6mI43vV+Y1K69TDh/Zhh1FVAYxBRqbc7wz331ev5bTfcfoslWVXNDg824Uq\n\tbFH7P3lPsnEVcc2pHTp5GhiDhkCwdSGIE6hd6sNx2xRA8ssfYcSWi1JHd/OSZw/6tP4u\n\tQqBT5VVjUpcY7zDmvEpX6ai7OblXjQ3D5zr1JPJdvgslp6SM4HhidMtjeeOhzZHS84g4\n\tn9vnE77WyDq/x6KUSdCDeXd3yao3xcR+qCrT8J1hjglDVhAE06QGvVQymWCgPqOg2ukD\n\t7e4A==","X-Gm-Message-State":"AOAM533EhVhLjJ/tnDqgF1t0LoCQWuDAt23G8NzyIlpLqgf6Dxw4lp4d\n\tZRQ/gEPqJWmUG1L/jOldc4adtpNrQ7f2Cw==","X-Google-Smtp-Source":"ABdhPJx1UEb/Wa2uIq1SurCMRqMXKDkOyfxH3HiAqRe4n93okU931OoNfmynBzeLrtMQij34kkfKUA==","X-Received":"by 2002:a6b:5c0c:: with SMTP id\n\tz12mr18018558ioh.171.1630442468448; \n\tTue, 31 Aug 2021 13:41:08 -0700 (PDT)","Message-ID":"","From":"Nicolas Dufresne ","To":"Vedant Paranjape ","Date":"Tue, 31 Aug 2021 16:41:06 -0400","In-Reply-To":"","References":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>\n\t<4c8aba66992febb60e9003bb6037295518c12c48.camel@ndufresne.ca>\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.40.4 (3.40.4-1.fc34) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","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":19227,"web_url":"https://patchwork.libcamera.org/comment/19227/","msgid":"","date":"2021-08-31T20:43:31","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"On Wed, 1 Sep, 2021, 02:11 Nicolas Dufresne, wrote:\n\n> Le mercredi 01 septembre 2021 à 02:04 +0530, Vedant Paranjape a écrit :\n> >\n> >\n> > On Wed, 1 Sep, 2021, 01:59 Nicolas Dufresne, \n> wrote:\n> > > Le mercredi 01 septembre 2021 à 01:32 +0530, Vedant Paranjape a écrit :\n> > > > Lot of code used in single stream test is boiler plate and common\n> across\n> > > > every gstreamer test. Factored out this code into a base class called\n> > > > GstreamerTest.\n> > > >\n> > > > Also updated the gstreamer_single_stream_test to use the\n> GstreamerTest\n> > > > base class\n> > > >\n> > > > Signed-off-by: Vedant Paranjape \n> > >\n> > > For my part, this looks like a good start, we will likely modify and\n> adapt\n> > > test\n> > > while adding few more and it will get more stable later.\n> > >\n> > > Reviewed-by: Nicolas Dufresne \n> > >\n> > > p.s. I'm starting a discussion inline about the test design itself,\n> why they\n> > > need to 2s instead of stop whenever a success condition is met. But\n> this\n> > > isn't\n> > > a\n> > > change from the already merged test, so not a blocker for this.\n> >\n> > I was the one who made it timeout at 2s, as without timeout the test\n> kept on\n> > running forever, what's the success condition here?\n>\n> This is just a test run, I'd use pad probe, and after couple of non-zero\n> frames,\n> I'd stop and quit. This can be done with a GMainLoop, using\n> g_main_loop_quit(),\n> or using an application message, that get catched as success in your\n> process_event() function.\n>\n> The second way is thinner I believe.\n>\n\nWhat benefits do these method have over the way I did it?\n\n>\n> > > > ---\n> > > > .../gstreamer_single_stream_test.cpp | 145\n> +++-------------\n> > > > test/gstreamer/gstreamer_test.cpp | 157\n> ++++++++++++++++++\n> > > > test/gstreamer/gstreamer_test.h | 39 +++++\n> > > > test/gstreamer/meson.build | 2 +-\n> > > > 4 files changed, 221 insertions(+), 122 deletions(-)\n> > > > create mode 100644 test/gstreamer/gstreamer_test.cpp\n> > > > create mode 100644 test/gstreamer/gstreamer_test.h\n> > > >\n> > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > index 4c8d4804..c27e4d36 100644\n> > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > @@ -14,104 +14,48 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \"gstreamer_test.h\"\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> > > > +class GstreamerSingleStreamTest : public GstreamerTest, public Test\n> > > > {\n> > > > +public:\n> > > > + GstreamerSingleStreamTest()\n> > > > + : GstreamerTest()\n> > > > + {\n> > > > + }\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\n> > > ASan\n> > > > - * enabled, and as GStreamer is most likely not, this\n> causes\n> > > 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\n> > > been\n> > > > - * loaded by then. Fix this issue by disabling\n> spawning of a\n> > > > - * child helper process when scanning the build\n> 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:\n> %s\\n\",\n> > > > - errInit ? errInit->message :\n> \"unknown\n> > > error\");\n> > > > + if (status_ != TestPass)\n> > > > + return status_;\n> > > >\n> > > > - return TestFail;\n> > > > - }\n> > > > + g_autoptr(GstElement) convert0__ =\n> > > gst_element_factory_make(\"videoconvert\", \"convert0\");\n> > > > + g_autoptr(GstElement) sink0__ =\n> > > gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > > + g_object_ref_sink(convert0__);\n> > > > + g_object_ref_sink(sink0__);\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> > > > + if (!convert0__ || !sink0__) {\n> > > > + g_printerr(\"Not all elements could be created.\n> > > %p.%p\\n\",\n> > > > + convert0__, sink0__);\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> > > > - /* Create the elements */\n> > > > - libcameraSrc_ =\n> gst_element_factory_make(\"libcamerasrc\",\n> > > \"libcamera\");\n> > > > - convert0_ = gst_element_factory_make(\"videoconvert\",\n> > > \"convert0\");\n> > > > - sink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > > -\n> > > > - /* Create the empty pipeline_ */\n> > > > - pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > -\n> > > > - if (!pipeline_ || !convert0_ || !sink0_ ||\n> !libcameraSrc_) {\n> > > > - g_printerr(\"Not all elements could be created.\n> > > %p.%p.%p.%p\\n\",\n> > > > - pipeline_, convert0_, sink0_,\n> > > libcameraSrc_);\n> > > > - if (pipeline_)\n> > > > - gst_object_unref(pipeline_);\n> > > > - if (convert0_)\n> > > > - gst_object_unref(convert0_);\n> > > > - if (sink0_)\n> > > > - gst_object_unref(sink0_);\n> > > > - if (libcameraSrc_)\n> > > > - gst_object_unref(libcameraSrc_);\n> > > > - gst_deinit();\n> > > > + convert0_ = reinterpret_cast > > *>(g_steal_pointer(&convert0__));\n> > > > + sink0_ = reinterpret_cast > > *>(g_steal_pointer(&sink0__));\n> > > >\n> > > > + if (gstreamer_create_pipeline() != TestPass)\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> > > > convert0_,\n> > > sink0_, NULL);\n> > > > if (gst_element_link_many(libcameraSrc_, convert0_,\n> sink0_,\n> > > NULL) != TRUE) {\n> > > > @@ -119,57 +63,16 @@ protected:\n> > > > return TestFail;\n> > > > }\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\n> > > state.\\n\");\n> > > > + if (gstreamer_start_pipeline() != TestPass)\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,\n> > > 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> > > > + if (gstreamer_process_event() != TestPass)\n> > > > + return TestFail;\n> > > >\n> > > > - return TestFail;\n> > > > + return TestPass;\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 *convert0_;\n> > > > GstElement *sink0_;\n> > > > };\n> > > > diff --git a/test/gstreamer/gstreamer_test.cpp\n> > > b/test/gstreamer/gstreamer_test.cpp\n> > > > new file mode 100644\n> > > > index 00000000..22128c4c\n> > > > --- /dev/null\n> > > > +++ b/test/gstreamer/gstreamer_test.cpp\n> > > > @@ -0,0 +1,157 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > + *\n> > > > + * libcamera Gstreamer element API tests\n> > > > + */\n> > > > +\n> > > > +#include \"gstreamer_test.h\"\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> > > > +GstreamerTest::GstreamerTest()\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\n> error\");\n> > > > +\n> > > > + status_ = TestFail;\n> > > > + return;\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> > > > + g_autoptr(GstPlugin) plugin = gst_registry_lookup(registry,\n> > > \"libgstlibcamera.so\");\n> > > > + if (plugin) {\n> > > > + gst_registry_remove_plugin(registry, 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> > > > +\n> > > > + status_ = TestFail;\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + status_ = TestPass;\n> > > > +}\n> > > > +\n> > > > +GstreamerTest::~GstreamerTest()\n> > > > +{\n> > > > + if (pipeline_)\n> > > > + gst_object_unref(pipeline_);\n> > > > + if (status_ == TestFail) {\n> > > > + gst_object_unref(libcameraSrc_);\n> > > > + }\n> > > > +\n> > > > + gst_deinit();\n> > > > +}\n> > > > +\n> > > > +int GstreamerTest::gstreamer_create_pipeline()\n> > > > +{\n> > > > + g_autoptr(GstElement) libcameraSrc__ =\n> > > gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > > > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > + g_object_ref_sink(libcameraSrc__);\n> > > > +\n> > > > + if (!libcameraSrc__ || !pipeline_) {\n> > > > + g_printerr(\"Unable to create create pipeline %p.%p\\n\",\n> > > > + libcameraSrc__,\n> pipeline_);\n> > > > +\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + libcameraSrc_ = reinterpret_cast > > *>(g_steal_pointer(&libcameraSrc__));\n> > > > +\n> > > > + return TestPass;\n> > > > +}\n> > > > +\n> > > > +int GstreamerTest::gstreamer_start_pipeline()\n> > > > +{\n> > > > + GstStateChangeReturn ret;\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\n> > > state.\\n\");\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + return TestPass;\n> > > > +}\n> > > > +\n> > > > +int GstreamerTest::gstreamer_process_event()\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 = gst_bus_timed_pop_filtered(bus,\n> timeout,\n> > > 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> > >\n> > > Isn't it unfortunate design that all test must last 2s ? Just opening\n> the\n> > > discussion, this was like this before.\n> > >\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> > > > +void GstreamerTest::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> > > > diff --git a/test/gstreamer/gstreamer_test.h\n> > > b/test/gstreamer/gstreamer_test.h\n> > > > new file mode 100644\n> > > > index 00000000..cdffdea9\n> > > > --- /dev/null\n> > > > +++ b/test/gstreamer/gstreamer_test.h\n> > > > @@ -0,0 +1,39 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > + *\n> > > > + * gstreamer_test.cpp - GStreamer test base class\n> > > > + */\n> > > > +\n> > > > +#ifndef __LIBCAMERA_GSTREAMER_TEST_H__\n> > > > +#define __LIBCAMERA_GSTREAMER_TEST_H__\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/source_paths.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +using namespace std;\n> > > > +\n> > > > +class GstreamerTest\n> > > > +{\n> > > > +public:\n> > > > + GstreamerTest();\n> > > > + virtual ~GstreamerTest();\n> > > > +\n> > > > +protected:\n> > > > + virtual int gstreamer_create_pipeline();\n> > > > + int gstreamer_start_pipeline();\n> > > > + int gstreamer_process_event();\n> > > > + void gstreamer_print_error(GstMessage *msg);\n> > > > +\n> > > > + GstElement *pipeline_;\n> > > > + GstElement *libcameraSrc_;\n> > > > + int status_;\n> > > > +};\n> > > > +\n> > > > +#endif /* __LIBCAMERA_GSTREAMER_TEST_H__ */\n> > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > index b99aa0da..aca53b92 100644\n> > > > --- a/test/gstreamer/meson.build\n> > > > +++ b/test/gstreamer/meson.build\n> > > > @@ -10,7 +10,7 @@ gstreamer_tests = [\n> > > > gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > > >\n> > > > foreach t : gstreamer_tests\n> > > > - exe = executable(t[0], t[1],\n> > > > + exe = executable(t[0], t[1], 'gstreamer_test.cpp',\n> > > > dependencies : [libcamera_private,\n> gstreamer_dep],\n> > > > link_with : test_libraries,\n> > > > include_directories : test_includes_internal)\n> > >\n> > >\n>\n>\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 64772BD87D\n\tfor ;\n\tTue, 31 Aug 2021 20:43:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F96069166;\n\tTue, 31 Aug 2021 22:43:44 +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 7E92968890\n\tfor ;\n\tTue, 31 Aug 2021 22:43:43 +0200 (CEST)","by mail-yb1-xb35.google.com with SMTP id c206so795020ybb.12\n\tfor ;\n\tTue, 31 Aug 2021 13:43:43 -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=\"BxcEi35/\"; 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=vpr+ZsggWwF+Q4+cA+gw9V/idJBK1vcdJr/EHd/N+XA=;\n\tb=BxcEi35/R+WAJDwb/XGAgyq+gONHAdGQHrXqOOZxomZi6kxUw7BFni0gBBpzUbwTVU\n\tNYvBbkWYm4GOSIJzw/SpHbijqgEH5QdL7X6R99PotYl4VjgnvJTH/yH9q/9Ney65VFf1\n\tkY9QnG56qdB6SfYjgRaBunaAxzJgKK9YnuBqruARW2RnvT0txlVw4gZ+mQIx7sLg6hec\n\tS5mJtjLKw5DAgmA7CoL7d1wG4qzteAvIXmVNXytb8kFIuWVZXOurErcTzNc5itO+N4iW\n\t9pvR0WeaXf9Ms129zrM0rXdFCKEl13/Sgz6cAYEUiiVOxUjViLm7aRrGqYAtnuMDZtm3\n\t2PFQ==","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=vpr+ZsggWwF+Q4+cA+gw9V/idJBK1vcdJr/EHd/N+XA=;\n\tb=BeIFZrJEn3y4y3zmxypq4j7NLCJuJtDSvKSWBlofNV01Lzg2JP/peSnqr3aFH5EjzH\n\tF0yNTJIC0kX9YqZsWOMWTIJ8CwCzmlR5dqYSK2zI+vEGfQGlV6XUH04HjESB59cRUpuv\n\tvA0IumhZFNKADxfkK954go3SKGYrcAuKBZnNNX9lVKI+v/CmxZ7Gk/yuwQrkFBxp9yto\n\tu98gNr9svCYJxTx/m8i5/RGzRv+iVW3X0lOv9J+pG9wrwCBP8OrstxMwidPu5BAdb5ag\n\tUoCo494A0Qzs9DXrVOW/a+2q9u9nh9LhZW4MoYj4fjgFCiLijCc9TueI5T/Ok1dLtPuB\n\tsfiA==","X-Gm-Message-State":"AOAM532HivVf7R3bb0VkGleuLXwI5POEoqpG0T9ZLCIUaqU4drA2ngM5\n\tz/AQDAgn2iromo8dljCOsrdK5JGtZ48uA96adQSd6+V0QjF4SQ==","X-Google-Smtp-Source":"ABdhPJwW0RIMsatnE4M/zvjjysGLSvvHYilyRe8+sZnHW48EsaVBvFhEhPE6mJPeboL5YfkM98yf1Qn9aFUbypwpTPg=","X-Received":"by 2002:a05:6902:1201:: with SMTP id\n\ts1mr5175992ybu.432.1630442622255; \n\tTue, 31 Aug 2021 13:43:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>\n\t<4c8aba66992febb60e9003bb6037295518c12c48.camel@ndufresne.ca>\n\t\n\t","In-Reply-To":"","From":"Vedant Paranjape ","Date":"Wed, 1 Sep 2021 02:13:31 +0530","Message-ID":"","To":"Nicolas Dufresne ","Content-Type":"multipart/alternative; boundary=\"00000000000053876b05cae101dd\"","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","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":19231,"web_url":"https://patchwork.libcamera.org/comment/19231/","msgid":"","date":"2021-08-31T21:03:15","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Aug 31, 2021 at 04:41:06PM -0400, Nicolas Dufresne wrote:\n> Le mercredi 01 septembre 2021 à 02:04 +0530, Vedant Paranjape a écrit :\n> > On Wed, 1 Sep, 2021, 01:59 Nicolas Dufresne wrote:\n> > > Le mercredi 01 septembre 2021 à 01:32 +0530, Vedant Paranjape a écrit :\n> > > > Lot of code used in single stream test is boiler plate and common across\n> > > > every gstreamer test. Factored out this code into a base class called\n> > > > GstreamerTest.\n> > > > \n> > > > Also updated the gstreamer_single_stream_test to use the GstreamerTest\n> > > > base class\n> > > > \n> > > > Signed-off-by: Vedant Paranjape \n> > > \n> > > For my part, this looks like a good start, we will likely modify and adapt test\n> > > while adding few more and it will get more stable later.\n> > > \n> > > Reviewed-by: Nicolas Dufresne \n> > > \n> > > p.s. I'm starting a discussion inline about the test design itself, why they\n> > > need to 2s instead of stop whenever a success condition is met. But this isn't a\n> > > change from the already merged test, so not a blocker for this.\n> > \n> > I was the one who made it timeout at 2s, as without timeout the test kept on\n> > running forever, what's the success condition here?\n> \n> This is just a test run, I'd use pad probe, and after couple of non-zero frames,\n> I'd stop and quit. This can be done with a GMainLoop, using g_main_loop_quit(),\n> or using an application message, that get catched as success in your\n> process_event() function.\n\nPlease don't forget to include a timeout.\n\n> The second way is thinner I believe.\n> \n> > > > ---\n> > > >   .../gstreamer_single_stream_test.cpp          | 145 +++-------------\n> > > >   test/gstreamer/gstreamer_test.cpp             | 157 ++++++++++++++++++\n> > > >   test/gstreamer/gstreamer_test.h               |  39 +++++\n> > > >   test/gstreamer/meson.build                    |   2 +-\n> > > >   4 files changed, 221 insertions(+), 122 deletions(-)\n> > > >   create mode 100644 test/gstreamer/gstreamer_test.cpp\n> > > >   create mode 100644 test/gstreamer/gstreamer_test.h\n> > > > \n> > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > /test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > index 4c8d4804..c27e4d36 100644\n> > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > @@ -14,104 +14,48 @@\n> > > >   \n> > > >   #include \n> > > >   \n> > > > +#include \"gstreamer_test.h\"\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 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> > > > +class GstreamerSingleStreamTest : public GstreamerTest, public Test\n> > > >   {\n> > > > +public:\n> > > > +     GstreamerSingleStreamTest()\n> > > > +             : GstreamerTest()\n> > > > +     {\n> > > > +     }\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 > San\n> > > > -              * enabled, and as GStreamer is most likely not, this causes > he\n> > > > -              * ASan link order check to fail when gst-plugin-scanner\n> > > > -              * dlopen()s the plugin as many libraries will have already > een\n> > > > -              * loaded by then. Fix this issue by disabling spawning of 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 > rror\");\n> > > > +             if (status_ != TestPass)\n> > > > +                     return status_;\n> > > >   \n> > > > -                     return TestFail;\n> > > > -             }\n> > > > +             g_autoptr(GstElement) convert0__ = > st_element_factory_make(\"videoconvert\", \"convert0\");\n> > > > +             g_autoptr(GstElement) sink0__ = > st_element_factory_make(\"fakesink\", \"sink0\");\n> > > > +             g_object_ref_sink(convert0__);\n> > > > +             g_object_ref_sink(sink0__);\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> > > > +             if (!convert0__ || !sink0__) {\n> > > > +                     g_printerr(\"Not all elements could be created. > p.%p\\n\",\n> > > > +                                convert0__, sink0__);\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> > > > -             /* Create the elements */\n> > > > -             libcameraSrc_ = gst_element_factory_make(\"libcamerasrc\", > libcamera\");\n> > > > -             convert0_ = gst_element_factory_make(\"videoconvert\", > convert0\");\n> > > > -             sink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > > -\n> > > > -             /* Create the empty pipeline_ */\n> > > > -             pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > -\n> > > > -             if (!pipeline_ || !convert0_ || !sink0_ || !libcameraSrc_) {\n> > > > -                     g_printerr(\"Not all elements could be created. > p.%p.%p.%p\\n\",\n> > > > -                                pipeline_, convert0_, sink0_, > ibcameraSrc_);\n> > > > -                     if (pipeline_)\n> > > > -                             gst_object_unref(pipeline_);\n> > > > -                     if (convert0_)\n> > > > -                             gst_object_unref(convert0_);\n> > > > -                     if (sink0_)\n> > > > -                             gst_object_unref(sink0_);\n> > > > -                     if (libcameraSrc_)\n> > > > -                             gst_object_unref(libcameraSrc_);\n> > > > -                     gst_deinit();\n> > > > +             convert0_ = reinterpret_cast >(g_steal_pointer(&convert0__));\n> > > > +             sink0_ = reinterpret_cast >(g_steal_pointer(&sink0__));\n> > > >   \n> > > > +             if (gstreamer_create_pipeline() != TestPass)\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> > > > convert0_, > ink0_, NULL);\n> > > >                if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, > ULL) != TRUE) {\n> > > > @@ -119,57 +63,16 @@ protected:\n> > > >                        return TestFail;\n> > > >                }\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 > tate.\\n\");\n> > > > +             if (gstreamer_start_pipeline() != TestPass)\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 | > ST_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, > imeout, 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> > > > +             if (gstreamer_process_event() != TestPass)\n> > > > +                     return TestFail;\n> > > >   \n> > > > -             return TestFail;\n> > > > +             return TestPass;\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 *convert0_;\n> > > >        GstElement *sink0_;\n> > > >   };\n> > > > diff --git a/test/gstreamer/gstreamer_test.cpp > /test/gstreamer/gstreamer_test.cpp\n> > > > new file mode 100644\n> > > > index 00000000..22128c4c\n> > > > --- /dev/null\n> > > > +++ b/test/gstreamer/gstreamer_test.cpp\n> > > > @@ -0,0 +1,157 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > + *\n> > > > + * libcamera Gstreamer element API tests\n> > > > + */\n> > > > +\n> > > > +#include \"gstreamer_test.h\"\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 handled by\n> > > > +      * using a suppression file instead of disabling leak detection.\n> > > > +      */\n> > > > +     return \"detect_leaks=false\";\n> > > > +}\n> > > > +}\n> > > > +\n> > > > +GstreamerTest::GstreamerTest()\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> > > > +             status_ = TestFail;\n> > > > +             return;\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> > > > +     g_autoptr(GstPlugin) plugin = gst_registry_lookup(registry, > libgstlibcamera.so\");\n> > > > +     if (plugin) {\n> > > > +             gst_registry_remove_plugin(registry, plugin);\n> > > > +     }\n> > > > +\n> > > > +     std::string path = libcamera::utils::libcameraBuildPath() + > src/gstreamer\";\n> > > > +     if (!gst_registry_scan_path(registry, path.c_str())) {\n> > > > +             g_printerr(\"Failed to add plugin to registry\\n\");\n> > > > +\n> > > > +             status_ = TestFail;\n> > > > +             return;\n> > > > +     }\n> > > > +\n> > > > +     status_ = TestPass;\n> > > > +}\n> > > > +\n> > > > +GstreamerTest::~GstreamerTest()\n> > > > +{\n> > > > +     if (pipeline_)\n> > > > +             gst_object_unref(pipeline_);\n> > > > +     if (status_ == TestFail) {\n> > > > +             gst_object_unref(libcameraSrc_);\n> > > > +     }\n> > > > +\n> > > > +     gst_deinit();\n> > > > +}\n> > > > +\n> > > > +int GstreamerTest::gstreamer_create_pipeline()\n> > > > +{\n> > > > +     g_autoptr(GstElement) libcameraSrc__ = > st_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > > > +     pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > +     g_object_ref_sink(libcameraSrc__);\n> > > > +\n> > > > +     if (!libcameraSrc__ || !pipeline_) {\n> > > > +             g_printerr(\"Unable to create create pipeline %p.%p\\n\",\n> > > > +                                             libcameraSrc__, pipeline_);\n> > > > +\n> > > > +             return TestFail;\n> > > > +     }\n> > > > +\n> > > > +     libcameraSrc_ = reinterpret_cast >(g_steal_pointer(&libcameraSrc__));\n> > > > +\n> > > > +     return TestPass;\n> > > > +}\n> > > > +\n> > > > +int GstreamerTest::gstreamer_start_pipeline()\n> > > > +{\n> > > > +     GstStateChangeReturn ret;\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 > tate.\\n\");\n> > > > +             return TestFail;\n> > > > +     }\n> > > > +\n> > > > +     return TestPass;\n> > > > +}\n> > > > +\n> > > > +int GstreamerTest::gstreamer_process_event()\n> > > > +{\n> > > > +     /* Wait until error or EOS or timeout after 2 seconds */\n> > > > +     constexpr GstMessageType msgType =\n> > > > +             static_cast(GST_MESSAGE_ERROR | > ST_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, > sgType);\n> > > > +\n> > > > +     gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > > > +\n> > > > +     /* Parse error message */\n> > > > +     if (msg == NULL) {\n> > > > +             return TestPass;\n> > > > +     }\n> > > \n> > > Isn't it unfortunate design that all test must last 2s ? Just opening the\n> > > discussion, this was like this before.\n> > > \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> > > > +void GstreamerTest::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> > > > diff --git a/test/gstreamer/gstreamer_test.h > /test/gstreamer/gstreamer_test.h\n> > > > new file mode 100644\n> > > > index 00000000..cdffdea9\n> > > > --- /dev/null\n> > > > +++ b/test/gstreamer/gstreamer_test.h\n> > > > @@ -0,0 +1,39 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > + *\n> > > > + * gstreamer_test.cpp - GStreamer test base class\n> > > > + */\n> > > > +\n> > > > +#ifndef __LIBCAMERA_GSTREAMER_TEST_H__\n> > > > +#define __LIBCAMERA_GSTREAMER_TEST_H__\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/source_paths.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +using namespace std;\n> > > > +\n> > > > +class GstreamerTest\n> > > > +{\n> > > > +public:\n> > > > +     GstreamerTest();\n> > > > +     virtual ~GstreamerTest();\n> > > > +\n> > > > +protected:\n> > > > +     virtual int gstreamer_create_pipeline();\n> > > > +     int gstreamer_start_pipeline();\n> > > > +     int gstreamer_process_event();\n> > > > +     void gstreamer_print_error(GstMessage *msg);\n> > > > +\n> > > > +     GstElement *pipeline_;\n> > > > +     GstElement *libcameraSrc_;\n> > > > +     int status_;\n> > > > +};\n> > > > +\n> > > > +#endif /* __LIBCAMERA_GSTREAMER_TEST_H__ */\n> > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > index b99aa0da..aca53b92 100644\n> > > > --- a/test/gstreamer/meson.build\n> > > > +++ b/test/gstreamer/meson.build\n> > > > @@ -10,7 +10,7 @@ gstreamer_tests = [\n> > > >   gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > > >   \n> > > >   foreach t : gstreamer_tests\n> > > > -    exe = executable(t[0], t[1],\n> > > > +    exe = executable(t[0], t[1], 'gstreamer_test.cpp',\n> > > >                        dependencies : [libcamera_private, gstreamer_dep],\n> > > >                        link_with : test_libraries,\n> > > >                        include_directories : test_includes_internal)","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 98F3ABD87D\n\tfor ;\n\tTue, 31 Aug 2021 21:03:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1252B69166;\n\tTue, 31 Aug 2021 23:03:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF65068890\n\tfor ;\n\tTue, 31 Aug 2021 23:03:30 +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 BC78724F;\n\tTue, 31 Aug 2021 23:03:29 +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=\"tkik1z/f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630443810;\n\tbh=/KGDP4a34iMKCNaU15ozVXca+2aW1YhJ7OfEj98TM3E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tkik1z/fktbfrnavBFYSAxAIaKHlO/NQDwRiQQzuIvoV1XmAUzoV6G/Xug9L3BQok\n\tmPZKbx0GeaFnY4F23FNKjERwK7Z0y2l2ZVcEAc1iAvSFADC6Z8n6jFOYpfkEC8wjwc\n\ttgZZ5ObJyJ0hsjhQM1JdMIeJ6YikUZLaGm52P7sQ=","Date":"Wed, 1 Sep 2021 00:03:15 +0300","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Message-ID":"","References":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>\n\t<4c8aba66992febb60e9003bb6037295518c12c48.camel@ndufresne.ca>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","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,\n\tVedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19263,"web_url":"https://patchwork.libcamera.org/comment/19263/","msgid":"","date":"2021-09-01T15:54:55","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 01 septembre 2021 à 02:13 +0530, Vedant Paranjape a écrit :\n> \n> \n> On Wed, 1 Sep, 2021, 02:11 Nicolas Dufresne, wrote:\n> > Le mercredi 01 septembre 2021 à 02:04 +0530, Vedant Paranjape a écrit :\n> > > \n> > > \n> > > On Wed, 1 Sep, 2021, 01:59 Nicolas Dufresne, wrote:\n> > > > Le mercredi 01 septembre 2021 à 01:32 +0530, Vedant Paranjape a écrit :\n> > > > > Lot of code used in single stream test is boiler plate and common\n> > > > > across\n> > > > > every gstreamer test. Factored out this code into a base class called\n> > > > > GstreamerTest.\n> > > > > \n> > > > > Also updated the gstreamer_single_stream_test to use the GstreamerTest\n> > > > > base class\n> > > > > \n> > > > > Signed-off-by: Vedant Paranjape \n> > > > \n> > > > For my part, this looks like a good start, we will likely modify and\n> > > > adapt\n> > > > test\n> > > > while adding few more and it will get more stable later.\n> > > > \n> > > > Reviewed-by: Nicolas Dufresne \n> > > > \n> > > > p.s. I'm starting a discussion inline about the test design itself, why\n> > they\n> > > > need to 2s instead of stop whenever a success condition is met. But this\n> > > > isn't\n> > > > a\n> > > > change from the already merged test, so not a blocker for this.\n> > > \n> > > I was the one who made it timeout at 2s, as without timeout the test kept\n> > > on\n> > > running forever, what's the success condition here?\n> > \n> > This is just a test run, I'd use pad probe, and after couple of non-zero\n> > frames,\n> > I'd stop and quit. This can be done with a GMainLoop, using\n> > g_main_loop_quit(),\n> > or using an application message, that get catched as success in your\n> > process_event() function.\n> > \n> > The second way is thinner I believe.\n> \n> What benefits do these method have over the way I did it?\n\nConsider long term, with growing number of test, if all tests take 2s to run,\nand run one after the other, running the test will be very very long.\n\nWith a test that only runs the time requires to figure-out if the camera is\nspitting non-fully black images, only few ms are needed.\n\n> \n> > > \n> > > > > ---\n> > > > >   .../gstreamer_single_stream_test.cpp          | 145 +++-------------\n> > > > >   test/gstreamer/gstreamer_test.cpp             | 157\n> > > > > ++++++++++++++++++\n> > > > >   test/gstreamer/gstreamer_test.h               |  39 +++++\n> > > > >   test/gstreamer/meson.build                    |   2 +-\n> > > > >   4 files changed, 221 insertions(+), 122 deletions(-)\n> > > > >   create mode 100644 test/gstreamer/gstreamer_test.cpp\n> > > > >   create mode 100644 test/gstreamer/gstreamer_test.h\n> > > > > \n> > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > > index 4c8d4804..c27e4d36 100644\n> > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > > @@ -14,104 +14,48 @@\n> > > > >   \n> > > > >   #include \n> > > > >   \n> > > > > +#include \"gstreamer_test.h\"\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\n> > 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> > > > > +class GstreamerSingleStreamTest : public GstreamerTest, public Test\n> > > > >   {\n> > > > > +public:\n> > > > > +     GstreamerSingleStreamTest()\n> > > > > +             : GstreamerTest()\n> > > > > +     {\n> > > > > +     }\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\n> > > > ASan\n> > > > > -              * enabled, and as GStreamer is most likely not, this\n> > causes\n> > > > the\n> > > > > -              * ASan link order check to fail when gst-plugin-scanner\n> > > > > -              * dlopen()s the plugin as many libraries will have\n> > already\n> > > > been\n> > > > > -              * loaded by then. Fix this issue by disabling spawning\n> > > > > of\n> > a\n> > > > > -              * child helper process when scanning the build\n> > > > > 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:\n> > > > > %s\\n\",\n> > > > > -                                errInit ? errInit->message : \"unknown\n> > > > error\");\n> > > > > +             if (status_ != TestPass)\n> > > > > +                     return status_;\n> > > > >   \n> > > > > -                     return TestFail;\n> > > > > -             }\n> > > > > +             g_autoptr(GstElement) convert0__ =\n> > > > gst_element_factory_make(\"videoconvert\", \"convert0\");\n> > > > > +             g_autoptr(GstElement) sink0__ =\n> > > > gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > > > +             g_object_ref_sink(convert0__);\n> > > > > +             g_object_ref_sink(sink0__);\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> > > > > +             if (!convert0__ || !sink0__) {\n> > > > > +                     g_printerr(\"Not all elements could be created.\n> > > > %p.%p\\n\",\n> > > > > +                                convert0__, sink0__);\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> > > > > -             /* Create the elements */\n> > > > > -             libcameraSrc_ = gst_element_factory_make(\"libcamerasrc\",\n> > > > \"libcamera\");\n> > > > > -             convert0_ = gst_element_factory_make(\"videoconvert\",\n> > > > \"convert0\");\n> > > > > -             sink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > > > -\n> > > > > -             /* Create the empty pipeline_ */\n> > > > > -             pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > > -\n> > > > > -             if (!pipeline_ || !convert0_ || !sink0_ ||\n> > > > > !libcameraSrc_)\n> > {\n> > > > > -                     g_printerr(\"Not all elements could be created.\n> > > > %p.%p.%p.%p\\n\",\n> > > > > -                                pipeline_, convert0_, sink0_,\n> > > > libcameraSrc_);\n> > > > > -                     if (pipeline_)\n> > > > > -                             gst_object_unref(pipeline_);\n> > > > > -                     if (convert0_)\n> > > > > -                             gst_object_unref(convert0_);\n> > > > > -                     if (sink0_)\n> > > > > -                             gst_object_unref(sink0_);\n> > > > > -                     if (libcameraSrc_)\n> > > > > -                             gst_object_unref(libcameraSrc_);\n> > > > > -                     gst_deinit();\n> > > > > +             convert0_ = reinterpret_cast > > > *>(g_steal_pointer(&convert0__));\n> > > > > +             sink0_ = reinterpret_cast > > > *>(g_steal_pointer(&sink0__));\n> > > > >   \n> > > > > +             if (gstreamer_create_pipeline() != TestPass)\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> > > > > convert0_,\n> > > > sink0_, NULL);\n> > > > >                if (gst_element_link_many(libcameraSrc_, convert0_,\n> > sink0_,\n> > > > NULL) != TRUE) {\n> > > > > @@ -119,57 +63,16 @@ protected:\n> > > > >                        return TestFail;\n> > > > >                }\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\n> > > > state.\\n\");\n> > > > > +             if (gstreamer_start_pipeline() != TestPass)\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,\n> > > > 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> > > > > +             if (gstreamer_process_event() != TestPass)\n> > > > > +                     return TestFail;\n> > > > >   \n> > > > > -             return TestFail;\n> > > > > +             return TestPass;\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 *convert0_;\n> > > > >        GstElement *sink0_;\n> > > > >   };\n> > > > > diff --git a/test/gstreamer/gstreamer_test.cpp\n> > > > b/test/gstreamer/gstreamer_test.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..22128c4c\n> > > > > --- /dev/null\n> > > > > +++ b/test/gstreamer/gstreamer_test.cpp\n> > > > > @@ -0,0 +1,157 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > > + *\n> > > > > + * libcamera Gstreamer element API tests\n> > > > > + */\n> > > > > +\n> > > > > +#include \"gstreamer_test.h\"\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\n> > by\n> > > > > +      * using a suppression file instead of disabling leak detection.\n> > > > > +      */\n> > > > > +     return \"detect_leaks=false\";\n> > > > > +}\n> > > > > +}\n> > > > > +\n> > > > > +GstreamerTest::GstreamerTest()\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\n> > > > > error\");\n> > > > > +\n> > > > > +             status_ = TestFail;\n> > > > > +             return;\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> > > > > +     g_autoptr(GstPlugin) plugin = gst_registry_lookup(registry,\n> > > > \"libgstlibcamera.so\");\n> > > > > +     if (plugin) {\n> > > > > +             gst_registry_remove_plugin(registry, 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> > > > > +\n> > > > > +             status_ = TestFail;\n> > > > > +             return;\n> > > > > +     }\n> > > > > +\n> > > > > +     status_ = TestPass;\n> > > > > +}\n> > > > > +\n> > > > > +GstreamerTest::~GstreamerTest()\n> > > > > +{\n> > > > > +     if (pipeline_)\n> > > > > +             gst_object_unref(pipeline_);\n> > > > > +     if (status_ == TestFail) {\n> > > > > +             gst_object_unref(libcameraSrc_);\n> > > > > +     }\n> > > > > +\n> > > > > +     gst_deinit();\n> > > > > +}\n> > > > > +\n> > > > > +int GstreamerTest::gstreamer_create_pipeline()\n> > > > > +{\n> > > > > +     g_autoptr(GstElement) libcameraSrc__ =\n> > > > gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > > > > +     pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > > +     g_object_ref_sink(libcameraSrc__);\n> > > > > +\n> > > > > +     if (!libcameraSrc__ || !pipeline_) {\n> > > > > +             g_printerr(\"Unable to create create pipeline %p.%p\\n\",\n> > > > > +                                             libcameraSrc__,\n> > pipeline_);\n> > > > > +\n> > > > > +             return TestFail;\n> > > > > +     }\n> > > > > +\n> > > > > +     libcameraSrc_ = reinterpret_cast > > > *>(g_steal_pointer(&libcameraSrc__));\n> > > > > +\n> > > > > +     return TestPass;\n> > > > > +}\n> > > > > +\n> > > > > +int GstreamerTest::gstreamer_start_pipeline()\n> > > > > +{\n> > > > > +     GstStateChangeReturn ret;\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\n> > > > state.\\n\");\n> > > > > +             return TestFail;\n> > > > > +     }\n> > > > > +\n> > > > > +     return TestPass;\n> > > > > +}\n> > > > > +\n> > > > > +int GstreamerTest::gstreamer_process_event()\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 = gst_bus_timed_pop_filtered(bus,\n> > timeout,\n> > > > 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> > > > \n> > > > Isn't it unfortunate design that all test must last 2s ? Just opening\n> > > > the\n> > > > discussion, this was like this before.\n> > > > \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> > > > > +void GstreamerTest::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> > > > > diff --git a/test/gstreamer/gstreamer_test.h\n> > > > b/test/gstreamer/gstreamer_test.h\n> > > > > new file mode 100644\n> > > > > index 00000000..cdffdea9\n> > > > > --- /dev/null\n> > > > > +++ b/test/gstreamer/gstreamer_test.h\n> > > > > @@ -0,0 +1,39 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > > + *\n> > > > > + * gstreamer_test.cpp - GStreamer test base class\n> > > > > + */\n> > > > > +\n> > > > > +#ifndef __LIBCAMERA_GSTREAMER_TEST_H__\n> > > > > +#define __LIBCAMERA_GSTREAMER_TEST_H__\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/source_paths.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +using namespace std;\n> > > > > +\n> > > > > +class GstreamerTest\n> > > > > +{\n> > > > > +public:\n> > > > > +     GstreamerTest();\n> > > > > +     virtual ~GstreamerTest();\n> > > > > +\n> > > > > +protected:\n> > > > > +     virtual int gstreamer_create_pipeline();\n> > > > > +     int gstreamer_start_pipeline();\n> > > > > +     int gstreamer_process_event();\n> > > > > +     void gstreamer_print_error(GstMessage *msg);\n> > > > > +\n> > > > > +     GstElement *pipeline_;\n> > > > > +     GstElement *libcameraSrc_;\n> > > > > +     int status_;\n> > > > > +};\n> > > > > +\n> > > > > +#endif /* __LIBCAMERA_GSTREAMER_TEST_H__ */\n> > > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > > index b99aa0da..aca53b92 100644\n> > > > > --- a/test/gstreamer/meson.build\n> > > > > +++ b/test/gstreamer/meson.build\n> > > > > @@ -10,7 +10,7 @@ gstreamer_tests = [\n> > > > >   gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > > > >   \n> > > > >   foreach t : gstreamer_tests\n> > > > > -    exe = executable(t[0], t[1],\n> > > > > +    exe = executable(t[0], t[1], 'gstreamer_test.cpp',\n> > > > >                        dependencies : [libcamera_private,\n> > gstreamer_dep],\n> > > > >                        link_with : test_libraries,\n> > > > >                        include_directories : test_includes_internal)\n> > > > \n> > > > \n> > \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 4EA88BDC71\n\tfor ;\n\tWed, 1 Sep 2021 15:55:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE43C6916C;\n\tWed, 1 Sep 2021 17:55:00 +0200 (CEST)","from mail-qv1-xf2f.google.com (mail-qv1-xf2f.google.com\n\t[IPv6:2607:f8b0:4864:20::f2f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06CBA60288\n\tfor ;\n\tWed, 1 Sep 2021 17:54:59 +0200 (CEST)","by mail-qv1-xf2f.google.com with SMTP id e18so23659qvo.1\n\tfor ;\n\tWed, 01 Sep 2021 08:54:58 -0700 (PDT)","from nicolas-tpx395.localdomain (173-246-12-168.qc.cable.ebox.net.\n\t[173.246.12.168])\n\tby smtp.gmail.com with ESMTPSA id 67sm288742qkl.1.2021.09.01.08.54.56\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 01 Sep 2021 08:54:56 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20150623.gappssmtp.com\n\theader.i=@ndufresne-ca.20150623.gappssmtp.com\n\theader.b=\"jvY2RGpe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=g1rIzNKMHcQKh+R9s7taIW4gG5S40xZdbULiVPv9QCA=;\n\tb=jvY2RGpe3VOZLbTe0UReB3FodB+Sfr1OQEbgm/4KfN3kOlAcq0Ng/+/YldVDoLAe8r\n\t0JBopXrhN7l5/0OEsX3ycvzColtEyYgNzkPwCHW+y8GXhdfQsbYrYGmmpbiYytaG2T7t\n\tzsy/CxQPy3ofNPw2cg+Hkx0vbEW85zQbb61IFcdOyHVcNwZyJWxQ+UDRTiU2jLTBWh6x\n\td3x1pGDs9V8y+TtLisx+ll+r/ahyHoPxmnBfF9F6AwrFZRBTbMIB5Cn6+AEhQ5LaImpy\n\tRQwlL/eGxKRXOwthQ+pS2LAY7FsJYfMTpZ3o2Ol2iS0xEAvZcJjFfX45yeDHcslSu5j3\n\txQbg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=g1rIzNKMHcQKh+R9s7taIW4gG5S40xZdbULiVPv9QCA=;\n\tb=LINPgPPgNj7b3CIfS7ZkLBz75wEodIQaaJM4NAjuo5JDXCC/B+Wy713zp5aXPC/3qg\n\tSMpVuh4MGDACRiWbbisFLe+roQzXSlEywilON44E5+G8aafWloGnTQVGXZCrArJT/zmv\n\tZD56zCxOBqHBLbuzhKy3yrCEmKUZDtcmEhIfKbAAlXw3avJRE+0xwgAtDpKeDodC2cx/\n\tLLZDHm56+cYDSbQHQYNUcBIzlfDy/+JBx2u3P3CU77yJhWx9qdZzdvEBXj7ha2yc3iao\n\tOVPR+ShhNNRrBmnOiS6fYwRL/Zn7wayHbjU42CNA0EraLCpwwxNeMxMOQGcxR8vr68Zt\n\teLCw==","X-Gm-Message-State":"AOAM533QO+d146kdpm/p4huexBDt8Yha/pycBsWJGTH6QCka2/9clR+q\n\tNJabfH9aiDBIgLhjPhgtATP3lw==","X-Google-Smtp-Source":"ABdhPJzAJsDZX/UmlcIb6JEnDBF+5RWDHdShL4iWh75lJSN+LfgPrjL7q6TwEP4Rsb30YsglJnAxPg==","X-Received":"by 2002:a05:6214:28b:: with SMTP id\n\tl11mr35548606qvv.18.1630511697316; \n\tWed, 01 Sep 2021 08:54:57 -0700 (PDT)","Message-ID":"","From":"Nicolas Dufresne ","To":"Vedant Paranjape ","Date":"Wed, 01 Sep 2021 11:54:55 -0400","In-Reply-To":"","References":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>\n\t<4c8aba66992febb60e9003bb6037295518c12c48.camel@ndufresne.ca>\n\t\n\t\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.40.4 (3.40.4-1.fc34) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","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":19264,"web_url":"https://patchwork.libcamera.org/comment/19264/","msgid":"","date":"2021-09-01T16:18:54","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Got it, thanks!\n\nOn Wed, Sep 1, 2021 at 9:24 PM Nicolas Dufresne \nwrote:\n\n> Le mercredi 01 septembre 2021 à 02:13 +0530, Vedant Paranjape a écrit :\n> >\n> >\n> > On Wed, 1 Sep, 2021, 02:11 Nicolas Dufresne, \n> wrote:\n> > > Le mercredi 01 septembre 2021 à 02:04 +0530, Vedant Paranjape a écrit :\n> > > >\n> > > >\n> > > > On Wed, 1 Sep, 2021, 01:59 Nicolas Dufresne, \n> wrote:\n> > > > > Le mercredi 01 septembre 2021 à 01:32 +0530, Vedant Paranjape a\n> écrit :\n> > > > > > Lot of code used in single stream test is boiler plate and common\n> > > > > > across\n> > > > > > every gstreamer test. Factored out this code into a base class\n> called\n> > > > > > GstreamerTest.\n> > > > > >\n> > > > > > Also updated the gstreamer_single_stream_test to use the\n> GstreamerTest\n> > > > > > base class\n> > > > > >\n> > > > > > Signed-off-by: Vedant Paranjape >\n> > > > >\n> > > > > For my part, this looks like a good start, we will likely modify\n> and\n> > > > > adapt\n> > > > > test\n> > > > > while adding few more and it will get more stable later.\n> > > > >\n> > > > > Reviewed-by: Nicolas Dufresne \n> > > > >\n> > > > > p.s. I'm starting a discussion inline about the test design\n> itself, why\n> > > they\n> > > > > need to 2s instead of stop whenever a success condition is met.\n> But this\n> > > > > isn't\n> > > > > a\n> > > > > change from the already merged test, so not a blocker for this.\n> > > >\n> > > > I was the one who made it timeout at 2s, as without timeout the test\n> kept\n> > > > on\n> > > > running forever, what's the success condition here?\n> > >\n> > > This is just a test run, I'd use pad probe, and after couple of\n> non-zero\n> > > frames,\n> > > I'd stop and quit. This can be done with a GMainLoop, using\n> > > g_main_loop_quit(),\n> > > or using an application message, that get catched as success in your\n> > > process_event() function.\n> > >\n> > > The second way is thinner I believe.\n> >\n> > What benefits do these method have over the way I did it?\n>\n> Consider long term, with growing number of test, if all tests take 2s to\n> run,\n> and run one after the other, running the test will be very very long.\n>\n> With a test that only runs the time requires to figure-out if the camera is\n> spitting non-fully black images, only few ms are needed.\n>\n> >\n> > > >\n> > > > > > ---\n> > > > > > .../gstreamer_single_stream_test.cpp | 145\n> +++-------------\n> > > > > > test/gstreamer/gstreamer_test.cpp | 157\n> > > > > > ++++++++++++++++++\n> > > > > > test/gstreamer/gstreamer_test.h | 39 +++++\n> > > > > > test/gstreamer/meson.build | 2 +-\n> > > > > > 4 files changed, 221 insertions(+), 122 deletions(-)\n> > > > > > create mode 100644 test/gstreamer/gstreamer_test.cpp\n> > > > > > create mode 100644 test/gstreamer/gstreamer_test.h\n> > > > > >\n> > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > > b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > > > index 4c8d4804..c27e4d36 100644\n> > > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > > > @@ -14,104 +14,48 @@\n> > > > > >\n> > > > > > #include \n> > > > > >\n> > > > > > +#include \"gstreamer_test.h\"\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\n> > > 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> > > > > > +class GstreamerSingleStreamTest : public GstreamerTest, public\n> Test\n> > > > > > {\n> > > > > > +public:\n> > > > > > + GstreamerSingleStreamTest()\n> > > > > > + : GstreamerTest()\n> > > > > > + {\n> > > > > > + }\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\n> compiled\n> > > with\n> > > > > ASan\n> > > > > > - * enabled, and as GStreamer is most likely not,\n> this\n> > > causes\n> > > > > 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\n> > > > > been\n> > > > > > - * loaded by then. Fix this issue by disabling\n> spawning\n> > > > > > of\n> > > a\n> > > > > > - * child helper process when scanning the build\n> > > > > > 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:\n> > > > > > %s\\n\",\n> > > > > > - errInit ? errInit->message :\n> \"unknown\n> > > > > error\");\n> > > > > > + if (status_ != TestPass)\n> > > > > > + return status_;\n> > > > > >\n> > > > > > - return TestFail;\n> > > > > > - }\n> > > > > > + g_autoptr(GstElement) convert0__ =\n> > > > > gst_element_factory_make(\"videoconvert\", \"convert0\");\n> > > > > > + g_autoptr(GstElement) sink0__ =\n> > > > > gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > > > > + g_object_ref_sink(convert0__);\n> > > > > > + g_object_ref_sink(sink0__);\n> > > > > >\n> > > > > > - /*\n> > > > > > - * Remove the system libcamera plugin, if any, and\n> 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,\n> plugin);\n> > > > > > - gst_object_unref(plugin);\n> > > > > > - }\n> > > > > > + if (!convert0__ || !sink0__) {\n> > > > > > + g_printerr(\"Not all elements could be\n> created.\n> > > > > %p.%p\\n\",\n> > > > > > + convert0__, sink0__);\n> > > > > >\n> > > > > > - std::string path =\n> > > > > > libcamera::utils::libcameraBuildPath()\n> > > > > > - + \"src/gstreamer\";\n> > > > > > - if (!gst_registry_scan_path(registry,\n> path.c_str())) {\n> > > > > > - g_printerr(\"Failed to add plugin to\n> > > > > > registry\\n\");\n> > > > > > - gst_deinit();\n> > > > > > return TestFail;\n> > > > > > }\n> > > > > >\n> > > > > > - /* Create the elements */\n> > > > > > - libcameraSrc_ =\n> gst_element_factory_make(\"libcamerasrc\",\n> > > > > \"libcamera\");\n> > > > > > - convert0_ =\n> gst_element_factory_make(\"videoconvert\",\n> > > > > \"convert0\");\n> > > > > > - sink0_ = gst_element_factory_make(\"fakesink\",\n> \"sink0\");\n> > > > > > -\n> > > > > > - /* Create the empty pipeline_ */\n> > > > > > - pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > > > -\n> > > > > > - if (!pipeline_ || !convert0_ || !sink0_ ||\n> > > > > > !libcameraSrc_)\n> > > {\n> > > > > > - g_printerr(\"Not all elements could be\n> created.\n> > > > > %p.%p.%p.%p\\n\",\n> > > > > > - pipeline_, convert0_, sink0_,\n> > > > > libcameraSrc_);\n> > > > > > - if (pipeline_)\n> > > > > > - gst_object_unref(pipeline_);\n> > > > > > - if (convert0_)\n> > > > > > - gst_object_unref(convert0_);\n> > > > > > - if (sink0_)\n> > > > > > - gst_object_unref(sink0_);\n> > > > > > - if (libcameraSrc_)\n> > > > > > - gst_object_unref(libcameraSrc_);\n> > > > > > - gst_deinit();\n> > > > > > + convert0_ = reinterpret_cast > > > > *>(g_steal_pointer(&convert0__));\n> > > > > > + sink0_ = reinterpret_cast > > > > *>(g_steal_pointer(&sink0__));\n> > > > > >\n> > > > > > + if (gstreamer_create_pipeline() != TestPass)\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_),\n> libcameraSrc_,\n> > > > > > convert0_,\n> > > > > sink0_, NULL);\n> > > > > > if (gst_element_link_many(libcameraSrc_,\n> convert0_,\n> > > sink0_,\n> > > > > NULL) != TRUE) {\n> > > > > > @@ -119,57 +63,16 @@ protected:\n> > > > > > return TestFail;\n> > > > > > }\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\n> the\n> > > > > > playing\n> > > > > state.\\n\");\n> > > > > > + if (gstreamer_start_pipeline() != TestPass)\n> > > > > > return TestFail;\n> > > > > > - }\n> > > > > >\n> > > > > > - /* Wait until error or EOS or timeout after 2\n> seconds */\n> > > > > > - constexpr GstMessageType msgType =\n> > > > > > -\n> static_cast(GST_MESSAGE_ERROR |\n> > > > > GST_MESSAGE_EOS);\n> > > > > > - constexpr GstClockTime timeout = 2 * GST_SECOND;\n> > > > > > -\n> > > > > > - g_autoptr(GstBus) bus =\n> gst_element_get_bus(pipeline_);\n> > > > > > - g_autoptr(GstMessage) msg =\n> > > gst_bus_timed_pop_filtered(bus,\n> > > > > 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\n> received.\\n\");\n> > > > > > - break;\n> > > > > > - }\n> > > > > > + if (gstreamer_process_event() != TestPass)\n> > > > > > + return TestFail;\n> > > > > >\n> > > > > > - return TestFail;\n> > > > > > + return TestPass;\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 *convert0_;\n> > > > > > GstElement *sink0_;\n> > > > > > };\n> > > > > > diff --git a/test/gstreamer/gstreamer_test.cpp\n> > > > > b/test/gstreamer/gstreamer_test.cpp\n> > > > > > new file mode 100644\n> > > > > > index 00000000..22128c4c\n> > > > > > --- /dev/null\n> > > > > > +++ b/test/gstreamer/gstreamer_test.cpp\n> > > > > > @@ -0,0 +1,157 @@\n> > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > > > + *\n> > > > > > + * libcamera Gstreamer element API tests\n> > > > > > + */\n> > > > > > +\n> > > > > > +#include \"gstreamer_test.h\"\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\n> > > by\n> > > > > > + * using a suppression file instead of disabling leak\n> detection.\n> > > > > > + */\n> > > > > > + return \"detect_leaks=false\";\n> > > > > > +}\n> > > > > > +}\n> > > > > > +\n> > > > > > +GstreamerTest::GstreamerTest()\n> > > > > > +{\n> > > > > > + /*\n> > > > > > + * GStreamer by default spawns a process to run the\n> > > > > > + * gst-plugin-scanner helper. If libcamera is compiled with\n> 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 already\n> been\n> > > > > > + * loaded by then. Fix this issue by disabling spawning of 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> > > > > > + status_ = TestFail;\n> > > > > > + return;\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> > > > > > + g_autoptr(GstPlugin) plugin = gst_registry_lookup(registry,\n> > > > > \"libgstlibcamera.so\");\n> > > > > > + if (plugin) {\n> > > > > > + gst_registry_remove_plugin(registry, 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> > > > > > +\n> > > > > > + status_ = TestFail;\n> > > > > > + return;\n> > > > > > + }\n> > > > > > +\n> > > > > > + status_ = TestPass;\n> > > > > > +}\n> > > > > > +\n> > > > > > +GstreamerTest::~GstreamerTest()\n> > > > > > +{\n> > > > > > + if (pipeline_)\n> > > > > > + gst_object_unref(pipeline_);\n> > > > > > + if (status_ == TestFail) {\n> > > > > > + gst_object_unref(libcameraSrc_);\n> > > > > > + }\n> > > > > > +\n> > > > > > + gst_deinit();\n> > > > > > +}\n> > > > > > +\n> > > > > > +int GstreamerTest::gstreamer_create_pipeline()\n> > > > > > +{\n> > > > > > + g_autoptr(GstElement) libcameraSrc__ =\n> > > > > gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > > > > > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > > > > > + g_object_ref_sink(libcameraSrc__);\n> > > > > > +\n> > > > > > + if (!libcameraSrc__ || !pipeline_) {\n> > > > > > + g_printerr(\"Unable to create create pipeline\n> %p.%p\\n\",\n> > > > > > + libcameraSrc__,\n> > > pipeline_);\n> > > > > > +\n> > > > > > + return TestFail;\n> > > > > > + }\n> > > > > > +\n> > > > > > + libcameraSrc_ = reinterpret_cast > > > > *>(g_steal_pointer(&libcameraSrc__));\n> > > > > > +\n> > > > > > + return TestPass;\n> > > > > > +}\n> > > > > > +\n> > > > > > +int GstreamerTest::gstreamer_start_pipeline()\n> > > > > > +{\n> > > > > > + GstStateChangeReturn ret;\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\n> > > > > state.\\n\");\n> > > > > > + return TestFail;\n> > > > > > + }\n> > > > > > +\n> > > > > > + return TestPass;\n> > > > > > +}\n> > > > > > +\n> > > > > > +int GstreamerTest::gstreamer_process_event()\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 = gst_bus_timed_pop_filtered(bus,\n> > > timeout,\n> > > > > 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> > > > >\n> > > > > Isn't it unfortunate design that all test must last 2s ? Just\n> opening\n> > > > > the\n> > > > > discussion, this was like this before.\n> > > > >\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> > > > > > +void GstreamerTest::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> > > > > > diff --git a/test/gstreamer/gstreamer_test.h\n> > > > > b/test/gstreamer/gstreamer_test.h\n> > > > > > new file mode 100644\n> > > > > > index 00000000..cdffdea9\n> > > > > > --- /dev/null\n> > > > > > +++ b/test/gstreamer/gstreamer_test.h\n> > > > > > @@ -0,0 +1,39 @@\n> > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2021, Vedant Paranjape\n> > > > > > + *\n> > > > > > + * gstreamer_test.cpp - GStreamer test base class\n> > > > > > + */\n> > > > > > +\n> > > > > > +#ifndef __LIBCAMERA_GSTREAMER_TEST_H__\n> > > > > > +#define __LIBCAMERA_GSTREAMER_TEST_H__\n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \"libcamera/internal/source_paths.h\"\n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +using namespace std;\n> > > > > > +\n> > > > > > +class GstreamerTest\n> > > > > > +{\n> > > > > > +public:\n> > > > > > + GstreamerTest();\n> > > > > > + virtual ~GstreamerTest();\n> > > > > > +\n> > > > > > +protected:\n> > > > > > + virtual int gstreamer_create_pipeline();\n> > > > > > + int gstreamer_start_pipeline();\n> > > > > > + int gstreamer_process_event();\n> > > > > > + void gstreamer_print_error(GstMessage *msg);\n> > > > > > +\n> > > > > > + GstElement *pipeline_;\n> > > > > > + GstElement *libcameraSrc_;\n> > > > > > + int status_;\n> > > > > > +};\n> > > > > > +\n> > > > > > +#endif /* __LIBCAMERA_GSTREAMER_TEST_H__ */\n> > > > > > diff --git a/test/gstreamer/meson.build\n> b/test/gstreamer/meson.build\n> > > > > > index b99aa0da..aca53b92 100644\n> > > > > > --- a/test/gstreamer/meson.build\n> > > > > > +++ b/test/gstreamer/meson.build\n> > > > > > @@ -10,7 +10,7 @@ gstreamer_tests = [\n> > > > > > gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> > > > > >\n> > > > > > foreach t : gstreamer_tests\n> > > > > > - exe = executable(t[0], t[1],\n> > > > > > + exe = executable(t[0], t[1], 'gstreamer_test.cpp',\n> > > > > > dependencies : [libcamera_private,\n> > > gstreamer_dep],\n> > > > > > link_with : test_libraries,\n> > > > > > include_directories :\n> test_includes_internal)\n> > > > >\n> > > > >\n> > >\n> > >\n>\n>\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 B2A16BD87D\n\tfor ;\n\tWed, 1 Sep 2021 16:19:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 34E6468934;\n\tWed, 1 Sep 2021 18:19:10 +0200 (CEST)","from mail-yb1-xb2b.google.com (mail-yb1-xb2b.google.com\n\t[IPv6:2607:f8b0:4864:20::b2b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0581260288\n\tfor ;\n\tWed, 1 Sep 2021 18:19:09 +0200 (CEST)","by mail-yb1-xb2b.google.com with SMTP id c6so5902787ybm.10\n\tfor ;\n\tWed, 01 Sep 2021 09:19:08 -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=\"UL9TKPM/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=XCeMEgU6OoXs8w5/gAmoDVqTvVzT9oD8uz0o87n3Vy8=;\n\tb=UL9TKPM/3V6PwBk3quBwA+mAiCsbkwu/fGaiihd4w8luILPfP0nC9KqnHjefgax62x\n\tQ7tzDLcA6f1wxaJrpTmTpM+q4Jxp+zFvh8bG2TtwohTyl8MP1rOVRyXMW8hbDPQbgFp9\n\tlURh2etx/wKA/O5CYrY+b0TzRTrmMTKg1E/6rnkfGXC6X3mKOQ6bgxWvW823zL0kfHlK\n\tstzWnbNlitaCxL4/02YZey+3Lpe8sMMdHSyTqsEHjGe2X/bHRGLLUPtFXq4kWxnPYU5L\n\t97MBchZ1XoYS9emPfaknPYLtot9/EloHq0V5V8EHQCgY0Fl6JTlqRsQHQKWrl36oKkXa\n\t65Yg==","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=XCeMEgU6OoXs8w5/gAmoDVqTvVzT9oD8uz0o87n3Vy8=;\n\tb=doYl4tksqbrpVGeoPIzDBvmF6Snure4QIhghKneXsN7pqga+zUtqHHG0y43x/5Cr9l\n\t3wcCF0xONYFUHBiEZ/HYYtqDFcw/ZjD5qba4ZXnhXAhtMASmw9VANedFQ1d+STPg6HJ/\n\tUQopBZgOk/9k83eGzVPdgCF8dFquD7Z3+hK7i0ZBCxBYev0VTP7z+eSKaTlhvjorCyTW\n\tqSTK6WcP2lQdIgrXnx39NGP3RozZZSnW7Gwmpj40iYSCtWRtUBfxrYxo5KGYWK9wAhJw\n\tROu0+rG01PenCYjGKnZSmcqup9qsfNbKwexY+ag2DG8SfXuIGJwvvTI4AN179JREejQS\n\t74cw==","X-Gm-Message-State":"AOAM530roEg248QIm4h7xMUMXxJgDEE7KzrHErLHNozcaEhI/X1vSAIE\n\tWf1qn/LKd7UqzQdaBMk6LZsVUsvQ4fDhmm71teg=","X-Google-Smtp-Source":"ABdhPJxBsUcGea2xAtoENk3X5t/WBHzZbwIaUaJ+iCiF1/hmqIl/hyDlYMKMVxSe0Zh9zYdDH5VDc6lNCRFiw1CK+ng=","X-Received":"by 2002:a25:99c8:: with SMTP id q8mr400674ybo.63.1630513147298; \n\tWed, 01 Sep 2021 09:19:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>\n\t<4c8aba66992febb60e9003bb6037295518c12c48.camel@ndufresne.ca>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Vedant Paranjape ","Date":"Wed, 1 Sep 2021 21:48:54 +0530","Message-ID":"","To":"Nicolas Dufresne ","Content-Type":"multipart/alternative; boundary=\"000000000000f2482805caf16cc6\"","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","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":19528,"web_url":"https://patchwork.libcamera.org/comment/19528/","msgid":"<20210908040045.GD968527@pyrite.rasen.tech>","date":"2021-09-08T04:00:45","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Vedant,\n\nOn Wed, Sep 01, 2021 at 01:32:17AM +0530, Vedant Paranjape wrote:\n> Lot of code used in single stream test is boiler plate and common across\n> every gstreamer test. Factored out this code into a base class called\n> GstreamerTest.\n> \n> Also updated the gstreamer_single_stream_test to use the GstreamerTest\n> base class\n> \n> Signed-off-by: Vedant Paranjape \n> ---\n> .../gstreamer_single_stream_test.cpp | 145 +++-------------\n> test/gstreamer/gstreamer_test.cpp | 157 ++++++++++++++++++\n> test/gstreamer/gstreamer_test.h | 39 +++++\n> test/gstreamer/meson.build | 2 +-\n> 4 files changed, 221 insertions(+), 122 deletions(-)\n> create mode 100644 test/gstreamer/gstreamer_test.cpp\n> create mode 100644 test/gstreamer/gstreamer_test.h\n> \n> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> index 4c8d4804..c27e4d36 100644\n> --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> @@ -14,104 +14,48 @@\n> \n> #include \n> \n> +#include \"gstreamer_test.h\"\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> +class GstreamerSingleStreamTest : public GstreamerTest, public Test\n> {\n> +public:\n> +\tGstreamerSingleStreamTest()\n> +\t\t: GstreamerTest()\n> +\t{\n> +\t}\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> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n> \n> -\t\t\treturn TestFail;\n> -\t\t}\n> +\t\tg_autoptr(GstElement) convert0__ = gst_element_factory_make(\"videoconvert\", \"convert0\");\n\nFor local variables, you just remove the ending underscore altogether\n(also the other instances where you do so).\n\n> +\t\tg_autoptr(GstElement) sink0__ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> +\t\tg_object_ref_sink(convert0__);\n> +\t\tg_object_ref_sink(sink0__);\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> +\t\tif (!convert0__ || !sink0__) {\n> +\t\t\tg_printerr(\"Not all elements could be created. %p.%p\\n\",\n> +\t\t\t\t convert0__, sink0__);\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> -\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\tsink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> -\n> -\t\t/* Create the empty pipeline_ */\n> -\t\tpipeline_ = gst_pipeline_new(\"test-pipeline\");\n> -\n> -\t\tif (!pipeline_ || !convert0_ || !sink0_ || !libcameraSrc_) {\n> -\t\t\tg_printerr(\"Not all elements could be created. %p.%p.%p.%p\\n\",\n> -\t\t\t\t pipeline_, convert0_, sink0_, 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 (sink0_)\n> -\t\t\t\tgst_object_unref(sink0_);\n> -\t\t\tif (libcameraSrc_)\n> -\t\t\t\tgst_object_unref(libcameraSrc_);\n> -\t\t\tgst_deinit();\n> +\t\tconvert0_ = reinterpret_cast(g_steal_pointer(&convert0__));\n> +\t\tsink0_ = reinterpret_cast(g_steal_pointer(&sink0__));\n> \n> +\t\tif (gstreamer_create_pipeline() != TestPass)\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_, convert0_, sink0_, NULL);\n> \t\tif (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) {\n> @@ -119,57 +63,16 @@ protected:\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> -\t\tif (ret == GST_STATE_CHANGE_FAILURE) {\n> -\t\t\tg_printerr(\"Unable to set the pipeline to the playing state.\\n\");\n> +\t\tif (gstreamer_start_pipeline() != TestPass)\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> +\t\tif (gstreamer_process_event() != TestPass)\n> +\t\t\treturn TestFail;\n> \n> -\t\treturn TestFail;\n> +\t\treturn TestPass;\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 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}\n> -\n> -\tGstElement *pipeline_;\n> -\tGstElement *libcameraSrc_;\n> \tGstElement *convert0_;\n> \tGstElement *sink0_;\n> };\n> diff --git a/test/gstreamer/gstreamer_test.cpp b/test/gstreamer/gstreamer_test.cpp\n> new file mode 100644\n> index 00000000..22128c4c\n> --- /dev/null\n> +++ b/test/gstreamer/gstreamer_test.cpp\n> @@ -0,0 +1,157 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Vedant Paranjape\n> + *\n> + * libcamera Gstreamer element API tests\n> + */\n> +\n> +#include \"gstreamer_test.h\"\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> +GstreamerTest::GstreamerTest()\n> +{\n> +\t/*\n> +\t* GStreamer by default spawns a process to run the\n> +\t* gst-plugin-scanner helper. If libcamera is compiled with ASan\n> +\t* enabled, and as GStreamer is most likely not, this causes the\n> +\t* ASan link order check to fail when gst-plugin-scanner\n> +\t* dlopen()s the plugin as many libraries will have already been\n> +\t* loaded by then. Fix this issue by disabling spawning of a\n> +\t* child helper process when scanning the build directory for\n> +\t* plugins.\n> +\t*/\n> +\tgst_registry_fork_set_enabled(false);\n> +\n> +\t/* Initialize GStreamer */\n> +\tg_autoptr(GError) errInit = NULL;\n> +\tif (!gst_init_check(nullptr, nullptr, &errInit)) {\n> +\t\tg_printerr(\"Could not initialize GStreamer: %s\\n\",\n> +\t\t\t errInit ? errInit->message : \"unknown error\");\n> +\n> +\t\tstatus_ = TestFail;\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t* Remove the system libcamera plugin, if any, and add the\n> +\t* plugin from the build directory.\n> +\t*/\n> +\tGstRegistry *registry = gst_registry_get();\n> +\tg_autoptr(GstPlugin) plugin = gst_registry_lookup(registry, \"libgstlibcamera.so\");\n> +\tif (plugin) {\n> +\t\tgst_registry_remove_plugin(registry, plugin);\n> +\t}\n\nYou don't need braces here.\n\n> +\n> +\tstd::string path = libcamera::utils::libcameraBuildPath() + \"src/gstreamer\";\n> +\tif (!gst_registry_scan_path(registry, path.c_str())) {\n> +\t\tg_printerr(\"Failed to add plugin to registry\\n\");\n> +\n> +\t\tstatus_ = TestFail;\n> +\t\treturn;\n> +\t}\n> +\n> +\tstatus_ = TestPass;\n> +}\n> +\n> +GstreamerTest::~GstreamerTest()\n> +{\n> +\tif (pipeline_)\n> +\t\tgst_object_unref(pipeline_);\n> +\tif (status_ == TestFail) {\n> +\t\tgst_object_unref(libcameraSrc_);\n> +\t}\n\nYou don't need braces here.\n\nHow come you unref libcameraSrc_ but not the others upon failure?\nWhat's the difference between libcameraSrc_ and the other pipeline\ncomponents? Is TestFail both sufficient and necessary to need to free\nlibcameraSrc_?\n\n> +\n> +\tgst_deinit();\n> +}\n> +\n> +int GstreamerTest::gstreamer_create_pipeline()\n\nAll of these functions should be camelCase. Also since they're all\nmembers of GstreamerTest, I think you can drop the gstreamer prefix too.\n\n> +{\n> +\tg_autoptr(GstElement) libcameraSrc__ = gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> +\tpipeline_ = gst_pipeline_new(\"test-pipeline\");\n> +\tg_object_ref_sink(libcameraSrc__);\n\nYou can just drop the underscores completely.\n\n\nHm other than those, I think it's ready.\n\n\nPaul\n\n> +\n> +\tif (!libcameraSrc__ || !pipeline_) {\n> +\t\tg_printerr(\"Unable to create create pipeline %p.%p\\n\",\n> +\t\t\t\t\t\tlibcameraSrc__, pipeline_);\n> +\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tlibcameraSrc_ = reinterpret_cast(g_steal_pointer(&libcameraSrc__));\n> +\n> +\treturn TestPass;\n> +}\n> +\n> +int GstreamerTest::gstreamer_start_pipeline()\n> +{\n> +\tGstStateChangeReturn ret;\n> +\n> +\t/* Start playing */\n> +\tret = gst_element_set_state(pipeline_, GST_STATE_PLAYING);\n> +\tif (ret == GST_STATE_CHANGE_FAILURE) {\n> +\t\tg_printerr(\"Unable to set the pipeline to the playing state.\\n\");\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\treturn TestPass;\n> +}\n> +\n> +int GstreamerTest::gstreamer_process_event()\n> +{\n> +\t/* Wait until error or EOS or timeout after 2 seconds */\n> +\tconstexpr GstMessageType msgType =\n> +\t\tstatic_cast(GST_MESSAGE_ERROR | GST_MESSAGE_EOS);\n> +\tconstexpr GstClockTime timeout = 2 * GST_SECOND;\n> +\n> +\tg_autoptr(GstBus) bus = gst_element_get_bus(pipeline_);\n> +\tg_autoptr(GstMessage) msg = gst_bus_timed_pop_filtered(bus, timeout, msgType);\n> +\n> +\tgst_element_set_state(pipeline_, GST_STATE_NULL);\n> +\n> +\t/* Parse error message */\n> +\tif (msg == NULL) {\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tswitch (GST_MESSAGE_TYPE(msg)) {\n> +\tcase GST_MESSAGE_ERROR:\n> +\t\tgstreamer_print_error(msg);\n> +\t\tbreak;\n> +\tcase GST_MESSAGE_EOS:\n> +\t\tg_print(\"End-Of-Stream reached.\\n\");\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tg_printerr(\"Unexpected message received.\\n\");\n> +\t\tbreak;\n> +\t}\n> +\n> +\treturn TestFail;\n> +}\n> +\n> +void GstreamerTest::gstreamer_print_error(GstMessage *msg)\n> +{\n> +\tg_autoptr(GError) err = NULL;\n> +\tg_autofree gchar *debug_info = NULL;\n> +\n> +\tgst_message_parse_error(msg, &err, &debug_info);\n> +\tg_printerr(\"Error received from element %s: %s\\n\",\n> +\t\t GST_OBJECT_NAME(msg->src), err->message);\n> +\tg_printerr(\"Debugging information: %s\\n\",\n> +\t\t debug_info ? debug_info : \"none\");\n> +}\n> +\n> diff --git a/test/gstreamer/gstreamer_test.h b/test/gstreamer/gstreamer_test.h\n> new file mode 100644\n> index 00000000..cdffdea9\n> --- /dev/null\n> +++ b/test/gstreamer/gstreamer_test.h\n> @@ -0,0 +1,39 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Vedant Paranjape\n> + *\n> + * gstreamer_test.cpp - GStreamer test base class\n> + */\n> +\n> +#ifndef __LIBCAMERA_GSTREAMER_TEST_H__\n> +#define __LIBCAMERA_GSTREAMER_TEST_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/source_paths.h\"\n> +\n> +#include \n> +\n> +using namespace std;\n> +\n> +class GstreamerTest\n> +{\n> +public:\n> +\tGstreamerTest();\n> +\tvirtual ~GstreamerTest();\n> +\n> +protected:\n> +\tvirtual int gstreamer_create_pipeline();\n> +\tint gstreamer_start_pipeline();\n> +\tint gstreamer_process_event();\n> +\tvoid gstreamer_print_error(GstMessage *msg);\n> +\n> +\tGstElement *pipeline_;\n> +\tGstElement *libcameraSrc_;\n> +\tint status_;\n> +};\n> +\n> +#endif /* __LIBCAMERA_GSTREAMER_TEST_H__ */\n> diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> index b99aa0da..aca53b92 100644\n> --- a/test/gstreamer/meson.build\n> +++ b/test/gstreamer/meson.build\n> @@ -10,7 +10,7 @@ gstreamer_tests = [\n> gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> \n> foreach t : gstreamer_tests\n> - exe = executable(t[0], t[1],\n> + exe = executable(t[0], t[1], 'gstreamer_test.cpp',\n> dependencies : [libcamera_private, gstreamer_dep],\n> link_with : test_libraries,\n> include_directories : test_includes_internal)\n> -- \n> 2.25.1\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 DAF0FBDB1D\n\tfor ;\n\tWed, 8 Sep 2021 04:00:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40FC26916E;\n\tWed, 8 Sep 2021 06:00:55 +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 E0B7F6024C\n\tfor ;\n\tWed, 8 Sep 2021 06:00:53 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4941C317;\n\tWed, 8 Sep 2021 06:00:51 +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=\"OT8s5x3U\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631073653;\n\tbh=LD5d+CEYWsLVhhsyFNF3HbMfj1ByWXz21oXAwrB2Vbw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OT8s5x3UyevLSpGoAYsEs1sPRuAH3LQL/vIC+0BFwuGR2VeR5YlRnMJrQrFNQlNuI\n\td6K/oxSAailHwUKr8RmhQspfxmTu97gfkRhYyApTVh5mtS+JuMl1xFUQBFpzBkOrCw\n\tB3z6af2UIvfszmREWuk8tRJ0kmaFee5CtsKA41js=","Date":"Wed, 8 Sep 2021 13:00:45 +0900","From":"paul.elder@ideasonboard.com","To":"Vedant Paranjape ","Message-ID":"<20210908040045.GD968527@pyrite.rasen.tech>","References":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","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":19529,"web_url":"https://patchwork.libcamera.org/comment/19529/","msgid":"","date":"2021-09-08T04:14:58","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Paul,\nThanks for the review.\n\n\nOn Wed, Sep 8, 2021 at 9:30 AM wrote:\n\n> Hi Vedant,\n>\n> On Wed, Sep 01, 2021 at 01:32:17AM +0530, Vedant Paranjape wrote:\n> > Lot of code used in single stream test is boiler plate and common across\n> > every gstreamer test. Factored out this code into a base class called\n> > GstreamerTest.\n> >\n> > Also updated the gstreamer_single_stream_test to use the GstreamerTest\n> > base class\n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > .../gstreamer_single_stream_test.cpp | 145 +++-------------\n> > test/gstreamer/gstreamer_test.cpp | 157 ++++++++++++++++++\n> > test/gstreamer/gstreamer_test.h | 39 +++++\n> > test/gstreamer/meson.build | 2 +-\n> > 4 files changed, 221 insertions(+), 122 deletions(-)\n> > create mode 100644 test/gstreamer/gstreamer_test.cpp\n> > create mode 100644 test/gstreamer/gstreamer_test.h\n> >\n> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > index 4c8d4804..c27e4d36 100644\n> > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > @@ -14,104 +14,48 @@\n> >\n> > #include \n> >\n> > +#include \"gstreamer_test.h\"\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> > +class GstreamerSingleStreamTest : public GstreamerTest, public Test\n> > {\n> > +public:\n> > + GstreamerSingleStreamTest()\n> > + : GstreamerTest()\n> > + {\n> > + }\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> > + if (status_ != TestPass)\n> > + return status_;\n> >\n> > - return TestFail;\n> > - }\n> > + g_autoptr(GstElement) convert0__ =\n> gst_element_factory_make(\"videoconvert\", \"convert0\");\n>\n> For local variables, you just remove the ending underscore altogether\n> (also the other instances where you do so).\n>\n\nOkay, done !\n\n> + g_autoptr(GstElement) sink0__ =\n> gst_element_factory_make(\"fakesink\", \"sink0\");\n> > + g_object_ref_sink(convert0__);\n> > + g_object_ref_sink(sink0__);\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> > + if (!convert0__ || !sink0__) {\n> > + g_printerr(\"Not all elements could be created.\n> %p.%p\\n\",\n> > + convert0__, sink0__);\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> > - /* Create the elements */\n> > - libcameraSrc_ = gst_element_factory_make(\"libcamerasrc\",\n> \"libcamera\");\n> > - convert0_ = gst_element_factory_make(\"videoconvert\",\n> \"convert0\");\n> > - sink0_ = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > -\n> > - /* Create the empty pipeline_ */\n> > - pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > -\n> > - if (!pipeline_ || !convert0_ || !sink0_ || !libcameraSrc_)\n> {\n> > - g_printerr(\"Not all elements could be created.\n> %p.%p.%p.%p\\n\",\n> > - pipeline_, convert0_, sink0_,\n> libcameraSrc_);\n> > - if (pipeline_)\n> > - gst_object_unref(pipeline_);\n> > - if (convert0_)\n> > - gst_object_unref(convert0_);\n> > - if (sink0_)\n> > - gst_object_unref(sink0_);\n> > - if (libcameraSrc_)\n> > - gst_object_unref(libcameraSrc_);\n> > - gst_deinit();\n> > + convert0_ = reinterpret_cast *>(g_steal_pointer(&convert0__));\n> > + sink0_ = reinterpret_cast *>(g_steal_pointer(&sink0__));\n> >\n> > + if (gstreamer_create_pipeline() != TestPass)\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> convert0_, sink0_, NULL);\n> > if (gst_element_link_many(libcameraSrc_, convert0_,\n> sink0_, NULL) != TRUE) {\n> > @@ -119,57 +63,16 @@ protected:\n> > return TestFail;\n> > }\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> > + if (gstreamer_start_pipeline() != TestPass)\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> > + if (gstreamer_process_event() != TestPass)\n> > + return TestFail;\n> >\n> > - return TestFail;\n> > + return TestPass;\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 *convert0_;\n> > GstElement *sink0_;\n> > };\n> > diff --git a/test/gstreamer/gstreamer_test.cpp\n> b/test/gstreamer/gstreamer_test.cpp\n> > new file mode 100644\n> > index 00000000..22128c4c\n> > --- /dev/null\n> > +++ b/test/gstreamer/gstreamer_test.cpp\n> > @@ -0,0 +1,157 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Vedant Paranjape\n> > + *\n> > + * libcamera Gstreamer element API tests\n> > + */\n> > +\n> > +#include \"gstreamer_test.h\"\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> > +GstreamerTest::GstreamerTest()\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> > + status_ = TestFail;\n> > + return;\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> > + g_autoptr(GstPlugin) plugin = gst_registry_lookup(registry,\n> \"libgstlibcamera.so\");\n> > + if (plugin) {\n> > + gst_registry_remove_plugin(registry, plugin);\n> > + }\n>\n> You don't need braces here.\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> > +\n> > + status_ = TestFail;\n> > + return;\n> > + }\n> > +\n> > + status_ = TestPass;\n> > +}\n> > +\n> > +GstreamerTest::~GstreamerTest()\n> > +{\n> > + if (pipeline_)\n> > + gst_object_unref(pipeline_);\n> > + if (status_ == TestFail) {\n> > + gst_object_unref(libcameraSrc_);\n> > + }\n>\n> You don't need braces here.\n>\n> How come you unref libcameraSrc_ but not the others upon failure?\n> What's the difference between libcameraSrc_ and the other pipeline\n> components? Is TestFail both sufficient and necessary to need to free\n> libcameraSrc_?\n>\n\nThis is because the other elements are added to the pipeline, once that is\ndone successfully the pipeline owns them and takes care of their existence.\nSo, there's going to be a minor change here though. But, in this case,\nsince create pipeline is called before the elements are added to the\npipeline, I need\nto take care of libcameraSrc.\n\n> +\n> > + gst_deinit();\n> > +}\n> > +\n> > +int GstreamerTest::gstreamer_create_pipeline()\n>\n> All of these functions should be camelCase. Also since they're all\n> members of GstreamerTest, I think you can drop the gstreamer prefix too.\n>\n\nOkay\n\n> +{\n> > + g_autoptr(GstElement) libcameraSrc__ =\n> gst_element_factory_make(\"libcamerasrc\", \"libcamera\");\n> > + pipeline_ = gst_pipeline_new(\"test-pipeline\");\n> > + g_object_ref_sink(libcameraSrc__);\n>\n> You can just drop the underscores completely.\n>\n>\n> Hm other than those, I think it's ready.\n>\n>\n> Paul\n>\n> > +\n> > + if (!libcameraSrc__ || !pipeline_) {\n> > + g_printerr(\"Unable to create create pipeline %p.%p\\n\",\n> > + libcameraSrc__, pipeline_);\n> > +\n> > + return TestFail;\n> > + }\n> > +\n> > + libcameraSrc_ = reinterpret_cast *>(g_steal_pointer(&libcameraSrc__));\n> > +\n> > + return TestPass;\n> > +}\n> > +\n> > +int GstreamerTest::gstreamer_start_pipeline()\n> > +{\n> > + GstStateChangeReturn ret;\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\n> state.\\n\");\n> > + return TestFail;\n> > + }\n> > +\n> > + return TestPass;\n> > +}\n> > +\n> > +int GstreamerTest::gstreamer_process_event()\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 = gst_bus_timed_pop_filtered(bus,\n> 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> > +\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> > +void GstreamerTest::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> > diff --git a/test/gstreamer/gstreamer_test.h\n> b/test/gstreamer/gstreamer_test.h\n> > new file mode 100644\n> > index 00000000..cdffdea9\n> > --- /dev/null\n> > +++ b/test/gstreamer/gstreamer_test.h\n> > @@ -0,0 +1,39 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Vedant Paranjape\n> > + *\n> > + * gstreamer_test.cpp - GStreamer test base class\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_GSTREAMER_TEST_H__\n> > +#define __LIBCAMERA_GSTREAMER_TEST_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/source_paths.h\"\n> > +\n> > +#include \n> > +\n> > +using namespace std;\n> > +\n> > +class GstreamerTest\n> > +{\n> > +public:\n> > + GstreamerTest();\n> > + virtual ~GstreamerTest();\n> > +\n> > +protected:\n> > + virtual int gstreamer_create_pipeline();\n> > + int gstreamer_start_pipeline();\n> > + int gstreamer_process_event();\n> > + void gstreamer_print_error(GstMessage *msg);\n> > +\n> > + GstElement *pipeline_;\n> > + GstElement *libcameraSrc_;\n> > + int status_;\n> > +};\n> > +\n> > +#endif /* __LIBCAMERA_GSTREAMER_TEST_H__ */\n> > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > index b99aa0da..aca53b92 100644\n> > --- a/test/gstreamer/meson.build\n> > +++ b/test/gstreamer/meson.build\n> > @@ -10,7 +10,7 @@ gstreamer_tests = [\n> > gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> >\n> > foreach t : gstreamer_tests\n> > - exe = executable(t[0], t[1],\n> > + exe = executable(t[0], t[1], 'gstreamer_test.cpp',\n> > dependencies : [libcamera_private, gstreamer_dep],\n> > link_with : test_libraries,\n> > include_directories : test_includes_internal)\n> > --\n> > 2.25.1\n> >\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 45A1ABDC71\n\tfor ;\n\tWed, 8 Sep 2021 04:15:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE95A69170;\n\tWed, 8 Sep 2021 06:15:12 +0200 (CEST)","from mail-yb1-xb2d.google.com (mail-yb1-xb2d.google.com\n\t[IPv6:2607:f8b0:4864:20::b2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1358B6024C\n\tfor ;\n\tWed, 8 Sep 2021 06:15:11 +0200 (CEST)","by mail-yb1-xb2d.google.com with SMTP id v10so828974ybq.7\n\tfor ;\n\tTue, 07 Sep 2021 21:15:10 -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=\"fxzraffU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=niOdybyqyUEoM8olqsxaC3/rdaa0Eo9XQxBa7R+1De4=;\n\tb=fxzraffUWbUQuFciWSaULihFvhIpEudC6gBOhUUBjWcTmCXpKTQ8Yf6DkuhOp4O3Ry\n\ttl3IUg+f/DVGbGmuJpnUHyL7UjatRUgxWYFCV8K5ZD4IDWXtRbg54QiZUN3oOIVqHA/S\n\ty+7sEyjL3a9lvZdMBIOmilPmlrTIrKOv7gWVICLhaj5FNpYRnHkLK03aNW0vTC/r4/4j\n\tbVshGtx3dWFJXXOFgEHrbjlBa/a5teCyzLvw+0xqsXwn1LDJi0JXer1RTT2G6qheDrl1\n\twzpvX0xwP9RXaZB995QMdckr70X3RC/yezJ8+TcBNXoYFYIRDAmfcJBFENlgrTqU0pbj\n\tGStQ==","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=niOdybyqyUEoM8olqsxaC3/rdaa0Eo9XQxBa7R+1De4=;\n\tb=LUj906WsYy9xluI6l9pUFxId0jHxNZK+cVgYnqOBxrC4v3cXamtCulZsKSVNYfYjtP\n\t2kU2jpHk58vQMcyuTRKLeOLoQuoiesLr2T8+AWQpnf5sCRp/42FV0YFlFAwny0rjdPmW\n\tBsGsDB8F+lsNbDyODmAwWDaxfiYHj0fxDfXaE+k2cZ/uvaWGCx/fib7XgG6IMStzxd/0\n\tAIrthnodwC2qRlpH6yyC/omTTrH1K/10DZP77TMDBcBWY+/ZKT2g+ABTQ+ilvHmwZCIL\n\tBXlJ6vs86EuRKDUiDtIVdHpdOf5Go3o7SISHf584XI0O3hFXEjEaVj0zFoBPaTc0EMR7\n\tSqrQ==","X-Gm-Message-State":"AOAM532QIgtqnn19FbUgV1wTMRAIizxd/mLEdN/qNPl/SdgV8Msz/6gb\n\toDbyXBI4NM0z58ILTlDj+KqPHlHXRdSlrwEaIn+on5cBwGU=","X-Google-Smtp-Source":"ABdhPJwYhXaoHzIEGgA2DSrq2aFQy4ilIiXXWx9iIrwWMP85cGN9kfxDz9r4ciIB+UVErqHVs2xov0dDH3SUoob8mBs=","X-Received":"by 2002:a25:76ce:: with SMTP id\n\tr197mr2611483ybc.242.1631074509496; \n\tTue, 07 Sep 2021 21:15:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20210831200217.1323962-1-vedantparanjape160201@gmail.com>\n\t<20210908040045.GD968527@pyrite.rasen.tech>","In-Reply-To":"<20210908040045.GD968527@pyrite.rasen.tech>","From":"Vedant Paranjape ","Date":"Wed, 8 Sep 2021 09:44:58 +0530","Message-ID":"","To":"Paul Elder ","Content-Type":"multipart/alternative; boundary=\"000000000000bda6c205cb7420bf\"","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Factor out code\n\tinto a base class","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\" "}}]