[libcamera-devel] [PATCH v1] test: gstreamer: Simplify single stream test using functions from GstUtils

[Laurent Pinchart]

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 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");
> > > -             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