[libcamera-devel] [PATCH v1] test: gstreamer: Simplify single stream test using functions from GstUtils
Vedant Paranjape
vedantparanjape160201 at gmail.com
Tue Sep 21 19:19:54 CEST 2021
Hi Laurent,
These patches are stale. I have sent a new patch series replacing this
please take a look at it.
Even though your comments do apply there too.
Regards,
Vedant Paranjape
On Tue, 21 Sep, 2021, 22:47 Laurent Pinchart, <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Vedant,
>
> Thank you for the patch.
>
> 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 ?
>
> > + 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) ?
>
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210921/f2bd2a76/attachment-0001.htm>
More information about the libcamera-devel
mailing list