[libcamera-devel] [PATCH v2 3/3] gstreamer: Implement renegotiation

[Laurent Pinchart]

Hi Jaslo,

Thank you for the patch.

On Thu, Nov 30, 2023 at 04:43:10PM +0100, Jaslo Ziska via libcamera-devel wrote:
> 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 a 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.
> In the case of GST_FLOW_NOT_NEGOTIATED every source pad is checked for
> the reconfiguration flag with gst_pad_needs_reconfigure which does not
> clear this flag. If at least one pad requested a reconfiguration the
> function returns without an error and the renegotiation will happen
> later in the running task. If no pad requested a reconfiguration then
> the function will return with an error.
> 
> In gst_libcamera_src_task_run the source pads are checked for the
> reconfigure flag by calling gst_pad_check_reconfigure and if one pad
> returns true and the caps are not sufficient anymore then the
> negotiation is triggered. It is fine to trigger the negotiation after
> only a single pad returns true for gst_pad_check_reconfigure because the
> reconfigure flags are cleared in the gst_libcamera_src_negotiate
> function.
> 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 by calling
>    GstLibcameraSrcState::clearRequests because the completed buffers
>    have the wrong configuration.
> 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.
> 
> Signed-off-by: Jaslo Ziska <jaslo at ziska.de>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 72 ++++++++++++++++++++++++++-----
>  1 file changed, 61 insertions(+), 11 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index d448a750..8bd32306 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)
> @@ -302,18 +301,42 @@ 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:
> +		break;

We usually add blank lines between cases.

> +	case GST_FLOW_NOT_NEGOTIATED: {
> +		bool reconfigure = false;
> +		for (GstPad *srcpad : srcpads_) {
> +			if (gst_pad_needs_reconfigure(srcpad)) {
> +				reconfigure = true;
> +				break;
> +			}
>  		}
>  
> -		return -EPIPE;
> +		// if no pads needs a reconfiguration something went wrong

We use C-style comments:

		/* If no pads need a reconfiguration something went wrong. */

Same comment below.

I'll fix these minor issues when applying the series, no need to submit
a new version.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		if (!reconfigure)
> +			err = -EPIPE;
> +
> +		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);
> +
> +		err = -EPIPE;
> +		break;
>  	}
>  
>  	return err;
> @@ -463,6 +486,8 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
>  					 G_CALLBACK(gst_task_resume), self->task);
>  
>  		gst_libcamera_pad_set_pool(srcpad, pool);
> +
> +		gst_pad_check_reconfigure(srcpad); // clear all reconfigure flags
>  	}
>  
>  	return true;
> @@ -496,6 +521,31 @@ gst_libcamera_src_task_run(gpointer user_data)
>  		return;
>  	}
>  
> +	// check if a srcpad requested a renegotiation
> +	bool 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);
> +			if (!gst_pad_peer_query_accept_caps(srcpad, caps)) {
> +				reconfigure = true;
> +				break;
> +			}
> +		}
> +	}
> +
> +	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

-- 
Regards,

Laurent Pinchart