| Message ID | 20210915150907.736222-1-vedantparanjape160201@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Vedant, Thank you for the patch. On 9/15/21 8:39 PM, Vedant Paranjape wrote: > Simplify memory handling and complexity of the test by using > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported > by Umang Jain, described in his fix [2]. > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > [2] https://patchwork.libcamera.org/patch/13845/ To be honest, this patch in general introduces too many changes at once. The trick to get good and fast reviews is generally to break it down to focus one task per patch in most cases. This granularity helps the reviewer to be confident looking at the changes in front of them and give their R-b tags on those specific patches. It might happen that for e.g. I am not well-versed with the API you introduced, but rather I can see the other fixes good to go - but overall I will still be holding off my R-b because there is something I don't understand fully. This is just an example but you get the idea. If you could have broken this down to let's say 3 patches (piggybacking on my 2 patches earlier and introduce a 3rd patch by yourself about the new API) - it helps to establish a flow. The two patches had one R-b tags earlier so you are already closer to get them merged (need 2 R-b tags for merge-eligibility per patch) and the 3rd can be discussed meanwhile. It helps to move and unblock patches / fixes as part of development process. > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > --- > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > index 7292f3280617..a20d79109885 100644 > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > @@ -33,20 +33,15 @@ protected: > if (status_ != TestPass) > return status_; > > - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); > - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); > - g_object_ref_sink(convert0); > - g_object_ref_sink(sink0); > - > - if (!convert0 || !sink0) { > - g_printerr("Not all elements could be created. %p.%p\n", > - convert0, sink0); > + const gchar* streamDescription = "queue ! videoconvert ! fakesink"; > + g_autoptr(GError) error0 = NULL; > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > + if (!stream0_) { > + g_printerr("Not all bins could be created. %p\n", stream0_); > return TestFail; > } > - > - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); > - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); > + g_object_ref_sink(stream0_); > > if (createPipeline() != TestPass) > return TestFail; > @@ -57,8 +52,8 @@ protected: > int run() override > { > /* Build the pipeline */ > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL); > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) { > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > g_printerr("Elements could not be linked.\n"); > return TestFail; > } > @@ -72,9 +67,13 @@ protected: > return TestPass; > } > > + void cleanup() override > + { > + g_clear_object(&stream0_); > + } > + > private: > - GstElement *convert0_; > - GstElement *sink0_; > + GstElement *stream0_; > }; > > TEST_REGISTER(GstreamerSingleStreamTest)
Hi Umang, Thanks for the reply. I'll keep this in mind, next time I send patches. But can you test it and give a tested by tag? Also, I can help you understand parts that you are not familiar with. Regards, *Vedant Paranjape* On Sat, 18 Sep, 2021, 23:54 Umang Jain, <umang.jain@ideasonboard.com> wrote: > Hi Vedant, > > Thank you for the patch. > > On 9/15/21 8:39 PM, Vedant Paranjape wrote: > > Simplify memory handling and complexity of the test by using > > gst_parse_bin_from_description_full [1]. It also fixed memory leak > reported > > by Umang Jain, described in his fix [2]. > > > > [1] > https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > [2] https://patchwork.libcamera.org/patch/13845/ > > > To be honest, this patch in general introduces too many changes at once. > The trick to get good and fast reviews is generally to break it down to > focus one task per patch in most cases. This granularity helps the > reviewer to be confident looking at the changes in front of them and > give their R-b tags on those specific patches. It might happen that for > e.g. I am not well-versed with the API you introduced, but rather I can > see the other fixes good to go - but overall I will still be holding off > my R-b because there is something I don't understand fully. This is just > an example but you get the idea. > > If you could have broken this down to let's say 3 patches (piggybacking > on my 2 patches earlier and introduce a 3rd patch by yourself about the > new API) - it helps to establish a flow. The two patches had one R-b > tags earlier so you are already closer to get them merged (need 2 R-b > tags for merge-eligibility per patch) and the 3rd can be discussed > meanwhile. It helps to move and unblock patches / fixes as part of > development process. > > > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > --- > > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > b/test/gstreamer/gstreamer_single_stream_test.cpp > > index 7292f3280617..a20d79109885 100644 > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > @@ -33,20 +33,15 @@ protected: > > if (status_ != TestPass) > > return status_; > > > > - g_autoptr(GstElement) convert0 = > gst_element_factory_make("videoconvert", "convert0"); > > - g_autoptr(GstElement) sink0 = > gst_element_factory_make("fakesink", "sink0"); > > - g_object_ref_sink(convert0); > > - g_object_ref_sink(sink0); > > - > > - if (!convert0 || !sink0) { > > - g_printerr("Not all elements could be created. > %p.%p\n", > > - convert0, sink0); > > + const gchar* streamDescription = "queue ! videoconvert ! > fakesink"; > > + g_autoptr(GError) error0 = NULL; > > + stream0_ = > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > > > + if (!stream0_) { > > + g_printerr("Not all bins could be created. %p\n", > stream0_); > > return TestFail; > > } > > - > > - convert0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&convert0)); > > - sink0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&sink0)); > > + g_object_ref_sink(stream0_); > > > > if (createPipeline() != TestPass) > > return TestFail; > > @@ -57,8 +52,8 @@ protected: > > int run() override > > { > > /* Build the pipeline */ > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > convert0_, sink0_, NULL); > > - if (gst_element_link_many(libcameraSrc_, convert0_, > sink0_, NULL) != TRUE) { > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > stream0_, NULL); > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > g_printerr("Elements could not be linked.\n"); > > return TestFail; > > } > > @@ -72,9 +67,13 @@ protected: > > return TestPass; > > } > > > > + void cleanup() override > > + { > > + g_clear_object(&stream0_); > > + } > > + > > private: > > - GstElement *convert0_; > > - GstElement *sink0_; > > + GstElement *stream0_; > > }; > > > > TEST_REGISTER(GstreamerSingleStreamTest) >
Le mercredi 15 septembre 2021 à 20:39 +0530, Vedant Paranjape a écrit : > Simplify memory handling and complexity of the test by using > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported > by Umang Jain, described in his fix [2]. > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > [2] https://patchwork.libcamera.org/patch/13845/ > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> p.s. I personally found this patch relatively small and fast to review. > --- > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > index 7292f3280617..a20d79109885 100644 > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > @@ -33,20 +33,15 @@ protected: > if (status_ != TestPass) > return status_; > > - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); > - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); > - g_object_ref_sink(convert0); > - g_object_ref_sink(sink0); > - > - if (!convert0 || !sink0) { > - g_printerr("Not all elements could be created. %p.%p\n", > - convert0, sink0); > + const gchar* streamDescription = "queue ! videoconvert ! fakesink"; > + g_autoptr(GError) error0 = NULL; > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > + if (!stream0_) { > + g_printerr("Not all bins could be created. %p\n", stream0_); > return TestFail; > } > - > - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); > - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); > + g_object_ref_sink(stream0_); > > if (createPipeline() != TestPass) > return TestFail; > @@ -57,8 +52,8 @@ protected: > int run() override > { > /* Build the pipeline */ > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL); > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) { > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > g_printerr("Elements could not be linked.\n"); > return TestFail; > } > @@ -72,9 +67,13 @@ protected: > return TestPass; > } > > + void cleanup() override > + { > + g_clear_object(&stream0_); > + } > + > private: > - GstElement *convert0_; > - GstElement *sink0_; > + GstElement *stream0_; > }; > > TEST_REGISTER(GstreamerSingleStreamTest)
Hi Vedant, On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > Simplify memory handling and complexity of the test by using > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported > by Umang Jain, described in his fix [2]. "For further information, see this patch that wasn't merged" True, the patch isn't big or difficult to review, but you did squash two patches that were doing distinct things (though they did have the same end goal). Please split the patches and expand the commit messages (copying what Umang had is fine; he said so in the original patch series). Paul > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > [2] https://patchwork.libcamera.org/patch/13845/ > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > --- > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > index 7292f3280617..a20d79109885 100644 > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > @@ -33,20 +33,15 @@ protected: > if (status_ != TestPass) > return status_; > > - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); > - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); > - g_object_ref_sink(convert0); > - g_object_ref_sink(sink0); > - > - if (!convert0 || !sink0) { > - g_printerr("Not all elements could be created. %p.%p\n", > - convert0, sink0); > + const gchar* streamDescription = "queue ! videoconvert ! fakesink"; > + g_autoptr(GError) error0 = NULL; > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > + if (!stream0_) { > + g_printerr("Not all bins could be created. %p\n", stream0_); > return TestFail; > } > - > - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); > - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); > + g_object_ref_sink(stream0_); > > if (createPipeline() != TestPass) > return TestFail; > @@ -57,8 +52,8 @@ protected: > int run() override > { > /* Build the pipeline */ > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL); > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) { > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > g_printerr("Elements could not be linked.\n"); > return TestFail; > } > @@ -72,9 +67,13 @@ protected: > return TestPass; > } > > + void cleanup() override > + { > + g_clear_object(&stream0_); > + } > + > private: > - GstElement *convert0_; > - GstElement *sink0_; > + GstElement *stream0_; > }; > > TEST_REGISTER(GstreamerSingleStreamTest) > -- > 2.25.1 >
Hi Paul, I don't see the rationale here. I am not fixing the things what Umang mentioned. I just happen to add it in the commit message. @Nicolas Dufresne <nicolas@ndufresne.ca> and @Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> had told me long back to shift to using parse_bin, so this patch addressed the same. I'll remove the mention of Umang's patch, hope it solves the issue. Regards, Vedant Paranjape On Tue, 21 Sep, 2021, 15:25 , <paul.elder@ideasonboard.com> wrote: > Hi Vedant, > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > Simplify memory handling and complexity of the test by using > > gst_parse_bin_from_description_full [1]. It also fixed memory leak > reported > > by Umang Jain, described in his fix [2]. > > "For further information, see this patch that wasn't merged" > > True, the patch isn't big or difficult to review, but you did squash two > patches that were doing distinct things (though they did have the same > end goal). > > Please split the patches and expand the commit messages (copying what > Umang had is fine; he said so in the original patch series). > > > Paul > > > > > [1] > https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > [2] https://patchwork.libcamera.org/patch/13845/ > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > --- > > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > b/test/gstreamer/gstreamer_single_stream_test.cpp > > index 7292f3280617..a20d79109885 100644 > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > @@ -33,20 +33,15 @@ protected: > > if (status_ != TestPass) > > return status_; > > > > - g_autoptr(GstElement) convert0 = > gst_element_factory_make("videoconvert", "convert0"); > > - g_autoptr(GstElement) sink0 = > gst_element_factory_make("fakesink", "sink0"); > > - g_object_ref_sink(convert0); > > - g_object_ref_sink(sink0); > > - > > - if (!convert0 || !sink0) { > > - g_printerr("Not all elements could be created. > %p.%p\n", > > - convert0, sink0); > > + const gchar* streamDescription = "queue ! videoconvert ! > fakesink"; > > + g_autoptr(GError) error0 = NULL; > > + stream0_ = > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > > > + if (!stream0_) { > > + g_printerr("Not all bins could be created. %p\n", > stream0_); > > return TestFail; > > } > > - > > - convert0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&convert0)); > > - sink0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&sink0)); > > + g_object_ref_sink(stream0_); > > > > if (createPipeline() != TestPass) > > return TestFail; > > @@ -57,8 +52,8 @@ protected: > > int run() override > > { > > /* Build the pipeline */ > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > convert0_, sink0_, NULL); > > - if (gst_element_link_many(libcameraSrc_, convert0_, > sink0_, NULL) != TRUE) { > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > stream0_, NULL); > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > g_printerr("Elements could not be linked.\n"); > > return TestFail; > > } > > @@ -72,9 +67,13 @@ protected: > > return TestPass; > > } > > > > + void cleanup() override > > + { > > + g_clear_object(&stream0_); > > + } > > + > > private: > > - GstElement *convert0_; > > - GstElement *sink0_; > > + GstElement *stream0_; > > }; > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > -- > > 2.25.1 > > >
Hi, Umang has already sent a fix patch series, which got R-b from Nicolas. Apply that into master and then merge mine. I think this should solve the problem, I'll just rebase my patch then. On Tue, 21 Sep, 2021, 15:25 , <paul.elder@ideasonboard.com> wrote: > Hi Vedant, > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > Simplify memory handling and complexity of the test by using > > gst_parse_bin_from_description_full [1]. It also fixed memory leak > reported > > by Umang Jain, described in his fix [2]. > > "For further information, see this patch that wasn't merged" > > True, the patch isn't big or difficult to review, but you did squash two > patches that were doing distinct things (though they did have the same > end goal). > > Please split the patches and expand the commit messages (copying what > Umang had is fine; he said so in the original patch series). > > > Paul > > > > > [1] > https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > [2] https://patchwork.libcamera.org/patch/13845/ > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > --- > > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > b/test/gstreamer/gstreamer_single_stream_test.cpp > > index 7292f3280617..a20d79109885 100644 > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > @@ -33,20 +33,15 @@ protected: > > if (status_ != TestPass) > > return status_; > > > > - g_autoptr(GstElement) convert0 = > gst_element_factory_make("videoconvert", "convert0"); > > - g_autoptr(GstElement) sink0 = > gst_element_factory_make("fakesink", "sink0"); > > - g_object_ref_sink(convert0); > > - g_object_ref_sink(sink0); > > - > > - if (!convert0 || !sink0) { > > - g_printerr("Not all elements could be created. > %p.%p\n", > > - convert0, sink0); > > + const gchar* streamDescription = "queue ! videoconvert ! > fakesink"; > > + g_autoptr(GError) error0 = NULL; > > + stream0_ = > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > > > + if (!stream0_) { > > + g_printerr("Not all bins could be created. %p\n", > stream0_); > > return TestFail; > > } > > - > > - convert0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&convert0)); > > - sink0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&sink0)); > > + g_object_ref_sink(stream0_); > > > > if (createPipeline() != TestPass) > > return TestFail; > > @@ -57,8 +52,8 @@ protected: > > int run() override > > { > > /* Build the pipeline */ > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > convert0_, sink0_, NULL); > > - if (gst_element_link_many(libcameraSrc_, convert0_, > sink0_, NULL) != TRUE) { > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > stream0_, NULL); > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > g_printerr("Elements could not be linked.\n"); > > return TestFail; > > } > > @@ -72,9 +67,13 @@ protected: > > return TestPass; > > } > > > > + void cleanup() override > > + { > > + g_clear_object(&stream0_); > > + } > > + > > private: > > - GstElement *convert0_; > > - GstElement *sink0_; > > + GstElement *stream0_; > > }; > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > -- > > 2.25.1 > > >
Le mardi 21 septembre 2021 à 18:55 +0900, paul.elder@ideasonboard.com a écrit : > Hi Vedant, > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > Simplify memory handling and complexity of the test by using > > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported > > by Umang Jain, described in his fix [2]. > > "For further information, see this patch that wasn't merged" > > True, the patch isn't big or difficult to review, but you did squash two > patches that were doing distinct things (though they did have the same > end goal). > > Please split the patches and expand the commit messages (copying what > Umang had is fine; he said so in the original patch series). My personal opinion on the manner is that we are right now being over picky for some splitting that in my opinion is not a libcamera bug, but a test bug. I respect your request, but I'd like to underline that we need to make a tradeoff at some point, specially for code that isn't part of libcamera. This is rather discouraging on top of all the effort that is needed to "edit" email based patches. > > > Paul > > > > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > [2] https://patchwork.libcamera.org/patch/13845/ > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > --- > > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > > index 7292f3280617..a20d79109885 100644 > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > @@ -33,20 +33,15 @@ protected: > > if (status_ != TestPass) > > return status_; > > > > - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); > > - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); > > - g_object_ref_sink(convert0); > > - g_object_ref_sink(sink0); > > - > > - if (!convert0 || !sink0) { > > - g_printerr("Not all elements could be created. %p.%p\n", > > - convert0, sink0); > > + const gchar* streamDescription = "queue ! videoconvert ! fakesink"; > > + g_autoptr(GError) error0 = NULL; > > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > > > + if (!stream0_) { > > + g_printerr("Not all bins could be created. %p\n", stream0_); > > return TestFail; > > } > > - > > - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); > > - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); > > + g_object_ref_sink(stream0_); > > > > if (createPipeline() != TestPass) > > return TestFail; > > @@ -57,8 +52,8 @@ protected: > > int run() override > > { > > /* Build the pipeline */ > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL); > > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) { > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > g_printerr("Elements could not be linked.\n"); > > return TestFail; > > } > > @@ -72,9 +67,13 @@ protected: > > return TestPass; > > } > > > > + void cleanup() override > > + { > > + g_clear_object(&stream0_); > > + } > > + > > private: > > - GstElement *convert0_; > > - GstElement *sink0_; > > + GstElement *stream0_; > > }; > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > -- > > 2.25.1 > >
Hi Vedant, Thank you for the patch. On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > Simplify memory handling and complexity of the test by using > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported > by Umang Jain, described in his fix [2]. > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > [2] https://patchwork.libcamera.org/patch/13845/ > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > --- > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > index 7292f3280617..a20d79109885 100644 > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > @@ -33,20 +33,15 @@ protected: > if (status_ != TestPass) > return status_; > > - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); > - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); > - g_object_ref_sink(convert0); > - g_object_ref_sink(sink0); > - > - if (!convert0 || !sink0) { > - g_printerr("Not all elements could be created. %p.%p\n", > - convert0, sink0); > + const gchar* streamDescription = "queue ! videoconvert ! fakesink"; There's a queue introduced here that isn't mentioned in the commit message. Why is it needed ? > + g_autoptr(GError) error0 = NULL; > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0); That's a very long line. Wrapping at 80 columns wouldn't be very practical here, but stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0); would already be more readable I think. > > + if (!stream0_) { > + g_printerr("Not all bins could be created. %p\n", stream0_); Given that stream0_ is null here, why do you print it ? Can error0 be printed instead somehow (not as a %p I suppose) ? > return TestFail; > } > - > - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); > - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); > + g_object_ref_sink(stream0_); > > if (createPipeline() != TestPass) > return TestFail; > @@ -57,8 +52,8 @@ protected: > int run() override > { > /* Build the pipeline */ > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL); > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) { > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > g_printerr("Elements could not be linked.\n"); > return TestFail; > } > @@ -72,9 +67,13 @@ protected: > return TestPass; > } > > + void cleanup() override > + { > + g_clear_object(&stream0_); > + } > + > private: > - GstElement *convert0_; > - GstElement *sink0_; > + GstElement *stream0_; > }; > > TEST_REGISTER(GstreamerSingleStreamTest)
Hi Laurent, These patches are stale. I have sent a new patch series replacing this please take a look at it. Even though your comments do apply there too. Regards, Vedant Paranjape On Tue, 21 Sep, 2021, 22:47 Laurent Pinchart, < laurent.pinchart@ideasonboard.com> wrote: > Hi Vedant, > > Thank you for the patch. > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > Simplify memory handling and complexity of the test by using > > gst_parse_bin_from_description_full [1]. It also fixed memory leak > reported > > by Umang Jain, described in his fix [2]. > > > > [1] > https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > [2] https://patchwork.libcamera.org/patch/13845/ > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > --- > > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > b/test/gstreamer/gstreamer_single_stream_test.cpp > > index 7292f3280617..a20d79109885 100644 > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > @@ -33,20 +33,15 @@ protected: > > if (status_ != TestPass) > > return status_; > > > > - g_autoptr(GstElement) convert0 = > gst_element_factory_make("videoconvert", "convert0"); > > - g_autoptr(GstElement) sink0 = > gst_element_factory_make("fakesink", "sink0"); > > - g_object_ref_sink(convert0); > > - g_object_ref_sink(sink0); > > - > > - if (!convert0 || !sink0) { > > - g_printerr("Not all elements could be created. > %p.%p\n", > > - convert0, sink0); > > + const gchar* streamDescription = "queue ! videoconvert ! > fakesink"; > > There's a queue introduced here that isn't mentioned in the commit > message. Why is it needed ? > > > + g_autoptr(GError) error0 = NULL; > > + stream0_ = > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > That's a very long line. Wrapping at 80 columns wouldn't be very > practical here, but > > stream0_ = > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > > GST_PARSE_FLAG_FATAL_ERRORS, > &error0); > would already be more readable I think. > > > > > + if (!stream0_) { > > + g_printerr("Not all bins could be created. %p\n", > stream0_); > > Given that stream0_ is null here, why do you print it ? Can error0 be > printed instead somehow (not as a %p I suppose) ? > > > return TestFail; > > } > > - > > - convert0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&convert0)); > > - sink0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&sink0)); > > + g_object_ref_sink(stream0_); > > > > if (createPipeline() != TestPass) > > return TestFail; > > @@ -57,8 +52,8 @@ protected: > > int run() override > > { > > /* Build the pipeline */ > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > convert0_, sink0_, NULL); > > - if (gst_element_link_many(libcameraSrc_, convert0_, > sink0_, NULL) != TRUE) { > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > stream0_, NULL); > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > g_printerr("Elements could not be linked.\n"); > > return TestFail; > > } > > @@ -72,9 +67,13 @@ protected: > > return TestPass; > > } > > > > + void cleanup() override > > + { > > + g_clear_object(&stream0_); > > + } > > + > > private: > > - GstElement *convert0_; > > - GstElement *sink0_; > > + GstElement *stream0_; > > }; > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > -- > Regards, > > Laurent Pinchart >
Hi Laurent, Thanks for the review. On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Vedant, > > Thank you for the patch. > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > Simplify memory handling and complexity of the test by using > > gst_parse_bin_from_description_full [1]. It also fixed memory leak > reported > > by Umang Jain, described in his fix [2]. > > > > [1] > https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > [2] https://patchwork.libcamera.org/patch/13845/ > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > --- > > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > b/test/gstreamer/gstreamer_single_stream_test.cpp > > index 7292f3280617..a20d79109885 100644 > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > @@ -33,20 +33,15 @@ protected: > > if (status_ != TestPass) > > return status_; > > > > - g_autoptr(GstElement) convert0 = > gst_element_factory_make("videoconvert", "convert0"); > > - g_autoptr(GstElement) sink0 = > gst_element_factory_make("fakesink", "sink0"); > > - g_object_ref_sink(convert0); > > - g_object_ref_sink(sink0); > > - > > - if (!convert0 || !sink0) { > > - g_printerr("Not all elements could be created. > %p.%p\n", > > - convert0, sink0); > > + const gchar* streamDescription = "queue ! videoconvert ! > fakesink"; > > There's a queue introduced here that isn't mentioned in the commit > message. Why is it needed ? WARNING: from element /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage: Pipeline construction is invalid, please add queues. Because Gstreamer says so, it won't be needed with fakesink I think, I'll fix it :) > > + g_autoptr(GError) error0 = NULL; > > + stream0_ = > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > That's a very long line. Wrapping at 80 columns wouldn't be very > practical here, but > > stream0_ = > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > > GST_PARSE_FLAG_FATAL_ERRORS, > &error0); > would already be more readable I think. > > > > > + if (!stream0_) { > > + g_printerr("Not all bins could be created. %p\n", > stream0_); > > Given that stream0_ is null here, why do you print it ? Can error0 be > printed instead somehow (not as a %p I suppose) ? > Nice catch. I'll make the changes. > return TestFail; > > } > > - > > - convert0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&convert0)); > > - sink0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&sink0)); > > + g_object_ref_sink(stream0_); > > > > if (createPipeline() != TestPass) > > return TestFail; > > @@ -57,8 +52,8 @@ protected: > > int run() override > > { > > /* Build the pipeline */ > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > convert0_, sink0_, NULL); > > - if (gst_element_link_many(libcameraSrc_, convert0_, > sink0_, NULL) != TRUE) { > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > stream0_, NULL); > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > g_printerr("Elements could not be linked.\n"); > > return TestFail; > > } > > @@ -72,9 +67,13 @@ protected: > > return TestPass; > > } > > > > + void cleanup() override > > + { > > + g_clear_object(&stream0_); > > + } > > + > > private: > > - GstElement *convert0_; > > - GstElement *sink0_; > > + GstElement *stream0_; > > }; > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > -- > Regards, > > Laurent Pinchart > Regards, *Vedant Paranjape*
On Tue, Sep 21, 2021 at 08:55:17AM -0400, Nicolas Dufresne wrote: > Le mardi 21 septembre 2021 à 18:55 +0900, paul.elder@ideasonboard.com a écrit : > > Hi Vedant, > > > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > > Simplify memory handling and complexity of the test by using > > > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported > > > by Umang Jain, described in his fix [2]. > > > > "For further information, see this patch that wasn't merged" > > > > True, the patch isn't big or difficult to review, but you did squash two > > patches that were doing distinct things (though they did have the same > > end goal). > > > > Please split the patches and expand the commit messages (copying what > > Umang had is fine; he said so in the original patch series). > > My personal opinion on the manner is that we are right now being over picky for Perhaps. > some splitting that in my opinion is not a libcamera bug, but a test bug. I > respect your request, but I'd like to underline that we need to make a tradeoff > at some point, specially for code that isn't part of libcamera. It's still a test in libcamera. I think that such code and history organization still needs to be held to an equally high standard. Just because it's a test doesn't mean the quality and organization can be disregarded. > This is rather discouraging I see your point. My apologies for not looking at this earlier. > on top of all the effort that is needed to "edit" email based > patches. I'm not sure what the difference is between editing email-based patches vs commits on github is... both are more-or-less just rebasing and resending. Paul > > > > > > > Paul > > > > > > > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > > [2] https://patchwork.libcamera.org/patch/13845/ > > > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > > --- > > > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > > > index 7292f3280617..a20d79109885 100644 > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > @@ -33,20 +33,15 @@ protected: > > > if (status_ != TestPass) > > > return status_; > > > > > > - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); > > > - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); > > > - g_object_ref_sink(convert0); > > > - g_object_ref_sink(sink0); > > > - > > > - if (!convert0 || !sink0) { > > > - g_printerr("Not all elements could be created. %p.%p\n", > > > - convert0, sink0); > > > + const gchar* streamDescription = "queue ! videoconvert ! fakesink"; > > > + g_autoptr(GError) error0 = NULL; > > > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > > > > > + if (!stream0_) { > > > + g_printerr("Not all bins could be created. %p\n", stream0_); > > > return TestFail; > > > } > > > - > > > - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); > > > - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); > > > + g_object_ref_sink(stream0_); > > > > > > if (createPipeline() != TestPass) > > > return TestFail; > > > @@ -57,8 +52,8 @@ protected: > > > int run() override > > > { > > > /* Build the pipeline */ > > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL); > > > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) { > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > > g_printerr("Elements could not be linked.\n"); > > > return TestFail; > > > } > > > @@ -72,9 +67,13 @@ protected: > > > return TestPass; > > > } > > > > > > + void cleanup() override > > > + { > > > + g_clear_object(&stream0_); > > > + } > > > + > > > private: > > > - GstElement *convert0_; > > > - GstElement *sink0_; > > > + GstElement *stream0_; > > > }; > > > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > > -- > > > 2.25.1 > > > > >
Hi Vedant, On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote: > On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote: > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > > Simplify memory handling and complexity of the test by using > > > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported > > > by Umang Jain, described in his fix [2]. > > > > > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > > [2] https://patchwork.libcamera.org/patch/13845/ > > > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > > --- > > > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > > > index 7292f3280617..a20d79109885 100644 > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > @@ -33,20 +33,15 @@ protected: > > > if (status_ != TestPass) > > > return status_; > > > > > > - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); > > > - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); > > > - g_object_ref_sink(convert0); > > > - g_object_ref_sink(sink0); > > > - > > > - if (!convert0 || !sink0) { > > > - g_printerr("Not all elements could be created. %p.%p\n", > > > - convert0, sink0); > > > + const gchar* streamDescription = "queue ! videoconvert ! fakesink"; > > > > There's a queue introduced here that isn't mentioned in the commit > > message. Why is it needed ? > > WARNING: from element > /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage: > Pipeline construction is invalid, please add queues. > > Because Gstreamer says so, it won't be needed with fakesink I think, I'll > fix it :) I'm not disputing that, but it wasn't the point. Before this patch, we don't have a queue element inserted in the pipeline as far as I can tell. It's added by this patch, for a reason that isn't explained in the commit message. So, if the queue is needed, it should say, but the reason needs to be explained (including why the problem doesn't occur today). > > > + g_autoptr(GError) error0 = NULL; > > > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > > > That's a very long line. Wrapping at 80 columns wouldn't be very > > practical here, but > > > > stream0_ = > > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > > > > GST_PARSE_FLAG_FATAL_ERRORS, > > &error0); > > would already be more readable I think. > > > > > > > > + if (!stream0_) { > > > + g_printerr("Not all bins could be created. %p\n", > > stream0_); > > > > Given that stream0_ is null here, why do you print it ? Can error0 be > > printed instead somehow (not as a %p I suppose) ? > > Nice catch. I'll make the changes. > > > return TestFail; > > > } > > > - > > > - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); > > > - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); > > > + g_object_ref_sink(stream0_); > > > > > > if (createPipeline() != TestPass) > > > return TestFail; > > > @@ -57,8 +52,8 @@ protected: > > > int run() override > > > { > > > /* Build the pipeline */ > > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL); > > > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) { > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > > g_printerr("Elements could not be linked.\n"); > > > return TestFail; > > > } > > > @@ -72,9 +67,13 @@ protected: > > > return TestPass; > > > } > > > > > > + void cleanup() override > > > + { > > > + g_clear_object(&stream0_); > > > + } > > > + > > > private: > > > - GstElement *convert0_; > > > - GstElement *sink0_; > > > + GstElement *stream0_; > > > }; > > > > > > TEST_REGISTER(GstreamerSingleStreamTest)
Hi Laurent, autovideosink needs a queue to function correctly. So it was a carry over from the command description used in terminal. fakesink has no such requirement, thus it was not added earlier. On Wed, 22 Sep, 2021, 14:11 Laurent Pinchart, < laurent.pinchart@ideasonboard.com> wrote: > Hi Vedant, > > On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote: > > On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote: > > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > > > Simplify memory handling and complexity of the test by using > > > > gst_parse_bin_from_description_full [1]. It also fixed memory leak > reported > > > > by Umang Jain, described in his fix [2]. > > > > > > > > [1] > https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > > > [2] https://patchwork.libcamera.org/patch/13845/ > > > > > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > > > --- > > > > .../gstreamer_single_stream_test.cpp | 29 > +++++++++---------- > > > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > index 7292f3280617..a20d79109885 100644 > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > @@ -33,20 +33,15 @@ protected: > > > > if (status_ != TestPass) > > > > return status_; > > > > > > > > - g_autoptr(GstElement) convert0 = > gst_element_factory_make("videoconvert", "convert0"); > > > > - g_autoptr(GstElement) sink0 = > gst_element_factory_make("fakesink", "sink0"); > > > > - g_object_ref_sink(convert0); > > > > - g_object_ref_sink(sink0); > > > > - > > > > - if (!convert0 || !sink0) { > > > > - g_printerr("Not all elements could be created. > %p.%p\n", > > > > - convert0, sink0); > > > > + const gchar* streamDescription = "queue ! videoconvert > ! fakesink"; > > > > > > There's a queue introduced here that isn't mentioned in the commit > > > message. Why is it needed ? > > > > WARNING: from element > > > /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage: > > Pipeline construction is invalid, please add queues. > > > > Because Gstreamer says so, it won't be needed with fakesink I think, I'll > > fix it :) > > I'm not disputing that, but it wasn't the point. Before this patch, we > don't have a queue element inserted in the pipeline as far as I can > tell. It's added by this patch, for a reason that isn't explained in the > commit message. So, if the queue is needed, it should say, but the > reason needs to be explained (including why the problem doesn't occur > today). > > > > > + g_autoptr(GError) error0 = NULL; > > > > + stream0_ = > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > > > > > That's a very long line. Wrapping at 80 columns wouldn't be very > > > practical here, but > > > > > > stream0_ = > > > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > > > > > > GST_PARSE_FLAG_FATAL_ERRORS, > > > > &error0); > > > would already be more readable I think. > > > > > > > > > > > + if (!stream0_) { > > > > + g_printerr("Not all bins could be created. > %p\n", > > > stream0_); > > > > > > Given that stream0_ is null here, why do you print it ? Can error0 be > > > printed instead somehow (not as a %p I suppose) ? > > > > Nice catch. I'll make the changes. > > > > > return TestFail; > > > > } > > > > - > > > > - convert0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&convert0)); > > > > - sink0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&sink0)); > > > > + g_object_ref_sink(stream0_); > > > > > > > > if (createPipeline() != TestPass) > > > > return TestFail; > > > > @@ -57,8 +52,8 @@ protected: > > > > int run() override > > > > { > > > > /* Build the pipeline */ > > > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > convert0_, sink0_, NULL); > > > > - if (gst_element_link_many(libcameraSrc_, convert0_, > sink0_, NULL) != TRUE) { > > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, > stream0_, NULL); > > > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) > { > > > > g_printerr("Elements could not be linked.\n"); > > > > return TestFail; > > > > } > > > > @@ -72,9 +67,13 @@ protected: > > > > return TestPass; > > > > } > > > > > > > > + void cleanup() override > > > > + { > > > > + g_clear_object(&stream0_); > > > > + } > > > > + > > > > private: > > > > - GstElement *convert0_; > > > > - GstElement *sink0_; > > > > + GstElement *stream0_; > > > > }; > > > > > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > -- > Regards, > > Laurent Pinchart >
Hi Vedant, On 22/09/2021 11:10, Vedant Paranjape wrote: > Hi Laurent, > autovideosink needs a queue to function correctly. So it was a carry > over from the command description used in terminal. > > fakesink has no such requirement, thus it was not added earlier. > This should be written in the commit message to clarify it ;-). > On Wed, 22 Sep, 2021, 14:11 Laurent Pinchart, > <laurent.pinchart@ideasonboard.com > <mailto:laurent.pinchart@ideasonboard.com>> wrote: > > Hi Vedant, > > On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote: > > On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote: > > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > > > Simplify memory handling and complexity of the test by using > > > > gst_parse_bin_from_description_full [1]. It also fixed memory > leak reported > > > > by Umang Jain, described in his fix [2]. > > > > > > > > [1] > https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > <https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full> > > > > [2] https://patchwork.libcamera.org/patch/13845/ > <https://patchwork.libcamera.org/patch/13845/> > > > > > > > > Signed-off-by: Vedant Paranjape > <vedantparanjape160201@gmail.com > <mailto:vedantparanjape160201@gmail.com>> > > > > --- > > > > .../gstreamer_single_stream_test.cpp | 29 > +++++++++---------- > > > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > index 7292f3280617..a20d79109885 100644 > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > @@ -33,20 +33,15 @@ protected: > > > > if (status_ != TestPass) > > > > return status_; > > > > > > > > - g_autoptr(GstElement) convert0 = > gst_element_factory_make("videoconvert", "convert0"); > > > > - g_autoptr(GstElement) sink0 = > gst_element_factory_make("fakesink", "sink0"); Except that you are changing from fakesink to... fakesink, so, maybe should have it been introduced before ? Nevertheless just mention it in your v2 :-). Thanks ! PS: BTW, could you reply with text only mails and not HTML ? :-) > > > > - g_object_ref_sink(convert0); > > > > - g_object_ref_sink(sink0); > > > > - > > > > - if (!convert0 || !sink0) { > > > > - g_printerr("Not all elements could be > created. %p.%p\n", > > > > - convert0, sink0); > > > > + const gchar* streamDescription = "queue ! > videoconvert ! fakesink"; > > > > > > There's a queue introduced here that isn't mentioned in the commit > > > message. Why is it needed ? > > > > WARNING: from element > > > /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage: > > Pipeline construction is invalid, please add queues. > > > > Because Gstreamer says so, it won't be needed with fakesink I > think, I'll > > fix it :) > > I'm not disputing that, but it wasn't the point. Before this patch, we > don't have a queue element inserted in the pipeline as far as I can > tell. It's added by this patch, for a reason that isn't explained in the > commit message. So, if the queue is needed, it should say, but the > reason needs to be explained (including why the problem doesn't occur > today). > > > > > + g_autoptr(GError) error0 = NULL; > > > > + stream0_ = > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > > > > > That's a very long line. Wrapping at 80 columns wouldn't be very > > > practical here, but > > > > > > stream0_ = > > > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > > > > > > GST_PARSE_FLAG_FATAL_ERRORS, > > > > &error0); > > > would already be more readable I think. > > > > > > > > > > > + if (!stream0_) { > > > > + g_printerr("Not all bins could be > created. %p\n", > > > stream0_); > > > > > > Given that stream0_ is null here, why do you print it ? Can > error0 be > > > printed instead somehow (not as a %p I suppose) ? > > > > Nice catch. I'll make the changes. > > > > > return TestFail; > > > > } > > > > - > > > > - convert0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&convert0)); > > > > - sink0_ = reinterpret_cast<GstElement > *>(g_steal_pointer(&sink0)); > > > > + g_object_ref_sink(stream0_); > > > > > > > > if (createPipeline() != TestPass) > > > > return TestFail; > > > > @@ -57,8 +52,8 @@ protected: > > > > int run() override > > > > { > > > > /* Build the pipeline */ > > > > - gst_bin_add_many(GST_BIN(pipeline_), > libcameraSrc_, convert0_, sink0_, NULL); > > > > - if (gst_element_link_many(libcameraSrc_, > convert0_, sink0_, NULL) != TRUE) { > > > > + gst_bin_add_many(GST_BIN(pipeline_), > libcameraSrc_, stream0_, NULL); > > > > + if (gst_element_link(libcameraSrc_, stream0_) != > TRUE) { > > > > g_printerr("Elements could not be > linked.\n"); > > > > return TestFail; > > > > } > > > > @@ -72,9 +67,13 @@ protected: > > > > return TestPass; > > > > } > > > > > > > > + void cleanup() override > > > > + { > > > > + g_clear_object(&stream0_); > > > > + } > > > > + > > > > private: > > > > - GstElement *convert0_; > > > > - GstElement *sink0_; > > > > + GstElement *stream0_; > > > > }; > > > > > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > -- > Regards, > > Laurent Pinchart >
Hi Jean-Michel, On Wed, 22 Sep, 2021, 14:43 Jean-Michel Hautbois, < jeanmichel.hautbois@ideasonboard.com> wrote: > Hi Vedant, > > On 22/09/2021 11:10, Vedant Paranjape wrote: > > Hi Laurent, > > autovideosink needs a queue to function correctly. So it was a carry > > over from the command description used in terminal. > > > > fakesink has no such requirement, thus it was not added earlier. > > > > This should be written in the commit message to clarify it ;-). > I think there's some confusion. It was never in their before, I have maintained the original behaviour so why should it be mentioned? Adding queue was unintentional and a mistake and not done deliberately. > On Wed, 22 Sep, 2021, 14:11 Laurent Pinchart, > > <laurent.pinchart@ideasonboard.com > > <mailto:laurent.pinchart@ideasonboard.com>> wrote: > > > > Hi Vedant, > > > > On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote: > > > On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote: > > > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > > > > Simplify memory handling and complexity of the test by using > > > > > gst_parse_bin_from_description_full [1]. It also fixed memory > > leak reported > > > > > by Umang Jain, described in his fix [2]. > > > > > > > > > > [1] > > > https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > < > https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > > > > > > [2] https://patchwork.libcamera.org/patch/13845/ > > <https://patchwork.libcamera.org/patch/13845/> > > > > > > > > > > Signed-off-by: Vedant Paranjape > > <vedantparanjape160201@gmail.com > > <mailto:vedantparanjape160201@gmail.com>> > > > > > --- > > > > > .../gstreamer_single_stream_test.cpp | 29 > > +++++++++---------- > > > > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > > b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > > index 7292f3280617..a20d79109885 100644 > > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > > @@ -33,20 +33,15 @@ protected: > > > > > if (status_ != TestPass) > > > > > return status_; > > > > > > > > > > - g_autoptr(GstElement) convert0 = > > gst_element_factory_make("videoconvert", "convert0"); > > > > > - g_autoptr(GstElement) sink0 = > > gst_element_factory_make("fakesink", "sink0"); > > Except that you are changing from fakesink to... fakesink, so, maybe > should have it been introduced before ? Nevertheless just mention it in > your v2 :-). > I sent a v2 already, and it was a dumb mistake arising from blindly copy pasting stuff and not a feature :( Thanks ! > PS: BTW, could you reply with text only mails and not HTML ? :-) > Hmm, I just use Gmail. > > > > - g_object_ref_sink(convert0); > > > > > - g_object_ref_sink(sink0); > > > > > - > > > > > - if (!convert0 || !sink0) { > > > > > - g_printerr("Not all elements could be > > created. %p.%p\n", > > > > > - convert0, sink0); > > > > > + const gchar* streamDescription = "queue ! > > videoconvert ! fakesink"; > > > > > > > > There's a queue introduced here that isn't mentioned in the > commit > > > > message. Why is it needed ? > > > > > > WARNING: from element > > > > > > /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage: > > > Pipeline construction is invalid, please add queues. > > > > > > Because Gstreamer says so, it won't be needed with fakesink I > > think, I'll > > > fix it :) > > > > I'm not disputing that, but it wasn't the point. Before this patch, > we > > don't have a queue element inserted in the pipeline as far as I can > > tell. It's added by this patch, for a reason that isn't explained in > the > > commit message. So, if the queue is needed, it should say, but the > > reason needs to be explained (including why the problem doesn't occur > > today). > > > > > > > + g_autoptr(GError) error0 = NULL; > > > > > + stream0_ = > > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > > GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > > > > > > > That's a very long line. Wrapping at 80 columns wouldn't be very > > > > practical here, but > > > > > > > > stream0_ = > > > > gst_parse_bin_from_description_full(streamDescription, TRUE, > NULL, > > > > > > > > GST_PARSE_FLAG_FATAL_ERRORS, > > > > > > &error0); > > > > would already be more readable I think. > > > > > > > > > > > > > > + if (!stream0_) { > > > > > + g_printerr("Not all bins could be > > created. %p\n", > > > > stream0_); > > > > > > > > Given that stream0_ is null here, why do you print it ? Can > > error0 be > > > > printed instead somehow (not as a %p I suppose) ? > > > > > > Nice catch. I'll make the changes. > > > > > > > return TestFail; > > > > > } > > > > > - > > > > > - convert0_ = reinterpret_cast<GstElement > > *>(g_steal_pointer(&convert0)); > > > > > - sink0_ = reinterpret_cast<GstElement > > *>(g_steal_pointer(&sink0)); > > > > > + g_object_ref_sink(stream0_); > > > > > > > > > > if (createPipeline() != TestPass) > > > > > return TestFail; > > > > > @@ -57,8 +52,8 @@ protected: > > > > > int run() override > > > > > { > > > > > /* Build the pipeline */ > > > > > - gst_bin_add_many(GST_BIN(pipeline_), > > libcameraSrc_, convert0_, sink0_, NULL); > > > > > - if (gst_element_link_many(libcameraSrc_, > > convert0_, sink0_, NULL) != TRUE) { > > > > > + gst_bin_add_many(GST_BIN(pipeline_), > > libcameraSrc_, stream0_, NULL); > > > > > + if (gst_element_link(libcameraSrc_, stream0_) != > > TRUE) { > > > > > g_printerr("Elements could not be > > linked.\n"); > > > > > return TestFail; > > > > > } > > > > > @@ -72,9 +67,13 @@ protected: > > > > > return TestPass; > > > > > } > > > > > > > > > > + void cleanup() override > > > > > + { > > > > > + g_clear_object(&stream0_); > > > > > + } > > > > > + > > > > > private: > > > > > - GstElement *convert0_; > > > > > - GstElement *sink0_; > > > > > + GstElement *stream0_; > > > > > }; > > > > > > > > > > TEST_REGISTER(GstreamerSingleStreamTest) > > > > -- > > Regards, > > > > Laurent Pinchart > > Regards, Vedant Paranjape
Le mercredi 22 septembre 2021 à 11:41 +0300, Laurent Pinchart a écrit : > Hi Vedant, > > On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote: > > On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote: > > > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote: > > > > Simplify memory handling and complexity of the test by using > > > > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported > > > > by Umang Jain, described in his fix [2]. > > > > > > > > [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full > > > > [2] https://patchwork.libcamera.org/patch/13845/ > > > > > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > > > --- > > > > .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- > > > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > index 7292f3280617..a20d79109885 100644 > > > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > > > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > > > > @@ -33,20 +33,15 @@ protected: > > > > if (status_ != TestPass) > > > > return status_; > > > > > > > > - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); > > > > - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); > > > > - g_object_ref_sink(convert0); > > > > - g_object_ref_sink(sink0); > > > > - > > > > - if (!convert0 || !sink0) { > > > > - g_printerr("Not all elements could be created. %p.%p\n", > > > > - convert0, sink0); > > > > + const gchar* streamDescription = "queue ! videoconvert ! fakesink"; > > > > > > There's a queue introduced here that isn't mentioned in the commit > > > message. Why is it needed ? > > > > WARNING: from element > > /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage: > > Pipeline construction is invalid, please add queues. > > > > Because Gstreamer says so, it won't be needed with fakesink I think, I'll > > fix it :) > > I'm not disputing that, but it wasn't the point. Before this patch, we > don't have a queue element inserted in the pipeline as far as I can > tell. It's added by this patch, for a reason that isn't explained in the > commit message. So, if the queue is needed, it should say, but the > reason needs to be explained (including why the problem doesn't occur > today). The appropriate reason GStreamer core request queues is that the reported min- latency (sum of all reported latency in the chain) have gone larger then the reported max-latency (the about of data the pipeline can hold because buffers are dropped). This is a bit of a side effect of the introduction of the processing-deadline in GstVideoSink base class, which now declare some latency to account for the processing time, but does not have any queue to hold on the buffers if they arrive without that introduced processing delay. In libcamerasrc, we currently declare min-latency == max-latency, so basically we are saying that we can only hold as much latency as what we contribute. Normally the max-latency would be correlated to the number of allocated buffers assuming the capture thread can't starve. This way, the capture can keep going without loosing frames even if the pipeline is pushing back due to some added latency. Perhaps we want to fix that now ? We need an approximation of the frame duration though. - min-latency: This remains the observed latency for the first frame - max-latency: This is the min-latency + (num_buffers - 1) * estimated_duration > > > > > + g_autoptr(GError) error0 = NULL; > > > > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0); > > > > > > That's a very long line. Wrapping at 80 columns wouldn't be very > > > practical here, but > > > > > > stream0_ = > > > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, > > > > > > GST_PARSE_FLAG_FATAL_ERRORS, > > > &error0); > > > would already be more readable I think. > > > > > > > > > > > + if (!stream0_) { > > > > + g_printerr("Not all bins could be created. %p\n", > > > stream0_); > > > > > > Given that stream0_ is null here, why do you print it ? Can error0 be > > > printed instead somehow (not as a %p I suppose) ? > > > > Nice catch. I'll make the changes. > > > > > return TestFail; > > > > } > > > > - > > > > - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); > > > > - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); > > > > + g_object_ref_sink(stream0_); > > > > > > > > if (createPipeline() != TestPass) > > > > return TestFail; > > > > @@ -57,8 +52,8 @@ protected: > > > > int run() override > > > > { > > > > /* Build the pipeline */ > > > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL); > > > > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) { > > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); > > > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { > > > > g_printerr("Elements could not be linked.\n"); > > > > return TestFail; > > > > } > > > > @@ -72,9 +67,13 @@ protected: > > > > return TestPass; > > > > } > > > > > > > > + void cleanup() override > > > > + { > > > > + g_clear_object(&stream0_); > > > > + } > > > > + > > > > private: > > > > - GstElement *convert0_; > > > > - GstElement *sink0_; > > > > + GstElement *stream0_; > > > > }; > > > > > > > > TEST_REGISTER(GstreamerSingleStreamTest) >
diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp index 7292f3280617..a20d79109885 100644 --- a/test/gstreamer/gstreamer_single_stream_test.cpp +++ b/test/gstreamer/gstreamer_single_stream_test.cpp @@ -33,20 +33,15 @@ protected: if (status_ != TestPass) return status_; - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0"); - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0"); - g_object_ref_sink(convert0); - g_object_ref_sink(sink0); - - if (!convert0 || !sink0) { - g_printerr("Not all elements could be created. %p.%p\n", - convert0, sink0); + const gchar* streamDescription = "queue ! videoconvert ! fakesink"; + g_autoptr(GError) error0 = NULL; + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0); + if (!stream0_) { + g_printerr("Not all bins could be created. %p\n", stream0_); return TestFail; } - - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0)); - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0)); + g_object_ref_sink(stream0_); if (createPipeline() != TestPass) return TestFail; @@ -57,8 +52,8 @@ protected: int run() override { /* Build the pipeline */ - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL); - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) { + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL); + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) { g_printerr("Elements could not be linked.\n"); return TestFail; } @@ -72,9 +67,13 @@ protected: return TestPass; } + void cleanup() override + { + g_clear_object(&stream0_); + } + private: - GstElement *convert0_; - GstElement *sink0_; + GstElement *stream0_; }; TEST_REGISTER(GstreamerSingleStreamTest)
Simplify memory handling and complexity of the test by using gst_parse_bin_from_description_full [1]. It also fixed memory leak reported by Umang Jain, described in his fix [2]. [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full [2] https://patchwork.libcamera.org/patch/13845/ Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> --- .../gstreamer_single_stream_test.cpp | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-)