[PATCH 4/4] gstreamer: Reduce indentation in gst_libcamera_create_video_pool()

[Nicolas Dufresne]

Hi,

Le jeudi 22 mai 2025 à 14:55 +0200, Laurent Pinchart a écrit :
> Now that video pool creation is handled by a dedicated function, the
> logic can be simplified by returning early instead of nesting scopes. Do
> so to decrease indentation and improve readability.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 50 ++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 27 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 380f8368af8b..e0eb6e7b4227 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -528,47 +528,43 @@ gst_libcamera_create_video_pool(GstLibcameraSrc *self,
>  {
>  	g_autoptr(GstQuery) query = NULL;
>  	const gboolean need_pool = true;
> -	gboolean has_video_meta = false;
>  	GstBufferPool *video_pool = NULL;
>  
>  	gst_libcamera_extrapolate_info(info, stream_cfg.stride);
>  
>  	query = gst_query_new_allocation(caps, need_pool);
> -	if (!gst_pad_peer_query(srcpad, query))
> +	if (!gst_pad_peer_query(srcpad, query)) {
>  		GST_DEBUG_OBJECT(self, "Didn't get downstream ALLOCATION hints");
> -	else
> -		has_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL);
> +		return { NULL, 0 };

Ah, so this is your use of a tuple. It wasn't obvious in previous patch that there was this case
too. Please document briefly, it will help maintain this in the long run.


> +	}
>  
> -	if (!has_video_meta) {
> -		GstBufferPool *pool = NULL;
> +	if (gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL))
> +		return { NULL, 0 };
>  
> -		if (gst_query_get_n_allocation_pools(query) > 0)
> -			gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL);
> +	if (gst_query_get_n_allocation_pools(query) > 0)
> +		gst_query_parse_nth_allocation_pool(query, 0, &video_pool, NULL, NULL, NULL);

Once we figure-out a way to tell libcamera the number of buffers (just allocating N requests
does not seem to do it for me), the min should also be propagated, that will fix more stalls.

For this reason, it might be nice to move this before the find function, so we later have a
chance to save and use the min.

>  
> -		if (pool)
> -			video_pool = pool;
> -		else {
> -			GstStructure *config;
> -			guint min_buffers = 3;
> -			video_pool = gst_video_buffer_pool_new();
> +	if (!video_pool) {
> +		GstStructure *config;
> +		guint min_buffers = 3;
>  
> -			config = gst_buffer_pool_get_config(video_pool);
> -			gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0);
> +		video_pool = gst_video_buffer_pool_new();
> +		config = gst_buffer_pool_get_config(video_pool);
> +		gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0);

To be fixed in future update, I missed that min_buffers has been accidentally hard
coded. VideoPool is very flexible, but to avoid run-time allocation you should use the
information from gst_query_parse_nth_allocation_pool() to configure the pool.

One of the "NULL" hides the actual pipeline reported min. Since this is for the copy case,
we don't need an inflight buffer and can simply pass over the min/max. Normally we try and
use the size returned downstream too, downstream may want some tail padding which VideoPool
can accomodate for.

>  
> -			GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config);
> +		GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config);
>  
> -			gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);
> -		}
> +		gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);
> +	}
>  
> -		GST_WARNING_OBJECT(self, "Downstream doesn't support video meta, need to copy frame.");
> +	GST_WARNING_OBJECT(self, "Downstream doesn't support video meta, need to copy frame.");
>  
> -		if (!gst_buffer_pool_set_active(video_pool, true)) {
> -			gst_caps_unref(caps);
> -			GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> -					  ("Failed to active buffer pool"),
> -					  ("gst_libcamera_src_negotiate() failed."));
> -			return { NULL, -EINVAL };
> -		}
> +	if (!gst_buffer_pool_set_active(video_pool, true)) {
> +		gst_caps_unref(caps);
> +		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> +				  ("Failed to active buffer pool"),
> +				  ("gst_libcamera_src_negotiate() failed."));
> +		return { NULL, -EINVAL };

There seems to be more issues in that function, the pool configuration does not seem
complete and you may fail here trying to activate an unconfigured pool. But this is
not related to your changes so:

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

If I find anytime, I will provide some patches, yet I'm like you, I don't have a setup
to actually test it properly.

regards,
Nicolas

>  	}
>  
>  	return { video_pool, 0 };