[{"id":19715,"web_url":"https://patchwork.libcamera.org/comment/19715/","msgid":"<290afc41-106f-e532-b0bf-a7f6d67d1751@ideasonboard.com>","date":"2021-09-18T18:24:29","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Vedant,\n\nThank you for the patch.\n\nOn 9/15/21 8:39 PM, Vedant Paranjape wrote:\n> Simplify memory handling and complexity of the test by using\n> gst_parse_bin_from_description_full [1]. It also fixed memory leak reported\n> by Umang Jain, described in his fix [2].\n>\n> [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> [2] https://patchwork.libcamera.org/patch/13845/\n\n\nTo be honest, this patch in general introduces too many changes at once. \nThe trick to get good and fast reviews is generally to break it down to \nfocus one task per patch in most cases. This granularity helps the \nreviewer to be confident looking at the changes in front of them and \ngive their R-b tags on those specific patches. It might happen that for \ne.g. I am not well-versed with the API you introduced, but rather I can \nsee the other fixes good to go - but overall I will still be holding off \nmy R-b because there is something I don't understand fully. This is just \nan example but you get the idea.\n\nIf you could have broken this down to let's say 3 patches (piggybacking \non my 2 patches earlier and introduce a 3rd patch by yourself about the \nnew API) - it helps to establish a flow. The two patches had one R-b \ntags earlier so you are already closer to get them merged (need 2 R-b \ntags for merge-eligibility per patch) and the 3rd can be discussed \nmeanwhile. It helps to move and unblock patches / fixes as part of \ndevelopment process.\n\n\n>\n> Signed-off-by: Vedant Paranjape \n> ---\n> .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> 1 file changed, 14 insertions(+), 15 deletions(-)\n>\n> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> index 7292f3280617..a20d79109885 100644\n> --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> @@ -33,20 +33,15 @@ protected:\n> \t\tif (status_ != TestPass)\n> \t\t\treturn status_;\n> \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\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> +\t\tconst gchar* streamDescription = \"queue ! videoconvert ! fakesink\";\n> +\t\tg_autoptr(GError) error0 = NULL;\n> +\t\tstream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> \n> +\t\tif (!stream0_) {\n> +\t\t\tg_printerr(\"Not all bins could be created. %p\\n\", stream0_);\n> \t\t\treturn TestFail;\n> \t\t}\n> -\n> -\t\tconvert0_ = reinterpret_cast(g_steal_pointer(&convert0));\n> -\t\tsink0_ = reinterpret_cast(g_steal_pointer(&sink0));\n> +\t\tg_object_ref_sink(stream0_);\n> \n> \t\tif (createPipeline() != TestPass)\n> \t\t\treturn TestFail;\n> @@ -57,8 +52,8 @@ protected:\n> \tint run() override\n> \t{\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> +\t\tgst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);\n> +\t\tif (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> \t\t\tg_printerr(\"Elements could not be linked.\\n\");\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -72,9 +67,13 @@ protected:\n> \t\treturn TestPass;\n> \t}\n> \n> +\tvoid cleanup() override\n> +\t{\n> +\t\tg_clear_object(&stream0_);\n> +\t}\n> +\n> private:\n> -\tGstElement *convert0_;\n> -\tGstElement *sink0_;\n> +\tGstElement *stream0_;\n> };\n> \n> TEST_REGISTER(GstreamerSingleStreamTest)","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 D8B14BDC71\n\tfor ;\n\tSat, 18 Sep 2021 18:24:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4848969180;\n\tSat, 18 Sep 2021 20:24:37 +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 B5B9B60130\n\tfor ;\n\tSat, 18 Sep 2021 20:24:35 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.208])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 39EA33F0;\n\tSat, 18 Sep 2021 20:24:34 +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=\"qyq6Ygf/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631989475;\n\tbh=tbDfRF78R1nYUFYyrQOglf7adL+pslKh1YsQoXpUaxc=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=qyq6Ygf/m72cNoaTVz2HnQVaCkqsAzBftoOfc81GklGfMkVTjiQ/ZEyI/+cff40Pa\n\t04BYDTVucHU9AOKTXbG+OOhyoNCLlUdBAeALnnubVnpi1SA84ZRtTZnS3haMBBwyrz\n\t9L6urf6fJOTM0m9Bj9TdyasVytxOWO+Ef+zK00Eg=","To":"Vedant Paranjape ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>","From":"Umang Jain ","Message-ID":"<290afc41-106f-e532-b0bf-a7f6d67d1751@ideasonboard.com>","Date":"Sat, 18 Sep 2021 23:54:29 +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":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19716,"web_url":"https://patchwork.libcamera.org/comment/19716/","msgid":"","date":"2021-09-18T18:35:20","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Umang,\nThanks for the reply. I'll keep this in mind, next time I send patches.\n\nBut can you test it and give a tested by tag?\n\nAlso, I can help you understand parts that you are not familiar with.\n\nRegards,\n*Vedant Paranjape*\n\nOn Sat, 18 Sep, 2021, 23:54 Umang Jain, wrote:\n\n> Hi Vedant,\n>\n> Thank you for the patch.\n>\n> On 9/15/21 8:39 PM, Vedant Paranjape wrote:\n> > Simplify memory handling and complexity of the test by using\n> > gst_parse_bin_from_description_full [1]. It also fixed memory leak\n> reported\n> > by Umang Jain, described in his fix [2].\n> >\n> > [1]\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > [2] https://patchwork.libcamera.org/patch/13845/\n>\n>\n> To be honest, this patch in general introduces too many changes at once.\n> The trick to get good and fast reviews is generally to break it down to\n> focus one task per patch in most cases. This granularity helps the\n> reviewer to be confident looking at the changes in front of them and\n> give their R-b tags on those specific patches. It might happen that for\n> e.g. I am not well-versed with the API you introduced, but rather I can\n> see the other fixes good to go - but overall I will still be holding off\n> my R-b because there is something I don't understand fully. This is just\n> an example but you get the idea.\n>\n> If you could have broken this down to let's say 3 patches (piggybacking\n> on my 2 patches earlier and introduce a 3rd patch by yourself about the\n> new API) - it helps to establish a flow. The two patches had one R-b\n> tags earlier so you are already closer to get them merged (need 2 R-b\n> tags for merge-eligibility per patch) and the 3rd can be discussed\n> meanwhile. It helps to move and unblock patches / fixes as part of\n> development process.\n>\n>\n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> > 1 file changed, 14 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > index 7292f3280617..a20d79109885 100644\n> > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > @@ -33,20 +33,15 @@ protected:\n> > if (status_ != TestPass)\n> > return status_;\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> > - if (!convert0 || !sink0) {\n> > - g_printerr(\"Not all elements could be created.\n> %p.%p\\n\",\n> > - convert0, sink0);\n> > + const gchar* streamDescription = \"queue ! videoconvert !\n> fakesink\";\n> > + g_autoptr(GError) error0 = NULL;\n> > + stream0_ =\n> gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> >\n> > + if (!stream0_) {\n> > + g_printerr(\"Not all bins could be created. %p\\n\",\n> stream0_);\n> > return TestFail;\n> > }\n> > -\n> > - convert0_ = reinterpret_cast *>(g_steal_pointer(&convert0));\n> > - sink0_ = reinterpret_cast *>(g_steal_pointer(&sink0));\n> > + g_object_ref_sink(stream0_);\n> >\n> > if (createPipeline() != TestPass)\n> > return TestFail;\n> > @@ -57,8 +52,8 @@ protected:\n> > int run() override\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> > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n> stream0_, NULL);\n> > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> > g_printerr(\"Elements could not be linked.\\n\");\n> > return TestFail;\n> > }\n> > @@ -72,9 +67,13 @@ protected:\n> > return TestPass;\n> > }\n> >\n> > + void cleanup() override\n> > + {\n> > + g_clear_object(&stream0_);\n> > + }\n> > +\n> > private:\n> > - GstElement *convert0_;\n> > - GstElement *sink0_;\n> > + GstElement *stream0_;\n> > };\n> >\n> > TEST_REGISTER(GstreamerSingleStreamTest)\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 28A80BF01C\n\tfor ;\n\tSat, 18 Sep 2021 18:35:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B2CC69185;\n\tSat, 18 Sep 2021 20:35:37 +0200 (CEST)","from mail-qk1-x72e.google.com (mail-qk1-x72e.google.com\n\t[IPv6:2607:f8b0:4864:20::72e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC97460130\n\tfor ;\n\tSat, 18 Sep 2021 20:35:35 +0200 (CEST)","by mail-qk1-x72e.google.com with SMTP id 72so16572465qkk.7\n\tfor ;\n\tSat, 18 Sep 2021 11:35:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"OXLWEv5m\"; 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=mM24RONs0fZDrO6yi5Jnwgh3Bx/+EJSdkJbiqo2EtR4=;\n\tb=OXLWEv5mU40hpAsv4y6K23BCwxqR/jIZqW2Km3mDIxB94fOmuf3JdxOrCSiB1VM5zG\n\tAoqgzMVFumFBLg+8hoCaWyEuSNZnje+F5jvMvczzI3eqH5PM4GeS4MFwtSoSL+75VQ9c\n\tzzcV1qeL6vOW7Je9KBSvf6uoKaWaWPD3F2Aa05CSUCigHQoHYuT3PRFOvDO93ceWFWYk\n\tjmN8V0yVDsAuyrPfV8OEPAU/CzQ806rJ0eeglEQpBA7+DyE6+FvCd6wQ9p2+u6xanxGo\n\tOpKXsW7f8/AsoeiFfmHUZFPJqgg4rjXm2j3RQ/y8/n1533CKqT89OATp4lDPe+BD/3Zq\n\tjj2w==","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=mM24RONs0fZDrO6yi5Jnwgh3Bx/+EJSdkJbiqo2EtR4=;\n\tb=zTtKTo5EafMsPm2HXsEys5Ln53r5q7SvuSQgVKbWsyVbEAlLQV3Hqo9tte2B6pN3lO\n\t4DIQHLHlE0CpZrB5JJ0lAKA/Y6shldIOpzL+eabJZl6pWI55mwIyM/Ctb9qsrWmVYXQL\n\tE0W4G+lzSqxXbzqedZ55DplCsBb0I0AcGVL0WYwOTbFyGBZM9jY9DaQU8p4Jr/c7GdTS\n\tD+GVLEHNlozJDOTl8tupRXfUmTZczxaQ3m05UcJ0BLJRAGxREt0//F2r2b1l4Gthc/bR\n\tTMMUPo5z2RXr+Ce1BBP1mMX0d4wY87uAu2dbD/4uyLFiPcMJtCzkfES8IiYpT+/fvevI\n\tjF3Q==","X-Gm-Message-State":"AOAM531RCKuSPqvTzftA7TaRuMaGjf5Yb23ULjDOglJOg9H9RryDFSgb\n\tUyDWb6lgkaJCz+Iey98f3C+5Uc/4UyqN6H3ULu5WZC/Uj8M=","X-Google-Smtp-Source":"ABdhPJznQfYM1C7sgWndwH+AnZ9vT+bWWvKL38qtq5P/C3Q+FX8eq5avYy1k8YEAIlseh7UeSlx5SkxWybn2lua99hU=","X-Received":"by 2002:a25:c648:: with SMTP id\n\tk69mr22072584ybf.242.1631990134242; \n\tSat, 18 Sep 2021 11:35:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t<290afc41-106f-e532-b0bf-a7f6d67d1751@ideasonboard.com>","In-Reply-To":"<290afc41-106f-e532-b0bf-a7f6d67d1751@ideasonboard.com>","From":"Vedant Paranjape ","Date":"Sun, 19 Sep 2021 00:05:20 +0530","Message-ID":"","To":"Umang Jain ","Content-Type":"multipart/alternative; boundary=\"0000000000003a7f8e05cc4950bd\"","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19717,"web_url":"https://patchwork.libcamera.org/comment/19717/","msgid":"","date":"2021-09-20T15:27:39","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 15 septembre 2021 à 20:39 +0530, Vedant Paranjape a écrit :\n> Simplify memory handling and complexity of the test by using\n> gst_parse_bin_from_description_full [1]. It also fixed memory leak reported\n> by Umang Jain, described in his fix [2].\n> \n> [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> [2] https://patchwork.libcamera.org/patch/13845/\n> \n> Signed-off-by: Vedant Paranjape \n\nReviewed-by: Nicolas Dufresne \n\np.s. I personally found this patch relatively small and fast to review.\n\n> ---\n> .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> 1 file changed, 14 insertions(+), 15 deletions(-)\n> \n> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> index 7292f3280617..a20d79109885 100644\n> --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> @@ -33,20 +33,15 @@ protected:\n> \t\tif (status_ != TestPass)\n> \t\t\treturn status_;\n> \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\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> +\t\tconst gchar* streamDescription = \"queue ! videoconvert ! fakesink\";\n> +\t\tg_autoptr(GError) error0 = NULL;\n> +\t\tstream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> \n> +\t\tif (!stream0_) {\n> +\t\t\tg_printerr(\"Not all bins could be created. %p\\n\", stream0_);\n> \t\t\treturn TestFail;\n> \t\t}\n> -\n> -\t\tconvert0_ = reinterpret_cast(g_steal_pointer(&convert0));\n> -\t\tsink0_ = reinterpret_cast(g_steal_pointer(&sink0));\n> +\t\tg_object_ref_sink(stream0_);\n> \n> \t\tif (createPipeline() != TestPass)\n> \t\t\treturn TestFail;\n> @@ -57,8 +52,8 @@ protected:\n> \tint run() override\n> \t{\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> +\t\tgst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);\n> +\t\tif (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> \t\t\tg_printerr(\"Elements could not be linked.\\n\");\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -72,9 +67,13 @@ protected:\n> \t\treturn TestPass;\n> \t}\n> \n> +\tvoid cleanup() override\n> +\t{\n> +\t\tg_clear_object(&stream0_);\n> +\t}\n> +\n> private:\n> -\tGstElement *convert0_;\n> -\tGstElement *sink0_;\n> +\tGstElement *stream0_;\n> };\n> \n> TEST_REGISTER(GstreamerSingleStreamTest)","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 4CDB5BDC71\n\tfor ;\n\tMon, 20 Sep 2021 15:27:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A5C96918A;\n\tMon, 20 Sep 2021 17:27:44 +0200 (CEST)","from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com\n\t[IPv6:2607:f8b0:4864:20::72f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1786B69186\n\tfor ;\n\tMon, 20 Sep 2021 17:27:43 +0200 (CEST)","by mail-qk1-x72f.google.com with SMTP id a10so43889014qka.12\n\tfor ;\n\tMon, 20 Sep 2021 08:27:43 -0700 (PDT)","from nicolas-tpx395.localdomain (173-246-12-168.qc.cable.ebox.net.\n\t[173.246.12.168]) by smtp.gmail.com with ESMTPSA id\n\ta17sm1160046qtn.86.2021.09.20.08.27.40\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 20 Sep 2021 08:27:41 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20210112.gappssmtp.com\n\theader.i=@ndufresne-ca.20210112.gappssmtp.com\n\theader.b=\"httgRD3X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20210112.gappssmtp.com; s=20210112;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=tW9lC7toedxxD5Xd14XkncQyrsId7Y6xrLnr4osk4iY=;\n\tb=httgRD3XpbrtdVtYpI9qbxVqZA5YyaMcPKpXZJM2oi5kaRkJfAX70cQa2gInk5aPuW\n\tbAYq8kwf2uV133uFZ3BdCZ3Y9XVN+IBzU0ObZJfm2/gc5MlvaJpbR/ZOsQMTplOis9z4\n\tEf8X0aOd7AC2VSelKdlSp2PmMqrKVmfCF8EdpLKf+6Vla6WPJDe8/CJTG+Gq5enzOw5K\n\tS8NVPJH0rVuqzl9fCeJh2n/VkLIBEqJXqZrD8ObOzmqdOUUgWDJKKT7QLo0gm6ZcTUjo\n\tiBbMdOlhku9RmNRnp1bYvBR6Kluo+bkzbDhwOMOSHCcUgKqw6LfNrZ5Px1OZZfUCaB3n\n\to2pw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=tW9lC7toedxxD5Xd14XkncQyrsId7Y6xrLnr4osk4iY=;\n\tb=vWMCMO3tsQVn2ZZP1WAw8rVEoNMGUmruoLxx1HsycCCiBdmXfYXZOEjv0I3SNgccCV\n\tJcrxk6TGra5H5Vb7+gVPK1xT2wNivBSeGvTou+sUtSL4N1H+hru53viYPprCr4AenojK\n\tkz8UGZMzCnQnOV06uO/ggB74UmABQLycIcA732+qBzmG85O/4+auBwXngdHw9UP/bzdQ\n\tZydHJhnkMg5W7MCbSRDFA+EniZrxglHQsUj0PDdezggsv/fVp84KgJO5gXuFnle8q6Ty\n\twXT908M6mUGxnjzIOKAmCFQiVt0xjGL6oISesRwu3uQXQ4tgjV5R1upT/kstdHeE1ZeY\n\t/DGA==","X-Gm-Message-State":"AOAM533rUOGoToXA0steIhCAqILJG5gzy91ArGrInFk5yUHF6Zfr0VaZ\n\tI2xJ2eYp3KZhp+x/SHGnkVo8JQ==","X-Google-Smtp-Source":"ABdhPJwbLsaRobXWuBWlYaURIlTYQ2J9mDTYFzO7cTbihFyJ9dmdb4z+hfU/o4g7udMnGSvYSha4Tg==","X-Received":"by 2002:a05:620a:68c:: with SMTP id\n\tf12mr25019136qkh.363.1632151661680; \n\tMon, 20 Sep 2021 08:27:41 -0700 (PDT)","Message-ID":"","From":"Nicolas Dufresne ","To":"Vedant Paranjape , \n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 20 Sep 2021 11:27:39 -0400","In-Reply-To":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>","References":"<20210915150907.736222-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 v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19724,"web_url":"https://patchwork.libcamera.org/comment/19724/","msgid":"<20210921095533.GA4382@pyrite.rasen.tech>","date":"2021-09-21T09:55:33","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> Simplify memory handling and complexity of the test by using\n> gst_parse_bin_from_description_full [1]. It also fixed memory leak reported\n> by Umang Jain, described in his fix [2].\n\n\"For further information, see this patch that wasn't merged\"\n\nTrue, the patch isn't big or difficult to review, but you did squash two\npatches that were doing distinct things (though they did have the same\nend goal).\n\nPlease split the patches and expand the commit messages (copying what\nUmang had is fine; he said so in the original patch series).\n\n\nPaul\n\n> \n> [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> [2] https://patchwork.libcamera.org/patch/13845/\n> \n> Signed-off-by: Vedant Paranjape \n> ---\n> .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> 1 file changed, 14 insertions(+), 15 deletions(-)\n> \n> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> index 7292f3280617..a20d79109885 100644\n> --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> @@ -33,20 +33,15 @@ protected:\n> \t\tif (status_ != TestPass)\n> \t\t\treturn status_;\n> \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\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> +\t\tconst gchar* streamDescription = \"queue ! videoconvert ! fakesink\";\n> +\t\tg_autoptr(GError) error0 = NULL;\n> +\t\tstream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> \n> +\t\tif (!stream0_) {\n> +\t\t\tg_printerr(\"Not all bins could be created. %p\\n\", stream0_);\n> \t\t\treturn TestFail;\n> \t\t}\n> -\n> -\t\tconvert0_ = reinterpret_cast(g_steal_pointer(&convert0));\n> -\t\tsink0_ = reinterpret_cast(g_steal_pointer(&sink0));\n> +\t\tg_object_ref_sink(stream0_);\n> \n> \t\tif (createPipeline() != TestPass)\n> \t\t\treturn TestFail;\n> @@ -57,8 +52,8 @@ protected:\n> \tint run() override\n> \t{\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> +\t\tgst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);\n> +\t\tif (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> \t\t\tg_printerr(\"Elements could not be linked.\\n\");\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -72,9 +67,13 @@ protected:\n> \t\treturn TestPass;\n> \t}\n> \n> +\tvoid cleanup() override\n> +\t{\n> +\t\tg_clear_object(&stream0_);\n> +\t}\n> +\n> private:\n> -\tGstElement *convert0_;\n> -\tGstElement *sink0_;\n> +\tGstElement *stream0_;\n> };\n> \n> TEST_REGISTER(GstreamerSingleStreamTest)\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 1310CBDC71\n\tfor ;\n\tTue, 21 Sep 2021 09:55:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CCB469188;\n\tTue, 21 Sep 2021 11:55:44 +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 BC25269181\n\tfor ;\n\tTue, 21 Sep 2021 11:55:42 +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 923AD2BA;\n\tTue, 21 Sep 2021 11:55:40 +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=\"R4oS7l53\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632218142;\n\tbh=I0X8dBYyYBO3d0c0ZdH6WZvj+MtKJwsXVyFAdJz9gyM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R4oS7l53TclwAdn9nDfeVlB0EkNujz6lIF8E1izSyfYv843rKCzu4KXjUMU62w4tm\n\td7q6IltoUcjd10bG8T0nMwvyAhuYIj4TUhhXes7FIzB2ocZXc1NRoCH70CN2eMkWG7\n\tt/XkxSQLNmqH48H/rlxuQngxiDXUGP8K0t+9Z4Qg=","Date":"Tue, 21 Sep 2021 18:55:33 +0900","From":"paul.elder@ideasonboard.com","To":"Vedant Paranjape ","Message-ID":"<20210921095533.GA4382@pyrite.rasen.tech>","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19726,"web_url":"https://patchwork.libcamera.org/comment/19726/","msgid":"","date":"2021-09-21T10:20:19","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Paul,\nI don't see the rationale here. I am not fixing the things what Umang\nmentioned.\n\nI just happen to add it in the commit message. @Nicolas Dufresne\n and @Jean-Michel Hautbois\n had told me long back to shift to\nusing parse_bin, so this patch addressed the same. I'll remove the mention\nof Umang's patch, hope it solves the issue.\n\nRegards,\nVedant Paranjape\n\nOn Tue, 21 Sep, 2021, 15:25 , wrote:\n\n> Hi Vedant,\n>\n> On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > Simplify memory handling and complexity of the test by using\n> > gst_parse_bin_from_description_full [1]. It also fixed memory leak\n> reported\n> > by Umang Jain, described in his fix [2].\n>\n> \"For further information, see this patch that wasn't merged\"\n>\n> True, the patch isn't big or difficult to review, but you did squash two\n> patches that were doing distinct things (though they did have the same\n> end goal).\n>\n> Please split the patches and expand the commit messages (copying what\n> Umang had is fine; he said so in the original patch series).\n>\n>\n> Paul\n>\n> >\n> > [1]\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > [2] https://patchwork.libcamera.org/patch/13845/\n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> > 1 file changed, 14 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > index 7292f3280617..a20d79109885 100644\n> > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > @@ -33,20 +33,15 @@ protected:\n> > if (status_ != TestPass)\n> > return status_;\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> > - if (!convert0 || !sink0) {\n> > - g_printerr(\"Not all elements could be created.\n> %p.%p\\n\",\n> > - convert0, sink0);\n> > + const gchar* streamDescription = \"queue ! videoconvert !\n> fakesink\";\n> > + g_autoptr(GError) error0 = NULL;\n> > + stream0_ =\n> gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> >\n> > + if (!stream0_) {\n> > + g_printerr(\"Not all bins could be created. %p\\n\",\n> stream0_);\n> > return TestFail;\n> > }\n> > -\n> > - convert0_ = reinterpret_cast *>(g_steal_pointer(&convert0));\n> > - sink0_ = reinterpret_cast *>(g_steal_pointer(&sink0));\n> > + g_object_ref_sink(stream0_);\n> >\n> > if (createPipeline() != TestPass)\n> > return TestFail;\n> > @@ -57,8 +52,8 @@ protected:\n> > int run() override\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> > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n> stream0_, NULL);\n> > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> > g_printerr(\"Elements could not be linked.\\n\");\n> > return TestFail;\n> > }\n> > @@ -72,9 +67,13 @@ protected:\n> > return TestPass;\n> > }\n> >\n> > + void cleanup() override\n> > + {\n> > + g_clear_object(&stream0_);\n> > + }\n> > +\n> > private:\n> > - GstElement *convert0_;\n> > - GstElement *sink0_;\n> > + GstElement *stream0_;\n> > };\n> >\n> > TEST_REGISTER(GstreamerSingleStreamTest)\n> > --\n> > 2.25.1\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 D425DBDC71\n\tfor ;\n\tTue, 21 Sep 2021 10:20:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2FBBC69183;\n\tTue, 21 Sep 2021 12:20:35 +0200 (CEST)","from mail-qk1-x72a.google.com (mail-qk1-x72a.google.com\n\t[IPv6:2607:f8b0:4864:20::72a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 44F7360247\n\tfor ;\n\tTue, 21 Sep 2021 12:20:33 +0200 (CEST)","by mail-qk1-x72a.google.com with SMTP id f130so13380630qke.6\n\tfor ;\n\tTue, 21 Sep 2021 03:20:33 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"TI8FgnMi\"; 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=jxzk0z7O/sxUx13OXgvKeP0YaH86jzV84VmgoNN8RFs=;\n\tb=TI8FgnMite9xgOmw+E7nQbRD1KqOtP5cIj39y2A+TAOO1CUCxktCF/CSFY+Tu0fGIy\n\tTQhTK4x6zXper3YZRCrA8UYahthoJwzVBHvoBmwP1I1m7Y/uPzNJt8ui1VCkW23Qissp\n\tWvxBrKJZwXLbrz76wwYYqj5l/d3ik8rNrPS4Q3bgXGidVx/m41zDZo5q7iGS66V0cgO9\n\t9QfauDQbr1/felXXBbXc/VknB9sj6GssKBYZMaEPlZBk98oJf0krQHSI/zYVF0xwC0kW\n\tnvy5nv0BCiK9ZEo9bCwpsT0FiYBYuztznuRnioEg7UKRjbrEv8ZdeqwbxcW1N4Ivnn3j\n\t/pBA==","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=jxzk0z7O/sxUx13OXgvKeP0YaH86jzV84VmgoNN8RFs=;\n\tb=kCcpXnJxK//ywpoCTJNec9B6eAoGCOyMVcibn54IoIO4U1FUu52nSzXVcscK6UFqcP\n\t53IfsaYbRvOhoHsJMN2K5m/gwbykpr9muHsd167w0M+L34jXW/PJ88FInwb8UlxpBxZR\n\tfWQq75mgkwPourDBhJHfUlRschHelUdB7ptnzzSC0Aq5r1KYcmUpBZ9jnEO1V3KwAZRr\n\tBIJZNJt54T2e26jKlcTMarewHcCEUQ2R75FDCp/VtRp6LLJCaumTUYVn5dZRHdh31xyk\n\tM5BWc189Q/ymhkcU1AxQur1D0vAtTe2qtJgKXVXNG0mzieSQV1nPo9+22WbbPjll0mRK\n\txstw==","X-Gm-Message-State":"AOAM532q1ir99EZB7CcpUF7yCc5NIltOjCR/q4XyxNEx5n/cHH/9KXr6\n\t+Gb7YPAvX91qMHfAG0gHIpNTBgrzsxNpSe8XeQs=","X-Google-Smtp-Source":"ABdhPJzeYQqPeZlj3nEg8FoaL9Jmi3MeEl74NG/Pr8kjZq862YUcWqeS37Y7w5sv7jAo91FbIGN8ZNYsB40/JtPgEzs=","X-Received":"by 2002:a25:c504:: with SMTP id\n\tv4mr38624218ybe.308.1632219631911; \n\tTue, 21 Sep 2021 03:20:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t<20210921095533.GA4382@pyrite.rasen.tech>","In-Reply-To":"<20210921095533.GA4382@pyrite.rasen.tech>","From":"Vedant Paranjape ","Date":"Tue, 21 Sep 2021 15:50:19 +0530","Message-ID":"","To":"Paul Elder , \n\tJean-Michel Hautbois ","Content-Type":"multipart/alternative; boundary=\"0000000000005b055405cc7ebf7d\"","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19727,"web_url":"https://patchwork.libcamera.org/comment/19727/","msgid":"","date":"2021-09-21T10:24:08","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi,\nUmang has already sent a fix patch series, which got R-b from Nicolas.\n\nApply that into master and then merge mine. I think this should solve the\nproblem, I'll just rebase my patch then.\n\nOn Tue, 21 Sep, 2021, 15:25 , wrote:\n\n> Hi Vedant,\n>\n> On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > Simplify memory handling and complexity of the test by using\n> > gst_parse_bin_from_description_full [1]. It also fixed memory leak\n> reported\n> > by Umang Jain, described in his fix [2].\n>\n> \"For further information, see this patch that wasn't merged\"\n>\n> True, the patch isn't big or difficult to review, but you did squash two\n> patches that were doing distinct things (though they did have the same\n> end goal).\n>\n> Please split the patches and expand the commit messages (copying what\n> Umang had is fine; he said so in the original patch series).\n>\n>\n> Paul\n>\n> >\n> > [1]\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > [2] https://patchwork.libcamera.org/patch/13845/\n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> > 1 file changed, 14 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > index 7292f3280617..a20d79109885 100644\n> > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > @@ -33,20 +33,15 @@ protected:\n> > if (status_ != TestPass)\n> > return status_;\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> > - if (!convert0 || !sink0) {\n> > - g_printerr(\"Not all elements could be created.\n> %p.%p\\n\",\n> > - convert0, sink0);\n> > + const gchar* streamDescription = \"queue ! videoconvert !\n> fakesink\";\n> > + g_autoptr(GError) error0 = NULL;\n> > + stream0_ =\n> gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> >\n> > + if (!stream0_) {\n> > + g_printerr(\"Not all bins could be created. %p\\n\",\n> stream0_);\n> > return TestFail;\n> > }\n> > -\n> > - convert0_ = reinterpret_cast *>(g_steal_pointer(&convert0));\n> > - sink0_ = reinterpret_cast *>(g_steal_pointer(&sink0));\n> > + g_object_ref_sink(stream0_);\n> >\n> > if (createPipeline() != TestPass)\n> > return TestFail;\n> > @@ -57,8 +52,8 @@ protected:\n> > int run() override\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> > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n> stream0_, NULL);\n> > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> > g_printerr(\"Elements could not be linked.\\n\");\n> > return TestFail;\n> > }\n> > @@ -72,9 +67,13 @@ protected:\n> > return TestPass;\n> > }\n> >\n> > + void cleanup() override\n> > + {\n> > + g_clear_object(&stream0_);\n> > + }\n> > +\n> > private:\n> > - GstElement *convert0_;\n> > - GstElement *sink0_;\n> > + GstElement *stream0_;\n> > };\n> >\n> > TEST_REGISTER(GstreamerSingleStreamTest)\n> > --\n> > 2.25.1\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 E7599BF01C\n\tfor ;\n\tTue, 21 Sep 2021 10:24:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5FFF269183;\n\tTue, 21 Sep 2021 12:24:24 +0200 (CEST)","from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com\n\t[IPv6:2607:f8b0:4864:20::72f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2BA0C60247\n\tfor ;\n\tTue, 21 Sep 2021 12:24:23 +0200 (CEST)","by mail-qk1-x72f.google.com with SMTP id p4so52832433qki.3\n\tfor ;\n\tTue, 21 Sep 2021 03:24:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"pD8a0YjK\"; 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=ZuK1aOHI0mXrIyWXkt3LtUeC/jO99PE8ytQBMh4DbMU=;\n\tb=pD8a0YjKjR3/omjnni776kaZu1PbkIZOAAXBmdXiyrG0xUbEdKrkb+KZDBzd53viXa\n\tDVs7pZOLBdN3mi4cAscOFRuXUp7bALltamRMjheuWvZi3edQOUfBQ3iRNg81QU1BU4On\n\t/pqu6PnNQTRHLO0qMVGZNGFtJHu8K3xI0+X6GD6C/XmuEK3unvGyxNvlpgZEl17qA0hW\n\t/UxQkmcN1um4WtDoN9JU2nN0dLbN+Lt5Imtekju1Jgunk2YTXHgEjndsV0Zx84o+LTBd\n\tWmI+V+iBs+nueXTD6nhkfB2YPMcKGQNr/LharZBrzTD4rJQLI/wnUoFd1Ah8yexNml6t\n\t50IQ==","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=ZuK1aOHI0mXrIyWXkt3LtUeC/jO99PE8ytQBMh4DbMU=;\n\tb=xLZ264eAF/i2SSI7jGicRYRD30Uy2QJg5Pl8F2jmJh+ezmy1XeXA2VMr6biY90ozLA\n\tw5AyPwY+FO01zczGJzvtr1LEQ8D25AROJEQQM5mXdUhjvw80YkbS9EculiphS5AL2rkY\n\tA7jBf9k9rfib+LWcy+X8dZx8/GQjMeUxS2MyMuUfWzOnxuDVsmYiA6x7c12nDoGptzKm\n\tLmX3miTll51QcBdUnOnA9E+6JFER910+axPjXsjJkQMAOLzEZQiD32yQXUJA2MlARjF6\n\tOUm3lod0fAbCtYDeB/+2+YjrrHjyMSiXQdiUOXYR7+kHvhoiL+1e9yAk2Bggjva2hGp2\n\tfkaA==","X-Gm-Message-State":"AOAM533/+nzZuyR2IWjdhtsWPLIZst/4BZzqRJIKlfH3j71PNjTfTAPl\n\tCubxiNh2uknjy3Wn4Im3Xf0YjyXied77LLw/6+08AnnOrl8=","X-Google-Smtp-Source":"ABdhPJxwLbO5NDBrawfPod/CejreGil2aSx9yep/dKPcTI6S7MbKGiAWGX35TdKbm+L5uE+iM3oaR4VjXM7JMm6pq1A=","X-Received":"by 2002:a05:6902:1247:: with SMTP id\n\tt7mr37512687ybu.285.1632219860867; \n\tTue, 21 Sep 2021 03:24:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t<20210921095533.GA4382@pyrite.rasen.tech>","In-Reply-To":"<20210921095533.GA4382@pyrite.rasen.tech>","From":"Vedant Paranjape ","Date":"Tue, 21 Sep 2021 15:54:08 +0530","Message-ID":"","To":"Paul Elder ","Content-Type":"multipart/alternative; boundary=\"000000000000009d6205cc7ecd71\"","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19740,"web_url":"https://patchwork.libcamera.org/comment/19740/","msgid":"<5b334ddba45ccb03476f175a1f09d7cbbaa2e6fe.camel@ndufresne.ca>","date":"2021-09-21T12:55:17","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mardi 21 septembre 2021 à 18:55 +0900, paul.elder@ideasonboard.com a écrit :\n> Hi Vedant,\n> \n> On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > Simplify memory handling and complexity of the test by using\n> > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported\n> > by Umang Jain, described in his fix [2].\n> \n> \"For further information, see this patch that wasn't merged\"\n> \n> True, the patch isn't big or difficult to review, but you did squash two\n> patches that were doing distinct things (though they did have the same\n> end goal).\n> \n> Please split the patches and expand the commit messages (copying what\n> Umang had is fine; he said so in the original patch series).\n\nMy personal opinion on the manner is that we are right now being over picky for\nsome splitting that in my opinion is not a libcamera bug, but a test bug. I\nrespect your request, but I'd like to underline that we need to make a tradeoff\nat some point, specially for code that isn't part of libcamera. This is rather\ndiscouraging on top of all the effort that is needed to \"edit\" email based\npatches.\n\n> \n> \n> Paul\n> \n> > \n> > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > [2] https://patchwork.libcamera.org/patch/13845/\n> > \n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> > 1 file changed, 14 insertions(+), 15 deletions(-)\n> > \n> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > index 7292f3280617..a20d79109885 100644\n> > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > @@ -33,20 +33,15 @@ protected:\n> > \t\tif (status_ != TestPass)\n> > \t\t\treturn status_;\n> > \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\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> > +\t\tconst gchar* streamDescription = \"queue ! videoconvert ! fakesink\";\n> > +\t\tg_autoptr(GError) error0 = NULL;\n> > +\t\tstream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> > \n> > +\t\tif (!stream0_) {\n> > +\t\t\tg_printerr(\"Not all bins could be created. %p\\n\", stream0_);\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > -\n> > -\t\tconvert0_ = reinterpret_cast(g_steal_pointer(&convert0));\n> > -\t\tsink0_ = reinterpret_cast(g_steal_pointer(&sink0));\n> > +\t\tg_object_ref_sink(stream0_);\n> > \n> > \t\tif (createPipeline() != TestPass)\n> > \t\t\treturn TestFail;\n> > @@ -57,8 +52,8 @@ protected:\n> > \tint run() override\n> > \t{\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> > +\t\tgst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);\n> > +\t\tif (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> > \t\t\tg_printerr(\"Elements could not be linked.\\n\");\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > @@ -72,9 +67,13 @@ protected:\n> > \t\treturn TestPass;\n> > \t}\n> > \n> > +\tvoid cleanup() override\n> > +\t{\n> > +\t\tg_clear_object(&stream0_);\n> > +\t}\n> > +\n> > private:\n> > -\tGstElement *convert0_;\n> > -\tGstElement *sink0_;\n> > +\tGstElement *stream0_;\n> > };\n> > \n> > TEST_REGISTER(GstreamerSingleStreamTest)\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 4A301BDC71\n\tfor ;\n\tTue, 21 Sep 2021 12:55:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 14BF96918A;\n\tTue, 21 Sep 2021 14:55:22 +0200 (CEST)","from mail-qv1-xf29.google.com (mail-qv1-xf29.google.com\n\t[IPv6:2607:f8b0:4864:20::f29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78A0660247\n\tfor ;\n\tTue, 21 Sep 2021 14:55:20 +0200 (CEST)","by mail-qv1-xf29.google.com with SMTP id cf2so13374152qvb.10\n\tfor ;\n\tTue, 21 Sep 2021 05:55:20 -0700 (PDT)","from nicolas-tpx395.localdomain (173-246-12-168.qc.cable.ebox.net.\n\t[173.246.12.168]) by smtp.gmail.com with ESMTPSA id\n\tq12sm558804qkc.104.2021.09.21.05.55.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 21 Sep 2021 05:55:18 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20210112.gappssmtp.com\n\theader.i=@ndufresne-ca.20210112.gappssmtp.com\n\theader.b=\"DpmqhCOE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20210112.gappssmtp.com; s=20210112;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=6XQyBg4O92WcV8LSowdgkpqcBLwOipfgztSpGjGlQk8=;\n\tb=DpmqhCOEDWEinDgaOakSUUc3sk1P524fQcQfaTLmmddxcLNXgMSpxkwh6mudlDSt1d\n\tq66D9iUwCUCzRbMKd+cJwski98lv8fHray2vkjN/QQDeKRP8WjFcF1jdXrnLQHIL0QvW\n\t+XCo22TBCnCSDvMweMgXoayN8aGVAidfdy7oSYn5f0nkSrhYdLXXlsA9m/c3fQgxh6g5\n\tnNxPaOwraOZk3vKYkDe7MX4YI/mj4Sfapay744IBkfAY6BebOFn3aohJ1cBfkbrMKeWe\n\twskrgi4VhpyxzGdNR10WzKc4iKTPQuZ62obozoiAj5BAng29Eq6tC5xAGjhCwLLGuJHZ\n\tFTOw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=6XQyBg4O92WcV8LSowdgkpqcBLwOipfgztSpGjGlQk8=;\n\tb=Ksz3PUbJ4XgBdTfoUKOuL/4eW/8tg6Mcm6UVAuNcM1JcqbOad1pQtXF6jtRNZPWg/p\n\tI6n6yXqx9pYk3/JnmACfdFaV9ZxXO7ETam/fZRasI1vZCSiPKXG29BlWXSMhOL0+w6UJ\n\tFxqWY3ynJFT4TBn0mI0nQTI+TDyoS3n7csvyqm9jSKbEsP5+zIBg0MsoncM+pdeiWF/J\n\tiM/G7OrLkHaAHamXaQIXCWwy+CgcgVWh9iG7QVbZS8yK9eMl0yD08JhL+QWLxkDAPpy9\n\t4pvOBvu10FzakyUicA3qMWXgtNXtrRVGStGvZrmZdvBYoUGv7j7r5yXvEnwoQIKB36Aq\n\tMk1A==","X-Gm-Message-State":"AOAM530/ExvzKqVItZntWKdfsB4D7j986AZp08VpRlM5YKr17IAX+8Jg\n\tkdA3tZo6p0lmy+6VNkssC6f9xw==","X-Google-Smtp-Source":"ABdhPJxq8WGGZks+3Cfk6rYzY8C7WjNE//llevCac41+/HmkQ2YiaN1E9W23weO6Mz3MbKgZqXPrEQ==","X-Received":"by 2002:ad4:4b0b:: with SMTP id\n\tr11mr30817422qvw.49.1632228919390; \n\tTue, 21 Sep 2021 05:55:19 -0700 (PDT)","Message-ID":"<5b334ddba45ccb03476f175a1f09d7cbbaa2e6fe.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"paul.elder@ideasonboard.com, Vedant Paranjape\n\t","Date":"Tue, 21 Sep 2021 08:55:17 -0400","In-Reply-To":"<20210921095533.GA4382@pyrite.rasen.tech>","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t<20210921095533.GA4382@pyrite.rasen.tech>","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 v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19751,"web_url":"https://patchwork.libcamera.org/comment/19751/","msgid":"","date":"2021-09-21T17:16:37","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Vedant,\n\nThank you for the patch.\n\nOn Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> Simplify memory handling and complexity of the test by using\n> gst_parse_bin_from_description_full [1]. It also fixed memory leak reported\n> by Umang Jain, described in his fix [2].\n> \n> [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> [2] https://patchwork.libcamera.org/patch/13845/\n> \n> Signed-off-by: Vedant Paranjape \n> ---\n> .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> 1 file changed, 14 insertions(+), 15 deletions(-)\n> \n> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> index 7292f3280617..a20d79109885 100644\n> --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> @@ -33,20 +33,15 @@ protected:\n> \t\tif (status_ != TestPass)\n> \t\t\treturn status_;\n> \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\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> +\t\tconst gchar* streamDescription = \"queue ! videoconvert ! fakesink\";\n\nThere's a queue introduced here that isn't mentioned in the commit\nmessage. Why is it needed ?\n\n> +\t\tg_autoptr(GError) error0 = NULL;\n> +\t\tstream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n\nThat's a very long line. Wrapping at 80 columns wouldn't be very\npractical here, but\n\n\t\tstream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n\t\t\t\t\t\t\t GST_PARSE_FLAG_FATAL_ERRORS,\n\t\t\t\t\t\t\t &error0);\nwould already be more readable I think.\n\n> \n> +\t\tif (!stream0_) {\n> +\t\t\tg_printerr(\"Not all bins could be created. %p\\n\", stream0_);\n\nGiven that stream0_ is null here, why do you print it ? Can error0 be\nprinted instead somehow (not as a %p I suppose) ?\n\n> \t\t\treturn TestFail;\n> \t\t}\n> -\n> -\t\tconvert0_ = reinterpret_cast(g_steal_pointer(&convert0));\n> -\t\tsink0_ = reinterpret_cast(g_steal_pointer(&sink0));\n> +\t\tg_object_ref_sink(stream0_);\n> \n> \t\tif (createPipeline() != TestPass)\n> \t\t\treturn TestFail;\n> @@ -57,8 +52,8 @@ protected:\n> \tint run() override\n> \t{\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> +\t\tgst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);\n> +\t\tif (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> \t\t\tg_printerr(\"Elements could not be linked.\\n\");\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -72,9 +67,13 @@ protected:\n> \t\treturn TestPass;\n> \t}\n> \n> +\tvoid cleanup() override\n> +\t{\n> +\t\tg_clear_object(&stream0_);\n> +\t}\n> +\n> private:\n> -\tGstElement *convert0_;\n> -\tGstElement *sink0_;\n> +\tGstElement *stream0_;\n> };\n> \n> TEST_REGISTER(GstreamerSingleStreamTest)","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 B7CF7BDC71\n\tfor ;\n\tTue, 21 Sep 2021 17:17:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7B6E6918C;\n\tTue, 21 Sep 2021 19:17:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7085C60247\n\tfor ;\n\tTue, 21 Sep 2021 19:17:08 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D43342BA;\n\tTue, 21 Sep 2021 19:17:07 +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=\"Gf2K92wD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632244628;\n\tbh=QBV6A8WQTNPnjPx8hKV8kZ2O1daRtRub5m3J2POsmRM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Gf2K92wDTFqj45goRvOp4xXx+RnjdS/GSASoK7nDzYGuPSWmfgHaz7o2eadJZYtRi\n\tXirqyAVmuoptmL6+sP7bx0HDPuqtQZC6ldQrPYVNPpNZJzXFvmuK3ZcEz2FYAtScMo\n\tvRhGgnQWVeHE06l1gEMDYWBUmxvLEZ9tqVco+FJk=","Date":"Tue, 21 Sep 2021 20:16:37 +0300","From":"Laurent Pinchart ","To":"Vedant Paranjape ","Message-ID":"","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19752,"web_url":"https://patchwork.libcamera.org/comment/19752/","msgid":"","date":"2021-09-21T17:19:54","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Laurent,\nThese patches are stale. I have sent a new patch series replacing this\nplease take a look at it.\n\nEven though your comments do apply there too.\n\nRegards,\nVedant Paranjape\n\nOn Tue, 21 Sep, 2021, 22:47 Laurent Pinchart, <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Vedant,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > Simplify memory handling and complexity of the test by using\n> > gst_parse_bin_from_description_full [1]. It also fixed memory leak\n> reported\n> > by Umang Jain, described in his fix [2].\n> >\n> > [1]\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > [2] https://patchwork.libcamera.org/patch/13845/\n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> > 1 file changed, 14 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > index 7292f3280617..a20d79109885 100644\n> > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > @@ -33,20 +33,15 @@ protected:\n> > if (status_ != TestPass)\n> > return status_;\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> > - if (!convert0 || !sink0) {\n> > - g_printerr(\"Not all elements could be created.\n> %p.%p\\n\",\n> > - convert0, sink0);\n> > + const gchar* streamDescription = \"queue ! videoconvert !\n> fakesink\";\n>\n> There's a queue introduced here that isn't mentioned in the commit\n> message. Why is it needed ?\n>\n> > + g_autoptr(GError) error0 = NULL;\n> > + stream0_ =\n> gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n>\n> That's a very long line. Wrapping at 80 columns wouldn't be very\n> practical here, but\n>\n> stream0_ =\n> gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n>\n> GST_PARSE_FLAG_FATAL_ERRORS,\n> &error0);\n> would already be more readable I think.\n>\n> >\n> > + if (!stream0_) {\n> > + g_printerr(\"Not all bins could be created. %p\\n\",\n> stream0_);\n>\n> Given that stream0_ is null here, why do you print it ? Can error0 be\n> printed instead somehow (not as a %p I suppose) ?\n>\n> > return TestFail;\n> > }\n> > -\n> > - convert0_ = reinterpret_cast *>(g_steal_pointer(&convert0));\n> > - sink0_ = reinterpret_cast *>(g_steal_pointer(&sink0));\n> > + g_object_ref_sink(stream0_);\n> >\n> > if (createPipeline() != TestPass)\n> > return TestFail;\n> > @@ -57,8 +52,8 @@ protected:\n> > int run() override\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> > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n> stream0_, NULL);\n> > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> > g_printerr(\"Elements could not be linked.\\n\");\n> > return TestFail;\n> > }\n> > @@ -72,9 +67,13 @@ protected:\n> > return TestPass;\n> > }\n> >\n> > + void cleanup() override\n> > + {\n> > + g_clear_object(&stream0_);\n> > + }\n> > +\n> > private:\n> > - GstElement *convert0_;\n> > - GstElement *sink0_;\n> > + GstElement *stream0_;\n> > };\n> >\n> > TEST_REGISTER(GstreamerSingleStreamTest)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BA609BF01C\n\tfor ;\n\tTue, 21 Sep 2021 17:20:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E665F6918E;\n\tTue, 21 Sep 2021 19:20:10 +0200 (CEST)","from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com\n\t[IPv6:2607:f8b0:4864:20::72f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 23DD360247\n\tfor ;\n\tTue, 21 Sep 2021 19:20:09 +0200 (CEST)","by mail-qk1-x72f.google.com with SMTP id bk29so56682209qkb.8\n\tfor ;\n\tTue, 21 Sep 2021 10:20:09 -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=\"mt9wglaY\"; 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=owP2cFhYjVXuqLLPoQl0ULrfRHqR5YOPbRdFFPk53e0=;\n\tb=mt9wglaYev6Nyj3zk/TnBJYNx4QVJSPOAlha50r8B5LOnxR6s0Ui8zmwGUNUWi9K5P\n\tbuhXD0XL7LZL9HRotMiSlFedzb4zsrg5anYw3hmBsL6a3QFmlGnWfHtHQfCj2b6d3ngv\n\t9SP6B7ctSYccXklC/eEbuwb7GCUo4BW1Bc2DrCJlUuTgzOwoQvLDUOHVK2lHofcUCqBg\n\tabmP3TUTIX3zLa9Zt9A60igPQhec+5ybUWR6uN2Pt16+cCvEo3fOw+smcrW9RfIqv8Hs\n\tQRf8rnaI7vIZazo7nBVihh//kWOfvhPeEcXL8CCsf0rH/1AHLTDazdw+E+DXdKwBLDiz\n\th0ow==","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=owP2cFhYjVXuqLLPoQl0ULrfRHqR5YOPbRdFFPk53e0=;\n\tb=pDLwgF4ZI/xKJKWrMO/R+P+WX2i+TNsWE5XwgK4H4BsDZC2isE0aB5YQjpDjxCrn19\n\todmMjOyFi+C/eadnNgkr2Oha6742cQuQ4v7l1WdKaCt7dQpuTN5RRZrB8ccKcnO+6uhn\n\t4CeI4jkxiNXZAoRuU7fOBKuMFYgAr4gskDf471X4/I4LWSpYsLqs2p0Fkgii2htba5bl\n\tldyuohQbbD3iANky6pl1vHd/xBs6bSaUa0IRreUjo4iMeZDXADUNiXFSEWI4gm6PEFtt\n\td1f0Y1hqII19SOjiwwca3vIvaF9+UC4wubIgWfITL8H+uaIgmHV48wke0u4TIY0NPHvZ\n\t3vxQ==","X-Gm-Message-State":"AOAM533lVDaCgaRrUeE0HqmX8CsPHrr3lM/mksFjVmJEiEiA1gIhcjgm\n\tbi+6gPATG5Mw6MlYKlPIwtZalJR8ieNj+PxwSNw=","X-Google-Smtp-Source":"ABdhPJxhiUmgh5k49A6PvTgzid3SXNgHppMxuKLS3o5BtF1qtL9qHfOkqh6oQHm/8Lgg6Owoq7jgXxoROAXjOCMhIlE=","X-Received":"by 2002:a25:b5ce:: with SMTP id\n\td14mr39192055ybg.415.1632244807275; \n\tTue, 21 Sep 2021 10:20:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t","In-Reply-To":"","From":"Vedant Paranjape ","Date":"Tue, 21 Sep 2021 22:49:54 +0530","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000ec98bd05cc849b9a\"","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19753,"web_url":"https://patchwork.libcamera.org/comment/19753/","msgid":"","date":"2021-09-21T17:25:11","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Laurent,\nThanks for the review.\n\nOn Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Vedant,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > Simplify memory handling and complexity of the test by using\n> > gst_parse_bin_from_description_full [1]. It also fixed memory leak\n> reported\n> > by Umang Jain, described in his fix [2].\n> >\n> > [1]\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > [2] https://patchwork.libcamera.org/patch/13845/\n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> > 1 file changed, 14 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > index 7292f3280617..a20d79109885 100644\n> > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > @@ -33,20 +33,15 @@ protected:\n> > if (status_ != TestPass)\n> > return status_;\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> > - if (!convert0 || !sink0) {\n> > - g_printerr(\"Not all elements could be created.\n> %p.%p\\n\",\n> > - convert0, sink0);\n> > + const gchar* streamDescription = \"queue ! videoconvert !\n> fakesink\";\n>\n> There's a queue introduced here that isn't mentioned in the commit\n> message. Why is it needed ?\n\n\nWARNING: from element\n/GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage:\nPipeline construction is invalid, please add queues.\n\nBecause Gstreamer says so, it won't be needed with fakesink I think, I'll\nfix it :)\n\n\n> > + g_autoptr(GError) error0 = NULL;\n> > + stream0_ =\n> gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n>\n> That's a very long line. Wrapping at 80 columns wouldn't be very\n> practical here, but\n>\n> stream0_ =\n> gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n>\n> GST_PARSE_FLAG_FATAL_ERRORS,\n> &error0);\n> would already be more readable I think.\n>\n> >\n> > + if (!stream0_) {\n> > + g_printerr(\"Not all bins could be created. %p\\n\",\n> stream0_);\n>\n> Given that stream0_ is null here, why do you print it ? Can error0 be\n> printed instead somehow (not as a %p I suppose) ?\n>\n\nNice catch. I'll make the changes.\n\n> return TestFail;\n> > }\n> > -\n> > - convert0_ = reinterpret_cast *>(g_steal_pointer(&convert0));\n> > - sink0_ = reinterpret_cast *>(g_steal_pointer(&sink0));\n> > + g_object_ref_sink(stream0_);\n> >\n> > if (createPipeline() != TestPass)\n> > return TestFail;\n> > @@ -57,8 +52,8 @@ protected:\n> > int run() override\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> > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n> stream0_, NULL);\n> > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> > g_printerr(\"Elements could not be linked.\\n\");\n> > return TestFail;\n> > }\n> > @@ -72,9 +67,13 @@ protected:\n> > return TestPass;\n> > }\n> >\n> > + void cleanup() override\n> > + {\n> > + g_clear_object(&stream0_);\n> > + }\n> > +\n> > private:\n> > - GstElement *convert0_;\n> > - GstElement *sink0_;\n> > + GstElement *stream0_;\n> > };\n> >\n> > TEST_REGISTER(GstreamerSingleStreamTest)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n\nRegards,\n*Vedant Paranjape*","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 280D2BDC71\n\tfor ;\n\tTue, 21 Sep 2021 17:25:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B3EF6918C;\n\tTue, 21 Sep 2021 19:25:28 +0200 (CEST)","from mail-qk1-x72b.google.com (mail-qk1-x72b.google.com\n\t[IPv6:2607:f8b0:4864:20::72b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A840260247\n\tfor ;\n\tTue, 21 Sep 2021 19:25:26 +0200 (CEST)","by mail-qk1-x72b.google.com with SMTP id p4so57359815qki.3\n\tfor ;\n\tTue, 21 Sep 2021 10:25:26 -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=\"M3UOKq6y\"; 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=z2QakAURg2MMRs31hAoNLC/GeeLiJoyXMjdLqT8V5Ng=;\n\tb=M3UOKq6ylByaLb/i4dUTFLwxKVD8EObywiwBeaEbpsVf7Rmv7oQ0bAp+jRxysmnryx\n\tNuSMm5iWe/qfOxLpGdX/xlH+wSbB5PDWn5SuxYO6viCb82kmKR/Kx78Xt1gDpJGwWTQv\n\trj2/A3mp4EyOymapRuwHE9OF/y9++gmphUZOYkSkoCEabhSPf/7HlaR6VTg0pBKEGCWd\n\tNCNkYs0MVRaPJirEM0fjFD+0JgcOzS/2W1CTTuFDnnkxE8p7vxpCUFQTM2b9ndvG2Fg4\n\tWc4AhPqGx3GNKSC3ufH11eT9Tqa44RC+q35wIwHpyfLyPORCa8Q3uLd6GL4HkbIBT9r7\n\tyoiw==","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=z2QakAURg2MMRs31hAoNLC/GeeLiJoyXMjdLqT8V5Ng=;\n\tb=JDblaxkMllR66EvokLXyjoBsMYGfGdpRMfOBQS+7CRrJw3GbBUOav4Jk33KIdTFftY\n\teWKBaQ0P7cegifnKj4XqB0DdzCqQlQ41RdswB4eOX7dqP2vrdjdP79Tiw5teadZEmI8V\n\tQCAyf0cZXdL7OXA5Xphs6GI4H21RciD3dgk0/GDZjn0yIV7gm2R5Wl/mvn+FbCMQWiJ+\n\tq7HRk1TQqsc3zhkKLQTjQhgKpoCdis1NQno33ExwGtudyTkV0yeOF4bN2RHdeaJST+B2\n\t7IkNws28aSiEWg32nVBDm4Sk3dxMRcUsIfeg59UDOLgkzPOtcaZ2N1fxEKpacVnW1L/s\n\tlF+Q==","X-Gm-Message-State":"AOAM533QHLhp1k8TxgoQyWfMVM3BAt/9J32Bx7+16ABC0Z5lgHWP+ztz\n\tIHOqZAS8FG7qj5rQWT7VCDBb5QuuZiXnakYmgpAp6HQEbSGu0g==","X-Google-Smtp-Source":"ABdhPJy1N+zulESp0dr/RXZmDdYbgKlSxPjHoJQoQzU+4BOKWIoTSjLvkCbZrabmX6rua4smkzMlmNkwKLfm7xXMHB0=","X-Received":"by 2002:a05:6902:1247:: with SMTP id\n\tt7mr39956789ybu.285.1632245125208; \n\tTue, 21 Sep 2021 10:25:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t","In-Reply-To":"","From":"Vedant Paranjape ","Date":"Tue, 21 Sep 2021 22:55:11 +0530","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000dfdec005cc84aee7\"","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19770,"web_url":"https://patchwork.libcamera.org/comment/19770/","msgid":"<20210922050735.GF4382@pyrite.rasen.tech>","date":"2021-09-22T05:07:35","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Tue, Sep 21, 2021 at 08:55:17AM -0400, Nicolas Dufresne wrote:\n> Le mardi 21 septembre 2021 à 18:55 +0900, paul.elder@ideasonboard.com a écrit :\n> > Hi Vedant,\n> > \n> > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > > Simplify memory handling and complexity of the test by using\n> > > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported\n> > > by Umang Jain, described in his fix [2].\n> > \n> > \"For further information, see this patch that wasn't merged\"\n> > \n> > True, the patch isn't big or difficult to review, but you did squash two\n> > patches that were doing distinct things (though they did have the same\n> > end goal).\n> > \n> > Please split the patches and expand the commit messages (copying what\n> > Umang had is fine; he said so in the original patch series).\n> \n> My personal opinion on the manner is that we are right now being over picky for\n\nPerhaps.\n\n> some splitting that in my opinion is not a libcamera bug, but a test bug. I\n> respect your request, but I'd like to underline that we need to make a tradeoff\n> at some point, specially for code that isn't part of libcamera.\n\nIt's still a test in libcamera. I think that such code and history\norganization still needs to be held to an equally high standard. Just\nbecause it's a test doesn't mean the quality and organization can be\ndisregarded.\n\n> This is rather discouraging\n\nI see your point. My apologies for not looking at this earlier.\n\n> on top of all the effort that is needed to \"edit\" email based\n> patches.\n\nI'm not sure what the difference is between editing email-based patches\nvs commits on github is... both are more-or-less just rebasing and\nresending.\n\n\nPaul\n\n> \n> > \n> > \n> > Paul\n> > \n> > > \n> > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > > [2] https://patchwork.libcamera.org/patch/13845/\n> > > \n> > > Signed-off-by: Vedant Paranjape \n> > > ---\n> > > .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> > > 1 file changed, 14 insertions(+), 15 deletions(-)\n> > > \n> > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > index 7292f3280617..a20d79109885 100644\n> > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > @@ -33,20 +33,15 @@ protected:\n> > > \t\tif (status_ != TestPass)\n> > > \t\t\treturn status_;\n> > > \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\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> > > +\t\tconst gchar* streamDescription = \"queue ! videoconvert ! fakesink\";\n> > > +\t\tg_autoptr(GError) error0 = NULL;\n> > > +\t\tstream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> > > \n> > > +\t\tif (!stream0_) {\n> > > +\t\t\tg_printerr(\"Not all bins could be created. %p\\n\", stream0_);\n> > > \t\t\treturn TestFail;\n> > > \t\t}\n> > > -\n> > > -\t\tconvert0_ = reinterpret_cast(g_steal_pointer(&convert0));\n> > > -\t\tsink0_ = reinterpret_cast(g_steal_pointer(&sink0));\n> > > +\t\tg_object_ref_sink(stream0_);\n> > > \n> > > \t\tif (createPipeline() != TestPass)\n> > > \t\t\treturn TestFail;\n> > > @@ -57,8 +52,8 @@ protected:\n> > > \tint run() override\n> > > \t{\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> > > +\t\tgst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);\n> > > +\t\tif (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> > > \t\t\tg_printerr(\"Elements could not be linked.\\n\");\n> > > \t\t\treturn TestFail;\n> > > \t\t}\n> > > @@ -72,9 +67,13 @@ protected:\n> > > \t\treturn TestPass;\n> > > \t}\n> > > \n> > > +\tvoid cleanup() override\n> > > +\t{\n> > > +\t\tg_clear_object(&stream0_);\n> > > +\t}\n> > > +\n> > > private:\n> > > -\tGstElement *convert0_;\n> > > -\tGstElement *sink0_;\n> > > +\tGstElement *stream0_;\n> > > };\n> > > \n> > > TEST_REGISTER(GstreamerSingleStreamTest)\n> > > -- \n> > > 2.25.1\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 23EB3BDC71\n\tfor ;\n\tWed, 22 Sep 2021 05:07:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A59256012C;\n\tWed, 22 Sep 2021 07:07:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D15166012C\n\tfor ;\n\tWed, 22 Sep 2021 07:07:44 +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 A4E5DF1;\n\tWed, 22 Sep 2021 07:07:42 +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=\"k2cRRHje\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632287264;\n\tbh=sioTGZhAiGYUNZrLs34slcpaZJHJIOuns3IfCIar0Z0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k2cRRHjeX+PNuG/1+Pi97Zuf4fRgTEGJI8UlgPgHfj9sUp/TWoAtnbJDkoCBJM3T/\n\t9m3chPI9A6r0NSqPL10vBXbCRIkbd40Rg+NwI1ujJtUhfEC+3vEPqphPPmDAiGkEJE\n\t+xEcTee1bLLIkubQXsA0pD0o8rGIUH99vRbAKi+g=","Date":"Wed, 22 Sep 2021 14:07:35 +0900","From":"paul.elder@ideasonboard.com","To":"Nicolas Dufresne ","Message-ID":"<20210922050735.GF4382@pyrite.rasen.tech>","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t<20210921095533.GA4382@pyrite.rasen.tech>\n\t<5b334ddba45ccb03476f175a1f09d7cbbaa2e6fe.camel@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<5b334ddba45ccb03476f175a1f09d7cbbaa2e6fe.camel@ndufresne.ca>","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19780,"web_url":"https://patchwork.libcamera.org/comment/19780/","msgid":"","date":"2021-09-22T08:41:22","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Vedant,\n\nOn Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote:\n> On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote:\n> > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > > Simplify memory handling and complexity of the test by using\n> > > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported\n> > > by Umang Jain, described in his fix [2].\n> > >\n> > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > > [2] https://patchwork.libcamera.org/patch/13845/\n> > >\n> > > Signed-off-by: Vedant Paranjape \n> > > ---\n> > > .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> > > 1 file changed, 14 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > index 7292f3280617..a20d79109885 100644\n> > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > @@ -33,20 +33,15 @@ protected:\n> > > if (status_ != TestPass)\n> > > return status_;\n> > >\n> > > - g_autoptr(GstElement) convert0 = gst_element_factory_make(\"videoconvert\", \"convert0\");\n> > > - g_autoptr(GstElement) sink0 = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > - g_object_ref_sink(convert0);\n> > > - g_object_ref_sink(sink0);\n> > > -\n> > > - if (!convert0 || !sink0) {\n> > > - g_printerr(\"Not all elements could be created. %p.%p\\n\",\n> > > - convert0, sink0);\n> > > + const gchar* streamDescription = \"queue ! videoconvert ! fakesink\";\n> >\n> > There's a queue introduced here that isn't mentioned in the commit\n> > message. Why is it needed ?\n> \n> WARNING: from element\n> /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage:\n> Pipeline construction is invalid, please add queues.\n> \n> Because Gstreamer says so, it won't be needed with fakesink I think, I'll\n> fix it :)\n\nI'm not disputing that, but it wasn't the point. Before this patch, we\ndon't have a queue element inserted in the pipeline as far as I can\ntell. It's added by this patch, for a reason that isn't explained in the\ncommit message. So, if the queue is needed, it should say, but the\nreason needs to be explained (including why the problem doesn't occur\ntoday).\n\n> > > + g_autoptr(GError) error0 = NULL;\n> > > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> >\n> > That's a very long line. Wrapping at 80 columns wouldn't be very\n> > practical here, but\n> >\n> > stream0_ =\n> > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> >\n> > GST_PARSE_FLAG_FATAL_ERRORS,\n> > &error0);\n> > would already be more readable I think.\n> >\n> > >\n> > > + if (!stream0_) {\n> > > + g_printerr(\"Not all bins could be created. %p\\n\",\n> > stream0_);\n> >\n> > Given that stream0_ is null here, why do you print it ? Can error0 be\n> > printed instead somehow (not as a %p I suppose) ?\n> \n> Nice catch. I'll make the changes.\n> \n> > return TestFail;\n> > > }\n> > > -\n> > > - convert0_ = reinterpret_cast(g_steal_pointer(&convert0));\n> > > - sink0_ = reinterpret_cast(g_steal_pointer(&sink0));\n> > > + g_object_ref_sink(stream0_);\n> > >\n> > > if (createPipeline() != TestPass)\n> > > return TestFail;\n> > > @@ -57,8 +52,8 @@ protected:\n> > > int run() override\n> > > {\n> > > /* Build the pipeline */\n> > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL);\n> > > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) {\n> > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);\n> > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> > > g_printerr(\"Elements could not be linked.\\n\");\n> > > return TestFail;\n> > > }\n> > > @@ -72,9 +67,13 @@ protected:\n> > > return TestPass;\n> > > }\n> > >\n> > > + void cleanup() override\n> > > + {\n> > > + g_clear_object(&stream0_);\n> > > + }\n> > > +\n> > > private:\n> > > - GstElement *convert0_;\n> > > - GstElement *sink0_;\n> > > + GstElement *stream0_;\n> > > };\n> > >\n> > > TEST_REGISTER(GstreamerSingleStreamTest)","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 3E53FBDC71\n\tfor ;\n\tWed, 22 Sep 2021 08:41:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8E56A6918C;\n\tWed, 22 Sep 2021 10:41:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 561AA687DD\n\tfor ;\n\tWed, 22 Sep 2021 10:41:24 +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 C1062F1;\n\tWed, 22 Sep 2021 10:41:23 +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=\"i7uTzw20\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632300083;\n\tbh=VH3R8hoVx795E609NlKAcscDhjTFwmSbcpIQhwjqijE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i7uTzw20gTyoYV+gqcllRTXn+7YyJIYuOF52Pi0WOfiWDz/MxJoLHIt8NZxJ9ly/h\n\tr38zb7WE+uQp2TnbKOuZ8OLDgupvU+aelFMF3SVTnAKz4ldswAfzjfTcezNdWcBK2u\n\tbb8XRdTXhcqm55u9BFIvgt5p4Y5eSNA2hQ+eHt+I=","Date":"Wed, 22 Sep 2021 11:41:22 +0300","From":"Laurent Pinchart ","To":"Vedant Paranjape ","Message-ID":"","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19783,"web_url":"https://patchwork.libcamera.org/comment/19783/","msgid":"","date":"2021-09-22T09:10:28","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Laurent,\nautovideosink needs a queue to function correctly. So it was a carry over\nfrom the command description used in terminal.\n\nfakesink has no such requirement, thus it was not added earlier.\n\nOn Wed, 22 Sep, 2021, 14:11 Laurent Pinchart, <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Vedant,\n>\n> On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote:\n> > On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote:\n> > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > > > Simplify memory handling and complexity of the test by using\n> > > > gst_parse_bin_from_description_full [1]. It also fixed memory leak\n> reported\n> > > > by Umang Jain, described in his fix [2].\n> > > >\n> > > > [1]\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > > > [2] https://patchwork.libcamera.org/patch/13845/\n> > > >\n> > > > Signed-off-by: Vedant Paranjape \n> > > > ---\n> > > > .../gstreamer_single_stream_test.cpp | 29\n> +++++++++----------\n> > > > 1 file changed, 14 insertions(+), 15 deletions(-)\n> > > >\n> > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > index 7292f3280617..a20d79109885 100644\n> > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > @@ -33,20 +33,15 @@ protected:\n> > > > if (status_ != TestPass)\n> > > > return status_;\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> > > > - if (!convert0 || !sink0) {\n> > > > - g_printerr(\"Not all elements could be created.\n> %p.%p\\n\",\n> > > > - convert0, sink0);\n> > > > + const gchar* streamDescription = \"queue ! videoconvert\n> ! fakesink\";\n> > >\n> > > There's a queue introduced here that isn't mentioned in the commit\n> > > message. Why is it needed ?\n> >\n> > WARNING: from element\n> >\n> /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage:\n> > Pipeline construction is invalid, please add queues.\n> >\n> > Because Gstreamer says so, it won't be needed with fakesink I think, I'll\n> > fix it :)\n>\n> I'm not disputing that, but it wasn't the point. Before this patch, we\n> don't have a queue element inserted in the pipeline as far as I can\n> tell. It's added by this patch, for a reason that isn't explained in the\n> commit message. So, if the queue is needed, it should say, but the\n> reason needs to be explained (including why the problem doesn't occur\n> today).\n>\n> > > > + g_autoptr(GError) error0 = NULL;\n> > > > + stream0_ =\n> gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> > >\n> > > That's a very long line. Wrapping at 80 columns wouldn't be very\n> > > practical here, but\n> > >\n> > > stream0_ =\n> > > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> > >\n> > > GST_PARSE_FLAG_FATAL_ERRORS,\n> > >\n> &error0);\n> > > would already be more readable I think.\n> > >\n> > > >\n> > > > + if (!stream0_) {\n> > > > + g_printerr(\"Not all bins could be created.\n> %p\\n\",\n> > > stream0_);\n> > >\n> > > Given that stream0_ is null here, why do you print it ? Can error0 be\n> > > printed instead somehow (not as a %p I suppose) ?\n> >\n> > Nice catch. I'll make the changes.\n> >\n> > > return TestFail;\n> > > > }\n> > > > -\n> > > > - convert0_ = reinterpret_cast *>(g_steal_pointer(&convert0));\n> > > > - sink0_ = reinterpret_cast *>(g_steal_pointer(&sink0));\n> > > > + g_object_ref_sink(stream0_);\n> > > >\n> > > > if (createPipeline() != TestPass)\n> > > > return TestFail;\n> > > > @@ -57,8 +52,8 @@ protected:\n> > > > int run() override\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> > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_,\n> stream0_, NULL);\n> > > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE)\n> {\n> > > > g_printerr(\"Elements could not be linked.\\n\");\n> > > > return TestFail;\n> > > > }\n> > > > @@ -72,9 +67,13 @@ protected:\n> > > > return TestPass;\n> > > > }\n> > > >\n> > > > + void cleanup() override\n> > > > + {\n> > > > + g_clear_object(&stream0_);\n> > > > + }\n> > > > +\n> > > > private:\n> > > > - GstElement *convert0_;\n> > > > - GstElement *sink0_;\n> > > > + GstElement *stream0_;\n> > > > };\n> > > >\n> > > > TEST_REGISTER(GstreamerSingleStreamTest)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C709EBF01C\n\tfor ;\n\tWed, 22 Sep 2021 09:10:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F92B6918E;\n\tWed, 22 Sep 2021 11:10:44 +0200 (CEST)","from mail-qk1-x734.google.com (mail-qk1-x734.google.com\n\t[IPv6:2607:f8b0:4864:20::734])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92101687DD\n\tfor ;\n\tWed, 22 Sep 2021 11:10:42 +0200 (CEST)","by mail-qk1-x734.google.com with SMTP id 194so7031685qkj.11\n\tfor ;\n\tWed, 22 Sep 2021 02:10:42 -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=\"deHPrBc8\"; 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=TmjbMyiD5UW3bIQHf943+YD67njupf0PmkuoGb8AUWU=;\n\tb=deHPrBc88dUsm8uW0IyQgJcW9I4yulc3wMhq85f0VGqxeHv4ItpbvYlQdjM+8NcwdA\n\tjGHhpeE0F/2Kgyr6hWfkZv95CGpQMMn8Ue9wUeSR+weKhcsl78v1gXCrrn6bgoee++Iv\n\t8faCxBjjT4mKf9U1pSeKVOzmvLbH9K3dYTiwZqKmMLM377taiGqNOCFSVJf8PBausLjv\n\tw7BKRLoEZRdCPc1p6zQqlPhzXbjVA8tD2wcpteSbYF8ZBbaiNk79RydmBiFGyKkSxIri\n\t4dllRyaNBmcaHk76A/iAZOa6m8ZtKZzTxJRMrGpoaDKC6vg/iE7UM2NLUesvYDILHSnm\n\tljUw==","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=TmjbMyiD5UW3bIQHf943+YD67njupf0PmkuoGb8AUWU=;\n\tb=csusTqwkrN7UrD9ndVoih8XEecfIo1kQslru8jmGb6w5WpnB+cjM3ZsIkj+yxTkabE\n\teh1gfZQxd4kgch+NVWCPvESbt1YAnNQpSfUP3avJN2GMXvZj0gnapncWOggoXs9VjVMt\n\tTE9rvfN7lIVri1/sjXHBGeG3xVmcQWV1lP2g5/NAfiTBrTngylnbdVGGo7P+Xj4GO/yx\n\tUCuvYqOi3O4KHV1eqGl6S9NOMkl583TtI0Wbu/Yj4AV5r3hnIXdE3XF6/BxXizqJsnc2\n\t4JtPEEsQj39Buu15aXewwVxuBl51XuZXjzqkirVfDNafuK7kfUz0kUF+BPdAcl7RvkOf\n\t7aWg==","X-Gm-Message-State":"AOAM531HFO9XJ6l8y/vJeIxGxpqXoSeNrd4WnAbNepU6jWICTSU6neSQ\n\tD5a18sq4sxFHw/MzCIxsTSWlu2HB79DLOeBWZis=","X-Google-Smtp-Source":"ABdhPJya1ZivxzpeZGlRHF3pxtxLSUyDc0G9P18SbFNjtO7Skm3JW4aN+olTbazj7/TWtYar68lrbGr3y0aLRhy9iYM=","X-Received":"by 2002:a25:c504:: with SMTP id\n\tv4mr45448877ybe.308.1632301840954; \n\tWed, 22 Sep 2021 02:10:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Vedant Paranjape ","Date":"Wed, 22 Sep 2021 14:40:28 +0530","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000065799305cc91e36b\"","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19784,"web_url":"https://patchwork.libcamera.org/comment/19784/","msgid":"<5af66fa7-7f91-b013-e0f2-6bdab1a43459@ideasonboard.com>","date":"2021-09-22T09:13:51","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Vedant,\n\nOn 22/09/2021 11:10, Vedant Paranjape wrote:\n> Hi Laurent,\n> autovideosink needs a queue to function correctly. So it was a carry\n> over from the command description used in terminal.\n> \n> fakesink has no such requirement, thus it was not added earlier.\n> \n\nThis should be written in the commit message to clarify it ;-).\n\n> On Wed, 22 Sep, 2021, 14:11 Laurent Pinchart,\n> > wrote:\n> \n> Hi Vedant,\n> \n> On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote:\n> > On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote:\n> > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > > > Simplify memory handling and complexity of the test by using\n> > > > gst_parse_bin_from_description_full [1]. It also fixed memory\n> leak reported\n> > > > by Umang Jain, described in his fix [2].\n> > > >\n> > > > [1]\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> \n> > > > [2] https://patchwork.libcamera.org/patch/13845/\n> \n> > > >\n> > > > Signed-off-by: Vedant Paranjape\n> >\n> > > > ---\n> > > >  .../gstreamer_single_stream_test.cpp          | 29\n> +++++++++----------\n> > > >  1 file changed, 14 insertions(+), 15 deletions(-)\n> > > >\n> > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > index 7292f3280617..a20d79109885 100644\n> > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > @@ -33,20 +33,15 @@ protected:\n> > > >               if (status_ != TestPass)\n> > > >                       return status_;\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\nExcept that you are changing from fakesink to... fakesink, so, maybe\nshould have it been introduced before ? Nevertheless just mention it in\nyour v2 :-).\n\nThanks !\nPS: BTW, could you reply with text only mails and not HTML ? :-)\n\n> > > > -             g_object_ref_sink(convert0);\n> > > > -             g_object_ref_sink(sink0);\n> > > > -\n> > > > -             if (!convert0 || !sink0) {\n> > > > -                     g_printerr(\"Not all elements could be\n> created. %p.%p\\n\",\n> > > > -                                convert0, sink0);\n> > > > +             const gchar* streamDescription = \"queue !\n> videoconvert ! fakesink\";\n> > >\n> > > There's a queue introduced here that isn't mentioned in the commit\n> > > message. Why is it needed ?\n> >\n> > WARNING: from element\n> >\n> /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage:\n> > Pipeline construction is invalid, please add queues.\n> >\n> > Because Gstreamer says so, it won't be needed with fakesink I\n> think, I'll\n> > fix it :)\n> \n> I'm not disputing that, but it wasn't the point. Before this patch, we\n> don't have a queue element inserted in the pipeline as far as I can\n> tell. It's added by this patch, for a reason that isn't explained in the\n> commit message. So, if the queue is needed, it should say, but the\n> reason needs to be explained (including why the problem doesn't occur\n> today).\n> \n> > > > +             g_autoptr(GError) error0 = NULL;\n> > > > +             stream0_ =\n> gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> > >\n> > > That's a very long line. Wrapping at 80 columns wouldn't be very\n> > > practical here, but\n> > >\n> > >                 stream0_ =\n> > > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> > >\n> > >  GST_PARSE_FLAG_FATAL_ERRORS,\n> > >                                                               \n> &error0);\n> > > would already be more readable I think.\n> > >\n> > > >\n> > > > +             if (!stream0_) {\n> > > > +                     g_printerr(\"Not all bins could be\n> created. %p\\n\",\n> > > stream0_);\n> > >\n> > > Given that stream0_ is null here, why do you print it ? Can\n> error0 be\n> > > printed instead somehow (not as a %p I suppose) ?\n> >\n> > Nice catch. I'll make the changes.\n> >\n> > >                       return TestFail;\n> > > >               }\n> > > > -\n> > > > -             convert0_ = reinterpret_cast *>(g_steal_pointer(&convert0));\n> > > > -             sink0_ = reinterpret_cast *>(g_steal_pointer(&sink0));\n> > > > +             g_object_ref_sink(stream0_);\n> > > >\n> > > >               if (createPipeline() != TestPass)\n> > > >                       return TestFail;\n> > > > @@ -57,8 +52,8 @@ protected:\n> > > >       int run() override\n> > > >       {\n> > > >               /* Build the pipeline */\n> > > > -             gst_bin_add_many(GST_BIN(pipeline_),\n> libcameraSrc_, convert0_, sink0_, NULL);\n> > > > -             if (gst_element_link_many(libcameraSrc_,\n> convert0_, sink0_, NULL) != TRUE) {\n> > > > +             gst_bin_add_many(GST_BIN(pipeline_),\n> libcameraSrc_, stream0_, NULL);\n> > > > +             if (gst_element_link(libcameraSrc_, stream0_) !=\n> TRUE) {\n> > > >                       g_printerr(\"Elements could not be\n> linked.\\n\");\n> > > >                       return TestFail;\n> > > >               }\n> > > > @@ -72,9 +67,13 @@ protected:\n> > > >               return TestPass;\n> > > >       }\n> > > >\n> > > > +     void cleanup() override\n> > > > +     {\n> > > > +             g_clear_object(&stream0_);\n> > > > +     }\n> > > > +\n> > > >  private:\n> > > > -     GstElement *convert0_;\n> > > > -     GstElement *sink0_;\n> > > > +     GstElement *stream0_;\n> > > >  };\n> > > >\n> > > >  TEST_REGISTER(GstreamerSingleStreamTest)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DFAB5BDC71\n\tfor ;\n\tWed, 22 Sep 2021 09:13:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3DBDD6918C;\n\tWed, 22 Sep 2021 11:13:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13B97687DD\n\tfor ;\n\tWed, 22 Sep 2021 11:13:54 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:4e1a:9328:3582:ccf0])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A88FDF1;\n\tWed, 22 Sep 2021 11:13:53 +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=\"EGN49qlH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632302033;\n\tbh=aSgSx+1+bpTTRswM5mka3RUCXFjF/klhOHBnU8C/vmg=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=EGN49qlHuN0TehhBTcnfSVMOnGiRJ7NkaQzrulvCkJ9KmZ2WC1a0GjuRoJ/7G69j0\n\tsSXzq/015U5RtQqbvqgZfCt42B3ALF3rwpKm9z1H87uDRm1gqTO2AQHNGIcftu1Cc8\n\ta+BxyTB8ZUPMALvFc7PPIYc8R6rkg00eTOz3jYbk=","To":"Vedant Paranjape ,\n\tLaurent Pinchart ","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t\n\t\n\t\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<5af66fa7-7f91-b013-e0f2-6bdab1a43459@ideasonboard.com>","Date":"Wed, 22 Sep 2021 11:13:51 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19785,"web_url":"https://patchwork.libcamera.org/comment/19785/","msgid":"","date":"2021-09-22T09:19:15","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Jean-Michel,\n\nOn Wed, 22 Sep, 2021, 14:43 Jean-Michel Hautbois, <\njeanmichel.hautbois@ideasonboard.com> wrote:\n\n> Hi Vedant,\n>\n> On 22/09/2021 11:10, Vedant Paranjape wrote:\n> > Hi Laurent,\n> > autovideosink needs a queue to function correctly. So it was a carry\n> > over from the command description used in terminal.\n> >\n> > fakesink has no such requirement, thus it was not added earlier.\n> >\n>\n> This should be written in the commit message to clarify it ;-).\n>\n\nI think there's some confusion. It was never in their before, I have\nmaintained the original behaviour so why should it be mentioned?\n\nAdding queue was unintentional and a mistake and not done deliberately.\n\n> On Wed, 22 Sep, 2021, 14:11 Laurent Pinchart,\n> > > > wrote:\n> >\n> > Hi Vedant,\n> >\n> > On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote:\n> > > On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote:\n> > > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > > > > Simplify memory handling and complexity of the test by using\n> > > > > gst_parse_bin_from_description_full [1]. It also fixed memory\n> > leak reported\n> > > > > by Umang Jain, described in his fix [2].\n> > > > >\n> > > > > [1]\n> >\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > <\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> >\n> > > > > [2] https://patchwork.libcamera.org/patch/13845/\n> > \n> > > > >\n> > > > > Signed-off-by: Vedant Paranjape\n> > > >\n> > > > > ---\n> > > > > .../gstreamer_single_stream_test.cpp | 29\n> > +++++++++----------\n> > > > > 1 file changed, 14 insertions(+), 15 deletions(-)\n> > > > >\n> > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > > index 7292f3280617..a20d79109885 100644\n> > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > > @@ -33,20 +33,15 @@ protected:\n> > > > > if (status_ != TestPass)\n> > > > > return status_;\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>\n> Except that you are changing from fakesink to... fakesink, so, maybe\n> should have it been introduced before ? Nevertheless just mention it in\n> your v2 :-).\n>\n\nI sent a v2 already, and it was a dumb mistake arising from blindly copy\npasting stuff and not a feature :(\n\nThanks !\n> PS: BTW, could you reply with text only mails and not HTML ? :-)\n>\n\nHmm, I just use Gmail.\n\n> > > > - g_object_ref_sink(convert0);\n> > > > > - g_object_ref_sink(sink0);\n> > > > > -\n> > > > > - if (!convert0 || !sink0) {\n> > > > > - g_printerr(\"Not all elements could be\n> > created. %p.%p\\n\",\n> > > > > - convert0, sink0);\n> > > > > + const gchar* streamDescription = \"queue !\n> > videoconvert ! fakesink\";\n> > > >\n> > > > There's a queue introduced here that isn't mentioned in the\n> commit\n> > > > message. Why is it needed ?\n> > >\n> > > WARNING: from element\n> > >\n> >\n> /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage:\n> > > Pipeline construction is invalid, please add queues.\n> > >\n> > > Because Gstreamer says so, it won't be needed with fakesink I\n> > think, I'll\n> > > fix it :)\n> >\n> > I'm not disputing that, but it wasn't the point. Before this patch,\n> we\n> > don't have a queue element inserted in the pipeline as far as I can\n> > tell. It's added by this patch, for a reason that isn't explained in\n> the\n> > commit message. So, if the queue is needed, it should say, but the\n> > reason needs to be explained (including why the problem doesn't occur\n> > today).\n> >\n> > > > > + g_autoptr(GError) error0 = NULL;\n> > > > > + stream0_ =\n> > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> > GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> > > >\n> > > > That's a very long line. Wrapping at 80 columns wouldn't be very\n> > > > practical here, but\n> > > >\n> > > > stream0_ =\n> > > > gst_parse_bin_from_description_full(streamDescription, TRUE,\n> NULL,\n> > > >\n> > > > GST_PARSE_FLAG_FATAL_ERRORS,\n> > > >\n> > &error0);\n> > > > would already be more readable I think.\n> > > >\n> > > > >\n> > > > > + if (!stream0_) {\n> > > > > + g_printerr(\"Not all bins could be\n> > created. %p\\n\",\n> > > > stream0_);\n> > > >\n> > > > Given that stream0_ is null here, why do you print it ? Can\n> > error0 be\n> > > > printed instead somehow (not as a %p I suppose) ?\n> > >\n> > > Nice catch. I'll make the changes.\n> > >\n> > > > return TestFail;\n> > > > > }\n> > > > > -\n> > > > > - convert0_ = reinterpret_cast > *>(g_steal_pointer(&convert0));\n> > > > > - sink0_ = reinterpret_cast > *>(g_steal_pointer(&sink0));\n> > > > > + g_object_ref_sink(stream0_);\n> > > > >\n> > > > > if (createPipeline() != TestPass)\n> > > > > return TestFail;\n> > > > > @@ -57,8 +52,8 @@ protected:\n> > > > > int run() override\n> > > > > {\n> > > > > /* Build the pipeline */\n> > > > > - gst_bin_add_many(GST_BIN(pipeline_),\n> > libcameraSrc_, convert0_, sink0_, NULL);\n> > > > > - if (gst_element_link_many(libcameraSrc_,\n> > convert0_, sink0_, NULL) != TRUE) {\n> > > > > + gst_bin_add_many(GST_BIN(pipeline_),\n> > libcameraSrc_, stream0_, NULL);\n> > > > > + if (gst_element_link(libcameraSrc_, stream0_) !=\n> > TRUE) {\n> > > > > g_printerr(\"Elements could not be\n> > linked.\\n\");\n> > > > > return TestFail;\n> > > > > }\n> > > > > @@ -72,9 +67,13 @@ protected:\n> > > > > return TestPass;\n> > > > > }\n> > > > >\n> > > > > + void cleanup() override\n> > > > > + {\n> > > > > + g_clear_object(&stream0_);\n> > > > > + }\n> > > > > +\n> > > > > private:\n> > > > > - GstElement *convert0_;\n> > > > > - GstElement *sink0_;\n> > > > > + GstElement *stream0_;\n> > > > > };\n> > > > >\n> > > > > TEST_REGISTER(GstreamerSingleStreamTest)\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 1B695BF01C\n\tfor ;\n\tWed, 22 Sep 2021 09:19:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 719D36918E;\n\tWed, 22 Sep 2021 11:19:31 +0200 (CEST)","from mail-qk1-x736.google.com (mail-qk1-x736.google.com\n\t[IPv6:2607:f8b0:4864:20::736])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E2E85687DD\n\tfor ;\n\tWed, 22 Sep 2021 11:19:29 +0200 (CEST)","by mail-qk1-x736.google.com with SMTP id t4so7155524qkb.9\n\tfor ;\n\tWed, 22 Sep 2021 02:19:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"ca3nlpyN\"; 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=VdFsEjQjpMyvgyRuk4zyLmL5qaxq4ph5DhjudBvsBfE=;\n\tb=ca3nlpyNFaE6pZBYIe35zyrpisuyMThVhq5oRDUJ6cy8wmTIveCRGuxCgAkTMK7Czl\n\t43JaXW+emAuMn9FFONvd+n+qbgyqWFGqK6VSHlznn/YyK0A6kcHr9DYiTDUAjR5skeUQ\n\tPVvH9C+ykqhYw2yFXjEr03hv84Az0eBWnsMn+N9yKLo4fa6gmZnPFD7Vgr9n96VftpWR\n\t/SJ+MX+S/dmvaH2Dp2U7wOPf7LVeGyieNRg56/BAJBWSWAdYVwa1lgeZ0RxnjV+8W+YZ\n\tIpz/9jz8CaYZ80dhV7R8hyxGy6cvvMRh4K3c+OGXn2lWRMHT2ao3U8wIxcKSNyH+y8MC\n\t8w4g==","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=VdFsEjQjpMyvgyRuk4zyLmL5qaxq4ph5DhjudBvsBfE=;\n\tb=Xkxr2YC7AmzrrvKe91OOJsjlG+XsT8psNM+6SylrPa0US6//6g1Rqici4299M5NInh\n\tnD+8TK9kMKPJvN3DWkqpXVasnL1Sy7cQth9RUMVPqOXXxX2SDxz8L6cHpEzyFiuzkV1c\n\t6gq9iYw3wBCf4+fa4d6JdhBJ63w28npwZR+ZDiZ7FJ2m6UaTrXgn9QoK3S+O+jwgS+Ro\n\tmXIoJKjkeH22bMIrrJHL8KrBzbsT6MInukNcLNPqoB7ccSv6xgQcDQz+jSSBNZrOYXyL\n\t8rKhsPhYscO0jxiAZcsh8JvGBEhkUUYD7pIjiFxHxlEgskqZuowsKgH+mCHvIiHG1aKG\n\ttTPg==","X-Gm-Message-State":"AOAM533jiN1h+CMfqa1Pt5SDLZwGrf7UEqEmTEdsFBuxcB2JiawAYn2G\n\tP6haI4hKkZJPEQ7W5Y5G7bXFDzYH6MjSepW64H24T5cE37U=","X-Google-Smtp-Source":"ABdhPJzRWKy5E6lc9QQuSV+dXgw2rwdD6N1n6F6AqktJfeVs1fn0B2WVAdz+ZGc2Ff5Rib+QxhPF1Bx6Cv/PvrWEWwk=","X-Received":"by 2002:a25:c504:: with SMTP id\n\tv4mr45489666ybe.308.1632302368428; \n\tWed, 22 Sep 2021 02:19:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\n\t\n\t\n\t\n\t\n\t<5af66fa7-7f91-b013-e0f2-6bdab1a43459@ideasonboard.com>","In-Reply-To":"<5af66fa7-7f91-b013-e0f2-6bdab1a43459@ideasonboard.com>","From":"Vedant Paranjape ","Date":"Wed, 22 Sep 2021 14:49:15 +0530","Message-ID":"","To":"Jean-Michel Hautbois ","Content-Type":"multipart/alternative; boundary=\"000000000000d6174305cc920242\"","Subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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":19793,"web_url":"https://patchwork.libcamera.org/comment/19793/","msgid":"<1a2272fe8ea0103dc85b69fb634a6bac260b4e1a.camel@ndufresne.ca>","date":"2021-09-22T14:31:59","subject":"Re: [libcamera-devel] [PATCH v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 22 septembre 2021 à 11:41 +0300, Laurent Pinchart a écrit :\n> Hi Vedant,\n> \n> On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote:\n> > On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote:\n> > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:\n> > > > Simplify memory handling and complexity of the test by using\n> > > > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported\n> > > > by Umang Jain, described in his fix [2].\n> > > > \n> > > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full\n> > > > [2] https://patchwork.libcamera.org/patch/13845/\n> > > > \n> > > > Signed-off-by: Vedant Paranjape \n> > > > ---\n> > > > .../gstreamer_single_stream_test.cpp | 29 +++++++++----------\n> > > > 1 file changed, 14 insertions(+), 15 deletions(-)\n> > > > \n> > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > index 7292f3280617..a20d79109885 100644\n> > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp\n> > > > @@ -33,20 +33,15 @@ protected:\n> > > > if (status_ != TestPass)\n> > > > return status_;\n> > > > \n> > > > - g_autoptr(GstElement) convert0 = gst_element_factory_make(\"videoconvert\", \"convert0\");\n> > > > - g_autoptr(GstElement) sink0 = gst_element_factory_make(\"fakesink\", \"sink0\");\n> > > > - g_object_ref_sink(convert0);\n> > > > - g_object_ref_sink(sink0);\n> > > > -\n> > > > - if (!convert0 || !sink0) {\n> > > > - g_printerr(\"Not all elements could be created. %p.%p\\n\",\n> > > > - convert0, sink0);\n> > > > + const gchar* streamDescription = \"queue ! videoconvert ! fakesink\";\n> > > \n> > > There's a queue introduced here that isn't mentioned in the commit\n> > > message. Why is it needed ?\n> > \n> > WARNING: from element\n> > /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage:\n> > Pipeline construction is invalid, please add queues.\n> > \n> > Because Gstreamer says so, it won't be needed with fakesink I think, I'll\n> > fix it :)\n> \n> I'm not disputing that, but it wasn't the point. Before this patch, we\n> don't have a queue element inserted in the pipeline as far as I can\n> tell. It's added by this patch, for a reason that isn't explained in the\n> commit message. So, if the queue is needed, it should say, but the\n> reason needs to be explained (including why the problem doesn't occur\n> today).\n\nThe appropriate reason GStreamer core request queues is that the reported min-\nlatency (sum of all reported latency in the chain) have gone larger then the\nreported max-latency (the about of data the pipeline can hold because buffers\nare dropped). This is a bit of a side effect of the introduction of the\nprocessing-deadline in GstVideoSink base class, which now declare some latency\nto account for the processing time, but does not have any queue to hold on the\nbuffers if they arrive without that introduced processing delay.\n\nIn libcamerasrc, we currently declare min-latency == max-latency, so basically\nwe are saying that we can only hold as much latency as what we contribute. \nNormally the max-latency would be correlated to the number of allocated buffers\nassuming the capture thread can't starve. This way, the capture can keep going\nwithout loosing frames even if the pipeline is pushing back due to some added\nlatency.\n\nPerhaps we want to fix that now ? We need an approximation of the frame duration\nthough.\n\n - min-latency: This remains the observed latency for the first frame\n - max-latency: This is the min-latency + (num_buffers - 1) * estimated_duration\n\n> \n> > > > + g_autoptr(GError) error0 = NULL;\n> > > > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);\n> > > \n> > > That's a very long line. Wrapping at 80 columns wouldn't be very\n> > > practical here, but\n> > > \n> > > stream0_ =\n> > > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,\n> > > \n> > > GST_PARSE_FLAG_FATAL_ERRORS,\n> > > &error0);\n> > > would already be more readable I think.\n> > > \n> > > > \n> > > > + if (!stream0_) {\n> > > > + g_printerr(\"Not all bins could be created. %p\\n\",\n> > > stream0_);\n> > > \n> > > Given that stream0_ is null here, why do you print it ? Can error0 be\n> > > printed instead somehow (not as a %p I suppose) ?\n> > \n> > Nice catch. I'll make the changes.\n> > \n> > > return TestFail;\n> > > > }\n> > > > -\n> > > > - convert0_ = reinterpret_cast(g_steal_pointer(&convert0));\n> > > > - sink0_ = reinterpret_cast(g_steal_pointer(&sink0));\n> > > > + g_object_ref_sink(stream0_);\n> > > > \n> > > > if (createPipeline() != TestPass)\n> > > > return TestFail;\n> > > > @@ -57,8 +52,8 @@ protected:\n> > > > int run() override\n> > > > {\n> > > > /* Build the pipeline */\n> > > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL);\n> > > > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) {\n> > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);\n> > > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {\n> > > > g_printerr(\"Elements could not be linked.\\n\");\n> > > > return TestFail;\n> > > > }\n> > > > @@ -72,9 +67,13 @@ protected:\n> > > > return TestPass;\n> > > > }\n> > > > \n> > > > + void cleanup() override\n> > > > + {\n> > > > + g_clear_object(&stream0_);\n> > > > + }\n> > > > +\n> > > > private:\n> > > > - GstElement *convert0_;\n> > > > - GstElement *sink0_;\n> > > > + GstElement *stream0_;\n> > > > };\n> > > > \n> > > > TEST_REGISTER(GstreamerSingleStreamTest)\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 0B0B9BF01C\n\tfor ;\n\tWed, 22 Sep 2021 14:32:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B19D6918C;\n\tWed, 22 Sep 2021 16:32:03 +0200 (CEST)","from mail-qk1-x731.google.com (mail-qk1-x731.google.com\n\t[IPv6:2607:f8b0:4864:20::731])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DE516917F\n\tfor ;\n\tWed, 22 Sep 2021 16:32:02 +0200 (CEST)","by mail-qk1-x731.google.com with SMTP id a10so9984860qka.12\n\tfor ;\n\tWed, 22 Sep 2021 07:32:02 -0700 (PDT)","from nicolas-tpx395.localdomain (173-246-12-168.qc.cable.ebox.net.\n\t[173.246.12.168]) by smtp.gmail.com with ESMTPSA id\n\ty24sm1963604qkj.102.2021.09.22.07.31.59\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 22 Sep 2021 07:32:00 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20210112.gappssmtp.com\n\theader.i=@ndufresne-ca.20210112.gappssmtp.com\n\theader.b=\"c+3+Vcgl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20210112.gappssmtp.com; s=20210112;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=nlnC2mJX59bYNkPTqw6HMK7UVHFFHoiI05RobXM3yLo=;\n\tb=c+3+VcglotyFp85vWu9EDOzL0kG6vOvEtAfNHyGm0nTQmz/OnfPP7x/JwDqpc4IVsv\n\tF9UgQr8tliqYvyxot4e80iYGOGFgcdGSaE/JY2W3j607W/b/akTDLWt4X27nyr/ytEkg\n\t3ZXkjtGmzY2gGoZXstCDMyk2vfSVuqqX1M2MiMb5q1v2+FjCRJuX0Ncl/OW1fTR82lT3\n\tYaXjWNfL1yrKzpIf5BvcUv+rJ8N+38DXs5KAkfjhEsxNgTQC/GNKAPNtJVCxztSE+NLj\n\tCTuZkdSOtKjK4U6qCub/oDIjrJKuvHW2fGj9j1fd8CYAEUuAZ2v9OzTQoKgpK/NvQe3H\n\tt96w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=nlnC2mJX59bYNkPTqw6HMK7UVHFFHoiI05RobXM3yLo=;\n\tb=Sqkmjd/3N7oc0+pENHQfqpYOpLlzqlV+bxOliLPR1U8lQuGrmy3gSmiIvh7XJQ/GIo\n\tDsl8uSR/oLNLsabegWgMAaAmCbuMb7MlJ9DsDgBHE0J2oUclI+/wLvYzcT/IW6qlLMe1\n\tb+vT5hijSaB7fgRQAP6BIN3oElYEWjaXNCZeqpP7sSF8X82SJenGCrRcDJbtpVa6R4hV\n\tgbuCLIOy0OFJsutCxIh/OyFgVxRcmwd8qtQ8CsCaVvQU4w8PsRa7D+WvV4v121YSerjJ\n\tYJVhJ/WRYUwMoEDg/5qhXjLer//ktfR+PJbWmuXfsOYTTVDqk6hJT5OsW8qTxO7/8xRk\n\trg0w==","X-Gm-Message-State":"AOAM5318WZiHWs9XpVb4RStLCOtNCHb9m5YcCIf2kZi6TETswMap0rfR\n\tVU98vqe1u5QGP2CpPI7qXOvUxQ==","X-Google-Smtp-Source":"ABdhPJzO7gX5XLSf7D754NzkWDlzI0VKIMjGkdwswMde+quvzcnmosNudEF7y5+JoP/ObKzBm9N8iw==","X-Received":"by 2002:a37:618c:: with SMTP id v134mr70165qkb.231.1632321120732;\n\tWed, 22 Sep 2021 07:32:00 -0700 (PDT)","Message-ID":"<1a2272fe8ea0103dc85b69fb634a6bac260b4e1a.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Laurent Pinchart , Vedant Paranjape\n\t","Date":"Wed, 22 Sep 2021 10:31:59 -0400","In-Reply-To":"","References":"<20210915150907.736222-1-vedantparanjape160201@gmail.com>\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 v1] test: gstreamer: Simplify single\n\tstream test using functions from GstUtils","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\" "}}]