[{"id":3869,"web_url":"https://patchwork.libcamera.org/comment/3869/","msgid":"<20200229141422.GP18738@pendragon.ideasonboard.com>","date":"2020-02-29T14:14:22","subject":"Re: [libcamera-devel] [PATCH v2 15/27] 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 Thu, Feb 27, 2020 at 03:03:55PM -0500, Nicolas Dufresne wrote:\n> This is not expected to work in every possible cases, but should be sufficient as\n> an initial implementation. What is does is that it turns the StreamFormats into\n\ns/is does is/it does is/\n\n> caps and queries downstream caps with that as a filter.\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 trust the order\n> in StreamFormats as being sorted best first, but this is not currently in\n> libcamera. A todo has been added in the head of this file as a reminder to fix\n> that in the core.\n> \n> Signed-off-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 82 ++++++++++++++++++++++++++++++-\n> 1 file changed, 81 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index e2c63d1..2c5c34c 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -6,6 +6,12 @@\n> * gstlibcamerasrc.cpp - GStreamer Capture Element\n> */\n> \n> +/**\n> + * \\todo libcamera UVC drivers picks the lowest possible resolution first, this\n> + * should be fixed so that we get a decent resolution and framerate for the\n> + * role by default.\n> + */\n> +\n> #include \"gstlibcamera-utils.h\"\n> #include \"gstlibcamerapad.h\"\n> #include \"gstlibcamerasrc.h\"\n> @@ -23,6 +29,7 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n> struct GstLibcameraSrcState {\n> \tstd::unique_ptr cm;\n> \tstd::shared_ptr cam;\n> +\tstd::unique_ptr config;\n> \tstd::vector srcpads;\n> };\n> \n> @@ -131,16 +138,89 @@ gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n> \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> \tGLibRecLocker(&self->stream_lock);\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\t/* Create stream-id and push stream-start .*/\n\nThis probably belongs to a previous patch (and the space should go after\nthe period, not before).\n\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\ns/ \\./. /\n\nSearch and replace went wrong ? :-)\n\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\nSame here.\n\n> +\tstate->config = state->cam->generateConfiguration(roles);\n> +\t/* \\todo Check if camera may increase or decrease the number of streams\n> +\t * regardless of the number of roles.*/\n\n\t/*\n\t * ...\n\t */\n\nAnd to answer your question, it may decrease it, but not increase it.\nThis should be better documented in libcamera itself, let's keep the\ntodo here as a reminder.\n\n> +\tg_assert(state->config->size() == state->srcpads.size());\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/* Retrieve the supported caps. */\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/*\n> +\t * Regardless if it has been modified, create clean caps and push the\n> +\t * caps event. Downstream will decide if the caps are acceptable.\n> +\t */\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> +\t{\n> +\t\tgint ret = state->cam->configure(state->config.get());\n> +\t\tif (ret) {\n> +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> +\t\t\t\t\t (\"Failed to configure camera: %s\", g_strerror(-ret)),\n> +\t\t\t\t\t (\"Camera::configure() failed with error code %i\", ret));\n> +\t\t\tgst_task_stop(task);\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n\nIf the braces are here for the sole purpose of declaring ret, you can\nmove gint ret; to the beginning of the function and remove the braces.\n\n> +\n> +done:\n> +\tswitch (flow_ret) {\n> +\tcase GST_FLOW_NOT_NEGOTIATED:\n> +\t\tGST_ELEMENT_FLOW_ERROR(self, flow_ret);\n> +\t\tgst_task_stop(task);\n> +\t\treturn;\n\nMaybe s/return/break/ for symmetry wit the default case ?\n\nReviewed-by: Laurent Pinchart \n\n> +\tdefault:\n> +\t\tbreak;\n> \t}\n> }\n>","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 AA7C562689\n\tfor ;\n\tSat, 29 Feb 2020 15:14:45 +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 249192AF;\n\tSat, 29 Feb 2020 15:14:45 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1582985685;\n\tbh=JX/2mR7ftYiNCidLz6cPudBJwvPVXBFkprh1d1k10qc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dM2D7qNbGg3b9dllFnEQFKRzPF8kRNFZCXKxDKeZNRam9lJ139cDKwQkJI9Pv3OUH\n\tXmlvnDQxb3VW+mD+KrlJzxfOf6pL98yaTUidp71JKqaENrCNhXixVVoY+j63cdHUDm\n\tUb5IaZK7H7Vk2qeXJVKxUWJ45mUcgWXVDx8iRaGo=","Date":"Sat, 29 Feb 2020 16:14:22 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200229141422.GP18738@pendragon.ideasonboard.com>","References":"<20200227200407.490616-1-nicolas.dufresne@collabora.com>\n\t<20200227200407.490616-16-nicolas.dufresne@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200227200407.490616-16-nicolas.dufresne@collabora.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 15/27] 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":"Sat, 29 Feb 2020 14:14:45 -0000"}},{"id":3968,"web_url":"https://patchwork.libcamera.org/comment/3968/","msgid":"<0f4a478613de0b8ccf948419990f718146f3ea75.camel@collabora.com>","date":"2020-03-06T18:18:10","subject":"Re: [libcamera-devel] [PATCH v2 15/27] gst: libcamerasrc: Implement\n\tminimal caps negotiation","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le samedi 29 février 2020 à 16:14 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> Thank you for the patch.\n> \n> On Thu, Feb 27, 2020 at 03:03:55PM -0500, Nicolas Dufresne wrote:\n> > This is not expected to work in every possible cases, but should be\n> > sufficient as\n> > an initial implementation. What is does is that it turns the StreamFormats\n> > into\n> \n> s/is does is/it does is/\n> \n> > caps and queries downstream caps with that as a filter.\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 trust the\n> > order\n> > in StreamFormats as being sorted best first, but this is not currently in\n> > libcamera. A todo has been added in the head of this file as a reminder to\n> > fix\n> > that in the core.\n> > \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 82 ++++++++++++++++++++++++++++++-\n> > 1 file changed, 81 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > b/src/gstreamer/gstlibcamerasrc.cpp\n> > index e2c63d1..2c5c34c 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -6,6 +6,12 @@\n> > * gstlibcamerasrc.cpp - GStreamer Capture Element\n> > */\n> > \n> > +/**\n> > + * \\todo libcamera UVC drivers picks the lowest possible resolution first,\n> > this\n> > + * should be fixed so that we get a decent resolution and framerate for the\n> > + * role by default.\n> > + */\n> > +\n> > #include \"gstlibcamera-utils.h\"\n> > #include \"gstlibcamerapad.h\"\n> > #include \"gstlibcamerasrc.h\"\n> > @@ -23,6 +29,7 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);\n> > struct GstLibcameraSrcState {\n> > \tstd::unique_ptr cm;\n> > \tstd::shared_ptr cam;\n> > +\tstd::unique_ptr config;\n> > \tstd::vector srcpads;\n> > };\n> > \n> > @@ -131,16 +138,89 @@ gst_libcamera_src_task_enter(GstTask *task, GThread\n> > *thread, gpointer user_data)\n> > \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > \tGLibRecLocker(&self->stream_lock);\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\t/* Create stream-id and push stream-start .*/\n> \n> This probably belongs to a previous patch (and the space should go after\n> the period, not before).\n> \n> > \t\tg_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad,\n> > 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> \n> s/ \\./. /\n> \n> Search and replace went wrong ? :-)\n> \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> \n> Same here.\n> \n> > +\tstate->config = state->cam->generateConfiguration(roles);\n> > +\t/* \\todo Check if camera may increase or decrease the number of streams\n> > +\t * regardless of the number of roles.*/\n> \n> \t/*\n> \t * ...\n> \t */\n> \n> And to answer your question, it may decrease it, but not increase it.\n> This should be better documented in libcamera itself, let's keep the\n> todo here as a reminder.\n> \n> > +\tg_assert(state->config->size() == state->srcpads.size());\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/* Retrieve the supported caps. */\n> > +\t\tg_autoptr(GstCaps) filter =\n> > gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> > +\t\tg_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad,\n> > 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/*\n> > +\t * Regardless if it has been modified, create clean caps and push the\n> > +\t * caps event. Downstream will decide if the caps are acceptable.\n> > +\t */\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 =\n> > 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> > +\t{\n> > +\t\tgint ret = state->cam->configure(state->config.get());\n> > +\t\tif (ret) {\n> > +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > +\t\t\t\t\t (\"Failed to configure camera: %s\",\n> > g_strerror(-ret)),\n> > +\t\t\t\t\t (\"Camera::configure() failed with\n> > error code %i\", ret));\n> > +\t\t\tgst_task_stop(task);\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\t}\n> \n> If the braces are here for the sole purpose of declaring ret, you can\n> move gint ret; to the beginning of the function and remove the braces.\n\nOk, it can only be at the top (or at least befor the goto) as the compiler won't\nlet a goto cross a declaration.\n\n> \n> > +\n> > +done:\n> > +\tswitch (flow_ret) {\n> > +\tcase GST_FLOW_NOT_NEGOTIATED:\n> > +\t\tGST_ELEMENT_FLOW_ERROR(self, flow_ret);\n> > +\t\tgst_task_stop(task);\n> > +\t\treturn;\n> \n> Maybe s/return/break/ for symmetry wit the default case ?\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > +\tdefault:\n> > +\t\tbreak;\n> > \t}\n> > }\n> >","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 1525E60424\n\tfor ;\n\tFri, 6 Mar 2020 19:18:18 +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 545FE297131;\n\tFri, 6 Mar 2020 18:18:18 +0000 (GMT)"],"Message-ID":"<0f4a478613de0b8ccf948419990f718146f3ea75.camel@collabora.com>","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Fri, 06 Mar 2020 13:18:10 -0500","In-Reply-To":"<20200229141422.GP18738@pendragon.ideasonboard.com>","References":"<20200227200407.490616-1-nicolas.dufresne@collabora.com>\n\t<20200227200407.490616-16-nicolas.dufresne@collabora.com>\n\t<20200229141422.GP18738@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.4 (3.34.4-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 15/27] 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":"Fri, 06 Mar 2020 18:18:19 -0000"}}]