[libcamera-devel] [PATCH] gstreamer: Implement EOS handling
Jaslo Ziska
jaslo at ziska.de
Fri Nov 10 12:00:09 CET 2023
Hi Nicolas,
nicolas at 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 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?
>>
>> 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
More information about the libcamera-devel
mailing list