<div dir="auto"><div>Hi Jean-Michel,<br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 22 Sep, 2021, 14:43 Jean-Michel Hautbois, <<a href="mailto:jeanmichel.hautbois@ideasonboard.com">jeanmichel.hautbois@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 22/09/2021 11:10, Vedant Paranjape wrote:<br>
> Hi Laurent,<br>
> autovideosink needs a queue to function correctly. So it was a carry<br>
> over from the command description used in terminal.<br>
> <br>
> fakesink has no such requirement, thus it was not added earlier.<br>
> <br>
<br>
This should be written in the commit message to clarify it ;-).<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I think there's some confusion. It was never in their before, I have maintained the original behaviour so why should it be mentioned?</div><div dir="auto"><br></div><div dir="auto">Adding queue was unintentional and a mistake and not done deliberately.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> On Wed, 22 Sep, 2021, 14:11 Laurent Pinchart,<br>
> <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank" rel="noreferrer">laurent.pinchart@ideasonboard.com</a><br>
> <mailto:<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank" rel="noreferrer">laurent.pinchart@ideasonboard.com</a>>> wrote:<br>
> <br>
> 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<br>
> leak reported<br>
> > > > by Umang Jain, described in his fix [2].<br>
> > > ><br>
> > > > [1]<br>
> <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>
> <<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>
> <<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<br>
> <<a href="mailto:vedantparanjape160201@gmail.com" target="_blank" rel="noreferrer">vedantparanjape160201@gmail.com</a><br>
> <mailto:<a href="mailto:vedantparanjape160201@gmail.com" target="_blank" rel="noreferrer">vedantparanjape160201@gmail.com</a>>><br>
> > > > ---<br>
> > > > .../gstreamer_single_stream_test.cpp | 29<br>
> +++++++++----------<br>
> > > > 1 file changed, 14 insertions(+), 15 deletions(-)<br>
> > > ><br>
> > > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp<br>
> 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 =<br>
> gst_element_factory_make("videoconvert", "convert0");<br>
> > > > - g_autoptr(GstElement) sink0 =<br>
> gst_element_factory_make("fakesink", "sink0");<br>
<br>
Except that you are changing from fakesink to... fakesink, so, maybe<br>
should have it been introduced before ? Nevertheless just mention it in<br>
your v2 :-).<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I sent a v2 already, and it was a dumb mistake arising from blindly copy pasting stuff and not a feature :(</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks !<br>
PS: BTW, could you reply with text only mails and not HTML ? :-)<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Hmm, I just use Gmail.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > > > - g_object_ref_sink(convert0);<br>
> > > > - g_object_ref_sink(sink0);<br>
> > > > -<br>
> > > > - if (!convert0 || !sink0) {<br>
> > > > - g_printerr("Not all elements could be<br>
> created. %p.%p\n",<br>
> > > > - convert0, sink0);<br>
> > > > + const gchar* streamDescription = "queue !<br>
> 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>
> ><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<br>
> 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_ =<br>
> gst_parse_bin_from_description_full(streamDescription, TRUE, NULL,<br>
> 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>
> > > <br>
> &error0);<br>
> > > would already be more readable I think.<br>
> > ><br>
> > > ><br>
> > > > + if (!stream0_) {<br>
> > > > + g_printerr("Not all bins could be<br>
> created. %p\n",<br>
> > > stream0_);<br>
> > ><br>
> > > Given that stream0_ is null here, why do you print it ? Can<br>
> 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<br>
> *>(g_steal_pointer(&convert0));<br>
> > > > - sink0_ = reinterpret_cast<GstElement<br>
> *>(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_),<br>
> libcameraSrc_, convert0_, sink0_, NULL);<br>
> > > > - if (gst_element_link_many(libcameraSrc_,<br>
> convert0_, sink0_, NULL) != TRUE) {<br>
> > > > + gst_bin_add_many(GST_BIN(pipeline_),<br>
> libcameraSrc_, stream0_, NULL);<br>
> > > > + if (gst_element_link(libcameraSrc_, stream0_) !=<br>
> TRUE) {<br>
> > > > g_printerr("Elements could not be<br>
> 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></div><div dir="auto"><br></div><div dir="auto">Regards,</div><div dir="auto">Vedant Paranjape</div></div>