[libcamera-devel] [PATCH v1] test: gstreamer: Simplify single stream test using functions from GstUtils
Vedant Paranjape
vedantparanjape160201 at gmail.com
Wed Sep 22 11:19:15 CEST 2021
Hi Jean-Michel,
On Wed, 22 Sep, 2021, 14:43 Jean-Michel Hautbois, <
jeanmichel.hautbois at 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 at ideasonboard.com
> > <mailto:laurent.pinchart at 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 at gmail.com
> > <mailto:vedantparanjape160201 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210922/462610a4/attachment-0001.htm>
More information about the libcamera-devel
mailing list