[libcamera-devel,1/2] gstreamer: Move negotiation logic to separate function
diff mbox series

Message ID 20231122135406.14921-2-jaslo@ziska.de
State Superseded
Headers show
Series
  • gstreamer: Implement renegotiation
Related show

Commit Message

Jaslo Ziska Nov. 22, 2023, 1:43 p.m. UTC
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@ziska.de>
---
 src/gstreamer/gstlibcamerasrc.cpp | 176 +++++++++++++++---------------
 1 file changed, 88 insertions(+), 88 deletions(-)

Comments

Nicolas Dufresne Nov. 22, 2023, 3:08 p.m. UTC | #1
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@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@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

Patch
diff mbox series

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)
+{
+	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