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

[Laurent Pinchart]

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

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.

> > > > ?
> > > > 
> > > > > [...]
-- 
Regards,

Laurent Pinchart