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

[Jaslo Ziska]

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?

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