| Message ID | 20231110121133.17950-1-jaslo@ziska.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jaslo, Thank you for the patch. On Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote: > Add a function for the send_event method which handles the GST_EVENT_EOS > event. This function will set an atomic to the received event and push > this EOS event to all source pads in the running task. > > Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a > source element which enables it to receive EOS events sent to the > (pipeline) bin containing it. > This in turn enables libcamerasrc to for example receive EOS events from > gst-launch-1.0 when using the -e flag. You should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with Bug: https://bugs.libcamera.org/show_bug.cgi?id=91 Does this patch fix the bug completely or partially ? Nicolas mentioned in the review of v1 that there are two different EOS cases to consider, one related to stopping the pipeline with Ctrl-C, and the other one to downstream elements sending EOS. The commit message should explain that only the first case is addressed. > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> > Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message. > > src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 63d99571..8e197956 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -9,7 +9,6 @@ > /** > * \todo The following is a list of items that needs implementation in the GStreamer plugin > * - Implement GstElement::send_event > - * + Allowing application to send EOS > * + Allowing application to use FLUSH/FLUSH_STOP > * + Prevent the main thread from accessing streaming thread > * - Implement renegotiation (even if slow) > @@ -29,6 +28,7 @@ > > #include "gstlibcamerasrc.h" > > +#include <atomic> > #include <queue> > #include <vector> > > @@ -144,6 +144,8 @@ struct _GstLibcameraSrc { > gchar *camera_name; > controls::AfModeEnum auto_focus_mode = controls::AfModeManual; > > + std::atomic<GstEvent *> pending_eos; > + > GstLibcameraSrcState *state; > GstLibcameraAllocator *allocator; > GstFlowCombiner *flow_combiner; > @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data) > > bool doResume = false; > > + if (GstEvent *event = self->pending_eos.exchange(nullptr)) { I don't think we ever declare variables in if () statements in libcamera, you may want to write this as GstEvent *event = self->pending_eos.exchange(nullptr); if (event) { > + 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 +757,28 @@ 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: { > + if (GstEvent *old_event = self->pending_eos.exchange(event)) > + gst_event_unref(old_event); s/old_event/oldEvent/ gst_event_unref() includes a null check, so you could simplify this to GstEvent *oldEvent = self->pending_eos.exchange(event)) gst_event_unref(oldEvent); > + > + ret = TRUE; > + break; > + } > + default: > + gst_event_unref(event); > + break; > + } > + > + return ret; > +} > + > static void > gst_libcamera_src_finalize(GObject *object) > { > @@ -779,6 +811,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 +878,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",
Le vendredi 10 novembre 2023 à 18:29 +0200, Laurent Pinchart a écrit : > Hi Jaslo, > > Thank you for the patch. > > On Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote: > > Add a function for the send_event method which handles the GST_EVENT_EOS > > event. This function will set an atomic to the received event and push > > this EOS event to all source pads in the running task. > > > > Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a > > source element which enables it to receive EOS events sent to the > > (pipeline) bin containing it. > > This in turn enables libcamerasrc to for example receive EOS events from > > gst-launch-1.0 when using the -e flag. > > You should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=91 Yes please do. > > Does this patch fix the bug completely or partially ? Nicolas mentioned > in the review of v1 that there are two different EOS cases to consider, > one related to stopping the pipeline with Ctrl-C, and the other one to > downstream elements sending EOS. The commit message should explain that > only the first case is addressed. It covers it fully. I've identified problem in the reporter gstreamear pipeline (missing -e) here: https://bugs.libcamera.org/show_bug.cgi?id=91#c13 As submitter can't edit description in bz I expected this information to be hard to find and the bug to be useless for future improvement, so please close with this patch being merged. For the other EOS case, it very niche, but certainly best would be to add it to the todo, rather the bz in my opinion. regards, Nicolas > > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> > > Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > --- > > Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message. > > > > src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++- > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 63d99571..8e197956 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -9,7 +9,6 @@ > > /** > > * \todo The following is a list of items that needs implementation in the GStreamer plugin > > * - Implement GstElement::send_event > > - * + Allowing application to send EOS > > * + Allowing application to use FLUSH/FLUSH_STOP > > * + Prevent the main thread from accessing streaming thread > > * - Implement renegotiation (even if slow) > > @@ -29,6 +28,7 @@ > > > > #include "gstlibcamerasrc.h" > > > > +#include <atomic> > > #include <queue> > > #include <vector> > > > > @@ -144,6 +144,8 @@ struct _GstLibcameraSrc { > > gchar *camera_name; > > controls::AfModeEnum auto_focus_mode = controls::AfModeManual; > > > > + std::atomic<GstEvent *> pending_eos; > > + > > GstLibcameraSrcState *state; > > GstLibcameraAllocator *allocator; > > GstFlowCombiner *flow_combiner; > > @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > bool doResume = false; > > > > + if (GstEvent *event = self->pending_eos.exchange(nullptr)) { > > I don't think we ever declare variables in if () statements in > libcamera, you may want to write this as > > GstEvent *event = self->pending_eos.exchange(nullptr); > if (event) { > > > + 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 +757,28 @@ 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: { > > + if (GstEvent *old_event = self->pending_eos.exchange(event)) > > + gst_event_unref(old_event); > > s/old_event/oldEvent/ > > gst_event_unref() includes a null check, so you could simplify this to > > GstEvent *oldEvent = self->pending_eos.exchange(event)) > gst_event_unref(oldEvent); > > > + > > + ret = TRUE; > > + break; > > + } > > + default: > > + gst_event_unref(event); > > + break; > > + } > > + > > + return ret; > > +} > > + > > static void > > gst_libcamera_src_finalize(GObject *object) > > { > > @@ -779,6 +811,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 +878,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", >
Hi 2023. november 10., péntek 17:29 keltezéssel, Laurent Pinchart via libcamera-devel írta: > [...] > > + if (GstEvent *event = self->pending_eos.exchange(nullptr)) { > > I don't think we ever declare variables in if () statements in > libcamera, you may want to write this as > > GstEvent *event = self->pending_eos.exchange(nullptr); > if (event) { > [...] IIRC that is valid since at least C++11. Or is this more of a stylistic choice? Regards, Barnabás Pőcze
Le vendredi 10 novembre 2023 à 18:29 +0200, Laurent Pinchart via libcamera-devel a écrit : > Hi Jaslo, > > Thank you for the patch. > > On Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote: > > Add a function for the send_event method which handles the GST_EVENT_EOS > > event. This function will set an atomic to the received event and push > > this EOS event to all source pads in the running task. > > > > Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a > > source element which enables it to receive EOS events sent to the > > (pipeline) bin containing it. > > This in turn enables libcamerasrc to for example receive EOS events from > > gst-launch-1.0 when using the -e flag. > > You should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=91 > > Does this patch fix the bug completely or partially ? Nicolas mentioned > in the review of v1 that there are two different EOS cases to consider, > one related to stopping the pipeline with Ctrl-C, and the other one to > downstream elements sending EOS. The commit message should explain that > only the first case is addressed. > > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> > > Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > --- > > Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message. > > > > src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++- > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 63d99571..8e197956 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -9,7 +9,6 @@ > > /** > > * \todo The following is a list of items that needs implementation in the GStreamer plugin > > * - Implement GstElement::send_event > > - * + Allowing application to send EOS > > * + Allowing application to use FLUSH/FLUSH_STOP > > * + Prevent the main thread from accessing streaming thread > > * - Implement renegotiation (even if slow) > > @@ -29,6 +28,7 @@ > > > > #include "gstlibcamerasrc.h" > > > > +#include <atomic> > > #include <queue> > > #include <vector> > > > > @@ -144,6 +144,8 @@ struct _GstLibcameraSrc { > > gchar *camera_name; > > controls::AfModeEnum auto_focus_mode = controls::AfModeManual; > > > > + std::atomic<GstEvent *> pending_eos; > > + > > GstLibcameraSrcState *state; > > GstLibcameraAllocator *allocator; > > GstFlowCombiner *flow_combiner; > > @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > bool doResume = false; > > > > + if (GstEvent *event = self->pending_eos.exchange(nullptr)) { > > I don't think we ever declare variables in if () statements in > libcamera, you may want to write this as > > GstEvent *event = self->pending_eos.exchange(nullptr); > if (event) { Personally I like te in-scope declaration better. An you can improve it with: if (g_autoptr(GstEvent) event = ...) > > > + for (GstPad *srcpad : state->srcpads_) > > + gst_pad_push_event(srcpad, gst_event_ref(event)); > > + gst_event_unref(event); Witch let you drop this. > > + > > + return; > > + } > > + > > /* > > * Create and queue one request. If no buffers are available the > > * function returns -ENOBUFS, which we ignore here as that's not a > > @@ -747,6 +757,28 @@ 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: { > > + if (GstEvent *old_event = self->pending_eos.exchange(event)) > > + gst_event_unref(old_event); > > s/old_event/oldEvent/ > > gst_event_unref() includes a null check, so you could simplify this to Please don't, there is a run-time check, but it can be compiled out. You will always get an error message (and a crashed if you decided to drop run-time checks, which are just protections). > > GstEvent *oldEvent = self->pending_eos.exchange(event)) > gst_event_unref(oldEvent); > > > + > > + ret = TRUE; > > + break; > > + } > > + default: > > + gst_event_unref(event); > > + break; > > + } > > + > > + return ret; > > +} > > + > > static void > > gst_libcamera_src_finalize(GObject *object) > > { > > @@ -779,6 +811,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 +878,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", >
On Fri, Nov 10, 2023 at 08:55:38PM -0500, Nicolas Dufresne wrote: > Le vendredi 10 novembre 2023 à 18:29 +0200, Laurent Pinchart via libcamera-devel a écrit : > > On Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote: > > > Add a function for the send_event method which handles the GST_EVENT_EOS > > > event. This function will set an atomic to the received event and push > > > this EOS event to all source pads in the running task. > > > > > > Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a > > > source element which enables it to receive EOS events sent to the > > > (pipeline) bin containing it. > > > This in turn enables libcamerasrc to for example receive EOS events from > > > gst-launch-1.0 when using the -e flag. > > > > You should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=91 > > > > Does this patch fix the bug completely or partially ? Nicolas mentioned > > in the review of v1 that there are two different EOS cases to consider, > > one related to stopping the pipeline with Ctrl-C, and the other one to > > downstream elements sending EOS. The commit message should explain that > > only the first case is addressed. > > > > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> > > > Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > --- > > > Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message. > > > > > > src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++- > > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > > index 63d99571..8e197956 100644 > > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > @@ -9,7 +9,6 @@ > > > /** > > > * \todo The following is a list of items that needs implementation in the GStreamer plugin > > > * - Implement GstElement::send_event > > > - * + Allowing application to send EOS > > > * + Allowing application to use FLUSH/FLUSH_STOP > > > * + Prevent the main thread from accessing streaming thread > > > * - Implement renegotiation (even if slow) > > > @@ -29,6 +28,7 @@ > > > > > > #include "gstlibcamerasrc.h" > > > > > > +#include <atomic> > > > #include <queue> > > > #include <vector> > > > > > > @@ -144,6 +144,8 @@ struct _GstLibcameraSrc { > > > gchar *camera_name; > > > controls::AfModeEnum auto_focus_mode = controls::AfModeManual; > > > > > > + std::atomic<GstEvent *> pending_eos; > > > + > > > GstLibcameraSrcState *state; > > > GstLibcameraAllocator *allocator; > > > GstFlowCombiner *flow_combiner; > > > @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > > > bool doResume = false; > > > > > > + if (GstEvent *event = self->pending_eos.exchange(nullptr)) { > > > > I don't think we ever declare variables in if () statements in > > libcamera, you may want to write this as > > > > GstEvent *event = self->pending_eos.exchange(nullptr); > > if (event) { > > Personally I like te in-scope declaration better. An you can improve it with: > > if (g_autoptr(GstEvent) event = ...) > > > > + for (GstPad *srcpad : state->srcpads_) > > > + gst_pad_push_event(srcpad, gst_event_ref(event)); > > > + gst_event_unref(event); > > Witch let you drop this. I probably dislike assignments in if () statements due to them being frowned upon in the kernel coding style. I personally find them error-prone, and compilers agree with me I think, as they will warn asking if the author meant to compare instead of assign. When the variable is declared in the if () statement it's obviously not an issue anymore, so I'm OK with it here, even if I'm not sure to accept it more globally in the libcamera coding style. > > > + > > > + return; > > > + } > > > + > > > /* > > > * Create and queue one request. If no buffers are available the > > > * function returns -ENOBUFS, which we ignore here as that's not a > > > @@ -747,6 +757,28 @@ 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: { > > > + if (GstEvent *old_event = self->pending_eos.exchange(event)) > > > + gst_event_unref(old_event); > > > > s/old_event/oldEvent/ > > > > gst_event_unref() includes a null check, so you could simplify this to > > Please don't, there is a run-time check, but it can be compiled out. You will > always get an error message (and a crashed if you decided to drop run-time > checks, which are just protections). My bad, I didn't realize that. If we want to use a g_autoptr above, I would use one here too for consistency. > > GstEvent *oldEvent = self->pending_eos.exchange(event)) > > gst_event_unref(oldEvent); > > > > > + > > > + ret = TRUE; > > > + break; > > > + } > > > + default: > > > + gst_event_unref(event); > > > + break; > > > + } > > > + > > > + return ret; > > > +} > > > + > > > static void > > > gst_libcamera_src_finalize(GObject *object) > > > { > > > @@ -779,6 +811,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 +878,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",
On Fri, Nov 10, 2023 at 10:15:25PM +0000, Barnabás Pőcze wrote: > Hi > > > 2023. november 10., péntek 17:29 keltezéssel, Laurent Pinchart via libcamera-devel írta: > > > [...] > > > + if (GstEvent *event = self->pending_eos.exchange(nullptr)) { > > > > I don't think we ever declare variables in if () statements in > > libcamera, you may want to write this as > > > > > GstEvent *event = self->pending_eos.exchange(nullptr); > > if (event) { > > [...] > > IIRC that is valid since at least C++11. Or is this more of a stylistic choice? It's a coding style choice. I really dislike assignments in if () statements, such as GstEvent *event; if (event = self->pending_eos.exchange(nullptr)) because it's very error-prone (easy to confuse the assignement and comparison operator during review). The kernel coding style, which influenced me a lot, doesn't like it either, and compilers generate a warning these days. The error-prone argument is obviously moot if declaring the variable in the if () statement, as if (GstEvent *event = self->pending_eos.exchange(nullptr)) would compile and if (GstEvent *event == self->pending_eos.exchange(nullptr)) wouldn't (hopefully, I haven't tried). It still "feels" weird due to the assignment-in-if-statements explanation, so maybe we can accept this in the coding style. I'm OK doing so in the implementation of the GStreamer element, even if we don't make it a standard practice in the libcamera core.
Le lundi 13 novembre 2023 à 01:18 +0200, Laurent Pinchart via libcamera-devel a écrit : > On Fri, Nov 10, 2023 at 08:55:38PM -0500, Nicolas Dufresne wrote: > > Le vendredi 10 novembre 2023 à 18:29 +0200, Laurent Pinchart via libcamera-devel a écrit : > > > On Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote: > > > > Add a function for the send_event method which handles the GST_EVENT_EOS > > > > event. This function will set an atomic to the received event and push > > > > this EOS event to all source pads in the running task. > > > > > > > > Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a > > > > source element which enables it to receive EOS events sent to the > > > > (pipeline) bin containing it. > > > > This in turn enables libcamerasrc to for example receive EOS events from > > > > gst-launch-1.0 when using the -e flag. > > > > > > You should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=91 > > > > > > Does this patch fix the bug completely or partially ? Nicolas mentioned > > > in the review of v1 that there are two different EOS cases to consider, > > > one related to stopping the pipeline with Ctrl-C, and the other one to > > > downstream elements sending EOS. The commit message should explain that > > > only the first case is addressed. > > > > > > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> > > > > Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > --- > > > > Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message. > > > > > > > > src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++- > > > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > > > index 63d99571..8e197956 100644 > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > > @@ -9,7 +9,6 @@ > > > > /** > > > > * \todo The following is a list of items that needs implementation in the GStreamer plugin > > > > * - Implement GstElement::send_event > > > > - * + Allowing application to send EOS > > > > * + Allowing application to use FLUSH/FLUSH_STOP > > > > * + Prevent the main thread from accessing streaming thread > > > > * - Implement renegotiation (even if slow) > > > > @@ -29,6 +28,7 @@ > > > > > > > > #include "gstlibcamerasrc.h" > > > > > > > > +#include <atomic> > > > > #include <queue> > > > > #include <vector> > > > > > > > > @@ -144,6 +144,8 @@ struct _GstLibcameraSrc { > > > > gchar *camera_name; > > > > controls::AfModeEnum auto_focus_mode = controls::AfModeManual; > > > > > > > > + std::atomic<GstEvent *> pending_eos; > > > > + > > > > GstLibcameraSrcState *state; > > > > GstLibcameraAllocator *allocator; > > > > GstFlowCombiner *flow_combiner; > > > > @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > > > > > bool doResume = false; > > > > > > > > + if (GstEvent *event = self->pending_eos.exchange(nullptr)) { > > > > > > I don't think we ever declare variables in if () statements in > > > libcamera, you may want to write this as > > > > > > GstEvent *event = self->pending_eos.exchange(nullptr); > > > if (event) { > > > > Personally I like te in-scope declaration better. An you can improve it with: > > > > if (g_autoptr(GstEvent) event = ...) > > > > > > + for (GstPad *srcpad : state->srcpads_) > > > > + gst_pad_push_event(srcpad, gst_event_ref(event)); > > > > + gst_event_unref(event); > > > > Witch let you drop this. > > I probably dislike assignments in if () statements due to them being > frowned upon in the kernel coding style. I personally find them > error-prone, and compilers agree with me I think, as they will warn > asking if the author meant to compare instead of assign. When the > variable is declared in the if () statement it's obviously not an issue > anymore, so I'm OK with it here, even if I'm not sure to accept it more > globally in the libcamera coding style. Note that the use of g_autoptr can happen with traditional variable declaration, something like this: g_autoptr(GstEvent) event = self->pending_eos.exchange(nullptr); if (event) .... /* end of scope will lead to an unref */ The delta is mostly that the unref happens later, in such a short function, the benefit is theoretical, since the following line returns. > > > > > + > > > > + return; > > > > + } > > > > + > > > > /* > > > > * Create and queue one request. If no buffers are available the > > > > * function returns -ENOBUFS, which we ignore here as that's not a > > > > @@ -747,6 +757,28 @@ 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: { > > > > + if (GstEvent *old_event = self->pending_eos.exchange(event)) > > > > + gst_event_unref(old_event); > > > > > > s/old_event/oldEvent/ > > > > > > gst_event_unref() includes a null check, so you could simplify this to > > > > Please don't, there is a run-time check, but it can be compiled out. You will > > always get an error message (and a crashed if you decided to drop run-time > > checks, which are just protections). > > My bad, I didn't realize that. > > If we want to use a g_autoptr above, I would use one here too for > consistency. I agree. > > > > GstEvent *oldEvent = self->pending_eos.exchange(event)) > > > gst_event_unref(oldEvent); > > > > > > > + > > > > + ret = TRUE; > > > > + break; > > > > + } > > > > + default: > > > > + gst_event_unref(event); > > > > + break; > > > > + } > > > > + > > > > + return ret; > > > > +} > > > > + > > > > static void > > > > gst_libcamera_src_finalize(GObject *object) > > > > { > > > > @@ -779,6 +811,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 +878,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", >
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 63d99571..8e197956 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -9,7 +9,6 @@ /** * \todo The following is a list of items that needs implementation in the GStreamer plugin * - Implement GstElement::send_event - * + Allowing application to send EOS * + Allowing application to use FLUSH/FLUSH_STOP * + Prevent the main thread from accessing streaming thread * - Implement renegotiation (even if slow) @@ -29,6 +28,7 @@ #include "gstlibcamerasrc.h" +#include <atomic> #include <queue> #include <vector> @@ -144,6 +144,8 @@ struct _GstLibcameraSrc { gchar *camera_name; controls::AfModeEnum auto_focus_mode = controls::AfModeManual; + std::atomic<GstEvent *> pending_eos; + GstLibcameraSrcState *state; GstLibcameraAllocator *allocator; GstFlowCombiner *flow_combiner; @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data) bool doResume = false; + if (GstEvent *event = self->pending_eos.exchange(nullptr)) { + 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 +757,28 @@ 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: { + if (GstEvent *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 +811,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 +878,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",