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

[Nicolas Dufresne]

Le mardi 21 septembre 2021 à 18:55 +0900, paul.elder at ideasonboard.com a écrit :
> Hi Vedant,
> 
> 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].
> 
> "For further information, see this patch that wasn't merged"
> 
> True, the patch isn't big or difficult to review, but you did squash two
> patches that were doing distinct things (though they did have the same
> end goal).
> 
> Please split the patches and expand the commit messages (copying what
> Umang had is fine; he said so in the original patch series).

My personal opinion on the manner is that we are right now being over picky for
some splitting that in my opinion is not a libcamera bug, but a test bug. I
respect your request, but I'd like to underline that we need to make a tradeoff
at some point, specially for code that isn't part of libcamera. This is rather
discouraging on top of all the effort that is needed to "edit" email based
patches.

> 
> 
> Paul
> 
> > 
> > [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";
> > +		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)
> > -- 
> > 2.25.1
> >