| Message ID | 20231106165545.47116-1-jaslo@ziska.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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
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
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
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
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
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
+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
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
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
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
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
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
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
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",