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

[Jaslo Ziska]

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?

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