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

[Jaslo Ziska]

Hi,

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> Hello,
>
> On Tue, Nov 14, 2023 at 01:18:57PM +0100, Jaslo Ziska via 
> libcamera-devel wrote:
>> This commit implements EOS handling for events sent to the 
>> libcamerasrc
>> element by the send_event method (which can happen when 
>> pressing
>> Ctrl-C while running gst-launch-1.0 -e, see below). EOS events 
>> from
>> downstream elements returning GST_FLOW_EOS are not considered 
>> here.
>>
>> To archive this 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.
>>
>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=91
>> Signed-off-by: Jaslo Ziska <jaslo at ziska.de>
>> Acked-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
>> ---
>> Thanks for all the advise on v2 everyone!
>>
>> In this revision I explain which EOS events are handled in the 
>> commit message a little bit more and link to the bug report.
>>
>> As recommended, g_autoptr is now used. I decided to declare 
>> g_autoptr(event) outside of the if-statement for now, I was not 
>> sure what the consensus on this is at the moment.
>>
>> Regards,
>>
>> Jaslo
>>
>>  src/gstreamer/gstlibcamerasrc.cpp | 36 
>>  ++++++++++++++++++++++++++++++-
>>  1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp 
>> b/src/gstreamer/gstlibcamerasrc.cpp
>> index 63d99571..767017db 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;
>>
>> +	g_autoptr(GstEvent) event = 
>> self->pending_eos.exchange(nullptr);
>> +	if (event) {
>> +		for (GstPad *srcpad : state->srcpads_)
>> +			gst_pad_push_event(srcpad, gst_event_ref(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,27 @@ 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: {
>> +		g_autoptr(GstEvent) oldEvent = 
>> self->pending_eos.exchange(event);
>
> I'm afraid this causes a compilation breakage with clang :-(
>
> ../../src/gstreamer/gstlibcamerasrc.cpp:768:23: error: unused 
> variable 'oldEvent' [-Werror,-Wunused-variable]
>                 g_autoptr(GstEvent) oldEvent = 
>                 self->pending_eos.exchange(event);
>                                     ^
>
> The patch has been merged, can you propose a fix quickly, or 
> should I
> revert to give you more time ?

It's ok, I can do it right now. Should the fix be a new commit or 
alter the old one? Also do you have any preferred way of fixing 
this?

Jaslo

>> +
>> +		ret = TRUE;
>> +		break;
>> +	}
>> +	default:
>> +		gst_event_unref(event);
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static void
>>  gst_libcamera_src_finalize(GObject *object)
>>  {
>> @@ -779,6 +810,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 +877,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",