[libcamera-devel] [PATCH 1/2] gstreamer: Move negotiation logic to separate function

[Nicolas Dufresne]

Hi thanks for the patch.

Le mercredi 22 novembre 2023 à 14:43 +0100, Jaslo Ziska via libcamera-devel a
écrit :
> Move the code which negotiates all the source pad caps into a separate
> function called gst_libcamera_src_negotiate. When the negotiation fails
> this function will return FALSE and TRUE otherwise.
> 
> Use this function instead of doing the negotiation manually in
> gst_libcamera_src_task_enter and remove the now redundant error handling
> code accordingly.
> 
> Signed-off-by: Jaslo Ziska <jaslo at ziska.de>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 176 +++++++++++++++---------------
>  1 file changed, 88 insertions(+), 88 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 767017db..e7a49fef 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -379,6 +379,87 @@ gst_libcamera_src_open(GstLibcameraSrc *self)
>  	return true;
>  }
>  
> +static gboolean
> +gst_libcamera_src_negotiate(GstLibcameraSrc *self)

Try and document the locking in a comment above, this is very helpful for future
work.

nit: Just a general rule we have followed, if the function is a GObject virtual
funtion, callback, etc. then we use gboolean, otherwise we always use the C++
bool.

With these minor changes, the refactoring looks good.

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

> +{
> +	GstLibcameraSrcState *state = self->state;
> +
> +	g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps");
> +
> +	for (gsize i = 0; i < state->srcpads_.size(); i++) {
> +		GstPad *srcpad = state->srcpads_[i];
> +		StreamConfiguration &stream_cfg = state->config_->at(i);
> +
> +		/* Retrieve the supported caps. */
> +		g_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> +		g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter);
> +		if (gst_caps_is_empty(caps))
> +			return FALSE;
> +
> +		/* Fixate caps and configure the stream. */
> +		caps = gst_caps_make_writable(caps);
> +		gst_libcamera_configure_stream_from_caps(stream_cfg, caps);
> +		gst_libcamera_get_framerate_from_caps(caps, element_caps);
> +	}
> +
> +	/* Validate the configuration. */
> +	if (state->config_->validate() == CameraConfiguration::Invalid)
> +		return FALSE;
> +
> +	int ret = state->cam_->configure(state->config_.get());
> +	if (ret) {
> +		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> +				  ("Failed to configure camera: %s", g_strerror(-ret)),
> +				  ("Camera::configure() failed with error code %i", ret));
> +		return FALSE;
> +	}
> +
> +	/* Check frame duration bounds within controls::FrameDurationLimits */
> +	gst_libcamera_clamp_and_set_frameduration(state->initControls_,
> +						  state->cam_->controls(), element_caps);
> +
> +	/*
> +	 * Regardless if it has been modified, create clean caps and push the
> +	 * caps event. Downstream will decide if the caps are acceptable.
> +	 */
> +	for (gsize i = 0; i < state->srcpads_.size(); i++) {
> +		GstPad *srcpad = state->srcpads_[i];
> +		const StreamConfiguration &stream_cfg = state->config_->at(i);
> +
> +		g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);
> +		gst_libcamera_framerate_to_caps(caps, element_caps);
> +
> +		if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))
> +			return FALSE;
> +
> +	}
> +
> +	if (self->allocator)
> +		g_clear_object(&self->allocator);
> +
> +	self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get());
> +	if (!self->allocator) {
> +		GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,
> +				  ("Failed to allocate memory"),
> +				  ("gst_libcamera_allocator_new() failed."));
> +		return FALSE;
> +	}
> +
> +	for (gsize i = 0; i < state->srcpads_.size(); i++) {
> +		GstPad *srcpad = state->srcpads_[i];
> +		const StreamConfiguration &stream_cfg = state->config_->at(i);
> +
> +		GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,
> +								stream_cfg.stream());
> +		g_signal_connect_swapped(pool, "buffer-notify",
> +					 G_CALLBACK(gst_task_resume), self->task);
> +
> +		gst_libcamera_pad_set_pool(srcpad, pool);
> +	}
> +
> +	return TRUE;
> +}
> +
>  static void
>  gst_libcamera_src_task_run(gpointer user_data)
>  {
> @@ -468,11 +549,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
>  	GLibRecLocker lock(&self->stream_lock);
>  	GstLibcameraSrcState *state = self->state;
> -	GstFlowReturn flow_ret = GST_FLOW_OK;
>  	gint ret;
>  
> -	g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps");
> -
>  	GST_DEBUG_OBJECT(self, "Streaming thread has started");
>  
>  	gint stream_id_num = 0;
> @@ -500,89 +578,22 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  	}
>  	g_assert(state->config_->size() == state->srcpads_.size());
>  
> -	for (gsize i = 0; i < state->srcpads_.size(); i++) {
> -		GstPad *srcpad = state->srcpads_[i];
> -		StreamConfiguration &stream_cfg = state->config_->at(i);
> -
> -		/* Retrieve the supported caps. */
> -		g_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> -		g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter);
> -		if (gst_caps_is_empty(caps)) {
> -			flow_ret = GST_FLOW_NOT_NEGOTIATED;
> -			break;
> -		}
> -
> -		/* Fixate caps and configure the stream. */
> -		caps = gst_caps_make_writable(caps);
> -		gst_libcamera_configure_stream_from_caps(stream_cfg, caps);
> -		gst_libcamera_get_framerate_from_caps(caps, element_caps);
> -	}
> -
> -	if (flow_ret != GST_FLOW_OK)
> -		goto done;
> -
> -	/* Validate the configuration. */
> -	if (state->config_->validate() == CameraConfiguration::Invalid) {
> -		flow_ret = GST_FLOW_NOT_NEGOTIATED;
> -		goto done;
> -	}
> -
> -	ret = state->cam_->configure(state->config_.get());
> -	if (ret) {
> -		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> -				  ("Failed to configure camera: %s", g_strerror(-ret)),
> -				  ("Camera::configure() failed with error code %i", ret));
> +	if (!gst_libcamera_src_negotiate(self)) {
> +		state->initControls_.clear();
> +		GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED);
>  		gst_task_stop(task);
> -		flow_ret = GST_FLOW_NOT_NEGOTIATED;
> -		goto done;
> +		return;
>  	}
>  
> -	/* Check frame duration bounds within controls::FrameDurationLimits */
> -	gst_libcamera_clamp_and_set_frameduration(state->initControls_,
> -						  state->cam_->controls(), element_caps);
> -
> -	/*
> -	 * Regardless if it has been modified, create clean caps and push the
> -	 * caps event. Downstream will decide if the caps are acceptable.
> -	 */
> -	for (gsize i = 0; i < state->srcpads_.size(); i++) {
> -		GstPad *srcpad = state->srcpads_[i];
> -		const StreamConfiguration &stream_cfg = state->config_->at(i);
> -
> -		g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);
> -		gst_libcamera_framerate_to_caps(caps, element_caps);
> -
> -		if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {
> -			flow_ret = GST_FLOW_NOT_NEGOTIATED;
> -			break;
> -		}
> +	self->flow_combiner = gst_flow_combiner_new();
> +	for (GstPad *srcpad : state->srcpads_) {
> +		gst_flow_combiner_add_pad(self->flow_combiner, srcpad);
>  
>  		/* Send an open segment event with time format. */
>  		GstSegment segment;
>  		gst_segment_init(&segment, GST_FORMAT_TIME);
>  		gst_pad_push_event(srcpad, gst_event_new_segment(&segment));
> -	}
> -
> -	self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get());
> -	if (!self->allocator) {
> -		GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,
> -				  ("Failed to allocate memory"),
> -				  ("gst_libcamera_allocator_new() failed."));
> -		gst_task_stop(task);
> -		return;
> -	}
> -
> -	self->flow_combiner = gst_flow_combiner_new();
> -	for (gsize i = 0; i < state->srcpads_.size(); i++) {
> -		GstPad *srcpad = state->srcpads_[i];
> -		const StreamConfiguration &stream_cfg = state->config_->at(i);
> -		GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,
> -								stream_cfg.stream());
> -		g_signal_connect_swapped(pool, "buffer-notify",
> -					 G_CALLBACK(gst_task_resume), task);
>  
> -		gst_libcamera_pad_set_pool(srcpad, pool);
> -		gst_flow_combiner_add_pad(self->flow_combiner, srcpad);
>  	}
>  
>  	if (self->auto_focus_mode != controls::AfModeManual) {
> @@ -605,17 +616,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  		gst_task_stop(task);
>  		return;
>  	}
> -
> -done:
> -	state->initControls_.clear();
> -	switch (flow_ret) {
> -	case GST_FLOW_NOT_NEGOTIATED:
> -		GST_ELEMENT_FLOW_ERROR(self, flow_ret);
> -		gst_task_stop(task);
> -		break;
> -	default:
> -		break;
> -	}
>  }
>  
>  static void