<div dir="auto">Hi Laurent,<div dir="auto">autovideosink needs a queue to function correctly. So it was a carry over from the command description used in terminal.</div><div dir="auto"><br></div><div dir="auto">fakesink has no such requirement, thus it was not added earlier.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 22 Sep, 2021, 14:11 Laurent Pinchart, <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Vedant,<br>
<br>
On Tue, Sep 21, 2021 at 10:55:11PM +0530, Vedant Paranjape wrote:<br>
> On Tue, Sep 21, 2021 at 10:47 PM Laurent Pinchart wrote:<br>
> > On Wed, Sep 15, 2021 at 08:39:07PM +0530, Vedant Paranjape wrote:<br>
> > > Simplify memory handling and complexity of the test by using<br>
> > > gst_parse_bin_from_description_full [1]. It also fixed memory leak reported<br>
> > > by Umang Jain, described in his fix [2].<br>
> > ><br>
> > > [1] <a href="https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full" rel="noreferrer noreferrer" target="_blank">https://gstreamer.freedesktop.org/documentation/gstreamer/gstutils.html?gi-language=c#gst_parse_bin_from_description_full</a><br>
> > > [2] <a href="https://patchwork.libcamera.org/patch/13845/" rel="noreferrer noreferrer" target="_blank">https://patchwork.libcamera.org/patch/13845/</a><br>
> > ><br>
> > > Signed-off-by: Vedant Paranjape <<a href="mailto:vedantparanjape160201@gmail.com" target="_blank" rel="noreferrer">vedantparanjape160201@gmail.com</a>><br>
> > > ---<br>
> > > .../gstreamer_single_stream_test.cpp | 29 +++++++++----------<br>
> > > 1 file changed, 14 insertions(+), 15 deletions(-)<br>
> > ><br>
> > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp<br>
> > > index 7292f3280617..a20d79109885 100644<br>
> > > --- a/test/gstreamer/gstreamer_single_stream_test.cpp<br>
> > > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp<br>
> > > @@ -33,20 +33,15 @@ protected:<br>
> > > if (status_ != TestPass)<br>
> > > return status_;<br>
> > ><br>
> > > - g_autoptr(GstElement) convert0 = gst_element_factory_make("videoconvert", "convert0");<br>
> > > - g_autoptr(GstElement) sink0 = gst_element_factory_make("fakesink", "sink0");<br>
> > > - g_object_ref_sink(convert0);<br>
> > > - g_object_ref_sink(sink0);<br>
> > > -<br>
> > > - if (!convert0 || !sink0) {<br>
> > > - g_printerr("Not all elements could be created. %p.%p\n",<br>
> > > - convert0, sink0);<br>
> > > + const gchar* streamDescription = "queue ! videoconvert ! fakesink";<br>
> ><br>
> > There's a queue introduced here that isn't mentioned in the commit<br>
> > message. Why is it needed ?<br>
> <br>
> WARNING: from element<br>
> /GstPipeline:pipeline0/GstAutoVideoSink:autovideosink0/GstXvImageSink:autovideosink0-actual-sink-xvimage:<br>
> Pipeline construction is invalid, please add queues.<br>
> <br>
> Because Gstreamer says so, it won't be needed with fakesink I think, I'll<br>
> fix it :)<br>
<br>
I'm not disputing that, but it wasn't the point. Before this patch, we<br>
don't have a queue element inserted in the pipeline as far as I can<br>
tell. It's added by this patch, for a reason that isn't explained in the<br>
commit message. So, if the queue is needed, it should say, but the<br>
reason needs to be explained (including why the problem doesn't occur<br>
today).<br>
<br>
> > > + g_autoptr(GError) error0 = NULL;<br>
> > > + stream0_ = gst_parse_bin_from_description_full(streamDescription, TRUE, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &error0);<br>
> ><br>
> > That's a very long line. Wrapping at 80 columns wouldn't be very<br>
> > practical here, but<br>
> ><br>
> > stream0_ =<br>
> > gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,<br>
> ><br>
> > GST_PARSE_FLAG_FATAL_ERRORS,<br>
> > &error0);<br>
> > would already be more readable I think.<br>
> ><br>
> > ><br>
> > > + if (!stream0_) {<br>
> > > + g_printerr("Not all bins could be created. %p\n",<br>
> > stream0_);<br>
> ><br>
> > Given that stream0_ is null here, why do you print it ? Can error0 be<br>
> > printed instead somehow (not as a %p I suppose) ?<br>
> <br>
> Nice catch. I'll make the changes.<br>
> <br>
> > return TestFail;<br>
> > > }<br>
> > > -<br>
> > > - convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0));<br>
> > > - sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0));<br>
> > > + g_object_ref_sink(stream0_);<br>
> > ><br>
> > > if (createPipeline() != TestPass)<br>
> > > return TestFail;<br>
> > > @@ -57,8 +52,8 @@ protected:<br>
> > > int run() override<br>
> > > {<br>
> > > /* Build the pipeline */<br>
> > > - gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, convert0_, sink0_, NULL);<br>
> > > - if (gst_element_link_many(libcameraSrc_, convert0_, sink0_, NULL) != TRUE) {<br>
> > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, stream0_, NULL);<br>
> > > + if (gst_element_link(libcameraSrc_, stream0_) != TRUE) {<br>
> > > g_printerr("Elements could not be linked.\n");<br>
> > > return TestFail;<br>
> > > }<br>
> > > @@ -72,9 +67,13 @@ protected:<br>
> > > return TestPass;<br>
> > > }<br>
> > ><br>
> > > + void cleanup() override<br>
> > > + {<br>
> > > + g_clear_object(&stream0_);<br>
> > > + }<br>
> > > +<br>
> > > private:<br>
> > > - GstElement *convert0_;<br>
> > > - GstElement *sink0_;<br>
> > > + GstElement *stream0_;<br>
> > > };<br>
> > ><br>
> > > TEST_REGISTER(GstreamerSingleStreamTest)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div>