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

[Nicolas Dufresne]

Le vendredi 10 novembre 2023 à 18:29 +0200, Laurent Pinchart a écrit :
> 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

Yes please do.

> 
> 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.

It covers it fully. I've identified problem in the reporter gstreamear pipeline
(missing -e) here:

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

As submitter can't edit description in bz I expected this information to be hard
to find and the bug to be useless for future improvement, so please close with
this patch being merged. For the other EOS case, it very niche, but certainly
best would be to add it to the todo, rather the bz in my opinion.

regards,
Nicolas

> 
> > 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",
>