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

[nicolas at ndufresne.ca]

Le jeudi 09 novembre 2023 à 13:34 +0100, Jaslo Ziska via libcamera-
devel a écrit :
> Hi Barnabás,
> 
> thanks for the suggestion! I tested it and it does work and makes 
> the code a lot more readable in my opinion.
> 
> I just do not know how gstreamer- / glib-like the code is supposed 
> to stay as other elements for example use GMutex or GRecMutex 
> instead of the C++ native ones. Does anyone else have an opinion 
> on this?

I've fine with using the C++ way.

Nicolas

> 
> Regards,
> 
> Jaslo
> 
> Barnabás Pőcze <pobrn at protonmail.com> writes:
> 
> > Hi
> > 
> > 
> > I am not too familiar with gstreamer, so I cannot comment on 
> > that part,
> > but I believe using a single `std::atomic` object would be 
> > better. Please see below.
> > 
> > 
> > 2023. november 6., hétfő 17:52 keltezéssel, Jaslo Ziska via 
> > libcamera-devel <libcamera-devel at lists.libcamera.org> írta:
> > 
> > > ---
> > > Hi,
> > > 
> > > I recently implemented basic EOS handling for the libcamerasrc 
> > > gstreamer element.
> > > 
> > > I basically looked at how GstBaseSrc does it and tried to do it 
> > > similarly.
> > > 
> > > I have no idea whether the locking is correct or if this is 
> > > thread safe but so far it worked without any issues for my 
> > > purposes.
> > > 
> > > You can also now run the following command and receive a 
> > > working video file:
> > > 
> > > gst-launch-1.0 -e libcamerasrc ! videoconvert ! x264enc ! 
> > > h264parse ! mp4mux ! filesink location=test.mp4
> > > 
> > > Looking forward for feedback!
> > > 
> > > Cheers,
> > > 
> > > Jaslo
> > > 
> > >  src/gstreamer/gstlibcamerasrc.cpp | 47 
> > >  +++++++++++++++++++++++++++++++
> > >  1 file changed, 47 insertions(+)
> > > 
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp 
> > > b/src/gstreamer/gstlibcamerasrc.cpp
> > > index 63d99571..a4681e1e 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -144,6 +144,9 @@ struct _GstLibcameraSrc {
> > >  	gchar *camera_name;
> > >  	controls::AfModeEnum auto_focus_mode = 
> > >  controls::AfModeManual;
> > > 
> > > +	GstEvent *pending_eos;
> > > +	int has_pending_eos;
> > 
> > std::atomic<GstEvent *> instead of the above two?
> > 
> > 
> > > +
> > >  	GstLibcameraSrcState *state;
> > >  	GstLibcameraAllocator *allocator;
> > >  	GstFlowCombiner *flow_combiner;
> > > @@ -397,6 +400,21 @@ gst_libcamera_src_task_run(gpointer 
> > > user_data)
> > > 
> > >  	bool doResume = false;
> > > 
> > > +	if (g_atomic_int_get(&self->has_pending_eos)) {
> > > +		g_atomic_int_set(&self->has_pending_eos, FALSE);
> > 
> > Then you could just do
> > 
> >   if (auto *event = self->pending_eos.exchange(nullptr)) {
> >     ...
> >   }
> > 
> > > +
> > > +		GST_OBJECT_LOCK(self);
> > > +		GstEvent *event = self->pending_eos;
> > > +		self->pending_eos = NULL;
> > > +		GST_OBJECT_UNLOCK(self);
> > > +
> > > +		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 +765,32 @@ 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: {
> > > +		GST_OBJECT_LOCK(self);
> > > +		g_atomic_int_set(&self->has_pending_eos, TRUE);
> > > +		if (self->pending_eos)
> > > +			gst_event_unref(self->pending_eos);
> > > +		self->pending_eos = event;
> > > +		GST_OBJECT_UNLOCK(self);
> > 
> > And you could do
> > 
> >   if (auto *old_event = self->pending_eos.exchange(event))
> >     gst_event_unref(old_event);
> > 
> > 
> > > +
> > > +		ret = TRUE;
> > > +		break;
> > > +	}
> > > +	default:
> > > +		gst_event_unref(event);
> > > +		break;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  static void
> > >  gst_libcamera_src_finalize(GObject *object)
> > >  {
> > > @@ -779,6 +823,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 +890,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",
> > > --
> > > 2.42.1
> > 
> > 
> > Regards,
> > Barnabás Pőcze