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

[Umang Jain]

Hi,

On 11/23/23 2:28 AM, Kieran Bingham via libcamera-devel wrote:
> Quoting Nicolas Dufresne (2023-11-22 19:21:50)
>> Le mercredi 22 novembre 2023 à 17:24 +0000, Barnabás Pőcze a écrit :
>>> Hi
>>>
>>>
>>> 2023. november 22., szerda 18:08 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel at lists.libcamera.org> írta:
>>>
>>>> Quoting Laurent Pinchart via libcamera-devel (2023-11-22 16:38:49)
>>>>
>>>>> Hello,
>>>>>
>>>>> On Tue, Nov 14, 2023 at 01:18:57PM +0100, Jaslo Ziska via libcamera-devel wrote:
>>>>>
>>>>>> This commit implements EOS handling for events sent to the libcamerasrc
>>>>>> element by the send_event method (which can happen when pressing
>>>>>> Ctrl-C while running gst-launch-1.0 -e, see below). EOS events from
>>>>>> downstream elements returning GST_FLOW_EOS are not considered here.
>>>>>>
>>>>>> To archive this add a function for the send_event method which handles
>>>>>> the GST_EVENT_EOS event. This function will set an atomic to the
>>>>>> received event and push this EOS event to all source pads in the running
>>>>>> task.
>>>>>>
>>>>>> Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a
>>>>>> source element which enables it to receive EOS events sent to the
>>>>>> (pipeline) bin containing it.
>>>>>> This in turn enables libcamerasrc to for example receive EOS events from
>>>>>> gst-launch-1.0 when using the -e flag.
>>>>>>
>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=91
>>>>>> Signed-off-by: Jaslo Ziska jaslo at ziska.de
>>>>>> Acked-by: Nicolas Dufresne nicolas.dufresne at collabora.com
>>>>>> ---
>>>> <snip>
>>>>
>>>>>> +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: {
>>>>>> + g_autoptr(GstEvent) oldEvent = self->pending_eos.exchange(event);
>>>>> I'm afraid this causes a compilation breakage with clang :-(
>>>>>
>>>>> ../../src/gstreamer/gstlibcamerasrc.cpp:768:23: error: unused variable 'oldEvent' [-Werror,-Wunused-variable]
>>>>> g_autoptr(GstEvent) oldEvent = self->pending_eos.exchange(event);
>>>>> ^
>>>>>
>>>>> The patch has been merged, can you propose a fix quickly, or should I
>>>>> revert to give you more time ?
>>>>
>>>> I'm afraid with a broken build we're blocked on merging more patches -
>>>> of which I'm now trying to do - so I'd probably ask if we can revert
>>>> this until it's fixed please.
>>>> [...]
>>> [[maybe_unused]] g_autoptr(GstEvent) oldEvent = self->pending_eos.exchange(event);
>> Or just, like the original patch did.
>>
>>     GstEvent *oldEvent = self->pending_eos.exchange(event);
>>     gst_clear_event(&oldEvent);
>>
>> Not sure why this blocks anything, sounds a bit dramatic handling of a build
>> warning. In this case, its quite obvious that this is a false positive in clang
>> warning machinery, you will hit more of these with clang in my experience.
> Sorry, it wasn't supposed to be dramatic. A real fix is fine too, but a
> revert and re-apply fixed isnt hard either.

I will post a revert and a re-appllied patch for review.
>
> This patch was merged without going through our compliler matrix.

This was my mistake. Apologies.

Can you please collect and run through the compiler matrix please and 
handle the merge along with other patches?

> On my pc, I can't merge if the compiler matrix fails, but thats not set
> up for everyone. I've got four patches now lined up to merge for
> Raspberry Pi and blocked. I can wait ... if we have a fix

Yes, sorry :-/
> --
> Kieran
>
>
>> Nicolas
>>
>>> ?
>>>
>>>> [...]
>>> Regards,
>>> Barnabás Pőcze