[libcamera-devel] [PATCH 1/2] test: gstreamer: Simplify elements' ownerships

[Nicolas Dufresne]

Le mardi 14 septembre 2021 à 17:46 +0530, Umang Jain a écrit :
> In gstreamer, when elements are created, usually a floating [1]
> reference is returned which simply means, there is no ownership
> transfer (yet). Once can simply check for NULL and return through
> an error path, without bothering to clean up. Hence, g_autoptr is
> not much of help here.
> 
> If the NULL checks have been passed successfully, elements are ready
> to use. However, we must claim ownership/reference it before using
> them via g_object_ref_sink().
> 
> This patch build upon this principle and removes the g_autoptr
> from gstreamer tests whereever necessary to tide up the code.
> 
> [1] https://gstreamer.freedesktop.org/documentation/additional/design/MT-refcounting.html?gi-language=c#refcounting1
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>

Note, I still made a comment below ....

> ---
>  test/gstreamer/gstreamer_single_stream_test.cpp | 14 ++++++--------
>  test/gstreamer/gstreamer_test.cpp               |  9 ++++-----
>  2 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp
> index 7292f328..d992455c 100644
> --- a/test/gstreamer/gstreamer_single_stream_test.cpp
> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp
> @@ -33,20 +33,18 @@ 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);
> +		convert0_ = gst_element_factory_make("videoconvert", "convert0");
> +		sink0_ = gst_element_factory_make("fakesink", "sink0");
>  
> -		if (!convert0 || !sink0) {
> +		if (!convert0_ || !sink0_) {
>  			g_printerr("Not all elements could be created. %p.%p\n",
> -				   convert0, sink0);
> +				   convert0_, sink0_);
>  
>  			return TestFail;
>  		}
>  
> -		convert0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&convert0));
> -		sink0_ = reinterpret_cast<GstElement *>(g_steal_pointer(&sink0));
> +		g_object_ref_sink(convert0_);
> +		g_object_ref_sink(sink0_);

I prefer when ref_sink are inline or close to their allocation.

>  
>  		if (createPipeline() != TestPass)
>  			return TestFail;
> diff --git a/test/gstreamer/gstreamer_test.cpp b/test/gstreamer/gstreamer_test.cpp
> index 41712505..227a5c37 100644
> --- a/test/gstreamer/gstreamer_test.cpp
> +++ b/test/gstreamer/gstreamer_test.cpp
> @@ -78,18 +78,17 @@ GstreamerTest::~GstreamerTest()
>  
>  int GstreamerTest::createPipeline()
>  {
> -	g_autoptr(GstElement) libcameraSrc = gst_element_factory_make("libcamerasrc", "libcamera");
> +	libcameraSrc_ = gst_element_factory_make("libcamerasrc", "libcamera");
>  	pipeline_ = gst_pipeline_new("test-pipeline");
> -	g_object_ref_sink(libcameraSrc);
>  
> -	if (!libcameraSrc || !pipeline_) {
> +	if (!libcameraSrc_ || !pipeline_) {
>  		g_printerr("Unable to create create pipeline %p.%p\n",
> -			   libcameraSrc, pipeline_);
> +			   libcameraSrc_, pipeline_);
>  
>  		return TestFail;
>  	}
>  
> -	libcameraSrc_ = reinterpret_cast<GstElement *>(g_steal_pointer(&libcameraSrc));
> +	g_object_ref_sink(libcameraSrc_);
>  
>  	return TestPass;
>  }