[{"id":3667,"web_url":"https://patchwork.libcamera.org/comment/3667/","msgid":"<20200211195030.GF20823@pendragon.ideasonboard.com>","date":"2020-02-11T19:50:30","subject":"Re: [libcamera-devel] [PATCH v1 10/23] gst: libcamerasrc:\n\tStart/Stop the streaming thread","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nThank you for the patch.\n\nThe commit message mentions start/stop, but the patch doesn't actually\nstart or stop the task. Should it be rewritten to instead say \"Add a\ntask for the streaming thread\", or something similar ?\n\nOn Tue, Jan 28, 2020 at 10:31:57PM -0500, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne \n> \n> Use a GstTask as our internal streaming thread. Unlike GstBaseSrc, we\n> will be running an streaming thread at the element level rather then\n\ns/an/a/\ns/then/than/\n\n> per pad. This is needed to combine buffer request for multiple pads.\n> \n> Signed-off-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 45 +++++++++++++++++++++++++++++++\n> 1 file changed, 45 insertions(+)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index abb376b..4d0c7d0 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -26,7 +26,11 @@ struct GstLibcameraSrcState {\n> \n> struct _GstLibcameraSrc {\n> \tGstElement parent;\n> +\n> +\tGRecMutex stream_lock;\n> +\tGstTask *task;\n> \tGstPad *srcpad;\n> +\n> \tgchar *camera_name;\n> \n> \tGstLibcameraSrcState *state;\n> @@ -112,6 +116,30 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n> \treturn true;\n> }\n> \n> +static void\n> +gst_libcamera_src_task_run(gpointer user_data)\n> +{\n> +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> +\n> +\tGST_DEBUG_OBJECT(self, \"Streaming thread it now capturing\");\n\ns/it/is/ ?\n\n> +}\n> +\n> +static void\n> +gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n> +{\n> +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> +\n> +\tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> +}\n> +\n> +static void\n> +gst_libcamera_src_task_leave(GstTask *task, GThread *thread, gpointer user_data)\n> +{\n> +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> +\n> +\tGST_DEBUG_OBJECT(self, \"Streaming thread is about to stop\");\n> +}\n> +\n> static void\n> gst_libcamera_src_close(GstLibcameraSrc *self)\n> {\n> @@ -182,9 +210,18 @@ gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n> \t\t\treturn GST_STATE_CHANGE_FAILURE;\n> \t\tbreak;\n> \tcase GST_STATE_CHANGE_READY_TO_PAUSED:\n> +\t\t/* This needs to be called after pads activation */\n\ns/activation/activation./\n\n(I'll stop mentioning this too, could you please apply it to the other\npatches too ?)\n\n> +\t\tif (!gst_task_pause(self->task))\n> +\t\t\treturn GST_STATE_CHANGE_FAILURE;\n> +\t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> +\t\tbreak;\n> \tcase GST_STATE_CHANGE_PLAYING_TO_PAUSED:\n> \t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> \t\tbreak;\n> +\tcase GST_STATE_CHANGE_PAUSED_TO_READY:\n> +\t\t/* FIXME maybe this should happen before pad deactivation ? */\n\nThis can be fixed later if needed, but I'm curious to know what this\nmeans :-) Is there anything that prevents fixing this now ? Or is it\njust a matter of investigating whether this is required or not ? A\n\"\\todo Investigate if the task should be joined before pad deactivation\"\nwould be better in that case.\n\n> +\t\tgst_task_join(self->task);\n> +\t\tbreak;\n> \tcase GST_STATE_CHANGE_READY_TO_NULL:\n> \t\tgst_libcamera_src_close(self);\n> \t\tbreak;\n> @@ -201,6 +238,8 @@ gst_libcamera_src_finalize(GObject *object)\n> \tGObjectClass *klass = G_OBJECT_CLASS(gst_libcamera_src_parent_class);\n> \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(object);\n> \n> +\tg_rec_mutex_clear(&self->stream_lock);\n> +\tg_clear_object(&self->task);\n> \tg_free(self->camera_name);\n> \tdelete self->state;\n> \n> @@ -213,6 +252,12 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> \tGstLibcameraSrcState *state = new GstLibcameraSrcState();\n> \tGstPadTemplate *templ = gst_element_get_pad_template(GST_ELEMENT(self), \"src\");\n> \n> +\tg_rec_mutex_init(&self->stream_lock);\n> +\tself->task = gst_task_new(gst_libcamera_src_task_run, self, nullptr);\n> +\tgst_task_set_enter_callback(self->task, gst_libcamera_src_task_enter, self, nullptr);\n> +\tgst_task_set_leave_callback(self->task, gst_libcamera_src_task_leave, self, nullptr);\n> +\tgst_task_set_lock(self->task, &self->stream_lock);\n> +\n> \tself->srcpad = gst_pad_new_from_template(templ, \"src\");\n> \tgst_element_add_pad(GST_ELEMENT(self), self->srcpad);\n> \tself->state = state;","headers":{"Return-Path":"","Received":["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 A50EE60F3C\n\tfor ;\n\tTue, 11 Feb 2020 20:50:47 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AF4E19DA;\n\tTue, 11 Feb 2020 20:50:46 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581450647;\n\tbh=6PuF0L5yL8QeBJkO09Kmn1FlCFOBeT5gfk0/hKrmLic=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=suFS16r5pghdKGNpHUHohLbPzOuVbzTtznXt9hYvHb4nZT0E+iL3P60Iy3lx2Xe0n\n\th8/8v90KHNpXHtuiAaM9MMyGbRywZjCfglPuAoPvkh7TYTYPOcD7ZPGztE2H+VZCar\n\t6qlcClYZnGEIn1utoHtl+V/isjl/Yfcn+fDNG2No=","Date":"Tue, 11 Feb 2020 21:50:30 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Message-ID":"<20200211195030.GF20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-11-nicolas@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200129033210.278800-11-nicolas@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 10/23] gst: libcamerasrc:\n\tStart/Stop the streaming thread","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Feb 2020 19:50:47 -0000"}},{"id":3673,"web_url":"https://patchwork.libcamera.org/comment/3673/","msgid":"","date":"2020-02-11T22:38:32","subject":"Re: [libcamera-devel] [PATCH v1 10/23] gst: libcamerasrc:\n\tStart/Stop the streaming thread","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"On mar, 2020-02-11 at 21:50 +0200, Laurent Pinchart wrote:\n> Hi Nicolas,\n> \n> Thank you for the patch.\n> \n> The commit message mentions start/stop, but the patch doesn't actually\n> start or stop the task. Should it be rewritten to instead say \"Add a\n> task for the streaming thread\", or something similar ?\n\nWe do gst_task_pause(), which effectively start the thread (enter callback is\ncalled), and gst_task_join() which do gst_task_stop() and wait (leave callback\nis called). Your suggestion is good though, it's an implementation detail that\nit does start and stop a thread.\n\n> \n> On Tue, Jan 28, 2020 at 10:31:57PM -0500, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne \n> > \n> > Use a GstTask as our internal streaming thread. Unlike GstBaseSrc, we\n> > will be running an streaming thread at the element level rather then\n> \n> s/an/a/\n> s/then/than/\n> \n> > per pad. This is needed to combine buffer request for multiple pads.\n> > \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 45 +++++++++++++++++++++++++++++++\n> > 1 file changed, 45 insertions(+)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > b/src/gstreamer/gstlibcamerasrc.cpp\n> > index abb376b..4d0c7d0 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -26,7 +26,11 @@ struct GstLibcameraSrcState {\n> > \n> > struct _GstLibcameraSrc {\n> > \tGstElement parent;\n> > +\n> > +\tGRecMutex stream_lock;\n> > +\tGstTask *task;\n> > \tGstPad *srcpad;\n> > +\n> > \tgchar *camera_name;\n> > \n> > \tGstLibcameraSrcState *state;\n> > @@ -112,6 +116,30 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n> > \treturn true;\n> > }\n> > \n> > +static void\n> > +gst_libcamera_src_task_run(gpointer user_data)\n> > +{\n> > +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > +\n> > +\tGST_DEBUG_OBJECT(self, \"Streaming thread it now capturing\");\n> \n> s/it/is/ ?\n> \n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer\n> > user_data)\n> > +{\n> > +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > +\n> > +\tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_src_task_leave(GstTask *task, GThread *thread, gpointer\n> > user_data)\n> > +{\n> > +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > +\n> > +\tGST_DEBUG_OBJECT(self, \"Streaming thread is about to stop\");\n> > +}\n> > +\n> > static void\n> > gst_libcamera_src_close(GstLibcameraSrc *self)\n> > {\n> > @@ -182,9 +210,18 @@ gst_libcamera_src_change_state(GstElement *element,\n> > GstStateChange transition)\n> > \t\t\treturn GST_STATE_CHANGE_FAILURE;\n> > \t\tbreak;\n> > \tcase GST_STATE_CHANGE_READY_TO_PAUSED:\n> > +\t\t/* This needs to be called after pads activation */\n> \n> s/activation/activation./\n> \n> (I'll stop mentioning this too, could you please apply it to the other\n> patches too ?)\n> \n> > +\t\tif (!gst_task_pause(self->task))\n> > +\t\t\treturn GST_STATE_CHANGE_FAILURE;\n> > +\t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> > +\t\tbreak;\n> > \tcase GST_STATE_CHANGE_PLAYING_TO_PAUSED:\n> > \t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> > \t\tbreak;\n> > +\tcase GST_STATE_CHANGE_PAUSED_TO_READY:\n> > +\t\t/* FIXME maybe this should happen before pad deactivation ? */\n> \n> This can be fixed later if needed, but I'm curious to know what this\n> means :-) Is there anything that prevents fixing this now ? Or is it\n> just a matter of investigating whether this is required or not ? A\n> \"\\todo Investigate if the task should be joined before pad deactivation\"\n> would be better in that case.\n\nI might be able to remove it in V2. For sources using BaseSrc, we often do\npoll() or select() in the create() function. Create function is basically the\nrun function of a GstTask (with special BaseSrc wrapper). What one needs to know\nhere is that this transition is responsible of deactivating pads. To deactivate\npads, you need to take the stream lock (a recursive mutex on the pads). This\npads is taken each time the run function of a task is called. If you don't\ncancel any blocking waits, the pad deactivation will block, slowing down the\ndeactivation, or blocking forever in some cases. To unblock these things in the\nstate transition, you have to do so before chaining to the base class\n(GstElement), hence \"before pad deactivation\".\n\nThere is effectively a upper and a lowe part to the state transition virtual\nfunction implementation. In this case, it seems I only need the lower part. For\nthreaded filters, when handling a flush-start/stop sequence, it is much more\ninvolving.\n\nNow I had no idea what it would look like in the end when I wrote this. What it\nended up is that libcamera calls us from it's own thread. In that thread we\nqueue and wakeup the task (take it out of pause). All I had to do is to make\nsure it's not possible to wakup the task after we have decided to shut it down.\nI think this is covered in later patch, and so there is nothing to unlock ever.\nAnd this comment can be removed.\n\n> \n> > +\t\tgst_task_join(self->task);\n> > +\t\tbreak;\n> > \tcase GST_STATE_CHANGE_READY_TO_NULL:\n> > \t\tgst_libcamera_src_close(self);\n> > \t\tbreak;\n> > @@ -201,6 +238,8 @@ gst_libcamera_src_finalize(GObject *object)\n> > \tGObjectClass *klass = G_OBJECT_CLASS(gst_libcamera_src_parent_class);\n> > \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(object);\n> > \n> > +\tg_rec_mutex_clear(&self->stream_lock);\n> > +\tg_clear_object(&self->task);\n> > \tg_free(self->camera_name);\n> > \tdelete self->state;\n> > \n> > @@ -213,6 +252,12 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> > \tGstLibcameraSrcState *state = new GstLibcameraSrcState();\n> > \tGstPadTemplate *templ = gst_element_get_pad_template(GST_ELEMENT(self),\n> > \"src\");\n> > \n> > +\tg_rec_mutex_init(&self->stream_lock);\n> > +\tself->task = gst_task_new(gst_libcamera_src_task_run, self, nullptr);\n> > +\tgst_task_set_enter_callback(self->task, gst_libcamera_src_task_enter,\n> > self, nullptr);\n> > +\tgst_task_set_leave_callback(self->task, gst_libcamera_src_task_leave,\n> > self, nullptr);\n> > +\tgst_task_set_lock(self->task, &self->stream_lock);\n> > +\n> > \tself->srcpad = gst_pad_new_from_template(templ, \"src\");\n> > \tgst_element_add_pad(GST_ELEMENT(self), self->srcpad);\n> > \tself->state = state;","headers":{"Return-Path":"","Received":["from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8312A60F3C\n\tfor ;\n\tTue, 11 Feb 2020 23:38:42 +0100 (CET)","from nicolas-tpx395.localdomain (unknown [IPv6:2610:98:8005::527])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id DCC8428FC8E;\n\tTue, 11 Feb 2020 22:38:41 +0000 (GMT)"],"Message-ID":"","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 11 Feb 2020 17:38:32 -0500","In-Reply-To":"<20200211195030.GF20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-11-nicolas@ndufresne.ca>\n\t<20200211195030.GF20823@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.3 (3.34.3-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v1 10/23] gst: libcamerasrc:\n\tStart/Stop the streaming thread","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Feb 2020 22:38:42 -0000"}}]