[{"id":34341,"web_url":"https://patchwork.libcamera.org/comment/34341/","msgid":"","date":"2025-05-22T17:53:44","subject":"Re: [PATCH 4/4] gstreamer: Reduce indentation in\n\tgst_libcamera_create_video_pool()","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi,\n\nLe jeudi 22 mai 2025 à 14:55 +0200, Laurent Pinchart a écrit :\n> Now that video pool creation is handled by a dedicated function, the\n> logic can be simplified by returning early instead of nesting scopes. Do\n> so to decrease indentation and improve readability.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n>  src/gstreamer/gstlibcamerasrc.cpp | 50 ++++++++++++++-----------------\n>  1 file changed, 23 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 380f8368af8b..e0eb6e7b4227 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -528,47 +528,43 @@ gst_libcamera_create_video_pool(GstLibcameraSrc *self,\n>  {\n>  \tg_autoptr(GstQuery) query = NULL;\n>  \tconst gboolean need_pool = true;\n> -\tgboolean has_video_meta = false;\n>  \tGstBufferPool *video_pool = NULL;\n>  \n>  \tgst_libcamera_extrapolate_info(info, stream_cfg.stride);\n>  \n>  \tquery = gst_query_new_allocation(caps, need_pool);\n> -\tif (!gst_pad_peer_query(srcpad, query))\n> +\tif (!gst_pad_peer_query(srcpad, query)) {\n>  \t\tGST_DEBUG_OBJECT(self, \"Didn't get downstream ALLOCATION hints\");\n> -\telse\n> -\t\thas_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL);\n> +\t\treturn { NULL, 0 };\n\nAh, so this is your use of a tuple. It wasn't obvious in previous patch that there was this case\ntoo. Please document briefly, it will help maintain this in the long run.\n\n\n> +\t}\n>  \n> -\tif (!has_video_meta) {\n> -\t\tGstBufferPool *pool = NULL;\n> +\tif (gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL))\n> +\t\treturn { NULL, 0 };\n>  \n> -\t\tif (gst_query_get_n_allocation_pools(query) > 0)\n> -\t\t\tgst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL);\n> +\tif (gst_query_get_n_allocation_pools(query) > 0)\n> +\t\tgst_query_parse_nth_allocation_pool(query, 0, &video_pool, NULL, NULL, NULL);\n\nOnce we figure-out a way to tell libcamera the number of buffers (just allocating N requests\ndoes not seem to do it for me), the min should also be propagated, that will fix more stalls.\n\nFor this reason, it might be nice to move this before the find function, so we later have a\nchance to save and use the min.\n\n>  \n> -\t\tif (pool)\n> -\t\t\tvideo_pool = pool;\n> -\t\telse {\n> -\t\t\tGstStructure *config;\n> -\t\t\tguint min_buffers = 3;\n> -\t\t\tvideo_pool = gst_video_buffer_pool_new();\n> +\tif (!video_pool) {\n> +\t\tGstStructure *config;\n> +\t\tguint min_buffers = 3;\n>  \n> -\t\t\tconfig = gst_buffer_pool_get_config(video_pool);\n> -\t\t\tgst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0);\n> +\t\tvideo_pool = gst_video_buffer_pool_new();\n> +\t\tconfig = gst_buffer_pool_get_config(video_pool);\n> +\t\tgst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0);\n\nTo be fixed in future update, I missed that min_buffers has been accidentally hard\ncoded. VideoPool is very flexible, but to avoid run-time allocation you should use the\ninformation from gst_query_parse_nth_allocation_pool() to configure the pool.\n\nOne of the \"NULL\" hides the actual pipeline reported min. Since this is for the copy case,\nwe don't need an inflight buffer and can simply pass over the min/max. Normally we try and\nuse the size returned downstream too, downstream may want some tail padding which VideoPool\ncan accomodate for.\n\n>  \n> -\t\t\tGST_DEBUG_OBJECT(self, \"Own pool config is %\" GST_PTR_FORMAT, config);\n> +\t\tGST_DEBUG_OBJECT(self, \"Own pool config is %\" GST_PTR_FORMAT, config);\n>  \n> -\t\t\tgst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> -\t\t}\n> +\t\tgst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> +\t}\n>  \n> -\t\tGST_WARNING_OBJECT(self, \"Downstream doesn't support video meta, need to copy frame.\");\n> +\tGST_WARNING_OBJECT(self, \"Downstream doesn't support video meta, need to copy frame.\");\n>  \n> -\t\tif (!gst_buffer_pool_set_active(video_pool, true)) {\n> -\t\t\tgst_caps_unref(caps);\n> -\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> -\t\t\t\t\t  (\"Failed to active buffer pool\"),\n> -\t\t\t\t\t  (\"gst_libcamera_src_negotiate() failed.\"));\n> -\t\t\treturn { NULL, -EINVAL };\n> -\t\t}\n> +\tif (!gst_buffer_pool_set_active(video_pool, true)) {\n> +\t\tgst_caps_unref(caps);\n> +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> +\t\t\t\t  (\"Failed to active buffer pool\"),\n> +\t\t\t\t  (\"gst_libcamera_src_negotiate() failed.\"));\n> +\t\treturn { NULL, -EINVAL };\n\nThere seems to be more issues in that function, the pool configuration does not seem\ncomplete and you may fail here trying to activate an unconfigured pool. But this is\nnot related to your changes so:\n\nReviewed-by: Nicolas Dufresne \n\nIf I find anytime, I will provide some patches, yet I'm like you, I don't have a setup\nto actually test it properly.\n\nregards,\nNicolas\n\n>  \t}\n>  \n>  \treturn { video_pool, 0 };","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 34486BD78E\n\tfor ;\n\tThu, 22 May 2025 17:53:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D16568D96;\n\tThu, 22 May 2025 19:53:48 +0200 (CEST)","from bali.collaboradmins.com (bali.collaboradmins.com\n\t[IPv6:2a01:4f8:201:9162::2])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A192B68D8B\n\tfor ;\n\tThu, 22 May 2025 19:53:46 +0200 (CEST)","from [IPv6:2606:6d00:17:b2fc::5ac] (unknown\n\t[IPv6:2606:6d00:17:b2fc::5ac])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby bali.collaboradmins.com (Postfix) with ESMTPSA id 8BAF117E0FD3;\n\tThu, 22 May 2025 19:53:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=collabora.com header.i=@collabora.com\n\theader.b=\"Aw7Sjjgo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1747936426;\n\tbh=3be1umwYO5tJ6XkSG9ILliJMxdQplTXDrDV0ODarucg=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=Aw7SjjgofpVEze8EkFNSVHSNiJKvBpHxkenc142ny6l0WKFaB6GmdNVdMGeLbmSIb\n\t6af6M9XhQMRpY7s5unbcrbqskHW1zJm3cq0bWYomKO/4RoyG30wfBYg9r4zDe+xd3Z\n\tfALyfRRWoY2zumlkLoCzkFkS6BjvcjSG/eVz5/ALP/LSzoFYAr5Ut1/BTMmkHsvTb4\n\taEqd7D2dfSb2cg0ZLEDKSZScgsbrGyHVMd+d82CeXOiTfLN1viZ9lhfF71Y5vIAuPK\n\tm++hOEj10x8/aW1wLFuoSY/g4JEv/a/RnP5ptq8Wk4n4DpZ/1ctJAHUWr7LQ6etYKw\n\ttfwdEGU6WrAWQ==","Message-ID":"","Subject":"Re: [PATCH 4/4] gstreamer: Reduce indentation in\n\tgst_libcamera_create_video_pool()","From":"Nicolas Dufresne ","To":"Laurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org","Cc":"Hou Qi ","Date":"Thu, 22 May 2025 13:53:44 -0400","In-Reply-To":"<20250522125521.6465-5-laurent.pinchart@ideasonboard.com>","References":"<20250522125521.6465-1-laurent.pinchart@ideasonboard.com>\n\t<20250522125521.6465-5-laurent.pinchart@ideasonboard.com>","Organization":"Collabora Canada","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.56.1 (3.56.1-1.fc42) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","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":34344,"web_url":"https://patchwork.libcamera.org/comment/34344/","msgid":"","date":"2025-05-23T05:58:36","subject":"RE: [EXT] [PATCH 4/4] gstreamer: Reduce indentation in\n\tgst_libcamera_create_video_pool()","submitter":{"id":195,"url":"https://patchwork.libcamera.org/api/people/195/","name":"Qi Hou","email":"qi.hou@nxp.com"},"content":"Hi,\n\n> -----Original Message-----\n> From: Laurent Pinchart \n> Sent: 2025年5月22日 20:55\n> To: libcamera-devel@lists.libcamera.org\n> Cc: Qi Hou ; Nicolas Dufresne\n> \n> Subject: [EXT] [PATCH 4/4] gstreamer: Reduce indentation in\n> gst_libcamera_create_video_pool()\n> \n> Caution: This is an external email. Please take care when clicking links or\n> opening attachments. When in doubt, report the message using the 'Report\n> this email' button\n> \n> \n> Now that video pool creation is handled by a dedicated function, the logic can\n> be simplified by returning early instead of nesting scopes. Do so to decrease\n> indentation and improve readability.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 50 ++++++++++++++-----------------\n> 1 file changed, 23 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> b/src/gstreamer/gstlibcamerasrc.cpp\n> index 380f8368af8b..e0eb6e7b4227 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -528,47 +528,43 @@ gst_libcamera_create_video_pool(GstLibcameraSrc\n> *self, {\n> g_autoptr(GstQuery) query = NULL;\n> const gboolean need_pool = true;\n> - gboolean has_video_meta = false;\n> GstBufferPool *video_pool = NULL;\n> \n> gst_libcamera_extrapolate_info(info, stream_cfg.stride);\n> \n> query = gst_query_new_allocation(caps, need_pool);\n> - if (!gst_pad_peer_query(srcpad, query))\n> + if (!gst_pad_peer_query(srcpad, query)) {\n> GST_DEBUG_OBJECT(self, \"Didn't get downstream\n> ALLOCATION hints\");\n> - else\n> - has_video_meta = gst_query_find_allocation_meta(query,\n> GST_VIDEO_META_API_TYPE, NULL);\n> + return { NULL, 0 };\n> + }\n\nI think the failure of gst_pad_peer_query() is not a problem, because connected plugin such as filesink does not provide propose_allocation(). This case is like that of query success but downstream doesn't support videometa. Both require creating internal video pool.\n\nRegards,\nQi Hou\n\n> \n> - if (!has_video_meta) {\n> - GstBufferPool *pool = NULL;\n> + if (gst_query_find_allocation_meta(query,\n> GST_VIDEO_META_API_TYPE, NULL))\n> + return { NULL, 0 };\n> \n> - if (gst_query_get_n_allocation_pools(query) > 0)\n> - gst_query_parse_nth_allocation_pool(query, 0,\n> &pool, NULL, NULL, NULL);\n> + if (gst_query_get_n_allocation_pools(query) > 0)\n> + gst_query_parse_nth_allocation_pool(query, 0,\n> + &video_pool, NULL, NULL, NULL);\n> \n> - if (pool)\n> - video_pool = pool;\n> - else {\n> - GstStructure *config;\n> - guint min_buffers = 3;\n> - video_pool = gst_video_buffer_pool_new();\n> + if (!video_pool) {\n> + GstStructure *config;\n> + guint min_buffers = 3;\n> \n> - config = gst_buffer_pool_get_config(video_pool);\n> - gst_buffer_pool_config_set_params(config, caps,\n> info->size, min_buffers, 0);\n> + video_pool = gst_video_buffer_pool_new();\n> + config = gst_buffer_pool_get_config(video_pool);\n> + gst_buffer_pool_config_set_params(config, caps,\n> + info->size, min_buffers, 0);\n> \n> - GST_DEBUG_OBJECT(self, \"Own pool config is %\"\n> GST_PTR_FORMAT, config);\n> + GST_DEBUG_OBJECT(self, \"Own pool config is %\"\n> + GST_PTR_FORMAT, config);\n> \n> -\n> gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> - }\n> +\n> gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> + }\n> \n> - GST_WARNING_OBJECT(self, \"Downstream doesn't support\n> video meta, need to copy frame.\");\n> + GST_WARNING_OBJECT(self, \"Downstream doesn't support video\n> meta,\n> + need to copy frame.\");\n> \n> - if (!gst_buffer_pool_set_active(video_pool, true)) {\n> - gst_caps_unref(caps);\n> - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> - (\"Failed to active buffer\n> pool\"),\n> -\n> (\"gst_libcamera_src_negotiate() failed.\"));\n> - return { NULL, -EINVAL };\n> - }\n> + if (!gst_buffer_pool_set_active(video_pool, true)) {\n> + gst_caps_unref(caps);\n> + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> + (\"Failed to active buffer pool\"),\n> + (\"gst_libcamera_src_negotiate()\n> failed.\"));\n> + return { NULL, -EINVAL };\n> }\n> \n> return { video_pool, 0 };\n> --\n> Regards,\n> \n> Laurent Pinchart","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 93206C31E9\n\tfor ;\n\tFri, 23 May 2025 05:58:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B666368D96;\n\tFri, 23 May 2025 07:58:39 +0200 (CEST)","from EUR05-AM6-obe.outbound.protection.outlook.com\n\t(mail-am6eur05on20626.outbound.protection.outlook.com\n\t[IPv6:2a01:111:f403:2612::626])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F4C761679\n\tfor ;\n\tFri, 23 May 2025 07:58:38 +0200 (CEST)","from PAXPR04MB8285.eurprd04.prod.outlook.com\n\t(2603:10a6:102:1ca::15)\n\tby PA1PR04MB10178.eurprd04.prod.outlook.com (2603:10a6:102:463::19)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8769.21;\n\tFri, 23 May 2025 05:58:37 +0000","from PAXPR04MB8285.eurprd04.prod.outlook.com\n\t([fe80::e003:8fb:64ea:acfd]) by\n\tPAXPR04MB8285.eurprd04.prod.outlook.com\n\t([fe80::e003:8fb:64ea:acfd%3]) with mapi id 15.20.8746.030;\n\tFri, 23 May 2025 05:58:37 +0000"],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=nxp.com header.i=@nxp.com header.b=\"cbWlDFf9\";\n\tdkim-atps=neutral","dkim=none (message not signed)\n\theader.d=none;dmarc=none action=none header.from=nxp.com;"],"ARC-Seal":"i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none;\n\tb=CHcCd+h1vrReRrCk/veH81y4pv5rpJqbKq21YhDOINa1s53yv9wDlxXYwKjUUQ8769hHu/GJk3uIp9JZWbkoIJt33dF+XHNh9wrIe+zsNTePxAjdfLU1voOIS8geTLS9dZIAFoIpx6OL3nKJjERpkTw7yOFuTjdVe6OF386NLljQ8oQx1YsmypSWnH4m/x7GqeCgkOBYDz84M0iuDMCnHeuZTuO8XrB4vppWQgC3ZHIq2K1nXQDj1IBCyDtVUCP0NqMH/FJKv6W9DcVZrjUkCBnuCCztYQnxB23bC8hMX9UnWzSn1lr8Kvt8lU5RxmbJbQlXi82K/EdmiscqPPadnw==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com;\n\ts=arcselector10001;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1;\n\tbh=ykPClkn1gQYw5tIutMeCTwevQO9m4P2RbP5CWAHqPX0=;\n\tb=nm6isBgDs3asXUsLcd0fcWLrFCQ1lzcsLClE2JRcR95Gufwkx12yy8n8WGaijsBKcGdVsScoV4KsCrUyeoMzXWPg5r/iNA82XcmaDYzCuJ/QP1lD/7dddGWy5Z7NzQCGm1sLc8j9uNKPL+9TL27Vwd0x9bYjkaWw4qEjGVRAzVw8dDvZLUshGX/UVczlkx69QnN5SNEHGUhshTBkkPUfM0iDBQTp1TwSLrh9e8hA806ZMjG3v3iXSVReVj3fTlLOz9hvnIvMOUSgVBUvIIYmhnsYUnbrwcEvfE/5f5mVhnvBQd2Qsgj1QED3v+5nsf65k22QoXUocn4OqA3v6uapHg==","ARC-Authentication-Results":"i=1; mx.microsoft.com 1; spf=pass\n\tsmtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com;\n\tdkim=pass header.d=nxp.com; arc=none","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector1;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;\n\tbh=ykPClkn1gQYw5tIutMeCTwevQO9m4P2RbP5CWAHqPX0=;\n\tb=cbWlDFf9tT7xcFTdwYNLvAFERIvQOPr/gArTQX7jh75SJgp0N8AP90FXv91H5X71drksBAYf/vr/4LrlWKmv+boZsRs4FBEOC+jSZ5A1D0N4Z2Do3Q9WfDWdnkTPyQEqLLn7TV4at6qSJallGWFipr0bbiuHUKrRTAWmRHLGwpHpHMnzvJie8MHtxsHzPynwkff1BDnCLk3KamPGWlS01Rd7gENJkhxs3/hvpdJ5CftkOdY5RXHCiV6+w9jWseoEcXKomF6vvxDYDJzCdjUEN3UbCkavdk1hleF1ULoQw5X7nKbvLExHEENMNrfYOHAKLporVH1fNSqICyncQ+DZfw==","From":"Qi Hou ","To":"Laurent Pinchart ,\n\t\"libcamera-devel@lists.libcamera.org\"\n\t","CC":"Nicolas Dufresne ","Subject":"RE: [EXT] [PATCH 4/4] gstreamer: Reduce indentation in\n\tgst_libcamera_create_video_pool()","Thread-Topic":"[EXT] [PATCH 4/4] gstreamer: Reduce indentation in\n\tgst_libcamera_create_video_pool()","Thread-Index":"AQHbyxjP4G8xMLvoXEaANeZ1J4vOTLPftIew","Date":"Fri, 23 May 2025 05:58:36 +0000","Message-ID":"","References":"<20250522125521.6465-1-laurent.pinchart@ideasonboard.com>\n\t<20250522125521.6465-5-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20250522125521.6465-5-laurent.pinchart@ideasonboard.com>","Accept-Language":"zh-CN, en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","authentication-results":["lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=nxp.com header.i=@nxp.com header.b=\"cbWlDFf9\";\n\tdkim-atps=neutral","dkim=none (message not signed)\n\theader.d=none;dmarc=none action=none header.from=nxp.com;"],"x-ms-publictraffictype":"Email","x-ms-traffictypediagnostic":"PAXPR04MB8285:EE_|PA1PR04MB10178:EE_","x-ms-office365-filtering-correlation-id":"7510ee49-f292-4005-3c1e-08dd99bedbfc","x-ld-processed":"686ea1d3-bc2b-4c6f-a92c-d99c5c301635,ExtAddr","x-ms-exchange-senderadcheck":"1","x-ms-exchange-antispam-relay":"0","x-microsoft-antispam":"BCL:0;\n\tARA:13230040|1800799024|366016|376014|38070700018; ","x-microsoft-antispam-message-info":"=?gb2312?b?bUU3K1RnUEJjTzM0UGQ4L2RI?=\n\t=?gb2312?b?MHJWcmtFRFNLTURsNHM1NDhWOHl4Sy9QdDkwTkRTZkEraTEvR1Mx?=\n\t=?gb2312?b?NkZaeUJTaitkQ2hYaVBmUmg4UDJUODZwQ2RHRGlERVdwNS94VDY1?=\n\t=?gb2312?b?OHZHOHJsWUtkc0lVTXBaWm1EY3pMUWVwNW9hNkUycUFOUTlTelFj?=\n\t=?gb2312?b?Y2lrT1JYUlkwY1NxSFpwdlM0QjFscWdKNnAwOFc0TkVFR2gySlJi?=\n\t=?gb2312?b?N1g3VnM5OUQzWVoxZXBnY2ZSZ3p1MWVjM0IwK0xmZFEvZjVCSVpk?=\n\t=?gb2312?b?dDF4eVowRitocWhRMm5JN1lLcGtOSW5OekVhc2pNVVdrSjM2bnZl?=\n\t=?gb2312?b?OElNUUVPYTJsWE1uVCtrM1pMdjJmNkxVemM2REdXUFFEWW5JSnNp?=\n\t=?gb2312?b?b2FnTXFHSmllTDAvVWt6RnV5MDZIVDhta1FyS2FlbGxkVmJoUEJh?=\n\t=?gb2312?b?MkFqcnBGOThSTEhySnRYMXhJNWdPTlo3eFpGaHQ2SHN3WGE5dm1o?=\n\t=?gb2312?b?ZExSN2s1a1VibVJJa1V6UnFSeUZGQThNZ2ZrZXFoV3ZPS2NBSjZJ?=\n\t=?gb2312?b?Yk85aGZQbXBzZEZPUXdFZGZFMkE3NTBMcitIb05aZ3VWMjJNNmQr?=\n\t=?gb2312?b?d1JGblE0OHJhdEE1WXhFNGN1TDVVUFIvVnkza2dpUlRLL0ZYbmF4?=\n\t=?gb2312?b?aDljalNHVk9RV3BZVXJDcWR0VWlxeDNLZ2lLUGVMYk1xTDZTVSs3?=\n\t=?gb2312?b?ekNQR0VkelFtYTlxeHJ5Uy9meVdRTWpkaWx4SFF0WEcwV3l2U1FU?=\n\t=?gb2312?b?RytaQkFXNGh3MXNjL1ppb0FnYmE2Q3MzM3lkWDN5bC9SNk1BZFNZ?=\n\t=?gb2312?b?TFVPTjFwaS91NWV2SzZKSzJuYWJnWmF0eVJPd1NGWHBQR2g2cVM5?=\n\t=?gb2312?b?MjBaa2k3NyswSzlZNDQ3S05aK3RHR202RVRVTHcwazNhWVVtMVVJ?=\n\t=?gb2312?b?Sk1JRmZTa2d1VXhtdWQ4Qzc2eHV6YWl2eS94OHRvKzlVOU02bTJY?=\n\t=?gb2312?b?dHVkSFQvdU13c1V6MkdmQlN4MjBFbGV2SXFCeVFZUUJSUTJUZ0dk?=\n\t=?gb2312?b?WklxVUtGMFZ5WVlnWCtaSXAxMGp6QTVDM0lHYnovMEhVdzlJaXJk?=\n\t=?gb2312?b?TCtGQTNpRGQyY1luc3MwZlFwNFJxYlFNU1Q1bmtwSEdFSWFWRDNn?=\n\t=?gb2312?b?WmsrK0NWcTQ0RGFZS0VMOWN1dFFGcElOV0tJUnI4ZE1zSUpJbUJ1?=\n\t=?gb2312?b?NHhCTms5akRUVUFsR0o0L3ZXOVhpZ2xiTVJrTm13RTU2cldzbFJE?=\n\t=?gb2312?b?QzhRemkzbFlvZUhPQWJhaWNEdDRJOVgwaW5RK2o2Y2QyeTNKSnpD?=\n\t=?gb2312?b?aGt2QUFld0ZTUzg0NElDYnJwTmhleDJLcHAwcm12OWZNTWFHRVpu?=\n\t=?gb2312?b?Sm5HRERzTEZwUitKTWU0VUFZY2h4THl1L0xwZDlNOWwxbFZZdDhm?=\n\t=?gb2312?b?MjF3d1NRcC9QNzBFaWpNdWVjQVhPd3J0ejdrbjBoZ1YrWkZIdDJn?=\n\t=?gb2312?b?M0N0NzUrM1AyR1RBcEN3SFdEOWh2eXNJMVc4NXhjODBKSEZFS1Rh?=\n\t=?gb2312?b?WStVY3dlL2RseVduVE5lanl2bzFiSXY1T1BxZFoycTIva0huMExh?=\n\t=?gb2312?b?S3orb0Q5RkVEd2d1cHBMMWZhNHFVYXdRcExKMGp1QnNKSHN2QXlP?=\n\t=?gb2312?b?UkZnS05jMjlYNWlSdlVqNk8xVFlWb0s5bzRRNlcvKzJ5QmRHZE1F?=\n\t=?gb2312?b?MTEvcHhBN0xtOXM4UG1hQUtobVAwdkZDZ2NNcFJ5Q3RSNkJQeXVl?=\n\t=?gb2312?b?L1p3MUpQeWR0NlR4WkdGUlNBdSt6RUUyVFp6MHVyblMrUk1pTWs1?=\n\t=?gb2312?b?bi9CKzE5a1VDczR6Yk1ZSFdGSXhhd05qSXRuR3dsWEUrMlJadkJk?=\n\t=?gb2312?b?SzJmNGxuZEpFdW0zMmd0NmdNSGx2djM0aEJhaFVPbVllVHlxSDB3?=\n\t=?gb2312?b?by9kQmljWUlCM0tzdEhyVXVqWCtDUWdwaTQxRW5VNE8xVDk5aEZh?=\n\t=?gb2312?b?RFc0QjFsb3JZTXJXaDNZNzdXQnpjSXFUdUU2L0hYOWdNS0Y2dnJv?=\n\t=?gb2312?b?b1pPNzVkaGI1MTliTmRKVEhTQjFwTmFYb3dsdElvWit3PT0=?=","x-forefront-antispam-report":"CIP:255.255.255.255; CTRY:; LANG:zh-cn; SCL:1; \n\tSRV:; IPV:NLI; SFV:NSPM;\n\tH:PAXPR04MB8285.eurprd04.prod.outlook.com; PTR:; \n\tCAT:NONE; SFS:(13230040)(1800799024)(366016)(376014)(38070700018);\n\tDIR:OUT; SFP:1101; ","x-ms-exchange-antispam-messagedata-chunkcount":"1","x-ms-exchange-antispam-messagedata-0":"=?gb2312?b?MFlSaG9qMit4MnQxandGYnJh?=\n\t=?gb2312?b?dzI2NkF6OTVTRlB0aEZWcmlISUZFRjJaaG1Xd1ZIQVVpUWNmUlg5?=\n\t=?gb2312?b?WDlLUlJkcVRmdjQ0S2IxMjRwS1lyZE01aVM2RllMK2thWTNjNTY5?=\n\t=?gb2312?b?a2w1bzI0MEVKTldkVzdBOFpWMk1wQldHTU9uSXlKYnMweHVneDVF?=\n\t=?gb2312?b?NldlL0NGSHc2WjNJbXBnMVd3czVTS3FUbDZlYkc2NWgzUVlINExr?=\n\t=?gb2312?b?VjNVdWVkM3U2eHBNNllFK3ZvbHlRMlBqWDBvK0U3Z3Uxa0ltd2tT?=\n\t=?gb2312?b?Q2o0SkJteStweGhibmh1YlJJRTdLWFFkTU9mN2Q1N0hJNjExNTRL?=\n\t=?gb2312?b?SE9IaEJQMVVkUGJtNXRFOFBad2J2UEJmUUcvaWFjN25iMHFDRWlM?=\n\t=?gb2312?b?dG9lcmNUd0YvNjhnYXlweTB0Sk9FR0lJb2NHU3dGWExCNnExK0Nh?=\n\t=?gb2312?b?cXJ2L1BiVnVZU2FDMFBNcmlWblQ2RGVpZUZvOUxSSmdCTklPTEFq?=\n\t=?gb2312?b?OVgrT0lJVU1HVlNUZmQxT0cvNzNoN09lZjBvajVHQW9uc05tMGhX?=\n\t=?gb2312?b?Y1crNlRMSnhEMmJkUm1OWXNVa016eDBld3dOTkNSVWdLUnVxY1FN?=\n\t=?gb2312?b?ajZmbVEycjEzUCs3RE5MczVQUCtqbmRvVVhLQWY4NDA4dHluNStP?=\n\t=?gb2312?b?dDJzSzFUT2xlYVFuYmpMUkt4VElTbVcwLzdoWnJ3QTdTVVNmYlFF?=\n\t=?gb2312?b?TnNPa0JXQlVBblJRK2h1blBkSmxKOE9CMndEamRmM3ROYzc4QzRK?=\n\t=?gb2312?b?SzFMWTIxb2xZNkxNTGxJZVhHanVDVFBZemtKQTNoUlVaZ0tqQWw0?=\n\t=?gb2312?b?L0t6ZUVVVXRBNzZUV2RUbW4vYllsUDVsNXhGcHl3RHlEclYreGI0?=\n\t=?gb2312?b?NjFyY3publNGanNta0pTemFFRG5WSW5lbVJnZXNOa0hXc0dNY1p2?=\n\t=?gb2312?b?by9xYXNCQ3hOVzZRdlpJRTRYQVA0QjEzN0dpYXY1cTJHVzFyTkt1?=\n\t=?gb2312?b?aFlOb1pPd2tCTDI4YWRDQ25KQXlFVm1lMXRGWXZZQmFsRzZWa0Rw?=\n\t=?gb2312?b?dWFBNDF2bTBzWVZIbUNDMFFKaG5ZcVI1ak5KS2VUTWpOLy82alY4?=\n\t=?gb2312?b?MHNhRnd4YjNRd1hjamZ0WFNsOStGekIvTXhhdDY3RGlzRVJad0J5?=\n\t=?gb2312?b?UnlXSHVwM0xrQjhUTThVd0F4NGFaZVp2NUxnRm5sRkRyZ0dHT29C?=\n\t=?gb2312?b?RnpienRGdnJVNEc0ZmxZeG5QdWk4UkROR2ZOSnI3Z2loR1Myc2Fx?=\n\t=?gb2312?b?VXAzUkxZaDRtQ0prVGZwYnp1Zlo1Y1ppMlR0Tnpoa291ajNoMUcv?=\n\t=?gb2312?b?ZUJzUXNTSFpNNDFJVFo5UXM1VlRsOUNRZCtRa0IvUzVZaklPUmNY?=\n\t=?gb2312?b?OEExbUJ4NHJBWFM3djVvUVRvRmd0Z0xYVlJERXhpaGlyK2F3QVpm?=\n\t=?gb2312?b?R0hEV3JkRE9ZaXByRlVtR3NLUmxiTzdyME1zWGROOVNNS3pWbVlM?=\n\t=?gb2312?b?bk5UZDB1K0lhSFRxSEo2SnFnNGtqYUx0bmlUbndRdVQzbWJITUdq?=\n\t=?gb2312?b?YjJxMEovL2hZOG5xeVpJN0tEaXIvaWVQTkFuWDA0WmR3MGhSdE1S?=\n\t=?gb2312?b?L1A2Q21aamQyQm1DSTR4SnRKNVRoK3lGUWlXbnBzN09na1FZUjJo?=\n\t=?gb2312?b?OENQdzgxMnBidUlDZllEa0pMMmk1ZW5hdFZHRisvY1JFTFNnY2FD?=\n\t=?gb2312?b?OG80MzQwbFBaMUh4UmpEdTVYbitVMi9mNVRmcmNaVVE5RzdmeGNq?=\n\t=?gb2312?b?MkJZaHhPeG5xRTR5di9JWm5ubk02SUpIWUxEcjJrNm1uSjNjZXpo?=\n\t=?gb2312?b?a3dJUGJiR2hndytRZ1o5czh3R0tCNVFsVGY5OHhtQm02MXl4ZmNS?=\n\t=?gb2312?b?RklHVnk4cWdNRzA4d2l2Y3FyRStaODRzUDFuNzJMVVNiK2N2WVkx?=\n\t=?gb2312?b?VHpVY1NDTU5TUmNnT2FMRzA1dkN5NGVJejRBUWE1eS9VUVJiY1hO?=\n\t=?gb2312?b?MHNZMkYvVUxzK2srbzV6RTZuVEN6eHlxaEhOMnZwSncwYmtsTXpa?=\n\t=?gb2312?b?Q1FmaC9TQUptT0d5YzJwWE1mbTBDVG90QkwrUFZMdGI0SmhTK2ZD?=\n\t=?gb2312?b?REJwSHYyWmsvelYwTGVMVE9aK0Q0N1dkZUFGbFFSZU1JQkRoQ0ZC?=\n\t=?gb2312?b?K3VqUkxBZDdnM2p5QUowYmVsZENhSWtUL1VEQ2dtTFMwZzZPamVD?=\n\t=?gb2312?b?WT0=?=","Content-Type":"text/plain; charset=\"gb2312\"","Content-Transfer-Encoding":"base64","MIME-Version":"1.0","X-OriginatorOrg":"nxp.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-AuthSource":"PAXPR04MB8285.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"7510ee49-f292-4005-3c1e-08dd99bedbfc","X-MS-Exchange-CrossTenant-originalarrivaltime":"23 May 2025 05:58:36.9521\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"686ea1d3-bc2b-4c6f-a92c-d99c5c301635","X-MS-Exchange-CrossTenant-mailboxtype":"HOSTED","X-MS-Exchange-CrossTenant-userprincipalname":"TXOXT8SM1dlIRUaij7ylJrx5QrPAl6y39ixm8TdUuF+u5F1MXz3aWib7Oj141sEm","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"PA1PR04MB10178","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":34402,"web_url":"https://patchwork.libcamera.org/comment/34402/","msgid":"<20250603224133.GK2942@pendragon.ideasonboard.com>","date":"2025-06-03T22:41:33","subject":"Re: [PATCH 4/4] gstreamer: Reduce indentation in\n\tgst_libcamera_create_video_pool()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, May 22, 2025 at 01:53:44PM -0400, Nicolas Dufresne wrote:\n> Le jeudi 22 mai 2025 à 14:55 +0200, Laurent Pinchart a écrit :\n> > Now that video pool creation is handled by a dedicated function, the\n> > logic can be simplified by returning early instead of nesting scopes. Do\n> > so to decrease indentation and improve readability.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> >  src/gstreamer/gstlibcamerasrc.cpp | 50 ++++++++++++++-----------------\n> >  1 file changed, 23 insertions(+), 27 deletions(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 380f8368af8b..e0eb6e7b4227 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -528,47 +528,43 @@ gst_libcamera_create_video_pool(GstLibcameraSrc *self,\n> >  {\n> >  \tg_autoptr(GstQuery) query = NULL;\n> >  \tconst gboolean need_pool = true;\n> > -\tgboolean has_video_meta = false;\n> >  \tGstBufferPool *video_pool = NULL;\n> >  \n> >  \tgst_libcamera_extrapolate_info(info, stream_cfg.stride);\n> >  \n> >  \tquery = gst_query_new_allocation(caps, need_pool);\n> > -\tif (!gst_pad_peer_query(srcpad, query))\n> > +\tif (!gst_pad_peer_query(srcpad, query)) {\n> >  \t\tGST_DEBUG_OBJECT(self, \"Didn't get downstream ALLOCATION hints\");\n> > -\telse\n> > -\t\thas_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL);\n> > +\t\treturn { NULL, 0 };\n> \n> Ah, so this is your use of a tuple. It wasn't obvious in previous patch that there was this case\n> too. Please document briefly, it will help maintain this in the long run.\n\nSure, I'll document it.\n\n> > +\t}\n> >  \n> > -\tif (!has_video_meta) {\n> > -\t\tGstBufferPool *pool = NULL;\n> > +\tif (gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL))\n> > +\t\treturn { NULL, 0 };\n> >  \n> > -\t\tif (gst_query_get_n_allocation_pools(query) > 0)\n> > -\t\t\tgst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL);\n> > +\tif (gst_query_get_n_allocation_pools(query) > 0)\n> > +\t\tgst_query_parse_nth_allocation_pool(query, 0, &video_pool, NULL, NULL, NULL);\n> \n> Once we figure-out a way to tell libcamera the number of buffers (just allocating N requests\n> does not seem to do it for me), the min should also be propagated, that will fix more stalls.\n> \n> For this reason, it might be nice to move this before the find function, so we later have a\n> chance to save and use the min.\n\nI'm fine with that, but I'd like to do so in a separate patch, to avoid\nintroducing functional changes here. I have no idea what those function\ncalls do and if their order is important :-)\n\n> >  \n> > -\t\tif (pool)\n> > -\t\t\tvideo_pool = pool;\n> > -\t\telse {\n> > -\t\t\tGstStructure *config;\n> > -\t\t\tguint min_buffers = 3;\n> > -\t\t\tvideo_pool = gst_video_buffer_pool_new();\n> > +\tif (!video_pool) {\n> > +\t\tGstStructure *config;\n> > +\t\tguint min_buffers = 3;\n> >  \n> > -\t\t\tconfig = gst_buffer_pool_get_config(video_pool);\n> > -\t\t\tgst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0);\n> > +\t\tvideo_pool = gst_video_buffer_pool_new();\n> > +\t\tconfig = gst_buffer_pool_get_config(video_pool);\n> > +\t\tgst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0);\n> \n> To be fixed in future update, I missed that min_buffers has been accidentally hard\n> coded. VideoPool is very flexible, but to avoid run-time allocation you should use the\n> information from gst_query_parse_nth_allocation_pool() to configure the pool.\n> \n> One of the \"NULL\" hides the actual pipeline reported min. Since this is for the copy case,\n> we don't need an inflight buffer and can simply pass over the min/max. Normally we try and\n> use the size returned downstream too, downstream may want some tail padding which VideoPool\n> can accomodate for.\n>\n> >  \n> > -\t\t\tGST_DEBUG_OBJECT(self, \"Own pool config is %\" GST_PTR_FORMAT, config);\n> > +\t\tGST_DEBUG_OBJECT(self, \"Own pool config is %\" GST_PTR_FORMAT, config);\n> >  \n> > -\t\t\tgst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> > -\t\t}\n> > +\t\tgst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> > +\t}\n> >  \n> > -\t\tGST_WARNING_OBJECT(self, \"Downstream doesn't support video meta, need to copy frame.\");\n> > +\tGST_WARNING_OBJECT(self, \"Downstream doesn't support video meta, need to copy frame.\");\n> >  \n> > -\t\tif (!gst_buffer_pool_set_active(video_pool, true)) {\n> > -\t\t\tgst_caps_unref(caps);\n> > -\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > -\t\t\t\t\t  (\"Failed to active buffer pool\"),\n> > -\t\t\t\t\t  (\"gst_libcamera_src_negotiate() failed.\"));\n> > -\t\t\treturn { NULL, -EINVAL };\n> > -\t\t}\n> > +\tif (!gst_buffer_pool_set_active(video_pool, true)) {\n> > +\t\tgst_caps_unref(caps);\n> > +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > +\t\t\t\t  (\"Failed to active buffer pool\"),\n> > +\t\t\t\t  (\"gst_libcamera_src_negotiate() failed.\"));\n> > +\t\treturn { NULL, -EINVAL };\n> \n> There seems to be more issues in that function, the pool configuration does not seem\n> complete and you may fail here trying to activate an unconfigured pool. But this is\n> not related to your changes so:\n> \n> Reviewed-by: Nicolas Dufresne \n> \n> If I find anytime, I will provide some patches, yet I'm like you, I don't have a setup\n> to actually test it properly.\n\nThat would be very nice, as this is really outside of my comfort zone.\nMaybe Hou could test the patches ?\n\n> >  \t}\n> >  \n> >  \treturn { video_pool, 0 };","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 2DF19C3237\n\tfor ;\n\tTue, 3 Jun 2025 22:41:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3964168DBB;\n\tWed, 4 Jun 2025 00:41:45 +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 197DF615F8\n\tfor ;\n\tWed, 4 Jun 2025 00:41:43 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 386F1752;\n\tWed, 4 Jun 2025 00:41:40 +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=\"LOeqsKca\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1748990500;\n\tbh=izH8TVeib7Ure8VAjNdiuiRXpzjBvjzERpvpBE6eYEg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LOeqsKcacP780zJYZ/Yztk8B3YT/wnJtK16K/Wl51iLakN5YT1AfMsFhRkMo7WrTs\n\tUHUdhehnAfMatGNvvOPitCfK8FcsnUSFaSKnFqkRXSKPQbY+3Ev7mEx7eHnSgij5MF\n\tgjGnBdS42Gx0DERcAOJSZ5TR2g0MdwQwILo+OQds=","Date":"Wed, 4 Jun 2025 01:41:33 +0300","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org, Hou Qi ","Subject":"Re: [PATCH 4/4] gstreamer: Reduce indentation in\n\tgst_libcamera_create_video_pool()","Message-ID":"<20250603224133.GK2942@pendragon.ideasonboard.com>","References":"<20250522125521.6465-1-laurent.pinchart@ideasonboard.com>\n\t<20250522125521.6465-5-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":34404,"web_url":"https://patchwork.libcamera.org/comment/34404/","msgid":"<20250603233353.GB29935@pendragon.ideasonboard.com>","date":"2025-06-03T23:33:53","subject":"Re: [EXT] [PATCH 4/4] gstreamer: Reduce indentation in\n\tgst_libcamera_create_video_pool()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, May 23, 2025 at 05:58:36AM +0000, Qi Hou wrote:\n> On 2025年5月22日 20:55, Laurent Pinchart wrote:\n> > \n> > Now that video pool creation is handled by a dedicated function, the logic can\n> > be simplified by returning early instead of nesting scopes. Do so to decrease\n> > indentation and improve readability.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 50 ++++++++++++++-----------------\n> > 1 file changed, 23 insertions(+), 27 deletions(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 380f8368af8b..e0eb6e7b4227 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -528,47 +528,43 @@ gst_libcamera_create_video_pool(GstLibcameraSrc\n> > *self, {\n> > g_autoptr(GstQuery) query = NULL;\n> > const gboolean need_pool = true;\n> > - gboolean has_video_meta = false;\n> > GstBufferPool *video_pool = NULL;\n> > \n> > gst_libcamera_extrapolate_info(info, stream_cfg.stride);\n> > \n> > query = gst_query_new_allocation(caps, need_pool);\n> > - if (!gst_pad_peer_query(srcpad, query))\n> > + if (!gst_pad_peer_query(srcpad, query)) {\n> > GST_DEBUG_OBJECT(self, \"Didn't get downstream ALLOCATION hints\");\n> > - else\n> > - has_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL);\n> > + return { NULL, 0 };\n> > + }\n> \n> I think the failure of gst_pad_peer_query() is not a problem, because\n> connected plugin such as filesink does not provide\n> propose_allocation(). This case is like that of query success but\n> downstream doesn't support videometa. Both require creating internal\n> video pool.\n\nYou're right, I introduced a change in behaviour by mistake here. I'll\nfix it. Thank you for noticing.\n\n> > - if (!has_video_meta) {\n> > - GstBufferPool *pool = NULL;\n> > + if (gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL))\n> > + return { NULL, 0 };\n> > \n> > - if (gst_query_get_n_allocation_pools(query) > 0)\n> > - gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL);\n> > + if (gst_query_get_n_allocation_pools(query) > 0)\n> > + gst_query_parse_nth_allocation_pool(query, 0, &video_pool, NULL, NULL, NULL);\n> > \n> > - if (pool)\n> > - video_pool = pool;\n> > - else {\n> > - GstStructure *config;\n> > - guint min_buffers = 3;\n> > - video_pool = gst_video_buffer_pool_new();\n> > + if (!video_pool) {\n> > + GstStructure *config;\n> > + guint min_buffers = 3;\n> > \n> > - config = gst_buffer_pool_get_config(video_pool);\n> > - gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0);\n> > + video_pool = gst_video_buffer_pool_new();\n> > + config = gst_buffer_pool_get_config(video_pool);\n> > + gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0);\n> > \n> > - GST_DEBUG_OBJECT(self, \"Own pool config is %\" GST_PTR_FORMAT, config);\n> > + GST_DEBUG_OBJECT(self, \"Own pool config is %\" GST_PTR_FORMAT, config);\n> > \n> > - gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> > - }\n> > + gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> > + }\n> > \n> > - GST_WARNING_OBJECT(self, \"Downstream doesn't support video meta, need to copy frame.\");\n> > + GST_WARNING_OBJECT(self, \"Downstream doesn't support video meta, need to copy frame.\");\n> > \n> > - if (!gst_buffer_pool_set_active(video_pool, true)) {\n> > - gst_caps_unref(caps);\n> > - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > - (\"Failed to active buffer pool\"),\n> > - (\"gst_libcamera_src_negotiate() failed.\"));\n> > - return { NULL, -EINVAL };\n> > - }\n> > + if (!gst_buffer_pool_set_active(video_pool, true)) {\n> > + gst_caps_unref(caps);\n> > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > + (\"Failed to active buffer pool\"),\n> > + (\"gst_libcamera_src_negotiate() failed.\"));\n> > + return { NULL, -EINVAL };\n> > }\n> > \n> > return { video_pool, 0 };","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 0417AC3237\n\tfor ;\n\tTue, 3 Jun 2025 23:34:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9047068DAB;\n\tWed, 4 Jun 2025 01:34:05 +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 3808361869\n\tfor ;\n\tWed, 4 Jun 2025 01:34:03 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8B5C2E8A;\n\tWed, 4 Jun 2025 01:34:00 +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=\"IClAba3D\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1748993640;\n\tbh=Ic8cE3XpuT7FMHt8KbB5cg2QHuDiEb6ERTvzs5QSXcY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IClAba3D9YTaNJ7LITBs/3K8Vhax+VM8jO8ntep7Tzv9rtk6iFaF4UFTJp3sJjC35\n\tdv+WGEvtOPo6gGWwjSZrgl0C8Y+yVrqqWNTvHlh8p0YC+DVmiNvC0HmNNN1B5uGwtN\n\tfk+p+IV78HfahTc/3XzQplQmcHFJXx8Hs5i9RNQk=","Date":"Wed, 4 Jun 2025 02:33:53 +0300","From":"Laurent Pinchart ","To":"Qi Hou ","Cc":"\"libcamera-devel@lists.libcamera.org\"\n\t, \n\tNicolas Dufresne ","Subject":"Re: [EXT] [PATCH 4/4] gstreamer: Reduce indentation in\n\tgst_libcamera_create_video_pool()","Message-ID":"<20250603233353.GB29935@pendragon.ideasonboard.com>","References":"<20250522125521.6465-1-laurent.pinchart@ideasonboard.com>\n\t<20250522125521.6465-5-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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\" "}}]