[{"id":28077,"web_url":"https://patchwork.libcamera.org/comment/28077/","msgid":"<20231110162936.GD7466@pendragon.ideasonboard.com>","date":"2023-11-10T16:29:36","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jaslo,\n\nThank you for the patch.\n\nOn Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote:\n> Add a function for the send_event method which handles the GST_EVENT_EOS\n> event. This function will set an atomic to the received event and push\n> this EOS event to all source pads in the running task.\n> \n> Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a\n> source element which enables it to receive EOS events sent to the\n> (pipeline) bin containing it.\n> This in turn enables libcamerasrc to for example receive EOS events from\n> gst-launch-1.0 when using the -e flag.\n\nYou should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with\n\nBug: https://bugs.libcamera.org/show_bug.cgi?id=91\n\nDoes this patch fix the bug completely or partially ? Nicolas mentioned\nin the review of v1 that there are two different EOS cases to consider,\none related to stopping the pipeline with Ctrl-C, and the other one to\ndownstream elements sending EOS. The commit message should explain that\nonly the first case is addressed.\n\n> Signed-off-by: Jaslo Ziska <jaslo@ziska.de>\n> Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> ---\n> Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message.\n> \n>  src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++-\n>  1 file changed, 36 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 63d99571..8e197956 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -9,7 +9,6 @@\n>  /**\n>   * \\todo The following is a list of items that needs implementation in the GStreamer plugin\n>   *  - Implement GstElement::send_event\n> - *    + Allowing application to send EOS\n>   *    + Allowing application to use FLUSH/FLUSH_STOP\n>   *    + Prevent the main thread from accessing streaming thread\n>   *  - Implement renegotiation (even if slow)\n> @@ -29,6 +28,7 @@\n> \n>  #include \"gstlibcamerasrc.h\"\n> \n> +#include <atomic>\n>  #include <queue>\n>  #include <vector>\n> \n> @@ -144,6 +144,8 @@ struct _GstLibcameraSrc {\n>  \tgchar *camera_name;\n>  \tcontrols::AfModeEnum auto_focus_mode = controls::AfModeManual;\n> \n> +\tstd::atomic<GstEvent *> pending_eos;\n> +\n>  \tGstLibcameraSrcState *state;\n>  \tGstLibcameraAllocator *allocator;\n>  \tGstFlowCombiner *flow_combiner;\n> @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \n>  \tbool doResume = false;\n> \n> +\tif (GstEvent *event = self->pending_eos.exchange(nullptr)) {\n\nI don't think we ever declare variables in if () statements in\nlibcamera, you may want to write this as\n\n\tGstEvent *event = self->pending_eos.exchange(nullptr);\n\tif (event) {\n\n> +\t\tfor (GstPad *srcpad : state->srcpads_)\n> +\t\t\tgst_pad_push_event(srcpad, gst_event_ref(event));\n> +\t\tgst_event_unref(event);\n> +\n> +\t\treturn;\n> +\t}\n> +\n>  \t/*\n>  \t * Create and queue one request. If no buffers are available the\n>  \t * function returns -ENOBUFS, which we ignore here as that's not a\n> @@ -747,6 +757,28 @@ gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n>  \treturn ret;\n>  }\n> \n> +static gboolean\n> +gst_libcamera_src_send_event(GstElement *element, GstEvent *event)\n> +{\n> +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);\n> +\tgboolean ret = FALSE;\n> +\n> +\tswitch (GST_EVENT_TYPE(event)) {\n> +\tcase GST_EVENT_EOS: {\n> +\t\tif (GstEvent *old_event = self->pending_eos.exchange(event))\n> +\t\t\tgst_event_unref(old_event);\n\ns/old_event/oldEvent/\n\ngst_event_unref() includes a null check, so you could simplify this to\n\n\t\tGstEvent *oldEvent = self->pending_eos.exchange(event))\n\t\tgst_event_unref(oldEvent);\n\n> +\n> +\t\tret = TRUE;\n> +\t\tbreak;\n> +\t}\n> +\tdefault:\n> +\t\tgst_event_unref(event);\n> +\t\tbreak;\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n>  static void\n>  gst_libcamera_src_finalize(GObject *object)\n>  {\n> @@ -779,6 +811,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n>  \tstate->srcpads_.push_back(gst_pad_new_from_template(templ, \"src\"));\n>  \tgst_element_add_pad(GST_ELEMENT(self), state->srcpads_.back());\n> \n> +\tGST_OBJECT_FLAG_SET(self, GST_ELEMENT_FLAG_SOURCE);\n> +\n>  \t/* C-style friend. */\n>  \tstate->src_ = self;\n>  \tself->state = state;\n> @@ -844,6 +878,7 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n>  \telement_class->request_new_pad = gst_libcamera_src_request_new_pad;\n>  \telement_class->release_pad = gst_libcamera_src_release_pad;\n>  \telement_class->change_state = gst_libcamera_src_change_state;\n> +\telement_class->send_event = gst_libcamera_src_send_event;\n> \n>  \tgst_element_class_set_metadata(element_class,\n>  \t\t\t\t       \"libcamera Source\", \"Source/Video\",","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4F424C3284\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Nov 2023 16:29:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A94E3629AF;\n\tFri, 10 Nov 2023 17:29:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 62A6261DB6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Nov 2023 17:29:30 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C3E249E;\n\tFri, 10 Nov 2023 17:29:07 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1699633771;\n\tbh=8prwJhpKWKnSZJP8++GP/BppYVKX6Ya60kKP1Vv3JJM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=kR3To0IUKQ2+A6JQqAo2pLblkvFHjka9qB2eksXQH8m0kcH5gc9a/nVMQ7tznzhPy\n\tHWuQeCO3HZe/Ft0dgsszL2OZew+CsdMcwMKSuAmbiXqiyADK4LndqUPFiz2AahLhrE\n\tywVVfdAlCNX8LGpelzyuJjob6M5nO7W9Gk6nlW8NBkLGYgKWwg+Nhp1MDsIQTS8H4Z\n\t+VD3e34sbPM3B8PJ3AmDU/CpvWEvrYbY5m008AjUR6nJsdN6ITi8jRSWZsrtCe3igz\n\tNvzTUwiqPIBDzv5kZVM8L742x1b22PaNlpYAvTcKWBAoWrV5UfS0Pds1D/kTwR97eE\n\ty2CPG09OyrQ/g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1699633747;\n\tbh=8prwJhpKWKnSZJP8++GP/BppYVKX6Ya60kKP1Vv3JJM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VcXVK9pmsmcopBdPrLveaxRwXd6LvgFm4fh6c5MVr2xMFCJwFurrTrSzdUYOgfjVK\n\tKMLaYXZ1inYa27WomVPzPz/vyemFZ43rdQgVJ8rG4Hz6KJ+LmhmdmytG7Yr1fihkg0\n\tE4aEZZ8a3+K+RdkhqiPzxp/cBrwmqTUI6sAO+suU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VcXVK9pm\"; dkim-atps=neutral","Date":"Fri, 10 Nov 2023 18:29:36 +0200","To":"Jaslo Ziska <jaslo@ziska.de>","Message-ID":"<20231110162936.GD7466@pendragon.ideasonboard.com>","References":"<20231110121133.17950-1-jaslo@ziska.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231110121133.17950-1-jaslo@ziska.de>","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne <nicolas.dufresne@collabora.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28078,"web_url":"https://patchwork.libcamera.org/comment/28078/","msgid":"<5fdd42e03fba9bb7ff7a7509c491350b28ccf08b.camel@collabora.com>","date":"2023-11-10T17:23:58","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le vendredi 10 novembre 2023 à 18:29 +0200, Laurent Pinchart a écrit :\n> Hi Jaslo,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote:\n> > Add a function for the send_event method which handles the GST_EVENT_EOS\n> > event. This function will set an atomic to the received event and push\n> > this EOS event to all source pads in the running task.\n> > \n> > Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a\n> > source element which enables it to receive EOS events sent to the\n> > (pipeline) bin containing it.\n> > This in turn enables libcamerasrc to for example receive EOS events from\n> > gst-launch-1.0 when using the -e flag.\n> \n> You should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=91\n\nYes please do.\n\n> \n> Does this patch fix the bug completely or partially ? Nicolas mentioned\n> in the review of v1 that there are two different EOS cases to consider,\n> one related to stopping the pipeline with Ctrl-C, and the other one to\n> downstream elements sending EOS. The commit message should explain that\n> only the first case is addressed.\n\nIt covers it fully. I've identified problem in the reporter gstreamear pipeline\n(missing -e) here:\n\nhttps://bugs.libcamera.org/show_bug.cgi?id=91#c13\n\nAs submitter can't edit description in bz I expected this information to be hard\nto find and the bug to be useless for future improvement, so please close with\nthis patch being merged. For the other EOS case, it very niche, but certainly\nbest would be to add it to the todo, rather the bz in my opinion.\n\nregards,\nNicolas\n\n> \n> > Signed-off-by: Jaslo Ziska <jaslo@ziska.de>\n> > Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> > ---\n> > Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message.\n> > \n> >  src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++-\n> >  1 file changed, 36 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 63d99571..8e197956 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -9,7 +9,6 @@\n> >  /**\n> >   * \\todo The following is a list of items that needs implementation in the GStreamer plugin\n> >   *  - Implement GstElement::send_event\n> > - *    + Allowing application to send EOS\n> >   *    + Allowing application to use FLUSH/FLUSH_STOP\n> >   *    + Prevent the main thread from accessing streaming thread\n> >   *  - Implement renegotiation (even if slow)\n> > @@ -29,6 +28,7 @@\n> > \n> >  #include \"gstlibcamerasrc.h\"\n> > \n> > +#include <atomic>\n> >  #include <queue>\n> >  #include <vector>\n> > \n> > @@ -144,6 +144,8 @@ struct _GstLibcameraSrc {\n> >  \tgchar *camera_name;\n> >  \tcontrols::AfModeEnum auto_focus_mode = controls::AfModeManual;\n> > \n> > +\tstd::atomic<GstEvent *> pending_eos;\n> > +\n> >  \tGstLibcameraSrcState *state;\n> >  \tGstLibcameraAllocator *allocator;\n> >  \tGstFlowCombiner *flow_combiner;\n> > @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > \n> >  \tbool doResume = false;\n> > \n> > +\tif (GstEvent *event = self->pending_eos.exchange(nullptr)) {\n> \n> I don't think we ever declare variables in if () statements in\n> libcamera, you may want to write this as\n> \n> \tGstEvent *event = self->pending_eos.exchange(nullptr);\n> \tif (event) {\n> \n> > +\t\tfor (GstPad *srcpad : state->srcpads_)\n> > +\t\t\tgst_pad_push_event(srcpad, gst_event_ref(event));\n> > +\t\tgst_event_unref(event);\n> > +\n> > +\t\treturn;\n> > +\t}\n> > +\n> >  \t/*\n> >  \t * Create and queue one request. If no buffers are available the\n> >  \t * function returns -ENOBUFS, which we ignore here as that's not a\n> > @@ -747,6 +757,28 @@ gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n> >  \treturn ret;\n> >  }\n> > \n> > +static gboolean\n> > +gst_libcamera_src_send_event(GstElement *element, GstEvent *event)\n> > +{\n> > +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);\n> > +\tgboolean ret = FALSE;\n> > +\n> > +\tswitch (GST_EVENT_TYPE(event)) {\n> > +\tcase GST_EVENT_EOS: {\n> > +\t\tif (GstEvent *old_event = self->pending_eos.exchange(event))\n> > +\t\t\tgst_event_unref(old_event);\n> \n> s/old_event/oldEvent/\n> \n> gst_event_unref() includes a null check, so you could simplify this to\n> \n> \t\tGstEvent *oldEvent = self->pending_eos.exchange(event))\n> \t\tgst_event_unref(oldEvent);\n> \n> > +\n> > +\t\tret = TRUE;\n> > +\t\tbreak;\n> > +\t}\n> > +\tdefault:\n> > +\t\tgst_event_unref(event);\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> >  static void\n> >  gst_libcamera_src_finalize(GObject *object)\n> >  {\n> > @@ -779,6 +811,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> >  \tstate->srcpads_.push_back(gst_pad_new_from_template(templ, \"src\"));\n> >  \tgst_element_add_pad(GST_ELEMENT(self), state->srcpads_.back());\n> > \n> > +\tGST_OBJECT_FLAG_SET(self, GST_ELEMENT_FLAG_SOURCE);\n> > +\n> >  \t/* C-style friend. */\n> >  \tstate->src_ = self;\n> >  \tself->state = state;\n> > @@ -844,6 +878,7 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> >  \telement_class->request_new_pad = gst_libcamera_src_request_new_pad;\n> >  \telement_class->release_pad = gst_libcamera_src_release_pad;\n> >  \telement_class->change_state = gst_libcamera_src_change_state;\n> > +\telement_class->send_event = gst_libcamera_src_send_event;\n> > \n> >  \tgst_element_class_set_metadata(element_class,\n> >  \t\t\t\t       \"libcamera Source\", \"Source/Video\",\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A8E50BDE6B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Nov 2023 17:24:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 190A6629AB;\n\tFri, 10 Nov 2023 18:24:11 +0100 (CET)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D73FC61DB6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Nov 2023 18:24:08 +0100 (CET)","from nicolas-tpx395.localdomain (cola.collaboradmins.com\n\t[195.201.22.229])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id ECAAF660739C;\n\tFri, 10 Nov 2023 17:24:07 +0000 (GMT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1699637051;\n\tbh=a2NnVqBQgTbZGlt3z6daVZbo55oWDhQQ1umYI3ceL88=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=NjYN1aP66qGTKl5r/ZOGBt/VPJG5Nyu6Ro60zpwhLY76CDMrW+FA+jDJ3AKF6AQFK\n\t/KytQXvLu/KhUb3R70EvFvDoHghVbSJ/CzeO89iv4d1wUYG94c8TMMJvWZUpE8arwb\n\tk0bWgAFl1SfBp4Bu2vtBYxPtKm/P0s+JuQvofwemHaR004q0rilT3h1Erp7V75n/EN\n\tFL4ihvPfd69ZVmb/vpxBheB8NYHeoLZN1iI8OUv2PceZzNjh9dZR1IC8tWEY/TLhlw\n\tzk9uhAepKPrKsEaiJtID+IgKUvN7UUvy19Zsougobi2Qyjs6qGkHw5NPQ5s0+bo6PM\n\tsko6334lGRSVg==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1699637048;\n\tbh=a2NnVqBQgTbZGlt3z6daVZbo55oWDhQQ1umYI3ceL88=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=CG54f6/RhCExzhA7FK0yggKAbcE0uCBzBI1INBIV87KDb0JqbM3MeHo23D33TSZTC\n\tPSofHObqLpUxl9p/Ygz+Nh11tStLGB9rY0i1aodt89nmPdy4JtCttPfnsT61f5T2fC\n\tpiDWbS9qCqWh5wJSBYHa1iWOYEcXe4m/AfiV3DkxRjHDCIOLWcUe4EEzITqq/AkDqU\n\tHoDf74GyH+wm3GZRNUJbq/K59yx94MoJqqtA+y7QUqyUdtPoimnZrFTopCn/ESqzPe\n\tkX/PAzHqPgQ/Cr4WcGVG1HBB+KG2k42KSxo0SYbz2rGklpAMhNuqnPWSKJqxFvLSwG\n\tC2B3+G3XRIN/Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"CG54f6/R\"; dkim-atps=neutral","Message-ID":"<5fdd42e03fba9bb7ff7a7509c491350b28ccf08b.camel@collabora.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Jaslo Ziska\n\t<jaslo@ziska.de>","Date":"Fri, 10 Nov 2023 12:23:58 -0500","In-Reply-To":"<20231110162936.GD7466@pendragon.ideasonboard.com>","References":"<20231110121133.17950-1-jaslo@ziska.de>\n\t<20231110162936.GD7466@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.48.4 (3.48.4-1.fc38) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Nicolas Dufresne via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Nicolas Dufresne <nicolas.dufresne@collabora.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28079,"web_url":"https://patchwork.libcamera.org/comment/28079/","msgid":"<MxeohaSO6AUf10acY2QQd307QQ3wbq9qAXe0r9yzOwSoyXbC4aEboW0ebfUMAIMbSQO53R7EhZVcJulKYhO_GW6zSfkxekQEkdngH6D-x7k=@protonmail.com>","date":"2023-11-10T22:15:25","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2023. november 10., péntek 17:29 keltezéssel, Laurent Pinchart via libcamera-devel írta:\n\n> [...]\n> > +\tif (GstEvent *event = self->pending_eos.exchange(nullptr)) {\n> \n> I don't think we ever declare variables in if () statements in\n> libcamera, you may want to write this as\n\n> \n> \tGstEvent *event = self->pending_eos.exchange(nullptr);\n> \tif (event) {\n> [...]\n\nIIRC that is valid since at least C++11. Or is this more of a stylistic choice?\n\nRegards,\nBarnabás Pőcze","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3C971C3284\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Nov 2023 22:15:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63235629B6;\n\tFri, 10 Nov 2023 23:15:41 +0100 (CET)","from mail-40131.protonmail.ch (mail-40131.protonmail.ch\n\t[185.70.40.131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2385261DB6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Nov 2023 23:15:40 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1699654541;\n\tbh=mqvAo/SZ4CJroMbxdQEJ4ozBKHVp38yslWBBUqRTKEs=;\n\th=Date:To:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=KdvOXDVU8TH3RI0zCil0EKaVmub5z0b0qrld6ijCFwHprwxYlGPewv0HpAjQ8M6pk\n\tuUWt5LOxBajH9xC1i/TkB3HIYcBLNB04JtUPrs6Fcz7UR1XR+u9ZJCciihMfONjxyR\n\tbFsytvZkILmVZAyjbVevH37dzEz76xu4A4Sme+mcTQgGMGq/8rmV9OyhmlV7w+M+wV\n\tP6n975Ny3DkS0sk61Fo2cYX+4pZwF4ef40mC9Sh+nOq21X3qaBXZK2BLMMnY0q3r7Y\n\tEnbAGROpntLnA+OmtOS4iHtkihta1Eo1vJ+rlkVNUO+WyDSozVl931JesstNnc5cFc\n\tuFpehrGvsCwrw==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1699654538; x=1699913738;\n\tbh=mqvAo/SZ4CJroMbxdQEJ4ozBKHVp38yslWBBUqRTKEs=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=djPBYblFuTt6ueOCNCL9tg28oHTRg9K01mcS0/kTJgFX7Wi5wymo0qgOX2lohzO2O\n\tqUFTsR9ahRLoZXjbXgrtb7Hdxs319Ju6e6WtiLIdm6+zxgt2BZrdHpyBcbeU7oTfdr\n\tpFMNQvNQomhm7esliyS4AmUZh8OxW8WCvTtTSZheoPWWD3ddKEzbvRAUERcNLwXsDH\n\tT183UUdSca+5oIhGzJdj83LfFkXx8txEKevwDkhX9NX15Z0tZcf1+29zGTz6EZRIdR\n\tQm7ikutb5KTH/domiVufX/HcO3vHea5W3qXIOvCjj9U4Ybffit63OEhxhGovX2YQRn\n\tpYHoSLmaZyAHQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=protonmail.com\n\theader.i=@protonmail.com\n\theader.b=\"djPBYblF\"; dkim-atps=neutral","Date":"Fri, 10 Nov 2023 22:15:25 +0000","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<MxeohaSO6AUf10acY2QQd307QQ3wbq9qAXe0r9yzOwSoyXbC4aEboW0ebfUMAIMbSQO53R7EhZVcJulKYhO_GW6zSfkxekQEkdngH6D-x7k=@protonmail.com>","In-Reply-To":"<20231110162936.GD7466@pendragon.ideasonboard.com>","References":"<20231110121133.17950-1-jaslo@ziska.de>\n\t<20231110162936.GD7466@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze_via_libcamera-devel?=\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Jaslo Ziska <jaslo@ziska.de>, libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne <nicolas.dufresne@collabora.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28080,"web_url":"https://patchwork.libcamera.org/comment/28080/","msgid":"<d326b8a6c8a31097a200eb40034c44f2a557d55a.camel@collabora.com>","date":"2023-11-11T01:55:38","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le vendredi 10 novembre 2023 à 18:29 +0200, Laurent Pinchart via libcamera-devel\na écrit :\n> Hi Jaslo,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote:\n> > Add a function for the send_event method which handles the GST_EVENT_EOS\n> > event. This function will set an atomic to the received event and push\n> > this EOS event to all source pads in the running task.\n> > \n> > Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a\n> > source element which enables it to receive EOS events sent to the\n> > (pipeline) bin containing it.\n> > This in turn enables libcamerasrc to for example receive EOS events from\n> > gst-launch-1.0 when using the -e flag.\n> \n> You should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=91\n> \n> Does this patch fix the bug completely or partially ? Nicolas mentioned\n> in the review of v1 that there are two different EOS cases to consider,\n> one related to stopping the pipeline with Ctrl-C, and the other one to\n> downstream elements sending EOS. The commit message should explain that\n> only the first case is addressed.\n> \n> > Signed-off-by: Jaslo Ziska <jaslo@ziska.de>\n> > Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> > ---\n> > Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message.\n> > \n> >  src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++-\n> >  1 file changed, 36 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 63d99571..8e197956 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -9,7 +9,6 @@\n> >  /**\n> >   * \\todo The following is a list of items that needs implementation in the GStreamer plugin\n> >   *  - Implement GstElement::send_event\n> > - *    + Allowing application to send EOS\n> >   *    + Allowing application to use FLUSH/FLUSH_STOP\n> >   *    + Prevent the main thread from accessing streaming thread\n> >   *  - Implement renegotiation (even if slow)\n> > @@ -29,6 +28,7 @@\n> > \n> >  #include \"gstlibcamerasrc.h\"\n> > \n> > +#include <atomic>\n> >  #include <queue>\n> >  #include <vector>\n> > \n> > @@ -144,6 +144,8 @@ struct _GstLibcameraSrc {\n> >  \tgchar *camera_name;\n> >  \tcontrols::AfModeEnum auto_focus_mode = controls::AfModeManual;\n> > \n> > +\tstd::atomic<GstEvent *> pending_eos;\n> > +\n> >  \tGstLibcameraSrcState *state;\n> >  \tGstLibcameraAllocator *allocator;\n> >  \tGstFlowCombiner *flow_combiner;\n> > @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > \n> >  \tbool doResume = false;\n> > \n> > +\tif (GstEvent *event = self->pending_eos.exchange(nullptr)) {\n> \n> I don't think we ever declare variables in if () statements in\n> libcamera, you may want to write this as\n> \n> \tGstEvent *event = self->pending_eos.exchange(nullptr);\n> \tif (event) {\n\nPersonally I like te in-scope declaration better. An you can improve it with:\n\n\tif (g_autoptr(GstEvent) event = ...)\n\n> \n> > +\t\tfor (GstPad *srcpad : state->srcpads_)\n> > +\t\t\tgst_pad_push_event(srcpad, gst_event_ref(event));\n> > +\t\tgst_event_unref(event);\n\nWitch let you drop this.\n\n> > +\n> > +\t\treturn;\n> > +\t}\n> > +\n> >  \t/*\n> >  \t * Create and queue one request. If no buffers are available the\n> >  \t * function returns -ENOBUFS, which we ignore here as that's not a\n> > @@ -747,6 +757,28 @@ gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n> >  \treturn ret;\n> >  }\n> > \n> > +static gboolean\n> > +gst_libcamera_src_send_event(GstElement *element, GstEvent *event)\n> > +{\n> > +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);\n> > +\tgboolean ret = FALSE;\n> > +\n> > +\tswitch (GST_EVENT_TYPE(event)) {\n> > +\tcase GST_EVENT_EOS: {\n> > +\t\tif (GstEvent *old_event = self->pending_eos.exchange(event))\n> > +\t\t\tgst_event_unref(old_event);\n> \n> s/old_event/oldEvent/\n> \n> gst_event_unref() includes a null check, so you could simplify this to\n\nPlease don't, there is a run-time check, but it can be compiled out. You will\nalways get an error message (and a crashed if you decided to drop run-time\nchecks, which are just protections).\n\n> \n> \t\tGstEvent *oldEvent = self->pending_eos.exchange(event))\n> \t\tgst_event_unref(oldEvent);\n> \n> > +\n> > +\t\tret = TRUE;\n> > +\t\tbreak;\n> > +\t}\n> > +\tdefault:\n> > +\t\tgst_event_unref(event);\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> >  static void\n> >  gst_libcamera_src_finalize(GObject *object)\n> >  {\n> > @@ -779,6 +811,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> >  \tstate->srcpads_.push_back(gst_pad_new_from_template(templ, \"src\"));\n> >  \tgst_element_add_pad(GST_ELEMENT(self), state->srcpads_.back());\n> > \n> > +\tGST_OBJECT_FLAG_SET(self, GST_ELEMENT_FLAG_SOURCE);\n> > +\n> >  \t/* C-style friend. */\n> >  \tstate->src_ = self;\n> >  \tself->state = state;\n> > @@ -844,6 +878,7 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> >  \telement_class->request_new_pad = gst_libcamera_src_request_new_pad;\n> >  \telement_class->release_pad = gst_libcamera_src_release_pad;\n> >  \telement_class->change_state = gst_libcamera_src_change_state;\n> > +\telement_class->send_event = gst_libcamera_src_send_event;\n> > \n> >  \tgst_element_class_set_metadata(element_class,\n> >  \t\t\t\t       \"libcamera Source\", \"Source/Video\",\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 26A23BDE6B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 11 Nov 2023 01:55:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40F9D629B6;\n\tSat, 11 Nov 2023 02:55:54 +0100 (CET)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[46.235.227.172])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E17DC61DB2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 11 Nov 2023 02:55:51 +0100 (CET)","from nicolas-tpx395.localdomain (cola.collaboradmins.com\n\t[195.201.22.229])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id AFEBF6607400;\n\tSat, 11 Nov 2023 01:55:50 +0000 (GMT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1699667754;\n\tbh=VkddBVQJI7fLyHpdIUiJxJ+mRKhq972F+AccRJN1kQ0=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=qTq7CJl7R9JS9vGe2r1cxyWpuF/dtBOrYn8MfgVhpx4tl1k8XH+B/gKlaVdz8+I4R\n\tofLqAGk/PnEEYjp3h3f3JqSjuUA2ULDHvblZALA7wlkCCZD682EMG/9KHn5yMzSYcZ\n\tnxabAUyHix+uf7rUXKmJCsN7tQudmApFL4OqokopRzppV161A+gQpLTisgTU/iWtTc\n\tJzn0d4KzY6Jtp0eSzQPvbel20wwGcvjRYeWakEGvKno0Yb7ZFD4oz2de9y0DUIZF1u\n\tPkjP+4H52+l7SwNMurc4m0eGclFa4v8qaYQ/Mostv47la9fkI/eVOF+HExnyri8J3Y\n\taQzA3NiktA0Rw==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1699667751;\n\tbh=VkddBVQJI7fLyHpdIUiJxJ+mRKhq972F+AccRJN1kQ0=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=UuKxq3/Frff6LlRm5Z/u4E67zpAP8IQ3rVfmy0VCfrp5h5pdjdWq6LZP4H87f+Bjt\n\tL9+RcIcAuElrFM8ljGEGWEgwt2B1MLtXw4pvShJQbFOBlcWX2ZoIiemOWbB3L6qwZ7\n\tH/pSP4LciwAwYenbo4wdS/aDM3/oSELD56aBqlPGWzP1SNDruQ5WB61dAP/iKl17Q+\n\tyJPan4ifuS6D9X4i6CB9sIHN2zASUPTGlSikpQ1r1vSTivXidthOwUaqTODwrmD6Sh\n\tFXo6rtSaCM4AT5gRdyJ3PH7awe6GXA5mtrSd+8Jhg4oPBpforG9AcEsbOTqoGFMuJQ\n\tLovcWSUfQUF1A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"UuKxq3/F\"; dkim-atps=neutral","Message-ID":"<d326b8a6c8a31097a200eb40034c44f2a557d55a.camel@collabora.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Jaslo Ziska\n\t<jaslo@ziska.de>","Date":"Fri, 10 Nov 2023 20:55:38 -0500","In-Reply-To":"<20231110162936.GD7466@pendragon.ideasonboard.com>","References":"<20231110121133.17950-1-jaslo@ziska.de>\n\t<20231110162936.GD7466@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.48.4 (3.48.4-1.fc38) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Nicolas Dufresne via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Nicolas Dufresne <nicolas.dufresne@collabora.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28081,"web_url":"https://patchwork.libcamera.org/comment/28081/","msgid":"<20231112231803.GA20690@pendragon.ideasonboard.com>","date":"2023-11-12T23:18:03","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Nov 10, 2023 at 08:55:38PM -0500, Nicolas Dufresne wrote:\n> Le vendredi 10 novembre 2023 à 18:29 +0200, Laurent Pinchart via libcamera-devel a écrit :\n> > On Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote:\n> > > Add a function for the send_event method which handles the GST_EVENT_EOS\n> > > event. This function will set an atomic to the received event and push\n> > > this EOS event to all source pads in the running task.\n> > > \n> > > Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a\n> > > source element which enables it to receive EOS events sent to the\n> > > (pipeline) bin containing it.\n> > > This in turn enables libcamerasrc to for example receive EOS events from\n> > > gst-launch-1.0 when using the -e flag.\n> > \n> > You should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with\n> > \n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=91\n> > \n> > Does this patch fix the bug completely or partially ? Nicolas mentioned\n> > in the review of v1 that there are two different EOS cases to consider,\n> > one related to stopping the pipeline with Ctrl-C, and the other one to\n> > downstream elements sending EOS. The commit message should explain that\n> > only the first case is addressed.\n> > \n> > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de>\n> > > Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> > > ---\n> > > Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message.\n> > > \n> > >  src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++-\n> > >  1 file changed, 36 insertions(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index 63d99571..8e197956 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -9,7 +9,6 @@\n> > >  /**\n> > >   * \\todo The following is a list of items that needs implementation in the GStreamer plugin\n> > >   *  - Implement GstElement::send_event\n> > > - *    + Allowing application to send EOS\n> > >   *    + Allowing application to use FLUSH/FLUSH_STOP\n> > >   *    + Prevent the main thread from accessing streaming thread\n> > >   *  - Implement renegotiation (even if slow)\n> > > @@ -29,6 +28,7 @@\n> > > \n> > >  #include \"gstlibcamerasrc.h\"\n> > > \n> > > +#include <atomic>\n> > >  #include <queue>\n> > >  #include <vector>\n> > > \n> > > @@ -144,6 +144,8 @@ struct _GstLibcameraSrc {\n> > >  \tgchar *camera_name;\n> > >  \tcontrols::AfModeEnum auto_focus_mode = controls::AfModeManual;\n> > > \n> > > +\tstd::atomic<GstEvent *> pending_eos;\n> > > +\n> > >  \tGstLibcameraSrcState *state;\n> > >  \tGstLibcameraAllocator *allocator;\n> > >  \tGstFlowCombiner *flow_combiner;\n> > > @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > > \n> > >  \tbool doResume = false;\n> > > \n> > > +\tif (GstEvent *event = self->pending_eos.exchange(nullptr)) {\n> > \n> > I don't think we ever declare variables in if () statements in\n> > libcamera, you may want to write this as\n> > \n> > \tGstEvent *event = self->pending_eos.exchange(nullptr);\n> > \tif (event) {\n> \n> Personally I like te in-scope declaration better. An you can improve it with:\n> \n> \tif (g_autoptr(GstEvent) event = ...)\n> \n> > > +\t\tfor (GstPad *srcpad : state->srcpads_)\n> > > +\t\t\tgst_pad_push_event(srcpad, gst_event_ref(event));\n> > > +\t\tgst_event_unref(event);\n> \n> Witch let you drop this.\n\nI probably dislike assignments in if () statements due to them being\nfrowned upon in the kernel coding style. I personally find them\nerror-prone, and compilers agree with me I think, as they will warn\nasking if the author meant to compare instead of assign. When the\nvariable is declared in the if () statement it's obviously not an issue\nanymore, so I'm OK with it here, even if I'm not sure to accept it more\nglobally in the libcamera coding style.\n\n> > > +\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > >  \t/*\n> > >  \t * Create and queue one request. If no buffers are available the\n> > >  \t * function returns -ENOBUFS, which we ignore here as that's not a\n> > > @@ -747,6 +757,28 @@ gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n> > >  \treturn ret;\n> > >  }\n> > > \n> > > +static gboolean\n> > > +gst_libcamera_src_send_event(GstElement *element, GstEvent *event)\n> > > +{\n> > > +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);\n> > > +\tgboolean ret = FALSE;\n> > > +\n> > > +\tswitch (GST_EVENT_TYPE(event)) {\n> > > +\tcase GST_EVENT_EOS: {\n> > > +\t\tif (GstEvent *old_event = self->pending_eos.exchange(event))\n> > > +\t\t\tgst_event_unref(old_event);\n> > \n> > s/old_event/oldEvent/\n> > \n> > gst_event_unref() includes a null check, so you could simplify this to\n> \n> Please don't, there is a run-time check, but it can be compiled out. You will\n> always get an error message (and a crashed if you decided to drop run-time\n> checks, which are just protections).\n\nMy bad, I didn't realize that.\n\nIf we want to use a g_autoptr above, I would use one here too for\nconsistency.\n\n> > \t\tGstEvent *oldEvent = self->pending_eos.exchange(event))\n> > \t\tgst_event_unref(oldEvent);\n> > \n> > > +\n> > > +\t\tret = TRUE;\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\tdefault:\n> > > +\t\tgst_event_unref(event);\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > >  static void\n> > >  gst_libcamera_src_finalize(GObject *object)\n> > >  {\n> > > @@ -779,6 +811,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> > >  \tstate->srcpads_.push_back(gst_pad_new_from_template(templ, \"src\"));\n> > >  \tgst_element_add_pad(GST_ELEMENT(self), state->srcpads_.back());\n> > > \n> > > +\tGST_OBJECT_FLAG_SET(self, GST_ELEMENT_FLAG_SOURCE);\n> > > +\n> > >  \t/* C-style friend. */\n> > >  \tstate->src_ = self;\n> > >  \tself->state = state;\n> > > @@ -844,6 +878,7 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > >  \telement_class->request_new_pad = gst_libcamera_src_request_new_pad;\n> > >  \telement_class->release_pad = gst_libcamera_src_release_pad;\n> > >  \telement_class->change_state = gst_libcamera_src_change_state;\n> > > +\telement_class->send_event = gst_libcamera_src_send_event;\n> > > \n> > >  \tgst_element_class_set_metadata(element_class,\n> > >  \t\t\t\t       \"libcamera Source\", \"Source/Video\",","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 50B7FC3213\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 12 Nov 2023 23:18:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D327629BC;\n\tMon, 13 Nov 2023 00:17:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4086D61DB8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 13 Nov 2023 00:17:57 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E522223;\n\tMon, 13 Nov 2023 00:17:32 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1699831079;\n\tbh=GhsCAzGJNIFIJtHKk3BMgGNWSBeTSeegRcPAzY38Q7w=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=MeHCgi0/qcc+RV0C3M+1uzIzkJx3V7oCnKU8642kI8u0x7YbwLIyfYVsQ9q/J3jBO\n\tsG3Ep/L8hMAxS++0avkZskGJAZ4SKFC7Bonqrf4yW0Fk2xAwqL6VY79/8PeANEwyJm\n\tu8vNXJcP91wWchXUzaX9nU2eYR8bZy42cYywbOisWmzdv7WJMateoWWGu84zb3SwiN\n\t10J1BIfE12Y8rwU03DKmGgurJQ34o7RavTdeSLfsr0oIyhKRu4knYHZdYvkHyZspbt\n\tVmZTA0dWvQpA1s7hJ5nL7sF9TcLVhzsBR03PfBghBuUDkmcE6jNPWf9Ht5nAcPQwet\n\tEZRO4lHkSFIvw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1699831052;\n\tbh=GhsCAzGJNIFIJtHKk3BMgGNWSBeTSeegRcPAzY38Q7w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fhWIn8XzonXL39hXxplUFmkCF7t5xVBA3TxjK6oYCkVBO3jurJlBUvbh1v9+WJaAD\n\t5dB9gxztmRQhCphNv/asr62FMJm6GP11BRQu7/OD6hvx11xN1yhlEyFf6l93WcLKie\n\tjwV3QKBagxE3tPvEv9kWrklsAaf/xBWaG0I/Y7sY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fhWIn8Xz\"; dkim-atps=neutral","Date":"Mon, 13 Nov 2023 01:18:03 +0200","To":"Nicolas Dufresne <nicolas.dufresne@collabora.com>","Message-ID":"<20231112231803.GA20690@pendragon.ideasonboard.com>","References":"<20231110121133.17950-1-jaslo@ziska.de>\n\t<20231110162936.GD7466@pendragon.ideasonboard.com>\n\t<d326b8a6c8a31097a200eb40034c44f2a557d55a.camel@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<d326b8a6c8a31097a200eb40034c44f2a557d55a.camel@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Jaslo Ziska <jaslo@ziska.de>, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28082,"web_url":"https://patchwork.libcamera.org/comment/28082/","msgid":"<20231112232351.GB20690@pendragon.ideasonboard.com>","date":"2023-11-12T23:23:51","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Nov 10, 2023 at 10:15:25PM +0000, Barnabás Pőcze wrote:\n> Hi\n> \n> \n> 2023. november 10., péntek 17:29 keltezéssel, Laurent Pinchart via libcamera-devel írta:\n> \n> > [...]\n> > > +\tif (GstEvent *event = self->pending_eos.exchange(nullptr)) {\n> > \n> > I don't think we ever declare variables in if () statements in\n> > libcamera, you may want to write this as\n> \n> > \n> > \tGstEvent *event = self->pending_eos.exchange(nullptr);\n> > \tif (event) {\n> > [...]\n> \n> IIRC that is valid since at least C++11. Or is this more of a stylistic choice?\n\nIt's a coding style choice. I really dislike assignments in if ()\nstatements, such as\n\n\tGstEvent *event;\n\n\tif (event = self->pending_eos.exchange(nullptr))\n\nbecause it's very error-prone (easy to confuse the assignement and\ncomparison operator during review). The kernel coding style, which\ninfluenced me a lot, doesn't like it either, and compilers generate a\nwarning these days.\n\nThe error-prone argument is obviously moot if declaring the variable in\nthe if () statement, as\n\n\tif (GstEvent *event = self->pending_eos.exchange(nullptr))\n\nwould compile and\n\n\tif (GstEvent *event == self->pending_eos.exchange(nullptr))\n\nwouldn't (hopefully, I haven't tried). It still \"feels\" weird due to the\nassignment-in-if-statements explanation, so maybe we can accept this in\nthe coding style. I'm OK doing so in the implementation of the GStreamer\nelement, even if we don't make it a standard practice in the libcamera\ncore.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 46D7ABDE6B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 12 Nov 2023 23:23:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD3BD629BA;\n\tMon, 13 Nov 2023 00:23:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2511E61DB8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 13 Nov 2023 00:23:45 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6D93D223;\n\tMon, 13 Nov 2023 00:23:20 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1699831426;\n\tbh=PeFwaCGHsggCdOj3yp7vaJCcjcswdfCSBLVRcL6LVis=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=HjzuSsewk7grSd5+JaluauBUvsKsFP+1iK4RArloXYpoC8YooQSFchdsocPHglFRS\n\tbkPjwC8HWVvjeGfEUD1r5bxAwijYLDgku+GA28Y+KAY80yq0L4fmPNFqqyK4KWrZPm\n\tJ201FulyQisw8A+Mxv2cGmfXbjwGpyyaI2PlRcrAjOFInyvhw/t3mQfAdgfenS5Hqe\n\t7jTHGDdLi7/4xz0uDeDYuFv89s7aPmPwG3BHJMYBRov5Y0yMSp34d5R4uZ9iLOkuT0\n\txkGKJUHLPCRd9Pn6TBGDOq2xwVAxjnQi8/b1rc/mpixg4fsarOMPbUadkrDRhGD45y\n\tsBXp3AdjvmBxg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1699831400;\n\tbh=PeFwaCGHsggCdOj3yp7vaJCcjcswdfCSBLVRcL6LVis=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jCQMv5bbJ3YmnwadZOaVywSN7axRXCTHG15ZHPuhfhLlRZ5fn1bOUmRo3pkOlOc4F\n\t0VlBL7KjjtU7L9VPtoJHFN+37d70lYk6ozBxlYQf2DT5YWwH1PyO054U24/RtRhcxN\n\te5TsmKWW1s5Iym6/9a6y1MPum/wIfTmjYega6aYA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jCQMv5bb\"; dkim-atps=neutral","Date":"Mon, 13 Nov 2023 01:23:51 +0200","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Message-ID":"<20231112232351.GB20690@pendragon.ideasonboard.com>","References":"<20231110121133.17950-1-jaslo@ziska.de>\n\t<20231110162936.GD7466@pendragon.ideasonboard.com>\n\t<MxeohaSO6AUf10acY2QQd307QQ3wbq9qAXe0r9yzOwSoyXbC4aEboW0ebfUMAIMbSQO53R7EhZVcJulKYhO_GW6zSfkxekQEkdngH6D-x7k=@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<MxeohaSO6AUf10acY2QQd307QQ3wbq9qAXe0r9yzOwSoyXbC4aEboW0ebfUMAIMbSQO53R7EhZVcJulKYhO_GW6zSfkxekQEkdngH6D-x7k=@protonmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Jaslo Ziska <jaslo@ziska.de>, libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne <nicolas.dufresne@collabora.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28083,"web_url":"https://patchwork.libcamera.org/comment/28083/","msgid":"<d4eb0a4107a5292737fbf47d9041e50a737b4a5f.camel@collabora.com>","date":"2023-11-12T23:52:06","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le lundi 13 novembre 2023 à 01:18 +0200, Laurent Pinchart via libcamera-devel a\nécrit :\n> On Fri, Nov 10, 2023 at 08:55:38PM -0500, Nicolas Dufresne wrote:\n> > Le vendredi 10 novembre 2023 à 18:29 +0200, Laurent Pinchart via libcamera-devel a écrit :\n> > > On Fri, Nov 10, 2023 at 01:07:29PM +0100, Jaslo Ziska via libcamera-devel wrote:\n> > > > Add a function for the send_event method which handles the GST_EVENT_EOS\n> > > > event. This function will set an atomic to the received event and push\n> > > > this EOS event to all source pads in the running task.\n> > > > \n> > > > Also set the GST_ELEMENT_FLAG_SOURCE flag to identify libcamerasrc as a\n> > > > source element which enables it to receive EOS events sent to the\n> > > > (pipeline) bin containing it.\n> > > > This in turn enables libcamerasrc to for example receive EOS events from\n> > > > gst-launch-1.0 when using the -e flag.\n> > > \n> > > You should reference https://bugs.libcamera.org/show_bug.cgi?id=91 with\n> > > \n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=91\n> > > \n> > > Does this patch fix the bug completely or partially ? Nicolas mentioned\n> > > in the review of v1 that there are two different EOS cases to consider,\n> > > one related to stopping the pipeline with Ctrl-C, and the other one to\n> > > downstream elements sending EOS. The commit message should explain that\n> > > only the first case is addressed.\n> > > \n> > > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de>\n> > > > Acked-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> > > > ---\n> > > > Use std::atomic<GstEvent *> instead of glib_atomic_int_* plus a seperate GstEvent pointer and add a commit message.\n> > > > \n> > > >  src/gstreamer/gstlibcamerasrc.cpp | 37 ++++++++++++++++++++++++++++++-\n> > > >  1 file changed, 36 insertions(+), 1 deletion(-)\n> > > > \n> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > index 63d99571..8e197956 100644\n> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > @@ -9,7 +9,6 @@\n> > > >  /**\n> > > >   * \\todo The following is a list of items that needs implementation in the GStreamer plugin\n> > > >   *  - Implement GstElement::send_event\n> > > > - *    + Allowing application to send EOS\n> > > >   *    + Allowing application to use FLUSH/FLUSH_STOP\n> > > >   *    + Prevent the main thread from accessing streaming thread\n> > > >   *  - Implement renegotiation (even if slow)\n> > > > @@ -29,6 +28,7 @@\n> > > > \n> > > >  #include \"gstlibcamerasrc.h\"\n> > > > \n> > > > +#include <atomic>\n> > > >  #include <queue>\n> > > >  #include <vector>\n> > > > \n> > > > @@ -144,6 +144,8 @@ struct _GstLibcameraSrc {\n> > > >  \tgchar *camera_name;\n> > > >  \tcontrols::AfModeEnum auto_focus_mode = controls::AfModeManual;\n> > > > \n> > > > +\tstd::atomic<GstEvent *> pending_eos;\n> > > > +\n> > > >  \tGstLibcameraSrcState *state;\n> > > >  \tGstLibcameraAllocator *allocator;\n> > > >  \tGstFlowCombiner *flow_combiner;\n> > > > @@ -397,6 +399,14 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > > > \n> > > >  \tbool doResume = false;\n> > > > \n> > > > +\tif (GstEvent *event = self->pending_eos.exchange(nullptr)) {\n> > > \n> > > I don't think we ever declare variables in if () statements in\n> > > libcamera, you may want to write this as\n> > > \n> > > \tGstEvent *event = self->pending_eos.exchange(nullptr);\n> > > \tif (event) {\n> > \n> > Personally I like te in-scope declaration better. An you can improve it with:\n> > \n> > \tif (g_autoptr(GstEvent) event = ...)\n> > \n> > > > +\t\tfor (GstPad *srcpad : state->srcpads_)\n> > > > +\t\t\tgst_pad_push_event(srcpad, gst_event_ref(event));\n> > > > +\t\tgst_event_unref(event);\n> > \n> > Witch let you drop this.\n> \n> I probably dislike assignments in if () statements due to them being\n> frowned upon in the kernel coding style. I personally find them\n> error-prone, and compilers agree with me I think, as they will warn\n> asking if the author meant to compare instead of assign. When the\n> variable is declared in the if () statement it's obviously not an issue\n> anymore, so I'm OK with it here, even if I'm not sure to accept it more\n> globally in the libcamera coding style.\n\nNote that the use of g_autoptr can happen with traditional variable declaration,\nsomething like this:\n\ng_autoptr(GstEvent) event = self->pending_eos.exchange(nullptr);\nif (event)\n  ....\n\n/* end of scope will lead to an unref */\n\nThe delta is mostly that the unref happens later, in such a short function, the\nbenefit is theoretical, since the following line returns.\n\n> \n> > > > +\n> > > > +\t\treturn;\n> > > > +\t}\n> > > > +\n> > > >  \t/*\n> > > >  \t * Create and queue one request. If no buffers are available the\n> > > >  \t * function returns -ENOBUFS, which we ignore here as that's not a\n> > > > @@ -747,6 +757,28 @@ gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n> > > >  \treturn ret;\n> > > >  }\n> > > > \n> > > > +static gboolean\n> > > > +gst_libcamera_src_send_event(GstElement *element, GstEvent *event)\n> > > > +{\n> > > > +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);\n> > > > +\tgboolean ret = FALSE;\n> > > > +\n> > > > +\tswitch (GST_EVENT_TYPE(event)) {\n> > > > +\tcase GST_EVENT_EOS: {\n> > > > +\t\tif (GstEvent *old_event = self->pending_eos.exchange(event))\n> > > > +\t\t\tgst_event_unref(old_event);\n> > > \n> > > s/old_event/oldEvent/\n> > > \n> > > gst_event_unref() includes a null check, so you could simplify this to\n> > \n> > Please don't, there is a run-time check, but it can be compiled out. You will\n> > always get an error message (and a crashed if you decided to drop run-time\n> > checks, which are just protections).\n> \n> My bad, I didn't realize that.\n> \n> If we want to use a g_autoptr above, I would use one here too for\n> consistency.\n\nI agree.\n\n> \n> > > \t\tGstEvent *oldEvent = self->pending_eos.exchange(event))\n> > > \t\tgst_event_unref(oldEvent);\n> > > \n> > > > +\n> > > > +\t\tret = TRUE;\n> > > > +\t\tbreak;\n> > > > +\t}\n> > > > +\tdefault:\n> > > > +\t\tgst_event_unref(event);\n> > > > +\t\tbreak;\n> > > > +\t}\n> > > > +\n> > > > +\treturn ret;\n> > > > +}\n> > > > +\n> > > >  static void\n> > > >  gst_libcamera_src_finalize(GObject *object)\n> > > >  {\n> > > > @@ -779,6 +811,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> > > >  \tstate->srcpads_.push_back(gst_pad_new_from_template(templ, \"src\"));\n> > > >  \tgst_element_add_pad(GST_ELEMENT(self), state->srcpads_.back());\n> > > > \n> > > > +\tGST_OBJECT_FLAG_SET(self, GST_ELEMENT_FLAG_SOURCE);\n> > > > +\n> > > >  \t/* C-style friend. */\n> > > >  \tstate->src_ = self;\n> > > >  \tself->state = state;\n> > > > @@ -844,6 +878,7 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > >  \telement_class->request_new_pad = gst_libcamera_src_request_new_pad;\n> > > >  \telement_class->release_pad = gst_libcamera_src_release_pad;\n> > > >  \telement_class->change_state = gst_libcamera_src_change_state;\n> > > > +\telement_class->send_event = gst_libcamera_src_send_event;\n> > > > \n> > > >  \tgst_element_class_set_metadata(element_class,\n> > > >  \t\t\t\t       \"libcamera Source\", \"Source/Video\",\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F0998C3213\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 12 Nov 2023 23:52:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B09C629BC;\n\tMon, 13 Nov 2023 00:52:18 +0100 (CET)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85C1261DB8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 13 Nov 2023 00:52:17 +0100 (CET)","from nicolas-tpx395.localdomain (cola.collaboradmins.com\n\t[195.201.22.229])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id 76F01660731F;\n\tSun, 12 Nov 2023 23:52:16 +0000 (GMT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1699833138;\n\tbh=rb0uc3aH1QOnN2uchSQdOJxM7HpIoleZvGX5IedS5g0=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=WXBNDPqdIphWCoNgQbKtmh+VrZ+R3CF6XSDXGXR+lTzBMK5vkfTW1MaMXT8xOFhT5\n\tovrmF4fv2/fFieRZJSE2KY0jHx/78etbuEZX+RMgKhTPW+0oGg7AmNLN6Ei1v57rrW\n\tKnxlCuP24ywmzhJMMwnQrQ3Pc1Q4V+jqoK32O9R560d1LwIUptsm3mqRNJT5poS4Y/\n\tVDLwI62ieMN30Kin5bIW3MV058r/Hwk3SMQk1RfpbCQdFAla3OzcCAkDQ2UdzvNwtk\n\tSsxi1CXwyUO7fIOJ+sFwf/2h8yY68tTn0z1YOVN8pLOJVDzyGuhTucCDr0KRp8XHta\n\tGVSu6BYT5GWdA==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1699833137;\n\tbh=rb0uc3aH1QOnN2uchSQdOJxM7HpIoleZvGX5IedS5g0=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=MK8mPh+Rq6+kgTv6NhF8DeTlE56oO9yaUMdwTzojjDRkVfO6MzsKT1TurP7cqQiI6\n\tYrLBO1rTepo/bEWE+DboN5fGbjxTlozr8LQdKTMmMJgHByZ2vpdvoarwPTEE0oIu/9\n\t0U2kDy09trYilVDw/r5t7Lrt6fzj51KHEp3pVluHAy/1ok1nOD2zM/1xZ8ZLegko6J\n\tGyW5AF8w/FjaPPr0Qc47+44ztIoMQmpylnnl2LptN/YEdxCZs/5/VweNBzRfRDjKCD\n\t6BTWDg4JnnguDLO+X9TYOGAWv7BOd3c4+nA9D4FYb4hks/dmpnbVepZDeaFWeI3UCV\n\teXQOVeVzVDueg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"MK8mPh+R\"; dkim-atps=neutral","Message-ID":"<d4eb0a4107a5292737fbf47d9041e50a737b4a5f.camel@collabora.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Date":"Sun, 12 Nov 2023 18:52:06 -0500","In-Reply-To":"<20231112231803.GA20690@pendragon.ideasonboard.com>","References":"<20231110121133.17950-1-jaslo@ziska.de>\n\t<20231110162936.GD7466@pendragon.ideasonboard.com>\n\t<d326b8a6c8a31097a200eb40034c44f2a557d55a.camel@collabora.com>\n\t<20231112231803.GA20690@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.48.4 (3.48.4-1.fc38) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Implement element EOS\n\thandling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Nicolas Dufresne via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Nicolas Dufresne <nicolas.dufresne@collabora.com>","Cc":"Jaslo Ziska <jaslo@ziska.de>, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]