[PATCH v2 2/7] gstreamer: Factor out video pool creation

[Nicolas Dufresne]

Le jeudi 05 juin 2025 à 11:50 +0300, Laurent Pinchart a écrit :
> On Wed, Jun 04, 2025 at 01:08:16PM -0400, Nicolas Dufresne wrote:
> > Le mercredi 04 juin 2025 à 16:07 +0300, Laurent Pinchart a écrit :
> > > The gst_libcamera_src_negotiate() function uses 5 identation levels,
> > > causing long lines. Move video pool creation to a separate function to
> > > increase readability.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > Changes since v1:
> > > 
> > > - Document the gst_libcamera_create_video_pool() function
> > > - Keep the gst_libcamera_extrapolate_info() call out of the new function
> > > - Reorder function arguments
> > > ---
> > >  src/gstreamer/gstlibcamerasrc.cpp | 114 +++++++++++++++++++-----------
> > >  1 file changed, 72 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > index 6ede43b06a8a..79b94c7ee7c2 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -29,6 +29,7 @@
> > > 
> > >  #include <atomic>
> > >  #include <queue>
> > > +#include <tuple>
> > >  #include <vector>
> > > 
> > >  #include <libcamera/camera.h>
> > > @@ -521,6 +522,72 @@ gst_libcamera_src_open(GstLibcameraSrc *self)
> > >  	return true;
> > >  }
> > > 
> > > +/**
> > > + * \brief Create a video pool for a pad
> > > + * \param[in] self The libcamerasrc instance
> > > + * \param[in] srcpad The pad
> > > + * \param[in] caps The pad caps
> > > + * \param[in] info The video info for the pad
> > > + *
> > > + * This function creates and returns a video buffers pool for the given pad if
> > 
> > Usually no s to buffer when saying "buffer pool". Its like "a noodle soup" versus
> > "a bowl of noodles".
> > 
> > > + * needed to accommodate stride mismatch. If the peer element supports the meta
> > > + * API, the stride will be negotiated, and no pool if needed.
> > 
> > if needed -> is needed ? 
> 
> Oops. I've updated the comment to
> 
>  * This function creates and returns a video buffer pool for the given pad if
>  * needed to accommodate stride mismatch. If the peer element supports stride
>  * negotiation through the meta API, no pool is needed and the function will
>  * return a null pool.
>  *
>  * \return A tuple containing the video buffers pool pointer and an error code

Ack, thanks.

> 
> > > + *
> > > + * \return A tuple containing the video buffers pool pointer and an error code.
> > > + * When no error occurs, the pool can be null if the peer element supports
> > > + * stride negotiation.
> > 
> > Sounds great.
> > 
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > 
> > > + */
> > > +static std::tuple<GstBufferPool *, int>
> > > +gst_libcamera_create_video_pool(GstLibcameraSrc *self, GstPad *srcpad,
> > > +				GstCaps *caps, const GstVideoInfo *info)
> > > +{
> > > +	GstQuery *query = NULL;
> > > +	const gboolean need_pool = true;
> > > +	gboolean has_video_meta = false;
> > > +	GstBufferPool *video_pool = NULL;
> > > +
> > > +	query = gst_query_new_allocation(caps, need_pool);
> > > +	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);
> > > +
> > > +	if (!has_video_meta) {
> > > +		GstBufferPool *pool = NULL;
> > > +
> > > +		if (gst_query_get_n_allocation_pools(query) > 0)
> > > +			gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL);
> > > +
> > > +		if (pool)
> > > +			video_pool = pool;
> > > +		else {
> > > +			GstStructure *config;
> > > +			guint min_buffers = 3;
> > > +			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);
> > > +
> > > +			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_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 };
> > > +		}
> > > +	}
> > > +
> > > +	gst_query_unref(query);
> > > +	return { video_pool, 0 };
> > > +}
> > > +
> > >  /* Must be called with stream_lock held. */
> > >  static bool
> > >  gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> > > @@ -603,50 +670,13 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> > >  		/* Stride mismatch between camera stride and that calculated by video-info. */
> > >  		if (static_cast<unsigned int>(info.stride[0]) != stream_cfg.stride &&
> > >  		    GST_VIDEO_INFO_FORMAT(&info) != GST_VIDEO_FORMAT_ENCODED) {
> > > -			GstQuery *query = NULL;
> > > -			const gboolean need_pool = true;
> > > -			gboolean has_video_meta = false;
> > > -
> > >  			gst_libcamera_extrapolate_info(&info, stream_cfg.stride);
> > > 
> > > -			query = gst_query_new_allocation(caps, need_pool);
> > > -			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);
> > > -
> > > -			if (!has_video_meta) {
> > > -				GstBufferPool *pool = NULL;
> > > -
> > > -				if (gst_query_get_n_allocation_pools(query) > 0)
> > > -					gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL);
> > > -
> > > -				if (pool)
> > > -					video_pool = pool;
> > > -				else {
> > > -					GstStructure *config;
> > > -					guint min_buffers = 3;
> > > -					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);
> > > -
> > > -					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_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 false;
> > > -				}
> > > -			}
> > > -			gst_query_unref(query);
> > > +			std::tie(video_pool, ret) =
> > > +				gst_libcamera_create_video_pool(self, srcpad,
> > > +								caps, &info);
> > > +			if (ret)
> > > +				return false;
> > >  		}
> > > 
> > >  		GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,