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

[Umang Jain]

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)