[{"id":3681,"web_url":"https://patchwork.libcamera.org/comment/3681/","msgid":"<20200211234742.GO20823@pendragon.ideasonboard.com>","date":"2020-02-11T23:47:42","subject":"Re: [libcamera-devel] [PATCH v1 15/23] gst: libcamerasrc: Implement\n\tminimal caps negotiation","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\nOn Tue, Jan 28, 2020 at 10:32:02PM -0500, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne \n> \n> This is not expected to work in every possible cases, but should be sufficient as\n> an initial implementation. What is does it that it turns the StreamFormats into\n\ns/is does it/it does is/\n\n> caps and query downstream caps with that as a filter.\n\ns/query/queries/\n\n> The result is the subset of caps that can be used. We then keep the first\n> structure in that result and fixate using the default values found in\n> StreamConfiguration as a default in case a range is available.\n> \n> We then validate this configuration and turn the potentially modified\n> configuration into caps that we push downstream. Note that we strust the order\n\ns/strust/trust/\n\n> in StreamFormats as being sorted best first, but this is not currently in\n> libcamera.\n\nCould you add a \\todo comment in the code to address this ?\n\n> Signed-off-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 71 +++++++++++++++++++++++++++++++\n> 1 file changed, 71 insertions(+)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index b1a21dc..7b478fa 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -24,6 +24,7 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n> struct GstLibcameraSrcState {\n> \tstd::shared_ptr cm;\n> \tstd::shared_ptr cam;\n> +\tstd::unique_ptr config;\n> \tstd::vector srcpads;\n> };\n> \n> @@ -132,16 +133,86 @@ gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n> \tSTREAM_LOCKER(user_data);\n> \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> \tGstLibcameraSrcState *state = self->state;\n> +\tGstFlowReturn flow_ret = GST_FLOW_OK;\n> \n> \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> \n> \tguint group_id = gst_util_group_id_next();\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 = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), nullptr);\n> \t\tGstEvent *event = gst_event_new_stream_start(stream_id);\n> \t\tgst_event_set_group_id(event, group_id);\n> \t\tgst_pad_push_event(srcpad, event);\n> +\n> +\t\t/* Collect the streams roles for the next iteration */\n> +\t\troles.push_back(gst_libcamera_pad_get_role(srcpad));\n> +\t}\n> +\n> +\t/* Generate the stream configurations, there should be one per pad */\n> +\tstate->config = state->cam->generateConfiguration(roles);\n> +\tg_assert(state->config->size() == state->srcpads.size());\n\nWhat if more pads have been created than the camera can handle ? Is\nthere a way to fail more gracefully than with a g_assert() ? It doesn't\nhave to be implement now, but would be nice in the future. Maybe a \\todo\ncomment with a brief description of how to fix it ?\n\n> +\n> +\tfor (gsize i = 0; i < state->srcpads.size(); i++) {\n> +\t\tGstPad *srcpad = state->srcpads[i];\n> +\t\tStreamConfiguration &stream_cfg = state->config->at(i);\n> +\n> +\t\t/* Retreive the supported caps */\n\ns/Retreive/Retrieve/\n\n> +\t\tg_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> +\t\tg_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter);\n> +\t\tif (gst_caps_is_empty(caps)) {\n> +\t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\t/* Fixate caps and configure the stream */\n> +\t\tcaps = gst_caps_make_writable(caps);\n> +\t\tgst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n> +\t}\n> +\n> +\tif (flow_ret != GST_FLOW_OK)\n> +\t\tgoto done;\n> +\n> +\t/* Validate the configuration */\n> +\tif (state->config->validate() == CameraConfiguration::Invalid) {\n> +\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> +\t\tgoto done;\n> +\t}\n> +\n> +\t/* Regardless if it has been modified, create clean caps and push the\n> +\t * caps event, downstream will decide if hte caps are acceptable */\n\ns/hte/the/\n\nComment style:\n\n\t/*\n\t * ...\n\t */\n\n> +\tfor (gsize i = 0; i < state->srcpads.size(); i++) {\n> +\t\tGstPad *srcpad = state->srcpads[i];\n> +\t\tconst StreamConfiguration &stream_cfg = state->config->at(i);\n> +\n> +\t\tg_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> +\t\tif (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {\n> +\t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tret = state->cam->configure(state->config.get());\n> +\tif (ret) {\n> +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> +\t\t\t\t (\"Failed to configure camera: %s\", g_strerror(-ret)),\n> +\t\t\t\t (\"Camera.configure() failed with error code %i\", ret));\n\ns/.configure/::configure/\n\n> +\t\tgst_task_stop(task);\n> +\t\treturn;\n\nJust for my information, why is there no need to push an EOS events on\npads here ? And what if the previous loop fails and sets flow_ret to\nGST_FLOW_NOT_NEGOTIATED and this then fails, is it expected that the\ndone: label won't be reached in that case ?\n\n> +\t}\n> +\n> +done:\n> +\tswitch (flow_ret) {\n> +\tcase GST_FLOW_NOT_NEGOTIATED:\n> +\t\tfor (GstPad *srcpad : state->srcpads) {\n> +\t\t\tgst_pad_push_event(srcpad, gst_event_new_eos());\n> +\t\t}\n\nNo need for braces.\n\n> +\t\tGST_ELEMENT_FLOW_ERROR(self, flow_ret);\n> +\t\tgst_task_stop(task);\n> +\t\treturn;\n> +\tdefault:\n> +\t\tbreak;\n> \t}\n> }\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3161F61020\n\tfor ;\n\tWed, 12 Feb 2020 00:47:59 +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 860649DA;\n\tWed, 12 Feb 2020 00:47:58 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581464878;\n\tbh=fJqJhbWUC3MPmST9AtsP0Q+6WdSoo/QMsLA/BZJzadE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NKFCO77ZzqNSPdXwePIgPymCYWBN70ndHQuRu2P9Q9SJo4wMip4CEodTC5hsJayM9\n\tStVZ6lP+t4iNJKJMzdM3r3AQ++7Fs+U789zR/FxvJJo7us84RL9HkiMhBuMLHmabkM\n\t9lg18/bqNUuB9TYKTxt8kXpBaTY1pbfwf5Cf4eZU=","Date":"Wed, 12 Feb 2020 01:47:42 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Message-ID":"<20200211234742.GO20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-16-nicolas@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200129033210.278800-16-nicolas@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 15/23] gst: libcamerasrc: Implement\n\tminimal caps negotiation","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 23:47:59 -0000"}},{"id":3692,"web_url":"https://patchwork.libcamera.org/comment/3692/","msgid":"<6258f43f9373be4e0c7c499099785f8f74152849.camel@ndufresne.ca>","date":"2020-02-12T17:30:38","subject":"Re: [libcamera-devel] [PATCH v1 15/23] gst: libcamerasrc: Implement\n\tminimal caps negotiation","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 12 février 2020 à 01:47 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 28, 2020 at 10:32:02PM -0500, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne \n> > \n> > This is not expected to work in every possible cases, but should be sufficient as\n> > an initial implementation. What is does it that it turns the StreamFormats into\n> \n> s/is does it/it does is/\n> \n> > caps and query downstream caps with that as a filter.\n> \n> s/query/queries/\n> \n> > The result is the subset of caps that can be used. We then keep the first\n> > structure in that result and fixate using the default values found in\n> > StreamConfiguration as a default in case a range is available.\n> > \n> > We then validate this configuration and turn the potentially modified\n> > configuration into caps that we push downstream. Note that we strust the order\n> \n> s/strust/trust/\n> \n> > in StreamFormats as being sorted best first, but this is not currently in\n> > libcamera.\n> \n> Could you add a \\todo comment in the code to address this ?\n> \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 71 +++++++++++++++++++++++++++++++\n> > 1 file changed, 71 insertions(+)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index b1a21dc..7b478fa 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -24,6 +24,7 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n> > struct GstLibcameraSrcState {\n> > \tstd::shared_ptr cm;\n> > \tstd::shared_ptr cam;\n> > +\tstd::unique_ptr config;\n> > \tstd::vector srcpads;\n> > };\n> > \n> > @@ -132,16 +133,86 @@ gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n> > \tSTREAM_LOCKER(user_data);\n> > \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > \tGstLibcameraSrcState *state = self->state;\n> > +\tGstFlowReturn flow_ret = GST_FLOW_OK;\n> > \n> > \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> > \n> > \tguint group_id = gst_util_group_id_next();\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 = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), nullptr);\n> > \t\tGstEvent *event = gst_event_new_stream_start(stream_id);\n> > \t\tgst_event_set_group_id(event, group_id);\n> > \t\tgst_pad_push_event(srcpad, event);\n> > +\n> > +\t\t/* Collect the streams roles for the next iteration */\n> > +\t\troles.push_back(gst_libcamera_pad_get_role(srcpad));\n> > +\t}\n> > +\n> > +\t/* Generate the stream configurations, there should be one per pad */\n> > +\tstate->config = state->cam->generateConfiguration(roles);\n> > +\tg_assert(state->config->size() == state->srcpads.size());\n> \n> What if more pads have been created than the camera can handle ? Is\n> there a way to fail more gracefully than with a g_assert() ? It doesn't\n> have to be implement now, but would be nice in the future. Maybe a \\todo\n> comment with a brief description of how to fix it ?\n\nShould add a \\todo, I didn't remember this. In fact, I should check if\nthe size is the same and fail otherwise, there is nothing I can do\nabout this. User wanted N stream, and this is not possible.\n\nI don't think there is any clean way to do it right now. Normally I\nshould reject the pad request, but at that moment, I have no\ninformation about frame size, or anything.\n\nNote that there is no such thing as multiple pads at the moment, that's\nnot implemented yet. I just wanted to have relatively generic code to\navoid the follow up looking like a rewrite.\n\n> \n> > +\n> > +\tfor (gsize i = 0; i < state->srcpads.size(); i++) {\n> > +\t\tGstPad *srcpad = state->srcpads[i];\n> > +\t\tStreamConfiguration &stream_cfg = state->config->at(i);\n> > +\n> > +\t\t/* Retreive the supported caps */\n> \n> s/Retreive/Retrieve/\n> \n> > +\t\tg_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> > +\t\tg_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter);\n> > +\t\tif (gst_caps_is_empty(caps)) {\n> > +\t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\t/* Fixate caps and configure the stream */\n> > +\t\tcaps = gst_caps_make_writable(caps);\n> > +\t\tgst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n> > +\t}\n> > +\n> > +\tif (flow_ret != GST_FLOW_OK)\n> > +\t\tgoto done;\n> > +\n> > +\t/* Validate the configuration */\n> > +\tif (state->config->validate() == CameraConfiguration::Invalid) {\n> > +\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > +\t\tgoto done;\n> > +\t}\n> > +\n> > +\t/* Regardless if it has been modified, create clean caps and push the\n> > +\t * caps event, downstream will decide if hte caps are acceptable */\n> \n> s/hte/the/\n> \n> Comment style:\n> \n> \t/*\n> \t * ...\n> \t */\n> \n> > +\tfor (gsize i = 0; i < state->srcpads.size(); i++) {\n> > +\t\tGstPad *srcpad = state->srcpads[i];\n> > +\t\tconst StreamConfiguration &stream_cfg = state->config->at(i);\n> > +\n> > +\t\tg_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> > +\t\tif (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {\n> > +\t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tret = state->cam->configure(state->config.get());\n> > +\tif (ret) {\n> > +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > +\t\t\t\t (\"Failed to configure camera: %s\", g_strerror(-ret)),\n> > +\t\t\t\t (\"Camera.configure() failed with error code %i\", ret));\n> \n> s/.configure/::configure/\n> \n> > +\t\tgst_task_stop(task);\n> > +\t\treturn;\n> \n> Just for my information, why is there no need to push an EOS events on\n> pads here ? And what if the previous loop fails and sets flow_ret to\n> GST_FLOW_NOT_NEGOTIATED and this then fails, is it expected that the\n> done: label won't be reached in that case ?\n> \n> > +\t}\n> > +\n> > +done:\n> > +\tswitch (flow_ret) {\n> > +\tcase GST_FLOW_NOT_NEGOTIATED:\n> > +\t\tfor (GstPad *srcpad : state->srcpads) {\n> > +\t\t\tgst_pad_push_event(srcpad, gst_event_new_eos());\n> > +\t\t}\n> \n> No need for braces.\n> \n> > +\t\tGST_ELEMENT_FLOW_ERROR(self, flow_ret);\n> > +\t\tgst_task_stop(task);\n> > +\t\treturn;\n> > +\tdefault:\n> > +\t\tbreak;\n> > \t}\n> > }\n> >","headers":{"Return-Path":"","Received":["from mail-qk1-x743.google.com (mail-qk1-x743.google.com\n\t[IPv6:2607:f8b0:4864:20::743])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4CFC6043B\n\tfor ;\n\tWed, 12 Feb 2020 18:30:41 +0100 (CET)","by mail-qk1-x743.google.com with SMTP id c188so2840113qkg.4\n\tfor ;\n\tWed, 12 Feb 2020 09:30:41 -0800 (PST)","from skullcanyon ([192.222.193.21])\n\tby smtp.gmail.com with ESMTPSA id\n\tz16sm536715qkg.87.2020.02.12.09.30.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 12 Feb 2020 09:30:39 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=FEMbaVaIeoj0cS3JtecxE43K9zcb79s5DSk8bFQfhGM=;\n\tb=w9JXGkBYo1t1ZwUuN9po0ncv7s8gf5HqK1OG1piy+bK9aQ6KyzZ5IYR7f1dpFTv32n\n\tdCcFxLTiwEUBLaMJhEsqKB2apTERrqTvWqy8XhJzQNENJqiquLhQhw7qeh3RkEJd3TSE\n\tc0wY/f0tEuZtRFGUYYa3+iZIK42r804ULdMZnxjXdCahm6QYXb0A/xU5pScWgsD1NZcD\n\tzgJ1e5yBuVFWCDSxh0xPYAublZ+bIhnx9843RkhDm/Yu3UCxr7GRyKWOLIshzQz+0Be6\n\tBvimdz5CL86N0OaWrHNO1V5DNertwQ9OSrP6JHAklOFi2OyBezFYzhZ8s9YICvq05A21\n\tW03w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=FEMbaVaIeoj0cS3JtecxE43K9zcb79s5DSk8bFQfhGM=;\n\tb=ZEoe3HJK+FJjNwb2ZNPkuymhkaJLTJT1sAIWjo9YlSPa/Zx7pj4eYskfXwqrBv6po2\n\tiHUXcSj9QIM45EGH7JGx+AFLJ4g7PNN4VJh171HbPWUmR11zPzcEiHLbz4yKQgfjEYXZ\n\tz0QGayoO/a1FQCLFy8x00qbmVN6sibyuhYWfUC6ECjPUbwoGX8A7wGlK2c7LRlK7PW1/\n\tGNn4U4oOZJBs+HYV67cG5M0qNnd966tTQRTEtSlU5C+EdWcRxoIbRnCVyY7/vvmLLW4r\n\t7Qcn8sUwg4xzmsRQ+WECH9bDVX4FwKQlLTwqZWvHQJKM1SBst9XQzNrckkl2ZN47Fuf/\n\ttKSg==","X-Gm-Message-State":"APjAAAUGfWYfDYKj2klCopCLoJwkBebcT+Pp+Z3cb9blRNeGcs5achLb\n\t4IfWTtNCWJyQ+TQXtYUAPDnwROk/dgqp5g==","X-Google-Smtp-Source":"APXvYqwK+yI9vU9C0qWfJKdrJI+rhTAwC7QRVI31wZrvq9xmUV/EDCG18Kxv0GYXObHSwUXI7SVOcA==","X-Received":"by 2002:ae9:ebd8:: with SMTP id\n\tb207mr12216761qkg.353.1581528640418; \n\tWed, 12 Feb 2020 09:30:40 -0800 (PST)","Message-ID":"<6258f43f9373be4e0c7c499099785f8f74152849.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Wed, 12 Feb 2020 12:30:38 -0500","In-Reply-To":"<20200211234742.GO20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-16-nicolas@ndufresne.ca>\n\t<20200211234742.GO20823@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":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 15/23] gst: libcamerasrc: Implement\n\tminimal caps negotiation","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":"Wed, 12 Feb 2020 17:30:42 -0000"}},{"id":3693,"web_url":"https://patchwork.libcamera.org/comment/3693/","msgid":"<3f78b1e716d73e7a833a0ff85f9ffa9166e56ae0.camel@ndufresne.ca>","date":"2020-02-12T17:34:46","subject":"Re: [libcamera-devel] [PATCH v1 15/23] gst: libcamerasrc: Implement\n\tminimal caps negotiation","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 12 février 2020 à 01:47 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 28, 2020 at 10:32:02PM -0500, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne \n> > \n> > This is not expected to work in every possible cases, but should be sufficient as\n> > an initial implementation. What is does it that it turns the StreamFormats into\n> \n> s/is does it/it does is/\n> \n> > caps and query downstream caps with that as a filter.\n> \n> s/query/queries/\n> \n> > The result is the subset of caps that can be used. We then keep the first\n> > structure in that result and fixate using the default values found in\n> > StreamConfiguration as a default in case a range is available.\n> > \n> > We then validate this configuration and turn the potentially modified\n> > configuration into caps that we push downstream. Note that we strust the order\n> \n> s/strust/trust/\n> \n> > in StreamFormats as being sorted best first, but this is not currently in\n> > libcamera.\n> \n> Could you add a \\todo comment in the code to address this ?\n> \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 71 +++++++++++++++++++++++++++++++\n> > 1 file changed, 71 insertions(+)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index b1a21dc..7b478fa 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -24,6 +24,7 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n> > struct GstLibcameraSrcState {\n> > \tstd::shared_ptr cm;\n> > \tstd::shared_ptr cam;\n> > +\tstd::unique_ptr config;\n> > \tstd::vector srcpads;\n> > };\n> > \n> > @@ -132,16 +133,86 @@ gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n> > \tSTREAM_LOCKER(user_data);\n> > \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > \tGstLibcameraSrcState *state = self->state;\n> > +\tGstFlowReturn flow_ret = GST_FLOW_OK;\n> > \n> > \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> > \n> > \tguint group_id = gst_util_group_id_next();\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 = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), nullptr);\n> > \t\tGstEvent *event = gst_event_new_stream_start(stream_id);\n> > \t\tgst_event_set_group_id(event, group_id);\n> > \t\tgst_pad_push_event(srcpad, event);\n> > +\n> > +\t\t/* Collect the streams roles for the next iteration */\n> > +\t\troles.push_back(gst_libcamera_pad_get_role(srcpad));\n> > +\t}\n> > +\n> > +\t/* Generate the stream configurations, there should be one per pad */\n> > +\tstate->config = state->cam->generateConfiguration(roles);\n> > +\tg_assert(state->config->size() == state->srcpads.size());\n> \n> What if more pads have been created than the camera can handle ? Is\n> there a way to fail more gracefully than with a g_assert() ? It doesn't\n> have to be implement now, but would be nice in the future. Maybe a \\todo\n> comment with a brief description of how to fix it ?\n> \n> > +\n> > +\tfor (gsize i = 0; i < state->srcpads.size(); i++) {\n> > +\t\tGstPad *srcpad = state->srcpads[i];\n> > +\t\tStreamConfiguration &stream_cfg = state->config->at(i);\n> > +\n> > +\t\t/* Retreive the supported caps */\n> \n> s/Retreive/Retrieve/\n> \n> > +\t\tg_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> > +\t\tg_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter);\n> > +\t\tif (gst_caps_is_empty(caps)) {\n> > +\t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\t/* Fixate caps and configure the stream */\n> > +\t\tcaps = gst_caps_make_writable(caps);\n> > +\t\tgst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n> > +\t}\n> > +\n> > +\tif (flow_ret != GST_FLOW_OK)\n> > +\t\tgoto done;\n> > +\n> > +\t/* Validate the configuration */\n> > +\tif (state->config->validate() == CameraConfiguration::Invalid) {\n> > +\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > +\t\tgoto done;\n> > +\t}\n> > +\n> > +\t/* Regardless if it has been modified, create clean caps and push the\n> > +\t * caps event, downstream will decide if hte caps are acceptable */\n> \n> s/hte/the/\n> \n> Comment style:\n> \n> \t/*\n> \t * ...\n> \t */\n> \n> > +\tfor (gsize i = 0; i < state->srcpads.size(); i++) {\n> > +\t\tGstPad *srcpad = state->srcpads[i];\n> > +\t\tconst StreamConfiguration &stream_cfg = state->config->at(i);\n> > +\n> > +\t\tg_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> > +\t\tif (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {\n> > +\t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tret = state->cam->configure(state->config.get());\n> > +\tif (ret) {\n> > +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > +\t\t\t\t (\"Failed to configure camera: %s\", g_strerror(-ret)),\n> > +\t\t\t\t (\"Camera.configure() failed with error code %i\", ret));\n> \n> s/.configure/::configure/\n> \n> > +\t\tgst_task_stop(task);\n> > +\t\treturn;\n> \n> Just for my information, why is there no need to push an EOS events on\n> pads here ? And what if the previous loop fails and sets flow_ret to\n> GST_FLOW_NOT_NEGOTIATED and this then fails, is it expected that the\n> done: label won't be reached in that case ?\n\nSorry, clicked send and forgot about this one. I have no idea why there\nis a EOS being sent in the first place, the flow return handling was\ncopied from GstBaseSrc. At this stage, we haven't sent anything on any\npads yet, so it's not clear. Maybe they wanted to terminate the pre-\nroll, maybe not. It pre-dates Git, so no history to figure-out.\n\n> \n> > +\t}\n> > +\n> > +done:\n> > +\tswitch (flow_ret) {\n> > +\tcase GST_FLOW_NOT_NEGOTIATED:\n> > +\t\tfor (GstPad *srcpad : state->srcpads) {\n> > +\t\t\tgst_pad_push_event(srcpad, gst_event_new_eos());\n> > +\t\t}\n> \n> No need for braces.\n\nI'll stop sending EOS for now, as I don't know why we do this.\n\n> \n> > +\t\tGST_ELEMENT_FLOW_ERROR(self, flow_ret);\n> > +\t\tgst_task_stop(task);\n> > +\t\treturn;\n> > +\tdefault:\n> > +\t\tbreak;\n> > \t}\n> > }\n> >","headers":{"Return-Path":"","Received":["from mail-qt1-x844.google.com (mail-qt1-x844.google.com\n\t[IPv6:2607:f8b0:4864:20::844])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 305A96043B\n\tfor ;\n\tWed, 12 Feb 2020 18:34:49 +0100 (CET)","by mail-qt1-x844.google.com with SMTP id d9so2174974qte.12\n\tfor ;\n\tWed, 12 Feb 2020 09:34:49 -0800 (PST)","from skullcanyon ([192.222.193.21])\n\tby smtp.gmail.com with ESMTPSA id\n\tq25sm549617qkc.60.2020.02.12.09.34.46\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 12 Feb 2020 09:34:47 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=6AAoZ10FNEr1sVj78ZF7r04oZX9cLEvD48oFYV8PVk8=;\n\tb=UNU5FF56lKvJDakbIMRyMQI4V10hKx2WFZSl5mvoNqv2pgEkV6i2khflGEfVo0E+S6\n\tmNcF3a6VfoBqiWV2VcfliizPCWkJZrX9XU46qJ0zE1Luuwrtt3pDqRy8kk24xDbckQwZ\n\tf+1ZFHzrsV69nCKn+4EddfuJMY9YEdAp/mKO/drOqmB3ZMvt9SD4KvGTqM6c2kbvPKGc\n\tjLQ0ZwVKyzzm5pYSNUEyS/Sb1VMLOtkAVCMxAm4TteJPOnBzb7mfez9d9QtOB8kxEp2n\n\tXTqGkomG4twWoEzoUwaKcQz2tRDTSl4pRs2BkaniEdUOhC+vfOgWZ2XKxPAnpZD125fo\n\tVa2g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=6AAoZ10FNEr1sVj78ZF7r04oZX9cLEvD48oFYV8PVk8=;\n\tb=qgu3KaqGTldKM3kr9TNC2H9F0O369Z+H0f8mrXa4ARvaygZWPwjkfJoj/IrQuyCJ5Q\n\tI5OIXmFC2/0u6DlyWdUzCKaWxpJOo8rThN+Se8lcuKu+dEO398vh49q8kXs5A5oGb5za\n\toi2lHwnl81NWiudmNo0OrGbnG8HrWGdQbp1tefRTsveRbazp2AKB5HzkyyKDLCI7MGGW\n\tk1OXRvp1bqg7C+jE2TRo5oFAkWzNUWxdAj9lHbjPjrpRxJoBApb2hFQYl4Bat/qRlXVG\n\tDJ6GtvXV6xQ9iQuu5fah7vcMRnpoBq6CTSInbqETyxy57LdOCbT3Ue6IharlR1Fz/7qK\n\t7Vzg==","X-Gm-Message-State":"APjAAAUEVISYhVP6JEn6pmf9BDMI/72W16xo1IJJhJvnAA2JpIchFAp4\n\ttmKynICSaQvuqQrvlqTjjs2aO/OuCk6EvQ==","X-Google-Smtp-Source":"APXvYqypdMbLl8H1SwnV82HQVqm7g/l9VI20zKR8gyBuEO3OfIqw63NKjUUBQnoD5UFHific6zb/og==","X-Received":"by 2002:ac8:1415:: with SMTP id\n\tk21mr8222318qtj.300.1581528887666; \n\tWed, 12 Feb 2020 09:34:47 -0800 (PST)","Message-ID":"<3f78b1e716d73e7a833a0ff85f9ffa9166e56ae0.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Wed, 12 Feb 2020 12:34:46 -0500","In-Reply-To":"<20200211234742.GO20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-16-nicolas@ndufresne.ca>\n\t<20200211234742.GO20823@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":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 15/23] gst: libcamerasrc: Implement\n\tminimal caps negotiation","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":"Wed, 12 Feb 2020 17:34:49 -0000"}}]