[libcamera-devel] [PATCH] gstreamer: Implement EOS handling
Nicolas Dufresne
nicolas.dufresne at collabora.com
Thu Nov 9 16:07:38 CET 2023
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 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