[{"id":19668,"web_url":"https://patchwork.libcamera.org/comment/19668/","msgid":"<89c39c9a-9b01-8e81-3436-f5124e654771@ideasonboard.com>","date":"2021-09-14T07:23:45","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Add a test for\n\tgstreamer multi stream","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Vedant,\n\nOn 9/14/21 12:04 PM, Vedant Paranjape wrote:\n> This patch adds a test to test if multi stream using libcamera's\n> gstreamer element works.\n\n\nCan you please document how and when this test is run?\n\nCurrently, my system (with one UVC and vimc) has been able to run \nsingle_stream_test but multi_stream_test is run:\n\n\n11/65 libcamera:gstreamer / single_stream_test OK             2.53s\n12/65 libcamera:gstreamer / multi_stream_test SKIP           0.07s\n\nWhat's your test setup where this tests passes. I assume I have to \nsatisfy the following condition from the test, in order to run:\n\n                 if (camera->streams().size() > 1) {\n\n                         cameraFound = true;\n\n                         ....\n\n                 }\n\nBut I don't know how, looking at the patch. So can you explain (not just \nhere but in the patch / commit message as well) ? It shall help others \nas well.\n\n>\n> Signed-off-by: Vedant Paranjape \n> Reviewed-by: Paul Elder \n> Tested-by: Paul Elder \n> ---\n> .../gstreamer/gstreamer_multi_stream_test.cpp | 138 ++++++++++++++++++\n> test/gstreamer/meson.build | 1 +\n> 2 files changed, 139 insertions(+)\n> create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp\n\n\nRight off the bat, there is a checkstyle failure: Can you look into it, \nplease?\n\n[uajain@fedora libcamera]$ ./utils/checkstyle.py HEAD\n-----------------------------------------------------------------------------------------------\n9d0efed89b03d57790db4eebefa909bf169ce789 test: gstreamer: Add a test for \ngstreamer multi stream\n-----------------------------------------------------------------------------------------------\n--- test/gstreamer/gstreamer_multi_stream_test.cpp\n+++ test/gstreamer/gstreamer_multi_stream_test.cpp\n@@ -104,8 +104,7 @@\n          GstPad *queue0_sink_pad = gst_element_get_static_pad(queue0_, \n\"sink\");\n          GstPad *queue1_sink_pad = gst_element_get_static_pad(queue1_, \n\"sink\");\n\n-        if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK\n-            || gst_pad_link(request_pad, queue1_sink_pad) != \nGST_PAD_LINK_OK) {\n+        if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK \n|| gst_pad_link(request_pad, queue1_sink_pad) != GST_PAD_LINK_OK) {\n              if (queue0_sink_pad)\n                  gst_object_unref(queue0_sink_pad);\n              if (queue1_sink_pad)\n---\n1 potential issue detected, please review\n\n>\n> diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> new file mode 100644\n> index 000000000000..aba86a3f8e27\n> --- /dev/null\n> +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> @@ -0,0 +1,138 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Vedant Paranjape\n> + *\n> + * gstreamer_multi_stream_test.cpp - GStreamer multi stream capture test\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/source_paths.h\"\n> +\n> +#include \n> +\n> +#include \"gstreamer_test.h\"\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +\n> +class GstreamerMultiStreamTest : public GstreamerTest, public Test\n> +{\n> +public:\n> +\tGstreamerMultiStreamTest()\n> +\t\t: GstreamerTest()\n> +\t{\n> +\t}\n> +\n> +protected:\n> +\tint init() override\n> +\t{\n> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n> +\n> +\t\t/* Check if platform support multistream output */\n> +\t\tlibcamera::CameraManager cm;\n> +\t\tcm.start();\n> +\t\tbool cameraFound = false;\n> +\t\tfor (auto &camera : cm.cameras()) {\n> +\t\t\tif (camera->streams().size() > 1) {\n> +\t\t\t\tcameraName = camera->id();\n> +\t\t\t\tcameraFound = true;\n> +\t\t\t\tcm.stop();\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (!cameraFound) {\n> +\t\t\tcm.stop();\n> +\t\t\treturn TestSkip;\n> +\t\t}\n> +\n> +\t\tg_autoptr(GstElement) convert0 = gst_element_factory_make(\"videoconvert\", \"convert0\");\n> +\t\tg_autoptr(GstElement) convert1 = gst_element_factory_make(\"videoconvert\", \"convert1\");\n> +\t\tg_autoptr(GstElement) sink0 = gst_element_factory_make(\"fakesink\", \"sink0\");\n> +\t\tg_autoptr(GstElement) sink1 = gst_element_factory_make(\"fakesink\", \"sink1\");\n> +\t\tg_autoptr(GstElement) queue0 = gst_element_factory_make(\"queue\", \"queue0\");\n> +\t\tg_autoptr(GstElement) queue1 = gst_element_factory_make(\"queue\", \"queue1\");\n> +\t\tg_object_ref_sink(convert0);\n> +\t\tg_object_ref_sink(convert1);\n> +\t\tg_object_ref_sink(sink0);\n> +\t\tg_object_ref_sink(sink1);\n> +\t\tg_object_ref_sink(queue0);\n> +\t\tg_object_ref_sink(queue1);\n\n\nWhat's the idea here of first taking ownership by locally scope \ng_autoptr() pointers and then setting to the private members below? Is \nit to validate/null-check them? I would directly set to private members \nand null-check them directly instead, and if it fails, so be it and \nreturn TestFail. Would you be tempted to take this route?\n\n> +\n> +\t\tif (!convert0 || !convert1 || !sink0 || !sink1 || !queue0 || !queue1) {\n> +\t\t\tg_printerr(\"Not all elements could be created. %p.%p.%p.%p.%p.%p\\n\",\n> +\t\t\t\t convert0, convert1, sink0, sink1, queue0, queue1);\n> +\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tconvert0_ = reinterpret_cast(g_steal_pointer(&convert0));\n> +\t\tconvert1_ = reinterpret_cast(g_steal_pointer(&convert1));\n> +\t\tsink0_ = reinterpret_cast(g_steal_pointer(&sink0));\n> +\t\tsink1_ = reinterpret_cast(g_steal_pointer(&sink1));\n> +\t\tqueue0_ = reinterpret_cast(g_steal_pointer(&queue0));\n> +\t\tqueue1_ = reinterpret_cast(g_steal_pointer(&queue1));\n> +\n> +\t\tif (createPipeline() != TestPass)\n> +\t\t\treturn TestFail;\n\n\nShould this `if` block be present before you set the elements above?\n\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\tg_object_set(libcameraSrc_, \"camera-name\", cameraName.c_str(), NULL);\n> +\n> +\t\t/* Build the pipeline */\n> +\t\tgst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, queue0_, queue1_,\n> +\t\t\t\t convert0_, convert1_, sink0_, sink1_, NULL);\n> +\t\tif (gst_element_link_many(queue0_, convert0_, sink0_, NULL) != TRUE ||\n> +\t\t gst_element_link_many(queue1_, convert1_, sink1_, NULL) != TRUE) {\n> +\t\t\tg_printerr(\"Elements could not be linked.\\n\");\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tg_autoptr(GstPad) src_pad = gst_element_get_static_pad(libcameraSrc_, \"src\");\n> +\t\tg_autoptr(GstPad) request_pad = gst_element_get_request_pad(libcameraSrc_, \"src_%u\");\n> +\t\tGstPad *queue0_sink_pad = gst_element_get_static_pad(queue0_, \"sink\");\n> +\t\tGstPad *queue1_sink_pad = gst_element_get_static_pad(queue1_, \"sink\");\n\n\nWhy can't you use g_autoptr() for queue0_sink_pad and queue1_sink_pad \ntoo? gst_element_get_static_pad() will return a [Full] transfer-able \nvalue, so it should be un-reffed after usage (like you do below). If you \nuse g_autoptr() the manual _unref shall be taken care of by itself no? \nAm I missing something? It'll help cleanup error paths.\n\n> +\n> +\t\tif (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK\n> +\t\t\t|| gst_pad_link(request_pad, queue1_sink_pad) != GST_PAD_LINK_OK) {\n> +\t\t\tif (queue0_sink_pad)\n> +\t\t\t\tgst_object_unref(queue0_sink_pad);\n> +\t\t\tif (queue1_sink_pad)\n> +\t\t\t\tgst_object_unref(queue1_sink_pad);\n> +\t\t\tg_printerr(\"Pads could not be linked.\\n\");\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\t\tgst_object_unref(queue0_sink_pad);\n> +\t\tgst_object_unref(queue1_sink_pad);\n> +\n> +\t\tif (startPipeline() != TestPass)\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (processEvent() != TestPass)\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +private:\n> +\tstd::string cameraName;\n> +\tGstElement *convert0_;\n> +\tGstElement *convert1_;\n> +\tGstElement *sink0_;\n> +\tGstElement *sink1_;\n> +\tGstElement *queue0_;\n> +\tGstElement *queue1_;\n> +};\n> +\n> +TEST_REGISTER(GstreamerMultiStreamTest)\n> diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> index aca53b920365..13652e87d05c 100644\n> --- a/test/gstreamer/meson.build\n> +++ b/test/gstreamer/meson.build\n> @@ -6,6 +6,7 @@ endif\n> \n> gstreamer_tests = [\n> ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> + ['multi_stream_test', 'gstreamer_multi_stream_test.cpp'],\n> ]\n> gstreamer_dep = dependency('gstreamer-1.0', required: true)\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DF6AEBDB1D\n\tfor ;\n\tTue, 14 Sep 2021 07:23:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4309169189;\n\tTue, 14 Sep 2021 09:23:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5648C60249\n\tfor ;\n\tTue, 14 Sep 2021 09:23:51 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E0BF2A5;\n\tTue, 14 Sep 2021 09:23:49 +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=\"kMZCPVuT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631604230;\n\tbh=V8vdKLL3cJZSx5BlHGW9GpcLVl/hesojiAtJaikzIdo=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=kMZCPVuTRJbl9TU5HR7r84rZlY18vDyeEOOi4Y4GUFgKPPTAlRoWgFsW7FmT47hp+\n\ticdzOY9SSJBDGkLs8pqXkCarz0ZJMOH1kCl6ouTGxqzcQ4qidRlfDc/Kf5VkfpAJIN\n\t0RgGJp79igwHXornP+F4Ikacc5a3zG6Q+WPnFoHw=","To":"Vedant Paranjape ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210914063404.53621-1-vedantparanjape160201@gmail.com>","From":"Umang Jain ","Message-ID":"<89c39c9a-9b01-8e81-3436-f5124e654771@ideasonboard.com>","Date":"Tue, 14 Sep 2021 12:53:45 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210914063404.53621-1-vedantparanjape160201@gmail.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Add a test for\n\tgstreamer multi stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19670,"web_url":"https://patchwork.libcamera.org/comment/19670/","msgid":"","date":"2021-09-14T08:42:30","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Add a test for\n\tgstreamer multi stream","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Umang,\n\n\nOn Tue, Sep 14, 2021 at 12:53 PM Umang Jain \nwrote:\n\n> Hi Vedant,\n>\n> On 9/14/21 12:04 PM, Vedant Paranjape wrote:\n> > This patch adds a test to test if multi stream using libcamera's\n> > gstreamer element works.\n>\n>\n> Can you please document how and when this test is run?\n>\n> Currently, my system (with one UVC and vimc) has been able to run\n> single_stream_test but multi_stream_test is run:\n>\n>\n> 11/65 libcamera:gstreamer / single_stream_test OK 2.53s\n> 12/65 libcamera:gstreamer / multi_stream_test SKIP 0.07s\n>\n> What's your test setup where this tests passes. I assume I have to\n> satisfy the following condition from the test, in order to run:\n>\n> if (camera->streams().size() > 1) {\n>\n> cameraFound = true;\n>\n> ....\n>\n> }\n>\n> But I don't know how, looking at the patch. So can you explain (not just\n> here but in the patch / commit message as well) ? It shall help others\n> as well.\n>\n\nMultistream will run on rpi and IPU pipelines only. Will add it to commit\nmessage.\n\n>\n> > Signed-off-by: Vedant Paranjape \n> > Reviewed-by: Paul Elder \n> > Tested-by: Paul Elder \n> > ---\n> > .../gstreamer/gstreamer_multi_stream_test.cpp | 138 ++++++++++++++++++\n> > test/gstreamer/meson.build | 1 +\n> > 2 files changed, 139 insertions(+)\n> > create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp\n>\n>\n> Right off the bat, there is a checkstyle failure: Can you look into it,\n> please?\n>\n> [uajain@fedora libcamera]$ ./utils/checkstyle.py HEAD\n>\n> -----------------------------------------------------------------------------------------------\n> 9d0efed89b03d57790db4eebefa909bf169ce789 test: gstreamer: Add a test for\n> gstreamer multi stream\n>\n> -----------------------------------------------------------------------------------------------\n> --- test/gstreamer/gstreamer_multi_stream_test.cpp\n> +++ test/gstreamer/gstreamer_multi_stream_test.cpp\n> @@ -104,8 +104,7 @@\n> GstPad *queue0_sink_pad = gst_element_get_static_pad(queue0_,\n> \"sink\");\n> GstPad *queue1_sink_pad = gst_element_get_static_pad(queue1_,\n> \"sink\");\n>\n> - if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK\n> - || gst_pad_link(request_pad, queue1_sink_pad) !=\n> GST_PAD_LINK_OK) {\n> + if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK\n> || gst_pad_link(request_pad, queue1_sink_pad) != GST_PAD_LINK_OK) {\n> if (queue0_sink_pad)\n> gst_object_unref(queue0_sink_pad);\n> if (queue1_sink_pad)\n> ---\n> 1 potential issue detected, please review\n>\n\nPlease see Paul's review !\n\n>\n> > diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp\n> b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> > new file mode 100644\n> > index 000000000000..aba86a3f8e27\n> > --- /dev/null\n> > +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> > @@ -0,0 +1,138 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Vedant Paranjape\n> > + *\n> > + * gstreamer_multi_stream_test.cpp - GStreamer multi stream capture test\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/source_paths.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"gstreamer_test.h\"\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> > +\n> > +class GstreamerMultiStreamTest : public GstreamerTest, public Test\n> > +{\n> > +public:\n> > + GstreamerMultiStreamTest()\n> > + : GstreamerTest()\n> > + {\n> > + }\n> > +\n> > +protected:\n> > + int init() override\n> > + {\n> > + if (status_ != TestPass)\n> > + return status_;\n> > +\n> > + /* Check if platform support multistream output */\n> > + libcamera::CameraManager cm;\n> > + cm.start();\n> > + bool cameraFound = false;\n> > + for (auto &camera : cm.cameras()) {\n> > + if (camera->streams().size() > 1) {\n> > + cameraName = camera->id();\n> > + cameraFound = true;\n> > + cm.stop();\n> > + break;\n> > + }\n> > + }\n> > +\n> > + if (!cameraFound) {\n> > + cm.stop();\n> > + return TestSkip;\n> > + }\n> > +\n> > + g_autoptr(GstElement) convert0 =\n> gst_element_factory_make(\"videoconvert\", \"convert0\");\n> > + g_autoptr(GstElement) convert1 =\n> gst_element_factory_make(\"videoconvert\", \"convert1\");\n> > + g_autoptr(GstElement) sink0 =\n> gst_element_factory_make(\"fakesink\", \"sink0\");\n> > + g_autoptr(GstElement) sink1 =\n> gst_element_factory_make(\"fakesink\", \"sink1\");\n> > + g_autoptr(GstElement) queue0 =\n> gst_element_factory_make(\"queue\", \"queue0\");\n> > + g_autoptr(GstElement) queue1 =\n> gst_element_factory_make(\"queue\", \"queue1\");\n> > + g_object_ref_sink(convert0);\n> > + g_object_ref_sink(convert1);\n> > + g_object_ref_sink(sink0);\n> > + g_object_ref_sink(sink1);\n> > + g_object_ref_sink(queue0);\n> > + g_object_ref_sink(queue1);\n>\n>\n> What's the idea here of first taking ownership by locally scope\n> g_autoptr() pointers and then setting to the private members below? Is\n> it to validate/null-check them? I would directly set to private members\n> and null-check them directly instead, and if it fails, so be it and\n> return TestFail. Would you be tempted to take this route?\n>\n\nI was doing it before, and it let to a lot of repetitive code and buggy\nerror handling. Also, Laurent and Nicolas were kinda reserved against it.\n\n> +\n> > + if (!convert0 || !convert1 || !sink0 || !sink1 || !queue0\n> || !queue1) {\n> > + g_printerr(\"Not all elements could be created.\n> %p.%p.%p.%p.%p.%p\\n\",\n> > + convert0, convert1, sink0, sink1,\n> queue0, queue1);\n> > +\n> > + return TestFail;\n> > + }\n> > +\n> > + convert0_ = reinterpret_cast *>(g_steal_pointer(&convert0));\n> > + convert1_ = reinterpret_cast *>(g_steal_pointer(&convert1));\n> > + sink0_ = reinterpret_cast *>(g_steal_pointer(&sink0));\n> > + sink1_ = reinterpret_cast *>(g_steal_pointer(&sink1));\n> > + queue0_ = reinterpret_cast *>(g_steal_pointer(&queue0));\n> > + queue1_ = reinterpret_cast *>(g_steal_pointer(&queue1));\n> > +\n> > + if (createPipeline() != TestPass)\n> > + return TestFail;\n>\n>\n> Should this `if` block be present before you set the elements above?\n>\n\nThis was a good catch, all those elements won't be unrefed if\ncreatePipeline fails. I will make the fix !\n\n> +\n> > + return TestPass;\n> > + }\n> > +\n> > + int run() override\n> > + {\n> > + g_object_set(libcameraSrc_, \"camera-name\",\n> cameraName.c_str(), NULL);\n> > +\n> > + /* Build the pipeline */\n> > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n> queue0_, queue1_,\n> > + convert0_, convert1_, sink0_, sink1_,\n> NULL);\n> > + if (gst_element_link_many(queue0_, convert0_, sink0_,\n> NULL) != TRUE ||\n> > + gst_element_link_many(queue1_, convert1_, sink1_,\n> NULL) != TRUE) {\n> > + g_printerr(\"Elements could not be linked.\\n\");\n> > + return TestFail;\n> > + }\n> > +\n> > + g_autoptr(GstPad) src_pad =\n> gst_element_get_static_pad(libcameraSrc_, \"src\");\n> > + g_autoptr(GstPad) request_pad =\n> gst_element_get_request_pad(libcameraSrc_, \"src_%u\");\n> > + GstPad *queue0_sink_pad =\n> gst_element_get_static_pad(queue0_, \"sink\");\n> > + GstPad *queue1_sink_pad =\n> gst_element_get_static_pad(queue1_, \"sink\");\n>\n>\n> Why can't you use g_autoptr() for queue0_sink_pad and queue1_sink_pad\n> too? gst_element_get_static_pad() will return a [Full] transfer-able\n> value, so it should be un-reffed after usage (like you do below). If you\n> use g_autoptr() the manual _unref shall be taken care of by itself no?\n> Am I missing something? It'll help cleanup error paths.\n>\n\nThe pad must be unrefed immediately after use, that is after the pads are\nlinked, and if I use an autoptr it won't unref right after the if.\nI had thought to put the code where pads are created and linked into an\ninner scope like this, but then it will be over kill.\n\nfunc\n{\n { // code // }\n}\n\nI assume this is a nitpick and not a blocker. I don't see much improvement\nby doing so, It'd be a different scenario if I had, say 10 pads.\n\n> +\n> > + if (gst_pad_link(src_pad, queue0_sink_pad) !=\n> GST_PAD_LINK_OK\n> > + || gst_pad_link(request_pad, queue1_sink_pad) !=\n> GST_PAD_LINK_OK) {\n> > + if (queue0_sink_pad)\n> > + gst_object_unref(queue0_sink_pad);\n> > + if (queue1_sink_pad)\n> > + gst_object_unref(queue1_sink_pad);\n> > + g_printerr(\"Pads could not be linked.\\n\");\n> > + return TestFail;\n> > + }\n> > + gst_object_unref(queue0_sink_pad);\n> > + gst_object_unref(queue1_sink_pad);\n> > +\n> > + if (startPipeline() != TestPass)\n> > + return TestFail;\n> > +\n> > + if (processEvent() != TestPass)\n> > + return TestFail;\n> > +\n> > + return TestPass;\n> > + }\n> > +\n> > +private:\n> > + std::string cameraName;\n> > + GstElement *convert0_;\n> > + GstElement *convert1_;\n> > + GstElement *sink0_;\n> > + GstElement *sink1_;\n> > + GstElement *queue0_;\n> > + GstElement *queue1_;\n> > +};\n> > +\n> > +TEST_REGISTER(GstreamerMultiStreamTest)\n> > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > index aca53b920365..13652e87d05c 100644\n> > --- a/test/gstreamer/meson.build\n> > +++ b/test/gstreamer/meson.build\n> > @@ -6,6 +6,7 @@ endif\n> >\n> > gstreamer_tests = [\n> > ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> > + ['multi_stream_test', 'gstreamer_multi_stream_test.cpp'],\n> > ]\n> > gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> >\n>\n\nRegards,\nVedant 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 CD15BBDB1D\n\tfor ;\n\tTue, 14 Sep 2021 08:42:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 251E969187;\n\tTue, 14 Sep 2021 10:42:46 +0200 (CEST)","from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com\n\t[IPv6:2607:f8b0:4864:20::b36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B21860249\n\tfor ;\n\tTue, 14 Sep 2021 10:42:44 +0200 (CEST)","by mail-yb1-xb36.google.com with SMTP id k65so26508489yba.13\n\tfor ;\n\tTue, 14 Sep 2021 01:42:44 -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=\"cxuIe+Vf\"; 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=Fs5fPMO2847Sma8p39ZgYOF2CdVAdUxWDm7fspLnQR8=;\n\tb=cxuIe+VfV98fflkDMjqBvAStr2s+mX8diMkznzgYPXnReha3zdjwzpRGAIWaovEacB\n\tcrC3vZGQYMBlhDtsmO1QvKA0i+3ok5JK+edJB0fPo0cHGFCY+DLQyTOlad220IN/rdAC\n\tQubLw6EykXGccTgQkPLBV2NBzhm3YsHyUlMRwrGvTFEnW7i7oJiig8Bdmh0rZ02iFl9Z\n\tL+Erhk4TL/PFOxq/ApmuItoYlL5oivZnwz/ENfgvm0CD7G6YcFEPPvXuiQH0kcO6FFtb\n\tw3RckLugFeVBOJBzb6U4FdRziWHjbkpEzb4wuSiz4jW6WBZQB5urKvu2JxM+AG8lzf+Q\n\th2TQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Fs5fPMO2847Sma8p39ZgYOF2CdVAdUxWDm7fspLnQR8=;\n\tb=Zt5UC1eS47WxJXjV28eH1BK//JUhfdW9De2N1vGWaVvT4vgSPn+emtkUGAXrUnoMEh\n\tzJ/NqJIv0kD1JrVkIf2ZeKIfxyVHZVVaq9K0CdtAqaURKUKuxOAggk7buhGk5IV8mpUy\n\tIWQSgxVLNMtQ1c/8Uo0iEu8YM/a8fLmgVW+U6zEm7ouYEgqe9GTIPnbpUZX1bn1/2Efl\n\tUXxoI8DvD78tNTu9/Yvk36m8gUqfPJgjBeBiU4lyBglDLsrRZZUBdOar/DmRxy6XrNyP\n\tqN6elPDnPEdgXxw0eH/srLILpffAY5kkNXGao7V3Q4ACeUUKkCPVUrR+hDKgEwn+TqF8\n\tu+BA==","X-Gm-Message-State":"AOAM533iwD+KwUsfR8nM9oTbTpFDAe/RDYOfRD2H3WSR8VJ++ajBogC/\n\tROe9MPYG9X2++LoY3DzJlXRUnK03Ciw08m0svZY=","X-Google-Smtp-Source":"ABdhPJx2U28nyPTchYKMaiN84pELqKVNSqOKYNamNFYcLrG5vSs934uBlmTKNi09imP3Qmqzvi9dQj6B87Q7K+QJ1Xw=","X-Received":"by 2002:a25:c648:: with SMTP id\n\tk69mr21497259ybf.242.1631608962345; \n\tTue, 14 Sep 2021 01:42:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20210914063404.53621-1-vedantparanjape160201@gmail.com>\n\t<89c39c9a-9b01-8e81-3436-f5124e654771@ideasonboard.com>","In-Reply-To":"<89c39c9a-9b01-8e81-3436-f5124e654771@ideasonboard.com>","From":"Vedant Paranjape ","Date":"Tue, 14 Sep 2021 14:12:30 +0530","Message-ID":"","To":"Umang Jain ","Content-Type":"multipart/alternative; boundary=\"0000000000009cf2f705cbf09055\"","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Add a test for\n\tgstreamer multi stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"LibCamera Devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19671,"web_url":"https://patchwork.libcamera.org/comment/19671/","msgid":"<6f92ba13-adc1-1ce8-9cba-05296f6c8d8b@ideasonboard.com>","date":"2021-09-14T09:38:15","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Add a test for\n\tgstreamer multi stream","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Vedant\n\nOn 9/14/21 2:12 PM, Vedant Paranjape wrote:\n> Hi Umang,\n>\n>\n> On Tue, Sep 14, 2021 at 12:53 PM Umang Jain \n> wrote:\n>\n>> Hi Vedant,\n>>\n>> On 9/14/21 12:04 PM, Vedant Paranjape wrote:\n>>> This patch adds a test to test if multi stream using libcamera's\n>>> gstreamer element works.\n>>\n>> Can you please document how and when this test is run?\n>>\n>> Currently, my system (with one UVC and vimc) has been able to run\n>> single_stream_test but multi_stream_test is run:\n>>\n>>\n>> 11/65 libcamera:gstreamer / single_stream_test OK 2.53s\n>> 12/65 libcamera:gstreamer / multi_stream_test SKIP 0.07s\n>>\n>> What's your test setup where this tests passes. I assume I have to\n>> satisfy the following condition from the test, in order to run:\n>>\n>> if (camera->streams().size() > 1) {\n>>\n>> cameraFound = true;\n>>\n>> ....\n>>\n>> }\n>>\n>> But I don't know how, looking at the patch. So can you explain (not just\n>> here but in the patch / commit message as well) ? It shall help others\n>> as well.\n>>\n> Multistream will run on rpi and IPU pipelines only. Will add it to commit\n> message.\n\n\nOK, I'll see if I can carve some timeout to test on IPU3 maybe? You are \ntesting RPi I assume?\n\n>\n>>> Signed-off-by: Vedant Paranjape \n>>> Reviewed-by: Paul Elder \n>>> Tested-by: Paul Elder \n>>> ---\n>>> .../gstreamer/gstreamer_multi_stream_test.cpp | 138 ++++++++++++++++++\n>>> test/gstreamer/meson.build | 1 +\n>>> 2 files changed, 139 insertions(+)\n>>> create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp\n>>\n>> Right off the bat, there is a checkstyle failure: Can you look into it,\n>> please?\n>>\n>> [uajain@fedora libcamera]$ ./utils/checkstyle.py HEAD\n>>\n>> -----------------------------------------------------------------------------------------------\n>> 9d0efed89b03d57790db4eebefa909bf169ce789 test: gstreamer: Add a test for\n>> gstreamer multi stream\n>>\n>> -----------------------------------------------------------------------------------------------\n>> --- test/gstreamer/gstreamer_multi_stream_test.cpp\n>> +++ test/gstreamer/gstreamer_multi_stream_test.cpp\n>> @@ -104,8 +104,7 @@\n>> GstPad *queue0_sink_pad = gst_element_get_static_pad(queue0_,\n>> \"sink\");\n>> GstPad *queue1_sink_pad = gst_element_get_static_pad(queue1_,\n>> \"sink\");\n>>\n>> - if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK\n>> - || gst_pad_link(request_pad, queue1_sink_pad) !=\n>> GST_PAD_LINK_OK) {\n>> + if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK\n>> || gst_pad_link(request_pad, queue1_sink_pad) != GST_PAD_LINK_OK) {\n>> if (queue0_sink_pad)\n>> gst_object_unref(queue0_sink_pad);\n>> if (queue1_sink_pad)\n>> ---\n>> 1 potential issue detected, please review\n>>\n> Please see Paul's review !\n\n\nWhere? Now that you mention it, I see tags  from Paul but the v1 had no \ncomments and v2 has couple of Laurent's comments. Are you sure you only \nhave one of v1 and one of v2 series versioning?\n\n>\n>>> diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp\n>> b/test/gstreamer/gstreamer_multi_stream_test.cpp\n>>> new file mode 100644\n>>> index 000000000000..aba86a3f8e27\n>>> --- /dev/null\n>>> +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp\n>>> @@ -0,0 +1,138 @@\n>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>> +/*\n>>> + * Copyright (C) 2021, Vedant Paranjape\n>>> + *\n>>> + * gstreamer_multi_stream_test.cpp - GStreamer multi stream capture test\n>>> + */\n>>> +\n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +\n>>> +#include \n>>> +\n>>> +#include \"libcamera/internal/source_paths.h\"\n>>> +\n>>> +#include \n>>> +\n>>> +#include \"gstreamer_test.h\"\n>>> +#include \"test.h\"\n>>> +\n>>> +using namespace std;\n>>> +\n>>> +class GstreamerMultiStreamTest : public GstreamerTest, public Test\n>>> +{\n>>> +public:\n>>> + GstreamerMultiStreamTest()\n>>> + : GstreamerTest()\n>>> + {\n>>> + }\n>>> +\n>>> +protected:\n>>> + int init() override\n>>> + {\n>>> + if (status_ != TestPass)\n>>> + return status_;\n>>> +\n>>> + /* Check if platform support multistream output */\n>>> + libcamera::CameraManager cm;\n>>> + cm.start();\n>>> + bool cameraFound = false;\n>>> + for (auto &camera : cm.cameras()) {\n>>> + if (camera->streams().size() > 1) {\n>>> + cameraName = camera->id();\n>>> + cameraFound = true;\n>>> + cm.stop();\n>>> + break;\n>>> + }\n>>> + }\n>>> +\n>>> + if (!cameraFound) {\n>>> + cm.stop();\n>>> + return TestSkip;\n>>> + }\n>>> +\n>>> + g_autoptr(GstElement) convert0 =\n>> gst_element_factory_make(\"videoconvert\", \"convert0\");\n>>> + g_autoptr(GstElement) convert1 =\n>> gst_element_factory_make(\"videoconvert\", \"convert1\");\n>>> + g_autoptr(GstElement) sink0 =\n>> gst_element_factory_make(\"fakesink\", \"sink0\");\n>>> + g_autoptr(GstElement) sink1 =\n>> gst_element_factory_make(\"fakesink\", \"sink1\");\n>>> + g_autoptr(GstElement) queue0 =\n>> gst_element_factory_make(\"queue\", \"queue0\");\n>>> + g_autoptr(GstElement) queue1 =\n>> gst_element_factory_make(\"queue\", \"queue1\");\n>>> + g_object_ref_sink(convert0);\n>>> + g_object_ref_sink(convert1);\n>>> + g_object_ref_sink(sink0);\n>>> + g_object_ref_sink(sink1);\n>>> + g_object_ref_sink(queue0);\n>>> + g_object_ref_sink(queue1);\n>>\n>> What's the idea here of first taking ownership by locally scope\n>> g_autoptr() pointers and then setting to the private members below? Is\n>> it to validate/null-check them? I would directly set to private members\n>> and null-check them directly instead, and if it fails, so be it and\n>> return TestFail. Would you be tempted to take this route?\n>>\n> I was doing it before, and it let to a lot of repetitive code and buggy\n> error handling. Also, Laurent and Nicolas were kinda reserved against it.\n\n\nIf it was repetitive, I guess you can encapsulate all that in a helper \nfunction, to be called on error path to cleanup and return TestFail.\n\n>\n>> +\n>>> + if (!convert0 || !convert1 || !sink0 || !sink1 || !queue0\n>> || !queue1) {\n>>> + g_printerr(\"Not all elements could be created.\n>> %p.%p.%p.%p.%p.%p\\n\",\n>>> + convert0, convert1, sink0, sink1,\n>> queue0, queue1);\n>>> +\n>>> + return TestFail;\n>>> + }\n>>> +\n>>> + convert0_ = reinterpret_cast> *>(g_steal_pointer(&convert0));\n>>> + convert1_ = reinterpret_cast> *>(g_steal_pointer(&convert1));\n>>> + sink0_ = reinterpret_cast> *>(g_steal_pointer(&sink0));\n>>> + sink1_ = reinterpret_cast> *>(g_steal_pointer(&sink1));\n>>> + queue0_ = reinterpret_cast> *>(g_steal_pointer(&queue0));\n>>> + queue1_ = reinterpret_cast> *>(g_steal_pointer(&queue1));\n>>> +\n>>> + if (createPipeline() != TestPass)\n>>> + return TestFail;\n>>\n>> Should this `if` block be present before you set the elements above?\n>>\n> This was a good catch, all those elements won't be unrefed if\n> createPipeline fails. I will make the fix !\n>\n>> +\n>>> + return TestPass;\n>>> + }\n>>> +\n>>> + int run() override\n>>> + {\n>>> + g_object_set(libcameraSrc_, \"camera-name\",\n>> cameraName.c_str(), NULL);\n>>> +\n>>> + /* Build the pipeline */\n>>> + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n>> queue0_, queue1_,\n>>> + convert0_, convert1_, sink0_, sink1_,\n>> NULL);\n>>> + if (gst_element_link_many(queue0_, convert0_, sink0_,\n>> NULL) != TRUE ||\n>>> + gst_element_link_many(queue1_, convert1_, sink1_,\n>> NULL) != TRUE) {\n>>> + g_printerr(\"Elements could not be linked.\\n\");\n>>> + return TestFail;\n>>> + }\n>>> +\n>>> + g_autoptr(GstPad) src_pad =\n>> gst_element_get_static_pad(libcameraSrc_, \"src\");\n>>> + g_autoptr(GstPad) request_pad =\n>> gst_element_get_request_pad(libcameraSrc_, \"src_%u\");\n>>> + GstPad *queue0_sink_pad =\n>> gst_element_get_static_pad(queue0_, \"sink\");\n>>> + GstPad *queue1_sink_pad =\n>> gst_element_get_static_pad(queue1_, \"sink\");\n>>\n>>\n>> Why can't you use g_autoptr() for queue0_sink_pad and queue1_sink_pad\n>> too? gst_element_get_static_pad() will return a [Full] transfer-able\n>> value, so it should be un-reffed after usage (like you do below). If you\n>> use g_autoptr() the manual _unref shall be taken care of by itself no?\n>> Am I missing something? It'll help cleanup error paths.\n>>\n> The pad must be unrefed immediately after use, that is after the pads are\n\n\nI referred some documentation and I didn't see the word \"immediately\" \nanywhere.\n\nThe pads needs to be un-reffed after use, but I don't think \"immediate \nfree\" is a requirement. It's bad design upfront and I think gstreamer \nfolks would have known it. Can you provide link to documentation which \nmade you think it should be \"immediately\" un-reffed and not when \nfunction returns (which happens in case g_autoptr is used)?\n\n> linked, and if I use an autoptr it won't unref right after the if.\n> I had thought to put the code where pads are created and linked into an\n> inner scope like this, but then it will be over kill.\n>\n> func\n> {\n> { // code // }\n> }\n\n\nI have seen this design as well for GLib projects as well, mainly for \ng_autoptr and mutexes combine and don't think it's an overkill.\n\n>\n> I assume this is a nitpick and not a blocker. I don't see much improvement\n> by doing so, It'd be a different scenario if I had, say 10 pads.\n\n\nHere as well, I would suggest to get a helper function if you are \nmanaging so many pads at once.\n\nI'll check if I am able to run the test on platforms available to me and \nincorporate the suggestions. This is just a reading and understanding of \nthe code.\n\n>\n>> +\n>>> + if (gst_pad_link(src_pad, queue0_sink_pad) !=\n>> GST_PAD_LINK_OK\n>>> + || gst_pad_link(request_pad, queue1_sink_pad) !=\n>> GST_PAD_LINK_OK) {\n>>> + if (queue0_sink_pad)\n>>> + gst_object_unref(queue0_sink_pad);\n>>> + if (queue1_sink_pad)\n>>> + gst_object_unref(queue1_sink_pad);\n>>> + g_printerr(\"Pads could not be linked.\\n\");\n>>> + return TestFail;\n>>> + }\n>>> + gst_object_unref(queue0_sink_pad);\n>>> + gst_object_unref(queue1_sink_pad);\n>>> +\n>>> + if (startPipeline() != TestPass)\n>>> + return TestFail;\n>>> +\n>>> + if (processEvent() != TestPass)\n>>> + return TestFail;\n>>> +\n>>> + return TestPass;\n>>> + }\n>>> +\n>>> +private:\n>>> + std::string cameraName;\n>>> + GstElement *convert0_;\n>>> + GstElement *convert1_;\n>>> + GstElement *sink0_;\n>>> + GstElement *sink1_;\n>>> + GstElement *queue0_;\n>>> + GstElement *queue1_;\n>>> +};\n>>> +\n>>> +TEST_REGISTER(GstreamerMultiStreamTest)\n>>> diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n>>> index aca53b920365..13652e87d05c 100644\n>>> --- a/test/gstreamer/meson.build\n>>> +++ b/test/gstreamer/meson.build\n>>> @@ -6,6 +6,7 @@ endif\n>>>\n>>> gstreamer_tests = [\n>>> ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n>>> + ['multi_stream_test', 'gstreamer_multi_stream_test.cpp'],\n>>> ]\n>>> gstreamer_dep = dependency('gstreamer-1.0', required: true)\n>>>\n> Regards,\n> Vedant Paranjape\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 1FA6EBDB1D\n\tfor ;\n\tTue, 14 Sep 2021 09:38:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47D2769187;\n\tTue, 14 Sep 2021 11:38:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5ACF260131\n\tfor ;\n\tTue, 14 Sep 2021 11:38:21 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 672DF2A5;\n\tTue, 14 Sep 2021 11:38:20 +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=\"tft6DFxB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631612301;\n\tbh=oTDe9dRZLk0S2Ta/MRFkz3FlnkpkbK4gCACwPhd4Pm4=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=tft6DFxB+r3dTZPxXvxvS9wNb+zDXQ9OuNZThWqwWyn5HG3gA4E0wduaIglEd3t7X\n\tuUkIlve069eI5lao7njIf+zLyzZ/1/cOq61oOZimO7Vo75em+fLMTnp35w0FmGQQ7z\n\tHWze+XtbDNTI68a/bh6RgtteE5MbC7dyx31VMrrI=","To":"Vedant Paranjape ","References":"<20210914063404.53621-1-vedantparanjape160201@gmail.com>\n\t<89c39c9a-9b01-8e81-3436-f5124e654771@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<6f92ba13-adc1-1ce8-9cba-05296f6c8d8b@ideasonboard.com>","Date":"Tue, 14 Sep 2021 15:08:15 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Add a test for\n\tgstreamer multi stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"LibCamera Devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19675,"web_url":"https://patchwork.libcamera.org/comment/19675/","msgid":"","date":"2021-09-14T10:28:45","subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Add a test for\n\tgstreamer multi stream","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Umang,\n\n\nOn Tue, Sep 14, 2021 at 3:08 PM Umang Jain \nwrote:\n\n> Hi Vedant\n>\n> On 9/14/21 2:12 PM, Vedant Paranjape wrote:\n> > Hi Umang,\n> >\n> >\n> > On Tue, Sep 14, 2021 at 12:53 PM Umang Jain >\n> > wrote:\n> >\n> >> Hi Vedant,\n> >>\n> >> On 9/14/21 12:04 PM, Vedant Paranjape wrote:\n> >>> This patch adds a test to test if multi stream using libcamera's\n> >>> gstreamer element works.\n> >>\n> >> Can you please document how and when this test is run?\n> >>\n> >> Currently, my system (with one UVC and vimc) has been able to run\n> >> single_stream_test but multi_stream_test is run:\n> >>\n> >>\n> >> 11/65 libcamera:gstreamer / single_stream_test OK 2.53s\n> >> 12/65 libcamera:gstreamer / multi_stream_test SKIP 0.07s\n> >>\n> >> What's your test setup where this tests passes. I assume I have to\n> >> satisfy the following condition from the test, in order to run:\n> >>\n> >> if (camera->streams().size() > 1) {\n> >>\n> >> cameraFound = true;\n> >>\n> >> ....\n> >>\n> >> }\n> >>\n> >> But I don't know how, looking at the patch. So can you explain (not just\n> >> here but in the patch / commit message as well) ? It shall help others\n> >> as well.\n> >>\n> > Multistream will run on rpi and IPU pipelines only. Will add it to commit\n> > message.\n>\n>\n> OK, I'll see if I can carve some timeout to test on IPU3 maybe? You are\n> testing RPi I assume?\n>\n\nYes\n\n>\n> >>> Signed-off-by: Vedant Paranjape \n> >>> Reviewed-by: Paul Elder \n> >>> Tested-by: Paul Elder \n> >>> ---\n> >>> .../gstreamer/gstreamer_multi_stream_test.cpp | 138\n> ++++++++++++++++++\n> >>> test/gstreamer/meson.build | 1 +\n> >>> 2 files changed, 139 insertions(+)\n> >>> create mode 100644 test/gstreamer/gstreamer_multi_stream_test.cpp\n> >>\n> >> Right off the bat, there is a checkstyle failure: Can you look into it,\n> >> please?\n> >>\n> >> [uajain@fedora libcamera]$ ./utils/checkstyle.py HEAD\n> >>\n> >>\n> -----------------------------------------------------------------------------------------------\n> >> 9d0efed89b03d57790db4eebefa909bf169ce789 test: gstreamer: Add a test for\n> >> gstreamer multi stream\n> >>\n> >>\n> -----------------------------------------------------------------------------------------------\n> >> --- test/gstreamer/gstreamer_multi_stream_test.cpp\n> >> +++ test/gstreamer/gstreamer_multi_stream_test.cpp\n> >> @@ -104,8 +104,7 @@\n> >> GstPad *queue0_sink_pad = gst_element_get_static_pad(queue0_,\n> >> \"sink\");\n> >> GstPad *queue1_sink_pad = gst_element_get_static_pad(queue1_,\n> >> \"sink\");\n> >>\n> >> - if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK\n> >> - || gst_pad_link(request_pad, queue1_sink_pad) !=\n> >> GST_PAD_LINK_OK) {\n> >> + if (gst_pad_link(src_pad, queue0_sink_pad) != GST_PAD_LINK_OK\n> >> || gst_pad_link(request_pad, queue1_sink_pad) != GST_PAD_LINK_OK) {\n> >> if (queue0_sink_pad)\n> >> gst_object_unref(queue0_sink_pad);\n> >> if (queue1_sink_pad)\n> >> ---\n> >> 1 potential issue detected, please review\n> >>\n> > Please see Paul's review !\n>\n>\n> Where? Now that you mention it, I see tags from Paul but the v1 had no\n> comments and v2 has couple of Laurent's comments. Are you sure you only\n> have one of v1 and one of v2 series versioning?\n>\n\nNo, Paul was the one who commented on v2. One with Laurent's comments is\npretty old and outdated, as I now have a base class for gstreamer test,\nearlier one didn't\nPaul's review: https://patchwork.libcamera.org/patch/13813/\n\n>\n> >>> diff --git a/test/gstreamer/gstreamer_multi_stream_test.cpp\n> >> b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> >>> new file mode 100644\n> >>> index 000000000000..aba86a3f8e27\n> >>> --- /dev/null\n> >>> +++ b/test/gstreamer/gstreamer_multi_stream_test.cpp\n> >>> @@ -0,0 +1,138 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2021, Vedant Paranjape\n> >>> + *\n> >>> + * gstreamer_multi_stream_test.cpp - GStreamer multi stream capture\n> test\n> >>> + */\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +#include \n> >>> +\n> >>> +#include \n> >>> +\n> >>> +#include \"libcamera/internal/source_paths.h\"\n> >>> +\n> >>> +#include \n> >>> +\n> >>> +#include \"gstreamer_test.h\"\n> >>> +#include \"test.h\"\n> >>> +\n> >>> +using namespace std;\n> >>> +\n> >>> +class GstreamerMultiStreamTest : public GstreamerTest, public Test\n> >>> +{\n> >>> +public:\n> >>> + GstreamerMultiStreamTest()\n> >>> + : GstreamerTest()\n> >>> + {\n> >>> + }\n> >>> +\n> >>> +protected:\n> >>> + int init() override\n> >>> + {\n> >>> + if (status_ != TestPass)\n> >>> + return status_;\n> >>> +\n> >>> + /* Check if platform support multistream output */\n> >>> + libcamera::CameraManager cm;\n> >>> + cm.start();\n> >>> + bool cameraFound = false;\n> >>> + for (auto &camera : cm.cameras()) {\n> >>> + if (camera->streams().size() > 1) {\n> >>> + cameraName = camera->id();\n> >>> + cameraFound = true;\n> >>> + cm.stop();\n> >>> + break;\n> >>> + }\n> >>> + }\n> >>> +\n> >>> + if (!cameraFound) {\n> >>> + cm.stop();\n> >>> + return TestSkip;\n> >>> + }\n> >>> +\n> >>> + g_autoptr(GstElement) convert0 =\n> >> gst_element_factory_make(\"videoconvert\", \"convert0\");\n> >>> + g_autoptr(GstElement) convert1 =\n> >> gst_element_factory_make(\"videoconvert\", \"convert1\");\n> >>> + g_autoptr(GstElement) sink0 =\n> >> gst_element_factory_make(\"fakesink\", \"sink0\");\n> >>> + g_autoptr(GstElement) sink1 =\n> >> gst_element_factory_make(\"fakesink\", \"sink1\");\n> >>> + g_autoptr(GstElement) queue0 =\n> >> gst_element_factory_make(\"queue\", \"queue0\");\n> >>> + g_autoptr(GstElement) queue1 =\n> >> gst_element_factory_make(\"queue\", \"queue1\");\n> >>> + g_object_ref_sink(convert0);\n> >>> + g_object_ref_sink(convert1);\n> >>> + g_object_ref_sink(sink0);\n> >>> + g_object_ref_sink(sink1);\n> >>> + g_object_ref_sink(queue0);\n> >>> + g_object_ref_sink(queue1);\n> >>\n> >> What's the idea here of first taking ownership by locally scope\n> >> g_autoptr() pointers and then setting to the private members below? Is\n> >> it to validate/null-check them? I would directly set to private members\n> >> and null-check them directly instead, and if it fails, so be it and\n> >> return TestFail. Would you be tempted to take this route?\n> >>\n> > I was doing it before, and it let to a lot of repetitive code and buggy\n> > error handling. Also, Laurent and Nicolas were kinda reserved against it.\n>\n>\n> If it was repetitive, I guess you can encapsulate all that in a helper\n> function, to be called on error path to cleanup and return TestFail.\n>\n\nI prefer using g_autoptr based approach, I'll ping @Nicolas Dufresne\n for his views, ~he suggested me to do it this way !\n\n\n> >\n> >> +\n> >>> + if (!convert0 || !convert1 || !sink0 || !sink1 || !queue0\n> >> || !queue1) {\n> >>> + g_printerr(\"Not all elements could be created.\n> >> %p.%p.%p.%p.%p.%p\\n\",\n> >>> + convert0, convert1, sink0, sink1,\n> >> queue0, queue1);\n> >>> +\n> >>> + return TestFail;\n> >>> + }\n> >>> +\n> >>> + convert0_ = reinterpret_cast >> *>(g_steal_pointer(&convert0));\n> >>> + convert1_ = reinterpret_cast >> *>(g_steal_pointer(&convert1));\n> >>> + sink0_ = reinterpret_cast >> *>(g_steal_pointer(&sink0));\n> >>> + sink1_ = reinterpret_cast >> *>(g_steal_pointer(&sink1));\n> >>> + queue0_ = reinterpret_cast >> *>(g_steal_pointer(&queue0));\n> >>> + queue1_ = reinterpret_cast >> *>(g_steal_pointer(&queue1));\n> >>> +\n> >>> + if (createPipeline() != TestPass)\n> >>> + return TestFail;\n> >>\n> >> Should this `if` block be present before you set the elements above?\n> >>\n> > This was a good catch, all those elements won't be unrefed if\n> > createPipeline fails. I will make the fix !\n> >\n> >> +\n> >>> + return TestPass;\n> >>> + }\n> >>> +\n> >>> + int run() override\n> >>> + {\n> >>> + g_object_set(libcameraSrc_, \"camera-name\",\n> >> cameraName.c_str(), NULL);\n> >>> +\n> >>> + /* Build the pipeline */\n> >>> + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n> >> queue0_, queue1_,\n> >>> + convert0_, convert1_, sink0_, sink1_,\n> >> NULL);\n> >>> + if (gst_element_link_many(queue0_, convert0_, sink0_,\n> >> NULL) != TRUE ||\n> >>> + gst_element_link_many(queue1_, convert1_, sink1_,\n> >> NULL) != TRUE) {\n> >>> + g_printerr(\"Elements could not be linked.\\n\");\n> >>> + return TestFail;\n> >>> + }\n> >>> +\n> >>> + g_autoptr(GstPad) src_pad =\n> >> gst_element_get_static_pad(libcameraSrc_, \"src\");\n> >>> + g_autoptr(GstPad) request_pad =\n> >> gst_element_get_request_pad(libcameraSrc_, \"src_%u\");\n> >>> + GstPad *queue0_sink_pad =\n> >> gst_element_get_static_pad(queue0_, \"sink\");\n> >>> + GstPad *queue1_sink_pad =\n> >> gst_element_get_static_pad(queue1_, \"sink\");\n> >>\n> >>\n> >> Why can't you use g_autoptr() for queue0_sink_pad and queue1_sink_pad\n> >> too? gst_element_get_static_pad() will return a [Full] transfer-able\n> >> value, so it should be un-reffed after usage (like you do below). If you\n> >> use g_autoptr() the manual _unref shall be taken care of by itself no?\n> >> Am I missing something? It'll help cleanup error paths.\n> >>\n> > The pad must be unrefed immediately after use, that is after the pads are\n>\n>\n> I referred some documentation and I didn't see the word \"immediately\"\n> anywhere.\n>\n> The pads needs to be un-reffed after use, but I don't think \"immediate\n> free\" is a requirement. It's bad design upfront and I think gstreamer\n> folks would have known it. Can you provide link to documentation which\n> made you think it should be \"immediately\" un-reffed and not when\n> function returns (which happens in case g_autoptr is used)?\n>\n\nI'm not a gstreamer expert, so I just followed the gstreamer example code,\nhttps://gstreamer.freedesktop.org/documentation/tutorials/basic/multithreading-and-pad-availability.html?gi-language=c\nIt unrefs immediately. @Nicolas Dufresne can you\nconfirm this ?\n\n> linked, and if I use an autoptr it won't unref right after the if.\n> > I had thought to put the code where pads are created and linked into an\n> > inner scope like this, but then it will be over kill.\n> >\n> > func\n> > {\n> > { // code // }\n> > }\n>\n>\n> I have seen this design as well for GLib projects as well, mainly for\n> g_autoptr and mutexes combine and don't think it's an overkill.\n>\n>\n> > I assume this is a nitpick and not a blocker. I don't see much\n> improvement\n> > by doing so, It'd be a different scenario if I had, say 10 pads.\n>\n>\n> Here as well, I would suggest to get a helper function if you are\n> managing so many pads at once.\n>\n\nI just stated if there would be 10 pads, I'd think about that, but this\ntest will only ever have two pads.\n\nI'll check if I am able to run the test on platforms available to me and\n> incorporate the suggestions. This is just a reading and understanding of\n> the code.\n>\n\nThanks. Please, let me know if it works !\n\n>\n> >> +\n> >>> + if (gst_pad_link(src_pad, queue0_sink_pad) !=\n> >> GST_PAD_LINK_OK\n> >>> + || gst_pad_link(request_pad, queue1_sink_pad) !=\n> >> GST_PAD_LINK_OK) {\n> >>> + if (queue0_sink_pad)\n> >>> + gst_object_unref(queue0_sink_pad);\n> >>> + if (queue1_sink_pad)\n> >>> + gst_object_unref(queue1_sink_pad);\n> >>> + g_printerr(\"Pads could not be linked.\\n\");\n> >>> + return TestFail;\n> >>> + }\n> >>> + gst_object_unref(queue0_sink_pad);\n> >>> + gst_object_unref(queue1_sink_pad);\n> >>> +\n> >>> + if (startPipeline() != TestPass)\n> >>> + return TestFail;\n> >>> +\n> >>> + if (processEvent() != TestPass)\n> >>> + return TestFail;\n> >>> +\n> >>> + return TestPass;\n> >>> + }\n> >>> +\n> >>> +private:\n> >>> + std::string cameraName;\n> >>> + GstElement *convert0_;\n> >>> + GstElement *convert1_;\n> >>> + GstElement *sink0_;\n> >>> + GstElement *sink1_;\n> >>> + GstElement *queue0_;\n> >>> + GstElement *queue1_;\n> >>> +};\n> >>> +\n> >>> +TEST_REGISTER(GstreamerMultiStreamTest)\n> >>> diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> >>> index aca53b920365..13652e87d05c 100644\n> >>> --- a/test/gstreamer/meson.build\n> >>> +++ b/test/gstreamer/meson.build\n> >>> @@ -6,6 +6,7 @@ endif\n> >>>\n> >>> gstreamer_tests = [\n> >>> ['single_stream_test', 'gstreamer_single_stream_test.cpp'],\n> >>> + ['multi_stream_test', 'gstreamer_multi_stream_test.cpp'],\n> >>> ]\n> >>> gstreamer_dep = dependency('gstreamer-1.0', required: true)\n> >>>\n> > Regards,\n> > Vedant Paranjape\n> >\n>\n\nRegards,\nVedant 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 D5536BDC71\n\tfor ;\n\tTue, 14 Sep 2021 10:29:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5562160249;\n\tTue, 14 Sep 2021 12:29:01 +0200 (CEST)","from mail-yb1-xb30.google.com (mail-yb1-xb30.google.com\n\t[IPv6:2607:f8b0:4864:20::b30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D756160132\n\tfor ;\n\tTue, 14 Sep 2021 12:28:59 +0200 (CEST)","by mail-yb1-xb30.google.com with SMTP id s16so27181642ybe.0\n\tfor ;\n\tTue, 14 Sep 2021 03:28:59 -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=\"PysjF1d+\"; 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=2nynFsxpZq51Xzv7+uoa/25CTPxxqK40VDr9dpFL23M=;\n\tb=PysjF1d+sLp95jqz/MCGqolQcUpm9ZjGTgnMDTSvNT3/jHOn2sVu78UnzNGxU2n+sF\n\tP7UUU+Z0yNvLZ6BLH7VUA5IIvdaUHgob3JPXuh1sLeLgVHqGgpsUzV86ceKPlji9ZpNm\n\tUx8jmL7a90nZgbfdWsm09/SLf9L0NO4pJM2Kks0YEjvSErxsTQGF64ksVVACqjXuLveZ\n\twJOX/e+y+QGmnCoLwIQGN6VRZit5znxv8rOm0pcSUbUqizUCeg/BguOYa2pSD2zm3PNz\n\tBv+e+50mO7bYN3PNqhcfki/gCJ4ixgqS6yN85eIibKKhIs82Z/dfH8rPuIUPPAx+aGcr\n\t6mQg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=2nynFsxpZq51Xzv7+uoa/25CTPxxqK40VDr9dpFL23M=;\n\tb=qTUvgP5bWJPMFzIK8n180r/hmNfiH8Gl4mTB0V7JCwlXzRapSPHD1dItQiWJoZ/iky\n\tnhiODINWq8MlhB2mRNIyPYatArCDrq/GbIL2JoOAjBQQAAGOCF6rEGypjfs70udKgKZW\n\tvz+C3UiAydvuZNJdYwljOx7gIeMvDGKDJFoXphtRp0UEPltzKxfiC9kb80NlT77+4ds9\n\tGX/dem2JrdoEptIsksq199BKtY8hfTpHxBtDOfzEush570ghrr6RRi46sYRZXzDDelEJ\n\tASL2DCSIhtoeuQlzSMqB1cSJIOxF+FvyoLmHiVoegFV7dvloAhPdcbs/AjSLPD6bv6oc\n\t2AAg==","X-Gm-Message-State":"AOAM53101zh++RU6GlfKA5GvLaxW+WnPqqzVvnDtDDXzDjEqICLuVdm5\n\tn5rprcKhlfYZQWaP0oX+976y5/BXfCrciV87m+wHAcffxfStGSbl","X-Google-Smtp-Source":"ABdhPJwEtAXO0NdfI6McsfXnnYhnQcKgayD/WHb0NHspHFkvJcxez4glm5OKSR0jJd8SLr1ePYKKSwCHuUGzedP4KDQ=","X-Received":"by 2002:a25:c648:: with SMTP id\n\tk69mr22004860ybf.242.1631615338592; \n\tTue, 14 Sep 2021 03:28:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20210914063404.53621-1-vedantparanjape160201@gmail.com>\n\t<89c39c9a-9b01-8e81-3436-f5124e654771@ideasonboard.com>\n\t\n\t<6f92ba13-adc1-1ce8-9cba-05296f6c8d8b@ideasonboard.com>","In-Reply-To":"<6f92ba13-adc1-1ce8-9cba-05296f6c8d8b@ideasonboard.com>","From":"Vedant Paranjape ","Date":"Tue, 14 Sep 2021 15:58:45 +0530","Message-ID":"","To":"Umang Jain ,\n\tNicolas Dufresne ","Content-Type":"multipart/alternative; boundary=\"000000000000aabdaa05cbf20c34\"","Subject":"Re: [libcamera-devel] [PATCH v3] test: gstreamer: Add a test for\n\tgstreamer multi stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"LibCamera Devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]