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

19213 diff mbox series

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@ziska.de>
Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
Thanks for all the advise on v2 everyone!

In this revision I explain which EOS events are handled in the commit message a little bit more and link to the bug report.

As recommended, g_autoptr is now used. I decided to declare g_autoptr(event) outside of the if-statement for now, I was not sure what the consensus on this is at the moment.

Regards,

Jaslo

 src/gstreamer/gstlibcamerasrc.cpp | 36 ++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

--
2.42.1

Le mardi 14 novembre 2023 à 13:18 +0100, Jaslo Ziska a écrit :
> 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@ziska.de>
> Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
> Thanks for all the advise on v2 everyone!
> 
> In this revision I explain which EOS events are handled in the commit message a little bit more and link to the bug report.
> 
> As recommended, g_autoptr is now used. I decided to declare g_autoptr(event) outside of the if-statement for now, I was not sure what the consensus on this is at the moment.
> 
> Regards,
> 
> Jaslo
> 
>  src/gstreamer/gstlibcamerasrc.cpp | 36 ++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 63d99571..767017db 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -9,7 +9,6 @@
>  /**
>   * \todo The following is a list of items that needs implementation in the GStreamer plugin
>   *  - Implement GstElement::send_event
> - *    + Allowing application to send EOS
>   *    + Allowing application to use FLUSH/FLUSH_STOP
>   *    + Prevent the main thread from accessing streaming thread
>   *  - Implement renegotiation (even if slow)
> @@ -29,6 +28,7 @@
> 
>  #include "gstlibcamerasrc.h"
> 
> +#include <atomic>
>  #include <queue>
>  #include <vector>
> 
> @@ -144,6 +144,8 @@ struct _GstLibcameraSrc {
>  	gchar *camera_name;
>  	controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
> 
> +	std::atomic<GstEvent *> pending_eos;
> +
>  	GstLibcameraSrcState *state;
>  	GstLibcameraAllocator *allocator;
>  	GstFlowCombiner *flow_combiner;
> @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data)
> 
>  	bool doResume = false;
> 
> +	g_autoptr(GstEvent) event = self->pending_eos.exchange(nullptr);
> +	if (event) {
> +		for (GstPad *srcpad : state->srcpads_)
> +			gst_pad_push_event(srcpad, gst_event_ref(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 +757,27 @@ 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: {
> +		g_autoptr(GstEvent) oldEvent = self->pending_eos.exchange(event);
> +
> +		ret = TRUE;
> +		break;
> +	}
> +	default:
> +		gst_event_unref(event);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  static void
>  gst_libcamera_src_finalize(GObject *object)
>  {
> @@ -779,6 +810,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);
> +
>  	/* C-style friend. */
>  	state->src_ = self;
>  	self->state = state;
> @@ -844,6 +877,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

Hi Jalso,

Thank you for the patch.

On 11/14/23 5:48 PM, 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@ziska.de>
> Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

I will test this on my RPi 4b and merge this today.

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>


> ---
> Thanks for all the advise on v2 everyone!
>
> In this revision I explain which EOS events are handled in the commit message a little bit more and link to the bug report.
>
> As recommended, g_autoptr is now used. I decided to declare g_autoptr(event) outside of the if-statement for now, I was not sure what the consensus on this is at the moment.
>
> Regards,
>
> Jaslo
>
>   src/gstreamer/gstlibcamerasrc.cpp | 36 ++++++++++++++++++++++++++++++-
>   1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 63d99571..767017db 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -9,7 +9,6 @@
>   /**
>    * \todo The following is a list of items that needs implementation in the GStreamer plugin
>    *  - Implement GstElement::send_event
> - *    + Allowing application to send EOS
>    *    + Allowing application to use FLUSH/FLUSH_STOP
>    *    + Prevent the main thread from accessing streaming thread
>    *  - Implement renegotiation (even if slow)
> @@ -29,6 +28,7 @@
>
>   #include "gstlibcamerasrc.h"
>
> +#include <atomic>
>   #include <queue>
>   #include <vector>
>
> @@ -144,6 +144,8 @@ struct _GstLibcameraSrc {
>   	gchar *camera_name;
>   	controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
>
> +	std::atomic<GstEvent *> pending_eos;
> +
>   	GstLibcameraSrcState *state;
>   	GstLibcameraAllocator *allocator;
>   	GstFlowCombiner *flow_combiner;
> @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data)
>
>   	bool doResume = false;
>
> +	g_autoptr(GstEvent) event = self->pending_eos.exchange(nullptr);
> +	if (event) {
> +		for (GstPad *srcpad : state->srcpads_)
> +			gst_pad_push_event(srcpad, gst_event_ref(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 +757,27 @@ 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: {
> +		g_autoptr(GstEvent) oldEvent = self->pending_eos.exchange(event);
> +
> +		ret = TRUE;
> +		break;
> +	}
> +	default:
> +		gst_event_unref(event);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>   static void
>   gst_libcamera_src_finalize(GObject *object)
>   {
> @@ -779,6 +810,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);
> +
>   	/* C-style friend. */
>   	state->src_ = self;
>   	self->state = state;
> @@ -844,6 +877,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

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@ziska.de>
> Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
> Thanks for all the advise on v2 everyone!
> 
> In this revision I explain which EOS events are handled in the commit message a little bit more and link to the bug report.
> 
> As recommended, g_autoptr is now used. I decided to declare g_autoptr(event) outside of the if-statement for now, I was not sure what the consensus on this is at the moment.
> 
> Regards,
> 
> Jaslo
> 
>  src/gstreamer/gstlibcamerasrc.cpp | 36 ++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 63d99571..767017db 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -9,7 +9,6 @@
>  /**
>   * \todo The following is a list of items that needs implementation in the GStreamer plugin
>   *  - Implement GstElement::send_event
> - *    + Allowing application to send EOS
>   *    + Allowing application to use FLUSH/FLUSH_STOP
>   *    + Prevent the main thread from accessing streaming thread
>   *  - Implement renegotiation (even if slow)
> @@ -29,6 +28,7 @@
> 
>  #include "gstlibcamerasrc.h"
> 
> +#include <atomic>
>  #include <queue>
>  #include <vector>
> 
> @@ -144,6 +144,8 @@ struct _GstLibcameraSrc {
>  	gchar *camera_name;
>  	controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
> 
> +	std::atomic<GstEvent *> pending_eos;
> +
>  	GstLibcameraSrcState *state;
>  	GstLibcameraAllocator *allocator;
>  	GstFlowCombiner *flow_combiner;
> @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data)
> 
>  	bool doResume = false;
> 
> +	g_autoptr(GstEvent) event = self->pending_eos.exchange(nullptr);
> +	if (event) {
> +		for (GstPad *srcpad : state->srcpads_)
> +			gst_pad_push_event(srcpad, gst_event_ref(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 +757,27 @@ 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: {
> +		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 ?

> +
> +		ret = TRUE;
> +		break;
> +	}
> +	default:
> +		gst_event_unref(event);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  static void
>  gst_libcamera_src_finalize(GObject *object)
>  {
> @@ -779,6 +810,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);
> +
>  	/* C-style friend. */
>  	state->src_ = self;
>  	self->state = state;
> @@ -844,6 +877,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",

Hi,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> 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@ziska.de>
>> Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>> ---
>> Thanks for all the advise on v2 everyone!
>>
>> In this revision I explain which EOS events are handled in the 
>> commit message a little bit more and link to the bug report.
>>
>> As recommended, g_autoptr is now used. I decided to declare 
>> g_autoptr(event) outside of the if-statement for now, I was not 
>> sure what the consensus on this is at the moment.
>>
>> Regards,
>>
>> Jaslo
>>
>>  src/gstreamer/gstlibcamerasrc.cpp | 36 
>>  ++++++++++++++++++++++++++++++-
>>  1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp 
>> b/src/gstreamer/gstlibcamerasrc.cpp
>> index 63d99571..767017db 100644
>> --- a/src/gstreamer/gstlibcamerasrc.cpp
>> +++ b/src/gstreamer/gstlibcamerasrc.cpp
>> @@ -9,7 +9,6 @@
>>  /**
>>   * \todo The following is a list of items that needs 
>>   implementation in the GStreamer plugin
>>   *  - Implement GstElement::send_event
>> - *    + Allowing application to send EOS
>>   *    + Allowing application to use FLUSH/FLUSH_STOP
>>   *    + Prevent the main thread from accessing streaming 
>>   thread
>>   *  - Implement renegotiation (even if slow)
>> @@ -29,6 +28,7 @@
>>
>>  #include "gstlibcamerasrc.h"
>>
>> +#include <atomic>
>>  #include <queue>
>>  #include <vector>
>>
>> @@ -144,6 +144,8 @@ struct _GstLibcameraSrc {
>>  	gchar *camera_name;
>>  	controls::AfModeEnum auto_focus_mode = 
>>  controls::AfModeManual;
>>
>> +	std::atomic<GstEvent *> pending_eos;
>> +
>>  	GstLibcameraSrcState *state;
>>  	GstLibcameraAllocator *allocator;
>>  	GstFlowCombiner *flow_combiner;
>> @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer 
>> user_data)
>>
>>  	bool doResume = false;
>>
>> +	g_autoptr(GstEvent) event = 
>> self->pending_eos.exchange(nullptr);
>> +	if (event) {
>> +		for (GstPad *srcpad : state->srcpads_)
>> +			gst_pad_push_event(srcpad, gst_event_ref(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 +757,27 @@ 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: {
>> +		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 ?

It's ok, I can do it right now. Should the fix be a new commit or 
alter the old one? Also do you have any preferred way of fixing 
this?

Jaslo

>> +
>> +		ret = TRUE;
>> +		break;
>> +	}
>> +	default:
>> +		gst_event_unref(event);
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static void
>>  gst_libcamera_src_finalize(GObject *object)
>>  {
>> @@ -779,6 +810,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);
>> +
>>  	/* C-style friend. */
>>  	state->src_ = self;
>>  	self->state = state;
>> @@ -844,6 +877,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",

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@ziska.de>
> > Acked-by: Nicolas Dufresne <nicolas.dufresne@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.

--
Kieran

Hi


2023. november 22., szerda 18:08 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel@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@ziska.de
> > > Acked-by: Nicolas Dufresne nicolas.dufresne@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);

?

> [...]

Regards,
Barnabás Pőcze

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@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@ziska.de
> > > > Acked-by: Nicolas Dufresne nicolas.dufresne@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.

Nicolas

> 
> ?
> 
> > [...]
> 
> Regards,
> Barnabás Pőcze

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@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@ziska.de
> > > > > Acked-by: Nicolas Dufresne nicolas.dufresne@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.

This patch was merged without going through our compliler matrix.
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

--
Kieran


> 
> Nicolas
> 
> > 
> > ?
> > 
> > > [...]
> > 
> > Regards,
> > Barnabás Pőcze
>

Le mercredi 22 novembre 2023 à 20:58 +0000, Kieran Bingham a écrit :
> 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@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@ziska.de
> > > > > > Acked-by: Nicolas Dufresne nicolas.dufresne@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.
> 
> This patch was merged without going through our compliler matrix.
> 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

As a maintainer, it feels like if would have been simpler to just apply one of
the two suggestions we made.

Nicolas

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

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@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@ziska.de
>>>>>> Acked-by: Nicolas Dufresne nicolas.dufresne@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

Hi Nicolas,

On Wed, Nov 22, 2023 at 03:59:45PM -0500, Nicolas Dufresne wrote:
> Le mercredi 22 novembre 2023 à 20:58 +0000, Kieran Bingham a écrit :
> > Quoting Nicolas Dufresne (2023-11-22 19:21:50)
> > > Le mercredi 22 novembre 2023 à 17:24 +0000, Barnabás Pőcze a écrit :
> > > > 2023. november 22., szerda 18:08 keltezéssel, Kieran Bingham via libcamera-devel írta:
> > > > > Quoting Laurent Pinchart via libcamera-devel (2023-11-22 16:38:49)
> > > > > > 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@ziska.de
> > > > > > > Acked-by: Nicolas Dufresne nicolas.dufresne@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.

No drama here :-) This broke the build as libcamera is compiled with
-Werror, thus breaking any script that checks further commits before
pushing them.

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

Possibly, yes. I've reported issues to gcc in the past to fix false
positives, I'll let someone else handle clang :-)

> > Sorry, it wasn't supposed to be dramatic. A real fix is fine too, but a
> > revert and re-apply fixed isnt hard either.
> > 
> > This patch was merged without going through our compliler matrix.
> > 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

On a side note, we should have automated compilation testing on the
server side before the end of the year, likely based on the fd.o gitlab
instance.

> As a maintainer, it feels like if would have been simpler to just apply one of
> the two suggestions we made.

As I just replied to Umang's series with a proposed fix, I don't see a
need to revert the commit here. I proposed reverting as an option to not
put any pressure on anyone for submitting an urgent fix. Given that the
fix is here, and nothing has been reverted so far, fixing on top is
best.

I like your suggestion best, it's more explicit. The [[maybe_unused]]
may work, but I don't know what optimization the compiler may try to
make that would have side effects there.

> > > > ?
> > > > 
> > > > > [...]

Le jeudi 23 novembre 2023 à 10:14 +0530, Umang Jain a écrit :
> 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@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@ziska.de
> > > > > > > Acked-by: Nicolas Dufresne nicolas.dufresne@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.

I'll start being annoying, but why all this complexity needed ? Why not just
make a patch that fix the previous one, you add add "fixes" tag to make it
clear.

Nicolas

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

Quoting Nicolas Dufresne (2023-11-23 15:49:29)
> Le jeudi 23 novembre 2023 à 10:14 +0530, Umang Jain a écrit :
> > Hi,
> > > > > 2023. november 22., szerda 18:08 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> írta:
> > > > > > 
> > > > > > 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.
> 
> I'll start being annoying, but why all this complexity needed ? Why not just
> make a patch that fix the previous one, you add add "fixes" tag to make it
> clear.

Yeah - mailing lists are rubbish ... if only this was all in a single
thread or context and everything was in one place, then the final result
here would have been more obvious...

https://patchwork.libcamera.org/patch/19230/

--
Kieran