[libcamera-devel] [PATCH v2] gstreamer: Implement element EOS handling

[Laurent Pinchart]

Hi Jaslo,

Thank you for the patch.

On Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote:
> Add a function for the send_event method which handles the GST_EVENT_EOS
> event. This function will set an atomic to the received event and push
> this EOS event to all source pads in the running task.
> 
> Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a
> source element which enables it to receive EOS events sent to the
> (pipeline) bin containing it.
> This in turn enables libcamerasrc to for example receive EOS events from
> gst-launch-1.0 when using the -e flag.

You should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with

Bug: https://bugs.libcamera.org/show_bug.cgi?id=91

Does this patch fix the bug completely or partially ? Nicolas mentioned
in the review of v1 that there are two different EOS cases to consider,
one related to stopping the pipeline with Ctrl-C, and the other one to
downstream elements sending EOS. The commit message should explain that
only the first case is addressed.

> Signed-off-by: Jaslo Ziska <jaslo at ziska.de>
> Acked-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> ---
> Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message.
> 
>  src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 63d99571..8e197956 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -9,7 +9,6 @@
>  /**
>   * \todo The following is a list of items that needs implementation in the GStreamer plugin
>   *  - Implement GstElement::send_event
> - *    + Allowing application to send EOS
>   *    + Allowing application to use FLUSH/FLUSH_STOP
>   *    + Prevent the main thread from accessing streaming thread
>   *  - Implement renegotiation (even if slow)
> @@ -29,6 +28,7 @@
> 
>  #include "gstlibcamerasrc.h"
> 
> +#include <atomic>
>  #include <queue>
>  #include <vector>
> 
> @@ -144,6 +144,8 @@ struct _GstLibcameraSrc {
>  	gchar *camera_name;
>  	controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
> 
> +	std::atomic<GstEvent *> pending_eos;
> +
>  	GstLibcameraSrcState *state;
>  	GstLibcameraAllocator *allocator;
>  	GstFlowCombiner *flow_combiner;
> @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data)
> 
>  	bool doResume = false;
> 
> +	if (GstEvent *event = self->pending_eos.exchange(nullptr)) {

I don't think we ever declare variables in if () statements in
libcamera, you may want to write this as

	GstEvent *event = self->pending_eos.exchange(nullptr);
	if (event) {

> +		for (GstPad *srcpad : state->srcpads_)
> +			gst_pad_push_event(srcpad, gst_event_ref(event));
> +		gst_event_unref(event);
> +
> +		return;
> +	}
> +
>  	/*
>  	 * Create and queue one request. If no buffers are available the
>  	 * function returns -ENOBUFS, which we ignore here as that's not a
> @@ -747,6 +757,28 @@ gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)
>  	return ret;
>  }
> 
> +static gboolean
> +gst_libcamera_src_send_event(GstElement *element, GstEvent *event)
> +{
> +	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);
> +	gboolean ret = FALSE;
> +
> +	switch (GST_EVENT_TYPE(event)) {
> +	case GST_EVENT_EOS: {
> +		if (GstEvent *old_event = self->pending_eos.exchange(event))
> +			gst_event_unref(old_event);

s/old_event/oldEvent/

gst_event_unref() includes a null check, so you could simplify this to

		GstEvent *oldEvent = self->pending_eos.exchange(event))
		gst_event_unref(oldEvent);

> +
> +		ret = TRUE;
> +		break;
> +	}
> +	default:
> +		gst_event_unref(event);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  static void
>  gst_libcamera_src_finalize(GObject *object)
>  {
> @@ -779,6 +811,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)
>  	state->srcpads_.push_back(gst_pad_new_from_template(templ, "src"));
>  	gst_element_add_pad(GST_ELEMENT(self), state->srcpads_.back());
> 
> +	GST_OBJECT_FLAG_SET(self, GST_ELEMENT_FLAG_SOURCE);
> +
>  	/* C-style friend. */
>  	state->src_ = self;
>  	self->state = state;
> @@ -844,6 +878,7 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>  	element_class->request_new_pad = gst_libcamera_src_request_new_pad;
>  	element_class->release_pad = gst_libcamera_src_release_pad;
>  	element_class->change_state = gst_libcamera_src_change_state;
> +	element_class->send_event = gst_libcamera_src_send_event;
> 
>  	gst_element_class_set_metadata(element_class,
>  				       "libcamera Source", "Source/Video",

-- 
Regards,

Laurent Pinchart