[{"id":19103,"web_url":"https://patchwork.libcamera.org/comment/19103/","msgid":"<7a9c3894-eac6-e303-c1fa-f721bef395fa@ideasonboard.com>","date":"2021-08-26T13:44:28","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 25/08/2021 16:07, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne \n> \n> Even thought this is not formaly documented, all pipeline managers sets a\n\ns/thought/though/\n\ns/formaly/formally/\n\n> default value to StreamConfiguration::size. The original fixation code was\n\nThis is documented at by the function Camera::generateConfiguration()\n\nhttps://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n\nI suspect the real issue (of documentation) is that - we have lots of\ndocumentation, but it's hard to find the parts that are needed at the\ntime they are needed.\n\n\n> attempting to use it, but as it was truncating the caps to its first structure\n> it would never actually find a best match.\n> \n> In this patch, instead of truncating, we weight various matches using the\n> product of the width and height delta. We also split delta from ranges\n> appart and prefer fixed size over them as ranges are not reliable.\n\ns/appart/apart/\n\n\n> This patch also removes the related todo, as it seems that libcamera core won't\n> go further then providing this default value and won't be sorting the format and\n> size lists.\n\nThe formats and sizes might be sorted numerically ... but there's no\nindication of preference there indeed. They don't even really mean that\nthe frame size is supported by the device (though the pixel format is a\nbit more indicative).\n\n\nMaybe the Sizes and Formats shouldn't be used at all by gstreamer, it\nmight be that they are providing information which is seen as more\nexpressive than it really is, and gstreamer should do it's own\ntry/validate negotiation phase to determine the possibilities.\n\n\n\n> Signed-off-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---\n> src/gstreamer/gstlibcamerasrc.cpp | 4 ---\n> 2 files changed, 47 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index 61370d5f..007d6a64 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -136,14 +136,57 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> \t\t\t\t\t GstCaps *caps)\n> {\n> \tGstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);\n> +\tguint i;\n> +\tgint best_fixed = -1, best_in_range = -1;\n> +\tGstStructure *s;\n> +\n> +\t/*\n> +\t * These are delta weight computed from:\n> +\t * ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)\n> +\t */\n> +\tguint best_fixed_delta = G_MAXUINT;\n> +\tguint best_in_range_delta = G_MAXUINT;\n> \n> \t/* First fixate the caps using default configuration value. */\n> \tg_assert(gst_caps_is_writable(caps));\n> -\tcaps = gst_caps_truncate(caps);\n> -\tGstStructure *s = gst_caps_get_structure(caps, 0);\n> \n> -\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n> -\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n> +\t/* Lookup the structure for a close match to the stream_cfg.size */\n\nI'm worried that this is placing too much weight on the caps which have\nbeen generated from the StreamFormats.\n\nhttps://libcamera.org/api-html/classlibcamera_1_1StreamFormats.html\nstates:\n\n> All sizes retrieved from StreamFormats shall be treated as advisory\n> and no size shall be considered to be supported until it has been \n> verified using CameraConfiguration::validate().\n\nIf a role has been passed to the generateConfiguration() [0], then the\nStreams (and therefore their StreamConfiguration) are provided as\nsuggested default configurations (which should have already been run\nthrough validation).\n\n\n[0]\nhttps://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n\n\nHowever, having discussed this with you last night, I think the code\nhere is correct, as it matches against the caps of the whole pipeline,\nbut we might want to have a validation phase either during\ngst_libcamera_stream_formats_to_caps() or after it to further\nfilter/restrict what is actually added as supported formats.\n\nI've already seen that this is reporting formats to the caps, that are\nnot actually available on the IPU3, and so its' failing to correctly\nconfigure the pipeline as expected.\n\nBut I think that's a separate issue on top... Or I wonder if we could\nhandle that better at the libcamera layer ...\n\n\n> +\tfor (i = 0; i < gst_caps_get_size(caps); i++) {\n> +\t\ts = gst_caps_get_structure(caps, i);\n> +\t\tgint width, height;\n> +\t\tguint delta;\n> +\n> +\t\tif (gst_structure_has_field_typed(s, \"width\", G_TYPE_INT) &&\n> +\t\t gst_structure_has_field_typed(s, \"height\", G_TYPE_INT)) {\n> +\t\t\tgst_structure_get_int(s, \"width\", &width);\n> +\t\t\tgst_structure_get_int(s, \"width\", &height);\n\ns/width/height/ here, looks like your setting width * width.\n\nIn fact, that might explain one of the bugs I hit with this...\n\nThe IPU3 was ending up using an odd default size which was possible\nbased on the best match of 720x720 now I see this ...\n\n(I'll test again later).\n\n\nAnyway, with s/width/height/ fixed above here:\n\nReviewed-by: Kieran Bingham \n\n\n\n> +\n> +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n> +\n> +\t\t\tif (delta < best_fixed_delta) {\n> +\t\t\t\tbest_fixed_delta = delta;\n> +\t\t\t\tbest_fixed = i;\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n> +\t\t\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n> +\t\t\tgst_structure_get_int(s, \"width\", &width);\n> +\t\t\tgst_structure_get_int(s, \"width\", &height);\n> +\n> +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n> +\n> +\t\t\tif (delta < best_in_range_delta) {\n> +\t\t\t\tbest_in_range_delta = delta;\n> +\t\t\t\tbest_in_range = i;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\t/* Prefer reliable fixed value over ranges */\n> +\tif (best_fixed >= 0)\n> +\t\ts = gst_caps_get_structure(caps, best_fixed);\n> +\telse\n> +\t\ts = gst_caps_get_structure(caps, best_in_range);\n> \n> \tif (gst_structure_has_name(s, \"video/x-raw\")) {\n> \t\tconst gchar *format = gst_video_format_to_string(gst_format);\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index b243aeb3..4c7b36ae 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -25,10 +25,6 @@\n> * - Add timestamp support\n> * - Use unique names to select the camera devices\n> * - Add GstVideoMeta support (strides and offsets)\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 \"gstlibcamerasrc.h\"\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 BE3EFBD87C\n\tfor ;\n\tThu, 26 Aug 2021 13:44:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 36E566890C;\n\tThu, 26 Aug 2021 15:44:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6FD766888F\n\tfor ;\n\tThu, 26 Aug 2021 15:44:31 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E4E3D191F;\n\tThu, 26 Aug 2021 15:44:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uPwv+ybi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629985471;\n\tbh=vstt75mD8j78c4VyGWwLxkqQo45X5bnS3xpbC7mg3Xo=;\n\th=To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=uPwv+ybi/BZH6aiAlp1ObeHyByVgZZixlP21nGaJ+Oxa+uNJPZ42BrVAyYcJTd1A2\n\tv7rMxDOMsvYC25QBih3AsVEfRbHTXa0hhcW/To00jGNQMlxMD9lv3EgfmcAvg4T07L\n\tIIoILLw0iTfYW+HS3RUNm0BZqy/FMUnbEiU1H5e4=","To":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210825150744.1183551-1-nicolas@ndufresne.ca>","From":"Kieran Bingham ","Message-ID":"<7a9c3894-eac6-e303-c1fa-f721bef395fa@ideasonboard.com>","Date":"Thu, 26 Aug 2021 14:44:28 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210825150744.1183551-1-nicolas@ndufresne.ca>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","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":"Nicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19104,"web_url":"https://patchwork.libcamera.org/comment/19104/","msgid":"<5e020975-52a9-4560-3f68-b88ae1e38a70@ideasonboard.com>","date":"2021-08-26T13:48:30","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"One more catch ...\n\nOn 26/08/2021 14:44, Kieran Bingham wrote:\n> On 25/08/2021 16:07, Nicolas Dufresne wrote:\n>> From: Nicolas Dufresne \n>>\n>> Even thought this is not formaly documented, all pipeline managers sets a\n> \n> s/thought/though/\n> \n> s/formaly/formally/\n> \n>> default value to StreamConfiguration::size. The original fixation code was\n> \n> This is documented at by the function Camera::generateConfiguration()\n> \n> https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n> \n> I suspect the real issue (of documentation) is that - we have lots of\n> documentation, but it's hard to find the parts that are needed at the\n> time they are needed.\n> \n> \n>> attempting to use it, but as it was truncating the caps to its first structure\n>> it would never actually find a best match.\n>>\n>> In this patch, instead of truncating, we weight various matches using the\n>> product of the width and height delta. We also split delta from ranges\n>> appart and prefer fixed size over them as ranges are not reliable.\n> \n> s/appart/apart/\n> \n> \n>> This patch also removes the related todo, as it seems that libcamera core won't\n>> go further then providing this default value and won't be sorting the format and\n>> size lists.\n> \n> The formats and sizes might be sorted numerically ... but there's no\n> indication of preference there indeed. They don't even really mean that\n> the frame size is supported by the device (though the pixel format is a\n> bit more indicative).\n> \n> \n> Maybe the Sizes and Formats shouldn't be used at all by gstreamer, it\n> might be that they are providing information which is seen as more\n> expressive than it really is, and gstreamer should do it's own\n> try/validate negotiation phase to determine the possibilities.\n> \n> \n> \n>> Signed-off-by: Nicolas Dufresne \n>> ---\n>> src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---\n>> src/gstreamer/gstlibcamerasrc.cpp | 4 ---\n>> 2 files changed, 47 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n>> index 61370d5f..007d6a64 100644\n>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>> @@ -136,14 +136,57 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n>> \t\t\t\t\t GstCaps *caps)\n>> {\n>> \tGstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);\n>> +\tguint i;\n>> +\tgint best_fixed = -1, best_in_range = -1;\n>> +\tGstStructure *s;\n>> +\n>> +\t/*\n>> +\t * These are delta weight computed from:\n>> +\t * ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)\n>> +\t */\n>> +\tguint best_fixed_delta = G_MAXUINT;\n>> +\tguint best_in_range_delta = G_MAXUINT;\n>> \n>> \t/* First fixate the caps using default configuration value. */\n>> \tg_assert(gst_caps_is_writable(caps));\n>> -\tcaps = gst_caps_truncate(caps);\n>> -\tGstStructure *s = gst_caps_get_structure(caps, 0);\n>> \n>> -\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n>> -\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n>> +\t/* Lookup the structure for a close match to the stream_cfg.size */\n> \n> I'm worried that this is placing too much weight on the caps which have\n> been generated from the StreamFormats.\n> \n> https://libcamera.org/api-html/classlibcamera_1_1StreamFormats.html\n> states:\n> \n>> All sizes retrieved from StreamFormats shall be treated as advisory\n>> and no size shall be considered to be supported until it has been \n>> verified using CameraConfiguration::validate().\n> \n> If a role has been passed to the generateConfiguration() [0], then the\n> Streams (and therefore their StreamConfiguration) are provided as\n> suggested default configurations (which should have already been run\n> through validation).\n> \n> \n> [0]\n> https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n> \n> \n> However, having discussed this with you last night, I think the code\n> here is correct, as it matches against the caps of the whole pipeline,\n> but we might want to have a validation phase either during\n> gst_libcamera_stream_formats_to_caps() or after it to further\n> filter/restrict what is actually added as supported formats.\n> \n> I've already seen that this is reporting formats to the caps, that are\n> not actually available on the IPU3, and so its' failing to correctly\n> configure the pipeline as expected.\n> \n> But I think that's a separate issue on top... Or I wonder if we could\n> handle that better at the libcamera layer ...\n> \n> \n>> +\tfor (i = 0; i < gst_caps_get_size(caps); i++) {\n>> +\t\ts = gst_caps_get_structure(caps, i);\n>> +\t\tgint width, height;\n>> +\t\tguint delta;\n>> +\n>> +\t\tif (gst_structure_has_field_typed(s, \"width\", G_TYPE_INT) &&\n>> +\t\t gst_structure_has_field_typed(s, \"height\", G_TYPE_INT)) {\n>> +\t\t\tgst_structure_get_int(s, \"width\", &width);\n>> +\t\t\tgst_structure_get_int(s, \"width\", &height);\n> \n> s/width/height/ here, looks like your setting width * width.\n> \n> In fact, that might explain one of the bugs I hit with this...\n> \n> The IPU3 was ending up using an odd default size which was possible\n> based on the best match of 720x720 now I see this ...\n> \n> (I'll test again later).\n> \n> \n> Anyway, with s/width/height/ fixed above here:\n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> \n>> +\n>> +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n>> +\n>> +\t\t\tif (delta < best_fixed_delta) {\n>> +\t\t\t\tbest_fixed_delta = delta;\n>> +\t\t\t\tbest_fixed = i;\n>> +\t\t\t}\n>> +\t\t} else {\n>> +\t\t\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n>> +\t\t\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n>> +\t\t\tgst_structure_get_int(s, \"width\", &width);\n>> +\t\t\tgst_structure_get_int(s, \"width\", &height);\n>> +\n\nAnd s/width/height/ here also.\n\n\nWith those two fixed, I now get:\n\n\ngst-launch-1.0 libcamerasrc ! queue ! glimagesink\n...\n INFO Camera camera.cpp:937 configuring streams: (0) 1280x720-NV12\n\non the IPU3.\nand\n\ngst-launch-1.0 libcamerasrc ! queue ! glimagesink\n...\n INFO Camera camera.cpp:937 configuring streams: (0) 1920x1080-NV12\n\non my Brio.\n\nTested-by: Kieran Bingham \n\n\n\n>> +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n>> +\n>> +\t\t\tif (delta < best_in_range_delta) {\n>> +\t\t\t\tbest_in_range_delta = delta;\n>> +\t\t\t\tbest_in_range = i;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t}\n>> +\n>> +\t/* Prefer reliable fixed value over ranges */\n>> +\tif (best_fixed >= 0)\n>> +\t\ts = gst_caps_get_structure(caps, best_fixed);\n>> +\telse\n>> +\t\ts = gst_caps_get_structure(caps, best_in_range);\n>> \n>> \tif (gst_structure_has_name(s, \"video/x-raw\")) {\n>> \t\tconst gchar *format = gst_video_format_to_string(gst_format);\n>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>> index b243aeb3..4c7b36ae 100644\n>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>> @@ -25,10 +25,6 @@\n>> * - Add timestamp support\n>> * - Use unique names to select the camera devices\n>> * - Add GstVideoMeta support (strides and offsets)\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 \"gstlibcamerasrc.h\"\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 C2B7DBD87D\n\tfor ;\n\tThu, 26 Aug 2021 13:48:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CADE6890C;\n\tThu, 26 Aug 2021 15:48:35 +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 3FA0A6888F\n\tfor ;\n\tThu, 26 Aug 2021 15:48:33 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B2AAD191F;\n\tThu, 26 Aug 2021 15:48:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"m0+PCw+R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629985712;\n\tbh=DKNnQSl/4kBFUMPvblOh5jJRuj+rWhG6QBKzJ6yYoAk=;\n\th=Subject:From:To:Cc:References:Reply-To:Date:In-Reply-To:From;\n\tb=m0+PCw+RtKS/kLW31WO6YD+kdXJOLparNQWabOjulMEi7FaX4HmLZigntloaGUPkI\n\tp8HYIyBCJAmDpULSLOZOQvK5nmyabvMCRzowbmXGPmMadKGFWqtMIi0qTIW2f8oWK4\n\txMir7H116nTzGELvW145gjNqMFqmOskwlT+SEitE=","From":"Kieran Bingham ","To":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210825150744.1183551-1-nicolas@ndufresne.ca>\n\t<7a9c3894-eac6-e303-c1fa-f721bef395fa@ideasonboard.com>","Organization":"Ideas on Board","Message-ID":"<5e020975-52a9-4560-3f68-b88ae1e38a70@ideasonboard.com>","Date":"Thu, 26 Aug 2021 14:48:30 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<7a9c3894-eac6-e303-c1fa-f721bef395fa@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"Nicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19105,"web_url":"https://patchwork.libcamera.org/comment/19105/","msgid":"<9b5270eb178d503903c0d1bb18a207f282888c84.camel@collabora.com>","date":"2021-08-26T14:04:32","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le jeudi 26 août 2021 à 14:44 +0100, Kieran Bingham a écrit :\n> On 25/08/2021 16:07, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne \n> > \n> > Even thought this is not formaly documented, all pipeline managers sets a\n> \n> s/thought/though/\n> \n> s/formaly/formally/\n> \n> > default value to StreamConfiguration::size. The original fixation code was\n> \n> This is documented at by the function Camera::generateConfiguration()\n> \n> https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n> \n> I suspect the real issue (of documentation) is that - we have lots of\n> documentation, but it's hard to find the parts that are needed at the\n> time they are needed.\n> \n> \n> > attempting to use it, but as it was truncating the caps to its first structure\n> > it would never actually find a best match.\n> > \n> > In this patch, instead of truncating, we weight various matches using the\n> > product of the width and height delta. We also split delta from ranges\n> > appart and prefer fixed size over them as ranges are not reliable.\n> \n> s/appart/apart/\n> \n> \n> > This patch also removes the related todo, as it seems that libcamera core won't\n> > go further then providing this default value and won't be sorting the format and\n> > size lists.\n> \n> The formats and sizes might be sorted numerically ... but there's no\n> indication of preference there indeed. They don't even really mean that\n> the frame size is supported by the device (though the pixel format is a\n> bit more indicative).\n> \n> \n> Maybe the Sizes and Formats shouldn't be used at all by gstreamer, it\n> might be that they are providing information which is seen as more\n> expressive than it really is, and gstreamer should do it's own\n> try/validate negotiation phase to determine the possibilities.\n> \n> \n> \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---\n> > src/gstreamer/gstlibcamerasrc.cpp | 4 ---\n> > 2 files changed, 47 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > index 61370d5f..007d6a64 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -136,14 +136,57 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> > \t\t\t\t\t GstCaps *caps)\n> > {\n> > \tGstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);\n> > +\tguint i;\n> > +\tgint best_fixed = -1, best_in_range = -1;\n> > +\tGstStructure *s;\n> > +\n> > +\t/*\n> > +\t * These are delta weight computed from:\n> > +\t * ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)\n> > +\t */\n> > +\tguint best_fixed_delta = G_MAXUINT;\n> > +\tguint best_in_range_delta = G_MAXUINT;\n> > \n> > \t/* First fixate the caps using default configuration value. */\n> > \tg_assert(gst_caps_is_writable(caps));\n> > -\tcaps = gst_caps_truncate(caps);\n> > -\tGstStructure *s = gst_caps_get_structure(caps, 0);\n> > \n> > -\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n> > -\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n> > +\t/* Lookup the structure for a close match to the stream_cfg.size */\n> \n> I'm worried that this is placing too much weight on the caps which have\n> been generated from the StreamFormats.\n> \n> https://libcamera.org/api-html/classlibcamera_1_1StreamFormats.html\n> states:\n> \n> > All sizes retrieved from StreamFormats shall be treated as advisory\n> > and no size shall be considered to be supported until it has been \n> > verified using CameraConfiguration::validate().\n> \n> If a role has been passed to the generateConfiguration() [0], then the\n> Streams (and therefore their StreamConfiguration) are provided as\n> suggested default configurations (which should have already been run\n> through validation).\n\nFeel free to edit this part out, or ask for a V2. What \"other project\" (of\ncourse I mean gst here) do is that we add links with \"Refer to ...\". In that\nspecific case, instead of leaving StreamConfiguration::size and\nStreamConfiguration::format blank, or duplicating the doc, it could be extended\nwith a link that refers to were these are documented.\n\n> \n> \n> [0]\n> https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n> \n> \n> However, having discussed this with you last night, I think the code\n> here is correct, as it matches against the caps of the whole pipeline,\n> but we might want to have a validation phase either during\n> gst_libcamera_stream_formats_to_caps() or after it to further\n> filter/restrict what is actually added as supported formats.\n> \n> I've already seen that this is reporting formats to the caps, that are\n> not actually available on the IPU3, and so its' failing to correctly\n> configure the pipeline as expected.\n> \n> But I think that's a separate issue on top... Or I wonder if we could\n> handle that better at the libcamera layer ...\n\nWhat I was saying is that you don't want to fix this theoretically, as you'll\nnever see the end of it. At the moment, libcamera provides format/size(fixed)\npairs and then unreliable ranges.\n\nWhat new sine I wrote that gst code, is that we generate unreliable fixed sizes\nfrom ranges now. I suggest to check IPU code, that might be one of these that\nendup being picked and failed.\n\nI was expected that using the default size would greatly help avoid bad pick,\nsince, can you check if you have managed to hit that default but you had a\nformat miss-match ? If that was the case instead of a \"unreliable\" generated\nfixed size, we could make improve it short term by adding preferred format\naround this.\n\n> \n> \n> > +\tfor (i = 0; i < gst_caps_get_size(caps); i++) {\n> > +\t\ts = gst_caps_get_structure(caps, i);\n> > +\t\tgint width, height;\n> > +\t\tguint delta;\n> > +\n> > +\t\tif (gst_structure_has_field_typed(s, \"width\", G_TYPE_INT) &&\n> > +\t\t gst_structure_has_field_typed(s, \"height\", G_TYPE_INT)) {\n> > +\t\t\tgst_structure_get_int(s, \"width\", &width);\n> > +\t\t\tgst_structure_get_int(s, \"width\", &height);\n> \n> s/width/height/ here, looks like your setting width * width.\n\nInteresting, indeed, will fix.\n\n> \n> In fact, that might explain one of the bugs I hit with this...\n> \n> The IPU3 was ending up using an odd default size which was possible\n> based on the best match of 720x720 now I see this ...\n> \n> (I'll test again later).\n> \n> \n> Anyway, with s/width/height/ fixed above here:\n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> \n> > +\n> > +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n> > +\n> > +\t\t\tif (delta < best_fixed_delta) {\n> > +\t\t\t\tbest_fixed_delta = delta;\n> > +\t\t\t\tbest_fixed = i;\n> > +\t\t\t}\n> > +\t\t} else {\n> > +\t\t\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n> > +\t\t\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n> > +\t\t\tgst_structure_get_int(s, \"width\", &width);\n> > +\t\t\tgst_structure_get_int(s, \"width\", &height);\n> > +\n> > +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n> > +\n> > +\t\t\tif (delta < best_in_range_delta) {\n> > +\t\t\t\tbest_in_range_delta = delta;\n> > +\t\t\t\tbest_in_range = i;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* Prefer reliable fixed value over ranges */\n> > +\tif (best_fixed >= 0)\n> > +\t\ts = gst_caps_get_structure(caps, best_fixed);\n> > +\telse\n> > +\t\ts = gst_caps_get_structure(caps, best_in_range);\n> > \n> > \tif (gst_structure_has_name(s, \"video/x-raw\")) {\n> > \t\tconst gchar *format = gst_video_format_to_string(gst_format);\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index b243aeb3..4c7b36ae 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -25,10 +25,6 @@\n> > * - Add timestamp support\n> > * - Use unique names to select the camera devices\n> > * - Add GstVideoMeta support (strides and offsets)\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 \"gstlibcamerasrc.h\"\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 863EEBD87D\n\tfor ;\n\tThu, 26 Aug 2021 14:04:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 08BDC68915;\n\tThu, 26 Aug 2021 16:04:46 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2BE3E6888F\n\tfor ;\n\tThu, 26 Aug 2021 16:04:45 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nicolas) with ESMTPSA id 4FCF81F44236"],"Message-ID":"<9b5270eb178d503903c0d1bb18a207f282888c84.camel@collabora.com>","From":"Nicolas Dufresne ","To":"Kieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 26 Aug 2021 10:04:32 -0400","In-Reply-To":"<7a9c3894-eac6-e303-c1fa-f721bef395fa@ideasonboard.com>","References":"<20210825150744.1183551-1-nicolas@ndufresne.ca>\n\t<7a9c3894-eac6-e303-c1fa-f721bef395fa@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.40.4 (3.40.4-1.fc34) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19106,"web_url":"https://patchwork.libcamera.org/comment/19106/","msgid":"<4edc8a13-7bf4-f76e-5e74-eeb263857a91@ideasonboard.com>","date":"2021-08-26T14:31:25","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 26/08/2021 15:04, Nicolas Dufresne wrote:\n> Le jeudi 26 août 2021 à 14:44 +0100, Kieran Bingham a écrit :\n>> On 25/08/2021 16:07, Nicolas Dufresne wrote:\n>>> From: Nicolas Dufresne \n>>>\n>>> Even thought this is not formaly documented, all pipeline managers sets a\n>>\n>> s/thought/though/\n>>\n>> s/formaly/formally/\n>>\n>>> default value to StreamConfiguration::size. The original fixation code was\n>>\n>> This is documented at by the function Camera::generateConfiguration()\n>>\n>> https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n>>\n>> I suspect the real issue (of documentation) is that - we have lots of\n>> documentation, but it's hard to find the parts that are needed at the\n>> time they are needed.\n>>\n>>\n>>> attempting to use it, but as it was truncating the caps to its first structure\n>>> it would never actually find a best match.\n>>>\n>>> In this patch, instead of truncating, we weight various matches using the\n>>> product of the width and height delta. We also split delta from ranges\n>>> appart and prefer fixed size over them as ranges are not reliable.\n>>\n>> s/appart/apart/\n>>\n>>\n>>> This patch also removes the related todo, as it seems that libcamera core won't\n>>> go further then providing this default value and won't be sorting the format and\n>>> size lists.\n>>\n>> The formats and sizes might be sorted numerically ... but there's no\n>> indication of preference there indeed. They don't even really mean that\n>> the frame size is supported by the device (though the pixel format is a\n>> bit more indicative).\n>>\n>>\n>> Maybe the Sizes and Formats shouldn't be used at all by gstreamer, it\n>> might be that they are providing information which is seen as more\n>> expressive than it really is, and gstreamer should do it's own\n>> try/validate negotiation phase to determine the possibilities.\n>>\n>>\n>>\n>>> Signed-off-by: Nicolas Dufresne \n>>> ---\n>>> src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---\n>>> src/gstreamer/gstlibcamerasrc.cpp | 4 ---\n>>> 2 files changed, 47 insertions(+), 8 deletions(-)\n>>>\n>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n>>> index 61370d5f..007d6a64 100644\n>>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>>> @@ -136,14 +136,57 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n>>> \t\t\t\t\t GstCaps *caps)\n>>> {\n>>> \tGstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);\n>>> +\tguint i;\n>>> +\tgint best_fixed = -1, best_in_range = -1;\n>>> +\tGstStructure *s;\n>>> +\n>>> +\t/*\n>>> +\t * These are delta weight computed from:\n>>> +\t * ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)\n>>> +\t */\n>>> +\tguint best_fixed_delta = G_MAXUINT;\n>>> +\tguint best_in_range_delta = G_MAXUINT;\n>>> \n>>> \t/* First fixate the caps using default configuration value. */\n>>> \tg_assert(gst_caps_is_writable(caps));\n>>> -\tcaps = gst_caps_truncate(caps);\n>>> -\tGstStructure *s = gst_caps_get_structure(caps, 0);\n>>> \n>>> -\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n>>> -\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n>>> +\t/* Lookup the structure for a close match to the stream_cfg.size */\n>>\n>> I'm worried that this is placing too much weight on the caps which have\n>> been generated from the StreamFormats.\n>>\n>> https://libcamera.org/api-html/classlibcamera_1_1StreamFormats.html\n>> states:\n>>\n>>> All sizes retrieved from StreamFormats shall be treated as advisory\n>>> and no size shall be considered to be supported until it has been \n>>> verified using CameraConfiguration::validate().\n>>\n>> If a role has been passed to the generateConfiguration() [0], then the\n>> Streams (and therefore their StreamConfiguration) are provided as\n>> suggested default configurations (which should have already been run\n>> through validation).\n> \n> Feel free to edit this part out, or ask for a V2. What \"other project\" (of\n> course I mean gst here) do is that we add links with \"Refer to ...\". In that\n> specific case, instead of leaving StreamConfiguration::size and\n> StreamConfiguration::format blank, or duplicating the doc, it could be extended\n> with a link that refers to were these are documented.\n\nI don't think size and format need to reference elsewhere, they /are/\nthe size and format of that StreamConfiguration.\n\nThe StreamConfiguration (as a whole) is the object which is returned,\nand it 'is' the default suggested configuration.\n\nThe given default StreamConfiguration has a size and a format ...\n\nThere's nothing special about 'size' and 'format' of the\nStreamConfiguration - it's the object as a whole that we're discussing here.\n\n\nPerhaps a better way to describe it is\n\n\"Pipeline handlers will return a default StreamConfiguration for a given\nStreamRole. That StreamConfiguration comprises of a validated Size and\nFormat which can be used to start the device\".\n\n\nI think what is confusing is that a StreamConfiguration has this 'extra'\nthing called formats, and sizes. They are /not/ the active\nStreamConfiguration - but they are possible values (hints) that can be\nput back into the StreamConfiguration to try validating.\n\n\n\n>> [0]\n>> https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n>>\n>>\n>> However, having discussed this with you last night, I think the code\n>> here is correct, as it matches against the caps of the whole pipeline,\n>> but we might want to have a validation phase either during\n>> gst_libcamera_stream_formats_to_caps() or after it to further\n>> filter/restrict what is actually added as supported formats.\n>>\n>> I've already seen that this is reporting formats to the caps, that are\n>> not actually available on the IPU3, and so its' failing to correctly\n>> configure the pipeline as expected.\n>>\n>> But I think that's a separate issue on top... Or I wonder if we could\n>> handle that better at the libcamera layer ...\n> \n> What I was saying is that you don't want to fix this theoretically, as you'll\n> never see the end of it. At the moment, libcamera provides format/size(fixed)\n> pairs and then unreliable ranges.\n> \n> What new sine I wrote that gst code, is that we generate unreliable fixed sizes\n> from ranges now. I suggest to check IPU code, that might be one of these that\n> endup being picked and failed.\n> \n> I was expected that using the default size would greatly help avoid bad pick,\n> since, can you check if you have managed to hit that default but you had a\n> format miss-match ? If that was the case instead of a \"unreliable\" generated\n> fixed size, we could make improve it short term by adding preferred format\n> around this.\n\n\nI think the default size was picked - but the width * width bug gave a\ndifferent default, that's identified and fixed now though ...\n\nWhat does still happen is that the 'arbitrary unvalidated list' is\nexposed to cheese, so it lets me pick one of those sizes.\n\nI select it, and it seems to fail validation - and that causes the\ngstreamer pipeline to stop with:\n\n(cheese:93188): cheese-WARNING **: 15:29:24.060: Internal data stream\nerror.: ../../src/gstreamer/gstlibcamerasrc.cpp(323):\ngst_libcamera_src_task_run ():\n/GstCameraBin:camerabin/GstWrapperCameraBinSrc:camera_source/GstBin:bin19/GstLibcameraSrc:libcamerasrc2:\nstreaming stopped, reason not-negotiated (-4)\n\n\nBut it doesn't resume when changing back to a supported size.\n\nAnyway, I don't think that's an issue caused by this patch, and this\npatch already brings improvements, so I'm going to move to the v2 patch\nthat you've posted ;-)\n\n\n>>\n>>\n>>> +\tfor (i = 0; i < gst_caps_get_size(caps); i++) {\n>>> +\t\ts = gst_caps_get_structure(caps, i);\n>>> +\t\tgint width, height;\n>>> +\t\tguint delta;\n>>> +\n>>> +\t\tif (gst_structure_has_field_typed(s, \"width\", G_TYPE_INT) &&\n>>> +\t\t gst_structure_has_field_typed(s, \"height\", G_TYPE_INT)) {\n>>> +\t\t\tgst_structure_get_int(s, \"width\", &width);\n>>> +\t\t\tgst_structure_get_int(s, \"width\", &height);\n>>\n>> s/width/height/ here, looks like your setting width * width.\n> \n> Interesting, indeed, will fix.\n> \n>>\n>> In fact, that might explain one of the bugs I hit with this...\n>>\n>> The IPU3 was ending up using an odd default size which was possible\n>> based on the best match of 720x720 now I see this ...\n>>\n>> (I'll test again later).\n>>\n>>\n>> Anyway, with s/width/height/ fixed above here:\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>\n>>\n>>> +\n>>> +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n>>> +\n>>> +\t\t\tif (delta < best_fixed_delta) {\n>>> +\t\t\t\tbest_fixed_delta = delta;\n>>> +\t\t\t\tbest_fixed = i;\n>>> +\t\t\t}\n>>> +\t\t} else {\n>>> +\t\t\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n>>> +\t\t\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n>>> +\t\t\tgst_structure_get_int(s, \"width\", &width);\n>>> +\t\t\tgst_structure_get_int(s, \"width\", &height);\n>>> +\n>>> +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n>>> +\n>>> +\t\t\tif (delta < best_in_range_delta) {\n>>> +\t\t\t\tbest_in_range_delta = delta;\n>>> +\t\t\t\tbest_in_range = i;\n>>> +\t\t\t}\n>>> +\t\t}\n>>> +\t}\n>>> +\n>>> +\t/* Prefer reliable fixed value over ranges */\n>>> +\tif (best_fixed >= 0)\n>>> +\t\ts = gst_caps_get_structure(caps, best_fixed);\n>>> +\telse\n>>> +\t\ts = gst_caps_get_structure(caps, best_in_range);\n>>> \n>>> \tif (gst_structure_has_name(s, \"video/x-raw\")) {\n>>> \t\tconst gchar *format = gst_video_format_to_string(gst_format);\n>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>>> index b243aeb3..4c7b36ae 100644\n>>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>>> @@ -25,10 +25,6 @@\n>>> * - Add timestamp support\n>>> * - Use unique names to select the camera devices\n>>> * - Add GstVideoMeta support (strides and offsets)\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 \"gstlibcamerasrc.h\"\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 54A35BD87D\n\tfor ;\n\tThu, 26 Aug 2021 14:31:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D72C68915;\n\tThu, 26 Aug 2021 16:31:30 +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 8F19B6888F\n\tfor ;\n\tThu, 26 Aug 2021 16:31:28 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0BED2191F;\n\tThu, 26 Aug 2021 16:31:27 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hpta8T2u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629988288;\n\tbh=T+uoUo/kRk5W3uLFiUD5ly9mafSyefDLWoX0fOiUFN4=;\n\th=From:To:References:Subject:Date:In-Reply-To:From;\n\tb=hpta8T2uxCW2o3aiDLz2UKpMxPor5rLocY0nOFeFcgDFD/OM1o2m7J86JhLdoa2Jq\n\teifrlOKNKpQNgw6n5TlETGKjiANAwxS9kiqMoJ1wo3nom3i2/lWHodTSHNq0TwtCGv\n\tdj01akLRhSSH/+FQnt6vmpB/QiDlfvYRf8j7c1iI=","From":"Kieran Bingham ","To":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210825150744.1183551-1-nicolas@ndufresne.ca>\n\t<7a9c3894-eac6-e303-c1fa-f721bef395fa@ideasonboard.com>\n\t<9b5270eb178d503903c0d1bb18a207f282888c84.camel@collabora.com>","Message-ID":"<4edc8a13-7bf4-f76e-5e74-eeb263857a91@ideasonboard.com>","Date":"Thu, 26 Aug 2021 15:31:25 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<9b5270eb178d503903c0d1bb18a207f282888c84.camel@collabora.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19109,"web_url":"https://patchwork.libcamera.org/comment/19109/","msgid":"<945cc52d279555a272eedeb723f69707f4f87525.camel@collabora.com>","date":"2021-08-26T14:34:54","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le jeudi 26 août 2021 à 15:31 +0100, Kieran Bingham a écrit :\n> \n> On 26/08/2021 15:04, Nicolas Dufresne wrote:\n> > Le jeudi 26 août 2021 à 14:44 +0100, Kieran Bingham a écrit :\n> > > On 25/08/2021 16:07, Nicolas Dufresne wrote:\n> > > > From: Nicolas Dufresne \n> > > > \n> > > > Even thought this is not formaly documented, all pipeline managers sets a\n> > > \n> > > s/thought/though/\n> > > \n> > > s/formaly/formally/\n> > > \n> > > > default value to StreamConfiguration::size. The original fixation code was\n> > > \n> > > This is documented at by the function Camera::generateConfiguration()\n> > > \n> > > https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n> > > \n> > > I suspect the real issue (of documentation) is that - we have lots of\n> > > documentation, but it's hard to find the parts that are needed at the\n> > > time they are needed.\n> > > \n> > > \n> > > > attempting to use it, but as it was truncating the caps to its first structure\n> > > > it would never actually find a best match.\n> > > > \n> > > > In this patch, instead of truncating, we weight various matches using the\n> > > > product of the width and height delta. We also split delta from ranges\n> > > > appart and prefer fixed size over them as ranges are not reliable.\n> > > \n> > > s/appart/apart/\n> > > \n> > > \n> > > > This patch also removes the related todo, as it seems that libcamera core won't\n> > > > go further then providing this default value and won't be sorting the format and\n> > > > size lists.\n> > > \n> > > The formats and sizes might be sorted numerically ... but there's no\n> > > indication of preference there indeed. They don't even really mean that\n> > > the frame size is supported by the device (though the pixel format is a\n> > > bit more indicative).\n> > > \n> > > \n> > > Maybe the Sizes and Formats shouldn't be used at all by gstreamer, it\n> > > might be that they are providing information which is seen as more\n> > > expressive than it really is, and gstreamer should do it's own\n> > > try/validate negotiation phase to determine the possibilities.\n> > > \n> > > \n> > > \n> > > > Signed-off-by: Nicolas Dufresne \n> > > > ---\n> > > > src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---\n> > > > src/gstreamer/gstlibcamerasrc.cpp | 4 ---\n> > > > 2 files changed, 47 insertions(+), 8 deletions(-)\n> > > > \n> > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > index 61370d5f..007d6a64 100644\n> > > > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > @@ -136,14 +136,57 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> > > > \t\t\t\t\t GstCaps *caps)\n> > > > {\n> > > > \tGstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);\n> > > > +\tguint i;\n> > > > +\tgint best_fixed = -1, best_in_range = -1;\n> > > > +\tGstStructure *s;\n> > > > +\n> > > > +\t/*\n> > > > +\t * These are delta weight computed from:\n> > > > +\t * ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)\n> > > > +\t */\n> > > > +\tguint best_fixed_delta = G_MAXUINT;\n> > > > +\tguint best_in_range_delta = G_MAXUINT;\n> > > > \n> > > > \t/* First fixate the caps using default configuration value. */\n> > > > \tg_assert(gst_caps_is_writable(caps));\n> > > > -\tcaps = gst_caps_truncate(caps);\n> > > > -\tGstStructure *s = gst_caps_get_structure(caps, 0);\n> > > > \n> > > > -\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n> > > > -\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n> > > > +\t/* Lookup the structure for a close match to the stream_cfg.size */\n> > > \n> > > I'm worried that this is placing too much weight on the caps which have\n> > > been generated from the StreamFormats.\n> > > \n> > > https://libcamera.org/api-html/classlibcamera_1_1StreamFormats.html\n> > > states:\n> > > \n> > > > All sizes retrieved from StreamFormats shall be treated as advisory\n> > > > and no size shall be considered to be supported until it has been \n> > > > verified using CameraConfiguration::validate().\n> > > \n> > > If a role has been passed to the generateConfiguration() [0], then the\n> > > Streams (and therefore their StreamConfiguration) are provided as\n> > > suggested default configurations (which should have already been run\n> > > through validation).\n> > \n> > Feel free to edit this part out, or ask for a V2. What \"other project\" (of\n> > course I mean gst here) do is that we add links with \"Refer to ...\". In that\n> > specific case, instead of leaving StreamConfiguration::size and\n> > StreamConfiguration::format blank, or duplicating the doc, it could be extended\n> > with a link that refers to were these are documented.\n> \n> I don't think size and format need to reference elsewhere, they /are/\n> the size and format of that StreamConfiguration.\n\nWell they have a very special semantic, as before being set and validate, they\nmeant to be a \"suggested default value\". I also believe that me being tricked\nthinking this was undocumented is enough to justify improving it.\n\n> \n> The StreamConfiguration (as a whole) is the object which is returned,\n> and it 'is' the default suggested configuration.\n> \n> The given default StreamConfiguration has a size and a format ...\n> \n> There's nothing special about 'size' and 'format' of the\n> StreamConfiguration - it's the object as a whole that we're discussing here.\n> \n> \n> Perhaps a better way to describe it is\n> \n> \"Pipeline handlers will return a default StreamConfiguration for a given\n> StreamRole. That StreamConfiguration comprises of a validated Size and\n> Format which can be used to start the device\".\n> \n> \n> I think what is confusing is that a StreamConfiguration has this 'extra'\n> thing called formats, and sizes. They are /not/ the active\n> StreamConfiguration - but they are possible values (hints) that can be\n> put back into the StreamConfiguration to try validating.\n> \n> \n> \n> > > [0]\n> > > https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n> > > \n> > > \n> > > However, having discussed this with you last night, I think the code\n> > > here is correct, as it matches against the caps of the whole pipeline,\n> > > but we might want to have a validation phase either during\n> > > gst_libcamera_stream_formats_to_caps() or after it to further\n> > > filter/restrict what is actually added as supported formats.\n> > > \n> > > I've already seen that this is reporting formats to the caps, that are\n> > > not actually available on the IPU3, and so its' failing to correctly\n> > > configure the pipeline as expected.\n> > > \n> > > But I think that's a separate issue on top... Or I wonder if we could\n> > > handle that better at the libcamera layer ...\n> > \n> > What I was saying is that you don't want to fix this theoretically, as you'll\n> > never see the end of it. At the moment, libcamera provides format/size(fixed)\n> > pairs and then unreliable ranges.\n> > \n> > What new sine I wrote that gst code, is that we generate unreliable fixed sizes\n> > from ranges now. I suggest to check IPU code, that might be one of these that\n> > endup being picked and failed.\n> > \n> > I was expected that using the default size would greatly help avoid bad pick,\n> > since, can you check if you have managed to hit that default but you had a\n> > format miss-match ? If that was the case instead of a \"unreliable\" generated\n> > fixed size, we could make improve it short term by adding preferred format\n> > around this.\n> \n> \n> I think the default size was picked - but the width * width bug gave a\n> different default, that's identified and fixed now though ...\n> \n> What does still happen is that the 'arbitrary unvalidated list' is\n> exposed to cheese, so it lets me pick one of those sizes.\n> \n> I select it, and it seems to fail validation - and that causes the\n> gstreamer pipeline to stop with:\n> \n> (cheese:93188): cheese-WARNING **: 15:29:24.060: Internal data stream\n> error.: ../../src/gstreamer/gstlibcamerasrc.cpp(323):\n> gst_libcamera_src_task_run ():\n> /GstCameraBin:camerabin/GstWrapperCameraBinSrc:camera_source/GstBin:bin19/GstLibcameraSrc:libcamerasrc2:\n> streaming stopped, reason not-negotiated (-4)\n> \n> \n> But it doesn't resume when changing back to a supported size.\n> \n> Anyway, I don't think that's an issue caused by this patch, and this\n> patch already brings improvements, so I'm going to move to the v2 patch\n> that you've posted ;-)\n> \n> \n> > > \n> > > \n> > > > +\tfor (i = 0; i < gst_caps_get_size(caps); i++) {\n> > > > +\t\ts = gst_caps_get_structure(caps, i);\n> > > > +\t\tgint width, height;\n> > > > +\t\tguint delta;\n> > > > +\n> > > > +\t\tif (gst_structure_has_field_typed(s, \"width\", G_TYPE_INT) &&\n> > > > +\t\t gst_structure_has_field_typed(s, \"height\", G_TYPE_INT)) {\n> > > > +\t\t\tgst_structure_get_int(s, \"width\", &width);\n> > > > +\t\t\tgst_structure_get_int(s, \"width\", &height);\n> > > \n> > > s/width/height/ here, looks like your setting width * width.\n> > \n> > Interesting, indeed, will fix.\n> > \n> > > \n> > > In fact, that might explain one of the bugs I hit with this...\n> > > \n> > > The IPU3 was ending up using an odd default size which was possible\n> > > based on the best match of 720x720 now I see this ...\n> > > \n> > > (I'll test again later).\n> > > \n> > > \n> > > Anyway, with s/width/height/ fixed above here:\n> > > \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > \n> > > \n> > > > +\n> > > > +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n> > > > +\n> > > > +\t\t\tif (delta < best_fixed_delta) {\n> > > > +\t\t\t\tbest_fixed_delta = delta;\n> > > > +\t\t\t\tbest_fixed = i;\n> > > > +\t\t\t}\n> > > > +\t\t} else {\n> > > > +\t\t\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n> > > > +\t\t\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n> > > > +\t\t\tgst_structure_get_int(s, \"width\", &width);\n> > > > +\t\t\tgst_structure_get_int(s, \"width\", &height);\n> > > > +\n> > > > +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n> > > > +\n> > > > +\t\t\tif (delta < best_in_range_delta) {\n> > > > +\t\t\t\tbest_in_range_delta = delta;\n> > > > +\t\t\t\tbest_in_range = i;\n> > > > +\t\t\t}\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > +\t/* Prefer reliable fixed value over ranges */\n> > > > +\tif (best_fixed >= 0)\n> > > > +\t\ts = gst_caps_get_structure(caps, best_fixed);\n> > > > +\telse\n> > > > +\t\ts = gst_caps_get_structure(caps, best_in_range);\n> > > > \n> > > > \tif (gst_structure_has_name(s, \"video/x-raw\")) {\n> > > > \t\tconst gchar *format = gst_video_format_to_string(gst_format);\n> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > index b243aeb3..4c7b36ae 100644\n> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > @@ -25,10 +25,6 @@\n> > > > * - Add timestamp support\n> > > > * - Use unique names to select the camera devices\n> > > > * - Add GstVideoMeta support (strides and offsets)\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 \"gstlibcamerasrc.h\"\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 04C68BD87D\n\tfor ;\n\tThu, 26 Aug 2021 14:35:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 66D7168915;\n\tThu, 26 Aug 2021 16:35:07 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59C186888F\n\tfor ;\n\tThu, 26 Aug 2021 16:35:06 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nicolas) with ESMTPSA id 80AFB1F44444"],"Message-ID":"<945cc52d279555a272eedeb723f69707f4f87525.camel@collabora.com>","From":"Nicolas Dufresne ","To":"Kieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 26 Aug 2021 10:34:54 -0400","In-Reply-To":"<4edc8a13-7bf4-f76e-5e74-eeb263857a91@ideasonboard.com>","References":"<20210825150744.1183551-1-nicolas@ndufresne.ca>\n\t<7a9c3894-eac6-e303-c1fa-f721bef395fa@ideasonboard.com>\n\t<9b5270eb178d503903c0d1bb18a207f282888c84.camel@collabora.com>\n\t<4edc8a13-7bf4-f76e-5e74-eeb263857a91@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.40.4 (3.40.4-1.fc34) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19111,"web_url":"https://patchwork.libcamera.org/comment/19111/","msgid":"","date":"2021-08-26T14:40:47","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 26/08/2021 15:34, Nicolas Dufresne wrote:\n> Le jeudi 26 août 2021 à 15:31 +0100, Kieran Bingham a écrit :\n>>\n>> On 26/08/2021 15:04, Nicolas Dufresne wrote:\n>>> Le jeudi 26 août 2021 à 14:44 +0100, Kieran Bingham a écrit :\n>>>> On 25/08/2021 16:07, Nicolas Dufresne wrote:\n>>>>> From: Nicolas Dufresne \n>>>>>\n>>>>> Even thought this is not formaly documented, all pipeline managers sets a\n>>>>\n>>>> s/thought/though/\n>>>>\n>>>> s/formaly/formally/\n>>>>\n>>>>> default value to StreamConfiguration::size. The original fixation code was\n>>>>\n>>>> This is documented at by the function Camera::generateConfiguration()\n>>>>\n>>>> https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n>>>>\n>>>> I suspect the real issue (of documentation) is that - we have lots of\n>>>> documentation, but it's hard to find the parts that are needed at the\n>>>> time they are needed.\n>>>>\n>>>>\n>>>>> attempting to use it, but as it was truncating the caps to its first structure\n>>>>> it would never actually find a best match.\n>>>>>\n>>>>> In this patch, instead of truncating, we weight various matches using the\n>>>>> product of the width and height delta. We also split delta from ranges\n>>>>> appart and prefer fixed size over them as ranges are not reliable.\n>>>>\n>>>> s/appart/apart/\n>>>>\n>>>>\n>>>>> This patch also removes the related todo, as it seems that libcamera core won't\n>>>>> go further then providing this default value and won't be sorting the format and\n>>>>> size lists.\n>>>>\n>>>> The formats and sizes might be sorted numerically ... but there's no\n>>>> indication of preference there indeed. They don't even really mean that\n>>>> the frame size is supported by the device (though the pixel format is a\n>>>> bit more indicative).\n>>>>\n>>>>\n>>>> Maybe the Sizes and Formats shouldn't be used at all by gstreamer, it\n>>>> might be that they are providing information which is seen as more\n>>>> expressive than it really is, and gstreamer should do it's own\n>>>> try/validate negotiation phase to determine the possibilities.\n>>>>\n>>>>\n>>>>\n>>>>> Signed-off-by: Nicolas Dufresne \n>>>>> ---\n>>>>> src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---\n>>>>> src/gstreamer/gstlibcamerasrc.cpp | 4 ---\n>>>>> 2 files changed, 47 insertions(+), 8 deletions(-)\n>>>>>\n>>>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n>>>>> index 61370d5f..007d6a64 100644\n>>>>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>>>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>>>>> @@ -136,14 +136,57 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n>>>>> \t\t\t\t\t GstCaps *caps)\n>>>>> {\n>>>>> \tGstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);\n>>>>> +\tguint i;\n>>>>> +\tgint best_fixed = -1, best_in_range = -1;\n>>>>> +\tGstStructure *s;\n>>>>> +\n>>>>> +\t/*\n>>>>> +\t * These are delta weight computed from:\n>>>>> +\t * ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)\n>>>>> +\t */\n>>>>> +\tguint best_fixed_delta = G_MAXUINT;\n>>>>> +\tguint best_in_range_delta = G_MAXUINT;\n>>>>> \n>>>>> \t/* First fixate the caps using default configuration value. */\n>>>>> \tg_assert(gst_caps_is_writable(caps));\n>>>>> -\tcaps = gst_caps_truncate(caps);\n>>>>> -\tGstStructure *s = gst_caps_get_structure(caps, 0);\n>>>>> \n>>>>> -\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n>>>>> -\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n>>>>> +\t/* Lookup the structure for a close match to the stream_cfg.size */\n>>>>\n>>>> I'm worried that this is placing too much weight on the caps which have\n>>>> been generated from the StreamFormats.\n>>>>\n>>>> https://libcamera.org/api-html/classlibcamera_1_1StreamFormats.html\n>>>> states:\n>>>>\n>>>>> All sizes retrieved from StreamFormats shall be treated as advisory\n>>>>> and no size shall be considered to be supported until it has been \n>>>>> verified using CameraConfiguration::validate().\n>>>>\n>>>> If a role has been passed to the generateConfiguration() [0], then the\n>>>> Streams (and therefore their StreamConfiguration) are provided as\n>>>> suggested default configurations (which should have already been run\n>>>> through validation).\n>>>\n>>> Feel free to edit this part out, or ask for a V2. What \"other project\" (of\n>>> course I mean gst here) do is that we add links with \"Refer to ...\". In that\n>>> specific case, instead of leaving StreamConfiguration::size and\n>>> StreamConfiguration::format blank, or duplicating the doc, it could be extended\n>>> with a link that refers to were these are documented.\n>>\n>> I don't think size and format need to reference elsewhere, they /are/\n>> the size and format of that StreamConfiguration.\n> \n> Well they have a very special semantic, as before being set and validate, they\n> meant to be a \"suggested default value\". I also believe that me being tricked\n> thinking this was undocumented is enough to justify improving it.\n\n\nAbsolutely, that's what I'm trying to get to the bottom of.\n\nBut ... It's not the size and format that are special. It's the whole\nStreamConfiguration.\n\n\nThe size and format are just two parts of that, though admittedly, they\nare possibly the only parts of relevance to an application at this stage\nin the configuration process.\n\n\nBut also it's more specific to the StreamRole that is given. So the\nStreamConfiguration is not set as a default arbitrarily, it's given as a\ndefault for the requested StreamRole.\n\nIf there is no StreamRole, then there would be no StreamConfiguration\nreturned, and so there would be no given size or format.\n\nSo this discussion really, is about StreamRoles - ... And their use with\ngenerateConfiguration() ... so perhaps it's the StreamRoles that need\nbetter documentation.\n\n\n>>\n>> The StreamConfiguration (as a whole) is the object which is returned,\n>> and it 'is' the default suggested configuration.\n>>\n>> The given default StreamConfiguration has a size and a format ...\n>>\n>> There's nothing special about 'size' and 'format' of the\n>> StreamConfiguration - it's the object as a whole that we're discussing here.\n>>\n>>\n>> Perhaps a better way to describe it is\n>>\n>> \"Pipeline handlers will return a default StreamConfiguration for a given\n>> StreamRole. That StreamConfiguration comprises of a validated Size and\n>> Format which can be used to start the device\".\n>>\n>>\n>> I think what is confusing is that a StreamConfiguration has this 'extra'\n>> thing called formats, and sizes. They are /not/ the active\n>> StreamConfiguration - but they are possible values (hints) that can be\n>> put back into the StreamConfiguration to try validating.\n>>\n>>\n>>\n>>>> [0]\n>>>> https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d\n>>>>\n>>>>\n>>>> However, having discussed this with you last night, I think the code\n>>>> here is correct, as it matches against the caps of the whole pipeline,\n>>>> but we might want to have a validation phase either during\n>>>> gst_libcamera_stream_formats_to_caps() or after it to further\n>>>> filter/restrict what is actually added as supported formats.\n>>>>\n>>>> I've already seen that this is reporting formats to the caps, that are\n>>>> not actually available on the IPU3, and so its' failing to correctly\n>>>> configure the pipeline as expected.\n>>>>\n>>>> But I think that's a separate issue on top... Or I wonder if we could\n>>>> handle that better at the libcamera layer ...\n>>>\n>>> What I was saying is that you don't want to fix this theoretically, as you'll\n>>> never see the end of it. At the moment, libcamera provides format/size(fixed)\n>>> pairs and then unreliable ranges.\n>>>\n>>> What new sine I wrote that gst code, is that we generate unreliable fixed sizes\n>>> from ranges now. I suggest to check IPU code, that might be one of these that\n>>> endup being picked and failed.\n>>>\n>>> I was expected that using the default size would greatly help avoid bad pick,\n>>> since, can you check if you have managed to hit that default but you had a\n>>> format miss-match ? If that was the case instead of a \"unreliable\" generated\n>>> fixed size, we could make improve it short term by adding preferred format\n>>> around this.\n>>\n>>\n>> I think the default size was picked - but the width * width bug gave a\n>> different default, that's identified and fixed now though ...\n>>\n>> What does still happen is that the 'arbitrary unvalidated list' is\n>> exposed to cheese, so it lets me pick one of those sizes.\n>>\n>> I select it, and it seems to fail validation - and that causes the\n>> gstreamer pipeline to stop with:\n>>\n>> (cheese:93188): cheese-WARNING **: 15:29:24.060: Internal data stream\n>> error.: ../../src/gstreamer/gstlibcamerasrc.cpp(323):\n>> gst_libcamera_src_task_run ():\n>> /GstCameraBin:camerabin/GstWrapperCameraBinSrc:camera_source/GstBin:bin19/GstLibcameraSrc:libcamerasrc2:\n>> streaming stopped, reason not-negotiated (-4)\n>>\n>>\n>> But it doesn't resume when changing back to a supported size.\n>>\n>> Anyway, I don't think that's an issue caused by this patch, and this\n>> patch already brings improvements, so I'm going to move to the v2 patch\n>> that you've posted ;-)\n>>\n>>\n>>>>\n>>>>\n>>>>> +\tfor (i = 0; i < gst_caps_get_size(caps); i++) {\n>>>>> +\t\ts = gst_caps_get_structure(caps, i);\n>>>>> +\t\tgint width, height;\n>>>>> +\t\tguint delta;\n>>>>> +\n>>>>> +\t\tif (gst_structure_has_field_typed(s, \"width\", G_TYPE_INT) &&\n>>>>> +\t\t gst_structure_has_field_typed(s, \"height\", G_TYPE_INT)) {\n>>>>> +\t\t\tgst_structure_get_int(s, \"width\", &width);\n>>>>> +\t\t\tgst_structure_get_int(s, \"width\", &height);\n>>>>\n>>>> s/width/height/ here, looks like your setting width * width.\n>>>\n>>> Interesting, indeed, will fix.\n>>>\n>>>>\n>>>> In fact, that might explain one of the bugs I hit with this...\n>>>>\n>>>> The IPU3 was ending up using an odd default size which was possible\n>>>> based on the best match of 720x720 now I see this ...\n>>>>\n>>>> (I'll test again later).\n>>>>\n>>>>\n>>>> Anyway, with s/width/height/ fixed above here:\n>>>>\n>>>> Reviewed-by: Kieran Bingham \n>>>>\n>>>>\n>>>>\n>>>>> +\n>>>>> +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n>>>>> +\n>>>>> +\t\t\tif (delta < best_fixed_delta) {\n>>>>> +\t\t\t\tbest_fixed_delta = delta;\n>>>>> +\t\t\t\tbest_fixed = i;\n>>>>> +\t\t\t}\n>>>>> +\t\t} else {\n>>>>> +\t\t\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n>>>>> +\t\t\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n>>>>> +\t\t\tgst_structure_get_int(s, \"width\", &width);\n>>>>> +\t\t\tgst_structure_get_int(s, \"width\", &height);\n>>>>> +\n>>>>> +\t\t\tdelta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);\n>>>>> +\n>>>>> +\t\t\tif (delta < best_in_range_delta) {\n>>>>> +\t\t\t\tbest_in_range_delta = delta;\n>>>>> +\t\t\t\tbest_in_range = i;\n>>>>> +\t\t\t}\n>>>>> +\t\t}\n>>>>> +\t}\n>>>>> +\n>>>>> +\t/* Prefer reliable fixed value over ranges */\n>>>>> +\tif (best_fixed >= 0)\n>>>>> +\t\ts = gst_caps_get_structure(caps, best_fixed);\n>>>>> +\telse\n>>>>> +\t\ts = gst_caps_get_structure(caps, best_in_range);\n>>>>> \n>>>>> \tif (gst_structure_has_name(s, \"video/x-raw\")) {\n>>>>> \t\tconst gchar *format = gst_video_format_to_string(gst_format);\n>>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>>>>> index b243aeb3..4c7b36ae 100644\n>>>>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>>>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>>>>> @@ -25,10 +25,6 @@\n>>>>> * - Add timestamp support\n>>>>> * - Use unique names to select the camera devices\n>>>>> * - Add GstVideoMeta support (strides and offsets)\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 \"gstlibcamerasrc.h\"\n>>>>>\n>>>\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 29A90BD87C\n\tfor ;\n\tThu, 26 Aug 2021 14:40:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9962D6891B;\n\tThu, 26 Aug 2021 16:40:50 +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 E4B8A6888F\n\tfor ;\n\tThu, 26 Aug 2021 16:40:49 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 73E05191F;\n\tThu, 26 Aug 2021 16:40:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GXYodu9y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629988849;\n\tbh=TGKEp6ENcR99a23yPLTsmYCPYh7HXvr/BR04Ykmfmzk=;\n\th=From:Subject:To:References:Date:In-Reply-To:From;\n\tb=GXYodu9yYHGY/wjgVV3uyKVHWoXrzmVbTSbeZ6pbzlAcDfz9G2T+Z/X5bY4aUwhYW\n\t7JN608zHPAQjB6Af4mGBBpGL4Z9/lFAFW28ZqsYg2hmB/8KU2QGHbctIHRmWlA0k+d\n\tNNBrOsHlNVmFao9sWGGBds6xrcAFEQqwKaTWi5LU=","From":"Kieran Bingham ","To":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210825150744.1183551-1-nicolas@ndufresne.ca>\n\t<7a9c3894-eac6-e303-c1fa-f721bef395fa@ideasonboard.com>\n\t<9b5270eb178d503903c0d1bb18a207f282888c84.camel@collabora.com>\n\t<4edc8a13-7bf4-f76e-5e74-eeb263857a91@ideasonboard.com>\n\t<945cc52d279555a272eedeb723f69707f4f87525.camel@collabora.com>","Message-ID":"","Date":"Thu, 26 Aug 2021 15:40:47 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<945cc52d279555a272eedeb723f69707f4f87525.camel@collabora.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Fix usage of default size\n\tfor fixation","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]