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

[Laurent Pinchart]

On Fri, Nov 10, 2023 at 08:55:38PM -0500, Nicolas Dufresne wrote:
> Le vendredi 10 novembre 2023 à 18:29 +0200, Laurent Pinchart via libcamera-devel a écrit :
> > 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) {
> 
> Personally I like te in-scope declaration better. An you can improve it with:
> 
> 	if (g_autoptr(GstEvent) event = ...)
> 
> > > +		for (GstPad *srcpad : state->srcpads_)
> > > +			gst_pad_push_event(srcpad, gst_event_ref(event));
> > > +		gst_event_unref(event);
> 
> Witch let you drop this.

I probably dislike assignments in if () statements due to them being
frowned upon in the kernel coding style. I personally find them
error-prone, and compilers agree with me I think, as they will warn
asking if the author meant to compare instead of assign. When the
variable is declared in the if () statement it's obviously not an issue
anymore, so I'm OK with it here, even if I'm not sure to accept it more
globally in the libcamera coding style.

> > > +
> > > +		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
> 
> Please don't, there is a run-time check, but it can be compiled out. You will
> always get an error message (and a crashed if you decided to drop run-time
> checks, which are just protections).

My bad, I didn't realize that.

If we want to use a g_autoptr above, I would use one here too for
consistency.

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