[libcamera-devel] [PATCH] gstreamer: Implement EOS handling

[Kieran Bingham]

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?

--
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