[libcamera-devel] gstreamer: Implement EOS handling
diff mbox series

Message ID 20231106165545.47116-1-jaslo@ziska.de
State Superseded
Headers show
Series
  • [libcamera-devel] gstreamer: Implement EOS handling
Related show

Commit Message

Jaslo Ziska Nov. 6, 2023, 4:52 p.m. UTC
---
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(+)

--
2.42.1

Comments

Barnabás Pőcze Nov. 7, 2023, 6:40 p.m. UTC | #1
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@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
Umang Jain Nov. 8, 2023, 7:36 a.m. UTC | #2
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

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

Apart from a missing commit mesasge, the patch looks good to me. I'll 
put this on my radar soon for testing it rigorously.
>   	/* 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
Jaslo Ziska Nov. 9, 2023, 12:34 p.m. UTC | #3
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@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@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
Jaslo Ziska Nov. 9, 2023, 12:51 p.m. UTC | #4
Hi,

Umang Jain <umang.jain@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
Kieran Bingham Nov. 9, 2023, 1:40 p.m. UTC | #5
Quoting Jaslo Ziska via libcamera-devel (2023-11-09 12:51:15)
> Hi,
> 
> Umang Jain <umang.jain@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 :)

Can you also add/clarify /how/ the EOS event is being sent in the commit
message please?

Is this issue occuring when you run the pipeline and press ctrl-c for
instance? or is there some other detail?

--
Kieran


> 
> >>      /* 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
Jaslo Ziska Nov. 9, 2023, 2:20 p.m. UTC | #6
Hi Kieran,

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:
> Quoting Jaslo Ziska via libcamera-devel (2023-11-09 12:51:15)
>> Hi,
>>
>> Umang Jain <umang.jain@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 :)
>
> Can you also add/clarify /how/ the EOS event is being sent in 
> the commit
> message please?

Sure, do you mean I should describe how the libcamerasrc element 
receives the EOS event from the pipeline or how the element is 
sending the event to its pads (or both)?

Also should I create a new patch with only the new commit message 
right now or wait for some actual changes?

>
> Is this issue occuring when you run the pipeline and press 
> ctrl-c for
> instance? or is there some other detail?

Yes, exactly, this is what is being fixed, as gst-launch-1.0 sends 
the EOS event to the pipeline bin and the bin apparently looks for 
the GST_ELEMENT_FLAG_SOURCE flag on an element which than receives 
the EOS event.

Regards,

Jaslo
Kieran Bingham Nov. 9, 2023, 3:05 p.m. UTC | #7
+Nicolas

Quoting Jaslo Ziska (2023-11-09 14:20:22)
> Hi Kieran,
> 
> Kieran Bingham <kieran.bingham@ideasonboard.com> writes:
> > Quoting Jaslo Ziska via libcamera-devel (2023-11-09 12:51:15)
> >> Hi,
> >>
> >> Umang Jain <umang.jain@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 :)
> >
> > Can you also add/clarify /how/ the EOS event is being sent in 
> > the commit
> > message please?
> 
> Sure, do you mean I should describe how the libcamerasrc element 
> receives the EOS event from the pipeline or how the element is 
> sending the event to its pads (or both)?
> 
> Also should I create a new patch with only the new commit message 
> right now or wait for some actual changes?

I would hold off a few more days.

Nicolas - is this something you could cast an eye over please?

--
Kieran

> 
> >
> > Is this issue occuring when you run the pipeline and press 
> > ctrl-c for
> > instance? or is there some other detail?
> 
> Yes, exactly, this is what is being fixed, as gst-launch-1.0 sends 
> the EOS event to the pipeline bin and the bin apparently looks for 
> the GST_ELEMENT_FLAG_SOURCE flag on an element which than receives 
> the EOS event.
> 
> Regards,
> 
> Jaslo
Nicolas Dufresne Nov. 9, 2023, 3:07 p.m. UTC | #8
Thanks,

will review/test soon.

Le jeudi 09 novembre 2023 à 15:05 +0000, Kieran Bingham a écrit :
> +Nicolas
> 
> Quoting Jaslo Ziska (2023-11-09 14:20:22)
> > Hi Kieran,
> > 
> > Kieran Bingham <kieran.bingham@ideasonboard.com> writes:
> > > Quoting Jaslo Ziska via libcamera-devel (2023-11-09 12:51:15)
> > > > Hi,
> > > > 
> > > > Umang Jain <umang.jain@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 :)
> > > 
> > > Can you also add/clarify /how/ the EOS event is being sent in 
> > > the commit
> > > message please?
> > 
> > Sure, do you mean I should describe how the libcamerasrc element 
> > receives the EOS event from the pipeline or how the element is 
> > sending the event to its pads (or both)?
> > 
> > Also should I create a new patch with only the new commit message 
> > right now or wait for some actual changes?
> 
> I would hold off a few more days.
> 
> Nicolas - is this something you could cast an eye over please?
> 
> --
> Kieran
> 
> > 
> > > 
> > > Is this issue occuring when you run the pipeline and press 
> > > ctrl-c for
> > > instance? or is there some other detail?
> > 
> > Yes, exactly, this is what is being fixed, as gst-launch-1.0 sends 
> > the EOS event to the pipeline bin and the bin apparently looks for 
> > the GST_ELEMENT_FLAG_SOURCE flag on an element which than receives 
> > the EOS event.
> > 
> > Regards,
> > 
> > Jaslo
Nicolas Dufresne via libcamera-devel Nov. 9, 2023, 3:14 p.m. UTC | #9
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@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@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
Nicolas Dufresne via libcamera-devel Nov. 9, 2023, 3:23 p.m. UTC | #10
Le jeudi 09 novembre 2023 à 13:40 +0000, Kieran Bingham via libcamera-
devel a écrit :
> Quoting Jaslo Ziska via libcamera-devel (2023-11-09 12:51:15)
> > Hi,
> > 
> > Umang Jain <umang.jain@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 :)
> 
> Can you also add/clarify /how/ the EOS event is being sent in the commit
> message please?
> 
> Is this issue occuring when you run the pipeline and press ctrl-c for
> instance? or is there some other detail?

This patch covers cases where the application decides to stop the
pipeline by sending it an EOS. To CTRL+C combined with -e option of
gst-launch-1.0 is a good example.

This patch does not cover (and this can be done later/seperatly):

  libcamerasrc ! identity eos-after=10 ! fakesink

Which is the case when downstream decides to stop. This case is a
little more complex for multi-pad elements and is actually nice to
tackle separately (its also more niche).

Nicolas

> 
> --
> Kieran
> 
> 
> > 
> > > >      /* 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
Nicolas Dufresne via libcamera-devel Nov. 9, 2023, 4:21 p.m. UTC | #11
Hi Jaslo,

Le lundi 06 novembre 2023 à 17:52 +0100, Jaslo Ziska via libcamera-
devel a écrit :
> ---
> 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

With a proper commit message, and the C++ atomic changes suggested I
think this change is good and can be merged. Please include this in
your v2:

Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> 
>  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);
> +
>  	/* 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
Jaslo Ziska Nov. 10, 2023, 11 a.m. UTC | #12
Hi Nicolas,

nicolas@ndufresne.ca writes:
> Le jeudi 09 novembre 2023 à 13:40 +0000, Kieran Bingham via 
> libcamera-
> devel a écrit :
>> Quoting Jaslo Ziska via libcamera-devel (2023-11-09 12:51:15)
>> > Hi,
>> >
>> > Umang Jain <umang.jain@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 :)
>>
>> Can you also add/clarify /how/ the EOS event is being sent in 
>> the commit
>> message please?
>>
>> Is this issue occuring when you run the pipeline and press 
>> ctrl-c for
>> instance? or is there some other detail?
>
> This patch covers cases where the application decides to stop 
> the
> pipeline by sending it an EOS. To CTRL+C combined with -e option 
> of
> gst-launch-1.0 is a good example.
>
> This patch does not cover (and this can be done 
> later/seperatly):
>
>   libcamerasrc ! identity eos-after=10 ! fakesink
>
> Which is the case when downstream decides to stop. This case is 
> a
> little more complex for multi-pad elements and is actually nice 
> to
> tackle separately (its also more niche).

Isn't this already implemented when checking for GST_FLOW_EOS in 
GstLibcameraSrcState::processRequest()? Or is this something else?

Regards,

Jaslo

>
> Nicolas
>
>>
>> --
>> Kieran
>>
>>
>> >
>> > > >      /* 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
Nicolas Dufresne Nov. 12, 2023, 11:56 p.m. UTC | #13
Hi Jaslo,

Le vendredi 10 novembre 2023 à 12:00 +0100, Jaslo Ziska a écrit :
> > 
> > This patch does not cover (and this can be done 
> > later/seperatly):
> > 
> >    libcamerasrc ! identity eos-after=10 ! fakesink
> > 
> > Which is the case when downstream decides to stop. This case is 
> > a
> > little more complex for multi-pad elements and is actually nice 
> > to
> > tackle separately (its also more niche).
> 
> Isn't this already implemented when checking for GST_FLOW_EOS in 
> GstLibcameraSrcState::processRequest()? Or is this something else?

You are correct, my bad. I didn't realize we had some handling. I'm not sure its
done properly though, since it seems pretty unfortunate that one FLOW_EOS on one
streams causes all streams to abort. I'd need to check demuxers, tee, rtspsrc
and other implementation to understand what is expected.

> 
> Regards,
> 
> Jaslo

Nicolas

Patch
diff mbox series

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);
+
 	/* 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",