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

[paul.elder at ideasonboard.com]

On Tue, Sep 21, 2021 at 08:55:17AM -0400, Nicolas Dufresne wrote:
> 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

Perhaps.

> 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.

It's still a test in libcamera. I think that such code and history
organization still needs to be held to an equally high standard. Just
because it's a test doesn't mean the quality and organization can be
disregarded.

> This is rather discouraging

I see your point. My apologies for not looking at this earlier.

> on top of all the effort that is needed to "edit" email based
> patches.

I'm not sure what the difference is between editing email-based patches
vs commits on github is... both are more-or-less just rebasing and
resending.


Paul

> 
> > 
> > 
> > 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
> > > 
> 
>