[libcamera-devel] [PATCH] gstreamer: Implement EOS handling
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Nov 9 16:05:35 CET 2023
+Nicolas
Quoting Jaslo Ziska (2023-11-09 14:20:22)
> Hi Kieran,
>
> Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> > Quoting Jaslo Ziska via libcamera-devel (2023-11-09 12:51:15)
> >> Hi,
> >>
> >> Umang Jain <umang.jain at ideasonboard.com> writes:
> >> > Hi Jalso
> >> >
> >> > On 11/6/23 10:22 PM, Jaslo Ziska via libcamera-devel wrote:
> >> >> ---
> >> >> Hi,
> >> >>
> >> >> I recently implemented basic EOS handling for the
> >> >> libcamerasrc
> >> >> gstreamer element.
> >> >
> >> > Ah thanks, I remember we do track a bug report here:
> >> > https://bugs.libcamera.org/show_bug.cgi?id=91
> >>
> >> Ah yes, I remember that I stumbled upon this when I was first
> >> investigating this, I will update the bug report.
> >>
> >> >>
> >> >> I basically looked at how GstBaseSrc does it and tried to do
> >> >> it
> >> >> similarly.
> >> >>
> >> >> I have no idea whether the locking is correct or if this is
> >> >> thread safe but so far it worked without any issues for my
> >> >> purposes.
> >> >>
> >> >> You can also now run the following command and receive a
> >> >> working video file:
> >> >>
> >> >> gst-launch-1.0 -e libcamerasrc ! videoconvert ! x264enc !
> >> >> h264parse ! mp4mux ! filesink location=test.mp4
> >> >
> >> > Good to see that the diff is tested :-D
> >>
> >> Speaking of testing, I just ran the test suite and, except for
> >> the
> >> multi_stream_test (which was skipped) the other tests
> >> succeeded.
> >>
> >> >>
> >> >> Looking forward for feedback!
> >> >
> >> > It looks logical and on the right track to me atleast. But
> >> > there
> >> > might be other implications regarding the threading and
> >> > locking mechanisms.
> >>
> >> Thanks! That is what I worry about too, maybe someone can
> >> review
> >> this.
> >>
> >> >> Cheers,
> >> >>
> >> >> Jaslo
> >> >>
> >> >> src/gstreamer/gstlibcamerasrc.cpp | 47
> >> >> +++++++++++++++++++++++++++++++
> >> >> 1 file changed, 47 insertions(+)
> >> >>
> >> >> diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> >> >> b/src/gstreamer/gstlibcamerasrc.cpp
> >> >> index 63d99571..a4681e1e 100644
> >> >> --- a/src/gstreamer/gstlibcamerasrc.cpp
> >> >> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> >> >> @@ -144,6 +144,9 @@ struct _GstLibcameraSrc {
> >> >> gchar *camera_name;
> >> >> controls::AfModeEnum auto_focus_mode =
> >> >> controls::AfModeManual;
> >> >>
> >> >> + GstEvent *pending_eos;
> >> >> + int has_pending_eos;
> >> >> +
> >> >> GstLibcameraSrcState *state;
> >> >> GstLibcameraAllocator *allocator;
> >> >> GstFlowCombiner *flow_combiner;
> >> >> @@ -397,6 +400,21 @@ gst_libcamera_src_task_run(gpointer
> >> >> user_data)
> >> >>
> >> >> bool doResume = false;
> >> >>
> >> >> + if (g_atomic_int_get(&self->has_pending_eos)) {
> >> >> + g_atomic_int_set(&self->has_pending_eos,
> >> >> FALSE);
> >> >> +
> >> >> + GST_OBJECT_LOCK(self);
> >> >> + GstEvent *event = self->pending_eos;
> >> >> + self->pending_eos = NULL;
> >> >> + GST_OBJECT_UNLOCK(self);
> >> >> +
> >> >> + for (GstPad *srcpad : state->srcpads_)
> >> >> + gst_pad_push_event(srcpad,
> >> >> gst_event_ref(event));
> >> >> + gst_event_unref(event);
> >> >> +
> >> >> + return;
> >> >> + }
> >> >> +
> >> >> /*
> >> >> * Create and queue one request. If no buffers are
> >> >> available the
> >> >> * function returns -ENOBUFS, which we ignore here as
> >> >> that's not a
> >> >> @@ -747,6 +765,32 @@
> >> >> gst_libcamera_src_change_state(GstElement
> >> >> *element, GstStateChange transition)
> >> >> return ret;
> >> >> }
> >> >>
> >> >> +static gboolean
> >> >> +gst_libcamera_src_send_event(GstElement *element, GstEvent
> >> >> *event)
> >> >> +{
> >> >> + GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);
> >> >> + gboolean ret = FALSE;
> >> >> +
> >> >> + switch (GST_EVENT_TYPE(event)) {
> >> >> + case GST_EVENT_EOS: {
> >> >> + GST_OBJECT_LOCK(self);
> >> >> + g_atomic_int_set(&self->has_pending_eos, TRUE);
> >> >> + if (self->pending_eos)
> >> >> + gst_event_unref(self->pending_eos);
> >> >> + self->pending_eos = event;
> >> >> + GST_OBJECT_UNLOCK(self);
> >> >> +
> >> >> + ret = TRUE;
> >> >> + break;
> >> >> + }
> >> >> + default:
> >> >> + gst_event_unref(event);
> >> >> + break;
> >> >> + }
> >> >> +
> >> >> + return ret;
> >> >> +}
> >> >> +
> >> >> static void
> >> >> gst_libcamera_src_finalize(GObject *object)
> >> >> {
> >> >> @@ -779,6 +823,8 @@ gst_libcamera_src_init(GstLibcameraSrc
> >> >> *self)
> >> >> state->srcpads_.push_back(gst_pad_new_from_template(templ,
> >> >> "src"));
> >> >> gst_element_add_pad(GST_ELEMENT(self),
> >> >> state->srcpads_.back());
> >> >>
> >> >> + GST_OBJECT_FLAG_SET(self, GST_ELEMENT_FLAG_SOURCE);
> >> >> +
> >> >
> >> > I always thought libcamerasrc was inheriting from GstBaseSrc,
> >> > but in fact it's not.
> >> > So, setting the GST_ELEMENT_FLAG_SOURCE makes sense here.
> >>
> >> This flag is actually required to receive the eos signal when
> >> it
> >> is send to the pipeline bin and not the element directly. Took
> >> me
> >> a while to figure out.
> >>
> >> > Apart from a missing commit mesasge, the patch looks good to
> >> > me.
> >> > I'll put this on my radar soon for testing it
> >> > rigorously.
> >>
> >> Oh yeah, a commit message would indeed be nice, completely
> >> forgot
> >> that, thanks :)
> >
> > 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
More information about the libcamera-devel
mailing list