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

[Jaslo Ziska]

Hi,

Umang Jain <umang.jain at ideasonboard.com> writes:
> Hi Jalso
>
> On 11/6/23 10:22 PM, Jaslo Ziska via libcamera-devel wrote:
>> ---
>> Hi,
>>
>> I recently implemented basic EOS handling for the libcamerasrc 
>> gstreamer element.
>
> Ah thanks, I remember we do track a bug report here:
> https://bugs.libcamera.org/show_bug.cgi?id=91

Ah yes, I remember that I stumbled upon this when I was first 
investigating this, I will update the bug report.

>>
>> 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
>
> Good to see that the diff is tested :-D

Speaking of testing, I just ran the test suite and, except for the 
multi_stream_test (which was skipped) the other tests succeeded.

>>
>> Looking forward for feedback!
>
> It looks logical and on the right track to me atleast. But there 
> might be other implications regarding the threading and
> locking mechanisms.

Thanks! That is what I worry about too, maybe someone can review 
this.

>> 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;
>> +
>>   	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);
>> +
>> +		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);
>> +
>> +		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);
>> +
>
> I always thought libcamerasrc was inheriting from GstBaseSrc, 
> but in fact it's not.
> So, setting the GST_ELEMENT_FLAG_SOURCE makes sense here.

This flag is actually required to receive the eos signal when it 
is send to the pipeline bin and not the element directly. Took me 
a while to figure out.

> Apart from a missing commit mesasge, the patch looks good to me. 
> I'll put this on my radar soon for testing it
> rigorously.

Oh yeah, a commit message would indeed be nice, completely forgot 
that, thanks :)

>>   	/* 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,

Jaslo