[{"id":18477,"web_url":"https://patchwork.libcamera.org/comment/18477/","msgid":"<20210802044640.GP2167@pyrite.rasen.tech>","date":"2021-08-02T04:46:40","subject":"Re: [libcamera-devel] [RFC PATCH v2] gstreamer: Factor out\n\t_pad_push_stream_start from _task_enter","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Vedant,\n\nOn Mon, Aug 02, 2021 at 12:21:44AM +0530, Vedant Paranjape wrote:\n> This patch creates gst_libcamera_pad_push_stream_start function to\n> create stream id and to push the stream start.\n> \n> Update functional code in gst_libcamera_src_task_enter(), which creates\n> stream id and pushes the stream start with the refactored\n> function gst_libcamera_pad_push_stream_start().\n\nThe code looks better than before, but I'm not quite seeing the\nrationale. Why is this change important? Perhaps you told me before, but\nthe \"why\" is what should be in the commit message, so that reviewers can\nsee. To some extent, the \"what\" isn't as important, since the reviewer\ncan simply read the patch (though it is useful when the patch is long\nand complex).\n\n\nPaul\n\n> \n> Signed-off-by: Vedant Paranjape \n> ---\n> src/gstreamer/gstlibcamerapad.cpp | 19 +++++++++++++++++++\n> src/gstreamer/gstlibcamerapad.h | 2 ++\n> src/gstreamer/gstlibcamerasrc.cpp | 7 +------\n> 3 files changed, 22 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp\n> index c00e81c8..d4211050 100644\n> --- a/src/gstreamer/gstlibcamerapad.cpp\n> +++ b/src/gstreamer/gstlibcamerapad.cpp\n> @@ -20,6 +20,7 @@ struct _GstLibcameraPad {\n> \tGstLibcameraPool *pool;\n> \tGQueue pending_buffers;\n> \tGstClockTime latency;\n> +\tgint stream_id_num;\n> };\n> \n> enum {\n> @@ -81,6 +82,7 @@ static void\n> gst_libcamera_pad_init(GstLibcameraPad *self)\n> {\n> \tGST_PAD_QUERYFUNC(self) = gst_libcamera_pad_query;\n> +\tself->stream_id_num = 0;\n> }\n> \n> static GType\n> @@ -155,6 +157,23 @@ gst_libcamera_pad_get_stream(GstPad *pad)\n> \treturn nullptr;\n> }\n> \n> +void\n> +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint group_id)\n> +{\n> +\tauto *self = GST_LIBCAMERA_PAD(pad);\t\n> +\t{\n> +\t\tGLibLocker lock(GST_OBJECT(self));\n> +\t\tself->stream_id_num++;\n> +\t}\n> +\n> +\tg_autoptr(GstElement) element = gst_pad_get_parent_element(pad);\n> +\tg_autofree gchar *stream_id_intermediate = g_strdup_printf(\"%i%i\", group_id, self->stream_id_num);\n> +\tg_autofree gchar *stream_id = gst_pad_create_stream_id(pad, element, stream_id_intermediate);\n> +\tGstEvent *event = gst_event_new_stream_start(stream_id);\n> +\tgst_event_set_group_id(event, group_id);\n> +\tgst_pad_push_event(pad, event);\n> +}\n> +\n> void\n> gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer)\n> {\n> diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/gstlibcamerapad.h\n> index 779f2d13..7693374f 100644\n> --- a/src/gstreamer/gstlibcamerapad.h\n> +++ b/src/gstreamer/gstlibcamerapad.h\n> @@ -26,6 +26,8 @@ void gst_libcamera_pad_set_pool(GstPad *pad, GstLibcameraPool *pool);\n> \n> libcamera::Stream *gst_libcamera_pad_get_stream(GstPad *pad);\n> \n> +void gst_libcamera_pad_push_stream_start(GstPad *pad, const guint group_id);\n> +\n> void gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer);\n> \n> GstFlowReturn gst_libcamera_pad_push_pending(GstPad *pad);\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index b18f1efb..bb8ea07a 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -361,15 +361,10 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \n> \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> \n> -\tgint stream_id_num = 0;\n> \tStreamRoles roles;\n> \tfor (GstPad *srcpad : state->srcpads_) {\n> \t\t/* Create stream-id and push stream-start. */\n> -\t\tg_autofree gchar *stream_id_intermediate = g_strdup_printf(\"%i%i\", state->group_id_, stream_id_num++);\n> -\t\tg_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), stream_id_intermediate);\n> -\t\tGstEvent *event = gst_event_new_stream_start(stream_id);\n> -\t\tgst_event_set_group_id(event, state->group_id_);\n> -\t\tgst_pad_push_event(srcpad, event);\n> +\t\tgst_libcamera_pad_push_stream_start(srcpad, state->group_id_);\n> \n> \t\t/* Collect the streams roles for the next iteration. */\n> \t\troles.push_back(gst_libcamera_pad_get_role(srcpad));\n> -- \n> 2.25.1\n>","headers":{"Return-Path":"","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 CB60DBD878\n\tfor ;\n\tMon, 2 Aug 2021 04:46:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C29E6026A;\n\tMon, 2 Aug 2021 06:46:49 +0200 (CEST)","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 85DA26026A\n\tfor ;\n\tMon, 2 Aug 2021 06:46:47 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D494D4A3;\n\tMon, 2 Aug 2021 06:46:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"V2EqDbMr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627879607;\n\tbh=2XiI3J2w2dAXJfYu2q5Ri2a/LBkO/iXh2KOJBaJTm0o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V2EqDbMrZ90B6Dzi9xp/BXOdGV4U/RDsNw8/KifX1rArwDdNzzFjhLCjEhvvGVt8u\n\tOfA2hPGOlFwNuYOme/ZoKUYrVzqHU24sjIrEXr5g72IAxuv9Yk++IccPvU8vHy+9Ci\n\tMc5gA/COtucyJki/39x5uOWYjX8P7KVQEOHQc6QI=","Date":"Mon, 2 Aug 2021 13:46:40 +0900","From":"paul.elder@ideasonboard.com","To":"Vedant Paranjape ","Message-ID":"<20210802044640.GP2167@pyrite.rasen.tech>","References":"<20210801185144.21572-1-vedantparanjape160201@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210801185144.21572-1-vedantparanjape160201@gmail.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2] gstreamer: Factor out\n\t_pad_push_stream_start from _task_enter","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18479,"web_url":"https://patchwork.libcamera.org/comment/18479/","msgid":"","date":"2021-08-02T05:56:56","subject":"Re: [libcamera-devel] [RFC PATCH v2] gstreamer: Factor out\n\t_pad_push_stream_start from _task_enter","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Paul,\nThis change is needed as the specific code is pad specific and pretty\ngeneric. Also it might be used again in reconfig function. So to reduce\ncode duplication, this is needed. ood?\n\nI will add this explaination in commit message, does it sound good to you?\n\nRegards,\nVedant Paranjape\n\nOn Mon, 2 Aug, 2021, 10:16 , wrote:\n\n> Hi Vedant,\n>\n> On Mon, Aug 02, 2021 at 12:21:44AM +0530, Vedant Paranjape wrote:\n> > This patch creates gst_libcamera_pad_push_stream_start function to\n> > create stream id and to push the stream start.\n> >\n> > Update functional code in gst_libcamera_src_task_enter(), which creates\n> > stream id and pushes the stream start with the refactored\n> > function gst_libcamera_pad_push_stream_start().\n>\n> The code looks better than before, but I'm not quite seeing the\n> rationale. Why is this change important? Perhaps you told me before, but\n> the \"why\" is what should be in the commit message, so that reviewers can\n> see. To some extent, the \"what\" isn't as important, since the reviewer\n> can simply read the patch (though it is useful when the patch is long\n> and complex).\n>\n>\n> Paul\n>\n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > src/gstreamer/gstlibcamerapad.cpp | 19 +++++++++++++++++++\n> > src/gstreamer/gstlibcamerapad.h | 2 ++\n> > src/gstreamer/gstlibcamerasrc.cpp | 7 +------\n> > 3 files changed, 22 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamerapad.cpp\n> b/src/gstreamer/gstlibcamerapad.cpp\n> > index c00e81c8..d4211050 100644\n> > --- a/src/gstreamer/gstlibcamerapad.cpp\n> > +++ b/src/gstreamer/gstlibcamerapad.cpp\n> > @@ -20,6 +20,7 @@ struct _GstLibcameraPad {\n> > GstLibcameraPool *pool;\n> > GQueue pending_buffers;\n> > GstClockTime latency;\n> > + gint stream_id_num;\n> > };\n> >\n> > enum {\n> > @@ -81,6 +82,7 @@ static void\n> > gst_libcamera_pad_init(GstLibcameraPad *self)\n> > {\n> > GST_PAD_QUERYFUNC(self) = gst_libcamera_pad_query;\n> > + self->stream_id_num = 0;\n> > }\n> >\n> > static GType\n> > @@ -155,6 +157,23 @@ gst_libcamera_pad_get_stream(GstPad *pad)\n> > return nullptr;\n> > }\n> >\n> > +void\n> > +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint group_id)\n> > +{\n> > + auto *self = GST_LIBCAMERA_PAD(pad);\n> > + {\n> > + GLibLocker lock(GST_OBJECT(self));\n> > + self->stream_id_num++;\n> > + }\n> > +\n> > + g_autoptr(GstElement) element = gst_pad_get_parent_element(pad);\n> > + g_autofree gchar *stream_id_intermediate = g_strdup_printf(\"%i%i\",\n> group_id, self->stream_id_num);\n> > + g_autofree gchar *stream_id = gst_pad_create_stream_id(pad,\n> element, stream_id_intermediate);\n> > + GstEvent *event = gst_event_new_stream_start(stream_id);\n> > + gst_event_set_group_id(event, group_id);\n> > + gst_pad_push_event(pad, event);\n> > +}\n> > +\n> > void\n> > gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer)\n> > {\n> > diff --git a/src/gstreamer/gstlibcamerapad.h\n> b/src/gstreamer/gstlibcamerapad.h\n> > index 779f2d13..7693374f 100644\n> > --- a/src/gstreamer/gstlibcamerapad.h\n> > +++ b/src/gstreamer/gstlibcamerapad.h\n> > @@ -26,6 +26,8 @@ void gst_libcamera_pad_set_pool(GstPad *pad,\n> GstLibcameraPool *pool);\n> >\n> > libcamera::Stream *gst_libcamera_pad_get_stream(GstPad *pad);\n> >\n> > +void gst_libcamera_pad_push_stream_start(GstPad *pad, const guint\n> group_id);\n> > +\n> > void gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer);\n> >\n> > GstFlowReturn gst_libcamera_pad_push_pending(GstPad *pad);\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> b/src/gstreamer/gstlibcamerasrc.cpp\n> > index b18f1efb..bb8ea07a 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -361,15 +361,10 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> >\n> > GST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> >\n> > - gint stream_id_num = 0;\n> > StreamRoles roles;\n> > for (GstPad *srcpad : state->srcpads_) {\n> > /* Create stream-id and push stream-start. */\n> > - g_autofree gchar *stream_id_intermediate =\n> g_strdup_printf(\"%i%i\", state->group_id_, stream_id_num++);\n> > - g_autofree gchar *stream_id =\n> gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), stream_id_intermediate);\n> > - GstEvent *event = gst_event_new_stream_start(stream_id);\n> > - gst_event_set_group_id(event, state->group_id_);\n> > - gst_pad_push_event(srcpad, event);\n> > + gst_libcamera_pad_push_stream_start(srcpad,\n> state->group_id_);\n> >\n> > /* Collect the streams roles for the next iteration. */\n> > roles.push_back(gst_libcamera_pad_get_role(srcpad));\n> > --\n> > 2.25.1\n> >\n>","headers":{"Return-Path":"","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 839D5BD878\n\tfor ;\n\tMon, 2 Aug 2021 05:57:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0F98687B6;\n\tMon, 2 Aug 2021 07:57:10 +0200 (CEST)","from mail-yb1-xb2d.google.com (mail-yb1-xb2d.google.com\n\t[IPv6:2607:f8b0:4864:20::b2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67C046026A\n\tfor ;\n\tMon, 2 Aug 2021 07:57:09 +0200 (CEST)","by mail-yb1-xb2d.google.com with SMTP id z128so306565ybc.10\n\tfor ;\n\tSun, 01 Aug 2021 22:57:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"sXxDY6B/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=TufU0cjEiAE1oX91EW0hzeZkA0rz1aAQc8WdzFSFcHk=;\n\tb=sXxDY6B/opwAsTuLdWa0vwiZHix1VM2tZEpEsbxFjrLD+zOR9YFLHrj1b30aVLUP8K\n\tUjWBIuwUTFjplTRwjCY3eCXp1CfCmwKzJYx7WxFc4CcfbPnpNjy03tRiG+71lYdUryht\n\tUDwCfpjWh+GzZ0PqXYrDFTg0/sqqyGDQqVCMJdhAmDld42oQUGDeEn1xy5o6HbRf0v9J\n\tiqUiyE5gSXcXiZYn8WeiJAfS4ZHjEUSAJM1WAc9Inkbd8XZXrRgNY3x7LWdb/96igK94\n\tQJ5J3VddbigFC98ihu8xMBTSoTePVA55BfHB65ONtDWWMAYxsfraWMxxwgRxJckH10Ve\n\taShg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=TufU0cjEiAE1oX91EW0hzeZkA0rz1aAQc8WdzFSFcHk=;\n\tb=gwcbhNSTI732G3HxADxPGHWTyLSj45KjWx/Vm9g5E/yGHrtaq8qewV9SIbN5GB8GOC\n\t6b9WgIWjfeB1dFRmdJdk0GHy7pz0AOxbbMDX83w0gPEEXdVqhGnw1B+3a0DbXiHkWj+U\n\tWNaiqo5RNy7toO4rGlCJVoCURAIisqHqKjTDRq/13VmQ1v3GbNB8mq+n6H2u2kskvyaJ\n\ttwFos3warEmmdr0QhHHterN7W9TST3I3d5zY5BzOUmLEJ0dlDuiy/UoysBPu6ExQJRpa\n\t0j7r68NXnSn7y8zSvfmuMfdlRCvzGURK7zQMNtXCb9lU/C5o1iiF+2/5U0rvhZIH627f\n\tQIyA==","X-Gm-Message-State":"AOAM530gISs0PxuPzVM2Q4bxy/e1/aZVkei96MET2wjwF9c1Lnn8bTUn\n\tx/uz6R2ZAxM9HlNROB1kjwcVBLc2DI1Hvd13fLA=","X-Google-Smtp-Source":"ABdhPJx0/PJcCrqZ84JeeRrCUuu13k6vFnXcZ1gT41MU1/BTPPh+76Ye0Cmka+JfpeGzsNtlkYfmd5wu3ZMETESXlFk=","X-Received":"by 2002:a25:b787:: with SMTP id\n\tn7mr18799378ybh.468.1627883828196; \n\tSun, 01 Aug 2021 22:57:08 -0700 (PDT)","MIME-Version":"1.0","References":"<20210801185144.21572-1-vedantparanjape160201@gmail.com>\n\t<20210802044640.GP2167@pyrite.rasen.tech>","In-Reply-To":"<20210802044640.GP2167@pyrite.rasen.tech>","From":"Vedant Paranjape ","Date":"Mon, 2 Aug 2021 11:26:56 +0530","Message-ID":"","To":"Paul Elder ","Content-Type":"multipart/alternative; boundary=\"00000000000050b9f505c88d3da3\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2] gstreamer: Factor out\n\t_pad_push_stream_start from _task_enter","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18480,"web_url":"https://patchwork.libcamera.org/comment/18480/","msgid":"<20210802065633.GR2167@pyrite.rasen.tech>","date":"2021-08-02T06:56:33","subject":"Re: [libcamera-devel] [RFC PATCH v2] gstreamer: Factor out\n\t_pad_push_stream_start from _task_enter","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Vedant,\n\nOn Mon, Aug 02, 2021 at 11:26:56AM +0530, Vedant Paranjape wrote:\n> Hi Paul,\n> This change is needed as the specific code is pad specific and pretty generic.\n\n\"pad-specific\" and \"generic\"? :D\n\nAs in, the code is specific to pads (so it belongs in pad and not src),\nand generic as a function for pads?\n\n> Also it might be used again in reconfig function. So to reduce code\n\ns/might/will ?\n\n> duplication, this is needed. ood?\n> \n> I will add this explaination in commit message, does it sound good to you?\n\niirc you also said there was some significance in changing the stream\nid. Explaining that in the commit message would be good too.\n\n\nPaul\n\n> \n> On Mon, 2 Aug, 2021, 10:16 , wrote:\n> \n> Hi Vedant,\n> \n> On Mon, Aug 02, 2021 at 12:21:44AM +0530, Vedant Paranjape wrote:\n> > This patch creates gst_libcamera_pad_push_stream_start function to\n> > create stream id and to push the stream start.\n> >\n> > Update functional code in gst_libcamera_src_task_enter(), which creates\n> > stream id and pushes the stream start with the refactored\n> > function gst_libcamera_pad_push_stream_start().\n> \n> The code looks better than before, but I'm not quite seeing the\n> rationale. Why is this change important? Perhaps you told me before, but\n> the \"why\" is what should be in the commit message, so that reviewers can\n> see. To some extent, the \"what\" isn't as important, since the reviewer\n> can simply read the patch (though it is useful when the patch is long\n> and complex).\n> \n> \n> Paul\n> \n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> >  src/gstreamer/gstlibcamerapad.cpp | 19 +++++++++++++++++++\n> >  src/gstreamer/gstlibcamerapad.h   |  2 ++\n> >  src/gstreamer/gstlibcamerasrc.cpp |  7 +------\n> >  3 files changed, 22 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/\n> gstlibcamerapad.cpp\n> > index c00e81c8..d4211050 100644\n> > --- a/src/gstreamer/gstlibcamerapad.cpp\n> > +++ b/src/gstreamer/gstlibcamerapad.cpp\n> > @@ -20,6 +20,7 @@ struct _GstLibcameraPad {\n> >       GstLibcameraPool *pool;\n> >       GQueue pending_buffers;\n> >       GstClockTime latency;\n> > +     gint stream_id_num;\n> >  };\n> > \n> >  enum {\n> > @@ -81,6 +82,7 @@ static void\n> >  gst_libcamera_pad_init(GstLibcameraPad *self)\n> >  {\n> >       GST_PAD_QUERYFUNC(self) = gst_libcamera_pad_query;\n> > +     self->stream_id_num = 0;\n> >  }\n> > \n> >  static GType\n> > @@ -155,6 +157,23 @@ gst_libcamera_pad_get_stream(GstPad *pad)\n> >       return nullptr;\n> >  }\n> > \n> > +void\n> > +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint group_id)\n> > +{\n> > +     auto *self = GST_LIBCAMERA_PAD(pad);   \n> > +     {\n> > +             GLibLocker lock(GST_OBJECT(self));\n> > +             self->stream_id_num++;\n> > +     }\n> > +\n> > +     g_autoptr(GstElement) element = gst_pad_get_parent_element(pad);\n> > +     g_autofree gchar *stream_id_intermediate = g_strdup_printf(\"%i%i\",\n> group_id, self->stream_id_num);\n> > +     g_autofree gchar *stream_id = gst_pad_create_stream_id(pad,\n> element, stream_id_intermediate);\n> > +     GstEvent *event = gst_event_new_stream_start(stream_id);\n> > +     gst_event_set_group_id(event, group_id);\n> > +     gst_pad_push_event(pad, event);\n> > +}\n> > +\n> >  void\n> >  gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer)\n> >  {\n> > diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/\n> gstlibcamerapad.h\n> > index 779f2d13..7693374f 100644\n> > --- a/src/gstreamer/gstlibcamerapad.h\n> > +++ b/src/gstreamer/gstlibcamerapad.h\n> > @@ -26,6 +26,8 @@ void gst_libcamera_pad_set_pool(GstPad *pad,\n> GstLibcameraPool *pool);\n> > \n> >  libcamera::Stream *gst_libcamera_pad_get_stream(GstPad *pad);\n> > \n> > +void gst_libcamera_pad_push_stream_start(GstPad *pad, const guint\n> group_id);\n> > +\n> >  void gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer);\n> > \n> >  GstFlowReturn gst_libcamera_pad_push_pending(GstPad *pad);\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/\n> gstlibcamerasrc.cpp\n> > index b18f1efb..bb8ea07a 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -361,15 +361,10 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> > \n> >       GST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> > \n> > -     gint stream_id_num = 0;\n> >       StreamRoles roles;\n> >       for (GstPad *srcpad : state->srcpads_) {\n> >               /* Create stream-id and push stream-start. */\n> > -             g_autofree gchar *stream_id_intermediate = g_strdup_printf\n> (\"%i%i\", state->group_id_, stream_id_num++);\n> > -             g_autofree gchar *stream_id = gst_pad_create_stream_id\n> (srcpad, GST_ELEMENT(self), stream_id_intermediate);\n> > -             GstEvent *event = gst_event_new_stream_start(stream_id);\n> > -             gst_event_set_group_id(event, state->group_id_);\n> > -             gst_pad_push_event(srcpad, event);\n> > +             gst_libcamera_pad_push_stream_start(srcpad, state->\n> group_id_);\n> > \n> >               /* Collect the streams roles for the next iteration. */\n> >               roles.push_back(gst_libcamera_pad_get_role(srcpad));\n> > --\n> > 2.25.1\n> >\n>","headers":{"Return-Path":"","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 225B1C3232\n\tfor ;\n\tMon, 2 Aug 2021 06:56:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C386687BF;\n\tMon, 2 Aug 2021 08:56:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82CDD6026A\n\tfor ;\n\tMon, 2 Aug 2021 08:56:41 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A6BD74A3;\n\tMon, 2 Aug 2021 08:56:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Bqr4kcS5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627887401;\n\tbh=CRyAOeQXMkVs8m0fIPVe/Ff2ufe7bn27VkhmIdV/w8I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Bqr4kcS53wJo+wlf7UPfJ1PDPEemekp1FDcPzLB6Q8vjjREJTtouGQJLAb4S3Yvpr\n\t8cWYiO308VcwSEgvg9sflnplki/KZ03URSiR01HGwke1ze25NFQBztamrxJGBHpgNn\n\tEjfFiEkFlrgwxc2zezMfW9V1odZRQMoELNYhZ8L4=","Date":"Mon, 2 Aug 2021 15:56:33 +0900","From":"paul.elder@ideasonboard.com","To":"Vedant Paranjape ","Message-ID":"<20210802065633.GR2167@pyrite.rasen.tech>","References":"<20210801185144.21572-1-vedantparanjape160201@gmail.com>\n\t<20210802044640.GP2167@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2] gstreamer: Factor out\n\t_pad_push_stream_start from _task_enter","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18484,"web_url":"https://patchwork.libcamera.org/comment/18484/","msgid":"","date":"2021-08-02T07:15:50","subject":"Re: [libcamera-devel] [RFC PATCH v2] gstreamer: Factor out\n\t_pad_push_stream_start from _task_enter","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Paul\n\nOn Mon, 2 Aug, 2021, 12:26 , wrote:\n\n> Hi Vedant,\n>\n> On Mon, Aug 02, 2021 at 11:26:56AM +0530, Vedant Paranjape wrote:\n> > Hi Paul,\n> > This change is needed as the specific code is pad specific and pretty\n> generic.\n>\n> \"pad-specific\" and \"generic\"? :D\n\n\nOops, I meant the \"the pad-specific code\" is used in several places and not\nlibcamera specific.\n\n\n> As in, the code is specific to pads (so it belongs in pad and not src),\n> and generic as a function for pads?\n>\n\nYes, it should belong to the pads. Generic as in \"typical gstreamer code\nfor pads\", I meant to say in the sense that it can be reused in lot of\nplaces again.\n\n\n> > Also it might be used again in reconfig function. So to reduce code\n>\n> s/might/will ?\n>\n> > duplication, this is needed. ood?\n> >\n> > I will add this explaination in commit message, does it sound good to\n> you?\n>\n> iirc you also said there was some significance in changing the stream\n> id. Explaining that in the commit message would be good too.\n>\n\nYup, the one where I said, \"stream id is something unique and should stay\ndifferent for all streams during lifetime of a application\", right ?\n\nRegards\nVedant\n\nPaul\n>\n> >\n> > On Mon, 2 Aug, 2021, 10:16 , wrote:\n> >\n> > Hi Vedant,\n> >\n> > On Mon, Aug 02, 2021 at 12:21:44AM +0530, Vedant Paranjape wrote:\n> > > This patch creates gst_libcamera_pad_push_stream_start function to\n> > > create stream id and to push the stream start.\n> > >\n> > > Update functional code in gst_libcamera_src_task_enter(), which\n> creates\n> > > stream id and pushes the stream start with the refactored\n> > > function gst_libcamera_pad_push_stream_start().\n> >\n> > The code looks better than before, but I'm not quite seeing the\n> > rationale. Why is this change important? Perhaps you told me before,\n> but\n> > the \"why\" is what should be in the commit message, so that reviewers\n> can\n> > see. To some extent, the \"what\" isn't as important, since the\n> reviewer\n> > can simply read the patch (though it is useful when the patch is long\n> > and complex).\n> >\n> >\n> > Paul\n> >\n> > >\n> > > Signed-off-by: Vedant Paranjape \n> > > ---\n> > > src/gstreamer/gstlibcamerapad.cpp | 19 +++++++++++++++++++\n> > > src/gstreamer/gstlibcamerapad.h | 2 ++\n> > > src/gstreamer/gstlibcamerasrc.cpp | 7 +------\n> > > 3 files changed, 22 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/\n> > gstlibcamerapad.cpp\n> > > index c00e81c8..d4211050 100644\n> > > --- a/src/gstreamer/gstlibcamerapad.cpp\n> > > +++ b/src/gstreamer/gstlibcamerapad.cpp\n> > > @@ -20,6 +20,7 @@ struct _GstLibcameraPad {\n> > > GstLibcameraPool *pool;\n> > > GQueue pending_buffers;\n> > > GstClockTime latency;\n> > > + gint stream_id_num;\n> > > };\n> > >\n> > > enum {\n> > > @@ -81,6 +82,7 @@ static void\n> > > gst_libcamera_pad_init(GstLibcameraPad *self)\n> > > {\n> > > GST_PAD_QUERYFUNC(self) = gst_libcamera_pad_query;\n> > > + self->stream_id_num = 0;\n> > > }\n> > >\n> > > static GType\n> > > @@ -155,6 +157,23 @@ gst_libcamera_pad_get_stream(GstPad *pad)\n> > > return nullptr;\n> > > }\n> > >\n> > > +void\n> > > +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint\n> group_id)\n> > > +{\n> > > + auto *self = GST_LIBCAMERA_PAD(pad);\n> > > + {\n> > > + GLibLocker lock(GST_OBJECT(self));\n> > > + self->stream_id_num++;\n> > > + }\n> > > +\n> > > + g_autoptr(GstElement) element =\n> gst_pad_get_parent_element(pad);\n> > > + g_autofree gchar *stream_id_intermediate =\n> g_strdup_printf(\"%i%i\",\n> > group_id, self->stream_id_num);\n> > > + g_autofree gchar *stream_id = gst_pad_create_stream_id(pad,\n> > element, stream_id_intermediate);\n> > > + GstEvent *event = gst_event_new_stream_start(stream_id);\n> > > + gst_event_set_group_id(event, group_id);\n> > > + gst_pad_push_event(pad, event);\n> > > +}\n> > > +\n> > > void\n> > > gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer)\n> > > {\n> > > diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/\n> > gstlibcamerapad.h\n> > > index 779f2d13..7693374f 100644\n> > > --- a/src/gstreamer/gstlibcamerapad.h\n> > > +++ b/src/gstreamer/gstlibcamerapad.h\n> > > @@ -26,6 +26,8 @@ void gst_libcamera_pad_set_pool(GstPad *pad,\n> > GstLibcameraPool *pool);\n> > >\n> > > libcamera::Stream *gst_libcamera_pad_get_stream(GstPad *pad);\n> > >\n> > > +void gst_libcamera_pad_push_stream_start(GstPad *pad, const guint\n> > group_id);\n> > > +\n> > > void gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer\n> *buffer);\n> > >\n> > > GstFlowReturn gst_libcamera_pad_push_pending(GstPad *pad);\n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/\n> > gstlibcamerasrc.cpp\n> > > index b18f1efb..bb8ea07a 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -361,15 +361,10 @@ gst_libcamera_src_task_enter(GstTask *task,\n> > [[maybe_unused]] GThread *thread,\n> > >\n> > > GST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> > >\n> > > - gint stream_id_num = 0;\n> > > StreamRoles roles;\n> > > for (GstPad *srcpad : state->srcpads_) {\n> > > /* Create stream-id and push stream-start. */\n> > > - g_autofree gchar *stream_id_intermediate =\n> g_strdup_printf\n> > (\"%i%i\", state->group_id_, stream_id_num++);\n> > > - g_autofree gchar *stream_id =\n> gst_pad_create_stream_id\n> > (srcpad, GST_ELEMENT(self), stream_id_intermediate);\n> > > - GstEvent *event =\n> gst_event_new_stream_start(stream_id);\n> > > - gst_event_set_group_id(event, state->group_id_);\n> > > - gst_pad_push_event(srcpad, event);\n> > > + gst_libcamera_pad_push_stream_start(srcpad, state->\n> > group_id_);\n> > >\n> > > /* Collect the streams roles for the next iteration.\n> */\n> > > roles.push_back(gst_libcamera_pad_get_role(srcpad));\n> > > --\n> > > 2.25.1\n> > >\n> >\n>","headers":{"Return-Path":"","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 626A9BD878\n\tfor ;\n\tMon, 2 Aug 2021 07:16:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC06E687B6;\n\tMon, 2 Aug 2021 09:16:04 +0200 (CEST)","from mail-yb1-xb2a.google.com (mail-yb1-xb2a.google.com\n\t[IPv6:2607:f8b0:4864:20::b2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E05636026E\n\tfor ;\n\tMon, 2 Aug 2021 09:16:02 +0200 (CEST)","by mail-yb1-xb2a.google.com with SMTP id m193so12367057ybf.9\n\tfor ;\n\tMon, 02 Aug 2021 00:16:02 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"ptz08OWM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=zaxPmDslqxhRZLQ3qk3/8NmlYwUPXT353nzcZrCtDfY=;\n\tb=ptz08OWMPkr66th7Mg9+7O4+9ebEf9vGYVHcaYwl0IU9DGUqSzLxpvkux8Ipa+JoZ7\n\toznVmDHMHbtICfWW0PHM06FJQlyZaw5YcJZfYXsrfDTbyStkmyEI1NgKEM2ugJBtq5qp\n\tXA7pqOcLmNR0Ya0Nn01voiVK1tpJ9/wQIohMuDYKyMY8Uy9/qF67JyPnpPCH8nes7i7r\n\tuFBq1JJG5JespBWbGHAtRGx2i91lEiGihGLpcFosy4LC35D/xEtGNwLlGmamzojzewlX\n\tswX5CzK/UTMHKIJ8DgDbTBFzzzi/umPugrYrnvLGa4FyGBLG/weolHzmhD7mVDV+Oyh0\n\tOA0w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=zaxPmDslqxhRZLQ3qk3/8NmlYwUPXT353nzcZrCtDfY=;\n\tb=hpsBnDLxayYOq33URNJOR0fx2VLZnVXfZD/kZeNir52ZZ+uzpN8o0oe+S+PczSvtoB\n\tSXVpkqAQ+hQHSGIKxZ8tMzGg4g51xMM/EgomVLbViYh+FiqH3XG2W/hugMHow1rbQs4u\n\tt2DeeHW/sSOKg1xWaiPDDn+N8K8MuJqyBMn3nzTpGqpX5MFFaGxzfnUWTXgtpHjbeCbf\n\ttiZRuaErpKp96qZLQ0KkM63dwU4MHZ2mZ6Y1io4Pwym2Z9isb011gig3GuSuJuQJLCMK\n\tGmrbOxpTFeFTs7gCFoOqH5eFeYbKxTFgU3gs9z4UpW7E7u3f2OldZAwC5z+UTW1SjHv5\n\tPcvg==","X-Gm-Message-State":"AOAM530JNxQrvqfXkn10HX9ZH+8tq1WTfFnuC+sHLQxDBfAqMOCVXewb\n\tENYY8FTYESEaF9Lo/OK8Z8+DlLzP928O3oZQXKyUldeWveiq8Q==","X-Google-Smtp-Source":"ABdhPJyKaBJqpBkEsBe6nURLzaJcNE72dv3WnzjtKJHXZAZS+4U+Uwz/qZme6QGAecWar82il/DKx9Mdhpxc5A44HfE=","X-Received":"by 2002:a5b:c52:: with SMTP id\n\td18mr20412285ybr.248.1627888561720; \n\tMon, 02 Aug 2021 00:16:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20210801185144.21572-1-vedantparanjape160201@gmail.com>\n\t<20210802044640.GP2167@pyrite.rasen.tech>\n\t\n\t<20210802065633.GR2167@pyrite.rasen.tech>","In-Reply-To":"<20210802065633.GR2167@pyrite.rasen.tech>","From":"Vedant Paranjape ","Date":"Mon, 2 Aug 2021 12:45:50 +0530","Message-ID":"","To":"Paul Elder ","Content-Type":"multipart/alternative; boundary=\"0000000000007490f205c88e57f6\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2] gstreamer: Factor out\n\t_pad_push_stream_start from _task_enter","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]