[libcamera-devel] [PATCH 2/2] gstreamer: Implement renegotiation

[Nicolas Dufresne]

Le mercredi 22 novembre 2023 à 14:43 +0100, Jaslo Ziska via libcamera-devel a
écrit :
> This commit implements renegotiation of the camera configuration and
> source pad caps. A renegotiation can happen when a downstream element
> decides to change caps or the pipeline is dynamically changed.
> 
> To handle the renegotiation the GST_FLOW_NOT_NEGOTIATED return value has
> to be handled in GstLibcameraSrcState::processRequest. Otherwise the
> default would be to print an error and stop streaming.
> To archive this in a clean way the if-statement is altered into a switch
> statement which now also has a case for GST_FLOW_NOT_NEGOTIATED. Just
> like the case for GST_FLOW_OK, the function will return without an error
> because the renegotiation will happen later in the running task.

That's an interesting comment, I don't think we expect GST_FLOW_NOT_NEGOTIATED
in the renegotiation process. Downstream should keep handling the current
format, emit an upstream reconfigure event and wait (may drop) but returning
that flow is not really expected. How did you get that ?

> 
> In gst_libcamera_src_task_run all the source pads are checked for the
> reconfigure flag by calling gst_pad_check_reconfigure. It is important
> to iterate all source pads and not break after one pad requested a
> reconfiguration because gst_pad_check_reconfigure clears the reconfigure
> flag and if two pads request a reconfiguration at the same time the
> renegotiation would happen twice.

nit: We may want to break on first, and then just systematically clear all the
reconfigure flags in the negotiation() function. This would then become handy if
we need to renegotiation due to some "signal" behind sent by libcamera itself.

> If a source pad requested a reconfiguration it is first checked whether
> the old caps are still sufficient. If they are not the renegotiation
> will happen.
> If any pad requested a reconfiguration the following will happen:
> 1. The camera is stopped because changing the configuration may not
>    happen while running.
> 2. The completedRequests queue will be cleared because the completed
>    buffers have the wrong configuration (see below).
> 3. The new caps are negotiated by calling gst_libcamera_src_negotiate.
>    When the negotiation fails streaming will stop.
> 4. The camera is started again.
> 
> Clearing the completedRequests queue is archived with a new method in
> GstLibcameraSrcState called clearRequests. This function is now also
> used in gst_libcamera_src_task_leave as the code there did the same
> thing.

Might want to slim down the patch by doing this refactoring in its own patch ?

> 
> In gst_libcamera_src_task_enter, after the initial negotiation, a call
> to gst_pad_check_reconfigure was added to clear the reconfigure flag to
> avoid triggering a renegotiation when running the task for the first
> time.
> 
> Signed-off-by: Jaslo Ziska <jaslo at ziska.de>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 72 +++++++++++++++++++++++--------
>  1 file changed, 55 insertions(+), 17 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index e7a49fef..868fa20a 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -11,7 +11,6 @@
>   *  - Implement GstElement::send_event
>   *    + Allowing application to use FLUSH/FLUSH_STOP
>   *    + Prevent the main thread from accessing streaming thread
> - *  - Implement renegotiation (even if slow)
>   *  - Implement GstElement::request-new-pad (multi stream)
>   *    + Evaluate if a single streaming thread is fine
>   *  - Add application driven request (snapshot)
> @@ -133,6 +132,7 @@ struct GstLibcameraSrcState {
>  	int queueRequest();
>  	void requestCompleted(Request *request);
>  	int processRequest();
> +	void clearRequests();
>  };
>  
>  struct _GstLibcameraSrc {
> @@ -301,23 +301,39 @@ int GstLibcameraSrcState::processRequest()
>  							srcpad, ret);
>  	}
>  
> -	if (ret != GST_FLOW_OK) {
> -		if (ret == GST_FLOW_EOS) {
> -			g_autoptr(GstEvent) eos = gst_event_new_eos();
> -			guint32 seqnum = gst_util_seqnum_next();
> -			gst_event_set_seqnum(eos, seqnum);
> -			for (GstPad *srcpad : srcpads_)
> -				gst_pad_push_event(srcpad, gst_event_ref(eos));
> -		} else if (ret != GST_FLOW_FLUSHING) {
> -			GST_ELEMENT_FLOW_ERROR(src_, ret);
> -		}
> +	switch (ret) {
> +	case GST_FLOW_OK:
> +	case GST_FLOW_NOT_NEGOTIATED:
> +		break;
> +	case GST_FLOW_EOS: {
> +		g_autoptr(GstEvent) eos = gst_event_new_eos();
> +		guint32 seqnum = gst_util_seqnum_next();
> +		gst_event_set_seqnum(eos, seqnum);
> +		for (GstPad *srcpad : srcpads_)
> +			gst_pad_push_event(srcpad, gst_event_ref(eos));
> +
> +		err = -EPIPE;
> +		break;
> +	}
> +	case GST_FLOW_FLUSHING:
> +		err = -EPIPE;
> +		break;
> +	default:
> +		GST_ELEMENT_FLOW_ERROR(src_, ret);
>  
> -		return -EPIPE;
> +		err = -EPIPE;
> +		break;
>  	}
>  
>  	return err;
>  }
>  
> +void GstLibcameraSrcState::clearRequests()
> +{
> +	GLibLocker locker(&lock_);
> +	completedRequests_ = {};
> +}
> +
>  static bool
>  gst_libcamera_src_open(GstLibcameraSrc *self)
>  {
> @@ -488,6 +504,31 @@ gst_libcamera_src_task_run(gpointer user_data)
>  		return;
>  	}
>  
> +	// check if a srcpad requested a renegotiation
> +	gboolean reconfigure = FALSE;
> +	for (GstPad *srcpad : state->srcpads_) {
> +		if (gst_pad_check_reconfigure(srcpad)) {
> +			// check whether the caps even need changing
> +			g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad);
> +			g_autoptr(GstCaps) intersection = gst_pad_peer_query_caps(srcpad, caps);

Just wondering, but maybe gst_pad_peer_query_accept_caps() would do the job,
with a slightly improved performance ? This is only possible since
gst_pad_get_current_caps() is fixed. At this point, it should cannot be null
either.

> +
> +			if (gst_caps_is_empty(intersection))
> +				reconfigure = TRUE;
> +		}
> +	}
> +
> +	if (reconfigure) {
> +		state->cam_->stop();
> +		state->clearRequests();
> +
> +		if (!gst_libcamera_src_negotiate(self)) {
> +			GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED);
> +			gst_task_stop(self->task);
> +		}
> +
> +		state->cam_->start(&state->initControls_);
> +	}
> +
>  	/*
>  	 * Create and queue one request. If no buffers are available the
>  	 * function returns -ENOBUFS, which we ignore here as that's not a
> @@ -594,6 +635,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  		gst_segment_init(&segment, GST_FORMAT_TIME);
>  		gst_pad_push_event(srcpad, gst_event_new_segment(&segment));
>  
> +		gst_pad_check_reconfigure(srcpad); // clear reconfigure flag
>  	}
>  
>  	if (self->auto_focus_mode != controls::AfModeManual) {
> @@ -629,11 +671,7 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task,
>  	GST_DEBUG_OBJECT(self, "Streaming thread is about to stop");
>  
>  	state->cam_->stop();
> -
> -	{
> -		GLibLocker locker(&state->lock_);
> -		state->completedRequests_ = {};
> -	}
> +	state->clearRequests();
>  
>  	{
>  		GLibRecLocker locker(&self->stream_lock);