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

[Vedant Paranjape]

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 at 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 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";
> > +             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)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210919/24d620e9/attachment-0001.htm>