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

[Nicolas Dufresne]

Le lundi 13 novembre 2023 à 01:18 +0200, Laurent Pinchart via libcamera-devel a
écrit :
> 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.

Note that the use of g_autoptr can happen with traditional variable declaration,
something like this:

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

/* end of scope will lead to an unref */

The delta is mostly that the unref happens later, in such a short function, the
benefit is theoretical, since the following line returns.

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

I agree.

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